From cdbc925d8938ea868f7c65ccb4cc2cc68efbd68f Mon Sep 17 00:00:00 2001 From: Jamkris Date: Fri, 15 May 2026 15:00:42 +0900 Subject: [PATCH] fix(ci): flag refs/pull checkouts under pull_request_target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `pull_request_target` rule's `expressionPattern` matches only the canonical `github.event.pull_request.head.{ref,sha,repo.full_name}` interpolations. It does not match the second canonical form of the same exploit — fetching `refs/pull//{head,merge}` directly: - uses: actions/checkout@v4 with: ref: refs/pull/${{ github.event.pull_request.number }}/merge The merge-ref variant is what GitHub's own security guidance calls out as the highest-severity privilege-escalation pattern under `pull_request_target`: it materialises the PR's merge commit (attacker code spliced with base), executes inside a workflow that has full repo-scoped tokens, and gives the attacker the chance to exfiltrate secrets or push to default branches. `refs/pull/N/head` is functionally equivalent — same source, same trust boundary. Reproduced on `main` before this commit: $ cat /tmp/bad.yml name: bad on: { pull_request_target: { types: [opened] } } permissions: { contents: read } jobs: do: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 with: ref: refs/pull/${{ github.event.pull_request.number }}/merge persist-credentials: false - run: npm ci --ignore-scripts $ ECC_WORKFLOWS_DIR=/tmp node scripts/ci/validate-workflow-security.js Validated workflow security for 1 workflow files $ echo $? 0 Expected: violation flagging the refs/pull checkout under pull_request_target. Actual: passes silently. Fix: add a `refPattern` to the `pull_request_target` rule: /^\s*ref:\s*['"]?[^'"\n]*refs\/(?:remotes\/)?pull\/[^'"\n\s]+/m and apply it per checkout step inside the existing event-gated loop. The pattern matches the ref VALUE so it catches all interpolation shapes — `refs/pull/123/head`, `refs/pull/${{ github.event.pull_request.number }}/merge`, `${{ env.FOO }}/refs/pull/N/head` — without enumerating the possible interpolations themselves. Scoping: the rule is already gated on the workflow containing `pull_request_target:`, so non-privileged `pull_request` workflows that legitimately check out a PR ref are not affected. After this commit the reproduction above exits 1 with: ERROR: bad.yml:10 - pull_request_target must not checkout an untrusted pull_request head ref/repository Three new regression tests in `tests/ci/validate-workflow-security.test.js`: - rejects pull_request_target + refs/pull//merge - rejects pull_request_target + hardcoded refs/pull//head - allows pull_request_target with no `with.ref:` (base-ref checkout — the safe pattern from GitHub's own guidance) Test count: 17 → 20 in this file; full `yarn test` still green. Together with the previous commit, this closes the two independent `validate-workflow-security.js` bypasses I found. --- scripts/ci/validate-workflow-security.js | 21 ++++++++++++++++ tests/ci/validate-workflow-security.test.js | 28 +++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/scripts/ci/validate-workflow-security.js b/scripts/ci/validate-workflow-security.js index bdefcdb7..4e65cdef 100644 --- a/scripts/ci/validate-workflow-security.js +++ b/scripts/ci/validate-workflow-security.js @@ -21,6 +21,15 @@ const RULES = [ eventPattern: /\bpull_request_target\s*:/m, description: 'pull_request_target must not checkout an untrusted pull_request head ref/repository', expressionPattern: /\$\{\{\s*github\.event\.pull_request\.head\.(?:ref|sha|repo\.full_name)\s*\}\}/g, + // Even without the standard `github.event.pull_request.head.*` expression, + // a checkout under `pull_request_target` that fetches a `refs/pull//{head,merge}` + // ref pulls attacker-controlled code into a workflow with write-scoped + // tokens. GitHub's security guidance treats both forms equivalently; + // we match the ref value directly so any interpolation that resolves + // to such a ref (`refs/pull/${{ github.event.pull_request.number }}/merge`, + // a hardcoded `refs/pull/123/head`, a `${{ env.X }}` that the maintainer + // assumes is safe, etc.) trips the same rule. + refPattern: /^\s*ref:\s*['"]?[^'"\n]*refs\/(?:remotes\/)?pull\/[^'"\n\s]+/m, }, ]; @@ -127,6 +136,18 @@ function findViolations(filePath, source) { line: step.startLine + getLineNumber(step.text, match.index) - 1, }); } + if (rule.refPattern) { + const refMatch = step.text.match(rule.refPattern); + if (refMatch) { + violations.push({ + filePath, + event: rule.event, + description: rule.description, + expression: refMatch[0].trim(), + line: step.startLine + getLineNumber(step.text, refMatch.index) - 1, + }); + } + } } } diff --git a/tests/ci/validate-workflow-security.test.js b/tests/ci/validate-workflow-security.test.js index cacbcc39..9c51d6d2 100644 --- a/tests/ci/validate-workflow-security.test.js +++ b/tests/ci/validate-workflow-security.test.js @@ -99,6 +99,34 @@ function run() { assert.match(result.stderr, /pull_request\.head\.sha/); })) passed++; else failed++; + // `refs/pull//{head,merge}` under `pull_request_target` is the canonical + // privilege-escalation pattern that the standard `github.event.pull_request.head.*` + // expression check did not cover. Either form pulls attacker-controlled code + // into a privileged workflow. + + if (test('rejects pull_request_target checkout fetching refs/pull/N/merge', () => { + const result = runValidator({ + 'unsafe-pr-target-merge-ref.yml': `name: Unsafe\non:\n pull_request_target:\n types: [opened]\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.number }}/merge\n persist-credentials: false\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on refs/pull/N/merge under pull_request_target'); + assert.match(result.stderr, /pull_request_target must not checkout an untrusted pull_request head ref/); + })) passed++; else failed++; + + if (test('rejects pull_request_target checkout fetching hardcoded refs/pull/N/head', () => { + const result = runValidator({ + 'unsafe-pr-target-head-ref.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/123/head\n persist-credentials: false\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on hardcoded refs/pull/N/head'); + assert.match(result.stderr, /pull_request_target must not checkout an untrusted pull_request head ref/); + })) passed++; else failed++; + + if (test('allows pull_request_target checkout of the base ref (no with.ref)', () => { + const result = runValidator({ + 'safe-pr-target-base.yml': `name: Safe\non:\n pull_request_target:\njobs:\n inspect:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n persist-credentials: false\n - run: echo inspecting base\n`, + }); + assert.strictEqual(result.status, 0, result.stderr || result.stdout); + })) 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`,