From d0c9afc16ec12d44dbeeb195e86328a154458e18 Mon Sep 17 00:00:00 2001 From: 0xsysr3ll <31414959+0xSysR3ll@users.noreply.github.com> Date: Wed, 31 Dec 2025 13:44:45 +0100 Subject: [PATCH] fix(webpush): improve iOS push subscription endpoint cleanup (#2140) --- server/entity/UserPushSubscription.ts | 9 +- server/lib/notifications/agents/webpush.ts | 28 +++++- ...4-AddUniqueConstraintToPushSubscription.ts | 19 ++++ ...4-AddUniqueConstraintToPushSubscription.ts | 17 ++++ server/routes/user/index.ts | 97 ++++++++++++++----- .../UserNotificationsWebPush/index.tsx | 53 ++++++++-- src/utils/pushSubscriptionHelpers.ts | 41 ++++++-- 7 files changed, 222 insertions(+), 42 deletions(-) create mode 100644 server/migration/postgres/1765233385034-AddUniqueConstraintToPushSubscription.ts create mode 100755 server/migration/sqlite/1765233385034-AddUniqueConstraintToPushSubscription.ts diff --git a/server/entity/UserPushSubscription.ts b/server/entity/UserPushSubscription.ts index b3c4d5d2b..e35a01548 100644 --- a/server/entity/UserPushSubscription.ts +++ b/server/entity/UserPushSubscription.ts @@ -1,8 +1,15 @@ import { DbAwareColumn } from '@server/utils/DbColumnHelper'; -import { Column, Entity, ManyToOne, PrimaryGeneratedColumn } from 'typeorm'; +import { + Column, + Entity, + ManyToOne, + PrimaryGeneratedColumn, + Unique, +} from 'typeorm'; import { User } from './User'; @Entity() +@Unique(['endpoint', 'user']) export class UserPushSubscription { @PrimaryGeneratedColumn() public id: number; diff --git a/server/lib/notifications/agents/webpush.ts b/server/lib/notifications/agents/webpush.ts index fcb7416e0..84ccb10fe 100644 --- a/server/lib/notifications/agents/webpush.ts +++ b/server/lib/notifications/agents/webpush.ts @@ -24,6 +24,15 @@ interface PushNotificationPayload { isAdmin?: boolean; } +interface WebPushError extends Error { + statusCode?: number; + status?: number; + body?: string | unknown; + response?: { + body?: string | unknown; + }; +} + class WebPushAgent extends BaseAgent implements NotificationAgent @@ -188,19 +197,30 @@ class WebPushAgent notificationPayload ); } catch (e) { + const webPushError = e as WebPushError; + const statusCode = webPushError.statusCode || webPushError.status; + const errorMessage = webPushError.message || String(e); + + // RFC 8030: 410/404 are permanent failures, others are transient + const isPermanentFailure = statusCode === 410 || statusCode === 404; + logger.error( - 'Error sending web push notification; removing subscription', + isPermanentFailure + ? 'Error sending web push notification; removing invalid subscription' + : 'Error sending web push notification (transient error, keeping subscription)', { label: 'Notifications', recipient: pushSub.user.displayName, type: Notification[type], subject: payload.subject, - errorMessage: e.message, + errorMessage, + statusCode: statusCode || 'unknown', } ); - // Failed to send notification so we need to remove the subscription - userPushSubRepository.remove(pushSub); + if (isPermanentFailure) { + await userPushSubRepository.remove(pushSub); + } } }; diff --git a/server/migration/postgres/1765233385034-AddUniqueConstraintToPushSubscription.ts b/server/migration/postgres/1765233385034-AddUniqueConstraintToPushSubscription.ts new file mode 100644 index 000000000..af245cfd6 --- /dev/null +++ b/server/migration/postgres/1765233385034-AddUniqueConstraintToPushSubscription.ts @@ -0,0 +1,19 @@ +import type { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddUniqueConstraintToPushSubscription1765233385034 + implements MigrationInterface +{ + name = 'AddUniqueConstraintToPushSubscription1765233385034'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "user_push_subscription" ADD CONSTRAINT "UQ_6427d07d9a171a3a1ab87480005" UNIQUE ("endpoint", "userId")` + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "user_push_subscription" DROP CONSTRAINT "UQ_6427d07d9a171a3a1ab87480005"` + ); + } +} diff --git a/server/migration/sqlite/1765233385034-AddUniqueConstraintToPushSubscription.ts b/server/migration/sqlite/1765233385034-AddUniqueConstraintToPushSubscription.ts new file mode 100755 index 000000000..48a8c6d65 --- /dev/null +++ b/server/migration/sqlite/1765233385034-AddUniqueConstraintToPushSubscription.ts @@ -0,0 +1,17 @@ +import type { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddUniqueConstraintToPushSubscription1765233385034 + implements MigrationInterface +{ + name = 'AddUniqueConstraintToPushSubscription1765233385034'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `CREATE UNIQUE INDEX "UQ_6427d07d9a171a3a1ab87480005" ON "user_push_subscription" ("endpoint", "userId")` + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DROP INDEX "UQ_6427d07d9a171a3a1ab87480005"`); + } +} diff --git a/server/routes/user/index.ts b/server/routes/user/index.ts index e3a5fd074..107327bc1 100644 --- a/server/routes/user/index.ts +++ b/server/routes/user/index.ts @@ -4,7 +4,7 @@ import TautulliAPI from '@server/api/tautulli'; import { MediaType } from '@server/constants/media'; import { MediaServerType } from '@server/constants/server'; import { UserType } from '@server/constants/user'; -import { getRepository } from '@server/datasource'; +import dataSource, { getRepository } from '@server/datasource'; import Media from '@server/entity/Media'; import { MediaRequest } from '@server/entity/MediaRequest'; import { User } from '@server/entity/User'; @@ -25,7 +25,8 @@ import { getHostname } from '@server/utils/getHostname'; import { Router } from 'express'; import gravatarUrl from 'gravatar-url'; import { findIndex, sortBy } from 'lodash'; -import { In } from 'typeorm'; +import type { EntityManager } from 'typeorm'; +import { In, Not } from 'typeorm'; import userSettingsRoutes from './usersettings'; const router = Router(); @@ -188,30 +189,82 @@ router.post< } >('/registerPushSubscription', async (req, res, next) => { try { - const userPushSubRepository = getRepository(UserPushSubscription); + // This prevents race conditions where two requests both pass the checks + await dataSource.transaction( + async (transactionalEntityManager: EntityManager) => { + const transactionalRepo = + transactionalEntityManager.getRepository(UserPushSubscription); - const existingSubs = await userPushSubRepository.find({ - relations: { user: true }, - where: { auth: req.body.auth, user: { id: req.user?.id } }, - }); + // Check for existing subscription by auth or endpoint within transaction + const existingSubscription = await transactionalRepo.findOne({ + relations: { user: true }, + where: [ + { auth: req.body.auth, user: { id: req.user?.id } }, + { endpoint: req.body.endpoint, user: { id: req.user?.id } }, + ], + }); - if (existingSubs.length > 0) { - logger.debug( - 'User push subscription already exists. Skipping registration.', - { label: 'API' } - ); - return res.status(204).send(); - } + if (existingSubscription) { + // If endpoint matches but auth is different, update with new keys (iOS refresh case) + if ( + existingSubscription.endpoint === req.body.endpoint && + existingSubscription.auth !== req.body.auth + ) { + existingSubscription.auth = req.body.auth; + existingSubscription.p256dh = req.body.p256dh; + existingSubscription.userAgent = req.body.userAgent; - const userPushSubscription = new UserPushSubscription({ - auth: req.body.auth, - endpoint: req.body.endpoint, - p256dh: req.body.p256dh, - userAgent: req.body.userAgent, - user: req.user, - }); + await transactionalRepo.save(existingSubscription); - userPushSubRepository.save(userPushSubscription); + logger.debug( + 'Updated existing push subscription with new keys for same endpoint.', + { label: 'API' } + ); + return; + } + + logger.debug( + 'Duplicate subscription detected. Skipping registration.', + { label: 'API' } + ); + return; + } + + // Clean up old subscriptions from the same device (userAgent) for this user + // iOS can silently refresh endpoints, leaving stale subscriptions in the database + // Only clean up if we're creating a new subscription (not updating an existing one) + if (req.body.userAgent) { + const staleSubscriptions = await transactionalRepo.find({ + relations: { user: true }, + where: { + userAgent: req.body.userAgent, + user: { id: req.user?.id }, + // Only remove subscriptions with different endpoints (stale ones) + // Keep subscriptions that might be from different browsers/tabs + endpoint: Not(req.body.endpoint), + }, + }); + + if (staleSubscriptions.length > 0) { + await transactionalRepo.remove(staleSubscriptions); + logger.debug( + `Removed ${staleSubscriptions.length} stale push subscription(s) from same device.`, + { label: 'API' } + ); + } + } + + const userPushSubscription = new UserPushSubscription({ + auth: req.body.auth, + endpoint: req.body.endpoint, + p256dh: req.body.p256dh, + userAgent: req.body.userAgent, + user: req.user, + }); + + await transactionalRepo.save(userPushSubscription); + } + ); return res.status(204).send(); } catch (e) { diff --git a/src/components/UserProfile/UserSettings/UserNotificationSettings/UserNotificationsWebPush/index.tsx b/src/components/UserProfile/UserSettings/UserNotificationSettings/UserNotificationsWebPush/index.tsx index cf054673f..0558b7fc7 100644 --- a/src/components/UserProfile/UserSettings/UserNotificationSettings/UserNotificationsWebPush/index.tsx +++ b/src/components/UserProfile/UserSettings/UserNotificationSettings/UserNotificationsWebPush/index.tsx @@ -109,15 +109,28 @@ const UserWebPushSettings = () => { // Deletes/disables corresponding push subscription from database const disablePushNotifications = async (endpoint?: string) => { try { - await unsubscribeToPushNotifications(user?.id, endpoint); - - // Delete from backend if endpoint is available - if (subEndpoint) { - await deletePushSubscriptionFromBackend(subEndpoint); - } + const unsubscribedEndpoint = await unsubscribeToPushNotifications( + user?.id, + endpoint + ); localStorage.setItem('pushNotificationsEnabled', 'false'); setWebPushEnabled(false); + + // Only delete the current browser's subscription, not all devices + const endpointToDelete = unsubscribedEndpoint || subEndpoint || endpoint; + if (endpointToDelete) { + try { + await axios.delete( + `/api/v1/user/${user?.id}/pushSubscription/${encodeURIComponent( + endpointToDelete + )}` + ); + } catch { + // Ignore deletion failures - backend cleanup is best effort + } + } + addToast(intl.formatMessage(messages.webpushhasbeendisabled), { autoDismiss: true, appearance: 'success', @@ -157,7 +170,33 @@ const UserWebPushSettings = () => { useEffect(() => { const verifyWebPush = async () => { const enabled = await verifyPushSubscription(user?.id, currentSettings); - setWebPushEnabled(enabled); + let isEnabled = enabled; + + if (!enabled && 'serviceWorker' in navigator) { + const { subscription } = await getPushSubscription(); + if (subscription) { + isEnabled = true; + } + } + + if (!isEnabled && dataDevices && dataDevices.length > 0) { + const currentUserAgent = navigator.userAgent; + const hasMatchingDevice = dataDevices.some( + (device) => device.userAgent === currentUserAgent + ); + + if (hasMatchingDevice) { + isEnabled = true; + } + } + + setWebPushEnabled(isEnabled); + if (localStorage.getItem('pushNotificationsEnabled') === null) { + localStorage.setItem( + 'pushNotificationsEnabled', + isEnabled ? 'true' : 'false' + ); + } }; if (user?.id) { diff --git a/src/utils/pushSubscriptionHelpers.ts b/src/utils/pushSubscriptionHelpers.ts index 4cc146a40..7cf0906da 100644 --- a/src/utils/pushSubscriptionHelpers.ts +++ b/src/utils/pushSubscriptionHelpers.ts @@ -49,13 +49,17 @@ export const verifyPushSubscription = async ( currentSettings.vapidPublic ).toString(); + if (currentServerKey !== expectedServerKey) { + return false; + } + const endpoint = subscription.endpoint; const { data } = await axios.get( `/api/v1/user/${userId}/pushSubscription/${encodeURIComponent(endpoint)}` ); - return expectedServerKey === currentServerKey && data.endpoint === endpoint; + return data.endpoint === endpoint; } catch { return false; } @@ -65,20 +69,39 @@ export const verifyAndResubscribePushSubscription = async ( userId: number | undefined, currentSettings: PublicSettingsResponse ): Promise => { + if (!userId) { + return false; + } + + const { subscription } = await getPushSubscription(); const isValid = await verifyPushSubscription(userId, currentSettings); if (isValid) { return true; } + if (subscription) { + return false; + } + if (currentSettings.enablePushRegistration) { try { - // Unsubscribe from the backend to clear the existing push subscription (keys and endpoint) - await unsubscribeToPushNotifications(userId); + const oldEndpoint = await unsubscribeToPushNotifications(userId); - // Subscribe again to generate a fresh push subscription with updated keys and endpoint await subscribeToPushNotifications(userId, currentSettings); + if (oldEndpoint) { + try { + await axios.delete( + `/api/v1/user/${userId}/pushSubscription/${encodeURIComponent( + oldEndpoint + )}` + ); + } catch (error) { + // Ignore errors when deleting old endpoint (it might not exist) + } + } + return true; } catch (error) { throw new Error(`[SW] Resubscribe failed: ${error.message}`); @@ -136,24 +159,26 @@ export const subscribeToPushNotifications = async ( export const unsubscribeToPushNotifications = async ( userId: number | undefined, endpoint?: string -) => { +): Promise => { if (!('serviceWorker' in navigator) || !userId) { - return; + return null; } try { const { subscription } = await getPushSubscription(); if (!subscription) { - return false; + return null; } const { endpoint: currentEndpoint } = subscription.toJSON(); if (!endpoint || endpoint === currentEndpoint) { await subscription.unsubscribe(); - return true; + return currentEndpoint ?? null; } + + return null; } catch (error) { throw new Error( `Issue unsubscribing to push notifications: ${error.message}`