From 080c113d69c9bd6986d4940fcc44f6d2025c26fc Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 11:47:36 +0200 Subject: [PATCH 1/5] Let the forge's tools never weaken protection or merge past the checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gitea_api took any endpoint with a token minted for the forge's administrator with write:admin: an agent could drop a branch's protection or its required checks, set the merge check's status, write to main or merge its own pull request (novox/hq ADR 0266 §7, the review of 2026-10-09). gitea_api now reads anything but the administration and writes only what an author writes; protection is only strengthened; a merge needs every required status a success and merges only that head; a guarded repository is not deleted; and the token is minted without write:admin, a kept one re-minted at the next start. --- modules/gitea/client.ts | 8 +- modules/gitea/guard.ts | 178 +++++++++++++++++++++++++++++++ modules/gitea/test/guard.test.ts | 165 ++++++++++++++++++++++++++++ modules/gitea/test/token.test.ts | 47 +++++--- modules/gitea/token.ts | 32 ++++-- modules/gitea/tools/index.ts | 77 +++++++++++-- 6 files changed, 476 insertions(+), 31 deletions(-) create mode 100644 modules/gitea/guard.ts create mode 100644 modules/gitea/test/guard.test.ts diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 10577d13..368dbc96 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -463,10 +463,14 @@ export class GiteaClient { return res.text(); } - async mergePullRequest(owner: string, repo: string, index: number, method = "merge", deleteBranch = false): Promise { + /** Merge a pull request — never forced past its checks, and, given the head that was checked, only that head: + * the forge refuses the merge when the branch moved since (novox/hq ADR 0266). */ + async mergePullRequest(owner: string, repo: string, index: number, method = "merge", deleteBranch = false, + headCommit?: string): Promise { await this.request(`/repos/${owner}/${repo}/pulls/${index}/merge`, { method: "POST", - body: JSON.stringify({ Do: method, delete_branch_after_merge: deleteBranch }), + body: JSON.stringify({ Do: method, delete_branch_after_merge: deleteBranch, force_merge: false, + ...(headCommit ? { head_commit_id: headCommit } : {}) }), }); } diff --git a/modules/gitea/guard.ts b/modules/gitea/guard.ts new file mode 100644 index 00000000..61721b5d --- /dev/null +++ b/modules/gitea/guard.ts @@ -0,0 +1,178 @@ +// What the forge's tools refuse (novox/hq ADR 0266): whoever may call a tool includes agents, and the token +// the tools hold is minted for the forge's administrator. Through `gitea_api` any endpoint was one call away: +// an agent could drop a branch's protection or its required checks, write to main, set the merge check's +// status, or merge its own pull request — and a module merged into the catalogue is what the next push sends, +// as root, to every node. So: +// +// - `gitea_api` reads anything but the forge's administration, and writes only what an author writes: +// issues, comments, labels, pull requests (opened, edited, reviewed — never merged), branches, and a +// file on a branch that is neither the repository's default branch nor one a protection rule names. +// Everything else — administration, protection, settings, collaborators, teams, hooks, keys, tokens, +// statuses, a merge, a repository deleted or moved — has its own tool or is the operator's, at the +// forge as themselves; +// - `gitea_branch_protection_set` only makes protection stronger; +// - `gitea_merge_pull_request` merges only a head whose every required status succeeded, and only that +// head; +// - `gitea_delete_repo` refuses a repository any protection rule guards. +// +// Pure, so it is tested without a forge; the tools do the asking (tools/index.ts). + +import type { BranchProtection, ProtectionWanted } from "./protection.js"; + +/** A refusal of policy, in the words the caller sees. Never an error of the forge's. */ +export class Refused extends Error { + constructor(message: string) { + super(message); + this.name = "Refused"; + } +} + +/** What `gitea_api` may do with one call, as far as the call alone says. */ +export type ApiJudgement = + | { allowed: true } + | { refused: string } + /** A file written: allowed only on a branch that is not the default branch and that no rule names. */ + | { contents: { owner: string; repo: string; branch?: string; newBranch?: string } }; + +const READS = new Set(["GET", "HEAD"]); + +const seg = "([^/]+)"; +const repoPath = `^/repos/${seg}/${seg}`; + +/** The writes an author makes, by method and path. Anything not here is refused. */ +const AUTHOR_WRITES: ReadonlyArray<{ methods: string[]; path: RegExp }> = [ + { methods: ["POST"], path: new RegExp(`${repoPath}/issues$`) }, + { methods: ["PATCH"], path: new RegExp(`${repoPath}/issues/\\d+$`) }, + { methods: ["POST"], path: new RegExp(`${repoPath}/issues/\\d+/comments$`) }, + { methods: ["PATCH", "DELETE"], path: new RegExp(`${repoPath}/issues/comments/\\d+$`) }, + { methods: ["POST", "PUT", "DELETE"], path: new RegExp(`${repoPath}/issues/\\d+/labels(/\\d+)?$`) }, + { methods: ["POST"], path: new RegExp(`${repoPath}/labels$`) }, + { methods: ["PATCH", "DELETE"], path: new RegExp(`${repoPath}/labels/\\d+$`) }, + { methods: ["POST"], path: new RegExp(`${repoPath}/pulls$`) }, + { methods: ["PATCH"], path: new RegExp(`${repoPath}/pulls/\\d+$`) }, + { methods: ["POST"], path: new RegExp(`${repoPath}/pulls/\\d+/reviews(/\\d+(/comments)?)?$`) }, + { methods: ["POST", "DELETE"], path: new RegExp(`${repoPath}/pulls/\\d+/requested_reviewers$`) }, + { methods: ["POST"], path: new RegExp(`${repoPath}/branches$`) }, + { methods: ["POST"], path: /^\/user\/repos$/ }, + { methods: ["POST"], path: /^\/markdown(\/raw)?$/ }, +]; + +/** A file written through the contents API: one file, or several in one commit. */ +const CONTENTS = new RegExp(`${repoPath}/contents(/.*)?$`); + +/** Reads that are no one's but the administrator's. */ +const ADMIN_READS = [/^\/admin(\/|$)/, /^\/users\/[^/]+\/tokens(\/|$)/]; + +/** + * The path as the forge would route it: relative to /api/v1, its query dropped, refused when it could be read + * as another path — a dot segment, an encoded slash or dot, a backslash, an empty segment. A path the forge and + * this judge could read differently is never judged. + */ +export function routedPath(raw: string): string { + let path = String(raw ?? "").trim(); + if (!path.startsWith("/")) path = "/" + path; + path = path.replace(/^\/api\/v1(?=\/|$)/, "") || "/"; + const q = path.search(/[?#]/); + if (q >= 0) path = path.slice(0, q); + if (/%2e|%2f|%5c|\\/i.test(path)) throw new Refused(`${raw}: an encoded dot, slash or backslash is not taken`); + path = path.replace(/\/+$/, "") || "/"; + const segments = path.split("/").slice(1); + if (path !== "/" && segments.some((s) => s === "" || s === "." || s === "..")) { + throw new Refused(`${raw}: a path with an empty or dot segment is not taken`); + } + return path; +} + +/** What `gitea_api` may do with this call, judged on the call alone (novox/hq ADR 0266). */ +export function judgeApiCall(method: string, rawPath: string, body: unknown): ApiJudgement { + const m = String(method || "GET").toUpperCase(); + let path: string; + try { + path = routedPath(rawPath); + } catch (err) { + return { refused: (err as Error).message }; + } + if (READS.has(m)) { + if (ADMIN_READS.some((r) => r.test(path))) { + return { refused: `${m} ${path} is the forge's administration, which no tool reads (novox/hq ADR 0266)` }; + } + return { allowed: true }; + } + const contents = path.match(CONTENTS); + if (contents && ["POST", "PUT", "DELETE"].includes(m)) { + const b = (body ?? {}) as Record; + const str = (v: unknown) => (typeof v === "string" && v.trim() !== "" ? v.trim() : undefined); + return { contents: { owner: contents[1], repo: contents[2], branch: str(b.branch), newBranch: str(b.new_branch) } }; + } + if (AUTHOR_WRITES.some((w) => w.methods.includes(m) && w.path.test(path))) return { allowed: true }; + return { + refused: `${m} ${path} is not a write gitea_api makes (novox/hq ADR 0266): it writes only what an author ` + + `writes — issues, comments, labels, pull requests (never their merge), branches, and files on a branch ` + + `that is not the default branch and that no protection rule names. The forge's administration, a ` + + `branch's protection, a repository's settings, collaborators, teams, hooks, keys, tokens and statuses ` + + `are the operator's at the forge, or have their own tool (gitea_merge_pull_request, gitea_commit_status, ` + + `gitea_branch_protection_set, which only strengthens). Nothing was sent`, + }; +} + +/** Whether a protection rule's name — a branch, or a glob of them (`*`, `**`) — covers a branch. */ +export function ruleCovers(rule: string, branch: string): boolean { + if (rule === branch) return true; + if (!/[*?[]/.test(rule)) return false; + let re = ""; + for (let i = 0; i < rule.length; i++) { + const c = rule[i]; + if (c === "*" && rule[i + 1] === "*") { + re += ".*"; + i++; + } else if (c === "*") re += "[^/]*"; + else if (c === "?") re += "[^/]"; + else re += c.replace(/[.+^${}()|[\]\\]/g, "\\$&"); + } + return new RegExp(`^${re}$`).test(branch); +} + +/** Refuses a file written to the default branch or to a branch a protection rule names. */ +export function judgeContentsWrite(target: string, defaultBranch: string, rules: BranchProtection[]): string | null { + if (target === defaultBranch) { + return `a file written to ${target}, the repository's default branch, is refused: a change reaches it by a pull ` + + `request that passes its checks (novox/hq ADR 0266). Write it on a branch of its own (new_branch). Nothing was sent`; + } + const guarding = rules.map((r) => String(r.rule_name ?? r.branch_name ?? "")).filter((r) => r && ruleCovers(r, target)); + if (guarding.length) { + return `a file written to ${target} is refused: the protection rule ${guarding.join(", ")} guards it ` + + `(novox/hq ADR 0266). Write it on a branch of its own. Nothing was sent`; + } + return null; +} + +/** Refuses a protection change that weakens the rule a branch has (novox/hq ADR 0266). */ +export function judgeProtectionChange(before: BranchProtection | null, want: ProtectionWanted): string | null { + if (!before) return null; // A new rule protects what nothing protected. + const had = before.enable_status_check ? [...new Set((before.status_check_contexts ?? []).filter(Boolean))] : []; + const wanted = new Set(want.statusChecks.map((c) => c.trim()).filter(Boolean)); + const dropped = had.filter((c) => !wanted.has(c)); + const why: string[] = []; + if (dropped.length) why.push(`it drops the required status ${dropped.join(", ")}`); + if (want.push === true && before.enable_push === false) why.push("it lets a person push to the branch directly"); + if (want.blockAdminOverride === false && before.block_admin_merge_override === true) { + why.push("it lets an administrator merge past a status that has not succeeded"); + } + if (!why.length) return null; + return `the protection of ${before.rule_name ?? before.branch_name} is only made stronger through a tool: ` + + `${why.join("; ")}. Whoever may call a tool includes agents, and a branch's protection is what keeps a ` + + `change from merging without its checks and the operator (novox/hq ADR 0266) — weakening it is the ` + + `operator's, at the forge as themselves. Nothing was changed`; +} + +/** The merge methods the forge takes for a merge that is a merge — never `manually-merged`. */ +export const MERGE_METHODS = new Set(["merge", "rebase", "rebase-merge", "squash", "fast-forward-only"]); + +/** A repository owner's or name's shape: what keeps a name from becoming another path. */ +export function checkName(what: string, value: unknown): string { + const v = String(value ?? "").trim(); + if (!/^[A-Za-z0-9_][A-Za-z0-9_.-]*$/.test(v) || v === "." || v === "..") { + throw new Refused(`${JSON.stringify(value)} is not a repository ${what}`); + } + return v; +} diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts new file mode 100644 index 00000000..31a04804 --- /dev/null +++ b/modules/gitea/test/guard.test.ts @@ -0,0 +1,165 @@ +// What the forge's tools refuse (guard.ts; novox/hq ADR 0266): an agent reaches the forge through these tools +// with a token minted for the forge's administrator, so the tools — not the forge — are what keeps it from +// dropping a branch's protection, writing to main, setting the merge check's status, or merging past the +// checks. Each route a review found is reproduced here against a fake forge, and each is refused with nothing +// sent; the operator-approved merge of a pull request whose checks passed still goes through. + +import { test, after, beforeEach } from "node:test"; +import assert from "node:assert/strict"; +import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; + +import { GiteaClient } from "../dist/client.js"; +import { ConfiguredToken } from "../dist/token.js"; +import { getGiteaTools } from "../dist/tools/index.js"; +import { judgeApiCall, judgeProtectionChange, ruleCovers, routedPath } from "../dist/guard.js"; + +interface Sent { method: string; path: string; body: any } + +const state = { + sent: [] as Sent[], + statuses: [] as { context: string; state: string }[], + rules: [] as any[], +}; +const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_status_check: true, + status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"], block_admin_merge_override: true }); + +beforeEach(() => { + state.sent = []; + state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "success" }]; + state.rules = [protectedMain()]; +}); + +const json = (res: ServerResponse, status: number, body: unknown): void => { + res.writeHead(status, { "Content-Type": "application/json" }); + res.end(body === null ? "" : JSON.stringify(body)); +}; +const read = (req: IncomingMessage): Promise => + new Promise((resolve) => { + let text = ""; + req.on("data", (c) => (text += c)); + req.on("end", () => resolve(text ? JSON.parse(text) : null)); + }); + +const server = createServer(async (req, res) => { + const url = new URL(req.url ?? "/", "http://fake"); + const path = url.pathname.replace(/^\/api\/v1/, ""); + const method = req.method ?? "GET"; + const body = await read(req); + if (method !== "GET") state.sent.push({ method, path, body }); + const repo = "/repos/novox/mesh-catalog"; + if (method === "GET" && path === repo) return json(res, 200, { full_name: "novox/mesh-catalog", default_branch: "main" }); + if (method === "GET" && path === `${repo}/branch_protections`) return json(res, 200, state.rules); + if (method === "GET" && path === `${repo}/pulls/7`) { + return json(res, 200, { number: 7, title: "x", state: "open", merged: false, head: { ref: "feat/x", sha: "abc1234" }, + base: { ref: "main" }, html_url: "http://fake/7" }); + } + if (method === "GET" && path === `${repo}/commits/abc1234/status`) { + return json(res, 200, { sha: "abc1234", state: "success", statuses: state.statuses }); + } + if (method === "POST" && path === `${repo}/pulls/7/merge`) return json(res, 200, null); + return json(res, method === "GET" ? 200 : 201, {}); +}); +await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); +after(() => new Promise((r) => server.close(() => r()))); +const { port } = server.address() as { port: number }; +const tools = getGiteaTools(new GiteaClient(`http://127.0.0.1:${port}`, new ConfiguredToken("t"))); +const tool = (name: string) => tools.find((t) => t.name === name)!; +const writes = () => state.sent.map((s) => `${s.method} ${s.path}`); + +test("gitea_api refuses the forge's administration, a branch's protection and the merge check's status, and sends nothing", async () => { + for (const [method, path, body] of [ + ["PATCH", "/admin/users/agent", { admin: true }], + ["DELETE", "/repos/novox/mesh-catalog/branch_protections/main", undefined], + ["PATCH", "/repos/novox/mesh-catalog/branch_protections/main", { enable_status_check: false }], + ["POST", "/repos/novox/mesh-catalog/statuses/abc1234", { context: "mesh/merge-gate", state: "success" }], + ["POST", "/repos/novox/mesh-catalog/pulls/7/merge", { Do: "merge", force_merge: true }], + ["PUT", "/repos/novox/mesh-catalog/collaborators/agent", { permission: "admin" }], + ["POST", "/repos/novox/mesh-catalog/hooks", { type: "gitea" }], + ["POST", "/repos/novox/mesh-catalog/keys", { key: "ssh-ed25519 AAAA" }], + ["PATCH", "/repos/novox/mesh-catalog", { default_branch: "agent" }], + ["DELETE", "/repos/novox/mesh-catalog", undefined], + ["POST", "/repos/novox/mesh-catalog/transfer", { new_owner: "agent" }], + ["POST", "/users/mesh-admin/tokens", { name: "mine", scopes: ["all"] }], + ["PUT", "/orgs/novox/members/agent", undefined], + ["PATCH", "/repos/novox/mesh-catalog/issues/1/../../branch_protections/main", { enable_push: true }], + ["PATCH", "/repos/novox/mesh-catalog/issues/1/%2e%2e/%2e%2e/branch_protections/main", { enable_push: true }], + ] as const) { + await assert.rejects(tool("gitea_api").run({ path, method, ...(body ? { body } : {}) }), + /not a write gitea_api makes|not taken|administration/, `${method} ${path}`); + } + await assert.rejects(tool("gitea_api").run({ path: "/admin/users" }), /administration/); + assert.deepEqual(writes(), []); +}); + +test("gitea_api refuses a file written to the default branch or one a rule guards, and writes one on a branch of its own", async () => { + const put = (body: Record) => + tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/contents/modules/sudo/module.json", method: "PUT", body }); + await assert.rejects(put({ content: "e30=", sha: "x" }), /default branch/); + await assert.rejects(put({ content: "e30=", sha: "x", branch: "main" }), /default branch/); + state.rules.push({ rule_name: "release/*" }); + await assert.rejects(put({ content: "e30=", sha: "x", branch: "release/1" }), /protection rule release\/\*/); + assert.deepEqual(writes(), []); + await put({ content: "e30=", sha: "x", branch: "main", new_branch: "feat/sudo-docs" }); + assert.deepEqual(writes(), ["PUT /repos/novox/mesh-catalog/contents/modules/sudo/module.json"]); +}); + +test("gitea_api still writes what an author writes", async () => { + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/issues/3/comments", method: "POST", body: { body: "hi" } }); + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7/reviews", method: "POST", body: { event: "COMMENT" } }); + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/branch_protections" }); + assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/issues/3/comments", "POST /repos/novox/mesh-catalog/pulls/7/reviews"]); +}); + +test("gitea_branch_protection_set only strengthens", async () => { + const set = (args: Record) => + tool("gitea_branch_protection_set").run({ owner: "novox", repo: "mesh-catalog", branch: "main", ...args }); + await assert.rejects(set({ status_checks: "" }), /drops the required status mesh\/merge-gate, mesh\/repo-check/); + await assert.rejects(set({ status_checks: "mesh/merge-gate" }), /drops the required status mesh\/repo-check/); + await assert.rejects(set({ status_checks: "mesh/merge-gate,mesh/repo-check", push: true }), /push to the branch directly/); + await assert.rejects(set({ status_checks: "mesh/merge-gate,mesh/repo-check", block_admin_override: false }), + /merge past a status/); + assert.deepEqual(writes(), []); + await set({ status_checks: "mesh/merge-gate,mesh/repo-check,mesh/delivery" }); + assert.deepEqual(writes(), ["PATCH /repos/novox/mesh-catalog/branch_protections/main"]); +}); + +test("gitea_merge_pull_request merges only a head whose required statuses all succeeded, and only that head", async () => { + const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }); + state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "failure" }]; + await assert.rejects(merge(), /not merged: .*mesh\/repo-check \(failure\)/); + state.statuses = [{ context: "mesh/merge-gate", state: "success" }]; + await assert.rejects(merge(), /mesh\/repo-check \(missing\)/); + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, + method: "manually-merged" }), /not a merge method/); + assert.deepEqual(writes(), []); + // The operator said yes and the checks passed: the flow that stays. + state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "success" }]; + await merge(); + const sent = state.sent.find((s) => s.path.endsWith("/pulls/7/merge"))!; + assert.equal(sent.body.force_merge, false); + assert.equal(sent.body.head_commit_id, "abc1234"); +}); + +test("gitea_delete_repo refuses a repository a protection rule guards", async () => { + await assert.rejects(tool("gitea_delete_repo").run({ owner: "novox", name: "mesh-catalog", confirm: true }), + /guarded by the protection rule main/); + assert.deepEqual(writes(), []); +}); + +test("a repository owner or name is never another path", async () => { + await assert.rejects(tool("gitea_update_pull_request").run({ owner: "novox", repo: "mesh-catalog/branch_protections/main?", + number: 1, title: "x" }), /not a repository name/); + await assert.rejects(tool("gitea_get_issue").run({ owner: "..", repo: "admin", number: 1 }), /not a repository owner/); + assert.deepEqual(writes(), []); +}); + +test("the judges, without a forge", () => { + assert.equal(routedPath("/api/v1/repos/a/b/issues?x=1"), "/repos/a/b/issues"); + assert.throws(() => routedPath("/repos/a/b/../c"), /dot segment/); + assert.ok("allowed" in judgeApiCall("GET", "/repos/a/b/branch_protections", undefined)); + assert.ok("refused" in judgeApiCall("POST", "/repos/a/b/branch_protections", {})); + assert.ok("contents" in judgeApiCall("POST", "/repos/a/b/contents", { new_branch: "x" })); + assert.ok(ruleCovers("release/**", "release/a/b") && ruleCovers("feat/*", "feat/a") && !ruleCovers("feat/*", "feat/a/b")); + assert.equal(judgeProtectionChange(null, { statusChecks: [] }), null); + assert.equal(judgeProtectionChange(protectedMain(), { statusChecks: ["mesh/repo-check", "mesh/merge-gate"] }), null); +}); diff --git a/modules/gitea/test/token.test.ts b/modules/gitea/test/token.test.ts index c6f3b4a7..48201041 100644 --- a/modules/gitea/test/token.test.ts +++ b/modules/gitea/test/token.test.ts @@ -17,7 +17,7 @@ import { join } from "node:path"; import { collectTools } from "@novox/mesh-sdk/tools"; import { AdminRefused, MintedToken, TOKEN_SCOPES } from "../dist/token.js"; import { GiteaClient } from "../dist/client.js"; -import "../dist/tools/index.js"; +import { getGiteaTools } from "../dist/tools/index.js"; const ADMIN = "mesh-admin"; const PASSWORD = "the-vault-minted-this"; @@ -230,10 +230,12 @@ test("first start: mints with the admin account, keeps the token at 0600, asks f assert.equal(repos[0]?.full_name, "novox/hq"); assert.equal(forge.mints, 1); - assert.deepEqual(forge.lastScopes, ["write:repository", "write:issue", "read:user", "write:admin"]); + // Never write:admin (novox/hq ADR 0266): the forge's administration is the operator's, not a tool's. + assert.deepEqual(forge.lastScopes, ["write:repository", "write:issue", "read:user"]); assert.deepEqual(forge.lastScopes, [...TOKEN_SCOPES]); const token = forge.tokens.get("mesh-tools")!; assert.equal(await readFile(file, "utf8"), token + "\n"); + assert.equal(await readFile(file + ".scopes", "utf8"), "read:user,write:issue,write:repository\n"); assert.equal((await stat(file)).mode & 0o777, 0o600); // Said that it minted, and where it keeps it — never what it is. assert.ok(logs.some((l) => l.startsWith("minted a token")), logs.join("\n")); @@ -268,32 +270,45 @@ test("the forge rejects the kept token (its data was restored): minted afresh, o assert.ok(logs.some((l) => l.startsWith("the forge rejected the kept token")), logs.join("\n")); }); -test("a kept token from before write:admin: the forge refuses the admin route for the scope, the token is re-minted with the whole list, and the call goes through", async () => { +test("a token kept with write:admin by an earlier build is minted again without it at the next start, and the forge drops the old one", async () => { + const { env, file, logs } = await delivered(forge); + await minted(env, logs).listRepos(); + const old = forge.tokens.get("mesh-tools")!; + forge.scopesOf.set(old, ["write:repository", "write:issue", "read:user", "write:admin"]); + await rm(file + ".scopes"); // an earlier build kept no scopes beside it + const before = forge.mints; + + const again: string[] = []; + await minted(env, again).listRepos(); + + assert.equal(forge.mints, before + 1); + assert.deepEqual(forge.lastScopes, [...TOKEN_SCOPES]); + assert.ok(!forge.lastScopes!.includes("write:admin")); + assert.notEqual(forge.tokens.get("mesh-tools"), old); + assert.ok(![...forge.tokens.values()].includes(old), "the old token still stands on the forge"); + assert.ok(again.some((l) => l.includes("minted with other scopes")), again.join("\n")); + // An admin route through the escape hatch is refused before it is sent. + const tool = getGiteaTools(minted(env, again)).find((t) => t.name === "gitea_api")!; + await assert.rejects(tool.run({ path: "/admin/users/mesh_novox_builder", method: "PATCH", body: { admin: true } }), + /not a write gitea_api makes/); +}); + +test("a kept token lacking a scope the tools need: the forge refuses the call for the scope, the token is re-minted with the whole list, and the call goes through", async () => { const { env, file, logs } = await delivered(forge); const client = minted(env, logs); await client.listRepos(); const before = forge.mints; const old = forge.tokens.get("mesh-tools")!; - forge.scopesOf.set(old, ["write:repository", "write:issue", "read:user"]); // minted by the previous build + forge.scopesOf.set(old, ["write:repository", "write:issue"]); // minted before read:user was added - const user = await client.api<{ login: string; is_admin: boolean }>("/admin/users/mesh_novox_builder", { - method: "PATCH", - body: JSON.stringify({ admin: true }), - }); + const repos = await client.api("/user/repos"); - assert.equal(user.is_admin, true); + assert.equal(repos.length, 1); assert.equal(forge.mints, before + 1); assert.deepEqual(forge.lastScopes, [...TOKEN_SCOPES]); assert.notEqual(forge.tokens.get("mesh-tools"), old); assert.equal(await readFile(file, "utf8"), forge.tokens.get("mesh-tools") + "\n"); assert.ok(logs.some((l) => l.startsWith("the forge rejected the kept token")), logs.join("\n")); - // A 403 that is not about scopes is the forge's answer, not a reason to mint. - const again = forge.mints; - await assert.rejects( - client.api("/admin/users/untouchable", { method: "PATCH", body: JSON.stringify({ admin: true }) }), - /403 .*untouchable/, - ); - assert.equal(forge.mints, again); }); test("the kept file is gone but the forge still holds a token by that name: replaced, not refused", async () => { diff --git a/modules/gitea/token.ts b/modules/gitea/token.ts index fb1e53a9..3aac30b1 100644 --- a/modules/gitea/token.ts +++ b/modules/gitea/token.ts @@ -34,15 +34,18 @@ export const TOKEN_NAME = "mesh-tools"; * It sits under the `user` category despite listing repositories, not `repository` * — confirmed against the running forge (1.27.3), which answered * `required=[read:user]` to a token carrying only the other two. - * write:admin — /admin/users: the forge's own users are the mesh's to settle, such as making - * the builder's login a site admin so every repository the mesh may build is - * clonable (novox/hq 229). Nothing under /orgs or write:user. + * + * **Not write:admin** (novox/hq ADR 0266). It was here so a person could settle the forge's users through + * `gitea_api` (novox/hq 229); whoever may call a tool includes agents, and the forge's administration is the + * operator's. The provisioner settles users with the admin account's own login (GiteaAdmin), never this token. + * A token kept with a different list is minted again with this one at the next start (the scopes are kept + * beside it), and the forge drops the old one by name — so the scope a kept token had does not outlive this. * * A token kept from before a scope was added lacks it: the forge answers such a call with * `403 token does not have at least one of required scope(s)`, and the client treats that like a * 401 — the source re-mints by name, with the whole list, and the call is retried once. */ -export const TOKEN_SCOPES: readonly string[] = ["write:repository", "write:issue", "read:user", "write:admin"]; +export const TOKEN_SCOPES: readonly string[] = ["write:repository", "write:issue", "read:user"]; /** Where a client's token comes from, and what to do when the forge says it is wrong. */ export interface TokenSource { @@ -153,11 +156,12 @@ export class MintedToken implements TokenSource { if (!this.readFile) { this.readFile = true; const kept = this.read(); - if (kept !== null) { + if (kept !== null && this.keptScopes() === this.scopesLine()) { this.held = kept; this.log(`reusing the token kept at ${this.opts.file}`); return kept; } + if (kept !== null) return this.mint("the kept token was minted with other scopes than the tools take now — minting one with them"); } return this.mint("no token kept — minting one"); } @@ -202,13 +206,29 @@ export class MintedToken implements TokenSource { } } - /** Write the token at 0600, whole or not at all: a temp file beside it, then a rename. */ + /** The scopes as kept beside the token: sorted, one line. */ + private scopesLine(): string { + return [...this.scopes].sort().join(","); + } + + /** The scopes the kept token was minted with, "" when not kept (a token kept before they were). */ + private keptScopes(): string { + try { + return readFileSync(`${this.opts.file}.scopes`, "utf8").trim(); + } catch { + return ""; + } + } + + /** Write the token at 0600, whole or not at all: a temp file beside it, then a rename. Its scopes beside it. */ private keep(token: string): void { mkdirSync(dirname(this.opts.file), { recursive: true, mode: 0o700 }); const tmp = `${this.opts.file}.tmp`; writeFileSync(tmp, token + "\n", { mode: 0o600 }); chmodSync(tmp, 0o600); renameSync(tmp, this.opts.file); + writeFileSync(`${this.opts.file}.scopes.tmp`, this.scopesLine() + "\n", { mode: 0o600 }); + renameSync(`${this.opts.file}.scopes.tmp`, `${this.opts.file}.scopes`); } private async doMint(): Promise { diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 33b70d41..c2bf056a 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -13,6 +13,9 @@ import { emit } from "@novox/mesh-sdk/events"; import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, deliveryStatus, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; +import { + MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeProtectionChange, +} from "../guard.js"; /** The forge's container, where its repositories and git are: the note is written there (novox/hq ADR 0239). */ const forgeContainer = process.env.MESH_GITEA_CONTAINER || "gitea"; @@ -37,7 +40,26 @@ function parseLabels(raw: unknown): string[] { .filter(Boolean); } +/** Every tool's repository owner and name checked before it runs: a name is never another path (guard.ts). */ +function namesChecked(tools: ToolDefinition[]): ToolDefinition[] { + return tools.map((t) => ({ + ...t, + run: async (args: Record) => { + if (args.owner !== undefined) checkName("owner", args.owner); + if (args.repo !== undefined) checkName("name", args.repo); + if ((t.name === "gitea_create_repo" || t.name === "gitea_delete_repo") && args.name !== undefined) { + checkName("name", args.name); + } + return t.run(args); + }, + })); +} + export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { + return namesChecked(allGiteaTools(gitea)); +} + +function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { return [ // ---- Repositories ---- { @@ -80,6 +102,14 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, run: async (args) => { if (!args.confirm) return { deleted: false, reason: "confirm must be true to delete a repository" }; + // A repository a protection rule guards is the mesh's: deleted and made again, it would be one without + // its protection, under the name the mesh builds from (novox/hq ADR 0266). + const guarded = await gitea.branchProtections(String(args.owner), String(args.name)); + if (guarded.length) { + throw new Refused(`${String(args.owner)}/${String(args.name)} is guarded by the protection rule ` + + `${guarded.map((r) => r.rule_name ?? r.branch_name).join(", ")}: deleting it is the operator's, at the forge ` + + `as themselves, never a tool's (novox/hq ADR 0266). Nothing was deleted`); + } await gitea.deleteRepo(String(args.owner), String(args.name)); return { deleted: true, repo: `${String(args.owner)}/${String(args.name)}` }; }, @@ -230,7 +260,9 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_merge_pull_request", - description: "Merge a pull request, optionally deleting the source branch afterwards.", + description: "Merge a pull request whose head has every status its base branch's protection requires as a success " + + "(mesh/merge-gate, mesh/repo-check), optionally deleting the source branch afterwards. Refused otherwise — no " + + "tool merges past a check — and only the head that was checked is merged.", input: { owner: { type: "string", description: "the repository owner" }, repo: { type: "string", description: "the repository name" }, @@ -243,8 +275,22 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { const repo = String(args.repo); const number = Number(args.number); const method = args.method ? String(args.method) : "merge"; + if (!MERGE_METHODS.has(method)) throw new Refused(`${method} is not a merge method: ${[...MERGE_METHODS].join(", ")}`); const deleteBranch = args.delete_branch === undefined ? true : Boolean(args.delete_branch); - await gitea.mergePullRequest(owner, repo, number, method, deleteBranch); + // **Only a head whose required statuses all succeeded** (novox/hq ADR 0266): read here rather than left + // to the forge, whose administrator — the account this tool's token is minted for — may merge past a + // rule that lets it. A rule that cannot be read is no rule passed. + const pull = await gitea.getPullRequest(owner, repo, number); + if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); + if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); + const rule = await gitea.branchProtection(owner, repo, pull.base); + const statuses = newestByContext((await gitea.combinedStatus(owner, repo, pull.head_sha)).statuses); + const verdict = mergeableUnder(pull.base, rule, statuses); + if (verdict.mergeable !== true) { + throw new Refused(`${owner}/${repo}#${number} is not merged: ${verdict.says}. A pull request merges once ` + + `its checks succeed; no tool merges past them (novox/hq ADR 0266)`); + } + await gitea.mergePullRequest(owner, repo, number, method, deleteBranch, pull.head_sha); // The merge commit only exists now; answered so the caller can follow what is built from it. // pull.merged is the events entrypoint's to announce (index.ts), once, within its poll. const merged = await gitea.getPullRequest(owner, repo, number); @@ -395,7 +441,7 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_branch_protection_set", - description: "Make a branch require commit statuses before a pull request merges into it — the mesh's merge check sets `mesh/merge-gate` (the module graph's gate) and `mesh/repo-check` (the repository's own tests). Edits the branch's rule (its required statuses replaced by these, everything else kept unless given) or creates one, which refuses direct pushes unless push=true. Answers the rule before and after.", + description: "Only ever stronger (novox/hq ADR 0266): a change that drops a required status, lets a person push directly or lets an administrator merge past a status is refused — that is the operator's, at the forge. Make a branch require commit statuses before a pull request merges into it — the mesh's merge check sets `mesh/merge-gate` (the module graph's gate) and `mesh/repo-check` (the repository's own tests). Edits the branch's rule (its required statuses replaced by these, everything else kept unless given) or creates one, which refuses direct pushes unless push=true. Answers the rule before and after.", input: { owner: { type: "string", description: "the repository owner" }, repo: { type: "string", description: "the repository name" }, @@ -408,11 +454,14 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { const owner = String(args.owner), repo = String(args.repo), branch = String(args.branch ?? "").trim(); if (!branch) throw new Error("name the branch to protect"); const before = await gitea.branchProtection(owner, repo, branch); - const { created, rule } = await gitea.setBranchProtection(owner, repo, branch, { + const want = { statusChecks: String(args.status_checks ?? "").split(","), push: args.push === undefined ? undefined : Boolean(args.push), blockAdminOverride: args.block_admin_override === undefined ? undefined : Boolean(args.block_admin_override), - }); + }; + const weakens = judgeProtectionChange(before, want); + if (weakens) throw new Refused(weakens); + const { created, rule } = await gitea.setBranchProtection(owner, repo, branch, want); return { branch, created, before: before ? summarised(before) : null, after: summarised(rule) }; }, }, @@ -562,14 +611,28 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { // ---- Escape hatch ---- { name: "gitea_api", - description: "Make an authenticated Gitea API call for any endpoint without a dedicated tool. Path is relative to /api/v1.", + description: "Make an authenticated Gitea API call for an endpoint without a dedicated tool. Path is relative to /api/v1. " + + "Reads anything but the forge's administration; writes only what an author writes — issues, comments, labels, " + + "pull requests (not their merge: gitea_merge_pull_request), branches, and files on a branch that is not the " + + "default branch and that no protection rule names. Administration, protection, settings, collaborators, teams, " + + "hooks, keys, tokens and statuses are refused (novox/hq ADR 0266).", input: { path: { type: "string", description: "API path relative to /api/v1, e.g. /repos/owner/repo/branches" }, method: { type: "string", description: "GET | POST | PUT | PATCH | DELETE (default GET)" }, body: { type: "object", description: "JSON request body for POST/PUT/PATCH" }, }, run: async (args) => { - const method = args.method ? String(args.method) : "GET"; + const method = (args.method ? String(args.method) : "GET").toUpperCase(); + const judged = judgeApiCall(method, String(args.path ?? ""), args.body); + if ("refused" in judged) throw new Refused(judged.refused); + if ("contents" in judged) { + const { owner, repo, branch, newBranch } = judged.contents; + const found = await gitea.api<{ default_branch?: string }>(`/repos/${checkName("owner", owner)}/${checkName("name", repo)}`); + const defaultBranch = String(found?.default_branch ?? "main"); + const refused = judgeContentsWrite(newBranch ?? branch ?? defaultBranch, defaultBranch, + await gitea.branchProtections(owner, repo)); + if (refused) throw new Refused(refused); + } const result = await gitea.api(String(args.path), { method, ...(args.body ? { body: JSON.stringify(args.body) } : {}), From ff648ad32b48d25f3041c3a9f51abaa84a0b0588 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 12:26:37 +0200 Subject: [PATCH 2/5] Send the path gitea_api judged, keep a pull request's base, merge only into a guarded branch, and delete only the tools' own idle repositories Follow-ups of the review of 2026-10-09: the escape hatch sent the raw path it had judged in normalised form; a pull request's base could be moved through it; the merge tool would merge into a branch no rule guards; a rule named for a branch could undercut the glob rule guarding it; and the delete tool took any unguarded repository. It now takes only a repository of the tools' own user with no protection and no commit in 30 days, named in full. --- modules/gitea/guard.ts | 58 ++++++++++++++++++++++++++++++-- modules/gitea/test/guard.test.ts | 49 +++++++++++++++++++++++++-- modules/gitea/tools/index.ts | 41 ++++++++++++++-------- 3 files changed, 128 insertions(+), 20 deletions(-) diff --git a/modules/gitea/guard.ts b/modules/gitea/guard.ts index 61721b5d..7fa757f3 100644 --- a/modules/gitea/guard.ts +++ b/modules/gitea/guard.ts @@ -104,6 +104,12 @@ export function judgeApiCall(method: string, rawPath: string, body: unknown): Ap const str = (v: unknown) => (typeof v === "string" && v.trim() !== "" ? v.trim() : undefined); return { contents: { owner: contents[1], repo: contents[2], branch: str(b.branch), newBranch: str(b.new_branch) } }; } + // A pull request's base changed through the escape hatch would merge what was checked against one branch into + // another; gitea_update_pull_request changes a title and a body, never a base. + if (m === "PATCH" && /^\/repos\/[^/]+\/[^/]+\/pulls\/\d+$/.test(path) && body && typeof body === "object" && + "base" in (body as Record)) { + return { refused: `${m} ${path} changing a pull request's base is refused (novox/hq ADR 0266): its checks were judged against the base it had. Open a new pull request. Nothing was sent` }; + } if (AUTHOR_WRITES.some((w) => w.methods.includes(m) && w.path.test(path))) return { allowed: true }; return { refused: `${m} ${path} is not a write gitea_api makes (novox/hq ADR 0266): it writes only what an author ` + @@ -146,9 +152,25 @@ export function judgeContentsWrite(target: string, defaultBranch: string, rules: return null; } -/** Refuses a protection change that weakens the rule a branch has (novox/hq ADR 0266). */ -export function judgeProtectionChange(before: BranchProtection | null, want: ProtectionWanted): string | null { - if (!before) return null; // A new rule protects what nothing protected. +/** + * Refuses a protection change that weakens what guards a branch (novox/hq ADR 0266): the branch's own rule, or — + * when it has none — every glob rule that covers it, since a rule named for the branch takes the glob's place. + * A rule made new takes the forge's defaults for what it does not say: pushes refused, an administrator's + * override allowed. + */ +export function judgeProtectionChange(before: BranchProtection | null, want: ProtectionWanted, + covering: BranchProtection[] = []): string | null { + if (!before) { + for (const glob of covering) { + const weaker = judgeProtectionChange(glob, { + ...want, + push: want.push ?? false, + blockAdminOverride: want.blockAdminOverride ?? false, + }); + if (weaker) return weaker; + } + return null; // A new rule protects what nothing protected. + } const had = before.enable_status_check ? [...new Set((before.status_check_contexts ?? []).filter(Boolean))] : []; const wanted = new Set(want.statusChecks.map((c) => c.trim()).filter(Boolean)); const dropped = had.filter((c) => !wanted.has(c)); @@ -165,6 +187,36 @@ export function judgeProtectionChange(before: BranchProtection | null, want: Pro `operator's, at the forge as themselves. Nothing was changed`; } +/** The token's own user's repositories, untouched for this long, are the only ones a tool deletes. */ +export const DELETABLE_AFTER_DAYS = 30; + +/** + * Refuses deleting a repository (novox/hq ADR 0266): deleted and made again under its name, a repository the mesh + * builds from would be one without its protection. A tool deletes only a repository of the token's own user — the + * mesh builds from organisations' and people's repositories, never the tools' own — that no protection rule + * guards, with no commit for DELETABLE_AFTER_DAYS days, and only when confirm names it in full. + */ +export function judgeDelete(fullName: string, confirm: unknown, owner: string, self: string, + rules: BranchProtection[], lastCommit: Date | null, now: Date): string | null { + if (String(confirm ?? "") !== fullName) { + return `confirm must be the repository's full name, ${fullName}, to delete it. Nothing was deleted`; + } + if (owner.toLowerCase() !== self.toLowerCase()) { + return `${fullName} is not a repository of the tools' own user (${self}): a tool deletes only those, never an ` + + `organisation's or a person's — the mesh builds from them (novox/hq ADR 0266). Deleting it is the operator's, at ` + + `the forge as themselves. Nothing was deleted`; + } + if (rules.length) { + return `${fullName} is guarded by the protection rule ${rules.map((r) => r.rule_name ?? r.branch_name).join(", ")}: ` + + `deleting it is the operator's, at the forge as themselves (novox/hq ADR 0266). Nothing was deleted`; + } + if (lastCommit && now.getTime() - lastCommit.getTime() < DELETABLE_AFTER_DAYS * 24 * 3600 * 1000) { + return `${fullName} had a commit on ${lastCommit.toISOString().slice(0, 10)}, within ${DELETABLE_AFTER_DAYS} days: ` + + `a repository in use is not deleted by a tool (novox/hq ADR 0266). Nothing was deleted`; + } + return null; +} + /** The merge methods the forge takes for a merge that is a merge — never `manually-merged`. */ export const MERGE_METHODS = new Set(["merge", "rebase", "rebase-merge", "squash", "fast-forward-only"]); diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 31a04804..02da7e17 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -19,6 +19,7 @@ const state = { sent: [] as Sent[], statuses: [] as { context: string; state: string }[], rules: [] as any[], + lastCommit: "2026-01-01T00:00:00Z", }; const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"], block_admin_merge_override: true }); @@ -49,6 +50,11 @@ const server = createServer(async (req, res) => { const repo = "/repos/novox/mesh-catalog"; if (method === "GET" && path === repo) return json(res, 200, { full_name: "novox/mesh-catalog", default_branch: "main" }); if (method === "GET" && path === `${repo}/branch_protections`) return json(res, 200, state.rules); + if (method === "GET" && path === "/user") return json(res, 200, { login: "mesh-admin" }); + if (method === "GET" && path === "/repos/mesh-admin/scratch/branch_protections") return json(res, 200, []); + if (method === "GET" && /^\/repos\/[^/]+\/[^/]+\/commits$/.test(path)) { + return json(res, 200, [{ commit: { committer: { date: state.lastCommit } } }]); + } if (method === "GET" && path === `${repo}/pulls/7`) { return json(res, 200, { number: 7, title: "x", state: "open", merged: false, head: { ref: "feat/x", sha: "abc1234" }, base: { ref: "main" }, html_url: "http://fake/7" }); @@ -140,10 +146,47 @@ test("gitea_merge_pull_request merges only a head whose required statuses all su assert.equal(sent.body.head_commit_id, "abc1234"); }); -test("gitea_delete_repo refuses a repository a protection rule guards", async () => { - await assert.rejects(tool("gitea_delete_repo").run({ owner: "novox", name: "mesh-catalog", confirm: true }), - /guarded by the protection rule main/); +test("gitea_delete_repo deletes only the tools' own user's repository, unguarded, untouched for 30 days, named in full", async () => { + const del = (owner: string, name: string, confirm: unknown) => tool("gitea_delete_repo").run({ owner, name, confirm }); + await assert.rejects(del("novox", "mesh-catalog", true), /confirm must be the repository's full name/); + await assert.rejects(del("novox", "mesh-catalog", "novox/mesh-catalog"), /not a repository of the tools' own user/); + state.lastCommit = new Date().toISOString(); + await assert.rejects(del("mesh-admin", "scratch", "mesh-admin/scratch"), /within 30 days/); assert.deepEqual(writes(), []); + state.lastCommit = "2026-01-01T00:00:00Z"; + await del("mesh-admin", "scratch", "mesh-admin/scratch"); + assert.deepEqual(writes(), ["DELETE /repos/mesh-admin/scratch"]); +}); + +test("gitea_api sends the path it judged, and refuses moving a pull request's base", async () => { + await tool("gitea_api").run({ path: "/api/v1/repos/novox/mesh-catalog/issues/3/comments/?x=1", method: "POST", body: { body: "hi" } }); + assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/issues/3/comments"]); + await assert.rejects(tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7", method: "PATCH", + body: { base: "unguarded" } }), /changing a pull request's base/); + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7", method: "PATCH", body: { title: "x" } }); + assert.equal(writes().length, 2); +}); + +test("gitea_merge_pull_request merges only into a branch whose protection requires the checks", async () => { + state.rules = []; + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }), + /no protection rule requiring the checks guards/); + state.rules = [{ rule_name: "main", enable_push: false, enable_status_check: false, status_check_contexts: [] }]; + await assert.rejects(tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7 }), + /no protection rule requiring the checks guards/); + assert.deepEqual(writes(), []); +}); + +test("a rule named for a branch is not made weaker than the glob rule that guards it now", async () => { + state.rules.push({ rule_name: "release/*", enable_push: false, enable_status_check: true, + status_check_contexts: ["mesh/merge-gate"], block_admin_merge_override: true }); + const set = (args: Record) => + tool("gitea_branch_protection_set").run({ owner: "novox", repo: "mesh-catalog", branch: "release/1", ...args }); + await assert.rejects(set({ status_checks: "" }), /drops the required status mesh\/merge-gate/); + await assert.rejects(set({ status_checks: "mesh/merge-gate" }), /merge past a status/); + assert.deepEqual(writes(), []); + await set({ status_checks: "mesh/merge-gate", block_admin_override: true }); + assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/branch_protections"]); }); test("a repository owner or name is never another path", async () => { diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index c2bf056a..91357c7f 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -14,7 +14,8 @@ import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, deliveryStatus, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; import { - MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeProtectionChange, + MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, routedPath, + ruleCovers, } from "../guard.js"; /** The forge's container, where its repositories and git are: the note is written there (novox/hq ADR 0239). */ @@ -94,22 +95,23 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { }, { name: "gitea_delete_repo", - description: "Delete a repository. Destructive and irreversible — requires confirm=true.", + description: "Delete a repository of the tools' own user that no protection rule guards and that had no commit " + + "in 30 days. Destructive and irreversible — confirm must be the repository's full name, owner/name. An " + + "organisation's or a person's repository is the operator's to delete, at the forge (novox/hq ADR 0266).", input: { owner: { type: "string", description: "the repository owner" }, name: { type: "string", description: "the repository name" }, - confirm: { type: "boolean", description: "must be true to actually delete" }, + confirm: { type: "string", description: "the repository's full name, owner/name, to actually delete it" }, }, run: async (args) => { - if (!args.confirm) return { deleted: false, reason: "confirm must be true to delete a repository" }; - // A repository a protection rule guards is the mesh's: deleted and made again, it would be one without - // its protection, under the name the mesh builds from (novox/hq ADR 0266). - const guarded = await gitea.branchProtections(String(args.owner), String(args.name)); - if (guarded.length) { - throw new Refused(`${String(args.owner)}/${String(args.name)} is guarded by the protection rule ` + - `${guarded.map((r) => r.rule_name ?? r.branch_name).join(", ")}: deleting it is the operator's, at the forge ` + - `as themselves, never a tool's (novox/hq ADR 0266). Nothing was deleted`); - } + const owner = String(args.owner), name = String(args.name), full = `${owner}/${name}`; + const self = (await gitea.api<{ login?: string }>("/user"))?.login ?? ""; + const commits = await gitea.api>( + `/repos/${owner}/${name}/commits?limit=1`).catch(() => []); + const when = commits?.[0]?.commit?.committer?.date ?? commits?.[0]?.created; + const refused = judgeDelete(full, args.confirm, owner, self, await gitea.branchProtections(owner, name), + when ? new Date(when) : null, new Date()); + if (refused) throw new Refused(refused); await gitea.deleteRepo(String(args.owner), String(args.name)); return { deleted: true, repo: `${String(args.owner)}/${String(args.name)}` }; }, @@ -284,6 +286,12 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); const rule = await gitea.branchProtection(owner, repo, pull.base); + // Only into a branch whose protection requires the checks: a pull request whose base was moved to a + // branch nothing guards would merge past every check (novox/hq ADR 0266). + if (!rule || !rule.enable_status_check || !(rule.status_check_contexts ?? []).filter(Boolean).length) { + throw new Refused(`${owner}/${repo}#${number} merges into ${pull.base}, which no protection rule requiring ` + + `the checks guards: a tool merges only into such a branch (novox/hq ADR 0266). Nothing was merged`); + } const statuses = newestByContext((await gitea.combinedStatus(owner, repo, pull.head_sha)).statuses); const verdict = mergeableUnder(pull.base, rule, statuses); if (verdict.mergeable !== true) { @@ -459,7 +467,9 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { push: args.push === undefined ? undefined : Boolean(args.push), blockAdminOverride: args.block_admin_override === undefined ? undefined : Boolean(args.block_admin_override), }; - const weakens = judgeProtectionChange(before, want); + const covering = before ? [] : (await gitea.branchProtections(owner, repo)) + .filter((r) => ruleCovers(String(r.rule_name ?? r.branch_name ?? ""), branch)); + const weakens = judgeProtectionChange(before, want, covering); if (weakens) throw new Refused(weakens); const { created, rule } = await gitea.setBranchProtection(owner, repo, branch, want); return { branch, created, before: before ? summarised(before) : null, after: summarised(rule) }; @@ -633,7 +643,10 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { await gitea.branchProtections(owner, repo)); if (refused) throw new Refused(refused); } - const result = await gitea.api(String(args.path), { + // The path judged is the path sent: normalised, its query kept. + const raw = String(args.path ?? ""); + const query = raw.includes("?") ? raw.slice(raw.indexOf("?")).split("#")[0] : ""; + const result = await gitea.api(routedPath(raw) + query, { method, ...(args.body ? { body: JSON.stringify(args.body) } : {}), }); From ea772a9cce2ea258d1f218b4ca4a2132ae486d28 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 12:57:16 +0200 Subject: [PATCH 3/5] Let gitea_api send only the query keys it needs, never sudo, and never approve a pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirmation review of 2026-10-09 measured GET /user?sudo=… answer the operator's own account: the query went to the forge unjudged, and the tools' token is the site administrator's, so any caller could act as any user. gitea_api now sends only allow-listed query keys and refuses sudo in any case, and a review that approves or requests changes is the operator's. --- modules/gitea/guard.ts | 43 ++++++++++++++++++++++++++++++++ modules/gitea/test/guard.test.ts | 29 ++++++++++++++++++++- modules/gitea/tools/index.ts | 7 +++--- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/modules/gitea/guard.ts b/modules/gitea/guard.ts index 7fa757f3..4b451f64 100644 --- a/modules/gitea/guard.ts +++ b/modules/gitea/guard.ts @@ -83,12 +83,45 @@ export function routedPath(raw: string): string { return path; } +/** + * The query keys `gitea_api` sends: what the forge's listing and reading endpoints take. Any other key is refused, + * `sudo` above all — the forge lets a site administrator's token act as any user named by it, and the tools' token is + * one (the confirmation review of 2026-10-09 read the operator's own account through `GET /user?sudo=…`). The token + * keys are refused too: authentication is the client's, never the caller's. + */ +export const QUERY_KEYS: ReadonlySet = new Set([ + "limit", "page", "state", "type", "q", "sort", "order", "since", "before", "ref", "sha", "path", "labels", + "milestones", "keyword", "stat", "verification", "files", "not", "mode", "include_desc", "archived", "private", + "exclusive", "uid", "priority_repo_id", "created_by", "assigned_by", "mentioned_by", "whitespace", "binary", +]); + +/** The query a call sends: only allow-listed keys, each as given; refused when any other key is there. */ +export function sentQuery(raw: string): string { + const at = String(raw ?? "").indexOf("?"); + if (at < 0) return ""; + const query = String(raw).slice(at + 1).split("#")[0]; + const params = new URLSearchParams(query); + const kept = new URLSearchParams(); + for (const [key, value] of params) { + if (key.toLowerCase() === "sudo") { + throw new Refused(`the query key ${key} is refused: it would act as another user of the forge (novox/hq ADR 0266). Nothing was sent`); + } + if (!QUERY_KEYS.has(key)) { + throw new Refused(`the query key ${key} is not one gitea_api sends: it sends ${[...QUERY_KEYS].join(", ")} (novox/hq ADR 0266). Nothing was sent`); + } + kept.append(key, value); + } + const out = kept.toString(); + return out ? "?" + out : ""; +} + /** What `gitea_api` may do with this call, judged on the call alone (novox/hq ADR 0266). */ export function judgeApiCall(method: string, rawPath: string, body: unknown): ApiJudgement { const m = String(method || "GET").toUpperCase(); let path: string; try { path = routedPath(rawPath); + sentQuery(rawPath); } catch (err) { return { refused: (err as Error).message }; } @@ -110,6 +143,16 @@ export function judgeApiCall(method: string, rawPath: string, body: unknown): Ap "base" in (body as Record)) { return { refused: `${m} ${path} changing a pull request's base is refused (novox/hq ADR 0266): its checks were judged against the base it had. Open a new pull request. Nothing was sent` }; } + // An approval, or a request for changes, is a reviewer's word on a merge: the operator's, never a tool's — the + // tools' token is the forge's administrator, and its approval would count as anyone's (novox/hq ADR 0266). + if (m === "POST" && /^\/repos\/[^/]+\/[^/]+\/pulls\/\d+\/reviews(\/\d+)?$/.test(path)) { + const event = String((body as Record | null)?.event ?? "").trim().toUpperCase(); + if (event !== "" && event !== "COMMENT" && event !== "PENDING") { + return { refused: `${m} ${path} with the event ${event} is refused: approving a pull request, or requesting ` + + `changes on it, is the operator's at the forge as themselves, never a tool's (novox/hq ADR 0266). A review ` + + `that only comments is sent. Nothing was sent` }; + } + } if (AUTHOR_WRITES.some((w) => w.methods.includes(m) && w.path.test(path))) return { allowed: true }; return { refused: `${m} ${path} is not a write gitea_api makes (novox/hq ADR 0266): it writes only what an author ` + diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 02da7e17..3b7d1f7d 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -17,6 +17,7 @@ interface Sent { method: string; path: string; body: any } const state = { sent: [] as Sent[], + asked: [] as string[], statuses: [] as { context: string; state: string }[], rules: [] as any[], lastCommit: "2026-01-01T00:00:00Z", @@ -26,6 +27,7 @@ const protectedMain = () => ({ rule_name: "main", enable_push: false, enable_sta beforeEach(() => { state.sent = []; + state.asked = []; state.statuses = [{ context: "mesh/merge-gate", state: "success" }, { context: "mesh/repo-check", state: "success" }]; state.rules = [protectedMain()]; }); @@ -46,6 +48,7 @@ const server = createServer(async (req, res) => { const path = url.pathname.replace(/^\/api\/v1/, ""); const method = req.method ?? "GET"; const body = await read(req); + state.asked.push(`${method} ${path}${url.search}`); if (method !== "GET") state.sent.push({ method, path, body }); const repo = "/repos/novox/mesh-catalog"; if (method === "GET" && path === repo) return json(res, 200, { full_name: "novox/mesh-catalog", default_branch: "main" }); @@ -159,7 +162,7 @@ test("gitea_delete_repo deletes only the tools' own user's repository, unguarded }); test("gitea_api sends the path it judged, and refuses moving a pull request's base", async () => { - await tool("gitea_api").run({ path: "/api/v1/repos/novox/mesh-catalog/issues/3/comments/?x=1", method: "POST", body: { body: "hi" } }); + await tool("gitea_api").run({ path: "/api/v1/repos/novox/mesh-catalog/issues/3/comments/?page=1", method: "POST", body: { body: "hi" } }); assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/issues/3/comments"]); await assert.rejects(tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7", method: "PATCH", body: { base: "unguarded" } }), /changing a pull request's base/); @@ -206,3 +209,27 @@ test("the judges, without a forge", () => { assert.equal(judgeProtectionChange(null, { statusChecks: [] }), null); assert.equal(judgeProtectionChange(protectedMain(), { statusChecks: ["mesh/repo-check", "mesh/merge-gate"] }), null); }); + +test("gitea_api never acts as another user: sudo, in any case, and any query key it does not send are refused", async () => { + for (const path of ["/user?sudo=jschoubben", "/user?SUDO=jschoubben", "/repos/novox/mesh-catalog/issues?state=open&Sudo=jschoubben", + "/repos/novox/mesh-catalog/issues?token=abc", "/repos/novox/mesh-catalog/issues?access_token=abc"]) { + await assert.rejects(tool("gitea_api").run({ path }), /query key/, path); + } + await assert.rejects(tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/issues/3/comments?sudo=jschoubben", + method: "POST", body: { body: "as the operator" } }), /query key sudo/); + assert.deepEqual(state.asked, []); + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/issues?state=open&limit=5" }); + assert.deepEqual(state.asked, ["GET /repos/novox/mesh-catalog/issues?state=open&limit=5"]); +}); + +test("gitea_api never approves a pull request nor requests changes: those are the operator's", async () => { + for (const [path, event] of [["/repos/novox/mesh-catalog/pulls/7/reviews", "APPROVE"], + ["/repos/novox/mesh-catalog/pulls/7/reviews", "approve"], ["/repos/novox/mesh-catalog/pulls/7/reviews", "REQUEST_CHANGES"], + ["/repos/novox/mesh-catalog/pulls/7/reviews/5", "APPROVE"]]) { + await assert.rejects(tool("gitea_api").run({ path, method: "POST", body: { event, body: "lgtm" } }), + /operator's at the forge/, `${path} ${event}`); + } + assert.deepEqual(writes(), []); + await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7/reviews", method: "POST", body: { event: "COMMENT", body: "a note" } }); + assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/pulls/7/reviews"]); +}); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 91357c7f..7f96734d 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -14,7 +14,7 @@ import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, deliveryStatus, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; import { - MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, routedPath, + MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, routedPath, sentQuery, ruleCovers, } from "../guard.js"; @@ -643,10 +643,9 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { await gitea.branchProtections(owner, repo)); if (refused) throw new Refused(refused); } - // The path judged is the path sent: normalised, its query kept. + // The path judged is the path sent: normalised, with only the allow-listed query keys (guard.ts). const raw = String(args.path ?? ""); - const query = raw.includes("?") ? raw.slice(raw.indexOf("?")).split("#")[0] : ""; - const result = await gitea.api(routedPath(raw) + query, { + const result = await gitea.api(routedPath(raw) + sentQuery(raw), { method, ...(args.body ? { body: JSON.stringify(args.body) } : {}), }); From 5eab586116e3208492f35b9e89a6a906ad022d22 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 13:35:15 +0200 Subject: [PATCH 4/5] Put no caller's text in a forge path unjudged: every segment checked, every sent path checked again The confirmation review of 2026-10-09 found the dedicated tools building paths from caller text: a branch named .. became DELETE /repos/o/r/ (the repository), a file path ../../../../version reached /api/v1/version, and a sha with ?sudo= set a status as any user. Each caller-supplied segment now goes through one helper refusing empty, . and .. and / ? # %; a sha must be a commit; and the client judges every path once more as it sends it: no dot or empty segment, encoded or not, and only the allow-listed query keys. --- modules/gitea/client.ts | 26 +++++++++++++------ modules/gitea/guard.ts | 43 ++++++++++++++++++++++++++++++++ modules/gitea/test/guard.test.ts | 32 ++++++++++++++++++++++++ modules/gitea/test/pulls.test.ts | 9 ++++--- modules/gitea/tools/index.ts | 4 ++- 5 files changed, 102 insertions(+), 12 deletions(-) diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 368dbc96..a3a7506f 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -6,6 +6,15 @@ import { readFileSync } from "node:fs"; import { ConfiguredToken, MintedToken, type TokenSource } from "./token.js"; import { protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; +import { Refused, checkSentPath, pathSegment, pathSegments } from "./guard.js"; +import { isCommit } from "./checks.js"; + +/** A commit, by its sha or the start of one, as it goes in a path: anything else is refused before it is sent. */ +function commitSegment(sha: unknown): string { + const v = String(sha ?? ""); + if (!isCommit(v)) throw new Refused(`${JSON.stringify(sha)} is not a commit. Nothing was sent`); + return v; +} export type { BranchProtection, ProtectionWanted } from "./protection.js"; /** A repository, trimmed to what the mesh cares about. */ @@ -135,6 +144,7 @@ export class GiteaClient { } private send(path: string, options: RequestInit, token: string): Promise { + checkSentPath(path); return fetch(`${this.baseUrl}/api/v1${path}`, { ...options, headers: { @@ -337,12 +347,12 @@ export class GiteaClient { /** Set a commit's status — what a pull request whose head it is shows beside it (novox/hq to-be 45 §9). * The forge keeps one per context, the newest, so setting it again replaces it. */ async setCommitStatus(owner: string, repo: string, sha: string, status: CommitStatus): Promise { - await this.request(`/repos/${owner}/${repo}/statuses/${sha}`, { method: "POST", body: JSON.stringify(status) }); + await this.request(`/repos/${owner}/${repo}/statuses/${commitSegment(sha)}`, { method: "POST", body: JSON.stringify(status) }); } /** A commit's statuses by context, the newest of each: what a merge's head was checked as (novox/hq ADR 0239). */ async commitStatuses(owner: string, repo: string, sha: string): Promise> { - const raw = await this.request(`/repos/${owner}/${repo}/commits/${sha}/status`); + const raw = await this.request(`/repos/${owner}/${repo}/commits/${commitSegment(sha)}/status`); const out: Record = {}; for (const s of raw?.statuses ?? []) if (s?.context && !out[s.context]) out[s.context] = String(s.status ?? s.state ?? ""); return out; @@ -351,14 +361,14 @@ export class GiteaClient { /** A commit's statuses whole, as the forge combines them for a ref (a sha, or the start of one): the commit * it resolved to and every status with its description, link, times and setter (novox/hq ADR 0239). */ async combinedStatus(owner: string, repo: string, ref: string): Promise<{ sha: string; state: string; statuses: unknown[] }> { - const raw = await this.request(`/repos/${owner}/${repo}/commits/${encodeURIComponent(ref)}/status?limit=50`); + const raw = await this.request(`/repos/${owner}/${repo}/commits/${pathSegment("commit or branch", ref)}/status?limit=50`); return { sha: String(raw?.sha ?? ""), state: String(raw?.state ?? ""), statuses: Array.isArray(raw?.statuses) ? raw.statuses : [] }; } /** The forge's compare of two commits, `base...head`: the commits on head that base lacks, and how many * (novox/hq ADR 0270). Reads only. */ async compare(owner: string, repo: string, base: string, head: string): Promise { - return this.request(`/repos/${owner}/${repo}/compare/${encodeURIComponent(base)}...${encodeURIComponent(head)}`); + return this.request(`/repos/${owner}/${repo}/compare/${commitSegment(base)}...${commitSegment(head)}`); } /** Replace a comment's body: the delivery's view, kept current in place (novox/hq ADR 0239). */ @@ -386,7 +396,7 @@ export class GiteaClient { const had = await this.branchProtection(owner, repo, branch); const body = protectionBody(want, !had); if (had) { - const rule = await this.request(`/repos/${owner}/${repo}/branch_protections/${encodeURIComponent(branch)}`, + const rule = await this.request(`/repos/${owner}/${repo}/branch_protections/${pathSegments("branch", branch)}`, { method: "PATCH", body: JSON.stringify(body) }); return { created: false, rule }; } @@ -440,7 +450,7 @@ export class GiteaClient { /** One file's contents at a ref (default the repository's default branch), decoded. */ async getFile(owner: string, repo: string, path: string, ref?: string): Promise<{ path: string; ref?: string; sha: string; size: number; content: string }> { const qs = ref ? `?ref=${encodeURIComponent(ref)}` : ""; - const f = await this.request(`/repos/${owner}/${repo}/contents/${path.split("/").map(encodeURIComponent).join("/")}${qs}`); + const f = await this.request(`/repos/${owner}/${repo}/contents/${pathSegments("file path", path.replace(/^\/+|\/+$/g, ""))}${qs}`); if (!f || f.type !== "file") throw new Error(`Gitea API: ${path} is not a file`); const content = f.encoding === "base64" ? Buffer.from(String(f.content ?? ""), "base64").toString("utf8") : String(f.content ?? ""); return { path, ref, sha: String(f.sha ?? ""), size: Number(f.size ?? content.length), content }; @@ -452,7 +462,7 @@ export class GiteaClient { } async deleteBranch(owner: string, repo: string, branch: string): Promise { - await this.request(`/repos/${owner}/${repo}/branches/${encodeURIComponent(branch)}`, { method: "DELETE" }); + await this.request(`/repos/${owner}/${repo}/branches/${pathSegments("branch", branch)}`, { method: "DELETE" }); } /** A request whose answer is text, not JSON — a diff. Same token handling as request(). */ @@ -756,5 +766,5 @@ export function directoriesAbove(paths: string[]): string[] { /** A repository path for the forge's URL: each segment escaped, the slashes kept. */ function encodePath(path: string): string { - return path.split("/").map(encodeURIComponent).join("/"); + return pathSegments("file path", path); } diff --git a/modules/gitea/guard.ts b/modules/gitea/guard.ts index 4b451f64..b0d68b0f 100644 --- a/modules/gitea/guard.ts +++ b/modules/gitea/guard.ts @@ -271,3 +271,46 @@ export function checkName(what: string, value: unknown): string { } return v; } + +/** + * One caller-supplied path segment, encoded: refused when empty, `.` or `..`, or holding `/`, `?`, `#` or `%` — any + * of which, encoded or not, could make the request another path or carry another query (the confirmation review of + * 2026-10-09: a branch named `..` became DELETE /repos/o/r/, the repository itself). Every value a caller gives that + * lands in a path goes through this, or through pathSegments. + */ +export function pathSegment(what: string, value: unknown): string { + const v = String(value ?? ""); + if (v === "" || v === "." || v === ".." || /[/?#%\\]/.test(v) || /[\u0000-\u001f]/.test(v)) { + throw new Refused(`${JSON.stringify(value)} is not a ${what} this tool puts in a path. Nothing was sent`); + } + return encodeURIComponent(v); +} + +/** A caller-supplied value that is several segments — a branch like feat/x, a file's path — each judged as one. */ +export function pathSegments(what: string, value: unknown): string { + return String(value ?? "").split("/").map((part) => pathSegment(what, part)).join("/"); +} + +/** + * Every path the client sends, judged once more where it is sent: no empty, `.` or `..` segment, encoded or not, and + * only the query keys the tools send (sentQuery). What a caller's text could still make of a path built elsewhere + * stops here, before the forge. + */ +export function checkSentPath(path: string): void { + const at = path.search(/[?#]/); + const route = at < 0 ? path : path.slice(0, at); + if (path.includes("#")) throw new Refused(`${path}: a path with a fragment is not sent`); + const segments = route.replace(/^\/+/, "").split("/"); + for (const s of segments) { + let decoded: string; + try { + decoded = decodeURIComponent(s); + } catch { + throw new Refused(`${path}: a segment that does not decode is not sent`); + } + if (decoded === "" || decoded === "." || decoded === ".." || decoded.includes("?")) { + throw new Refused(`${path}: an empty or dot segment is not sent. Nothing was sent`); + } + } + if (at >= 0) sentQuery(path.slice(at)); +} diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 3b7d1f7d..423c83c1 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -233,3 +233,35 @@ test("gitea_api never approves a pull request nor requests changes: those are th await tool("gitea_api").run({ path: "/repos/novox/mesh-catalog/pulls/7/reviews", method: "POST", body: { event: "COMMENT", body: "a note" } }); assert.deepEqual(writes(), ["POST /repos/novox/mesh-catalog/pulls/7/reviews"]); }); + +test("no tool's caller text becomes another path or another user: `..` and `?sudo=` are refused before the forge", async () => { + const repo = { owner: "novox", repo: "mesh-catalog" }; + const cases: Array<[string, Record]> = [ + ["gitea_delete_branch", { ...repo, branch: ".." }], + ["gitea_delete_branch", { ...repo, branch: "feat/../.." }], + ["gitea_delete_branch", { ...repo, branch: "x?sudo=jschoubben" }], + ["gitea_delete_branch", { ...repo, branch: "%2e%2e" }], + ["gitea_get_file", { ...repo, path: "../../../../version" }], + ["gitea_get_file", { ...repo, path: "README.md?sudo=jschoubben" }], + ["gitea_commit_status", { ...repo, sha: "abc1234?sudo=jschoubben", context: "mesh/delivery", state: "success" }], + ["gitea_commit_status", { ...repo, sha: "../../../admin/users", context: "mesh/delivery", state: "success" }], + ["gitea_commit_statuses", { ...repo, sha: ".." }], + ["gitea_commit_statuses", { ...repo, sha: "main?sudo=jschoubben" }], + ["gitea_branch_protection_set", { ...repo, branch: "..", status_checks: "mesh/merge-gate" }], + ["gitea_branch_protection_set", { ...repo, branch: "main?sudo=jschoubben", status_checks: "mesh/merge-gate" }], + ["gitea_contains", { ...repo, commit: "..", in: "abc1234" }], + ]; + for (const [name, args] of cases) { + state.asked = []; + await assert.rejects(tool(name).run(args), (err: unknown) => err instanceof Error, `${name} ${JSON.stringify(args)}`); + // Nothing that is not a read of the repository's own rules reached the forge. + assert.deepEqual(state.asked.filter((a) => !a.startsWith("GET /repos/novox/mesh-catalog/branch_protections")), [], + `${name} ${JSON.stringify(args)} reached the forge`); + } + // What is ordinary still goes: a branch with a slash, a file in a directory. + state.asked = []; + await tool("gitea_delete_branch").run({ ...repo, branch: "feat/x" }); + await tool("gitea_get_file").run({ ...repo, path: "modules/gitea/module.json" }).catch(() => undefined); + assert.deepEqual(state.asked, ["DELETE /repos/novox/mesh-catalog/branches/feat/x", + "GET /repos/novox/mesh-catalog/contents/modules/gitea/module.json"]); +}); diff --git a/modules/gitea/test/pulls.test.ts b/modules/gitea/test/pulls.test.ts index 91736eb2..8e228a85 100644 --- a/modules/gitea/test/pulls.test.ts +++ b/modules/gitea/test/pulls.test.ts @@ -119,9 +119,12 @@ test("a commit status is set on the commit, under the merge check's context", as await new Promise((r) => server.listen(0, r)); const port = (server.address() as any).port; const client = new GiteaClient(`http://127.0.0.1:${port}`, "t"); - await client.setCommitStatus("novox", "mesh-catalog", "abc123", { state: "failure", context: "mesh/merge-gate", description: "x" }); - server.close(); - assert.equal(seen!.path, "POST /api/v1/repos/novox/mesh-catalog/statuses/abc123"); + try { + await client.setCommitStatus("novox", "mesh-catalog", "abc1234", { state: "failure", context: "mesh/merge-gate", description: "x" }); + } finally { + server.close(); + } + assert.equal(seen!.path, "POST /api/v1/repos/novox/mesh-catalog/statuses/abc1234"); assert.equal(seen!.body.state, "failure"); assert.equal(seen!.body.context, "mesh/merge-gate"); }); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 7f96734d..0f4ea5a0 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -14,7 +14,7 @@ import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, deliveryStatus, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; import { - MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, routedPath, sentQuery, + MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, pathSegments, routedPath, sentQuery, ruleCovers, } from "../guard.js"; @@ -461,6 +461,8 @@ function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { run: async (args) => { const owner = String(args.owner), repo = String(args.repo), branch = String(args.branch ?? "").trim(); if (!branch) throw new Error("name the branch to protect"); + pathSegments("branch", branch); // a rule's name is a branch's, never a path's + const before = await gitea.branchProtection(owner, repo, branch); const want = { statusChecks: String(args.status_checks ?? "").split(","), From 1dca181e03e1ef086be9f99c1dc3ecb725fd021f Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 13:41:35 +0200 Subject: [PATCH 5/5] Put every repository path through the segment helper, gitea_open_pulls's included After merging #159 (issue 347): every /repos// the client builds now takes its owner and name through one helper, whether they came from a caller or from the forge's answer; the open-pulls fixture's shas are commits. --- modules/gitea/test/announcing.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/gitea/test/announcing.test.ts b/modules/gitea/test/announcing.test.ts index ac607bb8..a1299d7e 100644 --- a/modules/gitea/test/announcing.test.ts +++ b/modules/gitea/test/announcing.test.ts @@ -42,10 +42,10 @@ test("every open pull request is read with what its base requires and what its h ] : []); } if (url.startsWith("/api/v1/repos/novox/hq/pulls/243")) { - return send({ number: 243, state: "open", base: { ref: "main" }, head: { ref: "x", sha: "aaa111" }, html_url: "h" }); + return send({ number: 243, state: "open", base: { ref: "main" }, head: { ref: "x", sha: "aaa1111" }, html_url: "h" }); } if (url.startsWith("/api/v1/repos/novox/mesh-sdk/pulls/14")) { - return send({ number: 14, state: "open", base: { ref: "main" }, head: { ref: "y", sha: "bbb222" }, html_url: "s" }); + return send({ number: 14, state: "open", base: { ref: "main" }, head: { ref: "y", sha: "bbb2222" }, html_url: "s" }); } if (url.startsWith("/api/v1/repos/novox/hq/branch_protections")) { return send([{ rule_name: "main", enable_status_check: true, status_check_contexts: ["mesh/repo-check"] }]); @@ -53,8 +53,8 @@ test("every open pull request is read with what its base requires and what its h if (url.startsWith("/api/v1/repos/novox/mesh-sdk/branch_protections")) { return send([{ rule_name: "main", enable_status_check: true, status_check_contexts: ["mesh/merge-gate", "mesh/repo-check"] }]); } - if (url.includes("/commits/aaa111/status")) return send({ statuses: [] }); - if (url.includes("/commits/bbb222/status")) return send({ statuses: [{ context: "mesh/merge-gate", status: "pending" }] }); + if (url.includes("/commits/aaa1111/status")) return send({ statuses: [] }); + if (url.includes("/commits/bbb2222/status")) return send({ statuses: [{ context: "mesh/merge-gate", status: "pending" }] }); res.statusCode = 404; res.end("{}"); }); @@ -63,8 +63,8 @@ test("every open pull request is read with what its base requires and what its h try { const open = await client.openPulls(); assert.deepEqual(open.map((p) => [p.owner, p.repo, p.number, p.base, p.head_sha, p.required, p.statuses]), [ - ["novox", "hq", 243, "main", "aaa111", ["mesh/repo-check"], []], - ["novox", "mesh-sdk", 14, "main", "bbb222", ["mesh/merge-gate", "mesh/repo-check"], ["mesh/merge-gate"]], + ["novox", "hq", 243, "main", "aaa1111", ["mesh/repo-check"], []], + ["novox", "mesh-sdk", 14, "main", "bbb2222", ["mesh/merge-gate", "mesh/repo-check"], ["mesh/merge-gate"]], ]); } finally { server.close();