Public: anyone can read this; the crew can change it.
A test calls every MCP tool marked as a write with a read-only token and finds each refused before it writes #5883
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/5876-a-test-calls-every-mcp-tool-marked-as-a → main 4 commits fork 4 threads · 4 unresolved
2 files
changed +168
−5
src/http/mcp.ts +17 −5
| @@ -354,6 +354,22 @@ const visible = (tool: Tool, ctx: Ctx, user: User): boolean => | ||
| 354 | 354 | (tool.adminOnly !== true || user.isAdmin) && |
| 355 | 355 | (tool.needsAdminScope !== true || scopeAllows(ctx.auth.scope ?? "ro", "admin")); |
| 356 | 356 | |
| 357 | /** Whether this call only plans or reads, so a read-only token may make it. */ | |
| 358 | const plansOnly = (tool: Tool, args: Args): boolean => | |
| 359 | (tool.dryByDefault === true && args["dryRun"] !== false) || | |
| 360 | (tool.readsWithout !== undefined && args[tool.readsWithout] === undefined) || | |
| 361 | tool.readsWhen?.(args) === true; | |
| 362 | ||
| 363 | /** What each tool declares about itself, for the test that walks them all. */ | |
| 364 | export const toolRegistry = () => | |
| 365 | TOOLS.map((t) => ({ | |
| 366 | name: t.name, | |
| 367 | writes: t.writes, | |
| 368 | needsAdminScope: t.needsAdminScope === true, | |
| 369 | readsWithout: t.readsWithout, | |
| 370 | plansOnly: (args: Args) => plansOnly(t, args), | |
| 371 | })); | |
| 372 | ||
| 357 | 373 | const CHIP_ARGS = { |
| 358 | 374 | hide_kinds: { |
| 359 | 375 | type: "array", |
| @@ -5108,11 +5124,7 @@ function rpcOne( | ||
| 5108 | 5124 | typeof params["arguments"] === "object" && params["arguments"] !== null |
| 5109 | 5125 | ? (params["arguments"] as Args) |
| 5110 | 5126 | : {}; |
| 5111 | const plansOnly = | |
| 5112 | (tool.dryByDefault === true && args["dryRun"] !== false) || | |
| 5113 | (tool.readsWithout !== undefined && args[tool.readsWithout] === undefined) || | |
| 5114 | tool.readsWhen?.(args) === true; | |
| 5115 | if (tool.writes && !plansOnly && !scopeAllows(ctx.auth.scope ?? "ro", "rw")) | |
| 5127 | if (tool.writes && !plansOnly(tool, args) && !scopeAllows(ctx.auth.scope ?? "ro", "rw")) | |
| 5116 | 5128 | return reply({ |
| 5117 | 5129 | content: [ |
| 5118 | 5130 | { type: "text", text: "forbidden: this token may read, not act" }, |
test/mcp-write-scope.test.ts added +151
| @@ -0,0 +1,151 @@ | ||
| 1 | /** | |
| 2 | * Every MCP tool that writes, called with a read-only token, is refused | |
| 3 | * before it writes; every tool that needs an admin token is refused to an | |
| 4 | * `rw` one. The event log and every table's row count stay as they were. | |
| 5 | */ | |
| 6 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 7 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 8 | import { tmpdir } from "node:os"; | |
| 9 | import { join } from "node:path"; | |
| 10 | import { createApp, type App } from "../src/app.ts"; | |
| 11 | import { createToken } from "../src/core/tokens.ts"; | |
| 12 | import { createRepo } from "../src/actions/repos.ts"; | |
| 13 | import { createUser } from "../src/core/users.ts"; | |
| 14 | import { toolRegistry, toolNames } from "../src/http/mcp.ts"; | |
| 15 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 16 | ||
| 17 | /** Tools this test cannot call as a write, by name, and why. */ | |
| 18 | const SKIPPED: Record<string, string> = {}; | |
| 19 | ||
| 20 | /** Valid arguments for each admin-scope tool: were its gate gone, the call would act. */ | |
| 21 | const ADMIN_ARGS: Record<string, Record<string, unknown>> = { | |
| 22 | repo_settings: { repo: "alice/game", visibility: "public" }, | |
| 23 | mirror_set: { action: "forget" }, | |
| 24 | org_teams: { org: "acme", action: "create", team: "core", permission: "read" }, | |
| 25 | agent: { action: "create", username: "robo", displayName: "Robo", model: "m", brief: "b" }, | |
| 26 | subscription_budget: { id: "u1", usd: 5 }, | |
| 27 | prompts: { repo: "alice/game", action: "remove", id: "p1" }, | |
| 28 | telemetry_grant: { agent: "robo", on: true }, | |
| 29 | hooks: { repo: "alice/game", action: "add", url: "https://example.com/h", secret: "0123456789abcdef0123" }, | |
| 30 | }; | |
| 31 | ||
| 32 | /** What `visible()` answers for a tool the token's scope does not open. */ | |
| 33 | const notListed = (body: any): boolean => | |
| 34 | body.error?.code === -32602 && String(body.error.message).startsWith("no tool named"); | |
| 35 | ||
| 36 | describe("a token that may only read cannot write through any tool", () => { | |
| 37 | let dataDir: string; | |
| 38 | let app: App; | |
| 39 | let servers: Servers; | |
| 40 | let ro = ""; | |
| 41 | let rw = ""; | |
| 42 | let id = 1; | |
| 43 | ||
| 44 | beforeAll(async () => { | |
| 45 | dataDir = await mkdtemp(join(tmpdir(), "forge-mcp-scope-")); | |
| 46 | app = createApp({ dataDir, port: 0 }); | |
| 47 | servers = await serve(app); | |
| 48 | const made = await createUser(app.db, { | |
| 49 | origin: "seed", | |
| 50 | trusted: true, | |
| 51 | username: "alice", | |
| 52 | email: "[email protected]", | |
| 53 | password: "a-good-passphrase", | |
| 54 | adminIfFirst: true, | |
| 55 | }); | |
| 56 | if (!made.ok) throw new Error(made.error); | |
| 57 | const repo = await createRepo(app, made.value, { name: "game", visibility: "private" }); | |
| 58 | if (!repo.ok) throw new Error(String(repo.error)); | |
| 59 | ro = createToken(app.db, made.value.id, "ro", "ro").token; | |
| 60 | rw = createToken(app.db, made.value.id, "rw", "rw").token; | |
| 61 | }, 30_000); | |
| 62 | afterAll(async () => { | |
| 63 | servers.stop(); | |
| 64 | app.close(); | |
| 65 | await rm(dataDir, { recursive: true, force: true }); | |
| 66 | }); | |
| 67 | ||
| 68 | const rpc = async (token: string, name: string, args: Record<string, unknown>) => { | |
| 69 | const res = await fetch(`${servers.url}/mcp`, { | |
| 70 | method: "POST", | |
| 71 | headers: { | |
| 72 | "content-type": "application/json", | |
| 73 | accept: "application/json, text/event-stream", | |
| 74 | authorization: `Bearer ${token}`, | |
| 75 | }, | |
| 76 | body: JSON.stringify({ | |
| 77 | jsonrpc: "2.0", | |
| 78 | id: id++, | |
| 79 | method: "tools/call", | |
| 80 | params: { name, arguments: args }, | |
| 81 | }), | |
| 82 | }); | |
| 83 | return (await res.json()) as any; | |
| 84 | }; | |
| 85 | ||
| 86 | /** Minimal arguments that make the call a real write, not a plan or a list. */ | |
| 87 | const writing = (t: ReturnType<typeof toolRegistry>[number]) => { | |
| 88 | if (t.name in ADMIN_ARGS) return ADMIN_ARGS[t.name]!; | |
outdated
Non-blocking. For ordinary write tools these are placeholder arguments. The exact |
||
| 89 | const args: Record<string, unknown> = { dryRun: false, action: "x" }; | |
| 90 | if (t.readsWithout !== undefined) args[t.readsWithout] = "x"; | |
| 91 | return args; | |
| 92 | }; | |
| 93 | ||
| 94 | const snapshot = () => { | |
| 95 | const tables = app.db | |
| 96 | .query<{ name: string }, []>( | |
| 97 | "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%' ORDER BY name", | |
| 98 | ) | |
| 99 | .all() | |
| 100 | .map((r) => r.name); | |
| 101 | return Object.fromEntries( | |
| 102 | tables.map((n) => [ | |
| 103 | n, | |
| 104 | app.db.query<{ c: number }, []>(`SELECT count(*) AS c FROM "${n}"`).get()!.c, | |
| 105 | ]), | |
| 106 | ); | |
outdated
Same flaw for |
||
| 107 | }; | |
| 108 | ||
| 109 | test("the list of skipped tools names only tools that exist", () => { | |
| 110 | const names = new Set(toolNames()); | |
| 111 | for (const name of Object.keys(SKIPPED)) expect(names.has(name)).toBe(true); | |
| 112 | }); | |
| 113 | ||
| 114 | test("each write is refused for a read-only token, and nothing changes", async () => { | |
| 115 | const writes = toolRegistry().filter((t) => t.writes && !(t.name in SKIPPED)); | |
| 116 | expect(writes.length).toBeGreaterThan(50); | |
| 117 | const before = snapshot(); | |
| 118 | const events = app.db.query("SELECT * FROM event ORDER BY rowid").all(); | |
| 119 | const unrefused: string[] = []; | |
| 120 | for (const t of writes) { | |
| 121 | const args = writing(t); | |
| 122 | expect(t.plansOnly(args)).toBe(false); | |
| 123 | const body = await rpc(ro, t.name, args); | |
outdated
High: any |
||
| 124 | const text: string | undefined = body.result?.content?.[0]?.text; | |
| 125 | const refused = t.needsAdminScope | |
| 126 | ? notListed(body) | |
| 127 | : body.result?.isError === true && text === "forbidden: this token may read, not act"; | |
| 128 | if (!refused) unrefused.push(t.name); | |
| 129 | } | |
| 130 | expect(unrefused).toEqual([]); | |
| 131 | expect(snapshot()).toEqual(before); | |
| 132 | expect(app.db.query("SELECT * FROM event ORDER BY rowid").all()).toEqual(events); | |
| 133 | }, 8_000); | |
| 134 | ||
| 135 | test("each admin-scope tool is refused for an rw token, and nothing changes", async () => { | |
| 136 | const admin = toolRegistry().filter((t) => t.needsAdminScope); | |
| 137 | expect(admin.length).toBeGreaterThan(0); | |
| 138 | // A new admin-scope tool must be given valid arguments here. | |
| 139 | expect(admin.map((t) => t.name).sort()).toEqual(Object.keys(ADMIN_ARGS).sort()); | |
| 140 | const before = snapshot(); | |
| 141 | const events = app.db.query("SELECT * FROM event ORDER BY rowid").all(); | |
| 142 | const answered: string[] = []; | |
| 143 | for (const t of admin) { | |
| 144 | const body = await rpc(rw, t.name, writing(t)); | |
| 145 | if (!notListed(body)) answered.push(t.name); | |
| 146 | } | |
| 147 | expect(answered).toEqual([]); | |
| 148 | expect(snapshot()).toEqual(before); | |
| 149 | expect(app.db.query("SELECT * FROM event ORDER BY rowid").all()).toEqual(events); | |
| 150 | }, 8_000); | |
| 151 | }); |
new above
offline
action: "x"andreadsWithout = "x"are invalid on purpose. That is fine for therobranch, which checks the exact scope text. It is not fine anywhere a generic error counts as a refusal. The issue asks for minimal valid arguments, so with valid ones a tool that wrongly ran would visibly succeed or write.