diff --git a/server/src/mcp/nest-mcp-policy.ts b/server/src/mcp/nest-mcp-policy.ts index 375d9d682..469f25270 100644 --- a/server/src/mcp/nest-mcp-policy.ts +++ b/server/src/mcp/nest-mcp-policy.ts @@ -1,9 +1,11 @@ -import type { McpAccessPolicy } from '@trek/nest-mcp'; -import { canRead, canWrite } from './scopes'; +import type { McpAccessGroup, McpAccessPolicy, McpAccessValidator } from '@trek/nest-mcp'; +import { ALL_SCOPES, canRead, canWrite, type ScopeGroup } from './scopes'; /** * TREK's context shape for @trek/nest-mcp handlers, predicates and the * access policy — mirrors what registerTools() receives per session. + * The group registry types every decorator's `access.group` against the + * scope-derived union, so a typo'd group is a compile error in `.mcp.ts`. */ declare module '@trek/nest-mcp' { interface McpContext { @@ -11,8 +13,22 @@ declare module '@trek/nest-mcp' { scopes: string[] | null; isStaticToken: boolean; } + // The augmentation pattern requires an empty single-extends interface — + // same false positive (and same disable) as McpContext in @trek/nest-mcp. + // eslint-disable-next-line @typescript-eslint/no-empty-object-type + interface McpAccessGroupRegistry extends Record {} } +type AssertExact = [A] extends [B] ? ([B] extends [A] ? true : never) : never; + +/** + * Compile-time lockstep: the augmented `access.group` union must be exactly + * `ScopeGroup` — fails `npm run typecheck` if the augmentation above drifts + * (which is also the proof that unknown groups like 'budgets' are rejected). + * Exported so the policy unit test has a runtime touchpoint. + */ +export const MCP_ACCESS_GROUPS_MATCH_SCOPE_GROUPS: AssertExact = true; + /** * Resolves declarative `access: { group, mode }` markers with the exact * scopes.ts semantics the legacy registrars used at registration time: @@ -22,3 +38,17 @@ declare module '@trek/nest-mcp' { */ export const trekMcpAccessPolicy: McpAccessPolicy = ({ group, mode }, ctx) => mode === 'write' ? canWrite(ctx.scopes, group) : canRead(ctx.scopes, group); + +const VALID_GROUP_MODES: ReadonlySet = new Set(ALL_SCOPES); + +/** + * Boot gate run by registry.validate(): a declarative marker must resolve to + * a real scope — `{ group, mode }` requires `group:mode` ∈ ALL_SCOPES. Catches + * what the group typing alone cannot: mode drift on read-only groups (`geo` + * and `weather` have no `:write` scope, so `{ group: 'weather', mode: + * 'write' }` type-checks but would silently deny scoped tokens while passing + * `scopes: null` sessions). The offending entry is named by the registry's + * aggregated error. + */ +export const trekMcpValidateAccess: McpAccessValidator = ({ group, mode }) => + VALID_GROUP_MODES.has(`${group}:${mode}`) ? null : `no '${group}:${mode}' scope in SCOPES`; diff --git a/server/src/mcp/scopes.ts b/server/src/mcp/scopes.ts index 5473a245b..2ef5062a7 100644 --- a/server/src/mcp/scopes.ts +++ b/server/src/mcp/scopes.ts @@ -36,6 +36,9 @@ export const SCOPES = { export type Scope = typeof SCOPES[keyof typeof SCOPES]; +/** 'trips' | 'places' | ... — derived from SCOPES; adding a scope extends it. */ +export type ScopeGroup = Scope extends `${infer G}:${string}` ? G : never; + export const ALL_SCOPES: Scope[] = Object.values(SCOPES) as Scope[]; export interface ScopeInfo { @@ -88,13 +91,13 @@ export function canReadTrips(scopes: string[] | null): boolean { } /** group:write grants write access; for trips canReadTrips handles read */ -export function canWrite(scopes: string[] | null, group: string): boolean { +export function canWrite(scopes: string[] | null, group: ScopeGroup): boolean { if (!scopes) return true; return scopes.includes(`${group}:write`); } /** group:read OR group:write grant read access */ -export function canRead(scopes: string[] | null, group: string): boolean { +export function canRead(scopes: string[] | null, group: ScopeGroup): boolean { if (!scopes) return true; return scopes.some(s => s === `${group}:read` || s === `${group}:write`); } diff --git a/server/src/nest/README.md b/server/src/nest/README.md index f854c8654..c404b25c1 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -228,8 +228,12 @@ bridges. reinitialize-proof, same pattern as `nest/todo/todo.bridge.ts` — and exports the legacy function names 1:1. Container code injects the service; only outside-container code imports the - bridge. *(Design decision, settled with the tags pilot: MCP tools stay outside - the container and use the bridge. The alternative — handing the Nest app to the + bridge. When porting an MCP registrar, note the `access: { group, mode }` + markers are typed against the scope-derived `ScopeGroup` union and + boot-validated by `trekMcpValidateAccess` (`src/mcp/nest-mcp-policy.ts`) — + an unknown group, or `mode: 'write'` on a read-only group (`geo`, + `weather`), fails app boot. *(Design decision, settled with the tags pilot: + MCP tools stay outside the container and use the bridge. The alternative — handing the Nest app to the MCP layer via `app.get(XService)` — was rejected: it would thread the container through `mcpHandler` + every tool registrar and force a Nest bootstrap into the container-less `tests/helpers/mcp-harness.ts` used by ~25 MCP suites.)* diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 33bc34fcb..d88128fef 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -47,7 +47,7 @@ import { AuditModule } from './audit/audit.module'; import { PermissionsModule } from './permissions/permissions.module'; import { PluginsModule } from './plugins/plugins.module'; import { McpModule } from '@trek/nest-mcp'; -import { trekMcpAccessPolicy } from '../mcp/nest-mcp-policy'; +import { trekMcpAccessPolicy, trekMcpValidateAccess } from '../mcp/nest-mcp-policy'; import { TrekExceptionFilter } from './common/trek-exception.filter'; import { SpaFallbackFilter } from './platform/spa-fallback.filter'; import { IdempotencyInterceptor } from './common/idempotency.interceptor'; @@ -58,7 +58,7 @@ import { IdempotencyInterceptor } from './common/idempotency.interceptor'; * migrated. */ @Module({ - imports: [AppConfigModule, DatabaseModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy }), WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, MapsModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule], + imports: [AppConfigModule, DatabaseModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }), WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, MapsModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule], controllers: [HealthController], providers: [ HealthService, diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 14548e1f9..d70939cb8 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -1,6 +1,6 @@ import { createTestRegistry, type McpRegistry } from '@trek/nest-mcp'; import { db } from '../../src/db/database'; -import { trekMcpAccessPolicy } from '../../src/mcp/nest-mcp-policy'; +import { trekMcpAccessPolicy, trekMcpValidateAccess } from '../../src/mcp/nest-mcp-policy'; import { AssignmentsMcp } from '../../src/nest/assignments/assignments.mcp'; import { AssignmentsService } from '../../src/nest/assignments/assignments.service'; import { BudgetMcp } from '../../src/nest/budget/budget.mcp'; @@ -54,6 +54,6 @@ export function createMcpTestRegistry(): McpRegistry { new CollabMcp(new CollabService(dbService, permissionsService)), new VacayMcp(new VacayService(dbService)), ], - { accessPolicy: trekMcpAccessPolicy }, + { accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }, ); } diff --git a/server/tests/unit/mcp/nest-mcp-policy.test.ts b/server/tests/unit/mcp/nest-mcp-policy.test.ts new file mode 100644 index 000000000..32db5a8c9 --- /dev/null +++ b/server/tests/unit/mcp/nest-mcp-policy.test.ts @@ -0,0 +1,81 @@ +/** + * Unit tests for the TREK access-group boot gate in + * server/src/mcp/nest-mcp-policy.ts. Pure functions + a bare registry — no DB. + * + * The compile-time half of the group typing (McpAccessGroup === ScopeGroup) + * is asserted in src by MCP_ACCESS_GROUPS_MATCH_SCOPE_GROUPS, which + * `npm run typecheck` covers — tests are not typechecked, so the bad-group + * case below has to cast its way past the union on purpose. + */ +import { describe, it, expect } from 'vitest'; +import { createTestRegistry, McpController, Tool, type McpDeclarativeAccess } from '@trek/nest-mcp'; +import { + MCP_ACCESS_GROUPS_MATCH_SCOPE_GROUPS, + trekMcpAccessPolicy, + trekMcpValidateAccess, +} from '../../../src/mcp/nest-mcp-policy'; +import { ALL_SCOPES, type ScopeGroup } from '../../../src/mcp/scopes'; + +const listing = { kind: 'tool' as const, name: 'x', className: 'X', methodName: 'x' }; + +describe('trekMcpValidateAccess', () => { + it('accepts every {group, mode} combination that resolves to a real scope', () => { + const combos = ALL_SCOPES.filter((s) => s.endsWith(':read') || s.endsWith(':write')).map((s) => { + const [group, mode] = s.split(':'); + return { group, mode } as McpDeclarativeAccess; + }); + expect(combos.length).toBeGreaterThan(0); + for (const access of combos) { + expect(trekMcpValidateAccess(access, listing)).toBeNull(); + } + }); + + it('rejects an unknown group', () => { + const access = { group: 'budgets', mode: 'write' } as unknown as McpDeclarativeAccess; + expect(trekMcpValidateAccess(access, listing)).toBe("no 'budgets:write' scope in SCOPES"); + }); + + it('rejects write mode on the read-only geo and weather groups', () => { + expect(trekMcpValidateAccess({ group: 'geo', mode: 'write' }, listing)).toBe("no 'geo:write' scope in SCOPES"); + expect(trekMcpValidateAccess({ group: 'weather', mode: 'write' }, listing)).toBe( + "no 'weather:write' scope in SCOPES", + ); + expect(trekMcpValidateAccess({ group: 'geo', mode: 'read' }, listing)).toBeNull(); + expect(trekMcpValidateAccess({ group: 'weather', mode: 'read' }, listing)).toBeNull(); + }); + + it('fails createTestRegistry construction for a bad group, naming the entry', () => { + @McpController() + class BadGroupMcp { + @Tool({ name: 'bad_tool', access: { group: 'budgets' as ScopeGroup, mode: 'write' } }) + bad() {} + } + expect(() => + createTestRegistry([new BadGroupMcp()], { + accessPolicy: trekMcpAccessPolicy, + validateAccess: trekMcpValidateAccess, + }), + ).toThrow( + /invalid access declarations: tool "bad_tool" \(BadGroupMcp\.bad\): no 'budgets:write' scope in SCOPES/, + ); + }); + + it('has a compile-checked group union (runtime touchpoint)', () => { + expect(MCP_ACCESS_GROUPS_MATCH_SCOPE_GROUPS).toBe(true); + }); +}); + +describe('trekMcpAccessPolicy', () => { + it('keeps the null-scopes (static token) full-access contract', () => { + const ctx = { userId: 1, scopes: null, isStaticToken: true }; + expect(trekMcpAccessPolicy({ group: 'vacay', mode: 'write' }, ctx)).toBe(true); + expect(trekMcpAccessPolicy({ group: 'trips', mode: 'read' }, ctx)).toBe(true); + }); + + it('resolves scoped sessions with the scopes.ts semantics', () => { + const ctx = { userId: 1, scopes: ['budget:write'], isStaticToken: false }; + expect(trekMcpAccessPolicy({ group: 'budget', mode: 'write' }, ctx)).toBe(true); + expect(trekMcpAccessPolicy({ group: 'budget', mode: 'read' }, ctx)).toBe(true); + expect(trekMcpAccessPolicy({ group: 'trips', mode: 'read' }, ctx)).toBe(false); + }); +}); diff --git a/server/tests/unit/mcp/scopes.test.ts b/server/tests/unit/mcp/scopes.test.ts index cbeeb38e7..15a310eb1 100644 --- a/server/tests/unit/mcp/scopes.test.ts +++ b/server/tests/unit/mcp/scopes.test.ts @@ -45,6 +45,37 @@ describe('ALL_SCOPES', () => { expect(Array.isArray(ALL_SCOPES)).toBe(true); expect(ALL_SCOPES.length).toBeGreaterThan(0); }); + + it('derives exactly the 14 known scope groups (ScopeGroup lockstep)', () => { + // The runtime half of the ScopeGroup lockstep — the type half is + // MCP_ACCESS_GROUPS_MATCH_SCOPE_GROUPS in src/mcp/nest-mcp-policy.ts, + // covered by `npm run typecheck`. If this list changes, the MCP + // access-group union and boot gate change with it — update deliberately. + const groups = [...new Set(ALL_SCOPES.map((s) => s.split(':')[0]))].sort(); + expect(groups).toEqual([ + 'atlas', + 'budget', + 'collab', + 'collections', + 'geo', + 'journey', + 'notifications', + 'packing', + 'places', + 'reservations', + 'todos', + 'trips', + 'vacay', + 'weather', + ]); + }); + + it('has no :write scope for the read-only geo and weather groups', () => { + expect(ALL_SCOPES).not.toContain('geo:write'); + expect(ALL_SCOPES).not.toContain('weather:write'); + expect(ALL_SCOPES).toContain('geo:read'); + expect(ALL_SCOPES).toContain('weather:read'); + }); }); // ---------------------------------------------------------------------------