History

mcp: drop unused listSources and add cross-user isolation test

listSources on McpBindingStore had no callers; the user-facing list goes through ctx.core.sources.list which is properly scope-filtered. Adds a cross-user test to lock in that users in the same org can't see each other's user-scoped sources while still seeing org-scoped ones.

Rhys Sullivan committed 6 months ago

Commit 8203f3b1 parent e9fad474 files · +180 −12Browse files

Unified · Split

4 files
  1. bun.lock +1 −0
  2. packages/plugins/mcp/package.json +1 −0
  3. packages/plugins/mcp/src/sdk/binding-store.ts +0 −12
  4. packages/plugins/mcp/src/sdk/cross-user-isolation.test.ts +178 −0
bun.lock+1−0
@@ -624,6 +624,7 @@ · 623 unmodified lines
624 "@effect/vitest": "catalog:",624 "@effect/vitest": "catalog:",
625 "@executor/api": "workspace:*",625 "@executor/api": "workspace:*",
626 "@executor/react": "workspace:*",626 "@executor/react": "workspace:*",
627 "@executor/storage-core": "workspace:*",
627 "@types/node": "catalog:",628 "@types/node": "catalog:",
628 "@types/react": "catalog:",629 "@types/react": "catalog:",
629 "bun-types": "catalog:",630 "bun-types": "catalog:",
packages/plugins/mcp/package.json+1−0
@@ -60,6 +60,7 @@ · 59 unmodified lines
60 "@effect/vitest": "catalog:",60 "@effect/vitest": "catalog:",
61 "@executor/api": "workspace:*",61 "@executor/api": "workspace:*",
62 "@executor/react": "workspace:*",62 "@executor/react": "workspace:*",
63 "@executor/storage-core": "workspace:*",
63 "@types/node": "catalog:",64 "@types/node": "catalog:",
64 "@types/react": "catalog:",65 "@types/react": "catalog:",
65 "bun-types": "catalog:",66 "bun-types": "catalog:",
packages/plugins/mcp/src/sdk/binding-store.ts+0−12
@@ -135,7 +135,6 @@ export interface McpBindingStore { · 134 unmodified lines
135 scope: string,135 scope: string,
136 ) => Effect.Effect<void, StorageFailure>;136 ) => Effect.Effect<void, StorageFailure>;
137 137
138 readonly listSources: () => Effect.Effect<readonly McpStoredSource[], StorageFailure>;
139 readonly getSource: (138 readonly getSource: (
140 namespace: string,139 namespace: string,
141 scope: string,140 scope: string,
@@ -211,17 +210,6 @@ export const makeMcpStore = ({ · 69 unmodified lines
211 })210 })
212 .pipe(Effect.asVoid),211 .pipe(Effect.asVoid),
213 212
214 listSources: () =>
215 Effect.gen(function* () {
216 const rows = yield* db.findMany({ model: "mcp_source" });
217 return rows.map((row) => ({
218 namespace: row.id,
219 scope: row.scope_id,
220 name: row.name,
221 config: decodeSourceData(coerceJson(row.config)),
222 }));
223 }),
224
225 getSource: (namespace, scope) =>213 getSource: (namespace, scope) =>
226 Effect.gen(function* () {214 Effect.gen(function* () {
227 const row = yield* db.findOne({215 const row = yield* db.findOne({
packages/plugins/mcp/src/sdk/cross-user-isolation.test.ts+178−0
@@ -0,0 +1,178 @@
1// ---------------------------------------------------------------------------
2// MCP cross-user (same-org) source isolation.
3//
4// Mirrors the prod executor scope stack:
5// inner = `user-org:${userId}:${orgId}`
6// outer = `${orgId}`
7//
8// Two users in the same org share the outer scope but have distinct inner
9// scopes. A source added at user A's inner scope must NOT appear when
10// user B calls `executor.sources.list()`. A source added at the shared
11// org scope MUST appear for both users (that's the whole point of the
12// outer scope — asserting it rules out a false positive where scope
13// filtering is simply broken).
14// ---------------------------------------------------------------------------
15
16import { describe, expect, it } from "@effect/vitest";
17import { Effect } from "effect";
18
19import {
20 Scope,
21 ScopeId,
22 collectSchemas,
23 createExecutor,
24 makeInMemoryBlobStore,
25} from "@executor/sdk";
26import { makeMemoryAdapter } from "@executor/storage-core/testing/memory";
27
28import { mcpPlugin } from "./plugin";
29
30// Shared memory adapter + blob store across the two executors — this is
31// what makes the leak possible in the first place. Production shares a
32// single Postgres across every request for an org.
33const makeSharedOrgExecutors = () =>
34 Effect.gen(function* () {
35 const plugins = [mcpPlugin()] as const;
36 const schema = collectSchemas(plugins);
37 const adapter = makeMemoryAdapter({ schema });
38 const blobs = makeInMemoryBlobStore();
39
40 const ORG_ID = "org-acme";
41 const USER_A = "user-alice";
42 const USER_B = "user-bob";
43
44 const orgScopeId = ScopeId.make(ORG_ID);
45 const aInnerId = ScopeId.make(`user-org:${USER_A}:${ORG_ID}`);
46 const bInnerId = ScopeId.make(`user-org:${USER_B}:${ORG_ID}`);
47
48 const orgScope = new Scope({
49 id: orgScopeId,
50 name: "Acme",
51 createdAt: new Date(),
52 });
53
54 const makeFor = (innerId: ScopeId) =>
55 createExecutor({
56 scopes: [
57 new Scope({ id: innerId, name: "Personal", createdAt: new Date() }),
58 orgScope,
59 ],
60 adapter,
61 blobs,
62 plugins,
63 });
64
65 const execA = yield* makeFor(aInnerId);
66 const execB = yield* makeFor(bInnerId);
67
68 return {
69 execA,
70 execB,
71 aInnerId: aInnerId as string,
72 bInnerId: bInnerId as string,
73 orgScopeId: orgScopeId as string,
74 };
75 });
76
77// Port 1 is reserved — addSource's discovery fails, but the source row
78// still persists. Same trick the existing multi-scope tests use so we
79// don't need an actual MCP server.
80const seedSource = (
81 addSource: (c: {
82 readonly transport: "remote";
83 readonly scope: string;
84 readonly name: string;
85 readonly endpoint: string;
86 readonly remoteTransport: "auto";
87 readonly namespace: string;
88 }) => Effect.Effect<unknown, unknown>,
89 args: {
90 readonly scope: string;
91 readonly name: string;
92 readonly namespace: string;
93 },
94) =>
95 addSource({
96 transport: "remote",
97 scope: args.scope,
98 name: args.name,
99 endpoint: `http://127.0.0.1:1/${args.namespace}`,
100 remoteTransport: "auto",
101 namespace: args.namespace,
102 }).pipe(Effect.either);
103
104describe("MCP cross-user isolation within the same org", () => {
105 it.effect("user B does not see user A's inner-scope MCP source", () =>
106 Effect.gen(function* () {
107 const { execA, execB, aInnerId } = yield* makeSharedOrgExecutors();
108
109 // User A adds a personal source at their inner scope.
110 yield* seedSource(execA.mcp.addSource, {
111 scope: aInnerId,
112 name: "Alice Personal",
113 namespace: "alice_personal",
114 });
115
116 // User B lists sources — must NOT see alice_personal.
117 const bSources = yield* execB.sources.list();
118 const bIds = bSources.map((s) => s.id);
119
120 expect(bIds).not.toContain("alice_personal");
121 }),
122 );
123
124 it.effect("user B DOES see org-scope sources (sanity: filter not over-broad)", () =>
125 Effect.gen(function* () {
126 const { execA, execB, orgScopeId } = yield* makeSharedOrgExecutors();
127
128 // An org-scope source — shared across everyone in the org.
129 yield* seedSource(execA.mcp.addSource, {
130 scope: orgScopeId,
131 name: "Org Shared",
132 namespace: "org_shared",
133 });
134
135 const bSources = yield* execB.sources.list();
136 const bIds = bSources.map((s) => s.id);
137
138 expect(bIds).toContain("org_shared");
139 }),
140 );
141
142 it.effect("A sees A's source, B sees org's, neither sees the other's inner", () =>
143 Effect.gen(function* () {
144 const { execA, execB, aInnerId, bInnerId, orgScopeId } =
145 yield* makeSharedOrgExecutors();
146
147 yield* seedSource(execA.mcp.addSource, {
148 scope: aInnerId,
149 name: "Alice Personal",
150 namespace: "alice_personal",
151 });
152 yield* seedSource(execB.mcp.addSource, {
153 scope: bInnerId,
154 name: "Bob Personal",
155 namespace: "bob_personal",
156 });
157 yield* seedSource(execA.mcp.addSource, {
158 scope: orgScopeId,
159 name: "Org Shared",
160 namespace: "org_shared",
161 });
162
163 const aSources = yield* execA.sources.list();
164 const bSources = yield* execB.sources.list();
165
166 const aIds = aSources.map((s) => s.id);
167 const bIds = bSources.map((s) => s.id);
168
169 expect(aIds).toContain("alice_personal");
170 expect(aIds).toContain("org_shared");
171 expect(aIds).not.toContain("bob_personal");
172
173 expect(bIds).toContain("bob_personal");
174 expect(bIds).toContain("org_shared");
175 expect(bIds).not.toContain("alice_personal");
176 }),
177 );
178});