Check collaborator permissions, check run links, run paging and MCP batch size; reserve more logins (#102)
Claude-Session: https://claude.ai/code/session_016yhgAu6oKfqes812Yd4qjo Co-authored-by: Claude <noreply@anthropic.com>
Canberk Demirci authored · GitHub committed signed1 parent c5bc7eecommit 14d20cffa288a2b7c087e396b59de44dcbd8b7eaBrowse files
7 files changed+36−6
Files (7)
modifiedapps/diffinery/docs/accounts/invitations.md+1−1View file
| @@ -214,7 +214,7 @@ | ||
| 214 | 214 | The **Join Diffinery** page suggests a username made from the part of the email address before the `@`. Rules: |
| 215 | 215 | |
| 216 | 216 | - Starts with a letter or digit, then letters, digits, `-` and `_`, up to 39 characters. |
| 217 | -- Not already taken (case-insensitive) and not reserved. Reserved names include `api`, `mcp`, `settings`, `new`, `notifications`, `invite`, `login`, `admin`, `explore`, `docs`, `auth`, `signin`, `signup`, `security` and `diffinery`. | |
| 217 | +- Not already taken (case-insensitive) and not reserved. Reserved names include `api`, `mcp`, `settings`, `new`, `notifications`, `invite`, `login`, `admin`, `explore`, `docs`, `auth`, `signin`, `signup`, `security`, `diffinery`, `ghost`, `github-actions`, `noreply`, `support`, `abuse`, `postmaster`, `root` and `staff`. | |
| 218 | 218 | |
| 219 | 219 | Your username is part of your repository URLs (`https://diffinery.com/<username>/<repo>`). You can change it later under **Settings → Profile**, which changes those URLs; existing clones then need a new remote URL (see [Profile and preferences](/docs/accounts/profile-and-preferences#change-your-username)). |
| 220 | 220 |
modifiedapps/diffinery/docs/accounts/permissions.md+1−1View file
| @@ -115,7 +115,7 @@ | ||
| 115 | 115 | -H "Authorization: Bearer dfy_..." |
| 116 | 116 | ``` |
| 117 | 117 | |
| 118 | -Without a body, `PUT` gives `write`. Both need admin on the repository and a token with the `admin` scope. | |
| 118 | +Without a body, `PUT` gives `write`. Both need admin on the repository and a token with the `admin` scope. Any other `permission` than `pull`, `read`, `triage`, `push`, `write`, `maintain` or `admin` answers `422 permission must be pull, triage, push, maintain or admin`, and an organization's login answers `422` too: add its members instead, or transfer the repository to it. | |
| 119 | 119 | |
| 120 | 120 | ## Organization roles |
| 121 | 121 |
modifiedapps/diffinery/docs/reference/limits.md+1−0View file
| @@ -62,6 +62,7 @@ | ||
| 62 | 62 | | Labels | Names up to 100 characters; 100 labels per issue | The REST API. The web UI's label form allows names up to 50 characters. | |
| 63 | 63 | | Assignees | 10 per issue or pull request | `an issue can have at most 10 assignees`. | |
| 64 | 64 | | Issue and pull request text | Titles up to 256 characters; bodies, comments and review text up to 65,536 characters | As on GitHub, everywhere they're written (web, REST, MCP). Longer text fails with `422 body is too long (maximum is 65536 characters)`. | |
| 65 | +| MCP batch | 20 requests | One `POST /mcp` with a JSON-RPC batch of more than 20 requests answers `400` with `at most 20 requests in one batch`. | | |
| 65 | 66 | | `repository_dispatch` | `event_type` up to 100 characters; `client_payload` up to 10 top-level properties and 64 KiB | `POST .../dispatches`. | |
| 66 | 67 | | Check run name | 255 characters | `POST .../check-runs`. | |
| 67 | 68 | | Deployment `required_contexts` | 100 contexts | `POST .../deployments`. | |
modifiedapps/diffinery/src/http/api.ts+7−3View file
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | import { encryptSecret } from "../lib/crypto.js"; |
| 7 | 7 | import { publicJwk } from "../lib/jwt.js"; |
| 8 | 8 | import { HttpError, b64, fromB64 } from "../lib/util.js"; |
| 9 | -import { atLeast, type ProtectionRules, type Role } from "../repo/types.js"; | |
| 9 | +import { atLeast, ROLE_LEVEL, type ProtectionRules, type Role } from "../repo/types.js"; | |
| 10 | 10 | import type { JobInfo, RunInfo } from "../repo/actions.js"; |
| 11 | 11 | import type { CommitInfo } from "../repo/RepoDO.js"; |
| 12 | 12 | import type { QueueEntry } from "../repo/queue.js"; |
| @@ -303,9 +303,11 @@ | ||
| 303 | 303 | const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); |
| 304 | 304 | const who = await c.get("dir").accountByLogin(c.req.param("login")); |
| 305 | 305 | if (!who) throw new HttpError(404, "user not found"); |
| 306 | + if (who.kind === "org") throw new HttpError(422, "an organization can't be a collaborator: add its members, or transfer the repository to it"); | |
| 306 | 307 | const b = await body<{ permission?: Role | "pull" | "push" }>(c).catch(() => ({ permission: "write" as Role })); |
| 307 | 308 | const map: Record<string, Role> = { pull: "read", push: "write" }; |
| 308 | 309 | const role = map[b.permission ?? ""] ?? (b.permission as Role) ?? "write"; |
| 310 | + if (!(role in ROLE_LEVEL)) throw new HttpError(422, "permission must be pull, triage, push, maintain or admin"); | |
| 309 | 311 | await c.get("dir").setCollaborator(repo.id, who.id, role); |
| 310 | 312 | auditRepo(c, caller, repo, "repo.collaborator.set", { login: who.login, role }); |
| 311 | 313 | return c.body(null, 204); |
| @@ -772,6 +774,8 @@ | ||
| 772 | 774 | if (/^diffinery-(actions|agent-review)/i.test(caller.login)) throw new HttpError(403, "this account can't report check runs"); |
| 773 | 775 | const sha = typeof b.head_sha === "string" ? await stub.resolve(b.head_sha) : null; |
| 774 | 776 | if (!sha) throw new HttpError(422, "head_sha is not a commit in this repository", { errors: [{ resource: "CheckRun", code: "invalid", field: "head_sha" }] }); |
| 777 | + // Shown as a link on the pull request, so only web addresses (as the web UI already insists). | |
| 778 | + if (b.details_url !== undefined && b.details_url !== null && (typeof b.details_url !== "string" || b.details_url.length > 2000 || !/^https?:\/\/[^\s]+$/i.test(b.details_url))) throw new HttpError(422, "details_url must be an http or https URL", { errors: [{ resource: "CheckRun", code: "invalid", field: "details_url" }] }); | |
| 775 | 779 | const status = b.conclusion ? "completed" : (b.status ?? "queued"); |
| 776 | 780 | const { annotations: _a, text: _t, ...output } = b.output ?? {}; |
| 777 | 781 | let check = await stub.upsertCheck({ sha, name: b.name, status, conclusion: b.conclusion, detailsUrl: b.details_url, output: b.output ? output : undefined, app: caller.login, creator: { id: caller.id, login: caller.login, kind: caller.kind } }); |
| @@ -845,8 +849,8 @@ | ||
| 845 | 849 | }); |
| 846 | 850 | api.get("/repos/:owner/:repo/actions/runs", async (c) => { |
| 847 | 851 | const { stub } = await loadRepo(c, P(c).owner, P(c).repo); |
| 848 | - const perPage = Math.min(Number(c.req.query("per_page") ?? 25), 100); | |
| 849 | - const page = Number(c.req.query("page") ?? 1); | |
| 852 | + const perPage = Math.min(Math.max(Math.trunc(Number(c.req.query("per_page") ?? 25)) || 25, 1), 100); | |
| 853 | + const page = Math.min(Math.max(Math.trunc(Number(c.req.query("page") ?? 1)) || 1, 1), 1000); | |
| 850 | 854 | const res = await stub.runs({ workflow: c.req.query("workflow"), branch: c.req.query("branch"), status: c.req.query("status"), event: c.req.query("event"), sha: c.req.query("head_sha"), limit: perPage, offset: (page - 1) * perPage }); |
| 851 | 855 | setLinks(c, { page, perPage, offset: (page - 1) * perPage }, res.total); |
| 852 | 856 | return c.json({ total_count: res.total, workflow_runs: res.runs.map((r) => runJson(c, P(c).owner, P(c).repo, r)) }); |
modifiedapps/diffinery/src/lib/util.ts+1−1View file
| @@ -79,7 +79,7 @@ | ||
| 79 | 79 | } |
| 80 | 80 | |
| 81 | 81 | /** Top-level paths the app serves; no account may use them as its login. */ |
| 82 | -export const RESERVED_LOGINS = new Set(["api", "mcp", "runner", "settings", "new", "notifications", "invite", "request-access", "healthz", "twirp", "login", "logout", "admin", "explore", "favicon.ico", "robots.txt", "about", "docs", "static", "assets", "www", "git", "diffinery", "auth", "signin", "signup", "security", "v2", "imports", "-", "diffinery-agent-review"]); | |
| 82 | +export const RESERVED_LOGINS = new Set(["api", "mcp", "runner", "settings", "new", "notifications", "invite", "request-access", "healthz", "twirp", "login", "logout", "admin", "explore", "favicon.ico", "robots.txt", "about", "docs", "static", "assets", "www", "git", "diffinery", "auth", "signin", "signup", "security", "v2", "imports", "-", "diffinery-agent-review", "ghost", "github-actions", "diffinery-actions", "noreply", "no-reply", "support", "abuse", "postmaster", "root", "staff"]); | |
| 83 | 83 | |
| 84 | 84 | export const isValidLogin = (s: string): boolean => isValidName(s) && !s.includes(".") && !RESERVED_LOGINS.has(s.toLowerCase()); |
| 85 | 85 |
modifiedapps/diffinery/src/mcp/server.ts+4−0View file
| @@ -617,6 +617,9 @@ | ||
| 617 | 617 | |
| 618 | 618 | mcp.get("/mcp", (c) => c.json({ message: "Diffinery MCP: POST JSON-RPC here with Authorization: Bearer <token>" }, 405, { allow: "POST" })); |
| 619 | 619 | mcp.delete("/mcp", (c) => c.body(null, 405)); |
| 620 | +/** JSON-RPC requests in one batch: each is a tool call, and one HTTP request mustn't fan out without bound. */ | |
| 621 | +export const MAX_BATCH = 20; | |
| 622 | + | |
| 620 | 623 | mcp.post("/mcp", async (c) => { |
| 621 | 624 | // Every MCP call needs an active account (a token for an agent or a person). |
| 622 | 625 | requireAccount(c); |
| @@ -628,6 +631,7 @@ | ||
| 628 | 631 | } |
| 629 | 632 | const batch = Array.isArray(payload); |
| 630 | 633 | const reqs = (batch ? payload : [payload]) as RpcRequest[]; |
| 634 | + if (reqs.length > MAX_BATCH) return c.json(rpcError(null, -32600, `at most ${MAX_BATCH} requests in one batch`), 400); | |
| 631 | 635 | const out: unknown[] = []; |
| 632 | 636 | for (const r of reqs) { |
| 633 | 637 | if (!r || typeof r !== "object" || typeof r.method !== "string") { |
modifiedapps/diffinery/test/e2e/security.test.mjs+21−0View file
| @@ -2070,3 +2070,24 @@ | ||
| 2070 | 2070 | for (let i = 0; i < 5; i++) await ok("POST", "/diffinery/emails/test", {}, T.rita); |
| 2071 | 2071 | assert.match((await status(429, "POST", "/diffinery/emails/test", {}, T.rita)).message, /at most 5 test emails an hour/); |
| 2072 | 2072 | }); |
| 2073 | + | |
| 2074 | +test("security: inputs that were taken on trust are checked (collaborators, check links, paging, MCP batches, reserved names)", async () => { | |
| 2075 | + const R = `/repos/${OWNER}/checked-inputs`; | |
| 2076 | + const sha = await repoWithReadme("checked-inputs"); | |
| 2077 | + // A collaborator's permission is one of GitHub's, and an organization can't be one. | |
| 2078 | + assert.match((await status(422, "PUT", `${R}/collaborators/rita`, { permission: "root" })).message, /permission must be/); | |
| 2079 | + assert.match((await status(422, "PUT", `${R}/collaborators/guild`, { permission: "push" })).message, /organization can't be a collaborator/); | |
| 2080 | + await status(204, "PUT", `${R}/collaborators/rita`, { permission: "triage" }); | |
| 2081 | + // A check run's link is a web address. | |
| 2082 | + for (const details_url of ["javascript:alert(1)", "data:text/html,hi", "//evil.example/x"]) await status(422, "POST", `${R}/check-runs`, { name: "ext", head_sha: sha, details_url }); | |
| 2083 | + await status(201, "POST", `${R}/check-runs`, { name: "ext", head_sha: sha, details_url: "https://ci.example/run/1" }); | |
| 2084 | + // Paging that isn't a number falls back to the defaults instead of an odd query. | |
| 2085 | + assert.equal((await ok("GET", `${R}/actions/runs?page=-3&per_page=abc`)).total_count, 0); | |
| 2086 | + // One MCP request carries at most 20 calls. | |
| 2087 | + const batch = Array.from({ length: 21 }, (_, i) => ({ jsonrpc: "2.0", id: i, method: "ping" })); | |
| 2088 | + const r = await fetch(`${base}/mcp`, { method: "POST", headers: { authorization: `Bearer ${T.olive}`, "content-type": "application/json", accept: "application/json, text/event-stream" }, body: JSON.stringify(batch) }); | |
| 2089 | + assert.equal(r.status, 400); | |
| 2090 | + assert.match((await r.json()).error.message, /at most 20 requests/); | |
| 2091 | + // Names people would trust aren't for the taking. | |
| 2092 | + for (const login of ["ghost", "github-actions", "noreply", "support", "abuse"]) await status(422, "POST", "/user/orgs", { login }); | |
| 2093 | +}); |