From 29974d3ab995fac98bea3efa918cb5b15844daef Mon Sep 17 00:00:00 2001 From: Tsuki <76603360+sudongyuer@users.noreply.github.com> Date: Fri, 12 Jun 2026 21:58:23 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(mobile):=20preserve=20authen?= =?UTF-8?q?ticated=20legacy=20unregister=20cleanup=20(#15723)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #15719 addressing a Codex P2 review note. After #15719, legacy v1.0.7 clients that only send `deviceId` were silent-OKed unconditionally. But `publicProcedure` still receives `ctx.userId` from `createLambdaContext` — and in the *active* sign-out path (the user is still authenticated when logout fires) that userId is valid. Skipping the delete in that case orphans the existing `(userId, deviceId)` row, so `PushChannel.deliver` keeps fanning notifications out to a signed-out device. Expo's `DeviceNotRegistered` receipt only fires on uninstall, not on logout, so the cron worker doesn't catch this either. Fix: add a Path B fallback — when `ctx.userId` is available, run the original `(userId, deviceId)` delete. Path A (expoToken pair) still wins when present; Path C (silent OK) is now reserved for the case the original PR was actually targeting: a v1.0.7 client whose session is already gone, which is the source of the 401 storm. Path matrix: expoToken present → Path A: precise delete by (expoToken, deviceId) no expoToken, ctx.userId present → Path B: legacy (userId, deviceId) delete no expoToken, no session → Path C: silent OK, cron cleans up Tests added: - legacy + valid session → falls back to (userId, deviceId) - legacy + no session → silent OK - expoToken always takes precedence over userId fallback --- .../lambda/__tests__/pushToken.test.ts | 41 ++++++++++++++++--- apps/server/src/routers/lambda/pushToken.ts | 39 ++++++++++++------ 2 files changed, 63 insertions(+), 17 deletions(-) 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 }; }), });