From 3ff487efa8d5c34764987e5b526c408dac177aea Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Mon, 24 Aug 2026 12:41:09 +0100 Subject: [PATCH] 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"]); + } +}