fix(mcp): harden tenant isolation and caller identity by chitcommit · Pull Request #180 · chittyapps/chittyfinance

chitcommit · 2026-10-03T13:04:59Z

Closes #179.

Changes

scopes finance://tenants to storage.getUserTenants(userId) rather than global getTenants()
accepts verified ChittyAuth bearer JWTs for channel-neutral actor binding
records auth method in request context
removes generic X-User-Id and ?userId= caller impersonation paths
preserves X-Chitty-User-Id only for the existing service-token compatibility lane
adds MCP membership-isolation and caller-context regression tests

Architecture

No new auth service, database, or schema. ChittyID/ChittyAuth remain identity authority; tenant_users remains financial authorization authority. This is the prerequisite for projecting the same ChittyFinance MCP capability into ChatGPT, Claude, and ChittyClaw/OpenClaw.

Validation

Branch-level diff reviewed. Repository CI is the execution proof for typecheck/tests; no deployment is included.

Summary by CodeRabbit

New Features
- MCP requests can now use verified ChittyAuth bearer tokens for user and agent access, while legacy service-token authentication remains supported.
- Tenant resources now show only tenants the authenticated caller belongs to, including their membership roles.
Security
- Tenant access is checked against caller membership. Generic user-ID headers and query parameters are no longer accepted to identify service-token callers.
Documentation
- Updated MCP endpoint and authentication guidance to use /mcp and clarify supported authentication methods.

chatgpt-codex-connector · 2026-10-03T13:05:12Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-03T13:08:42.606162Z	`54666dd`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-10-03T13:05:13Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 3c79b26b-ca19-4791-88fd-267def577e2e

📥 Commits

Reviewing files that changed from the base of the PR and between cefe04c and c8c5847.

📒 Files selected for processing (11)

AGENTS.md
CHARTER.md
CHITTY.md
SECURITY.md
server/__tests__/caller-context.test.ts
server/__tests__/mcp.test.ts
server/__tests__/middleware-caller.test.ts
server/env.ts
server/middleware/auth.ts
server/middleware/caller.ts
server/routes/mcp.ts

 ____________________________________________________________________________________________________________________________________
< Contrary to popular belief, Unix is user friendly. It just happens to be very selective about who it decides to make friends with. >
 ------------------------------------------------------------------------------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

claude · 2026-10-03T13:05:59Z

Review

The direction is right: scoping finance://tenants to getUserTenants(userId) and removing X-User-Id / ?userId= impersonation both close real holes. I read the diff and the surrounding wiring in server/app.ts, but I did not run the tests.

Must fix

server/__tests__/middleware-caller.test.ts will now fail. It builds an app with no authMethod set, and that file isn't in this PR.
- returns 404 when the caller is not found sends X-Chitty-User-Id. With no authMethod, that header is now ignored, so the response is 400 instead of 404.
- loads the caller from the fallback x-user-id header asserts the exact behaviour this PR removes.
Delete the x-user-id test, or flip it to expect 400. Set authMethod: 'service' in the harness for the header-based test. The new caller-context.test.ts duplicates much of this file, so merging the two would be tidier.

Should address

No tests for the new hybridAuth bearer-JWT path. It is the security-critical change and nothing covers it. Cases to add:
- a valid JWT whose sub has no local user returns 401
- an invalid JWT returns 401
- a service token still works
- the JWT path sets authMethod and userId
- a JWT carrying an X-Chitty-User-Id header cannot override the bound user. This already holds because the bound user wins, but it deserves a regression test.
The finance://tenants test only checks that the mock is called with user-1. It doesn't show membership scoping end to end.
JWT verification strictness. Bearer JWTs now authenticate API calls, not just browser cookies. Confirm verifyChittyAuthJWT enforces iss, aud (this service), exp and the allowed algorithms. Without an audience check, a token minted for another ChittyOS service could be replayed here.
Error semantics. A bearer whose sub has no linked local user now returns a bare unauthorized, where the cookie path used user_not_found. Consider keeping the more specific code for debuggability.
Tenant check for the /mcp routes. The tenant list is scoped by getUserTenants, but the other resources and tools still use tenantId from tenantMiddleware. Please confirm that middleware verifies membership of userId in the tenant. It is fail-closed since fix(tenant): fail closed when tenant membership cannot be verified #144, but nothing in this PR relies on that or tests it for /mcp.

Minor

The authMethod Variable is typed non-optional, but it is unset on some paths (for example when callerContext runs without auth in tests). Making it authMethod?: would be more honest.
resolveChittyAuthBearer uses Parameters<MiddlewareHandler<HonoEnv>>[0]. Importing Context<HonoEnv> is clearer.
Breaking change: any client using X-User-Id or ?userId= will now get 400. A short note in the PR would help.
readResource still types storage as any, which hides the getUserTenants shape. Typing it as SystemStorage would catch mock drift like this one.

