diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index 20bee8fd8..6a1a9d04d 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -163,6 +163,9 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const tripActions = useRef(useTripStore.getState()).current const can = useCanDo() const canEditDays = can('day_edit', trip) + // The calendar subscription hands out a link that reads the trip without an + // account, so it sits behind the same permission as the public share link. + const canManageShare = can('share_manage', trip) const { noteUi, setNoteUi, noteInputRef, dayNotes, openAddNote: _openAddNote, openEditNote: _openEditNote, cancelNote, saveNote, deleteNote: _deleteNote, moveNote: _moveNote } = useDayNotes(tripId) @@ -956,6 +959,7 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { tripActions, can, canEditDays, + canManageShare, noteUi, setNoteUi, noteInputRef, @@ -1128,6 +1132,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP tripActions, can, canEditDays, + canManageShare, noteUi, setNoteUi, noteInputRef, @@ -1275,6 +1280,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP setUndoHover={setUndoHover} lastActionLabel={lastActionLabel} canEditDays={canEditDays} + canManageShare={canManageShare} onReorderDays={onReorderDays} onAddDay={onAddDay} /> diff --git a/client/src/components/Planner/DayPlanSidebarToolbar.test.tsx b/client/src/components/Planner/DayPlanSidebarToolbar.test.tsx index a354acb3e..1cf4f43a9 100644 --- a/client/src/components/Planner/DayPlanSidebarToolbar.test.tsx +++ b/client/src/components/Planner/DayPlanSidebarToolbar.test.tsx @@ -133,6 +133,18 @@ describe('DayPlanSidebarToolbar', () => { expect(setIcsHover).toHaveBeenCalledWith(true) }) + it('FE-PLANNER-DPTOOLBAR-005b: without share_manage the menu offers the download but not the subscription', async () => { + // The subscription mints a link that reads the trip without an account, so + // it needs share_manage; the one-off download is a file this member may + // already read. Leaving the entry visible would only produce a dialog whose + // enable button the server refuses. + const user = userEvent.setup() + render() + await user.hover(screen.getByText('ICS')) + expect(screen.getByText('Download ICS')).toBeInTheDocument() + expect(screen.queryByText('Subscribe to calendar')).not.toBeInTheDocument() + }) + it('FE-PLANNER-DPTOOLBAR-006: leaving the ICS area closes the menu after the grace period', async () => { const user = userEvent.setup() const setIcsHover = vi.fn((_v: boolean) => {}) diff --git a/client/src/components/Planner/DayPlanSidebarToolbar.tsx b/client/src/components/Planner/DayPlanSidebarToolbar.tsx index ff743b52d..2d3c0515d 100644 --- a/client/src/components/Planner/DayPlanSidebarToolbar.tsx +++ b/client/src/components/Planner/DayPlanSidebarToolbar.tsx @@ -31,6 +31,14 @@ interface DayPlanSidebarToolbarProps { setUndoHover: (v: boolean) => void lastActionLabel: string | null canEditDays?: boolean + /** + * Gates "Subscribe to calendar" only. The one-off "Download ICS" below it + * stays open to every member: it is a file they already have the right to + * read, while the subscription mints a link that works without an account. + * Defaults to true so a caller that has not wired the permission through + * keeps today's menu rather than silently losing an entry. + */ + canManageShare?: boolean onReorderDays?: (orderedIds: number[]) => void onAddDay?: (position?: number) => void } @@ -40,7 +48,7 @@ export function DayPlanSidebarToolbar({ allConnectionsShown = false, onToggleAllConnections, t, locale, toast, setIcsHover, expandedDays, setExpandedDays, onUndo, canUndo, undoHover, setUndoHover, lastActionLabel, - canEditDays, onReorderDays, onAddDay, + canEditDays, canManageShare = true, onReorderDays, onAddDay, }: DayPlanSidebarToolbarProps) { const [reorderOpen, setReorderOpen] = useState(false) const [subscribeOpen, setSubscribeOpen] = useState(false) @@ -155,23 +163,25 @@ export function DayPlanSidebarToolbar({ Download ICS - + {canManageShare && ( + + )} )} - {subscribeOpen && ( + {subscribeOpen && canManageShare && ( { if (!planner.trip || pdfBusy) return @@ -98,16 +102,18 @@ export default function MExportSheet({ planner, shell }: MTripSheetsProps) { sub={`${planner.trip?.title || 'trip'}.ics`} onClick={() => void downloadIcs()} /> - setSubscribeOpen(true)} - /> + {canManageShare && ( + setSubscribeOpen(true)} + /> + )} - {subscribeOpen && ( + {subscribeOpen && canManageShare && ( { expect(screen.getByText('Auto-updates in your calendar app')).toBeInTheDocument() }) + it('FE-MOB-EXPSH-002b: drops the subscription row without share_manage, keeps the two exports', () => { + // Same gate as the desktop toolbar: the subscription hands out a link that + // works without an account, the download does not. + renderSheet({ can: vi.fn((action: string) => action !== 'share_manage') }) + expect(screen.getByText('PDF')).toBeInTheDocument() + expect(screen.getByText('Download .ics')).toBeInTheDocument() + expect(screen.queryByText('Subscribe to calendar')).not.toBeInTheDocument() + }) + it('FE-MOB-EXPSH-003: hands the planner slices and the flattened day notes to the PDF export', async () => { const { planner } = renderSheet({ places: [{ id: 9, name: 'Fushimi Inari' }], diff --git a/server/src/nest/feeds/feeds.controller.ts b/server/src/nest/feeds/feeds.controller.ts index f7a1db98f..7526e8fe3 100644 --- a/server/src/nest/feeds/feeds.controller.ts +++ b/server/src/nest/feeds/feeds.controller.ts @@ -2,7 +2,6 @@ import { Controller, Delete, Get, - HttpException, Param, Post, Put, @@ -16,7 +15,7 @@ import { FeedsService } from './feeds.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import type { User } from '../../types'; -import { DatabaseService } from '../database/database.service'; +import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; // Resolve the public origin used to build feed URLs. APP_URL wins — it is the // canonical externally-reachable URL behind a reverse proxy. When it is unset @@ -73,23 +72,21 @@ export class FeedsPublicController { * POST = enable (mint a token, idempotent) * PUT = rotate (new token, invalidates the old URL) * DELETE = disable (clear the token, public URL stops resolving) + * + * All four need `share_manage`, not merely trip access. The token is the only + * credential the anonymous /api/feed/trip/:token.ics route asks for, so handing + * it out is handing out the trip, and rotating it cancels every subscription the + * owner and the other members already set up. Reading is gated for the same + * reason as writing: GET returns the credential itself, not its status. Same + * call the invite link makes (trip-invite.controller.ts), and the default policy + * keeps `share_manage` with the owner while an instance may lower it to + * trip_member. */ @Controller('api/trips/:tripId/feed') -@UseGuards(JwtAuthGuard) +@UseGuards(JwtAuthGuard, TripAccessGuard) +@RequirePermission('share_manage') export class TripFeedTokenController { - constructor( - private readonly feeds: FeedsService, - private readonly dbs: DatabaseService, - ) {} - - private assertAccess(tripId: string, userId: number): void { - const row = this.dbs.connection - .prepare( - 'SELECT id FROM trips WHERE id = ? AND (user_id = ? OR id IN (SELECT trip_id FROM trip_members WHERE user_id = ?))', - ) - .get(tripId, userId, userId); - if (!row) throw new HttpException({ error: 'Trip not found' }, 404); - } + constructor(private readonly feeds: FeedsService) {} @Get('token') get(@CurrentUser() user: User, @Param('tripId') tripId: string, @Req() req: Request) { @@ -98,20 +95,17 @@ export class TripFeedTokenController { @Post('token') generate(@CurrentUser() user: User, @Param('tripId') tripId: string, @Req() req: Request) { - this.assertAccess(tripId, user.id); return this.feeds.generateTripToken(tripId, user.id, resolveFeedBase(req)); } @Put('token') rotate(@CurrentUser() user: User, @Param('tripId') tripId: string, @Req() req: Request) { - this.assertAccess(tripId, user.id); - return this.feeds.rotateTripToken(tripId, resolveFeedBase(req)); + return this.feeds.rotateTripToken(tripId, user.id, resolveFeedBase(req)); } @Delete('token') disable(@CurrentUser() user: User, @Param('tripId') tripId: string) { - this.assertAccess(tripId, user.id); - this.feeds.disableTripToken(tripId); + this.feeds.disableTripToken(tripId, user.id); return { feed_url: null }; } } diff --git a/server/src/nest/feeds/feeds.module.ts b/server/src/nest/feeds/feeds.module.ts index 399db42f5..1e699bdd6 100644 --- a/server/src/nest/feeds/feeds.module.ts +++ b/server/src/nest/feeds/feeds.module.ts @@ -1,13 +1,15 @@ import { Module } from '@nestjs/common'; import { CalendarModule } from '../calendar/calendar.module'; import { DatabaseModule } from '../database/database.module'; +import { PermissionsModule } from '../permissions/permissions.module'; import { FeedsService } from './feeds.service'; import { FeedsPublicController, TripFeedTokenController, UserFeedTokenController } from './feeds.controller'; @Module({ // Calendars, not the trip aggregate: feeds only ever needed an ICS string, and // importing TripsModule for it pulled budget, packing, places and the rest in. - imports: [CalendarModule, DatabaseModule], + // Permissions comes in for TripAccessGuard, which gates the trip feed token. + imports: [CalendarModule, DatabaseModule, PermissionsModule], controllers: [FeedsPublicController, TripFeedTokenController, UserFeedTokenController], providers: [FeedsService], }) diff --git a/server/src/nest/feeds/feeds.service.ts b/server/src/nest/feeds/feeds.service.ts index 93faf7d89..d3265ac60 100644 --- a/server/src/nest/feeds/feeds.service.ts +++ b/server/src/nest/feeds/feeds.service.ts @@ -42,25 +42,41 @@ export class FeedsService { return { feed_url: row?.feed_token ? feedUrl(row.feed_token, 'trip', base) : null }; } + /** + * The three writes carry the acting user and scope the UPDATE to a trip that + * user can reach, the same predicate tripTokenRow reads through. The route + * guard is what decides whether they may manage the credential at all; this is + * the second lock, so a caller that reaches the service another way cannot + * mint or clear a token on a trip id it merely guessed. + */ + private static readonly REACHABLE = + 'id = ? AND (user_id = ? OR id IN (SELECT trip_id FROM trip_members WHERE user_id = ?))'; + /** Enable (idempotent): mint a token only if the trip has none yet. */ generateTripToken(tripId: string, userId: number, base: string): { feed_url: string } { const row = this.tripTokenRow(tripId, userId); if (row?.feed_token) return { feed_url: feedUrl(row.feed_token, 'trip', base) }; const token = randomUUID(); - this.db.prepare('UPDATE trips SET feed_token = ? WHERE id = ?').run(token, tripId); + this.db + .prepare(`UPDATE trips SET feed_token = ? WHERE ${FeedsService.REACHABLE}`) + .run(token, tripId, userId, userId); return { feed_url: feedUrl(token, 'trip', base) }; } /** Rotate: always issue a fresh token, invalidating the previous URL. */ - rotateTripToken(tripId: string, base: string): { feed_url: string } { + rotateTripToken(tripId: string, userId: number, base: string): { feed_url: string } { const token = randomUUID(); - this.db.prepare('UPDATE trips SET feed_token = ? WHERE id = ?').run(token, tripId); + this.db + .prepare(`UPDATE trips SET feed_token = ? WHERE ${FeedsService.REACHABLE}`) + .run(token, tripId, userId, userId); return { feed_url: feedUrl(token, 'trip', base) }; } /** Disable: clear the token so the public URL stops resolving. */ - disableTripToken(tripId: string): void { - this.db.prepare('UPDATE trips SET feed_token = NULL WHERE id = ?').run(tripId); + disableTripToken(tripId: string, userId: number): void { + this.db + .prepare(`UPDATE trips SET feed_token = NULL WHERE ${FeedsService.REACHABLE}`) + .run(tripId, userId, userId); } // ── User (all-trips) feed token ────────────────────────────────────────── diff --git a/server/src/nest/share/share.controller.ts b/server/src/nest/share/share.controller.ts index ce5715bda..ee0429877 100644 --- a/server/src/nest/share/share.controller.ts +++ b/server/src/nest/share/share.controller.ts @@ -11,9 +11,10 @@ import { Public } from '../auth/public.decorator'; /** * /api/trips/:tripId/share-link — manage a trip's public read-only share token. * - * Byte-identical to the legacy Express route (server/src/routes/share.ts): trip - * access (404), the 'share_manage' permission (403), and the create-vs-update - * status split (201 on first creation, 200 on a subsequent update). + * Trip access answers 404, the 'share_manage' permission answers 403, and the + * create-vs-update status split is preserved (201 on first creation, 200 on a + * subsequent update). The permission now covers GET as well: it used to require + * trip access alone, which let any member read out the owner's public token. */ @Controller('api/trips/:tripId/share-link') @UseGuards(JwtAuthGuard) @@ -52,9 +53,12 @@ export class TripShareController { @Get() get(@CurrentUser() user: User, @Param('tripId') tripId: string) { - if (!this.share.verifyTripAccess(tripId, user.id)) { - throw new HttpException({ error: 'Trip not found' }, 404); - } + // The token is the whole credential for the anonymous /api/shared/:token + // page, so reading it needs the same share_manage permission as creating or + // deleting it, not just trip access. Trip membership lets someone read the + // trip while signed in; it does not let them hand out a copy that works + // without an account and outlives their membership. + this.requireManage(tripId, user); const info = this.share.get(tripId); return info ? info : { token: null }; } diff --git a/server/src/nest/share/share.mcp.ts b/server/src/nest/share/share.mcp.ts index 0a2806573..9368cfef1 100644 --- a/server/src/nest/share/share.mcp.ts +++ b/server/src/nest/share/share.mcp.ts @@ -32,8 +32,11 @@ export class ShareMcp { }) async getShareLink({ tripId }: { tripId: number }, ctx: McpContext) { // Read parity with the REST route GET /api/trips/:tripId/share-link, which - // only requires trip membership (share_manage gates create/delete, not read). + // requires share_manage on every verb including this one: the payload is the + // token itself, and a token is an anonymous copy of the trip. Leaving this + // one on membership alone would just move the same hole to MCP. if (!this.share.verifyTripAccess(String(tripId), ctx.userId)) return noAccess(); + if (!hasTripPermission('share_manage', tripId, ctx.userId)) return permissionDenied(); const link = this.share.get(String(tripId)); return ok({ link }); } diff --git a/server/src/nest/trip-read-model/trip-read-model.service.ts b/server/src/nest/trip-read-model/trip-read-model.service.ts index 12feb55e3..39deb1952 100644 --- a/server/src/nest/trip-read-model/trip-read-model.service.ts +++ b/server/src/nest/trip-read-model/trip-read-model.service.ts @@ -10,6 +10,7 @@ import { PlacesService } from '../places/places.service'; import { TodoService } from '../todo/todo.service'; import { FilesService } from '../files/files.service'; import { TripMembersService } from '../trip-members/trip-members.service'; +import { withoutFeedToken } from '../trips/trips.service'; /** * The two read aggregates over a trip: the MCP summary and the offline bundle. @@ -47,7 +48,9 @@ export class TripReadModelService { // ── Trip summary (used by MCP get_trip_summary tool) ────────────────────── getTripSummary(tripId: number, viewerUserId?: number) { - const trip = this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record | undefined; + const trip = withoutFeedToken( + this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record | undefined, + ); if (!trip) return null; const ownerRow = this.getOwner(tripId); diff --git a/server/src/nest/trips/trips.rpc.ts b/server/src/nest/trips/trips.rpc.ts index 552e89196..c73cae9f4 100644 --- a/server/src/nest/trips/trips.rpc.ts +++ b/server/src/nest/trips/trips.rpc.ts @@ -11,7 +11,7 @@ import { DaysService } from '../days/days.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; import { TripMembersService } from '../trip-members/trip-members.service'; import { TripMembershipService } from '../trip-membership/trip-membership.service'; -import { TripsService, NotFoundError, ValidationError } from './trips.service'; +import { TripsService, NotFoundError, ValidationError, withoutFeedToken } from './trips.service'; const TRIP_EDIT_ACTION = 'trip_edit'; const MEMBER_MANAGE_ACTION = 'member_manage'; @@ -47,7 +47,9 @@ export class TripsRpc { @PluginMethod('trips.getById', { permission: 'db:read:trips' }) getById(params: Record, ctx: PluginRpcContext): unknown { return this.guards.tripRead(params, ctx, () => - this.db.prepare('SELECT * FROM trips WHERE id = ?').get(num(params.tripId, 'tripId')), + // db:read:trips is a read grant on the trip, not on the credential that + // publishes it anonymously — see withoutFeedToken. + withoutFeedToken(this.db.prepare('SELECT * FROM trips WHERE id = ?').get(num(params.tripId, 'tripId'))), ); } diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 761a0bf7b..975bbe129 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -16,8 +16,27 @@ import { NotFoundError, ValidationError } from '../common/domain-errors'; export const MS_PER_DAY = 86400000; export const MAX_TRIP_DAYS = 365; +/** + * Strips `feed_token` from a trip row on its way out. + * + * The column is the sole credential for the anonymous /api/feed/trip/:token.ics + * route, and `SELECT t.*` hands it to every reader of the trip. Gating the + * token endpoint on `share_manage` means nothing while any member can read the + * same value out of the trip payload, so the two go together. No TREK client + * reads the field (it is absent from client/ and shared/ entirely). + */ +export function withoutFeedToken(row: T): T { + if (row && typeof row === 'object') delete (row as Record).feed_token; + return row; +} + +// `NULL AS feed_token` after `t.*` rather than an explicit column list: the +// duplicate name wins in the row object, so the credential is blanked once here +// instead of at each of the nine call sites, and the next migration that adds a +// column does not have to remember to extend a hand-maintained list. export const TRIP_SELECT = ` SELECT t.*, + NULL AS feed_token, (SELECT COUNT(*) FROM days d WHERE d.trip_id = t.id) as day_count, (SELECT COUNT(*) FROM places p WHERE p.trip_id = t.id) as place_count, CASE WHEN t.user_id = :userId THEN 1 ELSE 0 END as is_owner, diff --git a/server/tests/e2e/feeds.e2e.test.ts b/server/tests/e2e/feeds.e2e.test.ts index af2158a60..7b35ad7c6 100644 --- a/server/tests/e2e/feeds.e2e.test.ts +++ b/server/tests/e2e/feeds.e2e.test.ts @@ -34,7 +34,23 @@ const { db } = vi.hoisted(() => { return { db: tmp }; }); -vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: () => {} })); +// canAccessTrip/isOwner back TripAccessGuard, which now gates the trip token +// routes. They run the same predicates as the real module against the temp db. +vi.mock('../../src/db/database', () => ({ + db, + closeDb: () => {}, + reinitialize: () => {}, + canAccessTrip: (tripId: number, userId: number) => + db + .prepare( + `SELECT t.id, t.user_id FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? + WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL)`, + ) + .get(userId, tripId, userId), + isOwner: (tripId: number, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), +})); // Own the calendar parts so we control the events and can assert which trips were pulled. const SAMPLE_EVENT = @@ -158,6 +174,49 @@ describe('Calendar-feed e2e (real auth guard + temp SQLite)', () => { expect(res.body).toEqual({ error: 'Trip not found' }); }); + // ── share_manage on the token routes ─────────────────────────────────────── + // The token is the only credential /api/feed/trip/:token.ics asks for, so a + // plain member must not be able to read, mint, rotate or clear it. Under the + // default policy share_manage sits with the trip owner. + + it('403 on all four verbs for a member without share_manage', async () => { + db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (5, 2)').run(); + const member = sessionCookie(2); + + for (const res of [ + await request(server).get('/api/trips/5/feed/token').set('Cookie', member), + await request(server).post('/api/trips/5/feed/token').set('Cookie', member), + await request(server).put('/api/trips/5/feed/token').set('Cookie', member), + await request(server).delete('/api/trips/5/feed/token').set('Cookie', member), + ]) { + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'No permission' }); + } + // Refused, not silently applied: the column is untouched. + expect((db.prepare('SELECT feed_token FROM trips WHERE id = 5').get() as { feed_token: string | null }).feed_token).toBeNull(); + }); + + it('a non-member still gets 404 rather than 403, so the 403 is no existence oracle', async () => { + const stranger = sessionCookie(2); + for (const res of [ + await request(server).get('/api/trips/5/feed/token').set('Cookie', stranger), + await request(server).put('/api/trips/5/feed/token').set('Cookie', stranger), + await request(server).delete('/api/trips/5/feed/token').set('Cookie', stranger), + ]) { + expect(res.status).toBe(404); + } + }); + + it('a token issued before the tightening keeps working anonymously', async () => { + const gen = await request(server).post('/api/trips/5/feed/token').set('Cookie', sessionCookie(1)); + const token = gen.body.feed_url.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; + db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (5, 2)').run(); + + // The member may no longer manage it, but existing subscriptions must not break. + expect((await request(server).delete('/api/trips/5/feed/token').set('Cookie', sessionCookie(2))).status).toBe(403); + expect((await request(server).get(`/api/feed/trip/${token}.ics`)).status).toBe(200); + }); + // ── Public trip feed ─────────────────────────────────────────────────────── it('public trip feed: 200 text/calendar with injected refresh hints', async () => { diff --git a/server/tests/integration/share.test.ts b/server/tests/integration/share.test.ts index 220102e8a..42d89dd56 100644 --- a/server/tests/integration/share.test.ts +++ b/server/tests/integration/share.test.ts @@ -268,6 +268,58 @@ describe('Shared trip access', () => { .send({}); expect(res.status).toBe(404); }); + + it('SHARE-026 — a plain member cannot read the share token', async () => { + // Reading the link hands out the credential for the anonymous /api/shared + // page, which outlives the membership that was used to fetch it. Under the + // default policy share_manage stays with the owner. + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await request(app).post(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(owner.id)).send({}); + + const res = await request(app).get(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(member.id)); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'No permission' }); + }); + + it('SHARE-027 — the owner still reads it, and a non-member still gets 404 rather than 403', async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await request(app).post(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(owner.id)).send({}); + + const mine = await request(app).get(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(owner.id)); + expect(mine.status).toBe(200); + expect(mine.body.token).toBeTruthy(); + + // 404, not 403: a stranger must not learn that the trip id exists. + const theirs = await request(app).get(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(stranger.id)); + expect(theirs.status).toBe(404); + }); + + it('SHARE-028 — a member may read it once the instance lowers share_manage to trip_member', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await request(app).post(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(owner.id)).send({}); + + const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions.bridge'); + testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_share_manage', 'trip_member')").run(); + invalidatePermissionsCache(); + try { + const res = await request(app).get(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(member.id)); + expect(res.status).toBe(200); + expect(res.body.token).toBeTruthy(); + } finally { + // Module-scoped cache: leaving it set would decide the next file's tests. + testDb.prepare("DELETE FROM app_settings WHERE key = 'perm_share_manage'").run(); + invalidatePermissionsCache(); + } + }); }); describe('Shared trip — day assignments and notes', () => { diff --git a/server/tests/integration/trips.test.ts b/server/tests/integration/trips.test.ts index 0535e2eb1..aa473aad6 100644 --- a/server/tests/integration/trips.test.ts +++ b/server/tests/integration/trips.test.ts @@ -1032,6 +1032,24 @@ describe('ICS export', () => { const res = await request(app).get(`/api/trips/${trip.id}/export.ics`); expect(res.status).toBe(401); }); + + it('TRIP-025b — the trip payload never carries feed_token, not even for the owner', async () => { + // feed_token is the sole credential for the anonymous ICS feed. TRIP_SELECT + // reads `t.*`, so without the blanking it ships in every trip response and + // gating /feed/token on share_manage would achieve nothing: any member + // could read the value straight out of GET /api/trips/:id. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip' }); + testDb.prepare('UPDATE trips SET feed_token = ? WHERE id = ?').run('secret-feed-token', trip.id); + + const one = await request(app).get(`/api/trips/${trip.id}`).set('Cookie', authCookie(user.id)); + expect(one.status).toBe(200); + expect(one.body.trip.feed_token ?? null).toBeNull(); + expect(JSON.stringify(one.body)).not.toContain('secret-feed-token'); + + const list = await request(app).get('/api/trips').set('Cookie', authCookie(user.id)); + expect(JSON.stringify(list.body)).not.toContain('secret-feed-token'); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/unit/nest/feeds.service.test.ts b/server/tests/unit/nest/feeds.service.test.ts index 64d6c9a07..e1a9f97a8 100644 --- a/server/tests/unit/nest/feeds.service.test.ts +++ b/server/tests/unit/nest/feeds.service.test.ts @@ -151,6 +151,9 @@ describe('trip feed token lifecycle', () => { expect(svc.getTripToken(tripId, outsider.id, BASE)).toEqual({ feed_url: null }); }); + // Membership is what the service checks, and that stays true: whether the + // caller may manage the credential at all is decided one layer up, by + // TripAccessGuard + @RequirePermission('share_manage') on the controller. it('FEED-SVC-005: a trip shared with the user as a member resolves too', () => { const { trip, tripId } = seedTrip('tok-trip'); const { user: member } = createUser(testDb); @@ -178,7 +181,7 @@ describe('trip feed token lifecycle', () => { const before = svc.generateTripToken(tripId, user.id, BASE).feed_url; const oldToken = before.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; - const after = svc.rotateTripToken(tripId, BASE).feed_url; + const after = svc.rotateTripToken(tripId, user.id, BASE).feed_url; expect(after).not.toBe(before); expect(svc.buildTripIcs(oldToken)).toBeNull(); @@ -189,11 +192,27 @@ describe('trip feed token lifecycle', () => { const url = svc.generateTripToken(tripId, user.id, BASE).feed_url; const token = url.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; - svc.disableTripToken(tripId); + svc.disableTripToken(tripId, user.id); expect(svc.getTripToken(tripId, user.id, BASE)).toEqual({ feed_url: null }); expect(svc.buildTripIcs(token)).toBeNull(); }); + + it('FEED-SVC-008b: the writes refuse a trip the acting user cannot reach', () => { + // The route guard is what enforces share_manage; this is the second lock, so + // a caller reaching the service another way cannot mint or clear a token on + // a trip id it merely guessed. + const { user, tripId } = seedTrip(); + const { user: outsider } = createUser(testDb); + const mine = svc.generateTripToken(tripId, user.id, BASE).feed_url; + const myToken = mine.match(/trip\/([0-9a-f-]+)\.ics$/)![1]; + + svc.rotateTripToken(tripId, outsider.id, BASE); + expect(svc.getTripToken(tripId, user.id, BASE).feed_url).toBe(mine); + + svc.disableTripToken(tripId, outsider.id); + expect(svc.buildTripIcs(myToken)).not.toBeNull(); + }); }); // ── User (all-trips) feed token ─────────────────────────────────────────────── diff --git a/server/tests/unit/nest/share.controller.test.ts b/server/tests/unit/nest/share.controller.test.ts index 9277ea1b0..a794a54e4 100644 --- a/server/tests/unit/nest/share.controller.test.ts +++ b/server/tests/unit/nest/share.controller.test.ts @@ -53,8 +53,13 @@ describe('TripShareController', () => { expect(updatedRes.statusCode).toBe(200); }); - it('GET 404 without access, returns info or a null token', () => { + it('GET 404 without access, 403 without share_manage, returns info or a null token', () => { expect(thrown(() => new TripShareController(svc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) })).get(user, '5'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + // Reading returns the token itself, so it needs the same permission as + // creating it — a member with plain trip access must not get a copy. + const get = vi.fn(); + expect(thrown(() => new TripShareController(svc({ canManage: vi.fn().mockReturnValue(false), get } as Partial)).get(user, '5'))).toEqual({ status: 403, body: { error: 'No permission' } }); + expect(get).not.toHaveBeenCalled(); expect(new TripShareController(svc({ get: vi.fn().mockReturnValue({ token: 't' }) } as Partial)).get(user, '5')).toEqual({ token: 't' }); expect(new TripShareController(svc({ get: vi.fn().mockReturnValue(null) } as Partial)).get(user, '5')).toEqual({ token: null }); }); diff --git a/wiki/Admin-Permissions.md b/wiki/Admin-Permissions.md index 841c857c9..4bbda504d 100644 --- a/wiki/Admin-Permissions.md +++ b/wiki/Admin-Permissions.md @@ -60,7 +60,7 @@ Actions are grouped into five categories: | `budget_edit` | Create, edit, or delete budget items | | `packing_edit` | Manage packing items and bags | | `collab_edit` | Create notes, polls, and send messages | -| `share_manage` | Create or delete public share links | +| `share_manage` | Read, create or delete public share links and calendar feed links | ## Changing permissions