From e06d03825719f46ae21c358f9f2e222ceefdaeb8 Mon Sep 17 00:00:00 2001
From: Jamkris <dltmdgus1412@gmail.com>
Date: Mon, 18 May 2026 09:59:58 +0900
Subject: [PATCH] fix(ci): match quoted write-all + dedupe duplicate checkout
 violations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 scripts/ci/validate-workflow-security.js | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/scripts/ci/validate-workflow-security.js b/scripts/ci/validate-workflow-security.js
index 4e65cdef..ec7f82ef 100644
--- a/scripts/ci/validate-workflow-security.js
+++ b/scripts/ci/validate-workflow-security.js
@@ -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({