rastrillo / aviso Public

Worker helper: verify the renewal's key, fall back on a refused notification

Codex's review of the worker, four findings, all real. A browser's
replacement subscription reuses the old options, so after a server
key rotation it is signed for a key the server no longer holds; saved
under the fetched key it would look valid and never receive anything.
The helper now compares and replaces it. A decoder can return options
the platform refuses (uncloneable data, say), and that escaped without
a notification; showNotification failures now fall back, while a
fallback the platform refuses is the app's bug and surfaces. External
URLs — payloads, custom decoders, fallbackURL — must be root-relative
with no on-origin absolute exception; only the absolute form a
previous handlePush stored in notification.data is reduced and
re-validated on click. The tests load the helper through an
importScripts stand-in, drive an app-style sw.js through
event.waitUntil to check the promise is handed over synchronously,
and use a matchAll fake that refuses any options but the correct ones.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 week ago da04de6417d47c8708f323a5938643c5b63ecfa8 parent 3c7b03d

2 files changed, +164 −31

js/aviso-sw.js +53 −13
js/aviso-sw.test.mjs +111 −18

diff --git a/js/aviso-sw.js b/js/aviso-sw.js

index 4708b89..f903787 100644

--- a/js/aviso-sw.js

+++ b/js/aviso-sw.js

@@ -15,15 +15,21 @@

return out;

}

+ function toBase64url(buf) {

+ let s = "";

+ const bytes = new Uint8Array(buf);

+ for (const b of bytes) s += String.fromCharCode(b);

+ return btoa(s).replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, "");

+ }

// validateURL admits only a root-relative path on the app's own

// origin: no scheme, no protocol-relative "//", no backslashes, and

// the resolved URL must still be on origin. Applied to the default

// decoder, custom decoder output and fallbackURL alike — a click

- // must never navigate off the app. An absolute URL on the origin

- // (what a previous handlePush stored) is reduced to its path first.

+ // must never navigate off the app, and an absolute URL is refused

+ // even on-origin so the rule has no exceptions to reason about.

function validateURL(raw, origin) {

if (typeof raw !== "string" || raw === "") return null;

- if (raw.indexOf(origin + "/") === 0) raw = raw.slice(origin.length);

if (raw[0] !== "/" || raw[1] === "/" || raw.indexOf("\\") !== -1) return null;

let u;

try { u = new URL(raw, origin); } catch (e) { return null; }

@@ -31,6 +37,14 @@

return u.href;

}

+ // storedURL re-validates what a previous handlePush put in

+ // notification.data — validateURL's absolute form — by reducing it

+ // to its path and running the same rule. Anything else is refused.

+ function storedURL(raw, origin) {

+ if (typeof raw !== "string" || raw.indexOf(origin + "/") !== 0) return null;

+ return validateURL(raw.slice(origin.length), origin);

+ }

// decodeDefault reads {title, body, url, tag}; title is required.

function decodeDefault(event, origin) {

if (!event.data) return null;

@@ -40,10 +54,9 @@

const options = { data: {} };

if (typeof p.body === "string") options.body = p.body;

if (typeof p.tag === "string") options.tag = p.tag;

- if (p.url !== undefined) {

- const href = validateURL(p.url, origin);

- if (href) options.data.url = href;

- }

+ // Kept root-relative here; show() is the one place that validates

+ // and resolves, for this decoder and custom ones alike.

+ if (typeof p.url === "string") options.data.url = p.url;

return { title: p.title, options };

}

@@ -51,17 +64,36 @@

// push for a worker that receives without showing, so a payload the

// decoder cannot read shows the app's fallback rather than nothing.

