diff --git a/review-enrichment/src/analyzers/commit-signature.ts b/review-enrichment/src/analyzers/commit-signature.ts new file mode 100644 index 0000000000..13340b870c --- /dev/null +++ b/review-enrichment/src/analyzers/commit-signature.ts @@ -0,0 +1,108 @@ +// Commit-signature / verified-author provenance analyzer (#1517). Detects two supply-chain signals that the +// no-checkout reviewer cannot assess on their own: +// "unsigned" — the PR head commit is not signed/verified by GitHub +// (commit.verification.verified = false; reason exposed as context). +// "new-committer" — the committing author has no prior commits in this repo, yet the repo's recent history +// is ≥80% verified-commit signed — a potential impersonation/injection vector. +// Network: two or three GitHub REST calls (head commit, recent repo commits, author history) under the shared +// AbortSignal timeout. Fail-safe: returns [] on any error or missing prerequisite. +import type { EnrichRequest, CommitSignatureFinding } from "../types.js"; + +const MAX_HISTORY_COMMITS = 20; +const MIN_HISTORY_FOR_PATTERN = 3; // need ≥ this many non-head commits to infer the repo's signing pattern +const VERIFIED_RATIO_THRESHOLD = 0.8; // only flag new-committer when ≥80% of recent commits are verified + +// Allowlists to prevent path traversal when these values are interpolated into API URL paths. +const SHA_RE = /^[a-f0-9]{7,40}$/i; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // must start with alphanumeric — rejects ".." and other dot-only traversal segments + +interface GitHubCommit { + sha: string; + commit: { + verification: { + verified: boolean; + reason: string; + }; + }; + author: { login: string } | null; +} + +/** Fetch head-commit verification status, then optionally check for never-before-seen committers. */ +export async function scanCommitSignature( + req: EnrichRequest, + fetchFn: typeof fetch, + opts?: { signal?: AbortSignal }, +): Promise { + const { repoFullName, headSha, githubToken } = req; + if (!githubToken || !headSha) return []; + + if (!SHA_RE.test(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: Record = { + Authorization: `Bearer ${githubToken}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; + + // Phase 1: fetch the head commit and check its verification status. + let headCommit: GitHubCommit; + try { + const resp = await fetchFn( + `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(headSha)}`, + { headers, signal: opts?.signal }, + ); + if (!resp.ok) return []; + headCommit = (await resp.json()) as GitHubCommit; + } catch { + return []; + } + + const authorLogin = headCommit.author?.login ?? null; + const verification = headCommit.commit.verification; + + if (!verification.verified) { + return [{ headSha, authorLogin, kind: "unsigned", reason: verification.reason }]; + } + + // Phase 2: check whether this is a new committer in a repo with a verified-commit pattern. + // Skip the check when the author identity is unknown (no GitHub user linked to the commit email). + if (!authorLogin) return []; + + try { + const recentResp = await fetchFn( + `https://api.github.com/repos/${owner}/${repo}/commits?per_page=${MAX_HISTORY_COMMITS}`, + { headers, signal: opts?.signal }, + ); + if (!recentResp.ok) return []; + const recentCommits = (await recentResp.json()) as GitHubCommit[]; + + // Exclude the current head from history so the ratio reflects the pre-existing signing pattern. + const others = recentCommits.filter((c) => c.sha !== headSha); + if (others.length < MIN_HISTORY_FOR_PATTERN) return []; + + const verifiedCount = others.filter((c) => c.commit.verification.verified).length; + if (verifiedCount / others.length < VERIFIED_RATIO_THRESHOLD) return []; + + // Repo uses verified commits — does this author have any prior commits here? + const authorHistoryResp = await fetchFn( + `https://api.github.com/repos/${owner}/${repo}/commits?author=${encodeURIComponent(authorLogin)}&per_page=3`, + { headers, signal: opts?.signal }, + ); + if (!authorHistoryResp.ok) return []; + const authorHistory = (await authorHistoryResp.json()) as { sha: string }[]; + + const priorCommits = authorHistory.filter((c) => c.sha !== headSha); + if (priorCommits.length === 0) { + return [{ headSha, authorLogin, kind: "new-committer", reason: null }]; + } + } catch { + return []; + } + + return []; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index ccb30f107b..f573f38698 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -14,6 +14,7 @@ import { scanInstallScripts } from "./analyzers/install-scripts.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; +import { scanCommitSignature } from "./analyzers/commit-signature.js"; import { scanCodeowners } from "./analyzers/codeowners.js"; import { scanSecretLog } from "./analyzers/secret-log.js"; import { renderBrief } from "./render.js"; @@ -29,6 +30,7 @@ const ANALYZERS: Record = { actionPin: (req) => scanActionPins(req), eol: (req) => scanEol(req), redos: (req) => scanRedos(req), + commitSignature: (req, signal) => scanCommitSignature(req, fetch, { signal }), codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }), secretLog: (req, signal) => scanSecretLog(req, signal), }; diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 5270f795e0..a7db50020f 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -129,6 +129,27 @@ export function renderBrief( } } + const commitSigs = findings.commitSignature ?? []; + if (commitSigs.length) { + lines.push("### Commit-signature / verified-author provenance"); + for (const item of commitSigs) { + const sha = safeCodeSpan(item.headSha.slice(0, 8)); + if (item.kind === "unsigned") { + const reason = safeCodeSpan(item.reason ?? "unknown"); + lines.push( + `- ${sha} — commit is not signed or verified (reason: ${reason}); sign commits via gpg or ssh`, + ); + } else { + const who = item.authorLogin + ? safeCodeSpan(item.authorLogin) + : "the commit author"; + lines.push( + `- ${sha} — ${who} is a first-time committer in this repository, which otherwise uses verified commits (supply-chain risk: potential impersonation)`, + ); + } + } + } + const codeownersViolations = findings.codeowners ?? []; if (codeownersViolations.length) { const allOwners = new Set(codeownersViolations.flatMap((f) => f.owners)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 0136d320af..41708927ee 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -93,6 +93,17 @@ export interface RedosFinding { pattern: string; } +/** A commit-signature or verified-author provenance signal (#1517). + * "unsigned": head commit is not signed/verified by GitHub. + * "new-committer": the author has no prior commits in a repo that otherwise uses verified commits + * (supply-chain / impersonation risk). */ +export interface CommitSignatureFinding { + headSha: string; + authorLogin: string | null; + kind: "unsigned" | "new-committer"; + reason: string | null; +} + /** A changed file governed by a CODEOWNERS rule where the PR author is not listed as an owner (#1515). * The blast radius (distinct ownership domains crossed) is derived at render time from the full findings set. */ export interface CodeownersFinding { @@ -118,6 +129,7 @@ export interface BriefFindings { installScript?: InstallScriptFinding[]; eol?: EolFinding[]; redos?: RedosFinding[]; + commitSignature?: CommitSignatureFinding[]; codeowners?: CodeownersFinding[]; secretLog?: SecretLogFinding[]; } diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 94c475f4d4..59a9466acf 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -21,6 +21,7 @@ import { scanPatchForRedos, scanRedos, } from "../dist/analyzers/redos.js"; +import { scanCommitSignature } from "../dist/analyzers/commit-signature.js"; import { findOwners, parseCodeowners, @@ -1034,6 +1035,244 @@ test("buildBrief: eol analyzer runs (real now, 2023 cycle is past)", async () => } }); +// ── scanCommitSignature ────────────────────────────────────────────────────── + +const VERIFIED_COMMIT = { + sha: "abc1234abc1234ab", + commit: { verification: { verified: true, reason: "valid" } }, + author: { login: "alice" }, +}; +const UNSIGNED_COMMIT = { + sha: "abc1234abc1234ab", + commit: { verification: { verified: false, reason: "unsigned" } }, + author: { login: "alice" }, +}; +const REPO_VERIFIED_HISTORY = Array.from({ length: 5 }, (_, i) => ({ + sha: `old${i}`, + commit: { verification: { verified: true, reason: "valid" } }, + author: { login: "bob" }, +})); + +const sigReq = (overrides = {}) => ({ + repoFullName: "owner/repo", + prNumber: 1, + headSha: "abc1234abc1234ab", + githubToken: "tok", + ...overrides, +}); + +function makeSigFetch(headCommit, recentCommits = [], authorHistory = []) { + return async (url) => { + const u = String(url); + if (u.includes("/commits/abc1234abc1234ab")) + return { ok: true, json: async () => headCommit }; + if (u.includes("/commits?author=")) + return { ok: true, json: async () => authorHistory }; + if (u.includes("/commits?per_page=")) + return { ok: true, json: async () => recentCommits }; + return { ok: false, json: async () => ({}) }; + }; +} + +test("scanCommitSignature: returns [] when githubToken is absent", async () => { + const findings = await scanCommitSignature( + sigReq({ githubToken: undefined }), + async () => { throw new Error("should not fetch"); }, + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] when headSha is absent", async () => { + const findings = await scanCommitSignature( + sigReq({ headSha: undefined }), + async () => { throw new Error("should not fetch"); }, + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] on non-ok head commit response", async () => { + const findings = await scanCommitSignature( + sigReq(), + async () => ({ ok: false, json: async () => ({}) }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] on network error (fail-safe)", async () => { + const findings = await scanCommitSignature(sigReq(), async () => { + throw new Error("network down"); + }); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: flags unsigned commit with its reason", async () => { + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(UNSIGNED_COMMIT), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "unsigned"); + assert.equal(findings[0].reason, "unsigned"); + assert.equal(findings[0].authorLogin, "alice"); + assert.equal(findings[0].headSha, "abc1234abc1234ab"); +}); + +test("scanCommitSignature: returns [] for verified commit when author login is null", async () => { + const noLoginCommit = { + sha: "abc1234abc1234ab", + commit: { verification: { verified: true, reason: "valid" } }, + author: null, + }; + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(noLoginCommit), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] when recent commits response is non-ok", async () => { + const fetch = async (url) => { + const u = String(url); + if (u.includes("/commits/abc1234abc1234ab")) + return { ok: true, json: async () => VERIFIED_COMMIT }; + return { ok: false, json: async () => ({}) }; + }; + const findings = await scanCommitSignature(sigReq(), fetch); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] when repo has insufficient commit history", async () => { + // Only 2 non-head commits — below MIN_HISTORY_FOR_PATTERN (3) + const twoCommits = [ + { sha: "old0", commit: { verification: { verified: true, reason: "valid" } }, author: { login: "bob" } }, + { sha: "old1", commit: { verification: { verified: true, reason: "valid" } }, author: { login: "bob" } }, + ]; + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(VERIFIED_COMMIT, twoCommits), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] when repo history is not mostly verified (<80%)", async () => { + // 3 commits, only 1 verified (33%) + const mixedHistory = [ + { sha: "old0", commit: { verification: { verified: true, reason: "valid" } }, author: { login: "bob" } }, + { sha: "old1", commit: { verification: { verified: false, reason: "unsigned" } }, author: { login: "bob" } }, + { sha: "old2", commit: { verification: { verified: false, reason: "unsigned" } }, author: { login: "bob" } }, + ]; + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(VERIFIED_COMMIT, mixedHistory), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] when author history response is non-ok", async () => { + const fetch = async (url) => { + const u = String(url); + if (u.includes("/commits/abc1234abc1234ab")) + return { ok: true, json: async () => VERIFIED_COMMIT }; + if (u.includes("/commits?per_page=")) + return { ok: true, json: async () => REPO_VERIFIED_HISTORY }; + return { ok: false, json: async () => ({}) }; + }; + const findings = await scanCommitSignature(sigReq(), fetch); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: flags new-committer in verified repo when author has no prior commits", async () => { + // authorHistory returns empty list → alice has no prior commits + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(VERIFIED_COMMIT, REPO_VERIFIED_HISTORY, []), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "new-committer"); + assert.equal(findings[0].authorLogin, "alice"); + assert.equal(findings[0].reason, null); +}); + +test("scanCommitSignature: returns [] for known committer (has prior commits in repo)", async () => { + // authorHistory contains a prior commit (different SHA) + const priorCommit = { sha: "prior000" }; + const findings = await scanCommitSignature( + sigReq(), + makeSigFetch(VERIFIED_COMMIT, REPO_VERIFIED_HISTORY, [priorCommit]), + ); + assert.deepEqual(findings, []); +}); + +test("scanCommitSignature: returns [] on phase-2 network error (fail-safe)", async () => { + let calls = 0; + const fetch = async (url) => { + const u = String(url); + if (u.includes("/commits/abc1234abc1234ab")) + return { ok: true, json: async () => VERIFIED_COMMIT }; + calls++; + throw new Error("network down"); + }; + const findings = await scanCommitSignature(sigReq(), fetch); + assert.deepEqual(findings, []); + assert.ok(calls >= 1, "phase 2 did attempt a fetch before throwing"); +}); + +test("renderBrief: renders unsigned-commit finding with sha and reason", () => { + const r = renderBrief({ + commitSignature: [ + { headSha: "deadbeef0011", authorLogin: "alice", kind: "unsigned", reason: "unsigned" }, + ], + }); + assert.match(r.promptSection, /Commit-signature/); + assert.match(r.promptSection, /`deadbeef`/); + assert.match(r.promptSection, /not signed or verified/); + assert.match(r.promptSection, /`unsigned`/); +}); + +test("renderBrief: renders new-committer finding with sha and login", () => { + const r = renderBrief({ + commitSignature: [ + { headSha: "cafe1234abcd", authorLogin: "eve", kind: "new-committer", reason: null }, + ], + }); + assert.match(r.promptSection, /Commit-signature/); + assert.match(r.promptSection, /`cafe1234`/); + assert.match(r.promptSection, /`eve`.*first-time committer|first-time committer.*`eve`/); + assert.match(r.promptSection, /supply-chain risk/); +}); + +test("renderBrief: renders new-committer finding when authorLogin is null", () => { + const r = renderBrief({ + commitSignature: [ + { headSha: "aabb1234ccdd", authorLogin: null, kind: "new-committer", reason: null }, + ], + }); + assert.match(r.promptSection, /the commit author.*first-time committer|first-time committer/); +}); + +test("buildBrief: commitSignature analyzer is wired into the orchestrator", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => { + const u = String(url); + if (u.includes("/commits/abc123")) + return { ok: true, json: async () => UNSIGNED_COMMIT }; + return { ok: true, json: async () => ({}) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "owner/repo", + prNumber: 1, + headSha: "abc1234abc1234ab", + githubToken: "tok", + }); + assert.equal(brief.analyzerStatus.commitSignature, "ok"); + assert.equal(brief.findings.commitSignature.length, 1); + assert.equal(brief.findings.commitSignature[0].kind, "unsigned"); + } finally { + globalThis.fetch = realFetch; + } +}); + test("codeOnly: blanks string messages, keeps ${...} interpolation bodies", () => { assert.equal(codeOnly('"a secret here"'), " "); assert.equal(codeOnly("'plain'"), " "); @@ -1181,6 +1420,36 @@ test("renderBrief: renders the secret-log block, code-spanning + sanitizing", () assert.match(r.promptSection, /a secret\/credential/); }); +test("buildBrief: commitSignature analyzer runs and reports unsigned commit", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => { + if (String(url).includes("/commits/abc1234abc1234ab")) + return { + ok: true, + json: async () => ({ + sha: "abc1234abc1234ab", + commit: { verification: { verified: false, reason: "unsigned" } }, + author: { login: "alice" }, + }), + }; + return { ok: true, json: async () => ({}) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + headSha: "abc1234abc1234ab", + githubToken: "tok", + }); + assert.equal(brief.analyzerStatus.commitSignature, "ok"); + assert.equal(brief.findings.commitSignature.length, 1); + assert.equal(brief.findings.commitSignature[0].kind, "unsigned"); + assert.match(brief.promptSection, /Commit-signature/); + } finally { + globalThis.fetch = realFetch; + } +}); + test("buildBrief: secret-log analyzer runs (pure, no network)", async () => { const realFetch = globalThis.fetch; globalThis.fetch = async () => ({ ok: true, json: async () => ({}) }); @@ -1202,3 +1471,37 @@ test("buildBrief: secret-log analyzer runs (pure, no network)", async () => { globalThis.fetch = realFetch; } }); + +test("scanCommitSignature: returns [] when headSha fails SHA format validation (path traversal guard)", async () => { + let fetched = false; + const fakeFetch = async () => { fetched = true; return { ok: true, json: async () => ({}) }; }; + for (const badSha of ["../../../evil", "refs/heads/main", "HEAD~1", "", "xyz!@#"]) { + const findings = await scanCommitSignature(sigReq({ headSha: badSha }), fakeFetch); + assert.deepEqual(findings, [], `expected [] for headSha=${JSON.stringify(badSha)}`); + } + assert.equal(fetched, false, "no fetch should occur for invalid headSha"); +}); + +test("scanCommitSignature: returns [] when owner or repo segment is a path traversal sequence", async () => { + let fetched = false; + const fakeFetch = async () => { fetched = true; return { ok: true, json: async () => ({}) }; }; + // "../evil/repo" → owner="..", repo="evil" — ".." starts with '.' so SLUG_RE rejects it + // "owner/../commits" → repo=".." — same + // "./sneaky/repo" → owner="." — "." starts with '.', SLUG_RE rejects it + for (const badRepo of ["../evil/repo", "owner/../commits", "./sneaky/repo"]) { + const findings = await scanCommitSignature(sigReq({ repoFullName: badRepo }), fakeFetch); + assert.deepEqual(findings, [], `expected [] for repoFullName=${JSON.stringify(badRepo)}`); + } + assert.equal(fetched, false, "no fetch should occur for invalid owner/repo"); +}); + +test("scanCommitSignature: valid short SHA (7 chars) passes format check and proceeds to fetch", async () => { + let didFetch = false; + const fetch = async () => { + didFetch = true; + return { ok: false, json: async () => ({}) }; + }; + const findings = await scanCommitSignature(sigReq({ headSha: "abc1234" }), fetch); + assert.deepEqual(findings, []); + assert.ok(didFetch, "a valid 7-char SHA should reach the fetch"); +});