fix(core): apply baseline security headers set-if-absent so host-set values win (#1393)#1558
Conversation
…values win (emdash-cms#1393) finalizeResponse() unconditionally set X-Content-Type-Options, Referrer-Policy, and Permissions-Policy on every response. Because the middleware registers with order: 'pre' (emdash-cms#1282), on the response path it runs after the host app's own middleware, so it overwrote any of these headers the host had already set on its public routes -- letting the CMS dictate the security headers of the entire host site. Apply the three set-if-absent (matching the existing CSP guard) so a host that sets a stricter value on its own routes wins; EmDash still provides baseline defaults when the host sets nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 216f513 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/core
size/L
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach: This is the right fix for #1393. EmDash's middleware registers order: 'pre', so finalizeResponse runs on the response path after the host app's own middleware and was unconditionally overwriting host-set X-Content-Type-Options / Referrer-Policy / Permissions-Policy on every public route — letting the CMS dictate the host site's security headers. Switching these three to set-if-absent (if (!res.headers.has(name))), matching the existing guard pattern in the same function, is the correct, minimal remedy: a host that sets a stricter value wins, and EmDash still supplies its baseline when the host sets nothing. This is a bug fix (not a feature), so no prior Discussion is required, and the approach fits EmDash's conventions.
What I checked:
- Traced
finalizeResponseand confirmed it is the single chokepoint applied to every response path (plugin fast-route, anonymous public, no-config, anddoInit/renderAndFinalize), so the change is uniform — no path skips it or double-applies. Headers.has()/set()are case-insensitive/normalizing, so no case-sensitivity bug.- Admin/API routes still receive the three headers:
auth.tsonly setsContent-Security-Policy(still unconditional, untouched by this PR), not these three, so they remain absent and get set. The admin's framing protection (CSPframe-ancestors 'none') is unaffected. - The two other call sites that set
X-Content-Type-Options: nosniff(routes/api/media/file/[...key].ts,routes/api/admin/plugins/registry/artifact.ts) use the identical value and also set their own CSP, so set-if-absent changes nothing for them. - No existing test asserted the old clobbering behavior, so nothing regresses.
- Read the new test: its
vi.hoisted/MOCK_RUNTIMEmock shape matches the establishedmiddleware-prerender.test.tspattern; it genuinely reachesfinalizeResponse(anonymous fast path →runAnon→finalizeResponse, withcreateRequestScopedDbmocked tonulland the setup probe mocked to succeed), and it would fail without the fix (the first case's host headers would be overwritten). Both branches — host-set preserved, and baseline applied when host sets none — are covered. Changeset is present.
Conclusion: The implementation is clean and correct, and the regression test is real. Approving — two minor, non-blocking suggestions below: a changeset-format nit (commit-message title line) and an observation that the same clobbering pattern still exists for X-Frame-Options when a host sets it without CSP, which is worth a small follow-up for consistency but is correctly out of scope here.
| "emdash": patch | ||
| --- | ||
|
|
||
| fix(core): apply baseline security headers set-if-absent so host-set values win (#1393) |
There was a problem hiding this comment.
[suggestion] This entry opens with a fix(core): … commit-message title line. The other changesets in .changeset/ (session-get-hang.md, layout-prefetch.md, forward-plugin-blocks.md) start directly with release-note prose after the frontmatter, and AGENTS.md reminds that a changeset is "not a commit message" — the body below is fine, just the title line is the wrong shape. Fold the issue ref into a present-tense prose lead instead.
| fix(core): apply baseline security headers set-if-absent so host-set values win (#1393) | |
| Fixes EmDash overwriting host-set `X-Content-Type-Options`, `Referrer-Policy`, and `Permissions-Policy` on public routes (#1393). |
| if (!res.headers.has("Content-Security-Policy")) { | ||
| res.headers.set("X-Frame-Options", "SAMEORIGIN"); |
There was a problem hiding this comment.
[suggestion] This block still sets X-Frame-Options unconditionally whenever CSP is absent. Since finalizeResponse runs after host middleware on the response path, a host that sets a stricter X-Frame-Options (e.g. DENY) on a public route — without also setting CSP — gets clobbered with SAMEORIGIN, the same class of bug this PR fixes for the other three headers, and an actual downgrade. The CSP-gating is intentional (a CSP frame-ancestors directive supersedes X-Frame-Options), so only the host-value case needs an additional guard. Out of scope for #1393, but a cheap consistency follow-up.
| if (!res.headers.has("Content-Security-Policy")) { | |
| res.headers.set("X-Frame-Options", "SAMEORIGIN"); | |
| if (!res.headers.has("Content-Security-Policy") && !res.headers.has("X-Frame-Options")) { | |
| res.headers.set("X-Frame-Options", "SAMEORIGIN"); |
github-actions
Bot
added
review/approved
…elease note Per CONTRIBUTING.md, lead with the observable effect and drop the commit-message title line and internal mechanics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
1 participant
What does this PR do?
finalizeResponse()unconditionally setX-Content-Type-Options,Referrer-Policy, andPermissions-Policyon every response. Because the middleware registers withorder: 'pre'(#1282), on the response path it runs after the host app's own middleware, so it overwrote any of these headers the host had already set on its public routes — letting the CMS dictate the security headers of the entire host site (the reporter's host sets a stricterPermissions-Policy).These three headers are now applied set-if-absent (
if (!res.headers.has(name))), matching the existingContent-Security-Policyguard in the same function, so a host that sets a stricter value on its own routes wins. EmDash still provides its baseline defaults when the host sets nothing.Closes #1393
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (targeted: new + existing astro middleware suites)pnpm formathas been runAI-generated code disclosure