Fix forwarded header parsing#586
Merged
Merged
Conversation
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes GHSA-8q44-v65j-jc3q by replacing the old leftmost-XFF approach (attacker-controlled) with a new Confidence Score: 4/5Safe to merge; the core security fix is correct and well-tested. Two P2 hardening items remain around proto/host header sanitisation. No P0 or P1 findings. The right-to-left XFF traversal correctly addresses the GHSA advisory. The two P2 comments are defence-in-depth improvements that require a misconfigured or compromised trusted proxy to matter in practice. code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs — proto and host header sanitisation
|
| Filename | Overview |
|---|---|
| code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs | New middleware implementing right-to-left XFF chain traversal to fix GHSA-8q44-v65j-jc3q; core logic is correct but X-Forwarded-Proto and X-Forwarded-Host values are not sanitised before being written to the request context |
| code/backend/Cleanuparr.Api/Auth/TrustedNetworkAuthenticationHandler.cs | Simplified to use injected DataContext and delegates IP resolution to the new middleware; ResolveClientIp now just reads the already-rewritten RemoteIpAddress |
| code/backend/Cleanuparr.Api/Extensions/HttpRequestExtensions.cs | GetExternalBaseUrl simplified to read Scheme/Host directly now that middleware normalises them upstream |
| code/backend/Cleanuparr.Api.Tests/Middleware/TrustedForwardedHeadersMiddlewareTests.cs | New test suite covering spoofing, malformed entries, empty entries, IPv4-mapped loopback, custom CIDR trusted networks, and proto/host propagation |
| e2e/tests/13-trusted-network-xff-bug.spec.ts | E2E regression test for GHSA-8q44-v65j-jc3q using nginx attacker-simulation path; validates spoofed XFF and X-Real-IP are both rejected |
Reviews (1): Last reviewed commit: "added e2e tests" | Re-trigger Greptile
📝 WalkthroughWalkthroughA new middleware centralizes secure handling of X-Forwarded-* headers for requests from trusted networks, updating request context accordingly. The authentication handler was refactored to use dependency injection and simplified IP resolution. The system now processes forwarded headers early in the request pipeline rather than distributed across multiple components. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Proxy as Nginx Proxy
participant Middleware as TrustedForwarded<br/>HeadersMiddleware
participant AuthHandler as TrustedNetwork<br/>AuthHandler
participant DataCtx as DataContext
participant App as App Logic
Client->>Proxy: GET /resource<br/>(from public IP)
Proxy->>Middleware: Forward request<br/>RemoteIp=proxy_ip<br/>X-Forwarded-For: client_ip
Middleware->>DataCtx: Load TrustForwardedHeaders<br/>& TrustedNetworks
DataCtx-->>Middleware: Config + networks
Middleware->>Middleware: Validate proxy_ip<br/>against TrustedNetworks
alt Proxy is Trusted
Middleware->>Middleware: Parse X-Forwarded-For,<br/>update RemoteIpAddress
Middleware->>Middleware: Apply X-Forwarded-Proto,<br/>X-Forwarded-Host
else Proxy is Untrusted
Middleware->>Middleware: Skip header processing<br/>keep original metadata
end
Middleware->>AuthHandler: Pass updated HttpContext
AuthHandler->>AuthHandler: Call ResolveClientIp()<br/>returns RemoteIpAddress
AuthHandler->>AuthHandler: Check if IP is<br/>in TrustedNetworks
AuthHandler-->>App: Auth result
App-->>Client: HTTP 200 / 401
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/docker-compose.e2e.yml (1)
39-46: Potential race condition: no healthcheck for nginx service.The
depends_on: - apponly ensures theappcontainer starts, not that it's ready to accept connections. Since neitherappnornginxhave healthchecks, tests may start before both services are actually ready, leading to flaky test runs.Consider adding a healthcheck or startup delay mechanism, or have the test suite poll for readiness before running tests.
💡 Example healthcheck for nginx
nginx: image: nginx:1.27-alpine network_mode: host volumes: - ./nginx/nginx.conf:/etc/nginx/nginx.conf:ro depends_on: - app + healthcheck: + test: ["CMD-SHELL", "curl -sf http://localhost:8000/ || exit 1"] + interval: 2s + timeout: 2s + retries: 10 + start_period: 5sNote: This requires
curlin the container or using a netcat alternative. Alternatively, ensure the test harness waits for port 8000 availability before running tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/docker-compose.e2e.yml` around lines 39 - 46, Add a healthcheck for the nginx service (the nginx block) so Docker Compose waits for nginx to be considered healthy rather than just started by depends_on: - app; implement a healthcheck that probes the nginx endpoint (e.g., curl or a TCP check against the nginx port) and set sensible interval/retries/start-period values, and also add a complementary healthcheck for the app service (or ensure the test harness polls the app/nginx readiness) so tests only run after both nginx and app are truly ready.code/backend/Cleanuparr.Api/Auth/TrustedNetworkAuthenticationHandler.cs (1)
79-80: Add XML docs forResolveClientIp.This helper’s behavior changed materially in this PR, and callers now have to know that
TrustedForwardedHeadersMiddlewaremust have normalizedConnection.RemoteIpAddressfirst.As per coding guidelines, "Add XML documentation comments for public APIs in C#".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/backend/Cleanuparr.Api/Auth/TrustedNetworkAuthenticationHandler.cs` around lines 79 - 80, Add an XML documentation comment for the public helper ResolveClientIp (in TrustedNetworkAuthenticationHandler) that explains its behavior and contract: it returns httpContext.Connection.RemoteIpAddress and therefore requires that TrustedForwardedHeadersMiddleware has already normalized the Connection.RemoteIpAddress for forwarded client IPs; document the nullable return (IPAddress?) and any threading/context expectations so callers know they must run the middleware before invoking ResolveClientIp.code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs (1)
19-25: Document the new public middleware surface.The class summary helps, but the public constructor,
InvokeAsync, andApplyForwardedHeadersstill lack XML docs.As per coding guidelines, "Add XML documentation comments for public APIs in C#".
Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs` around lines 19 - 25, Add XML documentation comments for the public middleware API: add a summary and param/returns tags to the constructor TrustedForwardedHeadersMiddleware(RequestDelegate next), to the InvokeAsync(HttpContext context) method, and to the ApplyForwardedHeaders(...) method (the public method referenced in the class) describing their purpose, parameters, and behavior; ensure the constructor doc explains that it initializes the middleware with the next RequestDelegate, InvokeAsync documents that it processes the HttpContext and calls the next middleware asynchronously, and ApplyForwardedHeaders documents what headers it applies/filters and any exceptions or side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs`:
- Around line 94-103: The middleware is assigning raw comma-separated header
values to Request.Scheme and Request.Host; update
TrustedForwardedHeadersMiddleware to split the X-Forwarded-Proto and
X-Forwarded-Host header values on ',' and use the first non-empty trimmed token
(per-hop consumption) when setting context.Request.Scheme and
context.Request.Host (use new HostString(parsedHost)). Locate the assignments
where forwardedProto and forwardedHost are read and replace the raw value usage
with the parsed/trimmed first element.
- Around line 71-92: The loop in TrustedForwardedHeadersMiddleware mutates
context.Connection.RemoteIpAddress as it parses entries, allowing a malformed
trailing entry to leave RemoteIpAddress set to a trusted hop; instead, fully
validate the X-Forwarded-For chain first: iterate entries (from right-to-left)
and parse each using IPAddress.TryParse, detect any malformed entry and abort
without changing context, and determine the final client IP only after verifying
the chain with IsTrustedHop(trustedNetworks) logic; finally, if validation
succeeds and consumedAtLeastOne, assign the chosen IP to
context.Connection.RemoteIpAddress so ResolveClientIp() will see a
safely-validated value.
---
Nitpick comments:
In `@code/backend/Cleanuparr.Api/Auth/TrustedNetworkAuthenticationHandler.cs`:
- Around line 79-80: Add an XML documentation comment for the public helper
ResolveClientIp (in TrustedNetworkAuthenticationHandler) that explains its
behavior and contract: it returns httpContext.Connection.RemoteIpAddress and
therefore requires that TrustedForwardedHeadersMiddleware has already normalized
the Connection.RemoteIpAddress for forwarded client IPs; document the nullable
return (IPAddress?) and any threading/context expectations so callers know they
must run the middleware before invoking ResolveClientIp.
In `@code/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.cs`:
- Around line 19-25: Add XML documentation comments for the public middleware
API: add a summary and param/returns tags to the constructor
TrustedForwardedHeadersMiddleware(RequestDelegate next), to the
InvokeAsync(HttpContext context) method, and to the ApplyForwardedHeaders(...)
method (the public method referenced in the class) describing their purpose,
parameters, and behavior; ensure the constructor doc explains that it
initializes the middleware with the next RequestDelegate, InvokeAsync documents
that it processes the HttpContext and calls the next middleware asynchronously,
and ApplyForwardedHeaders documents what headers it applies/filters and any
exceptions or side effects.
In `@e2e/docker-compose.e2e.yml`:
- Around line 39-46: Add a healthcheck for the nginx service (the nginx block)
so Docker Compose waits for nginx to be considered healthy rather than just
started by depends_on: - app; implement a healthcheck that probes the nginx
endpoint (e.g., curl or a TCP check against the nginx port) and set sensible
interval/retries/start-period values, and also add a complementary healthcheck
for the app service (or ensure the test harness polls the app/nginx readiness)
so tests only run after both nginx and app are truly ready.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac52fe0f-ffde-4610-94bf-cfa992545c62
📒 Files selected for processing (12)
code/backend/Cleanuparr.Api.Tests/Middleware/TrustedForwardedHeadersMiddlewareTests.cscode/backend/Cleanuparr.Api/Auth/TrustedNetworkAuthenticationHandler.cscode/backend/Cleanuparr.Api/DependencyInjection/ApiDI.cscode/backend/Cleanuparr.Api/Extensions/HttpRequestExtensions.cscode/backend/Cleanuparr.Api/Features/Auth/Controllers/AuthController.cscode/backend/Cleanuparr.Api/Middleware/TrustedForwardedHeadersMiddleware.csdocs/docs/configuration/general/index.mdxe2e/docker-compose.e2e.ymle2e/nginx/nginx.confe2e/tests/13-trusted-network-xff-bug.spec.tse2e/tests/helpers/app-api.tse2e/tests/helpers/test-config.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-8q44-v65j-jc3q