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'); });