From d742f647c35f3f38bb9fe54af76adebe6590648a Mon Sep 17 00:00:00 2001 From: "Svetoslav Petkov (hero101)" Date: Fri, 31 Jul 2026 12:46:43 +0300 Subject: [PATCH 1/2] fix: stop a null user profile killing the server from a notification payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three notification payload builders dereference `user.profile.displayName` without a guard. Every caller loads the user with `relations: { profile: true }`, so a null profile means the ROW is incomplete — not that the relation was forgotten — and one such row makes the builder throw. That alone would be a failed request. It is worse than that: `notifyPlatformGlobalRoleChange` invokes its builder WITHOUT `await` and without a catch (both call sites), so the rejection escapes as an unhandled rejection, and under Node's default `--unhandled-rejections=throw` the process terminates. Observed twice during live verification of workspace#027-platform-role-redesign — once via platform role revocation, once via `createDiscussion` — each time taking the whole server down mid-run. - `resolveUserDisplayName()` falls back to the user's name, then their email, so a notification still carries a usable identifier instead of failing. Applied at all three sites (`buildPlatformUserRemovedNotificationPayload`, `getUserPayloadOrFail`, `createUserPayloadFromUser`). - `notifyPlatformGlobalRoleChange` wraps its dispatch in try/catch and logs through Winston. Notifying is best-effort by design; with un-awaited call sites it must never be able to take the process down, whatever the adapter does next. Pre-existing on develop and not introduced by 027 — that feature only makes the role-revocation path hot enough to hit it reliably. Co-Authored-By: Claude Opus 5 (1M context) --- .../platform.role.resolver.mutations.ts | 27 +++++++++++++--- .../notification.external.adapter.ts | 31 +++++++++++++++++-- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/platform/platform-role/platform.role.resolver.mutations.ts b/src/platform/platform-role/platform.role.resolver.mutations.ts index 802dbb7430..7c8a10a010 100644 --- a/src/platform/platform-role/platform.role.resolver.mutations.ts +++ b/src/platform/platform-role/platform.role.resolver.mutations.ts @@ -1,4 +1,5 @@ import { RoleChangeType } from '@alkemio/notifications-lib'; +import { LogContext } from '@common/enums'; import { AuthorizationPrivilege } from '@common/enums/authorization.privilege'; import { LicensingCredentialBasedCredentialType } from '@common/enums/licensing.credential.based.credential.type'; import { RoleName } from '@common/enums/role.name'; @@ -14,12 +15,14 @@ import { UserLookupService } from '@domain/community/user-lookup/user.lookup.ser import { AccountService } from '@domain/space/account/account.service'; import { AccountLicenseService } from '@domain/space/account/account.service.license'; import { AccountLookupService } from '@domain/space/account.lookup/account.lookup.service'; +import { Inject, LoggerService } from '@nestjs/common'; import { Args, Mutation, Resolver } from '@nestjs/graphql'; import { PlatformService } from '@platform/platform/platform.service'; import { NotificationInputPlatformGlobalRoleChange } from '@services/adapters/notification-adapter/dto/platform/notification.dto.input.platform.global.role.change'; import { NotificationPlatformAdapter } from '@services/adapters/notification-adapter/notification.platform.adapter'; import { InstrumentResolver } from '@src/apm/decorators'; import { CurrentActor } from '@src/common/decorators'; +import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston'; import { AssignPlatformRoleInput } from './dto/platform.role.dto.assign'; import { RemovePlatformRoleInput } from './dto/platform.role.dto.remove'; @@ -37,7 +40,8 @@ export class PlatformRoleResolverMutations { private roleSetService: RoleSetService, private userLookupService: UserLookupService, private roleSetAuthorizationService: RoleSetAuthorizationService, - private platformService: PlatformService + private platformService: PlatformService, + @Inject(WINSTON_MODULE_NEST_PROVIDER) private readonly logger: LoggerService ) {} @Mutation(() => IUser, { @@ -195,8 +199,23 @@ export class PlatformRoleResolverMutations { type: type, role: role, }; - await this.notificationPlatformAdapter.platformGlobalRoleChanged( - notificationInput - ); + // Both call sites above invoke this WITHOUT `await`, so anything this + // rejects with becomes an unhandled rejection — and under Node's default + // `--unhandled-rejections=throw` that terminates the process rather than + // failing the request. Observed twice in live verification: a user row + // with a null profile made the payload builder throw, and the server + // exited mid-run. Notifying is best-effort by design; it must never be + // able to take the process down, whatever the adapter does next. + try { + await this.notificationPlatformAdapter.platformGlobalRoleChanged( + notificationInput + ); + } catch (error: any) { + this.logger.error( + `Unable to dispatch platform global role change notification (user=${user.id}, role=${role}, type=${type}): ${error?.message}`, + error?.stack, + LogContext.NOTIFICATIONS + ); + } } } diff --git a/src/services/adapters/notification-external-adapter/notification.external.adapter.ts b/src/services/adapters/notification-external-adapter/notification.external.adapter.ts index 14d883bdb5..68ea88cb83 100644 --- a/src/services/adapters/notification-external-adapter/notification.external.adapter.ts +++ b/src/services/adapters/notification-external-adapter/notification.external.adapter.ts @@ -883,7 +883,7 @@ export class NotificationExternalAdapter { ); const result: NotificationEventPayloadPlatformUserRemoved = { user: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), email: user.email, }, ...basePayload, @@ -1237,7 +1237,7 @@ export class NotificationExternalAdapter { lastName: user.lastName, email: user.email, profile: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), url: userURL, }, type: ActorType.USER, @@ -1252,13 +1252,38 @@ export class NotificationExternalAdapter { lastName: user.lastName, email: user.email, profile: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), url: this.urlGeneratorService.createUrlForUserNameID(user.nameID), }, type: ActorType.USER, }; } + /** + * A notification payload must never be the thing that takes a request — or + * the process — down. + * + * Every caller of the three payload builders that read `profile.displayName` + * loads the user with `relations: { profile: true }`, so a null profile means + * the ROW is incomplete, not that the relation was forgotten. Dereferencing + * it unguarded turned one such row into a `TypeError`, and because + * `notifyPlatformGlobalRoleChange` invokes its builder without `await` and + * without a catch, that rejection reached Node's default + * `--unhandled-rejections=throw` and killed the server outright — observed + * twice during live verification of workspace#027-platform-role-redesign, + * once via role revocation and once via `createDiscussion`. + * + * Falls back to the user's name, then their email, so the notification still + * carries a usable human identifier instead of failing to send. + */ + private resolveUserDisplayName(user: IUser): string { + return ( + user.profile?.displayName || + `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() || + user.email + ); + } + private getPlatformURL(): string { return this.configService.get('hosting.endpoint_cluster', { infer: true }); } From c3642c668867c23eb52444fc687fadb3cd824ab7 Mon Sep 17 00:00:00 2001 From: "Svetoslav Petkov (hero101)" Date: Mon, 3 Aug 2026 13:52:45 +0300 Subject: [PATCH 2/2] fix: dispatch role-change notifications through the shared catch helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on PR review. The catch added for the null-profile crash was bespoke: a local try/catch logging a template string, which buries user/role/type in prose and leaves nothing queryable. Replace it with the same `dispatchNotification(promise, eventLabel)` helper used by role.set.resolver.mutations.membership.ts, so every best-effort notification dispatch in the codebase emits an identically shaped record ({ message, event, error } + stack + LogContext.NOTIFICATIONS) and a log query returns uniform results across call sites. Also drops the unguarded `.stack` read on a catch value that is not guaranteed to be an Error. Safe because platformGlobalRoleChanged is async: a synchronous throw becomes a rejected promise, so the `.catch` sees everything. Also adds the regression coverage Copilot asked for, which had none: Adapter — buildPlatformUserRemovedNotificationPayload reaches all three patched dereferences in one call (subject user, triggering user via getUserPayloadOrFail, recipients via createUserPayloadFromUser), so one test covers the whole surface. Pins the fallback order (profile.displayName -> trimmed first/last -> email) and the live role revocation path. Resolver — both mutation entry points assert a rejecting adapter is fully contained: the mutation still resolves its user, and the failure is logged in the shared format above, asserted as an exact record so the shape stays pinned. Uses vi.waitFor because the dispatch is fire-and-forget, so a direct assertion would race the microtask queue. Verified by reverting resolveUserDisplayName to the unguarded dereference: 3 of the 4 new adapter tests fail. Co-authored-by: Claude Opus 5 (1M context) --- .../platform.role.resolver.mutations.spec.ts | 65 ++++++++ .../platform.role.resolver.mutations.ts | 51 ++++-- .../notification.external.adapter.spec.ts | 148 ++++++++++++++++++ 3 files changed, 247 insertions(+), 17 deletions(-) diff --git a/src/platform/platform-role/platform.role.resolver.mutations.spec.ts b/src/platform/platform-role/platform.role.resolver.mutations.spec.ts index a169efc461..6a741f92d7 100644 --- a/src/platform/platform-role/platform.role.resolver.mutations.spec.ts +++ b/src/platform/platform-role/platform.role.resolver.mutations.spec.ts @@ -1,4 +1,5 @@ import { RoleChangeType } from '@alkemio/notifications-lib'; +import { LogContext } from '@common/enums'; import { AuthorizationPrivilege } from '@common/enums/authorization.privilege'; import { LicensingCredentialBasedCredentialType } from '@common/enums/licensing.credential.based.credential.type'; import { RoleName } from '@common/enums/role.name'; @@ -11,11 +12,13 @@ import { LicenseService } from '@domain/common/license/license.service'; import { UserLookupService } from '@domain/community/user-lookup/user.lookup.service'; import { AccountService } from '@domain/space/account/account.service'; import { AccountLicenseService } from '@domain/space/account/account.service.license'; +import { LoggerService } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { PlatformService } from '@platform/platform/platform.service'; import { NotificationPlatformAdapter } from '@services/adapters/notification-adapter/notification.platform.adapter'; import { MockWinstonProvider } from '@test/mocks/winston.provider.mock'; import { defaultMockerFactory } from '@test/utils/default.mocker.factory'; +import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston'; import { type Mock } from 'vitest'; import { PlatformRoleResolverMutations } from './platform.role.resolver.mutations'; @@ -31,6 +34,7 @@ describe('PlatformRoleResolverMutations', () => { let licenseService: LicenseService; let notificationPlatformAdapter: NotificationPlatformAdapter; let roleSetAuthorizationService: RoleSetAuthorizationService; + let logger: LoggerService; const mockActorContext = { actorID: 'actor-1', @@ -67,6 +71,7 @@ describe('PlatformRoleResolverMutations', () => { licenseService = module.get(LicenseService); notificationPlatformAdapter = module.get(NotificationPlatformAdapter); roleSetAuthorizationService = module.get(RoleSetAuthorizationService); + logger = module.get(WINSTON_MODULE_NEST_PROVIDER); }); describe('assignPlatformRoleToUser', () => { @@ -182,6 +187,38 @@ describe('PlatformRoleResolverMutations', () => { }) ); }); + + // The notification is dispatched fire-and-forget, so a rejection that + // escapes would become an unhandled rejection and — under Node's default + // `--unhandled-rejections=throw` — take the whole process down rather than + // fail the request. Notifying is best-effort; it must stay contained. + it('should contain a notification dispatch failure and log it', async () => { + const roleData = { + actorID: 'user-target', + role: RoleName.GLOBAL_ADMIN, + }; + ( + notificationPlatformAdapter.platformGlobalRoleChanged as Mock + ).mockRejectedValue( + new TypeError("Cannot read properties of null (reading 'displayName')") + ); + + await expect( + resolver.assignPlatformRoleToUser(mockActorContext, roleData as any) + ).resolves.toBe(mockUser); + + await vi.waitFor(() => + expect(logger.error).toHaveBeenCalledWith( + { + message: 'Notification dispatch failed', + event: 'platformGlobalRoleChanged', + error: expect.stringContaining('TypeError'), + }, + expect.any(String), + LogContext.NOTIFICATIONS + ) + ); + }); }); describe('removePlatformRoleFromUser', () => { @@ -270,5 +307,33 @@ describe('PlatformRoleResolverMutations', () => { }) ); }); + + it('should contain a notification dispatch failure and log it', async () => { + const roleData = { + actorID: 'user-target', + role: RoleName.GLOBAL_ADMIN, + }; + ( + notificationPlatformAdapter.platformGlobalRoleChanged as Mock + ).mockRejectedValue( + new TypeError("Cannot read properties of null (reading 'displayName')") + ); + + await expect( + resolver.removePlatformRoleFromUser(mockActorContext, roleData as any) + ).resolves.toBe(mockUser); + + await vi.waitFor(() => + expect(logger.error).toHaveBeenCalledWith( + { + message: 'Notification dispatch failed', + event: 'platformGlobalRoleChanged', + error: expect.stringContaining('TypeError'), + }, + expect.any(String), + LogContext.NOTIFICATIONS + ) + ); + }); }); }); diff --git a/src/platform/platform-role/platform.role.resolver.mutations.ts b/src/platform/platform-role/platform.role.resolver.mutations.ts index 7c8a10a010..15efe3cea6 100644 --- a/src/platform/platform-role/platform.role.resolver.mutations.ts +++ b/src/platform/platform-role/platform.role.resolver.mutations.ts @@ -187,35 +187,52 @@ export class PlatformRoleResolverMutations { await this.licenseService.saveAll(licenses); } - private async notifyPlatformGlobalRoleChange( + private notifyPlatformGlobalRoleChange( triggeredBy: string, user: IUser, type: RoleChangeType, role: string - ) { + ): void { const notificationInput: NotificationInputPlatformGlobalRoleChange = { triggeredBy, userID: user.id, type: type, role: role, }; - // Both call sites above invoke this WITHOUT `await`, so anything this - // rejects with becomes an unhandled rejection — and under Node's default - // `--unhandled-rejections=throw` that terminates the process rather than - // failing the request. Observed twice in live verification: a user row - // with a null profile made the payload builder throw, and the server - // exited mid-run. Notifying is best-effort by design; it must never be - // able to take the process down, whatever the adapter does next. - try { - await this.notificationPlatformAdapter.platformGlobalRoleChanged( + this.dispatchNotification( + this.notificationPlatformAdapter.platformGlobalRoleChanged( notificationInput - ); - } catch (error: any) { - this.logger.error( - `Unable to dispatch platform global role change notification (user=${user.id}, role=${role}, type=${type}): ${error?.message}`, - error?.stack, + ), + 'platformGlobalRoleChanged' + ); + } + + /** + * Wraps a fire-and-forget notification dispatch with a `.catch` so an + * unhandled rejection from a downstream notification adapter (e.g. a user + * row with a null profile → TypeError while building the payload) does not + * crash the Node process. + * + * The notification is still side-effectful: failures are logged at ERROR + * with structured details so monitoring can pick them up. Notifications + * are intentionally not awaited at the resolver level; we don't want a + * downstream notification problem to fail the user-facing mutation. + */ + private dispatchNotification( + promise: Promise, + eventLabel: string + ): void { + void promise.catch(error => { + const stack = error instanceof Error ? (error.stack ?? '') : ''; + this.logger.error?.( + { + message: 'Notification dispatch failed', + event: eventLabel, + error: String(error), + }, + stack, LogContext.NOTIFICATIONS ); - } + }); } } diff --git a/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts b/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts index 87f848ecfb..e50190cba4 100644 --- a/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts +++ b/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts @@ -330,6 +330,154 @@ describe('NotificationExternalAdapter', () => { }); }); + // A user row with a null profile used to make these payload builders throw a + // TypeError, which — dispatched fire-and-forget — reached Node's default + // `--unhandled-rejections=throw` and killed the server outright. + describe('display name resolution with an incomplete user row', () => { + const buildRemovedPayloadFor = (subject: any, recipient: any) => + adapter.buildPlatformUserRemovedNotificationPayload( + NotificationEvent.PLATFORM_ADMIN_USER_PROFILE_REMOVED, + 'admin-1', + [recipient], + subject + ); + + beforeEach(() => { + vi.mocked(configService.get).mockReturnValue('https://platform.test'); + vi.mocked(urlGeneratorService.createUrlForUserNameID).mockReturnValue( + '/user/1' + ); + }); + + it('should build every payload slot without throwing when profiles are null', async () => { + // Covers all three patched dereferences at once: the subject user, the + // triggering user (fetched), and each recipient. + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: 'Admin', + lastName: 'User', + email: 'admin@test.com', + nameID: 'admin-user', + profile: null, + } as any); + + const result = await buildRemovedPayloadFor( + { + id: 'removed-1', + firstName: 'Removed', + lastName: 'User', + email: 'removed@test.com', + nameID: 'removed-user', + profile: null, + }, + { + id: 'recipient-1', + firstName: null, + lastName: null, + email: 'recipient@test.com', + nameID: 'recipient', + profile: null, + } + ); + + expect(result.user.displayName).toBe('Removed User'); + expect(result.triggeredBy.profile.displayName).toBe('Admin User'); + expect(result.recipients[0].profile.displayName).toBe( + 'recipient@test.com' + ); + }); + + it('should prefer the profile display name over the fallbacks', async () => { + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: 'Admin', + lastName: 'User', + email: 'admin@test.com', + nameID: 'admin-user', + profile: { displayName: 'Preferred Name' }, + } as any); + + const result = await buildRemovedPayloadFor( + { + email: 'removed@test.com', + firstName: 'Removed', + lastName: 'User', + profile: { displayName: 'Removed Display' }, + }, + { + id: 'recipient-1', + email: 'recipient@test.com', + nameID: 'recipient', + profile: { displayName: 'Recipient Display' }, + } + ); + + expect(result.user.displayName).toBe('Removed Display'); + expect(result.triggeredBy.profile.displayName).toBe('Preferred Name'); + expect(result.recipients[0].profile.displayName).toBe( + 'Recipient Display' + ); + }); + + it('should fall through an empty display name and blank names to the email', async () => { + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: ' ', + lastName: '', + email: 'admin@test.com', + nameID: 'admin-user', + profile: { displayName: '' }, + } as any); + + const result = await buildRemovedPayloadFor( + { + email: 'removed@test.com', + firstName: undefined, + lastName: undefined, + profile: undefined, + }, + { + id: 'recipient-1', + firstName: '', + lastName: '', + email: 'recipient@test.com', + nameID: 'recipient', + profile: { displayName: '' }, + } + ); + + expect(result.user.displayName).toBe('removed@test.com'); + expect(result.triggeredBy.profile.displayName).toBe('admin@test.com'); + expect(result.recipients[0].profile.displayName).toBe( + 'recipient@test.com' + ); + }); + + it('should build the global role change payload for a user with no profile', async () => { + // The exact live path: revoking a platform role from an incomplete row. + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'user-1', + firstName: 'Test', + lastName: 'User', + email: 'test@test.com', + nameID: 'test-user', + profile: null, + } as any); + + const result = + await adapter.buildPlatformGlobalRoleChangedNotificationPayload( + NotificationEvent.PLATFORM_ADMIN_GLOBAL_ROLE_CHANGED, + 'admin-1', + [], + 'user-1', + 'REMOVED' as any, + 'GLOBAL_ADMIN' + ); + + expect(result.triggeredBy.profile.displayName).toBe('Test User'); + }); + }); + describe('buildSpaceCommunityApplicationCreatedNotificationPayload', () => { it('should build application payload with applicant', async () => { vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({