diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index cb290c66..50a83ba1 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -72,6 +72,15 @@ export class FilesController { 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 + // otherwise leak the foreign reservation's title back to the caller. + private assertLinkTargets(tripId: string, body: { reservation_id?: string | null; assignment_id?: string | null; place_id?: string | null }) { + if (this.files.findForeignLinkTarget(tripId, body)) { + throw new HttpException({ error: 'Linked item does not belong to this trip' }, 400); + } + } + @Get() list(@CurrentUser() user: User, @Param('tripId') tripId: string, @Query('trash') trash?: string) { this.requireTrip(tripId, user); @@ -97,6 +106,7 @@ export class FilesController { if (!file) { throw new HttpException({ error: 'No file uploaded' }, 400); } + this.assertLinkTargets(tripId, { reservation_id: body.reservation_id, place_id: body.place_id }); const created = this.files.createFile(tripId, file, user.id, { place_id: body.place_id, description: body.description, @@ -116,6 +126,7 @@ export class FilesController { if (!file) { throw new HttpException({ error: 'File not found' }, 404); } + this.assertLinkTargets(tripId, { reservation_id: body.reservation_id, place_id: body.place_id }); const updated = this.files.updateFile(id, file, { description: body.description, place_id: body.place_id, reservation_id: body.reservation_id }); this.files.broadcast(tripId, 'file:updated', { file: updated }, socketId); return { file: updated }; @@ -203,6 +214,7 @@ export class FilesController { if (!file) { throw new HttpException({ error: 'File not found' }, 404); } + this.assertLinkTargets(tripId, { reservation_id: body.reservation_id, assignment_id: body.assignment_id, place_id: body.place_id }); const links = this.files.createFileLink(id, { reservation_id: body.reservation_id, assignment_id: body.assignment_id, place_id: body.place_id }); return { success: true, links }; } diff --git a/server/src/nest/files/files.service.ts b/server/src/nest/files/files.service.ts index b21e256b..48ecfeb1 100644 --- a/server/src/nest/files/files.service.ts +++ b/server/src/nest/files/files.service.ts @@ -43,6 +43,7 @@ export class FilesService { restoreFile(id: string) { return svc.restoreFile(id); } permanentDeleteFile(file: TripFile) { return svc.permanentDeleteFile(file); } emptyTrash(tripId: string) { return svc.emptyTrash(tripId); } + findForeignLinkTarget(tripId: string, opts: Parameters[1]) { return svc.findForeignLinkTarget(tripId, opts); } createFileLink(id: string, opts: Parameters[1]) { return svc.createFileLink(id, opts); } deleteFileLink(linkId: string, id: string) { return svc.deleteFileLink(linkId, id); } getFileLinks(id: string) { return svc.getFileLinks(id); } diff --git a/server/src/services/fileService.ts b/server/src/services/fileService.ts index f119344c..f596c4b8 100644 --- a/server/src/services/fileService.ts +++ b/server/src/services/fileService.ts @@ -55,6 +55,34 @@ export function formatFile(file: TripFile & { trip_id?: number; uploaded_by_avat }; } +// --------------------------------------------------------------------------- +// Trip-scoped link validation +// --------------------------------------------------------------------------- + +/** + * A file, and any reservation / day-assignment / place it points at, must all + * live in the same trip. FILE_SELECT and getFileLinks join the reservation and + * return its title, so without this guard a member of trip A could aim a file + * (or a file_link) at trip B's reservation id and read the title back. Returns + * the first field that escapes `tripId`, or null when every supplied id belongs + * to the trip. Absent / null / zero ids are ignored (they clear the link). + */ +export function findForeignLinkTarget( + tripId: string | number, + opts: { reservation_id?: string | number | null; assignment_id?: string | number | null; place_id?: string | number | null } +): 'reservation_id' | 'assignment_id' | 'place_id' | null { + if (opts.reservation_id && !db.prepare('SELECT 1 FROM reservations WHERE id = ? AND trip_id = ?').get(opts.reservation_id, tripId)) { + return 'reservation_id'; + } + if (opts.place_id && !db.prepare('SELECT 1 FROM places WHERE id = ? AND trip_id = ?').get(opts.place_id, tripId)) { + return 'place_id'; + } + if (opts.assignment_id && !db.prepare('SELECT 1 FROM day_assignments a JOIN days d ON a.day_id = d.id WHERE a.id = ? AND d.trip_id = ?').get(opts.assignment_id, tripId)) { + return 'assignment_id'; + } + return null; +} + // --------------------------------------------------------------------------- // File path resolution & validation // --------------------------------------------------------------------------- diff --git a/server/tests/integration/files.test.ts b/server/tests/integration/files.test.ts index 2acff907..c7223c1b 100644 --- a/server/tests/integration/files.test.ts +++ b/server/tests/integration/files.test.ts @@ -54,7 +54,7 @@ import { buildApp } from '../../src/bootstrap'; import { createTables } from '../../src/db/schema'; import { runMigrations } from '../../src/db/migrations'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; -import { createUser, createTrip, createReservation, addTripMember } from '../helpers/factories'; +import { createUser, createTrip, createReservation, createPlace, addTripMember } from '../helpers/factories'; import { authCookie, generateToken } from '../helpers/auth'; let nestApp: INestApplication; @@ -357,6 +357,117 @@ describe('File links', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Cross-trip link isolation (GHSA — reservation title disclosure) +// +// A file may only point at reservations / assignments / places from its own +// trip. The reservation JOIN returns the reservation title, so a member of one +// trip linking a file to another private trip's reservation id used to read the +// foreign title back. Every write path (link, upload, update) must reject it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('Cross-trip link isolation', () => { + it('SEC-FILE-LINK-001 — linking a file to a reservation from another trip is rejected (no title leak)', async () => { + const { user: attacker } = createUser(testDb); + const { user: victim } = createUser(testDb); + const attackerTrip = createTrip(testDb, attacker.id, { title: 'Attacker Trip' }); + const victimTrip = createTrip(testDb, victim.id, { title: 'Victim Trip' }); + const victimReservation = createReservation(testDb, victimTrip.id, { title: 'Victim Secret Flight', type: 'flight' }); + const upload = await uploadFile(attackerTrip.id, attacker.id, FIXTURE_PDF); + const fileId = upload.body.file.id; + + const link = await request(app) + .post(`/api/trips/${attackerTrip.id}/files/${fileId}/link`) + .set('Cookie', authCookie(attacker.id)) + .send({ reservation_id: victimReservation.id }); + expect(link.status).toBe(400); + + // Nothing was stored, so the title cannot leak back through the links list. + const links = await request(app) + .get(`/api/trips/${attackerTrip.id}/files/${fileId}/links`) + .set('Cookie', authCookie(attacker.id)); + expect(links.status).toBe(200); + expect(JSON.stringify(links.body)).not.toContain('Victim Secret Flight'); + expect((links.body.links as any[]).some((l) => l.reservation_id === victimReservation.id)).toBe(false); + }); + + it('SEC-FILE-LINK-002 — uploading a file with a foreign reservation_id is rejected (no title leak)', async () => { + const { user: attacker } = createUser(testDb); + const { user: victim } = createUser(testDb); + const attackerTrip = createTrip(testDb, attacker.id); + const victimTrip = createTrip(testDb, victim.id); + const victimReservation = createReservation(testDb, victimTrip.id, { title: 'Victim Secret Flight', type: 'flight' }); + + const res = await request(app) + .post(`/api/trips/${attackerTrip.id}/files`) + .set('Cookie', authCookie(attacker.id)) + .field('reservation_id', String(victimReservation.id)) + .attach('file', FIXTURE_PDF); + expect(res.status).toBe(400); + expect(JSON.stringify(res.body)).not.toContain('Victim Secret Flight'); + }); + + it('SEC-FILE-LINK-003 — updating a file with a foreign reservation_id is rejected (no title leak)', async () => { + const { user: attacker } = createUser(testDb); + const { user: victim } = createUser(testDb); + const attackerTrip = createTrip(testDb, attacker.id); + const victimTrip = createTrip(testDb, victim.id); + const victimReservation = createReservation(testDb, victimTrip.id, { title: 'Victim Secret Flight', type: 'flight' }); + const upload = await uploadFile(attackerTrip.id, attacker.id, FIXTURE_PDF); + const fileId = upload.body.file.id; + + const res = await request(app) + .put(`/api/trips/${attackerTrip.id}/files/${fileId}`) + .set('Cookie', authCookie(attacker.id)) + .send({ reservation_id: victimReservation.id }); + expect(res.status).toBe(400); + expect(JSON.stringify(res.body)).not.toContain('Victim Secret Flight'); + }); + + it('SEC-FILE-LINK-004 — linking a file to a place from another trip is rejected', async () => { + const { user: attacker } = createUser(testDb); + const { user: victim } = createUser(testDb); + const attackerTrip = createTrip(testDb, attacker.id); + const victimTrip = createTrip(testDb, victim.id); + const victimPlace = createPlace(testDb, victimTrip.id, { name: 'Victim Secret Place' }); + const upload = await uploadFile(attackerTrip.id, attacker.id, FIXTURE_PDF); + const fileId = upload.body.file.id; + + const link = await request(app) + .post(`/api/trips/${attackerTrip.id}/files/${fileId}/link`) + .set('Cookie', authCookie(attacker.id)) + .send({ place_id: victimPlace.id }); + expect(link.status).toBe(400); + + const links = await request(app) + .get(`/api/trips/${attackerTrip.id}/files/${fileId}/links`) + .set('Cookie', authCookie(attacker.id)); + expect((links.body.links as any[]).some((l) => l.place_id === victimPlace.id)).toBe(false); + }); + + it('SEC-FILE-LINK-005 — same-trip reservation links and uploads still succeed', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const resv = createReservation(testDb, trip.id, { title: 'My Own Flight', type: 'flight' }); + + // Upload carrying the trip's own reservation id is accepted. + const upload = await request(app) + .post(`/api/trips/${trip.id}/files`) + .set('Cookie', authCookie(user.id)) + .field('reservation_id', String(resv.id)) + .attach('file', FIXTURE_PDF); + expect(upload.status).toBe(201); + const fileId = upload.body.file.id; + + // And linking it to the same reservation works. + const link = await request(app) + .post(`/api/trips/${trip.id}/files/${fileId}/link`) + .set('Cookie', authCookie(user.id)) + .send({ reservation_id: resv.id }); + expect(link.status).toBe(200); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // Download // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/unit/nest/files.controller.test.ts b/server/tests/unit/nest/files.controller.test.ts index 20020555..c81fc37a 100644 --- a/server/tests/unit/nest/files.controller.test.ts +++ b/server/tests/unit/nest/files.controller.test.ts @@ -21,6 +21,7 @@ function fsvc(o: Partial = {}): FilesService { return { verifyTripAccess: vi.fn().mockReturnValue({ user_id: 1 }), can: vi.fn().mockReturnValue(true), + findForeignLinkTarget: vi.fn().mockReturnValue(null), broadcast: vi.fn(), ...o, } as unknown as FilesService;