From 3f8a7a1ea9096b5f7233d492409e5ae6941f900f Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 23 Aug 2026 09:34:45 +0200 Subject: [PATCH] fix(feeds): keep subscribable calendars working under default-deny The ICS feed routes are documented as public - the secret token in the URL is the credential - but nothing in the code said so, and the guard this release makes global is default-deny. Every calendar client subscribed to /api/feed/trip/.ics would have started getting 401 on upgrade, and a calendar app that stops fetching just goes quiet. The e2e suite asserted anonymous access the whole time. It booted [DatabaseModule, RealtimeModule, FeedsModule], and the APP_GUARDs live in AppModule, so it was asserting against an app that had no opinion on anonymity. It boots the real guard stack now, and fails without the fix. The boot ratchet could not have caught this either: it only looked at routes carrying @Public(), and GlobalAuthGuard stands down for ANY declared chain - so a chain of guards that refuse a caller but never resolve one answers strangers, invisibly. Split the inventory in two and write that half down as well. It turns up two routes nobody had listed: the passkey login ceremony, which genuinely runs before a session exists. --- .../src/nest/common/validate-route-guards.ts | 72 ++++++++++++++++--- server/src/nest/feeds/feeds.controller.ts | 2 + server/tests/e2e/feeds.e2e.test.ts | 17 ++++- .../tests/unit/nest/route-inventory.test.ts | 24 ++++++- 4 files changed, 104 insertions(+), 11 deletions(-) diff --git a/server/src/nest/common/validate-route-guards.ts b/server/src/nest/common/validate-route-guards.ts index 73222ae92..e566e2014 100644 --- a/server/src/nest/common/validate-route-guards.ts +++ b/server/src/nest/common/validate-route-guards.ts @@ -2,14 +2,27 @@ import { METHOD_METADATA, PATH_METADATA, GUARDS_METADATA } from '@nestjs/common/ import { ModulesContainer } from '@nestjs/core'; import type { INestApplication } from '@nestjs/common'; import { IS_PUBLIC, OPTIONAL_AUTH } from '../auth/public.decorator'; +import { JwtAuthGuard } from '../auth/jwt-auth.guard'; +import { CookieAuthGuard } from '../auth/cookie-auth.guard'; +import { OptionalJwtGuard } from '../auth/optional-jwt.guard'; export interface RouteGuardEntry { /** `ControllerClass.methodName` */ id: string; /** How the route is covered, for the failure message. */ - cover: 'public' | 'optional-auth' | 'declared-guards'; + cover: 'public' | 'optional-auth' | 'declared-guards' | 'declared-guards-anonymous'; } +/** + * The guards that actually resolve a caller. GlobalAuthGuard stands down for ANY + * declared chain, so a chain built only from other guards answers anonymously — + * that is what separates 'declared-guards' from 'declared-guards-anonymous'. + * TripAccessGuard and friends are absent on purpose: they refuse a request with + * no req.user, but they never resolve one, so they only ever appear behind one + * of these three. + */ +const AUTHENTICATING_GUARDS: unknown[] = [JwtAuthGuard, CookieAuthGuard, OptionalJwtGuard]; + /** * Boot-time gate: every registered route must be authenticated, or say why not. * @@ -23,7 +36,9 @@ export interface RouteGuardEntry { * declared @UseGuards chain (on the handler or the class). Everything else falls * to GlobalAuthGuard, which is fine — that is the default — so this gate does * not fail on it. What it reports is the inventory, so the exempt set is a list - * somebody can read. + * somebody can read. A declared chain is split in two, because GlobalAuthGuard + * stands down on any chain at all: one that authenticates, and one that answers + * strangers. */ export function collectRouteGuards(app: INestApplication): RouteGuardEntry[] { const container = app.get(ModulesContainer, { strict: false }); @@ -53,7 +68,10 @@ export function collectRouteGuards(app: INestApplication): RouteGuardEntry[] { if (isPublic) entries.push({ id, cover: 'public' }); else if (isOptional) entries.push({ id, cover: 'optional-auth' }); - else if (classGuards.length > 0 || handlerGuards.length > 0) entries.push({ id, cover: 'declared-guards' }); + else if (classGuards.length > 0 || handlerGuards.length > 0) { + const authenticates = [...handlerGuards, ...classGuards].some((g) => AUTHENTICATING_GUARDS.includes(g)); + entries.push({ id, cover: authenticates ? 'declared-guards' : 'declared-guards-anonymous' }); + } } } } @@ -62,9 +80,11 @@ export function collectRouteGuards(app: INestApplication): RouteGuardEntry[] { } /** - * The routes that answer without a session. Reviewed as a list on purpose: this - * is the whole anonymous surface of the server in one place, and a diff that - * grows it is a diff that widens what strangers can reach. + * The routes that answer without a session because they carry @Public(). + * Reviewed as a list on purpose: together with + * ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST this is the whole anonymous surface of the + * server in one place, and a diff that grows either is a diff that widens what + * strangers can reach. * * Entries are `ControllerClass.methodName`. Sorted, no duplicates. */ @@ -89,6 +109,10 @@ export const PUBLIC_ROUTE_ALLOW_LIST: string[] = [ 'FeaturesController.features', // The container/uptime probe. 'FeaturesController.health', + // Subscribable ICS feeds. The token in the path is the credential, and the + // calendar client polling it has no TREK session to send. + 'FeedsPublicController.tripFeed', + 'FeedsPublicController.userFeed', // The download link carries its own short-lived token. 'FilesDownloadController.download', 'HelpController.asset', @@ -119,17 +143,38 @@ export const PUBLIC_ROUTE_ALLOW_LIST: string[] = [ ]; /** - * Throws when a route carries @Public() without being listed above, or when a + * The other half of the anonymous surface: routes with no @Public() that still + * answer without a session, because the guards they declare never resolve a + * caller. GlobalAuthGuard stands down on a declared chain, so this is not a + * hole the gate should close — it is one it has to keep visible. + */ +export const ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST: string[] = [ + // The passkey login ceremony, which by definition runs before a session + // exists. PasskeyEnabledGuard only 404s when the admin toggle is off. + 'PasskeyController.loginOptions', + 'PasskeyController.loginVerify', +]; + +/** + * Throws when a route answers anonymously without being listed above, or when a * listed route no longer exists. Stale entries fail too, for the same reason the * body-contract gate fails on them: a list nobody prunes stops being a list. */ -export function validateRouteGuards(app: INestApplication, allowList: string[] = PUBLIC_ROUTE_ALLOW_LIST): void { +export function validateRouteGuards( + app: INestApplication, + allowList: string[] = PUBLIC_ROUTE_ALLOW_LIST, + anonymousGuardedAllowList: string[] = ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST, +): void { const entries = collectRouteGuards(app); const publicIds = entries.filter((e) => e.cover === 'public').map((e) => e.id); + const anonymousIds = entries.filter((e) => e.cover === 'declared-guards-anonymous').map((e) => e.id); const allowed = new Set(allowList); + const anonymousAllowed = new Set(anonymousGuardedAllowList); const undeclared = publicIds.filter((id) => !allowed.has(id)); const stale = allowList.filter((id) => !publicIds.includes(id)); + const undeclaredAnonymous = anonymousIds.filter((id) => !anonymousAllowed.has(id)); + const staleAnonymous = anonymousGuardedAllowList.filter((id) => !anonymousIds.includes(id)); const problems: string[] = []; if (undeclared.length > 0) { @@ -142,6 +187,17 @@ export function validateRouteGuards(app: INestApplication, allowList: string[] = `PUBLIC_ROUTE_ALLOW_LIST entries that are no longer @Public():\n ${stale.join('\n ')}`, ); } + if (undeclaredAnonymous.length > 0) { + problems.push( + 'route(s) whose declared guard chain never authenticates, but not in ' + + `ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST:\n ${undeclaredAnonymous.join('\n ')}`, + ); + } + if (staleAnonymous.length > 0) { + problems.push( + `ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST entries that now authenticate or are gone:\n ${staleAnonymous.join('\n ')}`, + ); + } if (problems.length > 0) { throw new Error( `Anonymous route surface changed.\n\n${problems.join('\n\n')}\n\n` + diff --git a/server/src/nest/feeds/feeds.controller.ts b/server/src/nest/feeds/feeds.controller.ts index 7526e8fe3..9bbc67649 100644 --- a/server/src/nest/feeds/feeds.controller.ts +++ b/server/src/nest/feeds/feeds.controller.ts @@ -16,6 +16,7 @@ import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import type { User } from '../../types'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; +import { Public } from '../auth/public.decorator'; // 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 @@ -35,6 +36,7 @@ function resolveFeedBase(req: Request): string { * The secret token in the URL acts as the access credential. */ @Controller('api/feed') +@Public('the secret token in the feed URL is the credential; calendar clients never send a session') export class FeedsPublicController { constructor(private readonly feeds: FeedsService) {} diff --git a/server/tests/e2e/feeds.e2e.test.ts b/server/tests/e2e/feeds.e2e.test.ts index 219706150..25520e195 100644 --- a/server/tests/e2e/feeds.e2e.test.ts +++ b/server/tests/e2e/feeds.e2e.test.ts @@ -18,6 +18,7 @@ import cookieParser from 'cookie-parser'; import type { Server } from 'http'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { Test } from '@nestjs/testing'; +import { APP_GUARD } from '@nestjs/core'; import { seedUser, sessionCookie } from './harness'; const { db } = vi.hoisted(() => { @@ -73,6 +74,9 @@ import { FeedsModule } from '../../src/nest/feeds/feeds.module'; import { CalendarService } from '../../src/nest/calendar/calendar.service'; import { RealtimeModule } from '../../src/nest/realtime/realtime.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; +import { AppConfigModule } from '../../src/nest/app-config/app-config.module'; +import { GlobalAuthGuard } from '../../src/nest/auth/global-auth.guard'; +import { MfaPolicyGuard } from '../../src/nest/auth/mfa-policy.guard'; const BASE = 'https://trek.example.test'; @@ -82,7 +86,18 @@ describe('Calendar-feed e2e (real auth guard + temp SQLite)', () => { let prevAppUrl: string | undefined; async function build() { - const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, RealtimeModule, FeedsModule] }).compile(); + // The global guards decide whether a route answers a stranger, and they live + // in AppModule — a suite that pins 'anonymous feed → 200' has to boot them, + // or it proves nothing about the app that actually runs (a missing @Public() + // on the feed controller passed here while production 401'd every calendar + // client). + const moduleRef = await Test.createTestingModule({ + imports: [AppConfigModule, DatabaseModule, RealtimeModule, FeedsModule], + providers: [ + { provide: APP_GUARD, useClass: GlobalAuthGuard }, + { provide: APP_GUARD, useClass: MfaPolicyGuard }, + ], + }).compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); diff --git a/server/tests/unit/nest/route-inventory.test.ts b/server/tests/unit/nest/route-inventory.test.ts index 2ff8a5dd6..9864908d7 100644 --- a/server/tests/unit/nest/route-inventory.test.ts +++ b/server/tests/unit/nest/route-inventory.test.ts @@ -18,7 +18,12 @@ vi.mock('../../../src/db/database', async () => { import { Test } from '@nestjs/testing'; import { AppModule } from '../../../src/nest/app.module'; import { DatabaseService } from '../../../src/nest/database/database.service'; -import { collectRouteGuards, PUBLIC_ROUTE_ALLOW_LIST, validateRouteGuards } from '../../../src/nest/common/validate-route-guards'; +import { + ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST, + collectRouteGuards, + PUBLIC_ROUTE_ALLOW_LIST, + validateRouteGuards, +} from '../../../src/nest/common/validate-route-guards'; async function buildApp() { const moduleRef = await Test.createTestingModule({ imports: [AppModule] }) @@ -70,8 +75,23 @@ describe('route guard inventory', () => { const entries = collectRouteGuards(app); // The inventory only lists routes that opted out of the default. Anything // absent from it falls to GlobalAuthGuard, which is the safe direction. - expect(entries.every((e) => ['public', 'optional-auth', 'declared-guards'].includes(e.cover))).toBe(true); + const covers = ['public', 'optional-auth', 'declared-guards', 'declared-guards-anonymous']; + expect(entries.every((e) => covers.includes(e.cover))).toBe(true); expect(entries.filter((e) => e.cover === 'public').length).toBe(PUBLIC_ROUTE_ALLOW_LIST.length); + expect(entries.filter((e) => e.cover === 'declared-guards-anonymous').map((e) => e.id)) + .toEqual(ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST); + } finally { + await app.close(); + } + }); + + it('ROUTES-006: a guard chain that never authenticates fails the gate unless it is written down', async () => { + const app = await buildApp(); + try { + expect(() => validateRouteGuards(app, PUBLIC_ROUTE_ALLOW_LIST, [])) + .toThrow(ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST[0]); + expect(() => validateRouteGuards(app, PUBLIC_ROUTE_ALLOW_LIST, [...ANONYMOUS_GUARDED_ROUTE_ALLOW_LIST, 'GhostController.anonymous'])) + .toThrow('GhostController.anonymous'); } finally { await app.close(); }