From aa95a74d9bf87efbdbe58bddf309c3bd5112d843 Mon Sep 17 00:00:00 2001 From: GildardoDev <267998055+GildardoDev@users.noreply.github.com> Date: Mon, 29 Jun 2026 15:29:34 -0500 Subject: [PATCH 1/2] feat(enrichment): commit-signature / verified-author provenance analyzer --- .../src/analyzers/commit-signature.ts | 167 ++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 26 +++ review-enrichment/src/types.ts | 19 ++ .../test/commit-signature.test.ts | 184 ++++++++++++++++++ 5 files changed, 398 insertions(+) create mode 100644 review-enrichment/src/analyzers/commit-signature.ts create mode 100644 review-enrichment/test/commit-signature.test.ts diff --git a/review-enrichment/src/analyzers/commit-signature.ts b/review-enrichment/src/analyzers/commit-signature.ts new file mode 100644 index 0000000000..c9210f8af6 --- /dev/null +++ b/review-enrichment/src/analyzers/commit-signature.ts @@ -0,0 +1,167 @@ +// Commit-signature / verified-author provenance analyzer (#1517). Inspects the PR head commit's signature +// verification verdict, its author/committer identity, and — when the head is from an author with no prior +// verified history in a repo that otherwise carries verified commits — flags a never-before-seen committer. +// These are supply-chain / impersonation signals the no-checkout `claude --print` reviewer cannot derive +// (no GitHub commit-verification API access, no repo history). Surfaces ONLY GitHub's public verification +// verdict (`verified` + `reason`) and boolean provenance flags — never tokens, emails, or private identities. +import type { EnrichRequest, CommitSignatureFinding } from "../types.js"; + +const GITHUB_API = "https://api.github.com"; +// Pull a bounded slice of recent commits — enough to decide "has any verified history" without paging the whole +// repo. The history check runs at most two such queries (author-filtered + repo-wide), matching how the other +// analyzers cap their network round-trips. +const HISTORY_PER_PAGE = 30; +// Only repository slugs that look like real `owner/repo` segments are ever interpolated into a request URL. +const SLUG_RE = /^[A-Za-z0-9._-]+$/; + +interface ScanOptions { + signal?: AbortSignal; +} + +// The slice of the GitHub commit payload this analyzer reads. Everything else on the response is ignored. +interface CommitResponse { + commit?: { + verification?: { verified?: boolean; reason?: string }; + author?: { name?: string }; + committer?: { name?: string }; + }; + author?: { login?: string } | null; + committer?: { login?: string } | null; +} + +interface HistoryCommit { + commit?: { verification?: { verified?: boolean } }; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; +} + +/** Fetch the head commit's verification + identity payload. Returns null on any error / non-200 (fail-safe). */ +export async function fetchHeadCommit( + owner: string, + repo: string, + headSha: string, + headers: Record, + fetchFn: typeof fetch, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) return null; + try { + const resp = await fetchFn( + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(headSha)}`, + { headers, signal }, + ); + if (!resp.ok) return null; + return (await resp.json()) as CommitResponse; + } catch { + return null; + } +} + +/** Fetch one bounded page of the repo's recent commits, optionally filtered to a single author, and report + * whether ANY of them carry a verified signature. Returns true/false on a definitive answer, or null when + * undeterminable (network error / non-200 / unexpected shape) — callers fail safe on null. */ +export async function hasVerifiedHistory( + owner: string, + repo: string, + headers: Record, + fetchFn: typeof fetch, + author?: string, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) return null; + const authorQuery = author ? `author=${encodeURIComponent(author)}&` : ""; + try { + const resp = await fetchFn( + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits?${authorQuery}per_page=${HISTORY_PER_PAGE}`, + { headers, signal }, + ); + if (!resp.ok) return null; + const commits = (await resp.json()) as HistoryCommit[]; + if (!Array.isArray(commits)) return null; + return commits.some((c) => c.commit?.verification?.verified === true); + } catch { + return null; + } +} + +/** Analyzer entrypoint: inspect the head commit's signature + author provenance. Fail-safe — returns no finding + * on a missing token / head SHA, an unresolvable repo slug, or any fetch error, and never throws. */ +export async function scanCommitSignature( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, githubToken, headSha } = req; + if (!githubToken || !headSha) return []; + + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const headers = githubHeaders(githubToken); + const head = await fetchHeadCommit( + owner, + repo, + headSha, + headers, + fetchFn, + options.signal, + ); + if (!head?.commit) return []; + + const verified = head.commit.verification?.verified === true; + const reason = head.commit.verification?.reason ?? "unknown"; + const authorLogin = head.author?.login; + const committerLogin = head.committer?.login; + // An author/committer login mismatch can indicate a rewritten/impersonated authorship; only compare when both + // logins are resolved (GitHub leaves them null for unmatched email identities, which is not itself a mismatch). + const authorMismatch = + Boolean(authorLogin) && + Boolean(committerLogin) && + authorLogin !== committerLogin; + + // A never-before-seen committer is only a signal when the repo otherwise HAS verified history but THIS author + // has none — a repo with no verified commits at all is simply unsigned, not impersonated. Two bounded history + // queries (author-filtered + repo-wide); either being undeterminable (null) fails safe to no flag. + let newCommitter = false; + if (authorLogin && !options.signal?.aborted) { + const authorVerified = await hasVerifiedHistory( + owner, + repo, + headers, + fetchFn, + authorLogin, + options.signal, + ); + if (authorVerified === false && !options.signal?.aborted) { + const repoVerified = await hasVerifiedHistory( + owner, + repo, + headers, + fetchFn, + undefined, + options.signal, + ); + newCommitter = repoVerified === true; + } + } + + // Nothing noteworthy: a verified head with a matching author and no new-committer signal needs no finding. + if (verified && !authorMismatch && !newCommitter) return []; + + const finding: CommitSignatureFinding = { + verified, + reason, + authorMismatch, + newCommitter, + ...(authorLogin ? { authorLogin } : {}), + }; + return [finding]; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 7550f7db89..e064109aca 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -20,6 +20,7 @@ import { scanCodeowners } from "./analyzers/codeowners.js"; import { scanSecretLog } from "./analyzers/secret-log.js"; import { scanAssetWeight } from "./analyzers/asset-weight.js"; import { scanTyposquat } from "./analyzers/typosquat.js"; +import { scanCommitSignature } from "./analyzers/commit-signature.js"; import { renderBrief } from "./render.js"; import { captureAnalyzerDegradation } from "./sentry.js"; @@ -41,6 +42,7 @@ const ANALYZERS: Record = { secretLog: (req, signal) => scanSecretLog(req, signal), assetWeight: (req, signal) => scanAssetWeight(req, fetch, { signal }), typosquat: (req, signal) => scanTyposquat(req, fetch, { signal }), + commitSignature: (req, signal) => scanCommitSignature(req, fetch, { signal }), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index e9fc157841..43f69775c6 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -254,6 +254,32 @@ export function renderBrief( } } + const commitSignatures = findings.commitSignature ?? []; + if (commitSignatures.length) { + lines.push( + "### Head-commit signature / author provenance (verify before merging)", + ); + for (const item of commitSignatures) { + const status = item.verified + ? "signature **verified**" + : "signature **unverified**"; + const flags: string[] = []; + if (item.authorMismatch) + flags.push("commit author and committer logins differ"); + if (item.newCommitter) + flags.push( + "author has no verified history in a repo that otherwise carries verified commits", + ); + const who = item.authorLogin + ? ` by ${safeCodeSpan(item.authorLogin)}` + : ""; + const detail = flags.length ? ` — ${flags.join("; ")}` : ""; + lines.push( + `- head commit${who}: ${status} (${safeCodeSpan(item.reason)})${detail}`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 70dcf66619..556052f19a 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -162,6 +162,24 @@ export interface TyposquatFinding { reason: string; } +/** A head commit whose signature/author provenance warrants scrutiny: an unsigned/unverified-signature head, an + * author/committer login mismatch, or a never-before-seen committer in a repo that otherwise has verified history + * — supply-chain/impersonation signals the no-checkout reviewer cannot derive. Surfaces ONLY the public GitHub + * verification verdict (`verified` + `reason`) and boolean provenance flags — never tokens, emails, or identities + * beyond the public commit author login GitHub already exposes. (#1517) */ +export interface CommitSignatureFinding { + /** GitHub's signature verification verdict for the head commit. */ + verified: boolean; + /** GitHub's machine-readable verification reason (e.g. `unsigned`, `valid`, `unknown_key`). Public-safe string. */ + reason: string; + /** The head commit author's GitHub login, when GitHub resolves one — public, already shown on the PR. */ + authorLogin?: string; + /** True when the commit author login differs from the committer login (a potential authorship mismatch). */ + authorMismatch: boolean; + /** True when the author login has no prior verified commit in a repo that otherwise carries verified history. */ + newCommitter: boolean; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -177,6 +195,7 @@ export interface BriefFindings { secretLog?: SecretLogFinding[]; assetWeight?: AssetWeightFinding[]; typosquat?: TyposquatFinding[]; + commitSignature?: CommitSignatureFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/commit-signature.test.ts b/review-enrichment/test/commit-signature.test.ts new file mode 100644 index 0000000000..27b2818b55 --- /dev/null +++ b/review-enrichment/test/commit-signature.test.ts @@ -0,0 +1,184 @@ +// Units for the commit-signature / verified-author provenance analyzer (#1517). Kept in its own file (not +// enrichment.test.ts) so concurrent analyzer PRs don't collide on a shared test file. Runs against the +// compiled dist/. All network is mocked — nothing here touches GitHub, so it never flakes offline/in CI. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + fetchHeadCommit, + hasVerifiedHistory, + scanCommitSignature, +} from "../dist/analyzers/commit-signature.js"; +import { renderBrief } from "../dist/render.js"; + +// A minimal Response-like shape (ok + status + json), matching the other analyzer tests. +const jsonResponse = (body, code = 200) => ({ + ok: code >= 200 && code < 300, + status: code, + json: async () => body, +}); + +// Base request: a head commit on a well-formed repo slug with a broker token present. +const req = (overrides = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + headSha: "deadbeef", + githubToken: "ghp_test", + ...overrides, +}); + +// A fetch router: returns the head-commit payload for the commits/{sha} URL, and history payloads for the +// commits?author / repo-wide commits list URLs. `authorHistory`/`repoHistory` are arrays of {verified} booleans. +const routedFetch = ({ head, authorHistory, repoHistory }) => + async (url) => { + if (/\/commits\/[^/?]+$/.test(url)) return jsonResponse(head); + const toCommits = (verifiedFlags) => + verifiedFlags.map((verified) => ({ commit: { verification: { verified } } })); + if (url.includes("author=")) + return jsonResponse(toCommits(authorHistory ?? [])); + return jsonResponse(toCommits(repoHistory ?? [])); + }; + +const verifiedHead = (login = "octo") => ({ + commit: { verification: { verified: true, reason: "valid" } }, + author: { login }, + committer: { login }, +}); + +const throwingFetch = async () => { + throw new Error("network down"); +}; + +test("fetchHeadCommit returns the payload on 200, null on a non-200 or error", async () => { + const head = verifiedHead(); + const ok = await fetchHeadCommit("o", "r", "sha", {}, async () => jsonResponse(head)); + assert.deepEqual(ok, head); + assert.equal(await fetchHeadCommit("o", "r", "sha", {}, async () => jsonResponse({}, 404)), null); + assert.equal(await fetchHeadCommit("o", "r", "sha", {}, throwingFetch), null); + assert.equal(await fetchHeadCommit("o", "r", "sha", {}, async () => jsonResponse(head), AbortSignal.abort()), null); +}); + +test("hasVerifiedHistory: true/false on a definitive page, null otherwise", async () => { + const page = (flags) => async () => jsonResponse(flags.map((v) => ({ commit: { verification: { verified: v } } }))); + assert.equal(await hasVerifiedHistory("o", "r", {}, page([true, false])), true); + assert.equal(await hasVerifiedHistory("o", "r", {}, page([false, false])), false); + assert.equal(await hasVerifiedHistory("o", "r", {}, async () => jsonResponse([], 500)), null); + assert.equal(await hasVerifiedHistory("o", "r", {}, async () => jsonResponse({ message: "x" })), null); // not an array + assert.equal(await hasVerifiedHistory("o", "r", {}, throwingFetch), null); +}); + +test("scanCommitSignature: a verified head with a matching author yields no finding", async () => { + const findings = await scanCommitSignature( + req(), + routedFetch({ head: verifiedHead(), authorHistory: [true], repoHistory: [true] }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: an unsigned head is flagged with its reason", async () => { + const head = { + commit: { verification: { verified: false, reason: "unsigned" } }, + author: { login: "octo" }, + committer: { login: "octo" }, + }; + // Author already has verified history → not a new committer; the finding is the unverified signature itself. + const findings = await scanCommitSignature( + req(), + routedFetch({ head, authorHistory: [true], repoHistory: [true] }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].verified, false); + assert.equal(findings[0].reason, "unsigned"); + assert.equal(findings[0].authorMismatch, false); + assert.equal(findings[0].newCommitter, false); + assert.equal(findings[0].authorLogin, "octo"); +}); + +test("scanCommitSignature: an author/committer login mismatch is flagged even when verified", async () => { + const head = { + commit: { verification: { verified: true, reason: "valid" } }, + author: { login: "octo" }, + committer: { login: "someone-else" }, + }; + const findings = await scanCommitSignature( + req(), + routedFetch({ head, authorHistory: [true], repoHistory: [true] }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].authorMismatch, true); + assert.equal(findings[0].verified, true); +}); + +test("scanCommitSignature: a new committer in a repo with verified history is flagged", async () => { + const head = { + commit: { verification: { verified: false, reason: "unsigned" } }, + author: { login: "newcomer" }, + committer: { login: "newcomer" }, + }; + const findings = await scanCommitSignature( + req(), + // author has no verified commits, but the repo otherwise does → impersonation signal. + routedFetch({ head, authorHistory: [false], repoHistory: [true] }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].newCommitter, true); +}); + +test("scanCommitSignature: an unverified head in a repo with NO verified history is not a new-committer signal", async () => { + const head = { + commit: { verification: { verified: false, reason: "unsigned" } }, + author: { login: "newcomer" }, + committer: { login: "newcomer" }, + }; + const findings = await scanCommitSignature( + req(), + routedFetch({ head, authorHistory: [false], repoHistory: [false] }), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].newCommitter, false); // unsigned, but not impersonation + assert.equal(findings[0].verified, false); +}); + +test("scanCommitSignature fails safe without a token or head SHA", async () => { + assert.deepEqual(await scanCommitSignature(req({ githubToken: undefined }), throwingFetch), []); + assert.deepEqual(await scanCommitSignature(req({ headSha: undefined }), throwingFetch), []); +}); + +test("scanCommitSignature fails safe on a malformed repo slug", async () => { + assert.deepEqual(await scanCommitSignature(req({ repoFullName: "not-a-slug" }), throwingFetch), []); + assert.deepEqual(await scanCommitSignature(req({ repoFullName: "o/r/extra" }), throwingFetch), []); +}); + +test("scanCommitSignature fails safe when the head fetch throws or returns no commit", async () => { + assert.deepEqual(await scanCommitSignature(req(), throwingFetch), []); + assert.deepEqual(await scanCommitSignature(req(), async () => jsonResponse({})), []); +}); + +test("scanCommitSignature stops on an already-aborted signal", async () => { + const findings = await scanCommitSignature(req(), routedFetch({ head: verifiedHead() }), { + signal: AbortSignal.abort(), + }); + assert.deepEqual(findings, []); +}); + +test("renderBrief emits a public-safe commit-signature block", () => { + const { promptSection } = renderBrief({ + commitSignature: [ + { + verified: false, + reason: "unsigned", + authorLogin: "newcomer", + authorMismatch: true, + newCommitter: true, + }, + ], + }); + assert.match(promptSection, /Head-commit signature \/ author provenance/); + assert.match(promptSection, /signature \*\*unverified\*\*/); + assert.match(promptSection, /unsigned/); + assert.match(promptSection, /author and committer logins differ/); + assert.match(promptSection, /no verified history/); + assert.match(promptSection, /newcomer/); + // Public-safe: no token, email, or local path ever appears in the rendered block. + assert.equal(promptSection.includes("ghp_"), false); + assert.equal(promptSection.includes("@"), false); +}); From ce164e5aaf01f7ecd800e9988011a2a4ff47b31d Mon Sep 17 00:00:00 2001 From: GildardoDev <267998055+GildardoDev@users.noreply.github.com> Date: Mon, 29 Jun 2026 15:36:24 -0500 Subject: [PATCH 2/2] fix(enrichment): reject non owner/repo slugs before any GitHub request in the commit-signature analyzer --- .../src/analyzers/commit-signature.ts | 5 ++++- review-enrichment/test/commit-signature.test.ts | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/review-enrichment/src/analyzers/commit-signature.ts b/review-enrichment/src/analyzers/commit-signature.ts index c9210f8af6..acc6bfc527 100644 --- a/review-enrichment/src/analyzers/commit-signature.ts +++ b/review-enrichment/src/analyzers/commit-signature.ts @@ -100,10 +100,13 @@ export async function scanCommitSignature( const { repoFullName, githubToken, headSha } = req; if (!githubToken || !headSha) return []; + // Require EXACTLY `owner/repo`. A 3+ segment value like `o/r/extra` would otherwise keep parts[0]/parts[1] + // and silently query the wrong repository (`o/r`) instead of failing safe, so reject anything that is not a + // clean two-segment slug before building any GitHub URL. const parts = repoFullName.split("/"); const owner = parts[0]; const repo = parts[1]; - if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; const headers = githubHeaders(githubToken); const head = await fetchHeadCommit( diff --git a/review-enrichment/test/commit-signature.test.ts b/review-enrichment/test/commit-signature.test.ts index 27b2818b55..c5491dab20 100644 --- a/review-enrichment/test/commit-signature.test.ts +++ b/review-enrichment/test/commit-signature.test.ts @@ -143,9 +143,19 @@ test("scanCommitSignature fails safe without a token or head SHA", async () => { assert.deepEqual(await scanCommitSignature(req({ headSha: undefined }), throwingFetch), []); }); -test("scanCommitSignature fails safe on a malformed repo slug", async () => { - assert.deepEqual(await scanCommitSignature(req({ repoFullName: "not-a-slug" }), throwingFetch), []); - assert.deepEqual(await scanCommitSignature(req({ repoFullName: "o/r/extra" }), throwingFetch), []); +test("scanCommitSignature fails closed on a malformed repo slug WITHOUT any network call", async () => { + // A spy that records invocation: a malformed slug must be rejected BEFORE any GitHub request, so the guard + // can never query the wrong repository. (A throwing fetch would be swallowed by the analyzer's fail-safe + // try/catch and could mask a slug that slipped through, so assert the call never happens instead.) + for (const repoFullName of ["not-a-slug", "o/r/extra", "/r", "o/", "a/b/c/d"]) { + let called = false; + const spyFetch: typeof fetch = async () => { + called = true; + return jsonResponse({}); + }; + assert.deepEqual(await scanCommitSignature(req({ repoFullName }), spyFetch), [], `${repoFullName} must yield no finding`); + assert.equal(called, false, `${repoFullName} must not trigger any GitHub request`); + } }); test("scanCommitSignature fails safe when the head fetch throws or returns no commit", async () => {