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..dea9a817ff 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"] @@ -79,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/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 50c6568fda..b3c638da8b 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -20,8 +20,10 @@ 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; using Microsoft.AspNetCore.ResponseCompression; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.Configuration; @@ -29,6 +31,7 @@ using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Hosting; using Mediator; +using System.Net; namespace FSH.Framework.Web; @@ -63,6 +66,72 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild } builder.Services.AddHttpContextAccessor(); + + // 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 => + { + // 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; + + // 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; + } + + foreach (var proxy in trustedProxy.KnownProxies) + { + 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) + { + 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); + } + }); + builder.Services.AddHeroDatabaseOptions(builder.Configuration); builder.Services.AddHeroRateLimiting(builder.Configuration); @@ -150,6 +219,11 @@ public static WebApplication UseHeroPlatform(this WebApplication app, Action +/// 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. +/// +/// 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 +{ + /// 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. + /// 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; +} diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..854deb9530 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -9,7 +9,8 @@ - + + @@ -122,9 +123,10 @@ - - - + + + + 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/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index e7a70abd05..4fc689599d 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") @@ -73,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) 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); + } +} diff --git a/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs new file mode 100644 index 0000000000..4c66fa4f11 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs @@ -0,0 +1,113 @@ +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"); + } + + [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 +} 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) @@ -153,6 +154,26 @@ 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. 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 + | 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 new file mode 100644 index 0000000000..0f0b7706f6 --- /dev/null +++ b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs @@ -0,0 +1,114 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; +using FSH.Framework.Shared.Multitenancy; +using FSH.Modules.Identity.Data; +using Integration.Tests.Infrastructure; + +namespace Integration.Tests.Tests.Security; + +/// +/// Runtime repro for audit finding API-02 (no UseForwardedHeaders → proxy IP collapses the real +/// 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 ForwardedClientIp = "203.0.113.7"; + + private readonly FshWebApplicationFactory _factory; + + public ForwardedHeadersIpTests(FshWebApplicationFactory factory) + { + _factory = factory; + } + + [Fact] + 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() + { + // 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); + + 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."); + 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) + { + 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); + request.Headers.Add(TestRemoteIpStartupFilter.RemoteIpHeader, connectionIp); + request.Headers.Add("X-Forwarded-For", forwardedFor); + request.Content = JsonContent.Create(new + { + email = TestConstants.RootAdminEmail, + password = TestConstants.DefaultPassword, + }); + + using var response = await client.SendAsync(request); + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + // 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> ReadSessionsAsync() + { + 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(); + return await db.UserSessions + .AsNoTracking() + .Select(s => new SessionRow(s.Id, s.IpAddress)) + .ToListAsync(); + } + + private sealed record SessionRow(Guid Id, string IpAddress); +}