From cafea186519a202ac63c42d62ac24a7242916e37 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 9 Oct 2026 09:19:28 -0400 Subject: [PATCH] Refuse protocol-relative notification paths and require an observed tap PUSH-01: appPath now rejects a path that resolves to a leading // after the URL parser handles dot segments (also %2e), backslashes and stripped tabs and newlines, and planClick throws if the final URL is off origin. Regression tests drive openNotification with the attack forms. PUSH-02: subscribePush no longer asks for notification permission where navigator.userActivation is unavailable; it throws a typed PermissionGestureError ('unobservable', or 'inactive' outside a tap). The test for that path now overrides userActivation directly: the old one passed undefined into a defaulted parameter and so never exercised it. Co-Authored-By: Claude Opus 5.5 --- web/src/push/notification.test.ts | 40 +++++++++++++++++++++++++++++++ web/src/push/notification.ts | 16 +++++++++---- web/src/push/subscribe.test.ts | 27 +++++++++++++++------ web/src/push/subscribe.ts | 28 +++++++++++++++++----- 4 files changed, 94 insertions(+), 17 deletions(-) diff --git a/web/src/push/notification.test.ts b/web/src/push/notification.test.ts index 4a3e968..2c432ae 100644 --- a/web/src/push/notification.test.ts +++ b/web/src/push/notification.test.ts @@ -33,6 +33,21 @@ describe('appPath', () => { it('resolves dot segments and keeps the result on origin', () => { expect(appPath('/plan/../discover', origin)).toBe('/discover') }) + + // Each of these starts with a single / but resolves to a path starting with + // //, which a browser reads as another host. + it.each([ + '/..//evil.example/path', + '/plan/..//evil.example/path', + '/..\\\\evil.example/path', + '/./\\evil.example', + '/.\\/evil.example', + '/%2e%2e//evil.example/path', + '/.\t.//evil.example/path', + '/..\n//evil.example', + ])('refuses %j, which resolves to a protocol-relative path', (raw) => { + expect(appPath(raw, origin)).toBeNull() + }) }) describe('parsePushPayload', () => { @@ -120,6 +135,10 @@ describe('clickPath', () => { }) describe('planClick', () => { + it('refuses a path that resolves off origin', () => { + expect(() => planClick([], '//evil.example/path', origin)).toThrow(/resolves off origin/) + }) + const at = (path: string, focused = false) => ({ url: origin + path, focused }) it('opens a new window when no MealQuest window is open', () => { @@ -188,4 +207,25 @@ describe('openNotification', () => { await openNotification({ url: 'https://evil.example/' }, all, origin) expect(all.openWindow).toHaveBeenCalledWith(`${origin}/`) }) + + it.each(['/..//evil.example/path', '/..\\\\evil.example/path', '/%2e%2e//evil.example/path', '/.\t.//evil.example/path'])( + 'never leaves origin for %j, which resolves to a protocol-relative path', + async (url) => { + const win = window('/plan', true) + const all = clients([win]) + await openNotification({ url }, all, origin) + expect(win.navigate).toHaveBeenCalledWith(`${origin}/`) + expect(all.openWindow).not.toHaveBeenCalled() + + const none = clients([]) + await openNotification({ url }, none, origin) + expect(none.openWindow).toHaveBeenCalledWith(`${origin}/`) + }, + ) + + it('still opens an internal path that uses dot segments', async () => { + const all = clients([]) + await openNotification({ url: '/plan/../discover' }, all, origin) + expect(all.openWindow).toHaveBeenCalledWith(`${origin}/discover`) + }) }) diff --git a/web/src/push/notification.ts b/web/src/push/notification.ts index d7b63ac..b86c9c4 100644 --- a/web/src/push/notification.ts +++ b/web/src/push/notification.ts @@ -31,7 +31,10 @@ function isRecord(value: unknown): value is Record { // appPath returns raw as a path on origin (path, query and fragment), or null // when raw is not a path-absolute URL that stays on origin. A payload never -// sends the household to another site. +// sends the household to another site. The path is checked after the parser +// has resolved it: dot segments (also as %2e), backslashes and stripped tabs +// and newlines can all turn an innocent-looking path into one starting with +// //, which a browser would read as another host. export function appPath(raw: string, origin: string): string | null { if (!raw.startsWith('/') || raw.startsWith('//')) return null let url: URL @@ -40,7 +43,7 @@ export function appPath(raw: string, origin: string): string | null { } catch { return null } - if (url.origin !== origin) return null + if (url.origin !== origin || url.pathname.startsWith('//')) return null return url.pathname + url.search + url.hash } @@ -130,9 +133,14 @@ export type ClickPlan = { action: 'focus'; window: W; url: string; navigate: // planClick decides a tap on a notification that opens path: the focused // MealQuest window if there is one, else the first open one, navigated to -// path unless it is already there; with none open, a new window. +// path unless it is already there; with none open, a new window. A path that +// resolves off origin is an error: a tap never leaves MealQuest. export function planClick(windows: readonly W[], path: string, origin: string): ClickPlan { - const url = new URL(path, origin).href + const resolved = new URL(path, origin) + if (resolved.origin !== origin) { + throw new Error(`MealQuest: notification path ${JSON.stringify(path)} resolves off origin to ${resolved.origin}`) + } + const url = resolved.href const ours = windows.filter((w) => URL.canParse(w.url) && new URL(w.url).origin === origin) const target = ours.find((w) => w.focused) ?? ours[0] if (target === undefined) return { action: 'open', url } diff --git a/web/src/push/subscribe.test.ts b/web/src/push/subscribe.test.ts index 0870725..5d26de3 100644 --- a/web/src/push/subscribe.test.ts +++ b/web/src/push/subscribe.test.ts @@ -5,6 +5,7 @@ import { VAPID_PUBLIC_KEY_PATH, decodeVapidKey, encodeBase64url, + PermissionGestureError, subscribePush, subscriptionBody, unsubscribePush, @@ -36,7 +37,7 @@ function subscription(key: ArrayBuffer | null = vapidKey.buffer, at = endpoint) type Setup = { permission?: NotificationPermission answer?: NotificationPermission - active?: boolean | undefined + active?: boolean current?: BrowserPushSubscription | null registered?: boolean subscribeError?: unknown @@ -63,7 +64,7 @@ function setup({ serviceWorker: { getRegistration: vi.fn(async () => (registered ? { pushManager } : undefined)) }, pushManager: true, notification, - userActivation: active === undefined ? undefined : { isActive: active }, + userActivation: { isActive: active }, userAgent: 'Mozilla/5.0 Test', } const calls: { path: string; request?: ApiRequest }[] = [] @@ -155,14 +156,26 @@ describe('subscribePush', () => { it('refuses to ask for permission outside a tap', async () => { const { env, api, notification } = setup({ active: false }) - await expect(subscribePush(api, env)).rejects.toThrow(/only be asked for from a tap/) + const error = await subscribePush(api, env).catch((e: unknown) => e) + expect(error).toBeInstanceOf(PermissionGestureError) + expect(error).toMatchObject({ reason: 'inactive', message: expect.stringMatching(/only be asked for from a tap/) }) expect(notification.requestPermission).not.toHaveBeenCalled() + expect(api.apiFetch).not.toHaveBeenCalled() }) - it('asks where the browser cannot say whether a tap is active', async () => { - const { env, api, notification } = setup({ active: undefined }) - expect(await subscribePush(api, env)).toBe('subscribed') - expect(notification.requestPermission).toHaveBeenCalled() + it('refuses to ask for permission where the browser cannot say whether a tap is active', async () => { + const { env, api, notification } = setup() + const error = await subscribePush(api, { ...env, userActivation: undefined }).catch((e: unknown) => e) + expect(error).toBeInstanceOf(PermissionGestureError) + expect(error).toMatchObject({ reason: 'unobservable', message: expect.stringMatching(/no navigator.userActivation/) }) + expect(notification.requestPermission).not.toHaveBeenCalled() + expect(api.apiFetch).not.toHaveBeenCalled() + }) + + it('does not need to observe a tap once permission is granted', async () => { + const { env, api, notification } = setup({ permission: 'granted' }) + expect(await subscribePush(api, { ...env, userActivation: undefined })).toBe('subscribed') + expect(notification.requestPermission).not.toHaveBeenCalled() }) it('does not ask again once permission is granted, even outside a tap', async () => { diff --git a/web/src/push/subscribe.ts b/web/src/push/subscribe.ts index 6b7349f..7bd182e 100644 --- a/web/src/push/subscribe.ts +++ b/web/src/push/subscribe.ts @@ -58,6 +58,23 @@ export function browserPushEnvironment(): PushEnvironment { type PushApi = Pick +// PermissionGestureError is why subscribePush refused to ask for notification +// permission: no tap is active right now ('inactive'), or the browser cannot +// say whether one is ('unobservable', no navigator.userActivation). +export class PermissionGestureError extends Error { + readonly reason: 'inactive' | 'unobservable' + + constructor(reason: 'inactive' | 'unobservable') { + super( + reason === 'inactive' + ? 'subscribePush: notification permission can only be asked for from a tap on a button' + : 'subscribePush: this browser cannot report whether a tap is active (no navigator.userActivation), so notification permission is not asked for', + ) + this.name = 'PermissionGestureError' + this.reason = reason + } +} + // encodeBase64url encodes bytes as unpadded base64url, as the server wants // subscription keys. export function encodeBase64url(bytes: ArrayBuffer): string { @@ -133,7 +150,9 @@ function errorName(error: unknown): string | undefined { // subscribePush turns on Web Push on this device. Call it only from the // household's tap on a button: when the browser has not yet been asked, it // asks for notification permission, which browsers allow only during a user -// gesture, and it refuses to ask outside one. Permission is asked before +// gesture, and it refuses to ask outside one — or where the browser cannot +// say whether a gesture is active — with a PermissionGestureError. Permission +// is asked before // anything is awaited so the gesture still counts. With permission, it // subscribes the browser with the server's VAPID public key — replacing a // subscription made with an older key — and stores the subscription on the @@ -144,11 +163,8 @@ export async function subscribePush(api: PushApi, env: PushEnvironment): Promise let permission = notification.permission if (permission === 'default') { - // Where the browser cannot say whether a gesture is active, it enforces - // the rule itself. - if (env.userActivation !== undefined && !env.userActivation.isActive) { - throw new Error('subscribePush: notification permission can only be asked for from a tap on a button') - } + if (env.userActivation === undefined) throw new PermissionGestureError('unobservable') + if (!env.userActivation.isActive) throw new PermissionGestureError('inactive') permission = await notification.requestPermission() } if (permission === 'denied') return 'denied'