Require share_manage for the two public credentials (#1883)

Both advisories are the same mistake: a route that hands out a bearer token
treated trip membership as authorisation. Membership lets someone read the trip
while they are signed in. It does not let them mint a URL that reads it without
an account, keeps working after they are removed, and can be rotated out from
under the owner's calendar subscriptions.

The calendar feed token now sits behind TripAccessGuard plus
@RequirePermission('share_manage'), the same shape the rest of the trip-scoped
controllers use, which also replaces a hand-written access check that GET never
called at all. GET is gated with the writes because it returns the credential
itself, not its status. The share link's GET does what its own POST and DELETE
already did, and what the invite link next door already does for exactly this
reason.

Two things had to come with it or the gate would have been decoration. The MCP
tool get_share_link read the same token on membership alone, so it would simply
have become the new way in. And feed_token rides along in every trip payload
because TRIP_SELECT reads t.*: any member could have read the value out of
GET /api/trips/:id, the summary tool or the plugin RPC. It is blanked once in
the select and stripped in the two places that select the row directly.

The three service writes take the acting user and scope their UPDATE to a trip
that user can reach. The guard decides who may manage the credential; this only
makes sure a caller arriving another way cannot write to an id it guessed.

Client side, "Subscribe to calendar" is now behind the same permission on
desktop and mobile. Without that a member would have kept an entry whose enable
button silently does nothing, because IcsSubscribeModal swallows every non-200.
The one-off ICS download stays where it is: that is a file they may already read.

Instances that want members to manage these links can lower share_manage to
trip_member in the admin panel, which the tests cover.

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

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],
 })

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 ──────────────────────────────────────────

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

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

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<string, unknown> | undefined;
+    const trip = withoutFeedToken(
+      this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record<string, unknown> | undefined,
+    );
     if (!trip) return null;
 
     const ownerRow = this.getOwner(tripId);

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

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<T>(row: T): T {
+  if (row && typeof row === 'object') delete (row as Record<string, unknown>).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,