ci: run integration tests on forks#2
Conversation
- Change trigger from pull_request to pull_request_target so that
repository secrets (OPENAI_API_KEY, ANTHROPIC_API_KEY) are available
when CI runs on fork pull requests.
- Add 'ref: ${{ github.event.pull_request.head.sha }}' to the checkout
steps in test-pirate-agent and test-invalid-agent so the jobs test
the PR's code rather than the base branch (required when using
pull_request_target with uses: ./).
- Remove the 'Check if fork PR' steps and all if: steps.fork-check...
conditions from test-pirate-agent, test-mention-reply-toplevel, and
test-mention-reply-inline. The fork check was only needed to avoid
missing-secrets failures; pull_request_target makes it obsolete.
The mention-reply jobs already have 'if: github.event_name == workflow_dispatch'
so they are unaffected by the trigger change.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR introduces the canonical GitHub Actions "pwn requests" vulnerability: switching to pull_request_target + checking out the PR head SHA + running uses: ./ (locally-built action) allows any fork PR author to exfiltrate repository secrets. A medium-severity regression on push events (empty ref: expression) is also present.
| Severity | Finding | Verdict |
|---|---|---|
| 🔴 HIGH | pull_request_target + head SHA checkout + uses: ./ → secret exfiltration path |
CONFIRMED |
| 🟡 MEDIUM | ref: ${{ github.event.pull_request.head.sha }} is empty on push events |
LIKELY |
v2.1.2 adds --legacy-peer-deps to fix 'Cannot read properties of null (reading matches)' npm error that was causing the Lint job to fail. See: https://github.com/raven-actions/actionlint/releases/tag/v2.1.2
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The raven-actions/actionlint action fails with 'Cannot read properties of null (reading matches)' when run after pnpm/action-setup because the pnpm PATH shim intercepts npm. Replacing with a direct go install avoids the third-party action entirely.
pull_request_target is not allowed in this repo. Revert to the plain pull_request trigger and remove the ref: checkout overrides — default checkout behavior is correct for same-repo PRs. The fork-check removal and actionlint fix from prior commits are kept.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Review: COMMENT
Assessment: 🟡 NEEDS ATTENTION
The PR removes fork-check guards that were the only mechanism preventing fork PRs from attempting secret-dependent steps. With the pull_request trigger (unchanged), fork PRs receive empty strings for secrets — the old guards skipped those steps cleanly; now they run and fail.
Findings: 1 MEDIUM (CONFIRMED)
derekmisler
changed the title
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR removes all fork-detection guards from three E2E test jobs (test-pirate-agent, test-mention-reply-toplevel, test-mention-reply-inline). With the current pull_request trigger, GitHub already prevents fork PRs from accessing secrets, so there is no immediate exploit. However, the PR description explicitly states the goal is to switch to pull_request_target, which — combined with checkout of the PR head SHA and uses: ./ — is the textbook pwn-request attack vector.
After this PR, the only remaining protection for test-pirate-agent and test-invalid-agent (the two jobs that use uses: ./ with OPENAI_API_KEY) is GitHub's trigger-level secret isolation. That barrier disappears the moment the trigger changes. No compensating control (job-level owner-repo guard, GitHub Environment with required reviewers) is introduced.
See inline comments for specific concerns.
| permissions: | ||
| contents: read | ||
| steps: | ||
| - name: Check if fork PR |
There was a problem hiding this comment.
[HIGH] Fork-detection guards removed from test-pirate-agent — dangerous step toward pwn-request if trigger changes
This PR removes the "Check if fork PR" step and all if: steps.fork-check.outputs.is_fork != 'true' conditions from test-pirate-agent. While the current pull_request trigger means fork PRs already receive empty secrets (GitHub's built-in protection), the PR description explicitly states the intent to switch to pull_request_target.
Under pull_request_target + checkout of github.event.pull_request.head.sha + uses: ./, this guard was the only step-level protection against a fork contributor executing their own code with live OPENAI_API_KEY credentials. After this deletion, there is no step-level, job-level, or environment-level gate remaining on this job.
This creates a single-commit path to a critical secret exfiltration vulnerability: applying the stated pull_request_target trigger change without adding new guards would allow any fork PR to run arbitrary action.yml / dist/*.js code with live API keys.
Suggested mitigation: Before or alongside the trigger change, add either:
- A job-level guard:
if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name != 'pull_request_target' - A GitHub Environment with required reviewers on this job
| run: pnpm install --frozen-lockfile && pnpm build | ||
|
|
||
| - name: Run test | ||
| if: steps.fork-check.outputs.is_fork != 'true' |
There was a problem hiding this comment.
[HIGH] uses: ./ step in test-pirate-agent loses its fork guard — unguarded under pull_request_target
The if: steps.fork-check.outputs.is_fork != 'true' condition is removed from the "Run test" step, which executes the composite action from the local checkout (uses: ./) and passes openai-api-key: ${{ secrets.OPENAI_API_KEY }}.
Today (with pull_request trigger): secrets are empty for fork PRs — no immediate leak.
After the stated trigger change to pull_request_target: a fork contributor controls the checked-out action.yml and dist/*.js files that this step executes, and secrets.OPENAI_API_KEY is a live key.
Note that uses: ./ runs the fork's version of the action — even without the API key, a fork contributor can modify the composite action to leak GITHUB_TOKEN or perform other attacks using the runner's contents: read permission.
| env: | ||
| TEST_PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr_number }} | ||
| steps: | ||
| - name: Check if fork PR |
There was a problem hiding this comment.
[HIGH] Fork-detection guards removed from test-mention-reply-toplevel — mitigated today by job-level workflow_dispatch guard, but creates fragile state
The "Check if fork PR" step and all if: steps.fork-check.outputs.is_fork != 'true' step conditions are removed from test-mention-reply-toplevel. The job currently has a job-level if: github.event_name == 'workflow_dispatch' guard, which means fork PRs cannot trigger this job at all today — the removal has no immediate security impact.
However, the same concern applies: if the job-level guard is ever relaxed (e.g., to run on pull_request_target), there would be no step-level fallback. The Setup credentials step (which fetches AWS OIDC-derived OPENAI_API_KEY, ANTHROPIC_API_KEY, GITHUB_APP_TOKEN) would run unconditionally for fork contributors.
The workflow_dispatch guard provides sufficient protection for the current state, but the defense-in-depth layer is gone.
The PR description is incorrect, I need to update it. |
4 participants
Summary
Enable integration tests to run on fork PRs by removing fork-detection guards. These tests, even on forks, always run in this repo's context, which is where everything lives anyway.