Merge remote-tracking branch 'gitea/develop' into fix/mcp-security-followups

apps/server/src/integrations/ai/ai-error.util.spec.ts

@@ -58,4 +58,26 @@ describe('describeProviderError', () => {
     // 'e | response body: ' + 300 chars + '…'
     expect(out.length).toBeLessThan('e | response body: '.length + 305);
   });
+
+  it('uses the fallback for a numeric or boolean (non-object, non-string) error', () => {
+    // typeof number / boolean is neither 'object' nor a non-empty 'string', so
+    // the early branch returns the fallback verbatim.
+    expect(describeProviderError(500, 'AI stream error')).toBe('AI stream error');
+    expect(describeProviderError(0, 'AI stream error')).toBe('AI stream error');
+    expect(describeProviderError(true)).toBe('Unknown error');
+    expect(describeProviderError(false, 'fb')).toBe('fb');
+  });
+
+  it('statusCode present but message undefined => "<code>:" with no trailing space', () => {
+    // `${code}: ${undefined ?? ''}`.trim() collapses to just "<code>:".
+    expect(describeProviderError({ statusCode: 503 })).toBe('503:');
+    // The trailing space after the colon is trimmed away.
+    expect(describeProviderError({ statusCode: 503 }).endsWith(': ')).toBe(false);
+  });
+
+  it('object with neither message nor statusCode nor body => fallback', () => {
+    expect(describeProviderError({}, 'AI stream error')).toBe('AI stream error');
+    // An object carrying only unrelated keys is still treated as message-less.
+    expect(describeProviderError({ foo: 'bar' } as never)).toBe('Unknown error');
+  });
 });

apps/server/src/integrations/ai/ai.service.spec.ts

@@ -171,4 +171,117 @@ describe('AiService.getChatModel role model override', () => {
     expect(aiProviderCredentialsRepo.find).not.toHaveBeenCalled();
     expect(secretBox.decryptSecret).not.toHaveBeenCalled();
   });
+
+  /**
+   * Build a service whose workspace driver is ollama (no apiKey, with a baseUrl).
+   * Complements makeService (which configures openai) for the same-driver and
+   * not-configured ollama cases.
+   */
+  function makeOllamaService(over: { baseUrl?: string } = {}) {
+    const aiSettings = {
+      resolve: jest.fn().mockResolvedValue({
+        driver: 'ollama',
+        chatModel: 'llama3',
+        apiKey: undefined,
+        baseUrl: over.baseUrl ?? 'http://localhost:11434/v1',
+      }),
+    };
+    const aiProviderCredentialsRepo = { find: jest.fn() };
+    const secretBox = { decryptSecret: jest.fn() };
+    const service = new AiService(
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiSettings as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiProviderCredentialsRepo as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      secretBox as any,
+    );
+    return { service, aiSettings, aiProviderCredentialsRepo, secretBox };
+  }
+
+  it('same-driver ollama override (workspace driver=ollama): reuses the workspace ollama baseUrl, no creds lookup/decrypt', async () => {
+    // Workspace driver IS ollama. A role that overrides to ollama (same driver)
+    // legitimately reuses the workspace's configured ollama endpoint — it must
+    // NOT hit the cross-driver 503 path, NOT query ai_provider_credentials, and
+    // NOT decrypt anything (ollama needs no key).
+    const { service, aiProviderCredentialsRepo, secretBox } = makeOllamaService();
+
+    const model = await service.getChatModel('ws-1', {
+      driver: 'ollama',
+      chatModel: 'llama3.1',
+      roleName: 'Local',
+    });
+
+    expect(model).toBeDefined();
+    expect(aiProviderCredentialsRepo.find).not.toHaveBeenCalled();
+    expect(secretBox.decryptSecret).not.toHaveBeenCalled();
+  });
+
+  it('chatModel-only override on an ollama workspace: reuses the workspace ollama baseUrl, no creds lookup', async () => {
+    // No override.driver on an ollama workspace => the workspace ollama driver +
+    // baseUrl are reused; no creds lookup, no decrypt (the cheap public-share
+    // model-only override path against an ollama workspace).
+    const { service, aiProviderCredentialsRepo, secretBox } = makeOllamaService();
+
+    const model = await service.getChatModel('ws-1', { chatModel: 'mistral' });
+
+    expect(model).toBeDefined();
+    expect(aiProviderCredentialsRepo.find).not.toHaveBeenCalled();
+    expect(secretBox.decryptSecret).not.toHaveBeenCalled();
+  });
+
+  it('blank chatModel guard: workspace has a driver but a blank chatModel and no override chatModel => AiNotConfiguredException', async () => {
+    // cfg.driver passes the first guard, but cfg.chatModel is blank and the
+    // override carries no chatModel, so the effective chatModel is empty.
+    const aiSettings = {
+      resolve: jest.fn().mockResolvedValue({
+        driver: 'openai',
+        chatModel: '',
+        apiKey: 'workspace-key',
+        baseUrl: undefined,
+      }),
+    };
+    const aiProviderCredentialsRepo = { find: jest.fn() };
+    const secretBox = { decryptSecret: jest.fn() };
+    const service = new AiService(
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiSettings as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiProviderCredentialsRepo as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      secretBox as any,
+    );
+
+    await expect(
+      // Override has only a roleName, no chatModel to fill the blank.
+      service.getChatModel('ws-1', { roleName: 'Writer' }),
+    ).rejects.toBeInstanceOf(AiNotConfiguredException);
+  });
+
+  it('non-ollama driver with a missing apiKey => AiNotConfiguredException', async () => {
+    // Workspace is openai (non-ollama) with a model but NO apiKey: the combined
+    // `driver !== ollama && !apiKey` guard must 503.
+    const aiSettings = {
+      resolve: jest.fn().mockResolvedValue({
+        driver: 'openai',
+        chatModel: 'gpt-4o-mini',
+        apiKey: undefined,
+        baseUrl: undefined,
+      }),
+    };
+    const aiProviderCredentialsRepo = { find: jest.fn() };
+    const secretBox = { decryptSecret: jest.fn() };
+    const service = new AiService(
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiSettings as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      aiProviderCredentialsRepo as any,
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      secretBox as any,
+    );
+
+    await expect(service.getChatModel('ws-1')).rejects.toBeInstanceOf(
+      AiNotConfiguredException,
+    );
+  });
 });

