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 <noreply@anthropic.com>
This commit is contained in:
@@ -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`)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -31,7 +31,10 @@ function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
|
||||
// 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<W> = { 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<W extends AppWindow>(windows: readonly W[], path: string, origin: string): ClickPlan<W> {
|
||||
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 }
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -58,6 +58,23 @@ export function browserPushEnvironment(): PushEnvironment {
|
||||
|
||||
type PushApi = Pick<ApiClient, 'apiFetch'>
|
||||
|
||||
// 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'
|
||||
|
||||
Reference in New Issue
Block a user