From 417bb2d31e5eaae08e0500a0525478606bfaea38 Mon Sep 17 00:00:00 2001 From: dakkar Date: Sun, 11 Feb 2024 12:21:56 +0000 Subject: [PATCH 1/3] Never return broken notifications #409 Since notifications are stored in Redis, we can't expect relational integrity: deleting a user will *not* delete notifications that mention it. But if we return notifications with missing bits (a `follow` without a `user`, for example), the frontend will get very confused and throw an exception while trying to render them. This change makes sure we never expose those broken notifications. For uniformity, I've applied the same logic to notes and roles mentioned in notifications, even if nobody reported breakage in those cases. Tested by creating a few types of notifications with a `notifierId`, then deleting their user. (cherry picked from commit 421f8d49e5d7a8dc3a798cc54716c767df8be3cb) --- .../entities/NotificationEntityService.ts | 75 +++++++++++++++---- 1 file changed, 59 insertions(+), 16 deletions(-) diff --git a/packages/backend/src/core/entities/NotificationEntityService.ts b/packages/backend/src/core/entities/NotificationEntityService.ts index 0663898edb..a572fe320c 100644 --- a/packages/backend/src/core/entities/NotificationEntityService.ts +++ b/packages/backend/src/core/entities/NotificationEntityService.ts @@ -64,21 +64,38 @@ export class NotificationEntityService implements OnModuleInit { packedNotes: Map>; packedUsers: Map>; }, - ): Promise> { + ): Promise | null> { const notification = src; - const noteIfNeed = NOTE_REQUIRED_NOTIFICATION_TYPES.has(notification.type) && 'noteId' in notification ? ( + const needsNote = NOTE_REQUIRED_NOTIFICATION_TYPES.has(notification.type) && 'noteId' in notification; + const noteIfNeed = needsNote ? ( hint?.packedNotes != null ? hint.packedNotes.get(notification.noteId) : this.noteEntityService.pack(notification.noteId, { id: meId }, { detail: true, }) ) : undefined; - const userIfNeed = 'notifierId' in notification ? ( + // if the note has been deleted, don't show this notification + if (needsNote && !noteIfNeed) { + return null; + } + + const needsUser = 'notifierId' in notification; + const userIfNeed = needsUser ? ( hint?.packedUsers != null ? hint.packedUsers.get(notification.notifierId) : this.userEntityService.pack(notification.notifierId, { id: meId }) ) : undefined; - const role = notification.type === 'roleAssigned' ? await this.roleEntityService.pack(notification.roleId) : undefined; + // if the user has been deleted, don't show this notification + if (needsUser && !userIfNeed) { + return null; + } + + const needsRole = notification.type === 'roleAssigned'; + const role = needsRole ? await this.roleEntityService.pack(notification.roleId) : undefined; + // if the role has been deleted, don't show this notification + if (needsRole && !role) { + return null; + } return await awaitAll({ id: notification.id, @@ -141,10 +158,10 @@ export class NotificationEntityService implements OnModuleInit { validNotifications = validNotifications.filter(x => (x.type !== 'receiveFollowRequest') || reqs.some(r => r.followerId === x.notifierId)); } - return await Promise.all(validNotifications.map(x => this.pack(x, meId, {}, { + return (await Promise.all(validNotifications.map(x => this.pack(x, meId, {}, { packedNotes, packedUsers, - }))); + })))).filter(n => n); } @bindThis @@ -159,23 +176,34 @@ export class NotificationEntityService implements OnModuleInit { packedNotes: Map>; packedUsers: Map>; }, - ): Promise> { + ): Promise | null> { const notification = src; - const noteIfNeed = NOTE_REQUIRED_GROUPED_NOTIFICATION_TYPES.has(notification.type) && 'noteId' in notification ? ( + const needsNote = NOTE_REQUIRED_GROUPED_NOTIFICATION_TYPES.has(notification.type) && 'noteId' in notification; + const noteIfNeed = needsNote ? ( hint?.packedNotes != null ? hint.packedNotes.get(notification.noteId) : this.noteEntityService.pack(notification.noteId, { id: meId }, { detail: true, }) ) : undefined; - const userIfNeed = 'notifierId' in notification ? ( + // if the note has been deleted, don't show this notification + if (needsNote && !noteIfNeed) { + return null; + } + + const needsUser = 'notifierId' in notification; + const userIfNeed = needsUser ? ( hint?.packedUsers != null ? hint.packedUsers.get(notification.notifierId) : this.userEntityService.pack(notification.notifierId, { id: meId }) ) : undefined; + // if the user has been deleted, don't show this notification + if (needsUser && !userIfNeed) { + return null; + } if (notification.type === 'reaction:grouped') { - const reactions = await Promise.all(notification.reactions.map(async reaction => { + const reactions = (await Promise.all(notification.reactions.map(async reaction => { const user = hint?.packedUsers != null ? hint.packedUsers.get(reaction.userId)! : await this.userEntityService.pack(reaction.userId, { id: meId }); @@ -183,7 +211,12 @@ export class NotificationEntityService implements OnModuleInit { user, reaction: reaction.reaction, }; - })); + }))).filter(r => r.user); + // if all users have been deleted, don't show this notification + if (!reactions.length) { + return null; + } + return await awaitAll({ id: notification.id, createdAt: new Date(notification.createdAt).toISOString(), @@ -192,14 +225,19 @@ export class NotificationEntityService implements OnModuleInit { reactions, }); } else if (notification.type === 'renote:grouped') { - const users = await Promise.all(notification.userIds.map(userId => { + const users = (await Promise.all(notification.userIds.map(userId => { const packedUser = hint?.packedUsers != null ? hint.packedUsers.get(userId) : null; if (packedUser) { return packedUser; } return this.userEntityService.pack(userId, { id: meId }); - })); + }))).filter(u => u); + // if all users have been deleted, don't show this notification + if (!users.length) { + return null; + } + return await awaitAll({ id: notification.id, createdAt: new Date(notification.createdAt).toISOString(), @@ -209,7 +247,12 @@ export class NotificationEntityService implements OnModuleInit { }); } - const role = notification.type === 'roleAssigned' ? await this.roleEntityService.pack(notification.roleId) : undefined; + const needsRole = notification.type === 'roleAssigned'; + const role = needsRole ? await this.roleEntityService.pack(notification.roleId) : undefined; + // if the role has been deleted, don't show this notification + if (needsRole && !role) { + return null; + } return await awaitAll({ id: notification.id, @@ -277,9 +320,9 @@ export class NotificationEntityService implements OnModuleInit { validNotifications = validNotifications.filter(x => (x.type !== 'receiveFollowRequest') || reqs.some(r => r.followerId === x.notifierId)); } - return await Promise.all(validNotifications.map(x => this.packGrouped(x, meId, {}, { + return (await Promise.all(validNotifications.map(x => this.packGrouped(x, meId, {}, { packedNotes, packedUsers, - }))); + })))).filter(n => n); } } From fae7cb20dce81a1e864d4d388323d4a036aea181 Mon Sep 17 00:00:00 2001 From: kakkokari-gtyih Date: Tue, 13 Feb 2024 15:23:49 +0900 Subject: [PATCH 2/3] Update Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f41188a4da..c545724424 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ - Enhance: モデレーターはすべてのユーザーのリアクション一覧を見られるように - Fix: 特定のキーワード及び正規表現にマッチする文字列を含むノートが投稿された際、エラーに出来るような設定項目を追加 #13207 * デフォルトは空欄なので適用前と同等の動作になります +- Fix: 破損した通知をクライアントに送信しないように + * 通知欄が無限にリロードされる問題が改善する可能性があります ### Client - Feat: 新しいゲームを追加 From 8d68c5078e4fdc39b2064ef73dd95f4ae27ae3b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=8B=E3=81=A3=E3=81=93=E3=81=8B=E3=82=8A?= <67428053+kakkokari-gtyih@users.noreply.github.com> Date: Mon, 19 Feb 2024 17:54:33 +0900 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68c6bce41c..6d20706994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,9 @@ ### Server - Fix: nodeinfoにenableMcaptchaとenableTurnstileが無いのを修正 - +- Fix: 破損した通知をクライアントに送信しないように + * 通知欄が無限にリロードされる問題が改善する可能性があります + ## 2024.2.0 ### Note @@ -39,8 +41,6 @@ * すべてのリモートユーザーのリアクション一覧を見えないようにします - Fix: 特定のキーワード及び正規表現にマッチする文字列を含むノートが投稿された際、エラーに出来るような設定項目を追加 #13207 * デフォルトは空欄なので適用前と同等の動作になります -- Fix: 破損した通知をクライアントに送信しないように - * 通知欄が無限にリロードされる問題が改善する可能性があります ### Client - Feat: 新しいゲームを追加