From 309fa4e8317afe74d755a3b5c2fda6a461424141 Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Wed, 23 Sep 2026 00:30:06 +0000 Subject: [PATCH 1/6] fix(security): make the server GitHub writer guard fail closed and refuse redacted identity fields (PEN-3391) Signed-off-by: Security Engineer --- .../github-egress-outbound-coverage.test.ts | 77 ++++--- .../github-write-egress-scrub.test.ts | 185 +++++++++++++++- .../helpers/github-writer-derivation.test.ts | 206 ++++++++++++++++++ .../helpers/github-writer-derivation.ts | 159 ++++++++++++++ server/src/services/github-app-auth.ts | 79 ++++++- .../services/github-review-gate-authority.ts | 15 +- 6 files changed, 679 insertions(+), 42 deletions(-) create mode 100644 server/src/__tests__/helpers/github-writer-derivation.test.ts create mode 100644 server/src/__tests__/helpers/github-writer-derivation.ts 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..1d081e486033 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,23 +522,35 @@ 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) { 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..5f1826662dfb --- /dev/null +++ b/server/src/__tests__/helpers/github-writer-derivation.test.ts @@ -0,0 +1,206 @@ +import { describe, expect, it } from "vitest"; + +import { + declaresOnlySafeReadMethods, + 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. + * Prettier pins double quotes repo-wide, which is why several of these were + * argued unreachable — but Prettier is a formatter, 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("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); + }); + }); +}); 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..2e2450127a39 --- /dev/null +++ b/server/src/__tests__/helpers/github-writer-derivation.ts @@ -0,0 +1,159 @@ +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 names a variable `methodName`, 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. */ +const METHOD_WORD = /\bmethod\b/g; + +/** + * 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(); +} + +/** + * Production files that reach into `__tests__/`. Must be empty for the + * exclusion in `serverSourceFiles` to hold; asserted by both callers rather + * than trusted. + */ +export function productionFilesImportingTestHelpers(serverSourceDirectory: string): string[] { + return serverSourceFiles(serverSourceDirectory).filter((entry) => + /from\s*"[^"]*__tests__\//.test(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..53dbc3c7f352 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -1170,6 +1170,68 @@ 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 failure is non-retryable by construction: the same input scrubs the same + * way every time, so a retry cannot succeed. 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 +1261,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 +1337,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..3d94f7258905 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,13 @@ 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). + 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 +347,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", From f74f6c6f8ed504eeb0c7f7601f1535eb9fbbc051 Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Wed, 23 Sep 2026 00:41:38 +0000 Subject: [PATCH 2/6] test(security): report what the fail-closed writer scan measured, not what it assumes (PEN-3391) Signed-off-by: Security Engineer --- .../__tests__/github-write-egress-scrub.test.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/server/src/__tests__/github-write-egress-scrub.test.ts b/server/src/__tests__/github-write-egress-scrub.test.ts index 1d081e486033..5aca3ecb0f2a 100644 --- a/server/src/__tests__/github-write-egress-scrub.test.ts +++ b/server/src/__tests__/github-write-egress-scrub.test.ts @@ -555,9 +555,20 @@ describe("the scrub is reachable from server/ at all", () => { 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"); } }); }); From 08469014c1fcd0872d789125dc6319a6b202fe2d Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Wed, 23 Sep 2026 12:19:35 +0000 Subject: [PATCH 3/6] fix(security): count method mentions case-insensitively (PEN-3391) Ally found that METHOD_WORD was case-sensitive while SAFE_READ_METHOD is case-insensitive. declaresOnlySafeReadMethods decides by counting safe literals against mentions, so a capitalised `Method: "GET"` in a comment added one safe hit and no mention, cancelling a genuine unpinned write in the same file and classifying it read-only. METHOD_WORD now uses /gi so both counts agree on case. A new case in the exhaustiveness suite pins the decoy shape, and the fail-closed doc no longer cites `methodName` (which \bmethod\b never matches) as a false positive example. Measured the nine ghFetch importers under server/src: case-insensitive mention counts equal the case-sensitive ones, so no in-tree read changes class. Controls: - positive: vitest run of github-writer-derivation.test.ts, github-write-egress-scrub.test.ts, github-egress-outbound-coverage.test.ts (3 files, 50 tests passed); tsc --noEmit in server exits 0. - negative: reverting METHOD_WORD to /g fails the new test (expected true to be false); restoring /gi returns 18/18 green. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Omar Ramadan --- .../helpers/github-writer-derivation.test.ts | 11 +++++++++++ .../__tests__/helpers/github-writer-derivation.ts | 15 ++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/helpers/github-writer-derivation.test.ts b/server/src/__tests__/helpers/github-writer-derivation.test.ts index 5f1826662dfb..061d0a1645f6 100644 --- a/server/src/__tests__/helpers/github-writer-derivation.test.ts +++ b/server/src/__tests__/helpers/github-writer-derivation.test.ts @@ -176,6 +176,17 @@ describe("server GitHub writer derivation (PEN-3391)", () => { 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. diff --git a/server/src/__tests__/helpers/github-writer-derivation.ts b/server/src/__tests__/helpers/github-writer-derivation.ts index 2e2450127a39..f22267b87abf 100644 --- a/server/src/__tests__/helpers/github-writer-derivation.ts +++ b/server/src/__tests__/helpers/github-writer-derivation.ts @@ -53,9 +53,10 @@ import path from "node:path"; * 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 names a variable `methodName`, 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 + * "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. @@ -85,8 +86,12 @@ import path from "node:path"; */ const GH_FETCH_REFERENCE = /\bghFetch\b/; -/** Every mention of the word, whatever its shape. */ -const METHOD_WORD = /\bmethod\b/g; +/** + * 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 From d9c94bda740c9cac244266763959983fbf2632af Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Wed, 23 Sep 2026 15:43:08 +0000 Subject: [PATCH 4/6] fix(security): widen the __tests__/ exclusion guard to every import spelling (PEN-3391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Ally's review of f74f6c6f (suggestions 2 and 3). The Important finding — METHOD_WORD case-sensitivity — was already fixed by 08469014; verified here by mutation rather than by reading: reverting /gi to /g fails exactly the new "capitalised safe literal in prose" case (1 failed, 17 passed), so the fix is real and its test is not vacuous. Suggestion 2. `productionFilesImportingTestHelpers` matched `/from\s*"[^"]*__tests__\//` — double-quoted static imports only. That is the double-quote assumption this PR rejects for the writer predicate, applied to the check that keeps the writer predicate's own scope sound. Widened to any quote style and to bare `import`/`require`, so side-effect, dynamic and re-export specifiers are seen. The assumption was weaker than the surrounding comments claim. They say "Prettier pins double quotes repo-wide"; this repo has no Prettier. There is no formatter or linter config, dependency or CI step anywhere in it — no .prettierrc, no eslint/biome/dprint, nothing in any package.json or workflow. Every import being double-quoted today is convention, enforced by nothing. The predicate stops short of matching `__tests__/` anywhere in a file. That would be more fail-closed but trips on prose — services/plugin-host-services.ts names a test file in a comment — and the predicate must stay empty over the real tree for its callers' assertion to mean anything. Both boundaries are now pinned by tests. Those tests matter because this function is the shape PEN-3391's own done-when warns about: it is exercised only over a tree where it returns [], which is what a predicate matching nothing also returns. The six widened spellings are asserted fail-first against the old form, so the widening is measured. Verified non-vacuous: restoring the narrow regex fails exactly those six. Suggestion 3. Removed the double blank line after gitHubIdentityFieldRedaction. Blank-line-only (`git diff --ignore-blank-lines` is empty). Ally could not tell whether a format check enforced it; none does, per the absence above. Tests: 59 passed across github-writer-derivation, github-write-egress-scrub and github-egress-outbound-coverage (27 in the derivation suite, up from 18). Signed-off-by: Security Engineer --- .../helpers/github-writer-derivation.test.ts | 48 +++++++++++++++++++ .../helpers/github-writer-derivation.ts | 32 ++++++++++++- server/src/services/github-app-auth.ts | 1 - 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/helpers/github-writer-derivation.test.ts b/server/src/__tests__/helpers/github-writer-derivation.test.ts index 061d0a1645f6..c94ceaf3fa69 100644 --- a/server/src/__tests__/helpers/github-writer-derivation.test.ts +++ b/server/src/__tests__/helpers/github-writer-derivation.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { declaresOnlySafeReadMethods, + importsFromTestHelpers, isSuspectedGitHubWriter, referencesGhFetch, } from "./github-writer-derivation.js"; @@ -214,4 +215,51 @@ describe("server GitHub writer derivation (PEN-3391)", () => { 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';`], + ["backtick specifier", "export { seed } from `../__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 index f22267b87abf..10184a08cdc3 100644 --- a/server/src/__tests__/helpers/github-writer-derivation.ts +++ b/server/src/__tests__/helpers/github-writer-derivation.ts @@ -143,14 +143,44 @@ export function serverSourceFiles(serverSourceDirectory: string): string[] { .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) => - /from\s*"[^"]*__tests__\//.test(readFileSync(path.join(serverSourceDirectory, entry), "utf8")), + importsFromTestHelpers(readFileSync(path.join(serverSourceDirectory, entry), "utf8")), ); } diff --git a/server/src/services/github-app-auth.ts b/server/src/services/github-app-auth.ts index 53dbc3c7f352..0ecf819ea6f5 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -1231,7 +1231,6 @@ export function gitHubIdentityFieldRedaction(value: string, field: string): stri 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 From f1a998fa35f53ea59ed27707385ce5e75ce53beb Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Thu, 24 Sep 2026 05:07:46 +0000 Subject: [PATCH 5/6] test(security): pin the reachable backtick spelling, drop the false Prettier premise (PEN-3391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to fixtures this PR introduced, both in the layer where a wrong premise is least visible and most load-bearing. 1. The `"backtick specifier"` row asserted the predicate against ``export { seed } from `...` ``, which is not valid JavaScript — a static import/export specifier must be a string literal, and Node's ESM parser rejects it with "Unexpected template string". The row therefore pinned a module that cannot exist, while the genuinely reachable backtick spelling — a dynamic ``import(`...`)`` — went unpinned. A fixture that quietly defines an unreachable accepted set is the exact failure this suite exists to catch one level up, so it should not sit inside the suite itself. Replaced with ``import(`...`)``, and the static re-export spelling is restored as its own single-quoted row so re-export coverage is not lost. Verified with Node's own parser rather than by reading, with valid and invalid controls so the check is discriminating: export { seed } from "..." -> valid (control) export { seed } from '...' -> valid (control) export { seed } from `...` -> INVALID (the fixture as written) import { seed } from `...` -> INVALID await import(`...`) -> valid (the replacement) export { seed from ;;; -> INVALID (control) 2. The `MISSED_BY_OLD` doc comment still asserted "Prettier pins double quotes repo-wide". That premise is false — this repo has no Prettier, and no other formatter or linter — and the sibling helper three files away already says so, so the PR shipped two comments making opposite claims. Corrected to match the measurement, keeping the independent point that a formatter would not be a security control even where it existed. Mutation-tested, each targeting one row: drop backtick from the quote class -> 1 failed, exactly "backtick dynamic import" (the new row is uniquely load-bearing) drop single-quote -> 3 failed, including the new re-export row (it shares the `from` + single-quote path, so it documents a distinct reachable spelling rather than adding unique regex coverage) restore the original narrow regex -> all 7 spellings fail (fail-first holds for every row) Guards green, and green again after merging current master forward (38 files of drift, zero overlap with this branch): 60 passed across github-writer-derivation, github-write-egress-scrub and github-egress-outbound-coverage. Real-tree derivation over the merged tree is unchanged — 446 files scanned, writers still exactly github-app-auth.ts and github-review-gate-authority.ts, and no production file imports from `__tests__/`. No production code changes; the derivation helper is byte-identical. Signed-off-by: Security Engineer --- .../helpers/github-writer-derivation.test.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/server/src/__tests__/helpers/github-writer-derivation.test.ts b/server/src/__tests__/helpers/github-writer-derivation.test.ts index c94ceaf3fa69..16a43a2c1897 100644 --- a/server/src/__tests__/helpers/github-writer-derivation.test.ts +++ b/server/src/__tests__/helpers/github-writer-derivation.test.ts @@ -49,9 +49,16 @@ const INLINE_DOUBLE_QUOTED = ` /** * Shapes the old predicate missed. Every one is a working GitHub write. - * Prettier pins double quotes repo-wide, which is why several of these were - * argued unreachable — but Prettier is a formatter, not a security control, and - * it does not normalise a variable or a shorthand into a literal at all. + * + * 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 = [ [ @@ -228,7 +235,13 @@ describe("server GitHub writer derivation (PEN-3391)", () => { const MISSED_SPELLINGS: ReadonlyArray = [ ["single-quoted static import", `import { seed } from '../__tests__/helpers/db.js';`], - ["backtick specifier", "export { 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";`], From 3a71f52c53f71ffa350e4e5a8981f682dd282efb Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Thu, 24 Sep 2026 13:02:33 +0000 Subject: [PATCH 6/6] docs(github-egress): stop claiming callers treat an identity-field refusal as terminal gitHubIdentityFieldRedaction's docstring called its refusal "non-retryable by construction". The refusal is deterministic, but the review-gate delivery loop re-queues every non-ok result with backoff and has no terminal failure state, so review_gate_status_context_not_publishable is retried indefinitely there, as the persisted/pull payload reasons already are. Say so, and point both the docstring and the call site at PEN-3504, which tracks giving those deliveries a terminal state. Comment-only; no behaviour change. Co-Authored-By: Claude Opus 5.5 (1M context) --- server/src/services/github-app-auth.ts | 11 ++++++++--- server/src/services/github-review-gate-authority.ts | 2 ++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/server/src/services/github-app-auth.ts b/server/src/services/github-app-auth.ts index 0ecf819ea6f5..1ad27bc40c19 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -1209,9 +1209,14 @@ export function scrubOutboundGitHubText(value: string, field: string): string { * 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 failure is non-retryable by construction: the same input scrubs the same - * way every time, so a retry cannot succeed. It is logged at `error` rather - * than `warn` because, unlike a redaction, no write happened. + * 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 diff --git a/server/src/services/github-review-gate-authority.ts b/server/src/services/github-review-gate-authority.ts index 3d94f7258905..dcccaccec95c 100644 --- a/server/src/services/github-review-gate-authority.ts +++ b/server/src/services/github-review-gate-authority.ts @@ -332,6 +332,8 @@ async function postPendingStatus(input: { // `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" }; }