LCORE-405: do not allow crendentials to be enabled for * origins
#408
Conversation
WalkthroughUpdated CORSConfiguration: default allow_credentials set to False and added validation to reject "*" in allow_origins when allow_credentials is True. Unit tests were expanded and renamed to cover valid/invalid combinations and the new failure case. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant CORSConfiguration
participant Validator as check_cors_configuration
App->>CORSConfiguration: Initialize(config)
CORSConfiguration->>Validator: Validate allow_origins & allow_credentials
alt allow_credentials=True and "*" in allow_origins
Validator-->>App: Raise ValueError
else Valid combination
Validator-->>App: Return OK
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
lcore-405-do-not-allow-crendentials-to-be-enabled-for-star-origins
branch
from
80fe4ad to
224684e
Compare
tisnik
force-pushed
the
lcore-405-do-not-allow-crendentials-to-be-enabled-for-star-origins
branch
from
224684e to
b9ca231
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/config.py (1)
43-47: Nit: improve error message spacing and grammarMinor wording polish and consistent field name: add a space after the first sentence, use “cannot”, and “allow_origins”.
Apply:
- raise ValueError( - "Invalid CORS configuration: allow_credentials can not be set to true when " - "allow origins contains '*' wildcard." - "Use explicit origins or disable credential." - ) + raise ValueError( + "Invalid CORS configuration: allow_credentials cannot be set to true when " + "allow_origins contains '*' wildcard. " + "Use explicit origins or disable credentials." + )Note: If you accept this wording change, update the test expectation accordingly (see test comment below).
tests/unit/models/test_config.py (1)
273-289: Exception type likely should be pydantic ValidationError instead of ValueErrorPydantic v2 wraps ValueError raised inside model validators into a ValidationError at instantiation time. Other tests in this suite already expect ValidationError for similar scenarios. Recommend aligning this test to expect ValidationError to avoid flakiness across environments.
Minimal change to align with pydantic behavior:
- with pytest.raises(ValueError, match=expected): + with pytest.raises(ValidationError, match=expected): # allow_credentials can not be true when allow_origins contains '*' CORSConfiguration( allow_origins=["*"], allow_credentials=True, allow_methods=["foo_method", "bar_method", "baz_method"], allow_headers=["foo_header", "bar_header", "baz_header"], )If you adopt the improved error message wording suggested in config.py, also update the expected string here:
- expected = ( - "Value error, Invalid CORS configuration: " - + "allow_credentials can not be set to true when allow origins contains '\\*' wildcard." - + "Use explicit origins or disable credential." - ) + expected = ( + "Value error, Invalid CORS configuration: " + + "allow_credentials cannot be set to true when allow_origins contains '\\*' wildcard. " + + "Use explicit origins or disable credentials." + )If you prefer to keep the current message text, apply only the exception type change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/models/config.py(1 hunks)tests/unit/models/test_config.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
CORSConfiguration(27-48)
🔇 Additional comments (6)
src/models/config.py (2)
40-47: Correctly disallow credentials with wildcard originsThe validator enforces the CORS/Fetch constraint and matches the stated objective. Good fail-fast behavior.
33-33: Secure-by-default: Credentials disabled for CORS by defaultI verified that changing
allow_credentialstoFalseonly affects:
- src/app/main.py: the
CORSMiddlewareis now instantiated withallow_credentials=Falseby default.- Tests in
tests/unit/models/test_config.pyeither explicitly setallow_credentialsor don’t assert on the default.Please double-check that no existing clients rely on cookies or authorization headers being allowed in cross-origin requests before merging.
tests/unit/models/test_config.py (4)
223-223: Assert updated default explicitlyAsserting the new default keeps the regression guard for the secure-by-default change.
228-241: Custom config with explicit origins and credentials disabled: LGTMCovers the permissive origins with non-credentialed case.
243-256: Explicit origins with credentials enabled: LGTMValidates the positive path when no wildcard is used.
258-271: Wildcard origins with credentials disabled: LGTMValidates the permissive default that remains compliant with the spec.
1 participant
Description
LCORE-405: do not allow crendentials to be enabled for
*originsType of change
Related Tickets & Documents
Summary by CodeRabbit