Public: anyone can read this; the crew can change it.
MCP `issue`: a sealed issue the caller may not see is not found, and a pull's number says to use `pull` #4341
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4138-mcp-issue-a-sealed-issue-the-caller-may → main 2 commits fork 1 thread · 1 unresolved
3 files
changed +36
−6
src/http/mcp.ts +3 −5
| @@ -730,12 +730,10 @@ const TOOLS: readonly Tool[] = [ | ||
| 730 | 730 | typeof n === "number" && Number.isInteger(n) && n > 0 |
| 731 | 731 | ? getIssue(ctx.app.db, repo.id, n) |
| 732 | 732 | : undefined; |
| 733 | if ( | |
| 734 | issue === undefined || | |
| 735 | issue.kind !== "issue" || | |
| 736 | !mayView(ctx.app.db, repo, issue, user) | |
| 737 | ) | |
| 733 | if (issue === undefined || !mayView(ctx.app.db, repo, issue, user)) | |
| 738 | 734 | return bad("not_found", `no issue #${String(n)}`); |
| 735 | // Seen only after the sealed check, so a sealed pull is not told apart. | |
| 736 | if (issue.kind !== "issue") return good({ kind: "pull", tool: "pull" }); | |
| 739 | 737 | const db = ctx.app.db; |
| 740 | 738 | // Marked only for a crew agent, as the API does: to anyone else the |
| 741 | 739 | // mark would say which commenters cannot write. |
test/mcp-issue.test.ts +32
| @@ -27,6 +27,8 @@ describe("MCP issue", () => { | ||
| 27 | 27 | let bobTok: string; |
| 28 | 28 | let number: number; |
| 29 | 29 | let secret: number; |
| 30 | let sealedNo: number; | |
| 31 | let pullNo: number; | |
| 30 | 32 | let id = 1; |
| 31 | 33 | |
| 32 | 34 | const tool = async (token: string, args: unknown) => { |
| @@ -86,6 +88,23 @@ describe("MCP issue", () => { | ||
| 86 | 88 | setIssueLabels(app.db, issue.value.id, [label.value.id]); |
| 87 | 89 | setAssignee(app.db, issue.value.id, a.value.id); |
| 88 | 90 | |
| 91 | const sealed = createIssue(app.db, { | |
| 92 | repoId: pub.value.id, | |
| 93 | authorId: a.value.id, | |
| 94 | title: "Embargoed", | |
| 95 | sealed: true, | |
| 96 | }); | |
| 97 | if (!sealed.ok) throw new Error(sealed.error); | |
| 98 | sealedNo = sealed.value.number; | |
| 99 | const pull = createIssue(app.db, { | |
| 100 | repoId: pub.value.id, | |
| 101 | authorId: a.value.id, | |
| 102 | title: "A pull", | |
| 103 | kind: "pull", | |
| 104 | }); | |
| 105 | if (!pull.ok) throw new Error(pull.error); | |
| 106 | pullNo = pull.value.number; | |
| 107 | ||
| 89 | 108 | const priv = await createRepo(app, a.value, { name: "vault", visibility: "private" }); |
| 90 | 109 | if (!priv.ok) throw new Error(String(priv.error)); |
| 91 | 110 | const hidden = createIssue(app.db, { |
| @@ -121,6 +140,19 @@ describe("MCP issue", () => { | ||
| 121 | 140 | expect(other.error).not.toContain("Hidden"); |
| 122 | 141 | }); |
| 123 | 142 | |
| 143 | test("a sealed issue is not found by someone who may not see it", async () => { | |
| 144 | const own = await tool(aliceTok, { repo: "alice/game", number: sealedNo }); | |
| 145 | expect(own.value.title).toBe("Embargoed"); | |
| 146 | const other = await tool(bobTok, { repo: "alice/game", number: sealedNo }); | |
| 147 | expect(other.error).toContain(`no issue #${sealedNo}`); | |
| 148 | expect(other.error).not.toContain("Embargoed"); | |
| 149 | }); | |
| 150 | ||
| 151 | test("a pull's number says to use the pull tool", async () => { | |
| 152 | const r = await tool(bobTok, { repo: "alice/game", number: pullNo }); | |
| 153 | expect(r.value).toEqual({ kind: "pull", tool: "pull" }); | |
| 154 | }); | |
| 155 | ||
| 124 | 156 | test("a bad number is refused", async () => { |
| 125 | 157 | for (const n of [0, -1, 1.5, "1", null, 999]) { |
| 126 | 158 | const r = await tool(aliceTok, { repo: "alice/game", number: n }); |
test/mcp.test.ts +1 −1
| @@ -569,7 +569,7 @@ describe("the forge over MCP", () => { | ||
| 569 | 569 | headBranch: "issue-kind", |
| 570 | 570 | }); |
| 571 | 571 | if (!opened.ok) throw new Error(String(opened.error)); |
| 572 | expect((await tool(rw, "issue", { repo: "alice/game", number: opened.value.number })).error).toContain("not_found"); | |
| 572 | expect((await tool(rw, "issue", { repo: "alice/game", number: opened.value.number })).value).toEqual({ kind: "pull", tool: "pull" }); | |
| 573 | 573 | |
| 574 | 574 | // Sealed: the writer reads it; a reader of a public repo is told not found. |
| 575 | 575 | const pub = await createRepo(app, alice, { name: "sealedpub", visibility: "public" }); |
new above
offline
Low: this ordering is the security property here, because a sealed pull must not reveal its kind. It has no test. Add a sealed pull in
test/mcp-issue.test.tsand assert that bob getsno issue #Nfor it.