fix(security): make the server GitHub writer guard fail closed and refuse redacted identity fields (PEN-3391)

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<Record<string, Coverage>> = {
  * `github-write-egress-scrub.test.ts`, not here.
  */
 const SERVER_WRITE_COVERAGE: Readonly<Record<string, Coverage>> = {
-  // 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", () => {

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");
     }
   });
 });