fix(mcp): security review follow-ups (#24) #48
10
.env.example
10
.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
|
||||
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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\$/);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
259
apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts
Normal file
259
apps/server/src/integrations/mcp/mcp-basic-login-gate.spec.ts
Normal file
@@ -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<void> } {
|
||||
// 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<void>;
|
||||
}
|
||||
).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();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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<JwtPayload>,
|
||||
expectedWorkspaceId: instanceWorkspace?.id,
|
||||
findUser: (sub, workspaceId) =>
|
||||
this.userRepo.findById(sub, workspaceId),
|
||||
findActiveSession: (sessionId) =>
|
||||
|
||||
Reference in New Issue
Block a user