347 Fill the gaps in the response security headers

src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs

@@ -0,0 +1,62 @@
+using Microsoft.AspNetCore.Http;
+
+namespace DfE.CheckPerformanceData.Web.Middleware;
+
+/// <summary>
+/// Sets the response security headers, and refuses TRACE.
+/// </summary>
+/// <remarks>
+/// 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 <c>'unsafe-inline'</c> and
+/// <c>'unsafe-eval'</c> in <c>script-src</c> 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.
+/// </remarks>
+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;
+}

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();
 

src/DfE.CheckPerformanceData.Web/Startup/RequestPipelineExtensions.cs

@@ -40,7 +40,7 @@ public static WebApplication UseCpdRequestPipeline(this WebApplication app)
 
         app.UseHttpsRedirection();
 
-        app.UseCpdContentSecurityPolicy();
+        app.UseMiddleware<SecurityHeadersMiddleware>();
 
         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();
-        });
-    }
 }

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<HttpContext> Run(string method = "GET", Action<HttpContext>? 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"]);
+    }
+}