ci(build-ci-image): use pull_request_target so fork PRs can access DEPOT_PROJECT_ID#7327
Conversation
…OT_PROJECT_ID Same fix as flyte-binary-v2: pull_request from a fork doesn't receive vars.*, so DEPOT_PROJECT_ID is empty and Validate fails. pull_request_target runs in the base-repo context where vars are available. Checkout pins to the PR head SHA (with persist-credentials: false) so fork-authored Dockerfile content is what gets built. Updated event_name guards to also match pull_request_target. Signed-off-by: Kevin Su <pingsutw@apache.org>
There was a problem hiding this comment.
Pull request overview
Updates the “Build and Publish CI Docker Image” GitHub Actions workflow to run for fork PRs by switching to pull_request_target, so repository variables (notably vars.DEPOT_PROJECT_ID) are available and Depot builds can proceed.
Changes:
- Switch workflow trigger from
pull_requesttopull_request_targetand document the security tradeoff. - Pin checkout to the PR head SHA (with
persist-credentials: false) so the PR’s Dockerfile/content is built underpull_request_target. - Update PR-only conditionals to also run under
pull_request_target.
Comments suppressed due to low confidence (1)
.github/workflows/build-ci-image.yml:14
- Switching this workflow to
pull_request_targetmakes the job run with base-repo permissions (packages: write,pull-requests: write,id-token: write) and it currently logs into GHCR withsecrets.GITHUB_TOKENand pushes images unconditionally. For fork PRs this means untrusted PR code can publish images to the base repo’s GHCR namespace, and becausedocker/metadata-actionenableslatest/v2based ongithub.ref(which is the base branch ref underpull_request_target), a fork PR targetingmaincan end up tagging/pushing:latest(and similarly forv2). This also creates a supply-chain risk for privileged workflows that pullci:pr-<N>(e.g./regenjobs). Please gate GHCR login/push (and anypackages: write) to pushes and/or in-repo PRs only (e.g.head.repo.full_name == github.repository+ label), and ensure PR builds only ever producepr-<N>tags (neverlatest/v2).
pull_request_target:
paths:
- 'gen.Dockerfile'
- '.github/workflows/build-ci-image.yml'
workflow_dispatch:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
build-ci-image.yml now triggers on pull_request_target (so fork PRs get vars.DEPOT_PROJECT_ID), but check-generate filtered listWorkflowRuns by event='pull_request', so it never found the run and timed out after 20m. Match by head_sha only — sufficient since the head SHA uniquely identifies the PR build. Signed-off-by: Kevin Su <pingsutw@apache.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - '.github/workflows/build-ci-image.yml' | ||
| pull_request: | ||
| pull_request_target: | ||
| paths: | ||
| - 'gen.Dockerfile' | ||
| - '.github/workflows/build-ci-image.yml' |
There was a problem hiding this comment.
Switching this workflow to pull_request_target changes github.ref to the PR base branch (e.g. refs/heads/main). With the current docker/metadata-action config and unconditional push: true, fork PRs can end up pushing branch tags (e.g. main) and also latest/v2 (because the enable expressions use github.ref). This contradicts the PR description’s “pr- only” guarantee and is a security/operational risk (tag overwrite). Consider gating pushing to base-repo PRs only (e.g. head.repo.full_name == github.repository) and/or making tags PR-scoped when event_name == 'pull_request_target' (disable latest/v2/branch tags on PR runs).
Why
Same root cause as #7320 (which fixed `flyte-binary-v2.yml`): the `Build and Publish CI Docker Image` workflow validates and uses `vars.DEPOT_PROJECT_ID`, but GitHub does not pass repository variables to workflows triggered by `pull_request` events from forks. As a result, the `Validate Depot project id` step fails on every fork PR (e.g. PR #7326 from `Sovietaced/flyte`):
```
##[error]DEPOT_PROJECT_ID repo variable is not set.
```
What changed
Security considerations
Test plan