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 01/13] 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 02/13] 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() From 85aa03b38f7e34d7dcc000b027f8e1df88d1e4b2 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 14 Aug 2026 04:33:36 -0300 Subject: [PATCH 03/13] fix(web): name the offending setting when trusted-proxy config is malformed 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. --- src/BuildingBlocks/Web/Extensions.cs | 16 ++- .../Web/TrustedProxyOptionsBindingTests.cs | 97 +++++++++++++++++++ .../Tests/Security/ForwardedHeadersIpTests.cs | 18 +++- 3 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 5ee1dfe320..5a270d4553 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -90,12 +90,24 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild foreach (var proxy in trustedProxy.KnownProxies) { - forwarded.KnownProxies.Add(IPAddress.Parse(proxy)); + if (!IPAddress.TryParse(proxy, out var address)) + { + throw new InvalidOperationException( + $"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)} contains \"{proxy}\", which is not a valid IP address (for example \"10.0.0.5\")."); + } + + forwarded.KnownProxies.Add(address); } foreach (var network in trustedProxy.KnownNetworks) { - forwarded.KnownIPNetworks.Add(System.Net.IPNetwork.Parse(network)); + if (!System.Net.IPNetwork.TryParse(network, out var parsedNetwork)) + { + throw new InvalidOperationException( + $"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownNetworks)} contains \"{network}\", which is not a valid CIDR network (for example \"10.0.0.0/8\")."); + } + + forwarded.KnownIPNetworks.Add(parsedNetwork); } }); diff --git a/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs new file mode 100644 index 0000000000..d4204524d9 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs @@ -0,0 +1,97 @@ +using FSH.Framework.Web; +using FSH.Framework.Web.TrustedProxy; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; + +namespace Framework.Tests.Web; + +/// +/// Pins the TrustedProxyOptions -> ForwardedHeadersOptions binding that AddHeroPlatform registers: which +/// upstreams end up trusted, that an unconfigured section keeps the framework's loopback-only default, and +/// that a malformed entry surfaces a message naming the offending setting rather than a bare FormatException. +/// +public sealed class TrustedProxyOptionsBindingTests +{ + private const string ProxyIp = "192.0.2.10"; + + private static ForwardedHeadersOptions Resolve(Dictionary settings) + { + // DisableDefaults keeps the host's environment-variable and appsettings providers out, so an ambient + // TrustedProxyOptions__* on the machine or CI runner can't change what "nothing configured" resolves to. + var builder = Host.CreateApplicationBuilder(new HostApplicationBuilderSettings + { + DisableDefaults = true, + }); + builder.Configuration.AddInMemoryCollection(settings); + builder.AddHeroPlatform(); + + using var provider = builder.Services.BuildServiceProvider(); + return provider.GetRequiredService>().Value; + } + + #region Trust boundary + + [Fact] + public void ForwardedHeaders_Should_KeepFrameworkLoopbackDefault_When_NothingConfigured() + { + // Act + var options = Resolve([]); + + // Assert - clearing the framework default here would make every caller a trusted proxy. + options.KnownProxies.ShouldNotBeEmpty(); + options.KnownIPNetworks.ShouldNotBeEmpty(); + } + + [Fact] + public void ForwardedHeaders_Should_TrustOnlyConfiguredProxy_When_KnownProxiesSet() + { + // Act + var options = Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)}:0"] = ProxyIp, + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.ForwardLimit)}"] = "2", + }); + + // Assert + options.KnownProxies.ShouldBe([System.Net.IPAddress.Parse(ProxyIp)]); + options.KnownIPNetworks.ShouldBeEmpty(); + options.ForwardLimit.ShouldBe(2); + } + + #endregion + + #region Malformed configuration + + [Fact] + public void ForwardedHeaders_Should_NameTheSetting_When_KnownProxyMalformed() + { + // Act + var exception = Should.Throw(() => Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)}:0"] = "not-an-ip", + })); + + // Assert + exception.Message.ShouldContain("TrustedProxyOptions:KnownProxies"); + exception.Message.ShouldContain("not-an-ip"); + } + + [Fact] + public void ForwardedHeaders_Should_NameTheSetting_When_KnownNetworkMalformed() + { + // Act + var exception = Should.Throw(() => Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownNetworks)}:0"] = "10.0.0.0/999", + })); + + // Assert + exception.Message.ShouldContain("TrustedProxyOptions:KnownNetworks"); + exception.Message.ShouldContain("10.0.0.0/999"); + } + + #endregion +} diff --git a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs index 01e7c8dd9c..997c15ab85 100644 --- a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs +++ b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs @@ -44,15 +44,27 @@ public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestArrivesF [Fact] public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesFromUntrustedSource() { - var recordedIp = await IssueTokenAndReadSessionIpAsync( + // Both arms send the SAME X-Forwarded-For and differ only in the source address. Asserting the + // untrusted outcome alone would pass even with forwarded-header processing removed entirely - the + // connection IP gets persisted either way - so the trusted arm is what makes this a boundary test. + var fromUntrusted = await IssueTokenAndReadSessionIpAsync( connectionIp: TestConstants.UntrustedSourceIp, forwardedFor: ForwardedClientIp); - recordedIp.ShouldBe( + var fromTrusted = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.TrustedProxyIp, + forwardedFor: ForwardedClientIp); + + fromUntrusted.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); + fromUntrusted.ShouldNotBe(ForwardedClientIp); + + fromTrusted.ShouldBe( + ForwardedClientIp, + "the identical header from the trusted proxy must be honored - otherwise the assertion above " + + "passes vacuously, satisfied by forwarded headers never being processed at all."); } private async Task IssueTokenAndReadSessionIpAsync(string connectionIp, string forwardedFor) From e7dbe6bc452f31c1a249d2ed351a32ae2f1abe2a Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 01:43:15 -0300 Subject: [PATCH 04/13] build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #1333, which is still open. Carried byte-identical to #1333's version of the file, comment included, so both stay mergeable in either order and this copy can simply be dropped once #1333 lands. --- src/Directory.Packages.props | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..7674befa8f 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -143,5 +143,12 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> + + \ No newline at end of file From 5def7d964ff475628143db397621e4571a365fef Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 05:11:26 -0300 Subject: [PATCH 05/13] fix(web): reject a trusted-proxy ForwardLimit below 1 at startup 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 #1358 --- src/BuildingBlocks/Web/Extensions.cs | 10 ++++++++++ .../Web/TrustedProxy/TrustedProxyOptions.cs | 2 ++ .../Web/TrustedProxyOptionsBindingTests.cs | 16 ++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 5a270d4553..f33e709184 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -77,6 +77,16 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild .GetSection(nameof(TrustedProxyOptions)).Get() ?? new TrustedProxyOptions(); builder.Services.Configure(forwarded => { + // A hop count below 1 is never what an operator means, and neither bad value announces itself: + // 0 truncates the unwind loop to zero iterations, so forwarded headers stop being processed with + // no error, while a negative value overflows the middleware's buffer allocation and 500s every + // request - including requests carrying no forwarded headers at all. Fail the boot instead. + if (trustedProxy.ForwardLimit < 1) + { + throw new InvalidOperationException( + $"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.ForwardLimit)} is {trustedProxy.ForwardLimit}, which is not a valid proxy hop count: it must be at least 1 (one hop per proxy in front of the app)."); + } + forwarded.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; forwarded.ForwardLimit = trustedProxy.ForwardLimit; diff --git a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs index 21d9b94e60..158eaccbe6 100644 --- a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs +++ b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs @@ -20,6 +20,8 @@ public sealed class TrustedProxyOptions /// 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. + /// Must be at least 1: anything lower is rejected at startup, since 0 would silently stop + /// forwarded-header processing and a negative value would fail every request. /// public int ForwardLimit { get; init; } = 1; } diff --git a/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs index d4204524d9..4c66fa4f11 100644 --- a/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs +++ b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs @@ -93,5 +93,21 @@ public void ForwardedHeaders_Should_NameTheSetting_When_KnownNetworkMalformed() exception.Message.ShouldContain("10.0.0.0/999"); } + [Theory] + [InlineData("-1")] // negative — overflows the middleware's buffer allocation, 500s every request + [InlineData("0")] // zero — truncates the unwind loop, forwarded headers silently stop being read + public void ForwardedHeaders_Should_NameTheSetting_When_ForwardLimitBelowOne(string forwardLimit) + { + // Act - no proxies or networks configured, so this has to be rejected before the trust-boundary block. + var exception = Should.Throw(() => Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.ForwardLimit)}"] = forwardLimit, + })); + + // Assert + exception.Message.ShouldContain("TrustedProxyOptions:ForwardLimit"); + exception.Message.ShouldContain($"is {forwardLimit}"); + } + #endregion } From cb62b102875b9e423236c3abb56b15f836257de1 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 05:32:44 -0300 Subject: [PATCH 06/13] docs(web): record why X-Forwarded-Host stays out of the flag list Review note from #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. --- src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs index 158eaccbe6..e49a748e9c 100644 --- a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs +++ b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs @@ -7,6 +7,13 @@ namespace FSH.Framework.Web.TrustedProxy; /// 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. +/// +/// Only X-Forwarded-For and X-Forwarded-Proto are honoured. X-Forwarded-Host is deliberately left +/// out: rewriting Request.Host from a header is a host-header injection primitive, and the endpoints +/// that build a public URL from the request (user registration and confirmation e-mails) would then +/// send links pointing wherever the header said. The trade-off is that Request.Host keeps the +/// internal host behind a proxy, and those links carry it. +/// /// public sealed class TrustedProxyOptions { From 8e0d7deb00378d24a6dd7269f59c123e394f1994 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:44:42 -0300 Subject: [PATCH 07/13] build(deps): bump Testcontainers to 4.14.0 and SourceLink past their 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 #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. --- src/Directory.Packages.props | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 7674befa8f..89162470ad 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -9,7 +9,8 @@ - + + @@ -122,9 +123,10 @@ - - - + + + + From 6816b7d76188d9d1cecf0c395179e3283df8cbad Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:45:29 -0300 Subject: [PATCH 08/13] fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub 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`. --- deploy/docker/README.md | 6 +++--- deploy/docker/docker-compose.yml | 3 ++- src/Host/FSH.Starter.AppHost/AppHost.cs | 3 +++ .../Infrastructure/MiddlewareWebApplicationFactory.cs | 3 ++- .../Infrastructure/FshWebApplicationFactory.cs | 3 ++- 5 files changed, 12 insertions(+), 6 deletions(-) diff --git a/deploy/docker/README.md b/deploy/docker/README.md index bcb1304593..0219164b7c 100644 --- a/deploy/docker/README.md +++ b/deploy/docker/README.md @@ -8,9 +8,9 @@ This brings up the full stack on a single host: | `admin` | `fsh/admin:local` | `FSH_ADMIN_PORT` (default 8081) | Operator console (nginx + React) | | `dashboard` | `fsh/dashboard:local` | `FSH_DASHBOARD_PORT` (default 8082) | Tenant dashboard (nginx + React) | | `migrator` | `fsh/dbmigrator:local` | — | One-shot: applies EF migrations + seeds the root tenant + creates the default admin user | -| `postgres` | `postgres:17-alpine` | (internal) | Identity, tenant catalog, module schemas | -| `redis` | `redis:7-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | -| `minio` | `minio/minio:latest` | (internal) | S3-compatible blob store for the Files module | +| `postgres` | `postgres:18-alpine` | (internal) | Identity, tenant catalog, module schemas | +| `redis` | `valkey/valkey:9.1.0-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | +| `minio` | `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` | (internal) | S3-compatible blob store for the Files module | The compose file does **not** include a reverse proxy or TLS terminator. You bring your own edge — Cloudflare Tunnel, AWS ALB, Tailscale Funnel, your existing nginx, anything that can route a TLS subdomain to a host:port on this machine. diff --git a/deploy/docker/docker-compose.yml b/deploy/docker/docker-compose.yml index d43c744f5b..9457a61232 100644 --- a/deploy/docker/docker-compose.yml +++ b/deploy/docker/docker-compose.yml @@ -54,7 +54,8 @@ services: # - "6379:6379" minio: - image: minio/minio:latest + # quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z container_name: fsh-minio restart: unless-stopped command: ["server", "/data", "--console-address", ":9001"] diff --git a/src/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index e7a70abd05..fb3506ab42 100644 --- a/src/Host/FSH.Starter.AppHost/AppHost.cs +++ b/src/Host/FSH.Starter.AppHost/AppHost.cs @@ -52,7 +52,10 @@ var minioUser = builder.AddParameter("minio-user", "minioadmin"); var minioPassword = builder.AddParameter("minio-password", "minioadmin", secret: true); +// quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. var minio = builder.AddContainer("minio", "minio/minio") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-09-07T16-13-09Z") .WithArgs("server", "/data", "--console-address", ":9001") .WithHttpEndpoint(port: 9000, targetPort: 9000, name: "api") .WithHttpEndpoint(port: 9001, targetPort: 9001, name: "console") diff --git a/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs index 4c2939c454..e8b7898023 100644 --- a/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs +++ b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs @@ -55,7 +55,8 @@ public sealed class MiddlewareWebApplicationFactory : WebApplicationFactory, I .WithCleanUp(true) .Build(); - private readonly MinioContainer _minio = new MinioBuilder("minio/minio:latest") + // quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + private readonly MinioContainer _minio = new MinioBuilder("quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z") .WithUsername(MinioAccessKey) .WithPassword(MinioSecretKey) .WithAutoRemove(true) From 6f0b8f85233191e0425549f4e8c1e82aef0be69c Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:37:21 -0300 Subject: [PATCH 09/13] fix(web): rebuild the forwarded-headers trust list instead of appending 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. --- src/BuildingBlocks/Web/Extensions.cs | 17 ++++- .../Web/ForwardedHeadersHostDefaultsTests.cs | 64 +++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/ForwardedHeadersHostDefaultsTests.cs diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index f33e709184..b3c638da8b 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -90,14 +90,25 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild forwarded.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; forwarded.ForwardLimit = trustedProxy.ForwardLimit; + // The trust list is always rebuilt from scratch, never appended to. Whatever is in the + // options when this runs depends on who configured them first, and with + // ASPNETCORE_FORWARDEDHEADERS_ENABLED=true that is 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 empty means the app + // rewrites RemoteIpAddress from an X-Forwarded-For sent by anyone at all. + forwarded.KnownProxies.Clear(); + forwarded.KnownIPNetworks.Clear(); + if (trustedProxy.KnownProxies.Length == 0 && trustedProxy.KnownNetworks.Length == 0) { + // Nothing configured: restate the framework's own default rather than inherit it, + // for the same reason. Local development runs behind Kestrel on loopback and still + // needs its forwarded headers honoured. + forwarded.KnownProxies.Add(IPAddress.IPv6Loopback); + forwarded.KnownIPNetworks.Add(new System.Net.IPNetwork(IPAddress.Loopback, 8)); return; } - forwarded.KnownProxies.Clear(); - forwarded.KnownIPNetworks.Clear(); - foreach (var proxy in trustedProxy.KnownProxies) { if (!IPAddress.TryParse(proxy, out var address)) diff --git a/src/Tests/Framework.Tests/Web/ForwardedHeadersHostDefaultsTests.cs b/src/Tests/Framework.Tests/Web/ForwardedHeadersHostDefaultsTests.cs new file mode 100644 index 0000000000..9890474297 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/ForwardedHeadersHostDefaultsTests.cs @@ -0,0 +1,64 @@ +using FSH.Framework.Web; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; + +namespace Framework.Tests.Web; + +/// +/// The sibling of for the one host shape that class cannot +/// reach. It builds through Host.CreateApplicationBuilder, which never runs +/// ConfigureWebDefaults, so the framework's loopback defaults are always still in place when +/// AddHeroPlatform looks at them. A web host started with FORWARDEDHEADERS_ENABLED registers +/// ForwardedHeadersOptionsSetup, which empties both trust lists — and an empty trust list is not +/// "trust nobody" in ForwardedHeadersMiddleware, it is "check nobody": the middleware only validates +/// the peer when at least one entry exists. That is the configuration this pins. +/// +public sealed class ForwardedHeadersHostDefaultsTests +{ + private static ForwardedHeadersOptions ResolveWithAspNetForwarding() + { + // Passed as a command-line arg rather than an environment variable: host configuration reads + // both, and an env var would leak into every other test running in this process. + var builder = WebApplication.CreateBuilder(new WebApplicationOptions + { + Args = ["--FORWARDEDHEADERS_ENABLED=true"], + EnvironmentName = "Development", + }); + builder.AddHeroPlatform(); + + // The premise of this whole test: ASP.NET registered its own setup for these options. If the + // flag ever stops reaching host configuration, the assertions below would pass for the wrong + // reason — nothing cleared the lists, so nothing had to restore them. + builder.Services.Any(d => + d.ServiceType == typeof(IConfigureOptions) && + d.ImplementationType?.Name == "ForwardedHeadersOptionsSetup") + .ShouldBeTrue("FORWARDEDHEADERS_ENABLED did not reach host configuration"); + + // Not builder.Build(): the host validates the whole container, and the modules that supply + // ICurrentUser and friends are not registered here. Only the options matter. + using var provider = builder.Services.BuildServiceProvider(); + return provider.GetRequiredService>().Value; + } + + [Fact] + public void ForwardedHeaders_Should_TrustSomeone_When_NothingConfiguredAndAspNetForwardingEnabled() + { + // Act + var options = ResolveWithAspNetForwarding(); + + // Assert — with both lists empty the middleware skips the peer check entirely and rewrites + // RemoteIpAddress from X-Forwarded-For sent by anyone at all. + (options.KnownProxies.Count + options.KnownIPNetworks.Count).ShouldBeGreaterThan( + 0, + "an empty trust list makes ForwardedHeadersMiddleware accept X-Forwarded-For from any peer"); + + // And what is restored is the framework's own default, not a trust policy of our own + // invention: with nothing configured the app must trust exactly loopback, no wider. + var frameworkDefaults = new ForwardedHeadersOptions(); + options.KnownProxies.ShouldBe(frameworkDefaults.KnownProxies); + options.KnownIPNetworks.ShouldBe(frameworkDefaults.KnownIPNetworks); + } +} From e1ab57b15bc64fd970c1cac483ad1511fc65611d Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:28:41 -0300 Subject: [PATCH 10/13] fix(infra): pull minio/mc from quay.io too, not just minio/minio 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 #1388, which owns the fix, so the copy stays byte-identical to it and can be dropped once that lands. --- deploy/docker/docker-compose.yml | 3 ++- src/Host/FSH.Starter.AppHost/AppHost.cs | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/deploy/docker/docker-compose.yml b/deploy/docker/docker-compose.yml index 9457a61232..dea9a817ff 100644 --- a/deploy/docker/docker-compose.yml +++ b/deploy/docker/docker-compose.yml @@ -80,7 +80,8 @@ services: # policy is set — objects are served via the API / presigned URLs, not a # public bucket. minio-init: - image: minio/mc:latest + # quay.io: minio/mc is gone from Docker Hub too. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/mc:RELEASE.2025-08-13T08-35-41Z container_name: fsh-minio-init restart: "no" depends_on: diff --git a/src/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index fb3506ab42..4fc689599d 100644 --- a/src/Host/FSH.Starter.AppHost/AppHost.cs +++ b/src/Host/FSH.Starter.AppHost/AppHost.cs @@ -76,6 +76,8 @@ """).ReplaceLineEndings("\n"); var minioInit = builder.AddContainer("minio-init", "minio/mc") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-08-13T08-35-41Z") .WithEntrypoint("/bin/sh") .WithArgs("-c", minioInitScript) .WithEnvironment("MC_USER", minioUser) From 99e551459679eab9802213090b72b97f259fd3a5 Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:34:55 -0300 Subject: [PATCH 11/13] build(deps): drop the dead SSH.NET pin 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). --- src/Directory.Packages.props | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 89162470ad..854deb9530 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -145,12 +145,5 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> - - \ No newline at end of file From 71000c36aff8a10e761b1a7d496256fff76098c2 Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 06:48:46 -0300 Subject: [PATCH 12/13] docs(web): say what a ForwardLimit above the real hop count costs 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. --- .../Web/TrustedProxy/TrustedProxyOptions.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs index e49a748e9c..76ae517bf3 100644 --- a/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs +++ b/src/BuildingBlocks/Web/TrustedProxy/TrustedProxyOptions.cs @@ -29,6 +29,14 @@ public sealed class TrustedProxyOptions /// which yields the nearest proxy's IP (or an attacker-injected value) in a multi-hop topology. /// Must be at least 1: anything lower is rejected at startup, since 0 would silently stop /// forwarded-header processing and a negative value would fail every request. + /// + /// Setting it higher than the real hop count is what turns this into a vulnerability: the + /// middleware trusts one entry per hop, counting from the right, and only the peer itself is + /// checked against the trust list. A limit of 2 with a single proxy in front means the value the + /// proxy appended is discarded in favour of the one the client sent, so the caller picks its own + /// RemoteIpAddress and every IP-based rate limit and audit entry follows it. Count the proxies + /// that actually rewrite the header, not the ones in the diagram. + /// /// public int ForwardLimit { get; init; } = 1; } From 00ec8e8afbe52504637f4def99eb0f43dbbda7ed Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:28:15 -0300 Subject: [PATCH 13/13] test(security): read the session this request created, not the newest 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` 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. --- .../FshWebApplicationFactory.cs | 8 +++++-- .../Tests/Security/ForwardedHeadersIpTests.cs | 24 ++++++++++++------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs index ae704f6650..ce391a60e9 100644 --- a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs +++ b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs @@ -158,8 +158,12 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) 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. + // applies (same quirk as storage below), so bind the trusted upstream here instead. Note what + // that leaves these tests covering: overwriting the flags, the forward limit and both trust + // lists wholesale replaces whatever the TrustedProxyOptions binding produced, so what runs + // against TestConstants.TrustedProxyIp is the real middleware and the real placement of + // UseForwardedHeaders, not the binding that feeds them in production. That binding is gated + // separately, by Framework.Tests/Web/TrustedProxyOptionsBindingTests. services.PostConfigure(forwarded => { forwarded.ForwardedHeaders = Microsoft.AspNetCore.HttpOverrides.ForwardedHeaders.XForwardedFor diff --git a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs index 997c15ab85..0f0b7706f6 100644 --- a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs +++ b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs @@ -67,8 +67,10 @@ public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesF "passes vacuously, satisfied by forwarded headers never being processed at all."); } - private async Task IssueTokenAndReadSessionIpAsync(string connectionIp, string forwardedFor) + private async Task IssueTokenAndReadSessionIpAsync(string connectionIp, string forwardedFor) { + var before = (await ReadSessionsAsync()).Select(s => s.Id).ToHashSet(); + using var client = _factory.CreateClient(); using var request = new HttpRequestMessage(HttpMethod.Post, $"{TestConstants.IdentityBasePath}/token/issue"); request.Headers.Add("tenant", TestConstants.RootTenantId); @@ -83,10 +85,16 @@ public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesF using var response = await client.SendAsync(request); response.StatusCode.ShouldBe(HttpStatusCode.OK); - return await GetNewestSessionIpAsync(); + // Reading "the newest session" would rank rows by a CreatedAt that two token issues in this + // collection can land on the same tick of, and Id is a random Guid, so a tiebreak on it picks + // deterministically but not necessarily correctly. The row this request created is the one that + // was not there before it, and asserting there is exactly one says so instead of assuming it. + var created = (await ReadSessionsAsync()).Where(s => !before.Contains(s.Id)).ToList(); + + return created.ShouldHaveSingleItem().IpAddress; } - private async Task GetNewestSessionIpAsync() + private async Task> ReadSessionsAsync() { using var scope = _factory.Services.CreateScope(); @@ -96,11 +104,11 @@ public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesF new MultiTenantContext(tenant); var db = scope.ServiceProvider.GetRequiredService(); - var session = await db.UserSessions + return await db.UserSessions .AsNoTracking() - .OrderByDescending(s => s.CreatedAt) - .FirstOrDefaultAsync(); - - return session?.IpAddress; + .Select(s => new SessionRow(s.Id, s.IpAddress)) + .ToListAsync(); } + + private sealed record SessionRow(Guid Id, string IpAddress); }