mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-30 18:46:00 +00:00
fix(files): reject cross-trip reservation/place/assignment links
A member of one trip could point a file at a reservation, place or day-assignment belonging to another, private trip — on upload, on a metadata update, or through the file-link endpoint. The reservation join in the file list and the links list then returned that trip's reservation title, disclosing it across the trip boundary and letting an attacker enumerate foreign reservation titles by their id. The file already had to belong to the caller's trip; now the linked reservation/place/assignment must too. findForeignLinkTarget checks each supplied id against the trip (assignments via day -> trip) and the upload, update and link handlers reject a cross-trip reference with 400 before it is stored. Same-trip links and clearing a link are unchanged.
This commit is contained in:
@@ -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 };
|
||||
}
|
||||
|
||||
@@ -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<typeof svc.findForeignLinkTarget>[1]) { return svc.findForeignLinkTarget(tripId, opts); }
|
||||
createFileLink(id: string, opts: Parameters<typeof svc.createFileLink>[1]) { return svc.createFileLink(id, opts); }
|
||||
deleteFileLink(linkId: string, id: string) { return svc.deleteFileLink(linkId, id); }
|
||||
getFileLinks(id: string) { return svc.getFileLinks(id); }
|
||||
|
||||
@@ -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
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -21,6 +21,7 @@ function fsvc(o: Partial<FilesService> = {}): 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;
|
||||
|
||||
Reference in New Issue
Block a user