Hotfix: revert the antiforgery Secure cookie — main returns 500 on every page - #351
Merged
Conversation
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
Setting AntiforgeryOptions.Cookie.SecurePolicy to Always outside development looked like the obvious counterpart to the session cookie, which already does exactly that. It is not. The session cookie system only marks the cookie; DefaultAntiforgery.CheckSSLConfig throws when the policy is Always and the request is not HTTPS, and _Layout mints a token on every page render. Every page therefore returned 500. It passed locally because development resolves to SameAsRequest and never exercises the branch, and it passed a manual HTTPS check because Kestrel was terminating TLS itself there so IsHttps was true. It failed in the review app, where the pod sits behind a TLS-terminating ingress and receives plain HTTP. Reproduced by running the container with ASPNETCORE_ENVIRONMENT=Review over HTTP: 500 on every page before this change, 200 after, with the same antiforgery exception in the log. UseForwardedHeaders is already wired with XForwardedProto, which would make Request.IsHttps true and the policy safe, but KnownProxies.Clear() leaves KnownNetworks at its loopback default, so the ingress's header is dropped and the app believes it is serving plain HTTP in every deployed environment. Correcting that means deciding which proxies to trust — trusting X-Forwarded-Proto from anywhere lets a client claim HTTPS — and that is a security decision in its own right, not something to settle inside a header change. So the cookie keeps its default here and the reason is recorded where the next person will look for it. The other five items in this change are unaffected: verified in the same Review-over-HTTP container that all four headers are still applied, TRACE is still refused, and the server banner is still suppressed. Refs #347
# Conflicts: # src/DfE.CheckPerformanceData.Web/Startup/CoreWebExtensions.cs
|
Review app for PR 351 was deleted |
paulc1983
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #349, which is on
mainand is currently returning 500 on everypage in every non-Development environment. Refs #347.
This reverts one line of #349. The other five items in that change are
untouched and still work.
What is broken
#349 set
AntiforgeryOptions.Cookie.SecurePolicytoAlwaysoutsidedevelopment, as the apparent counterpart to the session cookie, which already
does exactly that.
The session cookie system only marks the cookie. The antiforgery system does
not:
DefaultAntiforgery.CheckSSLConfigthrows when the policy isAlwaysand the request is not HTTPS.
_Layout.cshtmlmints an antiforgery token onevery page render, so every page throws.
System.InvalidOperationException: The antiforgery system has the configuration
value AntiforgeryOptions.Cookie.SecurePolicy = Always, but the current request
is not an SSL request.
at Microsoft.AspNetCore.Antiforgery.DefaultAntiforgery.CheckSSLConfig(HttpContext context)
at Microsoft.AspNetCore.Antiforgery.DefaultAntiforgery.GetAndStoreTokens(HttpContext httpContext)
at AspNetCoreGeneratedDocument.Views_Shared__Layout ... _Layout.cshtml:line 58
Deployed pods sit behind a TLS-terminating ingress and receive plain HTTP, so
Request.IsHttpsisfalseand the assertion fires on every request./healthcheckstill returns 200, because it is mapped before thatmiddleware. Health probes therefore report the service as fine while every page
is dead — worth knowing when checking whether an environment is affected.
Measured, not inferred
The same container image, run with
ASPNETCORE_ENVIRONMENT=Reviewover plainHTTP, on each tree:
//healthcheckmainWhy this is a revert and not a fix
UseForwardedHeaders()is already wired withXForwardedProto, which wouldmake
Request.IsHttpstrue behind the ingress and theAlwayspolicy correctand safe. It has no effect here because
CoreWebExtensionscallsoptions.KnownProxies.Clear()while leavingKnownNetworksat its loopbackdefault, so the middleware ignores the ingress's forwarded headers and the
application believes it is serving plain HTTP in every deployed environment.
Correcting that means choosing which proxies to trust — accepting
X-Forwarded-Protofrom anywhere lets a client claim HTTPS — which is asecurity decision in its own right and not one to settle inside a hotfix. It is
also a pre-existing condition: HSTS and the HTTPS-redirect logic are working
from the same false premise today, independently of #349.
So the cookie returns to the framework default, and the reason is recorded in a
comment at the point where the next person will reach for
Alwaysagain.What is unaffected
The five other items from #349 remain in place, verified in the same
Review-over-HTTP container:
X-Content-Type-Options,Referrer-Policy,Permissions-Policyand the content-security policy are all applied (4/4),TRACEis refused with 405, and Kestrel does not advertise itself.Testing
Against the branch merged with current
main:Category!=VisualRegression)The five E2E failures are
IncorrectGradeEnquiryTests, which fail the same wayon
mainin this environment and pass in CI — unrelated to this change.Also confirmed across two teardown and relaunch cycles, one of them a cold start
with volumes destroyed: the application serves 200, the worker starts with no
restarts, and the headers and
TRACEbehaviour hold each time.Follow-up worth raising separately
The forwarded-headers trust boundary. Until
KnownNetworksis addressed,Request.IsHttpsis false in every deployed environment, which blocks securingthis cookie properly and means the HSTS and HTTPS-redirect logic are operating
on a false premise.