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"]); + } +}