diff --git a/docs/technical-stakeholders-functional-overview.md b/docs/technical-stakeholders-functional-overview.md index aa7d205bc..73196bca0 100644 --- a/docs/technical-stakeholders-functional-overview.md +++ b/docs/technical-stakeholders-functional-overview.md @@ -204,7 +204,7 @@ Dev-only tooling (`Dev*Controller`s incl. impersonation used by E2E): `[AllowAno - The app is single-organisation-per-session; org selection happens inside DfE Sign-In. LA-Estab is composed as `{localAuthority.code}/{establishmentNumber}`, with a hardcoded `DSI/TEST` override for the DSI sandbox org (URN 990082). - **DataProtection** key ring persisted to blob (`data-protection-keys/keys.xml`) so OIDC state cookies survive multi-replica load balancing. - Antiforgery header name `X-XSRF-TOKEN` — all `fetch()` POSTs send it (`wwwroot/js/site.js` `getToken()` pattern). -- Security headers: CSP (script/style/img/connect/frame allow-list for self + GTM/GA/Clarity, `object-src 'none'`, `form-action 'self'`), HSTS outside dev, HTTPS redirect, forwarded headers for the AKS ingress. +- Security headers: CSP (script/style/img/connect/frame allow-list for self + GTM/GA/Clarity, `object-src 'none'`, `base-uri 'self'`, `frame-ancestors 'self'` so no other site can frame the service (#610), `form-action 'self'` plus the DfE Sign-in origin), HSTS outside dev, HTTPS redirect, forwarded headers for the AKS ingress. ## 10. Data model diff --git a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs index 69cdd69c9..a91bf28ff 100644 --- a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs +++ b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs @@ -9,8 +9,11 @@ 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 +/// The header set was already most of the way there — HSTS and a content-security policy were +/// in place — with a few gaps. (An earlier version of this remark said X-Frame-Options was set +/// too. It never was, by this app, and the policy carried no frame-ancestors either until +/// #610, so whether a page could be framed depended on whatever sat in front of the app.) 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. /// @@ -49,7 +52,15 @@ public sealed class SecurityHeadersMiddleware( "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'; "; + "base-uri 'self'; " + + // Which sites may put this service in a frame: this one only. frame-ancestors replaced + // X-Frame-Options in current browsers. Our code has never set X-Frame-Options; the only + // one the service sends is the SAMEORIGIN that ASP.NET Core's antiforgery adds to pages + // that render a token, which agrees with this directive but leaves every other response + // (and anything in front of the app: the AKS ingress, Front Door) uncovered — so without + // this directive the clickjacking surface was whatever the platform happened to set + // (#610). 'self' rather than 'none' so a same-origin frame stays possible. + "frame-ancestors 'self'; "; // form-action also governs every redirect a form submission follows, not only the form's own // URL. A POST made after the sign-in has expired is answered with a 302 to DfE Sign-in; with diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs index 8f1e1fadb..17b4b68ad 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Web/SecurityHeadersMiddlewareTests.cs @@ -6,8 +6,9 @@ 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 +// The response-header set was most of a good one — HSTS and a content-security policy were +// already there — with a few gaps a security assessment picked up (and, until #610, no +// frame-ancestors; the app has never sent X-Frame-Options, whatever this comment once said). 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 @@ -133,6 +134,22 @@ public async Task ContentSecurityPolicy_IsStillSet() Assert.Contains("form-action 'self'", csp); } + // frame-ancestors is the CSP directive that decides which sites may put this service in a + // frame; it replaced X-Frame-Options in every current browser. The policy had none, and the + // app has never set X-Frame-Options (the comment at the top of this file used to say it + // did), so until #610 whether a page could be framed — the clickjacking surface — depended + // on whatever sat in front of the app. Asserted as a whole directive rather than a + // substring: "frame-ancestors 'self' https://other.example" would pass a Contains. + [Fact] + public async Task FrameAncestors_AllowsOnlyThisSite() + { + var context = await Run(); + + var directives = context.Response.Headers["Content-Security-Policy"].ToString() + .Split(';', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries); + Assert.Contains("frame-ancestors 'self'", directives); + } + // A POST made after the sign-in has expired is answered with a 302 to DfE Sign-in. Chrome // applies form-action to every redirect a form submission follows, so with 'self' alone the // browser cancelled that navigation and the page simply did nothing — on every form in the