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: {