Allow outside contributors to run CI with secrets when approved#1782
Allow outside contributors to run CI with secrets when approved#1782
Conversation
|
Greptile SummaryThis PR modifies the GitHub Actions CI workflow to allow fork contributors' PRs to trigger CI runs by switching the trigger event from Critical concerns:
Confidence Score: 1/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pull_request_target event\n(opened / synchronize / labeled / unlabeled)"] --> B{Is PR from fork?}
B -- "No (same repo)" --> C[All jobs run normally]
B -- "Yes (fork)" --> D{Approval gate?}
D -- "MISSING ❌" --> E["Jobs run with secrets exposed\n(OPENAI, ANTHROPIC, BROWSERBASE, etc.)"]
E --> F["server-integration-tests\n✅ fork check removed"]
E --> G["run-evals\n✅ fork check removed"]
E --> H["run-e2e-local-tests\n❌ fork check still present\n(job is skipped)"]
E --> I["run-e2e-bb-tests\n❌ fork check still present\n(job is skipped)"]
C --> J["run-build"]
C --> K["run-lint"]
C --> F
C --> G
C --> H
C --> I
Last reviewed commit: 4167620 |
Additional Comments (2)
Switching from While the current checkout steps don't explicitly check out fork code (so fork-submitted scripts can't run), secrets like The PR title mentions "when approved", but there is no actual approval gate implemented in this workflow. A common safe pattern is to require a maintainer to apply a specific label (e.g., on:
pull_request_target:
types:
- opened
- synchronize
- labeled
- unlabeled
jobs:
check-approval:
if: >
github.event.pull_request.head.repo.full_name == github.repository ||
contains(github.event.pull_request.labels.*.name, 'safe-to-test')
...Without this gate, any contributor who opens a fork PR can trigger jobs that consume your API credits.
The As a result, fork PRs will run server integration tests and evals (which consume API credits for OPENAI, ANTHROPIC, and BROWSERBASE), but not the e2e local or Browserbase e2e tests. Either all jobs should allow fork PRs (with a proper approval gate in place), or none should — the current partial state is confusing and inconsistent. |
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 1/5
- There are two high-confidence, high-severity CI workflow issues in
.github/workflows/ci.yml, including a 9/10 finding that can invalidate PR verification by running CI against the base branch instead of the submitted PR changes. - The
pull_request_target+ defaultactions/checkout@v4behavior is merge-blocking risk because regressions can slip through untested, creating a strong chance of functional breakage after merge. - The removed fork guards on secret-using jobs (
server-integration-tests,run-evals) create a concrete security exposure path for sensitive keys, which is critical and should be fixed before merging. - Pay close attention to
.github/workflows/ci.yml- PR validation is misconfigured and secret-handling protections were weakened.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:4">
P0: Critical: `pull_request_target` with default `actions/checkout@v4` checks out the **base branch**, not the PR code. The CI will never test any changes from the pull request — it will always run against the existing base branch and appear to pass.
To actually test the PR code, every `checkout` step needs `ref: ${{ github.event.pull_request.head.sha }}`. However, doing so with `pull_request_target` would expose all repository secrets to arbitrary fork code (see next comment). The recommended safe pattern is to keep `pull_request` for building/testing untrusted code, and only use `pull_request_target` for a separate lightweight workflow that gates on an approval label before dispatching the secret-requiring jobs.</violation>
<violation number="2" location=".github/workflows/ci.yml:377">
P1: Security: Fork-guard conditions removed from secret-using jobs. The `server-integration-tests` and `run-evals` jobs expose sensitive API keys (`OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `BROWSERBASE_API_KEY`, `BRAINTRUST_API_KEY`, etc.) and previously required `github.event.pull_request.head.repo.full_name == github.repository` to prevent fork PRs from accessing them.
With `pull_request_target`, secrets are available to all triggered jobs regardless of PR origin. Removing these guards means any fork PR can trigger resource-consuming jobs with secret access. Consider implementing a label-based approval gate (e.g., require an `approved-to-run` label set by a maintainer) before running secret-using jobs.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Ext as Outside Contributor (Fork)
participant GHA as GitHub Actions Runner
participant Mnt as Maintainer
participant CI as CI Workflow (.yml)
participant Jobs as Integration & Eval Jobs
Note over Ext, Jobs: Flow for Forked Pull Request
Ext->>GHA: NEW: Push code & Open PR (pull_request_target)
alt Needs Approval (GitHub Policy)
Mnt->>GHA: Approve "Run Workflow"
end
GHA->>CI: Trigger Workflow in context of Base Repo
Note over CI: Workflow has access to Secrets/Env
CI->>CI: Run: build-server-sea / discover-tests
rect rgb(23, 37, 84)
Note right of CI: CHANGED: Guards removed (repo check)
CI->>Jobs: NEW: Trigger server/integration tests
CI->>Jobs: NEW: Trigger evaluation jobs
end
Jobs-->>GHA: Return Test Results
GHA-->>Ext: Display CI Status in PR UI
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ed" (#1786) Reverts #1782 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Reverts the approval-based CI for external contributors. CI now runs on pull_request and blocks secrets for forked PRs by skipping integration, E2E, and eval jobs. - **Refactors** - Removed the “Ensure Contributor Is Trusted to Run CI” workflow. - Switched CI trigger to pull_request; removed workflow_run logic. - Read labels from github.event.pull_request; removed API calls. - Simplified checkouts; dropped explicit head_sha refs. - Updated concurrency group to use github.ref. - Ignored docs-only changes in CI. <sup>Written for commit d6ace82. Summary will update on new commits. <a href="https://cubic.dev/pr/browserbase/stagehand/pull/1786">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. -->
PSA potential hackers: dont get excited, we don't have any real secrets in CI worth stealing, and our CI does not autodeploy anything to prod. All important secrets and CD processes are kept in our closed-source repos.
why
what changed
test plan
Summary by cubic
Add a gating workflow that blocks CI until a maintainer approves running secrets on forked PRs. CI now triggers from that gate, resolves labels and path filters under workflow_run, removes same-repo guards so integration/e2e/evals run on approved forks, and checks out the PR commit consistently across jobs.
Written for commit c682847. Summary will update on new commits. Review in cubic