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/4] 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/4] 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 3/4] 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 4/4] 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