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

Ghost · 2026-06-20T23:37:43+03:00

Ghost commented

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

Post-merge security-review follow-ups для mcp-per-user-auth (#13). Ветка от develop. Closes #24.

Что сделано (6 подпунктов чек-листа)

isInitializeRequest alignment (medium): isInitializeRequestBody теперь делегирует SDK-предикату isInitializeRequest (тот же, что в packages/mcp/http.ts). Голый {method:'initialize'} без id/params больше не триггерит side-effecting login() (audit-spam + рост user_sessions) до того, как http.ts его 400-ит.
trustProxy doc (medium): заметка в .env.example — приложение с trustProxy: true доверяет X-Forwarded-For; деплоить за доверенным прокси, иначе per-IP throttling (вкл. /mcp brute-force) спуфится. (doc-only, runtime не тронут.)
Bearer→workspace bind (low): verifyBearerAccess отвергает токен с payload.workspaceId != workspace инстанса (резолв через workspaceRepo.findFirst(), как Basic-путь). Опциональный параметр → no-op когда не задан (single-workspace OSS).
Timing-oracle (low): в verifyUserCredentials ветка missing/disabled теперь делает bcrypt-compare против dummy-хеша. По замечанию ревью cost dummy-хеша выставлен 12 (== продакшен saltRounds), иначе тайминг не выровнен; тест проверяет $2b$12$. Сообщение CREDENTIALS_MISMATCH_MESSAGE сохранено (его матчит /mcp-лимитер).
SSO/MFA тесты (low): новый mcp-basic-login-gate.spec.ts гоняет РЕАЛЬНЫЙ enforceBasicLoginGate (SSO enforced / EE-MFA bundled vs not / user-MFA / workspace-enforced-MFA), вместо заглушки гейта. EE-модуль мокается через jest.mock(..., {virtual:true}).

Косяк, пойманный на ревью

Первая версия timing-fix использовала dummy-хеш cost 10, а прод — cost 12 → bcrypt-сравнение быстрее → оракул частично оставался. Поправил на cost 12 + тест на префикс $2b$12$.

Не вошло (осознанно, отмечено ревью как SUGGESTION)

workspaceRepo.findFirst() на каждый Bearer-запрос не кешируется (лишний round-trip в single-workspace сборке) — принято как trade-off ради простоты/консистентности с Basic-путём.

Проверка

tsc --noEmit — без новых ошибок. jest src/integrations/mcp + contract spec — 71 passed.

🤖 Generated with Claude Code

Post-merge security-review follow-ups для mcp-per-user-auth (#13). Ветка от develop. Closes #24. ## Что сделано (6 подпунктов чек-листа) - **isInitializeRequest alignment (medium):** `isInitializeRequestBody` теперь делегирует SDK-предикату `isInitializeRequest` (тот же, что в packages/mcp/http.ts). Голый `{method:'initialize'}` без id/params больше не триггерит side-effecting `login()` (audit-spam + рост user_sessions) до того, как http.ts его 400-ит. - **trustProxy doc (medium):** заметка в `.env.example` — приложение с `trustProxy: true` доверяет X-Forwarded-For; деплоить за доверенным прокси, иначе per-IP throttling (вкл. /mcp brute-force) спуфится. (doc-only, runtime не тронут.) - **Bearer→workspace bind (low):** `verifyBearerAccess` отвергает токен с `payload.workspaceId != workspace инстанса` (резолв через `workspaceRepo.findFirst()`, как Basic-путь). Опциональный параметр → no-op когда не задан (single-workspace OSS). - **Timing-oracle (low):** в `verifyUserCredentials` ветка missing/disabled теперь делает bcrypt-compare против dummy-хеша. По замечанию ревью cost dummy-хеша выставлен **12** (== продакшен saltRounds), иначе тайминг не выровнен; тест проверяет `$2b$12$`. Сообщение CREDENTIALS_MISMATCH_MESSAGE сохранено (его матчит /mcp-лимитер). - **SSO/MFA тесты (low):** новый `mcp-basic-login-gate.spec.ts` гоняет РЕАЛЬНЫЙ `enforceBasicLoginGate` (SSO enforced / EE-MFA bundled vs not / user-MFA / workspace-enforced-MFA), вместо заглушки гейта. EE-модуль мокается через `jest.mock(..., {virtual:true})`. ## Косяк, пойманный на ревью Первая версия timing-fix использовала dummy-хеш cost 10, а прод — cost 12 → bcrypt-сравнение быстрее → оракул частично оставался. Поправил на cost 12 + тест на префикс `$2b$12$`. ## Не вошло (осознанно, отмечено ревью как SUGGESTION) `workspaceRepo.findFirst()` на каждый Bearer-запрос не кешируется (лишний round-trip в single-workspace сборке) — принято как trade-off ради простоты/консистентности с Basic-путём. ## Проверка `tsc --noEmit` — без новых ошибок. `jest src/integrations/mcp` + contract spec — 71 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 1 commit 2026-06-20 23:37:44 +03:00

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

Post-merge hardening from the #13 security review:
- isInitializeRequestBody now delegates to the SDK isInitializeRequest (same
  predicate as packages/mcp/http.ts), so a bare {method:'initialize'} with no
  id/params no longer triggers the side-effecting login() (audit-spam /
  user_sessions growth) before http.ts 400s it.
- Bind the Bearer path to the instance workspace: verifyBearerAccess rejects a
  token whose payload.workspaceId != the instance workspace (resolved via
  workspaceRepo.findFirst, consistent with the Basic path); optional param so
  it's a no-op when unset.
- Close the user-enumeration timing oracle in verifyUserCredentials: the
  missing/disabled branch now runs a bcrypt compare against a module-level dummy
  hash whose cost (12) matches production saltRounds, so both paths take one
  equal-cost bcrypt compare; the exact CREDENTIALS_MISMATCH_MESSAGE is preserved.
- Document the trusted-proxy requirement for the spoofable per-IP brute-force
  limiter in .env.example (trustProxy is on; deploy behind a trusted proxy).
- Add real-execution coverage for enforceBasicLoginGate (SSO enforced / EE-MFA
  bundled vs not / user-MFA / workspace-enforced-MFA) instead of stubbing the gate.

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

vvzvlad added 2 commits 2026-06-21 01:25:48 +03:00

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

test(mcp): keep real mcp-auth.helpers in gate spec mock (forward-compat with #49 )

Test / test (pull_request) Has been cancelled

730486ad12

After develop merged, mcp.service.ts calls decideBasicGate from mcp-auth.helpers.
The gate spec mocked the whole module returning only FailedLoginLimiter, so the
merged code crashed with 'decideBasicGate is not a function' (7/7 failing).
Spread jest.requireActual('./mcp-auth.helpers') so the real helpers are kept and
the gate exercises real logic; keep only FailedLoginLimiter stubbed so its
constructor runs without a real sweep timer.

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

Ghost merged commit eae68ba11f into develop

2026-06-21 01:28:10 +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

~~Ghost referenced this pull request 2026-06-21 02:05:01 +03:00~~

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

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#48