Repository navigation
feat(transport): verify a signed tenant context on every request - #82
Merged
Merged
Conversation
Phase 1 of the multi-user plan. The engine has never been able to tell one customer from another: RPG_MCP_TRANSPORT_TOKEN proves a caller is the web service, and SessionContext.sessionId was read straight from tool arguments with a `default` fallback, so tenant identity was whatever the caller — and therefore whatever the model — claimed it was. This establishes an identity the engine can trust, without yet consuming it. - storage/tenant-context.ts: AsyncLocalStorage-backed tenant scope. AsyncLocalStorage rather than a module-level variable is load-bearing: handlers are async and concurrent requests interleave at every await, so a plain variable would let one campaign resume holding another campaign's identity. requireTenant() throws rather than defaulting, so storage work cannot silently proceed unscoped. - transport/tenant-token.ts: HMAC-SHA256 verification of the x-rpg-tenant header the web host mints. Signs over the encoded payload segment so the two runtimes never need to agree on JSON key ordering. Returns a reason instead of throwing so the transport can log precisely while returning an opaque 401. - transport/http.ts: verifies the header before building the MCP server and wraps the whole handler subtree in runInTenant. An absent header is allowed and yields no scope — meta-tools (load_tool_schema, search_tools) are genuinely tenant-agnostic and the host loads schemas before any campaign exists. A header that is present but invalid is always rejected. Also carries the transport hardening from the same plan: startup now fails when RPG_MCP_TRANSPORT_TOKEN is unset rather than warning and serving /mcp unauthenticated, and the token is no longer accepted from the query string where proxies and access logs capture it. Both environments already set the variable, and nothing calls the HTTP transport with ?token=. No behaviour change for existing callers: nothing reads the tenant yet. That lands in Phase 2, when getDb() resolves a database per campaign.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mnehmos
added a commit
to Mnehmos/rpg-mcp-live
that referenced
this pull request
Aug 13, 2026
…d call (#222) Phase 1 of the multi-user plan (host side). Pairs with `Mnehmos/mnehmos.rpg.mcp` [#82](Mnehmos/mnehmos.rpg.mcp#82), which verifies the header this mints. ## Why The reference engine has no tenant isolation of its own, so the host has been compensating: `reference-engine-dm.ts` force-overwrites `worldId`/`partyId`/`sessionId` on outbound calls and gates `characterId` against an allowlist of ids the model legitimately learned this turn. That control is real and carefully built, but it lives *inside the request body*, alongside arguments the model produced. This moves tenant identity out of the payload entirely and into a signed header the model never sees and cannot influence. ## What **`reference-engine-tenant.ts`** — mints an HMAC-SHA256 token over `accountId`, `campaignId`, `worldId`, `partyId`, `iat`, `exp`. Short TTL (120s), since it is minted per outbound call. **`reference-engine-client.ts`** — takes the tenant as a **separate argument** to `callTool`, never a field on `args`, and signs it into `x-rpg-tenant`. It is omitted for tenant-agnostic meta-tools, which the engine serves unscoped, and omitted rather than faked when no secret is configured — failing closed at the engine beats inventing a context it would have to trust blindly. **adapter / dm** — tenant is derived from the authenticated `accountId`/`campaignId` plus the routing row, which is the only binding between a Lantern account and a set of reference-engine ids. `tenantFor()` keeps that derivation in one place across the nine adapter call sites. ## Risk **Low for this PR in isolation; the risk is in deploy ordering.** - Until `REFERENCE_ENGINE_TENANT_SECRET` is set, the host sends **no header** and behaviour is byte-for-byte unchanged. This PR is inert without configuration. - The engine must have `RPG_MCP_TENANT_SECRET` set **before** this ships with a secret configured, or every scoped call returns 401 and campaigns stop responding. Engine PR #82 tolerates an absent header, so the safe order is: merge #82 → set both secrets → merge this. - The secret must **differ per environment**. Staging and production currently share a transport token; repeating that here would let a staging compromise mint production tenant contexts. - No change to what the engine does with the header yet — nothing reads it for routing until Phase 2 makes `getDb()` resolve a database per campaign. This PR cannot change which data a call reaches. Rollback is a revert plus unsetting the secret; no data or schema is touched. ## Acceptance criteria - [x] A scoped `callTool` sends `x-rpg-tenant` carrying the authenticated `accountId`/`campaignId`. - [x] The tenant is a separate argument, never a field on `args`, so model output cannot influence it. - [x] The service auth token is still sent alongside it — the two answer different questions. - [x] Tenant-agnostic meta-tool calls (`load_tool_schema`) send **no** tenant header. - [x] With no secret configured, no header is sent and no unsigned context is invented. - [x] Two calls for different campaigns produce two distinct tokens; no cross-campaign reuse. - [x] Token verifies under the engine's algorithm and does not verify under a different secret. - [x] Existing behaviour unchanged when the secret is unset. ## Verification - `npm test` — 732 passed, no failures. - `npx tsc --noEmit` — exit 0. - `npm run build` — clean. 11 new tests, of which the load-bearing ones: - The signer test **verifies with raw crypto rather than calling the engine's verifier**, which lives in a separate repository. If either side changes the wire format without the other, one of the two suites fails. That is the only cross-repo contract check available short of an integration environment. - Asserts the token is two base64url segments with no `+`, `/`, or `=`, so it survives an HTTP header round-trip. - Asserts a token minted with one secret does not verify against another — the staging/production separation depends on it. - Client tests assert the header is present for scoped calls, carries the right campaign, sits alongside the service auth token, and is **absent** for meta-tool calls and when no secret is configured. Not covered: a live end-to-end call against a running engine with matching secrets. That is the deploy-time check, and it is why the ordering above matters. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
8 tasks done
zxrfire
pushed a commit
to zxrfire/mnehmos.rpg.mcp
that referenced
this pull request
Sep 8, 2026
…hmos#82) Phase 1 of the multi-user plan. The engine has never been able to tell one customer from another: RPG_MCP_TRANSPORT_TOKEN proves a caller is the web service, and SessionContext.sessionId was read straight from tool arguments with a `default` fallback, so tenant identity was whatever the caller — and therefore whatever the model — claimed it was. This establishes an identity the engine can trust, without yet consuming it. - storage/tenant-context.ts: AsyncLocalStorage-backed tenant scope. AsyncLocalStorage rather than a module-level variable is load-bearing: handlers are async and concurrent requests interleave at every await, so a plain variable would let one campaign resume holding another campaign's identity. requireTenant() throws rather than defaulting, so storage work cannot silently proceed unscoped. - transport/tenant-token.ts: HMAC-SHA256 verification of the x-rpg-tenant header the web host mints. Signs over the encoded payload segment so the two runtimes never need to agree on JSON key ordering. Returns a reason instead of throwing so the transport can log precisely while returning an opaque 401. - transport/http.ts: verifies the header before building the MCP server and wraps the whole handler subtree in runInTenant. An absent header is allowed and yields no scope — meta-tools (load_tool_schema, search_tools) are genuinely tenant-agnostic and the host loads schemas before any campaign exists. A header that is present but invalid is always rejected. Also carries the transport hardening from the same plan: startup now fails when RPG_MCP_TRANSPORT_TOKEN is unset rather than warning and serving /mcp unauthenticated, and the token is no longer accepted from the query string where proxies and access logs capture it. Both environments already set the variable, and nothing calls the HTTP transport with ?token=. No behaviour change for existing callers: nothing reads the tenant yet. That lands in Phase 2, when getDb() resolves a database per campaign.
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
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.
Phase 1 of the multi-user plan (engine side). Pairs with the host PR that mints the same header.
Why
The engine cannot currently tell one customer from another.
RPG_MCP_TRANSPORT_TOKENproves a caller is the web service; it says nothing about which customer a request is for.SessionContext.sessionIdwas read straight from tool arguments with a'default'fallback — so tenant identity was whatever the caller, and therefore whatever the model, claimed it was.This establishes an identity the engine can trust, without yet consuming it. Nothing reads the tenant for routing until Phase 2 makes
getDb()resolve a database per campaign.What
storage/tenant-context.ts— AsyncLocalStorage-backed tenant scope.AsyncLocalStorage rather than a module-level variable is load-bearing: handlers are async and concurrent requests interleave at every
await, so a plain current-tenant variable would let one campaign resume while another campaign's value is installed.requireTenant()throws rather than defaulting, so storage work cannot silently proceed unscoped.transport/tenant-token.ts— HMAC-SHA256 verification ofx-rpg-tenant. Signs over the encoded payload segment so the two runtimes never have to agree on JSON key ordering or whitespace. Returns a reason instead of throwing, so the transport logs precisely while returning an opaque 401 — distinguishing expired from forged helps an attacker more than it helps a client.transport/http.ts— verifies before building the MCP server, then wraps the whole handler subtree inrunInTenant.An absent header is allowed and yields no scope. This is deliberate:
load_tool_schemaandsearch_toolsare genuinely tenant-agnostic, and the host's catalog loads schemas before any campaign is in play. Those requests never reachrequireTenant(). A header that is present but invalid is always rejected — that is a caller asserting an identity it cannot prove.Also included
The transport hardening from the same plan:
RPG_MCP_TRANSPORT_TOKENis unset, rather than warning and serving/mcpunauthenticated. Verified both environments already set it.tokenquery parameter (onlywebsocket.tsstill supports it; untouched here).Tests
20 new tests. Notably:
campaignIdinto a validly-signed token and asserts rejection.timingSafeEqualthrows on unequal lengths, so that guard is tested rather than assumed.awaitpoints and asserts neither observes the other. That is the regression guard for AsyncLocalStorage over a module variable.Full suite: 2,332 passed / 7 skipped,
npm run buildclean.Deploy ordering
Safe to merge and deploy on its own — an absent header is allowed, so nothing changes until the host starts sending one.
RPG_MCP_TENANT_SECRETmust be set on this service before the host PR ships, or every scoped call returns 401.🤖 Generated with Claude Code