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

[marcelo-maciel]

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 useless — RequestContextService.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 ForwardedHeadersOptions — X-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 framework default (loopback only) stands. 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 TrustedProxyOptions → ForwardedHeadersOptions 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.

Full suite green locally: 1773 passed / 1 skipped / 0 failed (Integration.Tests 735/1, Architecture.Tests 51).

⚠️ 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 SSH.NET pin is carried from #1333

NU1903 / GHSA-q939-rpr3-3284 on SSH.NET 2025.1.0, which arrives transitively via Testcontainers (4.11.0 and 4.13.0 both pin 2025.1.0), fails restore for the whole solution under TreatWarningsAsErrors. It is not caused by this PR: building unmodified origin/main fails identically, re-verified today at 3f2959e6 (exit 1, NU1903 from Integration.Tests and Integration.Middleware.Tests). The advisory was published after these branches were last built, which is why previously-green PRs went red with no code change.

The fix properly belongs to #1333, which is still open. Rather than leave an approved PR red on someone else's advisory, the pin is carried here byte-identical to #1333's version of the file, comment included — the blob hashes match. That keeps both mergeable in either order, and once #1333 lands this copy can simply be dropped. If the pin changes during that PR's review, this copy should be matched rather than allowed to drift: the same pin under a reworded comment conflicts.

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:

• ForwardLimit validation — it's a plain int where the framework's own option is int?. A negative value throws OverflowException inside ForwardedHeadersMiddleware on every request (500s across the board, since UseForwardedHeaders runs after UseExceptionHandler), and 0 silently disables processing. Measured against the real middleware; filed separately as TrustedProxyOptions.ForwardLimit is an unvalidated int: a negative value 500s every request, zero silently disables forwarded headers #1358 rather than widening this PR.
• X-Forwarded-Host — intentionally excluded from the flags list; the doc-comment note about why is the follow-up you called out in review.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[iammukeshm]

Thanks for tackling the real-client-IP problem — the middleware placement is correct (after UseExceptionHandler, before response compression / CORS / HTTPS redirect / auth / rate-limit). Two blockers before this can land, though, both security:

🔴 HIGH — BuildingBlocks/Web/Extensions.cs: clearing the known-proxy allow-list reopens an IP-spoofing hole. KnownIPNetworks.Clear() + KnownProxies.Clear() makes ForwardedHeaders trust X-Forwarded-For from any source. If the app is ever reachable outside the proxy network, a client can forge its own IP — poisoning audit/session IPs and evading the IP-partitioned rate limiter. That's the same class of hole this PR is meant to close, just moved down a layer. Please configure the trusted ingress CIDR(s) in KnownIPNetworks instead of clearing them, and bind it from configuration so prod can lock it down without a code change.

🔴 HIGH — ForwardLimit left at the default (1). With the 2-hop ingress described in the PR (cloudflared → Caddy → app), reading only the rightmost hop yields Caddy's IP (bug unfixed) or an attacker-injected value. Set ForwardLimit to the real hop count.

⚠️ Also — this modifies protected src/BuildingBlocks (Golden Rule #4). Needs explicit maintainer sign-off; please call it out in the description.

nit — add a negative test proving an untrusted source's X-Forwarded-For is ignored once known-proxies are set; right now only the happy path is covered, so the security boundary is untested.

[marcelo-maciel]

Thanks for the careful review — all four addressed in 75475d3.

🔴 known-proxy allow-list no longer cleared. Added TrustedProxyOptions (config section of the same name: KnownProxies, KnownNetworks CIDRs, ForwardLimit) bound from configuration. ForwardedHeaders now trusts only the configured ingress proxies/networks; a forged X-Forwarded-For from any other source is ignored. When nothing is configured, the framework default (loopback only) stands, so an app reachable outside the proxy network can't be IP-spoofed — same failure closed, not moved down a layer. Prod locks the ingress CIDR(s) down via config, no code change.

🔴 ForwardLimit now configurable. TrustedProxyOptions.ForwardLimit feeds ForwardedHeadersOptions.ForwardLimit; operators set it to the real hop count (cloudflared → Caddy → app = 2). Empty config keeps the framework default of 1.

⚠️ BuildingBlocks / Golden Rule #4. Called out explicitly in the PR description now — this touches Web/Extensions.cs + adds Web/TrustedProxy/TrustedProxyOptions.cs. Requesting your sign-off; the change is scoped to forwarded-headers registration and doesn't alter any other building block.

nit — negative test added. ForwardedHeadersIpTests now covers both sides: X-Forwarded-For from the trusted proxy surfaces the real client IP, and the same header from an untrusted source is ignored (persisted IP = connection IP, never the forwarded value). TestServer has no socket, so the connection IP is stamped via a test-only startup filter. Full Integration.Tests green (735 passed / 1 skipped), Architecture.Tests green (51).

[iammukeshm]

Approving. All four points from my last review are properly addressed, and the negative test is the one that makes this trustworthy.

Specifically:

• KnownProxies.Clear() / KnownIPNetworks.Clear() now run only inside the branch where configured values replace them, and the early-return leaves the framework loopback default intact. Secure-by-default holds: no config means no trust, not universal trust.
• ForwardLimit is config-bound with the multi-hop rationale documented on the property.
• ForwardedHeadersIpTests proves the boundary in both directions. TestRemoteIpStartupFilter is a legitimate solution to a real constraint (TestServer has no socket, so Connection.RemoteIpAddress is null and the trust check is otherwise untestable) — running it as an IStartupFilter so it lands ahead of the app pipeline is the correct placement, and it's inert without the header. Using RFC 5737 documentation ranges for the fixtures is a nice touch.
• BuildingBlocks is called out in the description.

Middleware placement is right: after UseExceptionHandler, ahead of response compression / CORS / HTTPS redirect / auth / rate limiting, so everything downstream sees the real client.

nit (non-blocking)

IPAddress.Parse and IPNetwork.Parse inside the Configure lambda mean a typo'd CIDR surfaces as a bare FormatException at first options resolution, with nothing in the message naming TrustedProxyOptions. Given this config is edited exactly once per deployment, under time pressure, by someone wiring up an ingress — a TryParse with a message naming the offending entry and setting would pay for itself. Not blocking.

note for the follow-up, not this PR

X-Forwarded-Host is deliberately not in the flags list. That's the right default — honouring it without an allow-list is a host-header-injection primitive — but it does mean anything deriving a public URL from the request host still sees the internal host behind the proxy. That interacts directly with #1323's origin work; worth a line in the TrustedProxyOptions doc comment saying host is intentionally excluded and why, so the next person doesn't "fix" it by adding the flag.

BuildingBlocks sign-off (Golden Rule #4)

Granted. The change is confined to forwarded-headers registration plus the new options type, and alters no existing building-block behaviour.

[iammukeshm]

Holding the merge on one dependency, not on the code.

Golden Rule #10 — the docs change has to travel with the code. fullstackhero/docs#237 is still open and currently conflicting, so merging this now would ship a new required-ish config surface (TrustedProxyOptions, plus the production-checklist note that honouring X-Forwarded-Proto requires configuring the trusted ingress) with no documentation behind it. For a security-relevant setting where the failure mode of misconfiguration is silent — trusting nobody and quietly keeping the proxy IP — that's the one case where I don't want to defer the docs.

Rebase docs#237 and I'll merge both together. Nothing else outstanding here; the approval stands and this branch is CLEAN against main.

[marcelo-maciel]

Docs dependency cleared — fullstackhero/docs#237 is rebased and now MERGEABLE / CLEAN at 9dd7a5bc.

The conflict wasn't substantive: a96478cf style: replace all em dashes with hyphens across the site landed on docs main after this branch was cut, so main carries zero em dashes while my three files still had eight. Resolution keeps the same prose with - in place of —, matching the site convention. The changelog bullet stays under the existing ## 2026-07-13 heading alongside the branding entry; content is unchanged from what you reviewed (+45 / -9, same three files).

Verification note for that repo: it has no CI workflows, so there's no check rollup to read. npx astro check is green (0 errors, 0 warnings, 68 files). npm run build fails on both the branch and unmodified main with CannotFetchFontFile (a 404 fetching JetBrains Mono from fonts.gstatic.com inside buildEnd), so it's environmental and not a usable gate here.

No new commit on this branch

This PR is untouched — still 75475d30, still the 16 green checks you approved. I deliberately did not push the TryParse nit, for a reason worth flagging separately below.

Heads-up: NU1903 is currently breaking main, not just this PR

Building origin/main at 3f2959e6 today fails restore with TreatWarningsAsErrors:

error NU1903: Package 'SSH.NET' 2025.1.0 has a known high severity vulnerability
  https://github.com/advisories/GHSA-q939-rpr3-3284

SSH.NET arrives transitively via Testcontainers (both 4.11.0 and 4.13.0 pin 2025.1.0, so bumping Testcontainers doesn't help), and the advisory was published after these branches were last built — which is why previously-green PRs go red without a code change. The one-line pin to the patched 2026.0.0 is already sitting in #1333, which is green across all 12 checks. Once that merges, this branch rebases clean and stays green; pinning it here instead would drag repo-wide dependency scope into a security PR.

(The other advisory I hit locally, System.Security.Cryptography.Xml 10.0.8, is already fixed on main by the 10.0.10 pin — it only shows up on this branch because the branch predates it, and it disappears on rebase.)

Held for the follow-up, deliberately

• Your TryParse nit. Implemented and verified locally, not pushed: it's a 14-line change plus a test that pins the TrustedProxyOptions → ForwardedHeadersOptions binding, but pushing it today would replace a clean green sha with a red one for the SSH.NET reason above, on a PR you're already holding. It goes in right after fix(web): correct idempotent replay payload and serialize concurrent duplicates #1333 lands, or in the follow-up — your call which.
• The X-Forwarded-Host doc-comment note, per your own "note for the follow-up, not this PR".

Ready to merge on your side whenever the docs PR suits you.

[marcelo-maciel]

Pushed 85aa03b3. Two things in it, plus a heads-up you'll want before you look at the red CI.

What changed since 75475d30 (i.e. since your approval — unreviewed)

Your TryParse nit. IPAddress.Parse / IPNetwork.Parse inside the Configure lambda became TryParse + an InvalidOperationException naming the config path and the offending value:

TrustedProxyOptions:KnownNetworks contains "10.0.0.0/999", which is not a valid CIDR network (for example "10.0.0.0/8").

Two test gaps the nit exposed, both closed.

TrustedProxyOptionsBindingTests (new, Framework.Tests) pins the TrustedProxyOptions → ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only default when the section is absent, KnownProxies + ForwardLimit binding, and both malformed-entry messages. Worth being explicit that this was a real gap, not a hypothetical — renaming the config section previously broke nothing any test could observe, because FshWebApplicationFactory PostConfigures ForwardedHeadersOptions wholesale and clobbers whatever Configure did. The builder runs with DisableDefaults, otherwise an ambient TrustedProxyOptions__* env var on the runner silently changes what "nothing configured" resolves to (I hit exactly that while writing it).

ForwardedHeadersIpTests — the untrusted-source test was passing vacuously. It asserted only that the connection IP was persisted, which stays true when forwarded-header processing is absent entirely; I verified it stays green with app.UseForwardedHeaders() deleted outright. It now also sends the identical header from the trusted proxy and asserts that arm is honored, so the trust boundary is what it pins. That one dies as it should when the middleware call is removed.

Full suite green locally: 1773 passed / 1 skipped / 0 failed.

The red CI is not this PR

Backend CI on 85aa03b3 fails, and every failing job fails for exactly one reason — I checked the distinct error codes across all of them:

error NU1903: Warning As Error: Package 'SSH.NET' 2025.1.0 has a known high severity vulnerability

Nothing in the logs mentions this PR's code; restore never gets far enough to compile or run a test (No files were found with the provided path: **/*.trx). Building unmodified origin/main fails identically. The pin to the patched 2026.0.0 is in #1333. Frontend CI and DbMigrator Container Smoke are green here, which is consistent — they don't restore the affected projects.

I chose not to pin it in this PR: it's a repo-wide dependency decision and it doesn't belong inside a security change. Rebasing after #1333 merges should turn this green with no further edits.

Filed separately rather than widening this PR

#1358 — TrustedProxyOptions.ForwardLimit is a plain int where the framework's own option is int? (with null = unlimited), and it's unvalidated. Measured against the real ForwardedHeadersMiddleware: a negative value throws OverflowException on every request, including ones carrying no forwarded headers at all, and since UseForwardedHeaders runs after UseExceptionHandler that's a 500 across the board rather than a startup failure. 0 silently disables processing. -1 is a plausible thing to write reaching for "unlimited", precisely because the framework supports that via null. Not a regression from this PR — a gap in the config surface it introduces. Happy to send the fix wherever you'd rather have it.

The X-Forwarded-Host doc-comment note is still parked for the follow-up, per your review.

Docs #237 is rebased and MERGEABLE at 6d48b3cc, now including a line about the named error for a malformed entry so the docs match what actually ships.

[marcelo-maciel]

Pushed e7dbe6bc: one commit, carrying the SSH.NET pin so this stops being red on someone else's advisory.

That is the only change since your approval — no code, no tests, no config touched. src/Directory.Packages.props is now byte-identical to #1333's version of the file, comment included (same blob), so the two stay mergeable in either order and this copy can be dropped once #1333 lands. The description section on the advisory has been updated to match.

For the record, main itself is still red on this: dotnet restore src/FSH.Starter.slnx at 3f2959e6 exits 1 with NU1903 from Integration.Tests and Integration.Middleware.Tests.

Verified locally on the pushed tree, with the audit on rather than disabled:

• dotnet restore: exit 0, zero NU1903 in the log.
• dotnet build: exit 0.
• Full suite: 14 assemblies, 1806 passed / 0 failed / 1 skipped, including Integration at 748 passed / 1 skipped against a real Postgres.

One caveat stated rather than glossed: the first suite run had TenantThemeTests.UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA fail with a 401 where it expects 204. It is a pre-existing intermittent unrelated to this PR — the class passes 12/12 in isolation and the full Integration assembly passes 748/0 on re-run under the same load. Recording it rather than quietly re-running until green.

[marcelo-maciel]

Reopened as #1379.

This PR was closed automatically on 2026-09-14, when the head fork was deleted.
The branch and the commits are unchanged; the head commit is still e7dbe6bc452f31c1a249d2ed351a32ae2f1abe2a.