fix(cors): Patch 3 follow-up bugs from CWE-942 CORS hardening#1323
fix(cors): Patch 3 follow-up bugs from CWE-942 CORS hardening#1323MervinPraison merged 3 commits intomainfrom
Conversation
…wildcard origins - Replace allow_origins=['*'] with secure environment-based configurations - Update browser server to use BROWSER_CORS_ORIGINS env var with localhost defaults - Update jobs server to use JOBS_CORS_ORIGINS env var with localhost defaults - Update example API to use API_CORS_ORIGINS env var with localhost defaults - Remove wildcard defaults from ServerConfig, GatewayConfig, and AppConfig - Add secure localhost defaults to MCP server transport - Update tests and documentation to reflect secure defaults - Restrict CORS methods and headers to essential ones only Fixes #1321 - Addresses 3 instances of CWE-942 vulnerabilities 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
|
@gemini review this PR |
✅ Actions performedReview triggered.
|
Code Review by Qodo
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request addresses CORS security vulnerabilities (CWE-942) across the PraisonAI codebase by replacing wildcard origin configurations ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
This pull request significantly improves security by replacing wildcard CORS configurations with more restrictive defaults and environment-based configurations across multiple services. Default origins are now generally empty or restricted to local development addresses, and allowed HTTP methods and headers have been explicitly defined. Feedback suggests further tightening the browser extension origin wildcard and moving hardcoded transport defaults to configuration files for better environment flexibility.
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_origins=cors_origins, |
There was a problem hiding this comment.
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.
| self.cors_origins = [ | ||
| "http://localhost:3000", | ||
| "http://127.0.0.1:3000", | ||
| "http://localhost:8000", | ||
| "http://127.0.0.1:8000" | ||
| ] |
There was a problem hiding this comment.
Hardcoding localhost origins as a default is a security improvement, but these should ideally be configurable via environment variables or a configuration file rather than being hardcoded in the transport class to allow for different deployment environments.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above first. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai/praisonai/mcp_server/transports/http_stream.py (1)
358-366:⚠️ Potential issue | 🟡 Minor
allow_headers=["*"]is inconsistent with other CORS changes in this PR.Other files in this PR (e.g.,
browser/server.py,jobs/server.py) restrictallow_headersto an explicit list like["Authorization", "Content-Type", "Origin", "Accept"]. This middleware still uses the wildcard, which doesn't align with the PR objective to "restrict CORS to necessary methods and headers."🔧 Proposed fix for header restriction
middleware = [ Middleware( CORSMiddleware, allow_origins=self.cors_origins, allow_methods=["GET", "POST", "DELETE", "OPTIONS"], - allow_headers=["*"], + allow_headers=["Content-Type", "Accept", "Authorization", "MCP-Session-Id", "Mcp-Session-Id", "MCP-Protocol-Version", "Mcp-Protocol-Version", "Last-Event-ID"], ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/mcp_server/transports/http_stream.py` around lines 358 - 366, Update the CORSMiddleware instantiation in the middleware list so it uses an explicit allow_headers list instead of the wildcard; locate the middleware variable creation where CORSMiddleware is configured (referenced by CORSMiddleware and middleware) and replace allow_headers=["*"] with the same explicit header set used elsewhere (e.g. ["Authorization", "Content-Type", "Origin", "Accept"]) while keeping allow_origins=self.cors_origins and the allow_methods unchanged.src/praisonai-agents/praisonaiagents/server/server.py (1)
199-205:⚠️ Potential issue | 🟠 MajorRestrict CORS
allow_methodsandallow_headersto specific values for security.The CORS middleware still uses wildcard
["*"]on lines 203-204, which is inconsistent with other servers in this PR (jobs/server.py, browser/server.py, and mcp_server all restrict these to specific methods and headers). Since this is in the core SDK, align with the more restrictive implementations:Suggested fix
app = CORSMiddleware( app, allow_origins=self.config.cors_origins, - allow_methods=["*"], - allow_headers=["*"], + allow_methods=["GET", "POST", "OPTIONS"], + allow_headers=["Content-Type", "Authorization"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/server/server.py` around lines 199 - 205, The CORS setup for CORSMiddleware currently uses wildcards for allow_methods and allow_headers; update the CORSMiddleware instantiation (the app variable created where CORSMiddleware is called) to replace allow_methods=["*"] and allow_headers=["*"] with explicit lists (e.g., allow_methods=["GET","POST","PUT","PATCH","DELETE","OPTIONS"] and allow_headers=["Authorization","Content-Type","Accept","X-Requested-With"]) while keeping allow_origins=self.config.cors_origins so it matches the more restrictive implementations in the other servers.
🧹 Nitpick comments (3)
src/praisonai/praisonai/recipe/serve.py (1)
680-681: Defaultcors_methodsandcors_headersfall back to wildcards.While
cors_originsis now secure by default, thecors_methodsandcors_headersstill default to["*"]viaconfig.get("cors_methods", ["*"]). For consistency with the PR's security hardening goals and other files, consider restricting these defaults.♻️ Proposed fix for explicit method/header defaults
middleware.append( Middleware( CORSMiddleware, allow_origins=origins, - allow_methods=config.get("cors_methods", ["*"]), - allow_headers=config.get("cors_headers", ["*"]), + allow_methods=config.get("cors_methods", ["GET", "POST", "PUT", "DELETE", "OPTIONS"]), + allow_headers=config.get("cors_headers", ["Authorization", "Content-Type", "Origin", "Accept"]), allow_credentials=config.get("cors_credentials", False), max_age=config.get("cors_max_age", 600), ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/recipe/serve.py` around lines 680 - 681, The current CORS defaults use wildcards via config.get("cors_methods", ["*"]) and config.get("cors_headers", ["*"]); update these to restrictive explicit defaults instead: change the fallback for cors_methods in the allow_methods argument to a small safe list (e.g., ["GET","POST","OPTIONS"] or whatever your API needs) and change the fallback for cors_headers in allow_headers to an explicit safe list (e.g., ["Authorization","Content-Type","Accept"]). Locate the call that passes allow_methods and allow_headers (where config.get("cors_methods", ["*"]) and config.get("cors_headers", ["*"]) are used) and replace the wildcard fallbacks with the chosen explicit lists so the defaults are secure unless the config explicitly overrides them.src/praisonai-agents/praisonaiagents/server/server.py (1)
30-30: Security improvement: CORS origins default tightened.The change from
["*"]to[]correctly addresses CWE-942 by defaulting to no allowed cross-origin requests.Same minor style note as in
app/config.py:default_factory=listis more idiomatic.♻️ Optional: Use idiomatic default_factory
- cors_origins: List[str] = field(default_factory=lambda: []) + cors_origins: List[str] = field(default_factory=list),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/server/server.py` at line 30, The cors_origins dataclass field currently uses default_factory=lambda: [] which works but is non-idiomatic; update the field definition for cors_origins (the List[str] field named cors_origins) to use default_factory=list instead of a lambda to make the code clearer and idiomatic while keeping the empty list default that tightens CORS by allowing no origins by default.src/praisonai-agents/praisonaiagents/app/config.py (1)
24-24: Security improvement: CORS defaults tightened appropriately.The change from
["*"]to[]correctly remediates CWE-942 by defaulting to no allowed origins. This is a breaking change for users relying on permissive defaults, but aligns with security best practices.Minor style note:
default_factory=listis more idiomatic thandefault_factory=lambda: [].♻️ Optional: Use idiomatic default_factory
- cors_origins: List[str] = field(default_factory=lambda: []) + cors_origins: List[str] = field(default_factory=list),
Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/app/config.py` at line 24, Update the Pydantic field(s) that define CORS defaults (e.g., the cors_origins attribute in the config class) to use the idiomatic default_factory=list instead of default_factory=lambda: [] (apply the same change where cors_origins appears around the other instance noted) so the default remains an empty list but uses the clearer, more idiomatic factory reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/browser/server.py`:
- Around line 86-107: The CORS setup in server.py uses a wildcard string
"chrome-extension://*" in cors_origins which CORSMiddleware will treat as a
literal origin; update the app.add_middleware(CORSMiddleware, ...) call so that
chrome-extension origins are handled via allow_origin_regex (e.g.,
r"chrome-extension://.*") instead of being placed in cors_origins, keep the
other explicit origins in the cors_origins list and pass them via allow_origins,
and ensure allow_credentials/allow_methods/allow_headers remain unchanged;
alternatively, if you want stricter control, include an exact
chrome-extension://<extension-id> entry in cors_origins.
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/server/server.py`:
- Around line 199-205: The CORS setup for CORSMiddleware currently uses
wildcards for allow_methods and allow_headers; update the CORSMiddleware
instantiation (the app variable created where CORSMiddleware is called) to
replace allow_methods=["*"] and allow_headers=["*"] with explicit lists (e.g.,
allow_methods=["GET","POST","PUT","PATCH","DELETE","OPTIONS"] and
allow_headers=["Authorization","Content-Type","Accept","X-Requested-With"])
while keeping allow_origins=self.config.cors_origins so it matches the more
restrictive implementations in the other servers.
In `@src/praisonai/praisonai/mcp_server/transports/http_stream.py`:
- Around line 358-366: Update the CORSMiddleware instantiation in the middleware
list so it uses an explicit allow_headers list instead of the wildcard; locate
the middleware variable creation where CORSMiddleware is configured (referenced
by CORSMiddleware and middleware) and replace allow_headers=["*"] with the same
explicit header set used elsewhere (e.g. ["Authorization", "Content-Type",
"Origin", "Accept"]) while keeping allow_origins=self.cors_origins and the
allow_methods unchanged.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/app/config.py`:
- Line 24: Update the Pydantic field(s) that define CORS defaults (e.g., the
cors_origins attribute in the config class) to use the idiomatic
default_factory=list instead of default_factory=lambda: [] (apply the same
change where cors_origins appears around the other instance noted) so the
default remains an empty list but uses the clearer, more idiomatic factory
reference.
In `@src/praisonai-agents/praisonaiagents/server/server.py`:
- Line 30: The cors_origins dataclass field currently uses
default_factory=lambda: [] which works but is non-idiomatic; update the field
definition for cors_origins (the List[str] field named cors_origins) to use
default_factory=list instead of a lambda to make the code clearer and idiomatic
while keeping the empty list default that tightens CORS by allowing no origins
by default.
In `@src/praisonai/praisonai/recipe/serve.py`:
- Around line 680-681: The current CORS defaults use wildcards via
config.get("cors_methods", ["*"]) and config.get("cors_headers", ["*"]); update
these to restrictive explicit defaults instead: change the fallback for
cors_methods in the allow_methods argument to a small safe list (e.g.,
["GET","POST","OPTIONS"] or whatever your API needs) and change the fallback for
cors_headers in allow_headers to an explicit safe list (e.g.,
["Authorization","Content-Type","Accept"]). Locate the call that passes
allow_methods and allow_headers (where config.get("cors_methods", ["*"]) and
config.get("cors_headers", ["*"]) are used) and replace the wildcard fallbacks
with the chosen explicit lists so the defaults are secure unless the config
explicitly overrides them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2216aade-8397-4d1b-905f-735f0037e925
📒 Files selected for processing (10)
examples/python/api/secondary-market-research-api.pysrc/praisonai-agents/praisonaiagents/app/config.pysrc/praisonai-agents/praisonaiagents/gateway/config.pysrc/praisonai-agents/praisonaiagents/server/server.pysrc/praisonai-agents/tests/unit/server/test_server.pysrc/praisonai-agents/tests/unit/test_gateway_config.pysrc/praisonai/praisonai/browser/server.pysrc/praisonai/praisonai/jobs/server.pysrc/praisonai/praisonai/mcp_server/transports/http_stream.pysrc/praisonai/praisonai/recipe/serve.py
| # 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()] | ||
|
|
||
| # Default secure origins if none specified | ||
| if not cors_origins: | ||
| cors_origins = [ | ||
| "chrome-extension://*", # Allow Chrome extension 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 | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_origins=cors_origins, | ||
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"], | ||
| allow_headers=["Authorization", "Content-Type", "Origin", "Accept"], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Starlette CORSMiddleware support wildcard patterns like chrome-extension://* in allow_origins?
💡 Result:
No. In Starlette’s CORSMiddleware, allow_origins is only an explicit list of origin strings, with the only wildcard being the literal ["*"] (allow any origin). It does not support pattern entries like chrome-extension://*. [1]
If you need pattern matching (e.g., to allow Chrome extension origins), use allow_origin_regex. Internally, Starlette checks allow_origin_regex.fullmatch(origin) and otherwise falls back to origin in allow_origins (exact match). [2]
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 allow_origin_regex to support Chrome extension origins instead of wildcard patterns.
Starlette's CORSMiddleware does not support pattern matching in allow_origins—it only accepts an explicit list of origin strings or the literal ["*"] (allow any origin). The pattern "chrome-extension://*" will be treated as an exact match for that literal string, not as a wildcard pattern.
To allow Chrome extension origins, use the allow_origin_regex parameter instead:
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: allow_origins=["chrome-extension://<your-extension-id>"].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/browser/server.py` around lines 86 - 107, The CORS
setup in server.py uses a wildcard string "chrome-extension://*" in cors_origins
which CORSMiddleware will treat as a literal origin; update the
app.add_middleware(CORSMiddleware, ...) call so that chrome-extension origins
are handled via allow_origin_regex (e.g., r"chrome-extension://.*") instead of
being placed in cors_origins, keep the other explicit origins in the
cors_origins list and pass them via allow_origins, and ensure
allow_credentials/allow_methods/allow_headers remain unchanged; alternatively,
if you want stricter control, include an exact chrome-extension://<extension-id>
entry in cors_origins.
|
@claude You are the FINAL architecture reviewer. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review
|
…tp_stream headers, server.py methods/headers Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/b76a2075-da18-4ad5-82e8-79fc09457529 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Thorough review completed. Found and fixed 3 bugs; notes on other issues below. Bugs fixed (commit e8207f2):
Additional findings (not fixed — out of scope or trade-offs):
|
|
@claude fix all issues if any , make sure these are requiref fieates an dthen only implement no bloat. fix all |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review & Issue Analysis
|
|
@claude you can fix your changes here in this branch |
|
Claude finished @MervinPraison's task —— View job CORS Security Fixes Implementation ✅ COMPLETEDSuccessfully implemented all critical security fixes for the CORS hardening PR based on comprehensive reviewer feedback from Gemini, Qodo, CodeRabbit, and Copilot. All Issues Fixed ✅Critical Security Vulnerabilities Resolved:
Files Modified:
Security Improvements:
The CORS hardening PR is now ready for merge - all critical security vulnerabilities have been addressed. |
- Add environment-based CORS defaults (empty for production) - Validate env vars to reject wildcard origins (*) - Add missing Idempotency-Key header to jobs CORS - Unify WebSocket and HTTP CORS origin validation - Make http_stream transport environment-aware Fixes all critical security issues identified by code reviewers: - Production no longer allows localhost by default - Environment variables cannot bypass security with * - WebSocket uses same origin validation as CORS middleware - Jobs API properly supports Idempotency-Key header 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
The previous CORS hardening pass left three bugs that could break functionality or leave wildcards in place.
Fixes
chrome-extension://*is an invalid CORS origin (browser/server.py):CORSMiddlewaredoes exact string matching inallow_origins; the glob never matches real extension URLs likechrome-extension://abcdefghijklmnop.... Replaced withallow_origin_regex=r"chrome-extension://[a-z0-9]{32}":allow_headers=["*"]not restricted inhttp_stream.py:allow_methodswas restricted butallow_headerswas left as a wildcard in the same block. Restricted to essential headers + MCP-specific ones (Mcp-Session-Id,Last-Event-Id).allow_methods=["*"]/allow_headers=["*"]still inpraisonaiagents/server/server.py:cors_originsdefault was tightened but method/header wildcards were missed. Restricted toGET, POST, OPTIONSand essential headers.