fix(web): reject a trusted-proxy ForwardLimit below 1 at startup - #1386
Open
marcelo-maciel wants to merge 15 commits into
Open
marcelo-maciel wants to merge 15 commits into
marcelo-maciel wants to merge 15 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
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.
…advisories `dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on `main` and on every open PR alike. Advisory-database drift, not a regression from any change: a commit green on 2026-08-10 is red today with no edits. - `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903, GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already depends on the patched 2026.0.0, so the advisory clears with no transitive pin to remember to remove later. Same fix as fullstackhero#1369, so the two do not conflict. - `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902, GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the 8.x line has no patched release, so a transitive pin cannot fix it; the package itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401, past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is excluded from the template, so the scaffold never sees it. Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and `dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings and 0 errors.
MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:
pull access denied for minio/minio, repository does not exist or may
require 'docker login'
That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:
- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README
The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.
While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).
Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.
…ng to it AddHeroPlatform only added to KnownProxies/KnownIPNetworks, which assumes whatever is already there is the framework's loopback default. Under ASPNETCORE_FORWARDEDHEADERS_ENABLED=true, ConfigureWebDefaults registers ForwardedHeadersOptionsSetup, which empties both lists. An empty list is not "trust nobody" in ForwardedHeadersMiddleware: it only validates the peer when at least one entry exists, so the app rewrote RemoteIpAddress from an X-Forwarded-For sent by any caller, forging the rate-limit partition and the audit IP. Clear both lists unconditionally, then either restate the loopback default or apply the configured proxies/networks. The new test builds through WebApplication.CreateBuilder with the flag set, asserts ForwardedHeadersOptionsSetup is actually registered so it cannot pass vacuously, and checks the resolved lists equal a fresh ForwardedHeadersOptions.
Contributor
Author
|
Note for whoever merges this: this branch also carries the forwarded-headers trust-list fix from #1379, content-identical, because both change the same block in Verified by content rather than by SHA: the added and removed lines of both commits are identical, differing only in blob index and hunk offset (the |
This was referenced Sep 17, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers 404), and it is what `minio-init` runs: without it `dotnet run --project src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull, and the `fsh` bucket is never created, so the first upload fails with NoSuchBucket. Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical to it and can be dropped once that lands.
The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not help", but the branch also bumps Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two statements cannot both be true, and the bump is the one that is: with the pin removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903 and exits 0. It was carrying a transitive pin that no longer pins anything. The MessagePack pin above it stays: that one is still load-bearing (removing it brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).
The option documented the failure of setting it too low and the two invalid values, but not the one an operator is most likely to reach for: rounding it up "to be safe". The middleware trusts one entry per hop counting from the right and only ever checks the peer, so a limit of 2 behind a single proxy hands the caller its own RemoteIpAddress, and every IP-based rate limit and audit entry follows it.
… one `GetNewestSessionIpAsync` ordered `UserSessions` by `CreatedAt` and took the first row from the whole table. It is safe only because the collection runs serially; any other test in it issuing a token leaves the assertion reading a row this request did not create. Ordering is not what makes it correct either: `CreatedAt` comes from a single `TimeProvider.System` read and two issues can land on the same tick, and `Id` is a random `Guid`, so a tiebreak on it picks deterministically but not necessarily correctly. Snapshot the session ids before the request and take the one that was not there. `ShouldHaveSingleItem` asserts the correlation instead of assuming it. Also states what the factory's `PostConfigure<ForwardedHeadersOptions>` leaves these tests covering. It overwrites the flags, the forward limit and both trust lists wholesale, so the `TrustedProxyOptions` binding is not what runs here - the middleware and the placement of `UseForwardedHeaders` are. The binding has its own gate in `Framework.Tests/Web/TrustedProxyOptionsBindingTests`, and the comment now says so rather than reading as if this pinned production. Verified: `dotnet test --filter FullyQualifiedName~ForwardedHeadersIpTests` passes 2/2; inverting the new filter to `before.Contains(s.Id)` fails 2/2.
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).Framework.Testsis 140/140 green with everything 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 #1379, which introduces
TrustedProxyOptions: the branch is cut fromfix/web-forwarded-headers, so the diff againstmainshows that PR's commits until it merges. (#1334 was the closed original; #1379 is the live one, reopened after the fork was deleted.) Raised separately so a follow-up doesn't widen an approved security PR. Merge #1379 first: this PR then collapses to its own delta, and merging it first instead leaves #1379 with nothing to add. Measured rather than described, at the current heads (d7fb51afand00ec8e8a):git diff d7fb51af 00ec8e8a --statis 3 files, 43 insertions, 0 deletions, all of itForwardLimit- the option, its binding inExtensions.cs, andTrustedProxyOptionsBindingTests.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.Infra carve-outs, corrected after review. Two things in the out-of-topic hunks were wrong and
are fixed on the branch:
minio/minioto quay.io.minio/mcis gone from Docker Hub too(
hub.docker.com/v2/repositories/minio/mc/answers 404) and it is whatminio-initruns, so bothdotnet run --project src/Host/FSH.Starter.AppHostanddocker compose updied on the pull and thefshbucket was never created. Now pinned to the same quay tag #1388 uses.SSH.NETpin is gone: it pinned nothing. Its own comment claimed bumping Testcontainersdoes not help, but 4.14.0 — which this branch also carries — declares
SSH.NET >= 2026.0.0.Measured rather than argued: with the pin removed,
dotnet restore src/FSH.Starter.slnx --forcereports zero NU1902/NU1903 and exits 0. (The MessagePack pin next to it stays; removing that one
does bring its advisory straight back.)
With both applied,
deploy/docker/docker-compose.ymlandsrc/Directory.Packages.propsare nowgenuinely byte-identical to #1388 (
git diff --exit-code, checked today), which the earlier claimwas not.
One more from review: the doc covered the wrong mistake. It explained what setting
ForwardLimittoo low costs and rejects the two invalid values, but said nothing about the direction an operator
actually drifts in, rounding it up "to be safe". The middleware trusts one entry per hop counting
from the right and only ever checks the immediate peer, so a limit of 2 behind a single proxy
discards what the proxy appended in favour of what the client sent: the caller picks its own
RemoteIpAddress, and every IP-based rate limit and audit entry follows it. That is now on theoption itself, where someone changing the number will read it.
Carried from #1379, so the two do not drift.
ForwardedHeadersIpTestsread back "the newestsession" - it ordered
UserSessionsbyCreatedAtand took the first row of the whole table,which is safe only because the collection runs serially. It now snapshots the session ids before
the request and takes the one that was not there, with
ShouldHaveSingleItemasserting thecorrelation instead of assuming it. The factory comment that read as if the test pinned the
TrustedProxyOptionsbinding says what it actually covers. Same change, cherry-picked from that branch andbyte-identical here:
git diff d7fb51af 00ec8e8a --statlists only the threeForwardLimitfiles, so whichever merges second drops it as an empty cherry-pick. 2/2 green here; inverting the new filter
to
before.Contains(s.Id)fails 2/2.