Skip to content

fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline - #1379

Open
marcelo-maciel wants to merge 12 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/web-forwarded-headers
Open

marcelo-maciel wants to merge 12 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/web-forwarded-headers

Conversation

@marcelo-maciel

@marcelo-maciel marcelo-maciel commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Carries two infrastructure fixes that are not this PR's topic. Without them CI cannot even reach this PR's code.

  • Dependency bump: Microsoft.SourceLink.GitHub to 10.0.401 and Testcontainers to 4.14.0, clearing NU1902/NU1903 so restore succeeds. Those are the versions #1375 (SourceLink) and #1369 (Testcontainers) carry: this hunk is the union of the two, plus one comment per pin naming the advisory it answers.
  • MinIO (cfde65cd): minio/minio is gone from Docker Hub, so every Testcontainers-backed integration test dies on the image pull. Pulls from quay.io on a pinned tag instead. Same fix as #1388.

The MinIO hunk is byte-identical to #1388. The dependency hunk is not byte-identical to #1375 or #1369, which each carry half of it without the comments, but it is identical across all twelve PRs in this series: src/Directory.Packages.props resolves to the same blob (854deb95) at every head. Either way they merge in any order, and these copies can be dropped once the PRs that own them land.

Reopened from #1334. That PR was closed automatically on 2026-09-14, when the head fork
was deleted. It reopened at e7dbe6bc, and review has added commits on top since then (the
commit list above is the current one). The earlier review history stays on #1334.


Fixes audit finding API-02.

Problem

UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy (Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy's container IP. Consequences:

  • Rate limiting collapses — the anonymous auth policy and the global IP limiter (RateLimiting/Extensions.cs) partition by ip:{RemoteIpAddress}, so every request shares one bucket. One anonymous spike throttles every tenant's login; per-origin brute-force protection is gone.
  • Audit / session IPs are uselessRequestContextService.IpAddress (persisted on UserSession, audit trails) records the proxy IP for every request.

Fix

  • New TrustedProxyOptions (config section TrustedProxyOptions): KnownProxies (IPs), KnownNetworks (CIDRs) and ForwardLimit, bound from configuration.
  • Register ForwardedHeadersOptionsX-Forwarded-For + X-Forwarded-Proto. Trust is bound to the configured ingress proxies/networks; forwarded headers from any other source are ignored, so a client reaching the app directly cannot forge its IP/scheme. When nothing is configured, the loopback-only default is restated, not inherited: the trust list is cleared first and rebuilt every time, because a host started with ASPNETCORE_FORWARDEDHEADERS_ENABLED=true registers ForwardedHeadersOptionsSetup, which empties both lists before this runs. An empty list is not "trust nobody" in ForwardedHeadersMiddleware — it only validates the peer when at least one entry exists, so empty means the app would rewrite RemoteIpAddress from an X-Forwarded-For sent by any caller at all. ForwardLimit follows config so a multi-hop ingress (cloudflared → Caddy → app) unwinds the right number of hops.
  • Call app.UseForwardedHeaders() first in UseHeroPlatform, before HTTPS redirect / rate limiting / auth / audit read the client IP or scheme.
  • appsettings.json / appsettings.Production.json carry an empty TrustedProxyOptions section; prod sets the ingress CIDR(s) + hop count to activate real-client extraction (secure-by-default: no config ⇒ no trust).
  • A malformed KnownProxies / KnownNetworks entry now fails with an InvalidOperationException naming the config path and the offending value, instead of a bare FormatException.

Tests

ForwardedHeadersIpTests (integration):

  • happy path — a token-issue request arriving from the trusted proxy carrying X-Forwarded-For persists the real client IP on the UserSession.
  • negative — the same header from an untrusted source is ignored; the persisted IP is the connection IP, never the attacker-supplied forwarded value. The test also sends the identical header from the trusted proxy and asserts that arm is honored, so the trust boundary is what it pins rather than an outcome that would hold with forwarded-header processing absent entirely.

TestServer has no socket, so the connection IP is stamped via a test-only startup filter (X-Test-Remote-Ip).

TrustedProxyOptionsBindingTests (unit) pins the TrustedProxyOptionsForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only default when the section is absent, KnownProxies + ForwardLimit binding, and both malformed-entry messages. The host builder runs with DisableDefaults so an ambient TrustedProxyOptions__* on the machine or CI runner can't change what "nothing configured" resolves to.

ForwardedHeadersHostDefaultsTests (unit) covers the one host shape the binding tests cannot reach: it builds through WebApplication.CreateBuilder with FORWARDEDHEADERS_ENABLED set, asserts ForwardedHeadersOptionsSetup is actually registered so the test cannot pass vacuously, and then asserts the resolved lists equal a fresh ForwardedHeadersOptions. Red with the Clear() and the restatement reverted, green with them in place.

Full suite green locally: 1773 passed / 1 skipped / 0 failed (Integration.Tests 735/1, Architecture.Tests 51). Re-run after the trust-list fix: solution build under TreatWarningsAsErrors exit 0, Framework.Tests 138/138, Integration.Tests (Security filter) 9/9, Integration.Middleware.Tests 5/5.

⚠️ Touches protected src/BuildingBlocks (Golden Rule #4)

This modifies src/BuildingBlocks/Web/Extensions.cs and adds src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs — the shared framework wiring, so it needs maintainer sign-off. The change is confined to forwarded-headers registration + the new options type; no existing behavior of other building blocks is altered. Sign-off granted in review on 2026-08-08.

Changed after the last review

75475d30 was what was approved. 85aa03b3 adds, and has not been reviewed by anyone:

  • the TryParse + named-message change (the non-blocking nit from the approving review);
  • TrustedProxyOptionsBindingTests;
  • the extra assertion in the untrusted-source integration test.

No production behavior changes beyond the error message for malformed config.

The dependency bumps that restore needs

dotnet restore src/FSH.Starter.slnx fails on main under TreatWarningsAsErrors — not because of
this PR — so the branch carries the Testcontainers 4.14.0 and SourceLink bumps that clear it, in the
same shape as the PR that owns them.

It used to carry an explicit SSH.NET pin too, on the stated grounds that bumping Testcontainers
would not help. That was wrong: 4.14.0 declares SSH.NET >= 2026.0.0, and with the pin removed
dotnet restore --force reports zero NU1902/NU1903 and exits 0. The pin is gone.

Notes

Docs in fullstackhero/docs#237 (rebased, MERGEABLE): changelog entry + a new "Reverse proxy & forwarded headers" section (CORS & headers page) documenting TrustedProxyOptions, and a production-checklist note that honoring X-Forwarded-Proto requires configuring the trusted ingress.

Two things deliberately left out of this PR:


Infra carve-outs, corrected after review. Two things in the out-of-topic hunks were wrong and
are fixed on the branch:

  • The MinIO carve-out only moved minio/minio to quay.io. minio/mc is gone from Docker Hub too
    (hub.docker.com/v2/repositories/minio/mc/ answers 404) and it is what minio-init runs, so both
    dotnet run --project src/Host/FSH.Starter.AppHost and docker compose up died on the pull and the
    fsh bucket was never created. Now pinned to the same quay tag #1388 uses.
  • The SSH.NET pin is gone: it pinned nothing. Its own comment claimed bumping Testcontainers
    does 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 --force
    reports 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.yml and src/Directory.Packages.props are now
genuinely byte-identical to #1388 (git diff --exit-code, checked today), which the earlier claim
was not.


Two corrections in the tests themselves.

ForwardedHeadersIpTests read back "the newest session" - it ordered UserSessions by
CreatedAt and took the first row of the whole table. That 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. It now snapshots the
session ids before the request and takes the one that was not there, with ShouldHaveSingleItem
asserting the correlation instead of assuming it. 2/2 green; inverting the new filter to
before.Contains(s.Id) fails 2/2.

The factory comment overstated what these tests cover. Its
PostConfigure<ForwardedHeadersOptions> overwrites the flags, the forward limit and both trust
lists wholesale, so what runs against TestConstants.TrustedProxyIp is the real middleware and
the real placement of UseForwardedHeaders, not the TrustedProxyOptions binding that feeds them
in production. That binding has its own gate in
Framework.Tests/Web/TrustedProxyOptionsBindingTests, and the comment says so now rather than
reading as if this pinned production.

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.
…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.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Note for whoever merges this: the same forwarded-headers trust-list fix is carried, content-identical, on #1386. The two PRs touch the same block in AddHeroPlatform for different reasons (this one rebuilds the trust list, #1386 rejects a ForwardLimit below 1), so the commit is duplicated deliberately. Whichever merges first makes the other's copy drop as an empty cherry-pick; no manual reconciliation needed.

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 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).
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Pairing note, for whoever picks these up. #1386 is cut from this branch and is the only PR that depends on it. Measured rather than described: git diff a24f8d84 99e55145 --stat is 3 files, 35 insertions, 0 deletions, all of it ForwardLimit (the option, its binding in Extensions.cs, and TrustedProxyOptionsBindingTests).

Merge this one first and that PR collapses to exactly those 35 lines. Merge that one first and this one has nothing left to add. Either order works mechanically: both carry the same two infra carve-outs as identical hunks, so whichever lands second drops them as an empty cherry-pick.

… 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.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Pairing note, re-measured. The numbers in my earlier comment went stale: both branches have
moved since (ForwardLimit doc copy on #1386, and the session-read fix above, which is cherry-picked onto
that branch so the two stay content-identical where they overlap).

At the current heads, git diff d7fb51af 00ec8e8a --stat is 3 files, 43 insertions, 0 deletions,
all of it ForwardLimit: the option, its binding in Extensions.cs, and
TrustedProxyOptionsBindingTests.

The recommendation is unchanged. Merge this one first and that PR collapses to exactly those 43
lines. Merge that one first and this one has nothing left to add. Either order works mechanically:
everything they share is carried as identical hunks, so whichever lands second drops them as an
empty cherry-pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant