From e6bdaa22c7892d968e5ac85f6ac16cdac3179487 Mon Sep 17 00:00:00 2001 From: Amarjeet LNU Date: Wed, 19 Aug 2026 19:34:57 -0700 Subject: [PATCH] fix(ci): guard fast-integ-tests to same-repo PRs The job failed at the checkout step on its first real run (it could never run on its own PR, since pull_request_target uses the base branch's workflow definition). actions/checkout refuses to place a fork's head commit on a runner in a pull_request_target job: Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. The refusal is correct. fast-integ-tests is the only job in this workflow that puts PR code on a runner, and that runner also assumes CI_AWS_ROLE_ARN, so a fork PR could edit conftest.py and read the credentials out. Every other job here either checks out the base ref only (detect-changes, for diffing) or has no checkout at all and hands PR code to CodeBuild via source-version-override (codestyle-doc-tests, unit-tests, integ-tests, and every job in pr-checks-master-v2.yml and fortress-scan.yml). Opting out with allow-unsafe-pr-checkout would make this repo the sole exception, so guard the job instead. Same-repo PRs stay covered: a branch here already requires push access, so checking it out grants its author nothing new. This is a stopgap, not the destination. Almost every PR to this repo is from a fork, so the suite is not currently a gate; moving it into CodeBuild alongside the others is the fix, and needs a project provisioned in the CDK that owns them. Recorded in the job comment and the suite README so the gap is not mistaken for coverage. --- .github/workflows/pr-checks-master.yml | 26 +++++++++++++++++-- .../tests/integ/train/shallow/README.md | 15 +++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr-checks-master.yml b/.github/workflows/pr-checks-master.yml index 24d0c8349b..1b2d319d05 100644 --- a/.github/workflows/pr-checks-master.yml +++ b/.github/workflows/pr-checks-master.yml @@ -272,12 +272,34 @@ jobs: fast-integ-tests: runs-on: ubuntu-latest needs: [detect-changes] - if: contains(fromJson(needs.detect-changes.outputs.submodules), 'sagemaker-train') + # Same-repo PRs only, because this is the one job here that puts PR code on a + # runner. actions/checkout refuses to place a fork's head commit in a + # pull_request_target job -- correctly: this job holds the base repo's + # GITHUB_TOKEN and assumes CI_AWS_ROLE_ARN, so a fork could edit conftest.py + # and read those credentials out. That is the "pwn request" shape, and on a + # public repo it is a live credential-exfiltration path, so the guard skips + # the job rather than overriding the refusal with allow-unsafe-pr-checkout. + # + # Known consequence: nearly every PR to this repo comes from a fork, so this + # is not yet a gate on the PRs that matter. Closing that gap means running + # the suite where the other three test jobs already run PR code -- inside + # CodeBuild via source-version-override, which never exposes the runner's + # token or secrets to it (see integ-tests above). That needs a + # -ci-sagemaker-train-fast-integ-tests project in the CDK that owns the + # existing ones. Until it exists this suite is effectively a local signal -- + # whether the daily CI-health run over refs/heads/master reaches + # tests/integ/train/shallow depends on that buildspec's selection, which also + # lives outside this repo. + if: >- + contains(fromJson(needs.detect-changes.outputs.submodules), 'sagemaker-train') + && github.event.pull_request.head.repo.full_name == github.repository steps: - uses: actions/checkout@v3 with: # pull_request_target checks out the base ref by default; these tests - # must run against the PR's code. + # must run against the PR's code. Safe only because the job is guarded + # to same-repo PRs above -- a branch in this repo already requires push + # access, so checking it out grants nothing its author lacks. ref: 'refs/pull/${{ github.event.pull_request.number }}/head' - name: Set up Python diff --git a/sagemaker-train/tests/integ/train/shallow/README.md b/sagemaker-train/tests/integ/train/shallow/README.md index 02de12214d..a1d8dd6a28 100644 --- a/sagemaker-train/tests/integ/train/shallow/README.md +++ b/sagemaker-train/tests/integ/train/shallow/README.md @@ -8,6 +8,21 @@ What this suite changes about the gate is not which job runs, but what the exist one selects: the `gpu_intensive` marks added here deselect the deep tests that submit a job and wait for it, and this suite covers those code paths instead. +> **Current limitation — the job only runs on same-repo PRs.** `fast-integ-tests` +> is the one job in `pr-checks-master.yml` that puts PR code on a GitHub runner, +> and that runner assumes `CI_AWS_ROLE_ARN`. `actions/checkout` refuses to place a +> fork's head commit in a `pull_request_target` job for exactly that reason, so the +> job is guarded to same-repo PRs. Almost every PR here comes from a fork, so in +> practice this is presently a **local** signal rather than a gate. The fix is to +> run it the way the other three test jobs run PR code — inside CodeBuild via +> `source-version-override`, which never exposes the runner's token or secrets to +> it — which needs a CodeBuild project provisioned alongside the existing ones. +> +> Until then, run this suite locally when changing a submission path, and note that +> the deselection described above trades gate coverage for coverage that depends on +> whether the daily CI-health buildspec reaches this directory — that selection +> lives outside this repo and is worth confirming. + ## What a passing test proves Each test submits a real `CreateTrainingJob`, asserts the service returned a