diff --git a/.github/workflows/pr-test-loc.yml b/.github/workflows/pr-test-loc.yml index 438951ee3fa..a884aedc636 100644 --- a/.github/workflows/pr-test-loc.yml +++ b/.github/workflows/pr-test-loc.yml @@ -23,14 +23,20 @@ jobs: timeout-minutes: 2 steps: # Why no checkout: the Files API already has per-file additions/deletions. + # Why the default branch and never pull//head: this job holds a write-scoped + # GITHUB_TOKEN, so it may only execute reviewed code. A PR that edits these + # scripts takes effect once merged. - name: Count test vs non-test LoC env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} GITHUB_REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + TRUSTED_REF: ${{ github.event.repository.default_branch }} run: | + set -euo pipefail for script in pr-test-loc-table.mjs pr-test-loc-summary.mjs; do - gh api "repos/${GITHUB_REPOSITORY}/contents/.github/scripts/${script}?ref=pull/${{ github.event.pull_request.number }}/head" \ + gh api "repos/${GITHUB_REPOSITORY}/contents/.github/scripts/${script}?ref=${TRUSTED_REF}" \ --jq .content | base64 --decode > "$RUNNER_TEMP/${script}" done - node "$RUNNER_TEMP/pr-test-loc-summary.mjs" --update-pr "${{ github.event.pull_request.number }}" + node "$RUNNER_TEMP/pr-test-loc-summary.mjs" --update-pr "$PR_NUMBER" diff --git a/config/scripts/pr-test-loc-summary.test.mjs b/config/scripts/pr-test-loc-summary.test.mjs index 715b918bb32..14801ddc85d 100644 --- a/config/scripts/pr-test-loc-summary.test.mjs +++ b/config/scripts/pr-test-loc-summary.test.mjs @@ -15,6 +15,11 @@ import { const projectDir = resolve(import.meta.dirname, '../..') const locScript = join(projectDir, '.github/scripts/pr-test-loc-summary.mjs') +const locWorkflow = parse( + readFileSync(join(projectDir, '.github/workflows/pr-test-loc.yml'), 'utf8') +) +const locJob = locWorkflow.jobs.loc +const locStep = locJob.steps[0] const tempDirs = [] function runLoc(args, { env } = {}) { @@ -178,20 +183,36 @@ describe('PR test LoC summary', () => { }) it('is a no-checkout GitHub-hosted PR workflow', () => { - const workflow = parse( - readFileSync(join(projectDir, '.github/workflows/pr-test-loc.yml'), 'utf8') - ) - const locJob = workflow.jobs.loc - const serialized = JSON.stringify(workflow) - expect(locJob['runs-on']).toBe('ubuntu-latest') expect(locJob.steps).toHaveLength(1) - expect(locJob.steps[0].run).toContain('gh api') - expect(locJob.steps[0].run).toContain('pr-test-loc-table.mjs') - expect(locJob.steps[0].run).toContain('pr-test-loc-summary.mjs') - expect(locJob.steps[0].run).toContain('--update-pr') - expect(workflow.permissions['pull-requests']).toBe('write') - expect(serialized).not.toContain('actions/checkout') - expect(serialized).not.toContain('self-hosted') + expect(locStep.run).toContain('gh api') + expect(locStep.run).toContain('pr-test-loc-table.mjs') + expect(locStep.run).toContain('pr-test-loc-summary.mjs') + expect(locStep.run).toContain('--update-pr') + expect(locWorkflow.permissions['pull-requests']).toBe('write') + expect(JSON.stringify(locWorkflow)).not.toContain('actions/checkout') + expect(JSON.stringify(locWorkflow)).not.toContain('self-hosted') + }) + + // The write-scoped GITHUB_TOKEN makes any PR-authored code a privilege escalation. + it('executes only default-branch script code, never pull-request head code', () => { + const serialized = JSON.stringify(locWorkflow) + + expect(locStep.env.TRUSTED_REF).toBe('${{ github.event.repository.default_branch }}') + expect(locStep.run).toContain('?ref=${TRUSTED_REF}') + expect(serialized).not.toContain('pull_request_target') + expect(serialized).not.toContain('pull/') + expect(serialized).not.toContain('pull_request.head') + expect(serialized).not.toContain('/merge') + }) + + it('passes event data through env instead of interpolating it into the shell', () => { + expect(locStep.env.PR_NUMBER).toBe('${{ github.event.pull_request.number }}') + expect(locStep.run).toContain('--update-pr "$PR_NUMBER"') + expect(locStep.run).not.toContain('${{') + }) + + it('fails the step when a script download fails instead of running a truncated file', () => { + expect(locStep.run).toContain('set -euo pipefail') }) })