Unify security headers so CSP/HSTS aren't middleware-only (#368) - #548
Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Open
Conversation
…nly (Northgate-Systems#368) security-edge.ts's applySecurityHeaders() (the one wired into middleware.ts) was missing Content-Security-Policy and Strict-Transport-Security entirely - those two only existed as a second, hand-copied header list in next.config.ts. Confirmed live (next dev + curl) that next.config.ts's headers() does cover every response including 401/403/500/404s, so nothing was actually missing in practice, but the two lists had already drifted: next.config.ts had X-Frame-Options: SAMEORIGIN and Cross-Origin-Opener-Policy: same-origin-allow-popups while security-edge.ts used the stricter DENY / same-origin - two different policies for the same site depending on whether the request path was covered by the middleware matcher (config.matcher in middleware.ts excludes _next/static, images, .css/.js, etc.) or fell through to next.config.ts alone. - security-edge.ts now exports SECURITY_HEADERS, a single array with all 7 headers (including the CSP and HSTS that were missing) and is the one place applySecurityHeaders() and next.config.ts both read from. - next.config.ts imports SECURITY_HEADERS instead of keeping its own copy, so a static asset that skips the middleware now gets the exact same (stricter) values instead of the old weaker ones. - Removed the dead, unused, already-diverged duplicate of applySecurityHeaders/isAllowedOrigin/ALLOWED_ORIGINS in security.ts (never imported anywhere, confirmed via grep) - it disagreed with security-edge.ts on the CORS allowlist (missing the Vercel preview domain, null-origin handling flipped) and was exactly the kind of copy that caused this bug in the first place. - Added src/lib/__tests__/security-edge.test.ts covering the 5 headers named in the issue, header-value consistency, and isAllowedOrigin. Verified against the documented isValidStellarPublicKey / PR Northgate-Systems#529 baseline: temporarily restored the missing import in validations.ts to run the suite (4 failed/2 passed files, 10 tests baseline -> 6 passed/1 failed files, 77 tests with these changes; the 2 failures are a pre-existing bad test fixture address unrelated to this change), then reverted the import before this commit (git diff on validations.ts is empty).
|
Someone is attempting to deploy a commit to the codex723's projects Team on Vercel. A member of the Team first needs to authorize it. |
6 tasks
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
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.
Closes #368.
What was actually wrong
security-edge.ts'sapplySecurityHeaders()— the one wired intomiddleware.ts— was missingContent-Security-PolicyandStrict-Transport-Securityentirely. Those two only existed in a second, hand-copied header list innext.config.ts.I checked live (ran
next dev, curled/,/login,/dashboard,/api/health, a 401, a 500, a 404, and/favicon.ico) whether anything was actually missing from a real response.next.config.ts'sheaders()config covers every path (source: "/(.*)"), so in practice all 5 headers named in the issue were present everywhere — but the two lists had already drifted apart:next.config.ts:X-Frame-Options: SAMEORIGIN,Cross-Origin-Opener-Policy: same-origin-allow-popupssecurity-edge.ts:X-Frame-Options: DENY,Cross-Origin-Opener-Policy: same-originSo the site was actually running two different header policies depending on whether the request path was covered by the middleware matcher (
config.matcherinmiddleware.tsexcludes_next/static, images,.css/.js, etc.) or fell through tonext.config.tsalone — a static asset got the weaker values, everything else got the stricter ones set later by middleware. That divergence is exactly the kind of gap "not just some routes" is pointing at, and it would only get worse as either file gets edited independently in the future.Fix
security-edge.tsnow exportsSECURITY_HEADERS, a single array with all 7 headers (including the previously-missing CSP and HSTS), andapplySecurityHeaders()just iterates it.next.config.tsimportsSECURITY_HEADERSinstead of keeping its own copy, so excluded/static paths now get the exact same (stricter) values as everything else.applySecurityHeaders/isAllowedOrigin/ALLOWED_ORIGINSinsecurity.ts— confirmed via grep it's never imported anywhere, and it disagreed withsecurity-edge.tson the CORS allowlist (missing the Vercel preview domain, opposite null-origin handling). Leaving it in place is exactly what let the header values drift apart in the first place.src/lib/__tests__/security-edge.test.ts: all 5 required headers present, no duplicate keys, correct CSPdefault-src, strictX-Frame-Options,applySecurityHeadersapplies every entry (not a hand-picked subset), andisAllowedOrigin/ALLOWED_ORIGINSstay in sync.Verification
npx tsc --noEmit— no new errors (pre-existing unrelated errors only: the trackedisValidStellarPublicKeymissing-import bug in fix(validations): restore broken isValidStellarPublicKey import; feat: add /api/stellar/fee-estimate #529, and a pre-existing type mismatch in a transactions test).git diffonvalidations.tsis empty in this PR):git stash, unmodifiedmain): 4 failed / 2 passed test files, 10 tests.validations.test.ts(stellarSendSchema), unrelated to this change.npx eslinton all touched files — clean.next devbefore and after: confirmedX-Frame-OptionsandCross-Origin-Opener-Policynow match exactly between a middleware-covered route (/) and a middleware-excluded static path (/favicon.ico), both now also carrying full CSP + HSTS./claim