Item 1 will break CI. The rest are hardening and coverage.

claude · 2026-10-03T13:06:54Z

Review

Good, focused hardening. Removing X-User-Id / ?userId= impersonation, scoping finance://tenants to getUserTenants(userId), and binding the actor to a verified sub all line up with the stated goal. verifyChittyAuthJWT pins issuer, audience and ES256, so the new bearer-JWT lane rests on a sound primitive.

Issues / suggestions

Service token plus X-Chitty-User-Id is still unconstrained impersonation. This is preserved intentionally, but any holder of CHITTY_AUTH_SERVICE_TOKEN can act as any user, and finance://tenants now trusts that header for membership scoping. Worth a follow-up to deprecate the lane, or at least log the authMethod=service plus claimed user pair for audit.
X-Chitty-User-Id isn't validated in the service lane. callerContext accepts whatever the header says. The new test mocks getUser, but there is no case for an unknown user ID with the service lane. Please add one, and check that callerContext rejects it rather than continuing.
Lost error granularity on cookie JWTs. resolveChittyAuthBearer merges "invalid or expired token" and "no local user for this ChittyID" into session_expired. The old user_not_found response was more useful for debugging and for the frontend. Consider keeping the distinction.
Bearer path returns an opaque 401. A valid ChittyAuth JWT whose sub has no local user gets a plain unauthorized. Agent clients will find it hard to tell that account linking is needed. A distinct user_not_found would help.
Failed bearer verification costs a JWKS lookup. A wrong service token is now treated as a JWT and sent to jose. createRemoteJWKSet caches, so the cost is small. If the bearer clearly isn't a JWT (no two dots), you could short-circuit to 401.
Test gaps.
- There are no tests for hybridAuth itself: the service token lane, a valid JWT, an invalid JWT, and a JWT with an unknown sub.
- There is no test that authMethod is set per lane.
- SECURITY.md now claims "All protected routes verify caller membership before accepting X-Tenant-ID". Please confirm tenantMiddleware really does this and add a test, or soften the doc.
Typing. Variables.authMethod is non-optional but is unset on some paths. Prefer authMethod?: so the undefined case you handle in callerContext is visible to the compiler.
Minor.
- storage: any and membership: any keep the existing looseness, and getUserTenants has a real return type you could use.
- Assert getTenants is not called in the MCP isolation test, so a regression that fetches all tenants and filters afterward would be caught.
- The PR relies on CI for execution, so I haven't run the tests locally.

No blocking security flaws found in the diff itself. I'd request tests for #2 and #6 before merge.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54666dd40b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-10-03T13:08:39Z

+    const { app } = buildApp({ authMethod: 'service' });
+    const res = await app.request('/', { headers: { 'X-Chitty-User-Id': 'user-1' } });


Update the existing caller middleware suite

The new coverage sets authMethod: 'service', but the unchanged server/__tests__/middleware-caller.test.ts still invokes callerContext without that context and expects both X-Chitty-User-Id and the now-removed X-User-Id fallback to work. Running pnpm test:run on this commit therefore fails two tests; update that existing suite to establish the service-auth context for the Chitty header case and to expect rejection for the generic header.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-10-03T13:08:39Z

+  const storage = c.get('storage');
+  const user = await storage.getUserByChittyId(claims.sub);
+  if (!user) return null;


Reject inactive local users in the bearer resolver

When a locally deactivated user still has a valid ChittyAuth JWT, this resolver accepts the user and the new bearer path proceeds to every protected route. Local account status is an established authentication control—the password login rejects inactive users in server/routes/session.ts:99-101, and the ChittyID callback rejects them in server/routes/chittyid-auth.ts:153-155—so the new direct bearer lane bypasses deactivation unless this lookup also rejects !user.isActive.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-10-03T13:08:39Z

+ * Hybrid auth:
+ * - Path 1a: service Bearer token → service-to-service compatibility
+ * - Path 1b: ChittyAuth Bearer JWT → cryptographically bound end-user/agent caller
+ * - Path 2a: ChittyAuth JWT cookie → browser session
+ * - Path 2b: KV cookie → legacy browser session


Document the new JWT bearer authentication lane

This adds ChittyAuth JWTs as valid Authorization bearer credentials, but the published contract in docs/contracts/chittybooks-chittyfinance.md:34 still explicitly says bearer authentication is service-to-service only, while the MCP registry still advertises only a bearer service token. Consumers following those contracts cannot discover the intended channel-neutral flow and may continue sending an impersonation header or cookie instead; update the authentication documentation with this behavior change.

AGENTS.md reference: AGENTS.md:L48-L48

Useful? React with 👍 / 👎.

claude · 2026-10-03T13:09:29Z

Review

