LCORE-405: configurable CORS settings #406
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 56 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughReplaces hard-coded wildcard CORS in app startup with values from a new CORSConfiguration model on ServiceConfiguration; adds the model, updates tests and test fixtures, and reads cors at module level in src/app/main.py to initialize CORSMiddleware. Changes
Sequence Diagram(s)sequenceDiagram
participant App as FastAPI App (startup)
participant Config as ServiceConfiguration
participant Middleware as CORSMiddleware
App->>Config: load service_configuration
Config-->>App: return cors (CORSConfiguration)
App->>Middleware: initialize with cors.allow_origins, cors.allow_methods, cors.allow_headers, cors.allow_credentials
Middleware-->>App: attached to app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/models/config.py (2)
30-35: Use Field(default_factory=...) for mutable defaultsAvoid shared mutable defaults for lists to prevent surprising cross-instance mutations.
Apply this diff:
- allow_origins: list[str] = [ - "*" - ] # not AnyHttpUrl: we need to support "*" that is not valid URL + allow_origins: list[str] = Field(default_factory=lambda: ["*"]) # not AnyHttpUrl: we need to support "*" that is not valid URL allow_credentials: bool = False - allow_methods: list[str] = ["*"] - allow_headers: list[str] = ["*"] + allow_methods: list[str] = Field(default_factory=lambda: ["*"]) + allow_headers: list[str] = Field(default_factory=lambda: ["*"])Additionally, update the import (outside this hunk):
from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field
125-125: Avoid defaulting to a concrete model instanceUsing a model instance as a default value risks shared state between ServiceConfiguration instances. Prefer a factory.
Apply this diff:
- cors: CORSConfiguration = CORSConfiguration() + cors: CORSConfiguration = Field(default_factory=CORSConfiguration)Note: You already have the same pattern for tls_config; consider aligning that too in a follow-up for consistency.
📜 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/app/main.py(1 hunks)src/models/config.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app/main.py (1)
src/configuration.py (2)
configuration(55-60)service_configuration(63-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
src/app/main.py (1)
47-50: Nice: CORS wired to configurationMoving CORSMiddleware settings off hard-coded wildcards to configuration improves operability and security posture.
| cors = configuration.service_configuration.cors | ||
|
|
||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| allow_origins=cors.allow_origins, | ||
| allow_credentials=cors.allow_credentials, | ||
| allow_methods=cors.allow_methods, | ||
| allow_headers=cors.allow_headers, |
There was a problem hiding this comment.
Guard against invalid CORS at runtime (credentials + '*')
Given current defaults, this will configure CORSMiddleware with allow_credentials=True and wildcard origins, which browsers will reject. Either rely on config-model validation (preferred, see suggested change in config.py) or defensively disable credentials when wildcards are present.
Apply this diff to add a runtime guard:
-cors = configuration.service_configuration.cors
+cors = configuration.service_configuration.cors
+allow_credentials = cors.allow_credentials
+if cors.allow_credentials and "*" in cors.allow_origins:
+ logger.warning(
+ "CORS misconfiguration detected: allow_credentials=True with wildcard origins. "
+ "Overriding to allow_credentials=False to comply with browsers and CORS spec."
+ )
+ allow_credentials = False
app.add_middleware(
CORSMiddleware,
allow_origins=cors.allow_origins,
- allow_credentials=cors.allow_credentials,
+ allow_credentials=allow_credentials,
allow_methods=cors.allow_methods,
allow_headers=cors.allow_headers,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cors = configuration.service_configuration.cors | |
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=["*"], | |
| allow_credentials=True, | |
| allow_methods=["*"], | |
| allow_headers=["*"], | |
| allow_origins=cors.allow_origins, | |
| allow_credentials=cors.allow_credentials, | |
| allow_methods=cors.allow_methods, | |
| allow_headers=cors.allow_headers, | |
| cors = configuration.service_configuration.cors | |
| allow_credentials = cors.allow_credentials | |
| if cors.allow_credentials and "*" in cors.allow_origins: | |
| logger.warning( | |
| "CORS misconfiguration detected: allow_credentials=True with wildcard origins. " | |
| "Overriding to allow_credentials=False to comply with browsers and CORS spec." | |
| ) | |
| allow_credentials = False | |
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=cors.allow_origins, | |
| allow_credentials=allow_credentials, | |
| allow_methods=cors.allow_methods, | |
| allow_headers=cors.allow_headers, | |
| ) |
🤖 Prompt for AI Agents
In src/app/main.py around lines 43 to 50, the CORS middleware is being
constructed with allow_credentials=True while allow_origins may be the wildcard
('*'), which browsers reject; add a runtime guard that checks if
cors.allow_credentials is truthy and cors.allow_origins contains a wildcard
(exact '*' or pattern that means any origin) and if so set
allow_credentials_used=False (or override to False) and emit a warning log
explaining credentials were disabled due to wildcard origins; then pass the
possibly-overridden allow_credentials value into CORSMiddleware so credentials
are not enabled when origins are wildcarded (alternative: you may instead
raise/abort early if you prefer strict validation, but implement the defensive
override as described).
| class CORSConfiguration(BaseModel): | ||
| """CORS configuration.""" | ||
|
|
||
| allow_origins: list[str] = [ | ||
| "*" | ||
| ] # not AnyHttpUrl: we need to support "*" that is not valid URL | ||
| allow_credentials: bool = True | ||
| allow_methods: list[str] = ["*"] | ||
| allow_headers: list[str] = ["*"] | ||
|
|
||
| @model_validator(mode="after") | ||
| def check_cors_configuration(self) -> Self: | ||
| """Check CORS configuration.""" | ||
| return self | ||
|
|
There was a problem hiding this comment.
Block invalid/insecure combo: '*' origins with allow_credentials=True
Browsers reject credentialed requests when Access-Control-Allow-Origin is ''. Shipping defaults as allow_origins [''] and allow_credentials True is both insecure and functionally broken.
Apply this diff to use a safe default and enforce validation:
class CORSConfiguration(BaseModel):
"""CORS configuration."""
- allow_origins: list[str] = [
- "*"
- ] # not AnyHttpUrl: we need to support "*" that is not valid URL
- allow_credentials: bool = True
+ allow_origins: list[str] = [
+ "*"
+ ] # not AnyHttpUrl: we need to support "*" that is not valid URL
+ allow_credentials: bool = False
allow_methods: list[str] = ["*"]
allow_headers: list[str] = ["*"]
@model_validator(mode="after")
def check_cors_configuration(self) -> Self:
- """Check CORS configuration."""
- return self
+ """Check CORS configuration."""
+ # Credentials are not allowed with wildcard origins per CORS/Fetch spec.
+ if self.allow_credentials and "*" in self.allow_origins:
+ raise ValueError(
+ "Invalid CORS config: allow_credentials cannot be true when allow_origins contains '*'. "
+ "Use explicit origins or disable credentials."
+ )
+ return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CORSConfiguration(BaseModel): | |
| """CORS configuration.""" | |
| allow_origins: list[str] = [ | |
| "*" | |
| ] # not AnyHttpUrl: we need to support "*" that is not valid URL | |
| allow_credentials: bool = True | |
| allow_methods: list[str] = ["*"] | |
| allow_headers: list[str] = ["*"] | |
| @model_validator(mode="after") | |
| def check_cors_configuration(self) -> Self: | |
| """Check CORS configuration.""" | |
| return self | |
| class CORSConfiguration(BaseModel): | |
| """CORS configuration.""" | |
| allow_origins: list[str] = [ | |
| "*" | |
| ] # not AnyHttpUrl: we need to support "*" that is not valid URL | |
| allow_credentials: bool = False | |
| allow_methods: list[str] = ["*"] | |
| allow_headers: list[str] = ["*"] | |
| @model_validator(mode="after") | |
| def check_cors_configuration(self) -> Self: | |
| """Check CORS configuration.""" | |
| # Credentials are not allowed with wildcard origins per CORS/Fetch spec. | |
| if self.allow_credentials and "*" in self.allow_origins: | |
| raise ValueError( | |
| "Invalid CORS config: allow_credentials cannot be true when allow_origins contains '*'. " | |
| "Use explicit origins or disable credentials." | |
| ) | |
| return self |
🤖 Prompt for AI Agents
In src/models/config.py around lines 27 to 41, the model currently defaults
allow_origins to ["*"] while allow_credentials is True which is invalid and
insecure; change the default to a safe value (e.g., an empty list or a specific
origin like "http://localhost") and update the model_validator to enforce that
if allow_credentials is True then allow_origins must not contain "*" (raise a
ValueError with a clear message), or alternatively if "*" is present and
allow_credentials is True, set allow_credentials to False or require the
developer to provide explicit origins; ensure the validator runs after
initialization and returns Self when valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/integration/test_configuration.py (1)
50-56: Optional: Add an integration test for default CORS when omitted.A second integration test that loads a config without the service.cors section and asserts defaults would harden coverage end-to-end.
If helpful, I can add a minimal YAML (without cors) under tests/configuration/ and a companion test here.
tests/configuration/lightspeed-stack.yaml (1)
9-22: Nit: Clarify placeholder values to avoid future confusion.Using canonical verbs/headers (e.g., GET/POST and Authorization/Content-Type) or adding a short comment that these are test-only placeholders can prevent confusion if validation is introduced later.
Apply this diff to annotate the placeholders inline:
access_log: true cors: + # Note: placeholder values used only to exercise parsing in tests. + # In real configs use canonical HTTP verbs/headers (e.g., GET, POST, Authorization). allow_origins: - foo_origin - bar_origin - baz_origin allow_credentials: false allow_methods: - foo_method - bar_method - baz_method allow_headers: - foo_header - bar_header - baz_headertests/unit/models/test_config.py (2)
24-24: Import looks correct; consider asserting default cors in constructor test.Since ServiceConfiguration now includes a cors field, extend test_service_configuration_constructor to also assert s.cors == CORSConfiguration().
218-226: Consider validating invalid CORS combinations in the model.A common pitfall is using allow_origins ["*"] together with allow_credentials True. Browsers disallow wildcard origins with credentials. Adding a validator in CORSConfiguration to catch (or auto-correct) such combos would prevent runtime surprises and make failures early and explicit.
Example validator for src/models/config.py (for reference):
@model_validator(mode="after") def check_cors_configuration(self) -> Self: if not self.allow_origins: raise ValueError("allow_origins must not be empty") if not self.allow_methods: raise ValueError("allow_methods must not be empty") if not self.allow_headers: raise ValueError("allow_headers must not be empty") if "*" in self.allow_origins and self.allow_credentials: raise ValueError("allow_origins cannot contain '*' when allow_credentials is True") return selfIf you adopt this, update this defaults test (or defaults) accordingly (e.g., default allow_credentials to False when using ["*"]).
📜 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 (3)
tests/configuration/lightspeed-stack.yaml(1 hunks)tests/integration/test_configuration.py(1 hunks)tests/unit/models/test_config.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/integration/test_configuration.py (1)
src/configuration.py (1)
service_configuration(63-68)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
CORSConfiguration(27-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e_tests
🔇 Additional comments (6)
tests/integration/test_configuration.py (1)
50-56: LGTM: Solid coverage for service.cors block.The assertions validate all four fields and align with the YAML fixture. This gives good confidence that parsing/typing are correct.
tests/configuration/lightspeed-stack.yaml (1)
9-22: LGTM: Example CORS config matches test expectations.The fixture fields and values align with the integration/unit tests, ensuring deterministic assertions.
tests/unit/models/test_config.py (4)
218-226: LGTM: Default CORS values are verified.Covers the new model defaults comprehensively.
228-241: LGTM: Custom CORS configuration test is thorough.Covers all fields and asserts expected values.
466-472: LGTM: Dump includes service.cors when explicitly set.This ensures serialization consistency for the new field.
523-540: LGTM: Expected JSON includes full cors block.The assertions match the structure emitted by the models.
| # check 'service.cors' section | ||
| cors_config = cfg.service_configuration.cors | ||
| assert cors_config.allow_origins == ["foo_origin", "bar_origin", "baz_origin"] | ||
| assert cors_config.allow_credentials is False | ||
| assert cors_config.allow_methods == ["foo_method", "bar_method", "baz_method"] | ||
| assert cors_config.allow_headers == ["foo_header", "bar_header", "baz_header"] | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against import-time access to configuration in app startup.
Per src/configuration.py (lines 62-67), accessing service_configuration asserts that configuration is loaded. If src/app/main.py reads configuration.service_configuration.cors at module import time (as stated in the PR summary), startup can fail unless configuration is loaded beforehand. Consider deferring the CORS read to runtime (e.g., during app factory, lifespan event) or ensuring configuration is loaded before main is imported.
[offer_architecture_advice]
Run this script to confirm if main.py accesses CORS at import time and whether configuration is loaded before that anywhere:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for module-level access to configuration.service_configuration.cors:"
rg -n -C3 -P 'configuration\.service_configuration\.cors' --type=py
echo
echo "Searching for calls that load configuration (order review):"
rg -n -C3 -P '\bload_configuration\s*\(' --type=pyLength of output: 4482
Defer import-time CORS setup in src/app/main.py
The middleware is pulling in configuration.service_configuration.cors at module import time (line 43) but there’s no prior call to load_configuration() in that file. If someone does import src.app.main without first loading your YAML config, startup will crash.
• Location to fix:
– src/app/main.py:43
python cors = configuration.service_configuration.cors
• Suggested refactor:
– Move CORS retrieval and middleware registration into a function that runs after load_configuration()—for example, inside an app_factory() or an @app.on_event("startup") handler.
– Or, if you truly must keep it at module level, call configuration.load_configuration(...) before accessing service_configuration in that module.
🤖 Prompt for AI Agents
In src/app/main.py around line 43, the module currently accesses
configuration.service_configuration.cors at import time which can crash if
load_configuration() hasn't been called; move the CORS retrieval and middleware
registration out of module scope into a function that runs after configuration
is loaded (e.g., an app_factory() that calls
configuration.load_configuration(...) before reading service_configuration and
then registers the CORS middleware, or register the middleware inside an
@app.on_event("startup") handler that reads
configuration.service_configuration.cors), alternatively if you must keep it at
module level call configuration.load_configuration(...) first before accessing
service_configuration.
1 participant
Description
LCORE-405: configurable CORS settings
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests