From 046b17b26c933bafc045353827edaa2eeb514386 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 17:45:10 -0300 Subject: [PATCH 1/2] fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline 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. --- src/BuildingBlocks/Web/Extensions.cs | 19 +++++ .../Tests/Security/ForwardedHeadersIpTests.cs | 69 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 50c6568fda..770e8c49e6 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -22,6 +22,7 @@ using FSH.Framework.Web.Security; using FSH.Framework.Web.Versioning; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.ResponseCompression; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.Configuration; @@ -63,6 +64,19 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild } builder.Services.AddHttpContextAccessor(); + + // The app runs behind a reverse proxy (Caddy / cloudflared), so the real client IP and scheme + // arrive via X-Forwarded-*. Without this, RemoteIpAddress is the proxy's container IP, which + // collapses the rate-limit partition into one bucket and records useless audit IPs. Known + // networks/proxies are cleared to trust the immediate upstream; lock them down via config + // when the ingress topology is fixed. + builder.Services.Configure(forwarded => + { + forwarded.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; + forwarded.KnownIPNetworks.Clear(); + forwarded.KnownProxies.Clear(); + }); + builder.Services.AddHeroDatabaseOptions(builder.Configuration); builder.Services.AddHeroRateLimiting(builder.Configuration); @@ -150,6 +164,11 @@ public static WebApplication UseHeroPlatform(this WebApplication app, Action +/// Runtime repro for audit finding API-02 (no UseForwardedHeaders → proxy IP collapses the real +/// client IP). Token issuance persists a UserSession whose IpAddress comes from +/// RequestContextService.IpAddress => Connection.RemoteIpAddress. With a trusted-proxy +/// forwarded-headers config, a request carrying X-Forwarded-For should surface the real client IP; +/// because UseHeroPlatform never calls UseForwardedHeaders, the header is ignored. +/// +[Collection(FshCollectionDefinition.Name)] +public sealed class ForwardedHeadersIpTests +{ + private const string ForwardedIp = "203.0.113.7"; + + private readonly FshWebApplicationFactory _factory; + + public ForwardedHeadersIpTests(FshWebApplicationFactory factory) + { + _factory = factory; + } + + [Fact] + public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestCarriesXForwardedFor() + { + using var client = _factory.CreateClient(); + using var request = new HttpRequestMessage(HttpMethod.Post, $"{TestConstants.IdentityBasePath}/token/issue"); + request.Headers.Add("tenant", TestConstants.RootTenantId); + request.Headers.Add("X-Forwarded-For", ForwardedIp); + request.Content = JsonContent.Create(new + { + email = TestConstants.RootAdminEmail, + password = TestConstants.DefaultPassword, + }); + + using var response = await client.SendAsync(request); + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var recordedIp = await GetNewestSessionIpAsync(); + + recordedIp.ShouldBe( + ForwardedIp, + "behind a trusted proxy the persisted session IP should be the real client IP from " + + "X-Forwarded-For; without UseForwardedHeaders the app records the connection/loopback IP instead."); + } + + private async Task GetNewestSessionIpAsync() + { + using var scope = _factory.Services.CreateScope(); + + var tenantStore = scope.ServiceProvider.GetRequiredService>(); + var tenant = await tenantStore.GetAsync(TestConstants.RootTenantId); + scope.ServiceProvider.GetRequiredService().MultiTenantContext = + new MultiTenantContext(tenant); + + var db = scope.ServiceProvider.GetRequiredService(); + var session = await db.UserSessions + .AsNoTracking() + .OrderByDescending(s => s.CreatedAt) + .FirstOrDefaultAsync(); + + return session?.IpAddress; + } +} From 75475d308e921045eb0ed521fbab449e14b7f44a Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:42:22 -0300 Subject: [PATCH 2/2] fix(web): bind forwarded-headers trust to configured proxies Address review on #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. --- src/BuildingBlocks/Web/Extensions.cs | 34 ++++++++++--- .../Web/TrustedProxy/TrustedProxyOptions.cs | 25 +++++++++ .../appsettings.Production.json | 5 ++ src/Host/FSH.Starter.Api/appsettings.json | 5 ++ .../FshWebApplicationFactory.cs | 16 ++++++ .../Infrastructure/TestConstants.cs | 4 ++ .../TestRemoteIpStartupFilter.cs | 35 +++++++++++++ .../Tests/Security/ForwardedHeadersIpTests.cs | 51 ++++++++++++++----- 8 files changed, 156 insertions(+), 19 deletions(-) create mode 100644 src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs create mode 100644 src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 770e8c49e6..5ee1dfe320 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -20,6 +20,7 @@ using FSH.Framework.Web.RateLimiting; using FSH.Framework.Web.Realtime; using FSH.Framework.Web.Security; +using FSH.Framework.Web.TrustedProxy; using FSH.Framework.Web.Versioning; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.HttpOverrides; @@ -30,6 +31,7 @@ using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Hosting; using Mediator; +using System.Net; namespace FSH.Framework.Web; @@ -65,16 +67,36 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild builder.Services.AddHttpContextAccessor(); - // The app runs behind a reverse proxy (Caddy / cloudflared), so the real client IP and scheme - // arrive via X-Forwarded-*. Without this, RemoteIpAddress is the proxy's container IP, which - // collapses the rate-limit partition into one bucket and records useless audit IPs. Known - // networks/proxies are cleared to trust the immediate upstream; lock them down via config - // when the ingress topology is fixed. + // The app runs behind a reverse proxy (e.g. cloudflared → Caddy → app), so the real client IP + // and scheme arrive via X-Forwarded-*. Without this, RemoteIpAddress is the proxy's container + // IP, which collapses the rate-limit partition into one bucket and records useless audit IPs. + // Trust is bound to the configured ingress CIDRs/proxies (see TrustedProxyOptions): forwarded + // headers from any other source are ignored, so a client reaching the app directly cannot forge + // its IP/scheme. With nothing configured, the framework default (loopback only) stands. + var trustedProxy = builder.Configuration + .GetSection(nameof(TrustedProxyOptions)).Get() ?? new TrustedProxyOptions(); builder.Services.Configure(forwarded => { forwarded.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; - forwarded.KnownIPNetworks.Clear(); + forwarded.ForwardLimit = trustedProxy.ForwardLimit; + + if (trustedProxy.KnownProxies.Length == 0 && trustedProxy.KnownNetworks.Length == 0) + { + return; + } + forwarded.KnownProxies.Clear(); + forwarded.KnownIPNetworks.Clear(); + + foreach (var proxy in trustedProxy.KnownProxies) + { + forwarded.KnownProxies.Add(IPAddress.Parse(proxy)); + } + + foreach (var network in trustedProxy.KnownNetworks) + { + forwarded.KnownIPNetworks.Add(System.Net.IPNetwork.Parse(network)); + } }); builder.Services.AddHeroDatabaseOptions(builder.Configuration); diff --git a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs new file mode 100644 index 0000000000..21d9b94e60 --- /dev/null +++ b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs @@ -0,0 +1,25 @@ +namespace FSH.Framework.Web.TrustedProxy; + +/// +/// Trusted reverse-proxy configuration for X-Forwarded-* processing. Behind an ingress +/// (e.g. cloudflared → Caddy → app) the real client IP and scheme arrive via forwarded headers; +/// these settings bound which upstream sources are trusted so a client reaching the app from +/// outside the proxy network cannot forge its own IP/scheme. When no proxies or networks are +/// configured, the framework default (loopback only) stands and forwarded headers from any other +/// source are ignored. +/// +public sealed class TrustedProxyOptions +{ + /// Individual upstream proxy IP addresses whose X-Forwarded-* headers are trusted. + public string[] KnownProxies { get; init; } = []; + + /// Trusted upstream networks in CIDR notation (e.g. "10.0.0.0/8", "172.16.0.0/12"). + public string[] KnownNetworks { get; init; } = []; + + /// + /// Number of proxy hops to unwind from X-Forwarded-For. Must match the real ingress hop count + /// (cloudflared → Caddy → app is 2). The framework default of 1 reads only the rightmost hop, + /// which yields the nearest proxy's IP (or an attacker-injected value) in a multi-hop topology. + /// + public int ForwardLimit { get; init; } = 1; +} diff --git a/src/Host/FSH.Starter.Api/appsettings.Production.json b/src/Host/FSH.Starter.Api/appsettings.Production.json index 332724534b..8cf660327d 100644 --- a/src/Host/FSH.Starter.Api/appsettings.Production.json +++ b/src/Host/FSH.Starter.Api/appsettings.Production.json @@ -91,6 +91,11 @@ "Ip": { "PermitLimit": 300, "WindowSeconds": 60, "QueueLimit": 0 }, "Auth": { "PermitLimit": 10, "WindowSeconds": 60, "QueueLimit": 0 } }, + "TrustedProxyOptions": { + "KnownProxies": [], + "KnownNetworks": [], + "ForwardLimit": 1 + }, "Storage": { "Provider": "local" } diff --git a/src/Host/FSH.Starter.Api/appsettings.json b/src/Host/FSH.Starter.Api/appsettings.json index 293fdfebb6..6a0a968ba2 100644 --- a/src/Host/FSH.Starter.Api/appsettings.json +++ b/src/Host/FSH.Starter.Api/appsettings.json @@ -156,6 +156,11 @@ "QueueLimit": 0 } }, + "TrustedProxyOptions": { + "KnownProxies": [], + "KnownNetworks": [], + "ForwardLimit": 1 + }, "Storage": { "Provider": "local" }, diff --git a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs index ab8cfe3c65..99b621799c 100644 --- a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs +++ b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs @@ -153,6 +153,22 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) builder.ConfigureServices(services => { + // Stamp the connection IP from a test header so forwarded-headers trust checks are testable. + services.AddSingleton(); + + // The production TrustedProxyOptions read happens eagerly, before the test config overlay + // applies (same quirk as storage below), so bind the trusted upstream here instead. This + // exercises the real UseForwardedHeaders trust boundary against TestConstants.TrustedProxyIp. + services.PostConfigure(forwarded => + { + forwarded.ForwardedHeaders = Microsoft.AspNetCore.HttpOverrides.ForwardedHeaders.XForwardedFor + | Microsoft.AspNetCore.HttpOverrides.ForwardedHeaders.XForwardedProto; + forwarded.ForwardLimit = 1; + forwarded.KnownProxies.Clear(); + forwarded.KnownIPNetworks.Clear(); + forwarded.KnownProxies.Add(System.Net.IPAddress.Parse(TestConstants.TrustedProxyIp)); + }); + // Remove hosted services that need unavailable infra or race migrations (RolePermissionSync, // Hangfire server + stale-lock cleanup, OutboxDispatcher); we register our own InMemory server below. var hostedServicesToRemove = services diff --git a/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs b/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs index d5a2a6e49c..2b49c3398a 100644 --- a/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs +++ b/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs @@ -6,6 +6,10 @@ public static class TestConstants public const string RootAdminEmail = "admin@root.com"; public const string DefaultPassword = "123Pa$$word!"; + // Documentation IP ranges (RFC 5737) so the trusted-proxy fixture never collides with a real host. + public const string TrustedProxyIp = "192.0.2.10"; + public const string UntrustedSourceIp = "198.51.100.9"; + public const string JwtIssuer = "fsh.local"; public const string JwtAudience = "fsh.clients"; public const string JwtSigningKey = "integration-test-signing-key-that-is-at-least-32-chars-long!!"; diff --git a/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs b/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs new file mode 100644 index 0000000000..118c96a91b --- /dev/null +++ b/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs @@ -0,0 +1,35 @@ +using System.Net; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; + +namespace Integration.Tests.Infrastructure; + +/// +/// TestServer has no real socket, so Connection.RemoteIpAddress is null and the +/// forwarded-headers trust check (known proxies/networks) can't be exercised. This filter runs +/// before the app pipeline (hence before UseForwardedHeaders) and stamps the connection IP from the +/// X-Test-Remote-Ip header so a test can present itself as a trusted or untrusted upstream. +/// Inert for requests that don't carry the header. +/// +public sealed class TestRemoteIpStartupFilter : IStartupFilter +{ + public const string RemoteIpHeader = "X-Test-Remote-Ip"; + + public Action Configure(Action next) => + app => + { + app.Use(async (context, nextMiddleware) => + { + var header = context.Request.Headers[RemoteIpHeader].FirstOrDefault(); + if (!string.IsNullOrEmpty(header) && IPAddress.TryParse(header, out var ip)) + { + context.Connection.RemoteIpAddress = ip; + } + + await nextMiddleware(); + }); + + next(app); + }; +} diff --git a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs index d6229abb31..01e7c8dd9c 100644 --- a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs +++ b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs @@ -8,15 +8,18 @@ namespace Integration.Tests.Tests.Security; /// /// Runtime repro for audit finding API-02 (no UseForwardedHeaders → proxy IP collapses the real -/// client IP). Token issuance persists a UserSession whose IpAddress comes from -/// RequestContextService.IpAddress => Connection.RemoteIpAddress. With a trusted-proxy -/// forwarded-headers config, a request carrying X-Forwarded-For should surface the real client IP; -/// because UseHeroPlatform never calls UseForwardedHeaders, the header is ignored. +/// client IP) plus its security boundary. Token issuance persists a UserSession whose IpAddress comes +/// from RequestContextService.IpAddress => Connection.RemoteIpAddress. The forwarded-headers config +/// trusts only the configured upstream (TestConstants.TrustedProxyIp), so: +/// - a request arriving from the trusted proxy has its X-Forwarded-For honored (real client IP), and +/// - a request arriving from any other source has X-Forwarded-For ignored (spoofing is blocked). +/// TestServer has no socket, so the connection IP is stamped via the X-Test-Remote-Ip header (see +/// TestRemoteIpStartupFilter). /// [Collection(FshCollectionDefinition.Name)] public sealed class ForwardedHeadersIpTests { - private const string ForwardedIp = "203.0.113.7"; + private const string ForwardedClientIp = "203.0.113.7"; private readonly FshWebApplicationFactory _factory; @@ -26,12 +29,39 @@ public ForwardedHeadersIpTests(FshWebApplicationFactory factory) } [Fact] - public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestCarriesXForwardedFor() + public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestArrivesFromTrustedProxy() + { + var recordedIp = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.TrustedProxyIp, + forwardedFor: ForwardedClientIp); + + recordedIp.ShouldBe( + ForwardedClientIp, + "behind a trusted proxy the persisted session IP should be the real client IP from " + + "X-Forwarded-For."); + } + + [Fact] + public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesFromUntrustedSource() + { + var recordedIp = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.UntrustedSourceIp, + forwardedFor: ForwardedClientIp); + + recordedIp.ShouldBe( + TestConstants.UntrustedSourceIp, + "X-Forwarded-For from a source outside the trusted-proxy set must be ignored; the persisted " + + "IP should be the connection IP, never the attacker-supplied forwarded value."); + recordedIp.ShouldNotBe(ForwardedClientIp); + } + + private async Task IssueTokenAndReadSessionIpAsync(string connectionIp, string forwardedFor) { using var client = _factory.CreateClient(); using var request = new HttpRequestMessage(HttpMethod.Post, $"{TestConstants.IdentityBasePath}/token/issue"); request.Headers.Add("tenant", TestConstants.RootTenantId); - request.Headers.Add("X-Forwarded-For", ForwardedIp); + request.Headers.Add(TestRemoteIpStartupFilter.RemoteIpHeader, connectionIp); + request.Headers.Add("X-Forwarded-For", forwardedFor); request.Content = JsonContent.Create(new { email = TestConstants.RootAdminEmail, @@ -41,12 +71,7 @@ public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestCarriesX using var response = await client.SendAsync(request); response.StatusCode.ShouldBe(HttpStatusCode.OK); - var recordedIp = await GetNewestSessionIpAsync(); - - recordedIp.ShouldBe( - ForwardedIp, - "behind a trusted proxy the persisted session IP should be the real client IP from " + - "X-Forwarded-For; without UseForwardedHeaders the app records the connection/loopback IP instead."); + return await GetNewestSessionIpAsync(); } private async Task GetNewestSessionIpAsync()