From 7f971b7e6f4ac8c22d70017fb8d44cb9cab497cb Mon Sep 17 00:00:00 2001 From: Jamkris Date: Fri, 15 May 2026 14:58:47 +0900 Subject: [PATCH] fix(ci): treat 'permissions: write-all' as a write-permission gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `WRITE_PERMISSION_PATTERN` in `validate-workflow-security.js` enumerates named GitHub Actions scopes (`contents: write`, `issues: write`, etc.) to decide whether a workflow needs to: - disable `persist-credentials` on `actions/checkout` - pass `--ignore-scripts` to `npm ci` The pattern misses the top-level shorthand `permissions: write-all`, which is the strictly broader form — it grants every named scope write access in a single line. As a result, a workflow that opts into write-all currently slips both gates. Reproduced on `main` before this commit: $ cat /tmp/bad.yml name: bad on: [push] permissions: write-all jobs: do: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - run: npm ci $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js Validated workflow security for 1 workflow files $ echo $? 0 Expected: at least two violations (missing `persist-credentials: false`, missing `--ignore-scripts`). Actual: passes silently. Fix: add a sibling pattern `WRITE_ALL_PATTERN` that matches `^\s*permissions:\s*write-all\b` and OR it with `WRITE_PERMISSION_PATTERN` at the single gate. Both top-level and job-level `permissions:` blocks satisfy the `^\s*` prefix. After this commit the reproduction above exits 1 with: ERROR: bad.yml:8 - workflows with write permissions must disable checkout credential persistence ERROR: bad.yml:9 - workflows with write permissions must install npm dependencies with --ignore-scripts Three new regression tests in `tests/ci/validate-workflow-security.test.js`: - rejects write-all + credential-persisting checkout - rejects write-all + `npm ci` without `--ignore-scripts` - allows write-all when both gates are satisfied (no over-block) Test count: 14 → 17 in this file; full `yarn test` still green. A separate `refs/pull/N/merge` bypass under `pull_request_target` exists in the same validator and is fixed in the next commit. --- scripts/ci/validate-workflow-security.js | 8 +++++- tests/ci/validate-workflow-security.test.js | 28 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/scripts/ci/validate-workflow-security.js b/scripts/ci/validate-workflow-security.js index 22018b3b..bdefcdb7 100644 --- a/scripts/ci/validate-workflow-security.js +++ b/scripts/ci/validate-workflow-security.js @@ -25,6 +25,12 @@ const RULES = [ ]; const WRITE_PERMISSION_PATTERN = /^\s*(?:contents|issues|pull-requests|actions|checks|deployments|discussions|id-token|packages|pages|repository-projects|security-events|statuses):\s*write\b/m; +// `permissions: write-all` is GitHub Actions' shorthand for granting every +// 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; 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; @@ -124,7 +130,7 @@ function findViolations(filePath, source) { } } - if (WRITE_PERMISSION_PATTERN.test(source)) { + if (WRITE_PERMISSION_PATTERN.test(source) || WRITE_ALL_PATTERN.test(source)) { for (const step of checkoutSteps) { if (!/persist-credentials:\s*['"]?false['"]?\b/m.test(step.text)) { violations.push({ diff --git a/tests/ci/validate-workflow-security.test.js b/tests/ci/validate-workflow-security.test.js index b32f1176..cacbcc39 100644 --- a/tests/ci/validate-workflow-security.test.js +++ b/tests/ci/validate-workflow-security.test.js @@ -155,6 +155,34 @@ function run() { assert.strictEqual(result.status, 0, result.stderr || result.stdout); })) passed++; else failed++; + // `permissions: write-all` is GitHub Actions' shorthand for granting every + // scope write access. The named-scope pattern only catches `contents: write`, + // `issues: write`, etc., so workflows that opt into write-all were silently + // exempted from the persist-credentials and --ignore-scripts gates. + + if (test('rejects checkout credential persistence in workflows with permissions: write-all', () => { + const result = runValidator({ + 'unsafe-write-all-checkout.yml': `name: Unsafe\non:\n workflow_dispatch:\npermissions: write-all\njobs:\n release:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n - run: npm ci --ignore-scripts\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on write-all + credential-persisting checkout'); + assert.match(result.stderr, /write permissions must disable checkout credential persistence/); + })) passed++; else failed++; + + if (test('rejects npm ci without ignore-scripts in workflows with permissions: write-all', () => { + const result = runValidator({ + 'unsafe-write-all-install.yml': `name: Unsafe\non:\n workflow_dispatch:\npermissions: write-all\njobs:\n audit:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n persist-credentials: false\n - run: npm ci\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on write-all + npm ci without --ignore-scripts'); + assert.match(result.stderr, /npm ci must include --ignore-scripts/); + })) passed++; else failed++; + + if (test('allows compliant workflow with permissions: write-all', () => { + const result = runValidator({ + 'safe-write-all.yml': `name: Safe\non:\n workflow_dispatch:\npermissions: write-all\njobs:\n release:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n persist-credentials: false\n - run: npm ci --ignore-scripts\n`, + }); + assert.strictEqual(result.status, 0, result.stderr || result.stdout); + })) passed++; else failed++; + if (test('rejects actions/cache in workflows with id-token write', () => { const result = runValidator({ 'unsafe-oidc-cache.yml': `name: Unsafe\non:\n push:\npermissions:\n contents: read\n id-token: write\njobs:\n release:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/cache@v5\n with:\n path: ~/.npm\n key: cache\n`,