apps/server/src/integrations/mcp/mcp-auth.helpers.ts

@@ -384,6 +384,111 @@ export function isInitializeRequestBody(body: unknown): boolean {
   return isInitializeRequest(body);
 }
 
+/**
+ * The outcome of McpService.handle's pre-hijack gauntlet, as a pure value the
+ * caller acts on. Either send a JSON error with a fixed status (`respond`), or
+ * proceed to hijack the response and delegate to the MCP transport (`hijack`).
+ * Keeping this a pure decision (no FastifyReply, no res.hijack) makes the
+ * status/body mapping unit-testable, and guarantees no error path can leak the
+ * password or Authorization header — the body is only ever a fixed string or the
+ * UnauthorizedException's own message.
+ */
+export type McpHandleDecision =
+  | { kind: 'respond'; status: number; body: { error: string } }
+  | { kind: 'hijack' };
+
+/**
+ * Pure mapping of McpService.handle's auth/enablement gauntlet to a response
+ * decision. Precedence mirrors handle():
+ *   1. shared X-MCP-Token mismatch -> 401 {error:'Unauthorized'} (no hijack).
+ *   2. workspace MCP disabled      -> 403 {error:'MCP is disabled ...'}.
+ *   3. resolveSessionConfig threw:
+ *        - an UnauthorizedException -> 401 with err.message (a SPECIFIC reason;
+ *          never the password/header — the message is the only thing surfaced).
+ *        - any other error          -> 500 generic 'Internal server error'.
+ *   4. otherwise (auth resolved)   -> hijack and delegate to the transport.
+ */
+export function mapAuthResultToResponse(input: {
+  sharedTokenOk: boolean;
+  enabled: boolean;
+  error?: unknown;
+}): McpHandleDecision {
+  if (!input.sharedTokenOk) {
+    return { kind: 'respond', status: 401, body: { error: 'Unauthorized' } };
+  }
+
+  if (!input.enabled) {
+    return {
+      kind: 'respond',
+      status: 403,
+      body: { error: 'MCP is disabled for this workspace' },
+    };
+  }
+
+  if (input.error !== undefined) {
+    if (input.error instanceof UnauthorizedException) {
+      return {
+        kind: 'respond',
+        status: 401,
+        body: { error: input.error.message },
+      };
+    }
+    return {
+      kind: 'respond',
+      status: 500,
+      body: { error: 'Internal server error' },
+    };
+  }
+
+  return { kind: 'hijack' };
+}
+
+// Result of the EE MFA module's requirement check for the Basic gate. Both
+// flags absent/false means MFA does not block the password login.
+export interface BasicGateMfaResult {
+  userHasMfa?: boolean;
+  requiresMfaSetup?: boolean;
+}
+
+/**
+ * Pure decision logic for the /mcp HTTP-Basic pre-token gate, replicating EXACTLY
+ * what AuthController.login enforces before issuing a token, so the Basic path is
+ * not an SSO/MFA bypass. Framework-free (no ModuleRef, no on-disk EE MFA module)
+ * so the SSO/MFA decision is unit-testable in isolation:
+ *
+ *   - `ssoEnforced` true  -> throw Unauthorized ("enforced SSO"); a password
+ *      login is not allowed on an SSO-enforced workspace.
+ *   - otherwise, `mfa` is the EE MFA module's requirement result (or undefined
+ *      when no EE MFA module is bundled — a community/fork build). If MFA is
+ *      present and the user has MFA enabled OR needs MFA setup, throw Unauthorized
+ *      telling the caller to use a Bearer access token (Basic cannot complete MFA).
+ *   - no SSO + no MFA gate -> resolve (the Basic login is allowed to proceed).
+ *
+ * McpService.enforceBasicLoginGate wires the concrete `validateSsoEnforcement`
+ * result and the lazily-loaded MFA module result into this, so the gate decision
+ * itself carries no framework dependencies. Throws UnauthorizedException on
+ * rejection (surfaced as a clean 401); never logs the password.
+ */
+export function decideBasicGate(input: {
+  ssoEnforced: boolean;
+  mfa?: BasicGateMfaResult;
+}): void {
+  if (input.ssoEnforced) {
+    throw new UnauthorizedException(
+      'This workspace has enforced SSO login. Use SSO; MCP HTTP Basic is not allowed.',
+    );
+  }
+
+  const mfa = input.mfa;
+  if (mfa && (mfa.userHasMfa || mfa.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.',
+    );
+  }
+}
+
 /** Extract a Bearer token from an Authorization header (case-insensitive). */
 export function extractBearer(
   authHeader: string | undefined,

apps/server/src/integrations/mcp/mcp.service.spec.ts

@@ -9,6 +9,9 @@ import {
   sharedTokenMatches,
   clientIp,
   bindAccessJwtVerifier,
+  extractBearer,
+  decideBasicGate,
+  mapAuthResultToResponse,
   McpAuthDeps,
 } from './mcp-auth.helpers';
 import { JwtType } from '../../core/auth/dto/jwt-payload';
@@ -79,6 +82,26 @@ describe('parseBasicAuth', () => {
   });
 });
 
