From 7bb3172041caef53da2dbf0088c77d1532a0f024 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Mon, 18 May 2026 10:00:24 +0900 Subject: [PATCH] test(ci): coverage for round-1 fixes (quoted write-all, dedup, lifecycle scope) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three test changes in response to the round-1 review: 1. **Add quoted-write-all coverage** (cubic P0 follow-up). Two new cases assert the regex now matches the double-quoted and single-quoted YAML forms of `permissions: "write-all"`: - `rejects double-quoted permissions: "write-all"` - `rejects single-quoted permissions: 'write-all'` Both fixtures trigger only the persist-credentials gate, so they exercise the WRITE_ALL_PATTERN OR-clause in isolation. 2. **Add expression+ref dedup coverage** (greptile P2 follow-up). `emits a single violation when both expressionPattern and refPattern match the same step` — uses `refs/pull/${{ … head.sha }}/merge` as the fixture (which matches both patterns) and counts ERROR lines for the `pull_request_target` rule, asserting exactly one. Re-introducing the duplicate-push bug would re-fail this test immediately. 3. **Drop the `npm ci without --ignore-scripts under write-all` test** (greptile P2). That test happened to pass under the previous `--ignore-scripts` regex, but `UNSAFE_INSTALL_PATTERNS` (added in `f7035b56`) fires unconditionally for every workflow regardless of permissions. So the test was exercising a pre-existing code path that has nothing to do with WRITE_ALL_PATTERN. Reviewer flagged this could mislead future contributors into thinking lifecycle-script enforcement is gated on write permissions. Replaced by the surrounding `rejects checkout credential persistence in workflows with permissions: write-all` test (already present) and the new quoted-form tests above, which all exercise the actual persist-credentials gate that the WRITE_ALL_PATTERN clause newly activates. Test count: 22 → 24 (added 3 new, dropped 1). All green; `yarn lint` clean. The cohort comment above the write-all block was also tightened to explicitly note that "the lifecycle-script gate already fires unconditionally for every workflow" so the next reader sees the distinction up front. --- tests/ci/validate-workflow-security.test.js | 43 ++++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/tests/ci/validate-workflow-security.test.js b/tests/ci/validate-workflow-security.test.js index 9c51d6d2..89cf88a9 100644 --- a/tests/ci/validate-workflow-security.test.js +++ b/tests/ci/validate-workflow-security.test.js @@ -127,6 +127,22 @@ function run() { assert.strictEqual(result.status, 0, result.stderr || result.stdout); })) passed++; else failed++; + // When a checkout step matches both the expression-based rule + // (`github.event.pull_request.head.sha`) and the refPattern fallback + // (`refs/pull/...`), only one violation should be emitted — the + // expression match is the more specific signal and printing both would + // duplicate an otherwise identical ERROR line. + + if (test('emits a single violation when both expressionPattern and refPattern match the same step', () => { + const result = runValidator({ + 'unsafe-pr-target-both.yml': `name: Unsafe\non:\n pull_request_target:\njobs:\n inspect:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n ref: refs/pull/\${{ github.event.pull_request.head.sha }}/merge\n persist-credentials: false\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail'); + // Count ERROR: lines for this rule's description. Should be exactly 1. + const matches = (result.stderr || '').match(/ERROR:.*pull_request_target must not checkout an untrusted pull_request head ref/g) || []; + assert.strictEqual(matches.length, 1, `Expected exactly 1 violation, got ${matches.length}: ${result.stderr}`); + })) passed++; else failed++; + if (test('rejects shared cache use in pull_request_target workflows', () => { const result = runValidator({ 'unsafe-pr-target-cache.yml': `name: Unsafe\non:\n pull_request_target:\n branches: [main]\njobs:\n inspect:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/cache@v5\n with:\n path: ~/.npm\n key: cache\n - run: echo inspect\n`, @@ -186,7 +202,10 @@ function run() { // `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. + // exempted from the persist-credentials gate (the lifecycle-script gate + // already fires unconditionally for every workflow). The tests below + // exercise the persist-credentials path specifically — that's the gate the + // WRITE_ALL_PATTERN OR-clause newly activates. if (test('rejects checkout credential persistence in workflows with permissions: write-all', () => { const result = runValidator({ @@ -196,15 +215,27 @@ function run() { 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', () => { + // Quoted YAML forms (`"write-all"` and `'write-all'`) are valid YAML for the + // same scalar value. Verify the WRITE_ALL_PATTERN regex covers them — without + // the quote markers it silently slips the same persist-credentials gate. + + if (test('rejects double-quoted 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`, + 'unsafe-write-all-double.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 + npm ci without --ignore-scripts'); - assert.match(result.stderr, /npm ci must include --ignore-scripts/); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on quoted write-all + credential-persisting checkout'); + assert.match(result.stderr, /write permissions must disable checkout credential persistence/); })) passed++; else failed++; - if (test('allows compliant workflow with permissions: write-all', () => { + if (test('rejects single-quoted permissions: \'write-all\'', () => { + const result = runValidator({ + 'unsafe-write-all-single.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 single-quoted write-all + credential-persisting checkout'); + assert.match(result.stderr, /write permissions must disable checkout credential persistence/); + })) passed++; else failed++; + + if (test('allows compliant workflow with permissions: write-all (persist-credentials: false)', () => { 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`, });