From 30b1117ead40e4ccce117e91cf65c172d8edaac8 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Mon, 2 Nov 2026 09:49:21 -0500 Subject: [PATCH] fix(puter-js): turning push off in one tab stops other tabs' syncs Tabs of a site take a shared Web Lock to subscribe and announce, and again to confirm, without holding it while the confirmation push is on its way. A load sync re-reads the opt-in inside the lock and stops when it is gone. disable() tells the other tabs first, so their syncs and any enable() in flight stop, then forgets the opt-ins and removes the subscription inside the same lock. Without Web Locks the re-reads and the message still apply. --- .../src/modules/notifications/index.js | 42 +++-- .../src/modules/notifications/lib/webPush.js | 104 +++++++---- .../src/modules/notifications/webPush.test.js | 169 +++++++++++++++++- 3 files changed, 269 insertions(+), 46 deletions(-) diff --git a/src/puter-js/src/modules/notifications/index.js b/src/puter-js/src/modules/notifications/index.js index fb2ac78bf..cdf6fdb61 100644 --- a/src/puter-js/src/modules/notifications/index.js +++ b/src/puter-js/src/modules/notifications/index.js @@ -48,6 +48,8 @@ export const CLICK_PARAM = 'puter.notification'; /** Where a site's tabs tell each other that a user's opt-in changed. */ const OPT_IN_CHANNEL = 'puter-push-opt-in'; +/** Held by a site's tabs while one changes this browser's subscription. */ +const PUSH_LOCK = 'puter-push'; /** As long as `push-sw.js` gives a handler. */ const HANDLER_TIMEOUT_MS = 5_000; @@ -75,6 +77,14 @@ const warnStorage = (error) => { ); }; +/** Runs `run` alone among the site's tabs, where the browser has Web Locks. */ +const withPushLock = (run) => { + const locks = globalThis.navigator?.locks; + return typeof locks?.request === 'function' + ? locks.request(PUSH_LOCK, () => run()) + : run(); +}; + /** Hidden, or in a window without focus. */ const pageAway = () => { const doc = globalThis.document; @@ -362,6 +372,7 @@ export class NotificationsModule extends EventListener { */ async _enableWebPush(worker) { this.#useWorker(worker); + this.#listenForOptInChanges(); /** @type {() => void} */ let pending = () => {}; const sent = new Promise((resolve) => { @@ -374,6 +385,7 @@ export class NotificationsModule extends EventListener { signal, renewRemoved: true, interactive: true, + withLock: withPushLock, }); try { // The GUI reads the registered device right after. @@ -409,12 +421,12 @@ export class NotificationsModule extends EventListener { * @internal */ async _disableWebPush(worker) { - this.#cancelVerification(); - try { - return await removeWebPush(this.puter, { worker }); - } finally { + this.#stopSyncs(); + if (this.puter.env === 'web') this.#optInChanged('off'); + return await withPushLock(async () => { if (this.puter.env === 'web') await this.#forgetOptIns(); - } + return await removeWebPush(this.puter, { worker }); + }); } /** @@ -460,8 +472,7 @@ export class NotificationsModule extends EventListener { */ async _forgetPush(authToken) { if (this.puter.env !== 'web') return; - this.#syncs.clear(); - this.#cancelVerification(); + this.#stopSyncs(); try { if (authToken && webPushSupported()) { await removeWebPush(this.puter, { @@ -564,11 +575,11 @@ export class NotificationsModule extends EventListener { return on; } - /** Other tabs of the site re-read the opt-in this one just wrote. */ - #optInChanged() { + /** Other tabs re-read the opt-in; `off` also stops their syncs. */ + #optInChanged(kind = 'changed') { this.#listenForOptInChanges(); try { - this.#optInChannel?.postMessage('changed'); + this.#optInChannel?.postMessage(kind); } catch { // A closed channel; the other tabs read it on their next sync. } @@ -581,7 +592,10 @@ export class NotificationsModule extends EventListener { ) return; this.#optInChannel = new BroadcastChannel(OPT_IN_CHANNEL); - this.#optInChannel.onmessage = () => void this.#readOptIn(); + this.#optInChannel.onmessage = (event) => { + if (event.data === 'off') this.#stopSyncs(); + void this.#readOptIn(); + }; } async #syncIfOptedIn(worker) { @@ -590,6 +604,8 @@ export class NotificationsModule extends EventListener { syncWebPush(this.puter, { worker, signal: this.#stopVerifying.signal, + stillWanted: () => this.#readOptIn(), + withLock: withPushLock, }), worker, 'could not keep the push subscription current', @@ -631,9 +647,11 @@ export class NotificationsModule extends EventListener { await this.#recordOptIn(false); } - #cancelVerification() { + /** Ends this page's syncs in flight; the next `onPush` syncs again. */ + #stopSyncs() { this.#stopVerifying.abort(); this.#stopVerifying = new AbortController(); + this.#syncs.clear(); } /** diff --git a/src/puter-js/src/modules/notifications/lib/webPush.js b/src/puter-js/src/modules/notifications/lib/webPush.js index a16aa6292..79499f270 100644 --- a/src/puter-js/src/modules/notifications/lib/webPush.js +++ b/src/puter-js/src/modules/notifications/lib/webPush.js @@ -232,6 +232,12 @@ const announce = (puter, subscription, label, interactive) => { interactiveReauth: interactive }, ); +const turnedOff = () => + new PuterJSError( + 'Push notifications were turned off before this browser was confirmed', + 'push_verification_canceled', + ); + /** Whether `buffer` is the key Puter last subscribed this origin with. */ const earlierPuterKey = async (buffer) => { try { @@ -271,38 +277,38 @@ const currentSubscription = async (puter, registration, key) => { return null; }; -/** - * Announces `subscription` and, for one the server doesn't have yet, waits for - * the confirmation push and verifies it. - */ +/** Announces what `subscribe` gives, then confirms it if the server asks. */ const register = async ( puter, nav, - subscription, - { label, onPending, signal, interactive }, + subscribe, + { label, onPending, signal, interactive, whileWanted }, ) => { // Listening first: the confirmation can arrive before the POST returns. const challenge = nextChallenge(nav, signal); try { - const result = await announce(puter, subscription, label, interactive); + const { subscription, result } = await whileWanted(async () => { + const subscription = await subscribe(); + return { + subscription, + result: await announce(puter, subscription, label, interactive), + }; + }); if (result.status === 'active') return result.device; onPending?.(); const answer = await challenge.promise; - if (answer === null) { - throw new PuterJSError( - 'Push notifications were turned off before this browser was confirmed', - 'push_verification_canceled', - ); - } - const { device } = await request( - puter, - 'POST', - '/push/web/subscriptions/verify', - { - subscription: subscriptionBody(subscription), - challenge: answer, - }, - { interactiveReauth: false }, + if (answer === null) throw turnedOff(); + const { device } = await whileWanted(() => + request( + puter, + 'POST', + '/push/web/subscriptions/verify', + { + subscription: subscriptionBody(subscription), + challenge: answer, + }, + { interactiveReauth: false }, + ), ); return device; } finally { @@ -324,6 +330,8 @@ const register = async ( * signal?: AbortSignal; * renewRemoved?: boolean; * interactive?: boolean; + * stillWanted?: () => boolean | Promise; + * withLock?: (run: () => Promise) => Promise; * }} options * `onPending` runs once the server has the subscription and only its * confirmation is left. Aborting `signal` stops waiting for that @@ -331,6 +339,8 @@ const register = async ( * subscription the user removed in Puter instead of rejecting with * `push_device_removed`. `interactive` lets a rejected token open Puter's * sign-in for the key and the subscription; the confirmation never does. + * Subscribing and confirming each run in `withLock` while `stillWanted` + * holds, else reject with `push_verification_canceled`. * @returns {Promise>} The server's device record. */ export async function syncWebPush( @@ -343,6 +353,8 @@ export async function syncWebPush( signal, renewRemoved = false, interactive = false, + stillWanted = () => true, + withLock = (run) => run(), }, ) { const { publicKey } = await request( @@ -363,24 +375,50 @@ export async function syncWebPush( keepSiteWorker: puter.env !== 'gui', }); - let subscription = - (await currentSubscription(puter, registration, key)) ?? - (await subscribeWith(registration, key)); - await rememberPuterKey(publicKey); - - const options = { label, onPending, signal, interactive }; + /** @type {PushSubscription | null} */ + let subscription = null; + const options = { + label, + onPending, + signal, + interactive, + whileWanted: (run) => + withLock(async () => { + if (signal?.aborted || !(await stillWanted())) + throw turnedOff(); + return await run(); + }), + }; try { - return await register(puter, nav, subscription, options); + return await register( + puter, + nav, + async () => { + subscription = + (await currentSubscription(puter, registration, key)) ?? + (await subscribeWith(registration, key)); + await rememberPuterKey(publicKey); + return subscription; + }, + options, + ); } catch (error) { const renewable = RENEWABLE_SUBSCRIPTION_ERRORS.has(error?.code) || (renewRemoved && error?.code === 'push_device_removed'); - if (!renewable || signal?.aborted) throw error; + if (!renewable || !subscription || signal?.aborted) throw error; // A brand-new subscription the push service doesn't know yet, an // endpoint another account turned off here, or one the user removed. - await subscription.unsubscribe(); - subscription = await subscribeWith(registration, key); - return await register(puter, nav, subscription, options); + const stale = subscription; + return await register( + puter, + nav, + async () => { + await stale.unsubscribe(); + return await subscribeWith(registration, key); + }, + options, + ); } } diff --git a/src/puter-js/src/modules/notifications/webPush.test.js b/src/puter-js/src/modules/notifications/webPush.test.js index c13e35880..9a8f53222 100644 --- a/src/puter-js/src/modules/notifications/webPush.test.js +++ b/src/puter-js/src/modules/notifications/webPush.test.js @@ -590,6 +590,63 @@ describe('syncWebPush', () => { }); }, ); + + it('subscribes and announces inside withLock, and not at all once push is no longer wanted', async () => { + const nav = makeNav(); + const order = []; + server((route) => { + order.push(route); + return { status: 'active', device: { uid: 'd1' } }; + }); + const withLock = async (run) => { + order.push('lock'); + try { + return await run(); + } finally { + order.push('unlock'); + } + }; + nav.pushManager.subscribe.mockImplementation(async () => { + order.push('subscribe'); + return makeSubscription(); + }); + await syncWebPush(puter, { worker, nav, withLock }); + expect(order).toEqual([ + 'lock', + 'subscribe', + '/push/web/subscriptions', + 'unlock', + ]); + + mockRequest.mockClear(); + nav.pushManager.subscribe.mockClear(); + await expect( + syncWebPush(puter, { worker, nav, stillWanted: () => false }), + ).rejects.toMatchObject({ code: 'push_verification_canceled' }); + expect(nav.pushManager.subscribe).not.toHaveBeenCalled(); + expect(Object.keys(reauthByRoute())).toEqual(['/push/web/key']); + }); + + it('verifies only while push is still wanted when the confirmation arrives', async () => { + const nav = makeNav(); + const confirm = pendingServer(nav); + let wanted = true; + const syncing = syncWebPush(puter, { + worker, + nav, + onPending: () => { + wanted = false; + confirm(); + }, + stillWanted: () => wanted, + }); + await expect(syncing).rejects.toMatchObject({ + code: 'push_verification_canceled', + }); + expect(Object.keys(reauthByRoute())).not.toContain( + '/push/web/subscriptions/verify', + ); + }); }); describe('removeWebPush', () => { @@ -1743,8 +1800,9 @@ describe('disable', () => { const { module } = makeModule(); const enabling = module.enable(); await vi.waitFor(() => expect(announce).toBeDefined()); - await module.disable(); + const disabling = module.disable(); announce(); + await expect(disabling).resolves.toBe(true); await expect(enabling).rejects.toMatchObject({ code: 'push_verification_canceled', }); @@ -1763,6 +1821,115 @@ describe('disable', () => { }); }); +describe.each([ + ['with Web Locks', false], + ['without Web Locks', true], +])('another tab turning push off, %s', (_label, noLocks) => { + beforeEach(() => { + if (noLocks) + Object.defineProperty(globalThis.navigator, 'locks', { + value: undefined, + configurable: true, + }); + }); + afterEach(() => { + if (noLocks) delete globalThis.navigator.locks; + }); + + /** Requests these tabs made; earlier tests' tabs still hear the channel. */ + const routes = (...tabs) => + mockRequest.mock.calls + .filter(([host]) => tabs.some((tab) => tab.puter === host)) + .map(([, method, route]) => `${method} ${route}`); + + it('stops a load sync before it announces, and the device stays removed', async () => { + store.set(OPT_IN, true); + withBrowser(makeNav({ existing: makeSubscription() })); + let releaseKey; + mockRequest.mockImplementation(async (_p, method, route) => { + if (route === '/push/web/key') { + await new Promise((resolve) => { + releaseKey = resolve; + }); + return { publicKey: KEY }; + } + return method === 'DELETE' + ? { removed: true } + : { status: 'active', device: { uid: 'd1' } }; + }); + const onError = vi.fn(); + const { module: loading } = makeModule(); + await loading.onPush('chat', () => {}, { onError }); + await vi.waitFor(() => expect(releaseKey).toBeDefined()); + + const { module: other } = makeModule(); + await expect(other.disable()).resolves.toBe(true); + releaseKey(); + await flush(); + await flush(); + + expect(routes(loading, other)).toEqual([ + 'GET /push/web/key', + 'DELETE /push/web/subscriptions', + ]); + expect(onError).not.toHaveBeenCalled(); + expect(store.has(OPT_IN)).toBe(false); + }); + + it('stops a load sync waiting for its confirmation, without verifying', async () => { + store.set(OPT_IN, true); + const nav = makeNav(); + withBrowser(nav); + server((route) => + route === '/push/web/subscriptions' + ? { status: 'pending' } + : { device: { uid: 'd1' } }, + ); + const { module: loading } = makeModule(); + await loading.onPush('chat', () => {}, { onError: () => {} }); + await vi.waitFor(() => + expect(routes(loading)).toContain('POST /push/web/subscriptions'), + ); + + const { module: other } = makeModule(); + await other.disable(); + nav.post({ type: 'puter-push-challenge', challenge: 'c.sig' }); + await flush(); + await flush(); + + expect(routes(loading)).not.toContain( + 'POST /push/web/subscriptions/verify', + ); + }); + + it('makes an enable() in flight reject', async () => { + withBrowser(makeNav({ existing: makeSubscription() })); + let announce; + mockRequest.mockImplementation(async (_p, method, route) => { + if (route === '/push/web/key') return { publicKey: KEY }; + if (method === 'POST' && route === '/push/web/subscriptions') { + return await new Promise((resolve) => { + announce = () => resolve({ status: 'pending' }); + }); + } + return { removed: true }; + }); + const { module: enablingTab } = makeModule(); + const enabling = enablingTab.enable(); + await vi.waitFor(() => expect(announce).toBeDefined()); + + const { module: other } = makeModule(); + const disabling = other.disable(); + await flush(); + announce(); + await expect(enabling).rejects.toMatchObject({ + code: 'push_verification_canceled', + }); + await expect(disabling).resolves.toBe(true); + expect(store.has(OPT_IN)).toBe(false); + }); +}); + describe('sign-out', () => { it('removes the subscription with the leaving user`s token and forgets their push state', async () => { store.set(OPT_IN, true);