fix(ci): match quoted write-all + dedupe duplicate checkout violations

Two round-1 review findings, fixed together because they touch the
same regex/loop region of `findViolations`:

1. **cubic P0 — quoted write-all bypass**.
   `WRITE_ALL_PATTERN` was `/^\s*permissions:\s*write-all\b/m`, which
   does not match the perfectly valid YAML forms
   `permissions: "write-all"` and `permissions: 'write-all'`. A
   workflow that quoted the shorthand slipped right through the
   persist-credentials gate the previous commit was supposed to close.

   Reproduced before this commit:
     $ cat /tmp/q.yml
     name: bad
     on: [push]
     permissions: "write-all"
     jobs:
       do:
         runs-on: ubuntu-latest
         steps:
           - uses: actions/checkout@v4
     $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js
     Validated workflow security for 1 workflow files
     exit=0

   Fix: tighten the regex to
     /^\s*permissions:\s*["']?write-all["']?\s*$/m
   which accepts the bare, double-quoted, and single-quoted YAML forms
   while still anchoring on the `permissions:` key. The trailing `\s*$`
   prevents accidentally matching keys whose value happens to start
   with `write-all` (e.g. some future literal `write-all-something`).

2. **greptile P2 — duplicate violation when both patterns match**.
   A `ref: refs/pull/${{ github.event.pull_request.head.sha }}/merge`
   value matches both the `pull_request_target` rule's
   `expressionPattern` (the `head.sha` interpolation) and its
   `refPattern` (the `refs/pull/` literal). Each push generates an
   ERROR line with the same description and just a different
   `expression:` echo, so the reviewer sees the same violation twice.

   Fix: track `stepFlagged` inside the per-step loop and skip the
   `refPattern` fallback once any `expressionPattern` match has already
   produced a violation for this step. The `refPattern` is a fallback
   for ref-only forms (`refs/pull/123/head`, `${{ env.X }}` whose
   resolved value is a PR ref); when the more specific expression
   already fires, the fallback is redundant by definition.

After both fixes, the round-1 reproductions resolve cleanly:

  $ # quoted form now blocks
  $ ECC_WORKFLOWS_DIR=/tmp/q1/.github/workflows node scripts/ci/validate-workflow-security.js
  ERROR: quoted.yml:8 - workflows with write permissions must disable checkout credential persistence
  exit=1

  $ # combined head.sha + refs/pull now prints one ERROR, not two
  $ ECC_WORKFLOWS_DIR=/tmp/q2/.github/workflows node scripts/ci/validate-workflow-security.js
  ERROR: dup.yml:10 - pull_request_target must not checkout an untrusted pull_request head ref/repository
    Unsafe expression: ${{ github.event.pull_request.head.sha }}
  exit=1

Test additions land in the next commit.
This commit is contained in:
Jamkris
2026-05-18 09:59:58 +09:00
committed by Affaan Mustafa
parent cdbc925d89
commit e06d038257

View File

@@ -38,8 +38,9 @@ const WRITE_PERMISSION_PATTERN = /^\s*(?:contents|issues|pull-requests|actions|c
// scope write access. The named-scope pattern above misses it because there
// is no scope name on the left of the colon — just the literal `write-all`
// value at the permissions key. Treat both as equivalent for the purposes
// of the persist-credentials and lifecycle-script gates below.
const WRITE_ALL_PATTERN = /^\s*permissions:\s*write-all\b/m;
// of the persist-credentials gate below. The optional single/double quotes
// match valid YAML `permissions: "write-all"` / `'write-all'` forms.
const WRITE_ALL_PATTERN = /^\s*permissions:\s*["']?write-all["']?\s*$/m;
const NPM_AUDIT_PATTERN = /\bnpm\s+audit\b(?!\s+signatures\b)/;
const NPM_AUDIT_SIGNATURES_PATTERN = /\bnpm\s+audit\s+signatures\b/;
const ACTIONS_CACHE_PATTERN = /uses:\s*['"]?actions\/cache@/m;
@@ -127,6 +128,13 @@ function findViolations(filePath, source) {
}
for (const step of checkoutSteps) {
// Track whether the expression-based rule already produced a
// violation for this step. If it did, skip the refPattern fallback
// — a `refs/pull/${{ github.event.pull_request.head.sha }}/merge`
// value matches both patterns under the same rule, and the second
// push would print a duplicate ERROR line that says exactly the
// same thing with a different `expression:` echo.
let stepFlagged = false;
for (const match of step.text.matchAll(rule.expressionPattern)) {
violations.push({
filePath,
@@ -135,8 +143,9 @@ function findViolations(filePath, source) {
expression: match[0],
line: step.startLine + getLineNumber(step.text, match.index) - 1,
});
stepFlagged = true;
}
if (rule.refPattern) {
if (rule.refPattern && !stepFlagged) {
const refMatch = step.text.match(rule.refPattern);
if (refMatch) {
violations.push({