From bfd79b94bcab16916f9d6eba21f7429c739f70df Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 13:27:17 +0300 Subject: [PATCH] fix(mcp): close SSO/MFA bypass on Basic + stop non-init session mint Release-cycle review found the /mcp Basic path skipped the controller's pre-token gates and over-eagerly minted sessions: - SSO/MFA bypass (blocker): the Basic path called AuthService.login/ verifyUserCredentials directly, but validateSsoEnforcement + the lazy EE MFA gate live in AuthController.login. Now enforceBasicLoginGate runs in the Basic branch BEFORE any token is minted: validateSsoEnforcement(workspace) (reject on enforced SSO) and the same lazy-require MFA check the controller uses (reject MFA users -> 'use a Bearer access token'). No EE module bundled (this fork) -> no MFA gate, identical to the controller; a throw from the check fails closed (no token). Bearer/service-account paths are not gated (those JWTs are minted post-gate). - Non-init session mint: isSessionInit is now (no mcp-session-id) AND the body is a real JSON-RPC initialize (isInitializeRequestBody). A header-less non-initialize request takes the side-effect-free verifyCredentials path -> no user_sessions row, no USER_LOGIN audit, no lastLoginAt bump. - FailedLoginLimiter.sweep() now runs on an unref'd 60s interval, cleared on module destroy (was never scheduled -> unbounded Map growth under XFF rotation). - Subsequent (non-init) valid login no longer resets the global per-email brute bucket (only per-IP / per-IP+email); the email backstop is reset only on a deliberate init login. Note: in a hypothetical EE build, checkMfaRequirements is called with no FastifyReply (we only read requirement flags); a res-dereferencing EE impl would surface as a clean rejection (fail-closed), not a bypass. Co-Authored-By: Claude Opus 4.8 --- .../src/integrations/mcp/mcp-auth.helpers.ts | 59 +++++- .../src/integrations/mcp/mcp.service.spec.ts | 184 ++++++++++++++++++ .../src/integrations/mcp/mcp.service.ts | 110 ++++++++++- 3 files changed, 345 insertions(+), 8 deletions(-) diff --git a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts index 6eb17ab0..506e62ea 100644 --- a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts +++ b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts @@ -116,6 +116,20 @@ export interface McpAuthDeps { email?: string; password?: string; findWorkspace: () => Promise<{ id: string } | undefined>; + // Pre-token gate for the Basic path ONLY, replicating what AuthController.login + // does BEFORE issuing a token: validateSsoEnforcement(workspace) and the lazy + // EE MFA requirement check. It is invoked with the resolved (default) + // workspace right after it is loaded and BEFORE any login()/verifyCredentials() + // call, so an SSO-enforced workspace or an MFA-required user never gets a token + // via /mcp Basic. It MUST throw (UnauthorizedException) to reject; on a fork + // without the EE MFA module bundled it behaves exactly like the controller + // (no MFA module -> no MFA gate). The Bearer path skips this gate because those + // ACCESS JWTs were already minted post-gate by the normal controller login. + // Optional so existing callers/tests that don't exercise the gate are unchanged. + enforceBasicGate?: ( + workspace: { id: string }, + creds: { email: string; password: string }, + ) => Promise | void; // Full login: mints a user session + JWT, writes the USER_LOGIN audit event // and updates lastLoginAt. Called at MOST once per MCP session (at the // session-init request) so we do not spam the audit log / user_sessions table @@ -227,6 +241,25 @@ export async function verifyBearerAccess( return { sub: payload.sub, email: payload.email }; } +/** + * Detect a genuine JSON-RPC `initialize` request from an already-parsed body. + * Mirrors the @modelcontextprotocol/sdk `isInitializeRequest` signal that + * packages/mcp/src/http.ts uses to decide whether to mint a session, but + * framework/SDK-free so it is unit-testable and usable from the CommonJS + * McpService. An initialize request is a single JSON-RPC object whose `method` + * is exactly 'initialize'; a batch (array) body is never an initialize request. + * + * This is the second half of the session-INIT decision: `isSessionInit` is + * (no `mcp-session-id` header) AND `isInitializeRequestBody(body)`. Using it + * ensures the side-effecting login() (user_sessions insert + USER_LOGIN audit + + * lastLoginAt) only runs for a real initialize, never for an arbitrary + * header-less request that http.ts will subsequently 400. + */ +export function isInitializeRequestBody(body: unknown): boolean { + if (!body || typeof body !== 'object' || Array.isArray(body)) return false; + return (body as { method?: unknown }).method === 'initialize'; +} + /** Extract a Bearer token from an Authorization header (case-insensitive). */ export function extractBearer( authHeader: string | undefined, @@ -283,6 +316,22 @@ export async function resolveMcpSessionConfig( throw new UnauthorizedException('No workspace is configured.'); } + // SSO/MFA pre-token gate (BLOCKER fix): replicate the AuthController.login + // gates BEFORE any token is issued on the Basic path. If the workspace + // enforces SSO, or the EE MFA module is bundled and this user/workspace + // requires MFA, this throws and we never mint a token. The Bearer path is + // intentionally NOT gated here (its JWT was already minted post-gate). This + // runs on BOTH init and subsequent Basic requests, but it must run before + // login()/verifyCredentials so an SSO/MFA user cannot authenticate at all. + // We do NOT count a gate rejection toward the brute-force limiter: it is not + // a password-guess signal. + if (deps.enforceBasicGate) { + await deps.enforceBasicGate(workspace, { + email: basic.email, + password: basic.password, + }); + } + // Fix 1 (init vs subsequent): // - SESSION INIT (no mcp-session-id): full login() mints the user JWT // (the one allowed session creation + audit event for this MCP @@ -330,10 +379,16 @@ export async function resolveMcpSessionConfig( : 'Email or password does not match'; throw new UnauthorizedException(message); } - // Subsequent request, credentials valid: clear any prior failure budget. + // Subsequent request, credentials valid: clear the per-IP and per-IP+email + // budget, but DELIBERATELY do NOT reset the GLOBAL per-email key here. That + // email key is the only brute-force backstop that survives IP/XFF rotation; + // resetting it on every periodic tool call of a victim's live MCP session + // would repeatedly wipe a parallel attacker's failed-login budget for that + // email. The global email key is reset ONLY on a session-INIT login() + // success (above), which is a single deliberate authentication, not a + // high-frequency re-validation. deps.limiter.reset(ipKey); deps.limiter.reset(ipEmailKey); - deps.limiter.reset(emailKey); return { config: { apiUrl, getToken: async () => '' }, identity: `basic:${emailLc}`, diff --git a/apps/server/src/integrations/mcp/mcp.service.spec.ts b/apps/server/src/integrations/mcp/mcp.service.spec.ts index 1b4a0969..48c44f7d 100644 --- a/apps/server/src/integrations/mcp/mcp.service.spec.ts +++ b/apps/server/src/integrations/mcp/mcp.service.spec.ts @@ -4,6 +4,7 @@ import { FailedLoginLimiter, resolveMcpSessionConfig, isCredentialsFailure, + isInitializeRequestBody, verifyBearerAccess, McpAuthDeps, } from './mcp-auth.helpers'; @@ -31,6 +32,10 @@ function makeDeps(over: Partial = {}): McpAuthDeps { verifyAccessJwt: over.verifyAccessJwt ?? jest.fn().mockResolvedValue({ sub: 'user-1', email: 'u@e.com' }), + // Default gate is a no-op (pass-through), matching a build with no SSO + // enforcement and no EE MFA module. Individual tests override it to assert + // the SSO/MFA reject behaviour. + enforceBasicGate: over.enforceBasicGate, limiter: over.limiter ?? new FailedLoginLimiter(5, 60_000), clientIp: over.clientIp ?? '10.0.0.1', // Default to the session-INIT request (no mcp-session-id) so existing @@ -393,4 +398,183 @@ describe('resolveMcpSessionConfig', () => { expect(b.identity).toBe('basic:bob@example.com'); expect(a.identity).not.toBe(b.identity); }); + + // --- BLOCKER: SSO/MFA pre-token gate on the Basic path --- + + it('Basic rejected (no token) when the SSO/MFA gate throws (SSO enforced)', async () => { + const login = jest.fn().mockResolvedValue('issued-user-jwt'); + const verifyCredentials = jest.fn().mockResolvedValue(undefined); + // The service wires enforceBasicGate to validateSsoEnforcement + the lazy + // MFA check. Here we stub it to throw as it would for an SSO-enforced + // workspace; the gate runs BEFORE login()/verifyCredentials, so no token. + const enforceBasicGate = jest + .fn() + .mockRejectedValue( + new UnauthorizedException('This workspace has enforced SSO login.'), + ); + await expect( + resolveMcpSessionConfig( + basicHeader('user@example.com', 'pw'), + makeDeps({ login, verifyCredentials, enforceBasicGate }), + ), + ).rejects.toThrow(/enforced SSO/); + expect(enforceBasicGate).toHaveBeenCalledWith( + { id: 'ws-1' }, + { email: 'user@example.com', password: 'pw' }, + ); + // The pre-token gate fired first: no token-minting login() and no + // verifyCredentials() happened. + expect(login).not.toHaveBeenCalled(); + expect(verifyCredentials).not.toHaveBeenCalled(); + }); + + it('Basic rejected with a "use a Bearer token" message when MFA is required', async () => { + const login = jest.fn().mockResolvedValue('issued-user-jwt'); + // Mirror McpService.enforceBasicLoginGate when the EE MFA module is present + // and the user has MFA: it throws telling the caller to use a Bearer token. + const enforceBasicGate = jest + .fn() + .mockRejectedValue( + new UnauthorizedException( + 'This account requires multi-factor authentication. MCP HTTP Basic ' + + 'cannot complete MFA — log in normally and use a Bearer access token ' + + 'instead.', + ), + ); + await expect( + resolveMcpSessionConfig( + basicHeader('mfa-user@example.com', 'pw'), + makeDeps({ login, enforceBasicGate }), + ), + ).rejects.toThrow(/use a Bearer access token/); + expect(login).not.toHaveBeenCalled(); + }); + + it('Bearer path is NOT subjected to the Basic SSO/MFA gate', async () => { + // The gate is only consulted on the Basic branch. A Bearer token (minted + // post-gate by the normal login) must not be blocked by it. + const enforceBasicGate = jest.fn(); + const resolved = await resolveMcpSessionConfig( + 'Bearer some.jwt.value', + makeDeps({ enforceBasicGate }), + ); + expect(enforceBasicGate).not.toHaveBeenCalled(); + expect('getToken' in resolved.config).toBe(true); + }); + + it('a session-INIT login() success DOES reset the global per-email key', async () => { + const limiter = new FailedLoginLimiter(5, 60_000); + // Pre-load some failure budget on the global email key. + const emailKey = 'email:victim@example.com'; + limiter.recordFailure(emailKey); + limiter.recordFailure(emailKey); + await resolveMcpSessionConfig( + basicHeader('victim@example.com', 'pw'), + makeDeps({ limiter, isSessionInit: true }), + ); + // After a real init login, the deliberate authentication clears the email + // bucket entirely. + expect(limiter.isBlocked(emailKey)).toBe(false); + limiter.recordFailure(emailKey); + // Only one failure now (bucket was reset), so still far from threshold 5. + expect(limiter.isBlocked(emailKey)).toBe(false); + }); + + it('a SUBSEQUENT valid login does NOT reset the global per-email bucket (only per-IP keys)', async () => { + const limiter = new FailedLoginLimiter(2, 60_000); + const clientIp = '10.0.0.5'; + const emailLc = 'victim@example.com'; + const emailKey = `email:${emailLc}`; + const ipKey = `ip:${clientIp}`; + const ipEmailKey = `ip-email:${clientIp}:${emailLc}`; + // An attacker (different IP rotation) has driven the global email key to the + // threshold; also seed the per-IP keys for the victim's own IP. + limiter.recordFailure(emailKey); + limiter.recordFailure(emailKey); + limiter.recordFailure(ipKey); + limiter.recordFailure(ipEmailKey); + + // The victim's live session would be throttled too (shared email key), so to + // exercise the SUBSEQUENT success path we use a SEPARATE limiter assertion: + // verify the reset behaviour directly on the keys the helper touches. Build a + // limiter where only the per-IP budget is set so the request is not blocked. + const lim2 = new FailedLoginLimiter(2, 60_000); + lim2.recordFailure(emailKey); // 1 failure on the global email key + lim2.recordFailure(ipKey); + lim2.recordFailure(ipEmailKey); + const verifyCredentials = jest.fn().mockResolvedValue(undefined); + await resolveMcpSessionConfig( + basicHeader(emailLc, 'pw'), + makeDeps({ limiter: lim2, clientIp, verifyCredentials, isSessionInit: false }), + ); + expect(verifyCredentials).toHaveBeenCalled(); + // Per-IP keys were cleared by the subsequent success... + expect(lim2.isBlocked(ipKey)).toBe(false); + // ...but the global per-email key was DELIBERATELY left intact (still 1). + lim2.recordFailure(emailKey); // -> 2 == threshold + expect(lim2.isBlocked(emailKey)).toBe(true); + }); +}); + +describe('isInitializeRequestBody (session-INIT detection)', () => { + it('true only for a single JSON-RPC object with method === "initialize"', () => { + expect(isInitializeRequestBody({ jsonrpc: '2.0', method: 'initialize' })).toBe( + true, + ); + }); + + it('false for a non-initialize method (e.g. tools/call)', () => { + expect( + isInitializeRequestBody({ jsonrpc: '2.0', method: 'tools/call' }), + ).toBe(false); + }); + + it('false for a batch (array) body, null/undefined, or a non-object', () => { + expect( + isInitializeRequestBody([{ jsonrpc: '2.0', method: 'initialize' }]), + ).toBe(false); + expect(isInitializeRequestBody(undefined)).toBe(false); + expect(isInitializeRequestBody(null)).toBe(false); + expect(isInitializeRequestBody('initialize')).toBe(false); + }); +}); + +describe('isSessionInit decision (no mcp-session-id AND initialize body)', () => { + // The service computes isSessionInit = !mcp-session-id && isInitializeRequestBody(body). + // This proves a header-less but NON-initialize request is NOT treated as init, + // so it goes down the non-side-effecting verifyCredentials path (no orphan + // session/audit before http.ts 400s it). + const decide = (sessionId: string | undefined, body: unknown): boolean => + !sessionId && isInitializeRequestBody(body); + + it('no header + initialize body -> init', () => { + expect(decide(undefined, { method: 'initialize' })).toBe(true); + }); + + it('no header + non-initialize body -> NOT init (verifyCredentials path)', () => { + expect(decide(undefined, { method: 'tools/list' })).toBe(false); + }); + + it('has session-id -> never init regardless of body', () => { + expect(decide('sess-1', { method: 'initialize' })).toBe(false); + }); +}); + +describe('resolveMcpSessionConfig non-initialize request side effects', () => { + it('header-less NON-initialize request does NOT call session-minting login() (uses verifyCredentials)', async () => { + // Simulate the service decision: no mcp-session-id but body is NOT initialize + // -> isSessionInit false -> the helper must use verifyCredentials, not login. + const login = jest.fn().mockResolvedValue('issued-user-jwt'); + const verifyCredentials = jest.fn().mockResolvedValue(undefined); + const isSessionInit = isInitializeRequestBody({ method: 'tools/call' }); // false + await resolveMcpSessionConfig( + basicHeader('user@example.com', 'pw'), + makeDeps({ login, verifyCredentials, isSessionInit }), + ); + expect(login).not.toHaveBeenCalled(); + expect(verifyCredentials).toHaveBeenCalledWith( + { email: 'user@example.com', password: 'pw' }, + 'ws-1', + ); + }); }); diff --git a/apps/server/src/integrations/mcp/mcp.service.ts b/apps/server/src/integrations/mcp/mcp.service.ts index a18e2d6d..54682309 100644 --- a/apps/server/src/integrations/mcp/mcp.service.ts +++ b/apps/server/src/integrations/mcp/mcp.service.ts @@ -1,8 +1,10 @@ import { Injectable, Logger, + OnModuleDestroy, UnauthorizedException, } from '@nestjs/common'; +import { ModuleRef } from '@nestjs/core'; import { pathToFileURL } from 'node:url'; import { timingSafeEqual } from 'node:crypto'; import { IncomingMessage } from 'node:http'; @@ -13,11 +15,14 @@ import { UserRepo } from '@docmost/db/repos/user/user.repo'; import { UserSessionRepo } from '@docmost/db/repos/session/user-session.repo'; import { AuthService } from '../../core/auth/services/auth.service'; import { TokenService } from '../../core/auth/services/token.service'; +import { validateSsoEnforcement } from '../../core/auth/auth.util'; import { JwtType, JwtPayload } from '../../core/auth/dto/jwt-payload'; +import { Workspace } from '@docmost/db/types/entity.types'; import { FailedLoginLimiter, resolveMcpSessionConfig, verifyBearerAccess, + isInitializeRequestBody, DocmostMcpConfig, ResolvedMcpAuth, } from './mcp-auth.helpers'; @@ -58,7 +63,7 @@ const esmImport = new Function( ) as (specifier: string) => Promise; @Injectable() -export class McpService { +export class McpService implements OnModuleDestroy { private readonly logger = new Logger(McpService.name); private handler: McpHttpHandler | null = null; private handlerPromise: Promise | null = null; @@ -69,6 +74,13 @@ export class McpService { // this is the brute-force speed bump for /mcp. 5 failures per 60s window. private readonly failedLogins = new FailedLoginLimiter(5, 60_000); + // Periodically drop expired limiter buckets so never-revisited keys do not + // accumulate forever (unbounded memory growth / DoS via forgeable XFF keys). + // unref()'d so it never keeps the process alive; cleared on module destroy. + // Mirrors the sweepTimer pattern in packages/mcp/src/http.ts. + private readonly sweepIntervalMs = 60_000; + private readonly sweepTimer: NodeJS.Timeout; + constructor( private readonly environmentService: EnvironmentService, private readonly workspaceRepo: WorkspaceRepo, @@ -76,7 +88,22 @@ export class McpService { private readonly tokenService: TokenService, private readonly userRepo: UserRepo, private readonly userSessionRepo: UserSessionRepo, - ) {} + private readonly moduleRef: ModuleRef, + ) { + this.sweepTimer = setInterval(() => { + try { + this.failedLogins.sweep(); + } catch (err) { + this.logger.error('MCP failed-login limiter sweep failed', err as Error); + } + }, this.sweepIntervalMs); + // Do not let this interval hold the event loop open. + this.sweepTimer.unref?.(); + } + + onModuleDestroy(): void { + clearInterval(this.sweepTimer); + } // Service account the embedded MCP uses to talk back to this Docmost // instance over loopback REST + the collaboration WebSocket. Now OPTIONAL: @@ -188,15 +215,24 @@ export class McpService { const authHeader = req.headers['authorization'] as string | undefined; // A request carrying an mcp-session-id is operating on an ALREADY // established session (see packages/mcp/src/http.ts: a new session is only - // minted by an initialize POST with no session id). Only the INIT request - // should run the full, session-minting login(); subsequent requests only - // re-validate credentials (anti-fixation) with no side effects. - const isSessionInit = !req.headers['mcp-session-id']; + // minted by an initialize POST with no session id). The session-minting + // login() (user_sessions insert + USER_LOGIN audit + lastLoginAt bump) must + // run ONLY for a genuine session INITIALIZE: no mcp-session-id AND the + // JSON-RPC body is an `initialize` request — the same signal http.ts uses to + // decide whether to mint a session. Any other request (e.g. a non-initialize + // body with no session id, which http.ts will 400) uses the non-side- + // effecting verifyCredentials path so it never mints an orphan DB + // session/audit row before being rejected. + const isSessionInit = + !req.headers['mcp-session-id'] && + isInitializeRequestBody((req as unknown as { body?: unknown }).body); return resolveMcpSessionConfig(authHeader, { apiUrl: this.getApiUrl(), email: this.getEmail(), password: this.getPassword(), findWorkspace: () => this.workspaceRepo.findFirst(), + enforceBasicGate: (workspace, creds) => + this.enforceBasicLoginGate(workspace as Workspace, creds), login: (creds, workspaceId) => this.authService.login(creds, workspaceId), verifyCredentials: async (creds, workspaceId) => { await this.authService.verifyUserCredentials(creds, workspaceId); @@ -208,6 +244,68 @@ export class McpService { }); } + // Pre-token gate for the /mcp HTTP-Basic path, replicating EXACTLY what + // AuthController.login does before issuing a token, so the Basic path is not + // an SSO/MFA bypass: + // 1) validateSsoEnforcement(workspace) — reject if the workspace enforces + // SSO (a password login is not allowed there). + // 2) Lazily require the EE MFA module (same pattern/path as the controller). + // If it is bundled and the user has MFA enabled OR the workspace enforces + // MFA, reject the Basic path and tell the caller to use a Bearer token (a + // Bearer ACCESS JWT is only minted AFTER the normal gated login, so it is + // safe). A fork WITHOUT the EE module behaves exactly like the controller: + // no MFA module -> no MFA gate. + // Throws UnauthorizedException on rejection (surfaced as a clean 401, never a + // torn/hijacked response, never a token). Never logs the password. + private async enforceBasicLoginGate( + workspace: Workspace, + creds: { email: string; password: string }, + ): Promise { + // 1) SSO enforcement. validateSsoEnforcement throws BadRequestException; we + // re-surface it as Unauthorized so the /mcp 401 path is consistent and a + // token is never issued. + try { + validateSsoEnforcement(workspace); + } catch { + throw new UnauthorizedException( + 'This workspace has enforced SSO login. Use SSO; MCP HTTP Basic is not allowed.', + ); + } + + // 2) MFA gate — lazy-require the EE module exactly like AuthController.login. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let MfaModule: any; + try { + // eslint-disable-next-line @typescript-eslint/no-require-imports + MfaModule = require('./../../ee/mfa/services/mfa.service'); + } catch { + // No EE MFA module bundled in this build: same as the controller -> no + // MFA gate. (A community/fork build has no MFA, so Basic is allowed.) + return; + } + + const mfaService = this.moduleRef.get(MfaModule.MfaService, { + strict: false, + }); + // Use the same requirement check the controller uses. We pass NO FastifyReply + // (the controller passes `res` only to set a cookie on the no-MFA happy path, + // which we never take here): we only read the requirement flags. Be tolerant + // of either a (loginInput, workspace) or (loginInput, workspace, res) shape. + const mfaResult = await mfaService.checkMfaRequirements( + creds, + workspace, + undefined, + ); + + if (mfaResult && (mfaResult.userHasMfa || mfaResult.requiresMfaSetup)) { + throw new UnauthorizedException( + 'This account requires multi-factor authentication. MCP HTTP Basic ' + + 'cannot complete MFA — log in normally and use a Bearer access token ' + + 'instead.', + ); + } + } + // Lazily create the HTTP handler exactly once. The import is indirected so // the ESM-only @docmost/mcp package can be loaded from this CommonJS module. // The handler is created with a per-request RESOLVER (and an `identify` hook