From 3ff487efa8d5c34764987e5b526c408dac177aea Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Mon, 24 Aug 2026 12:41:09 +0100 Subject: [PATCH 1/2] Fill the gaps in the response security headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A security assessment picked up a handful of low-severity header and cookie items. Individually each is minor; together they are the cheapest security work available, and several had been noted informally for a while. The existing set was most of the way there — HSTS, a content-security policy and X-Frame-Options were already in place, and the session cookie already sets Secure outside development. This fills what was missing rather than starting from nothing: - X-Content-Type-Options: nosniff, so a browser stops second-guessing a declared content type. It matters here because the storage browser and the content pipeline both serve files an author supplied. - Referrer-Policy: strict-origin-when-cross-origin. Without it the full URL, window and request identifiers included, travels to any third-party origin in the Referer header. Same-origin navigation is unaffected. - Permissions-Policy denying camera, microphone, geolocation and the rest. The service asks for none of them, so an injected frame or script cannot ask on its behalf either. - The antiforgery cookie now follows the same environment-dependent Secure policy the session cookie already had; it had been left on the default. SameAsRequest in development keeps local HTTP working, where Always would have the browser drop the cookie and fail every form POST. - TRACE is refused with 405 before routing. Cross-site tracing is already closed by HttpOnly cookies and modern browsers, so this removes a surface rather than fixing an exploit. - Kestrel no longer advertises itself. The banner carries no version, but naming the stack tells a scanner which exploits are worth trying and buys nothing. The headers moved into one middleware. They only do anything if they are on every response, and spread across controllers a new endpoint silently misses them. The content-security policy moved there unchanged and is pinned by a test so folding it in cannot quietly drop it. Headers are assigned rather than appended: appending to one something upstream already set produces two of it, and a browser given two conflicting security headers may pick the one we did not want. Left alone deliberately: 'unsafe-inline' and 'unsafe-eval' in the policy's script-src. Removing them means threading a per-request nonce through every inline script and style, including those the frontend toolkit and the analytics tags emit — real regression risk that deserves its own change and its own testing rather than riding along with a header sweep. Refs #347 --- .../Middleware/SecurityHeadersMiddleware.cs | 62 +++++++++ .../Startup/CoreWebExtensions.cs | 18 ++- .../Startup/RequestPipelineExtensions.cs | 21 +--- .../Web/SecurityHeadersMiddlewareTests.cs | 118 ++++++++++++++++++ 4 files changed, 197 insertions(+), 22 deletions(-) create mode 100644 src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs create mode 100644 tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs diff --git a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs new file mode 100644 index 00000000..6cccd7bd --- /dev/null +++ b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs @@ -0,0 +1,62 @@ +using Microsoft.AspNetCore.Http; + +namespace DfE.CheckPerformanceData.Web.Middleware; + +/// +/// Sets the response security headers, and refuses TRACE. +/// +/// +/// The header set was already most of the way there — HSTS, a content-security policy and +/// X-Frame-Options were in place — with a few gaps. These headers only do anything if they are on +/// every response, so they belong in one middleware rather than spread across controllers where a +/// new endpoint silently misses them. +/// +/// The content-security policy moved here unchanged. Its 'unsafe-inline' and +/// 'unsafe-eval' in script-src are a known weakness, but removing them means +/// threading a per-request nonce through every inline script and style, including those the +/// frontend toolkit and the analytics tags emit. That is a change with real regression risk and +/// deserves its own testing rather than riding along with a header sweep. +/// +public sealed class SecurityHeadersMiddleware(RequestDelegate next) +{ + private const string ContentSecurityPolicy = + "default-src 'self'; " + + "script-src 'self' 'unsafe-inline' 'unsafe-eval' https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms; " + + "style-src 'self' 'unsafe-inline' https://*.googletagmanager.com https://fonts.googleapis.com; " + + "img-src 'self' data: blob: https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms https://fonts.gstatic.com; " + + "font-src 'self' data: https://fonts.gstatic.com; " + + "connect-src 'self' https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms; " + + "frame-src 'self' https://*.googletagmanager.com; " + + "object-src 'none'; " + + "base-uri 'self'; " + + "form-action 'self'"; + + // The service asks for none of these. Denying them means an injected frame or script cannot + // ask on its behalf either. + private const string PermissionsPolicy = + "camera=(), microphone=(), geolocation=(), payment=(), usb=(), magnetometer=(), gyroscope=()"; + + public async Task InvokeAsync(HttpContext context) + { + // Refused before anything else runs: TRACE has no use in this service, and a method that + // is going to be rejected should not first be routed, authorised and handled. + if (HttpMethods.IsTrace(context.Request.Method)) + { + context.Response.StatusCode = StatusCodes.Status405MethodNotAllowed; + return; + } + + // Assignment rather than Append: appending to a header something upstream already set + // produces two of it, and a browser given two conflicting security headers is entitled to + // pick the one we did not want. + Set(context, "Content-Security-Policy", ContentSecurityPolicy); + Set(context, "X-Content-Type-Options", "nosniff"); + Set(context, "Referrer-Policy", "strict-origin-when-cross-origin"); + Set(context, "Permissions-Policy", PermissionsPolicy); + + await next(context); + } + + private static void Set(HttpContext context, string name, string value) => + context.Response.Headers[name] = value; +} diff --git a/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs b/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs index 1dd4eee1..78d171b2 100644 --- a/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs +++ b/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs @@ -3,6 +3,8 @@ using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Extensions.DependencyInjection; +using Microsoft.AspNetCore.Http; + namespace DfE.CheckPerformanceData.Web.Startup; public static class CoreWebExtensions @@ -20,10 +22,22 @@ public static WebApplicationBuilder AddCpdCoreWeb(this WebApplicationBuilder bui builder.Services.AddAntiforgery(options => { options.HeaderName = "X-XSRF-TOKEN"; + // The session cookie already follows this policy; the antiforgery cookie was left on + // the default. SameAsRequest in development keeps local HTTP working — Always there + // would have the browser drop the cookie and every form POST fail antiforgery. + options.Cookie.SecurePolicy = builder.Environment.IsDevelopment() + ? CookieSecurePolicy.SameAsRequest + : CookieSecurePolicy.Always; }); - // Setting to null to allow controller-level request size limits - builder.WebHost.ConfigureKestrel(o => o.Limits.MaxRequestBodySize = null); + // Setting to null to allow controller-level request size limits. + // AddServerHeader off: the banner carries no version, but naming the stack tells a + // scanner which exploits are worth trying and buys nothing back. + builder.WebHost.ConfigureKestrel(o => + { + o.Limits.MaxRequestBodySize = null; + o.AddServerHeader = false; + }); builder.Services.AddControllersWithViews(); diff --git a/src/DfE.CheckPerformanceData.Web/Startup/RequestPipelineExtensions.cs b/src/DfE.CheckPerformanceData.Web/Startup/RequestPipelineExtensions.cs index ce34a17a..80341510 100644 --- a/src/DfE.CheckPerformanceData.Web/Startup/RequestPipelineExtensions.cs +++ b/src/DfE.CheckPerformanceData.Web/Startup/RequestPipelineExtensions.cs @@ -40,7 +40,7 @@ public static WebApplication UseCpdRequestPipeline(this WebApplication app) app.UseHttpsRedirection(); - app.UseCpdContentSecurityPolicy(); + app.UseMiddleware(); app.UseSession(); @@ -86,23 +86,4 @@ public static WebApplication UseCpdRequestPipeline(this WebApplication app) return app; } - - private static void UseCpdContentSecurityPolicy(this WebApplication app) - { - app.Use(async (context, next) => - { - context.Response.Headers.Append("Content-Security-Policy", - "default-src 'self'; " + - "script-src 'self' 'unsafe-inline' 'unsafe-eval' https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms; " + - "style-src 'self' 'unsafe-inline' https://*.googletagmanager.com https://fonts.googleapis.com; " + - "img-src 'self' data: blob: https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms https://fonts.gstatic.com; " + - "font-src 'self' data: https://fonts.gstatic.com; " + - "connect-src 'self' https://*.googletagmanager.com https://*.google-analytics.com https://*.analytics.google.com https://*.clarity.ms; " + - "frame-src 'self' https://*.googletagmanager.com; " + - "object-src 'none'; " + - "base-uri 'self'; " + - "form-action 'self'"); - await next(); - }); - } } diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs new file mode 100644 index 00000000..17e7a714 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs @@ -0,0 +1,118 @@ +using DfE.CheckPerformanceData.Web.Middleware; +using Microsoft.AspNetCore.Http; + +namespace DfE.CheckPerformanceData.Application.UnitTests.Web; + +// The response-header set was most of a good one — HSTS, a content-security policy and +// X-Frame-Options were already there — with a few gaps a security assessment picked up. These +// are cheap headers that only help if they are on every response, so they are set in one place +// rather than per-controller, and asserted here rather than left to a scan to notice. +public sealed class SecurityHeadersMiddlewareTests +{ + private static async Task Run(string method = "GET", Action? arrange = null) + { + var context = new DefaultHttpContext(); + context.Request.Method = method; + arrange?.Invoke(context); + + var sut = new SecurityHeadersMiddleware(_ => Task.CompletedTask); + await sut.InvokeAsync(context); + return context; + } + + // Stops a browser second-guessing a declared content type. It matters here because the + // storage browser and the content pipeline both serve files an author supplied. + [Fact] + public async Task Nosniff_IsSet() + { + var context = await Run(); + + Assert.Equal("nosniff", context.Response.Headers["X-Content-Type-Options"]); + } + + // Without this the full URL — window and request identifiers included — travels to any + // third-party origin in the Referer header. strict-origin-when-cross-origin keeps + // same-origin navigation intact and sends only the origin outward. + [Fact] + public async Task ReferrerPolicy_IsSetToStrictOriginWhenCrossOrigin() + { + var context = await Run(); + + Assert.Equal("strict-origin-when-cross-origin", context.Response.Headers["Referrer-Policy"]); + } + + // The service needs none of these, and denying them costs nothing. + [Theory] + [InlineData("camera=()")] + [InlineData("microphone=()")] + [InlineData("geolocation=()")] + public async Task PermissionsPolicy_DeniesHardwareItDoesNotUse(string directive) + { + var context = await Run(); + + Assert.Contains(directive, context.Response.Headers["Permissions-Policy"].ToString()); + } + + // The method has no legitimate use in this service. Cross-site tracing is already closed by + // HttpOnly cookies and modern browsers, so this is removing a surface rather than a fix. + [Fact] + public async Task Trace_IsRefused() + { + var context = await Run("TRACE"); + + Assert.Equal(StatusCodes.Status405MethodNotAllowed, context.Response.StatusCode); + } + + [Fact] + public async Task Trace_DoesNotReachTheRestOfThePipeline() + { + var context = new DefaultHttpContext(); + context.Request.Method = "TRACE"; + var reached = false; + + var sut = new SecurityHeadersMiddleware(_ => { reached = true; return Task.CompletedTask; }); + await sut.InvokeAsync(context); + + Assert.False(reached); + } + + [Theory] + [InlineData("GET")] + [InlineData("POST")] + [InlineData("HEAD")] + public async Task OrdinaryMethods_AreUntouched(string method) + { + var context = new DefaultHttpContext(); + context.Request.Method = method; + var reached = false; + + var sut = new SecurityHeadersMiddleware(_ => { reached = true; return Task.CompletedTask; }); + await sut.InvokeAsync(context); + + Assert.True(reached); + Assert.NotEqual(StatusCodes.Status405MethodNotAllowed, context.Response.StatusCode); + } + + // The policy was already here and is not being changed; this pins that folding the other + // headers in did not drop it. + [Fact] + public async Task ContentSecurityPolicy_IsStillSet() + { + var context = await Run(); + + var csp = context.Response.Headers["Content-Security-Policy"].ToString(); + Assert.Contains("default-src 'self'", csp); + Assert.Contains("object-src 'none'", csp); + Assert.Contains("form-action 'self'", csp); + } + + // Setting a header twice produces two of it. Something upstream may already have supplied + // one, and a duplicated security header is a header a browser may disagree with us about. + [Fact] + public async Task AHeaderAlreadySet_IsNotDuplicated() + { + var context = await Run(arrange: c => c.Response.Headers["X-Content-Type-Options"] = "nosniff"); + + Assert.Single(context.Response.Headers["X-Content-Type-Options"]); + } +} From 991e38672825466a2a315dfcd418fdd6de354731 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 25 Aug 2026 13:28:03 +0100 Subject: [PATCH 2/2] Do not force the antiforgery cookie Secure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting AntiforgeryOptions.Cookie.SecurePolicy to Always outside development looked like the obvious counterpart to the session cookie, which already does exactly that. It is not. The session cookie system only marks the cookie; DefaultAntiforgery.CheckSSLConfig throws when the policy is Always and the request is not HTTPS, and _Layout mints a token on every page render. Every page therefore returned 500. It passed locally because development resolves to SameAsRequest and never exercises the branch, and it passed a manual HTTPS check because Kestrel was terminating TLS itself there so IsHttps was true. It failed in the review app, where the pod sits behind a TLS-terminating ingress and receives plain HTTP. Reproduced by running the container with ASPNETCORE_ENVIRONMENT=Review over HTTP: 500 on every page before this change, 200 after, with the same antiforgery exception in the log. UseForwardedHeaders is already wired with XForwardedProto, which would make Request.IsHttps true and the policy safe, but KnownProxies.Clear() leaves KnownNetworks at its loopback default, so the ingress's header is dropped and the app believes it is serving plain HTTP in every deployed environment. Correcting that means deciding which proxies to trust — trusting X-Forwarded-Proto from anywhere lets a client claim HTTPS — and that is a security decision in its own right, not something to settle inside a header change. So the cookie keeps its default here and the reason is recorded where the next person will look for it. The other five items in this change are unaffected: verified in the same Review-over-HTTP container that all four headers are still applied, TRACE is still refused, and the server banner is still suppressed. Refs #347 --- .../Startup/CoreWebExtensions.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs b/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs index 78d171b2..e73c3b2f 100644 --- a/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs +++ b/src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs @@ -3,7 +3,6 @@ using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Extensions.DependencyInjection; -using Microsoft.AspNetCore.Http; namespace DfE.CheckPerformanceData.Web.Startup; @@ -22,12 +21,16 @@ public static WebApplicationBuilder AddCpdCoreWeb(this WebApplicationBuilder bui builder.Services.AddAntiforgery(options => { options.HeaderName = "X-XSRF-TOKEN"; - // The session cookie already follows this policy; the antiforgery cookie was left on - // the default. SameAsRequest in development keeps local HTTP working — Always there - // would have the browser drop the cookie and every form POST fail antiforgery. - options.Cookie.SecurePolicy = builder.Environment.IsDevelopment() - ? CookieSecurePolicy.SameAsRequest - : CookieSecurePolicy.Always; + // Cookie.SecurePolicy is deliberately left at its default. Setting it to Always + // outside development looks like the obvious counterpart to the session cookie, but + // the antiforgery system does not merely mark the cookie: DefaultAntiforgery + // .CheckSSLConfig throws when the policy is Always and the request is not HTTPS, and + // _Layout mints a token on every page render. Deployed pods sit behind a + // TLS-terminating ingress and receive plain HTTP, and UseForwardedHeaders does not + // correct Request.IsHttps here because KnownProxies.Clear() above leaves + // KnownNetworks at its loopback default, so the ingress's X-Forwarded-Proto is + // dropped. The result is a 500 on every page. Securing this cookie has to wait for + // the forwarded-headers trust boundary to be decided. }); // Setting to null to allow controller-level request size limits.