fix(mcp): scope governance audit reads to the caller's tenant

Reported by Hacktron (medium): the MCP governance audit tools read the
mcp_changes collection with no organization predicate. Because
isSuperUser is a PER-ORG signal on multi-org (EE) deployments
(MANAGE_ORGANIZATION on the caller's own org) and email/actorId is only
per-org unique (Migration067), one tenant's admin — or a colliding
email — could read another tenant's MCP change history via
list_all_changes / get_any_change (and, on collision, list_changes /
get_change).

Fix (underlying cause, fail-closed at every layer):
- McpChangeRecord carries organizationId, stamped on every record at
  write from the session's tenant.
- All four store reads (getChange, listChanges, getAnyChange,
  listAllChanges) filter by the caller's organization; indexes are
  org-prefixed. Pre-fix records (no org) match no org-scoped read.
- Every audit-read tool refuses with organization_scope_unavailable when
  the caller's org is unknown, so the empty-org sentinel never runs a
  read and two tenants can never alias on it — regardless of edition or
  server version. Governed writes are unaffected.
- The caller's tenant is surfaced via a new organizationId on
  /api/v1/users/me (UserProfileCE_DTO + buildUserProfileDTO), the
  necessary tenant source for the MCP layer.

Regression tests fail on the unpatched code (verified by reverting the
filter): real-Mongo cross-tenant isolation (store.integration), always-on
tool-layer isolation + fail-closed for admin and actor reads (app.test),
a coordinator assertion that org is stamped, and a Java assertion the
profile carries organizationId.

Council-reviewed (security): APPROVE — reported disclosure fixed and
fail-closed at every read path. Ship the server + MCP changes atomically:
audit isolation for actor reads is complete only against a server that
reports organizationId; otherwise audit reads degrade to unavailable
(safe), never a cross-tenant read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N19EppQVZ55hwByVy4HEX8

Author: salevine

app/client/packages/mcp/src/app.test.ts

@@ -301,9 +301,12 @@ describe("Appsmith API client", () => {
 });
 
 function createApi(
-  validateToken = jest.fn(async () => ({
+  // Typed as the real wrapper (() => Promise<unknown>) so a test can pass a custom profile mock without every
+  // field — the default carries a realistic organizationId so governed audit reads are tenant-scoped.
+  validateToken: AppsmithApi["validateToken"] = jest.fn(async () => ({
     username: "user@appsmith.com",
     isAnonymous: false,
+    organizationId: "org-default",
   })),
 ) {
   return (): AppsmithApi => ({
@@ -2647,23 +2650,43 @@ describe("governance-wrapped layout mutations", () => {
     async getChange(
       id: string,
       actorId: string,
+      organizationId: string,
     ): Promise<McpChangeRecord | undefined> {
-      return this.changes.find((c) => c.id === id && c.actorId === actorId);
+      return this.changes.find(
+        (c) =>
+          c.id === id &&
+          c.actorId === actorId &&
+          c.organizationId === organizationId,
+      );
     }
     async listChanges(
       actorId: string,
+      organizationId: string,
       limit: number,
     ): Promise<McpChangeRecord[]> {
       return this.changes
-        .filter((c) => c.actorId === actorId)
+        .filter(
+          (c) => c.actorId === actorId && c.organizationId === organizationId,
+        )
         .slice(-limit)
         .reverse();
     }
-    async getAnyChange(id: string): Promise<McpChangeRecord | undefined> {
-      return this.changes.find((c) => c.id === id);
+    async getAnyChange(
+      id: string,
+      organizationId: string,
+    ): Promise<McpChangeRecord | undefined> {
+      return this.changes.find(
+        (c) => c.id === id && c.organizationId === organizationId,
+      );
     }
-    async listAllChanges(limit: number): Promise<McpChangeRecord[]> {
-      return this.changes.slice(-limit).reverse();
+    async listAllChanges(
+      organizationId: string,
+      limit: number,
+    ): Promise<McpChangeRecord[]> {
+      return this.changes
+        .filter((c) => c.organizationId === organizationId)
+        .slice(-limit)
+        .reverse();
     }
   }
 
@@ -3283,9 +3306,15 @@ describe("governance-wrapped layout mutations", () => {
 
   it("list_all_changes: an admin sees cross-actor records; a non-admin is refused (M2-T2)", async () => {
     const store = new MemoryGovernanceStore();
-    const mk = (id: string, actorId: string, ms: number): McpChangeRecord => ({
+    const mk = (
+      id: string,
+      actorId: string,
+      organizationId: string,
+      ms: number,
+    ): McpChangeRecord => ({
       id,
       actorId,
+      organizationId,
       entityKey: "e",
       operation: "op",
       revisionBefore: "",
@@ -3295,10 +3324,12 @@ describe("governance-wrapped layout mutations", () => {
       summary: {},
     });
 
-    await store.saveChange(mk("c1", "alice@appsmith.com", 1000));
-    await store.saveChange(mk("c2", "bob@appsmith.com", 2000));
+    // Two records in org "acme" and one in a DIFFERENT tenant "globex" — the acme admin must never see globex.
+    await store.saveChange(mk("c1", "alice@appsmith.com", "acme", 1000));
+    await store.saveChange(mk("c2", "bob@appsmith.com", "acme", 2000));
+    await store.saveChange(mk("c3", "carol@globex.com", "globex", 3000));
 
-    // Non-admin (default validateToken has no adminSettingsVisible) -> refused, no records leaked.
+    // Non-admin (default validateToken reports no isSuperUser) -> refused, no records leaked.
     const nonAdmin = createMcpHttpServer(API_BASE_URL, createApi(), {
       governance: new McpGovernanceCoordinator(store),
     });
@@ -3307,14 +3338,15 @@ describe("governance-wrapped layout mutations", () => {
     expect(denied.body.code).toBe("admin_required");
     expect(denied.body.changes).toBeUndefined();
 
-    // Admin (validateToken reports isSuperUser: true) -> sees BOTH actors' records.
+    // Admin of org "acme" (isSuperUser is a PER-ORG signal in EE) -> sees acme's cross-actor records ONLY.
     const adminServer = createMcpHttpServer(
       API_BASE_URL,
       createApi(
         jest.fn(async () => ({
           username: "admin@appsmith.com",
           isAnonymous: false,
           isSuperUser: true,
+          organizationId: "acme",
         })),
       ),
       { governance: new McpGovernanceCoordinator(store) },
@@ -3325,6 +3357,10 @@ describe("governance-wrapped layout mutations", () => {
       "c1",
       "c2",
     ]);
+    // Cross-tenant isolation: globex's record is NEVER returned to the acme admin (the reported vulnerability).
+    expect(all.body.changes.some((c: { id: string }) => c.id === "c3")).toBe(
+      false,
+    );
     // Cross-actor audit MUST attribute each record to its actor.
     const byId = Object.fromEntries(
       all.body.changes.map((c: { id: string; actorId: string }) => [
@@ -3338,13 +3374,20 @@ describe("governance-wrapped layout mutations", () => {
     // The rollback SNAPSHOT object is never exposed even to an admin (only the rollbackAvailable boolean is).
     expect(all.body.changes[0].rollback).toBeUndefined();
 
-    // get_any_change: admin can fetch a specific cross-actor record (attributed); non-admin is refused.
+    // get_any_change: acme admin can fetch an acme record but NOT globex's, even knowing its id.
     const anyChange = await callTool(adminServer, "get_any_change", {
       changeId: "c2",
     });
 
     expect(anyChange.body.change.actorId).toBe("bob@appsmith.com");
 
+    const crossTenant = await callTool(adminServer, "get_any_change", {
+      changeId: "c3",
+    });
+
+    expect(crossTenant.body.change).toBeUndefined();
+    expect(crossTenant.body.error).toBe("change not found");
+
     const deniedGet = await callTool(nonAdmin, "get_any_change", {
       changeId: "c2",
     });
@@ -3357,6 +3400,71 @@ describe("governance-wrapped layout mutations", () => {
     expect(own.body.changes).toEqual([]);
   });
 
+  it("list_all_changes / get_any_change fail closed when the caller's organization is unknown (M2-T2 tenant isolation)", async () => {
+    const store = new MemoryGovernanceStore();
+
+    await store.saveChange({
+      id: "c1",
+      actorId: "alice@appsmith.com",
+      organizationId: "acme",
+      entityKey: "e",
+      operation: "op",
+      revisionBefore: "",
+      revisionAfter: "",
+      createdAt: new Date(1000),
+      rollback: {},
+      summary: {},
+    });
+
+    // Admin WITHOUT an organization on the profile (e.g. an older server that does not report it). The audit read
+    // cannot be tenant-scoped, so it must refuse rather than run an unscoped cross-tenant query.
+    const adminNoOrg = createMcpHttpServer(
+      API_BASE_URL,
+      createApi(
+        jest.fn(async () => ({
+          username: "admin@appsmith.com",
+          isAnonymous: false,
+          isSuperUser: true,
+        })),
+      ),
+      { governance: new McpGovernanceCoordinator(store) },
+    );
+
+    const listed = await callTool(adminNoOrg, "list_all_changes", {});
+
+    expect(listed.body.code).toBe("organization_scope_unavailable");
+    expect(listed.body.changes).toBeUndefined();
+
+    const fetched = await callTool(adminNoOrg, "get_any_change", {
+      changeId: "c1",
+    });
+
+    expect(fetched.body.code).toBe("organization_scope_unavailable");
+    expect(fetched.body.change).toBeUndefined();
+
+    // The ACTOR-scoped reads fail closed too: without an org they cannot be tenant-bounded, so two tenants could
+    // otherwise alias on the empty-org sentinel (colliding email is only per-org unique).
+    const noOrg = createMcpHttpServer(
+      API_BASE_URL,
+      createApi(
+        jest.fn(async () => ({
+          username: "alice@appsmith.com",
+          isAnonymous: false,
+        })),
+      ),
+      { governance: new McpGovernanceCoordinator(store) },
+    );
+
+    const ownList = await callTool(noOrg, "list_changes", {});
+
+    expect(ownList.body.code).toBe("organization_scope_unavailable");
+    expect(ownList.body.changes).toBeUndefined();
+
+    const ownGet = await callTool(noOrg, "get_change", { changeId: "c1" });
+
+    expect(ownGet.body.code).toBe("organization_scope_unavailable");
+  });
+
   it("get_capabilities advertises EXACTLY the registered tools (no drift), under all gates", async () => {
     for (const gate of [
       { dataEnabled: false, governed: false, js: false },
@@ -5145,23 +5253,43 @@ describe("M5 — build_application URLs + auto-publish", () => {
     async getChange(
       id: string,
       actorId: string,
+      organizationId: string,
     ): Promise<McpChangeRecord | undefined> {
-      return this.changes.find((c) => c.id === id && c.actorId === actorId);
+      return this.changes.find(
+        (c) =>
+          c.id === id &&
+          c.actorId === actorId &&
+          c.organizationId === organizationId,
+      );
     }
     async listChanges(
       actorId: string,
+      organizationId: string,
       limit: number,
     ): Promise<McpChangeRecord[]> {
       return this.changes
-        .filter((c) => c.actorId === actorId)
+        .filter(
+          (c) => c.actorId === actorId && c.organizationId === organizationId,
+        )
         .slice(-limit)
         .reverse();
     }
-    async getAnyChange(id: string): Promise<McpChangeRecord | undefined> {
-      return this.changes.find((c) => c.id === id);
+    async getAnyChange(
+      id: string,
+      organizationId: string,
+    ): Promise<McpChangeRecord | undefined> {
+      return this.changes.find(
+        (c) => c.id === id && c.organizationId === organizationId,
+      );
     }
-    async listAllChanges(limit: number): Promise<McpChangeRecord[]> {
-      return this.changes.slice(-limit).reverse();
+    async listAllChanges(
+      organizationId: string,
+      limit: number,
+    ): Promise<McpChangeRecord[]> {
+      return this.changes
+        .filter((c) => c.organizationId === organizationId)
+        .slice(-limit)
+        .reverse();
     }
   }