diff --git a/.env.example b/.env.example index 115b0b9d8d..275a4858d3 100644 --- a/.env.example +++ b/.env.example @@ -63,26 +63,26 @@ GITTENSORY_REVIEW_ENRICHMENT=false # # Unknown names warn and are ignored; a typo-only list runs no analyzers. # BEGIN GENERATED REES ANALYZERS # Current analyzer names: -# dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos -# provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild -# history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals -# undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety -# looseRange,terminology,todoMarker,magicNumber +# dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin +# workflowInjection,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot +# blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene +# pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # # Profile defaults: -# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol -# redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio,migrationSafety -# looseRange,terminology,todoMarker,magicNumber +# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin +# workflowInjection,eol,redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio +# migrationSafety,looseRange,terminology,todoMarker,magicNumber # balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency -# actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature -# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink -# approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene +# actionPin,workflowInjection,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot +# blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene +# pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber +# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin +# workflowInjection,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot +# blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol -# redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig -# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity -# ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio -# migrationSafety,looseRange,terminology,todoMarker,magicNumber # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index d622de7867..6857c461af 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -236,6 +236,27 @@ export const REES_ANALYZERS = [ notes: "Official actions/* and github/* actions are excluded to keep the signal focused.", }, }, + { + name: "workflowInjection", + title: "Workflow-injection / pwn-request risk", + category: "security", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: {}, + docs: { + summary: + "Detects GitHub Actions pwn-request risk: a pull_request_target/workflow_run workflow that runs elevated CI against attacker-controlled input.", + looksAt: + "Added/context lines in .github/workflows YAML patches: the trigger, an untrusted-ref checkout, run: steps that interpolate event fields, and the top-level permissions block.", + reports: + "Workflow file, line, and rule kind (untrusted checkout, unsafe interpolation, or missing permissions).", + network: "Pure local analyzer. No external network call.", + notes: + "Diff-only and file-scoped: an environment/permissions gate already present outside the diff's visible context is not seen, and job/step correlation is not performed. Never flags a plain pull_request trigger.", + }, + }, { name: "eol", title: "End-of-life runtimes", diff --git a/review-enrichment/README.md b/review-enrichment/README.md index 415fb409dc..78f8665913 100644 --- a/review-enrichment/README.md +++ b/review-enrichment/README.md @@ -38,6 +38,7 @@ inside the operator's trust boundary. The engine prefers a short-lived installat | `license` | Copyleft or unknown dependency licenses. | Calls deps.dev. | | `installScript` | npm packages that run install lifecycle hooks. | Calls the npm registry. | | `actionPin` | Third-party GitHub Actions pinned to mutable refs. | Pure local. | +| `workflowInjection` | GitHub Actions pwn-request/workflow-injection risk: an elevated-trust trigger checking out untrusted refs, unsafe `run:` interpolation, or missing `permissions:`. | Pure local. | | `eol` | Runtime/base-image pins that are EOL or close to EOL. | Calls endoflife.date. | | `redos` | Regex literals with catastrophic-backtracking structure. | Pure local. | | `provenance` | Missing package attestations plus binary/vendored/minified additions. | Calls npm/PyPI for attestations; path checks are local. | diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index fd2fc254ce..4aad737574 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -259,6 +259,29 @@ "notes": "Official actions/* and github/* actions are excluded to keep the signal focused." } }, + { + "name": "workflowInjection", + "title": "Workflow-injection / pwn-request risk", + "category": "security", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": {}, + "docs": { + "summary": "Detects GitHub Actions pwn-request risk: a pull_request_target/workflow_run workflow that runs elevated CI against attacker-controlled input.", + "looksAt": "Added/context lines in .github/workflows YAML patches: the trigger, an untrusted-ref checkout, run: steps that interpolate event fields, and the top-level permissions block.", + "reports": "Workflow file, line, and rule kind (untrusted checkout, unsafe interpolation, or missing permissions).", + "network": "Pure local analyzer. No external network call.", + "notes": "Diff-only and file-scoped: an environment/permissions gate already present outside the diff's visible context is not seen, and job/step correlation is not performed. Never flags a plain pull_request trigger." + } + }, { "name": "eol", "title": "End-of-life runtimes", diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 516784b8f3..676f213e0b 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -32,6 +32,7 @@ import { scanTerminology } from "./terminology.js"; import { scanTodoMarker } from "./todo-marker.js"; import { scanTyposquat } from "./typosquat.js"; import { scanUndocumentedExport } from "./undocumented-export.js"; +import { scanWorkflowInjection } from "./workflow-injection.js"; import type { AnalyzerDescriptor, AnalyzerFn, @@ -149,6 +150,25 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req) => scanActionPins(req), }), + descriptor({ + name: "workflowInjection", + title: "Workflow-injection / pwn-request risk", + category: "security", + cost: "local", + defaultEnabled: true, + requires: ["files"], + docs: { + summary: + "Detects GitHub Actions pwn-request risk: a pull_request_target/workflow_run workflow that runs elevated CI against attacker-controlled input.", + looksAt: + "Added/context lines in .github/workflows YAML patches: the trigger, an untrusted-ref checkout, run: steps that interpolate event fields, and the top-level permissions block.", + reports: "Workflow file, line, and rule kind (untrusted checkout, unsafe interpolation, or missing permissions).", + network: "Pure local analyzer. No external network call.", + notes: + "Diff-only and file-scoped: an environment/permissions gate already present outside the diff's visible context is not seen, and job/step correlation is not performed. Never flags a plain pull_request trigger.", + }, + run: (req) => scanWorkflowInjection(req), + }), descriptor({ name: "eol", title: "End-of-life runtimes", diff --git a/review-enrichment/src/analyzers/workflow-injection.ts b/review-enrichment/src/analyzers/workflow-injection.ts new file mode 100644 index 0000000000..c458758aeb --- /dev/null +++ b/review-enrichment/src/analyzers/workflow-injection.ts @@ -0,0 +1,177 @@ +// GitHub Actions workflow-injection / "pwn request" analyzer. Scans changed .github/workflows/* patches for the +// classic trust-boundary break: a workflow that runs with an ELEVATED token/context against ATTACKER-CONTROLLED +// input. `pull_request_target` and `workflow_run` both run in the base repo's context (secrets + a +// read/write token) even for a fork PR, unlike plain `pull_request` (fork PRs get a read-only token there, so +// it is never flagged). Pure compute, line-scanned from the diff — no repo checkout, same shape as the +// actions-pin/iac-misconfig siblings. +import type { EnrichRequest, WorkflowInjectionFinding } from "../types.js"; +import { isWorkflowPath } from "../workflow-path.js"; + +const MAX_FINDINGS = 25; +const MAX_LINE_CHARS = 2000; + +const HUNK_RE = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/; + +// The two triggers that hand a fork PR's event payload to a workflow running with base-repo privilege. Word +// boundaries keep `pull_request_target` from also matching the (safe) plain `pull_request` trigger. +const UNSAFE_TRIGGER_RE = /\b(?:pull_request_target|workflow_run)\b/; +const PULL_REQUEST_TARGET_RE = /\bpull_request_target\b/; + +// A job-level GitHub Environment (`environment:` key) requires manual approval before the job runs, which is the +// standard mitigation for a `pull_request_target` checkout of untrusted code. Its presence anywhere in the diff's +// visible lines is treated as evidence of an approval gate (a coarse, file-scoped signal — see docs.notes). +const ENVIRONMENT_KEY_RE = /^\s*environment:\s*(?:\S.*)?$/; +// A top-level (or job-level) `permissions:` key narrows the default `GITHUB_TOKEN` scope. Its absence from the +// diff's visible lines is the signal for rule 3 (see docs.notes for the same diff-only caveat). +const PERMISSIONS_KEY_RE = /^\s*permissions:\s*(?:\S.*)?$/; + +// A `ref:` step input bound to the untrusted PR head — the checkout step that actually pulls in attacker-authored +// code onto a runner that otherwise has base-repo secrets and a writable token. +const UNTRUSTED_REF_RE = + /^\s*ref:\s*["']?\$\{\{\s*(?:github\.event\.pull_request\.head\.(?:sha|ref|label)|github\.head_ref)\s*\}\}/; + +// A `run:` step key — either as a step's own key (` run: ...`) or as the FIRST key of a step's YAML list item +// (` - run: ...`) — optionally introducing a block scalar (`|`/`>`, with an optional chomp/indent indicator and +// digit). Group 1 is the column `run:` itself starts at (the list-item dash, when present, is absorbed into it so +// it lines up with where a sibling key like `if:`/`shell:` would be written), group 2 the block indicator +// (undefined for an inline scalar), group 3 any inline content on the same line. +const RUN_BLOCK_START_RE = /^(\s*(?:-\s+)?)run:\s*([|>][+-]?\d*)?\s*(.*)$/; +// The first non-whitespace column of a line, used to tell whether a line still belongs to a `run:` block scalar +// (deeper indent than the `run:` key) or has closed it (same-or-shallower indent, i.e. a sibling key or a new step). +const FIRST_COLUMN_RE = /^(\s*)\S/; + +// Untrusted event fields commonly interpolated straight into a shell string. The safe pattern routes each of +// these through an `env:` entry first (`TITLE: ${{ github.event.pull_request.title }}`) and references the shell +// variable in `run:` (`echo "$TITLE"`) — that never matches this pattern because the `${{ }}` never appears +// inside the `run:` block itself. +const UNSAFE_EVENT_FIELD_RE = + /\$\{\{\s*(?:github\.event\.pull_request\.(?:title|body|head\.ref|head\.label)|github\.event\.issue\.(?:title|body)|github\.event\.comment\.body|github\.head_ref)\s*\}\}/; + +/** + * Scan one workflow file's patch for injection/pwn-request risk, line-cited via hunk headers. Pure — never + * parses the YAML into a document, so there is nothing for malformed YAML to fail to parse; a file with no + * recognizable elevated-trust trigger (or pure noise) simply yields no findings. + */ +export function scanPatchForWorkflowInjection( + path: string, + patch: string, + maxFindings = MAX_FINDINGS, +): WorkflowInjectionFinding[] { + if (maxFindings <= 0) return []; + + const findings: WorkflowInjectionFinding[] = []; + const untrustedCheckoutLines: number[] = []; + + let newLine = 0; + let inHunk = false; + let inRunBlock = false; + let runBlockIndent = -1; + let hasUnsafeTrigger = false; + let hasPullRequestTarget = false; + let pullRequestTargetLine = 0; + let hasEnvironmentGate = false; + let hasPermissions = false; + + for (const rawLine of patch.split("\n")) { + const hunk = HUNK_RE.exec(rawLine); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + inRunBlock = false; + runBlockIndent = -1; + continue; + } + // Skip pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header. + if (!inHunk) continue; + + // A `-` removed line is old-file-only content: it does not exist in the new file, so it neither advances + // the new-file line counter nor contributes to trigger/gate/interpolation detection. `\` marks + // "No newline at end of file" and is likewise not a content line (same class as the actions-pin/iac-misconfig + // fix for this). + if (rawLine.startsWith("-") || rawLine.startsWith("\\")) continue; + + const isAdded = rawLine.startsWith("+"); + // Both an added line (`+`) and an unchanged context line (leading space) represent real NEW-file content — + // context lines matter here because the elevated trigger or an existing `permissions:`/`environment:` key is + // often untouched while the PR only adds a new step nearby (the diff's surrounding context still shows it). + const body = rawLine.slice(1); + + if (body.length > MAX_LINE_CHARS) { + newLine++; + continue; + } + + if (UNSAFE_TRIGGER_RE.test(body)) hasUnsafeTrigger = true; + if (PULL_REQUEST_TARGET_RE.test(body)) { + hasPullRequestTarget = true; + pullRequestTargetLine = newLine; + } + if (ENVIRONMENT_KEY_RE.test(body)) hasEnvironmentGate = true; + if (PERMISSIONS_KEY_RE.test(body)) hasPermissions = true; + + const runStart = RUN_BLOCK_START_RE.exec(body); + if (runStart) { + const indent = runStart[1]!.length; + // Group 3 (`(.*)$`) is not optional, so it always participates in a successful match (possibly as ""); the + // non-null assertion just tells TS what `noUncheckedIndexedAccess` cannot infer on its own. + const inline = runStart[3]!; + if (isAdded && inline && UNSAFE_EVENT_FIELD_RE.test(inline)) { + findings.push({ file: path, line: newLine, kind: "unsafe-interpolation" }); + } + inRunBlock = Boolean(runStart[2]); + runBlockIndent = indent; + } else if (inRunBlock) { + const firstColumn = FIRST_COLUMN_RE.exec(body); + const indent = firstColumn ? firstColumn[1]!.length : Number.POSITIVE_INFINITY; + if (indent <= runBlockIndent) { + inRunBlock = false; + runBlockIndent = -1; + } else if (isAdded && UNSAFE_EVENT_FIELD_RE.test(body)) { + findings.push({ file: path, line: newLine, kind: "unsafe-interpolation" }); + } + } + + if (isAdded && UNTRUSTED_REF_RE.test(body)) { + untrustedCheckoutLines.push(newLine); + } + + newLine++; + } + + if (!hasUnsafeTrigger) return []; + + if (hasPullRequestTarget && !hasPermissions) { + // pullRequestTargetLine is only ever set while processing a real content line inside a hunk (the loop above + // never tests PULL_REQUEST_TARGET_RE outside that guard), so hasPullRequestTarget can never be true here with + // pullRequestTargetLine still at its 0 initial value. + findings.push({ + file: path, + line: pullRequestTargetLine, + kind: "missing-permissions", + }); + } + + if (untrustedCheckoutLines.length && !hasEnvironmentGate) { + for (const line of untrustedCheckoutLines) { + findings.push({ file: path, line, kind: "untrusted-checkout" }); + } + } + + return findings.slice(0, maxFindings); +} + +/** Analyzer entrypoint: scan every changed workflow file for injection/pwn-request risk. */ +export async function scanWorkflowInjection( + req: EnrichRequest, +): Promise { + const findings: WorkflowInjectionFinding[] = []; + for (const file of req.files ?? []) { + if (!isWorkflowPath(file.path) || !file.patch) continue; + const remaining = MAX_FINDINGS - findings.length; + if (remaining <= 0) break; + findings.push( + ...scanPatchForWorkflowInjection(file.path, file.patch, remaining), + ); + } + return findings; +} diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index df05642f19..3b85441c3d 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -111,6 +111,30 @@ export function renderBrief( } } + const workflowInjections = findings.workflowInjection ?? []; + if (workflowInjections.length) { + const explainInjection = ( + kind: (typeof workflowInjections)[number]["kind"], + ): string => { + switch (kind) { + case "untrusted-checkout": + return "checks out the untrusted PR head under an elevated-trust trigger with no environment approval gate"; + case "unsafe-interpolation": + return "interpolates an untrusted event field directly into a shell step; route it through `env:` first"; + case "missing-permissions": + return "has no top-level `permissions:` block, so the workflow keeps the default broad token permissions"; + } + }; + lines.push( + "### GitHub Actions workflow-injection / pwn-request risk (review before merging)", + ); + for (const item of workflowInjections) { + lines.push( + `- ${safeCodeSpan(`${item.file}:${item.line}`)} — ${explainInjection(item.kind)}`, + ); + } + } + const eol = findings.eol ?? []; if (eol.length) { lines.push("### End-of-life runtimes (upgrade before merging)"); diff --git a/review-enrichment/src/scheduler.ts b/review-enrichment/src/scheduler.ts index 4395c3e4c3..d3726ace59 100644 --- a/review-enrichment/src/scheduler.ts +++ b/review-enrichment/src/scheduler.ts @@ -356,6 +356,7 @@ function inputSkipReason( ? null : "no_lockfile"; case "actionPin": + case "workflowInjection": return analysis.fileCategories.some((file) => file.category === "workflow") ? null : "no_workflow"; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 175aecb10c..56c94cda1e 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -119,6 +119,18 @@ export interface ActionPinFinding { ref: string; } +/** A GitHub Actions "pwn request" / workflow-injection trust-boundary risk: a workflow that runs with elevated + * privilege against attacker-controlled input. `untrusted-checkout`: a `pull_request_target`/`workflow_run` + * workflow checks out the untrusted PR head with no environment gate. `unsafe-interpolation`: a `run:` step + * interpolates an untrusted event field directly instead of routing it through `env:` first. `missing-permissions`: + * a `pull_request_target` workflow declares no top-level `permissions:` block, so it keeps the default broad + * token permissions. */ +export interface WorkflowInjectionFinding { + file: string; + line: number; + kind: "untrusted-checkout" | "unsafe-interpolation" | "missing-permissions"; +} + /** A runtime/base-image/engine pinned to a release that is past end-of-support (or EOL within 90 days). */ export interface EolFinding { file: string; @@ -454,6 +466,7 @@ export interface BriefFindings { secret?: SecretFinding[]; license?: LicenseFinding[]; actionPin?: ActionPinFinding[]; + workflowInjection?: WorkflowInjectionFinding[]; installScript?: InstallScriptFinding[]; heavyDependency?: HeavyDependencyFinding[]; eol?: EolFinding[]; diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 0b90e20549..236ab2bd3f 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -17,6 +17,7 @@ const EXPECTED_ANALYZERS = [ "installScript", "heavyDependency", "actionPin", + "workflowInjection", "eol", "redos", "provenance", diff --git a/review-enrichment/test/workflow-injection.test.ts b/review-enrichment/test/workflow-injection.test.ts new file mode 100644 index 0000000000..f03dfd84ce --- /dev/null +++ b/review-enrichment/test/workflow-injection.test.ts @@ -0,0 +1,453 @@ +// Units for the GitHub Actions workflow-injection / pwn-request analyzer (#2101-class). Kept separate so +// analyzer PRs avoid collisions, mirroring the actions-pin/iac-misconfig sibling test files. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + scanPatchForWorkflowInjection, + scanWorkflowInjection, +} from "../dist/analyzers/workflow-injection.js"; +import { + ANALYZER_DESCRIPTORS, + getAnalyzerDescriptor, +} from "../dist/analyzers/registry.js"; +import { buildBrief } from "../dist/brief.js"; +import { renderBrief } from "../dist/render.js"; + +const workflowPath = ".github/workflows/ci.yml"; + +/** Build an add-only unified-diff patch starting at new-file line `startLine`. The old-file range is irrelevant + * to the analyzer (only the `+X,Y` new-range is parsed), so a placeholder `-0,0` is always used. */ +function addedPatch(startLine: number, lines: string[]): string { + return [`@@ -0,0 +${startLine},${lines.length} @@`, ...lines.map((l) => `+${l}`)].join("\n"); +} + +test("scanPatchForWorkflowInjection flags an untrusted checkout under pull_request_target with no environment gate", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.sha }}", + ]), + ); + assert.deepEqual(findings, [ + { file: workflowPath, line: 7, kind: "untrusted-checkout" }, + ]); +}); + +test("scanPatchForWorkflowInjection flags an untrusted checkout under workflow_run via the github.head_ref form", () => { + // workflow_run is the second elevated-trust trigger; github.head_ref is the alternate untrusted-ref form. + // hasPullRequestTarget stays false here, so no missing-permissions finding should ride along. + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " workflow_run:", + "steps:", + " uses: actions/checkout@v4", + " ref: ${{ github.head_ref }}", + ]), + ); + assert.deepEqual(findings, [ + { file: workflowPath, line: 5, kind: "untrusted-checkout" }, + ]); +}); + +test("scanPatchForWorkflowInjection does not flag an untrusted checkout guarded by an environment gate", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "environment: production", + "steps:", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.sha }}", + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection does not flag pull_request_target workflows that never check out an untrusted ref", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + " uses: actions/checkout@v4", + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection flags a run: step that inline-interpolates an untrusted event field", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + ' - run: echo "${{ github.event.pull_request.title }}"', + ]), + ); + assert.deepEqual(findings, [ + { file: workflowPath, line: 6, kind: "unsafe-interpolation" }, + ]); +}); + +test("scanPatchForWorkflowInjection flags an unsafe interpolation inside a multi-line run: block scalar and stops at the closing sibling key", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + " run: |", + ' echo "hello"', + ' echo "${{ github.event.pull_request.title }}"', + " if: always()", + ]), + ); + assert.deepEqual(findings, [ + { file: workflowPath, line: 8, kind: "unsafe-interpolation" }, + ]); +}); + +test("scanPatchForWorkflowInjection does not flag every remaining event field kind when each is present verbatim", () => { + // Exercises the rest of the UNSAFE_EVENT_FIELD_RE alternation directly (issue title/body, comment body, + // github.head_ref) in one pass, each on its own single-line run: step. + const cases: string[] = [ + ' - run: echo "${{ github.event.pull_request.body }}"', + ' - run: echo "${{ github.event.pull_request.head.ref }}"', + ' - run: echo "${{ github.event.pull_request.head.label }}"', + ' - run: echo "${{ github.event.issue.title }}"', + ' - run: echo "${{ github.event.issue.body }}"', + ' - run: echo "${{ github.event.comment.body }}"', + ' - run: echo "${{ github.head_ref }}"', + ]; + for (const line of cases) { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + line, + ]), + ); + assert.deepEqual( + findings, + [{ file: workflowPath, line: 6, kind: "unsafe-interpolation" }], + `expected an unsafe-interpolation finding for: ${line}`, + ); + } +}); + +test("scanPatchForWorkflowInjection does not flag event fields safely routed through env: first", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + " env:", + " TITLE: ${{ github.event.pull_request.title }}", + " run: |", + ' echo "$TITLE"', + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection ignores unchanged (context) lines for both the run-block start and its interior", () => { + // A fully pre-existing, unmodified run: block that already carries the unsafe pattern must not be reported — + // it is not new risk this PR introduced. All lines below are context lines (unified-diff leading space). + const patch = [ + "@@ -1,7 +1,7 @@", + " on:", + " pull_request_target:", + " permissions:", + " contents: read", + " steps:", + " run: |", + ' echo "${{ github.event.pull_request.title }}"', + ].join("\n"); + assert.deepEqual(scanPatchForWorkflowInjection(workflowPath, patch), []); +}); + +test("scanPatchForWorkflowInjection flags a pull_request_target workflow missing a top-level permissions block", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "jobs:", + " build:", + "steps:", + " run: echo hi", + ]), + ); + assert.deepEqual(findings, [ + { file: workflowPath, line: 2, kind: "missing-permissions" }, + ]); +}); + +test("scanPatchForWorkflowInjection does not flag a pull_request_target workflow that declares permissions", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + " run: echo hi", + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection never flags a plain pull_request trigger, even with an otherwise-unsafe body", () => { + // pull_request gets a read-only GITHUB_TOKEN for a fork PR, so none of the three rules apply — no missing + // permissions, no untrusted checkout, no unsafe interpolation — despite every other ingredient being present. + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request:", + "steps:", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.sha }}", + ' - run: echo "${{ github.event.pull_request.title }}"', + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection degrades gracefully (no throw) on malformed/non-YAML patch content", () => { + const garbage = addedPatch(1, [ + "{{{{{ ][[ garbled ::::  not yaml at all ---", + "\ttab-indented\tline\twith\ttabs", + "ref: not-a-valid-value-at-all $$$", + ]); + assert.doesNotThrow(() => scanPatchForWorkflowInjection(workflowPath, garbage)); + assert.deepEqual(scanPatchForWorkflowInjection(workflowPath, garbage), []); +}); + +test("scanPatchForWorkflowInjection skips a pre-hunk preamble without shifting line numbers", () => { + const patch = [ + "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml", + "index 1111111..2222222 100644", + "--- a/.github/workflows/ci.yml", + "+++ b/.github/workflows/ci.yml", + "@@ -1,0 +1,2 @@", + "+on:", + "+ pull_request_target:", + ].join("\n"); + assert.deepEqual(scanPatchForWorkflowInjection(workflowPath, patch), [ + { file: workflowPath, line: 2, kind: "missing-permissions" }, + ]); +}); + +test("scanPatchForWorkflowInjection keeps line numbers correct across a removed line and a no-newline marker", () => { + const patch = [ + "@@ -1,1 +1,2 @@", + "-on: pull_request", + "\\ No newline at end of file", + "+on:", + "+ pull_request_target:", + ].join("\n"); + assert.deepEqual(scanPatchForWorkflowInjection(workflowPath, patch), [ + { file: workflowPath, line: 2, kind: "missing-permissions" }, + ]); +}); + +test("scanPatchForWorkflowInjection skips a line longer than the safety cap", () => { + const longRef = `ref: \${{ github.event.pull_request.head.sha }}${"x".repeat(2200)}`; + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + "steps:", + longRef, + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForWorkflowInjection returns nothing when maxFindings is exhausted up front", () => { + assert.deepEqual( + scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, ["on:", " pull_request_target:"]), + 0, + ), + [], + ); +}); + +test("scanPatchForWorkflowInjection caps its own findings at maxFindings", () => { + const findings = scanPatchForWorkflowInjection( + workflowPath, + addedPatch(1, [ + "on:", + " pull_request_target:", + "steps:", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.sha }}", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.ref }}", + " uses: actions/checkout@v4", + " ref: ${{ github.event.pull_request.head.label }}", + ]), + 2, + ); + assert.equal(findings.length, 2); +}); + +test("scanWorkflowInjection scans only changed workflow YAML files with patches", async () => { + const findings = await scanWorkflowInjection({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: ".github/workflows/ci.yml", + patch: addedPatch(1, ["on:", " pull_request_target:"]), + }, + { + path: "docs/workflow.yml", + patch: addedPatch(1, ["on:", " pull_request_target:"]), + }, + { + path: ".github/workflows/no-patch.yml", + }, + ], + }); + + assert.deepEqual(findings, [ + { file: ".github/workflows/ci.yml", line: 2, kind: "missing-permissions" }, + ]); +}); + +test("scanWorkflowInjection degrades gracefully (no throw) end to end on malformed patch content", async () => { + await assert.doesNotReject( + scanWorkflowInjection({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: ".github/workflows/ci.yml", + patch: addedPatch(1, ["]]][[[ not yaml ---"]), + }, + ], + }), + ); +}); + +test("scanWorkflowInjection stops once the overall finding cap is reached, without scanning later files", async () => { + const manyRefLines = Array.from({ length: 30 }, () => + " ref: ${{ github.event.pull_request.head.sha }}", + ); + const findings = await scanWorkflowInjection({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: ".github/workflows/first.yml", + patch: addedPatch(1, ["on:", " pull_request_target:", "permissions:", " contents: read", ...manyRefLines]), + }, + { + path: ".github/workflows/second.yml", + patch: addedPatch(1, [ + "on:", + " pull_request_target:", + "permissions:", + " contents: read", + " ref: ${{ github.event.pull_request.head.sha }}", + ]), + }, + ], + }); + + assert.equal(findings.length, 25); + assert.ok(findings.every((f) => f.file === ".github/workflows/first.yml")); +}); + +test("workflowInjection is registered with the expected descriptor shape", () => { + const descriptor = getAnalyzerDescriptor("workflowInjection"); + assert.ok(descriptor, "workflowInjection descriptor is missing from the registry"); + assert.equal(descriptor!.category, "security"); + assert.equal(descriptor!.cost, "local"); + assert.equal(descriptor!.defaultEnabled, true); + assert.deepEqual(descriptor!.requires, ["files"]); + assert.ok(descriptor!.docs.summary.length > 10); + assert.ok(descriptor!.docs.looksAt.length > 10); + assert.ok(descriptor!.docs.reports.length > 10); + assert.ok(descriptor!.docs.network.length > 10); + assert.ok(ANALYZER_DESCRIPTORS.some((d) => d.name === "workflowInjection")); +}); + +test("buildBrief skips workflowInjection when no workflow file changed, and runs it when one does", async () => { + const skipped = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [{ path: "src/index.ts", patch: "@@ -0,0 +1,1 @@\n+export {};" }], + }); + assert.equal(skipped.analyzerStatus.workflowInjection, "skipped"); + assert.equal(skipped.telemetry.analyzers.workflowInjection?.skipReason, "no_workflow"); + + const ran = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: ".github/workflows/ci.yml", + patch: addedPatch(1, ["on:", " pull_request_target:"]), + }, + ], + }); + assert.equal(ran.analyzerStatus.workflowInjection, "ok"); + assert.deepEqual(ran.findings.workflowInjection, [ + { file: ".github/workflows/ci.yml", line: 2, kind: "missing-permissions" }, + ]); + assert.match(ran.promptSection, /workflow-injection \/ pwn-request risk/); +}); + +test("renderBrief renders every workflow-injection finding kind and omits the section when empty", () => { + const { promptSection } = renderBrief({ + workflowInjection: [ + { file: workflowPath, line: 7, kind: "untrusted-checkout" }, + { file: workflowPath, line: 6, kind: "unsafe-interpolation" }, + { file: workflowPath, line: 2, kind: "missing-permissions" }, + ], + }); + assert.match(promptSection, /GitHub Actions workflow-injection \/ pwn-request risk/); + assert.match(promptSection, /checks out the untrusted PR head/); + assert.match(promptSection, /interpolates an untrusted event field/); + assert.match(promptSection, /no top-level `permissions:` block/); + assert.match(promptSection, new RegExp(`${workflowPath.replace(/\./g, "\\.")}:7`)); + + const empty = renderBrief({ workflowInjection: [] }); + assert.equal(empty.promptSection, ""); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 3690d8de16..cf22fa0b44 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -38,6 +38,7 @@ export const REES_ANALYZER_NAMES = [ "terminology", "todoMarker", "magicNumber", + "workflowInjection", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number];