diff --git a/apps/server/src/routers/lambda/__tests__/pushToken.test.ts b/apps/server/src/routers/lambda/__tests__/pushToken.test.ts index 7c5bafe591..90883d6ea2 100644 --- a/apps/server/src/routers/lambda/__tests__/pushToken.test.ts +++ b/apps/server/src/routers/lambda/__tests__/pushToken.test.ts @@ -112,20 +112,36 @@ describe('pushTokenRouter', () => { expect(mockUnregister).not.toHaveBeenCalled(); }); - it('should silently succeed without expoToken (1.0.7 legacy clients)', async () => { + it('should fall back to (userId, deviceId) for legacy clients with a session', async () => { + // Path B — v1.0.7 only sends deviceId; if the request still carries a + // valid session we MUST delete the row, otherwise PushChannel keeps + // notifying a signed-out device (Expo DeviceNotRegistered only fires on + // uninstall, not logout). + mockUnregister.mockResolvedValueOnce(undefined); const caller = createCaller(); const result = await caller.unregister({ deviceId: 'device-1' }); - // Cleanup happens via process-push-receipts cron — no DB delete here + expect(mockUnregister).toHaveBeenCalledWith('device-1'); + expect(mockDeleteByExpoTokenAndDevice).not.toHaveBeenCalled(); + expect(result).toEqual({ success: true }); + }); + + it('should silently succeed without expoToken AND without session', async () => { + // Path C — v1.0.7 + dead session: the only safe move is silent OK. + // Orphan row will be cleaned up by the process-push-receipts worker via + // Expo DeviceNotRegistered receipts. Returning 200 here stops the storm. + const caller = createCaller({ userId: undefined }); + + const result = await caller.unregister({ deviceId: 'device-1' }); + expect(mockDeleteByExpoTokenAndDevice).not.toHaveBeenCalled(); expect(mockUnregister).not.toHaveBeenCalled(); expect(result).toEqual({ success: true }); }); - it('should succeed for an unauthenticated caller (no userId)', async () => { - // The whole point of making this public: clients call it during sign-out - // when their session may already be gone. Must not 401. + it('should succeed for an unauthenticated caller carrying expoToken', async () => { + // New clients (>=1.0.8) hit Path A regardless of session. const caller = createCaller({ userId: undefined }); const result = await caller.unregister({ @@ -135,6 +151,21 @@ describe('pushTokenRouter', () => { expect(result).toEqual({ success: true }); expect(mockDeleteByExpoTokenAndDevice).toHaveBeenCalled(); + expect(mockUnregister).not.toHaveBeenCalled(); + }); + + it('should prefer expoToken precision over the legacy userId fallback', async () => { + // If both are available, always take Path A — the (expoToken, deviceId) + // pair is more precise and doesn't risk deleting a wrong row. + const caller = createCaller(); + + await caller.unregister({ + deviceId: 'device-1', + expoToken: 'ExponentPushToken[abc]', + }); + + expect(mockDeleteByExpoTokenAndDevice).toHaveBeenCalled(); + expect(mockUnregister).not.toHaveBeenCalled(); }); it('should reject empty deviceId', async () => { diff --git a/apps/server/src/routers/lambda/pushToken.ts b/apps/server/src/routers/lambda/pushToken.ts index f97f66aad3..ac40b51809 100644 --- a/apps/server/src/routers/lambda/pushToken.ts +++ b/apps/server/src/routers/lambda/pushToken.ts @@ -31,20 +31,25 @@ export const pushTokenRouter = router({ }), /** - * Public on purpose: clients call this during sign-out, when their session - * may already be invalid (expired token / cleared cookie). Authenticating by - * session here causes a 401 storm on every logout in the wild — the original - * intent was "clean up before clearing auth", but in practice the auth has - * already been cleared on the server long before logout fires. + * Public on purpose: clients call this during sign-out, and in the wild many + * of those calls arrive after the session is already gone (expired OIDC + * token / cleared cookie). Authenticating by session here causes a 401 + * storm on every such logout. * - * Authorization model: the caller presents the (deviceId, expoToken) pair it - * received at registration. Holding both = proof of ownership of the row, - * same trust model as APNs/FCM unregister. + * Authorization model (Path A — new clients ≥ 1.0.8): the caller presents the + * (deviceId, expoToken) pair it received at registration. Holding both = proof + * of ownership of the row, same trust model as APNs/FCM unregister. * - * Backwards compat: older clients (≤ 1.0.7) only send `deviceId`. We silently - * succeed in that case and let the `process-push-receipts` worker clean up - * stale rows via `DeviceNotRegistered` receipts from Expo. Returning 200 here - * is what actually stops the 401 storm in production. + * Backwards compat for v1.0.7 (only sends `deviceId`): + * - Path B — when the request still carries a valid session, fall back to + * the original (userId, deviceId) delete. This covers the *active* + * sign-out path so PushChannel doesn't keep notifying a signed-out device + * until the user uninstalls (Expo's DeviceNotRegistered receipt only + * fires on uninstall, not on logout). + * - Path C — when there's no session either, silently succeed. The orphan + * row will be cleaned up by the existing `process-push-receipts` worker + * via Expo's DeviceNotRegistered receipts. Returning 200 here is what + * actually stops the 401 storm in production. */ unregister: publicProcedure .use(serverDatabase) @@ -57,10 +62,20 @@ export const pushTokenRouter = router({ .mutation(async ({ ctx, input }) => { const { deviceId, expoToken } = input; + // Path A: new clients — precise delete by (expoToken, deviceId), no session needed if (expoToken) { await deletePushTokenByExpoTokenAndDevice(ctx.serverDB, { deviceId, expoToken }); + return { success: true }; } + // Path B: legacy v1.0.7 + valid session — fall back to (userId, deviceId) + if (ctx.userId) { + const pushTokenModel = new PushTokenModel(ctx.serverDB, ctx.userId); + await pushTokenModel.unregister(deviceId); + return { success: true }; + } + + // Path C: legacy v1.0.7 with no session — silent OK, cron worker cleans up return { success: true }; }), });