From 8c0280e28700314bcbca1aeefc56a472bb14a374 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 9 Aug 2026 19:12:51 +0200 Subject: [PATCH] Replace the copied requireTrip helpers with a TripAccessGuard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every trip-scoped controller carried its own three-line requireTrip, and a requireEdit next to it: eighteen copies of a security check, each of which had to remember that no access is a 404 'Trip not found' and never a 403 — the distinction that stops a stranger learning which trip ids exist. One of them getting it wrong would have looked like nothing. TripAccessGuard resolves :tripId once, refuses what the user cannot reach and hands the row to the handler through a @Trip() param decorator (a param decorator rather than a req.trip cast, because the request type that used to be widened for that is gone). @RequirePermission carries the same action string the domain services pass to checkPermission, so the HTTP and MCP paths cannot drift apart on which right they demand. Status codes and bodies are unchanged. Piloted on days, day-notes, todo, packing and files. Two things worth knowing: Files applies the guard PER HANDLER and leaves the upload route on its own check. Guards run before interceptors, so a class-level guard would answer 404 before multer had read the multipart body, and a response sent while the client is still streaming gets the socket destroyed — the caller sees ECONNRESET instead of the 404. Refusing after the body has been read costs the bandwidth and keeps the answer readable. The services keep verifyTripAccess and canEdit: the large majority of their callers are *.mcp.ts tools, which never pass through an HTTP guard. canEdit lost its incidental controller-test coverage in the move, so it now has direct tests asserting the action and the shared flag — the same pair the guard reproduces. Also fixes the backup zip-slip test that failed only on CI: it asserted a drive-letter path, which is absolute on win32 and an ordinary relative segment on Linux, so the branch it was guarding never fired there. --- server/src/nest/days/day-notes.controller.ts | 28 +-- server/src/nest/days/days.controller.ts | 37 ++-- server/src/nest/files/files.controller.ts | 70 ++++--- server/src/nest/packing/packing.controller.ts | 69 ++----- .../nest/permissions/permissions.module.ts | 12 +- .../src/nest/permissions/trip-access.guard.ts | 78 ++++++++ server/src/nest/permissions/trip.decorator.ts | 21 ++ server/src/nest/todo/todo.controller.ts | 35 +--- server/tests/unit/nest/backup.impl.test.ts | 7 +- .../tests/unit/nest/day-notes.service.test.ts | 26 +++ .../tests/unit/nest/days.controller.test.ts | 29 +-- server/tests/unit/nest/days.service.test.ts | 26 +++ .../tests/unit/nest/files.controller.test.ts | 59 +++--- .../unit/nest/packing.controller.test.ts | 26 +-- .../tests/unit/nest/todo.controller.test.ts | 14 +- server/tests/unit/nest/todo.service.test.ts | 26 +++ .../tests/unit/nest/trip-access.guard.test.ts | 182 ++++++++++++++++++ server/vitest.config.ts | 5 + 18 files changed, 513 insertions(+), 237 deletions(-) create mode 100644 server/src/nest/permissions/trip-access.guard.ts create mode 100644 server/src/nest/permissions/trip.decorator.ts create mode 100644 server/tests/unit/nest/trip-access.guard.test.ts diff --git a/server/src/nest/days/day-notes.controller.ts b/server/src/nest/days/day-notes.controller.ts index 02b2fbbf0..6cece7133 100644 --- a/server/src/nest/days/day-notes.controller.ts +++ b/server/src/nest/days/day-notes.controller.ts @@ -15,6 +15,7 @@ import { DayNotesService } from './day-notes.service'; import { DayNoteCreateDto, DayNoteUpdateDto } from './day-notes.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; /** * /api/trips/:tripId/days/:dayId/notes — free-text annotations on a day. @@ -27,30 +28,21 @@ import { CurrentUser } from '../auth/current-user.decorator'; * forwarded X-Socket-Id. */ @Controller('api/trips/:tripId/days/:dayId/notes') -@UseGuards(JwtAuthGuard) +// TripAccessGuard resolves :tripId and 404s a trip the user cannot reach; mutations +// add @RequirePermission('day_edit'), the same action string the service's canEdit +// passes, so the HTTP and MCP paths cannot demand different rights. +@UseGuards(JwtAuthGuard, TripAccessGuard) export class DayNotesController { constructor(private readonly notes: DayNotesService) {} - private requireTrip(tripId: string, user: User) { - const trip = this.notes.verifyTripAccess(tripId, user.id); - if (!trip) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - return trip; - } - private requireEdit(trip: NonNullable>, user: User): void { - if (!this.notes.canEdit(trip, user)) { - throw new HttpException({ error: 'No permission' }, 403); - } - } @Get() list(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string) { - this.requireTrip(tripId, user); return { notes: this.notes.list(dayId, tripId) }; } + @RequirePermission('day_edit') @Post() create( @CurrentUser() user: User, @@ -59,8 +51,6 @@ export class DayNotesController { @Body() body: DayNoteCreateDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.notes.dayExists(dayId, tripId)) { throw new HttpException({ error: 'Day not found' }, 404); } @@ -72,6 +62,7 @@ export class DayNotesController { return { note }; } + @RequirePermission('day_edit') @Put(':id') update( @CurrentUser() user: User, @@ -81,8 +72,6 @@ export class DayNotesController { @Body() body: DayNoteUpdateDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const current = this.notes.getNote(id, dayId, tripId); if (!current) { throw new HttpException({ error: 'Note not found' }, 404); @@ -92,6 +81,7 @@ export class DayNotesController { return { note }; } + @RequirePermission('day_edit') @Delete(':id') remove( @CurrentUser() user: User, @@ -100,8 +90,6 @@ export class DayNotesController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.notes.getNote(id, dayId, tripId)) { throw new HttpException({ error: 'Note not found' }, 404); } diff --git a/server/src/nest/days/days.controller.ts b/server/src/nest/days/days.controller.ts index 388e9ea7e..37380d2ef 100644 --- a/server/src/nest/days/days.controller.ts +++ b/server/src/nest/days/days.controller.ts @@ -15,6 +15,7 @@ import { DaysService, DayReorderError } from './days.service'; import { DayCreateDto, DayReorderDto, DayTransportDto, DayUpdateDto } from './days.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; /** * /api/trips/:tripId/days — trip itinerary days. @@ -25,30 +26,20 @@ import { CurrentUser } from '../auth/current-user.decorator'; * broadcasts with the forwarded X-Socket-Id. */ @Controller('api/trips/:tripId/days') -@UseGuards(JwtAuthGuard) +// TripAccessGuard resolves :tripId and 404s a trip the user cannot reach, so every +// handler below is already scoped. Mutations add @RequirePermission('day_edit'), +// which is the same action string days.service.canEdit passes — the MCP tools still +// go through that method, and this keeps the two from drifting. +@UseGuards(JwtAuthGuard, TripAccessGuard) export class DaysController { constructor(private readonly days: DaysService) {} - private requireTrip(tripId: string, user: User) { - const trip = this.days.verifyTripAccess(tripId, user.id); - if (!trip) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - return trip; - } - - private requireEdit(trip: NonNullable>, user: User): void { - if (!this.days.canEdit(trip, user)) { - throw new HttpException({ error: 'No permission' }, 403); - } - } - @Get() list(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return this.days.list(tripId); } + @RequirePermission('day_edit') @Post() create( @CurrentUser() user: User, @@ -56,8 +47,6 @@ export class DaysController { @Body() body: DayCreateDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // A `position` means "insert a new empty day here" (which on a dated trip // extends the trip and re-pins dates); without it, the legacy append. const day = body.position !== undefined @@ -70,6 +59,7 @@ export class DaysController { return { day }; } + @RequirePermission('day_edit') @Put('reorder') reorder( @CurrentUser() user: User, @@ -77,8 +67,6 @@ export class DaysController { @Body() body: DayReorderDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!Array.isArray(body.orderedIds)) { throw new HttpException({ error: 'orderedIds must be an array' }, 400); } @@ -94,6 +82,7 @@ export class DaysController { return { success: true }; } + @RequirePermission('day_edit') @Put(':id') update( @CurrentUser() user: User, @@ -102,8 +91,6 @@ export class DaysController { @Body() body: DayUpdateDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const current = this.days.getDay(id, tripId); if (!current) { throw new HttpException({ error: 'Day not found' }, 404); @@ -116,6 +103,7 @@ export class DaysController { return { day }; } + @RequirePermission('day_edit') @Put(':id/transport') transport( @CurrentUser() user: User, @@ -124,8 +112,6 @@ export class DaysController { @Body() body: DayTransportDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.days.getDay(id, tripId)) { throw new HttpException({ error: 'Day not found' }, 404); } @@ -134,6 +120,7 @@ export class DaysController { return { day }; } + @RequirePermission('day_edit') @Delete(':id') remove( @CurrentUser() user: User, @@ -141,8 +128,6 @@ export class DaysController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.days.getDay(id, tripId)) { throw new HttpException({ error: 'Day not found' }, 404); } diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index da4b2fba6..a4d805b4c 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -26,6 +26,9 @@ import type { User } from '../../types'; import { FilesService } from './files.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { TripAccessGuard } from '../permissions/trip-access.guard'; +import type { TripAccess } from '../database/database.service'; +import { Trip } from '../permissions/trip.decorator'; import { MAX_FILE_SIZE, MAX_VIDEO_SIZE, BLOCKED_EXTENSIONS, filesDir, isVideoExtension } from './files.constants'; import { getAllowedExtensions } from './files.bridge'; import { FileUploadDto, FileUpdateDto, FileLinkDto } from './files.dto'; @@ -67,6 +70,17 @@ const UPLOAD = { * the WebSocket broadcasts with the forwarded X-Socket-Id. */ @Controller('api/trips/:tripId/files') +// TripAccessGuard carries the trip access check, applied PER HANDLER rather than on +// the class. The per-handler RIGHT stays inline because files use three different ones +// (file_upload, file_edit, file_delete), which one class-level @RequirePermission could +// not express. +// +// `upload` is deliberately left off the guard and keeps its own check. Guards run +// before interceptors, so a class-level guard would answer 404 before multer had read +// the multipart body — and a response sent while the client is still streaming gets the +// socket destroyed, so the caller sees ECONNRESET instead of the 404. Refusing after +// the body has been read costs the bandwidth but keeps the answer readable, which is +// the behaviour every client already handles. @UseGuards(JwtAuthGuard) export class FilesController { constructor( @@ -74,13 +88,6 @@ export class FilesController { private readonly env: RuntimeEnvService, ) {} - private requireTrip(tripId: string, user: User) { - const trip = this.files.verifyTripAccess(tripId, user.id); - if (!trip) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - return trip; - } // A file may only point at reservations/assignments/places from its own trip. // Reject cross-trip ids before they are stored — the reservation JOIN would @@ -91,9 +98,9 @@ export class FilesController { } } + @UseGuards(TripAccessGuard) @Get() - list(@CurrentUser() user: User, @Param('tripId') tripId: string, @Query('trash') trash?: string) { - this.requireTrip(tripId, user); + list(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Query('trash') trash?: string) { return { files: this.files.listFiles(tripId, trash === 'true') }; } @@ -111,9 +118,12 @@ export class FilesController { // leaves up to the 500 MB video cap on disk (#823). const cleanup = () => { if (file?.path) { try { fs.unlinkSync(file.path); } catch { /* best-effort */ } } }; try { - const trip = this.requireTrip(tripId, user); - // Inline rather than DemoWriteGuard: requireTrip above answers 404 for a - // trip the caller cannot reach, and a guard would run before it. + const trip = this.files.verifyTripAccess(tripId, user.id); + if (!trip) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + // Inline rather than DemoWriteGuard: the 404 above has to come first, so a demo + // user still cannot learn which trips exist. if (isDemoWriteBlocked(this.env, user.email)) { throw new HttpException(DEMO_WRITE_ERROR, 403); } @@ -150,9 +160,9 @@ export class FilesController { return { file: created }; } + @UseGuards(TripAccessGuard) @Put(':id') - update(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Body() body: FileUpdateDto, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); + update(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Body() body: FileUpdateDto, @Headers('x-socket-id') socketId?: string) { if (!this.files.can('file_edit', trip, user)) { throw new HttpException({ error: 'No permission to edit files' }, 403); } @@ -166,9 +176,9 @@ export class FilesController { return { file: updated }; } + @UseGuards(TripAccessGuard) @Patch(':id/star') - star(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); + star(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { if (!this.files.can('file_edit', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -181,9 +191,9 @@ export class FilesController { return { file: updated }; } + @UseGuards(TripAccessGuard) @Delete('trash/empty') - async emptyTrash(@CurrentUser() user: User, @Param('tripId') tripId: string) { - const trip = this.requireTrip(tripId, user); + async emptyTrash(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string) { if (!this.files.can('file_delete', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -191,9 +201,9 @@ export class FilesController { return { success: true, deleted }; } + @UseGuards(TripAccessGuard) @Delete(':id/permanent') - async permanent(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); + async permanent(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { if (!this.files.can('file_delete', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -206,9 +216,9 @@ export class FilesController { return { success: true }; } + @UseGuards(TripAccessGuard) @Delete(':id') - remove(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); + remove(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { if (!this.files.can('file_delete', trip, user)) { throw new HttpException({ error: 'No permission to delete files' }, 403); } @@ -221,10 +231,10 @@ export class FilesController { return { success: true }; } + @UseGuards(TripAccessGuard) @Post(':id/restore') @HttpCode(200) // Express answers restore with res.json (200), not the POST-default 201. - restore(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); + restore(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { if (!this.files.can('file_delete', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -237,10 +247,10 @@ export class FilesController { return { file: restored }; } + @UseGuards(TripAccessGuard) @Post(':id/link') @HttpCode(200) // Express answers link with res.json (200). - link(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Body() body: FileLinkDto) { - const trip = this.requireTrip(tripId, user); + link(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Body() body: FileLinkDto) { if (!this.files.can('file_edit', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -253,9 +263,9 @@ export class FilesController { return { success: true, links }; } + @UseGuards(TripAccessGuard) @Delete(':id/link/:linkId') - unlink(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Param('linkId') linkId: string) { - const trip = this.requireTrip(tripId, user); + unlink(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string, @Param('linkId') linkId: string) { if (!this.files.can('file_edit', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } @@ -263,9 +273,9 @@ export class FilesController { return { success: true }; } + @UseGuards(TripAccessGuard) @Get(':id/links') - links(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string) { - this.requireTrip(tripId, user); + links(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string) { return { links: this.files.getFileLinks(id) }; } } diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index a2bbb5f85..a1f0bd42d 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -17,6 +17,7 @@ import { PackingService } from './packing.service'; import { isUpdateConflict } from '../common/conflictResult'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; import { PackingApplyTemplateDto, PackingBagMembersDto, @@ -52,32 +53,23 @@ type PackingItemRow = { is_private?: number; owner_id?: number | null; recipient * import arrays, the admin template gate) keep their exact legacy strings. */ @Controller('api/trips/:tripId/packing') -@UseGuards(JwtAuthGuard) +// TripAccessGuard resolves :tripId and 404s a trip the user cannot reach; mutations +// add @RequirePermission('packing_edit'), the same action string the service's canEdit +// passes, so the HTTP and MCP paths cannot demand different rights. +@UseGuards(JwtAuthGuard, TripAccessGuard) export class PackingController { constructor(private readonly packing: PackingService) {} /** Loads the trip or throws the legacy 404; returns it for the permission check. */ - private requireTrip(tripId: string, user: User) { - const trip = this.packing.verifyTripAccess(tripId, user.id); - if (!trip) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - return trip; - } - private requireEdit(trip: ReturnType, user: User): void { - if (!this.packing.canEdit(trip!, user)) { - throw new HttpException({ error: 'No permission' }, 403); - } - } @Get() list(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); // Pass the viewer so private items (#858) owned by other members are hidden. return { items: this.packing.listItems(tripId, user.id) }; } + @RequirePermission('packing_edit') @Post('import') importItems( @CurrentUser() user: User, @@ -85,8 +77,6 @@ export class PackingController { @Body() body: PackingImportDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // The schema guarantees an array; the empty-array rejection stays a bespoke 400. if (body.items.length === 0) { throw new HttpException({ error: 'items must be a non-empty array' }, 400); @@ -98,6 +88,7 @@ export class PackingController { return { items: created, count: created.length }; } + @RequirePermission('packing_edit') @Post() create( @CurrentUser() user: User, @@ -105,14 +96,13 @@ export class PackingController { @Body() body: PackingCreateItemDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // checked arrives as boolean or legacy 0/1 — the service coerces by truthiness. const item = this.packing.createItem(tripId, { name: body.name, category: body.category, checked: body.checked === undefined ? undefined : !!body.checked, is_private: body.is_private, visibility: body.visibility, recipient_ids: body.recipient_ids }, user.id); this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); return { item }; } + @RequirePermission('packing_edit') @Put('reorder') reorder( @CurrentUser() user: User, @@ -120,12 +110,11 @@ export class PackingController { @Body() body: PackingReorderDto, @Headers('x-socket-id') _socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); this.packing.reorderItems(tripId, body.orderedIds); return { success: true }; } + @RequirePermission('packing_edit') @Put(':id') update( @CurrentUser() user: User, @@ -135,8 +124,6 @@ export class PackingController { @Headers('x-socket-id') socketId?: string, @Headers('x-base-updated-at') ifMatch?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // Privacy state before the change, so a public↔private toggle (#858) can route // the broadcast correctly instead of leaking a freshly-privatized item. const before = this.packing.getItemPrivacy(tripId, id); @@ -156,6 +143,7 @@ export class PackingController { return { item: updated }; } + @RequirePermission('packing_edit') @Delete(':id') remove( @CurrentUser() user: User, @@ -163,8 +151,6 @@ export class PackingController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const deleted = this.packing.deleteItem(tripId, id); if (!deleted) { throw new HttpException({ error: 'Item not found' }, 404); @@ -174,6 +160,7 @@ export class PackingController { return { success: true }; } + @RequirePermission('packing_edit') @Put(':id/sharing') setSharing( @CurrentUser() user: User, @@ -182,8 +169,6 @@ export class PackingController { @Body() body: PackingSetSharingDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const updated = this.packing.setItemSharing(tripId, id, user.id, body.visibility, Array.isArray(body.recipient_ids) ? body.recipient_ids : []); if (!updated) { throw new HttpException({ error: 'Item not found' }, 404); @@ -198,6 +183,7 @@ export class PackingController { return { item: updated }; } + @RequirePermission('packing_edit') @Post(':id/clone') @HttpCode(201) clone( @@ -206,8 +192,6 @@ export class PackingController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const item = this.packing.cloneItem(tripId, id, user.id); if (!item) { throw new HttpException({ error: 'Item not found' }, 404); @@ -217,6 +201,7 @@ export class PackingController { return { item }; } + @RequirePermission('packing_edit') @Post(':id/contributors') addContributor( @CurrentUser() user: User, @@ -224,8 +209,6 @@ export class PackingController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const item = this.packing.addContributor(tripId, id, user.id); if (!item) { throw new HttpException({ error: 'Item not found or not a shared list item' }, 404); @@ -235,6 +218,7 @@ export class PackingController { return { item }; } + @RequirePermission('packing_edit') @Delete(':id/contributors/:userId') removeContributor( @CurrentUser() user: User, @@ -243,8 +227,6 @@ export class PackingController { @Param('userId') userId: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // You can drop your own pledge; the owner can remove anyone's. const target = parseInt(userId); const item = this.packing.removeContributor(tripId, id, target); @@ -257,10 +239,10 @@ export class PackingController { @Get('bags') listBags(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return { bags: this.packing.listBags(tripId) }; } + @RequirePermission('packing_edit') @Post('bags') createBag( @CurrentUser() user: User, @@ -268,8 +250,6 @@ export class PackingController { @Body() body: PackingCreateBagDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); // The schema requires a non-empty name; whitespace-only still 400s here. if (!body.name.trim()) { throw new HttpException({ error: 'Name is required' }, 400); @@ -279,6 +259,7 @@ export class PackingController { return { bag }; } + @RequirePermission('packing_edit') @Put('bags/:bagId') updateBag( @CurrentUser() user: User, @@ -287,8 +268,6 @@ export class PackingController { @Body() body: PackingUpdateBagDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const { name, color, weight_limit_grams, user_id } = body; // bodyKeys carries which keys the request actually provided (the presence- // sentinel protocol); the parsed body only ever holds known schema keys. @@ -300,6 +279,7 @@ export class PackingController { return { bag: updated }; } + @RequirePermission('packing_edit') @Delete('bags/:bagId') deleteBag( @CurrentUser() user: User, @@ -307,8 +287,6 @@ export class PackingController { @Param('bagId') bagId: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.packing.deleteBag(tripId, bagId)) { throw new HttpException({ error: 'Bag not found' }, 404); } @@ -318,10 +296,10 @@ export class PackingController { @Get('templates') listTemplates(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return { templates: this.packing.listTemplates() }; } + @RequirePermission('packing_edit') @Post('apply-template/:templateId') @HttpCode(200) applyTemplate( @@ -331,8 +309,6 @@ export class PackingController { @Body() body: PackingApplyTemplateDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const visibility = body?.visibility === 'personal' ? 'personal' : 'common'; const added = this.packing.applyTemplate(tripId, templateId, visibility, user.id); if (!added) { @@ -342,6 +318,7 @@ export class PackingController { return { items: added, count: added.length }; } + @RequirePermission('packing_edit') @Put('bags/:bagId/members') setBagMembers( @CurrentUser() user: User, @@ -350,8 +327,6 @@ export class PackingController { @Body() body: PackingBagMembersDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const members = this.packing.setBagMembers(tripId, bagId, body.user_ids); if (!members) { throw new HttpException({ error: 'Bag not found' }, 404); @@ -360,13 +335,13 @@ export class PackingController { return { members }; } + @RequirePermission('packing_edit') @Post('save-as-template') saveAsTemplate( @CurrentUser() user: User, @Param('tripId') tripId: string, @Body() body: PackingSaveTemplateDto, ) { - this.requireTrip(tripId, user); if (user.role !== 'admin') { throw new HttpException({ error: 'Admin access required' }, 403); } @@ -383,10 +358,10 @@ export class PackingController { @Get('category-assignees') categoryAssignees(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return { assignees: this.packing.getCategoryAssignees(tripId) }; } + @RequirePermission('packing_edit') @Put('category-assignees/:categoryName') updateCategoryAssignees( @CurrentUser() user: User, @@ -395,8 +370,6 @@ export class PackingController { @Body() body: PackingCategoryAssigneesDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const category = decodeURIComponent(categoryName); const rows = this.packing.updateCategoryAssignees(tripId, category, body.user_ids); this.packing.broadcast(tripId, 'packing:assignees', { category, assignees: rows }, socketId); diff --git a/server/src/nest/permissions/permissions.module.ts b/server/src/nest/permissions/permissions.module.ts index 49e9f4046..0a6256683 100644 --- a/server/src/nest/permissions/permissions.module.ts +++ b/server/src/nest/permissions/permissions.module.ts @@ -1,13 +1,15 @@ import { Module } from '@nestjs/common'; import { PermissionsService } from './permissions.service'; +import { TripAccessGuard } from './trip-access.guard'; /** Cross-cutting permissions domain (Wave 2). No controller/MCP surface of its * own — the admin HTTP surface stays with AdminModule. Exports - * PermissionsService for the 17 in-container consumers; deliberately NOT - * @Global so e2e TestingModules resolve it transitively through each - * consumer's explicit import. Registered in AppModule. */ + * PermissionsService for the 17 in-container consumers and TripAccessGuard for + * every trip-scoped controller; deliberately NOT @Global so e2e TestingModules + * resolve it transitively through each consumer's explicit import. Registered in + * AppModule. */ @Module({ - providers: [PermissionsService], - exports: [PermissionsService], + providers: [PermissionsService, TripAccessGuard], + exports: [PermissionsService, TripAccessGuard], }) export class PermissionsModule {} diff --git a/server/src/nest/permissions/trip-access.guard.ts b/server/src/nest/permissions/trip-access.guard.ts new file mode 100644 index 000000000..a8db1baf6 --- /dev/null +++ b/server/src/nest/permissions/trip-access.guard.ts @@ -0,0 +1,78 @@ +import { CanActivate, ExecutionContext, HttpException, Injectable, SetMetadata } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import type { Request } from 'express'; +import { DatabaseService, type TripAccess } from '../database/database.service'; +import { PermissionsService } from './permissions.service'; +import type { User } from '../../types'; + +/** Where the guard parks the resolved trip row for `@Trip()` to pick up. */ +export const TRIP_REQUEST_KEY = 'trekTrip'; + +/** The metadata key `@RequirePermission()` writes and the guard reads. */ +export const TRIP_PERMISSION_KEY = 'trekTripPermission'; + +/** + * Requires a permission on the trip the route is scoped to, on top of access. + * + * The action string is the same one the domain services pass to + * `PermissionsService.checkPermission` ('day_edit', 'budget_edit', …), so a route + * and its MCP counterpart cannot drift apart on which right they demand. + */ +export const RequirePermission = (action: string) => SetMetadata(TRIP_PERMISSION_KEY, action); + +type TripRequest = Request & { user?: User; [TRIP_REQUEST_KEY]?: TripAccess }; + +/** + * Resolves `:tripId` once per request, refuses what the user cannot reach, and hands + * the trip row to the handler. + * + * Every trip-scoped controller used to carry its own `requireTrip` helper doing the + * same three lines, and a `requireEdit` next to it. That is 18 copies of a security + * check, each of which had to remember that no access is a 404 "Trip not found" and + * never a 403 — the distinction that keeps a stranger from learning which trip ids + * exist. One of them getting it wrong would have looked like nothing. + * + * The status codes and bodies here are byte-identical to those copies, because the + * e2e suites assert them and the client branches on them. + * + * It deliberately does NOT replace the `verifyTripAccess`/`canEdit` methods on the + * domain services. Of their callers, the large majority are `*.mcp.ts` tools, which + * never pass through an HTTP guard; those methods stay exactly where they are. + */ +@Injectable() +export class TripAccessGuard implements CanActivate { + constructor( + private readonly db: DatabaseService, + private readonly permissions: PermissionsService, + private readonly reflector: Reflector, + ) {} + + canActivate(context: ExecutionContext): boolean { + const request = context.switchToHttp().getRequest(); + const user = request.user; + // JwtAuthGuard runs first and 401s an anonymous request, so a missing user here + // means the route was wired without it. Refuse rather than read `user.id` off + // undefined and turn a wiring mistake into a 500. + if (!user) throw new HttpException({ error: 'Unauthorized' }, 401); + + const tripId = Number((request.params as Record)?.tripId); + const trip = Number.isFinite(tripId) ? this.db.canAccessTrip(tripId, user.id) : undefined; + // A trip the user may not see is reported as absent, never as forbidden: a 403 + // would confirm the id exists to someone who has no business knowing. + if (!trip) throw new HttpException({ error: 'Trip not found' }, 404); + + const action = this.reflector.getAllAndOverride(TRIP_PERMISSION_KEY, [ + context.getHandler(), + context.getClass(), + ]); + if (action) { + const shared = trip.user_id !== user.id; + if (!this.permissions.checkPermission(action, user.role, trip.user_id, user.id, shared)) { + throw new HttpException({ error: 'No permission' }, 403); + } + } + + request[TRIP_REQUEST_KEY] = trip; + return true; + } +} diff --git a/server/src/nest/permissions/trip.decorator.ts b/server/src/nest/permissions/trip.decorator.ts new file mode 100644 index 000000000..9f518656b --- /dev/null +++ b/server/src/nest/permissions/trip.decorator.ts @@ -0,0 +1,21 @@ +import { createParamDecorator, type ExecutionContext } from '@nestjs/common'; +import type { TripAccess } from '../database/database.service'; +import { TRIP_REQUEST_KEY } from './trip-access.guard'; + +/** + * The trip row `TripAccessGuard` resolved for this request. + * + * A param decorator rather than a `req.trip` cast on purpose: the request type the + * controllers used to widen for that (`AuthRequest`) is gone, and re-introducing a + * per-controller cast would put the same untyped hole back. Only usable on a route + * that carries `@UseGuards(TripAccessGuard)` — without it there is nothing to read, + * and the handler would silently receive undefined, so it throws instead. + */ +export const Trip = createParamDecorator((_data: unknown, context: ExecutionContext): TripAccess => { + const request = context.switchToHttp().getRequest>(); + const trip = request[TRIP_REQUEST_KEY] as TripAccess | undefined; + if (!trip) { + throw new Error('@Trip() used on a route without @UseGuards(TripAccessGuard)'); + } + return trip; +}); diff --git a/server/src/nest/todo/todo.controller.ts b/server/src/nest/todo/todo.controller.ts index aab26af73..244c929e9 100644 --- a/server/src/nest/todo/todo.controller.ts +++ b/server/src/nest/todo/todo.controller.ts @@ -15,6 +15,7 @@ import { TodoService } from './todo.service'; import { TodoCreateItemDto, TodoUpdateItemDto, TodoReorderDto, TodoCategoryAssigneesDto } from './todo.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; +import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; /** * /api/trips/:tripId/todo — trip-scoped task list. @@ -28,30 +29,21 @@ import { CurrentUser } from '../auth/current-user.decorator'; * bespoke 'Item name is required' check). */ @Controller('api/trips/:tripId/todo') -@UseGuards(JwtAuthGuard) +// TripAccessGuard resolves :tripId and 404s a trip the user cannot reach; mutations +// add @RequirePermission('packing_edit'), the same action string the service's canEdit +// passes, so the HTTP and MCP paths cannot demand different rights. +@UseGuards(JwtAuthGuard, TripAccessGuard) export class TodoController { constructor(private readonly todo: TodoService) {} - private requireTrip(tripId: string, user: User) { - const trip = this.todo.verifyTripAccess(tripId, user.id); - if (!trip) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - return trip; - } - private requireEdit(trip: ReturnType, user: User): void { - if (!this.todo.canEdit(trip!, user)) { - throw new HttpException({ error: 'No permission' }, 403); - } - } @Get() list(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return { items: this.todo.listItems(tripId) }; } + @RequirePermission('packing_edit') @Post() create( @CurrentUser() user: User, @@ -59,26 +51,24 @@ export class TodoController { @Body() body: TodoCreateItemDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const { name, category, due_date, description, assigned_user_id, priority } = body; const item = this.todo.createItem(tripId, { name, category, due_date, description, assigned_user_id, priority }); this.todo.broadcast(tripId, 'todo:created', { item }, socketId); return { item }; } + @RequirePermission('packing_edit') @Put('reorder') reorder( @CurrentUser() user: User, @Param('tripId') tripId: string, @Body() body: TodoReorderDto, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); this.todo.reorderItems(tripId, body.orderedIds); return { success: true }; } + @RequirePermission('packing_edit') @Put(':id') update( @CurrentUser() user: User, @@ -87,8 +77,6 @@ export class TodoController { @Body() body: TodoUpdateItemDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const { name, checked, category, due_date, description, assigned_user_id, priority } = body; // bodyKeys carries which keys the request actually provided (the null-clear // protocol); the parsed body only ever holds known schema keys. @@ -106,6 +94,7 @@ export class TodoController { return { item: updated }; } + @RequirePermission('packing_edit') @Delete(':id') remove( @CurrentUser() user: User, @@ -113,8 +102,6 @@ export class TodoController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); if (!this.todo.deleteItem(tripId, id)) { throw new HttpException({ error: 'Item not found' }, 404); } @@ -124,10 +111,10 @@ export class TodoController { @Get('category-assignees') categoryAssignees(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.requireTrip(tripId, user); return { assignees: this.todo.getCategoryAssignees(tripId) }; } + @RequirePermission('packing_edit') @Put('category-assignees/:categoryName') updateCategoryAssignees( @CurrentUser() user: User, @@ -136,8 +123,6 @@ export class TodoController { @Body() body: TodoCategoryAssigneesDto, @Headers('x-socket-id') socketId?: string, ) { - const trip = this.requireTrip(tripId, user); - this.requireEdit(trip, user); const category = decodeURIComponent(categoryName); const rows = this.todo.updateCategoryAssignees(tripId, category, body.user_ids); this.todo.broadcast(tripId, 'todo:assignees', { category, assignees: rows }, socketId); diff --git a/server/tests/unit/nest/backup.impl.test.ts b/server/tests/unit/nest/backup.impl.test.ts index 0f3b802c8..8e9f3ffde 100644 --- a/server/tests/unit/nest/backup.impl.test.ts +++ b/server/tests/unit/nest/backup.impl.test.ts @@ -786,7 +786,12 @@ describe('BACKUP-061 restoreFromZip extraction', () => { expect(fsMock.createWriteStream).not.toHaveBeenCalled(); }); - it('BACKUP-061b — an absolute entry path is refused the same way', async () => { + // The `path.isAbsolute(rel)` half of the guard is only reachable where drive letters + // exist: on POSIX, path.join always keeps an entry under extractDir, so a leading + // slash is normalised away and only the `..` check above can fire. Asserting it + // unconditionally passed locally on Windows and failed on the Linux CI runner, which + // is the wrong way round for a security test to be discovered. + it.runIf(process.platform === 'win32')('BACKUP-061b — a drive-letter entry path is refused the same way', async () => { unzipperMock.Open.file.mockResolvedValueOnce({ files: [zipEntry('C:/Windows/system32/evil.dll')] }); const result = await restoreFromZip('/data/tmp/abs.zip'); diff --git a/server/tests/unit/nest/day-notes.service.test.ts b/server/tests/unit/nest/day-notes.service.test.ts index 1f81ad982..334ede046 100644 --- a/server/tests/unit/nest/day-notes.service.test.ts +++ b/server/tests/unit/nest/day-notes.service.test.ts @@ -223,3 +223,29 @@ describe('remove', () => { expect(testDb.prepare('SELECT * FROM day_notes WHERE id = ?').get(note.id)).toBeUndefined(); }); }); + +/** + * canEdit is MCP-only now: the HTTP path checks the same right through + * TripAccessGuard's @RequirePermission('day_edit'), so nothing in a controller test + * reaches this method any more. It stays because the *.mcp.ts tools never pass through + * an HTTP guard, and it is tested directly here for the same reason. + */ +describe('DayNotesService.canEdit', () => { + it('DAYNOTE-SVC-090 asks for day_edit and flags a non-owner as shared', () => { + const checkPermission = vi.fn(() => true); + const permissions = { checkPermission } as unknown as PermissionsService; + const withStub = new DayNotesService(new DatabaseService(testDb), permissions, new RealtimeService()); + const trip = { id: 1, user_id: 1 } as never; + + expect(withStub.canEdit(trip, { id: 1, role: 'user' } as never)).toBe(true); + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 1, 1, false); + + withStub.canEdit(trip, { id: 2, role: 'user' } as never); + // The shared flag is what the guard has to reproduce; getting it wrong would give a + // member the owner's rights on somebody else's trip. + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 1, 2, true); + + checkPermission.mockReturnValue(false); + expect(withStub.canEdit(trip, { id: 2, role: 'user' } as never)).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/days.controller.test.ts b/server/tests/unit/nest/days.controller.test.ts index 6d3506ad7..e9407157a 100644 --- a/server/tests/unit/nest/days.controller.test.ts +++ b/server/tests/unit/nest/days.controller.test.ts @@ -28,28 +28,23 @@ function notesSvc(o: Partial = {}): DayNotesService { } describe('DaysController (parity with the legacy /api/trips/:tripId/days route)', () => { - it('404 when trip not accessible', () => { - const svc = daysSvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => new DaysController(svc).list(user, '5'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); + // The 404 "Trip not found" and 403 "No permission" cases moved to + // trip-access.guard.test.ts with the check itself: TripAccessGuard resolves :tripId + // for the whole controller now, so a handler is only ever reached for a trip the + // user may see. What is left here is what the handlers themselves still decide. it('GET / returns the list service result verbatim (the { days } envelope)', () => { const svc = daysSvc({ list: vi.fn().mockReturnValue({ days: [{ id: 1 }] }) } as Partial); expect(new DaysController(svc).list(user, '5')).toEqual({ days: [{ id: 1 }] }); }); - it('POST / 403 without day_edit, then creates + broadcasts', () => { - expect(thrown(() => new DaysController(daysSvc({ canEdit: vi.fn().mockReturnValue(false) })).create(user, '5', {}))).toEqual({ status: 403, body: { error: 'No permission' } }); + it('POST / creates + broadcasts', () => { const create = vi.fn().mockReturnValue({ id: 9 }); const broadcast = vi.fn(); expect(new DaysController(daysSvc({ create, broadcast } as Partial)).create(user, '5', { date: '2026-07-01' }, 'sock')).toEqual({ day: { id: 9 } }); expect(create).toHaveBeenCalledWith('5', '2026-07-01', undefined); expect(broadcast).toHaveBeenCalledWith('5', 'day:created', { day: { id: 9 } }, 'sock'); }); - it('POST / 404 when the trip is not accessible', () => { - const svc = daysSvc({ verifyTripAccess: vi.fn().mockReturnValue(null) }); - expect(thrown(() => new DaysController(svc).create(user, '5', {}))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); it('POST / with a position inserts + broadcasts day:reordered', () => { const insert = vi.fn().mockReturnValue({ id: 12 }); const create = vi.fn(); const broadcast = vi.fn(); @@ -61,15 +56,7 @@ describe('DaysController (parity with the legacy /api/trips/:tripId/days route)' }); describe('PUT /reorder', () => { - it('404 when the trip is not accessible', () => { - const svc = daysSvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => new DaysController(svc).reorder(user, '5', { orderedIds: [1, 2] }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); - it('403 without day_edit', () => { - const svc = daysSvc({ canEdit: vi.fn().mockReturnValue(false) }); - expect(thrown(() => new DaysController(svc).reorder(user, '5', { orderedIds: [1, 2] }))).toEqual({ status: 403, body: { error: 'No permission' } }); - }); it('400 when orderedIds is missing', () => { expect(thrown(() => new DaysController(daysSvc()).reorder(user, '5', {}))).toEqual({ status: 400, body: { error: 'orderedIds must be an array' } }); @@ -130,9 +117,9 @@ describe('DayNotesController (parity with the legacy /api/.../days/:dayId/notes expect(DayNoteUpdateDto.schema.safeParse({ time: null }).success).toBe(true); }); - it('404 trip, 403 permission, 404 day, 400 empty text, then creates', () => { - expect(thrown(() => new DayNotesController(notesSvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) })).create(user, '5', '3', { text: 'ok' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - expect(thrown(() => new DayNotesController(notesSvc({ canEdit: vi.fn().mockReturnValue(false) })).create(user, '5', '3', { text: 'ok' }))).toEqual({ status: 403, body: { error: 'No permission' } }); + // Trip access and day_edit are TripAccessGuard's now (trip-access.guard.test.ts); + // what stays here is what this handler decides for itself. + it('404 day, 400 empty text, then creates', () => { expect(thrown(() => new DayNotesController(notesSvc({ dayExists: vi.fn().mockReturnValue(false) } as Partial)).create(user, '5', '3', { text: 'ok' }))).toEqual({ status: 404, body: { error: 'Day not found' } }); expect(thrown(() => new DayNotesController(notesSvc({ dayExists: vi.fn().mockReturnValue(true) } as Partial)).create(user, '5', '3', { text: ' ' }))).toEqual({ status: 400, body: { error: 'Text required' } }); const create = vi.fn().mockReturnValue({ id: 7 }); const broadcast = vi.fn(); diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index f0674d402..f32432771 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -548,3 +548,29 @@ describe('quirk fixes', () => { expect(assignments[0].place.tags[0]).toMatchObject({ id: tagId, name: 'Food', color: '#ff0000' }); }); }); + +/** + * canEdit is MCP-only now: the HTTP path checks the same right through + * TripAccessGuard's @RequirePermission('day_edit'), so nothing in a controller test + * reaches this method any more. It stays because the *.mcp.ts tools never pass through + * an HTTP guard, and it is tested directly here for the same reason. + */ +describe('DaysService.canEdit', () => { + it('DAY-SVC-090 asks for day_edit and flags a non-owner as shared', () => { + const checkPermission = vi.fn(() => true); + const permissions = { checkPermission } as unknown as PermissionsService; + const withStub = new DaysService(new DatabaseService(testDb), permissions, new RealtimeService(), new QueryHelpersService(new DatabaseService(testDb))); + const trip = { id: 1, user_id: 1 } as never; + + expect(withStub.canEdit(trip, { id: 1, role: 'user' } as never)).toBe(true); + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 1, 1, false); + + withStub.canEdit(trip, { id: 2, role: 'user' } as never); + // The shared flag is what the guard has to reproduce; getting it wrong would give a + // member the owner's rights on somebody else's trip. + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 1, 2, true); + + checkPermission.mockReturnValue(false); + expect(withStub.canEdit(trip, { id: 2, role: 'user' } as never)).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/files.controller.test.ts b/server/tests/unit/nest/files.controller.test.ts index e1a73dea1..c0272271d 100644 --- a/server/tests/unit/nest/files.controller.test.ts +++ b/server/tests/unit/nest/files.controller.test.ts @@ -40,15 +40,28 @@ function thrown(fn: () => unknown): { status: number; body: unknown } { beforeEach(() => vi.clearAllMocks()); afterEach(() => { delete process.env.DEMO_MODE; }); +/** The trip row TripAccessGuard resolves and hands to every handler via @Trip(). */ +const trip = { id: 5, user_id: 42 } as never; + +// The 404 "Trip not found" cases moved to trip-access.guard.test.ts with the check. describe('FilesController (parity with the legacy /api/trips/:tripId/files route)', () => { - it('GET / 404 without access, else lists with the trash flag', () => { - expect(thrown(() => new FilesController(fsvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }), new RuntimeEnvService()).list(user, '5'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + it('GET / lists with the trash flag', () => { const listFiles = vi.fn().mockReturnValue([{ id: 1 }]); - expect(new FilesController(fsvc({ listFiles } as Partial), new RuntimeEnvService()).list(user, '5', 'true')).toEqual({ files: [{ id: 1 }] }); + expect(new FilesController(fsvc({ listFiles } as Partial), new RuntimeEnvService()).list(user, trip, '5', 'true')).toEqual({ files: [{ id: 1 }] }); expect(listFiles).toHaveBeenCalledWith('5', true); }); describe('POST / (upload)', () => { + it('404 without trip access — upload keeps its own check so the body is read first', () => { + // The guard would answer before multer, and a response sent mid-upload destroys + // the socket: the caller would see ECONNRESET rather than this 404. + const svc = fsvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); + expect(thrown(() => new FilesController(svc, new RuntimeEnvService()).upload(user, '5', undefined, {}))).toEqual({ + status: 404, + body: { error: 'Trip not found' }, + }); + }); + const file = { filename: 'a.pdf' } as Express.Multer.File; it('403 in demo mode for a demo email', () => { process.env.DEMO_MODE = 'true'; @@ -82,37 +95,37 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route }); it('PUT /:id 403 without file_edit, 404 unknown, else updates + broadcasts', () => { - expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).update(user, '5', '9', {}))).toEqual({ status: 403, body: { error: 'No permission to edit files' } }); - expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).update(user, '5', '9', {}))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).update(user, trip, '5', '9', {}))).toEqual({ status: 403, body: { error: 'No permission to edit files' } }); + expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).update(user, trip, '5', '9', {}))).toEqual({ status: 404, body: { error: 'File not found' } }); const updateFile = vi.fn().mockReturnValue({ id: 9 }); const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9, description: 'x' }), updateFile, broadcast: vi.fn() } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).update(user, '5', '9', { description: 'new' })).toEqual({ file: { id: 9 } }); + expect(new FilesController(s, new RuntimeEnvService()).update(user, trip, '5', '9', { description: 'new' })).toEqual({ file: { id: 9 } }); }); it('PATCH /:id/star 403/404, else toggles', () => { - expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).star(user, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission' } }); - expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).star(user, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).star(user, trip, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission' } }); + expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).star(user, trip, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found' } }); const toggleStarred = vi.fn().mockReturnValue({ id: 9, starred: 1 }); const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9, starred: 0 }), toggleStarred, broadcast: vi.fn() } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).star(user, '5', '9')).toEqual({ file: { id: 9, starred: 1 } }); + expect(new FilesController(s, new RuntimeEnvService()).star(user, trip, '5', '9')).toEqual({ file: { id: 9, starred: 1 } }); expect(toggleStarred).toHaveBeenCalledWith('9', 0); }); it('DELETE /:id soft-delete 403/404, else success', () => { - expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).remove(user, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission to delete files' } }); - expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).remove(user, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(thrown(() => new FilesController(fsvc({ can: vi.fn().mockReturnValue(false) }), new RuntimeEnvService()).remove(user, trip, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission to delete files' } }); + expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).remove(user, trip, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found' } }); const softDeleteFile = vi.fn(); const broadcast = vi.fn(); const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), softDeleteFile, broadcast } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).remove(user, '5', '9', 'sock')).toEqual({ success: true }); + expect(new FilesController(s, new RuntimeEnvService()).remove(user, trip, '5', '9', 'sock')).toEqual({ success: true }); expect(broadcast).toHaveBeenCalledWith('5', 'file:deleted', { fileId: 9 }, 'sock'); }); it('POST /:id/restore 404 not in trash, else restores', () => { - expect(thrown(() => new FilesController(fsvc({ getDeletedFile: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).restore(user, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found in trash' } }); + expect(thrown(() => new FilesController(fsvc({ getDeletedFile: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).restore(user, trip, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found in trash' } }); const restoreFile = vi.fn().mockReturnValue({ id: 9 }); const s = fsvc({ getDeletedFile: vi.fn().mockReturnValue({ id: 9 }), restoreFile, broadcast: vi.fn() } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).restore(user, '5', '9')).toEqual({ file: { id: 9 } }); + expect(new FilesController(s, new RuntimeEnvService()).restore(user, trip, '5', '9')).toEqual({ file: { id: 9 } }); }); it('DELETE /:id/permanent 404 not in trash, else deletes', async () => { @@ -129,32 +142,28 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route }); it('POST /:id/link 404 unknown file, else links', () => { - expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).link(user, '5', '9', {}))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(thrown(() => new FilesController(fsvc({ getFileById: vi.fn().mockReturnValue(undefined) } as Partial), new RuntimeEnvService()).link(user, trip, '5', '9', {}))).toEqual({ status: 404, body: { error: 'File not found' } }); const createFileLink = vi.fn().mockReturnValue([{ id: 1 }]); const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), createFileLink } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).link(user, '5', '9', { reservation_id: 2 })).toEqual({ success: true, links: [{ id: 1 }] }); + expect(new FilesController(s, new RuntimeEnvService()).link(user, trip, '5', '9', { reservation_id: 2 })).toEqual({ success: true, links: [{ id: 1 }] }); }); it('DELETE /:id/link/:linkId removes the link; GET /:id/links lists', () => { const deleteFileLink = vi.fn(); - expect(new FilesController(fsvc({ deleteFileLink } as Partial), new RuntimeEnvService()).unlink(user, '5', '9', '3')).toEqual({ success: true }); + expect(new FilesController(fsvc({ deleteFileLink } as Partial), new RuntimeEnvService()).unlink(user, trip, '5', '9', '3')).toEqual({ success: true }); expect(deleteFileLink).toHaveBeenCalledWith('3', '9'); const s = fsvc({ getFileLinks: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); - expect(new FilesController(s, new RuntimeEnvService()).links(user, '5', '9')).toEqual({ links: [{ id: 1 }] }); + expect(new FilesController(s, new RuntimeEnvService()).links(user, trip, '5', '9')).toEqual({ links: [{ id: 1 }] }); }); it('the trash + link routes all reject without file_delete / file_edit', async () => { const denied = () => fsvc({ can: vi.fn().mockReturnValue(false) }); await expect(new FilesController(denied(), new RuntimeEnvService()).permanent(user, '5', '9')).rejects.toMatchObject({ status: 403 }); - expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).restore(user, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission' } }); - expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).link(user, '5', '9', {}))).toEqual({ status: 403, body: { error: 'No permission' } }); - expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).unlink(user, '5', '9', '3'))).toEqual({ status: 403, body: { error: 'No permission' } }); + expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).restore(user, trip, '5', '9'))).toEqual({ status: 403, body: { error: 'No permission' } }); + expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).link(user, trip, '5', '9', {}))).toEqual({ status: 403, body: { error: 'No permission' } }); + expect(thrown(() => new FilesController(denied(), new RuntimeEnvService()).unlink(user, trip, '5', '9', '3'))).toEqual({ status: 403, body: { error: 'No permission' } }); }); - it('GET /:id/links 404 without trip access', () => { - const s = fsvc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => new FilesController(s, new RuntimeEnvService()).links(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); }); describe('FilesDownloadController', () => { diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index 56c34a75d..4d7a97a7a 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -60,13 +60,9 @@ function thrown(fn: () => unknown): { status: number; body: unknown } { throw new Error('expected the handler to throw'); } +// The 404 "Trip not found" and 403 "No permission" cases moved to +// trip-access.guard.test.ts with the check itself. describe('PackingController (parity with the legacy /api/trips/:tripId/packing route)', () => { - it('404 when the trip is not accessible', () => { - const svc = makeService({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => new PackingController(svc).list(user, '5'))).toEqual({ - status: 404, body: { error: 'Trip not found' }, - }); - }); it('GET / returns items for an accessible trip', () => { const svc = makeService({ listItems: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); @@ -74,12 +70,6 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r }); describe('POST / (create)', () => { - it('403 without packing_edit permission', () => { - const svc = makeService({ canEdit: vi.fn().mockReturnValue(false) }); - expect(thrown(() => new PackingController(svc).create(user, '5', { name: 'Socks' }))).toEqual({ - status: 403, body: { error: 'No permission' }, - }); - }); // The missing-name 400 moved from a bespoke controller check into the // ZodValidationPipe (packingCreateItemRequestSchema) — direct method calls @@ -126,12 +116,6 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r // requires an array) — direct method calls bypass parameter pipes, so that // path is covered by the e2e suite and the schema spec in @trek/shared. - it('403 without packing_edit permission', () => { - const svc = makeService({ canEdit: vi.fn().mockReturnValue(false) }); - expect(thrown(() => new PackingController(svc).importItems(user, '5', { items: [{ name: 'a' }] }))).toEqual({ - status: 403, body: { error: 'No permission' }, - }); - }); it('imports (owned by the importer) and broadcasts per item', () => { const bulkImport = vi.fn().mockReturnValue([{ id: 1 }, { id: 2 }]); @@ -220,12 +204,6 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r expect(reorderItems).toHaveBeenCalledWith('5', [3, 1, 2]); }); - it('403 without packing_edit permission', () => { - const svc = makeService({ canEdit: vi.fn().mockReturnValue(false) }); - expect(thrown(() => new PackingController(svc).reorder(user, '5', { orderedIds: [1] }))).toEqual({ - status: 403, body: { error: 'No permission' }, - }); - }); }); describe('DELETE /:id (remove)', () => { diff --git a/server/tests/unit/nest/todo.controller.test.ts b/server/tests/unit/nest/todo.controller.test.ts index a79ef5bc2..621d19b04 100644 --- a/server/tests/unit/nest/todo.controller.test.ts +++ b/server/tests/unit/nest/todo.controller.test.ts @@ -27,13 +27,9 @@ function thrown(fn: () => unknown): { status: number; body: unknown } { throw new Error('expected the handler to throw'); } +// The 404 "Trip not found" and 403 "No permission" cases moved to +// trip-access.guard.test.ts with the check itself. describe('TodoController (parity with the legacy /api/trips/:tripId/todo route)', () => { - it('404 when the trip is not accessible', () => { - const svc = makeService({ verifyTripAccess: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => new TodoController(svc).list(user, '5'))).toEqual({ - status: 404, body: { error: 'Trip not found' }, - }); - }); it('GET / returns items', () => { const svc = makeService({ listItems: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); @@ -41,12 +37,6 @@ describe('TodoController (parity with the legacy /api/trips/:tripId/todo route)' }); describe('POST /', () => { - it('403 without permission', () => { - const svc = makeService({ canEdit: vi.fn().mockReturnValue(false) }); - expect(thrown(() => new TodoController(svc).create(user, '5', { name: 'Pack' }))).toEqual({ - status: 403, body: { error: 'No permission' }, - }); - }); // The missing-name 400 moved from a bespoke controller check into the // ZodValidationPipe (todoCreateItemRequestSchema) — direct method calls diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index 5c142a730..b52e7931a 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -289,3 +289,29 @@ describe('getCategoryAssignees / updateCategoryAssignees', () => { // TODO-SVC-021..024 (todo.bridge delegation) were deleted with the bridge — // its last consumer, the legacy get_trip_summary registrar in // src/mcp/tools/trips.ts, moved to the DI-discovered trips.mcp.ts. + +/** + * canEdit is MCP-only now: the HTTP path checks the same right through + * TripAccessGuard's @RequirePermission('packing_edit'), so nothing in a controller test + * reaches this method any more. It stays because the *.mcp.ts tools never pass through + * an HTTP guard, and it is tested directly here for the same reason. + */ +describe('TodoService.canEdit', () => { + it('TODO-SVC-090 asks for packing_edit and flags a non-owner as shared', () => { + const checkPermission = vi.fn(() => true); + const permissions = { checkPermission } as unknown as PermissionsService; + const withStub = new TodoService(new DatabaseService(testDb), permissions, new RealtimeService()); + const trip = { id: 1, user_id: 1 } as never; + + expect(withStub.canEdit(trip, { id: 1, role: 'user' } as never)).toBe(true); + expect(checkPermission).toHaveBeenLastCalledWith('packing_edit', 'user', 1, 1, false); + + withStub.canEdit(trip, { id: 2, role: 'user' } as never); + // The shared flag is what the guard has to reproduce; getting it wrong would give a + // member the owner's rights on somebody else's trip. + expect(checkPermission).toHaveBeenLastCalledWith('packing_edit', 'user', 1, 2, true); + + checkPermission.mockReturnValue(false); + expect(withStub.canEdit(trip, { id: 2, role: 'user' } as never)).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/trip-access.guard.test.ts b/server/tests/unit/nest/trip-access.guard.test.ts new file mode 100644 index 000000000..c84a1351f --- /dev/null +++ b/server/tests/unit/nest/trip-access.guard.test.ts @@ -0,0 +1,182 @@ +/** + * TripAccessGuard — the trip-scoping check that used to be a `requireTrip` helper + * copied into every trip-scoped controller. + * + * The cases here are the ones those copies each carried: an inaccessible trip is a 404 + * "Trip not found" and never a 403 (a 403 would confirm the id exists to someone with + * no business knowing), and a mutation additionally needs its own permission. They + * moved out of days.controller.test.ts with the check itself. + */ +import { describe, it, expect, vi } from 'vitest'; +import { HttpException } from '@nestjs/common'; +import { ROUTE_ARGS_METADATA } from '@nestjs/common/constants'; +import { Reflector } from '@nestjs/core'; +import { + RequirePermission, + TRIP_PERMISSION_KEY, + TRIP_REQUEST_KEY, + TripAccessGuard, +} from '../../../src/nest/permissions/trip-access.guard'; +import { Trip } from '../../../src/nest/permissions/trip.decorator'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import type { User } from '../../../src/types'; + +const owner = { id: 42, role: 'user' } as User; +const member = { id: 7, role: 'user' } as User; +/** Trip 5 belongs to user 42; user 7 is a member of it. Nothing else is reachable. */ +const TRIP = { id: 5, user_id: 42 }; + +function makeGuard(options: { checkPermission?: ReturnType; action?: string } = {}) { + const canAccessTrip = vi.fn((tripId: number, userId: number) => + tripId === 5 && (userId === 42 || userId === 7) ? TRIP : undefined, + ); + const checkPermission = options.checkPermission ?? vi.fn(() => true); + const reflector = { getAllAndOverride: vi.fn(() => options.action) } as unknown as Reflector; + const guard = new TripAccessGuard( + { canAccessTrip } as unknown as DatabaseService, + { checkPermission } as unknown as PermissionsService, + reflector, + ); + return { guard, canAccessTrip, checkPermission, reflector }; +} + +/** The slice of ExecutionContext the guard reads. */ +function ctx(request: Record) { + return { + switchToHttp: () => ({ getRequest: () => request }), + getHandler: () => function handler() {}, + getClass: () => class Controller {}, + } as never; +} + +const thrown = (run: () => unknown) => { + try { + run(); + return null; + } catch (e) { + return e instanceof HttpException ? { status: e.getStatus(), body: e.getResponse() } : e; + } +}; + +describe('TripAccessGuard', () => { + it('TRIPGUARD-001 lets a member through and parks the trip row on the request', () => { + const { guard, canAccessTrip } = makeGuard(); + const request = { user: member, params: { tripId: '5' } } as Record; + expect(guard.canActivate(ctx(request))).toBe(true); + expect(canAccessTrip).toHaveBeenCalledWith(5, 7); + expect(request[TRIP_REQUEST_KEY]).toBe(TRIP); + }); + + it('TRIPGUARD-002 a trip the user cannot reach is 404 "Trip not found", never 403', () => { + const { guard } = makeGuard(); + const stranger = { user: { id: 99, role: 'user' }, params: { tripId: '5' } }; + expect(thrown(() => guard.canActivate(ctx(stranger)))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + const missing = { user: owner, params: { tripId: '404' } }; + expect(thrown(() => guard.canActivate(ctx(missing)))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + }); + + it('TRIPGUARD-003 a non-numeric tripId is refused without touching the database', () => { + const { guard, canAccessTrip } = makeGuard(); + const request = { user: owner, params: { tripId: 'not-a-number' } }; + expect(thrown(() => guard.canActivate(ctx(request)))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(canAccessTrip).not.toHaveBeenCalled(); + }); + + it('TRIPGUARD-004 a missing tripId param is refused rather than read as NaN', () => { + const { guard } = makeGuard(); + expect(thrown(() => guard.canActivate(ctx({ user: owner, params: {} })))).toEqual({ + status: 404, + body: { error: 'Trip not found' }, + }); + expect(thrown(() => guard.canActivate(ctx({ user: owner })))).toEqual({ + status: 404, + body: { error: 'Trip not found' }, + }); + }); + + it('TRIPGUARD-005 no authenticated user is a 401, not a crash on user.id', () => { + // JwtAuthGuard runs first, so this only happens when a route was wired without it. + // Refusing beats turning a wiring mistake into a 500 nobody can read. + const { guard, canAccessTrip } = makeGuard(); + expect(thrown(() => guard.canActivate(ctx({ params: { tripId: '5' } })))).toEqual({ + status: 401, + body: { error: 'Unauthorized' }, + }); + expect(canAccessTrip).not.toHaveBeenCalled(); + }); + + it('TRIPGUARD-006 without @RequirePermission the permission service is never consulted', () => { + const { guard, checkPermission } = makeGuard(); + guard.canActivate(ctx({ user: member, params: { tripId: '5' } })); + expect(checkPermission).not.toHaveBeenCalled(); + }); + + it('TRIPGUARD-007 @RequirePermission passes the action and the shared flag through', () => { + const checkPermission = vi.fn(() => true); + const { guard } = makeGuard({ checkPermission, action: 'day_edit' }); + // A member editing somebody else's trip is the SHARED case… + guard.canActivate(ctx({ user: member, params: { tripId: '5' } })); + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 42, 7, true); + // …and the owner editing their own is not. + guard.canActivate(ctx({ user: owner, params: { tripId: '5' } })); + expect(checkPermission).toHaveBeenLastCalledWith('day_edit', 'user', 42, 42, false); + }); + + it('TRIPGUARD-008 a refused permission is 403 "No permission", and access still came first', () => { + const { guard } = makeGuard({ checkPermission: vi.fn(() => false), action: 'day_edit' }); + expect(thrown(() => guard.canActivate(ctx({ user: member, params: { tripId: '5' } })))).toEqual({ + status: 403, + body: { error: 'No permission' }, + }); + // A stranger gets the 404 rather than the 403: access is checked before rights, so + // the permission answer never leaks that the trip exists. + expect(thrown(() => guard.canActivate(ctx({ user: { id: 99, role: 'user' }, params: { tripId: '5' } })))).toEqual({ + status: 404, + body: { error: 'Trip not found' }, + }); + }); + + it('TRIPGUARD-009 the metadata is read from the handler first, then the class', () => { + const { guard, reflector } = makeGuard({ action: 'day_edit' }); + guard.canActivate(ctx({ user: owner, params: { tripId: '5' } })); + expect(reflector.getAllAndOverride).toHaveBeenCalledWith(TRIP_PERMISSION_KEY, [expect.any(Function), expect.any(Function)]); + }); + + it('TRIPGUARD-010 @RequirePermission writes the action under the key the guard reads', () => { + class Probe { + @RequirePermission('budget_edit') + handler() {} + } + expect(Reflect.getMetadata(TRIP_PERMISSION_KEY, Probe.prototype.handler)).toBe('budget_edit'); + }); +}); + +describe('@Trip() param decorator', () => { + /** + * Nest stores a custom param decorator's factory in ROUTE_ARGS_METADATA on the + * controller class, keyed by `${CUSTOM_ROUTE_ARGS_METADATA}:${index}`. Pulling it + * back out is how the factory gets exercised for real rather than asserted to be a + * function. + */ + function tripFactory(): (data: unknown, context: unknown) => unknown { + class Probe { + handler(@Trip() _trip: unknown) {} + } + const args = Reflect.getMetadata(ROUTE_ARGS_METADATA, Probe, 'handler') as Record< + string, + { factory: (data: unknown, context: unknown) => unknown } + >; + return Object.values(args)[0].factory; + } + + it('TRIPGUARD-011 returns the row the guard parked on the request', () => { + expect(tripFactory()(undefined, ctx({ [TRIP_REQUEST_KEY]: TRIP }))).toBe(TRIP); + }); + + it('TRIPGUARD-012 a route without the guard is a loud throw, not an undefined trip', () => { + // Silently handing the handler `undefined` would turn a missing guard into a + // crash somewhere further down, with nothing pointing at the wiring. + expect(() => tripFactory()(undefined, ctx({}))).toThrow(/without @UseGuards\(TripAccessGuard\)/); + }); +}); diff --git a/server/vitest.config.ts b/server/vitest.config.ts index c1808306b..99f6f7fcb 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -53,6 +53,11 @@ export default defineConfig({ // Raise an entry when you improve a domain. Never lower one to make a build pass — // that is the whole point of the mechanism. // + // Regenerate from a LINUX run if you can: CI runs on Linux, and a few branches are + // platform-specific (the drive-letter half of the backup zip-slip guard is only + // reachable on win32). Numbers taken on Windows can therefore sit a hair above what + // CI measures, which shows up as a threshold failure that reproduces nowhere local. + // // A file matched by a glob below is EXCLUDED from the catch-all, so the last entry // only covers what sits directly under src/nest (app.module.ts and the like). thresholds: {