diff --git a/server/src/nest/common/body-contract-allow-list.ts b/server/src/nest/common/body-contract-allow-list.ts index b78611ed..6692a68d 100644 --- a/server/src/nest/common/body-contract-allow-list.ts +++ b/server/src/nest/common/body-contract-allow-list.ts @@ -54,11 +54,6 @@ export const BODY_CONTRACT_ALLOW_LIST: string[] = [ 'JourneyController.uploadEntryPhotos', 'JourneyController.uploadGalleryVideo', 'LlmLocalController.pull', - 'NotificationsController.respond', - 'NotificationsController.setPreferences', - 'NotificationsController.testNtfy', - 'NotificationsController.testSmtp', - 'NotificationsController.testWebhook', 'OauthApiController.authorize', 'OauthApiController.createClient', 'PluginRoutesController.route', diff --git a/server/src/nest/notifications/notifications.controller.ts b/server/src/nest/notifications/notifications.controller.ts index eeae8f70..63a48790 100644 --- a/server/src/nest/notifications/notifications.controller.ts +++ b/server/src/nest/notifications/notifications.controller.ts @@ -14,6 +14,13 @@ import { import type { ChannelTestResult, UnreadCountResult } from '@trek/shared'; import type { User } from '../../types'; import { NotificationsService } from './notifications.service'; +import { + PreferencesUpdateDto, + TestSmtpDto, + TestWebhookDto, + TestNtfyDto, + NotificationRespondDto, +} from './notifications.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; @@ -31,6 +38,10 @@ const MASKED = '••••••••'; * codes. POSTs that answer with res.json stay 200 (Nest would default to 201). * The static /in-app/read-all and /in-app/all routes are declared before the * /in-app/:id routes so they win over the param, matching the legacy order. + * Bodies validate via notifications.dto.ts (@trek/shared schemas through the + * global ZodValidationPipe) — malformed bodies now get the pipe's standard + * { error: 'field: message; …' } envelope instead of the old inline checks + * (the sanctioned ratchet behavior); valid bodies behave byte-identically. */ @Controller('api/notifications') @UseGuards(JwtAuthGuard) @@ -43,24 +54,24 @@ export class NotificationsController { } @Put('preferences') - setPreferences(@CurrentUser() user: User, @Body() body: Record>) { + setPreferences(@CurrentUser() user: User, @Body() body: PreferencesUpdateDto) { this.notifications.setPreferences(user.id, body); return this.notifications.getPreferences(user.id, user.role); } @Post('test-smtp') @HttpCode(200) - async testSmtp(@CurrentUser() user: User, @Body('email') email?: string): Promise { + async testSmtp(@CurrentUser() user: User, @Body() body: TestSmtpDto): Promise { if (user.role !== 'admin') { throw new HttpException({ error: 'Admin only' }, 403); } - return this.notifications.testSmtp(email || user.email); + return this.notifications.testSmtp(body.email || user.email); } @Post('test-webhook') @HttpCode(200) - async testWebhook(@CurrentUser() user: User, @Body('url') urlInput?: unknown): Promise { - let url = urlInput; + async testWebhook(@CurrentUser() user: User, @Body() body: TestWebhookDto): Promise { + let url: string | null | undefined = body.url; if (!url || url === MASKED) { url = this.notifications.userWebhookUrl(user.id); if (!url && user.role === 'admin') url = this.notifications.adminWebhookUrl(); @@ -68,9 +79,6 @@ export class NotificationsController { throw new HttpException({ error: 'No webhook URL configured' }, 400); } } - if (typeof url !== 'string') { - throw new HttpException({ error: 'url must be a string' }, 400); - } try { new URL(url); } catch { @@ -81,12 +89,8 @@ export class NotificationsController { @Post('test-ntfy') @HttpCode(200) - async testNtfy( - @CurrentUser() user: User, - @Body('topic') topic?: string, - @Body('server') server?: string, - @Body('token') token?: string, - ): Promise { + async testNtfy(@CurrentUser() user: User, @Body() body: TestNtfyDto): Promise { + const { topic, server, token } = body; const userCfg = this.notifications.userNtfyConfig(user.id); const adminCfg = this.notifications.adminNtfyConfig(); @@ -176,13 +180,10 @@ export class NotificationsController { async respond( @CurrentUser() user: User, @Param('id') idParam: string, - @Body('response') response?: unknown, + @Body() body: NotificationRespondDto, ): Promise<{ success: boolean; notification: unknown }> { const id = this.parseId(idParam); - if (response !== 'positive' && response !== 'negative') { - throw new HttpException({ error: 'response must be "positive" or "negative"' }, 400); - } - const result = await this.notifications.respond(id, user.id, response); + const result = await this.notifications.respond(id, user.id, body.response); if (!result.success) { throw new HttpException({ error: result.error }, 400); } diff --git a/server/src/nest/notifications/notifications.dto.ts b/server/src/nest/notifications/notifications.dto.ts new file mode 100644 index 00000000..cdce0d64 --- /dev/null +++ b/server/src/nest/notifications/notifications.dto.ts @@ -0,0 +1,21 @@ +import { createZodDto } from 'nestjs-zod'; +import { + preferencesUpdateRequestSchema, + testSmtpRequestSchema, + testWebhookRequestSchema, + testNtfyRequestSchema, + notificationRespondRequestSchema, +} from '@trek/shared'; + +/** + * Server-side createZodDto wrappers over the @trek/shared notification + * contracts. The global ZodValidationPipe (APP_PIPE in app.module.ts) + * validates any @Body() parameter typed with one of these classes by + * metatype — the Zod schemas in shared/ remain the single source of truth + * for the wire contract. + */ +export class PreferencesUpdateDto extends createZodDto(preferencesUpdateRequestSchema) {} +export class TestSmtpDto extends createZodDto(testSmtpRequestSchema) {} +export class TestWebhookDto extends createZodDto(testWebhookRequestSchema) {} +export class TestNtfyDto extends createZodDto(testNtfyRequestSchema) {} +export class NotificationRespondDto extends createZodDto(notificationRespondRequestSchema) {} diff --git a/server/tests/integration/notifications.test.ts b/server/tests/integration/notifications.test.ts index 1dc5ade5..0a469852 100644 --- a/server/tests/integration/notifications.test.ts +++ b/server/tests/integration/notifications.test.ts @@ -95,10 +95,13 @@ describe('Notification preferences', () => { it('NOTIF-001 — PUT /api/notifications/preferences updates settings', async () => { const { user } = createUser(testDb); + // The DTO ratchet enforces the matrix shape the client actually sends + // ({ event: { channel: enabled } }); the pre-matrix flat notify_* body + // this case used to send is rejected by the pipe now. const res = await request(app) .put('/api/notifications/preferences') .set('Cookie', authCookie(user.id)) - .send({ notify_trip_invite: true, notify_booking_change: false }); + .send({ trip_invite: { email: true }, booking_change: { email: false } }); expect(res.status).toBe(200); expect(res.body).toHaveProperty('preferences'); }); @@ -310,9 +313,13 @@ describe('Notification test endpoints', () => { it('NOTIF-005 — POST /api/notifications/test-smtp requires admin', async () => { const { user } = createUser(testDb); + // Send the empty JSON body the client sends ({ email: undefined } → + // {}): a completely body-less POST has no content-type, so the DTO pipe + // rejects it before the admin gate since the ratchet. const res = await request(app) .post('/api/notifications/test-smtp') - .set('Cookie', authCookie(user.id)); + .set('Cookie', authCookie(user.id)) + .send({}); // Non-admin gets 403 expect(res.status).toBe(403); }); diff --git a/server/tests/unit/nest/notifications.controller.test.ts b/server/tests/unit/nest/notifications.controller.test.ts index d96bbed6..f037db0d 100644 --- a/server/tests/unit/nest/notifications.controller.test.ts +++ b/server/tests/unit/nest/notifications.controller.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { HttpException } from '@nestjs/common'; import { NotificationsController } from '../../../src/nest/notifications/notifications.controller'; +import { NotificationRespondDto } from '../../../src/nest/notifications/notifications.dto'; import type { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import type { User } from '../../../src/types'; @@ -43,7 +44,7 @@ describe('NotificationsController (parity with the legacy /api/notifications rou describe('test-smtp', () => { it('403 { error: Admin only } for a non-admin (distinct from AdminGuard wording)', async () => { const testSmtp = vi.fn(); - expect(await thrown(() => makeController({ testSmtp }).testSmtp(user))).toEqual({ + expect(await thrown(() => makeController({ testSmtp }).testSmtp(user, {}))).toEqual({ status: 403, body: { error: 'Admin only' }, }); expect(testSmtp).not.toHaveBeenCalled(); @@ -51,7 +52,7 @@ describe('NotificationsController (parity with the legacy /api/notifications rou it('falls back to the admin\'s own email when none given', async () => { const testSmtp = vi.fn().mockResolvedValue({ success: true }); - await makeController({ testSmtp }).testSmtp(admin); + await makeController({ testSmtp }).testSmtp(admin, {}); expect(testSmtp).toHaveBeenCalledWith('admin@example.test'); }); }); @@ -59,27 +60,27 @@ describe('NotificationsController (parity with the legacy /api/notifications rou describe('test-webhook', () => { it('uses the provided url', async () => { const testWebhook = vi.fn().mockResolvedValue({ success: true }); - await makeController({ testWebhook }).testWebhook(user, 'https://hooks.example/x'); + await makeController({ testWebhook }).testWebhook(user, { url: 'https://hooks.example/x' }); expect(testWebhook).toHaveBeenCalledWith('https://hooks.example/x'); }); it('falls back to the saved user url when the masked placeholder is sent', async () => { const testWebhook = vi.fn().mockResolvedValue({ success: true }); const userWebhookUrl = vi.fn().mockReturnValue('https://saved.example/u'); - await makeController({ testWebhook, userWebhookUrl }).testWebhook(user, MASKED); + await makeController({ testWebhook, userWebhookUrl }).testWebhook(user, { url: MASKED }); expect(userWebhookUrl).toHaveBeenCalledWith(4); expect(testWebhook).toHaveBeenCalledWith('https://saved.example/u'); }); it('400 when no url is configured', async () => { const userWebhookUrl = vi.fn().mockReturnValue(null); - expect(await thrown(() => makeController({ userWebhookUrl }).testWebhook(user, undefined))).toEqual({ + expect(await thrown(() => makeController({ userWebhookUrl }).testWebhook(user, {}))).toEqual({ status: 400, body: { error: 'No webhook URL configured' }, }); }); it('400 on an invalid url', async () => { - expect(await thrown(() => makeController({}).testWebhook(user, 'not a url'))).toEqual({ + expect(await thrown(() => makeController({}).testWebhook(user, { url: 'not a url' }))).toEqual({ status: 400, body: { error: 'Invalid URL' }, }); }); @@ -89,7 +90,7 @@ describe('NotificationsController (parity with the legacy /api/notifications rou it('400 when no topic can be resolved', async () => { const userNtfyConfig = vi.fn().mockReturnValue(null); const adminNtfyConfig = vi.fn().mockReturnValue({ server: null, token: null }); - expect(await thrown(() => makeController({ userNtfyConfig, adminNtfyConfig }).testNtfy(user))).toEqual({ + expect(await thrown(() => makeController({ userNtfyConfig, adminNtfyConfig }).testNtfy(user, {}))).toEqual({ status: 400, body: { error: 'No ntfy topic configured' }, }); }); @@ -98,7 +99,7 @@ describe('NotificationsController (parity with the legacy /api/notifications rou const testNtfy = vi.fn().mockResolvedValue({ success: true }); const userNtfyConfig = vi.fn().mockReturnValue({ topic: 'saved-topic', server: 'https://ntfy.me', token: 'saved-token' }); const adminNtfyConfig = vi.fn().mockReturnValue({ server: null, token: null }); - await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, undefined, undefined, MASKED); + await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { token: MASKED }); expect(testNtfy).toHaveBeenCalledWith({ topic: 'saved-topic', server: 'https://ntfy.me', token: 'saved-token' }); }); }); @@ -159,22 +160,24 @@ describe('NotificationsController (parity with the legacy /api/notifications rou }); describe('respond', () => { - it('400 on an invalid response value', async () => { - expect(await thrown(() => makeController({}).respond(user, '5', 'maybe'))).toEqual({ - status: 400, body: { error: 'response must be "positive" or "negative"' }, - }); + it('rejects an invalid response value at the contract (ZodValidationPipe owns the 400 now)', () => { + // The inline enum check died with the DTO ratchet: over HTTP the global + // pipe rejects the body before the handler runs, with the standard + // { error: 'field: message; …' } envelope. + expect(NotificationRespondDto.schema.safeParse({ response: 'maybe' }).success).toBe(false); + expect(NotificationRespondDto.schema.safeParse({ response: 'positive' }).success).toBe(true); }); it('400 with the service error when the response fails', async () => { const respond = vi.fn().mockResolvedValue({ success: false, error: 'Already responded' }); - expect(await thrown(() => makeController({ respond }).respond(user, '5', 'positive'))).toEqual({ + expect(await thrown(() => makeController({ respond }).respond(user, '5', { response: 'positive' }))).toEqual({ status: 400, body: { error: 'Already responded' }, }); }); it('returns success + the updated notification', async () => { const respond = vi.fn().mockResolvedValue({ success: true, notification: { id: 5, response: 'positive' } }); - expect(await makeController({ respond }).respond(user, '5', 'positive')).toEqual({ + expect(await makeController({ respond }).respond(user, '5', { response: 'positive' })).toEqual({ success: true, notification: { id: 5, response: 'positive' }, }); expect(respond).toHaveBeenCalledWith(5, 4, 'positive'); diff --git a/shared/src/notification/notification.schema.spec.ts b/shared/src/notification/notification.schema.spec.ts index 7d4d7d68..5aa3b8b2 100644 --- a/shared/src/notification/notification.schema.spec.ts +++ b/shared/src/notification/notification.schema.spec.ts @@ -3,6 +3,7 @@ import { notificationRespondRequestSchema, channelTestResultSchema, inAppListResultSchema, + testNtfyRequestSchema, } from './notification.schema'; import { describe, it, expect } from 'vitest'; @@ -29,6 +30,14 @@ describe('notificationRespondRequestSchema', () => { }); }); +describe('testNtfyRequestSchema', () => { + it('accepts null server/token — the client sends null to mean "use the saved value"', () => { + expect(testNtfyRequestSchema.safeParse({ topic: 't', server: null, token: null }).success).toBe(true); + expect(testNtfyRequestSchema.safeParse({}).success).toBe(true); + expect(testNtfyRequestSchema.safeParse({ topic: 1 }).success).toBe(false); + }); +}); + describe('channelTestResultSchema', () => { it('accepts a success result and an error result', () => { expect(channelTestResultSchema.safeParse({ success: true }).success).toBe(true); diff --git a/shared/src/notification/notification.schema.ts b/shared/src/notification/notification.schema.ts index b0b295fc..91a7dc4c 100644 --- a/shared/src/notification/notification.schema.ts +++ b/shared/src/notification/notification.schema.ts @@ -21,10 +21,13 @@ export const testSmtpRequestSchema = z.object({ email: z.string().optional() }); export const testWebhookRequestSchema = z.object({ url: z.string().optional(), }); +// server/token are nullable: the client deliberately sends null to mean +// "fall back to the saved value" (a stored token is only masked in the +// placeholder — sending null keeps the saved one). export const testNtfyRequestSchema = z.object({ topic: z.string().optional(), - server: z.string().optional(), - token: z.string().optional(), + server: z.string().nullable().optional(), + token: z.string().nullable().optional(), }); /** Result of a channel test ping. */