diff --git a/.env.example b/.env.example index b04078e3..a19fd2d7 100644 --- a/.env.example +++ b/.env.example @@ -2,6 +2,16 @@ APP_URL=http://localhost:3000 PORT=3000 +# --- Security / reverse proxy --- +# The app runs with Fastify `trustProxy` ENABLED, so it derives the client IP +# (req.ip) from the `X-Forwarded-For` header. That header is client-forgeable. +# Deploy this app behind a trusted reverse proxy that SETS/OVERWRITES (not +# appends) `X-Forwarded-For` with the real client IP. Without such a proxy, any +# per-IP throttling — including the /mcp Basic brute-force limiter — can be +# bypassed by an attacker who simply spoofs `X-Forwarded-For` to rotate IPs. +# (The /mcp limiter keeps a global per-email key as an IP-independent backstop, +# but the per-IP and per-IP+email keys rely on a trustworthy X-Forwarded-For.) + # minimum of 32 characters. Generate one with: openssl rand -hex 32 APP_SECRET=REPLACE_WITH_LONG_SECRET diff --git a/apps/server/src/core/auth/services/auth.service.ts b/apps/server/src/core/auth/services/auth.service.ts index b27df4bc..1c952f6e 100644 --- a/apps/server/src/core/auth/services/auth.service.ts +++ b/apps/server/src/core/auth/services/auth.service.ts @@ -41,6 +41,20 @@ import { } from '../../../integrations/audit/audit.service'; import { EnvironmentService } from '../../../integrations/environment/environment.service'; +// A valid bcrypt hash (cost 10, of an arbitrary throwaway string) used ONLY to +// equalize timing in verifyUserCredentials: when the email does not exist or +// the user is disabled, we still run ONE bcrypt comparison against this hash +// before throwing, so the missing/disabled path takes about the same time as +// the real-user wrong-password path. Without it, the "no bcrypt at all" branch +// returns measurably faster, leaking whether an email is registered (a user- +// enumeration timing oracle, now reachable via /mcp where throttling is only a +// spoofable in-memory limiter). This is never used as a real credential. +// The cost factor MUST match the production saltRounds (12 — see +// common/helpers/utils.ts hashPassword), otherwise the dummy compare runs +// faster than a real wrong-password compare and the timing oracle survives. +const DUMMY_PASSWORD_HASH = + '$2b$12$q/l637TULK3vU3Cmji0y8utpJS/UiftMi3Jdm4Tsi5EIv/0FE7WV.'; + @Injectable() export class AuthService { constructor( @@ -82,6 +96,12 @@ export class AuthService { // recognises this exact message via isCredentialsFailure. const errorMessage = CREDENTIALS_MISMATCH_MESSAGE; if (!user || isUserDisabled(user)) { + // Constant-time intent: run ONE bcrypt comparison (against a dummy hash) + // even when the user is missing/disabled, so this path takes about the + // same time as the real-user wrong-password path below. This closes the + // user-enumeration timing oracle (registered vs. not). The result is + // intentionally discarded — we always throw the same credentials error. + await comparePasswordHash(loginDto.password, DUMMY_PASSWORD_HASH); throw new UnauthorizedException(errorMessage); } diff --git a/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts b/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts index 30689bd6..e7b37e08 100644 --- a/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts +++ b/apps/server/src/core/auth/services/verify-user-credentials.contract.spec.ts @@ -100,4 +100,40 @@ describe('AuthService no-side-effect contract (item 4)', () => { expect(verifyBody.includes(effect)).toBe(false); } }); + + // Item 4: user-enumeration timing-oracle fix. When the email is missing or the + // user is disabled, verifyUserCredentials must still run ONE bcrypt comparison + // (against a dummy hash) BEFORE throwing, so the missing/disabled path takes + // about the same time as the real-user wrong-password path. Asserted at the + // source level for the same reason as the rest of this file: AuthService cannot + // be imported under this jest config to spy on comparePasswordHash live. + describe('constant-time missing/disabled branch (item 4)', () => { + // Isolate the body of the `if (!user || isUserDisabled(user)) { ... }` guard. + const guardMatch = verifyBody.match( + /if \(!user \|\| isUserDisabled\(user\)\) \{([\s\S]*?)\n {4}\}/, + ); + + it('the missing/disabled guard runs a bcrypt compare before throwing', () => { + expect(guardMatch).not.toBeNull(); + const guardBody = guardMatch![1]; + // It performs the dummy bcrypt comparison... + expect(guardBody).toContain('comparePasswordHash'); + // ...and only AFTER that throws the credentials error (compare precedes + // the throw STATEMENT — match `throw new`, not the word "throw" in a comment). + const compareIdx = guardBody.indexOf('comparePasswordHash'); + const throwIdx = guardBody.indexOf('throw new'); + expect(compareIdx).toBeGreaterThanOrEqual(0); + expect(throwIdx).toBeGreaterThan(compareIdx); + }); + + it('uses a module-level dummy hash constant (never a real credential)', () => { + // The dummy hash is a module-level constant referenced in the guard, not an + // inline literal recomputed per call. + expect(verifyBody).toContain('DUMMY_PASSWORD_HASH'); + // Cost factor MUST be 12 to match production saltRounds, otherwise the + // dummy compare is faster than a real wrong-password compare and the + // timing oracle survives. + expect(source).toMatch(/const DUMMY_PASSWORD_HASH =\s*'\$2b\$12\$/); + }); + }); }); diff --git a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts index 0d1237e7..c3aa3292 100644 --- a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts +++ b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts @@ -5,6 +5,7 @@ // the Authorization header. import { UnauthorizedException } from '@nestjs/common'; import { timingSafeEqual } from 'node:crypto'; +import { isInitializeRequest } from '@modelcontextprotocol/sdk/types.js'; import { JwtType } from '../../core/auth/dto/jwt-payload'; import { CREDENTIALS_MISMATCH_MESSAGE } from '../../core/auth/auth.constants'; @@ -291,6 +292,14 @@ export interface BearerVerifyDeps { workspaceId?: string; sessionId?: string; }>; + // The workspace id of THIS MCP instance, when the caller can resolve it (the + // community build is single-workspace, so McpService passes its default + // workspace's id). When provided, the token's `workspaceId` claim MUST equal + // it, mirroring JwtStrategy's `req.raw.workspaceId !== payload.workspaceId` + // guard so a valid ACCESS token from a DIFFERENT workspace cannot be replayed + // against this instance in a multi-workspace deployment. Optional so callers / + // tests that genuinely cannot resolve an instance workspace are unchanged. + expectedWorkspaceId?: string; // Load the user (or undefined) for the disabled check. findUser: ( sub: string, @@ -321,6 +330,19 @@ export async function verifyBearerAccess( throw new UnauthorizedException(generic); } + // Bind the token to THIS instance's workspace (mirrors JwtStrategy). When the + // caller resolved an instance workspace id, a token whose `workspaceId` claim + // points at another workspace is rejected, so a valid ACCESS token minted in + // workspace B cannot be replayed against an MCP instance serving workspace A. + // In the single-workspace community build expectedWorkspaceId equals the only + // workspace, so this is a no-op there; it only bites a multi-workspace deploy. + if ( + deps.expectedWorkspaceId && + payload.workspaceId !== deps.expectedWorkspaceId + ) { + throw new UnauthorizedException(generic); + } + const user = await deps.findUser(payload.sub, payload.workspaceId); if (!user || user.deactivatedAt || user.deletedAt) { throw new UnauthorizedException(generic); @@ -342,21 +364,24 @@ export async function verifyBearerAccess( /** * 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. + * Delegates to the @modelcontextprotocol/sdk `isInitializeRequest` predicate — + * the SAME predicate packages/mcp/src/http.ts uses to decide whether to mint a + * session — so the session-minting side (this server) and the session-creating + * side (http.ts) agree EXACTLY on what counts as an initialize request. The SDK + * predicate validates the full InitializeRequest shape (jsonrpc, id, method === + * 'initialize', params incl. protocolVersion); a bare `{ method: 'initialize' }` + * with no params, a batch (array) body, etc. are NOT initialize requests. * * 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. + * (no `mcp-session-id` header) AND `isInitializeRequestBody(body)`. Matching the + * SDK predicate exactly ensures the side-effecting login() (user_sessions insert + * + USER_LOGIN audit + lastLoginAt) only runs for a request http.ts will also + * accept as an initialize — never for an arbitrary header-less request that + * http.ts would subsequently 400 (which would otherwise spam the audit log / + * grow user_sessions without ever creating an MCP session). */ export function isInitializeRequestBody(body: unknown): boolean { - if (!body || typeof body !== 'object' || Array.isArray(body)) return false; - return (body as { method?: unknown }).method === 'initialize'; + return isInitializeRequest(body); } /** diff --git a/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts b/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts new file mode 100644 index 00000000..351b467b --- /dev/null +++ b/apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts @@ -0,0 +1,259 @@ +import { UnauthorizedException } from '@nestjs/common'; + +// --------------------------------------------------------------------------- +// These tests exercise the REAL McpService.enforceBasicLoginGate (the pre-token +// SSO/MFA gate on the /mcp HTTP-Basic path). Unlike the resolveMcpSessionConfig +// tests in mcp.service.spec.ts — which STUB the gate and only assert it runs +// before login()/verifyCredentials — here the gate logic is instantiated for +// real and only its LEAF dependencies are mocked: +// - the workspace object (plain object with/without enforceSso), +// - the user credentials (plain object), +// - the lazily-required EE MFA module (jest.mock with { virtual: true } so we +// can simulate BOTH "bundled" and "not bundled" community-build states), +// - the injected MfaService instance (via a stub moduleRef). +// +// McpService cannot normally be imported under jest because it imports +// AuthService, which drags in the React email-template graph +// (@docmost/transactional/emails/*) that the jest moduleNameMapper does not +// resolve. We therefore mock the heavy collaborator modules (auth.service, +// token.service, the @docmost/db repos and mcp-auth.helpers) at the module +// level so importing mcp.service.ts succeeds. None of those are touched by the +// gate itself, so the gate runs unmodified against the real code path. +// --------------------------------------------------------------------------- + +// The EE MFA module specifier the jest.mock below intercepts MUST be +// byte-for-byte the specifier that mcp.service.ts lazily require()s +// ('./../../ee/mfa/services/mfa.service'). jest.mock is hoisted above all +// non-hoisted code, so the path is inlined as a literal in the call below +// rather than referenced through a const (which would not yet be initialised). +// `{ virtual: true }` is required because the EE module does not exist in this +// OSS build (there is no src/ee directory) — without it jest cannot register a +// mock for a path it cannot resolve on disk. + +// Mutable handle the virtual mock factory reads, so each test can decide whether +// the EE module is "bundled" (factory returns a MfaService class) or "not +// bundled" (factory throws, mimicking the require() failing on a community +// build). jest.mock is hoisted, so the factory must close over this lazily. +let mfaModuleState: { bundled: boolean; checkMfaRequirements?: jest.Mock } = { + bundled: false, +}; + +jest.mock( + './../../ee/mfa/services/mfa.service', + () => { + if (!mfaModuleState.bundled) { + // Simulate a community/fork build with no EE MFA module: the real + // require() throws, which the gate catches as the "no MFA gate" path. + throw new Error('Cannot find module (EE MFA not bundled)'); + } + // "Bundled" build: expose a MfaService class token. The actual instance the + // gate calls is resolved through moduleRef.get(MfaModule.MfaService), which + // our stub moduleRef returns regardless of the token identity. + class MfaService {} + return { MfaService }; + }, + { virtual: true }, +); + +// --- Mock the heavy collaborator modules so importing mcp.service succeeds. --- +// The gate never calls into these; they exist only to satisfy the import graph. +jest.mock('../../core/auth/services/auth.service', () => ({ + AuthService: class AuthService {}, +})); +jest.mock('../../core/auth/services/token.service', () => ({ + TokenService: class TokenService {}, +})); +jest.mock('@docmost/db/repos/workspace/workspace.repo', () => ({ + WorkspaceRepo: class WorkspaceRepo {}, +})); +jest.mock('@docmost/db/repos/user/user.repo', () => ({ + UserRepo: class UserRepo {}, +})); +jest.mock('@docmost/db/repos/session/user-session.repo', () => ({ + UserSessionRepo: class UserSessionRepo {}, +})); +// mcp-auth.helpers exports runtime values the gate relies on (decideBasicGate, +// mapAuthResultToResponse, etc.). Keep the REAL helpers so the gate exercises +// real logic; only stub FailedLoginLimiter so its constructor runs without a +// real sweep timer. The module is framework-free and loads cleanly under jest +// (mcp.service.spec.ts already imports it directly), so requireActual is safe. +jest.mock('./mcp-auth.helpers', () => { + const actual = jest.requireActual('./mcp-auth.helpers'); + return { + ...actual, + FailedLoginLimiter: class FailedLoginLimiter { + sweep() {} + }, + }; +}); + +// Import AFTER the mocks are registered. +// eslint-disable-next-line @typescript-eslint/no-require-imports +import { McpService } from './mcp.service'; + +type GateCreds = { email: string; password: string }; + +// Build an McpService instance with stubbed constructor deps. We never call the +// auth/db collaborators from the gate, so undefined stand-ins are fine for all +// but moduleRef, which the MFA branch reads. +function makeService(opts: { + checkMfaRequirements?: jest.Mock; +}): { service: McpService; gate: (ws: unknown, creds: GateCreds) => Promise } { + // Stub moduleRef.get -> returns an object whose checkMfaRequirements is the + // provided mock. The gate calls moduleRef.get(MfaModule.MfaService). + const moduleRef = { + get: jest.fn().mockReturnValue({ + checkMfaRequirements: + opts.checkMfaRequirements ?? jest.fn().mockResolvedValue(undefined), + }), + }; + + const service = new McpService( + undefined as never, // environmentService + undefined as never, // workspaceRepo + undefined as never, // authService + undefined as never, // tokenService + undefined as never, // userRepo + undefined as never, // userSessionRepo + moduleRef as never, // moduleRef (read by the MFA branch) + ); + // Stop the constructor's unref'd sweep timer leaking across tests. + service.onModuleDestroy(); + + // enforceBasicLoginGate is private; reach it through the instance. Calling the + // REAL method (not a stub) is the whole point of this suite. + const gate = ( + service as unknown as { + enforceBasicLoginGate: (ws: unknown, creds: GateCreds) => Promise; + } + ).enforceBasicLoginGate.bind(service); + + return { service, gate }; +} + +const CREDS: GateCreds = { email: 'user@example.com', password: 'pw' }; + +describe('McpService.enforceBasicLoginGate (REAL gate, leaf deps mocked)', () => { + beforeEach(() => { + // Reset to the community-build default (no EE module) before each test. + mfaModuleState = { bundled: false }; + jest.clearAllMocks(); + }); + + describe('SSO enforcement (validateSsoEnforcement)', () => { + it('rejects with Unauthorized when the workspace enforces SSO, before any MFA/login', async () => { + const { gate } = makeService({}); + const workspace = { id: 'ws-1', enforceSso: true }; + + await expect(gate(workspace, CREDS)).rejects.toBeInstanceOf( + UnauthorizedException, + ); + // The /mcp 401 surfaces an SSO-specific message (not a generic MCP error). + await expect(gate(workspace, CREDS)).rejects.toThrow(/enforced SSO/i); + }); + + it('does NOT consult the MFA module when SSO is enforced (gate short-circuits)', async () => { + // Even if the EE module WERE bundled, the SSO branch throws first, so the + // moduleRef MFA lookup must never run. + mfaModuleState = { + bundled: true, + checkMfaRequirements: jest.fn(), + }; + const { service, gate } = makeService({ + checkMfaRequirements: mfaModuleState.checkMfaRequirements, + }); + const moduleRefGet = ( + service as unknown as { moduleRef: { get: jest.Mock } } + ).moduleRef.get; + + await expect( + gate({ id: 'ws-1', enforceSso: true }, CREDS), + ).rejects.toThrow(/enforced SSO/i); + // The SSO branch fired before the MFA require/lookup. + expect(moduleRefGet).not.toHaveBeenCalled(); + expect(mfaModuleState.checkMfaRequirements).not.toHaveBeenCalled(); + }); + }); + + describe('community build: EE MFA module NOT bundled', () => { + it('passes (no throw) when SSO is not enforced and the lazy require fails (no MFA gate)', async () => { + // mfaModuleState.bundled === false -> the virtual mock factory throws, + // exactly like require() of a missing EE module on a community build. + const { service, gate } = makeService({}); + const moduleRefGet = ( + service as unknown as { moduleRef: { get: jest.Mock } } + ).moduleRef.get; + + await expect( + gate({ id: 'ws-1', enforceSso: false }, CREDS), + ).resolves.toBeUndefined(); + // The require() failed, so the gate returned before touching moduleRef. + expect(moduleRefGet).not.toHaveBeenCalled(); + }); + }); + + describe('EE MFA module bundled', () => { + it('rejects with a "use a Bearer token" signal when the user has MFA enabled', async () => { + const check = jest.fn().mockResolvedValue({ + userHasMfa: true, + requiresMfaSetup: false, + }); + mfaModuleState = { bundled: true, checkMfaRequirements: check }; + const { gate } = makeService({ checkMfaRequirements: check }); + + const promise = gate({ id: 'ws-1', enforceSso: false }, CREDS); + await expect(promise).rejects.toBeInstanceOf(UnauthorizedException); + await expect( + gate({ id: 'ws-1', enforceSso: false }, CREDS), + ).rejects.toThrow(/Bearer access token/i); + // The real requirement check was consulted with the creds + workspace. + expect(check).toHaveBeenCalledWith( + CREDS, + { id: 'ws-1', enforceSso: false }, + undefined, + ); + }); + + it('rejects when the workspace enforces MFA (requiresMfaSetup)', async () => { + // requiresMfaSetup === true models a workspace that enforces MFA for a + // user who has not set it up yet; the Basic path cannot complete it. + const check = jest.fn().mockResolvedValue({ + userHasMfa: false, + requiresMfaSetup: true, + }); + mfaModuleState = { bundled: true, checkMfaRequirements: check }; + const { gate } = makeService({ checkMfaRequirements: check }); + + await expect( + gate({ id: 'ws-1', enforceSso: false }, CREDS), + ).rejects.toThrow(/Bearer access token/i); + }); + + it('passes when the user has no MFA and the workspace does not enforce it', async () => { + const check = jest.fn().mockResolvedValue({ + userHasMfa: false, + requiresMfaSetup: false, + }); + mfaModuleState = { bundled: true, checkMfaRequirements: check }; + const { gate } = makeService({ checkMfaRequirements: check }); + + await expect( + gate({ id: 'ws-1', enforceSso: false }, CREDS), + ).resolves.toBeUndefined(); + // The bundled module's requirement check WAS consulted (proving we took + // the bundled branch, not the community no-op branch). + expect(check).toHaveBeenCalledTimes(1); + }); + + it('passes when checkMfaRequirements returns a falsy result (no requirement flags)', async () => { + // Defensive: a bundled module that returns undefined must not reject. + const check = jest.fn().mockResolvedValue(undefined); + mfaModuleState = { bundled: true, checkMfaRequirements: check }; + const { gate } = makeService({ checkMfaRequirements: check }); + + await expect( + gate({ id: 'ws-1', enforceSso: false }, CREDS), + ).resolves.toBeUndefined(); + }); + }); +}); diff --git a/apps/server/src/integrations/mcp/mcp.service.spec.ts b/apps/server/src/integrations/mcp/mcp.service.spec.ts index e8a57748..cfa8472d 100644 --- a/apps/server/src/integrations/mcp/mcp.service.spec.ts +++ b/apps/server/src/integrations/mcp/mcp.service.spec.ts @@ -324,6 +324,31 @@ describe('verifyBearerAccess (Bearer revocation/disabled checks)', () => { ), ).rejects.toThrow('jwt expired'); }); + + // Item 3: bind the Bearer token to THIS instance's workspace (mirrors + // JwtStrategy). A token whose workspaceId claim differs from the instance + // workspace must be rejected; matching/absent expectedWorkspaceId is allowed. + it('rejects a token from a DIFFERENT workspace when expectedWorkspaceId is set', async () => { + await expect( + verifyBearerAccess('t', { + ...bearerDeps(), + expectedWorkspaceId: 'ws-OTHER', + }), + ).rejects.toThrow(UnauthorizedException); + }); + + it('accepts a token whose workspace matches expectedWorkspaceId', async () => { + const res = await verifyBearerAccess('t', { + ...bearerDeps(), + expectedWorkspaceId: 'ws-1', + }); + expect(res).toEqual({ sub: 'user-1', email: 'u@e.com' }); + }); + + it('does NOT enforce a workspace when expectedWorkspaceId is undefined (single-workspace no-op)', async () => { + const res = await verifyBearerAccess('t', bearerDeps()); + expect(res).toEqual({ sub: 'user-1', email: 'u@e.com' }); + }); }); describe('resolveMcpSessionConfig', () => { @@ -647,23 +672,48 @@ describe('resolveMcpSessionConfig', () => { }); }); -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, - ); +// A full, valid JSON-RPC InitializeRequest as the @modelcontextprotocol/sdk +// `isInitializeRequest` predicate (which isInitializeRequestBody now delegates +// to) requires: jsonrpc + id + method === 'initialize' + params.protocolVersion. +const fullInitializeRequest = { + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2024-11-05', + capabilities: {}, + clientInfo: { name: 'test-client', version: '1.0.0' }, + }, +}; + +describe('isInitializeRequestBody (session-INIT detection, matches SDK predicate)', () => { + it('true for a FULL valid InitializeRequest (the SDK predicate signal)', () => { + expect(isInitializeRequestBody(fullInitializeRequest)).toBe(true); + }); + + it('false for a bare { method: "initialize" } with no id/params (item 1)', () => { + // Item 1: this previously returned true (method-only check) and let an + // authenticated client POST a params-less body with no mcp-session-id, which + // ran the side-effecting login() before http.ts 400'd it. The SDK predicate + // rejects it (no id, no params.protocolVersion), so it no longer mints a + // session / audit row. + expect(isInitializeRequestBody({ method: 'initialize' })).toBe(false); + expect( + isInitializeRequestBody({ jsonrpc: '2.0', method: 'initialize' }), + ).toBe(false); + expect( + isInitializeRequestBody({ jsonrpc: '2.0', id: 1, method: 'initialize', params: {} }), + ).toBe(false); }); it('false for a non-initialize method (e.g. tools/call)', () => { expect( - isInitializeRequestBody({ jsonrpc: '2.0', method: 'tools/call' }), + isInitializeRequestBody({ ...fullInitializeRequest, 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([fullInitializeRequest])).toBe(false); expect(isInitializeRequestBody(undefined)).toBe(false); expect(isInitializeRequestBody(null)).toBe(false); expect(isInitializeRequestBody('initialize')).toBe(false); @@ -678,8 +728,14 @@ describe('isSessionInit decision (no mcp-session-id AND initialize body)', () => 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 + full initialize body -> init', () => { + expect(decide(undefined, fullInitializeRequest)).toBe(true); + }); + + it('no header + bare params-less initialize body -> NOT init (item 1)', () => { + // A header-less { method: 'initialize' } with no params is no longer treated + // as an init by the SDK predicate, so it does not mint a session via login(). + expect(decide(undefined, { method: 'initialize' })).toBe(false); }); it('no header + non-initialize body -> NOT init (verifyCredentials path)', () => { @@ -687,7 +743,7 @@ describe('isSessionInit decision (no mcp-session-id AND initialize body)', () => }); it('has session-id -> never init regardless of body', () => { - expect(decide('sess-1', { method: 'initialize' })).toBe(false); + expect(decide('sess-1', fullInitializeRequest)).toBe(false); }); }); diff --git a/apps/server/src/integrations/mcp/mcp.service.ts b/apps/server/src/integrations/mcp/mcp.service.ts index 0af88c65..cb746b90 100644 --- a/apps/server/src/integrations/mcp/mcp.service.ts +++ b/apps/server/src/integrations/mcp/mcp.service.ts @@ -156,6 +156,15 @@ export class McpService implements OnModuleDestroy { private async verifyMcpBearer( token: string, ): Promise<{ sub?: string; email?: string }> { + // Resolve THIS instance's workspace so verifyBearerAccess can bind the + // token's `workspaceId` claim to it (mirrors JwtStrategy). The community + // build is single-workspace (findFirst), so this is the default workspace + // and the check is a no-op here; it only rejects a foreign-workspace token + // in a multi-workspace deployment. Undefined (no workspace configured) means + // no check — the credentials path would already have failed with no + // workspace, and an undefined here keeps the helper a no-op rather than + // rejecting every token. + const instanceWorkspace = await this.workspaceRepo.findFirst(); // The revocation/disabled decision logic lives in the framework-free // verifyBearerAccess helper (unit-testable without the heavy auth graph); // this method only wires in the concrete TokenService + repos. @@ -165,6 +174,7 @@ export class McpService implements OnModuleDestroy { verifyJwt: bindAccessJwtVerifier(this.tokenService) as ( t: string, ) => Promise, + expectedWorkspaceId: instanceWorkspace?.id, findUser: (sub, workspaceId) => this.userRepo.findById(sub, workspaceId), findActiveSession: (sessionId) =>