Skip to content

[ISSUE #6292] Avoid mutating divide upstream for specify-domain - #6392

Draft
Federicorao wants to merge 1 commit into
apache:masterfrom
Federicorao:codex/fix-divide-specify-domain
Draft

[ISSUE #6292] Avoid mutating divide upstream for specify-domain#6392
Federicorao wants to merge 1 commit into
apache:masterfrom
Federicorao:codex/fix-divide-specify-domain

Conversation

@Federicorao

Copy link
Copy Markdown

Summary

  • derive the request domain from the specify-domain header without mutating the selected cached Upstream
  • add a DividePlugin regression test that verifies the exchange domain changes while the cached upstream URL remains unchanged

Verification

  • git diff --check
  • attempted ./mvnw -pl shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide -Dtest=DividePluginTest -DskipITs test, but the local shell cannot locate a Java runtime

Fixes #6292

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DividePlugin specify-domain

2 participants