Use environments to protect CI #796
Conversation
|
WalkthroughReworks the GitHub Actions CI workflow to replace pull_request_target label gating with environment-protected external-PR handling, restructures triggers, permissions, jobs, checkout and Node setup, and removes a single bug-bounty notice line from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
23-25: Tighten permissions to least privilege.Unless a step writes to PRs, set
pull-requests: read. Safer forpull_request_target.permissions: - contents: read - pull-requests: write + contents: read + pull-requests: read
54-56: Use setup-node v4 and a maintained Node version spec.Update action to v4 and prefer
20.x(orlts/*) to avoid pinning an invalid patch.- uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: - node-version: 20.19 + node-version: 20.x
58-60: Optional: Block postinstall scripts for untrusted contexts.If you keep a single job for all contexts, add
--ignore-scriptswhen running on external PRs to reduce supply‑chain risk.Example (conditional flag):
- name: Install dependencies run: npm ci --no-fund --no-audit ${{ (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository) && '--ignore-scripts' || '' }}
21-21: Optional: Enable cancel-in-progress to save runners.Typical CI behavior cancels superseded runs on new commits.
-concurrency: ${{ github.workflow }}--${{ github.ref }} +concurrency: + group: ${{ github.workflow }}--${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)bb.test(0 hunks)
💤 Files with no reviewable changes (1)
- bb.test
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
17-19: PR trigger runs only when ci.yml changes. Is this intended?With
paths: [.github/workflows/ci.yml], regular same-repo PRs won’t run this workflow viapull_request. Coverage then relies entirely onpull_request_target. Confirm this is intentional to avoid double runs; otherwise, drop the path filter.If you want standard PR CI, change to:
- pull_request: - types: [opened, synchronize, reopened] - paths: - - .github/workflows/ci.yml + pull_request: + types: [opened, synchronize, reopened]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
11-20: Avoid double-triggering the workflow for same-repo PRs.You trigger both
pull_requestandpull_request_target. Same-repo PRs will fire both, and yourtestsjob currently matches both, causing duplicate runs.Options:
- Prefer
pull_requestfor same-repo PRs; reservepull_request_targetonly for forked PRs (enforce via jobif).- Or drop
pull_request_targetand use a separate workflow for gated external tests.
22-24: Concurrency key may collide on pull_request_target.
github.refforpull_request_targetis the base branch (e.g., refs/heads/main), so multiple forked PR runs can cancel each other.Consider:
- group: ${{ github.workflow }}--${{ github.ref }} + group: ${{ github.workflow }}--${{ github.event_name == 'pull_request_target' && format('pr#{0}', github.event.pull_request.number) || github.ref }}
31-57: Basic validation job looks good; add npm cache for speed.- name: Install Node.js uses: actions/setup-node@v4 with: node-version: 20.x + cache: npm + cache-dependency-path: package-lock.json
79-83: Run scripts with strict shell flags.Harden the decrypt step.
- - name: Decrypt keyfile - run: ./.github/scripts/decrypt_secret.sh + - name: Decrypt keyfile + shell: bash + run: set -euo pipefail; ./.github/scripts/decrypt_secret.sh
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
26-29: Permissions tightened appropriately.
62-67: Do not set a dynamic environment; empty string is invalid and property access can break on push.Using
environment: ${{ ... || '' }}is invalid (empty env name), andgithub.event.pull_request.*is undefined onpush. Split jobs instead: one trusted job without environment, one gated job for external PRs withenvironment: external-testing.Apply this refactor:
- tests: + tests: name: Tests runs-on: ubuntu-latest - # SECURITY: Use environment protection for external contributors - environment: ${{ github.event.pull_request.head.repo.full_name != github.repository && 'external-testing' || '' }} - # Run tests with secrets for: - # 1. Push to main (trusted), OR - # 2. PR from same repository (trusted) - # For external PRs: environment protection requires manual approval + # Trusted contexts only (no environment gating here) if: | - github.event_name == 'push' || - github.event.pull_request.head.repo.full_name == github.repository + github.event_name == 'push' || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) @@ - name: Checkout sources uses: actions/checkout@v4 with: - # Environment protection provides security - we can safely checkout PR code - ref: ${{ github.event.pull_request.head.sha || github.sha }} + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} @@ AWS_REGION: ${{secrets.AWS_REGION}} + + tests-external: + name: Tests (external, gated) + runs-on: ubuntu-latest + if: github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository + environment: external-testing + permissions: + contents: read + pull-requests: read + steps: + - name: Checkout PR head (approved via environment) + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: 20.x + - name: Install dependencies + run: npm ci --no-fund --no-audit + - name: Build + run: npm run build + - name: Decrypt keyfile + run: ./.github/scripts/decrypt_secret.sh + env: + KEYFILE_PASSPHRASE: ${{secrets.KEYFILE_PASSPHRASE}} + - name: Run tests + run: npm run test + env: + AWS_BUCKET: ${{secrets.AWS_BUCKET}} + AWS_ACCESS_KEY_ID: ${{secrets.AWS_ACCESS_KEY_ID}} + AWS_SECRET_ACCESS_KEY: ${{secrets.AWS_SECRET_ACCESS_KEY}} + AZURE_ACCOUNT_ID: ${{secrets.AZURE_ACCOUNT_ID}} + AZURE_ACCOUNT_KEY: ${{secrets.AZURE_ACCOUNT_KEY}} + AZURE_CONTAINER_NAME: ${{secrets.AZURE_CONTAINER_NAME}} + AWS_REGION: ${{secrets.AWS_REGION}}
68-71: External PRs never start this job; environment approval is unreachable.The
ifexcludes forked PRs, so the gated path can’t run. Allowpull_request_targetfor forks or split jobs as suggested.Minimal fix if you keep a single job:
if: | - github.event_name == 'push' || - github.event.pull_request.head.repo.full_name == github.repository + github.event_name == 'push' || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository)Note: This still leaves the invalid dynamic
environment(see prior comment). Splitting jobs is safer.
73-78: Guard PR-only context in ref expression.
github.event.pull_request.head.shais undefined on push; guard by event name to avoid expression pitfalls.- ref: ${{ github.event.pull_request.head.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && github.event.pull_request.head.sha || github.sha }}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
22-24: Unify PR concurrency grouping across pull_request and pull_request_target.Currently only targets
pull_request_target. Group both by PR number.- group: ${{ github.workflow }}--${{ github.event_name == 'pull_request_target' && format('pr#{0}', github.event.pull_request.number) || github.ref }} + group: ${{ github.workflow }}--${{ (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && format('pr#{0}', github.event.pull_request.number) || github.ref }}
11-15: Trigger hygiene: pull_request_target is configured but no job runs for it.Either add the gated external job (preferred) or drop this trigger to avoid no-op runs.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
78-80: Guard checkout ref selection for non‑PR events.Avoid unguarded access to
github.event.pull_request.*onpush.- ref: ${{ github.event.pull_request.head.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && github.event.pull_request.head.sha || github.sha }}
26-29: Permissions tightened appropriately.contents: read and pull-requests: read are good defaults for these jobs.
65-65: Dynamic environment is invalid (empty string) and dereferences PR fields on push. Split jobs.
environment: ''is not allowed, and the expression touchesgithub.event.pull_request.*even onpush, which can break the run. Remove the dynamicenvironmentand create a separate gated job for external PRs.Apply this diff to remove the invalid dynamic environment on the trusted job:
- # SECURITY: Use environment protection for external contributors - environment: ${{ github.event.pull_request.head.repo.full_name != github.repository && 'external-testing' || '' }}Then add a gated job for external PRs (example):
external-pr: name: External PR (gated) runs-on: ubuntu-latest if: github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository environment: external-testing permissions: contents: read pull-requests: read steps: - name: Checkout trusted base uses: actions/checkout@v4 with: ref: ${{ github.sha }} - name: Install Node.js uses: actions/setup-node@v4 with: node-version: 20.x cache: npm cache-dependency-path: package-lock.json - name: Install dependencies run: npm ci --no-fund --no-audit - name: Build run: npm run build - name: Run tests run: npm run test env: AWS_BUCKET: ${{secrets.AWS_BUCKET}} AWS_ACCESS_KEY_ID: ${{secrets.AWS_ACCESS_KEY_ID}} AWS_SECRET_ACCESS_KEY: ${{secrets.AWS_SECRET_ACCESS_KEY}} AZURE_ACCOUNT_ID: ${{secrets.AZURE_ACCOUNT_ID}} AZURE_ACCOUNT_KEY: ${{secrets.AZURE_ACCOUNT_KEY}} AZURE_CONTAINER_NAME: ${{secrets.AZURE_CONTAINER_NAME}} AWS_REGION: ${{secrets.AWS_REGION}}
70-73: External PRs never run this job; environment gating is dead code.The
ifexcludes forked PRs, so approval is never requested. Either split jobs (preferred; see prior comment) or broaden the condition.If you keep a single job, minimally allow PR events:
if: | - github.event_name == 'push' || - (github.event.pull_request.head.repo.full_name == github.repository) + github.event_name == 'push' || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'pull_request_target')Note: With the above, ensure untrusted code paths are gated by an environment before exposing secrets.
2 participants
No description provided.