From 00af64d41c34b6306dbd6d4011df4ee553f2a928 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Mon, 19 Oct 2026 14:40:45 +0100 Subject: [PATCH 1/3] #610 Refuse framing by other sites in the content security policy frame-ancestors 'self' joins the policy base. The app never set X-Frame-Options, so the class remark and the test header that said it did are corrected. Co-Authored-By: Claude Fable 5.1 --- .../Middleware/SecurityHeadersMiddleware.cs | 15 ++++++++++--- .../Web/SecurityHeadersMiddlewareTests.cs | 21 +++++++++++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs index 69cdd69c9..f14618438 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,13 @@ 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, the app has never sent X-Frame-Options, and + // nothing in front of it (the AKS ingress, Front Door) is known to add either — 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 From ce31ba787fa978d059c9da9f07609c3709f77a5b Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Mon, 19 Oct 2026 14:41:00 +0100 Subject: [PATCH 2/3] #610 List frame-ancestors among the security headers in the overview Co-Authored-By: Claude Fable 5.1 --- docs/technical-stakeholders-functional-overview.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 5759bcb19c705d7080eec6315130dea01bd3ccaa Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Mon, 19 Oct 2026 15:43:47 +0100 Subject: [PATCH 3/3] #610 Say where the SAMEORIGIN X-Frame-Options on form pages comes from The directive comment said the app has never sent X-Frame-Options. Our code never set it, but ASP.NET Core's antiforgery adds SAMEORIGIN to every page that renders a token, which the review-app proof shows on the start page. The comment now says so, so a reviewer who curls the app does not read it as wrong. Co-Authored-By: Claude Fable 5.1 --- .../Middleware/SecurityHeadersMiddleware.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs index f14618438..a91bf28ff 100644 --- a/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs +++ b/src/DfE.CheckPerformanceData.Web/Middleware/SecurityHeadersMiddleware.cs @@ -54,10 +54,12 @@ public sealed class SecurityHeadersMiddleware( "object-src 'none'; " + "base-uri 'self'; " + // Which sites may put this service in a frame: this one only. frame-ancestors replaced - // X-Frame-Options in current browsers, the app has never sent X-Frame-Options, and - // nothing in front of it (the AKS ingress, Front Door) is known to add either — 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. + // 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