Solid, well-scoped hardening. Scoping finance://tenants to getUserTenants(userId) fixes the cross-tenant enumeration. Removing X-User-Id and ?userId= closes the impersonation paths. Binding the actor to a verified sub is the right direction. It follows repo conventions (storage access via SystemStorage, tenant scope from middleware).

Issues / suggestions

Bearer fall-through to JWT verification (auth.ts). Any non-matching bearer now triggers JWKS verification. Previously a bad token was a cheap 401. Consider a cheap pre-check (three segments, ES256 header) first. Also confirm verifyChittyAuthJWT caches the JWKS and checks iss, aud and exp. The diff doesn't show this. The audience check matters most: a ChittyAuth JWT minted for another service shouldn't authorize MCP here.
Lost error distinction for cookie JWT sessions. user_not_found (valid JWT, no linked local account) is now folded into session_expired, and the cookie is deleted in both cases. That makes an unlinked ChittyID harder to debug, and it deletes a valid cookie. Consider keeping a distinct code or a log line, on the bearer path too.
getUserTenants doesn't filter inactive tenants. finance://tenants can list tenants with isActive: false. Check whether tenantMiddleware rejects those, because listing them here would then be inconsistent.
The service-token lane is still full impersonation. Anyone holding CHITTY_AUTH_SERVICE_TOKEN can act as any user via X-Chitty-User-Id. It's documented as compatibility, but consider a sunset follow-up and audit-logging the asserted user id.
Typing. readResource(..., storage: any) and (membership: any) hide the getUserTenants shape. Typing against SystemStorage would catch drift. Context<HonoEnv> would also be clearer than Parameters<MiddlewareHandler<HonoEnv>>[0] in resolveChittyAuthBearer.

Tests
Good coverage of callerContext and MCP membership isolation. Gaps:

No test for the new hybridAuth bearer-JWT path: linked user, unlinked user (401), invalid JWT (401), and a service token taking precedence.
No test that X-Chitty-User-Id is ignored when authMethod is chittyauth or session. That is the key security property, and it is only implied right now.
The MCP test mocks userId directly. An end-to-end test through hybridAuth, callerContext and /mcp would show the pieces compose.

Docs
/mcp matches app.ts. The PR relies on CI as validation, so please make sure typecheck and tests are green before merge.

The hybridAuth JWT tests and the audience-check confirmation are the main items I'd want before merging.

🤖 Generated with Claude Code

chitcommit added 6 commits October 3, 2026 08:03

fix(auth): track verified caller method

7677953

fix(auth): accept ChittyAuth bearer identity

28e416e

fix(auth): narrow service caller identity input

4d1779e

fix(mcp): scope tenant resource to caller memberships

face2f9

test(mcp): prove membership-scoped tenant enumeration

5732123

test(auth): reject unbound caller impersonation

54666dd

docs(agents): document bound MCP caller identity

b14917f

chitcommit added 3 commits October 3, 2026 08:06

docs(security): document MCP JWT and tenant isolation

1e3da92

docs(charter): add canonical MCP endpoint contract

2da6939

docs(chitty): correct MCP endpoint and auth

7d2adbb

chitcommit mentioned this pull request Oct 3, 2026

Add channel-neutral execution context for ChatGPT, Claude, and ChittyClaw #181

Closed

test(auth): align caller middleware tests with hardened service lane

c8c5847

chatgpt-codex-connector Bot reviewed Oct 3, 2026

View reviewed changes

chitcommit enabled auto-merge (squash) October 3, 2026 13:08

chitcommit merged commit 5f4939d into main Oct 3, 2026
14 of 16 checks passed

chitcommit deleted the fix/179-mcp-tenant-delegation branch October 3, 2026 13:20

chitcommit mentioned this pull request Oct 3, 2026

Remediate high-severity braces advisory GHSA-vfj7-8cjw-p6xm #183

Open

Navigation Menu

fix(mcp): harden tenant isolation and caller identity - #180

fix(mcp): harden tenant isolation and caller identity#180
chitcommit merged 11 commits into
mainfrom
fix/179-mcp-tenant-delegation

chitcommit commented Oct 3, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 3, 2026 •

edited

Loading

Uh oh!

claude Bot commented Oct 3, 2026

Uh oh!

claude Bot commented Oct 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Uh oh!

claude Bot commented Oct 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		const { app } = buildApp({ authMethod: 'service' });
		const res = await app.request('/', { headers: { 'X-Chitty-User-Id': 'user-1' } });

Conversation

chitcommit commented Oct 3, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Architecture

Validation

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Oct 3, 2026

Uh oh!

claude Bot commented Oct 3, 2026

Review

Issues / suggestions

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Oct 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chitcommit commented Oct 3, 2026 •

edited by coderabbitai Bot

Loading

chatgpt-codex-connector Bot commented Oct 3, 2026 •

edited

Loading

coderabbitai Bot commented Oct 3, 2026 •

edited

Loading