Skip to content

Commit

Permalink
Never return broken notifications misskey-dev#409
Browse files Browse the repository at this point in the history 
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 421f8d4)
  • Loading branch information
@dakkar @kakkokari-gtyih
dakkar authored and kakkokari-gtyih committed Feb 13, 2024
1 parent 309a943 commit 417bb2d
Showing 1 changed file with 59 additions and 16 deletions.
75 changes: 59 additions & 16 deletions packages/backend/src/core/entities/NotificationEntityService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,21 +64,38 @@ export class NotificationEntityService implements OnModuleInit {
packedNotes: Map<MiNote['id'], Packed<'Note'>>;
packedUsers: Map<MiUser['id'], Packed<'UserLite'>>;
},
): Promise<Packed<'Notification'>> {
): Promise<Packed<'Notification'> | 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,
Expand Down Expand Up @@ -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
Expand All @@ -159,31 +176,47 @@ export class NotificationEntityService implements OnModuleInit {
packedNotes: Map<MiNote['id'], Packed<'Note'>>;
packedUsers: Map<MiUser['id'], Packed<'UserLite'>>;
},
): Promise<Packed<'Notification'>> {
): Promise<Packed<'Notification'> | 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 });
return {
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(),
Expand All @@ -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(),
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 417bb2d

Please sign in to comment.