+describe('extractBearer', () => {
+  it('extracts the token from a "Bearer <token>" header', () => {
+    expect(extractBearer('Bearer abc.def.ghi')).toBe('abc.def.ghi');
+  });
+
+  it('is case-insensitive on the scheme (lowercase + uppercase)', () => {
+    // The split keeps the token as-is; only the scheme is compared lowercased.
+    expect(extractBearer('bearer abc')).toBe('abc');
+    expect(extractBearer('BEARER abc')).toBe('abc');
+  });
+
+  it('returns undefined for a non-Bearer scheme (e.g. Basic)', () => {
+    expect(extractBearer('Basic abc')).toBeUndefined();
+  });
+
+  it('returns undefined for an undefined header', () => {
+    expect(extractBearer(undefined)).toBeUndefined();
+  });
+});
+
 describe('isCredentialsFailure', () => {
   it('is true for the credentials-mismatch UnauthorizedException', () => {
     expect(
@@ -185,6 +208,43 @@ describe('FailedLoginLimiter', () => {
     expect(lim.isBlocked(k, 0)).toBe(true);
     expect(lim.isBlocked(k, 1000)).toBe(false);
   });
+
+  describe('sweep (expired-bucket eviction, injectable clock)', () => {
+    // sweep() drops buckets whose windowStart is older than windowMs so
+    // never-revisited keys cannot accumulate forever. It takes an injectable
+    // `now` so the behaviour is deterministic without faking timers.
+    it('drops a bucket strictly older than windowMs', () => {
+      const lim = new FailedLoginLimiter(5, 1000);
+      // Seed a bucket at t=0 (windowStart=0).
+      lim.recordFailure('stale', 0);
+      // Sweep well past the window: now - windowStart = 5000 >= 1000 -> dropped.
+      lim.sweep(5000);
+      // A dropped bucket means a brand-new bucket is created on next touch, so
+      // the prior failure count is gone (a single fresh failure is far from 5).
+      lim.recordFailure('stale', 5001);
+      expect(lim.isBlocked('stale', 5001)).toBe(false);
+    });
+
+    it('drops a bucket exactly at the windowMs boundary (>= is inclusive)', () => {
+      const lim = new FailedLoginLimiter(1, 1000);
+      lim.recordFailure('boundary', 0); // windowStart=0, blocked at threshold 1
+      expect(lim.isBlocked('boundary', 0)).toBe(true);
+      // now - windowStart = 1000 == windowMs -> the >= check evicts it.
+      lim.sweep(1000);
+      // Re-touch at the same instant: a fresh bucket (count 0) is created, so the
+      // key is no longer blocked, proving the boundary bucket was swept.
+      expect(lim.isBlocked('boundary', 1000)).toBe(false);
+    });
+
+    it('retains a fresh bucket still within the window', () => {
+      const lim = new FailedLoginLimiter(1, 1000);
+      lim.recordFailure('fresh', 0); // windowStart=0
+      // now - windowStart = 999 < 1000 -> the bucket survives the sweep.
+      lim.sweep(999);
+      // Still blocked because the bucket (and its count) was retained.
+      expect(lim.isBlocked('fresh', 999)).toBe(true);
+    });
+  });
 });
 
 describe('verifyBearerAccess (Bearer revocation/disabled checks)', () => {
@@ -825,3 +885,138 @@ describe('bindAccessJwtVerifier enforces JwtType.ACCESS (item 3)', () => {
     expect(res).toEqual({ sub: 'user-1', email: undefined });
   });
 });
+
+describe('decideBasicGate (pure SSO/MFA pre-token gate, refactor R1)', () => {
+  // The pure decision extracted out of McpService.enforceBasicLoginGate. It is
+  // tested WITHOUT ModuleRef and WITHOUT an on-disk EE MFA module: the SSO verdict
+  // and the MFA requirement result are passed in as plain values.
+
+  it('SSO enforced -> throws Unauthorized ("enforced SSO")', () => {
+    expect(() => decideBasicGate({ ssoEnforced: true })).toThrow(
+      UnauthorizedException,
+    );
+    expect(() => decideBasicGate({ ssoEnforced: true })).toThrow(/enforced SSO/);
+    // SSO takes precedence even if MFA flags are also set.
+    expect(() =>
+      decideBasicGate({ ssoEnforced: true, mfa: { userHasMfa: true } }),
+    ).toThrow(/enforced SSO/);
+  });
+
+  it('no SSO + no MFA module (mfa undefined) -> resolves (Basic allowed)', () => {
+    // A community/fork build with no EE MFA module passes mfa: undefined and the
+    // gate must allow the password login (same as the controller with no MFA).
+    expect(() => decideBasicGate({ ssoEnforced: false })).not.toThrow();
+    expect(() =>
+      decideBasicGate({ ssoEnforced: false, mfa: undefined }),
+    ).not.toThrow();
+  });
+
+  it('MFA present + userHasMfa -> rejects ("use a Bearer access token")', () => {
+    expect(() =>
+      decideBasicGate({ ssoEnforced: false, mfa: { userHasMfa: true } }),
+    ).toThrow(/use a Bearer access token/);
+    expect(() =>
+      decideBasicGate({ ssoEnforced: false, mfa: { userHasMfa: true } }),
+    ).toThrow(UnauthorizedException);
+  });
+
+  it('MFA present + requiresMfaSetup -> rejects', () => {
+    expect(() =>
+      decideBasicGate({ ssoEnforced: false, mfa: { requiresMfaSetup: true } }),
+    ).toThrow(/use a Bearer access token/);
+  });
+
+  it('MFA present but none required (both flags false) -> resolves', () => {
+    expect(() =>
+      decideBasicGate({
+        ssoEnforced: false,
+        mfa: { userHasMfa: false, requiresMfaSetup: false },
+      }),
+    ).not.toThrow();
+  });
+});
+
+describe('mapAuthResultToResponse (handle status/body mapping, refactor R2)', () => {
+  // The pure response decision extracted out of McpService.handle. It maps the
+  // pre-hijack gauntlet (shared token, enablement, auth error) to either a fixed
+  // JSON error response or the hijack path — never leaking the password/header.
+
+  it('wrong X-MCP-Token -> 401 {error:"Unauthorized"} and NOT the hijack path', () => {
+    const d = mapAuthResultToResponse({ sharedTokenOk: false, enabled: true });
+    expect(d).toEqual({
+      kind: 'respond',
+      status: 401,
+      body: { error: 'Unauthorized' },
+    });
+  });
+
+  it('workspace MCP disabled -> 403', () => {
+    const d = mapAuthResultToResponse({ sharedTokenOk: true, enabled: false });
+    expect(d.kind).toBe('respond');
+    if (d.kind === 'respond') {
+      expect(d.status).toBe(403);
+      expect(d.body).toEqual({ error: 'MCP is disabled for this workspace' });
+    }
+  });
+
+  it('an UnauthorizedException -> 401 with err.message; no password/header leaked', () => {
+    // Construct an UnauthorizedException whose message is the SPECIFIC auth reason.
+    const err = new UnauthorizedException('Email or password does not match');
+    const d = mapAuthResultToResponse({
+      sharedTokenOk: true,
+      enabled: true,
+      error: err,
+    });
+    expect(d).toEqual({
+      kind: 'respond',
+      status: 401,
+      body: { error: 'Email or password does not match' },
+    });
+    // The surfaced body is ONLY the exception message — never the raw secret.
+    if (d.kind === 'respond') {
+      const serialized = JSON.stringify(d.body);
+      expect(serialized).not.toContain('password=');
+      expect(serialized).not.toContain('Authorization');
+      expect(serialized).not.toContain('Basic ');
+      expect(serialized).not.toContain('Bearer ');
+    }
+  });
+
+  it('a non-Unauthorized error -> 500 generic (no error detail surfaced)', () => {
+    const err = new Error('db blew up: connection string secret');
+    const d = mapAuthResultToResponse({
+      sharedTokenOk: true,
+      enabled: true,
+      error: err,
+    });
+    expect(d).toEqual({
+      kind: 'respond',
+      status: 500,
+      body: { error: 'Internal server error' },
+    });
+    // The generic body must NOT echo the underlying error message.
+    if (d.kind === 'respond') {
+      expect(d.body.error).not.toContain('secret');
+    }
+  });
+
+  it('happy path (auth resolved, no error) -> hijack', () => {
+    const d = mapAuthResultToResponse({ sharedTokenOk: true, enabled: true });
+    expect(d).toEqual({ kind: 'hijack' });
+  });
+
+  it('shared-token failure takes precedence over disabled/error', () => {
+    // Even with a disabled workspace and an error, a bad shared token is the
+    // first gate, so the response is the uniform 401 Unauthorized.
+    const d = mapAuthResultToResponse({
+      sharedTokenOk: false,
+      enabled: false,
+      error: new UnauthorizedException('should not surface'),
+    });
+    expect(d).toEqual({
+      kind: 'respond',
+      status: 401,
+      body: { error: 'Unauthorized' },
+    });
+  });
+});

apps/server/src/integrations/mcp/mcp.service.ts

@@ -25,6 +25,8 @@ import {
   sharedTokenMatches,
   clientIp,
   bindAccessJwtVerifier,
+  decideBasicGate,
+  mapAuthResultToResponse,
   DocmostMcpConfig,
   ResolvedMcpAuth,
 } from './mcp-auth.helpers';
@@ -241,49 +243,54 @@ export class McpService implements OnModuleDestroy {
     workspace: Workspace,
     creds: { email: string; password: string },
   ): Promise<void> {
-    // 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.
+    // 1) SSO enforcement. validateSsoEnforcement throws when the workspace
+    // enforces SSO; we only need the boolean verdict for the pure decision.
+    let ssoEnforced = false;
     try {
       validateSsoEnforcement(workspace);
     } catch {
-      throw new UnauthorizedException(
-        'This workspace has enforced SSO login. Use SSO; MCP HTTP Basic is not allowed.',
-      );
+      ssoEnforced = true;
     }
 
     // 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;
+    // On a fork WITHOUT the EE module bundled, mfaResult stays undefined and the
+    // pure gate behaves exactly like the controller (no MFA module -> no MFA
+    // gate). We only LOAD the module + read the requirement flags here; the
+    // accept/reject decision lives in the framework-free decideBasicGate so the
+    // SSO/MFA logic is unit-testable without ModuleRef or the on-disk EE module.
+    let mfaResult: { userHasMfa?: boolean; requiresMfaSetup?: boolean } | undefined;
+    // Only consult the MFA module when SSO has not already disqualified the
+    // request (SSO short-circuits, and skipping the load avoids a needless
+    // require on the SSO-reject path).
+    if (!ssoEnforced) {
+      // 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.)
+        MfaModule = undefined;
+      }
+
+      if (MfaModule) {
+        const mfaService = this.moduleRef.get(MfaModule.MfaService, {
+          strict: false,
+        });
+        // 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.
+        mfaResult = await mfaService.checkMfaRequirements(
+          creds,
+          workspace,
+          undefined,
+        );
+      }
     }
 
-    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.',
-      );
-    }
+    // Pure accept/reject decision (throws UnauthorizedException on rejection).
+    decideBasicGate({ ssoEnforced, mfa: mfaResult });
   }
 
   // Lazily create the HTTP handler exactly once. The import is indirected so
@@ -343,52 +350,61 @@ export class McpService implements OnModuleDestroy {
     // matching `X-MCP-Token` header. It now lives in its OWN header so it never
     // collides with `Authorization`, which carries the per-user credentials.
     const sharedToken = process.env.MCP_TOKEN;
-    if (sharedToken) {
-      const provided = req.headers['x-mcp-token'];
-      if (!sharedTokenMatches(sharedToken, provided)) {
-        res.status(401).send({ error: 'Unauthorized' });
-        return;
-      }
-    }
+    const sharedTokenOk = sharedToken
+      ? sharedTokenMatches(sharedToken, req.headers['x-mcp-token'])
+      : true;
 
-    if (!(await this.isEnabled())) {
-      res.status(403).send({ error: 'MCP is disabled for this workspace' });
-      return;
-    }
+    // Short-circuit checks (shared token, enablement) that do not need the auth
+    // resolution. Compute them up front so the response mapping is a single pure
+    // decision (mapAuthResultToResponse) that cannot leak the password/header.
+    const enabled = sharedTokenOk ? await this.isEnabled() : false;
 
     // Resolve + validate the per-session identity BEFORE hijacking the response
     // so bad credentials surface as a clean 401 JSON (never a torn response and
     // never a generic "MCP error"). The resolved config/identity is stashed on
     // the raw request for the package's resolver + identify hook to read back.
-    let resolved: ResolvedMcpAuth;
-    try {
-      resolved = await this.resolveSessionConfig(req);
-    } catch (err) {
-      if (err instanceof UnauthorizedException) {
-        // Warn once if the only thing missing is the service account, to keep
-        // the original operator hint.
-        if (
-          !this.credsConfigured() &&
-          !req.headers['authorization'] &&
-          !this.warnedMissingCreds
-        ) {
-          this.warnedMissingCreds = true;
-          this.logger.warn(
-            'MCP is enabled but received a request with no credentials and no ' +
-              'MCP_DOCMOST_EMAIL/MCP_DOCMOST_PASSWORD service account configured.',
-          );
+    let resolved: ResolvedMcpAuth | undefined;
+    let authError: unknown;
+    if (sharedTokenOk && enabled) {
+      try {
+        resolved = await this.resolveSessionConfig(req);
+      } catch (err) {
+        authError = err;
+        if (err instanceof UnauthorizedException) {
+          // Warn once if the only thing missing is the service account, to keep
+          // the original operator hint.
+          if (
+            !this.credsConfigured() &&
+            !req.headers['authorization'] &&
+            !this.warnedMissingCreds
+          ) {
+            this.warnedMissingCreds = true;
+            this.logger.warn(
+              'MCP is enabled but received a request with no credentials and no ' +
+                'MCP_DOCMOST_EMAIL/MCP_DOCMOST_PASSWORD service account configured.',
+            );
+          }
+        } else {
+          this.logger.error('MCP auth resolution failed', err as Error);
         }
-        res.status(401).send({ error: err.message });
-        return;
       }
-      this.logger.error('MCP auth resolution failed', err as Error);
-      res.status(500).send({ error: 'Internal server error' });
+    }
+
+    // Pure status/body mapping for the whole pre-hijack gauntlet.
+    const decision = mapAuthResultToResponse({
+      sharedTokenOk,
+      enabled,
+      error: authError,
+    });
+    if (decision.kind === 'respond') {
+      res.status(decision.status).send(decision.body);
       return;
     }
 
     // Stash the resolved auth on the raw request so the package's resolver +
     // identify hook (wired in getHandler) read it back instead of re-parsing.
-    (req.raw as unknown as Record<symbol, unknown>)[MCP_RESOLVED] = resolved;
+    (req.raw as unknown as Record<symbol, unknown>)[MCP_RESOLVED] =
+      resolved as ResolvedMcpAuth;
 
     // Hand the raw Node req/res to the MCP transport. hijack() tells Fastify
     // to stop managing this response so the transport can write to it directly.