From 5eab586116e3208492f35b9e89a6a906ad022d22 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 13:35:15 +0200 Subject: [PATCH] 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(","),