Fix cors allowed origin pattern matching - #4068
Conversation
|
@joemahady-comm Hi Joe, if there is no breaking change, can you add new test cases, and leave the old ones as is? That would indicate 100% backwards compatible. |
@fhanik The reason for the update to the tests is due to a legacy issue in the setup of the tests where an invalid regular expression was used "^*\.localhost$". This expression means match the start of the expression zero or more times. The invalid regular expression was passing due to the use of find() and was masked. As soon as we switched to match() it caused the tests too fail as the invalid regular expression was exposed. I've updated the the regular expression to "^.*\.localhost$" in the tests which means match any character zero or more times. This should have been the correct expression in the tests from the start. We are remaining backwards compatible just we needed to fix a long standing typo in the test suite which was previosuly hidden by the .find(). |
There was a problem hiding this comment.
🟡 Changes recommended
The security-sensitive matching behavior lacks focused regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens CORS origin matching while preserving compatibility with legacy unanchored patterns.
Changes:
- Replaces permissive
find()matching withmatches(). - Wraps unanchored origin patterns.
- Corrects malformed test regexes.
File summaries
| File | Description |
|---|---|
uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcZonePathTests.java |
Corrects zone-path localhost patterns. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java |
Corrects localhost patterns. |
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CorsFilter.java |
Tightens origin matching; focused spoofing and compatibility regression tests remain needed. |
Review details
Suppressed comments (3)
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CorsFilter.java:384
- This condition skips every one-sided anchored regex. For example, the existing
example.com$configuration pattern used inCorsFilterDefaultZoneTests.java:444previously matchedhttps://example.comviafind(), but nowmatches()rejects it because the pattern is not wrapped. Treat a pattern as already anchored only when it has both anchors; otherwise this is not backward compatible.
if (!patternToCompile.startsWith("^") && !patternToCompile.endsWith("$")) {
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CorsFilter.java:385
- Auto-prefixing an unanchored expression with
.*still accepts attacker-controlled sibling domains. For example,my-domain\.comcompiles to^.*(?:my-domain\.com)(?::\d+)?$, which matcheshttps://notmy-domain.comeven though that host is not a subdomain ofmy-domain.com. Avoid arbitrary substring matching here; parse and normalize the Origin and match its scheme/host with a label boundary, or require an explicit full-origin regex.
patternToCompile = "^.*(?:" + patternToCompile + ")(?::\\d+)?$";
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CorsFilter.java:387
- This introduces new rewriting, anchoring, and optional-port semantics for
cors.*.allowed.origins, but the configuration reference and identity-zone API docs still describe these values only as regex patterns. The repository documentation rule requires configuration/API behavior changes to update those docs; document the transformation and give safe anchored examples so operators can predict what is accepted.
String patternToCompile = allowedOrigin;
if (!patternToCompile.startsWith("^") && !patternToCompile.endsWith("$")) {
patternToCompile = "^.*(?:" + patternToCompile + ")(?::\\d+)?$";
}
configuration.getAllowedOriginPatterns().add(Pattern.compile(patternToCompile));
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This replaces #4008, correct |
Ticket: cors-allowed-origin-patterns-matched-with-find-instead-of-matches (Backward-Compatible Implementation)
Fix: Updated the CorsFilter to evaluate allowed origins using strict .matches() instead of the permissive .find() to prevent spoofed domain vulnerabilities (e.g., https://my-domain.com.attacker.com). To maintain backward compatibility for operators relying on legacy substring configurations, unanchored patterns are now conditionally auto-wrapped (e.g., ^.*(?:pattern)(?::\d+)?$) during compilation. This retains the intended substring support for dynamic schemes/subdomains and optional ports, while securely rejecting trailing malicious payloads. Test configurations were also updated to utilize syntactically correct regular expressions.