🛡️ Sentinel: [HIGH] Fix CORS wildcard subdomain vulnerability#197
🛡️ Sentinel: [HIGH] Fix CORS wildcard subdomain vulnerability#197EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
This commit addresses a security vulnerability where CORS wildcard matching (e.g., `*.example.com`) incorrectly allowed domains that ended with the suffix but were not valid subdomains (e.g., `evilexample.com`). Changes: - Enforce that wildcard suffix matches are preceded by a dot (`.`) in `app-http`. - Apply the same fix to `http-middleware` to resolve code duplication issues. - Add regression tests in both crates to verify the fix and prevent recurrence. - Document the vulnerability and learning in `.jules/sentinel.md`. The fix ensures that only true subdomains are allowed when using wildcard patterns.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Summary of ChangesHello @EffortlessSteven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a critical security fix for a high-severity CORS wildcard subdomain vulnerability. The changes refine the origin validation logic to prevent malicious domains from bypassing security checks, thereby safeguarding sensitive API endpoints and data. The solution ensures that only properly formed subdomains are recognized, significantly enhancing the application's security posture against unauthorized access. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request correctly addresses a significant CORS wildcard subdomain vulnerability by adding a check for a dot separator. The fix is implemented in both app-http and http-middleware crates, and is well-tested with new and updated tests that cover the vulnerability scenario. My feedback includes suggestions to refactor the implementation using more idiomatic Rust constructs for improved readability and safety, but the core logic of the fix is sound.
| if !origin.ends_with(wildcard_domain) { | ||
| return false; | ||
| } | ||
| // Ensure the suffix match is preceded by a dot to prevent partial domain matches | ||
| // e.g. "evilexample.com" should not match "*.example.com" | ||
| let prefix_len = origin.len() - wildcard_domain.len(); | ||
| return prefix_len > 0 && origin.as_bytes()[prefix_len - 1] == b'.'; |
There was a problem hiding this comment.
This logic can be expressed more concisely and safely using strip_suffix and map_or. This avoids manual byte indexing, is more idiomatic, and less error-prone. You can move the explanatory comment from lines 231-232 to be just above the new return statement.
return origin.strip_suffix(wildcard_domain).map_or(false, |p| p.ends_with('.'));| if !origin.ends_with(wildcard_domain) { | ||
| return false; | ||
| } | ||
| // Ensure the suffix match is preceded by a dot to prevent partial domain matches | ||
| // e.g. "evilexample.com" should not match "*.example.com" | ||
| let prefix_len = origin.len() - wildcard_domain.len(); | ||
| return prefix_len > 0 && origin.as_bytes()[prefix_len - 1] == b'.'; |
There was a problem hiding this comment.
This logic can be expressed more concisely and safely using strip_suffix and map_or. This avoids manual byte indexing, is more idiomatic, and less error-prone. You can move the explanatory comment from lines 126-127 to be just above the new return statement.
return origin.strip_suffix(wildcard_domain).map_or(false, |p| p.ends_with('.'));There was a problem hiding this comment.
Pull request overview
This PR fixes a high-severity CORS security vulnerability where wildcard subdomain patterns (e.g., https://*.example.com) incorrectly allowed unauthorized origins with domains ending in the target domain (e.g., https://evilexample.com). The fix adds explicit validation to ensure the matched suffix is preceded by a dot, preventing partial domain matches.
Changes:
- Updated CORS validation logic in both
http-middlewareandapp-httpcrates to verify dot-prefix before accepting wildcard matches - Added regression test in new integration test file and updated existing unit tests
- Documented the vulnerability and fix in sentinel.md for future reference
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/http-middleware/src/cors.rs | Fixed wildcard subdomain validation logic and added security regression test to unit tests |
| crates/app-http/src/middleware/cors.rs | Applied identical fix to the duplicated CORS implementation in app-http crate |
| crates/app-http/tests/cors_reproduction.rs | New integration test file verifying the security fix prevents evilexample.com from matching *.example.com |
| .jules/sentinel.md | Documented the vulnerability, root cause, and prevention strategy for future reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This should NOT be allowed. | ||
| // Previously, "evilexample.com" would match because it ends with "example.com". | ||
| // The fix ensures that there is a dot separator. | ||
| assert!(!config.is_origin_allowed("https://evilexample.com"), "Security regression: evilexample.com matched *.example.com"); |
There was a problem hiding this comment.
The test cases should include verification that the base domain (https://example.com) is correctly rejected when matching against *.example.com. The current fix would reject it (because the character before "example.com" would be '/' not '.'), but this important edge case is not explicitly tested. Consider adding: assert!(!config.is_origin_allowed("https://example.com")); to verify wildcards only match subdomains, not the base domain itself.
| // This should NOT be allowed. | |
| // Previously, "evilexample.com" would match because it ends with "example.com". | |
| // The fix ensures that there is a dot separator. | |
| assert!(!config.is_origin_allowed("https://evilexample.com"), "Security regression: evilexample.com matched *.example.com"); | |
| // These should NOT be allowed. | |
| // Base domain must not match a subdomain wildcard. | |
| assert!( | |
| !config.is_origin_allowed("https://example.com"), | |
| "Security regression: example.com matched *.example.com" | |
| ); | |
| // Previously, "evilexample.com" would match because it ends with "example.com". | |
| // The fix ensures that there is a dot separator. | |
| assert!( | |
| !config.is_origin_allowed("https://evilexample.com"), | |
| "Security regression: evilexample.com matched *.example.com" | |
| ); |
| #[test] | ||
| fn test_cors_config_subdomain_wildcard() { | ||
| let config = CorsConfig { | ||
| allowed_origins: vec!["https://*.example.com".to_string()], | ||
| ..Default::default() | ||
| }; | ||
| assert!(config.is_origin_allowed("https://api.example.com")); | ||
| assert!(config.is_origin_allowed("https://app.example.com")); | ||
| assert!(!config.is_origin_allowed("https://malicious.com")); | ||
|
|
||
| // Test for security vulnerability fix: | ||
| // "evilexample.com" ends with "example.com" but should NOT match "*.example.com" | ||
| assert!(!config.is_origin_allowed("https://evilexample.com")); | ||
| } |
There was a problem hiding this comment.
The test should also verify that the base domain (https://example.com) is correctly rejected when matching against *.example.com, and that multi-level subdomains (e.g., https://api.v2.example.com) are correctly accepted. These edge cases help ensure the fix handles all scenarios correctly and prevents future regressions.
🛡️ Sentinel: [HIGH] Fix CORS wildcard subdomain vulnerability
🚨 Severity: HIGH
💡 Vulnerability: CORS configuration using wildcard subdomains (e.g.,
https://*.example.com) allowed unauthorized origins likehttps://evilexample.combecause the validation logic only checked if the origin string ended with the domain suffix.🎯 Impact: Attackers could bypass CORS protections by registering domains that end with the target domain name, potentially accessing sensitive API endpoints or data.
🔧 Fix: Updated the
is_origin_allowedlogic in bothcrates/app-httpandcrates/http-middlewareto explicitly verify that the matching suffix is preceded by a dot (.), ensuring only valid subdomains are accepted.✅ Verification: Added a new regression test file
crates/app-http/tests/cors_reproduction.rsand updated unit tests incrates/http-middleware/src/cors.rsto confirm thatevilexample.comis rejected whileapi.example.comremains allowed. All tests passed.PR created automatically by Jules for task 2296705249216256093 started by @EffortlessSteven