[ISSUE #6292] Avoid mutating divide upstream for specify-domain - #6392
[ISSUE #6292] Avoid mutating divide upstream for specify-domain#6392Federicorao wants to merge 1 commit into
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Dual-lane review (code-reviewer + architect) of #6392 (head 36799f61). Architectural verdict: BLOCK — requesting changes, with a recommendation to close as superseded.
BLOCK — superseded by the already-merged #6412. Master contains commit d4db5e60d (#6412, merged 2026-07-05), which fixes the same issue (#6292) in the same DividePlugin block with a stricter approach: it filters upstreamList by u.getUrl().equals(requested) and builds a fresh Upstream copy, falling back to the load balancer otherwise — so a client cannot target a host that isn't a configured upstream. This PR does not filter; it lets any client-supplied specify-domain header value become HTTP_DOMAIN. mergeStateStatus=DIRTY confirms #6392 no longer applies cleanly. Rebasing/merging would either conflict or silently drop #6412's restriction, reintroducing a guard a maintainer already shipped. Recommend closing #6392 as superseded by #6412.
High — SPECIFY_DOMAIN is not validated against configured upstreams (SSRF / open proxy). buildDomain(upstream, specifyDomains.get(0)) prepends the protocol to an arbitrary, unvalidated header value and stores it as Constants.HTTP_DOMAIN, which URIPlugin.java:38 uses to build the target URI. A client can set SPECIFY_DOMAIN to any host and be proxied to it. This is pre-existing (the old upstream.setUrl(...) had the same hole), not introduced here, but as a fix scoped to this code it leaves the vulnerability open where #6412 closed it. If this PR is pursued, mirror #6412's guard: only honor SPECIFY_DOMAIN when it matches a configured upstream's URL; otherwise ignore/fallback.
Architect — the deeper ownership problem is unaddressed by either fix. Upstream mixes immutable configuration (url/protocol/weight) with shared mutable runtime stats (inflight/lag/responseStamp/succeeded) in one cached object and still exposes setUrl/setWeight/setStatus. setUrl is precisely what enabled the original bug. #6412's per-call copy patches one path; the foot-gun remains for the next plugin that touches a cached Upstream. The durable fix is to split Upstream into an immutable UpstreamConfig (cached; note equals/hashCode already key only on protocol+url) and a separate UpstreamStats, or at minimum deprecate the config setters on cached instances. The PR's buildDomain helper is a consumer-side band-aid, not a fix for that.
Low — duplicated domain logic. DividePlugin.buildDomain(Upstream, String) re-implements Upstream.buildDomain()'s (Upstream.java:471-477) protocol defaulting. Add an overload Upstream.buildDomain(String url) so the default-protocol policy stays single-sourced; otherwise the two will drift.
Low — test coverage gaps. doExecuteWithSpecifyDomainShouldNotMutateCachedUpstream proves the core fix (cached url unchanged) but doesn't cover blank/empty SPECIFY_DOMAIN (produces "http://"), multi-value header ordering, or the empty-protocol branch. Add cases for those.
Open question for the maintainer: is SPECIFY_DOMAIN meant to be a free-form override or strictly an alias for a configured upstream URL? #6412 assumes the latter; #6392 assumes the former. Please confirm against #6292/docs — that determines whether #6392's approach is even a valid alternative or whether #6412 is the canonical shape (in which case this PR should close).
Summary
Verification
Fixes #6292