fix(web): reject a trusted-proxy ForwardLimit below 1 at startup - #1365
Open
marcelo-maciel wants to merge 8 commits into
Open
fix(web): reject a trusted-proxy ForwardLimit below 1 at startup#1365marcelo-maciel wants to merge 8 commits into
marcelo-maciel wants to merge 8 commits into
Conversation
UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy (Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy container IP. That collapsed the rate-limit partitions into a single install-wide bucket (one anonymous spike throttles every tenant's login) and recorded a useless proxy IP on audit trails and user sessions. Register ForwardedHeadersOptions (X-Forwarded-For + X-Forwarded-Proto, known networks/proxies cleared to trust the immediate upstream) and call UseForwardedHeaders first in the pipeline, before HTTPS redirect / rate limiting / auth / audit read the client. Lock the trusted set down via ForwardedHeadersOptions when the ingress topology is fixed.
Address review on fullstackhero#1334. Instead of clearing the known-proxy allow-list (which trusts X-Forwarded-* from any source and reopens the IP-spoofing hole this PR is meant to close), trust only the ingress proxies/networks bound from the new TrustedProxyOptions, and honor a configurable ForwardLimit for the real multi-hop ingress. With nothing configured the framework default (loopback only) stands, so a client reaching the app directly can't forge its IP/scheme. Add a negative test proving an untrusted source's X-Forwarded-For is ignored, alongside the trusted-proxy happy path. TestServer has no socket, so the connection IP is stamped via a test-only startup filter.
…formed A typo'd entry in TrustedProxyOptions surfaced as a bare FormatException from IPAddress.Parse / IPNetwork.Parse, with nothing in the message pointing at the setting that caused it. For config an operator edits once per deployment, under time pressure, while wiring up an ingress, that is the wrong failure mode: the silent version of it leaves the app trusting nobody while looking configured. Both parses now use TryParse and throw an InvalidOperationException naming the config path and the offending value. Also closes two gaps the change exposed: - TrustedProxyOptionsBindingTests pins the TrustedProxyOptions -> ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only default when the section is absent, KnownProxies + ForwardLimit binding, and both malformed-entry messages. Before this, renaming the config section broke nothing that any test could see. The host builder runs with DisableDefaults so an ambient TrustedProxyOptions__* on the machine cannot change what "nothing configured" resolves to. - The untrusted-source integration test asserted only that the connection IP was persisted, which stays true when forwarded-header processing is absent entirely, so it passed with app.UseForwardedHeaders() removed. It now sends the identical header from the trusted proxy as well and asserts that arm is honored, so the trust boundary is what the test actually pins.
…1333 is open `NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by Testcontainers, fails `restore` for the whole solution under `TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix belongs to fullstackhero#1333, which is still open. Carried byte-identical to fullstackhero#1333's version of the file, comment included, so both stay mergeable in either order and this copy can simply be dropped once fullstackhero#1333 lands.
TrustedProxyOptions.ForwardLimit was passed straight to ForwardedHeadersOptions with no validation, and neither bad value announces itself. Zero truncates the unwind loop in ApplyForwarders to zero iterations, so X-Forwarded-* stop being processed with no error and no log while the config still reads as configured. A negative value makes the middleware allocate a negative-length buffer, which throws OverflowException on every request - including requests carrying no forwarded headers at all - and UseForwardedHeaders sits after UseExceptionHandler, so that surfaces as a plain 500 rather than a boot failure a smoke test catches. Reject anything below 1 where the malformed KnownProxies/KnownNetworks entries are already rejected, naming the setting and the offending value. The throw lands during startup, so a bad hop count fails the deploy instead of the traffic. Closes fullstackhero#1358
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review note from fullstackhero#1334, left for the follow-up: the flag list carries only X-Forwarded-For and X-Forwarded-Proto, and the omission is deliberate. Rewriting Request.Host from a header is a host-header injection primitive, and the three Identity endpoints that build a public URL from the request would then mail confirmation links pointing wherever the header said. The consequence an operator has to know is that Request.Host keeps the internal host behind a proxy, and those links carry it.
marcelo-maciel
force-pushed
the
fix/trusted-proxy-forward-limit
branch
from
August 17, 2026 09:11
972c5f3 to
cb62b10
Compare
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
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.
Closes #1358.
TrustedProxyOptions.ForwardLimitwas passed straight through toForwardedHeadersOptionswith no validation, so two values that nobody means are accepted in silence.What the two bad values actually do
Measured on .NET 10 by starting a real host with
UseExceptionHandler+UseForwardedHeadersin the same orderUseHeroPlatformmounts them, and issuing one request:ForwardLimit = -1ForwardLimit = 0X-Forwarded-Forfrom a trusted proxyRemoteIpAddressand scheme unchangedNegative is the sharp edge. In
ApplyForwarders,entryCountstarts at0, and there is no early return when no forwarded headers are present.0 > -1is true, soentryCountis clamped to-1andnew SetOfForwarders[entryCount]throwsOverflowException. BecauseUseForwardedHeaders()sits afterUseExceptionHandler(), that is a 500 on 100% of traffic rather than a startup failure a deploy smoke test would catch.-1is a plausible thing for an operator to write when reaching for "unlimited hops", precisely because the underlying framework option expresses exactly that vianull.Zero is the quiet one.
entryCount > 0truncates to0, the consume loop runs zero iterations, and forwarded headers stop being processed with no error and no log, reinstating the original symptom (proxy IP in rate-limit partitions and audit trails) while the config still reads as configured.The fix
Reject anything below
1where the malformedKnownProxies/KnownNetworksentries are already rejected, with a message naming the setting and the offending value. Verified by execution that a throw from inside thatConfigure<ForwardedHeadersOptions>delegate surfaces during startup (StartAsyncfails, the host never serves), so a bad hop count fails the deploy instead of the traffic.Scope
Extensions.cs: the guard, at the top of the delegate so it also covers the case where no proxies or networks are configured.TrustedProxyOptions.cs: the XML doc records the minimum.TrustedProxyOptionsBindingTests.cs: a theory over-1and0, mirroring the two existing "name the setting" tests. Both go red with the guard reverted (checked), andFramework.Testsis 139/139 green with it in place.A second commit answers the other note you left for the follow-up on this same type: the class doc now records that only
X-Forwarded-ForandX-Forwarded-Protoare honoured, that leavingX-Forwarded-Hostout is deliberate (rewritingRequest.Hostfrom a header is a host-header injection primitive, and the three Identity endpoints that build a public URL from the request would mail links pointing wherever the header said), and that the trade-off isRequest.Hostkeeping the internal host behind a proxy. Doc comment only, no behaviour change; docs#237 carries the same note.Nothing in
deploy/, the AppHost or eitherappsettingssets a value below1(both ship1), so this changes nothing for a correct deployment. It does turn an existing0into a hard startup failure, which is the intent.Two notes
This is stacked on #1334, which introduces
TrustedProxyOptions: the branch is cut fromfix/web-forwarded-headers, so the diff againstmainshows that PR's commits until it merges. Raised separately so a follow-up doesn't widen an approved security PR.This keeps
ForwardLimitanint, so "unlimited" stays inexpressible. The larger shape from #1358, making itint?to mirror the framework'snull-means-unlimited while still rejecting<= 0, changes the options contract and is still on the table if you prefer it. Say the word and I will send that instead.