refactor(auth): swap tool catalogue to useBitrix24Tenant + §11 observability contract (PR-2d)#213
Merged
Merged
Conversation
When OAuth lands, "Bearer doesn't work" support tickets degenerate into
tail-the-logs guessing games unless every failure mode lands a single,
structured, grep-able log line with a stable event name AND surfaces
the same error code to the user. §11 nails down that contract before
PR-2c starts writing handlers, so observability isn't an afterthought.
Specifies:
- A per-request requestId threaded through ALS (same scope as the
tenant context) so one curl-paste-into-jq reconstructs the full
timeline of a failed install/callback/refresh.
- A fixed taxonomy of event names: oauth.install.deny.<reason>,
oauth.callback.deny.<reason>, oauth.callback.exchange.{ok,fail},
oauth.refresh.{ok,fail.invalid-grant,fail.transient},
mcp.auth.deny.bearer-{unknown,revoked,orphan}. The suffix doubles
as the user-visible errorCode (uppercase) shown in the rendered
HTML and the MCP 401 WWW-Authenticate header.
- A logger-redactor lint guard so a handler can't accidentally log a
raw OAuth URL bypassing the scrubber.
- GET /api/oauth/_health — operator-gated counts-only endpoint
(tenants, bearers, pending states, last refresh ok/fail).
- Debug-level traces gated by NUXT_LOG_LEVEL=debug — boolean state
comparisons and LRU cache hits/misses, no secrets.
- Explicit non-scope: no Prometheus / OpenTelemetry hookup in PR-2c.
Also bumps the section numbering (old §11 Future hardening → §12,
old §12 Open questions → §13) and fixes back-references at lines 32,
261, and the §13 Q4 callout.
Every tool handler and every shared helper that calls Bitrix24 now goes through the OAuth-aware dispatcher `useBitrix24Tenant()` instead of the webhook singleton `useBitrix24()` directly. With NUXT_BITRIX24_OAUTH_ENABLED unset or false (the production default), the dispatcher returns the same webhook singleton — byte-identical behaviour. When PR-2c lands and the flag is flipped, the same call sites resolve to per-tenant `B24OAuth` instances with zero further code changes here. Touched: - server/mcp/tools/tasks/*.ts — 18 task tools - server/mcp/tools/users/*.ts — current-user, find-user - server/utils/task-lifecycle.ts — shared helper covering 7 lifecycle tools - server/utils/checklist.ts — shared helper covering 3 checklist tools - server/mcp/tools/meta/submit-feedback.ts — UNTOUCHED per §2 non-goal #5 (uses a GitHub PAT, not portal-bound) Skipped on purpose: - server/utils/bitrix24.ts — the webhook singleton itself, called by the dispatcher fallback branch. Its JSDoc now points readers to `useBitrix24Tenant` so new code doesn't reach for it by reflex. - server/utils/bitrix24-tenant.ts:44 — the one place that legitimately calls `useBitrix24()` (the dispatcher's webhook fallback). Tests: - New `tests/_setup.ts` stubs `useRuntimeConfig` and `defineNitroPlugin` with safe defaults so every tool test that transitively imports the dispatcher loads without `ReferenceError: useRuntimeConfig is not defined`. Per-test stubs (token-store, oauth-schema, bitrix24-tenant) still win via vitest's per-file `vi.stubGlobal`. - vitest.config.ts wires the setup via `test.setupFiles`. - Full suite green (552 passed) — no behavioural change with the flag off.
Five Sonnet agents (docs/programmer/tester/security/CTO) ran in parallel on the initial PR-2d push (ebb467a). Consolidated findings, fixed in this PR (no follow-up issues — all in scope): Security #2 — information disclosure via dispatcher throw `useBitrix24Tenant()` previously embedded `memberId` and `userId` in the "OAuth path not yet implemented" Error message. The MCP toolkit forwards a tool-handler throw to the agent (Claude/Cursor) as plaintext, so the tenant ids would have leaked across the wire on every flag-on request before PR-2c. Split: log through `useLogger().error(...)` (operator-visible, structured payload), throw a generic message (agent-visible). Tests in `bitrix24-tenant.test.ts` re-pinned: cross- tenant leak guard now asserts on `loggerError.mock.calls`, not on the thrown message. Programmer #5 — `TenantContext` type vs §11 ALS contract §11 promised `requestId` in ALS but `TenantContext` only had `{memberId, userId}`. Added `requestId?: string` as forward-compat — PR-2c populates it inside the middleware wrap without breaking any existing test fixture that constructs `TenantContext` with just two fields. §7 updated to describe the new shape. Tester #2 — no cross-tenant guard at tool level Added a `b24_user_me` test that flips `OAUTH_ENABLED=true` and asserts the tool throws (not silent webhook fallback). Closes the cross-tenant leak class through the catalogue, not just the dispatcher unit. Docs/CTO #1 — skill files teach obsolete pattern (BLOCKER) `skills/manage-bx24-template-mcp/{SKILL.md, adding-tools.md}` taught `useBitrix24()` as the canonical pattern. Any new tool added by a contributor following the skill would re-introduce the direct webhook call and undo PR-2d. Skills now teach `useBitrix24Tenant()`, including the test-mock pattern (`vi.mock('~/server/utils/bitrix24-tenant', …)`). Last-reviewed stamps bumped to 2026-06-04. CTO #2 — §10 rollout order not updated §10 originally said PR-2 → PR-3 → PR-4. Actual landing is PR-2a → PR-2b → PR-2d (this) → PR-2c. Added an alias table and rewrote each step to match reality so a future reviewer of PR-2c doesn't read a stale plan. Docs/Tester #1 — test stub contract clarity `tests/_setup.ts` JSDoc now spells out that per-file `vi.stubGlobal` overrides MUST live at module level (not inside `describe`), and documents why `defineNitroPlugin` is global. Moved `token-store.test.ts` singleton-suite stub from a `describe` body to module level to match. Security #3 — logger-redactor lint rule scope §11 originally promised one AST-based lint rule. Lint cannot catch template-literal or nested-object cases by syntax alone — §11 now spells out four fixture shapes the runtime redactor MUST cover and flags lint as a best-effort secondary defence, not the primary one. Security #4 — `/api/oauth/_health` authentication Re-using `NUXT_MCP_AUTH_TOKEN` (the agent's token) for an operator-tier endpoint is a privilege-separation smell. §11 now documents two acceptable patterns: nginx-level allow/deny on the route (recommended) OR a dedicated `NUXT_OAUTH_ADMIN_TOKEN`. The route fails closed if neither is configured; reusing the agent token is explicitly disallowed. Docs #5 — current-user.ts description TODO Added a TODO pointing at §13 Q2 — under OAuth the tool returns the Bearer-owning user, not the webhook owner. Description update is scheduled as a one-liner in PR-2c follow-up. Docs #4 — §8 #8 PR-3 vs PR-2c contradiction §8 said PR-3 extends the redactor; §11 said PR-2c does. Reconciled to PR-2c (redactor is a precondition for any OAuth log line). Out-of-scope follow-ups NOT done in this PR (low value or scope-correct): - Programmer #1 (double-dispatch perf concern): false alarm — runOne / runBatch are mutually exclusive. - Programmer #4 (webhook singleton memoised): confirmed, no action. - Tester #3 (object-identity test for double-dispatch tools): cosmetic. - Tester #4 (defineNitroPlugin duplication): documented as deliberate redundant pin. - CTO #5 (`tests/_setup.ts` long-term arch): kept as-is, JSDoc warns future authors not to extend it for per-file globals. Test stats: 553 passed (+1 cross-tenant guard test), typecheck clean, eslint clean.
This was referenced Jun 5, 2026
IgorShevchik
changed the title
Five Sonnet agents (docs/programmer/tester/security/CTO) ran in parallel on the round-2 HEAD (90c50fc). Consolidated findings, fixed in this PR. Docs #1 — CONTRIBUTING.md teaching obsolete pattern (BLOCKER, round-2 missed this file): Lines 109-110 still said `useBitrix24()` + `mocking useBitrix24`. The most visible "first stop" for new contributors. Now teaches `useBitrix24Tenant()` with the same "never bypass" caveat the SKILL files use, plus the new mock pattern. Docs/Programmer #1 (duplicate) — §7 phantom API: Round-2's §7 update inadvertently introduced `runWithRequestContext(event, fn)`, which does not exist; the real export is `runWithTenant(ctx, fn)`. Corrected. Docs #3 — stale PR-3 reference at §9 line ~282: Round-2 reconciled §8/§11 to PR-2c for the redactor, but missed the `PR-3 adds msw` line in §9. Same numbering fix. Docs #4 — §13 Q-numbering doesn't exist: `current-user.ts` TODO referenced `§13 Q2`; §13 uses plain numbered items (1/2/3), no Q-prefix. Same for §2 line 30's `§13 Q4` reference (the DXT/OOB question was Resolved in round-2, no longer "Q4"). Both references now point at the right place. Security #1 — "operator-visible" claim needed qualification: `useLogger().error(...)` writes to STDOUT — visible to `docker logs`, log-shippers, and downstream aggregators (NOT just an operator SSH-ing in). §11 and the dispatcher comment now spell this out, and contrast it with the audit-log JSONL (PR-2b) which has tighter file perms and is the canonical compliance store. Security #2 — `/api/oauth/_health` "fails closed" was just a doc promise: §11 now includes concrete PR-2c pseudocode for the gate (503 when neither nginx-isolation nor `NUXT_OAUTH_ADMIN_TOKEN` is configured; 401 on wrong admin token; 200 only on the happy path) and a mandatory CI test list pinning all four cases. Without these tests the route ships open on first deploy. Security #3 — PR-2c commit ordering enforcement: §11 now mandates that the redactor extension lands as the FIRST commit inside PR-2c, with zero OAuth-logging callers, and the redactor fixture tests go green before any caller exists. Reversing this order means a reviewer is asked to mentally redact while the code still passes raw URLs through — exactly the class of mistake the redactor is supposed to prevent. Tester #1 — N=10 concurrent test now checks bijection, not just match: The previous `.find()`-only assertion would silently pass on a "duplicate tenant + missing tenant" bug. Added a `new Set(seenMemberIds).size === 10` check so each tenant must appear exactly once. Tester #2 + Tester #4 + Security #4 + Programmer #2 — current-user.test.ts flag-on guard hardening: - Save the original `useRuntimeConfig` stub via `globalThis` and restore it in `afterEach` so test isolation survives a future `_setup.ts` shape extension (no more hardcoded `{bitrix24OauthEnabled:false}` in the restore path). - Mock `useLogger` so the dispatcher's `useLogger().error(...)` call doesn't materialise the real SDK logger and write to stderr during the test run. - Split the single regex-OR test into TWO tests: one without a tenant scope (must throw "outside a tenant scope", the wire-up bug branch), one inside `runWithTenant` (must throw "not yet implemented", the PR-2c-pending branch). Strict matchers on the exact message stop a future regression that removes one branch from being masked by the other branch matching the same regex. Follow-up issues (NOT done in this PR — explicit decision with the project owner): - #214 — PR-2c MUST populate requestId unconditionally + ship a throwing getRequestId() helper. CTO #1's "optional field invites forgot-to-set bugs" concern → guarded at the PR-2c scope. - #215 — Migrate ~30 tool tests to mock useBitrix24Tenant instead of useBitrix24. Mechanical, soft blocker for the test:unit:oauth CI matrix that lands with PR-2c. Out-of-scope follow-ups NOT done (low value or already covered): - Programmer false alarms: 7/7 verified clean (singleton memoised, no logger circular import, vi.resetModules preserves globalThis stubs, finally always runs after rejected await, etc.). - CTO #4 (3-commit squash) — PR title now updated to cover both swap AND §11 observability so the squashed commit message stays accurate. Test stats: 554 passed (+1 from the split test), typecheck clean, eslint clean.
PR-2d touched both SKILL.md and adding-tools.md (teaching the new useBitrix24Tenant pattern). The round-2 fix bumped the stamps to 2026-06-04, but the merge slips a day — refresh to 2026-06-05 so "last reviewed" tracks the actual merge into main.
IgorShevchik
pushed a commit
that referenced
this pull request
Jun 5, 2026
…ep 8) `useBitrix24Tenant()`'s OAuth-on branch previously threw "OAuth path not yet implemented (lands in PR-2c)" with the tenant ids on the operator-side logger. This commit replaces the throw with the real `useBitrix24OAuth(tenant.memberId, tenant.userId)` call — the per-tenant B24OAuth factory from step 7. Tools that have been routing through the dispatcher since PR-2d (#213) now resolve to per-tenant clients when the flag is on, without a single tool-side edit. Wired changes: - `server/utils/bitrix24-tenant.ts`: - Import `useBitrix24OAuth` from `~/server/utils/bitrix24-oauth`. - OAuth-on + tenant-bound path now `return useBitrix24OAuth(memberId, userIdNum)`. - `tenant.userId` is a string in `TenantContext` (matches audit-log shape); the factory expects a number. Defensive `Number.parseInt` with a `Number.isFinite` guard that throws + logs `oauth.tenant.dispatch.bad-user-id` if a non-numeric value slips through. In practice the Bearer middleware will only construct `TenantContext` from a verified `findByBearerHash` result whose `user_id` column is INTEGER NOT NULL, so this is belt-and-braces. - JSDoc rewritten — the "not yet implemented" caveat is gone; the behaviour description now matches the wired-up reality. - The no-tenant-scope branch also logs via the structured logger now (`oauth.tenant.dispatch.no-tenant-scope`) for consistency with the bad-user-id branch. - `server/api/oauth/_health.get.ts`: - Import `_readRefreshStatus` from `~/server/utils/bitrix24-oauth`. - `lastRefreshOk` / `lastRefreshFail` now reflect the factory's process-local tracker (both `null` on a fresh process, correctly signalling "no refresh has been attempted yet"). The log payload includes the refresh status fields alongside counts so a single `oauth.health.ok` log line carries the full health snapshot. Tests: - `tests/unit/utils/bitrix24-tenant.test.ts`: - Mock `~/server/utils/bitrix24-oauth` with a sentinel client so the dispatcher's tenant-to-client resolution is controllable without seeding a real SQLite store from inside the dispatcher's unit test. - Replaced "throws not yet implemented" tests with the corresponding wired-up assertions: factory called with correct args, dispatcher returns the factory's result. - Added a non-numeric-userId guard test. - The N=10 concurrent test now pins the bijection on factory call arguments instead of error-message payloads (cross-tenant leak guard survives at the new layer). - `tests/unit/tools/users/current-user.test.ts`: - The "tenant-scope-bound branch" test now asserts on the factory's `row missing` error (the realistic failure with no oauth_tokens seed) rather than the now-removed "not yet implemented" string. Total: 640 passed (+1 dispatcher test rename, -1 factory wiring replaces the stub). Refs: §7, §11, #214.
This was referenced Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two changes, both default-off-safe.
1.
docs/OAUTH-DESIGN.md §11— observability/debugging contract for PR-2c. Before PR-2c starts writing handlers, this section nails down the log taxonomy, error-code surface, health endpoint, and redactor invariants — so "Bearer doesn't work" support tickets land with a specificerrorCode(e.g.STATE-COOKIE-MISMATCH,BEARER-REVOKED) instead of a tail-the-logs guessing game. Per-requestrequestIdthreaded through ALS; debug-level traces gated byNUXT_LOG_LEVEL=debug. Explicit non-scope: no Prometheus/OTel hookup.2. PR-2d tool swap:
useBitrix24()→useBitrix24Tenant(). Every tool handler and every shared helper that touches Bitrix24 now goes through the OAuth-aware dispatcher from PR-2a (#209). With the flag default (off), the dispatcher returns the same webhook singleton — byte-identical behaviour. When PR-2c lands and someone flipsNUXT_BITRIX24_OAUTH_ENABLED=true, these call sites resolve to per-tenantB24OAuthinstances with zero further code changes here.What's in
server/mcp/tools/tasks/*.ts(18 files)useBitrix24import + call site swappedserver/mcp/tools/users/{current-user,find-user}.tsserver/utils/task-lifecycle.tsserver/utils/checklist.tsserver/utils/sdk-helpers.ts@param b24 — client from useBitrix24()→useBitrix24Tenant()server/utils/bitrix24.tstests/_setup.ts(new)useRuntimeConfig+defineNitroPluginvitest.config.tstest.setupFilesdocs/OAUTH-DESIGN.mdWhat's deliberately NOT touched
server/mcp/tools/meta/submit-feedback.ts— per §2 non-goal feat(tools): task lifecycle + rating — 7 v3 wrappers (start/pause/complete/approve/disapprove/defer/renew) + rate_task #5, it uses a GitHub PAT (not portal-bound) and stays on the webhook code path. Sanity-checked: it doesn't importuseBitrix24at all today.server/utils/bitrix24.ts(useBitrix24itself) — still exists as the webhook singleton called by the dispatcher's fallback branch. Its JSDoc now redirects new callers touseBitrix24Tenant.server/utils/bitrix24-tenant.ts:44— the one legitimate caller ofuseBitrix24()(the dispatcher's webhook fallback).Why one PR (not the 4a/4b/4c split from §10)
The §10 split exists "to keep blast radius small" — but with the flag default-off, blast radius IS zero. The mechanical swap is ~23 files × 2 lines each; splitting would just triple the merge ceremony. The split lands if/when this PR's review surfaces a real domain-specific concern; until then, ship as one.
Drives
Test plan
npx vitest run— 552 passed, 1 skipped, 2 todo. Zero changes (the swap is byte-identical with the flag off, andtests/_setup.tsstubs let every tool test that transitively imports the dispatcher load withoutReferenceError).npx nuxt typecheck— clean.npx eslint .— clean.grep -rn "useBitrix24\b" server/mcp/tools server/utils/{task-lifecycle,checklist}.ts | grep -v Tenant— empty. All swapped.pnpm devagainst a webhook config still works (any tool call resolves through the webhook flow as today).NUXT_BITRIX24_OAUTH_ENABLED=truebut no tenant context (PR-2c not yet wired) → any tool call throws the deliberate "wire-up pending" error frombitrix24-tenant.ts, confirming the flag is loud not silent.No BREAKING CHANGE in code or runtime behaviour. For Changelog:
Refs / order
main.useBitrix24Tenantdispatcher) — inmain.main.B24OAuthfactory + refresh + logger-redactor extension +/api/oauth/_healthper §11).Generated by Claude Code