Skip to content

Allow unsafe PR checkout on the integ and code review workflows. - #645

Open
simonmarty wants to merge 1 commit into
mainfrom
allow-unsafe-pr-checkout
Open

Allow unsafe PR checkout on the integ and code review workflows.#645
simonmarty wants to merge 1 commit into
mainfrom
allow-unsafe-pr-checkout

Conversation

@simonmarty

Copy link
Copy Markdown
Contributor

Description

Why is this change being made?

  1. The code review and integ workflows retrieve PR code to review and test it.
  2. These workflows must run as pull_request_target to perform OIDC with AWS roles from the upstream repo.
  3. Github updated the checkout action to require a flag to checkout unsafe code in pull_request_target backport allow-unsafe-pr-checkout to v6 actions/checkout#2500
  4. We already gate these workflows on environment review and label application, respectively.

What is changing?

  1. Add the flag.

Related Links

  • Issue #, if available:

Testing

How was this tested?

  1. No testing.

When testing locally, provide testing artifact(s):


Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why:
  • Integration tests pass locally
    If not, why:
  • I have updated integration tests (if needed)
    If not, why:
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why:

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty
simonmarty requested a review from a team as a code owner July 28, 2026 21:42
@simonmarty
simonmarty deployed to code-review July 28, 2026 21:42 — with GitHub Actions Active
@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 1m 35s —— View job


Review: PR #645 — Allow unsafe PR checkout on integ and code review workflows

  • Read the changed workflow files
  • Cross-check all actions/checkout usages in the repo
  • Verify the flag name and post review

The change is small, correct, and consistent with the referenced upstream change (actions/checkout#2500). Both edits add allow-unsafe-pr-checkout: true on checkouts that are triggered by pull_request_target and explicitly point ref at the PR head SHA, so the new flag is required for them to keep functioning. 👍

Correctness

  • build-test-image.yml:44 — the checkout uses ref: …pull_request.head.sha || github.sha, so under pull_request_target it genuinely checks out untrusted PR code and now needs the flag. ✅
  • code-review.yml:22 — same story: ref: github.event.pull_request.head.sha under pull_request_target. ✅

Security

This is the correct place to be cautious, and the existing gates are appropriately in place:

  • build-test-image.yml only runs the PR path when the safe-to-test label is present (contains(github.event.pull_request.labels.*.name, 'safe-to-test')), and that path holds id-token: write / packages: write. Requiring a maintainer-applied label before untrusted code is checked out with those permissions is the right mitigation.
  • code-review.yml is guarded by environment: code-review, so a maintainer/environment protection rule gates the run before untrusted code is fetched with id-token: write + pull-requests: write.

Both align with the stated rationale in the description. No new secret exposure is introduced by this change itself — it just re-enables behavior the checkout action recently began blocking by default.

One thing worth confirming (not a blocker)

integ.yml:104 also runs actions/checkout@v6, but with no ref — so under pull_request_target it checks out the base repo's default ref rather than the PR head. That's why it correctly does not need allow-unsafe-pr-checkout, and leaving it off is the safer choice. Just flagging the implication: the integration tests (bats scripts, manifests, etc.) run from the base branch, not the PR. So a PR that modifies integration tests won't have those modified tests exercised by this workflow — the PR-built image is tested against base-branch test code. That's a reasonable security posture (don't execute untrusted test scripts that have AWS OIDC credentials), but if the intent was ever to test PR-side test changes, that won't happen here. This is pre-existing behavior and out of scope for this PR — just make sure it matches your expectations.

Nits

  • Consider a short inline comment next to the flag (e.g. # required by actions/checkout to check out PR code under pull_request_target; gated by <label/environment>) so future readers understand why an "unsafe" flag is intentionally enabled. Minor, optional.

Summary

LGTM. The two-line change is minimal and correct, the flag name matches the upstream action's new input, and the pre-existing label/environment gates provide the necessary protection for checking out untrusted PR code under pull_request_target.

@simonmarty simonmarty added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.97%. Comparing base (6ce6999) to head (6dc2f24).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #645   +/-   ##
=======================================
  Coverage   60.97%   60.97%           
=======================================
  Files          11       11           
  Lines         720      720           
=======================================
  Hits          439      439           
  Misses        263      263           
  Partials       18       18           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants