mcp-per-user-auth (#13): security review follow-ups (hardening + cleanup) #24

New Issue

vvzvlad · 2026-06-20T19:32:43+03:00

vvzvlad commented

2026-06-20 19:32:43 +03:00

Follow-up items from the security review of #13 (feat/mcp-per-user-auth), merged as-is into develop (merge commit f72e44c9). No blocker auth bypass or credential leak was found; these are hardening / cleanup items.

medium

Align isSessionInit with the SDK isInitializeRequest — isInitializeRequestBody (apps/server/src/integrations/mcp/mcp-auth.helpers.ts) only checks method === 'initialize', while packages/mcp/src/http.ts uses the SDK's full InitializeRequestSchema.safeParse (requires jsonrpc, id, valid params). An authenticated client can POST {"method":"initialize"} without params and no mcp-session-id: McpService treats it as session-init and runs the side-effecting login() (inserts a user_sessions row, writes a USER_LOGIN audit event, bumps lastLoginAt), then http.ts 400s it without creating an MCP session. Result: repeatable audit-spam / session-table growth (authenticated abuse). Fix: reuse/import the SDK isInitializeRequest predicate (or tighten isInitializeRequestBody to require jsonrpc + params.protocolVersion) so the session-minting side and the session-creating side use the identical signal.
Document trusted-proxy requirement for the brute-force limiter — FastifyAdapter({ trustProxy: true }) (apps/server/src/main.ts) is unconditional, so clientIp derives req.ip from a client-forgeable X-Forwarded-For. The per-IP and per-IP+email limiter keys are spoofable when no trusted proxy sits in front. The global per-email key is the real backstop and is IP-independent. Action: document in .env.example that a trusted proxy / correct trustProxy hop config is required for per-IP throttling to be meaningful (and consider making trustProxy configurable).

low

Bind the Bearer path to the instance workspace — verifyMcpBearer / verifyBearerAccess (apps/server/src/integrations/mcp/mcp.service.ts) verifies signature/exp/type=ACCESS + not-disabled user + active session, but does not check payload.workspaceId against the MCP instance's workspace (unlike JwtStrategy which loads the workspace). Negligible for a single-workspace community build; in a multi-workspace deployment a token from another workspace could pass.
User-enumeration timing oracle in verifyUserCredentials — apps/server/src/core/auth/services/auth.service.ts: for a non-existent email it throws before comparePasswordHash; for an existing one it runs bcrypt. The timing difference reveals whether an email exists. Pre-existing (lifted from login()), but now reachable via /mcp where throttling is the in-memory limiter (spoofable per-IP keys) rather than the controller's ThrottlerGuard.
Cover the real SSO/MFA wiring in tests — apps/server/src/integrations/mcp/mcp.service.spec.ts SSO/MFA tests stub enforceBasicGate, so they only assert the helper calls the gate before login()/verifyCredentials. The actual McpService.enforceBasicLoginGate wiring (validateSsoEnforcement + lazy EE MFA require + reading mfaResult flags) is not instantiated by any test. Add coverage for the real gate (the most security-sensitive part).

Filed from a post-merge security review. Source files: apps/server/src/integrations/mcp/mcp-auth.helpers.ts, apps/server/src/integrations/mcp/mcp.service.ts, apps/server/src/core/auth/services/auth.service.ts, apps/server/src/main.ts, packages/mcp/src/http.ts.

Follow-up items from the security review of #13 (`feat/mcp-per-user-auth`), merged as-is into `develop` (merge commit f72e44c9). No blocker auth bypass or credential leak was found; these are hardening / cleanup items. ### medium - [ ] **Align `isSessionInit` with the SDK `isInitializeRequest`** — `isInitializeRequestBody` (`apps/server/src/integrations/mcp/mcp-auth.helpers.ts`) only checks `method === 'initialize'`, while `packages/mcp/src/http.ts` uses the SDK's full `InitializeRequestSchema.safeParse` (requires `jsonrpc`, `id`, valid `params`). An authenticated client can POST `{"method":"initialize"}` without `params` and no `mcp-session-id`: `McpService` treats it as session-init and runs the side-effecting `login()` (inserts a `user_sessions` row, writes a `USER_LOGIN` audit event, bumps `lastLoginAt`), then `http.ts` 400s it without creating an MCP session. Result: repeatable audit-spam / session-table growth (authenticated abuse). Fix: reuse/import the SDK `isInitializeRequest` predicate (or tighten `isInitializeRequestBody` to require `jsonrpc` + `params.protocolVersion`) so the session-minting side and the session-creating side use the identical signal. - [ ] **Document trusted-proxy requirement for the brute-force limiter** — `FastifyAdapter({ trustProxy: true })` (`apps/server/src/main.ts`) is unconditional, so `clientIp` derives `req.ip` from a client-forgeable `X-Forwarded-For`. The per-IP and per-IP+email limiter keys are spoofable when no trusted proxy sits in front. The global per-email key is the real backstop and is IP-independent. Action: document in `.env.example` that a trusted proxy / correct `trustProxy` hop config is required for per-IP throttling to be meaningful (and consider making `trustProxy` configurable). ### low - [ ] **Bind the Bearer path to the instance workspace** — `verifyMcpBearer` / `verifyBearerAccess` (`apps/server/src/integrations/mcp/mcp.service.ts`) verifies signature/exp/type=ACCESS + not-disabled user + active session, but does not check `payload.workspaceId` against the MCP instance's workspace (unlike `JwtStrategy` which loads the workspace). Negligible for a single-workspace community build; in a multi-workspace deployment a token from another workspace could pass. - [ ] **User-enumeration timing oracle in `verifyUserCredentials`** — `apps/server/src/core/auth/services/auth.service.ts`: for a non-existent email it throws before `comparePasswordHash`; for an existing one it runs bcrypt. The timing difference reveals whether an email exists. Pre-existing (lifted from `login()`), but now reachable via `/mcp` where throttling is the in-memory limiter (spoofable per-IP keys) rather than the controller's ThrottlerGuard. - [ ] **Cover the real SSO/MFA wiring in tests** — `apps/server/src/integrations/mcp/mcp.service.spec.ts` SSO/MFA tests stub `enforceBasicGate`, so they only assert the helper calls the gate before `login()`/`verifyCredentials`. The actual `McpService.enforceBasicLoginGate` wiring (`validateSsoEnforcement` + lazy EE MFA require + reading `mfaResult` flags) is not instantiated by any test. Add coverage for the real gate (the most security-sensitive part). --- _Filed from a post-merge security review. Source files: `apps/server/src/integrations/mcp/mcp-auth.helpers.ts`, `apps/server/src/integrations/mcp/mcp.service.ts`, `apps/server/src/core/auth/services/auth.service.ts`, `apps/server/src/main.ts`, `packages/mcp/src/http.ts`._

Ghost referenced this issue from a commit

2026-06-20 23:37:22 +03:00

fix(mcp): security review follow-ups (#24)

Ghost referenced a pull request that will close this issue

2026-06-20 23:37:43 +03:00

fix(mcp): security review follow-ups (#24) #48

Ghost closed this issue

2026-06-21 01:28:11 +03:00

Ghost referenced this issue from a commit

2026-06-21 01:28:11 +03:00

Merge pull request 'fix(mcp): security review follow-ups (#24)' (#48) from fix/mcp-security-followups into develop

Sign in to join this conversation.