Skip to content

fix(web): preserve CORS origin schemes and Vary headers - #7901

Merged
Aias00 merged 1 commit into
apache:masterfrom
cloudbase-ai:fix/issue-7370-cors-origin
Oct 23, 2026
Merged

Aias00 merged 1 commit into
apache:masterfrom
cloudbase-ai:fix/issue-7370-cors-origin

Conversation

@cloudbase-ai

Copy link
Copy Markdown
Contributor

Fixes #7370.

An allowed https://frontend.example origin is currently prefixed with http:// when the gateway receives an HTTP request, so it fails the allow-list check. Trim configured origins and preserve their explicit scheme; scheme-less entries continue to inherit the gateway request scheme.

Add Vary: Origin before response commitment, after upstream response headers have been applied. Preserve existing Vary tokens, recognize an existing Origin token case-insensitively, and retain Vary: *. Responses for absent or denied origins also vary by Origin so a cached response without CORS permission cannot be reused for an allowed request.

Wildcard and allowedAnyOrigin modes continue reflecting concrete origins, including with credentials. As requested by the issue, the literal opaque Origin: null does not receive CORS permission through those modes. Denied preflights continue down the filter chain, preserving #6720's behavior.

Validation

The regression checks fail against the original implementation. After the fix, all 24 CrossFilterTest cases pass, including cross-scheme/trimmed/scheme-less origins, scheme/host/port rejection, preflight headers, credentials, wildcard modes, opaque origins, missing Origin, and Vary headers applied downstream. The selected reactor also passes Checkstyle and Apache RAT:

./mvnw -B -ntp -nsu -pl shenyu-web -am verify \
  -Dtest=CrossFilterTest -Dsurefire.failIfNoSpecifiedTests=false \
  -Djacoco.skip=true -Dmaven.javadoc.skip=true

These tests commit WebFlux mock responses; browser/proxy end-to-end tests and the full repository test suite were not run.

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true. Focused verification is listed above.

Signed-off-by: cloudbase-ai <22498243+cloudbase-ai@users.noreply.github.com>

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

Approving. The scheme bug is fixed at the right test — oneOrigin.contains("://") asks "does this entry already carry a scheme", where the old startsWith(String.format("%s://", scheme)) asked "does it carry the gateway's scheme", which is exactly why https://frontend.example got prefixed with http:// and never matched. .map(String::trim) before the check is also correct, since otherwise " https://x ".contains("://") would still pass but the reflected Access-Control-Allow-Origin would have whitespace and fail the browser's byte comparison.

The Vary: Origin piece is designed correctly:

  • Registering beforeCommit before delegating to the chain is what makes it run after upstream headers have been merged, so an upstream Vary is already present when you look at it.
  • Adding it for absent and denied origins (not just allowed ones) is the part that actually makes a shared cache safe — testResponseWithoutOriginAlsoVariesByOrigin and testAllowListDoesNotReflectDisallowedOrigins pin it.
  • HttpHeaders.getVary() splits comma-separated values, so "Accept-Encoding, Origin" is seen as two tokens and does not get a duplicate Origin. testVaryPreservesExistingTokensWithoutDuplicatingOrigin covers the lowercase and * cases too, and skipping augmentation when Vary: * is present is RFC-correct.

One behaviour change that goes beyond the issue and should be called out in the description:

  • !"null".equals(origin) now excludes the literal Origin: null from the CORS branch entirely. Refusing to reflect null is the safer default, and I agree with it, but a deployment that previously relied on allowedAnyOrigin echoing Origin: null (sandboxed iframes, some webviews, file:// origins) silently stops getting Access-Control-Allow-Origin after this. That is a compat note, not a defect — just do not let it ship as an undocumented side effect.

Non-blocking:

  1. oneOrigin.contains("://") accepts anything with :// in it, including foo://bar or a typo like https:/frontend.example. A stricter startsWith("http://") || startsWith("https://") (or parsing with URI) would reject junk at config time instead of letting it sit in the allow list looking valid.
  2. Vary: Origin is now emitted on every response, including preflight and non-CORS traffic. Correct for cache safety, but it multiplies cache variants per origin for CDNs in front of the gateway. Expected trade-off — worth a line in the docs so operators are not surprised by a hit-rate drop.
  3. filterAndComplete was added to drive setComplete because the beforeCommit callback only runs on commit. That is a real behavioural coupling the old assertOrigin helper hid; good that the helper now commits explicitly, since tests that never commit would otherwise silently stop exercising the Vary path.

@Aias00
Aias00 merged commit ba5ed0f into apache:master Oct 23, 2026
41 of 51 checks passed
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.

[BUG] CrossFilter can omit CORS headers for allowed origins and does not set Vary: Origin

2 participants