From 77327ea8564f95f83e5965656fd74c60aeb1bb4a Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:39:50 +0100 Subject: [PATCH] fix: never patch or judge another client's push subscription that reuses this device's subscription id --- src/lib/__tests__/push-notifications.test.ts | 71 ++++++++++++++++++-- src/lib/push-notifications.ts | 30 +++++++-- 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index 917ea8ee..d8bcb2e2 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -59,7 +59,13 @@ const { DETACHED, SYNC_SESSION } = vi.hoisted(() => { }; return { SYNC_SESSION, - DETACHED: { loaded: true, types: ['EmailDelivery'] as string[], log: [] as unknown[], session: SYNC_SESSION }, + DETACHED: { + loaded: true, + types: ['EmailDelivery'] as string[], + deviceClientId: null as string | null, + log: [] as unknown[], + session: SYNC_SESSION, + }, }; }); @@ -82,7 +88,8 @@ vi.mock('../../api/jmap-client', () => ({ const [name, args, id] = calls[0]; DETACHED.log.push([name, args]); if (name === 'PushSubscription/get') { - return { methodResponses: [[name, { list: [{ id: args.ids[0], types: DETACHED.types }] }, id]] }; + const listed = { id: args.ids[0], types: DETACHED.types, ...(DETACHED.deviceClientId ? { deviceClientId: DETACHED.deviceClientId } : {}) }; + return { methodResponses: [[name, { list: [listed] }, id]] }; } return { methodResponses: [[name, { updated: { [Object.keys(args.update)[0]]: null } }, id]] }; } @@ -666,7 +673,7 @@ describe('push types for device sync (#34)', () => { await refreshPushSubscriptionTypes(OTHER); expect(DETACHED.log).toEqual([ ['load', OTHER], - ['PushSubscription/get', { ids: ['bob-sub'], properties: ['id', 'types'] }], + ['PushSubscription/get', { ids: ['bob-sub'], properties: ['id', 'deviceClientId', 'types'] }], ['PushSubscription/set', { update: { 'bob-sub': { types: ['EmailDelivery', 'CalendarEvent', 'Calendar'] } } }], ]); // The singleton's subscriptions were not touched. @@ -679,6 +686,40 @@ describe('push types for device sync (#34)', () => { expect(DETACHED.log.map((entry) => (entry as unknown[])[0])).toEqual(['load', 'PushSubscription/get']); }); + it("leaves alone another client's subscription that reuses the recorded id", async () => { + // Stalwart gives a destroyed subscription's id to the next one, such as + // the webmail's on the same account. + await AsyncStorage.setItem(SUB_KEY, 'existing'); + listMock.mockResolvedValue([{ ...healthy(['EmailDelivery']), deviceClientId: 'webmailwebmailwebmailwebmailwebm' }]); + await syncing({ [CONTACTS]: true }); + await refreshPushSubscriptionTypes(ACCOUNT_ID); + expect(updateMock).not.toHaveBeenCalled(); + + // The same through another account's client. + const OTHER = 'bob@other.example.net'; + await AsyncStorage.setItem(`push:subscriptionId:v2:${OTHER}`, 'b'); + await AsyncStorage.setItem(deviceClientIdKey(OTHER), 'bobsphonebobsphonebobsphonebobsp'); + await AsyncStorage.setItem(DEVICE_SYNC_KEY, JSON.stringify({ + state: { accounts: { [OTHER]: { enabled: { [CALENDAR]: true } } } }, + version: 0, + })); + DETACHED.log = []; + DETACHED.types = ['EmailDelivery']; + DETACHED.deviceClientId = 'webmailwebmailwebmailwebmailwebm'; + try { + await refreshPushSubscriptionTypes(OTHER); + expect(DETACHED.log.map((entry) => (entry as unknown[])[0])).toEqual(['load', 'PushSubscription/get']); + + // Its own subscription still gets the types. + DETACHED.log = []; + DETACHED.deviceClientId = 'bobsphonebobsphonebobsphonebobsp'; + await refreshPushSubscriptionTypes(OTHER); + expect(DETACHED.log.map((entry) => (entry as unknown[])[0])).toEqual(['load', 'PushSubscription/get', 'PushSubscription/set']); + } finally { + DETACHED.deviceClientId = null; + } + }); + it('leaves the subscription of an account that turned push off alone', async () => { await AsyncStorage.setItem(SUB_KEY, 'existing'); await AsyncStorage.setItem(`push:optedOut:v1:${ACCOUNT_ID}`, '1'); @@ -1497,6 +1538,24 @@ describe('resyncPushNotifications', () => { expect(await AsyncStorage.getItem(OPTED_OUT_KEY)).not.toBeNull(); }); + it("takes a recorded subscription whose id now names another client's for revoked", async () => { + // Revoked here, then the webmail on the same account subscribed and got + // the freed id. + await AsyncStorage.setItem(SUB_KEY, 'b'); + await AsyncStorage.setItem(EXPIRES_KEY, inDays(5)); + await AsyncStorage.setItem('push:accountIds:v1', JSON.stringify([ACCOUNT_ID])); + listMock.mockResolvedValue([ + { id: 'b', deviceClientId: 'webmailwebmailwebmailwebmailwebm', expires: inDays(7), types: ['EmailDelivery'] }, + ]); + + const result = await resyncPushNotifications({ relayBaseUrl: RELAY }); + + expect(result).toBeNull(); + expect(createMock).not.toHaveBeenCalled(); + expect(destroyMock.mock.calls.map((c) => c[0])).not.toContain('b'); + expect(await AsyncStorage.getItem(OPTED_OUT_KEY)).not.toBeNull(); + }); + it('re-creates the subscription once the server behind the account was replaced (0.16.23 to 1.0)', async () => { // A server reinstalled for Stalwart 1.0 holds none of the subscriptions, // which looks exactly like a revocation from elsewhere; the fingerprint @@ -1613,8 +1672,12 @@ describe('resyncPushNotifications', () => { expect(await AsyncStorage.getItem(OPTED_OUT_KEY)).toBeNull(); expect(await AsyncStorage.getItem(EXPIRES_KEY)).not.toBeNull(); vi.clearAllMocks(); + // Turning push off forgot the device client id, so the new subscription + // carries a new one. + const deviceClientId = await AsyncStorage.getItem(deviceClientIdKey(ACCOUNT_ID)); + expect(deviceClientId).not.toBe(OUR_DCID); listMock.mockResolvedValue([ - { id: 'new-server-id', deviceClientId: OUR_DCID, expires: inDays(7), types: ['EmailDelivery'] }, + { id: 'new-server-id', deviceClientId, expires: inDays(7), types: ['EmailDelivery'] }, ]); expect((await resyncPushNotifications({ relayBaseUrl: RELAY }))?.subscriptionId).toBe('new-server-id'); }); diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index 0e024f1b..8e34a513 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -1703,8 +1703,25 @@ async function wasRevokedOnServer(accountId: string): Promise { const expires = Date.parse((await AsyncStorage.getItem(subscriptionExpiresKey(accountId))) ?? ''); if (!storedServerId || !Number.isFinite(expires)) return false; if (expires - Date.now() < REVOKED_EXPIRY_MARGIN_MS) return false; + const deviceClientId = await AsyncStorage.getItem(deviceClientIdKey(accountId)); const subs = await listPushSubscriptions(); - return !subs.some((s) => s.id === storedServerId); + return !subs.some((s) => isOwnSubscription(s, storedServerId, deviceClientId)); +} + +/** + * Whether a subscription the server lists is the one this device recorded. + * Stalwart hands out short ids and gives a destroyed subscription's id to the + * next one created ("b" in every account), so the id alone can name another + * client's subscription on the same account, such as the webmail's. A + * subscription listed without a device client id, or one recorded before the + * device kept its own, is taken on the id. + */ +function isOwnSubscription( + s: { id: string; deviceClientId?: string | null }, + subscriptionId: string, + deviceClientId: string | null, +): boolean { + return s.id === subscriptionId && (!s.deviceClientId || !deviceClientId || s.deviceClientId === deviceClientId); } /** @@ -1781,10 +1798,11 @@ export async function refreshPushSubscriptionTypes(accountId: string): Promise s.id === subscriptionId); + const current = (await listPushSubscriptions()).find((s) => isOwnSubscription(s, subscriptionId, deviceClientId)); if (!current || sameTypes(current.types, types)) return; await updatePushSubscription(subscriptionId, { types: [...types] }); return; @@ -1794,11 +1812,13 @@ export async function refreshPushSubscriptionTypes(accountId: string): Promise | undefined) - ?.find((s) => s.id === subscriptionId); + const list = requireMethodResult(res, '0', 'PushSubscription/get').list as + | Array<{ id: string; deviceClientId?: string | null; types?: string[] | null }> + | undefined; + const current = list?.find((s) => isOwnSubscription(s, subscriptionId, deviceClientId)); if (!current || sameTypes(current.types, types)) return; await client.request( [['PushSubscription/set', { update: { [subscriptionId]: { types: [...types] } } }, '0']],