mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 05:26:47 +00:00
fix(public-api): a key narrowed away from days stops getting days back through places
Places, notes and reservations hang off days, so asking for one of them implies the day container. That implication ran ahead of the grant: a key minted with scopes ['trips','places'] and deliberately no 'days' still received every day's title and the day's own free-text notes, either by naming places or by naming nothing at all, since an absent include narrows to the granted sections and lands back in the same branch. Asking for days by name was a 403 the whole time, so the two routes disagreed about the same fields. The grant now reaches the service alongside the narrowed include. The container is still implied, because the places inside it are useless without their date, but the day's title and notes only go out to a key that was granted days. A key with full access is unaffected: it is granted days whether or not it asks.
This commit is contained in:
@@ -12,6 +12,7 @@ import { ApiTokenGuard } from './api-token.guard';
|
||||
import { PublicApiService } from './public-api.service';
|
||||
import {
|
||||
enforcePublicApiRateLimit,
|
||||
grantedScopes,
|
||||
narrowToGrant,
|
||||
requireScope,
|
||||
requireUserId,
|
||||
@@ -98,7 +99,10 @@ export class PublicApiController {
|
||||
}
|
||||
const allowed = narrowToGrant(asked, req);
|
||||
|
||||
const trip = this.api.getTrip(tripId, requireUserId(req), allowed);
|
||||
// The grant goes in as well as the narrowed include: days are implied by any
|
||||
// section that hangs off them, and the implied block must still be measured
|
||||
// against what the key may read.
|
||||
const trip = this.api.getTrip(tripId, requireUserId(req), allowed, grantedScopes(req));
|
||||
if (!trip) {
|
||||
throw new HttpException({ error: 'Trip not found' }, 404);
|
||||
}
|
||||
|
||||
@@ -61,7 +61,7 @@ export class PublicApiService {
|
||||
* turns both into the same 404, so the endpoint cannot be used to probe which
|
||||
* trip ids exist.
|
||||
*/
|
||||
getTrip(tripId: number, userId: number, include: PublicApiInclude[]): PublicApiTrip | null {
|
||||
getTrip(tripId: number, userId: number, include: PublicApiInclude[], granted: readonly string[] = include): PublicApiTrip | null {
|
||||
if (!this.db.canAccessTrip(tripId, userId)) return null;
|
||||
const row = this.db.get<TripRow>(
|
||||
`SELECT id, title, description, start_date, end_date, currency, is_archived, updated_at
|
||||
@@ -74,8 +74,13 @@ export class PublicApiService {
|
||||
// Places, notes and reservations hang off days, so asking for one of them
|
||||
// and not for `days` used to return the trip and nothing else — silently,
|
||||
// which is the worst way to answer. Days are implied instead.
|
||||
//
|
||||
// Implied, not granted: `granted` is what the key may read at all, and a key
|
||||
// narrowed to places must not get the day's title and free-text notes back
|
||||
// through the container those places arrive in. The shell it does get is the
|
||||
// join key the children are useless without.
|
||||
if (DAY_SCOPED.some((section) => include.includes(section))) {
|
||||
trip.days = this.buildDays(tripId, include);
|
||||
trip.days = this.buildDays(tripId, include, granted.includes('days'));
|
||||
}
|
||||
if (include.includes('places')) {
|
||||
trip.unplanned_places = this.buildUnplannedPlaces(tripId);
|
||||
@@ -100,7 +105,7 @@ export class PublicApiService {
|
||||
* rather than queried per day — a two-week trip would otherwise cost 42 round
|
||||
* trips for the same rows.
|
||||
*/
|
||||
private buildDays(tripId: number, include: PublicApiInclude[]): PublicApiDay[] {
|
||||
private buildDays(tripId: number, include: PublicApiInclude[], dayFields: boolean): PublicApiDay[] {
|
||||
const days = this.db.all<DayRow>(
|
||||
`SELECT id, day_number, date, title, notes
|
||||
FROM days WHERE trip_id = ? ORDER BY day_number ASC`,
|
||||
@@ -117,8 +122,8 @@ export class PublicApiService {
|
||||
return days.map((day) => ({
|
||||
date: day.date,
|
||||
day_number: day.day_number,
|
||||
title: day.title ?? null,
|
||||
notes: day.notes ?? null,
|
||||
title: dayFields ? day.title ?? null : null,
|
||||
notes: dayFields ? day.notes ?? null : null,
|
||||
places: placesByDay.get(day.id) ?? [],
|
||||
day_notes: notesByDay.get(day.id) ?? [],
|
||||
reservations: reservationsByDay.get(day.id) ?? [],
|
||||
|
||||
@@ -126,6 +126,8 @@ const MCP_TOKEN = 'trek_' + 'c'.repeat(48);
|
||||
/** Narrowed keys, all Ada's, so only the grant differs between them. */
|
||||
const TRIPS_ONLY_TOKEN = 'trek_' + 'd'.repeat(48);
|
||||
const TRIPS_DAYS_TOKEN = 'trek_' + 'e'.repeat(48);
|
||||
/** Places without days: the grant that used to get the day spine anyway. */
|
||||
const TRIPS_PLACES_TOKEN = 'trek_' + '1'.repeat(48);
|
||||
/** api_scopes that cannot be parsed — the fall-back-to-everything case. */
|
||||
const BROKEN_SCOPES_TOKEN = 'trek_' + 'f'.repeat(48);
|
||||
|
||||
@@ -178,6 +180,7 @@ describe('Public API v1 e2e (real guard + real SQL)', () => {
|
||||
// it, so if narrowing ever leaks into the default those tests fail first.
|
||||
seedLimitedToken(1, TRIPS_ONLY_TOKEN, JSON.stringify(['trips']));
|
||||
seedLimitedToken(1, TRIPS_DAYS_TOKEN, JSON.stringify(['trips', 'days', 'notes']));
|
||||
seedLimitedToken(1, TRIPS_PLACES_TOKEN, JSON.stringify(['trips', 'places']));
|
||||
seedLimitedToken(1, BROKEN_SCOPES_TOKEN, '{"not":"a list"');
|
||||
|
||||
// Ada owns trip 1; Bob owns trip 2; trip 3 is Bob's but Ada is a member.
|
||||
@@ -188,7 +191,7 @@ describe('Public API v1 e2e (real guard + real SQL)', () => {
|
||||
db.prepare("INSERT INTO trips (id, user_id, title, start_date) VALUES (3, 2, 'Shared', '2026-08-01')").run();
|
||||
db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (3, 1)').run();
|
||||
|
||||
db.prepare("INSERT INTO days (id, trip_id, day_number, date, title) VALUES (1, 1, 1, '2026-06-14', 'Ankunft')").run();
|
||||
db.prepare("INSERT INTO days (id, trip_id, day_number, date, title, notes) VALUES (1, 1, 1, '2026-06-14', 'Ankunft', 'Schlüssel beim Nachbarn abholen')").run();
|
||||
db.prepare("INSERT INTO days (id, trip_id, day_number, date) VALUES (2, 1, 2, '2026-06-15')").run();
|
||||
db.prepare("INSERT INTO categories (id, name) VALUES (1, 'Museum')").run();
|
||||
db.prepare(
|
||||
@@ -553,6 +556,35 @@ describe('Public API v1 e2e (real guard + real SQL)', () => {
|
||||
expect(JSON.stringify(res.body)).not.toContain('Uffizien');
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-008b: a grant without `days` gets the day shell, not the day', async () => {
|
||||
// Places hang off days, so the day container is implied and has to be. It is
|
||||
// the join key, though, not a way back to the section the key was narrowed to
|
||||
// exclude: the day's title and its own free-text notes stay behind.
|
||||
const res = await get('/api/v1/trips/1?include=places', TRIPS_PLACES_TOKEN);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.days).toHaveLength(2);
|
||||
expect(res.body.days[0]).toMatchObject({ date: '2026-06-14', title: null, notes: null });
|
||||
expect(res.body.days[0].places.map((pl: { name: string }) => pl.name)).toEqual(['Uffizien', 'Ponte Vecchio']);
|
||||
const serialised = JSON.stringify(res.body);
|
||||
expect(serialised).not.toContain('Ankunft');
|
||||
expect(serialised).not.toContain('Nachbarn');
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-008c: the same key with no include at all is narrowed the same way', async () => {
|
||||
// The route that made it a bypass rather than an oddity: `include` absent
|
||||
// means every section, and narrowToGrant reduces it to ['places'], which
|
||||
// implies days all over again.
|
||||
const res = await get('/api/v1/trips/1', TRIPS_PLACES_TOKEN);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.days[0]).toMatchObject({ title: null, notes: null });
|
||||
expect(JSON.stringify(res.body)).not.toContain('Nachbarn');
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-008d: a key that was granted days still reads both fields', async () => {
|
||||
const res = await get('/api/v1/trips/1', TRIPS_DAYS_TOKEN);
|
||||
expect(res.body.days[0]).toMatchObject({ title: 'Ankunft', notes: 'Schlüssel beim Nachbarn abholen' });
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-009: an include the key does cover still answers', async () => {
|
||||
const res = await get('/api/v1/trips/1?include=days,notes', TRIPS_DAYS_TOKEN);
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
@@ -273,7 +273,7 @@ describe('PublicApiController — what a narrowed key reaches', () => {
|
||||
expect(ctl.listTrips(req(ALL))).toEqual({ trips: [TRIP] });
|
||||
expect(ctl.listBucketList(req(ALL))).toEqual({ items: [] });
|
||||
expect(ctl.getTrip(req(ALL), '12', undefined)).toEqual(TRIP);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES]);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES], [...PUBLIC_API_SCOPES]);
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-U021: a trips-only key reads the trip list', () => {
|
||||
@@ -305,20 +305,21 @@ describe('PublicApiController — what a narrowed key reaches', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
apiController({ getTrip }).getTrip(req(limited('trips', 'days', 'notes')), '12', undefined);
|
||||
// `include` absent means "everything"; refusing a key for wanting sections it
|
||||
// never named would make a narrow key unable to read a trip at all.
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes']);
|
||||
// never named would make a narrow key unable to read a trip at all. The grant
|
||||
// rides along so the service can tell an implied day block from a granted one.
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes'], ['trips', 'days', 'notes']);
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-U025: a trips-only key gets the summary and nothing that hangs off it', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
apiController({ getTrip }).getTrip(req(limited('trips')), '12', undefined);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, []);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [], ['trips']);
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-U026: an empty include is treated like an absent one, and narrowed too', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
apiController({ getTrip }).getTrip(req(limited('trips', 'days')), '12', ' ');
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days']);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days'], ['trips', 'days']);
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-U027: a section named explicitly and not granted is a 403, not a silent drop', () => {
|
||||
@@ -340,7 +341,7 @@ describe('PublicApiController — what a narrowed key reaches', () => {
|
||||
it('PUBAPI-SCOPE-U028: an include the key covers is passed through untouched', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
apiController({ getTrip }).getTrip(req(limited('trips', 'days', 'notes')), '12', 'days, notes');
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes']);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes'], ['trips', 'days', 'notes']);
|
||||
});
|
||||
|
||||
it('PUBAPI-SCOPE-U029: a bad id is still a 400 — the scope check does not swallow it', () => {
|
||||
|
||||
@@ -112,19 +112,19 @@ describe('PublicApiController', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
const res = makeController({ getTrip }).getTrip(req(7), '12', undefined);
|
||||
expect(res).toEqual(TRIP);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES]);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES], expect.arrayContaining(['days']));
|
||||
});
|
||||
|
||||
it('treats an empty include the same as an absent one', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
makeController({ getTrip }).getTrip(req(7), '12', ' ');
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES]);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, [...PUBLIC_API_INCLUDES], expect.arrayContaining(['days']));
|
||||
});
|
||||
|
||||
it('narrows to the requested sections and tolerates spacing', () => {
|
||||
const getTrip = vi.fn().mockReturnValue(TRIP);
|
||||
makeController({ getTrip }).getTrip(req(7), '12', 'days, notes');
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes']);
|
||||
expect(getTrip).toHaveBeenCalledWith(12, 7, ['days', 'notes'], expect.arrayContaining(['days']));
|
||||
});
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user