From ac49191ec8788aa90a0a2774944969b9018e82c4 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 10:46:17 +0200 Subject: [PATCH] Describe the journey bodies instead of tightening them Sixteen of the thirty-four entries in the body-contract allow list were the JourneyController, and a strict schema already existed for six of them. That is the trap: eight of these handlers validate their own body and answer with their own text - 'Title is required', 'trip_id required', 'provider and asset_id required', 'entry_date is required', 'user_id required' - and those bodies are pinned by e2e and integration cases. Adopting the strict schemas would have the pipe answer first, with a different body. That is a client-visible change wearing the costume of a refactor. So the schemas follow the memories doctrine instead: loose objects, and every field the handler type-checks itself is unknown here. They describe the shape that already works, which is what the boot gate needs, and every hand-rolled 400 keeps answering. The spec inverts accordingly - it now asserts that each schema ACCEPTS what the handler still rejects, which is the property that makes the claim checkable. Two of them are optional at the top level, and that is not decoration: a multipart upload with files and no text fields arrives with no body at all, and JOURNEY-INT-045 pins 'No files uploaded' for it. Without the optional the pipe answered 'body: Invalid input: expected object' instead. The suite caught it. Backup joins them - its schema was already a passthrough, because the service normalises the body itself. Allow list: 34 entries down to 17. Tags and the two OAuth entries stay for now; tags reads @Body('name') per property, which is a signature change rather than a contract one. --- server/src/nest/backup/backup.controller.ts | 3 +- server/src/nest/backup/backup.dto.ts | 12 ++ .../nest/common/body-contract-allow-list.ts | 17 --- server/src/nest/journey/journey.controller.ts | 62 +++++---- server/src/nest/journey/journey.dto.ts | 51 +++++++ shared/src/journey/journey.schema.spec.ts | 93 ++++++++----- shared/src/journey/journey.schema.ts | 130 ++++++++++++++---- 7 files changed, 262 insertions(+), 106 deletions(-) create mode 100644 server/src/nest/backup/backup.dto.ts create mode 100644 server/src/nest/journey/journey.dto.ts diff --git a/server/src/nest/backup/backup.controller.ts b/server/src/nest/backup/backup.controller.ts index 567c3df7a..0cac39833 100644 --- a/server/src/nest/backup/backup.controller.ts +++ b/server/src/nest/backup/backup.controller.ts @@ -23,6 +23,7 @@ import { BackupService } from './backup.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { AdminGuard } from '../auth/admin.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { AutoBackupSettingsDto } from './backup.dto'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; import { getUploadTmpDir, MAX_BACKUP_UPLOAD_SIZE } from './backup.impl'; @@ -142,7 +143,7 @@ export class BackupController { } @Put('auto-settings') - updateAutoSettings(@CurrentUser() user: User, @Body() body: Record, @Req() req: Request) { + updateAutoSettings(@CurrentUser() user: User, @Body() body: AutoBackupSettingsDto, @Req() req: Request) { try { const settings = this.backup.updateAutoSettings(body || {}); this.audit.writeAudit({ userId: user.id, action: 'backup.auto_settings', ip: getClientIp(req), details: { enabled: settings.enabled, interval: settings.interval, keep_days: settings.keep_days } }); diff --git a/server/src/nest/backup/backup.dto.ts b/server/src/nest/backup/backup.dto.ts new file mode 100644 index 000000000..663e8d616 --- /dev/null +++ b/server/src/nest/backup/backup.dto.ts @@ -0,0 +1,12 @@ +import { createZodDto } from 'nestjs-zod'; +import { autoBackupSettingsRequestSchema } from '@trek/shared'; + +/** + * Server-side createZodDto wrapper over the @trek/shared backup contract. + * + * The schema is a passthrough that pins only the well-known toggles: the body is + * normalised server-side by parseAutoBackupBody, which coerces strings and + * clamps ranges, and the handler maps its throw to the 400. Tightening here + * would take that answer away from the service. + */ +export class AutoBackupSettingsDto extends createZodDto(autoBackupSettingsRequestSchema) {} diff --git a/server/src/nest/common/body-contract-allow-list.ts b/server/src/nest/common/body-contract-allow-list.ts index e2df38e66..e4384485e 100644 --- a/server/src/nest/common/body-contract-allow-list.ts +++ b/server/src/nest/common/body-contract-allow-list.ts @@ -8,26 +8,9 @@ * here — the gate throws on stale entries, so removal is enforced too. */ export const BODY_CONTRACT_ALLOW_LIST: string[] = [ - 'BackupController.updateAutoSettings', 'BookingImportController.confirm', 'BookingImportController.preview', 'BookingImportController.previewAsync', - 'JourneyController.addContributor', - 'JourneyController.addTrip', - 'JourneyController.create', - 'JourneyController.createEntry', - 'JourneyController.galleryProviderPhotos', - 'JourneyController.linkPhoto', - 'JourneyController.preferences', - 'JourneyController.providerPhotos', - 'JourneyController.reorderEntries', - 'JourneyController.setShareLink', - 'JourneyController.update', - 'JourneyController.updateContributor', - 'JourneyController.updateEntry', - 'JourneyController.updatePhoto', - 'JourneyController.uploadEntryPhotos', - 'JourneyController.uploadGalleryVideo', 'LlmLocalController.pull', 'OauthApiController.authorize', 'OauthApiController.createClient', diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index 062b2fee6..34b56357c 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -27,6 +27,12 @@ import { RequireAddon } from '../addons/require-addon.decorator'; import { ADDON_IDS } from '../../addons'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { + JourneyAddTripDto, JourneyContributorAddDto, JourneyContributorUpdateDto, JourneyCreateDto, + JourneyEntryCreateDto, JourneyEntryPhotoUploadDto, JourneyEntryUpdateDto, JourneyGalleryVideoDto, + JourneyLinkPhotoDto, JourneyPhotoUpdateDto, JourneyPreferencesDto, JourneyProviderPhotosDto, + JourneyReorderEntriesDto, JourneyShareLinkDto, JourneyUpdateDto, +} from './journey.dto'; import { isVideoMime, isVideoExtension, MAX_VIDEO_SIZE } from '../files/files.constants'; import { getAllowedExtensions } from '../files/files.bridge'; @@ -113,13 +119,15 @@ export class JourneyController { } @Post() - create(@CurrentUser() user: User, @Body() body: { title?: string; subtitle?: string; trip_ids?: unknown[] }) { + create(@CurrentUser() user: User, @Body() body: JourneyCreateDto) { if (!body.title || typeof body.title !== 'string' || !body.title.trim()) { throw new HttpException({ error: 'Title is required' }, 400); } return this.journey.createJourney(user.id, { - title: body.title.trim(), - subtitle: body.subtitle, + // The guard three lines up proved both of these; the schema stays + // permissive so that guard keeps answering, not the pipe. + title: (body.title as string).trim(), + subtitle: body.subtitle as string | undefined, trip_ids: Array.isArray(body.trip_ids) ? body.trip_ids.map(Number) : [], }); } @@ -136,7 +144,7 @@ export class JourneyController { // ── Entries (prefix /entries — before /:id) ───────────────────────────── @Patch('entries/:entryId') - updateEntry(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: Record, @Headers('x-socket-id') socketId?: string) { + updateEntry(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: JourneyEntryUpdateDto, @Headers('x-socket-id') socketId?: string) { const result = this.journey.updateEntry(Number(entryId), user.id, body, socketId); if (!result) { throw new HttpException({ error: 'Entry not found' }, 404); @@ -154,14 +162,14 @@ export class JourneyController { @Post('entries/:entryId/photos') @UseInterceptors(FilesInterceptor('photos', undefined, IMAGE_UPLOAD)) - async uploadEntryPhotos(@CurrentUser() user: User, @Param('entryId') entryId: string, @UploadedFiles() files: Express.Multer.File[] | undefined, @Body() body: { caption?: string }) { + async uploadEntryPhotos(@CurrentUser() user: User, @Param('entryId') entryId: string, @UploadedFiles() files: Express.Multer.File[] | undefined, @Body() body: JourneyEntryPhotoUploadDto) { if (!files?.length) { throw new HttpException({ error: 'No files uploaded' }, 400); } const results: unknown[] = []; for (const file of files) { const relativePath = `journey/${file.filename}`; - const photo = this.journey.addPhoto(Number(entryId), user.id, relativePath, undefined, body?.caption); + const photo = this.journey.addPhoto(Number(entryId), user.id, relativePath, undefined, body?.caption as string | undefined); if (!photo) continue; // Mirror to Immich only when the user explicitly opted in (#730). if (this.journey.immichAutoUploadEnabled(user.id)) { @@ -184,13 +192,13 @@ export class JourneyController { } @Post('entries/:entryId/provider-photos') - providerPhotos(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: { provider?: string; asset_id?: string; asset_ids?: unknown[]; caption?: string; passphrase?: string; media_type?: string; media_types?: unknown[] }) { + providerPhotos(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: JourneyProviderPhotosDto) { const pp = body.passphrase && typeof body.passphrase === 'string' ? body.passphrase : undefined; if (Array.isArray(body.asset_ids) && body.provider) { const added: unknown[] = []; body.asset_ids.forEach((id, i) => { const mt = Array.isArray(body.media_types) && body.media_types[i] === 'video' ? 'video' : 'image'; - const photo = this.journey.addProviderPhoto(Number(entryId), user.id, body.provider!, String(id), body.caption, pp, mt); + const photo = this.journey.addProviderPhoto(Number(entryId), user.id, String(body.provider), String(id), body.caption as string | undefined, pp, mt); if (photo) added.push(photo); }); return { photos: added, added: added.length }; @@ -198,7 +206,7 @@ export class JourneyController { if (!body.provider || !body.asset_id) { throw new HttpException({ error: 'provider and asset_id required' }, 400); } - const photo = this.journey.addProviderPhoto(Number(entryId), user.id, body.provider, body.asset_id, body.caption, pp, body.media_type === 'video' ? 'video' : 'image'); + const photo = this.journey.addProviderPhoto(Number(entryId), user.id, String(body.provider), String(body.asset_id), body.caption as string | undefined, pp, body.media_type === 'video' ? 'video' : 'image'); if (!photo) { throw new HttpException({ error: 'Not allowed or duplicate' }, 403); } @@ -206,7 +214,7 @@ export class JourneyController { } @Post('entries/:entryId/link-photo') - linkPhoto(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: { journey_photo_id?: unknown; photo_id?: unknown }) { + linkPhoto(@CurrentUser() user: User, @Param('entryId') entryId: string, @Body() body: JourneyLinkPhotoDto) { const journeyPhotoId = body.journey_photo_id ?? body.photo_id; if (!journeyPhotoId) { throw new HttpException({ error: 'journey_photo_id required' }, 400); @@ -227,7 +235,7 @@ export class JourneyController { } @Patch('photos/:photoId') - updatePhoto(@CurrentUser() user: User, @Param('photoId') photoId: string, @Body() body: Record) { + updatePhoto(@CurrentUser() user: User, @Param('photoId') photoId: string, @Body() body: JourneyPhotoUpdateDto) { const result = this.journey.updatePhoto(Number(photoId), user.id, body); if (!result) { throw new HttpException({ error: 'Photo not found' }, 404); @@ -268,7 +276,7 @@ export class JourneyController { @CurrentUser() user: User, @Param('id') id: string, @UploadedFiles() files: { video?: Express.Multer.File[]; poster?: Express.Multer.File[] } | undefined, - @Body() body: { duration_ms?: string }, + @Body() body: JourneyGalleryVideoDto, ) { const video = files?.video?.[0]; const poster = files?.poster?.[0]; @@ -298,13 +306,13 @@ export class JourneyController { } @Post(':id/gallery/provider-photos') - galleryProviderPhotos(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { provider?: string; asset_id?: string; asset_ids?: unknown[]; passphrase?: string; media_type?: string; media_types?: unknown[] }) { + galleryProviderPhotos(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyProviderPhotosDto) { const pp = body.passphrase && typeof body.passphrase === 'string' ? body.passphrase : undefined; if (Array.isArray(body.asset_ids) && body.provider) { const added: unknown[] = []; body.asset_ids.forEach((aid, i) => { const mt = Array.isArray(body.media_types) && body.media_types[i] === 'video' ? 'video' : 'image'; - const photo = this.journey.addProviderPhotoToGallery(Number(id), user.id, body.provider!, String(aid), undefined, pp, mt); + const photo = this.journey.addProviderPhotoToGallery(Number(id), user.id, String(body.provider), String(aid), undefined, pp, mt); if (photo) added.push(photo); }); return { photos: added, added: added.length }; @@ -312,7 +320,7 @@ export class JourneyController { if (!body.provider || !body.asset_id) { throw new HttpException({ error: 'provider and asset_id required' }, 400); } - const photo = this.journey.addProviderPhotoToGallery(Number(id), user.id, body.provider, body.asset_id, undefined, pp, body.media_type === 'video' ? 'video' : 'image'); + const photo = this.journey.addProviderPhotoToGallery(Number(id), user.id, String(body.provider), String(body.asset_id), undefined, pp, body.media_type === 'video' ? 'video' : 'image'); if (!photo) { throw new HttpException({ error: 'Not allowed or duplicate' }, 403); } @@ -342,7 +350,7 @@ export class JourneyController { } @Patch(':id') - update(@CurrentUser() user: User, @Param('id') id: string, @Body() body: Record) { + update(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyUpdateDto) { const result = this.journey.updateJourney(Number(id), user.id, body); if (!result) { throw new HttpException({ error: 'Journey not found' }, 404); @@ -375,7 +383,7 @@ export class JourneyController { // ── Journey trips ─────────────────────────────────────────────────────── @Post(':id/trips') @HttpCode(200) // Express answers with res.json (200). - addTrip(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { trip_id?: unknown }) { + addTrip(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyAddTripDto) { if (!body.trip_id) { throw new HttpException({ error: 'trip_id required' }, 400); } @@ -404,7 +412,7 @@ export class JourneyController { } @Post(':id/entries') - createEntry(@CurrentUser() user: User, @Param('id') id: string, @Body() body: Record & { entry_date?: unknown }, @Headers('x-socket-id') socketId?: string) { + createEntry(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyEntryCreateDto, @Headers('x-socket-id') socketId?: string) { if (!body.entry_date) { throw new HttpException({ error: 'entry_date is required' }, 400); } @@ -416,7 +424,7 @@ export class JourneyController { } @Put(':id/entries/reorder') - reorderEntries(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { orderedIds?: unknown }, @Headers('x-socket-id') socketId?: string) { + reorderEntries(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyReorderEntriesDto, @Headers('x-socket-id') socketId?: string) { const orderedIds = body.orderedIds; if (!Array.isArray(orderedIds) || !orderedIds.every((v) => Number.isFinite(Number(v)))) { throw new HttpException({ error: 'orderedIds must be an array of numbers' }, 400); @@ -429,18 +437,18 @@ export class JourneyController { // ── Contributors ──────────────────────────────────────────────────────── @Post(':id/contributors') - addContributor(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { user_id?: unknown; role?: 'editor' | 'viewer' }) { + addContributor(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyContributorAddDto) { if (!body.user_id) { throw new HttpException({ error: 'user_id required' }, 400); } - if (!this.journey.addContributor(Number(id), user.id, Number(body.user_id), body.role || 'viewer')) { + if (!this.journey.addContributor(Number(id), user.id, Number(body.user_id), (body.role as 'editor' | 'viewer') || 'viewer')) { throw new HttpException({ error: 'Not allowed' }, 403); } return { success: true }; } @Patch(':id/contributors/:userId') - updateContributor(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string, @Body() body: { role?: 'editor' | 'viewer' }) { + updateContributor(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string, @Body() body: JourneyContributorUpdateDto) { if (!this.journey.updateContributorRole(Number(id), user.id, Number(userId), body.role as 'editor' | 'viewer')) { throw new HttpException({ error: 'Not allowed' }, 403); } @@ -457,7 +465,7 @@ export class JourneyController { // ── User Preferences ──────────────────────────────────────────────────── @Patch(':id/preferences') - preferences(@CurrentUser() user: User, @Param('id') id: string, @Body() body: Record) { + preferences(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyPreferencesDto) { const result = this.journey.updateJourneyPreferences(Number(id), user.id, body); if (!result) { throw new HttpException({ error: 'Not allowed' }, 403); @@ -473,11 +481,11 @@ export class JourneyController { @Post(':id/share-link') @HttpCode(200) // Express answers with res.json (200). - setShareLink(@CurrentUser() user: User, @Param('id') id: string, @Body() body: { share_timeline?: boolean; share_gallery?: boolean; share_map?: boolean }) { + setShareLink(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyShareLinkDto) { const result = this.journey.createOrUpdateJourneyShareLink(Number(id), user.id, { - share_timeline: body.share_timeline, - share_gallery: body.share_gallery, - share_map: body.share_map, + share_timeline: body.share_timeline as boolean | undefined, + share_gallery: body.share_gallery as boolean | undefined, + share_map: body.share_map as boolean | undefined, }); if (!result) { throw new HttpException({ error: 'Not allowed' }, 403); diff --git a/server/src/nest/journey/journey.dto.ts b/server/src/nest/journey/journey.dto.ts new file mode 100644 index 000000000..d1df7e371 --- /dev/null +++ b/server/src/nest/journey/journey.dto.ts @@ -0,0 +1,51 @@ +import { createZodDto } from 'nestjs-zod'; +import { + journeyAddTripRequestSchema, + journeyContributorRequestSchema, + journeyContributorUpdateRequestSchema, + journeyCreateRequestSchema, + journeyEntryCreateRequestSchema, + journeyEntryPhotoUploadRequestSchema, + journeyEntryUpdateRequestSchema, + journeyGalleryVideoRequestSchema, + journeyLinkPhotoRequestSchema, + journeyPhotoUpdateRequestSchema, + journeyPreferencesRequestSchema, + journeyProviderPhotosRequestSchema, + journeyReorderEntriesRequestSchema, + journeyShareLinkRequestSchema, + journeyUpdateRequestSchema, +} from '@trek/shared'; + +/** + * Server-side createZodDto wrappers over the @trek/shared journey 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. + * + * The schemas behind them are deliberately permissive, and the bespoke 400s stay + * in the controller: eight of these handlers answer with their own message + * ('Title is required', 'trip_id required', 'provider and asset_id required', + * 'journey_photo_id required', 'entry_date is required', 'user_id required'), + * and those bodies are pinned. A guard-tight schema would have the pipe answer + * first, with a different body — a client-visible change, not a refactor. + * + * These sixteen routes were the largest block in + * common/body-contract-allow-list.ts; the boot gate throws on stale entries, so + * they were removed in the same change. + */ +export class JourneyCreateDto extends createZodDto(journeyCreateRequestSchema) {} +export class JourneyUpdateDto extends createZodDto(journeyUpdateRequestSchema) {} +export class JourneyAddTripDto extends createZodDto(journeyAddTripRequestSchema) {} +export class JourneyEntryCreateDto extends createZodDto(journeyEntryCreateRequestSchema) {} +export class JourneyEntryUpdateDto extends createZodDto(journeyEntryUpdateRequestSchema) {} +export class JourneyReorderEntriesDto extends createZodDto(journeyReorderEntriesRequestSchema) {} +export class JourneyContributorAddDto extends createZodDto(journeyContributorRequestSchema) {} +export class JourneyContributorUpdateDto extends createZodDto(journeyContributorUpdateRequestSchema) {} +export class JourneyPreferencesDto extends createZodDto(journeyPreferencesRequestSchema) {} +export class JourneyShareLinkDto extends createZodDto(journeyShareLinkRequestSchema) {} +export class JourneyProviderPhotosDto extends createZodDto(journeyProviderPhotosRequestSchema) {} +export class JourneyLinkPhotoDto extends createZodDto(journeyLinkPhotoRequestSchema) {} +export class JourneyPhotoUpdateDto extends createZodDto(journeyPhotoUpdateRequestSchema) {} +export class JourneyEntryPhotoUploadDto extends createZodDto(journeyEntryPhotoUploadRequestSchema) {} +export class JourneyGalleryVideoDto extends createZodDto(journeyGalleryVideoRequestSchema) {} diff --git a/shared/src/journey/journey.schema.spec.ts b/shared/src/journey/journey.schema.spec.ts index bb7230d8b..659cb2a4b 100644 --- a/shared/src/journey/journey.schema.spec.ts +++ b/shared/src/journey/journey.schema.spec.ts @@ -1,3 +1,4 @@ +import { describe, it, expect } from 'vitest'; import { journeyCreateRequestSchema, journeyAddTripRequestSchema, @@ -5,67 +6,91 @@ import { journeyContributorRequestSchema, journeyProviderPhotosRequestSchema, journeyShareLinkRequestSchema, + journeyUpdateRequestSchema, + journeyPreferencesRequestSchema, + journeyLinkPhotoRequestSchema, } from './journey.schema'; -import { describe, it, expect } from 'vitest'; - +/** + * These schemas describe the journey bodies rather than tighten them, and the + * cases below are what makes that checkable: every shape the handler still + * rejects itself has to PARSE here, or the global pipe would answer first with + * a different error body than the one the e2e cases pin. + * + * This inverts the assertions the file carried while the schemas were strict. + * That was the point of the change. + */ describe('journeyCreateRequestSchema', () => { - it('requires a title; subtitle + trip_ids optional', () => { + it('accepts the well-formed body', () => { expect(journeyCreateRequestSchema.safeParse({ title: 'Trip of a lifetime' }).success).toBe(true); expect(journeyCreateRequestSchema.safeParse({ title: 'X', trip_ids: [1, '2'] }).success).toBe(true); - expect(journeyCreateRequestSchema.safeParse({ subtitle: 'no title' }).success).toBe(false); + }); + + it('leaves the missing/blank/non-string title to the handler', () => { + // The handler answers 'Title is required' for all three. + expect(journeyCreateRequestSchema.safeParse({ subtitle: 'no title' }).success).toBe(true); + expect(journeyCreateRequestSchema.safeParse({ title: ' ' }).success).toBe(true); + expect(journeyCreateRequestSchema.safeParse({ title: 42 }).success).toBe(true); + }); + + it('leaves a non-array trip_ids to the handler, which reads it as none', () => { + expect(journeyCreateRequestSchema.safeParse({ title: 'X', trip_ids: 'nope' }).success).toBe(true); }); }); describe('journeyAddTripRequestSchema', () => { - it('requires a trip_id (string or number)', () => { + it('accepts an id and leaves the missing one to the handler', () => { expect(journeyAddTripRequestSchema.safeParse({ trip_id: 5 }).success).toBe(true); expect(journeyAddTripRequestSchema.safeParse({ trip_id: '5' }).success).toBe(true); - expect(journeyAddTripRequestSchema.safeParse({}).success).toBe(false); + // The handler answers 'trip_id required'. + expect(journeyAddTripRequestSchema.safeParse({}).success).toBe(true); }); }); describe('journeyReorderEntriesRequestSchema', () => { - it('requires a non-empty orderedIds array', () => { - expect(journeyReorderEntriesRequestSchema.safeParse({ orderedIds: [3, 1, 2] }).success).toBe(true); - expect(journeyReorderEntriesRequestSchema.safeParse({ orderedIds: [] }).success).toBe(false); + it('accepts a list, and an empty one, which the handler short-circuits', () => { + expect(journeyReorderEntriesRequestSchema.safeParse({ orderedIds: [3, '1'] }).success).toBe(true); + expect(journeyReorderEntriesRequestSchema.safeParse({ orderedIds: [] }).success).toBe(true); + expect(journeyReorderEntriesRequestSchema.safeParse({}).success).toBe(true); }); }); describe('journeyContributorRequestSchema', () => { - it('requires user_id; role limited to editor/viewer', () => { - expect(journeyContributorRequestSchema.safeParse({ user_id: 2 }).success).toBe(true); - expect(journeyContributorRequestSchema.safeParse({ user_id: 2, role: 'editor' }).success).toBe(true); - expect(journeyContributorRequestSchema.safeParse({ user_id: 2, role: 'admin' }).success).toBe(false); + it('accepts the roles and leaves an unknown one to the handler', () => { + expect(journeyContributorRequestSchema.safeParse({ user_id: 3, role: 'editor' }).success).toBe(true); + expect(journeyContributorRequestSchema.safeParse({ user_id: 3 }).success).toBe(true); + // Never validated: the handler casts it. + expect(journeyContributorRequestSchema.safeParse({ user_id: 3, role: 'admin' }).success).toBe(true); + // The handler answers 'user_id required'. + expect(journeyContributorRequestSchema.safeParse({}).success).toBe(true); }); }); describe('journeyProviderPhotosRequestSchema', () => { - it('requires a provider; accepts single asset_id or a batch', () => { - expect( - journeyProviderPhotosRequestSchema.safeParse({ - provider: 'immich', - asset_id: 'a1', - }).success, - ).toBe(true); - expect( - journeyProviderPhotosRequestSchema.safeParse({ - provider: 'immich', - asset_ids: ['a1', 'a2'], - }).success, - ).toBe(true); - expect(journeyProviderPhotosRequestSchema.safeParse({ asset_id: 'a1' }).success).toBe(false); + it('accepts single and batch, and leaves the incomplete pair to the handler', () => { + expect(journeyProviderPhotosRequestSchema.safeParse({ provider: 'immich', asset_id: 'a1' }).success).toBe(true); + expect(journeyProviderPhotosRequestSchema.safeParse({ provider: 'immich', asset_ids: ['a1', 2], media_types: ['video'] }).success).toBe(true); + // The handler answers 'provider and asset_id required'. + expect(journeyProviderPhotosRequestSchema.safeParse({ asset_id: 'a1' }).success).toBe(true); + expect(journeyProviderPhotosRequestSchema.safeParse({}).success).toBe(true); }); }); describe('journeyShareLinkRequestSchema', () => { - it('accepts optional share toggles', () => { - expect( - journeyShareLinkRequestSchema.safeParse({ - share_timeline: true, - share_gallery: false, - }).success, - ).toBe(true); + it('accepts the flags in whatever shape the client sends them', () => { + expect(journeyShareLinkRequestSchema.safeParse({ share_timeline: true, share_gallery: false }).success).toBe(true); expect(journeyShareLinkRequestSchema.safeParse({}).success).toBe(true); + // The controller coerces with !!; a string flag has always worked. + expect(journeyShareLinkRequestSchema.safeParse({ share_map: 'true' }).success).toBe(true); + }); +}); + +describe('the free-form bodies keep their unknown keys', () => { + it('does not strip what the services are handed whole', () => { + // The whole reason these are loose objects: a strict object drops unknown + // keys, and update/preferences/link are forwarded to the service as sent. + expect(journeyUpdateRequestSchema.parse({ title: 'X', cover_photo_id: 9 })).toEqual({ title: 'X', cover_photo_id: 9 }); + expect(journeyPreferencesRequestSchema.parse({ layout: 'grid', density: 2 })).toEqual({ layout: 'grid', density: 2 }); + expect(journeyLinkPhotoRequestSchema.parse({ journey_photo_id: 4, extra: true })).toEqual({ journey_photo_id: 4, extra: true }); }); }); diff --git a/shared/src/journey/journey.schema.ts b/shared/src/journey/journey.schema.ts index b88b97d68..a8e2fd1b1 100644 --- a/shared/src/journey/journey.schema.ts +++ b/shared/src/journey/journey.schema.ts @@ -7,50 +7,126 @@ import { z } from 'zod'; * * Authenticated routes live under /api/journeys (gated by the Journey addon); * the public read/photo-proxy routes live under /api/public/journey and are - * share-token validated. Access control lives inside journeyService (it returns - * null/false → the controller maps to 403/404), so these schemas pin the - * well-defined request bodies; entry create/update stay open-ended (forwarded - * to the service) and the bespoke 400/403/404 messages pin the rest. + * share-token validated. Access control lives inside the journey services (they + * return null/false → the controller maps to 403/404). + * + * Deliberately permissive, the same doctrine as memories (#1842): eight of the + * sixteen handlers validate their own body and answer with their own text — + * 'Title is required', 'trip_id required', 'provider and asset_id required', + * and so on — and those messages are pinned by e2e and integration cases. A + * strict schema would have the global pipe answer first, with a different body, + * which is a client-visible change wearing the costume of a refactor. So every + * field the handler type-checks itself is `unknown` here: the schema describes + * the shape that already works, and the boot-time contract gate gets something + * to point at. + * + * They are LOOSE objects for the second memories reason: a strict object strips + * unknown keys, and the update/entry/preferences bodies are forwarded to the + * services whole. Stripping there would silently drop a field added later. */ -export const journeyCreateRequestSchema = z.object({ - title: z.string().min(1), - subtitle: z.string().optional(), - trip_ids: z.array(z.union([z.string(), z.number()])).optional(), +export const journeyCreateRequestSchema = z.looseObject({ + // The handler does `!body.title || typeof body.title !== 'string' || !trim()` + // and answers 'Title is required'. Typing it here would pre-empt that. + title: z.unknown().optional(), + subtitle: z.unknown().optional(), + // `Array.isArray(body.trip_ids) ? body.trip_ids.map(Number) : []` — anything + // that is not an array is silently treated as none. + trip_ids: z.unknown().optional(), }); export type JourneyCreateRequest = z.infer; -export const journeyAddTripRequestSchema = z.object({ - trip_id: z.union([z.string(), z.number()]), +/** Free-form: forwarded to the service as-is. */ +export const journeyUpdateRequestSchema = z.looseObject({}); +export type JourneyUpdateRequest = z.infer; + +export const journeyAddTripRequestSchema = z.looseObject({ + // Handler answers 'trip_id required'. + trip_id: z.unknown().optional(), }); export type JourneyAddTripRequest = z.infer; -export const journeyReorderEntriesRequestSchema = z.object({ - orderedIds: z.array(z.union([z.string(), z.number()])).min(1), +export const journeyReorderEntriesRequestSchema = z.looseObject({ + // No .min(1): an empty list is accepted today and short-circuits in the + // handler, and the handler's own 'orderedIds must be...' check is the one + // that answers for a malformed list. + orderedIds: z.unknown().optional(), }); export type JourneyReorderEntriesRequest = z.infer; -export const journeyContributorRequestSchema = z.object({ - user_id: z.union([z.string(), z.number()]), - role: z.enum(['editor', 'viewer']).optional(), +export const journeyContributorRequestSchema = z.looseObject({ + // Handler answers 'user_id required'; role is cast, never validated. + user_id: z.unknown().optional(), + role: z.unknown().optional(), }); export type JourneyContributorRequest = z.infer; -export const journeyProviderPhotosRequestSchema = z.object({ - provider: z.string().min(1), - asset_id: z.string().optional(), - asset_ids: z.array(z.union([z.string(), z.number()])).optional(), - caption: z.string().optional(), - passphrase: z.string().optional(), +export const journeyContributorUpdateRequestSchema = z.looseObject({ + role: z.unknown().optional(), +}); +export type JourneyContributorUpdateRequest = z.infer; + +export const journeyProviderPhotosRequestSchema = z.looseObject({ + // Handler answers 'provider and asset_id required' when either is missing, + // and branches on Array.isArray(asset_ids) before that. + provider: z.unknown().optional(), + asset_id: z.unknown().optional(), + asset_ids: z.unknown().optional(), + caption: z.unknown().optional(), + passphrase: z.unknown().optional(), // Per-asset 'image' | 'video' discriminator, parallel to asset_ids (#823). - media_type: z.string().optional(), - media_types: z.array(z.string()).optional(), + media_type: z.unknown().optional(), + media_types: z.unknown().optional(), }); export type JourneyProviderPhotosRequest = z.infer; -export const journeyShareLinkRequestSchema = z.object({ - share_timeline: z.boolean().optional(), - share_gallery: z.boolean().optional(), - share_map: z.boolean().optional(), +export const journeyLinkPhotoRequestSchema = z.looseObject({ + // Handler answers 'journey_photo_id required'. + journey_photo_id: z.unknown().optional(), + photo_id: z.unknown().optional(), +}); +export type JourneyLinkPhotoRequest = z.infer; + +/** Free-form: forwarded to the service as-is. */ +export const journeyPhotoUpdateRequestSchema = z.looseObject({}); +export type JourneyPhotoUpdateRequest = z.infer; + +/** Free-form plus the entry_date the handler demands itself. */ +export const journeyEntryCreateRequestSchema = z.looseObject({ + entry_date: z.unknown().optional(), +}); +export type JourneyEntryCreateRequest = z.infer; + +/** Free-form: forwarded to the service as-is. */ +export const journeyEntryUpdateRequestSchema = z.looseObject({}); +export type JourneyEntryUpdateRequest = z.infer; + +/** + * Multipart field alongside the uploaded files. + * + * `.optional()` is not decoration: a multipart request with files and no text + * fields arrives with NO body at all, and the handler answers 'No files + * uploaded' for the empty case. Without this the pipe would answer + * 'body: Invalid input: expected object' first and the message would change. + */ +export const journeyEntryPhotoUploadRequestSchema = z.looseObject({ + caption: z.unknown().optional(), +}).optional(); +export type JourneyEntryPhotoUploadRequest = z.infer; + +/** Multipart field alongside the uploaded video — absent body for the same reason. */ +export const journeyGalleryVideoRequestSchema = z.looseObject({ + duration_ms: z.unknown().optional(), +}).optional(); +export type JourneyGalleryVideoRequest = z.infer; + +/** Free-form: per-user display preferences, forwarded whole. */ +export const journeyPreferencesRequestSchema = z.looseObject({}); +export type JourneyPreferencesRequest = z.infer; + +export const journeyShareLinkRequestSchema = z.looseObject({ + share_timeline: z.unknown().optional(), + share_gallery: z.unknown().optional(), + share_map: z.unknown().optional(), }); export type JourneyShareLinkRequest = z.infer;