diff --git a/server/src/__tests__/github-egress-outbound-coverage.test.ts b/server/src/__tests__/github-egress-outbound-coverage.test.ts index e83fc6780a66..201604700c7c 100644 --- a/server/src/__tests__/github-egress-outbound-coverage.test.ts +++ b/server/src/__tests__/github-egress-outbound-coverage.test.ts @@ -3,6 +3,12 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; +import { + productionFilesImportingTestHelpers, + serverFilesWritingToGitHub, + serverSourceFiles, +} from "./helpers/github-writer-derivation.js"; + /** * PEN-3152 done-when 2: "the outbound direction gets its own exhaustive * coverage assertion, in the shape of `mcp-seed-scrub-coverage.test.ts` — so a @@ -136,21 +142,29 @@ const WRAPPER_COVERAGE: Readonly> = { * `github-write-egress-scrub.test.ts`, not here. */ const SERVER_WRITE_COVERAGE: Readonly> = { - // githubPostCommitStatusDetailed (context, description, target_url), - // githubPostIssueComment (body) and githubPostCheckRun (name, title, summary, + // githubPostCommitStatusDetailed (description, target_url), + // githubPostIssueComment (body) and githubPostCheckRun (title, summary, // details_url) each scrub inside the helper, so every present and future // caller inherits it. The installation-token POST in this file carries a JWT // and no authored text, and is not scrubbed. + // + // The two IDENTITY fields — a status `context` and a check-run `name` — are + // deliberately not on that list. They are refused rather than redacted + // (`gitHubIdentityFieldRedaction`, PEN-3391): a redacted name addresses the + // status to something no lookup and no branch-protection rule can match, so + // scrubbing them would trade a leak for a silent gate-liveness failure. The + // refusal keeps the leak closed without that trade. "services/github-app-auth.ts": { kind: "egress-scrubbed", runtime: SERVER_EGRESS_SCRUB, }, // Builds its own requests — a caller-supplied token and an abort signal the // shared helper does not model — so it calls the scrub directly on the - // pending status's context, description and target_url. Its - // repository_dispatch client_payload is ids only (app, installation, - // delivery, PR number, head SHA) and is not scrubbed: it carries no authored - // text, and the detectors are tuned for prose, not protocol. + // pending status's description and target_url, and applies the same + // identity-field refusal to its context. Its repository_dispatch + // client_payload is ids only (app, installation, delivery, PR number, head + // SHA) and is not scrubbed: it carries no authored text, and the detectors + // are tuned for prose, not protocol. "services/github-review-gate-authority.ts": { kind: "egress-scrubbed", runtime: SERVER_EGRESS_SCRUB, @@ -195,10 +209,15 @@ function readSeededGitHubMcpCommand(): string { } /** - * Every non-test TypeScript file under `server/src`, as a path relative to it - * with forward slashes ("services/github-app-auth.ts"). + * Every non-test TypeScript file under `server/src`, and the subset of them + * this scan cannot prove is read-only. + * + * Both derivations now live in `helpers/github-writer-derivation.ts` and are + * shared with `github-write-egress-scrub.test.ts`. They used to be duplicated + * byte-for-byte here, which is how the walk came to be widened by hand in two + * places at once on #1754 — one copy away from diverging (PEN-3391). * - * Recursive, and that is the load-bearing part. This walked + * The walk is recursive, and that is the load-bearing part. It listed * `server/src/services` one level deep until Ally caught the scope on #1754: * `server/src/routes/` (which holds `github-webhook.ts`) and * `server/src/services/recovery/` were both invisible to it, so a new @@ -210,24 +229,19 @@ function readSeededGitHubMcpCommand(): string { * That is the third repeat of one shape: PEN-2527 enumerated `gh` and missed * the MCP server, PEN-3152 enumerated both wrappers and missed `server/`, and * this enumerated `services/` and missed its own siblings. Each time the - * derivation was correct over a set that was quietly too small. + * derivation was correct over a set that was quietly too small. PEN-3391 is the + * fourth, one level further down: the predicate itself enumerated a single + * spelling of a write (`ghFetch(` plus an inline double-quoted upper-case + * method) and missed aliased calls, quoted variants and non-literal methods. It + * is now fail-closed — see the helper for what that costs and what still + * escapes it. */ -function serverSourceFiles(): string[] { - const { readdirSync } = require("node:fs") as typeof import("node:fs"); - return readdirSync(serverSourceDirectory, { recursive: true, encoding: "utf8" }) - .map((entry) => entry.split(path.sep).join("/")) - .filter((entry) => entry.endsWith(".ts") && !entry.endsWith(".test.ts")) - .sort(); +function scannedServerSourceFiles(): string[] { + return serverSourceFiles(serverSourceDirectory); } -function serverFilesWritingToGitHub(): string[] { - return serverSourceFiles() - .filter((entry) => { - const source = readFileSync(path.join(serverSourceDirectory, entry), "utf8"); - if (!source.includes("ghFetch(")) return false; - return /method:\s*"(?:POST|PATCH|PUT|DELETE)"/.test(source); - }) - .sort(); +function scannedServerFilesWritingToGitHub(): string[] { + return serverFilesWritingToGitHub(serverSourceDirectory); } describe("outbound GitHub egress coverage", () => { @@ -293,7 +307,9 @@ describe("outbound GitHub egress coverage", () => { // paperclip-api reaches GitHub over HTTP from server/, touching no // wrapper. A new file that starts writing fails here until it is // classified — which is the whole mechanism PEN-3152 asked for. - expect(serverFilesWritingToGitHub()).toEqual(Object.keys(SERVER_WRITE_COVERAGE).sort()); + expect(scannedServerFilesWritingToGitHub()).toEqual( + Object.keys(SERVER_WRITE_COVERAGE).sort(), + ); }); it("derives that set from a walk that actually descends below server/src", () => { @@ -308,12 +324,21 @@ describe("outbound GitHub egress coverage", () => { // `routes/github-webhook.ts` is the specific file Ally named on #1754: // it imports `githubPostIssueComment` and so is one edit away from being // a direct writer itself. - const scanned = serverSourceFiles(); + const scanned = scannedServerSourceFiles(); expect(scanned).toContain("routes/github-webhook.ts"); expect( scanned.filter((entry) => entry.startsWith("services/recovery/")), "services/recovery/ is no longer reachable from the walk", ).not.toHaveLength(0); + + // The walk skips `__tests__/` so the shared derivation helper cannot + // classify itself as an unscrubbed writer by quoting its own regexes. + // That exclusion only excludes TEST code while nothing in the running + // server imports from there — checked, not assumed (PEN-3391). + expect( + productionFilesImportingTestHelpers(serverSourceDirectory), + "a production file imports from __tests__/, so skipping it no longer excludes only test code", + ).toEqual([]); }); it("the scrubber is reachable from server/, and the server wrapper still delegates to it", () => { diff --git a/server/src/__tests__/github-write-egress-scrub.test.ts b/server/src/__tests__/github-write-egress-scrub.test.ts index ea56e1f20695..5aca3ecb0f2a 100644 --- a/server/src/__tests__/github-write-egress-scrub.test.ts +++ b/server/src/__tests__/github-write-egress-scrub.test.ts @@ -1,9 +1,15 @@ -import { readdirSync, readFileSync } from "node:fs"; +import { readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { generateKeyPairSync } from "node:crypto"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + productionFilesImportingTestHelpers, + serverFilesWritingToGitHub, + serverSourceFiles, +} from "./helpers/github-writer-derivation.js"; + /** * PEN-3157: the egress scrub must be on the path of every server-side GitHub * write that can carry authored text. @@ -328,6 +334,148 @@ describe("githubPostCheckRun egress scrub", () => { }); }); +describe("identity fields are refused, not redacted (PEN-3391)", () => { + /** + * PEN-3391 done-when 3/4: `context` was scrubbed on the way out while the + * delivery outbox keyed its upsert on the RAW value and + * `githubGetLatestCommitStatusForContext` filtered on the RAW value. Had a + * context ever matched a detector, the status would have been published under + * a redacted name while every lookup used the unredacted one — the gate + * unable to observe its own status. + * + * The resolution is neither "scrub everywhere" nor "exempt it": the write is + * REFUSED when the identity would change. That keeps the leak closed AND + * makes publish/lookup agreement structural — the write proceeds only when + * the scrub is a no-op, so a caller keying on the raw value is provably + * keying on what was published. + * + * These tests exist so a later "make the scrub consistent" refactor cannot + * silently flip it back. The row asked for exactly that pin. + */ + it("refuses a commit status whose context carries credential-shaped material", async () => { + setCreds(); + const fetchMock = stubGitHub(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const result = await githubPostCommitStatusDetailed({ + repoFullName: REPO, + sha: SHA, + context: `review/${FAKE_GITHUB_PAT}`, + state: "success", + description: "ok", + }); + + expect(result).toEqual({ + ok: false, + retryable: false, + reason: "commit_status_context_not_publishable", + }); + // Nothing was published — not a redacted status, nothing at all. + expect( + fetchMock.mock.calls.filter(([url]) => String(url).includes("/statuses/")), + "a status was published despite the refusal", + ).toHaveLength(0); + // Non-retryable is load-bearing: the same input scrubs the same way every + // time, so a retrying caller would spin forever on a configuration bug. + expect(errorSpy).toHaveBeenCalled(); + const logged = String(errorSpy.mock.calls[0]?.[0] ?? ""); + expect(logged).toContain("REFUSED"); + // The log must name the classes, never the matched text — quoting it would + // re-publish the secret into the transcripts PEN-3139 is narrowing. + expect(logged).not.toContain(FAKE_GITHUB_PAT); + }); + + it("refuses a check-run whose name carries credential-shaped material", async () => { + setCreds(); + const fetchMock = stubGitHub(); + vi.spyOn(console, "error").mockImplementation(() => {}); + + const result = await githubPostCheckRun({ + repoFullName: REPO, + sha: SHA, + name: `verify-${FAKE_AWS_KEY_ID}`, + conclusion: "success", + title: "t", + summary: "s", + }); + + expect(result).toEqual({ + ok: false, + retryable: false, + reason: "check_run_name_not_publishable", + }); + expect( + fetchMock.mock.calls.filter(([url]) => String(url).includes("/check-runs")), + ).toHaveLength(0); + }); + + it("publishes an ordinary context and name byte-for-byte, so lookups still match", async () => { + // The other half of the pin, and the one that makes the refusal safe to + // ship: the overwhelmingly common path must be untouched. A required + // context is matched by exact string, so any rewriting here — including a + // well-meaning normalisation — breaks branch protection silently. + setCreds(); + const fetchMock = stubGitHub(); + + await expect( + githubPostCommitStatusDetailed({ + repoFullName: REPO, + sha: SHA, + context: "review/ally-complete", + state: "success", + description: "Ally reviewed this head, clean.", + }), + ).resolves.toEqual({ ok: true, statusCode: 201 }); + + expect(writtenBody(fetchMock).context).toBe("review/ally-complete"); + }); + + it("keeps the outbox key and the lookup filter equal to what was published", () => { + // The disagreement PEN-3391 names is between three sites, so pin the + // invariant that ties them rather than restating one of them: the outbox + // persists `input.context` verbatim and the lookup filters on + // `input.context`, and both are now correct precisely because the write + // helper publishes `input.context` unchanged or not at all. + const appAuth = readFileSync( + path.join(repoRoot, "server/src/services/github-app-auth.ts"), + "utf8", + ); + const statusHelper = appAuth.slice( + appAuth.indexOf("export async function githubPostCommitStatusDetailed"), + ); + // Refused, not redacted. If someone reintroduces the scrub here, the outbox + // key and the published context diverge again — and this fails. + expect(statusHelper).not.toContain('scrubOutboundGitHubText(input.context'); + expect(statusHelper).toContain('gitHubIdentityFieldRedaction(input.context'); + + const lookupHelper = appAuth.slice( + appAuth.indexOf("export async function githubGetLatestCommitStatusForContext"), + ); + expect(lookupHelper).toContain("status.context === input.context"); + + const outbox = readFileSync( + path.join(repoRoot, "server/src/services/github-status-delivery-outbox.ts"), + "utf8", + ); + expect(outbox).toContain("context: input.context"); + }); + + it("applies the same refusal to the one writer outside the shared helper", () => { + // github-review-gate-authority.ts builds its own request, so it is the + // place a per-call-site control goes stale. It scrubs its prose fields and + // must refuse on its identity field, exactly like the shared helper. + const authority = readFileSync( + path.join(repoRoot, "server/src/services/github-review-gate-authority.ts"), + "utf8", + ); + expect(authority).toContain("gitHubIdentityFieldRedaction(input.row.statusContext"); + expect(authority).not.toContain("scrubOutboundGitHubText(input.row.statusContext"); + // Its prose fields stay scrubbed — the refusal narrows nothing. + expect(authority).toContain('"commit-status description"'); + expect(authority).toContain('"commit-status target_url"'); + }); +}); + describe("the scrub is reachable from server/ at all", () => { it("is exported from the adapter-utils barrel", () => { // The mechanical fact PEN-3157 turned on: `server/` imports the package by @@ -358,9 +506,10 @@ describe("the scrub is reachable from server/ at all", () => { it("leaves no server-side GitHub writer outside the scrub", () => { // Derived from source at file granularity, the same way PEN-3152's outbound - // coverage table derives its writer set: a `ghFetch(` call plus a mutating - // method. A NEW file that starts writing to GitHub fails here until it - // either routes through the shared helpers or calls the scrub itself. + // coverage table derives its writer set — and now from the SAME function, in + // `helpers/github-writer-derivation.ts`. The two copies were byte-identical + // and were widened by hand in lockstep once already (PEN-3157); PEN-3391 + // extracted them so the next widening cannot reach only one. // // Enumerate-then-filter rather than grepping for an expected name: a // pathspec that matches nothing returns the same empty set as "everything @@ -373,30 +522,53 @@ describe("the scrub is reachable from server/ at all", () => { // outside it, so a new `ghFetch`-based write in either would ship // unscrubbed with this test green. Widening it finds the same two writers // today — the gap was in what the guard could see, not in what it covered. + // + // PEN-3391 fixed the second half of that same shape: the walk saw every + // file, but the PREDICATE recognised only `ghFetch(` plus an inline + // double-quoted upper-case method, so an aliased call or a `'post'` was + // classified as a read and never checked. It is now fail-closed — a + // candidate must be PROVABLY read-only — and the alphabet it accepts is + // pinned by a fail-first suite next to the helper. const serverSrc = path.join(repoRoot, "server/src"); - const scanned = readdirSync(serverSrc, { recursive: true, encoding: "utf8" }) - .map((entry) => entry.split(path.sep).join("/")) - .filter((entry) => entry.endsWith(".ts") && !entry.endsWith(".test.ts")); + const scanned = serverSourceFiles(serverSrc); // Scope control. A non-recursive regression still finds both writers below // (they sit directly in `services/`), so the only thing that catches it is // asserting the walk reaches a file it could not otherwise see. expect(scanned).toContain("routes/github-webhook.ts"); - const writers = scanned.filter((entry) => { - const source = readFileSync(path.join(serverSrc, entry), "utf8"); - if (!source.includes("ghFetch(")) return false; - return /method:\s*"(?:POST|PATCH|PUT|DELETE)"/.test(source); - }); + // The walk skips `__tests__/` so the derivation helper does not classify + // itself. That is sound only while the running server imports nothing from + // there, which is checked rather than assumed (PEN-3391). + expect( + productionFilesImportingTestHelpers(serverSrc), + "a production file imports from __tests__/, so skipping it no longer excludes only test code", + ).toEqual([]); + + const writers = serverFilesWritingToGitHub(serverSrc); // Positive control: the derivation must actually find the file we know // writes, or an empty `writers` would make the assertion below vacuous. + // The predicate's own alphabet — aliased calls, non-literal methods — is + // pinned separately in `helpers/github-writer-derivation.test.ts`, because + // a positive control over the tree can only prove it finds TODAY's writers. expect(writers).toContain("services/github-app-auth.ts"); for (const writer of writers) { const source = readFileSync(path.join(serverSrc, writer), "utf8"); - expect(source, `${writer} writes to GitHub without reaching the egress scrub`).toContain( - "scrubOutboundGitHubText", - ); + // The message has to describe what was actually measured, not what is + // most likely. The predicate is fail-closed, so a hit means "this scan + // could not PROVE this is read-only" — which includes the false-positive + // case of a file that merely says "method" somewhere. Reporting that as + // "writes to GitHub" would send someone hunting for a write that is not + // there, and the obvious way to make such a failure go away is to weaken + // the guard. So name the remedy for both cases. + expect( + source, + `${writer} references ghFetch and this scan cannot prove it is read-only, so it must ` + + `call scrubOutboundGitHubText. If it genuinely does not write, pin every \`method\` to ` + + `a quoted GET/HEAD/OPTIONS literal — see __tests__/helpers/github-writer-derivation.ts ` + + `for why the predicate errs in this direction.`, + ).toContain("scrubOutboundGitHubText"); } }); }); diff --git a/server/src/__tests__/helpers/github-writer-derivation.test.ts b/server/src/__tests__/helpers/github-writer-derivation.test.ts new file mode 100644 index 000000000000..16a43a2c1897 --- /dev/null +++ b/server/src/__tests__/helpers/github-writer-derivation.test.ts @@ -0,0 +1,278 @@ +import { describe, expect, it } from "vitest"; + +import { + declaresOnlySafeReadMethods, + importsFromTestHelpers, + isSuspectedGitHubWriter, + referencesGhFetch, +} from "./github-writer-derivation.js"; + +/** + * PEN-3391 done-when 1: "The writer predicate recognises a `ghFetch` write + * whose method is not an inline double-quoted literal … A fail-first check is + * required either way — a predicate that matches nothing returns the same empty + * set as full coverage." + * + * That last sentence is the whole reason this file exists. The guards that + * consume this predicate assert "every writer found carries the scrub", which + * a predicate matching NOTHING satisfies perfectly and vacuously. Their + * positive control (`expect(writers).toContain("services/github-app-auth.ts")`) + * proves the derivation finds the writers that exist *today*; it cannot prove + * it would find a writer written *tomorrow* in a shape the tree does not yet + * contain. Only synthetic sources can, so every shape below is one the tree + * does not contain. + * + * Each widened case is asserted twice: that `OLD_PREDICATE` — the exact + * predicate on `master` before this change — misses it, and that the new one + * catches it. The first half is what makes this a regression test rather than a + * restatement. If a future edit narrows the predicate back, the "old misses it" + * assertion still passes but its partner fails, and the failure names the + * shape. + */ + +/** + * Verbatim from `github-write-egress-scrub.test.ts:386-390` and + * `github-egress-outbound-coverage.test.ts:225-228` as they stood at PEN-3157's + * merged head (`f097134f`). Kept as a fixture so "the widening is real" is a + * measurement rather than a claim in a comment. + */ +function OLD_PREDICATE(source: string): boolean { + if (!source.includes("ghFetch(")) return false; + return /method:\s*"(?:POST|PATCH|PUT|DELETE)"/.test(source); +} + +/** A write the OLD predicate already caught — the shape both real writers use. */ +const INLINE_DOUBLE_QUOTED = ` + import { ghFetch } from "./github-fetch.js"; + await ghFetch(url, { method: "POST", headers, body }); +`; + +/** + * Shapes the old predicate missed. Every one is a working GitHub write. + * + * Several were argued unreachable on the grounds that "Prettier pins double + * quotes repo-wide". That premise is simply false: this repo has no Prettier, + * and no other formatter or linter either — the measurement is recorded on + * `TEST_HELPER_SPECIFIER` in `./github-writer-derivation.ts`. Every import + * being double-quoted today is convention, enforced by nothing. + * + * The premise would not license the conclusion even where it held. A formatter + * is not a security control, and it does not normalise a variable or a + * shorthand into a literal at all. + */ +const MISSED_BY_OLD: ReadonlyArray = [ + [ + "aliased through a local binding, so the file contains no `ghFetch(` at all", + // The live instance: services/github-external-object-provider.ts imports + // ghFetch, aliases it, and calls it through the alias. It is a read today. + // Adding a method to that one call is the whole distance to an unscrubbed + // write, and clause 1 excluded the file before the method test ran. + ` + import { ghFetch } from "./github-fetch.js"; + const fetchImpl = opts.fetch ?? ghFetch; + response = await fetchImpl(url, { method: "POST", headers, body }); + `, + ], + [ + "single-quoted method literal", + `import { ghFetch } from "./github-fetch.js"; + await ghFetch(url, { method: 'POST', headers });`, + ], + [ + "template-literal method", + "import { ghFetch } from \"./github-fetch.js\";\n" + + "await ghFetch(url, { method: `POST`, headers });", + ], + [ + "lower-case method literal (HTTP verbs are case-insensitive; fetch normalises them)", + `import { ghFetch } from "./github-fetch.js"; + await ghFetch(url, { method: "post", headers, body });`, + ], + [ + "method held in a variable", + `import { ghFetch } from "./github-fetch.js"; + const verb = shouldReplace ? "PUT" : "PATCH"; + await ghFetch(url, { method: verb, headers, body });`, + ], + [ + "object shorthand", + `import { ghFetch } from "./github-fetch.js"; + const method = "DELETE"; + await ghFetch(url, { method, headers });`, + ], + [ + "method assigned onto the init object after construction", + `import { ghFetch } from "./github-fetch.js"; + const init: RequestInit = { headers }; + init.method = "POST"; + await ghFetch(url, init);`, + ], +]; + +/** Genuine reads. Classifying these as writers would be a false positive. */ +const GENUINE_READS: ReadonlyArray = [ + [ + "no method key at all — the default GET, which is what seven of the nine candidates do", + `import { ghFetch } from "./github-fetch.js"; + const res = await ghFetch(url, { headers });`, + ], + [ + "explicit safe verb", + `import { ghFetch } from "./github-fetch.js"; + await ghFetch(url, { method: "GET", headers });`, + ], + [ + "explicit safe verb, lower-case and single-quoted", + `import { ghFetch } from "./github-fetch.js"; + await ghFetch(url, { method: 'head', headers });`, + ], + [ + "several safe verbs, every one a pinned literal", + `import { ghFetch } from "./github-fetch.js"; + await ghFetch(a, { method: "GET", headers }); + await ghFetch(b, { method: "HEAD", headers });`, + ], +]; + +describe("server GitHub writer derivation (PEN-3391)", () => { + describe("the candidate set is every file that names ghFetch", () => { + it("includes an importer that only ever calls through an alias", () => { + const source = MISSED_BY_OLD[0]?.[1] as string; + expect(source).not.toContain("ghFetch("); + expect(referencesGhFetch(source)).toBe(true); + }); + + it("excludes a file that never names ghFetch, whatever it posts", () => { + // Scope control. The guards only bind GitHub egress; a POST to any other + // host is a different question and must not be dragged in here. + const source = `await fetch("https://example.invalid", { method: "POST" });`; + expect(referencesGhFetch(source)).toBe(false); + expect(isSuspectedGitHubWriter(source)).toBe(false); + }); + }); + + describe("shapes the pre-PEN-3391 predicate missed", () => { + it.each(MISSED_BY_OLD)("catches a write %s", (_label, source) => { + // Fail-first: the old predicate really did let this through, so the new + // assertion below is measuring a widening and not restating a pass. + expect(OLD_PREDICATE(source)).toBe(false); + expect(isSuspectedGitHubWriter(source)).toBe(true); + }); + }); + + describe("shapes both predicates agree on", () => { + it("catches the inline double-quoted write the old predicate already caught", () => { + expect(OLD_PREDICATE(INLINE_DOUBLE_QUOTED)).toBe(true); + expect(isSuspectedGitHubWriter(INLINE_DOUBLE_QUOTED)).toBe(true); + }); + + it.each(GENUINE_READS)("does not classify %s as a writer", (_label, source) => { + expect(declaresOnlySafeReadMethods(source)).toBe(true); + expect(isSuspectedGitHubWriter(source)).toBe(false); + }); + }); + + describe("the read allowlist is exhaustive over the file, not satisfied by one hit", () => { + it("a safe verb does not excuse an unpinned one elsewhere in the same file", () => { + // The failure mode of an allowlist that merely searches: one recognised + // read makes the whole file read-only, and any number of writes ride + // along. Counting mentions against safe literals is what prevents it. + const source = `import { ghFetch } from "./github-fetch.js"; + await ghFetch(readUrl, { method: "GET", headers }); + await ghFetch(writeUrl, { method: verb, headers, body });`; + expect(declaresOnlySafeReadMethods(source)).toBe(false); + expect(isSuspectedGitHubWriter(source)).toBe(true); + }); + + it("a capitalised safe literal in prose does not cancel an unpinned write", () => { + // SAFE_READ_METHOD is case-insensitive, so `Method: "GET"` in a comment is + // a safe hit. Unless the mention count is case-insensitive too, that hit + // has no matching mention and offsets the real write below. + const source = `import { ghFetch } from "./github-fetch.js"; + /** Method: "GET" is the default. */ + await ghFetch(url, { method: verb, headers, body });`; + expect(declaresOnlySafeReadMethods(source)).toBe(false); + expect(isSuspectedGitHubWriter(source)).toBe(true); + }); + + it("classifies an ambiguous mention as a writer rather than a read", () => { + // Documenting the accepted false-positive direction. This file is a read, + // and the predicate calls it a writer because it says "method" in prose. + // The remedy is one legible failure naming the file; the alternative + // direction ships an unscrubbed credential. Asserted so that anyone who + // "fixes" the noise has to delete a test that says why it is there. + const source = `import { ghFetch } from "./github-fetch.js"; + // Uses the default method, which is GET. + await ghFetch(url, { headers });`; + expect(isSuspectedGitHubWriter(source)).toBe(true); + }); + }); + + describe("the residual gap is recorded, not silently absent", () => { + it("cannot see an init object assembled in another file", () => { + // The honest boundary of file-granular scanning, kept as an executable + // statement so it cannot rot into an unstated assumption. Nothing in the + // tree has this shape today — every ghFetch call site builds its options + // inline — and a "factor out the duplicate POST setup" refactor is what + // would introduce it. Closing it needs an import graph or a typed AST + // walk, which is a different mechanism, not a wider regex. + const caller = `import { ghFetch } from "./github-fetch.js"; + import { writeInit } from "./elsewhere.js"; + await ghFetch(url, writeInit(body));`; + expect(isSuspectedGitHubWriter(caller)).toBe(false); + }); + }); + + describe("the __tests__/ exclusion guard reads every import spelling", () => { + /** + * Verbatim from `productionFilesImportingTestHelpers` before this change. + * Same fail-first shape as `OLD_PREDICATE`: each widened spelling is + * asserted to slip past this one first, so the widening is measured rather + * than asserted. + */ + const OLD_TEST_IMPORT_PREDICATE = (source: string): boolean => + /from\s*"[^"]*__tests__\//.test(source); + + const MISSED_SPELLINGS: ReadonlyArray = [ + ["single-quoted static import", `import { seed } from '../__tests__/helpers/db.js';`], + ["single-quoted static re-export", `export { seed } from '../__tests__/helpers/db.js';`], + // Backticks reach a specifier only through `import(...)`. A static + // `from \`...\`` is a SyntaxError ("Unexpected template string"), so + // pinning that shape would assert the predicate against a module that + // cannot exist — a fixture describing an unreachable spelling, which is + // the exact failure this suite exists to catch one level up. + ["backtick dynamic import", "const { seed } = await import(`../__tests__/helpers/db.js`);"], + ["dynamic import", `const { seed } = await import("../__tests__/helpers/db.js");`], + ["single-quoted dynamic import", `await import('../__tests__/helpers/db.js');`], + ["side-effect import", `import "../__tests__/helpers/register.js";`], + ["require", `const { seed } = require("../__tests__/helpers/db.js");`], + ]; + + it.each(MISSED_SPELLINGS)("catches a %s", (_label, source) => { + expect(OLD_TEST_IMPORT_PREDICATE(source)).toBe(false); + expect(importsFromTestHelpers(source)).toBe(true); + }); + + it("still catches the double-quoted static import the old form caught", () => { + const source = `import { seed } from "../__tests__/helpers/db.js";`; + expect(OLD_TEST_IMPORT_PREDICATE(source)).toBe(true); + expect(importsFromTestHelpers(source)).toBe(true); + }); + + it("does not fire on a test path named in prose", () => { + // services/plugin-host-services.ts does exactly this. The predicate must + // stay empty over the real tree or its callers' assertion is vacuous, so + // matching `__tests__/` anywhere would be more fail-closed and less + // useful. This is the case that pins the boundary. + const source = `// See \`server/src/__tests__/plugin-events-ownership-check.test.ts\`. + import { helper } from "./helper.js";`; + expect(importsFromTestHelpers(source)).toBe(false); + }); + + it("does not fire on an unrelated import elsewhere in the file", () => { + const source = `import { a } from "./a.js"; + const label = "__tests__/ is excluded from the walk";`; + expect(importsFromTestHelpers(source)).toBe(false); + }); + }); +}); diff --git a/server/src/__tests__/helpers/github-writer-derivation.ts b/server/src/__tests__/helpers/github-writer-derivation.ts new file mode 100644 index 000000000000..10184a08cdc3 --- /dev/null +++ b/server/src/__tests__/helpers/github-writer-derivation.ts @@ -0,0 +1,194 @@ +import { readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; + +/** + * The single derivation of the server-side GitHub writer set (PEN-3391). + * + * ## Why this is shared rather than duplicated + * + * Two guards consume it — `github-write-egress-scrub.test.ts` ("leaves no + * server-side GitHub writer outside the scrub") and + * `github-egress-outbound-coverage.test.ts` ("classifies every server file that + * writes to GitHub"). They carried byte-identical copies of the predicate, and + * PEN-3157 widened only the *walk*, in both, by hand. A second divergence was + * one edit away: the next widening would have reached whichever copy its author + * happened to open. There is now one copy, so a widening cannot land by halves. + * + * ## Why the predicate is fail-CLOSED + * + * The predicate it replaces asked "can I see a write here?": + * + * if (!source.includes("ghFetch(")) return false; + * return /method:\s*"(?:POST|PATCH|PUT|DELETE)"/.test(source); + * + * Both clauses fail OPEN — an unrecognised writer is silently classified as a + * read, and a file the guard does not classify is a file the guard never + * checks. That is the wrong direction for a mechanism whose entire job is to + * catch the *next* writer, and it is the same shape this control has now been + * bitten by three times (PEN-2527 enumerated `gh` and missed the MCP server; + * PEN-3152 enumerated both wrappers and missed `server/`; PEN-3157 enumerated + * `services/` and missed its own siblings). Each time an enumeration was + * correct over a set that was quietly too small. + * + * So this asks the inverse — "can I PROVE this is not a write?" — and treats + * everything else as a writer: + * + * - **Clause 1 widened from a call to a reference.** `source.includes("ghFetch(")` + * requires a literal call *in the same file*. `github-external-object-provider.ts` + * imports `ghFetch`, aliases it (`const fetchImpl = opts.fetch ?? ghFetch`) + * and calls it through the alias — so it contains no `ghFetch(` substring and + * was excluded before the method test was ever reached. It is a read today + * (headers only), but adding `method: "POST"` to that one call would have + * shipped an unscrubbed GitHub write with BOTH guards green. Matching + * `\bghFetch\b` puts every importer in the candidate set regardless of how it + * later spells the call. + * - **Clause 2 inverted from an allowlist of mutating spellings to an allowlist + * of read spellings.** `/method:\s*"(?:POST|PATCH|PUT|DELETE)"/` recognises + * only an inline, double-quoted, upper-case literal. It misses `'POST'`, + * `` `POST` ``, `"post"` (HTTP methods are case-insensitive and `fetch` + * normalises the known ones, so this is a working write), a variable + * (`method: verb`), and the `{ method }` shorthand. Rather than chase that + * alphabet — the losing side of the trade, since the next spelling is always + * one more than the list — a candidate is read-only only when every mention + * of the word `method` is a quoted safe-verb literal. + * + * The cost of that inversion is false POSITIVES: a candidate that merely says + * "method" in a comment, or uses `method` as a bare identifier + * (`const method = pick()`), is classified as a writer and must carry the + * scrub. That is deliberate. A false positive is one legible test failure + * naming the file; a false negative is an unscrubbed + * credential on a public commit status. It costs nothing today — of the nine + * files under `server/src` that reference `ghFetch`, the word `method` appears + * in exactly the two that do write, and both already scrub. + * + * ## What still escapes it, and what would make that reachable + * + * A `RequestInit` assembled in one file and passed by reference into a + * `ghFetch` call in another leaves no `method` token in the calling file, so + * the caller reads as provably read-only. Nothing in the tree does this: every + * `ghFetch` call site builds its options inline, and `ghFetch` itself + * (`services/github-fetch.ts`) merely forwards an `init?: RequestInit` it never + * inspects. It becomes reachable the day someone factors request-building into + * a shared helper — a "remove the duplication between these two POSTs" refactor + * is exactly the innocent change that would do it. Closing that needs a real + * import graph or a typed AST walk, not a regex over one file; this is the + * boundary of what file-granular scanning can establish, and it is recorded + * here rather than left as an unstated assumption. + * + * `github-writer-derivation.test.ts` drives every shape named above through + * these functions, including the ones the old predicate missed, so the widening + * is pinned by a check that fails first rather than by this comment. + */ + +/** + * Any mention of `ghFetch` — an import, a call, or an alias assignment — puts a + * file in the candidate set. Deliberately NOT `ghFetch(`: see clause 1 above. + */ +const GH_FETCH_REFERENCE = /\bghFetch\b/; + +/** + * Every mention of the word, whatever its shape or case. Must match + * SAFE_READ_METHOD's case-insensitivity, or a capitalised safe literal credits a + * safe hit with no matching mention and cancels a real write. + */ +const METHOD_WORD = /\bmethod\b/gi; + +/** + * The only shape that proves a `method` is a read: a quoted literal naming a + * safe verb. Any quote style, either case — the point is the VALUE is pinned in + * source, not that it is spelled a particular way. + */ +const SAFE_READ_METHOD = /\bmethod\s*:\s*(["'`])\s*(?:GET|HEAD|OPTIONS)\s*\1/gi; + +/** True when the file names `ghFetch` at all, however it later calls it. */ +export function referencesGhFetch(source: string): boolean { + return GH_FETCH_REFERENCE.test(source); +} + +/** + * True when the file cannot be issuing a mutating request: it either never says + * `method`, or every mention is a quoted safe-verb literal. + */ +export function declaresOnlySafeReadMethods(source: string): boolean { + const mentions = source.match(METHOD_WORD)?.length ?? 0; + if (mentions === 0) return true; + const safe = source.match(SAFE_READ_METHOD)?.length ?? 0; + return safe === mentions; +} + +/** A candidate this scan cannot prove is read-only. Fail-closed by construction. */ +export function isSuspectedGitHubWriter(source: string): boolean { + return referencesGhFetch(source) && !declaresOnlySafeReadMethods(source); +} + +/** + * Every non-test TypeScript file under `server/src`, relative to it with + * forward slashes ("services/github-app-auth.ts"). + * + * Recursive, and that is load-bearing — Ally caught a one-level walk of + * `server/src/services` on #1754, which could not see `routes/github-webhook.ts` + * or `services/recovery/`. + * + * `__tests__/` is excluded because this file lives there: a scanner that + * described its own regexes would classify itself as an unscrubbed writer. The + * exclusion is sound only while nothing in the running server imports from + * `__tests__/`, which is not an assumption to leave unstated — + * `productionFilesImportingTestHelpers` below lets the callers assert it. + */ +export function serverSourceFiles(serverSourceDirectory: string): string[] { + return readdirSync(serverSourceDirectory, { recursive: true, encoding: "utf8" }) + .map((entry) => entry.split(path.sep).join("/")) + .filter((entry) => entry.endsWith(".ts") && !entry.endsWith(".test.ts")) + .filter((entry) => !entry.startsWith("__tests__/")) + .sort(); +} + +/** + * Any specifier that pulls `__tests__/` into a module — `from` for static + * imports and re-exports, bare `import`/`require` for side-effect and dynamic + * ones — in any quote style. + * + * The quote class is deliberate. This predicate is what keeps the `__tests__/` + * exclusion in `serverSourceFiles` sound, and an earlier form matched + * `from\s*"…"` only. That is the same double-quote assumption the writer + * predicate above rejects, applied to the check guarding the writer predicate's + * own scope — and the assumption is weaker here than the phrase "Prettier pins + * double quotes" suggests, because this repo has no Prettier: there is no + * formatter or linter config, dependency, or CI step anywhere in it. Every + * import being double-quoted today is convention, enforced by nothing. + * + * It stops short of matching `__tests__/` anywhere in the file, which would be + * more fail-closed but would trip on prose: `services/plugin-host-services.ts` + * names a test file in a comment, and this predicate must stay empty over the + * real tree for its callers' assertion to mean anything. + */ +const TEST_HELPER_SPECIFIER = /\b(?:from|import|require)\s*\(?\s*(["'`])[^"'`]*__tests__\//; + +/** True when the file pulls a module out of `__tests__/`, however it spells it. */ +export function importsFromTestHelpers(source: string): boolean { + return TEST_HELPER_SPECIFIER.test(source); +} + +/** + * Production files that reach into `__tests__/`. Must be empty for the + * exclusion in `serverSourceFiles` to hold; asserted by both callers rather + * than trusted. + * + * Note that an empty result is also what a predicate matching nothing returns, + * so the callers' assertion cannot distinguish the two. `importsFromTestHelpers` + * is exported and unit-tested against synthetic sources for that reason. + */ +export function productionFilesImportingTestHelpers(serverSourceDirectory: string): string[] { + return serverSourceFiles(serverSourceDirectory).filter((entry) => + importsFromTestHelpers(readFileSync(path.join(serverSourceDirectory, entry), "utf8")), + ); +} + +/** The fail-closed writer set: candidates this scan cannot prove are reads. */ +export function serverFilesWritingToGitHub(serverSourceDirectory: string): string[] { + return serverSourceFiles(serverSourceDirectory) + .filter((entry) => + isSuspectedGitHubWriter(readFileSync(path.join(serverSourceDirectory, entry), "utf8")), + ) + .sort(); +} diff --git a/server/src/services/github-app-auth.ts b/server/src/services/github-app-auth.ts index f2bebe29f308..1ad27bc40c19 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -1170,6 +1170,72 @@ export function scrubOutboundGitHubText(value: string, field: string): string { return result.text; } +/** + * Guard an **identity** field bound for GitHub: refuse the write rather than + * redact it (PEN-3391). + * + * ## Why identity fields are not scrubbed like prose + * + * `scrubOutboundGitHubText` redacts and proceeds. That is right for prose — a + * commit-status `description`, a check-run `summary` — where a redacted string + * is a degraded but still-serviceable version of the same message. + * + * It is wrong for a field that is a NAME. A commit status is addressed by + * `(repo, sha, context)` and a check-run by `(repo, sha, name)`; those values + * are what branch protection matches a required check against, what + * `githubGetLatestCommitStatusForContext` filters on, and what the delivery + * outbox keys its upsert on. Redacting one does not degrade the identity, it + * substitutes a DIFFERENT one. The status would be published under a name + * nothing looks up: branch protection would go on waiting for a context that + * will never arrive, the outbox would key its row on the unredacted value it + * was handed, and the gate would be unable to observe its own status. That is a + * silent gate-liveness failure — the control stays green while the thing it + * gates is stuck. + * + * PEN-3157 already reached this conclusion at the enqueue boundary, where it + * declined to scrub `context` because doing so "would risk the delivery + * identity". The same argument holds at the send boundary; it just bites later. + * The code did both things and justified only one, which is what PEN-3391 was + * filed to settle. + * + * ## Why refuse rather than simply exempt the field + * + * Exempting it (leaving `context` unscrubbed) would restore the leak the scrub + * exists to prevent: commit-status contexts are public on a public repo. + * Refusing keeps both properties at once — + * + * - **nothing leaks**, because a credential-bearing identity is never published; and + * - **publish and lookup cannot disagree**, because the write proceeds only + * when the scrub is a byte-for-byte no-op. Callers may therefore keep using + * the raw value as a key, and are provably keying on what was published. + * + * The refusal is deterministic: the same input scrubs the same way every time, + * so a retry cannot succeed. That is a property of this function, not a + * guarantee that callers stop retrying. The review-gate delivery loop + * (`github-review-gate-authority.ts`) re-queues every non-ok result with + * backoff and has no terminal failure state, so a refused context there is + * retried indefinitely, as `review_gate_persisted_payload_invalid` and + * `review_gate_pull_payload_invalid` already are (PEN-3504). It is logged at + * `error` rather than `warn` because, unlike a redaction, no write happened. + * + * A caller reaching this has a configuration bug — every context in this repo + * is a fixed operator-set literal — so the refusal is a loud stop, not a + * degradation path. + * + * @returns the detected classes when the write must be refused, or `null` when + * the field is publishable unchanged. + */ +export function gitHubIdentityFieldRedaction(value: string, field: string): string[] | null { + const result = scrubGitHubEgressText(value); + if (!result.redacted) return null; + console.error( + `[github-egress] REFUSED an outbound GitHub write: the ${field} is an identity field and ` + + `credential-shaped material was detected in it (${result.classes.join(", ")}). Nothing was ` + + `published — redacting it would address the status to a name no lookup can find.`, + ); + return result.classes; +} + /** * Post a commit status as the GitHub App with a classified result so callers * can retry transient failures and surface permanent configuration/permission @@ -1199,7 +1265,14 @@ export async function githubPostCommitStatusDetailed(input: { const description = input.description ? scrubOutboundGitHubText(input.description, "commit-status description").slice(0, 140) : undefined; - const context = scrubOutboundGitHubText(input.context, "commit-status context"); + // `context` is the status's IDENTITY, not prose: branch protection matches a + // required check on it and `githubGetLatestCommitStatusForContext` filters on + // it. Refuse rather than redact, so publish and lookup cannot disagree — see + // `gitHubIdentityFieldRedaction` (PEN-3391). + if (gitHubIdentityFieldRedaction(input.context, "commit-status context")) { + return { ok: false, retryable: false, reason: "commit_status_context_not_publishable" }; + } + const context = input.context; const targetUrl = input.targetUrl ? scrubOutboundGitHubText(input.targetUrl, "commit-status target_url") : input.targetUrl; @@ -1268,7 +1341,13 @@ export async function githubPostCheckRun(input: { // and a check-run has no 140-char cap — so it publishes MORE of it. Scrubbed // here like every other free-text field this file writes, so the helper's // callers inherit the control rather than each remembering it (PEN-3157). - const name = scrubOutboundGitHubText(input.name, "check-run name"); + // `name` is the check-run's IDENTITY — it is what a required check is matched + // on and what a `check-runs` read selects by — so it is refused, not redacted, + // for the same reason as a commit-status `context` (PEN-3391). + if (gitHubIdentityFieldRedaction(input.name, "check-run name")) { + return { ok: false, retryable: false, reason: "check_run_name_not_publishable" }; + } + const name = input.name; const title = scrubOutboundGitHubText(input.title, "check-run title"); const summary = scrubOutboundGitHubText(input.summary, "check-run summary"); const detailsUrl = input.detailsUrl diff --git a/server/src/services/github-review-gate-authority.ts b/server/src/services/github-review-gate-authority.ts index 2800ec74f2e7..dcccaccec95c 100644 --- a/server/src/services/github-review-gate-authority.ts +++ b/server/src/services/github-review-gate-authority.ts @@ -4,7 +4,11 @@ import { and, eq, inArray, lt, sql } from "drizzle-orm"; import { githubReviewGateDeliveries, type Db } from "@paperclipai/db"; import { loadConfig } from "../config.js"; import { logger } from "../middleware/logger.js"; -import { getInstallationTokenResult, scrubOutboundGitHubText } from "./github-app-auth.js"; +import { + getInstallationTokenResult, + gitHubIdentityFieldRedaction, + scrubOutboundGitHubText, +} from "./github-app-auth.js"; import { ghFetch, gitHubApiBase } from "./github-fetch.js"; const GITHUB_HOST = "github.com"; @@ -324,6 +328,15 @@ async function postPendingStatus(input: { // template or an id today; the scrub is a byte-for-byte no-op on those, and it // is here so that stays true if someone later interpolates a variable // (PEN-3157). + // + // `context` is the exception, and for the same reason as in the shared helper: + // it is the status's identity, so a redaction would publish under a name the + // outbox and branch protection cannot find. Refuse instead (PEN-3391). + // processDelivery re-queues this like any other failure; nothing here makes + // it terminal yet (PEN-3504). + if (gitHubIdentityFieldRedaction(input.row.statusContext, "commit-status context")) { + return { ok: false, reason: "review_gate_status_context_not_publishable" }; + } try { const response = await ghFetch( `${gitHubApiBase(GITHUB_HOST)}/repos/${input.row.repoFullName}/statuses/${input.sha}`, @@ -336,7 +349,7 @@ async function postPendingStatus(input: { }, body: JSON.stringify({ state: "pending", - context: scrubOutboundGitHubText(input.row.statusContext, "commit-status context"), + context: input.row.statusContext, description: scrubOutboundGitHubText( `Evaluating Ally review gate after signed webhook ${input.origin}.`, "commit-status description",