mirror of
https://github.com/affaan-m/everything-claude-code.git
synced 2026-05-18 14:53:05 +08:00
fix(ci): flag refs/pull checkouts under pull_request_target
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/<N>/{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/<N>/merge
- rejects pull_request_target + hardcoded refs/pull/<N>/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.
This commit is contained in:
@@ -21,6 +21,15 @@ const RULES = [
|
|||||||
eventPattern: /\bpull_request_target\s*:/m,
|
eventPattern: /\bpull_request_target\s*:/m,
|
||||||
description: 'pull_request_target must not checkout an untrusted pull_request head ref/repository',
|
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,
|
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/<N>/{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,
|
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,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -99,6 +99,34 @@ function run() {
|
|||||||
assert.match(result.stderr, /pull_request\.head\.sha/);
|
assert.match(result.stderr, /pull_request\.head\.sha/);
|
||||||
})) passed++; else failed++;
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
// `refs/pull/<N>/{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', () => {
|
if (test('rejects shared cache use in pull_request_target workflows', () => {
|
||||||
const result = runValidator({
|
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`,
|
'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`,
|
||||||
|
|||||||
Reference in New Issue
Block a user