From 0f0bf50cd339caa8d1544e2cd2d28979e3fe04c5 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 16 Aug 2026 21:37:36 +0200 Subject: [PATCH] fix(assignments): guard the per-assignment routes like the day routes AssignmentOpsController declared @RequirePermission('day_edit') on its mutations but carried only JwtAuthGuard. TripAccessGuard is what reads that decorator and what resolves :tripId, so on this controller the permission markers were metadata nobody looked at and the trip in the URL was never matched against the caller. Its sibling, DayAssignmentsController, has had the pair since the migration. The two participants handlers also went straight to the service with the id from the path, unlike move/time/transport, which ask getAssignmentForTrip first. They ask now too. --- .../assignments/assignments.controller.ts | 13 ++- server/tests/e2e/assignments.e2e.test.ts | 88 +++++++++++++++++++ .../unit/nest/assignments.controller.test.ts | 14 ++- 3 files changed, 110 insertions(+), 5 deletions(-) diff --git a/server/src/nest/assignments/assignments.controller.ts b/server/src/nest/assignments/assignments.controller.ts index d2c0dbecd..b076762de 100644 --- a/server/src/nest/assignments/assignments.controller.ts +++ b/server/src/nest/assignments/assignments.controller.ts @@ -113,9 +113,14 @@ export class DayAssignmentsController { /** * /api/trips/:tripId/assignments/:id/* — per-assignment ops (move, time, * participants), independent of the day path. Same parity rules as above. + * + * Same guard pair as the day controller, and for the same reason: TripAccessGuard + * is what resolves :tripId and what reads @RequirePermission. The per-handler + * getAssignmentForTrip calls answer a different question — whether the assignment + * sits on the trip in the URL, not whether the caller may be on that trip. */ @Controller('api/trips/:tripId/assignments') -@UseGuards(JwtAuthGuard) +@UseGuards(JwtAuthGuard, TripAccessGuard) export class AssignmentOpsController { constructor(private readonly assignments: AssignmentsService) {} @@ -142,6 +147,9 @@ export class AssignmentOpsController { @Get(':id/participants') participants(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string) { + if (!this.assignments.getAssignmentForTrip(id, tripId)) { + throw new HttpException({ error: 'Assignment not found' }, 404); + } return { participants: this.assignments.getParticipants(id) }; } @@ -191,6 +199,9 @@ export class AssignmentOpsController { @Body() body: AssignmentParticipantsDto, @Headers('x-socket-id') socketId?: string, ) { + if (!this.assignments.getAssignmentForTrip(id, tripId)) { + throw new HttpException({ error: 'Assignment not found' }, 404); + } const participants = this.assignments.setParticipants(id, body.user_ids); this.assignments.broadcast(tripId, 'assignment:participants', { assignmentId: Number(id), participants }, socketId); return { participants }; diff --git a/server/tests/e2e/assignments.e2e.test.ts b/server/tests/e2e/assignments.e2e.test.ts index ccde14883..e6b5c1ba2 100644 --- a/server/tests/e2e/assignments.e2e.test.ts +++ b/server/tests/e2e/assignments.e2e.test.ts @@ -194,4 +194,92 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => { expect(res.status).toBe(200); expect(res.body.assignment).toMatchObject({ id, day_id: 4, order_index: 0 }); }); + + // The per-assignment controller declared @RequirePermission but not the guard + // that reads it, so the decorators were inert metadata and :tripId was never + // checked against the caller at all. The handlers that ask + // getAssignmentForTrip(id, tripId) were happy as long as the assignment sat on + // the trip in the URL — which is true for the owner's trip too. + describe('a trip the caller cannot see', () => { + const FOREIGN_TRIP = 9; + + beforeEach(() => { + canAccessTrip.mockImplementation((tripId: unknown) => (Number(tripId) === 5 ? { id: 5, user_id: 1 } : undefined)); + db.prepare('INSERT OR IGNORE INTO days (id, trip_id) VALUES (30, ?), (31, ?)').run(FOREIGN_TRIP, FOREIGN_TRIP); + db.prepare('INSERT OR IGNORE INTO places (id, trip_id, name) VALUES (20, ?, ?)').run(FOREIGN_TRIP, 'Their hotel'); + }); + + const seedForeignAssignment = () => + Number(db.prepare('INSERT INTO day_assignments (day_id, place_id, order_index) VALUES (30, 20, 0)').run().lastInsertRowid); + + it('404s a move instead of reordering their itinerary', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/move`) + .set('Cookie', sessionCookie(1)) + .send({ new_day_id: 31, order_index: 0 }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT day_id FROM day_assignments WHERE id = ?').get(id)).toEqual({ day_id: 30 }); + }); + + it('404s a time change instead of rewriting their schedule', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/time`) + .set('Cookie', sessionCookie(1)) + .send({ place_time: '23:00', end_time: null }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT assignment_time FROM day_assignments WHERE id = ?').get(id)).toEqual({ assignment_time: null }); + }); + + it('404s a transport change', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/transport`) + .set('Cookie', sessionCookie(1)) + .send({ transport_mode: 'driving' }); + expect(res.status).toBe(404); + }); + + it('404s setting participants instead of writing to their assignment', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)) + .send({ user_ids: [1] }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 }); + }); + + it('404s reading participants instead of disclosing who is on it', async () => { + const id = seedForeignAssignment(); + db.prepare('INSERT INTO assignment_participants (assignment_id, user_id) VALUES (?, 2)').run(id); + const res = await request(server) + .get(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + }); + + // The guard has to reject an assignment borrowed from elsewhere even when the + // caller is legitimately on the trip named in the URL. + it('404s an assignment that belongs to another trip than the URL says', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/5/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)) + .send({ user_ids: [1] }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 }); + }); + + it('403s when the caller is on the trip but lacks day_edit', async () => { + const id = seedAssignment(); + checkPermission.mockReturnValue(false); + const res = await request(server) + .put(`/api/trips/5/assignments/${id}/time`) + .set('Cookie', sessionCookie(1)) + .send({ place_time: '09:00', end_time: null }); + expect(res.status).toBe(403); + }); + }); }); diff --git a/server/tests/unit/nest/assignments.controller.test.ts b/server/tests/unit/nest/assignments.controller.test.ts index e032dfa14..2079addef 100644 --- a/server/tests/unit/nest/assignments.controller.test.ts +++ b/server/tests/unit/nest/assignments.controller.test.ts @@ -78,8 +78,10 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', ( expect(reconcile).toHaveBeenCalledWith('5', 'sock'); }); - it('GET /:id/participants returns participants (access-only)', () => { - const s = svc({ getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial); + it('GET /:id/participants 404 when the assignment is on another trip, else returns participants (access-only)', () => { + expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined) } as Partial)).participants(user, '5', '9'))) + .toEqual({ status: 404, body: { error: 'Assignment not found' } }); + const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial); expect(new AssignmentOpsController(s).participants(user, '5', '9')).toEqual({ participants: [{ user_id: 2 }] }); }); @@ -92,9 +94,13 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', ( expect(reconcile).toHaveBeenCalledWith('5', 'sock'); }); - it('PUT /:id/participants sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => { + it('PUT /:id/participants 404 on a foreign assignment, else sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => { const setParticipants = vi.fn().mockReturnValue([{ user_id: 2 }]); const broadcast = vi.fn(); - expect(new AssignmentOpsController(svc({ setParticipants, broadcast } as Partial)).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] }); + expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined), setParticipants } as Partial)).setParticipants(user, '5', '9', { user_ids: [2] }))) + .toEqual({ status: 404, body: { error: 'Assignment not found' } }); + expect(setParticipants).not.toHaveBeenCalled(); + const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), setParticipants, broadcast } as Partial); + expect(new AssignmentOpsController(s).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] }); expect(setParticipants).toHaveBeenCalledWith('9', [2]); expect(broadcast).toHaveBeenCalledWith('5', 'assignment:participants', { assignmentId: 9, participants: [{ user_id: 2 }] }, 'sock'); });