async function handlePush(event, opts) {

+ if (!opts || typeof opts.fallback !== "function") {

+ throw new Error("aviso: fallback is required");

+ }

const origin = root.location.origin;

let n = null;

try {

- n = opts && opts.decode ? await opts.decode(event) : decodeDefault(event, origin);

+ n = opts.decode ? await opts.decode(event) : decodeDefault(event, origin);

} catch (e) { n = null; }

- if (!n || typeof n.title !== "string" || n.title === "") {

- if (!opts || typeof opts.fallback !== "function") {

- throw new Error("aviso: fallback is required");

+ if (usable(n)) {

+ try {

+ return await show(n, origin);

+ } catch (e) {

+ // A decoder can hand back options the platform refuses —

+ // uncloneable data, an invalid combination — and that must

+ // still end in a notification.

}

- n = await opts.fallback(event);

}

+ return show(await opts.fallback(event), origin);

+ }

+ function underKey(sub, key) {

+ const k = sub.options && sub.options.applicationServerKey;

+ return !!k && toBase64url(k) === key;

+ }

+ function usable(n) {

+ return !!n && typeof n.title === "string" && n.title !== "";

+ }

+ function show(n, origin) {

const options = Object.assign({}, n.options || {});

options.data = Object.assign({}, options.data || {});

if (options.data.url !== undefined) {

@@ -77,7 +109,7 @@

const origin = root.location.origin;

event.notification.close();

const data = event.notification.data || {};

- let href = data.url ? validateURL(data.url, origin) : null;

+ let href = data.url ? storedURL(data.url, origin) : null;

if (!href && opts && opts.fallbackURL) href = validateURL(opts.fallbackURL, origin);

if (!href) return;

const all = await root.clients.matchAll({ type: "window", includeUncontrolled: true });

@@ -101,7 +133,15 @@

let key;

try { key = await opts.publicKey(); } catch (e) { return false; }

if (typeof key !== "string" || key === "") return false;

+ // The browser's replacement subscription reuses the old options,

+ // so after a server key rotation it is signed for a key the server

+ // no longer holds: saved under the fetched key it would look valid

+ // and never receive anything. Replace it rather than label it.

let sub = event.newSubscription || null;

+ if (sub && !underKey(sub, key)) {

+ try { await sub.unsubscribe(); } catch (e) { /* replaced below either way */ }

+ sub = null;

+ }

if (!sub) {

try {

sub = await root.registration.pushManager.subscribe({

diff --git a/js/aviso-sw.test.mjs b/js/aviso-sw.test.mjs

index 68c749d..88b1f38 100644

--- a/js/aviso-sw.test.mjs

+++ b/js/aviso-sw.test.mjs

@@ -1,27 +1,58 @@

import { test } from "node:test";

import assert from "node:assert/strict";

import { generateKeyPairSync } from "node:crypto";

+import { readFileSync } from "node:fs";

+import { runInThisContext } from "node:vm";

globalThis.self = globalThis;

globalThis.location = { origin: "https://app.example" };

-await import("./aviso-sw.js");

+// Load the helper the way a worker does: importScripts evaluates a

+// classic script in the global scope. vm.runInThisContext is that.

+globalThis.importScripts = (path) => runInThisContext(readFileSync(new URL(path, import.meta.url), "utf8"), { filename: path });

+importScripts("./aviso-sw.js");

const { AvisoSW } = globalThis;

-// A real P-256 point: a browser's subscribe() rejects anything else.

-const { publicKey: pk } = generateKeyPairSync("ec", { namedCurve: "prime256v1" });

-const jwk = pk.export({ format: "jwk" });

-const KEY_BYTES = Uint8Array.from([4, ...Buffer.from(jwk.x, "base64url"), ...Buffer.from(jwk.y, "base64url")]);

+// Real P-256 points: a browser's subscribe() rejects anything else.

+function realPoint() {

+ const { publicKey } = generateKeyPairSync("ec", { namedCurve: "prime256v1" });

+ const jwk = publicKey.export({ format: "jwk" });

+ return Uint8Array.from([4, ...Buffer.from(jwk.x, "base64url"), ...Buffer.from(jwk.y, "base64url")]);

+const KEY_BYTES = realPoint();

const KEY = Buffer.from(KEY_BYTES).toString("base64url");

+const OLD_KEY_BYTES = realPoint();

+const asBuffer = (u8) => u8.buffer.slice(u8.byteOffset, u8.byteOffset + u8.byteLength);

+function fakeSub(endpoint, keyBytes) {

+ return {

+ endpoint,

+ options: { applicationServerKey: asBuffer(keyBytes) },

+ unsubscribed: false,

+ toJSON() { return { endpoint: this.endpoint, expirationTime: null, keys: { p256dh: "P", auth: "A" } }; },

+ async unsubscribe() { this.unsubscribed = true; return true; },

+ };

+// The platform refuses options it cannot store: a function inside

+// data is a DataCloneError in every browser.

+function cloneable(v) {

+ if (typeof v === "function") return false;

+ if (v && typeof v === "object") return Object.values(v).every(cloneable);

+ return true;

function rig() {

const shown = [], opened = [], subscribed = [];

globalThis.registration = {

- async showNotification(title, options) { shown.push({ title, options }); },

+ async showNotification(title, options) {

+ if (!cloneable(options)) throw new Error("DataCloneError");

+ shown.push({ title, options });

+ },

pushManager: {

async subscribe(opts) {

assert.equal(opts.userVisibleOnly, true);

assert.deepEqual(Array.from(opts.applicationServerKey), Array.from(KEY_BYTES));

- const s = { endpoint: "https://push.example/renewed", toJSON() { return { endpoint: this.endpoint, expirationTime: null, keys: { p256dh: "P", auth: "A" } }; } };

+ const s = fakeSub("https://push.example/renewed", KEY_BYTES);

subscribed.push(s);

return s;

@@ -29,7 +60,12 @@ function rig() {

};

globalThis.clients = {

windows: [],

- async matchAll() { return this.windows; },

+ async matchAll(opts) {

+ // A worker that forgets includeUncontrolled misses windows it

+ // does not yet control and opens duplicates; the fake refuses.

+ assert.deepEqual(opts, { type: "window", includeUncontrolled: true });

+ return this.windows;

+ },

async openWindow(u) { opened.push(u); },

};

return { shown, opened, subscribed };

@@ -39,11 +75,10 @@ function pushEvent(payload) {

return { data: payload === undefined ? null : { json() { return JSON.parse(payload); } } };

}

-test("validateURL admits only root-relative same-origin paths", () => {

+test("validateURL admits only root-relative same-origin paths, absolute refused even on-origin", () => {

const o = "https://app.example";

assert.equal(AvisoSW.validateURL("/inbox?x=1", o), "https://app.example/inbox?x=1");

- assert.equal(AvisoSW.validateURL("https://app.example/inbox", o), "https://app.example/inbox");

- for (const bad of ["", "inbox", "//evil.example/x", "https://evil.example/x", "/a\\b", "javascript:alert(1)", "https://app.example.evil/x", 42, null]) {

+ for (const bad of ["", "inbox", "//evil.example/x", "https://evil.example/x", "https://app.example/inbox", "/a\\b", "javascript:alert(1)", "https://app.example.evil/x", 42, null]) {

assert.equal(AvisoSW.validateURL(bad, o), null, String(bad));

}

});

@@ -58,11 +93,14 @@ test("handlePush shows the default payload and validates url", async () => {

assert.equal(r.shown[0].options.data.url, "https://app.example/inbox");

});

-test("handlePush drops an off-origin url but still shows", async () => {

+test("handlePush drops an off-origin or absolute url but still shows", async () => {

const r = rig();

await AvisoSW.handlePush(pushEvent('{"title":"Hi","url":"https://evil.example/"}'), { fallback: () => ({ title: "fb" }) });

+ await AvisoSW.handlePush(pushEvent('{"title":"Hi2","url":"https://app.example/inbox"}'), { fallback: () => ({ title: "fb" }) });

assert.equal(r.shown[0].title, "Hi");

assert.equal("url" in r.shown[0].options.data, false);

+ assert.equal(r.shown[1].title, "Hi2");

+ assert.equal("url" in r.shown[1].options.data, false);

});

test("handlePush falls back on malformed or missing payload", async () => {

@@ -74,9 +112,20 @@ test("handlePush falls back on malformed or missing payload", async () => {

assert.equal(r.shown[0].options.data.url, "https://app.example/");

});

+test("handlePush falls back when the platform refuses the decoded options", async () => {

+ const r = rig();

+ await AvisoSW.handlePush(pushEvent('{"blob":"..."}'), {

+ decode: async () => ({ title: "Custom", options: { data: { url: "/x", fn() {} } } }),

+ fallback: () => ({ title: "fb" }),

+ });

+ assert.deepEqual(r.shown.map((s) => s.title), ["fb"]);

+ // A fallback the platform refuses is the app's bug: it surfaces.

+ await assert.rejects(AvisoSW.handlePush(pushEvent('{"nope":1}'), { fallback: () => ({ title: "fb", options: { data: { fn() {} } } }) }), /DataCloneError/);

+});

test("handlePush requires a fallback", async () => {

rig();

- await assert.rejects(AvisoSW.handlePush(pushEvent('{"x":1}'), {}), /fallback is required/);

+ await assert.rejects(AvisoSW.handlePush(pushEvent('{"title":"fine"}'), {}), /fallback is required/);

});

test("handlePush uses a custom decoder and validates its url; a throwing decoder falls back", async () => {

@@ -102,20 +151,22 @@ test("handleClick focuses a matching window, else opens, else fallback, never of

const ev = (url) => ({ notification: { close() { closed++; }, data: url === undefined ? {} : { url } } });

await AvisoSW.handleClick(ev("https://app.example/inbox"), {});

assert.equal(focusedURL, "https://app.example/inbox");

- await AvisoSW.handleClick(ev("/other"), {});

+ await AvisoSW.handleClick(ev("https://app.example/other"), {});

assert.deepEqual(r.opened, ["https://app.example/other"]);

await AvisoSW.handleClick(ev(undefined), { fallbackURL: "/" });

assert.deepEqual(r.opened, ["https://app.example/other", "https://app.example/"]);

await AvisoSW.handleClick(ev("https://evil.example/"), {});

+ await AvisoSW.handleClick(ev("https://app.example.evil/"), {});

await AvisoSW.handleClick(ev("https://evil.example/"), { fallbackURL: "//evil.example/" });

+ await AvisoSW.handleClick(ev(undefined), { fallbackURL: "https://app.example/abs" }); // fallbackURL is external input: relative only

assert.equal(r.opened.length, 2);

- assert.equal(closed, 5);

+ assert.equal(closed, 7);

});

-test("handleSubscriptionChange saves a new subscription with a fetched key", async () => {

+test("handleSubscriptionChange saves a new subscription under the current key", async () => {

rig();

const saved = [];

- const sub = { endpoint: "https://push.example/new", toJSON() { return { endpoint: this.endpoint, expirationTime: 1, keys: { p256dh: "P", auth: "A" } }; } };

+ const sub = fakeSub("https://push.example/new", KEY_BYTES);

const ok = await AvisoSW.handleSubscriptionChange(

{ newSubscription: sub, oldSubscription: { endpoint: "https://push.example/old" } },

{ publicKey: async () => KEY, save: async (b) => { saved.push(b); return { ok: true }; } },

@@ -128,6 +179,22 @@ test("handleSubscriptionChange saves a new subscription with a fetched key", asy

});

+test("handleSubscriptionChange replaces a renewal made under a rotated key", async () => {

+ const r = rig();

+ const saved = [];

+ const stale = fakeSub("https://push.example/stale", OLD_KEY_BYTES);

+ const ok = await AvisoSW.handleSubscriptionChange(

+ { newSubscription: stale, oldSubscription: { endpoint: "https://push.example/old" } },

+ { publicKey: async () => KEY, save: async (b) => { saved.push(b); } },

+ );

+ assert.equal(ok, true);

+ assert.equal(stale.unsubscribed, true);

+ assert.equal(r.subscribed.length, 1);

+ assert.equal(saved[0].subscription.endpoint, "https://push.example/renewed");

+ assert.equal(saved[0].publicKey, KEY);

+ assert.equal(saved[0].previousEndpoint, "https://push.example/old");

+});

test("handleSubscriptionChange renews itself when the event carries no subscription", async () => {

const r = rig();

const saved = [];

@@ -140,9 +207,35 @@ test("handleSubscriptionChange renews itself when the event carries no subscript

test("handleSubscriptionChange fails silently on an expired session, a failed key fetch, or a throw", async () => {

rig();

- const sub = { endpoint: "e", toJSON() { return { endpoint: "e", keys: {} }; } };

+ const sub = fakeSub("e", KEY_BYTES);

assert.equal(await AvisoSW.handleSubscriptionChange({ newSubscription: sub }, { publicKey: async () => KEY, save: async () => ({ ok: false, status: 401 }) }), false);

assert.equal(await AvisoSW.handleSubscriptionChange({ newSubscription: sub }, { publicKey: async () => { throw new Error("offline"); }, save: async () => ({ ok: true }) }), false);

assert.equal(await AvisoSW.handleSubscriptionChange({ newSubscription: sub }, { publicKey: async () => KEY, save: async () => { throw new Error("net"); } }), false);

await assert.rejects(AvisoSW.handleSubscriptionChange({ newSubscription: sub }, {}), /publicKey and save are required/);

});

+// The app's sw.js, as SKILL.md shows it: listeners that hand the

+// helper's promise to event.waitUntil synchronously. A worker that

+// awaited before calling waitUntil would be terminated mid-flight.

+test("an app worker wires the helper through event.waitUntil synchronously", async () => {

+ const r = rig();

+ const listeners = {};

+ globalThis.addEventListener = (type, fn) => { listeners[type] = fn; };

+ runInThisContext(`

+ self.addEventListener("push", (e) => e.waitUntil(AvisoSW.handlePush(e, {

+ fallback: () => ({ title: "New activity", options: { data: { url: "/" } } }),

+ })));

+ self.addEventListener("notificationclick", (e) => e.waitUntil(AvisoSW.handleClick(e, { fallbackURL: "/" })));

+ `, { filename: "sw.js" });

+ let waited = null;

+ const ev = Object.assign(pushEvent('{"title":"Hi","url":"/inbox"}'), { waitUntil(p) { waited = p; } });

+ listeners.push(ev);

+ assert.ok(waited && typeof waited.then === "function", "waitUntil not called synchronously with a promise");

+ await waited;

+ assert.equal(r.shown[0].title, "Hi");

+ waited = null;

+ listeners.notificationclick({ notification: { close() {}, data: { url: "https://app.example/inbox" } }, waitUntil(p) { waited = p; } });

+ assert.ok(waited);

+ await waited;

+ assert.deepEqual(r.opened, ["https://app.example/inbox"]);

+});