From da32e49b0b77d000ac98603952377220cfc3467a Mon Sep 17 00:00:00 2001 From: Kevin Riehl Date: Sat, 12 Sep 2026 10:22:14 -0700 Subject: [PATCH] fix: push subscription payload; CSP hash computed from the shipped page (2.2.3) Enabling push in production failed with "Validation failed": the browser's PushSubscription.toJSON() carries expirationTime, and it was sent verbatim to a schema that is strict about unknown fields. The client now sends exactly endpoint + keys. The test doubles had been shaped like the schema rather than like a browser, which is why 1,112 tests said nothing. The CSP hash for index.html's inline theme script was hardcoded in the container's start-up script behind a comment asking for it to be regenerated by hand, and claiming a test asserted it; none did. The bytes are what the browser hashes, so an edit or a checkout with different line endings would have silently blocked the page's own script. The hash is now emitted by `npm run build` from the index.html that is actually shipped, copied into the image, and read at start; the container refuses to start without it. Verified against the built image: header hash equals served-script hash. For the record: a CSP console message in production is what led here, and the served page turned out to be correct - that message was a browser extension's injected script, which the policy is right to refuse. --- .gitignore | 1 + CHANGELOG.md | 31 ++++++++ Dockerfile | 4 + README.md | 2 +- docker/40-render-config.sh | 20 ++++- package-lock.json | 4 +- package.json | 4 +- scripts/csp-inline-script-hashes.mjs | 69 +++++++++++++++++ scripts/csp-inline-script-hashes.test.ts | 66 +++++++++++++++++ src/api/push.test.ts | 94 ++++++++++++++++++++++++ src/api/push.ts | 39 ++++++++-- src/lib/push.test.ts | 17 +++-- vite.config.ts | 2 +- 13 files changed, 333 insertions(+), 20 deletions(-) create mode 100644 scripts/csp-inline-script-hashes.mjs create mode 100644 scripts/csp-inline-script-hashes.test.ts create mode 100644 src/api/push.test.ts diff --git a/.gitignore b/.gitignore index 9a3d73a..323bf54 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ lerna-debug.log* node_modules dist dist-ssr +build *.local # Editor directories and files diff --git a/CHANGELOG.md b/CHANGELOG.md index 02bb6b5..212ce56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.2.3] - 2026-09-12 + +The first production deploy of the push opt-in. + +### Fixed + +- **Enabling push notifications no longer fails with "Validation failed".** + The browser's `PushSubscription.toJSON()` includes `expirationTime`, and it + was being sent verbatim to a server schema that is deliberately strict about + unknown fields. The client now sends exactly what the server accepts - + `endpoint` and `keys` - and refuses an incomplete subscription rather than + sending it. (The server also accepts the field now; either side alone fixes + it.) Nothing had ever called this path before 2.2.0, and the test doubles + were shaped like the schema rather than like a browser. + +### Changed + +- **The CSP hash for the inline theme script is computed at build time** from + the `index.html` that is actually shipped, copied into the image and read at + start, instead of being hardcoded in the container's start-up script behind + a comment asking for it to be regenerated by hand. The bytes are what the + browser hashes, so an edit to the script - or a checkout with different line + endings - would have silently blocked the page's own theme script. The + container now refuses to start without the file rather than serve a policy + that blocks its own page. Verified against the built image: the header's + hash equals the hash of the served script. (Investigating a CSP console + message in production is what surfaced this; the served page turned out to + be correct, and that message came from a browser extension's injected + script, which the policy is right to refuse.) +- Every text file is checked out with LF on every platform (`.gitattributes`). + ## [2.2.2] - 2026-09-12 Found by playing the app end to end against a clean-install development stack diff --git a/Dockerfile b/Dockerfile index 1888299..144c9dc 100644 --- a/Dockerfile +++ b/Dockerfile @@ -37,6 +37,10 @@ USER root RUN apk upgrade --no-cache COPY --from=build /app/dist /usr/share/nginx/html +# The CSP hash tokens for index.html's inline scripts, produced by `npm run +# build` from the shipped file. Outside the web root: it belongs to the +# server configuration, not the site. +COPY --from=build /app/build/csp-inline-script-hashes /etc/nginx/csp-inline-script-hashes COPY nginx.conf /etc/nginx/conf.d/default.conf COPY docker/40-render-config.sh /docker-entrypoint.d/40-render-config.sh diff --git a/README.md b/README.md index 4464a86..8f968df 100644 --- a/README.md +++ b/README.md @@ -113,7 +113,7 @@ Opt-in from **Profile → Notifications**. The service worker is registered at s ## Production image -`Dockerfile` has three stages: `dev` (Vite), `build`, and `production` — `nginxinc/nginx-unprivileged` serving `dist/` on **port 8080** as uid 101. `docker/40-render-config.sh` runs at container start to write `/config.js` and the security-header snippet (CSP with the API origin in `connect-src`, HSTS, frame and MIME protections) from `API_URL`. `nginx.conf` repeats the headers in every `location`, since nginx's `add_header` is not inherited. +`Dockerfile` has three stages: `dev` (Vite), `build`, and `production` — `nginxinc/nginx-unprivileged` serving `dist/` on **port 8080** as uid 101. `docker/40-render-config.sh` runs at container start to write `/config.js` and the security-header snippet (CSP with the API origin in `connect-src`, HSTS, frame and MIME protections) from `API_URL`. The CSP allows `index.html`'s inline theme script by hash; `npm run build` computes that hash from the built file (`scripts/csp-inline-script-hashes.mjs`) and the image carries it, so editing the script needs no manual step. `nginx.conf` repeats the headers in every `location`, since nginx's `add_header` is not inherited. ## Releasing diff --git a/docker/40-render-config.sh b/docker/40-render-config.sh index c9b047f..55bba16 100644 --- a/docker/40-render-config.sh +++ b/docker/40-render-config.sh @@ -55,9 +55,20 @@ connect_src="'self' ${api_origin} ${ws_origin}" # 'unsafe-inline' out of the policy - with 'unsafe-inline' a CSP buys almost # nothing against injected script, which is the main thing it is here to stop. # -# If that script is edited, this hash must be regenerated or the page will not -# apply its theme. It is asserted by the header test in the repo. -theme_script_hash="sha256-8evBcs4UKW13abYRRwTcDjBEK5h9IWhKnpEbZ+Mv4nQ=" +# The hash tokens are computed at build time from the index.html that is +# actually shipped (scripts/csp-inline-script-hashes.mjs, run by `npm run +# build`) and copied into the image by the Dockerfile. A hardcoded value used +# to live here, guarded by a comment asking for it to be regenerated by hand +# when the script changed; the bytes are what the browser hashes, so an edit +# or a different line ending would have silently blocked the page's own +# script. If the file is missing the build is broken, and refusing to start is +# better than serving a CSP that blocks the page. +hashes_file=/etc/nginx/csp-inline-script-hashes +if [ ! -s "$hashes_file" ]; then + echo "[config] ERROR: $hashes_file is missing or empty; the image was built without the CSP hash step" >&2 + exit 1 +fi +script_src_hashes=$(tr -d '\n\r' < "$hashes_file") mkdir -p /etc/nginx/snippets cat > /etc/nginx/snippets/security-headers.conf < /config.js" echo "[config] CSP connect-src: ${connect_src}" +echo "[config] CSP script-src hashes: ${script_src_hashes}" diff --git a/package-lock.json b/package-lock.json index f81b64d..a129887 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "movie-loop-frontend", - "version": "2.2.2", + "version": "2.2.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "movie-loop-frontend", - "version": "2.2.2", + "version": "2.2.3", "license": "MIT", "dependencies": { "@base-ui/react": "^1.2.0", diff --git a/package.json b/package.json index 8c89560..d9fcddf 100644 --- a/package.json +++ b/package.json @@ -1,13 +1,13 @@ { "name": "movie-loop-frontend", "private": true, - "version": "2.2.2", + "version": "2.2.3", "type": "module", "license": "MIT", "author": "Kevin Riehl", "scripts": { "dev": "vite", - "build": "tsc -b && vite build", + "build": "tsc -b && vite build && node scripts/csp-inline-script-hashes.mjs", "lint": "eslint . --max-warnings 0", "test": "vitest run", "test:watch": "vitest", diff --git a/scripts/csp-inline-script-hashes.mjs b/scripts/csp-inline-script-hashes.mjs new file mode 100644 index 0000000..0dc7830 --- /dev/null +++ b/scripts/csp-inline-script-hashes.mjs @@ -0,0 +1,69 @@ +// Emits the CSP `script-src` hash tokens for every inline script in the built +// index.html, so the container's Content-Security-Policy can allow exactly +// those scripts and nothing else. +// +// The theme bootstrap script is inline on purpose: it runs before first paint +// so the page does not flash the wrong theme, and moving it to a file would +// mean either a blocking request or the flash coming back. Allowing it by +// hash keeps 'unsafe-inline' out of the policy, which is the whole point of +// having one. +// +// The hash used to be hardcoded in docker/40-render-config.sh, with a comment +// saying it had to be regenerated by hand whenever the script changed and that +// a test asserted it. No test did. Any edit to the script - or a checkout with +// different line endings, since the bytes are what is hashed - would have +// silently blocked the page's own theme script, with nothing but a browser +// console message to show for it. Computing it here, from the bytes that are +// actually shipped, removes every way it could drift. +// +// Usage: node scripts/csp-inline-script-hashes.mjs [distDir] [outFile] +// default: dist -> build/csp-inline-script-hashes +// Output: one line, e.g. 'sha256-…' 'sha256-…', ready to splice into script-src. + +import { createHash } from 'node:crypto'; +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * The content of every ` + + + +`; + expect(inlineScripts(html)).toEqual([ + "\n (function(){ document.documentElement.classList.add('dark'); })();\n", + 'console.log(1)', + ]); + }); + + it('is empty for a page with no inline scripts', () => { + expect(inlineScripts('')).toEqual([]); + }); +}); + +describe('cspHashToken', () => { + it("produces the 'sha256-' token the browser expects", () => { + expect(cspHashToken('x')).toBe(sha('x')); + }); + + it('is sensitive to line endings - one of the ways a hardcoded value could drift', () => { + expect(cspHashToken('a\nb')).not.toBe(cspHashToken('a\r\nb')); + }); +}); + +describe('cspInlineScriptHashes', () => { + it('joins one token per inline script, in document order', () => { + const html = ''; + expect(cspInlineScriptHashes(html)).toBe(`${sha('one')} ${sha('two')}`); + }); + + it("matches the repo's own index.html theme script", () => { + // Whatever the checkout's line endings, the token must be the hash of the + // bytes as they are; that is what the browser hashes. + const html = readFileSync('index.html', 'utf8'); + const [body] = inlineScripts(html); + expect(body).toContain("localStorage.getItem('theme')"); + expect(cspInlineScriptHashes(html)).toBe(sha(body ?? '')); + }); +}); diff --git a/src/api/push.test.ts b/src/api/push.test.ts new file mode 100644 index 0000000..4b3542f --- /dev/null +++ b/src/api/push.test.ts @@ -0,0 +1,94 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * The push API module. The important case is the payload shape: the browser's + * PushSubscription.toJSON() includes `expirationTime`, and the server schema + * is strict, so sending toJSON() verbatim was a 400 "Validation failed" - the + * first time anyone pressed the button in production. + */ + +const apiGet = vi.fn(); +const apiPost = vi.fn(); +const apiDelete = vi.fn(); +vi.mock('./client', () => ({ + default: { + get: (...a: unknown[]) => apiGet(...a), + post: (...a: unknown[]) => apiPost(...a), + delete: (...a: unknown[]) => apiDelete(...a), + }, +})); + +const push = await import('./push'); + +const browserSubscription = ( + json: Partial = {}, +): PushSubscription => + ({ + toJSON: () => ({ + endpoint: 'https://push.example/abc', + expirationTime: null, + keys: { p256dh: 'p256', auth: 'auth' }, + ...json, + }), + }) as unknown as PushSubscription; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('toPushSubscriptionPayload', () => { + it('keeps endpoint and keys and drops expirationTime', () => { + expect(push.toPushSubscriptionPayload(browserSubscription())).toEqual({ + endpoint: 'https://push.example/abc', + keys: { p256dh: 'p256', auth: 'auth' }, + }); + }); + + it('drops a non-null expirationTime too', () => { + expect( + push.toPushSubscriptionPayload( + browserSubscription({ expirationTime: 1_700_000_000_000 }), + ), + ).not.toHaveProperty('expirationTime'); + }); + + it('refuses an incomplete subscription rather than sending it', () => { + expect(() => + push.toPushSubscriptionPayload(browserSubscription({ keys: { p256dh: 'p' } })), + ).toThrow(/incomplete push subscription/); + expect(() => + push.toPushSubscriptionPayload(browserSubscription({ endpoint: undefined })), + ).toThrow(/incomplete push subscription/); + }); +}); + +describe('subscribeToPush', () => { + it('posts only the fields the server accepts', async () => { + apiPost.mockResolvedValue({ data: { id: 's1' } }); + await expect(push.subscribeToPush(browserSubscription())).resolves.toEqual({ + id: 's1', + }); + expect(apiPost).toHaveBeenCalledWith('/notifications/push/subscribe', { + endpoint: 'https://push.example/abc', + keys: { p256dh: 'p256', auth: 'auth' }, + }); + }); +}); + +describe('getVapidKey / unsubscribeFromPush', () => { + it('reads the key', async () => { + apiGet.mockResolvedValue({ data: { key: 'BAbc' } }); + await expect(push.getVapidKey()).resolves.toBe('BAbc'); + expect(apiGet).toHaveBeenCalledWith('/notifications/push/vapid-key'); + }); + + it('sends the endpoint as the delete body', async () => { + apiDelete.mockResolvedValue({ data: { success: true } }); + await expect(push.unsubscribeFromPush('https://push.example/abc')).resolves.toEqual({ + success: true, + }); + expect(apiDelete).toHaveBeenCalledWith('/notifications/push/unsubscribe', { + data: { endpoint: 'https://push.example/abc' }, + }); + }); +}); diff --git a/src/api/push.ts b/src/api/push.ts index d1ba1fb..e2242b0 100644 --- a/src/api/push.ts +++ b/src/api/push.ts @@ -1,18 +1,47 @@ import apiClient from './client'; export async function getVapidKey(): Promise { - const { data } = await apiClient.get('/notifications/push/vapid-key'); + const { data } = await apiClient.get<{ key: string | null }>( + '/notifications/push/vapid-key', + ); return data.key; } +/** + * What the server's PushSubscription schema accepts. The browser's + * `PushSubscription.toJSON()` also carries `expirationTime`, which the strict + * schema rejected with "Validation failed" - the first time push was actually + * enabled in production, since nothing had ever called it before WP-17. + */ +export interface PushSubscriptionPayload { + endpoint: string; + keys: { p256dh: string; auth: string }; +} + +export function toPushSubscriptionPayload( + subscription: PushSubscription, +): PushSubscriptionPayload { + const json = subscription.toJSON(); + const p256dh = json.keys?.p256dh; + const auth = json.keys?.auth; + if (!json.endpoint || !p256dh || !auth) { + throw new Error('The browser returned an incomplete push subscription.'); + } + return { endpoint: json.endpoint, keys: { p256dh, auth } }; +} + export async function subscribeToPush(subscription: PushSubscription) { - const { data } = await apiClient.post('/notifications/push/subscribe', subscription.toJSON()); + const { data } = await apiClient.post<{ id: string }>( + '/notifications/push/subscribe', + toPushSubscriptionPayload(subscription), + ); return data; } export async function unsubscribeFromPush(endpoint: string) { - const { data } = await apiClient.delete('/notifications/push/unsubscribe', { - data: { endpoint }, - }); + const { data } = await apiClient.delete<{ success: boolean }>( + '/notifications/push/unsubscribe', + { data: { endpoint } }, + ); return data; } diff --git a/src/lib/push.test.ts b/src/lib/push.test.ts index 05da211..43eb29f 100644 --- a/src/lib/push.test.ts +++ b/src/lib/push.test.ts @@ -50,7 +50,13 @@ function fakeSubscription(endpoint = 'https://push.example/abc'): FakeSubscripti unsubscribe.mockResolvedValue(true); return { endpoint, - toJSON: () => ({ endpoint, keys: { p256dh: 'p', auth: 'a' } }), + // What a real browser returns: expirationTime is always present (usually + // null). The server schema is strict and does not accept it. + toJSON: () => ({ + endpoint, + expirationTime: null, + keys: { p256dh: 'p', auth: 'a' }, + }), unsubscribe, }; } @@ -152,10 +158,11 @@ describe('subscribeToPush', () => { expect(options.userVisibleOnly).toBe(true); // base64url decoded: 'BAbc_-12' -> 6 bytes expect(new Uint8Array(options.applicationServerKey as ArrayBuffer)).toHaveLength(6); - expect(apiPost).toHaveBeenCalledWith( - '/notifications/push/subscribe', - sub.toJSON(), - ); + // Only what the server accepts: no expirationTime. + expect(apiPost).toHaveBeenCalledWith('/notifications/push/subscribe', { + endpoint: 'https://push.example/abc', + keys: { p256dh: 'p', auth: 'a' }, + }); }); it('reuses an existing registration instead of registering again', async () => { diff --git a/vite.config.ts b/vite.config.ts index 8e651f6..cca5e96 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -70,7 +70,7 @@ export default defineConfig({ test: { environment: 'jsdom', setupFiles: ['./src/test/setup.ts'], - include: ['src/**/*.{test,spec}.{ts,tsx}'], + include: ['src/**/*.{test,spec}.{ts,tsx}', 'scripts/**/*.test.ts'], coverage: { provider: 'v8', reporter: ['text', 'json-summary'],