-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(cors): Patch 3 follow-up bugs from CWE-942 CORS hardening #1323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ae51696
e8207f2
4ef4373
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import logging | ||
| import signal | ||
| import sys | ||
| import os | ||
| from typing import Dict, Optional, Set | ||
| from dataclasses import dataclass | ||
|
|
||
|
|
@@ -82,13 +83,37 @@ def _get_app(self): | |
| version="1.0.0", | ||
| ) | ||
|
|
||
| # Enable CORS for extension | ||
| # Configure CORS origins based on environment | ||
| cors_origins = os.getenv("BROWSER_CORS_ORIGINS", "").split(",") | ||
| cors_origins = [origin.strip() for origin in cors_origins if origin.strip() and origin.strip() != "*"] | ||
|
|
||
| # Default secure origins if none specified | ||
| if not cors_origins: | ||
| # Environment-specific defaults for security | ||
| if os.getenv("ENVIRONMENT") == "production": | ||
| # In production, require explicit configuration | ||
| cors_origins = [] | ||
| else: | ||
| # Development defaults - restrict to local origins | ||
| cors_origins = [ | ||
| "http://localhost:3000", # Development frontend | ||
| "http://localhost:8000", # Local development | ||
| "http://127.0.0.1:3000", # Local development | ||
| "http://127.0.0.1:8000", # Local development | ||
| ] | ||
|
|
||
| # Enable CORS for extension with secure origins. | ||
| # allow_origin_regex enables Chrome extension support since extension IDs | ||
| # (chrome-extension://<32-char-id>) cannot be listed as exact strings | ||
| # and the glob pattern chrome-extension://* is NOT supported by CORSMiddleware. | ||
| # Set BROWSER_CORS_ORIGINS to restrict to a specific extension ID. | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_origins=cors_origins, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The allow_origins list is constructed from environment variables and defaults. If the environment variable BROWSER_CORS_ORIGINS is empty, it defaults to a hardcoded list. This hardcoded list includes 'chrome-extension://', which is a broad wildcard. While better than '', it should be restricted to specific extension IDs if possible to adhere to the principle of least privilege.
qodo-code-review[bot] marked this conversation as resolved.
|
||
| allow_origin_regex=r"chrome-extension://[a-z0-9]{32}", | ||
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"], | ||
| allow_headers=["Authorization", "Content-Type", "Origin", "Accept"], | ||
| ) | ||
|
qodo-code-review[bot] marked this conversation as resolved.
Comment on lines
+86
to
117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: No. In Starlette’s If you need pattern matching (e.g., to allow Chrome extension origins), use Example (allow any Chrome extension origin—very permissive): CORSMiddleware(..., allow_origin_regex=r"chrome-extension://.*")Or safer (allow one specific extension ID): CORSMiddleware(..., allow_origins=["chrome-extension://<extension_id>"])Sources: Starlette middleware docs (CORS args) [1]; FastAPI reference showing Starlette CORS origin-matching logic [2]. Use Starlette's To allow Chrome extension origins, use the app.add_middleware(
CORSMiddleware,
allow_origin_regex=r"chrome-extension://.*",
allow_credentials=True,
allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
allow_headers=["Authorization", "Content-Type", "Origin", "Accept"],
)For stricter control, allow only a specific extension ID: 🤖 Prompt for AI Agents |
||
|
|
||
| @app.get("/health") | ||
|
|
@@ -113,20 +138,32 @@ async def _handle_connection(self, websocket): | |
| import time | ||
| import uuid | ||
| import os | ||
| import re | ||
|
|
||
| # Use same CORS origins configuration for WebSocket validation | ||
| cors_origins = os.getenv("BROWSER_CORS_ORIGINS", "").split(",") | ||
| cors_origins = [origin.strip() for origin in cors_origins if origin.strip() and origin.strip() != "*"] | ||
|
|
||
| if not cors_origins: | ||
| if os.getenv("ENVIRONMENT") == "production": | ||
| cors_origins = [] | ||
| else: | ||
| cors_origins = [ | ||
| "http://localhost:3000", "http://localhost:8000", | ||
| "http://127.0.0.1:3000", "http://127.0.0.1:8000" | ||
| ] | ||
|
|
||
| origin = websocket.headers.get("origin") | ||
| allowed_origins = os.environ.get("ALLOWED_ORIGINS", "").split(",") | ||
| if origin: | ||
| import urllib.parse | ||
| parsed_origin = urllib.parse.urlparse(origin) | ||
| is_allowed = False | ||
|
|
||
| if parsed_origin.scheme in ("http", "https") and parsed_origin.hostname in ("localhost", "127.0.0.1"): | ||
| is_allowed = True | ||
| elif parsed_origin.scheme == "chrome-extension": | ||
| # Check exact origin matches | ||
| if origin in cors_origins: | ||
| is_allowed = True | ||
| if any(origin == allowed.strip() for allowed in allowed_origins if allowed.strip()): | ||
| # Check chrome extension regex pattern (same as CORS middleware) | ||
| elif parsed_origin.scheme == "chrome-extension" and re.match(r"chrome-extension://[a-z0-9]{32}", origin): | ||
| is_allowed = True | ||
|
|
||
| if not is_allowed: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.