fix(security): fail-closed CORS default for prod image (#319)#330
fix(security): fail-closed CORS default for prod image (#319)#330dmytrocraft wants to merge 3 commits into
Conversation
The committed .env shipped a working dev CORS allow-list
(CORS_ALLOW_ORIGIN='^https?://(localhost|127\.0\.0\.1)(:[0-9]+)?$').
nelmio_cors enables origin_regex + allow_credentials for all paths, and
the Dockerfile prod stage runs `composer dump-env prod`, baking that
localhost regex into the production image. Unless an operator set
CORS_ALLOW_ORIGIN at deploy time, the prod API reflected any
http(s)://localhost / 127.0.0.1 Origin into Access-Control-Allow-Origin
with Access-Control-Allow-Credentials: true (CWE-942), letting a
localhost browser context read authenticated cross-origin responses.
An empty value is not a safe fallback: nelmio compiles allow_origin:['']
to the regex {}i which matches EVERY origin (fail-open). The fix uses a
non-empty deny-all default instead.
Fix:
- .env: default CORS_ALLOW_ORIGIN to the deny-all regex '(?!)' (matches no
origin) and document it as a required HTTPS-only deploy variable. The
dev localhost regex no longer ships in the prod artifact.
- .env.dev: add the localhost allow-list for local development.
- .env.test: keep the localhost allow-list (documented as a test override).
- tests/Unit/Config/CorsAllowOriginDefaultTest.php: pin the fail-closed
default; assert it denies localhost/127.0.0.1/prod/evil origins, is not
empty (fail-open), keeps dev/test localhost allow-lists, and that nelmio
still binds allow_origin to %env(CORS_ALLOW_ORIGIN)% in all envs.
Local verification (one-off containers, vendor mounted read-only):
- phpunit Unit --filter Cors: OK (5 tests, 25 assertions); fails on
original .env (proves real regression test)
- phpunit full Unit suite: OK (2263 tests)
- deptrac: Violations 0, Errors 0
- psalm (changed file): No errors found
- php-cs-fixer (changed file): 0 of 1 files
Closes #319
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 17 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 5/5
- This looks low risk to merge: the only finding is low severity (3/10) and affects contributor workflow rather than runtime behavior.
- In
specs/security-319-prod-cors-dev-regex-credentials/stories.md, verification commands use machine-specific absolute paths (/home/kravtsov/Projects/...), so other developers will need to edit commands before running them. - Pay close attention to
specs/security-319-prod-cors-dev-regex-credentials/stories.md- replace hardcoded absolute paths with portable/project-relative commands.
Architecture diagram
sequenceDiagram
participant ENV as .env (Committed)
participant Dotenv as Symfony Dotenv
participant CI as Docker Build (prod)
participant Runtime as PHP Runtime (prod)
participant CORS as nelmio/cors-bundle
participant Browser as Browser (client)
Note over ENV,Browser: CORS Configuration Resolution Flow
ENV->>Dotenv: Contains CORS_ALLOW_ORIGIN='(?!)'
Dotenv->>CI: composer dump-env prod bakes into .env.local.php
CI->>Runtime: Production image ships baked .env.local.php
Runtime->>CORS: Reads %env(CORS_ALLOW_ORIGIN)%
CORS->>CORS: Compiles '(?!)' as regex {(?!)}i
alt Dev/Test environment (.env.dev / .env.test)
ENV->>Dotenv: Contains CORS_ALLOW_ORIGIN='^https?://(localhost|127\.0\.0\.1)(:[0-9]+)?$'
Dotenv->>Runtime: Overrides .env default
Runtime->>CORS: Reads %env(CORS_ALLOW_ORIGIN)% = localhost regex
Browser->>CORS: OPTIONS /api with Origin: http://localhost:3000
CORS->>CORS: Regex matches localhost
CORS-->>Browser: Access-Control-Allow-Origin: http://localhost:3000 + Access-Control-Allow-Credentials: true
else Production (no operator override)
Runtime->>CORS: Uses deny-all regex from baked .env
Browser->>CORS: OPTIONS /api with Origin: http://localhost
CORS->>CORS: Regex {(?!)}i matches NO origin
CORS-->>Browser: No CORS headers (fail-closed)
else Production (operator configured)
ENV->>Runtime: Operator sets CORS_ALLOW_ORIGIN='^https://(app|www)\.example\.com$'
Browser->>CORS: OPTIONS /api with Origin: https://app.example.com
CORS->>CORS: Regex matches trusted origin
CORS-->>Browser: Access-Control-Allow-Origin: https://app.example.com + Access-Control-Allow-Credentials: true
end
Note over ENV,CORS: Key Security Boundary: Production image never ships dev regex
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
=============================================
Coverage 100.00% 100.00%
- Complexity 0 2768 +2768
=============================================
Files 551 551
Lines 9541 9541
=============================================
Hits 9541 9541
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Replace hardcoded developer-machine absolute host paths in the spec verification commands with portable $APP_ROOT (git rev-parse) and a configurable $APP_IMAGE so any contributor can run them as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Security fix — Closes #319
Vulnerability (CWE-942, HIGH)
The committed
.envshipped a working dev CORS allow-list:config/packages/nelmio_cors.yamlenablesorigin_regex: trueandallow_credentials: truefor all paths (/api,/api/graphql,/authorize) and readsallow_originfrom%env(CORS_ALLOW_ORIGIN)%. The Dockerfile prod stage runscomposer dump-env prod, which bakes that localhost regex into the production image (.env.local.php). Unless an operator explicitly setCORS_ALLOW_ORIGINat deploy time, the production API reflected anyhttp(s)://localhost/127.0.0.1Origin intoAccess-Control-Allow-Origintogether withAccess-Control-Allow-Credentials: true. Because the JWT auth token is accepted from the__Host-auth_tokencookie and theAuthorizationheader, a browser context the browser treats asOrigin: http://localhost(local dev server, webview, browser-extension page, DNS-rebinding to 127.0.0.1) could read authenticated cross-origin responses.An empty value is not a safe fallback: nelmio compiles
allow_origin: ['']to the regex{}i, which matches every origin (fail-open, verified). So the fix uses a non-empty deny-all default.Fix
.env: defaultCORS_ALLOW_ORIGINto the deny-all regex'(?!)'(matches no origin) and document it as a required HTTPS-only deploy variable. The dev localhost regex no longer ships in the prod artifact — prod now fails closed..env.dev: add the localhost allow-list for local development..env.test: keep the localhost allow-list (documented as a test override).tests/Unit/Config/CorsAllowOriginDefaultTest.php: new regression test that pins the fail-closed default, asserts it denies localhost/127.0.0.1/prod/evil origins, asserts it is non-empty (not fail-open), confirms dev/test keep their localhost allow-lists, and confirms nelmio still bindsallow_originto%env(CORS_ALLOW_ORIGIN)%in dev/test/prod.Local verification (one-off containers, vendor mounted read-only)
phpunit Unit --filter Cors: OK (5 tests, 25 assertions) — and it fails against the original.env(proving it is a real regression test).phpunitfull Unit suite: OK (2263 tests) — no regressions.deptrac: Violations 0, Errors 0.psalm(changed file): No errors found.php-cs-fixer(changed file): 0 of 1 files.BMAD spec
specs/security-319-prod-cors-dev-regex-credentials/(prd.md,stories.md).🤖 Generated with Claude Code
Summary by cubic
Sets a fail-closed CORS default for the production image to prevent localhost origins with credentials from being accepted when
CORS_ALLOW_ORIGINisn’t set (CWE-942). Moves the localhost regex to dev/test and adds a regression test to lock this down.Bug Fixes
.env: defaultCORS_ALLOW_ORIGINto(?!)(deny-all) and document it as a required deploy variable..env.devand.env.test: use the localhost allow-list for local and CI.CorsAllowOriginDefaultTestto pin the deny-all default and verifyallow_origin: ['%env(CORS_ALLOW_ORIGIN)%']across dev/test/prod innelmio_cors.yaml(vianelmio/cors-bundle).$APP_ROOTand$APP_IMAGEinspecs/security-319-prod-cors-dev-regex-credentials/stories.md.Migration
CORS_ALLOW_ORIGINto your trusted HTTPS origins (regex), e.g. '^https://(app|www).example.com$'. An empty value is unsafe undernelmio/cors-bundle; until set, cross-origin requests are denied.Written for commit 7602d60. Summary will update on new commits.