Add CORS support to FastAPI application #1300
Conversation
|
|
|
|
WalkthroughAdds configurable CORS support: five new environment variables, conditional CORSMiddleware registration during server startup, duplicated documentation block, and a new test module covering preflight, response headers, origin whitelisting, env-var parsing, and wildcard/credentials scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server as FastAPI Server
participant Middleware as CORSMiddleware
participant App as Application Handler
Client->>Server: HTTP request (OPTIONS/GET) with Origin
activate Server
Server->>Middleware: Incoming request passes through middleware
activate Middleware
Middleware->>Middleware: Check CORS_ENABLED
alt CORS enabled
Middleware->>Middleware: Parse CORS_ALLOW_* env vars into lists
Middleware->>Middleware: Validate wildcard + credentials -> may disable credentials
Middleware->>App: Forward request to app/route
App-->>Middleware: Response
Middleware-->>Client: Add Access-Control-Allow-* headers (if allowed)
else CORS disabled
Middleware->>App: Forward request without CORS handling
App-->>Client: Response (no CORS headers)
end
deactivate Middleware
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/reference/environment-variables.mdholmes/common/env_vars.pyserver.pytests/test_cors.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/test_cors.pyserver.pyholmes/common/env_vars.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tests: match source structure under
tests/
Files:
tests/test_cors.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When writing documentation in the docs/ directory, always add a blank line between a header/bold text and a list, otherwise MkDocs won't render the list properly
Files:
docs/reference/environment-variables.md
🧬 Code graph analysis (1)
tests/test_cors.py (1)
holmes/utils/cache.py (1)
get(64-74)
🔇 Additional comments (4)
server.py (1)
150-152: LGTM: Clean parsing logic for comma-separated values.The parsing logic correctly handles comma-separated values with proper whitespace stripping and empty value filtering. This provides a good user experience for configuration.
tests/test_cors.py (3)
6-31: Imports inside fixtures violate coding guidelines but are necessary here.The fixtures import modules inside functions, which violates the coding guideline: "ALWAYS place Python imports at the top of the file, not inside functions or methods." However, this pattern is necessary here to support dynamic module reloading with patched environment variables for testing.
This is an acceptable exception for test fixtures that require environment isolation and module reloading.
Based on coding guidelines, imports should typically be at the top, but test fixtures requiring dynamic reloading are a valid exception.
Also applies to: 33-51
54-99: Excellent CORS behavior test coverage.The test suite comprehensively validates CORS functionality:
- Preflight OPTIONS requests with proper headers
- CORS headers on actual responses when enabled
- Origin whitelisting (rejects unlisted origins)
- Absence of CORS headers when disabled
This provides strong confidence in the CORS implementation.
101-163: Thorough validation of environment variable parsing.The test class comprehensively validates:
- Single origin parsing
- Multiple origins with whitespace handling
- Default CORS_ENABLED=false behavior
- CORS_ENABLED=true activation
This ensures the configuration layer works correctly.
| CORS_ENABLED = load_bool("CORS_ENABLED", False) | ||
| CORS_ALLOW_ORIGINS = os.environ.get("CORS_ALLOW_ORIGINS", "*") | ||
| CORS_ALLOW_CREDENTIALS = load_bool("CORS_ALLOW_CREDENTIALS", True) | ||
| CORS_ALLOW_METHODS = os.environ.get("CORS_ALLOW_METHODS", "*") | ||
| CORS_ALLOW_HEADERS = os.environ.get("CORS_ALLOW_HEADERS", "*") |
There was a problem hiding this comment.
Critical: Invalid default CORS configuration will be rejected by browsers.
The default combination of CORS_ALLOW_ORIGINS="*" with CORS_ALLOW_CREDENTIALS=True violates the CORS specification. Modern browsers will reject this configuration with an error like: "The value of the 'Access-Control-Allow-Origin' header in the response must not be the wildcard '*' when the request's credentials mode is 'include'."
When CORS is enabled, users must either:
- Set specific origins (not "*") when credentials are enabled, or
- Set
CORS_ALLOW_CREDENTIALS=falsewhen using wildcard origins
Consider adding runtime validation in server.py to detect and reject this invalid combination with a clear error message during startup.
🔎 Example validation logic to add in server.py
if CORS_ENABLED:
origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()]
# Validate CORS configuration
if "*" in origins and CORS_ALLOW_CREDENTIALS:
raise ValueError(
"Invalid CORS configuration: wildcard origin '*' cannot be used with "
"CORS_ALLOW_CREDENTIALS=true. Either specify explicit origins or set "
"CORS_ALLOW_CREDENTIALS=false."
)
logging.info(f"CORS enabled with origins: {origins}")
app.add_middleware(
CORSMiddleware,
allow_origins=origins,
allow_credentials=CORS_ALLOW_CREDENTIALS,
allow_methods=methods,
allow_headers=headers,
)🤖 Prompt for AI Agents
In holmes/common/env_vars.py around lines 124-128 the defaults set
CORS_ALLOW_ORIGINS="*" while CORS_ALLOW_CREDENTIALS=True which is invalid for
browsers; change the default CORS_ALLOW_CREDENTIALS to False to avoid unsafe
default, and add runtime validation in server.py during startup when
CORS_ENABLED is true: parse CORS_ALLOW_ORIGINS into a list, and if the list
contains "*" while CORS_ALLOW_CREDENTIALS is true raise a clear ValueError (and
log it) instructing the user to either specify explicit origins or disable
credentials so the service fails fast with actionable guidance.
| # Add CORS middleware if enabled | ||
| if CORS_ENABLED: | ||
| # Parse comma-separated values into lists | ||
| origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()] | ||
| methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()] | ||
| headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()] | ||
|
|
||
| logging.info(f"CORS enabled with origins: {origins}") | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=origins, | ||
| allow_credentials=CORS_ALLOW_CREDENTIALS, | ||
| allow_methods=methods, | ||
| allow_headers=headers, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation to prevent invalid CORS configuration at startup.
The CORS middleware setup is well-implemented, but it should validate the configuration before adding the middleware. Specifically, the combination of wildcard origin ("*") with credentials enabled is prohibited by the CORS specification and will cause browser errors.
🔎 Add validation logic before middleware setup
# Add CORS middleware if enabled
if CORS_ENABLED:
# Parse comma-separated values into lists
origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()]
methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()]
headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()]
+ # Validate CORS configuration
+ if "*" in origins and CORS_ALLOW_CREDENTIALS:
+ raise ValueError(
+ "Invalid CORS configuration: wildcard origin '*' cannot be used with "
+ "CORS_ALLOW_CREDENTIALS=true. Either specify explicit origins or set "
+ "CORS_ALLOW_CREDENTIALS=false."
+ )
+
logging.info(f"CORS enabled with origins: {origins}")
app.add_middleware(
CORSMiddleware,
allow_origins=origins,
allow_credentials=CORS_ALLOW_CREDENTIALS,
allow_methods=methods,
allow_headers=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.
| # Add CORS middleware if enabled | |
| if CORS_ENABLED: | |
| # Parse comma-separated values into lists | |
| origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()] | |
| methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()] | |
| headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()] | |
| logging.info(f"CORS enabled with origins: {origins}") | |
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=origins, | |
| allow_credentials=CORS_ALLOW_CREDENTIALS, | |
| allow_methods=methods, | |
| allow_headers=headers, | |
| ) | |
| # Add CORS middleware if enabled | |
| if CORS_ENABLED: | |
| # Parse comma-separated values into lists | |
| origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()] | |
| methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()] | |
| headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()] | |
| # Validate CORS configuration | |
| if "*" in origins and CORS_ALLOW_CREDENTIALS: | |
| raise ValueError( | |
| "Invalid CORS configuration: wildcard origin '*' cannot be used with " | |
| "CORS_ALLOW_CREDENTIALS=true. Either specify explicit origins or set " | |
| "CORS_ALLOW_CREDENTIALS=false." | |
| ) | |
| logging.info(f"CORS enabled with origins: {origins}") | |
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=origins, | |
| allow_credentials=CORS_ALLOW_CREDENTIALS, | |
| allow_methods=methods, | |
| allow_headers=headers, | |
| ) |
🤖 Prompt for AI Agents
In server.py around lines 147 to 161, the CORS setup currently parses lists and
adds CORSMiddleware but lacks validation: specifically, disallow the combination
of allow_origins containing "*" together with CORS_ALLOW_CREDENTIALS being true.
Before calling app.add_middleware, validate the parsed origins/methods/headers
and raise a clear startup error (or log and exit) if origins include "*" and
allow_credentials is true; also validate that origins is non-empty and that
methods/headers are sane (e.g., not empty lists) and fail fast with a
descriptive message so invalid CORS configs are caught at startup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server.py (1)
147-171: Consider validating for empty origins list.The wildcard + credentials validation is well implemented. However, if
CORS_ALLOW_ORIGINSis set to an empty string or only whitespace,originswill be an empty list, and the middleware will be added without any allowed origins—effectively blocking all cross-origin requests despite CORS being "enabled". This could cause confusion.🔎 Proposed validation for empty origins
if CORS_ENABLED: # Parse comma-separated values into lists origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()] methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()] headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()] allow_credentials = CORS_ALLOW_CREDENTIALS + if not origins: + logging.warning( + "CORS: CORS_ENABLED is true but CORS_ALLOW_ORIGINS is empty. " + "No cross-origin requests will be allowed." + ) + # Validate CORS configuration: wildcard origin with credentials is invalid if "*" in origins and allow_credentials:tests/test_cors.py (1)
141-153: Redundantos.environ.popand potential fragility withclear=True.The
patch.dict("os.environ", {}, clear=True)already clears all environment variables, making theos.environ.pop("CORS_ENABLED", None)on line 149 redundant. Additionally, clearing all env vars may cause the module reload to fail ifholmes.common.env_varsdepends on other environment variables (e.g., for required settings).🔎 Simplified test approach
def test_cors_enabled_default_is_false(self): """Test that CORS is disabled by default.""" - with patch.dict("os.environ", {}, clear=True): + # Only clear CORS_ENABLED to test the default, preserving other required env vars + import os + with patch.dict("os.environ", {k: v for k, v in os.environ.items() if k != "CORS_ENABLED"}): import importlib import holmes.common.env_vars - # Clear CORS_ENABLED from env if it exists - import os - os.environ.pop("CORS_ENABLED", None) - importlib.reload(holmes.common.env_vars) assert holmes.common.env_vars.CORS_ENABLED is False
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/reference/environment-variables.mdholmes/common/env_vars.pyserver.pytests/test_cors.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/reference/environment-variables.md
- holmes/common/env_vars.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
server.pytests/test_cors.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tests: match source structure under
tests/
Files:
tests/test_cors.py
🔇 Additional comments (10)
server.py (2)
30-30: LGTM!The CORSMiddleware import is correctly placed at the top of the file with other FastAPI-related imports.
42-46: LGTM!CORS environment variable imports are correctly grouped with the existing env_vars imports.
tests/test_cors.py (8)
1-4: LGTM!Standard test imports are correctly placed at the top of the file.
6-31: LGTM!The fixture correctly patches environment variables and reloads modules to pick up the new configuration. The dynamic imports inside the fixture are necessary for this reload pattern.
33-51: LGTM!Consistent fixture pattern for the CORS-disabled test case.
54-67: LGTM!Good coverage of CORS preflight (OPTIONS) request handling.
69-77: LGTM!Good test for verifying CORS headers are included on actual requests.
80-88: LGTM!Important security test verifying that unlisted origins don't receive CORS headers.
91-98: LGTM!Good test for verifying CORS headers are absent when CORS is disabled.
166-218: LGTM!Good test coverage for the wildcard origin with credentials edge case. The tests correctly verify that credentials are auto-disabled when using wildcard origins.
- Introduced CORS middleware configuration based on environment variables. - Added new environment variables for CORS settings: CORS_ENABLED, CORS_ALLOW_ORIGINS, CORS_ALLOW_CREDENTIALS, CORS_ALLOW_METHODS, and CORS_ALLOW_HEADERS. - CORS middleware is conditionally applied if CORS_ENABLED is set to true. This enhancement allows for better control over cross-origin requests, improving the application's flexibility in handling API calls from different origins. Signed-off-by: Mike Hsu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/common/env_vars.py (1)
124-128: Change default: CORS_ALLOW_CREDENTIALS should be False, not True.The default combination of
CORS_ALLOW_ORIGINS="*"(wildcard) withCORS_ALLOW_CREDENTIALS=Trueviolates the CORS specification. Althoughserver.pynow auto-corrects this at runtime by disabling credentials, the defaults themselves are misleading and force users into an auto-corrected configuration that differs from what they specified.Defaulting
CORS_ALLOW_CREDENTIALStoFalseprovides a valid, secure, and predictable starting point. Users who need credentials can explicitly enable them along with specific origins.🔎 Fix the default value
CORS_ENABLED = load_bool("CORS_ENABLED", False) CORS_ALLOW_ORIGINS = os.environ.get("CORS_ALLOW_ORIGINS", "*") -CORS_ALLOW_CREDENTIALS = load_bool("CORS_ALLOW_CREDENTIALS", True) +CORS_ALLOW_CREDENTIALS = load_bool("CORS_ALLOW_CREDENTIALS", False) CORS_ALLOW_METHODS = os.environ.get("CORS_ALLOW_METHODS", "*") CORS_ALLOW_HEADERS = os.environ.get("CORS_ALLOW_HEADERS", "*")
🧹 Nitpick comments (1)
server.py (1)
147-171: Good validation approach, but consider checking for empty configuration lists.The CORS setup correctly handles the wildcard-origin-with-credentials issue by auto-correcting it (setting
allow_credentials=False). This user-friendly approach is appropriate for the "Chill" review mode, though it differs from the past review suggestion to raise aValueError.However, empty lists could indicate misconfiguration. If
CORS_ALLOW_ORIGINSparses to an empty list (e.g., due to typos or whitespace-only values), no origins will be allowed, effectively blocking all CORS requests. Consider adding validation to warn users:🔎 Additional validation for empty configuration lists
# Add CORS middleware if enabled if CORS_ENABLED: # Parse comma-separated values into lists origins = [o.strip() for o in CORS_ALLOW_ORIGINS.split(",") if o.strip()] methods = [m.strip() for m in CORS_ALLOW_METHODS.split(",") if m.strip()] headers = [h.strip() for h in CORS_ALLOW_HEADERS.split(",") if h.strip()] allow_credentials = CORS_ALLOW_CREDENTIALS + # Validate non-empty configuration + if not origins: + logging.warning( + "CORS enabled but CORS_ALLOW_ORIGINS is empty. " + "No cross-origin requests will be allowed." + ) + if not methods: + logging.warning( + "CORS enabled but CORS_ALLOW_METHODS is empty. " + "This may block all CORS requests." + ) + if not headers: + logging.warning( + "CORS enabled but CORS_ALLOW_HEADERS is empty. " + "This may block requests with custom headers." + ) + # Validate CORS configuration: wildcard origin with credentials is invalid if "*" in origins and allow_credentials: logging.warning( "CORS: wildcard origin '*' with credentials=true is invalid. " "Automatically setting allow_credentials to false." ) allow_credentials = False logging.info(f"CORS enabled with origins: {origins}, credentials: {allow_credentials}") app.add_middleware( CORSMiddleware, allow_origins=origins, allow_credentials=allow_credentials, allow_methods=methods, allow_headers=headers, )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/reference/environment-variables.mdholmes/common/env_vars.pyserver.pytests/test_cors.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_cors.py
- docs/reference/environment-variables.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
server.pyholmes/common/env_vars.py
🔇 Additional comments (1)
server.py (1)
30-30: LGTM: CORS imports properly placed.The CORSMiddleware and CORS environment variable imports are correctly positioned after the custom certificate setup and follow the project's import guidelines.
Also applies to: 42-46
2 participants
Description
Add configurable CORS support to the FastAPI server via environment variables.
Changes
CORS_ENABLED,CORS_ALLOW_ORIGINS,CORS_ALLOW_CREDENTIALS,CORS_ALLOW_METHODS,CORS_ALLOW_HEADERSCORS_ENABLED=false) to maintain backward compatibilitydocs/reference/environment-variables.mdWhy
This allows users to configure CORS when deploying HolmesGPT as an API server, enabling cross-origin requests from web applications.
Testing
poetry run pytest tests/test_cors.py)Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.