trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns15g rEventsFlows

The MCP person and pins tools refuse a limit outside 1–200 instead of clamping it, with the words the HTTP list routes use #8514

Merged masoncrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/8502-the-mcp-person-and-pins-tools-refuse-a-l → main 2 commits fork 1 thread · 1 unresolved

  • Merged. mason/8502-the-mcp-person-and-pins-tools-refuse-a-l was merged into main as 173a228.

Review

Sign in to review this change.
5 files changed +31 −11
src/http/mcp/shared.ts +8 −1
@@ -1,5 +1,5 @@
11 import { mayView } from "../../core/sealed.ts";
2 import { getIssue } from "../../core/issues.ts";
2 import { getIssue, LIST_LIMIT_MAX } from "../../core/issues.ts";
33 import { canRead, getRepo, type Repo } from "../../core/repos.ts";
44 import { scopeAllows } from "../../core/tokens.ts";
55 import { getUserByName, type User } from "../../core/users.ts";
@@ -44,6 +44,13 @@ export const bad = (error: string, message: string): Answer => ({
4444 message,
4545 });
4646
47 /** The refusal for a `limit` that is not a whole number from 1 to LIST_LIMIT_MAX, as the HTTP list routes say it; undefined when it is absent or fine. */
48 export const badLimit = (limit: unknown): Answer | undefined =>
49 limit === undefined ||
50 (typeof limit === "number" && Number.isInteger(limit) && limit >= 1 && limit <= LIST_LIMIT_MAX)
51 ? undefined
52 : bad("invalid_limit", `limit is a whole number from 1 to ${LIST_LIMIT_MAX}`);
53
4754 export interface Tool {
4855 readonly name: string;
4956 /** Its line in /llms.txt's `## MCP tools`: markdown, one line; absent, the line reads `description`. */
src/http/mcp/tools/person.ts +5 −5
@@ -2,17 +2,17 @@ import { pageLimit } from "../../../core/issues.ts";
22 import { listReposByOwner } from "../../../core/repos.ts";
33 import { getUserByName, hiddenFrom } from "../../../core/users.ts";
44 import { paged, publicProfile } from "../../respond.ts";
5 import { good, bad, type Tool } from "../shared.ts";
5 import { good, bad, badLimit, type Tool } from "../shared.ts";
66
77 export const tool: Tool = {
88 name: "person",
9 doc: "`{ username, limit?, cursor? }`: a person's public profile and the repositories you may see, as `GET /api/v1/users/:username` and its `/repos`; a private one you cannot read is never listed",
9 doc: "`{ username, limit? (1 to 200), cursor? }`: a person's public profile and the repositories you may see, as `GET /api/v1/users/:username` and its `/repos`; a private one you cannot read is never listed",
1010 description: "A person's profile and repos you may see.",
1111 inputSchema: {
1212 type: "object",
1313 properties: {
1414 username: { type: "string" },
15 limit: { type: "integer" },
15 limit: { type: "integer", minimum: 1, maximum: 200 },
1616 cursor: { type: "string" },
1717 },
1818 required: ["username"],
@@ -22,8 +22,8 @@ export const tool: Tool = {
2222 const { username, limit, cursor } = args;
2323 if (typeof username !== "string" || username === "" || username.length > 100)
2424 return bad("invalid", "`username` is 1 to 100 characters");
25 if (limit !== undefined && (typeof limit !== "number" || !Number.isFinite(limit)))
26 return bad("invalid", "`limit` is a number");
25 const limitRefused = badLimit(limit);
sentinelcrew

Medium: badLimit answers code invalid with backticks around limit. The HTTP list routes and the MCP issues and page helpers answer invalid_limit with limit is a whole number from 1 to 200. Change badLimit in shared.ts to that, so a client sees one refusal across doors. Also add minimum: 1, maximum: 200 to the limit schema (line 15), as issues.ts does.
26 if (limitRefused) return limitRefused;
2727 if (cursor !== undefined && typeof cursor !== "string")
2828 return bad("invalid", "`cursor` is the one a previous answer gave");
2929 const person = getUserByName(ctx.app.db, username);
src/http/mcp/tools/pins.ts +5 −5
@@ -1,18 +1,18 @@
11 import { pageLimit } from "../../../core/issues.ts";
22 import { listPinned, pinRepo, unpinRepo } from "../../../actions/origins.ts";
33 import { canRead, getReposByIds } from "../../../core/repos.ts";
4 import { good, bad, type Tool, REPO_ARG, repoArg, isAnswer } from "../shared.ts";
4 import { good, bad, badLimit, type Tool, REPO_ARG, repoArg, isAnswer } from "../shared.ts";
55
66 export const tool: Tool = {
77 name: "pins",
8 doc: "`{ action?, repo?, limit?, cursor? }`: `list` (the default) the caller's pinned repositories, paged by `owner/name` as `GET /api/v1/user/pins` is (`next_cursor`), leaving out a pin whose repository the caller can no longer read (the API's list still names it); `pin` or `unpin` `repo`, as `PUT`/`DELETE /api/v1/user/pins/:owner/:name` do. A repository the caller cannot see is not found",
8 doc: "`{ action?, repo?, limit? (1 to 200), cursor? }`: `list` (the default) the caller's pinned repositories, paged by `owner/name` as `GET /api/v1/user/pins` is (`next_cursor`), leaving out a pin whose repository the caller can no longer read (the API's list still names it); `pin` or `unpin` `repo`, as `PUT`/`DELETE /api/v1/user/pins/:owner/:name` do. A repository the caller cannot see is not found",
99 description: "Your pinned repos: list, pin or unpin `repo`.",
1010 inputSchema: {
1111 type: "object",
1212 properties: {
1313 action: { type: "string", enum: ["list", "pin", "unpin"] },
1414 repo: REPO_ARG,
15 limit: { type: "integer" },
15 limit: { type: "integer", minimum: 1, maximum: 200 },
1616 cursor: { type: "string" },
1717 },
1818 },
@@ -24,8 +24,8 @@ export const tool: Tool = {
2424 return bad("invalid", "action is list, pin or unpin");
2525 if (action === "list") {
2626 const { limit, cursor } = args;
27 if (limit !== undefined && (typeof limit !== "number" || !Number.isFinite(limit)))
28 return bad("invalid", "`limit` is a number");
27 const limitRefused = badLimit(limit);
28 if (limitRefused) return limitRefused;
2929 if (cursor !== undefined && typeof cursor !== "string")
3030 return bad("invalid", "`cursor` is the one a previous answer gave");
3131 const size = pageLimit(limit as number | undefined);
test/mcp-person.test.ts +9
@@ -103,6 +103,15 @@ describe("MCP person", () => {
103103 expect(next.value.repos.items[0].name).not.toBe(first.value.repos.items[0].name);
104104 });
105105
106 test("a limit outside 1 to 200 is refused, 1 and 200 pass", async () => {
107 for (const bad of [0, -5, 9999, 201, 1.5, "x"])
108 expect((await tool(bobRo, { username: "guild", limit: bad })).error).toContain(
109 "invalid_limit",
110 );
111 expect((await tool(bobRo, { username: "guild", limit: 1 })).value.repos.items).toHaveLength(1);
112 expect((await tool(bobRo, { username: "guild", limit: 200 })).value).toBeDefined();
113 });
114
106115 test("a suspended person is not found to a non-admin, and still found by an admin", async () => {
107116 expect((await tool(carolRo, { username: "dave" })).error).toContain("no such person");
108117 expect((await tool(aliceRo, { username: "dave" })).value.username).toBe("dave");
test/mcp-pins.test.ts +4
@@ -121,6 +121,10 @@ describe("MCP pins", () => {
121121 expect(second.pins.map((p: any) => p.repo)).toContain("alice/pc");
122122 expect(second.next_cursor).toBeNull();
123123 expect((await tool(bobRw, { limit: "x" })).error).toContain("invalid");
124 for (const bad of [0, -5, 9999, 201, 1.5, "x"])
125 expect((await tool(bobRw, { limit: bad })).error).toContain("invalid_limit");
126 expect((await tool(bobRw, { limit: 1 })).value.pins).toHaveLength(1);
127 expect((await tool(bobRw, { limit: 200 })).value.pins.length).toBeGreaterThan(0);
124128 for (const n of ["pa", "pb", "pc"]) await tool(bobRw, { action: "unpin", repo: `alice/${n}` });
125129 });
126130

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs