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