fix(security): eliminate cross-tenant server-ref misroute in elicitation

[asachs01]

Summary

P1 security fix — replicated from the reference PR halopsa-mcp#65 (6-repo remediation wave, task_1784935505749). The identical bug exists byte-for-byte in this repo's src/utils/server-ref.ts.

The bug

The "server reference" pattern used by MCP elicitation/confirmation helpers (src/utils/elicitation.ts: elicitSelection / elicitText / elicitConfirmation) was implemented as a module-level singleton:

let _server: Server | null = null;
export function setServerRef(server: Server): void { _server = server; }
export function getServerRef(): Server | null { return _server; }

setServerRef(server) was called synchronously inside createMcpServer() (src/mcp-server.ts), which — in gateway/HTTP mode — is invoked once per inbound request (src/index.ts, src/worker.ts). getServerRef() was then read back later, deep inside async tool handlers, after arbitrary await gaps (e.g. after awaiting a Syncro API call, right before sending an elicitation/confirmation prompt).

Misroute mechanism: two concurrent tenant requests race through the shared global:

1. Tenant A's request creates server A, calls setServerRef(serverA), and starts awaiting async work (e.g. a Syncro API call inside a tool handler).
2. Before A resumes, tenant B's request arrives, creates server B, calls setServerRef(serverB) — overwriting the shared ref.
3. A's awaited work resolves; A calls elicitConfirmation(...), which reads getServerRef() — and gets server B.
4. Tenant A's confirmation/elicitation prompt is sent down tenant B's connection instead of A's (or vice versa, depending on timing).

Same shared-mutable-state-across-await-gaps bug class as the credential-leak fixes (liongard-mcp#58, ninjaone-mcp#71) and the identical server-ref fix in halopsa-mcp#65, but in this repo's server/transport routing subsystem for elicitation, not credential/token caching.

The fix

Replaced the module-level singleton with an AsyncLocalStorage<Server> context in src/utils/server-ref.ts, mirroring this repo's existing per-request credential isolation pattern (credentialStore in src/utils/credential-store.ts):

• runWithServerRef(server, fn) — binds server to the async context for the duration of fn (including everything it awaits/schedules). Used for per-request transports: src/index.ts (Node HTTP handleMcp, wrapping the server.connect(transport).then(...) chain) and src/worker.ts (Cloudflare Workers handleMcp), one call per inbound request.
• bindServerRef(server) — binds server for the remainder of the current + all future async execution, via AsyncLocalStorage.enterWith. Used only in src/index.ts's stdio path, which is single-session (one process = one caller, no concurrent tenants to isolate from).
• getServerRef() — reads from the ALS context instead of a module variable.

createMcpServer() (src/mcp-server.ts) no longer calls setServerRef at all — binding is now the caller's responsibility, at the point where the per-request (or per-session) scope is established.

There is zero module-level mutable state holding a tenant/request-specific server or transport reference after this fix.

Repo shape vs. the reference PR

This repo's index.ts and worker.ts control flow matched halopsa-mcp#65's shape closely enough to apply the identical wrapping pattern directly:

• src/index.ts's Node HTTP path uses the same server.connect(transport).then(() => transport.handleRequest(req, res)) .then()-chain style as halopsa's pre-fix code, and the same stdio single-session startup shape — both wrapped exactly as in the reference PR (runWithServerRef around the .then() chain; bindServerRef once at stdio startup).
• src/worker.ts's handleMcp already used async/await throughout (no .then()), so it was wrapped with runWithServerRef(server, async () => { ... }) around the full connect/handleRequest/close body — same approach as halopsa's Workers entrypoint, adapted to this repo's existing structure (no .then() needed).

No structural deviation was required — the same three call sites (Node HTTP handler, Workers fetch, stdio startup) needed the same three bindings.

Test

tests/utils/server-ref.test.ts adds a regression test meeting the 3-part standard (forced interleave, per-tenant value assertions, no swallowed assertions):

1. Forced interleave (deterministic, not timing-based): a manually-resolved "gate" promise. Tenant A binds its server (runWithServerRef(serverA, ...)) and suspends on await gate.promise — simulating an in-flight Syncro API call. Tenant B's entire request (bind, elicit, resolve) then runs to completion while A is still suspended — exactly like a second concurrent HTTP request racing in. Only then is the gate resolved and A allowed to resume and call elicitConfirmation.
2. Per-tenant value assertions: each tenant gets its own mock Server with its own elicitInput spy (tagged tenantId). The test asserts, by value, that tenant A's message was received by tenant A's mock (serverA.elicitInput) and tenant B's by tenant B's mock — plus explicit negative checks that A's message never reached B's mock and vice versa.
3. No swallowed assertions: all assertions are made after awaiting both tenant promises to completion; none are inside a callback that could silently never fire.

Verified the test actually catches the bug: temporarily reinstated a module-singleton implementation behind the same function names (runWithServerRef / bindServerRef / getServerRef) and reran just this test file:

FAIL  tests/utils/server-ref.test.ts > ... forced deterministic interleave ...
AssertionError: expected 'tenant-B' to be 'tenant-A' // Object.is equality

— the exact predicted misroute symptom: tenant A's resumed continuation reads tenant B's server. Restored the ALS-based fix and reran the full suite: green.

Test plan

• npm run typecheck — clean
• npm run lint — clean
• npm run build — clean
• npm test — 151/151 passing (147 pre-existing + 4 new in server-ref.test.ts)
• New regression test verified to fail against a reinstated module-singleton implementation, with the exact predicted wrong-tenant-routing symptom (expected 'tenant-B' to be 'tenant-A')
• New regression test verified to pass against the ALS-based fix
• CHANGELOG.md updated under ### Security (Keep a Changelog format), matching the detail level of halopsa-mcp#65

🤖 Generated with Claude Code

[asachs01]

Warden — Gate 3 verification: GREEN (template replication, verified not blind-pasted)

server-ref.ts matches the blessed halopsa-mcp#65 template (same ALS mechanism, adapted comments). Confirmed the wrap lands on syncro's actual entry points, not a near-miss:

• index.ts:116 — runWithServerRef(server, () => { server.connect(transport).then(() => transport.handleRequest(req, res)) }), single .then(), same shape as halopsa/atera — wraps the real handleMcp closure used from both credentialStore.run(gatewayCreds, handleMcp) (gateway mode) and called bare (env mode).
• worker.ts:82 — runWithServerRef(server, async () => {...}), plain async/await, same shape as halopsa.
• index.ts:175 — bindServerRef(server) in the stdio-only path.
• grep -rn setServerRef src/ returns zero hits outside comments — old function fully removed.

Tests: CI green. Ran myself: 14 files / 151 tests green, matches dev's count.

Negative control: reinstated the module singleton behind identical function names — failed immediately: expected 'tenant-B' to be 'tenant-A'. Reverted, reconfirmed 151/151 green.

Verdict: GREEN — ship. This closes the secondary-class Gate-3 queue — all 5 (atera, datto-rmm, itglue, superops, syncro) verified.

[github-actions]

🎉 This PR is included in version 1.4.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