diff --git a/server/src/nest/addons/mcp-addon-gate.ts b/server/src/nest/addons/mcp-addon-gate.ts new file mode 100644 index 000000000..4cb30d37b --- /dev/null +++ b/server/src/nest/addons/mcp-addon-gate.ts @@ -0,0 +1,19 @@ +import type { RequestHandler } from 'express'; +import { AddonsService } from './addons.service'; +import { ADDON_IDS } from '../../addons'; + +/** + * Gate: 404 (empty body) when the MCP addon is disabled (M2 — prevents feature + * fingerprinting). A factory over the injected AddonsService rather than a + * middleware class: consumers build it inside their module's configure() and + * hand the bare RequestHandler to consumer.apply() ahead of the SDK routers. + */ +export function createMcpAddonGate(addons: AddonsService): RequestHandler { + return (_req, res, next) => { + if (!addons.isAddonEnabled(ADDON_IDS.MCP)) { + res.status(404).end(); + return; + } + next(); + }; +} diff --git a/server/src/mcp/oauthProvider.ts b/server/src/nest/oauth/oauth-sdk.provider.ts similarity index 77% rename from server/src/mcp/oauthProvider.ts rename to server/src/nest/oauth/oauth-sdk.provider.ts index bd6843a30..1f44dcc67 100644 --- a/server/src/mcp/oauthProvider.ts +++ b/server/src/nest/oauth/oauth-sdk.provider.ts @@ -1,36 +1,24 @@ +import { Injectable } from '@nestjs/common'; import type { Response } from 'express'; -import type { OAuthServerProvider } from '@modelcontextprotocol/sdk/server/auth/provider'; +import type { OAuthServerProvider, AuthorizationParams } from '@modelcontextprotocol/sdk/server/auth/provider'; import type { OAuthClientInformationFull, OAuthTokenRevocationRequest, OAuthTokens } from '@modelcontextprotocol/sdk/shared/auth'; import type { AuthInfo } from '@modelcontextprotocol/sdk/server/auth/types'; -import type { AuthorizationParams } from '@modelcontextprotocol/sdk/server/auth/provider'; import type { OAuthRegisteredClientsStore } from '@modelcontextprotocol/sdk/server/auth/clients'; import { InvalidClientMetadataError, ServerError } from '@modelcontextprotocol/sdk/server/auth/errors'; -import { db } from '../db/database'; -import { - createOAuthClient, - consumeAuthCode, - issueTokens, - refreshTokens, - revokeToken as serviceRevokeToken, - verifyPKCE, - getUserByAccessToken, -} from '../nest/oauth/oauth.bridge'; -import { ALL_SCOPES } from './scopes'; -import { getMcpSafeUrl } from '../app-config'; -import { writeAudit } from '../nest/audit/audit.bridge'; +import { OauthService } from './oauth.service'; +import { AuditService } from '../audit/audit.service'; +import { ALL_SCOPES } from '../../mcp/scopes'; +import { getMcpSafeUrl } from '../../app-config'; -// --------------------------------------------------------------------------- -// DB row type (mirrors oauthService.ts) -// --------------------------------------------------------------------------- - -interface OAuthClientRow { - client_id: string; - name: string; - redirect_uris: string; // JSON array - allowed_scopes: string; // JSON array - is_public: number; // 0 | 1 - created_via: string; -} +/** + * TREK's adapters behind the MCP SDK's OAuth server interfaces, wrapping the + * injected OauthService. The SDK's authorizationHandler / clientRegistrationHandler + * Express routers are built over these instances in OauthModule.configure(). + * + * Pending authorization codes stay module-scoped in oauth.pending-codes.ts: the + * consent controller writes a code through the container singleton and + * exchangeAuthorizationCode reads it back through the same injected singleton. + */ // --------------------------------------------------------------------------- // Redirect URI validation (mirrors oauth.ts DCR checks) @@ -60,7 +48,7 @@ function assertValidRedirectUris(uris: string[]): void { // Row → SDK client info shape // --------------------------------------------------------------------------- -function rowToInfo(row: OAuthClientRow): OAuthClientInformationFull { +function rowToInfo(row: NonNullable>): OAuthClientInformationFull { return { client_id: row.client_id, client_name: row.name, @@ -76,13 +64,14 @@ function rowToInfo(row: OAuthClientRow): OAuthClientInformationFull { // Clients store // --------------------------------------------------------------------------- -export const trekClientsStore: OAuthRegisteredClientsStore = { +@Injectable() +export class TrekClientsStore implements OAuthRegisteredClientsStore { + constructor(private readonly oauth: OauthService) {} + async getClient(clientId: string): Promise { - const row = db.prepare( - 'SELECT client_id, name, redirect_uris, allowed_scopes, is_public, created_via FROM oauth_clients WHERE client_id = ?' - ).get(clientId) as OAuthClientRow | undefined; + const row = this.oauth.getSdkClient(clientId); return row ? rowToInfo(row) : undefined; - }, + } async registerClient( metadata: Omit, @@ -99,7 +88,7 @@ export const trekClientsStore: OAuthRegisteredClientsStore = { const scopes = rawScopes.filter(s => (ALL_SCOPES as string[]).includes(s)); if (scopes.length === 0) throw new InvalidClientMetadataError('No valid scopes requested'); - const result = createOAuthClient(null, name, uris, scopes, null, { isPublic, createdVia: 'dcr' }); + const result = this.oauth.createOAuthClient(null, name, uris, scopes, null, { isPublic, createdVia: 'dcr' }); if (result.error) throw new InvalidClientMetadataError(result.error); const c = result.client!; @@ -113,15 +102,22 @@ export const trekClientsStore: OAuthRegisteredClientsStore = { response_types: ['code'], ...(c.client_secret ? { client_secret: c.client_secret as string, client_secret_expires_at: 0 } : {}), }; - }, -}; + } +} // --------------------------------------------------------------------------- // OAuthServerProvider // --------------------------------------------------------------------------- -export const trekOAuthProvider: OAuthServerProvider = { - get clientsStore() { return trekClientsStore; }, +@Injectable() +export class TrekOAuthProvider implements OAuthServerProvider { + constructor( + private readonly clients: TrekClientsStore, + private readonly oauth: OauthService, + private readonly audit: AuditService, + ) {} + + get clientsStore(): OAuthRegisteredClientsStore { return this.clients; } // Redirects browser to the SPA consent page with OAuth params forwarded. async authorize(client: OAuthClientInformationFull, params: AuthorizationParams, res: Response): Promise { @@ -149,15 +145,15 @@ export const trekOAuthProvider: OAuthServerProvider = { const base = getMcpSafeUrl().replace(/\/+$/, ''); res.redirect(302, `${base}/oauth/consent?${qs.toString()}`); - }, + } // Not called because skipLocalPkceValidation = true. // PKCE verification is done inline in exchangeAuthorizationCode. - skipLocalPkceValidation: true, + readonly skipLocalPkceValidation = true; async challengeForAuthorizationCode(_client: OAuthClientInformationFull, _code: string): Promise { throw new ServerError('PKCE validation is handled by the provider directly'); - }, + } async exchangeAuthorizationCode( client: OAuthClientInformationFull, @@ -166,7 +162,7 @@ export const trekOAuthProvider: OAuthServerProvider = { redirectUri?: string, resource?: URL, ): Promise { - const pending = consumeAuthCode(code); + const pending = this.oauth.consumeAuthCode(code); if (!pending || pending.clientId !== client.client_id) throw new Error('Authorization grant is invalid.'); @@ -177,18 +173,18 @@ export const trekOAuthProvider: OAuthServerProvider = { if (pending.resource && resourceStr && pending.resource !== resourceStr) throw new Error('Authorization grant is invalid.'); - if (codeVerifier && !verifyPKCE(codeVerifier, pending.codeChallenge)) + if (codeVerifier && !this.oauth.verifyPKCE(codeVerifier, pending.codeChallenge)) throw new Error('Authorization grant is invalid.'); - const tokens = issueTokens(client.client_id, pending.userId, pending.scopes, null, pending.resource ?? null); - writeAudit({ + const tokens = this.oauth.issueTokens(client.client_id, pending.userId, pending.scopes, null, pending.resource ?? null); + this.audit.writeAudit({ userId: pending.userId, action: 'oauth.token.issue', details: { client_id: client.client_id, scopes: pending.scopes, audience: pending.resource ?? null }, ip: null, }); return tokens; - }, + } async exchangeRefreshToken( client: OAuthClientInformationFull, @@ -196,13 +192,13 @@ export const trekOAuthProvider: OAuthServerProvider = { _scopes?: string[], _resource?: URL, ): Promise { - const result = refreshTokens(refreshToken, client.client_id, client.client_secret, null); + const result = this.oauth.refreshTokens(refreshToken, client.client_id, client.client_secret, null); if (result.error) throw new Error(result.error === 'invalid_client' ? 'Invalid client credentials' : 'Refresh token is invalid or expired'); return result.tokens!; - }, + } async verifyAccessToken(token: string): Promise { - const info = getUserByAccessToken(token); + const info = this.oauth.getUserByAccessToken(token); if (!info) throw new Error('Invalid or expired token'); return { token, @@ -210,12 +206,12 @@ export const trekOAuthProvider: OAuthServerProvider = { scopes: info.scopes, extra: { user: info.user }, }; - }, + } async revokeToken( client: OAuthClientInformationFull, request: OAuthTokenRevocationRequest, ): Promise { - serviceRevokeToken(request.token, client.client_id, undefined, null); - }, -}; \ No newline at end of file + this.oauth.revokeToken(request.token, client.client_id, undefined, null); + } +} diff --git a/server/src/nest/oauth/oauth.bridge.ts b/server/src/nest/oauth/oauth.bridge.ts index 4c2075e75..40e3759bf 100644 --- a/server/src/nest/oauth/oauth.bridge.ts +++ b/server/src/nest/oauth/oauth.bridge.ts @@ -5,22 +5,17 @@ import { DatabaseService } from '../database/database.service'; import { OauthService } from './oauth.service'; /** - * Non-Nest entry point for the OAuth domain — for code running OUTSIDE the - * Nest container: the MCP transport (src/mcp/index.ts token verification) and - * the SDK provider adapter (src/mcp/oauthProvider.ts), which + * Non-Nest entry point for the OAuth domain — down to its last consumer: the + * MCP transport's token verification (src/mcp/index.ts), which * platform.routes.ts mounts on the Express app BEFORE app.init(), so - * `app.get(OauthService)` is not available to it. Exports the legacy - * services/oauthService names 1:1 so repointing a consumer is an - * import-path-only diff. Inside the container, inject OauthService instead. - * Delete this file when the SDK mount moves behind the container. + * `app.get(OauthService)` is not available to it. The SDK provider adapter + * moved behind the container (oauth-sdk.provider.ts) and dropped the other six + * exports. Inside the container, inject OauthService instead. Delete this file + * when the /mcp mount moves behind the container. * * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. The one piece of shared state — the - * pending authorization codes — is module-scoped in oauth.pending-codes.ts, so - * this instance and the container singleton see the same map. That is load- - * bearing: the consent controller writes the code through the container and - * the SDK reads it back through here (which is why consumeAuthCode is bridged - * and createAuthCode is not). + * the shared better-sqlite3 singleton, and getUserByAccessToken touches no + * module-scoped state beyond it. */ const dbs = new DatabaseService(db); const oauth = new OauthService(dbs, new AddonsService(dbs), new AuditService(dbs)); @@ -28,27 +23,3 @@ const oauth = new OauthService(dbs, new AddonsService(dbs), new AuditService(dbs export function getUserByAccessToken(rawToken: string) { return oauth.getUserByAccessToken(rawToken); } - -export function createOAuthClient(...args: Parameters) { - return oauth.createOAuthClient(...args); -} - -export function consumeAuthCode(code: string) { - return oauth.consumeAuthCode(code); -} - -export function issueTokens(...args: Parameters) { - return oauth.issueTokens(...args); -} - -export function refreshTokens(...args: Parameters) { - return oauth.refreshTokens(...args); -} - -export function revokeToken(...args: Parameters) { - return oauth.revokeToken(...args); -} - -export function verifyPKCE(codeVerifier: string, codeChallenge: string): boolean { - return oauth.verifyPKCE(codeVerifier, codeChallenge); -} diff --git a/server/src/nest/oauth/oauth.module.ts b/server/src/nest/oauth/oauth.module.ts index ddb1f1020..9dc0e1299 100644 --- a/server/src/nest/oauth/oauth.module.ts +++ b/server/src/nest/oauth/oauth.module.ts @@ -1,27 +1,66 @@ import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; +import type { MiddlewareConsumer, NestModule } from '@nestjs/common'; +import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize'; +import { clientRegistrationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/register'; import { OauthPublicController } from './oauth-public.controller'; import { OauthApiController } from './oauth-api.controller'; import { OauthService } from './oauth.service'; +import { TrekClientsStore, TrekOAuthProvider } from './oauth-sdk.provider'; import { AuditModule } from '../audit/audit.module'; import { AddonsModule } from '../addons/addons.module'; +import { AddonsService } from '../addons/addons.service'; +import { createMcpAddonGate } from '../addons/mcp-addon-gate'; /** * OAuth 2.1 server (MCP). Public token/userinfo/revoke endpoints + the SPA's - * authenticated consent/client/session management. /oauth/authorize, - * /oauth/register and /oauth/consent are still served by the MCP SDK's own - * router, mounted on the Express platform before app.init() — that path - * reaches the domain through oauth.bridge.ts, which is why the pending - * authorization codes live in a module, not on the provider. + * authenticated consent/client/session management, plus the SDK's own + * /oauth/authorize and /oauth/register Express routers, mounted here through + * MiddlewareConsumer over the injected TrekOAuthProvider/TrekClientsStore. + * forRoutes('') is an Express prefix mount — the same app.use('/path') + * shape (and the same req.url stripping) the pre-init platform mount used, and + * exactly what the SDK routers are built for. The handlers are constructed once + * per app instance in configure(), preserving the per-app lifetime of the DCR + * handler's built-in rate limiter. * - * Exports OauthService for AdminController, which serves the admin OAuth-session - * panel under /api/admin. That is the one in-container consumer; everything - * outside the container still goes through oauth.bridge.ts. + * The handlers bring their own body parsers; the global 100kb parsers run + * first and theirs no-op on the already-consumed stream. The one observable + * difference vs the pre-init mount: bracketed keys in a POST /oauth/authorize + * body now parse with qs (extended:true) rather than querystring — the SDK + * zod-rejects such bodies either way. + * + * Pending authorization codes live module-scoped in oauth.pending-codes.ts: + * the consent controller (container singleton) writes them, the SDK exchange + * path reads them back through the same injected singleton. + * + * Exports OauthService for AdminController (admin OAuth-session panel) and the + * MCP transport's token verification. */ @Module({ imports: [RateLimitModule, AuditModule, AddonsModule], controllers: [OauthPublicController, OauthApiController], - providers: [OauthService], + providers: [OauthService, TrekClientsStore, TrekOAuthProvider], exports: [OauthService], }) -export class OauthModule {} +export class OauthModule implements NestModule { + constructor( + private readonly addons: AddonsService, + private readonly provider: TrekOAuthProvider, + private readonly clients: TrekClientsStore, + ) {} + + configure(consumer: MiddlewareConsumer): void { + const mcpAddonGate = createMcpAddonGate(this.addons); + + // SDK authorize handler: validates OAuth params, calls provider.authorize() + // which redirects to the SPA consent page at /oauth/consent + consumer + .apply(mcpAddonGate, authorizationHandler({ provider: this.provider })) + .forRoutes('oauth/authorize'); + + // SDK DCR handler: accepts registrations without scope (fixes issue #959 bug 2) + consumer + .apply(mcpAddonGate, clientRegistrationHandler({ clientsStore: this.clients })) + .forRoutes('oauth/register'); + } +} diff --git a/server/src/nest/oauth/oauth.pending-codes.ts b/server/src/nest/oauth/oauth.pending-codes.ts index 5f74374aa..19dff12b6 100644 --- a/server/src/nest/oauth/oauth.pending-codes.ts +++ b/server/src/nest/oauth/oauth.pending-codes.ts @@ -1,13 +1,14 @@ /** * The short-lived authorization-code store. * - * Module-scoped + import-time interval on purpose (legacy parity, shared - * between bridge and container instances — atlas-geo interval precedent). - * This is not a stylistic choice: the SDK-mounted /oauth/authorize path reads - * a code through oauth.bridge (mcp/oauthProvider.ts) that the Nest consent - * controller wrote through the container singleton. Two instances would mean - * two maps, and the authorization-code flow would fail *silently* — the SDK - * only ever answers "Authorization grant is invalid." + * Module-scoped + import-time interval on purpose (legacy parity — atlas-geo + * interval precedent). This is not a stylistic choice: the consent controller + * writes a code through the container OauthService and the SDK exchange path + * (oauth-sdk.provider.ts) reads it back — through the same injected singleton + * in production, but the integration harness and any second OauthService + * instance must keep seeing the same map. Two maps would make the + * authorization-code flow fail *silently* — the SDK only ever answers + * "Authorization grant is invalid." * * No DB persistence: codes live two minutes and a restart invalidating them * is the correct behaviour. diff --git a/server/src/nest/oauth/oauth.service.ts b/server/src/nest/oauth/oauth.service.ts index 064bdda8d..14de55807 100644 --- a/server/src/nest/oauth/oauth.service.ts +++ b/server/src/nest/oauth/oauth.service.ts @@ -349,6 +349,22 @@ export class OauthService { // Token verification (used by MCP handler on every request) // ------------------------------------------------------------------------- + /** SDK clients-store read: the exact row shape the MCP SDK adapter + * (oauth-sdk.provider.ts) maps to OAuthClientInformationFull. */ + getSdkClient(clientId: string): { + client_id: string; + name: string; + redirect_uris: string; + allowed_scopes: string; + is_public: number; + created_via: string; + } | undefined { + return this.db.get( + 'SELECT client_id, name, redirect_uris, allowed_scopes, is_public, created_via FROM oauth_clients WHERE client_id = ?', + clientId, + ); + } + getUserByAccessToken(rawToken: string): OAuthTokenInfo | null { const hash = hashToken(rawToken); const row = this.db.get(` diff --git a/server/src/nest/platform/platform.routes.ts b/server/src/nest/platform/platform.routes.ts index 51807e4ec..efb8e1799 100644 --- a/server/src/nest/platform/platform.routes.ts +++ b/server/src/nest/platform/platform.routes.ts @@ -6,11 +6,6 @@ import { readEnv } from '../../app-config'; import { verifyJwtAndLoadUser } from '../auth/jwt-verify'; import { db } from '../../db/database'; import { mcpHandler } from '../../mcp'; -import { trekOAuthProvider, trekClientsStore } from '../../mcp/oauthProvider'; -import { isAddonEnabled } from '../addons/addons.bridge'; -import { ADDON_IDS } from '../../addons'; -import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize'; -import { clientRegistrationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/register'; // Platform / transport routes extracted verbatim from createApp() (app.ts) so they can be // mounted on either the legacy Express app or the NestJS Express instance (strangler A6/A8). @@ -100,27 +95,14 @@ export function applyPlatformUploads(app: express.Application): void { } /** - * The OAuth SDK + /mcp transport mounts still living on the pre-init Express - * layer. The rest of the former transport surface is behind the container now: - * /api/health (FeaturesController), OAuth discovery (DiscoveryController + the - * McpMetadataMiddleware bootstrap applies pre-init), and the /oauth/consent - * COOP override (ConsentCoopMiddleware via PlatformModule.configure). + * The /mcp transport mounts — the last pre-init platform routes. Everything + * else that used to live here is behind the container now: /api/health + * (FeaturesController), OAuth discovery (DiscoveryController + the metadata + * middleware bootstrap applies pre-init), /oauth/authorize + /oauth/register + * (OauthModule.configure over the injected SDK adapters), and the + * /oauth/consent COOP override (ConsentCoopMiddleware). */ export function applyPlatformTransport(app: express.Application): void { - // OAuth 2.1 — public endpoints - // Gate: 404 when MCP addon is disabled (M2 — prevents feature fingerprinting) - const mcpAddonGate = (_req: Request, res: Response, next: NextFunction) => { - if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); - next(); - }; - - // SDK authorize handler: validates OAuth params, calls provider.authorize() which redirects - // to the SPA consent page at /oauth/consent - app.use('/oauth/authorize', mcpAddonGate, authorizationHandler({ provider: trekOAuthProvider })); - - // SDK DCR handler: accepts registrations without scope (fixes issue #959 bug 2) - app.use('/oauth/register', mcpAddonGate, clientRegistrationHandler({ clientsStore: trekClientsStore })); - // MCP endpoint app.post('/mcp', mcpHandler); app.get('/mcp', mcpHandler); diff --git a/server/tests/integration/oauth-authorize.test.ts b/server/tests/integration/oauth-authorize.test.ts new file mode 100644 index 000000000..3217e8f5f --- /dev/null +++ b/server/tests/integration/oauth-authorize.test.ts @@ -0,0 +1,289 @@ +/** + * HTTP-level tests for GET /oauth/authorize — the MCP SDK authorizationHandler + * wrapping TREK's OAuthServerProvider. Written as byte-level parity oracles for + * the MCP/OAuth mount migration: they must pass identically while the handler + * is mounted pre-init AND after it moves behind the Nest container. + * + * This was the one platform surface with no HTTP coverage at all — + * trekOAuthProvider.authorize and trekClientsStore.getClient run for the first + * time under test here. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import request from 'supertest'; +import type { Application } from 'express'; +import type { INestApplication } from '@nestjs/common'; +import crypto from 'crypto'; + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: () => undefined, + isOwner: () => false, + }; + return { testDb: db, dbMock: mock }; +}); + +vi.mock('../../src/db/database', () => dbMock); +vi.mock('../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, + SESSION_DURATION: '24h', + SESSION_DURATION_MS: 86400000, + SESSION_DURATION_SECONDS: 86400, + DEFAULT_LANGUAGE: 'en', +})); + +vi.mock('../../src/app-config', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getMcpSafeUrl: () => 'https://trek.example.com' }; +}); + +vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); +vi.mock('../../src/mcp/sessionManager', () => ({ revokeUserSessions: vi.fn(), revokeUserSessionsForClient: vi.fn(), sessions: new Map() })); + +import { buildApp } from '../../src/bootstrap'; +import { createTables } from '../../src/db/schema'; +import { runMigrations } from '../../src/db/migrations'; +import { resetTestDb, resetRateLimits } from '../helpers/test-db'; +import { createUser } from '../helpers/factories'; +import { OauthService } from '../../src/nest/oauth/oauth.service'; +import { DatabaseService } from '../../src/nest/database/database.service'; +import { AddonsService } from '../../src/nest/addons/addons.service'; +import { AuditService } from '../../src/nest/audit/audit.service'; + +// The consent controller writes pending codes through the container instance; +// the SDK-mounted authorize path reads them back. The map is module-scoped in +// oauth.pending-codes.ts, so a hand-built service instance shares it — the +// same single-instance property the full auth-code loop below proves end-to-end. +const oauthDbs = new DatabaseService(testDb); +const containerSideOauth = new OauthService(oauthDbs, new AddonsService(oauthDbs), new AuditService(oauthDbs)); + +let nestApp: INestApplication; +let app: Application; + +function makePkce() { + const verifier = crypto.randomBytes(32).toString('base64url'); + const challenge = crypto.createHash('sha256').update(verifier).digest('base64url'); + return { verifier, challenge }; +} + +function setMcpEnabled(enabled: boolean) { + testDb.prepare( + "INSERT OR REPLACE INTO addons (id, name, description, type, icon, enabled, sort_order) VALUES ('mcp', 'MCP', 'AI assistant integration', 'integration', 'Terminal', ?, 12)" + ).run(enabled ? 1 : 0); +} + +/** DCR-register a public client and return its client_id. */ +async function registerClient(redirectUri = 'https://client.example.com/cb', scope = 'trips:read'): Promise { + const res = await request(app) + .post('/oauth/register') + .set('Content-Type', 'application/json') + .send({ redirect_uris: [redirectUri], token_endpoint_auth_method: 'none', scope }); + expect(res.status).toBe(201); + return res.body.client_id as string; +} + +beforeAll(async () => { + createTables(testDb); + runMigrations(testDb); + nestApp = await buildApp(); + app = nestApp.getHttpAdapter().getInstance(); +}); + +beforeEach(() => { + resetTestDb(testDb); + resetRateLimits(nestApp); + setMcpEnabled(true); +}); + +afterAll(async () => { + await nestApp.close(); + testDb.close(); +}); + +describe('GET /oauth/authorize — SDK authorizationHandler over trekOAuthProvider', () => { + it('AUTHZ-001 — happy path 302-redirects to the SPA consent page with the exact forwarded params', async () => { + const clientId = await registerClient(); + const { challenge } = makePkce(); + + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + state: 'st4te', + code_challenge: challenge, + code_challenge_method: 'S256', + resource: 'https://trek.example.com/mcp', + }); + + expect(res.status).toBe(302); + const expected = new URLSearchParams({ + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + state: 'st4te', + resource: 'https://trek.example.com/mcp', + }); + expect(res.headers.location).toBe(`https://trek.example.com/oauth/consent?${expected.toString()}`); + }); + + it('AUTHZ-002 — omitted resource defaults to the MCP endpoint and still reaches consent', async () => { + const clientId = await registerClient(); + const { challenge } = makePkce(); + + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }); + + expect(res.status).toBe(302); + const loc = new URL(res.headers.location); + expect(`${loc.origin}${loc.pathname}`).toBe('https://trek.example.com/oauth/consent'); + expect(loc.searchParams.get('client_id')).toBe(clientId); + // No resource param was sent, so none is forwarded. + expect(loc.searchParams.get('resource')).toBeNull(); + expect(loc.searchParams.get('state')).toBeNull(); + }); + + it('AUTHZ-003 — wrong resource 302-redirects back to the client with invalid_target', async () => { + const clientId = await registerClient(); + const { challenge } = makePkce(); + + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + state: 'xyz', + code_challenge: challenge, + code_challenge_method: 'S256', + resource: 'https://evil.example.com/other', + }); + + expect(res.status).toBe(302); + const loc = new URL(res.headers.location); + expect(`${loc.origin}${loc.pathname}`).toBe('https://client.example.com/cb'); + expect(loc.searchParams.get('error')).toBe('invalid_target'); + expect(loc.searchParams.get('error_description')).toBe('Requested resource must be the TREK MCP endpoint'); + expect(loc.searchParams.get('state')).toBe('xyz'); + }); + + it('AUTHZ-004 — unknown client_id is a 400 invalid_client, no redirect', async () => { + const { challenge } = makePkce(); + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: 'does-not-exist', + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid_client'); + }); + + it('AUTHZ-005 — unregistered redirect_uri is a 400 invalid_request, no redirect', async () => { + const clientId = await registerClient(); + const { challenge } = makePkce(); + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://attacker.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid_request'); + }); + + it('AUTHZ-006 — missing code_challenge redirects back to the client with invalid_request', async () => { + const clientId = await registerClient(); + const res = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + }); + expect(res.status).toBe(302); + const loc = new URL(res.headers.location); + expect(`${loc.origin}${loc.pathname}`).toBe('https://client.example.com/cb'); + expect(loc.searchParams.get('error')).toBe('invalid_request'); + }); + + it('AUTHZ-007 — MCP addon off: authorize and register both 404 with empty bodies', async () => { + setMcpEnabled(false); + const authz = await request(app).get('/oauth/authorize').query({ client_id: 'x' }); + expect(authz.status).toBe(404); + expect(authz.text).toBe(''); + + const reg = await request(app) + .post('/oauth/register') + .set('Content-Type', 'application/json') + .send({ redirect_uris: ['https://client.example.com/cb'], token_endpoint_auth_method: 'none' }); + expect(reg.status).toBe(404); + expect(reg.text).toBe(''); + }); + + it('AUTHZ-008 — full loop: register → authorize → consent-written code → token exchange', async () => { + const { user } = createUser(testDb); + const clientId = await registerClient(); + const { verifier, challenge } = makePkce(); + + // 1. authorize forwards to consent + const authz = await request(app).get('/oauth/authorize').query({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + resource: 'https://trek.example.com/mcp', + }); + expect(authz.status).toBe(302); + + // 2. the consent controller writes the code through the container + // singleton; the module-scoped pending-code map is what lets the + // SDK-side exchange see it. + const code = containerSideOauth.createAuthCode({ + clientId, + userId: user.id, + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read'], + resource: 'https://trek.example.com/mcp', + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + expect(code).toBeTruthy(); + + // 3. token exchange through the public token endpoint + const token = await request(app).post('/oauth/token').send({ + grant_type: 'authorization_code', + client_id: clientId, + code, + redirect_uri: 'https://client.example.com/cb', + code_verifier: verifier, + resource: 'https://trek.example.com/mcp', + }); + expect(token.status).toBe(200); + expect(token.body.access_token).toMatch(/^trekoa_/); + expect(token.body.token_type).toBe('Bearer'); + expect(token.body.scope).toBe('trips:read'); + }); +}); diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index caf77224c..d04448c01 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -58,7 +58,6 @@ import { runMigrations } from '../../src/db/migrations'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createUser } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; -import { createOAuthClient, getUserByAccessToken } from '../../src/nest/oauth/oauth.bridge'; import { ALL_SCOPES } from '../../src/mcp/scopes'; import { OauthService } from '../../src/nest/oauth/oauth.service'; import { DatabaseService } from '../../src/nest/database/database.service'; @@ -66,14 +65,15 @@ import { AddonsService } from '../../src/nest/addons/addons.service'; import { AuditService } from '../../src/nest/audit/audit.service'; // In production the consent controller writes pending codes through the -// container instance and the SDK-mounted routes read them back through the -// bridge. createAuthCode is not bridged (only the read side is), so the tests -// write codes the same way the consent controller does: through a service -// instance. The pending-code map is module-scoped in oauth.pending-codes.ts, -// so the routes under test see every code written here. +// container instance and the SDK routes read them back through the same +// injected singleton. These tests write codes the way the consent controller +// does: through a service instance. The pending-code map is module-scoped in +// oauth.pending-codes.ts, so the routes under test see every code written here. const oauthDbs = new DatabaseService(testDb); const containerSideOauth = new OauthService(oauthDbs, new AddonsService(oauthDbs), new AuditService(oauthDbs)); const createAuthCode = containerSideOauth.createAuthCode.bind(containerSideOauth); +const createOAuthClient = containerSideOauth.createOAuthClient.bind(containerSideOauth); +const getUserByAccessToken = containerSideOauth.getUserByAccessToken.bind(containerSideOauth); let nestApp: INestApplication; let app: Application; diff --git a/server/tests/unit/nest/mcp-addon-gate.test.ts b/server/tests/unit/nest/mcp-addon-gate.test.ts new file mode 100644 index 000000000..8a9ae06aa --- /dev/null +++ b/server/tests/unit/nest/mcp-addon-gate.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect, vi } from 'vitest'; +import { createMcpAddonGate } from '../../../src/nest/addons/mcp-addon-gate'; +import { ADDON_IDS } from '../../../src/addons'; +import type { AddonsService } from '../../../src/nest/addons/addons.service'; + +function gate(enabled: boolean) { + const addons = { isAddonEnabled: vi.fn(() => enabled) }; + return { addons, mw: createMcpAddonGate(addons as unknown as AddonsService) }; +} + +function makeRes() { + const res = { + statusCode: 200, + ended: false, + status: vi.fn(function (this: typeof res, c: number) { this.statusCode = c; return this; }), + end: vi.fn(function (this: typeof res) { this.ended = true; return this; }), + }; + return res; +} + +describe('createMcpAddonGate', () => { + it('GATE-001: 404s with an empty body when the MCP addon is off', () => { + const { addons, mw } = gate(false); + const res = makeRes(); + const next = vi.fn(); + mw({} as never, res as never, next); + expect(addons.isAddonEnabled).toHaveBeenCalledWith(ADDON_IDS.MCP); + expect(res.statusCode).toBe(404); + expect(res.ended).toBe(true); + expect(next).not.toHaveBeenCalled(); + }); + + it('GATE-002: continues when the addon is on', () => { + const { mw } = gate(true); + const res = makeRes(); + const next = vi.fn(); + mw({} as never, res as never, next); + expect(res.statusCode).toBe(200); + expect(next).toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/nest/oauth-sdk.provider.test.ts b/server/tests/unit/nest/oauth-sdk.provider.test.ts new file mode 100644 index 000000000..c113e8340 --- /dev/null +++ b/server/tests/unit/nest/oauth-sdk.provider.test.ts @@ -0,0 +1,461 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const h = vi.hoisted(() => ({ + getMcpSafeUrl: vi.fn(() => 'https://trek.example.test'), +})); + +vi.mock('../../../src/app-config', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getMcpSafeUrl: h.getMcpSafeUrl }; +}); + +const sdk = vi.hoisted(() => ({ + authorizeHandler: vi.fn(), + registerHandler: vi.fn(), +})); +vi.mock('@modelcontextprotocol/sdk/server/auth/handlers/authorize', () => ({ + authorizationHandler: vi.fn(() => sdk.authorizeHandler), +})); +vi.mock('@modelcontextprotocol/sdk/server/auth/handlers/register', () => ({ + clientRegistrationHandler: vi.fn(() => sdk.registerHandler), +})); + +import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize'; +import { clientRegistrationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/register'; +import { InvalidClientMetadataError, ServerError } from '@modelcontextprotocol/sdk/server/auth/errors'; +import { TrekClientsStore, TrekOAuthProvider } from '../../../src/nest/oauth/oauth-sdk.provider'; +import { OauthModule } from '../../../src/nest/oauth/oauth.module'; +import type { AddonsService } from '../../../src/nest/addons/addons.service'; +import { ALL_SCOPES } from '../../../src/mcp/scopes'; +import type { OauthService } from '../../../src/nest/oauth/oauth.service'; +import type { AuditService } from '../../../src/nest/audit/audit.service'; +import type { OAuthClientInformationFull } from '@modelcontextprotocol/sdk/shared/auth'; + +function makeOauth(overrides: Partial> = {}) { + return { + getSdkClient: vi.fn(), + createOAuthClient: vi.fn(() => ({ client: baseClientRecord() })), + consumeAuthCode: vi.fn(), + issueTokens: vi.fn(() => tokens()), + refreshTokens: vi.fn(() => ({ tokens: tokens() })), + getUserByAccessToken: vi.fn(), + revokeToken: vi.fn(), + verifyPKCE: vi.fn(() => true), + ...overrides, + } as unknown as OauthService; +} + +function makeAudit() { + return { writeAudit: vi.fn() } as unknown as AuditService; +} + +function baseClientRecord() { + return { + client_id: 'cid-1', + name: 'Registered', + redirect_uris: ['https://client.example.com/cb'], + allowed_scopes: ['trips:read'], + } as Record; +} + +function tokens() { + return { access_token: 'trekoa_x', refresh_token: 'trekrt_x', token_type: 'Bearer' as const, expires_in: 3600, scope: 'trips:read' }; +} + +function clientInfo(over: Partial = {}): OAuthClientInformationFull { + return { + client_id: 'cid-1', + redirect_uris: ['https://client.example.com/cb'], + ...over, + } as OAuthClientInformationFull; +} + +function pending(over: Partial> = {}) { + return { + clientId: 'cid-1', + userId: 7, + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read'], + resource: 'https://trek.example.test/mcp', + codeChallenge: 'chal', + codeChallengeMethod: 'S256' as const, + expiresAt: Date.now() + 60_000, + ...over, + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + h.getMcpSafeUrl.mockReturnValue('https://trek.example.test'); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// TrekClientsStore +// ───────────────────────────────────────────────────────────────────────────── + +describe('TrekClientsStore.getClient', () => { + it('SDKP-001: maps a public client row to SDK client info', async () => { + const oauth = makeOauth({ + getSdkClient: vi.fn(() => ({ + client_id: 'cid-1', name: 'App', redirect_uris: '["https://a.example.com/cb"]', + allowed_scopes: '["trips:read","trips:write"]', is_public: 1, created_via: 'dcr', + })), + }); + const info = await new TrekClientsStore(oauth).getClient('cid-1'); + expect(info).toEqual({ + client_id: 'cid-1', + client_name: 'App', + redirect_uris: ['https://a.example.com/cb'], + scope: 'trips:read trips:write', + token_endpoint_auth_method: 'none', + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + }); + }); + + it('SDKP-002: a confidential client maps to client_secret_post', async () => { + const oauth = makeOauth({ + getSdkClient: vi.fn(() => ({ + client_id: 'cid-2', name: 'Secret', redirect_uris: '[]', allowed_scopes: '["trips:read"]', + is_public: 0, created_via: 'ui', + })), + }); + const info = await new TrekClientsStore(oauth).getClient('cid-2'); + expect(info!.token_endpoint_auth_method).toBe('client_secret_post'); + }); + + it('SDKP-003: unknown client resolves undefined', async () => { + const store = new TrekClientsStore(makeOauth({ getSdkClient: vi.fn(() => undefined) })); + await expect(store.getClient('nope')).resolves.toBeUndefined(); + }); +}); + +describe('TrekClientsStore.registerClient', () => { + const store = () => new TrekClientsStore(makeOauth()); + + it('SDKP-010: rejects a malformed redirect URI', async () => { + await expect(store().registerClient({ redirect_uris: ['not a url'] } as never)) + .rejects.toThrowError(new InvalidClientMetadataError('Invalid redirect URI: not a url')); + }); + + it('SDKP-011: rejects dangerous schemes', async () => { + await expect(store().registerClient({ redirect_uris: ['javascript:alert(1)'] } as never)) + .rejects.toThrowError(new InvalidClientMetadataError('Dangerous redirect URI scheme: javascript:alert(1)')); + }); + + it('SDKP-012: rejects plain-http non-loopback and dot-less custom schemes', async () => { + const msg = 'redirect_uris must be HTTPS, loopback HTTP, or a private custom scheme'; + await expect(store().registerClient({ redirect_uris: ['http://evil.example.com/cb'] } as never)) + .rejects.toThrowError(new InvalidClientMetadataError(msg)); + await expect(store().registerClient({ redirect_uris: ['myapp://cb'] } as never)) + .rejects.toThrowError(new InvalidClientMetadataError(msg)); + }); + + it('SDKP-013: accepts https, loopback http, and reverse-DNS custom schemes', async () => { + for (const uri of [ + 'https://a.example.com/cb', + 'http://localhost:8080/cb', + 'http://127.0.0.1/cb', + 'http://[::1]/cb', + 'com.example.app:/oauth', + ]) { + const oauth = makeOauth(); + await new TrekClientsStore(oauth).registerClient({ + redirect_uris: [uri], token_endpoint_auth_method: 'none', scope: 'trips:read', + } as never); + expect(oauth.createOAuthClient).toHaveBeenCalled(); + } + }); + + it('SDKP-014: blank/missing client_name defaults to MCP Client; long names are clipped', async () => { + const oauth = makeOauth(); + const s = new TrekClientsStore(oauth); + await s.registerClient({ redirect_uris: ['https://a.example.com/cb'], scope: 'trips:read' } as never); + expect(vi.mocked(oauth.createOAuthClient).mock.calls[0][1]).toBe('MCP Client'); + + await s.registerClient({ + redirect_uris: ['https://a.example.com/cb'], scope: 'trips:read', client_name: ` ${'x'.repeat(150)} `, + } as never); + expect(vi.mocked(oauth.createOAuthClient).mock.calls[1][1]).toBe('x'.repeat(100)); + }); + + it('SDKP-015: absent scope defaults to every scope (consent still narrows later)', async () => { + const oauth = makeOauth(); + await new TrekClientsStore(oauth).registerClient({ + redirect_uris: ['https://a.example.com/cb'], token_endpoint_auth_method: 'none', + } as never); + expect(vi.mocked(oauth.createOAuthClient).mock.calls[0][3]).toEqual(ALL_SCOPES); + }); + + it('SDKP-016: unknown scopes are filtered; nothing valid left is an error', async () => { + const oauth = makeOauth(); + await new TrekClientsStore(oauth).registerClient({ + redirect_uris: ['https://a.example.com/cb'], scope: 'trips:read bogus:scope', + } as never); + expect(vi.mocked(oauth.createOAuthClient).mock.calls[0][3]).toEqual(['trips:read']); + + await expect(new TrekClientsStore(makeOauth()).registerClient({ + redirect_uris: ['https://a.example.com/cb'], scope: 'bogus:scope', + } as never)).rejects.toThrowError(new InvalidClientMetadataError('No valid scopes requested')); + }); + + it('SDKP-017: a service-side rejection surfaces as InvalidClientMetadataError', async () => { + const oauth = makeOauth({ createOAuthClient: vi.fn(() => ({ error: 'Maximum 10 redirect URIs per client', status: 400 })) }); + await expect(new TrekClientsStore(oauth).registerClient({ + redirect_uris: ['https://a.example.com/cb'], scope: 'trips:read', + } as never)).rejects.toThrowError(new InvalidClientMetadataError('Maximum 10 redirect URIs per client')); + }); + + it('SDKP-018: response shape — public client has no secret, confidential carries a never-expiring one', async () => { + const pub = await new TrekClientsStore(makeOauth()).registerClient({ + redirect_uris: ['https://a.example.com/cb'], token_endpoint_auth_method: 'none', scope: 'trips:read', + } as never); + expect(pub.token_endpoint_auth_method).toBe('none'); + expect(pub.client_secret).toBeUndefined(); + + const oauth = makeOauth({ + createOAuthClient: vi.fn(() => ({ client: { ...baseClientRecord(), client_secret: 'sec-1' } })), + }); + const conf = await new TrekClientsStore(oauth).registerClient({ + redirect_uris: ['https://a.example.com/cb'], scope: 'trips:read', + } as never); + expect(conf.token_endpoint_auth_method).toBe('client_secret_post'); + expect(conf.client_secret).toBe('sec-1'); + expect(conf.client_secret_expires_at).toBe(0); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// TrekOAuthProvider +// ───────────────────────────────────────────────────────────────────────────── + +function makeProvider(oauth = makeOauth(), audit = makeAudit()) { + const clients = new TrekClientsStore(oauth); + return { provider: new TrekOAuthProvider(clients, oauth, audit), clients, oauth, audit }; +} + +function makeRes() { + return { redirect: vi.fn() }; +} + +describe('TrekOAuthProvider basics', () => { + it('SDKP-020: exposes the injected clients store and skips local PKCE', () => { + const { provider, clients } = makeProvider(); + expect(provider.clientsStore).toBe(clients); + expect(provider.skipLocalPkceValidation).toBe(true); + }); + + it('SDKP-021: challengeForAuthorizationCode is unreachable by design', async () => { + const { provider } = makeProvider(); + await expect(provider.challengeForAuthorizationCode(clientInfo(), 'code')) + .rejects.toThrowError(new ServerError('PKCE validation is handled by the provider directly')); + }); +}); + +describe('TrekOAuthProvider.authorize', () => { + it('SDKP-030: forwards the exact params to the SPA consent page', async () => { + const { provider } = makeProvider(); + const res = makeRes(); + await provider.authorize(clientInfo(), { + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read', 'trips:write'], + codeChallenge: 'chal', + state: 'st', + resource: new URL('https://trek.example.test/mcp'), + } as never, res as never); + const qs = new URLSearchParams({ + client_id: 'cid-1', + redirect_uri: 'https://client.example.com/cb', + scope: 'trips:read trips:write', + code_challenge: 'chal', + code_challenge_method: 'S256', + state: 'st', + resource: 'https://trek.example.test/mcp', + }); + expect(res.redirect).toHaveBeenCalledWith(302, `https://trek.example.test/oauth/consent?${qs.toString()}`); + }); + + it('SDKP-031: omits state/resource params when the request carried none', async () => { + const { provider } = makeProvider(); + const res = makeRes(); + await provider.authorize(clientInfo(), { + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read'], + codeChallenge: 'chal', + } as never, res as never); + const target = vi.mocked(res.redirect).mock.calls[0][1] as string; + const url = new URL(target); + expect(url.searchParams.get('state')).toBeNull(); + expect(url.searchParams.get('resource')).toBeNull(); + }); + + it('SDKP-032: a foreign resource bounces back to the client with invalid_target (+state)', async () => { + const { provider } = makeProvider(); + const res = makeRes(); + await provider.authorize(clientInfo(), { + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read'], + codeChallenge: 'chal', + state: 'keep-me', + resource: new URL('https://other.example.com/api'), + } as never, res as never); + const target = new URL(vi.mocked(res.redirect).mock.calls[0][1] as string); + expect(`${target.origin}${target.pathname}`).toBe('https://client.example.com/cb'); + expect(target.searchParams.get('error')).toBe('invalid_target'); + expect(target.searchParams.get('error_description')).toBe('Requested resource must be the TREK MCP endpoint'); + expect(target.searchParams.get('state')).toBe('keep-me'); + }); + + it('SDKP-033: the invalid_target bounce omits state when there was none', async () => { + const { provider } = makeProvider(); + const res = makeRes(); + await provider.authorize(clientInfo(), { + redirectUri: 'https://client.example.com/cb', + scopes: ['trips:read'], + codeChallenge: 'chal', + resource: new URL('https://other.example.com/api'), + } as never, res as never); + const target = new URL(vi.mocked(res.redirect).mock.calls[0][1] as string); + expect(target.searchParams.get('state')).toBeNull(); + }); +}); + +describe('TrekOAuthProvider.exchangeAuthorizationCode', () => { + const invalid = 'Authorization grant is invalid.'; + + it('SDKP-040: unknown/expired code throws the uniform grant error', async () => { + const { provider } = makeProvider(makeOauth({ consumeAuthCode: vi.fn(() => null) })); + await expect(provider.exchangeAuthorizationCode(clientInfo(), 'code')).rejects.toThrow(invalid); + }); + + it('SDKP-041: a code minted for another client throws the same error', async () => { + const { provider } = makeProvider(makeOauth({ consumeAuthCode: vi.fn(() => pending({ clientId: 'someone-else' })) })); + await expect(provider.exchangeAuthorizationCode(clientInfo(), 'code')).rejects.toThrow(invalid); + }); + + it('SDKP-042: redirect_uri mismatch throws the same error', async () => { + const { provider } = makeProvider(makeOauth({ consumeAuthCode: vi.fn(() => pending()) })); + await expect(provider.exchangeAuthorizationCode(clientInfo(), 'code', undefined, 'https://wrong.example.com/cb')) + .rejects.toThrow(invalid); + }); + + it('SDKP-043: resource mismatch throws the same error', async () => { + const { provider } = makeProvider(makeOauth({ consumeAuthCode: vi.fn(() => pending()) })); + await expect(provider.exchangeAuthorizationCode( + clientInfo(), 'code', undefined, 'https://client.example.com/cb', new URL('https://other.example.com/'), + )).rejects.toThrow(invalid); + }); + + it('SDKP-044: failed PKCE throws the same error', async () => { + const oauth = makeOauth({ consumeAuthCode: vi.fn(() => pending()), verifyPKCE: vi.fn(() => false) }); + const { provider } = makeProvider(oauth); + await expect(provider.exchangeAuthorizationCode(clientInfo(), 'code', 'bad-verifier', 'https://client.example.com/cb')) + .rejects.toThrow(invalid); + expect(oauth.verifyPKCE).toHaveBeenCalledWith('bad-verifier', 'chal'); + }); + + it('SDKP-045: a valid exchange issues tokens and audits the issue', async () => { + const oauth = makeOauth({ consumeAuthCode: vi.fn(() => pending()) }); + const audit = makeAudit(); + const { provider } = makeProvider(oauth, audit); + const result = await provider.exchangeAuthorizationCode( + clientInfo(), 'code', 'verifier', 'https://client.example.com/cb', new URL('https://trek.example.test/mcp'), + ); + expect(result).toEqual(tokens()); + expect(oauth.issueTokens).toHaveBeenCalledWith('cid-1', 7, ['trips:read'], null, 'https://trek.example.test/mcp'); + expect(audit.writeAudit).toHaveBeenCalledWith({ + userId: 7, + action: 'oauth.token.issue', + details: { client_id: 'cid-1', scopes: ['trips:read'], audience: 'https://trek.example.test/mcp' }, + ip: null, + }); + }); + + it('SDKP-046: a pending code without a resource issues a null-audience token', async () => { + const oauth = makeOauth({ consumeAuthCode: vi.fn(() => pending({ resource: null })) }); + const { provider } = makeProvider(oauth); + await provider.exchangeAuthorizationCode(clientInfo(), 'code'); + expect(oauth.issueTokens).toHaveBeenCalledWith('cid-1', 7, ['trips:read'], null, null); + // No codeVerifier was supplied, so PKCE verification is skipped entirely. + expect(oauth.verifyPKCE).not.toHaveBeenCalled(); + }); +}); + +describe('TrekOAuthProvider.exchangeRefreshToken', () => { + it('SDKP-050: maps invalid_client and generic failures to their legacy messages', async () => { + const bad = makeProvider(makeOauth({ refreshTokens: vi.fn(() => ({ error: 'invalid_client' })) })); + await expect(bad.provider.exchangeRefreshToken(clientInfo(), 'rt')).rejects.toThrow('Invalid client credentials'); + + const expired = makeProvider(makeOauth({ refreshTokens: vi.fn(() => ({ error: 'invalid_grant' })) })); + await expect(expired.provider.exchangeRefreshToken(clientInfo(), 'rt')).rejects.toThrow('Refresh token is invalid or expired'); + }); + + it('SDKP-051: success returns the rotated tokens with the client secret forwarded', async () => { + const oauth = makeOauth(); + const { provider } = makeProvider(oauth); + const result = await provider.exchangeRefreshToken(clientInfo({ client_secret: 'sec' }), 'rt'); + expect(result).toEqual(tokens()); + expect(oauth.refreshTokens).toHaveBeenCalledWith('rt', 'cid-1', 'sec', null); + }); +}); + +describe('TrekOAuthProvider.verifyAccessToken / revokeToken', () => { + it('SDKP-060: an unknown token throws; a live one maps to AuthInfo', async () => { + const dead = makeProvider(makeOauth({ getUserByAccessToken: vi.fn(() => null) })); + await expect(dead.provider.verifyAccessToken('trekoa_dead')).rejects.toThrow('Invalid or expired token'); + + const info = { + user: { id: 7, username: 'u', email: 'u@example.com', role: 'user' as const }, + scopes: ['trips:read'], + clientId: 'cid-1', + audience: null, + }; + const live = makeProvider(makeOauth({ getUserByAccessToken: vi.fn(() => info) })); + await expect(live.provider.verifyAccessToken('trekoa_live')).resolves.toEqual({ + token: 'trekoa_live', + clientId: 'cid-1', + scopes: ['trips:read'], + extra: { user: info.user }, + }); + }); + + it('SDKP-061: revokeToken delegates with the caller client id and a null ip', async () => { + const oauth = makeOauth(); + const { provider } = makeProvider(oauth); + await provider.revokeToken(clientInfo(), { token: 'trekoa_x' }); + expect(oauth.revokeToken).toHaveBeenCalledWith('trekoa_x', 'cid-1', undefined, null); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// OauthModule.configure — the SDK routers behind the addon gate +// ───────────────────────────────────────────────────────────────────────────── + +describe('OauthModule.configure', () => { + it('SDKP-070: mounts gate + SDK handlers on their prefixes over the injected adapters', () => { + const { provider, clients } = makeProvider(); + const addons = { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService; + const applied: Array<{ handlers: unknown[]; route: string }> = []; + const consumer = { + apply: vi.fn((...handlers: unknown[]) => ({ + forRoutes: (route: string) => applied.push({ handlers, route }), + })), + }; + + new OauthModule(addons, provider, clients).configure(consumer as never); + + expect(vi.mocked(authorizationHandler)).toHaveBeenCalledWith({ provider }); + expect(vi.mocked(clientRegistrationHandler)).toHaveBeenCalledWith({ clientsStore: clients }); + expect(applied).toHaveLength(2); + const [authz, reg] = applied; + expect(authz.route).toBe('oauth/authorize'); + expect(authz.handlers[1]).toBe(sdk.authorizeHandler); + expect(reg.route).toBe('oauth/register'); + expect(reg.handlers[1]).toBe(sdk.registerHandler); + // Both mounts share one gate instance, built over the injected AddonsService. + expect(authz.handlers[0]).toBe(reg.handlers[0]); + const res = { status: vi.fn(() => ({ end: vi.fn() })), end: vi.fn() }; + (authz.handlers[0] as (rq: unknown, rs: unknown, nx: () => void) => void)({}, res, vi.fn()); + expect(addons.isAddonEnabled).toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/nest/oauth.service.test.ts b/server/tests/unit/nest/oauth.service.test.ts index c91cbc7ce..5c579a951 100644 --- a/server/tests/unit/nest/oauth.service.test.ts +++ b/server/tests/unit/nest/oauth.service.test.ts @@ -1111,22 +1111,24 @@ describe('branches the legacy suite could not reach', () => { }); describe('oauth.bridge delegation', () => { - // The bridge is what the SDK-mounted /oauth/authorize path and the MCP - // transport reach, both outside the container. - it('issues and verifies a token through the bridge instance', () => { + // The bridge is down to its one surviving export — what the MCP transport's + // token verification reaches outside the container. The SDK provider paths + // that used to be bridged live behind the container now (oauth-sdk.provider). + it('verifies a container-issued token through the bridge instance', () => { const { user } = createUser(testDb); const created = makeClient(user.id); const clientId = (created.client as { client_id: string }).client_id; - const tokens = bridge.issueTokens(clientId, user.id, ['trips:read']); + const tokens = issueTokens(clientId, user.id, ['trips:read']); expect(bridge.getUserByAccessToken(tokens.access_token)?.user.id).toBe(user.id); }); - it('shares the pending-code map with the container instance', () => { + it('shares the pending-code map across service instances', () => { // The load-bearing invariant: the consent controller writes the code through - // the DI singleton, the SDK provider reads it back through the bridge. Two - // maps here would kill the authorization-code flow silently. + // the DI singleton, the SDK exchange path reads it back. The map is module- + // scoped, so even a second hand-built instance must see it — two maps would + // kill the authorization-code flow silently. const code = createAuthCode({ clientId: 'c', userId: 42, @@ -1137,32 +1139,8 @@ describe('oauth.bridge delegation', () => { codeChallengeMethod: 'S256', })!; - expect(bridge.consumeAuthCode(code)?.userId).toBe(42); - }); - - it('verifyPKCE and revokeToken reach the same rows', () => { - const { verifier, challenge } = makePkce(); - expect(bridge.verifyPKCE(verifier, challenge)).toBe(true); - - const { user } = createUser(testDb); - const created = makeClient(user.id); - const clientId = (created.client as { client_id: string }).client_id; - const tokens = issueTokens(clientId, user.id, ['trips:read']); - - bridge.revokeToken(tokens.access_token, clientId, user.id); - - expect(getUserByAccessToken(tokens.access_token)).toBeNull(); - }); - - it('registers a client and refreshes a token pair through the bridge', () => { - const { user } = createUser(testDb); - const created = bridge.createOAuthClient(user.id, 'Via bridge', ['https://example.com/callback'], ['trips:read']); - const client = created.client as { client_id: string; client_secret: string }; - const tokens = issueTokens(client.client_id, user.id, ['trips:read']); - - const refreshed = bridge.refreshTokens(tokens.refresh_token, client.client_id, client.client_secret); - - expect(refreshed.tokens?.refresh_token).toMatch(/^trekrf_/); + const secondInstance = new OauthService(dbs, addonsStub, new AuditService(dbs)); + expect(secondInstance.consumeAuthCode(code)?.userId).toBe(42); }); }); @@ -1173,12 +1151,14 @@ describe('OauthModule', () => { const { OauthApiController } = await import('../../../src/nest/oauth/oauth-api.controller'); const { OauthService: Svc } = await import('../../../src/nest/oauth/oauth.service'); + const { TrekClientsStore, TrekOAuthProvider } = await import('../../../src/nest/oauth/oauth-sdk.provider'); + const controllers = Reflect.getMetadata('controllers', OauthModule); const providers = Reflect.getMetadata('providers', OauthModule); expect(controllers).toEqual([OauthPublicController, OauthApiController]); // RateLimitService is deliberately absent: it comes from the global // RateLimitModule so all consumers share one set of counters. - expect(providers).toEqual([Svc]); + expect(providers).toEqual([Svc, TrekClientsStore, TrekOAuthProvider]); }); }); diff --git a/server/tests/unit/nest/platform.controller.test.ts b/server/tests/unit/nest/platform.controller.test.ts index 44684a0aa..ad947414c 100644 --- a/server/tests/unit/nest/platform.controller.test.ts +++ b/server/tests/unit/nest/platform.controller.test.ts @@ -4,30 +4,15 @@ import { NotFoundException } from '@nestjs/common'; // --- hoisted mock fns so the vi.mock factories can reference them ----------------- const h = vi.hoisted(() => ({ verifyJwtAndLoadUser: vi.fn(), - isAddonEnabled: vi.fn(), dbPrepare: vi.fn(), existsSync: vi.fn(), - // SDK middleware spies — each returns a tagged handler so we can identify which - // app.use call received it. - authorizeHandler: vi.fn(), - registerHandler: vi.fn(), + // Tagged handler so we can identify which route registration received it. mcpHandler: vi.fn(), })); vi.mock('../../../src/nest/auth/jwt-verify', () => ({ verifyJwtAndLoadUser: h.verifyJwtAndLoadUser })); vi.mock('../../../src/db/database', () => ({ db: { prepare: h.dbPrepare } })); vi.mock('../../../src/mcp', () => ({ mcpHandler: h.mcpHandler })); -vi.mock('../../../src/mcp/oauthProvider', () => ({ trekOAuthProvider: {}, trekClientsStore: {} })); -vi.mock('../../../src/nest/addons/addons.bridge', () => ({ isAddonEnabled: h.isAddonEnabled })); - -// SDK handler factories return distinct tagged middleware so we never hit real -// wiring during registration. -vi.mock('@modelcontextprotocol/sdk/server/auth/handlers/authorize', () => ({ - authorizationHandler: vi.fn(() => h.authorizeHandler), -})); -vi.mock('@modelcontextprotocol/sdk/server/auth/handlers/register', () => ({ - clientRegistrationHandler: vi.fn(() => h.registerHandler), -})); vi.mock('node:fs', async (orig) => { const real = (await orig()) as Record; @@ -224,38 +209,6 @@ describe('applyPlatformTransport', () => { return calls; } - describe('mcpAddonGate (used on /oauth/authorize + /oauth/register)', () => { - function gate() { - // The gate is the first handler on the /oauth/authorize use registration. - return build().find((c) => c.method === 'use' && c.path === '/oauth/authorize')!.handlers[0]; - } - - it('404 when MCP is disabled', () => { - h.isAddonEnabled.mockReturnValue(false); - const res = makeRes(); - const next = vi.fn(); - gate()({}, res, next); - expect(res.statusCode).toBe(404); - expect(next).not.toHaveBeenCalled(); - }); - - it('calls next() when MCP is enabled', () => { - h.isAddonEnabled.mockReturnValue(true); - const res = makeRes(); - const next = vi.fn(); - gate()({}, res, next); - expect(next).toHaveBeenCalled(); - }); - }); - - it('wires the SDK authorize + register handlers behind the gate', () => { - const calls = build(); - const authorize = calls.find((c) => c.path === '/oauth/authorize')!; - const register = calls.find((c) => c.path === '/oauth/register')!; - expect(authorize.handlers).toContain(h.authorizeHandler); - expect(register.handlers).toContain(h.registerHandler); - }); - it('mounts the MCP handler on POST/GET/DELETE /mcp', () => { const calls = build(); expect(calls.find((c) => c.method === 'post' && c.path === '/mcp')!.handlers[0]).toBe(h.mcpHandler);