diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 366040cd..60ce4bf9 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -926,6 +926,13 @@ function runMigrations(db: Database.Database): void { CREATE UNIQUE INDEX IF NOT EXISTS idx_oauth_tokens_refresh ON oauth_tokens(refresh_token_hash); `); }, + // Migration: Refresh-token rotation chain tracking for replay detection + () => { + db.exec(` + ALTER TABLE oauth_tokens ADD COLUMN parent_token_id INTEGER REFERENCES oauth_tokens(id); + CREATE INDEX IF NOT EXISTS idx_oauth_tokens_parent ON oauth_tokens(parent_token_id); + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/mcp/index.ts b/server/src/mcp/index.ts index 27bd9057..167e4c15 100644 --- a/server/src/mcp/index.ts +++ b/server/src/mcp/index.ts @@ -9,19 +9,9 @@ import { isAddonEnabled } from '../services/adminService'; import { ADDON_IDS } from '../addons'; import { registerResources } from './resources'; import { registerTools } from './tools'; +import { McpSession, sessions, revokeUserSessions, revokeUserSessionsForClient } from './sessionManager'; -interface McpSession { - server: McpServer; - transport: StreamableHTTPServerTransport; - userId: number; - /** null = static trek_ token or JWT (full access); string[] = OAuth 2.1 scopes */ - scopes: string[] | null; - /** true when authenticated via static trek_ token — triggers deprecation prompt */ - isStaticToken: boolean; - lastActivity: number; -} - -const sessions = new Map(); +export { revokeUserSessions, revokeUserSessionsForClient }; const SESSION_TTL_MS = 60 * 60 * 1000; // 1 hour const sessionParsed = Number.parseInt(process.env.MCP_MAX_SESSION_PER_USER ?? ""); @@ -83,31 +73,38 @@ interface VerifyTokenResult { user: User; /** null = full access (static token or JWT); string[] = OAuth 2.1 scoped access */ scopes: string[] | null; + /** OAuth client_id when authenticated via OAuth 2.1; null otherwise */ + clientId: string | null; isStaticToken: boolean; } function verifyToken(authHeader: string | undefined): VerifyTokenResult | null { - const token = authHeader && authHeader.split(' ')[1]; - if (!token) return null; + if (!authHeader) return null; + // M8: strictly require "Bearer" scheme (RFC 6750) + const spaceIdx = authHeader.indexOf(' '); + if (spaceIdx === -1) return null; + const scheme = authHeader.slice(0, spaceIdx); + const token = authHeader.slice(spaceIdx + 1); + if (scheme.toLowerCase() !== 'bearer' || !token) return null; // OAuth 2.1 access token (trekoa_...) if (token.startsWith('trekoa_')) { const result = getUserByAccessToken(token); if (!result) return null; - return { user: result.user, scopes: result.scopes, isStaticToken: false }; + return { user: result.user, scopes: result.scopes, clientId: result.clientId, isStaticToken: false }; } // Long-lived static MCP token (trek_...) — full access + deprecation notice if (token.startsWith('trek_')) { const user = verifyMcpToken(token); if (!user) return null; - return { user, scopes: null, isStaticToken: true }; + return { user, scopes: null, clientId: null, isStaticToken: true }; } // Short-lived JWT (TREK web session used directly) — full access, no notice const user = verifyJwtToken(token); if (!user) return null; - return { user, scopes: null, isStaticToken: false }; + return { user, scopes: null, clientId: null, isStaticToken: false }; } export async function mcpHandler(req: Request, res: Response): Promise { @@ -121,7 +118,7 @@ export async function mcpHandler(req: Request, res: Response): Promise { res.status(401).json({ error: 'Access token required' }); return; } - const { user, scopes, isStaticToken } = tokenResult; + const { user, scopes, clientId, isStaticToken } = tokenResult; if (isRateLimited(user.id)) { res.status(429).json({ error: 'Too many requests. Please slow down.' }); @@ -174,13 +171,13 @@ export async function mcpHandler(req: Request, res: Response): Promise { prompts: { listChanged: true }, }, }); - registerResources(server, user.id); + registerResources(server, user.id, scopes); registerTools(server, user.id, scopes, isStaticToken); const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), onsessioninitialized: (sid) => { - sessions.set(sid, { server, transport, userId: user.id, scopes, isStaticToken, lastActivity: Date.now() }); + sessions.set(sid, { server, transport, userId: user.id, scopes, clientId, isStaticToken, lastActivity: Date.now() }); const authMethod = isStaticToken ? 'static-token' : scopes ? `oauth(${scopes.join(',')})` : 'jwt'; console.log(`[MCP] Session ${sid} created for user ${user.id} [${authMethod}]. Active sessions: ${sessions.size}`); }, @@ -200,17 +197,6 @@ export async function mcpHandler(req: Request, res: Response): Promise { } } -/** Terminate all active MCP sessions for a specific user (e.g. on token revocation). */ -export function revokeUserSessions(userId: number): void { - for (const [sid, session] of sessions) { - if (session.userId === userId) { - try { session.server.close(); } catch { /* ignore */ } - try { session.transport.close(); } catch { /* ignore */ } - sessions.delete(sid); - } - } -} - /** Close all active MCP sessions (call during graceful shutdown). */ export function closeMcpSessions(): void { clearInterval(sessionSweepInterval); diff --git a/server/src/mcp/resources.ts b/server/src/mcp/resources.ts index 384cbcd6..7330eeca 100644 --- a/server/src/mcp/resources.ts +++ b/server/src/mcp/resources.ts @@ -15,6 +15,7 @@ import { getNotifications } from '../services/inAppNotifications'; import { getActivePlanId, getActivePlan, getPlanData, getEntries as getVacayEntries, getHolidays } from '../services/vacayService'; import { isAddonEnabled } from '../services/adminService'; import { ADDON_IDS } from '../addons'; +import { canRead, canReadTrips } from './scopes'; function parseId(value: string | string[]): number | null { const n = Number(Array.isArray(value) ? value[0] : value); @@ -31,6 +32,16 @@ function accessDenied(uri: string) { }; } +function scopeDenied(uri: string) { + return { + contents: [{ + uri, + mimeType: 'application/json', + text: JSON.stringify({ error: 'Insufficient OAuth scope to access this resource' }), + }], + }; +} + function jsonContent(uri: string, data: unknown) { return { contents: [{ @@ -41,9 +52,9 @@ function jsonContent(uri: string, data: unknown) { }; } -export function registerResources(server: McpServer, userId: number): void { +export function registerResources(server: McpServer, userId: number, scopes: string[] | null): void { // List all accessible trips - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'trips', 'trek://trips', { description: 'All trips the user owns or is a member of', mimeType: 'application/json' }, @@ -54,7 +65,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Single trip detail - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'trip', new ResourceTemplate('trek://trips/{tripId}', { list: undefined }), { description: 'A single trip with metadata and member count', mimeType: 'application/json' }, @@ -67,7 +78,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Days with assigned places - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'trip-days', new ResourceTemplate('trek://trips/{tripId}/days', { list: undefined }), { description: 'Days of a trip with their assigned places', mimeType: 'application/json' }, @@ -81,7 +92,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Places in a trip - server.registerResource( + if (canRead(scopes, 'places')) server.registerResource( 'trip-places', new ResourceTemplate('trek://trips/{tripId}/places', { list: undefined }), { description: 'All places/POIs in a trip, optionally filtered by assignment status (e.g. ?assignment=unassigned)', mimeType: 'application/json' }, @@ -95,7 +106,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Budget items - if (isAddonEnabled(ADDON_IDS.BUDGET)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.BUDGET) && canRead(scopes, 'budget')) server.registerResource( 'trip-budget', new ResourceTemplate('trek://trips/{tripId}/budget', { list: undefined }), { description: 'Budget and expense items for a trip', mimeType: 'application/json' }, @@ -108,7 +119,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Packing checklist - if (isAddonEnabled(ADDON_IDS.PACKING)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.PACKING) && canRead(scopes, 'packing')) server.registerResource( 'trip-packing', new ResourceTemplate('trek://trips/{tripId}/packing', { list: undefined }), { description: 'Packing checklist for a trip', mimeType: 'application/json' }, @@ -121,7 +132,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Reservations (flights, hotels, restaurants) - server.registerResource( + if (canRead(scopes, 'reservations')) server.registerResource( 'trip-reservations', new ResourceTemplate('trek://trips/{tripId}/reservations', { list: undefined }), { description: 'Reservations (flights, hotels, restaurants) for a trip', mimeType: 'application/json' }, @@ -134,7 +145,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Day notes - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'day-notes', new ResourceTemplate('trek://trips/{tripId}/days/{dayId}/notes', { list: undefined }), { description: 'Notes for a specific day in a trip', mimeType: 'application/json' }, @@ -148,7 +159,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Accommodations (hotels, rentals) per trip - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'trip-accommodations', new ResourceTemplate('trek://trips/{tripId}/accommodations', { list: undefined }), { description: 'Accommodations (hotels, rentals) for a trip with check-in/out details', mimeType: 'application/json' }, @@ -161,7 +172,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Trip members (owner + collaborators) - server.registerResource( + if (canReadTrips(scopes)) server.registerResource( 'trip-members', new ResourceTemplate('trek://trips/{tripId}/members', { list: undefined }), { description: 'Owner and collaborators of a trip', mimeType: 'application/json' }, @@ -176,7 +187,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Collab notes for a trip - if (isAddonEnabled(ADDON_IDS.COLLAB)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.COLLAB) && canRead(scopes, 'collab')) server.registerResource( 'trip-collab-notes', new ResourceTemplate('trek://trips/{tripId}/collab-notes', { list: undefined }), { description: 'Shared collaborative notes for a trip', mimeType: 'application/json' }, @@ -189,7 +200,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Trip to-do list - if (isAddonEnabled(ADDON_IDS.PACKING)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.PACKING) && canRead(scopes, 'collab')) server.registerResource( 'trip-todos', new ResourceTemplate('trek://trips/{tripId}/todos', { list: undefined }), { description: 'To-do items for a trip, ordered by position', mimeType: 'application/json' }, @@ -201,7 +212,7 @@ export function registerResources(server: McpServer, userId: number): void { } ); - // All place categories (global, no trip filter) + // All place categories (global, no trip filter) — safe for any authenticated session server.registerResource( 'categories', 'trek://categories', @@ -213,7 +224,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // User's bucket list - if (isAddonEnabled(ADDON_IDS.ATLAS)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.ATLAS) && canRead(scopes, 'places')) server.registerResource( 'bucket-list', 'trek://bucket-list', { description: 'Your personal travel bucket list', mimeType: 'application/json' }, @@ -224,7 +235,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // User's visited countries - if (isAddonEnabled(ADDON_IDS.ATLAS)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.ATLAS) && canRead(scopes, 'places')) server.registerResource( 'visited-countries', 'trek://visited-countries', { description: 'Countries you have marked as visited in Atlas', mimeType: 'application/json' }, @@ -235,7 +246,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Budget per-person summary - if (isAddonEnabled(ADDON_IDS.BUDGET)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.BUDGET) && canRead(scopes, 'budget')) server.registerResource( 'trip-budget-per-person', new ResourceTemplate('trek://trips/{tripId}/budget/per-person', { list: undefined }), { description: 'Per-person budget summary for a trip (total spent per member, split breakdown)', mimeType: 'application/json' }, @@ -248,7 +259,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Budget settlement - if (isAddonEnabled(ADDON_IDS.BUDGET)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.BUDGET) && canRead(scopes, 'budget')) server.registerResource( 'trip-budget-settlement', new ResourceTemplate('trek://trips/{tripId}/budget/settlement', { list: undefined }), { description: 'Suggested settlement transactions to balance who owes whom', mimeType: 'application/json' }, @@ -261,7 +272,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Packing bags - if (isAddonEnabled(ADDON_IDS.PACKING)) server.registerResource( + if (isAddonEnabled(ADDON_IDS.PACKING) && canRead(scopes, 'packing')) server.registerResource( 'trip-packing-bags', new ResourceTemplate('trek://trips/{tripId}/packing/bags', { list: undefined }), { description: 'All packing bags for a trip with their members', mimeType: 'application/json' }, @@ -274,7 +285,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // In-app notifications - server.registerResource( + if (canRead(scopes, 'notifications')) server.registerResource( 'notifications-in-app', 'trek://notifications/in-app', { description: "The current user's in-app notifications (most recent 50, unread first)", mimeType: 'application/json' }, @@ -285,7 +296,7 @@ export function registerResources(server: McpServer, userId: number): void { ); // Atlas stats and regions (addon-gated) - if (isAddonEnabled(ADDON_IDS.ATLAS)) { + if (isAddonEnabled(ADDON_IDS.ATLAS) && canRead(scopes, 'places')) { server.registerResource( 'atlas-stats', 'trek://atlas/stats', @@ -308,7 +319,7 @@ export function registerResources(server: McpServer, userId: number): void { } // Collab polls & messages (addon-gated) - if (isAddonEnabled(ADDON_IDS.COLLAB)) { + if (isAddonEnabled(ADDON_IDS.COLLAB) && canRead(scopes, 'collab')) { server.registerResource( 'trip-collab-polls', new ResourceTemplate('trek://trips/{tripId}/collab/polls', { list: undefined }), @@ -335,7 +346,7 @@ export function registerResources(server: McpServer, userId: number): void { } // Vacay resources (addon-gated) - if (isAddonEnabled(ADDON_IDS.VACAY)) { + if (isAddonEnabled(ADDON_IDS.VACAY) && canRead(scopes, 'vacay')) { server.registerResource( 'vacay-plan', 'trek://vacay/plan', diff --git a/server/src/mcp/sessionManager.ts b/server/src/mcp/sessionManager.ts new file mode 100644 index 00000000..276a1573 --- /dev/null +++ b/server/src/mcp/sessionManager.ts @@ -0,0 +1,41 @@ +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; +import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp'; + +export interface McpSession { + server: McpServer; + transport: StreamableHTTPServerTransport; + userId: number; + /** null = static trek_ token or JWT (full access); string[] = OAuth 2.1 scopes */ + scopes: string[] | null; + /** OAuth 2.1 client_id that owns this session; null for static-token / JWT sessions */ + clientId: string | null; + /** true when authenticated via static trek_ token — triggers deprecation prompt */ + isStaticToken: boolean; + lastActivity: number; +} + +export const sessions = new Map(); + +/** Terminate all active MCP sessions for a specific user (e.g. on token revocation). */ +export function revokeUserSessions(userId: number): void { + for (const [sid, session] of sessions) { + if (session.userId === userId) { + try { session.server.close(); } catch { /* ignore */ } + try { session.transport.close(); } catch { /* ignore */ } + sessions.delete(sid); + } + } +} + +/** Terminate MCP sessions for a specific (user, OAuth client) pair. + * Used when an OAuth token or session is revoked so only the affected client's + * sessions are closed, not sessions from other clients for the same user. */ +export function revokeUserSessionsForClient(userId: number, clientId: string): void { + for (const [sid, session] of sessions) { + if (session.userId === userId && session.clientId === clientId) { + try { session.server.close(); } catch { /* ignore */ } + try { session.transport.close(); } catch { /* ignore */ } + sessions.delete(sid); + } + } +} diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index af17fb77..d30c01c3 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -12,6 +12,18 @@ export function extractToken(req: Request): string | null { return (authHeader && authHeader.split(' ')[1]) || null; } +function verifyJwtAndLoadUser(token: string): User | null { + try { + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; + const user = db.prepare( + 'SELECT id, username, email, role FROM users WHERE id = ?' + ).get(decoded.id) as User | undefined; + return user ?? null; + } catch { + return null; + } +} + const authenticate = (req: Request, res: Response, next: NextFunction): void => { const token = extractToken(req); @@ -20,20 +32,31 @@ const authenticate = (req: Request, res: Response, next: NextFunction): void => return; } - try { - const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; - const user = db.prepare( - 'SELECT id, username, email, role FROM users WHERE id = ?' - ).get(decoded.id) as User | undefined; - if (!user) { - res.status(401).json({ error: 'User not found', code: 'AUTH_REQUIRED' }); - return; - } - (req as AuthRequest).user = user; - next(); - } catch (err: unknown) { + const user = verifyJwtAndLoadUser(token); + if (!user) { res.status(401).json({ error: 'Invalid or expired token', code: 'AUTH_REQUIRED' }); + return; } + (req as AuthRequest).user = user; + next(); +}; + +/** Like `authenticate` but rejects requests that don't carry an httpOnly session cookie. + * Used on state-mutating OAuth endpoints (consent POST, client CRUD, session revoke) + * to prevent Bearer JWT tokens obtained by other means from managing OAuth clients. */ +const requireCookieAuth = (req: Request, res: Response, next: NextFunction): void => { + const cookieToken = (req as any).cookies?.trek_session; + if (!cookieToken) { + res.status(401).json({ error: 'Cookie session required for this endpoint', code: 'COOKIE_AUTH_REQUIRED' }); + return; + } + const user = verifyJwtAndLoadUser(cookieToken); + if (!user) { + res.status(401).json({ error: 'Invalid or expired session', code: 'AUTH_REQUIRED' }); + return; + } + (req as AuthRequest).user = user; + next(); }; const optionalAuth = (req: Request, res: Response, next: NextFunction): void => { @@ -74,4 +97,4 @@ const demoUploadBlock = (req: Request, res: Response, next: NextFunction): void next(); }; -export { authenticate, optionalAuth, adminOnly, demoUploadBlock }; +export { authenticate, requireCookieAuth, optionalAuth, adminOnly, demoUploadBlock }; diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 2739b07a..e16b1dc2 100644 --- a/server/src/routes/oauth.ts +++ b/server/src/routes/oauth.ts @@ -1,6 +1,6 @@ import express, { Request, Response } from 'express'; -import { authenticate } from '../middleware/auth'; -import { AuthRequest } from '../types'; +import { authenticate, requireCookieAuth, optionalAuth } from '../middleware/auth'; +import { AuthRequest, OptionalAuthRequest } from '../types'; import { isAddonEnabled } from '../services/adminService'; import { ALL_SCOPES, SCOPE_INFO } from '../mcp/scopes'; import { ADDON_IDS } from '../addons'; @@ -23,6 +23,41 @@ import { AuthorizeParams, } from '../services/oauthService'; import { getAppUrl } from '../services/oidcService'; +import { writeAudit, getClientIp, logWarn } from '../services/auditLog'; + +// --------------------------------------------------------------------------- +// Minimal in-file rate limiter (same pattern as auth.ts) +// --------------------------------------------------------------------------- + +interface RateEntry { count: number; first: number; } + +function makeRateLimiter(maxAttempts: number, windowMs: number, keyFn: (req: Request) => string) { + const store = new Map(); + setInterval(() => { + const now = Date.now(); + for (const [k, r] of store) if (now - r.first >= windowMs) store.delete(k); + }, windowMs).unref(); + + return (req: Request, res: Response, next: () => void): void => { + const key = keyFn(req); + const now = Date.now(); + const record = store.get(key); + if (record && record.count >= maxAttempts && now - record.first < windowMs) { + res.status(429).json({ error: 'too_many_requests', error_description: 'Too many attempts. Please try again later.' }); + return; + } + if (!record || now - record.first >= windowMs) { + store.set(key, { count: 1, first: now }); + } else { + record.count++; + } + next(); + }; +} + +const tokenLimiter = makeRateLimiter(30, 60_000, (req) => `${req.ip}|${req.body?.client_id ?? ''}`); +const validateLimiter = makeRateLimiter(30, 60_000, (req) => req.ip ?? 'unknown'); +const revokeLimiter = makeRateLimiter(10, 60_000, (req) => req.ip ?? 'unknown'); // --------------------------------------------------------------------------- // Public router: /.well-known, /oauth/token, /oauth/revoke @@ -31,7 +66,10 @@ import { getAppUrl } from '../services/oidcService'; export const oauthPublicRouter = express.Router(); // RFC 8414 discovery document -oauthPublicRouter.get('/.well-known/oauth-authorization-server', (_req: Request, res: Response) => { +oauthPublicRouter.get('/.well-known/oauth-authorization-server', (req: Request, res: Response) => { + // M2: return 404 (not 403) so feature presence isn't fingerprinted + if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + const base = (getAppUrl() || '').replace(/\/+$/, ''); res.json({ issuer: base, @@ -50,10 +88,15 @@ oauthPublicRouter.get('/.well-known/oauth-authorization-server', (_req: Request, }); // Token endpoint — handles authorization_code and refresh_token grants -oauthPublicRouter.post('/oauth/token', (req: Request, res: Response) => { +oauthPublicRouter.post('/oauth/token', tokenLimiter, (req: Request, res: Response) => { + // M1: RFC 6749 §5.1 — token responses must not be cached + res.set('Cache-Control', 'no-store'); + res.set('Pragma', 'no-cache'); + // Accept both JSON and application/x-www-form-urlencoded const body: Record = typeof req.body === 'object' ? req.body : {}; const { grant_type, code, redirect_uri, client_id, client_secret, code_verifier, refresh_token } = body; + const ip = getClientIp(req); if (!isAddonEnabled(ADDON_IDS.MCP)) { return res.status(403).json({ error: 'mcp_disabled', error_description: 'MCP is not enabled' }); @@ -70,28 +113,38 @@ oauthPublicRouter.post('/oauth/token', (req: Request, res: Response) => { } const pending = consumeAuthCode(code); + + // H5: collapse all invalid_grant cases to one message; log specifics server-side if (!pending) { - return res.status(400).json({ error: 'invalid_grant', error_description: 'Authorization code is invalid or expired' }); + writeAudit({ userId: null, action: 'oauth.token.grant_failed', details: { client_id, reason: 'code_invalid_or_expired' }, ip }); + return res.status(400).json({ error: 'invalid_grant', error_description: 'Authorization grant is invalid.' }); } if (pending.clientId !== client_id) { - return res.status(400).json({ error: 'invalid_grant', error_description: 'client_id mismatch' }); + writeAudit({ userId: pending.userId, action: 'oauth.token.grant_failed', details: { client_id, reason: 'client_id_mismatch' }, ip }); + return res.status(400).json({ error: 'invalid_grant', error_description: 'Authorization grant is invalid.' }); } + if (pending.redirectUri !== redirect_uri) { - return res.status(400).json({ error: 'invalid_grant', error_description: 'redirect_uri mismatch' }); + writeAudit({ userId: pending.userId, action: 'oauth.token.grant_failed', details: { client_id, reason: 'redirect_uri_mismatch' }, ip }); + return res.status(400).json({ error: 'invalid_grant', error_description: 'Authorization grant is invalid.' }); } // Verify client secret if (!authenticateClient(client_id, client_secret)) { + logWarn(`[OAuth] Invalid client credentials for client_id=${client_id} ip=${ip ?? '-'}`); + writeAudit({ userId: pending.userId, action: 'oauth.token.client_auth_failed', details: { client_id }, ip }); return res.status(401).json({ error: 'invalid_client', error_description: 'Invalid client credentials' }); } // Verify PKCE if (!verifyPKCE(code_verifier, pending.codeChallenge)) { - return res.status(400).json({ error: 'invalid_grant', error_description: 'PKCE verification failed' }); + writeAudit({ userId: pending.userId, action: 'oauth.token.grant_failed', details: { client_id, reason: 'pkce_failed' }, ip }); + return res.status(400).json({ error: 'invalid_grant', error_description: 'Authorization grant is invalid.' }); } const tokens = issueTokens(client_id, pending.userId, pending.scopes); + writeAudit({ userId: pending.userId, action: 'oauth.token.issue', details: { client_id, scopes: pending.scopes }, ip }); return res.json(tokens); } @@ -101,8 +154,11 @@ oauthPublicRouter.post('/oauth/token', (req: Request, res: Response) => { return res.status(400).json({ error: 'invalid_request', error_description: 'refresh_token is required' }); } - const result = refreshTokens(refresh_token, client_id, client_secret); + const result = refreshTokens(refresh_token, client_id, client_secret, ip); if (result.error) { + if (result.error === 'invalid_client') { + logWarn(`[OAuth] Invalid client credentials on refresh for client_id=${client_id} ip=${ip ?? '-'}`); + } return res.status(result.status || 400).json({ error: result.error, error_description: result.error === 'invalid_client' ? 'Invalid client credentials' : 'Refresh token is invalid or expired', @@ -116,19 +172,25 @@ oauthPublicRouter.post('/oauth/token', (req: Request, res: Response) => { }); // Token revocation endpoint (RFC 7009) -oauthPublicRouter.post('/oauth/revoke', (req: Request, res: Response) => { +oauthPublicRouter.post('/oauth/revoke', revokeLimiter, (req: Request, res: Response) => { + // M2: return 404 when MCP is disabled + if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + const body: Record = typeof req.body === 'object' ? req.body : {}; const { token, client_id, client_secret } = body; + const ip = getClientIp(req); if (!token || !client_id || !client_secret) { return res.status(400).json({ error: 'invalid_request', error_description: 'token, client_id, and client_secret are required' }); } if (!authenticateClient(client_id, client_secret)) { + logWarn(`[OAuth] Invalid client credentials on revoke for client_id=${client_id} ip=${ip ?? '-'}`); + writeAudit({ userId: null, action: 'oauth.token.client_auth_failed', details: { client_id, endpoint: 'revoke' }, ip }); return res.status(401).json({ error: 'invalid_client', error_description: 'Invalid client credentials' }); } - revokeToken(token, client_id); + revokeToken(token, client_id, undefined, ip); // RFC 7009 §2.2: always respond 200 even if token was already revoked or not found return res.status(200).json({}); }); @@ -140,19 +202,12 @@ oauthPublicRouter.post('/oauth/revoke', (req: Request, res: Response) => { export const oauthApiRouter = express.Router(); // SPA calls this on page load to validate OAuth params before rendering consent UI -oauthApiRouter.get('/authorize/validate', (req: Request, res: Response) => { +oauthApiRouter.get('/authorize/validate', validateLimiter, optionalAuth, (req: Request, res: Response) => { + // M2 / H3: gate by addon; 404 prevents feature fingerprinting for anonymous callers + if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + const params = req.query as Partial; - const userId = (req as any).cookies?.trek_session - ? (() => { - try { - const jwt = require('jsonwebtoken'); - const { JWT_SECRET } = require('../config'); - const decoded = jwt.verify((req as any).cookies.trek_session, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; - const userRow = require('../db/database').db.prepare('SELECT id FROM users WHERE id = ?').get(decoded.id) as { id: number } | undefined; - return userRow?.id ?? null; - } catch { return null; } - })() - : null; + const userId = (req as OptionalAuthRequest).user?.id ?? null; const result = validateAuthorizeRequest( { @@ -167,11 +222,22 @@ oauthApiRouter.get('/authorize/validate', (req: Request, res: Response) => { userId, ); + // H3: when caller is unauthenticated, strip client name / allowed_scopes from the response + // (validateAuthorizeRequest already does this, but be explicit here) + if (userId === null && result.valid) { + return res.json({ valid: result.valid, loginRequired: true }); + } + + // For unauthenticated error cases return a generic error to prevent oracle enumeration + if (userId === null && !result.valid) { + return res.json({ valid: false, error: 'invalid_request', error_description: 'Invalid authorization request' }); + } + return res.json(result); }); -// User submits consent (approve or deny) — requires cookie auth -oauthApiRouter.post('/authorize', authenticate, (req: Request, res: Response) => { +// User submits consent (approve or deny) — requires cookie-only auth (M7) +oauthApiRouter.post('/authorize', requireCookieAuth, (req: Request, res: Response) => { const { user } = req as AuthRequest; const { client_id, redirect_uri, scope, state, @@ -185,6 +251,7 @@ oauthApiRouter.post('/authorize', authenticate, (req: Request, res: Response) => code_challenge_method: string; approved: boolean; }; + const ip = getClientIp(req); if (!isAddonEnabled(ADDON_IDS.MCP)) { return res.status(403).json({ error: 'MCP is not enabled' }); @@ -217,8 +284,8 @@ oauthApiRouter.post('/authorize', authenticate, (req: Request, res: Response) => const scopes = validation.scopes!; - // Store consent so subsequent requests skip the screen - saveConsent(client_id, user.id, scopes); + // Store consent (union with any existing scopes) + saveConsent(client_id, user.id, scopes, ip); // Issue auth code const code = createAuthCode({ @@ -245,7 +312,7 @@ oauthApiRouter.get('/clients', authenticate, (req: Request, res: Response) => { return res.json({ clients: listOAuthClients(user.id) }); }); -oauthApiRouter.post('/clients', authenticate, (req: Request, res: Response) => { +oauthApiRouter.post('/clients', requireCookieAuth, (req: Request, res: Response) => { if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(403).json({ error: 'MCP is not enabled' }); const { user } = req as AuthRequest; const { name, redirect_uris, allowed_scopes } = req.body as { @@ -254,23 +321,23 @@ oauthApiRouter.post('/clients', authenticate, (req: Request, res: Response) => { allowed_scopes: string[]; }; - const result = createOAuthClient(user.id, name, redirect_uris, allowed_scopes); + const result = createOAuthClient(user.id, name, redirect_uris, allowed_scopes, getClientIp(req)); if (result.error) return res.status(result.status || 400).json({ error: result.error }); return res.status(201).json(result); }); -oauthApiRouter.post('/clients/:id/rotate', authenticate, (req: Request, res: Response) => { +oauthApiRouter.post('/clients/:id/rotate', requireCookieAuth, (req: Request, res: Response) => { if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(403).json({ error: 'MCP is not enabled' }); const { user } = req as AuthRequest; - const result = rotateOAuthClientSecret(user.id, req.params.id); + const result = rotateOAuthClientSecret(user.id, req.params.id, getClientIp(req)); if (result.error) return res.status(result.status || 400).json({ error: result.error }); return res.json({ client_secret: result.client_secret }); }); -oauthApiRouter.delete('/clients/:id', authenticate, (req: Request, res: Response) => { +oauthApiRouter.delete('/clients/:id', requireCookieAuth, (req: Request, res: Response) => { if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(403).json({ error: 'MCP is not enabled' }); const { user } = req as AuthRequest; - const result = deleteOAuthClient(user.id, req.params.id); + const result = deleteOAuthClient(user.id, req.params.id, getClientIp(req)); if (result.error) return res.status(result.status || 400).json({ error: result.error }); return res.json({ success: true }); }); @@ -283,10 +350,10 @@ oauthApiRouter.get('/sessions', authenticate, (req: Request, res: Response) => { return res.json({ sessions: listOAuthSessions(user.id) }); }); -oauthApiRouter.delete('/sessions/:id', authenticate, (req: Request, res: Response) => { +oauthApiRouter.delete('/sessions/:id', requireCookieAuth, (req: Request, res: Response) => { if (!isAddonEnabled(ADDON_IDS.MCP)) return res.status(403).json({ error: 'MCP is not enabled' }); const { user } = req as AuthRequest; - const result = revokeSession(user.id, Number(req.params.id)); + const result = revokeSession(user.id, Number(req.params.id), getClientIp(req)); if (result.error) return res.status(result.status || 400).json({ error: result.error }); return res.json({ success: true }); }); diff --git a/server/src/services/oauthService.ts b/server/src/services/oauthService.ts index e6502fbf..dbb7ee75 100644 --- a/server/src/services/oauthService.ts +++ b/server/src/services/oauthService.ts @@ -4,15 +4,21 @@ import { isAddonEnabled } from './adminService'; import { validateScopes } from '../mcp/scopes'; import { ADDON_IDS } from '../addons'; import { User } from '../types'; +import { writeAudit, logWarn } from './auditLog'; +import { revokeUserSessionsForClient } from '../mcp/sessionManager'; // --------------------------------------------------------------------------- // Constants // --------------------------------------------------------------------------- -const ACCESS_TOKEN_TTL_S = 60 * 60; // 1 hour -const REFRESH_TOKEN_TTL_MS = 30 * 24 * 60 * 60 * 1000; // 30 days rolling +const ACCESS_TOKEN_TTL_S = 60 * 60; // 1 hour +const REFRESH_TOKEN_TTL_MS = 30 * 24 * 60 * 60 * 1000; // 30 days rolling const AUTH_CODE_TTL_MS = 2 * 60 * 1000; // 2 minutes +// PKCE format (RFC 7636) +const CODE_CHALLENGE_RE = /^[A-Za-z0-9_-]{43}$/; +const CODE_VERIFIER_RE = /^[A-Za-z0-9\-._~]{43,128}$/; + // --------------------------------------------------------------------------- // In-memory auth code store (short-lived, no need for DB persistence) // --------------------------------------------------------------------------- @@ -61,6 +67,7 @@ interface OAuthTokenRow { access_token_expires_at: string; refresh_token_expires_at: string; revoked_at: string | null; + parent_token_id: number | null; } // --------------------------------------------------------------------------- @@ -71,6 +78,14 @@ function hashToken(raw: string): string { return createHash('sha256').update(raw).digest('hex'); } +/** Constant-time comparison of two hex-encoded SHA-256 hashes. */ +function timingSafeEqualHex(a: string, b: string): boolean { + if (a.length !== b.length) return false; + try { + return crypto.timingSafeEqual(Buffer.from(a, 'hex'), Buffer.from(b, 'hex')); + } catch { return false; } +} + function generateAccessToken(): string { return 'trekoa_' + randomBytes(24).toString('hex'); } @@ -99,6 +114,7 @@ export function createOAuthClient( name: string, redirectUris: string[], allowedScopes: string[], + ip?: string | null, ): { error?: string; status?: number; client?: Record } { if (!name?.trim()) return { error: 'Name is required', status: 400 }; if (name.trim().length > 100) return { error: 'Name must be 100 characters or less', status: 400 }; @@ -136,6 +152,8 @@ export function createOAuthClient( 'SELECT id, user_id, name, client_id, redirect_uris, allowed_scopes, created_at FROM oauth_clients WHERE id = ?' ).get(id) as OAuthClientRow; + writeAudit({ userId, action: 'oauth.client.create', details: { client_id: clientId, name: name.trim() }, ip }); + return { client: { id: row.id, @@ -153,8 +171,9 @@ export function createOAuthClient( export function rotateOAuthClientSecret( userId: number, clientRowId: string, + ip?: string | null, ): { error?: string; status?: number; client_secret?: string } { - const row = db.prepare('SELECT id FROM oauth_clients WHERE id = ? AND user_id = ?').get(clientRowId, userId) as OAuthClientRow | undefined; + const row = db.prepare('SELECT id, client_id FROM oauth_clients WHERE id = ? AND user_id = ?').get(clientRowId, userId) as OAuthClientRow | undefined; if (!row) return { error: 'Client not found', status: 404 }; const rawSecret = 'trekcs_' + randomBytes(24).toString('hex'); @@ -163,7 +182,13 @@ export function rotateOAuthClientSecret( db.prepare('UPDATE oauth_clients SET client_secret_hash = ? WHERE id = ?').run(secretHash, clientRowId); // Revoke all existing tokens for this client so old sessions are invalidated - db.prepare("UPDATE oauth_tokens SET revoked_at = datetime('now') WHERE client_id = (SELECT client_id FROM oauth_clients WHERE id = ?) AND revoked_at IS NULL").run(clientRowId); + db.prepare("UPDATE oauth_tokens SET revoked_at = datetime('now') WHERE client_id = ? AND revoked_at IS NULL").run(row.client_id); + + // Terminate active MCP sessions for this (user, client) pair + + revokeUserSessionsForClient(userId, row.client_id); + + writeAudit({ userId, action: 'oauth.client.rotate_secret', details: { client_id: row.client_id }, ip }); return { client_secret: rawSecret }; } @@ -171,10 +196,12 @@ export function rotateOAuthClientSecret( export function deleteOAuthClient( userId: number, clientRowId: string, + ip?: string | null, ): { error?: string; status?: number; success?: boolean } { - const row = db.prepare('SELECT id FROM oauth_clients WHERE id = ? AND user_id = ?').get(clientRowId, userId); + const row = db.prepare('SELECT id, client_id FROM oauth_clients WHERE id = ? AND user_id = ?').get(clientRowId, userId) as OAuthClientRow | undefined; if (!row) return { error: 'Client not found', status: 404 }; db.prepare('DELETE FROM oauth_clients WHERE id = ?').run(clientRowId); + writeAudit({ userId, action: 'oauth.client.delete', details: { client_id: row.client_id }, ip }); return { success: true }; } @@ -214,10 +241,14 @@ export function getConsent(clientId: string, userId: number): string[] | null { return row ? JSON.parse(row.scopes) : null; } -export function saveConsent(clientId: string, userId: number, scopes: string[]): void { +export function saveConsent(clientId: string, userId: number, scopes: string[], ip?: string | null): void { + // Union existing consent with newly approved scopes (M5: never narrow stored consent) + const existing = getConsent(clientId, userId) ?? []; + const merged = Array.from(new Set([...existing, ...scopes])); db.prepare( 'INSERT OR REPLACE INTO oauth_consents (client_id, user_id, scopes, updated_at) VALUES (?, ?, ?, CURRENT_TIMESTAMP)' - ).run(clientId, userId, JSON.stringify(scopes)); + ).run(clientId, userId, JSON.stringify(merged)); + writeAudit({ userId, action: 'oauth.consent.grant', details: { client_id: clientId, scopes: merged }, ip }); } export function isConsentSufficient(existingScopes: string[], requestedScopes: string[]): boolean { @@ -232,6 +263,7 @@ export function issueTokens( clientId: string, userId: number, scopes: string[], + parentTokenId: number | null = null, ): { access_token: string; refresh_token: string; @@ -250,9 +282,9 @@ export function issueTokens( db.prepare(` INSERT INTO oauth_tokens - (client_id, user_id, access_token_hash, refresh_token_hash, scopes, access_token_expires_at, refresh_token_expires_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `).run(clientId, userId, accessHash, refreshHash, JSON.stringify(scopes), accessExpiry.toISOString(), refreshExpiry.toISOString()); + (client_id, user_id, access_token_hash, refresh_token_hash, scopes, access_token_expires_at, refresh_token_expires_at, parent_token_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `).run(clientId, userId, accessHash, refreshHash, JSON.stringify(scopes), accessExpiry.toISOString(), refreshExpiry.toISOString(), parentTokenId); return { access_token: rawAccess, @@ -270,13 +302,14 @@ export function issueTokens( export interface OAuthTokenInfo { user: User; scopes: string[]; + clientId: string; } export function getUserByAccessToken(rawToken: string): OAuthTokenInfo | null { const hash = hashToken(rawToken); const row = db.prepare(` SELECT ot.scopes, ot.revoked_at, ot.access_token_expires_at, - ot.user_id, u.username, u.email, u.role + ot.user_id, ot.client_id, u.username, u.email, u.role FROM oauth_tokens ot JOIN users u ON ot.user_id = u.id WHERE ot.access_token_hash = ? @@ -289,50 +322,122 @@ export function getUserByAccessToken(rawToken: string): OAuthTokenInfo | null { return { user: { id: row.user_id, username: row.username, email: row.email, role: row.role as 'admin' | 'user' }, scopes: JSON.parse(row.scopes), + clientId: row.client_id, }; } // --------------------------------------------------------------------------- -// Token refresh (rotation) +// Token refresh (rotation + replay detection) // --------------------------------------------------------------------------- +/** Walk parent_token_id upward to find the root token id of this rotation chain. */ +function findChainRoot(tokenId: number): number { + let current = tokenId; + for (let i = 0; i < 100; i++) { + const row = db.prepare('SELECT id, parent_token_id FROM oauth_tokens WHERE id = ?').get(current) as { id: number; parent_token_id: number | null } | undefined; + if (!row || row.parent_token_id === null) return current; + current = row.parent_token_id; + } + return current; +} + +/** Revoke all tokens in the rotation chain rooted at rootId. Returns affected ids. */ +function revokeChain(rootId: number): number[] { + const rows = db.prepare(` + WITH RECURSIVE chain(id) AS ( + SELECT id FROM oauth_tokens WHERE id = ? + UNION ALL + SELECT t.id FROM oauth_tokens t JOIN chain c ON t.parent_token_id = c.id + ) + SELECT id FROM chain + `).all(rootId) as { id: number }[]; + const ids = rows.map(r => r.id); + if (ids.length > 0) { + db.prepare( + `UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE id IN (${ids.map(() => '?').join(',')}) AND revoked_at IS NULL` + ).run(...ids); + } + return ids; +} + export function refreshTokens( rawRefreshToken: string, clientId: string, clientSecret: string, + ip?: string | null, ): { error?: string; status?: number; tokens?: ReturnType } { const client = db.prepare('SELECT client_id, client_secret_hash FROM oauth_clients WHERE client_id = ?').get(clientId) as OAuthClientRow | undefined; if (!client) return { error: 'invalid_client', status: 401 }; - if (hashToken(clientSecret) !== client.client_secret_hash) return { error: 'invalid_client', status: 401 }; + if (!timingSafeEqualHex(hashToken(clientSecret), client.client_secret_hash)) return { error: 'invalid_client', status: 401 }; const hash = hashToken(rawRefreshToken); const row = db.prepare(` - SELECT id, client_id, user_id, scopes, refresh_token_expires_at, revoked_at + SELECT id, client_id, user_id, scopes, refresh_token_expires_at, revoked_at, parent_token_id FROM oauth_tokens WHERE refresh_token_hash = ? `).get(hash) as OAuthTokenRow | undefined; if (!row) return { error: 'invalid_grant', status: 400 }; if (row.client_id !== clientId) return { error: 'invalid_grant', status: 400 }; - if (row.revoked_at) return { error: 'invalid_grant', status: 400 }; + + // ---- Replay detection (C3) ---- + if (row.revoked_at) { + // A revoked refresh token was replayed — assume token theft. Cascade-revoke the chain. + const rootId = findChainRoot(row.id); + revokeChain(rootId); + + + revokeUserSessionsForClient(row.user_id, clientId); + + writeAudit({ + userId: row.user_id, + action: 'oauth.token.replay_detected', + details: { client_id: clientId }, + ip, + }); + logWarn(`[OAuth] Refresh token replay detected for user=${row.user_id} client=${clientId} ip=${ip ?? '-'}`); + + return { error: 'invalid_grant', status: 400 }; + } + if (new Date(row.refresh_token_expires_at) < new Date()) return { error: 'invalid_grant', status: 400 }; - // Revoke old pair immediately (rotation) + // Revoke old pair immediately (rotation) and issue new pair linked to old row db.prepare('UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE id = ?').run(row.id); - return { tokens: issueTokens(clientId, row.user_id, JSON.parse(row.scopes)) }; + // Terminate active MCP sessions for the old token's client so client must re-authenticate + + revokeUserSessionsForClient(row.user_id, clientId); + + const tokens = issueTokens(clientId, row.user_id, JSON.parse(row.scopes), row.id); + writeAudit({ userId: row.user_id, action: 'oauth.token.refresh', details: { client_id: clientId }, ip }); + + return { tokens }; } // --------------------------------------------------------------------------- // Token revocation // --------------------------------------------------------------------------- -export function revokeToken(rawToken: string, clientId: string): void { +export function revokeToken(rawToken: string, clientId: string, userId?: number, ip?: string | null): void { const hash = hashToken(rawToken); + + // Get the user_id for the token so we can revoke its MCP sessions + const row = db.prepare( + 'SELECT user_id FROM oauth_tokens WHERE (access_token_hash = ? OR refresh_token_hash = ?) AND client_id = ?' + ).get(hash, hash, clientId) as { user_id: number } | undefined; + db.prepare(` UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE (access_token_hash = ? OR refresh_token_hash = ?) AND client_id = ? `).run(hash, hash, clientId); + + const affectedUserId = row?.user_id ?? userId; + if (affectedUserId) { + + revokeUserSessionsForClient(affectedUserId, clientId); + writeAudit({ userId: affectedUserId, action: 'oauth.token.revoke', details: { client_id: clientId, method: 'token' }, ip }); + } } // --------------------------------------------------------------------------- @@ -356,10 +461,18 @@ export function listOAuthSessions(userId: number): Record[] { export function revokeSession( userId: number, sessionId: number, + ip?: string | null, ): { error?: string; status?: number; success?: boolean } { - const row = db.prepare('SELECT id FROM oauth_tokens WHERE id = ? AND user_id = ?').get(sessionId, userId); + const row = db.prepare('SELECT id, client_id FROM oauth_tokens WHERE id = ? AND user_id = ?').get(sessionId, userId) as { id: number; client_id: string } | undefined; if (!row) return { error: 'Session not found', status: 404 }; + db.prepare('UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE id = ?').run(sessionId); + + + revokeUserSessionsForClient(userId, row.client_id); + + writeAudit({ userId, action: 'oauth.token.revoke', details: { client_id: row.client_id, method: 'session' }, ip }); + return { success: true }; } @@ -405,6 +518,11 @@ export function validateAuthorizeRequest( return { valid: false, error: 'invalid_request', error_description: 'PKCE with code_challenge_method=S256 is required (OAuth 2.1)' }; } + // H1: Enforce code_challenge format (RFC 7636 §4.2) + if (!CODE_CHALLENGE_RE.test(params.code_challenge)) { + return { valid: false, error: 'invalid_request', error_description: 'code_challenge must be 43 base64url characters (S256)' }; + } + if (!params.client_id) { return { valid: false, error: 'invalid_request', error_description: 'client_id is required' }; } @@ -433,12 +551,9 @@ export function validateAuthorizeRequest( } if (userId === null) { - return { - valid: true, - client: { name: client.name, allowed_scopes: allowedScopes }, - scopes: grantedScopes, - loginRequired: true, - }; + // H3: return only the minimum required fields — do NOT expose scopes, client.name, or + // allowed_scopes to unauthenticated callers to prevent client enumeration. + return { valid: true, loginRequired: true }; } const existingConsent = getConsent(params.client_id, userId); @@ -457,8 +572,15 @@ export function validateAuthorizeRequest( // --------------------------------------------------------------------------- export function verifyPKCE(codeVerifier: string, codeChallenge: string): boolean { + // H1: validate code_verifier format before hashing + if (!CODE_VERIFIER_RE.test(codeVerifier)) return false; + const expected = crypto.createHash('sha256').update(codeVerifier).digest('base64url'); - return expected === codeChallenge; + // Constant-time compare (both are base64url strings of equal length for S256) + if (expected.length !== codeChallenge.length) return false; + try { + return crypto.timingSafeEqual(Buffer.from(expected), Buffer.from(codeChallenge)); + } catch { return false; } } // --------------------------------------------------------------------------- @@ -468,6 +590,7 @@ export function verifyPKCE(codeVerifier: string, codeChallenge: string): boolean export function authenticateClient(clientId: string, clientSecret: string): OAuthClientRow | null { const client = db.prepare('SELECT * FROM oauth_clients WHERE client_id = ?').get(clientId) as OAuthClientRow | undefined; if (!client) return null; - if (hashToken(clientSecret) !== client.client_secret_hash) return null; + // H4: constant-time comparison to prevent timing side-channel + if (!timingSafeEqualHex(hashToken(clientSecret), client.client_secret_hash)) return null; return client; } diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index a76a7850..da247acd 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -51,6 +51,7 @@ vi.mock('../../src/services/adminService', async (importOriginal) => { vi.mock('../../src/services/oidcService', () => ({ getAppUrl: () => '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 { createApp } from '../../src/app'; import { createTables } from '../../src/db/schema'; @@ -471,20 +472,21 @@ describe('POST /oauth/revoke', () => { // ───────────────────────────────────────────────────────────────────────────── describe('GET /api/oauth/authorize/validate', () => { - it('OAUTH-019 — returns 200 with valid:false when MCP addon disabled', async () => { + it('OAUTH-019 — returns 404 when MCP addon disabled (M2: prevents feature fingerprinting)', async () => { isAddonEnabledMock.mockReturnValue(false); const res = await request(app) .get('/api/oauth/authorize/validate') .query({ response_type: 'code', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'c', code_challenge_method: 'S256' }); - expect(res.status).toBe(200); - expect(res.body.valid).toBe(false); - expect(res.body.error).toBe('mcp_disabled'); + expect(res.status).toBe(404); }); - it('OAUTH-020 — returns 200 with valid:false for wrong response_type', async () => { + it('OAUTH-020 — returns 200 with valid:false for wrong response_type (authenticated)', async () => { + const { user } = createUser(testDb); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') - .query({ response_type: 'token', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'c', code_challenge_method: 'S256' }); + .set('Cookie', authCookie(user.id)) + .query({ response_type: 'token', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: challenge, code_challenge_method: 'S256' }); expect(res.status).toBe(200); expect(res.body.valid).toBe(false); expect(res.body.error).toBe('unsupported_response_type'); @@ -499,27 +501,32 @@ describe('GET /api/oauth/authorize/validate', () => { expect(res.body.error).toBe('invalid_request'); }); - it('OAUTH-022 — returns 200 with valid:false for unknown client_id', async () => { + it('OAUTH-022 — returns 200 with valid:false for unknown client_id (authenticated)', async () => { + const { user } = createUser(testDb); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') - .query({ response_type: 'code', client_id: 'unknown-client', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'abc', code_challenge_method: 'S256' }); + .set('Cookie', authCookie(user.id)) + .query({ response_type: 'code', client_id: 'unknown-client', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: challenge, code_challenge_method: 'S256' }); expect(res.status).toBe(200); expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_client'); }); - it('OAUTH-023 — returns 200 with valid:false for mismatched redirect_uri', async () => { + it('OAUTH-023 — returns 200 with valid:false for mismatched redirect_uri (authenticated)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) .query({ response_type: 'code', client_id: r.client!.client_id, redirect_uri: 'https://evil.example.com/cb', scope: 'trips:read', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); @@ -527,18 +534,20 @@ describe('GET /api/oauth/authorize/validate', () => { expect(res.body.error).toBe('invalid_redirect_uri'); }); - it('OAUTH-024 — returns 200 with valid:false for empty scope', async () => { + it('OAUTH-024 — returns 200 with valid:false for empty scope (authenticated)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) .query({ response_type: 'code', client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: '', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); @@ -546,18 +555,20 @@ describe('GET /api/oauth/authorize/validate', () => { expect(res.body.error).toBe('invalid_scope'); }); - it('OAUTH-025a — narrows scope to allowed intersection when client lacks some requested scopes', async () => { + it('OAUTH-025a — narrows scope to allowed intersection when client lacks some requested scopes (authenticated)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) .query({ response_type: 'code', client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'trips:read trips:delete', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); @@ -566,18 +577,20 @@ describe('GET /api/oauth/authorize/validate', () => { expect(res.body.scopes).toEqual(['trips:read']); }); - it('OAUTH-025b — returns 200 with valid:false when no requested scope is allowed', async () => { + it('OAUTH-025b — returns 200 with valid:false when no requested scope is allowed (authenticated)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) .query({ response_type: 'code', client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'budget:write', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); @@ -585,9 +598,10 @@ describe('GET /api/oauth/authorize/validate', () => { expect(res.body.error).toBe('invalid_scope'); }); - it('OAUTH-026 — returns 200 with loginRequired=true when no cookie session', async () => { + it('OAUTH-026 — unauthenticated valid request returns loginRequired=true (H3: minimal response, no client info)', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') @@ -596,17 +610,21 @@ describe('GET /api/oauth/authorize/validate', () => { client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'trips:read', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); expect(res.body.valid).toBe(true); expect(res.body.loginRequired).toBe(true); + // H3: client name and scopes must NOT be revealed to unauthenticated callers + expect(res.body.client).toBeUndefined(); + expect(res.body.allowed_scopes).toBeUndefined(); }); - it('OAUTH-027 — returns 200 with loginRequired or consentRequired when session present but no prior consent', async () => { + it('OAUTH-027 — authenticated with no prior consent returns consentRequired=true with client details', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .get('/api/oauth/authorize/validate') @@ -616,13 +634,15 @@ describe('GET /api/oauth/authorize/validate', () => { client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'trips:read', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); expect(res.body.valid).toBe(true); - // Either loginRequired=true (cookie not decoded in test env) or consentRequired=true (full decode working) - expect(res.body.loginRequired === true || res.body.consentRequired === true).toBe(true); + expect(res.body.consentRequired).toBe(true); + // Authenticated users get full client info (unlike unauthenticated H3 path) + expect(res.body.client).toBeDefined(); + expect(res.body.scopes).toBeDefined(); }); }); @@ -669,6 +689,7 @@ describe('POST /api/oauth/authorize', () => { it('OAUTH-031 — invalid params returns 400', async () => { const { user } = createUser(testDb); + const { challenge } = makePkce(); const res = await request(app) .post('/api/oauth/authorize') @@ -678,7 +699,7 @@ describe('POST /api/oauth/authorize', () => { client_id: 'unknown-client', redirect_uri: 'https://app.example.com/cb', scope: 'trips:read', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(400); @@ -687,6 +708,7 @@ describe('POST /api/oauth/authorize', () => { it('OAUTH-032 — happy path: approve returns redirect with code', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .post('/api/oauth/authorize') @@ -696,7 +718,7 @@ describe('POST /api/oauth/authorize', () => { client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'trips:read', - code_challenge: 'abc', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); @@ -876,3 +898,354 @@ describe('Sessions — /api/oauth/sessions', () => { expect(res.status).toBe(403); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Security behavior tests (M1, M2, H1, H3, H5, M5, M7, C3) +// ───────────────────────────────────────────────────────────────────────────── + +describe('M1 — Cache-Control headers on /oauth/token', () => { + it('OAUTH-SEC-001 — token endpoint sets Cache-Control: no-store', async () => { + const res = await request(app) + .post('/oauth/token') + .send({ grant_type: 'authorization_code', client_id: 'x', client_secret: 'y', code: 'z', redirect_uri: 'https://r.example.com/cb', code_verifier: 'v' }); + expect(res.headers['cache-control']).toBe('no-store'); + expect(res.headers['pragma']).toBe('no-cache'); + }); +}); + +describe('M2 — 404 when MCP disabled on discovery + revoke endpoints', () => { + it('OAUTH-SEC-002 — /.well-known/oauth-authorization-server returns 404 when disabled', async () => { + isAddonEnabledMock.mockReturnValue(false); + const res = await request(app).get('/.well-known/oauth-authorization-server'); + expect(res.status).toBe(404); + }); + + it('OAUTH-SEC-003 — /oauth/revoke returns 404 when disabled', async () => { + isAddonEnabledMock.mockReturnValue(false); + const res = await request(app) + .post('/oauth/revoke') + .send({ token: 'x', client_id: 'y', client_secret: 'z' }); + expect(res.status).toBe(404); + }); +}); + +describe('H1 — PKCE format validation', () => { + it('OAUTH-SEC-004 — short code_challenge (<43 chars) rejected on /authorize/validate', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const res = await request(app) + .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) + .query({ + response_type: 'code', + client_id: r.client!.client_id, + redirect_uri: 'https://app.example.com/cb', + scope: 'trips:read', + code_challenge: 'tooshort', + code_challenge_method: 'S256', + }); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); + expect(res.body.error).toBe('invalid_request'); + }); + + it('OAUTH-SEC-005 — wrong code_verifier format rejected on /oauth/token (invalid_grant)', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); + + const code = createAuthCode({ + clientId: r.client!.client_id as string, + userId: user.id, + redirectUri: 'https://app.example.com/cb', + scopes: ['trips:read'], + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + + // Submit a valid-looking but wrong-format verifier (too short) + const res = await request(app) + .post('/oauth/token') + .send({ + grant_type: 'authorization_code', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + code, + redirect_uri: 'https://app.example.com/cb', + code_verifier: 'short', + }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid_grant'); + }); +}); + +describe('H3 — Unauthenticated /authorize/validate returns minimal response', () => { + it('OAUTH-SEC-006 — invalid request by unauthenticated caller returns generic error (no oracle)', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); + + // Deliberately wrong redirect_uri — should get generic error, not invalid_redirect_uri + const res = await request(app) + .get('/api/oauth/authorize/validate') + .query({ + response_type: 'code', + client_id: r.client!.client_id, + redirect_uri: 'https://evil.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); + expect(res.body.error).toBe('invalid_request'); + // Must not leak specific error type or client details + expect(res.body.error).not.toBe('invalid_redirect_uri'); + expect(res.body.client).toBeUndefined(); + }); +}); + +describe('H5 — All invalid_grant cases return identical response body', () => { + it('OAUTH-SEC-007 — expired/bad code, client_id mismatch, redirect_uri mismatch all return same body', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { verifier, challenge } = makePkce(); + + const code = createAuthCode({ + clientId: r.client!.client_id as string, + userId: user.id, + redirectUri: 'https://app.example.com/cb', + scopes: ['trips:read'], + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + + // Bad code + const res1 = await request(app).post('/oauth/token').send({ + grant_type: 'authorization_code', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + code: 'bad-code-xyz', + redirect_uri: 'https://app.example.com/cb', + code_verifier: verifier, + }); + + // Redirect URI mismatch (need fresh code since code is single-use) + const code2 = createAuthCode({ + clientId: r.client!.client_id as string, + userId: user.id, + redirectUri: 'https://app.example.com/cb', + scopes: ['trips:read'], + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + const res2 = await request(app).post('/oauth/token').send({ + grant_type: 'authorization_code', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + code: code2, + redirect_uri: 'https://wrong.example.com/cb', + code_verifier: verifier, + }); + + expect(res1.status).toBe(400); + expect(res2.status).toBe(400); + expect(res1.body.error).toBe('invalid_grant'); + expect(res2.body.error).toBe('invalid_grant'); + // Both must use exactly the same error_description (H5) + expect(res1.body.error_description).toBe(res2.body.error_description); + }); +}); + +describe('M5 — Consent scope union (re-authorize adds to existing consent)', () => { + it('OAUTH-SEC-008 — second consent adds new scope without losing old scope', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read', 'places:read']); + const { challenge: ch1 } = makePkce(); + const { challenge: ch2 } = makePkce(); + + // First consent: trips:read + await request(app) + .post('/api/oauth/authorize') + .set('Cookie', authCookie(user.id)) + .send({ + approved: true, + client_id: r.client!.client_id, + redirect_uri: 'https://app.example.com/cb', + scope: 'trips:read', + code_challenge: ch1, + code_challenge_method: 'S256', + }); + + // Second consent: places:read — should not drop trips:read + await request(app) + .post('/api/oauth/authorize') + .set('Cookie', authCookie(user.id)) + .send({ + approved: true, + client_id: r.client!.client_id, + redirect_uri: 'https://app.example.com/cb', + scope: 'places:read', + code_challenge: ch2, + code_challenge_method: 'S256', + }); + + // Re-validate with trips:read — should now be auto-approved (consentRequired=false) + const { challenge: ch3 } = makePkce(); + const res = await request(app) + .get('/api/oauth/authorize/validate') + .set('Cookie', authCookie(user.id)) + .query({ + response_type: 'code', + client_id: r.client!.client_id, + redirect_uri: 'https://app.example.com/cb', + scope: 'trips:read', + code_challenge: ch3, + code_challenge_method: 'S256', + }); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(true); + expect(res.body.consentRequired).toBeFalsy(); + }); +}); + +describe('M7 — Cookie-only auth on privileged OAuth endpoints', () => { + it('OAUTH-SEC-009 — POST /api/oauth/authorize rejects Bearer JWT (no cookie)', async () => { + const { user } = createUser(testDb); + // Use a valid JWT in Authorization header (no cookie) — must be rejected + const jwt = require('jsonwebtoken'); + const token = jwt.sign({ id: user.id }, 'test-jwt-secret-for-trek-testing-only', { algorithm: 'HS256' }); + + const res = await request(app) + .post('/api/oauth/authorize') + .set('Authorization', `Bearer ${token}`) + .send({ approved: true, client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'c', code_challenge_method: 'S256' }); + expect(res.status).toBe(401); + expect(res.body.code).toBe('COOKIE_AUTH_REQUIRED'); + }); + + it('OAUTH-SEC-010 — POST /api/oauth/clients rejects Bearer JWT (no cookie)', async () => { + const jwt = require('jsonwebtoken'); + const { user } = createUser(testDb); + const token = jwt.sign({ id: user.id }, 'test-jwt-secret-for-trek-testing-only', { algorithm: 'HS256' }); + + const res = await request(app) + .post('/api/oauth/clients') + .set('Authorization', `Bearer ${token}`) + .send({ name: 'App', redirect_uris: ['https://app.example.com/cb'], allowed_scopes: ['trips:read'] }); + expect(res.status).toBe(401); + expect(res.body.code).toBe('COOKIE_AUTH_REQUIRED'); + }); + + it('OAUTH-SEC-011 — DELETE /api/oauth/sessions/:id rejects Bearer JWT (no cookie)', async () => { + const jwt = require('jsonwebtoken'); + const { user } = createUser(testDb); + const token = jwt.sign({ id: user.id }, 'test-jwt-secret-for-trek-testing-only', { algorithm: 'HS256' }); + + const res = await request(app) + .delete('/api/oauth/sessions/1') + .set('Authorization', `Bearer ${token}`); + expect(res.status).toBe(401); + expect(res.body.code).toBe('COOKIE_AUTH_REQUIRED'); + }); +}); + +describe('C3 — Refresh token replay detection', () => { + it('OAUTH-SEC-012 — replaying a rotated (old) refresh token returns invalid_grant', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { verifier, challenge } = makePkce(); + + const code = createAuthCode({ + clientId: r.client!.client_id as string, + userId: user.id, + redirectUri: 'https://app.example.com/cb', + scopes: ['trips:read'], + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + + // Get initial tokens + const t1 = await request(app).post('/oauth/token').send({ + grant_type: 'authorization_code', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + code, + redirect_uri: 'https://app.example.com/cb', + code_verifier: verifier, + }); + expect(t1.status).toBe(200); + const originalRefreshToken = t1.body.refresh_token; + + // Rotate once (legitimate use) + const t2 = await request(app).post('/oauth/token').send({ + grant_type: 'refresh_token', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + refresh_token: originalRefreshToken, + }); + expect(t2.status).toBe(200); + + // Replay the original (now rotated/revoked) refresh token — must be rejected + const t3 = await request(app).post('/oauth/token').send({ + grant_type: 'refresh_token', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + refresh_token: originalRefreshToken, + }); + expect(t3.status).toBe(400); + expect(t3.body.error).toBe('invalid_grant'); + }); + + it('OAUTH-SEC-013 — replaying old token also invalidates the new chain', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { verifier, challenge } = makePkce(); + + const code = createAuthCode({ + clientId: r.client!.client_id as string, + userId: user.id, + redirectUri: 'https://app.example.com/cb', + scopes: ['trips:read'], + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + + const t1 = await request(app).post('/oauth/token').send({ + grant_type: 'authorization_code', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + code, + redirect_uri: 'https://app.example.com/cb', + code_verifier: verifier, + }); + const originalRefreshToken = t1.body.refresh_token; + + // Legitimate rotate — get new token + const t2 = await request(app).post('/oauth/token').send({ + grant_type: 'refresh_token', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + refresh_token: originalRefreshToken, + }); + const newRefreshToken = t2.body.refresh_token; + + // Replay original — triggers chain revocation + await request(app).post('/oauth/token').send({ + grant_type: 'refresh_token', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + refresh_token: originalRefreshToken, + }); + + // New token (from legitimate rotation) must also be dead now + const t4 = await request(app).post('/oauth/token').send({ + grant_type: 'refresh_token', + client_id: r.client!.client_id, + client_secret: r.client!.client_secret, + refresh_token: newRefreshToken, + }); + expect(t4.status).toBe(400); + expect(t4.body.error).toBe('invalid_grant'); + }); +}); diff --git a/server/tests/unit/services/oauthService.test.ts b/server/tests/unit/services/oauthService.test.ts index 9179e8cb..5222b609 100644 --- a/server/tests/unit/services/oauthService.test.ts +++ b/server/tests/unit/services/oauthService.test.ts @@ -34,7 +34,7 @@ vi.mock('../../../src/services/apiKeyCrypto', () => ({ decrypt_api_key: (v: string) => v, maybe_encrypt_api_key: (v: string) => v, })); -vi.mock('../../../src/mcp', () => ({ revokeUserSessions: vi.fn() })); +vi.mock('../../../src/mcp/sessionManager', () => ({ revokeUserSessions: vi.fn(), revokeUserSessionsForClient: vi.fn(), sessions: new Map() })); vi.mock('../../../src/demo/demo-reset', () => ({ saveBaseline: vi.fn() })); vi.mock('../../../src/services/adminService', () => ({ isAddonEnabled: vi.fn().mockReturnValue(true), @@ -44,6 +44,13 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; +// PKCE helper — generates a valid code_verifier + code_challenge pair (RFC 7636) +function makePkce() { + const verifier = crypto.randomBytes(32).toString('base64url'); // 43 chars + const challenge = crypto.createHash('sha256').update(verifier).digest('base64url'); // 43 chars + return { verifier, challenge }; +} + import { createOAuthClient, listOAuthClients, @@ -520,6 +527,9 @@ describe('listOAuthSessions + revokeSession', () => { // --------------------------------------------------------------------------- describe('validateAuthorizeRequest', () => { + // Use a proper 43-char S256 code_challenge to pass H1 format validation + const { challenge: VALID_CHALLENGE } = makePkce(); + function makeParams(overrides: Partial<{ response_type: string; client_id: string; @@ -533,7 +543,7 @@ describe('validateAuthorizeRequest', () => { client_id: '', redirect_uri: 'https://example.com/callback', scope: 'trips:read', - code_challenge: 'abc123', + code_challenge: VALID_CHALLENGE, code_challenge_method: 'S256', ...overrides, }; @@ -699,3 +709,231 @@ describe('saveConsent + getConsent + isConsentSufficient', () => { expect(isConsentSufficient([], ['trips:read'])).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// M5 — saveConsent unions instead of replacing +// --------------------------------------------------------------------------- + +describe('saveConsent — scope union (M5)', () => { + it('unioning scopes: approving B after A leaves both in consent', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id, { scopes: ['trips:read', 'budget:write'] }); + const clientId = created.client!.client_id as string; + + saveConsent(clientId, user.id, ['trips:read']); + saveConsent(clientId, user.id, ['budget:write']); + + const consent = getConsent(clientId, user.id); + expect(consent).toContain('trips:read'); + expect(consent).toContain('budget:write'); + }); + + it('re-approving a superset scope still preserves previously-consented scopes', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id, { scopes: ['trips:read', 'trips:write'] }); + const clientId = created.client!.client_id as string; + + saveConsent(clientId, user.id, ['trips:read', 'trips:write']); + // approve only trips:read on a later request + saveConsent(clientId, user.id, ['trips:read']); + + const consent = getConsent(clientId, user.id); + // trips:write should NOT be removed (union semantics) + expect(consent).toContain('trips:read'); + expect(consent).toContain('trips:write'); + }); + + it('consent is sufficient after sequential approvals — no re-prompt needed', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id, { scopes: ['trips:read', 'budget:write'] }); + const clientId = created.client!.client_id as string; + + saveConsent(clientId, user.id, ['trips:read']); + saveConsent(clientId, user.id, ['budget:write']); + + // Should not require consent again for either scope + expect(isConsentSufficient(getConsent(clientId, user.id)!, ['trips:read'])).toBe(true); + expect(isConsentSufficient(getConsent(clientId, user.id)!, ['budget:write'])).toBe(true); + expect(isConsentSufficient(getConsent(clientId, user.id)!, ['trips:read', 'budget:write'])).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// C2 — getUserByAccessToken returns clientId +// --------------------------------------------------------------------------- + +describe('getUserByAccessToken — includes clientId (C2)', () => { + it('returns clientId matching the issuing OAuth client', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + + const { access_token } = issueTokens(clientId, user.id, ['trips:read']); + const info = getUserByAccessToken(access_token); + expect(info).not.toBeNull(); + expect(info!.clientId).toBe(clientId); + }); +}); + +// --------------------------------------------------------------------------- +// C3 — Refresh token replay detection and chain revocation +// --------------------------------------------------------------------------- + +describe('refreshTokens — replay detection (C3)', () => { + it('replaying a revoked refresh token returns invalid_grant', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + const rawSecret = created.client!.client_secret as string; + + // Issue tokens, then rotate once (old token becomes revoked) + const { refresh_token: firstRefresh } = issueTokens(clientId, user.id, ['trips:read']); + const rotateResult = refreshTokens(firstRefresh, clientId, rawSecret); + expect(rotateResult.error).toBeUndefined(); + const { refresh_token: secondRefresh } = rotateResult.tokens!; + + // Replay the FIRST (now revoked) refresh token + const replayResult = refreshTokens(firstRefresh, clientId, rawSecret); + expect(replayResult.error).toBe('invalid_grant'); + expect(replayResult.status).toBe(400); + }); + + it('replaying a revoked token also revokes the entire rotation chain', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + const rawSecret = created.client!.client_secret as string; + + // Issue → rotate once + const { refresh_token: first } = issueTokens(clientId, user.id, ['trips:read']); + const r1 = refreshTokens(first, clientId, rawSecret); + const { access_token: access2, refresh_token: second } = r1.tokens!; + + // Replay first (revoked) refresh token → chain revoke + refreshTokens(first, clientId, rawSecret); + + // The rotated access token should also be dead now + expect(getUserByAccessToken(access2)).toBeNull(); + + // The second refresh token should also be revoked + const r2 = refreshTokens(second, clientId, rawSecret); + expect(r2.error).toBe('invalid_grant'); + }); + + it('new rotation chain after replay is independent', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + const rawSecret = created.client!.client_secret as string; + + const { refresh_token: first } = issueTokens(clientId, user.id, ['trips:read']); + // Rotate once + const r1 = refreshTokens(first, clientId, rawSecret); + const { refresh_token: second } = r1.tokens!; + // Rotate again on the second token + const r2 = refreshTokens(second, clientId, rawSecret); + expect(r2.error).toBeUndefined(); + const { refresh_token: third } = r2.tokens!; + + // Replay the first revoked token → revokes chain containing first+second+third + refreshTokens(first, clientId, rawSecret); + + // third should now be revoked too (it's in the same chain) + const r3 = refreshTokens(third, clientId, rawSecret); + expect(r3.error).toBe('invalid_grant'); + }); +}); + +// --------------------------------------------------------------------------- +// H1 — PKCE code_challenge / code_verifier format validation +// --------------------------------------------------------------------------- + +describe('verifyPKCE — format validation (H1)', () => { + it('returns false for a code_verifier that is too short (< 43 chars)', () => { + const { challenge } = makePkce(); + expect(verifyPKCE('short', challenge)).toBe(false); + }); + + it('returns false for a code_verifier that is too long (> 128 chars)', () => { + const { challenge } = makePkce(); + const longVerifier = 'a'.repeat(129); + expect(verifyPKCE(longVerifier, challenge)).toBe(false); + }); + + it('returns false for a code_verifier with invalid characters', () => { + const { challenge } = makePkce(); + const badVerifier = 'A'.repeat(42) + ' '; // space is not allowed + expect(verifyPKCE(badVerifier, challenge)).toBe(false); + }); + + it('returns true for a valid 43-char verifier matching its challenge', () => { + const { verifier, challenge } = makePkce(); + expect(verifyPKCE(verifier, challenge)).toBe(true); + }); +}); + +describe('validateAuthorizeRequest — PKCE format (H1)', () => { + it('returns invalid_request when code_challenge is shorter than 43 chars', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + + const result = validateAuthorizeRequest({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://example.com/callback', + scope: 'trips:read', + code_challenge: 'tooshort', + code_challenge_method: 'S256', + }, user.id); + expect(result.valid).toBe(false); + expect(result.error).toBe('invalid_request'); + }); + + it('returns invalid_request when code_challenge contains invalid characters', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + + // 43 chars but includes '=' which is not base64url + const badChallenge = '='.repeat(43); + const result = validateAuthorizeRequest({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://example.com/callback', + scope: 'trips:read', + code_challenge: badChallenge, + code_challenge_method: 'S256', + }, user.id); + expect(result.valid).toBe(false); + expect(result.error).toBe('invalid_request'); + }); +}); + +// --------------------------------------------------------------------------- +// H3 — validateAuthorizeRequest: loginRequired response strips client info +// --------------------------------------------------------------------------- + +describe('validateAuthorizeRequest — unauthenticated strips client info (H3)', () => { + it('loginRequired response does not include client.name or allowed_scopes', () => { + const { user } = createUser(testDb); + const created = makeClient(user.id); + const clientId = created.client!.client_id as string; + const { challenge } = makePkce(); + + const result = validateAuthorizeRequest({ + response_type: 'code', + client_id: clientId, + redirect_uri: 'https://example.com/callback', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }, null /* unauthenticated */); + + expect(result.valid).toBe(true); + expect(result.loginRequired).toBe(true); + // Must NOT expose client metadata to unauthenticated callers + expect(result.client).toBeUndefined(); + expect(result.scopes).toBeUndefined(); + }); +});