Remove CORS policy for production environments#588
Merged
Conversation
Contributor
Author
|
@sourcery-ai review |
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRestricts CORS configuration to development environments only and tightens allowed origins for the dev SPA, ensuring production no longer uses a permissive wildcard CORS policy, with middleware updated accordingly and a new e2e test covering the previous wildcard bug. Sequence diagram for dev vs prod CORS handlingsequenceDiagram
actor DevUser
actor ProdUser
participant DevBrowser
participant ProdBrowser
participant Api
DevUser->>DevBrowser: Navigate to http://localhost:4200
DevBrowser->>Api: OPTIONS /api/resource
activate Api
Note over DevBrowser,Api: Origin http://localhost:4200
Api-->>DevBrowser: 200 with Access-Control-Allow-Origin http://localhost:4200
deactivate Api
DevBrowser->>Api: GET /api/resource
activate Api
Api-->>DevBrowser: 200 with data
deactivate Api
ProdUser->>ProdBrowser: Navigate to production frontend
ProdBrowser->>Api: OPTIONS /api/resource
activate Api
Note over ProdBrowser,Api: No CORS middleware in production
Api-->>ProdBrowser: 200 without wildcard CORS headers
deactivate Api
Note over ProdBrowser: Browser blocks cross-origin call if origin is not allowed
Flow diagram for ASP.NET middleware with dev-only CORSflowchart TD
Start["Start Program"] --> BuildBuilder["Create WebApplicationBuilder"]
BuildBuilder --> EnvCheck["Check builder.Environment.IsDevelopment"]
EnvCheck -->|true| AddCors["builder.Services.AddCors"]
AddCors --> AddPolicy["AddPolicy DevSpa with origins
http://localhost:4200,
http://127.0.0.1:4200"]
AddPolicy --> BuildApp["Build WebApplication"]
EnvCheck -->|false| BuildApp
BuildApp --> ConfigureApi["ConfigureApi extension"]
ConfigureApi --> SetupGuard["Use SetupGuardMiddleware"]
SetupGuard --> EnvCheckApp["Check app.Environment.IsDevelopment"]
EnvCheckApp -->|true| UseCorsDevSpa["app.UseCors DevSpa"]
UseCorsDevSpa --> Routing["app.UseRouting"]
EnvCheckApp -->|false| Routing
Routing --> Auth["app.UseAuthentication"]
Auth --> Authz["app.UseAuthorization"]
Authz --> MapEndpoints["Map controllers and SignalR hubs"]
MapEndpoints --> End["Run application"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The allowed CORS origins for the dev SPA are hard-coded; consider moving them to configuration so they can be adjusted without code changes and to avoid duplicating URLs in multiple places if they change.
- The CORS setup is now gated strictly on IsDevelopment, which will disable CORS in staging or other non-prod environments; confirm whether a broader non-production check (or environment-based configuration) would better match your deployment model.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The allowed CORS origins for the dev SPA are hard-coded; consider moving them to configuration so they can be adjusted without code changes and to avoid duplicating URLs in multiple places if they change.
- The CORS setup is now gated strictly on IsDevelopment, which will disable CORS in staging or other non-prod environments; confirm whether a broader non-production check (or environment-based configuration) would better match your deployment model.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR fixes the CORS wildcard vulnerability (GHSA-rwpc-36mg-fpvf) by replacing the Confidence Score: 4/5Safe to merge — the security fix is correct and no regressions are introduced; one minor test robustness concern exists. The backend changes are minimal, focused, and correct. The only finding is a P2 concern about the E2E test's unauthenticated status-code assertion, which does not affect correctness of the fix itself. e2e/tests/14-cors-wildcard-bug.spec.ts — the 200 status assertion may fail in unauthenticated E2E runs. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{IsDevelopment?}
B -- Yes --> C[UseCors DevSpa\nlocalhost:4200 only]
B -- No --> D[No CORS middleware\nproduction - same-origin only]
C --> E[UseRouting]
D --> E
E --> F[UseAuthentication]
F --> G[UseAuthorization]
G --> H[MapControllers / SignalR Hubs]
Reviews (1): Last reviewed commit: "added e2e tests" | Re-trigger Greptile |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe pull request modifies CORS policy registration to remove a permissive "Any" policy and replace it with a stricter "DevSpa" policy restricted to local SPA URLs in development environments only. A regression test is added to verify the vulnerability is not reintroduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Attacker as Attacker<br/>(malicious origin)
participant Client as Browser
participant Server as Server
Note over Attacker,Server: Vulnerable Configuration (Before)
Attacker->>Client: Initiate request from attacker origin
Client->>Server: OPTIONS preflight<br/>Origin: attacker.com
Server-->>Client: CORS: Allow-Origin: *<br/>(permits any origin)
Client->>Server: GET /api/auth/status<br/>Origin: attacker.com
Server-->>Client: 200 OK<br/>CORS: Allow-Origin: *
Note over Attacker: ❌ Vulnerability: Attacker origin allowed
Note over Attacker,Server: Fixed Configuration (After)
Attacker->>Client: Initiate request from attacker origin
Client->>Server: OPTIONS preflight<br/>Origin: attacker.com
Server-->>Client: CORS headers<br/>(restricted policy)<br/>Allow-Origin: NOT set
Client-xAttacker: Request blocked by browser
Note over Attacker: ✓ Fixed: Attacker origin rejected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests/14-cors-wildcard-bug.spec.ts (1)
15-17: Optionally strengthen ACAO assertions to require absence for disallowed origins.For this attacker-origin case, asserting
access-control-allow-originis undefined would make the regression test stricter.Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/14-cors-wildcard-bug.spec.ts` around lines 15 - 17, The test currently checks that the response header variable `acao` is neither `ATTACKER_ORIGIN` nor `'*'`, which is weaker; change the assertion(s) to require `acao` be undefined for disallowed origins by replacing the two expectations with a single assertion that `acao` is undefined (use the existing `acao` variable and `ATTACKER_ORIGIN` test case), and do the same for the other occurrence of these checks later in the file so both attacker-origin cases assert absence of the ACAO header.code/backend/Cleanuparr.Api/Program.cs (1)
91-94: Externalize allowed dev origins instead of hardcoding them.Consider loading the
DevSpaorigin list from configuration to avoid code changes when local dev topology changes.♻️ Suggested refactor
// CORS is needed only for development if (builder.Environment.IsDevelopment()) { + var devSpaOrigins = + builder.Configuration.GetSection("Cors:DevSpaOrigins").Get<string[]>() + ?? new[] { "http://localhost:4200", "http://127.0.0.1:4200" }; + builder.Services.AddCors(options => { options.AddPolicy("DevSpa", policy => { policy - .WithOrigins("http://localhost:4200", "http://127.0.0.1:4200") + .WithOrigins(devSpaOrigins) .AllowAnyHeader() .AllowAnyMethod() .AllowCredentials(); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/backend/Cleanuparr.Api/Program.cs` around lines 91 - 94, Replace the hardcoded origins passed to the CORS builder (.WithOrigins("http://localhost:4200", "http://127.0.0.1:4200")) with a list read from configuration (e.g., a "DevSpa:AllowedOrigins" array/string in appsettings or environment). In Program.cs obtain the IConfiguration (builder.Configuration) and read the array (or comma-separated string) into a string[] (with a safe fallback if null/empty), then pass that array into the existing .WithOrigins(...) call so the same CORS chain (.AllowAnyHeader().AllowAnyMethod().AllowCredentials()) uses configurable dev origins. Ensure parsing handles missing config and trims values before calling WithOrigins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/backend/Cleanuparr.Api/Program.cs`:
- Around line 91-94: Replace the hardcoded origins passed to the CORS builder
(.WithOrigins("http://localhost:4200", "http://127.0.0.1:4200")) with a list
read from configuration (e.g., a "DevSpa:AllowedOrigins" array/string in
appsettings or environment). In Program.cs obtain the IConfiguration
(builder.Configuration) and read the array (or comma-separated string) into a
string[] (with a safe fallback if null/empty), then pass that array into the
existing .WithOrigins(...) call so the same CORS chain
(.AllowAnyHeader().AllowAnyMethod().AllowCredentials()) uses configurable dev
origins. Ensure parsing handles missing config and trims values before calling
WithOrigins.
In `@e2e/tests/14-cors-wildcard-bug.spec.ts`:
- Around line 15-17: The test currently checks that the response header variable
`acao` is neither `ATTACKER_ORIGIN` nor `'*'`, which is weaker; change the
assertion(s) to require `acao` be undefined for disallowed origins by replacing
the two expectations with a single assertion that `acao` is undefined (use the
existing `acao` variable and `ATTACKER_ORIGIN` test case), and do the same for
the other occurrence of these checks later in the file so both attacker-origin
cases assert absence of the ACAO header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c76a5102-664c-4e70-892d-8c140eda8004
📒 Files selected for processing (3)
code/backend/Cleanuparr.Api/DependencyInjection/ApiDI.cscode/backend/Cleanuparr.Api/Program.cse2e/tests/14-cors-wildcard-bug.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to GHSA-rwpc-36mg-fpvf
Summary by Sourcery
Restrict CORS configuration to development and remove permissive wildcard policy from production.
Enhancements:
Tests: