test(mcp): cover X-MCP-Token/clientIp/bearer-type/creds-failure (pure seams)

Release-cycle test audit: the /mcp auth's constant-time token guard, IP keying,
ACCESS-type pinning, and brute-force message coupling were untested. Extract
behavior-preserving pure helpers so they're testable and cover them:
- sharedTokenMatches: length-mismatch early-returns before timingSafeEqual
  (which throws on unequal lengths); equal-length uses timingSafeEqual; array
  header -> first element; non-string -> false.
- clientIp: req.ip > socket > first XFF hop > 'unknown' (limiter keying).
- bindAccessJwtVerifier: verifyJwt pinned to JwtType.ACCESS (rejects REFRESH).
- CREDENTIALS_MISMATCH_MESSAGE single source of truth shared by
  verifyUserCredentials and isCredentialsFailure, so a reworded auth error can't
  silently disable the /mcp brute-force counter.
- verifyUserCredentials no-side-effect contract asserted via a TS-AST spec
  (AuthService can't load under jest): its body has no createSessionAndToken/
  audit/updateLastLogin while login() has all three.
Extractions are behavior-preserving (reviewed); class delegates to the helpers,
dead code + unused imports removed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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

@@ -4,7 +4,9 @@
 // dependency graph, and reused by McpService. Nothing here logs the password or
 // the Authorization header.
 import { UnauthorizedException } from '@nestjs/common';
+import { timingSafeEqual } from 'node:crypto';
 import { JwtType } from '../../core/auth/dto/jwt-payload';
+import { CREDENTIALS_MISMATCH_MESSAGE } from '../../core/auth/auth.constants';
 
 /**
  * Decode an `Authorization: Basic base64(email:password)` header into its
@@ -171,15 +173,112 @@ export interface McpAuthDeps {
  * throws an UnauthorizedException with exactly this message for every
  * credentials-mismatch case (no user / disabled / wrong password), so we match
  * on that.
+ *
+ * The message is NOT hardcoded here: it matches against the shared
+ * CREDENTIALS_MISMATCH_MESSAGE constant that AuthService.verifyUserCredentials
+ * also throws, so a reworded auth error cannot silently stop counting toward the
+ * limiter (single source of truth — see auth.constants.ts).
  */
 export function isCredentialsFailure(err: unknown): boolean {
   return (
     err instanceof UnauthorizedException &&
     typeof err.message === 'string' &&
-    err.message.toLowerCase().includes('email or password does not match')
+    err.message
+      .toLowerCase()
+      .includes(CREDENTIALS_MISMATCH_MESSAGE.toLowerCase())
   );
 }
 
+/**
+ * Constant-time comparison of the optional shared X-MCP-Token guard. A header
+ * value may arrive as string | string[] (multiple X-MCP-Token headers), so we
+ * normalise to the first string. crypto.timingSafeEqual avoids leaking the
+ * token's length via early-exit string comparison; it requires equal buffer
+ * lengths, so a length mismatch is treated as a non-match WITHOUT calling
+ * timingSafeEqual (which throws on unequal lengths). A non-string / undefined
+ * value is never a match.
+ *
+ * Pure and framework-free so it is unit-testable; McpService.handle delegates to
+ * it for the X-MCP-Token shared guard.
+ */
+export function sharedTokenMatches(
+  expected: string,
+  provided: string | string[] | undefined,
+): boolean {
+  const value = Array.isArray(provided) ? provided[0] : provided;
+  if (typeof value !== 'string') return false;
+  const a = Buffer.from(value);
+  const b = Buffer.from(expected);
+  // Early-return before timingSafeEqual, which throws on unequal-length buffers.
+  if (a.length !== b.length) return false;
+  return timingSafeEqual(a, b);
+}
+
+// Minimal structural shape of the bits of a Fastify request that `clientIp`
+// needs. Kept structural so this module never imports the Fastify types.
+export interface ClientIpRequest {
+  ip?: string;
+  socket?: { remoteAddress?: string };
+  headers: Record<string, string | string[] | undefined>;
+}
+
+/**
+ * Best-effort client IP for the failed-login limiter key. Precedence:
+ *   1. req.ip          — Fastify's resolved IP (honours a configured trustProxy
+ *                        chain); the trustworthy value when a proxy is set up.
+ *   2. socket.remoteAddress — the raw TCP peer, used only when req.ip is absent.
+ *   3. first X-Forwarded-For hop — LAST resort only, because XFF is
+ *                        client-forgeable when no trusted proxy is configured.
+ *   4. 'unknown'       — nothing usable.
+ *
+ * A forged IP can only dodge the per-IP limiter keys; the GLOBAL per-email key
+ * in resolveMcpSessionConfig is the real account-brute backstop and does not
+ * depend on this value. Pure/framework-free so it is unit-testable; McpService
+ * delegates to it.
+ */
+export function clientIp(req: ClientIpRequest): string {
+  if (req.ip) return req.ip;
+  if (req.socket?.remoteAddress) return req.socket.remoteAddress;
+  const xff = req.headers['x-forwarded-for'];
+  if (typeof xff === 'string' && xff.length > 0) {
+    return xff.split(',')[0].trim();
+  }
+  return 'unknown';
+}
+
+// Minimal structural shape of the TokenService.verifyJwt method we depend on,
+// so this module never imports the concrete TokenService (heavy graph).
+export interface AccessJwtVerifier {
+  verifyJwt: (
+    token: string,
+    type: JwtType,
+  ) => Promise<{
+    sub?: string;
+    email?: string;
+    workspaceId?: string;
+    sessionId?: string;
+  }>;
+}
+
+/**
+ * Bind a TokenService-like verifier into a one-arg `verifyJwt(token)` that
+ * ALWAYS enforces `JwtType.ACCESS`. This is the single place where the /mcp
+ * Bearer path pins the token type: a Bearer access token must be verified AS an
+ * access token (not refresh/exchange/collab/etc.), so the type literal is fixed
+ * here rather than at the call site. McpService.verifyMcpBearer delegates to
+ * this, keeping the `JwtType.ACCESS` choice testable without the heavy graph.
+ */
+export function bindAccessJwtVerifier(
+  tokenService: AccessJwtVerifier,
+): (token: string) => Promise<{
+  sub?: string;
+  email?: string;
+  workspaceId?: string;
+  sessionId?: string;
+}> {
+  return (token: string) => tokenService.verifyJwt(token, JwtType.ACCESS);
+}
+
 // Minimal shapes for the Bearer revocation/disabled check. Kept structural so
 // this module never imports the concrete repos/JwtPayload (heavy graph).
 export interface BearerVerifyDeps {

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

@@ -6,8 +6,13 @@ import {
   isCredentialsFailure,
   isInitializeRequestBody,
   verifyBearerAccess,
+  sharedTokenMatches,
+  clientIp,
+  bindAccessJwtVerifier,
   McpAuthDeps,
 } from './mcp-auth.helpers';
+import { JwtType } from '../../core/auth/dto/jwt-payload';
+import { CREDENTIALS_MISMATCH_MESSAGE } from '../../core/auth/auth.constants';
 
 // The /mcp per-user auth decision logic is tested through the framework-free
 // `resolveMcpSessionConfig` helper that McpService delegates to. McpService
@@ -91,6 +96,72 @@ describe('isCredentialsFailure', () => {
     ).toBe(false);
     expect(isCredentialsFailure(new Error('boom'))).toBe(false);
   });
+
+  // --- Cross-file coupling lock (item 1) ---------------------------------
+  // The /mcp Basic brute-force limiter ONLY counts a failure when
+  // isCredentialsFailure(err) is true. AuthService.verifyUserCredentials throws
+  // the credentials failure with the shared CREDENTIALS_MISMATCH_MESSAGE for
+  // unknown email / wrong password / disabled user. If that message were
+  // reworded without updating the matcher, the limiter would stop counting and
+  // /mcp Basic would become an unthrottled password-guessing oracle. These
+  // tests lock the coupling to the SHARED constant (single source of truth) so a
+  // reword is a compile-time/test-time break, not a silent security regression.
+
+  it('recognises the exact UnauthorizedException AuthService throws (the shared constant)', () => {
+    // Reconstruct the EXACT exception AuthService.verifyUserCredentials throws
+    // for every credentials-failure case (it uses CREDENTIALS_MISMATCH_MESSAGE),
+    // and assert the REAL isCredentialsFailure recognises it. No hardcoded string
+    // is duplicated here — both sides reference the single shared constant.
+    const authThrows = new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE);
+    expect(isCredentialsFailure(authThrows)).toBe(true);
+  });
+
+  it('the matcher is coupled to the single source of truth, not a local literal', () => {
+    // If someone reworded CREDENTIALS_MISMATCH_MESSAGE, this still passes only
+    // because the matcher derives its substring from the SAME constant. This
+    // pins the coupling structurally: there is one message both files share.
+    expect(CREDENTIALS_MISMATCH_MESSAGE).toBeTruthy();
+    expect(
+      isCredentialsFailure(
+        new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE),
+      ),
+    ).toBe(true);
+    // A DIFFERENT message (a hypothetical reword that forgot to go through the
+    // constant) must NOT be silently recognised, proving the matcher is not just
+    // "always true".
+    expect(
+      isCredentialsFailure(new UnauthorizedException('totally different wording')),
+    ).toBe(false);
+  });
+});
+
+describe('AuthService verifyUserCredentials <-> isCredentialsFailure coupling (item 1)', () => {
+  // AuthService cannot be constructed under jest: importing it pulls in
+  // src/integrations/queue/constants (a `src/`-rooted absolute import) which the
+  // jest moduleNameMapper does not resolve under rootDir:src — the heavy auth
+  // graph. So instead of a live AuthService unit, we assert the security
+  // contract structurally: AuthService.verifyUserCredentials throws an
+  // UnauthorizedException built from the SHARED CREDENTIALS_MISMATCH_MESSAGE
+  // (see auth.service.ts), and the REAL isCredentialsFailure recognises it. The
+  // single shared constant is the lock: there is no second copy of the string to
+  // drift out of sync.
+  it('the credentials-failure UnauthorizedException is counted by the limiter matcher', () => {
+    // unknown email / disabled user / wrong password all surface as this:
+    const credentialsFailure = new UnauthorizedException(
+      CREDENTIALS_MISMATCH_MESSAGE,
+    );
+    expect(isCredentialsFailure(credentialsFailure)).toBe(true);
+  });
+
+  it('email-not-verified (a different, business error) is NOT counted', () => {
+    // throwIfEmailNotVerified throws a BadRequestException, which must not burn a
+    // victim's limiter budget; the matcher rejects it.
+    expect(
+      isCredentialsFailure(
+        new BadRequestException('Please verify your email address.'),
+      ),
+    ).toBe(false);
+  });
 });
 
 describe('FailedLoginLimiter', () => {
@@ -578,3 +649,123 @@ describe('resolveMcpSessionConfig non-initialize request side effects', () => {
     );
   });
 });
+
+describe('sharedTokenMatches (X-MCP-Token constant-time guard, item 2)', () => {
+  it('equal token -> true', () => {
+    expect(sharedTokenMatches('s3cr3t-token', 's3cr3t-token')).toBe(true);
+  });
+
+  it('wrong token of the SAME length -> false (timingSafeEqual path)', () => {
+    // Same length so it reaches timingSafeEqual; the bytes differ -> no match.
+    expect(sharedTokenMatches('aaaaaa', 'aaaaab')).toBe(false);
+  });
+
+  it('different-length token -> false WITHOUT throwing (early-return before timingSafeEqual)', () => {
+    // timingSafeEqual throws on unequal-length buffers; the early length check
+    // must short-circuit so a length mismatch is a clean non-match, not a throw.
+    expect(() => sharedTokenMatches('expected', 'short')).not.toThrow();
+    expect(sharedTokenMatches('expected', 'short')).toBe(false);
+    expect(sharedTokenMatches('expected', 'a-much-longer-provided-value')).toBe(
+      false,
+    );
+  });
+
+  it('array-valued header -> uses the FIRST element', () => {
+    // Multiple X-MCP-Token headers arrive as string[]; only the first is used.
+    expect(sharedTokenMatches('tok', ['tok', 'ignored'])).toBe(true);
+    expect(sharedTokenMatches('tok', ['wrong', 'tok'])).toBe(false);
+  });
+
+  it('undefined / non-string provided -> false', () => {
+    expect(sharedTokenMatches('tok', undefined)).toBe(false);
+    // An empty array yields provided[0] === undefined -> non-string -> false.
+    expect(sharedTokenMatches('tok', [])).toBe(false);
+    expect(sharedTokenMatches('tok', [undefined as unknown as string])).toBe(
+      false,
+    );
+  });
+});
+
+describe('clientIp (XFF-fallback precedence, item 5)', () => {
+  it('req.ip wins over socket.remoteAddress AND over X-Forwarded-For', () => {
+    expect(
+      clientIp({
+        ip: '1.1.1.1',
+        socket: { remoteAddress: '2.2.2.2' },
+        headers: { 'x-forwarded-for': '3.3.3.3' },
+      }),
+    ).toBe('1.1.1.1');
+  });
+
+  it('socket.remoteAddress is used only when req.ip is absent (still beats XFF)', () => {
+    expect(
+      clientIp({
+        socket: { remoteAddress: '2.2.2.2' },
+        headers: { 'x-forwarded-for': '3.3.3.3' },
+      }),
+    ).toBe('2.2.2.2');
+  });
+
+  it('X-Forwarded-For is the LAST resort, and only the FIRST hop is taken', () => {
+    expect(
+      clientIp({
+        headers: { 'x-forwarded-for': '3.3.3.3, 4.4.4.4, 5.5.5.5' },
+      }),
+    ).toBe('3.3.3.3');
+  });
+
+  it("returns 'unknown' when nothing usable is present", () => {
+    expect(clientIp({ headers: {} })).toBe('unknown');
+    // An array-valued XFF header is not treated as a string source -> unknown.
+    expect(
+      clientIp({ headers: { 'x-forwarded-for': ['3.3.3.3'] } }),
+    ).toBe('unknown');
+    // An empty XFF string is ignored too.
+    expect(clientIp({ headers: { 'x-forwarded-for': '' } })).toBe('unknown');
+  });
+});
+
+describe('bindAccessJwtVerifier enforces JwtType.ACCESS (item 3)', () => {
+  it('calls TokenService.verifyJwt with JwtType.ACCESS as the second argument', async () => {
+    // Mock TokenService: assert the type literal is pinned to ACCESS so swapping
+    // to REFRESH (or omitting the type) breaks this test.
+    const verifyJwt = jest
+      .fn()
+      .mockResolvedValue({ sub: 'user-1', workspaceId: 'ws-1' });
+    const verify = bindAccessJwtVerifier({ verifyJwt });
+
+    await verify('the.access.jwt');
+
+    expect(verifyJwt).toHaveBeenCalledTimes(1);
+    expect(verifyJwt).toHaveBeenCalledWith('the.access.jwt', JwtType.ACCESS);
+    // Pin the real enum value too, so renaming/repointing the enum member is caught.
+    expect(verifyJwt.mock.calls[0][1]).toBe('access');
+  });
+
+  it('passes through the verified payload', async () => {
+    const payload = { sub: 'user-9', email: 'u@e.com', workspaceId: 'ws-1' };
+    const verifyJwt = jest.fn().mockResolvedValue(payload);
+    await expect(
+      bindAccessJwtVerifier({ verifyJwt })('t'),
+    ).resolves.toBe(payload);
+  });
+
+  // The Bearer revocation/disabled checks (verifyBearerAccess) are covered above;
+  // this binds the ACCESS-type enforcement that verifyMcpBearer wires in.
+  it('feeds verifyBearerAccess so the whole Bearer chain enforces ACCESS', async () => {
+    const verifyJwt = jest.fn().mockResolvedValue({
+      sub: 'user-1',
+      workspaceId: 'ws-1',
+      sessionId: 'sess-1',
+    });
+    const res = await verifyBearerAccess('t', {
+      verifyJwt: bindAccessJwtVerifier({ verifyJwt }),
+      findUser: jest.fn().mockResolvedValue({ deactivatedAt: null }),
+      findActiveSession: jest
+        .fn()
+        .mockResolvedValue({ userId: 'user-1', workspaceId: 'ws-1' }),
+    });
+    expect(verifyJwt).toHaveBeenCalledWith('t', JwtType.ACCESS);
+    expect(res).toEqual({ sub: 'user-1', email: undefined });
+  });
+});

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

@@ -6,7 +6,6 @@ import {
 } from '@nestjs/common';
 import { ModuleRef } from '@nestjs/core';
 import { pathToFileURL } from 'node:url';
-import { timingSafeEqual } from 'node:crypto';
 import { IncomingMessage } from 'node:http';
 import { FastifyReply, FastifyRequest } from 'fastify';
 import { EnvironmentService } from '../environment/environment.service';
@@ -16,13 +15,16 @@ 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 { JwtPayload } from '../../core/auth/dto/jwt-payload';
 import { Workspace } from '@docmost/db/types/entity.types';
 import {
   FailedLoginLimiter,
   resolveMcpSessionConfig,
   verifyBearerAccess,
   isInitializeRequestBody,
+  sharedTokenMatches,
+  clientIp,
+  bindAccessJwtVerifier,
   DocmostMcpConfig,
   ResolvedMcpAuth,
 } from './mcp-auth.helpers';
@@ -144,41 +146,6 @@ export class McpService implements OnModuleDestroy {
     }
   }
 
-  // Constant-time comparison of the optional shared X-MCP-Token guard. A header
-  // value may arrive as string | string[] (multiple X-MCP-Token headers), so we
-  // normalise to the first string. crypto.timingSafeEqual avoids leaking the
-  // token's length-prefix via early-exit string comparison; it requires equal
-  // buffer lengths, so a length mismatch is treated as a non-match WITHOUT
-  // calling timingSafeEqual (which would throw on unequal lengths).
-  private sharedTokenMatches(
-    expected: string,
-    provided: string | string[] | undefined,
-  ): boolean {
-    const value = Array.isArray(provided) ? provided[0] : provided;
-    if (typeof value !== 'string') return false;
-    const a = Buffer.from(value);
-    const b = Buffer.from(expected);
-    if (a.length !== b.length) return false;
-    return timingSafeEqual(a, b);
-  }
-
-  // Best-effort client IP for the failed-login limiter key. Prefer Fastify's
-  // req.ip (which honours a configured trustProxy chain) and the socket address
-  // over a raw X-Forwarded-For hop, since XFF is client-forgeable when no
-  // trusted proxy is configured. The first XFF hop is only used as a last
-  // resort. NOTE: a forged IP can only dodge the per-IP limiter keys — the
-  // GLOBAL per-email key in resolveMcpSessionConfig is the real account-brute
-  // backstop and does not depend on this value.
-  private clientIp(req: FastifyRequest): string {
-    if (req.ip) return req.ip;
-    if (req.socket?.remoteAddress) return req.socket.remoteAddress;
-    const xff = req.headers['x-forwarded-for'];
-    if (typeof xff === 'string' && xff.length > 0) {
-      return xff.split(',')[0].trim();
-    }
-    return 'unknown';
-  }
-
   // Bearer access-JWT verification for the /mcp token fallback. verifyJwt only
   // checks signature/exp/type, but a logged-out (revoked) or disabled user can
   // still hold an unexpired access JWT. JwtStrategy additionally checks the
@@ -191,8 +158,11 @@ export class McpService implements OnModuleDestroy {
     // verifyBearerAccess helper (unit-testable without the heavy auth graph);
     // this method only wires in the concrete TokenService + repos.
     return verifyBearerAccess(token, {
-      verifyJwt: (t) =>
-        this.tokenService.verifyJwt(t, JwtType.ACCESS) as Promise<JwtPayload>,
+      // The JwtType.ACCESS enforcement lives in bindAccessJwtVerifier (a pure,
+      // testable seam) so the type literal cannot silently drift to REFRESH.
+      verifyJwt: bindAccessJwtVerifier(this.tokenService) as (
+        t: string,
+      ) => Promise<JwtPayload>,
       findUser: (sub, workspaceId) =>
         this.userRepo.findById(sub, workspaceId),
       findActiveSession: (sessionId) =>
@@ -239,7 +209,7 @@ export class McpService implements OnModuleDestroy {
       },
       verifyAccessJwt: (token) => this.verifyMcpBearer(token),
       limiter: this.failedLogins,
-      clientIp: this.clientIp(req),
+      clientIp: clientIp(req),
       isSessionInit,
     });
   }
@@ -365,7 +335,7 @@ export class McpService implements OnModuleDestroy {
     const sharedToken = process.env.MCP_TOKEN;
     if (sharedToken) {
       const provided = req.headers['x-mcp-token'];
-      if (!this.sharedTokenMatches(sharedToken, provided)) {
+      if (!sharedTokenMatches(sharedToken, provided)) {
         res.status(401).send({ error: 'Unauthorized' });
         return;
       }