From ac8646aa3c6323c3c0b7051e09831f779859af6f Mon Sep 17 00:00:00 2001 From: Tejas Chopra Date: Sun, 16 Aug 2026 19:05:32 -0700 Subject: [PATCH] fix(ci): scope the release credential and stop persisting it to disk (#3062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `RELEASE_PLEASE_TOKEN` is currently a maintainer's personal PAT. It bypasses branch and tag protection on `main` (`release-please.yml` says so in its own comment), and forging a tag with it fires `release.yml` and `docker.yml` on `release: published`, which publish to PyPI, npm and GHCR. If it is a classic token with `repo` scope it is also valid against every other repository that account can reach. `release-metadata-sync.yml` made that credential readable on the runner. `actions/checkout` defaults to `persist-credentials: true`, writing the token into `.git/config`, and the very next step runs `scripts/version-sync.py` **from the checked-out branch**. The trigger is a push to the glob `release-please--branches--**`, which is not a protected namespace, so a principal with push access could land a modified `version-sync.py` and read it. Closes #2955. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Changes Made - Both release workflows now prefer a GitHub App installation token — scoped to this repository, expiring in an hour — over the PAT, via `actions/create-github-app-token@v3`. - The minting step is gated on `vars.RELEASE_APP_ID` and marked `continue-on-error`, so an unconfigured app falls through to the existing `PAT -> GITHUB_TOKEN` chain and nothing breaks today. - `release-metadata-sync.yml`'s checkout no longer persists credentials, and no longer receives a token at all. - The final push supplies the credential through the step's own `env` and an explicit remote URL, so it is never on disk while branch-supplied code runs. ## Testing - [x] Unit tests pass - [x] Linting passes (ruff check + format on the test file) - [ ] Type checking passes — N/A (YAML + test only) - [x] New tests added for new functionality ### Test Output ```text $ .venv/bin/python -m pytest tests/test_release_workflows.py -q 1 failed, 44 passed, 1 skipped in 0.23s ``` The single failure is `test_no_native_tls_in_wheel_build_tree`, which shells out to `cargo`. It reproduces identically on unmodified `main` on this machine (no Rust toolchain installed) and is unrelated to this change. New tests only: ```text $ .venv/bin/python -m pytest tests/test_release_workflows.py -q -k "persist_credentials or scoped_app_token" 3 passed, 46 deselected in 0.18s ``` Against the parent commit: ```text FAILED test_metadata_sync_does_not_persist_credentials_for_branch_supplied_code FAILED test_release_workflows_prefer_scoped_app_token[release-please.yml-release-please] FAILED test_release_workflows_prefer_scoped_app_token[release-metadata-sync.yml-sync] 3 failed, 46 deselected ``` ## Real Behavior Proof - Environment: macOS 15 (darwin 25.4.0), Python 3.12.13; workflows parsed with PyYAML, not executed on a runner. - Exact command / steps: parse both workflow files and assert (a) every `actions/checkout` step sets `persist-credentials: false` and receives no `token`, (b) exactly one gated `create-github-app-token` step exists per workflow, and (c) every credential consumer places `steps.app-token.outputs.token` ahead of `secrets.RELEASE_PLEASE_TOKEN` in its fallback chain. - Observed result: all three assertions pass on this branch and fail on the parent commit. Both files remain valid YAML. - **Not tested — important:** none of this has executed on a GitHub runner. I have not minted a real installation token, not confirmed the app-token step's `continue-on-error` fallback behaves as expected when `vars.RELEASE_APP_ID` is unset, and not performed a real push with the explicit-remote-URL form. The first live release run is the real test. ## Runtime Rollout Safety - Rollout-managed feature(s): none. - Minimum rollout channel: N/A. - Stable/default behavior changed: no, unless `vars.RELEASE_APP_ID` is set — without it both workflows resolve to exactly today's credential chain. - Kill switch / disable path: unset `vars.RELEASE_APP_ID` to fall back to the PAT. - Unsafe override required: none. - Qualification impact: none. - Rollback path: revert this commit. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes ## Additional Notes **This narrows blast radius; it does not make the trigger safe on its own.** For an `on: push` workflow GitHub reads the workflow file from the pushed ref, so a principal with push access can still edit this file on their branch. The durable fix is the scoped app token *plus revoking the personal PAT* — the revocation is a console action and is deliberately not in this commit. **Two repo settings are required to actually complete #2955**, and neither can land in git: ``` vars.RELEASE_APP_ID (repository variable) secrets.RELEASE_APP_PRIVATE_KEY (repository secret) ``` Until those exist this PR is a no-op on behavior and a defense-in-depth improvement on the `persist-credentials` path only. Co-authored-by: Tejas Chopra --- .github/workflows/release-metadata-sync.yml | 38 +++++++++--- .github/workflows/release-please.yml | 33 ++++++++--- tests/test_release_workflows.py | 66 +++++++++++++++++++++ 3 files changed, 121 insertions(+), 16 deletions(-) diff --git a/.github/workflows/release-metadata-sync.yml b/.github/workflows/release-metadata-sync.yml index ef775cf3b..2526b9770 100644 --- a/.github/workflows/release-metadata-sync.yml +++ b/.github/workflows/release-metadata-sync.yml @@ -50,15 +50,26 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 steps: + # Prefer a short-lived, repo-scoped GitHub App installation token over a + # personal PAT. Gated on the repo variable so an unconfigured app simply + # falls through to the existing chain instead of breaking the release. + - name: Mint installation token + id: app-token + if: ${{ vars.RELEASE_APP_ID != '' }} + continue-on-error: true + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ vars.RELEASE_APP_ID }} + private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + - uses: actions/checkout@v7 with: ref: ${{ github.ref_name }} - # PAT (not GITHUB_TOKEN) for the same reason release-please.yml uses one: - # a push made with GITHUB_TOKEN does not trigger workflows, so the release - # PR's checks would never re-run against the synced commit and would stay - # red. Falls back to GITHUB_TOKEN, where the sync still lands and a manual - # re-run of the PR's checks picks it up. - token: ${{ secrets.RELEASE_PLEASE_TOKEN || secrets.GITHUB_TOKEN }} + # Do NOT persist the credential into .git/config. The next step runs + # scripts/version-sync.py *from the checked-out branch*, and this job + # triggers on a push to the unprotected glob release-please--branches--**. + # A persisted token would be readable by that script. + persist-credentials: false - uses: actions/setup-python@v6 with: @@ -72,6 +83,14 @@ jobs: run: python scripts/verify-versions.py - name: Commit and push if anything changed + env: + # An app installation token if one was minted, else the existing + # chain. A PAT (not GITHUB_TOKEN) is still preferred here for the same + # reason release-please.yml wants one: a push made with GITHUB_TOKEN + # does not trigger workflows, so the release PR's checks would never + # re-run against the synced commit and would stay red. Supplied only + # to this step, after the branch-supplied script has already run. + SYNC_TOKEN: ${{ steps.app-token.outputs.token || secrets.RELEASE_PLEASE_TOKEN || secrets.GITHUB_TOKEN }} run: | if git diff --quiet; then echo "Already in sync — nothing to commit." @@ -81,7 +100,12 @@ jobs: git config user.email "41898282+github-actions[bot]@users.noreply.github.com" git add -A git commit -m "chore: sync generated version metadata" + # Push via an explicit remote URL because the checkout no longer + # persists credentials. Passed on stdin-free env expansion so the + # token is not written to the command line or into .git/config. # This push re-triggers this workflow. version-sync.py is idempotent, so # the next run finds no diff and exits above without pushing — the loop # terminates after one no-op run. - git push origin HEAD:"${GITHUB_REF_NAME}" + git push \ + "https://x-access-token:${SYNC_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" \ + HEAD:"${GITHUB_REF_NAME}" diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index ce458bfd8..c40893be2 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -42,16 +42,31 @@ jobs: release-please: runs-on: ubuntu-latest steps: + # Prefer a short-lived, repo-scoped GitHub App installation token. A + # personal PAT carries the maintainer's whole account — with a classic + # `repo` scope that reaches every other repository they can access — and + # this credential can tag past branch protection and reaches PyPI, npm and + # GHCR through the `release: published` publishes. An installation token is + # scoped to this repository and expires in an hour. Gated on the repo + # variable so an unconfigured app falls through instead of blocking a + # release. See #2955. + - name: Mint installation token + id: app-token + if: ${{ vars.RELEASE_APP_ID != '' }} + continue-on-error: true + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ vars.RELEASE_APP_ID }} + private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + - uses: googleapis/release-please-action@v5 with: - # PAT (not GITHUB_TOKEN): a release/tag created by GITHUB_TOKEN does - # NOT emit events that trigger other workflows, so release.yml - # (PyPI/npm) and docker.yml — which fire on `release: published` — - # never ran, and releases had to be cut by hand. A PAT is treated as a - # real user, so the release it creates DOES trigger those publishes; it - # also lets the bot tag past branch/tag protection. Falls back to - # GITHUB_TOKEN when the secret is unset (the release PR still opens; it - # just won't trigger the downstream publishes). - token: ${{ secrets.RELEASE_PLEASE_TOKEN || secrets.GITHUB_TOKEN }} + # Neither an app token nor a PAT is GITHUB_TOKEN, and that matters: a + # release/tag created by GITHUB_TOKEN does NOT emit events that trigger + # other workflows, so release.yml (PyPI/npm) and docker.yml — which fire + # on `release: published` — never ran, and releases had to be cut by + # hand. Falls back to GITHUB_TOKEN when nothing else is set (the release + # PR still opens; it just won't trigger the downstream publishes). + token: ${{ steps.app-token.outputs.token || secrets.RELEASE_PLEASE_TOKEN || secrets.GITHUB_TOKEN }} config-file: .release-please-config.json manifest-file: .release-please-manifest.json diff --git a/tests/test_release_workflows.py b/tests/test_release_workflows.py index 207827802..d43e1ce6d 100644 --- a/tests/test_release_workflows.py +++ b/tests/test_release_workflows.py @@ -1467,3 +1467,69 @@ def test_version_sync_covers_every_file_the_verifier_gates() -> None: "server.json", ]: assert fragment in sync, f"version-sync.py no longer propagates a version to {fragment}" + + +def test_metadata_sync_does_not_persist_credentials_for_branch_supplied_code() -> None: + """The release credential must not be readable by the synced branch's code. + + ``release-metadata-sync`` triggers on a push to the ``release-please--branches--**`` + glob, which is not a protected namespace, and then runs + ``scripts/version-sync.py`` *from the checked-out branch*. With + ``actions/checkout``'s default ``persist-credentials: true`` the token is + written to ``.git/config`` before that script runs, so anyone able to push a + matching branch could read it. The credential reaches PyPI, npm and GHCR via + the ``release: published`` publishes, so this is not a theoretical leak. + """ + workflow = yaml.safe_load( + (ROOT / ".github/workflows/release-metadata-sync.yml").read_text(encoding="utf-8") + ) + steps = workflow["jobs"]["sync"]["steps"] + + checkouts = [s for s in steps if str(s.get("uses", "")).startswith("actions/checkout")] + assert checkouts, "expected a checkout step" + for step in checkouts: + assert step.get("with", {}).get("persist-credentials") is False, step + # A token passed to checkout is exactly what persist-credentials would + # write to disk; the push step supplies it via env instead. + assert "token" not in step.get("with", {}), step + + +@pytest.mark.parametrize( + "workflow_path,job", + [ + (".github/workflows/release-please.yml", "release-please"), + (".github/workflows/release-metadata-sync.yml", "sync"), + ], +) +def test_release_workflows_prefer_scoped_app_token(workflow_path: str, job: str) -> None: + """A repo-scoped, short-lived app token must be preferred over the PAT. + + The PAT carries a maintainer's entire account and bypasses branch and tag + protection (#2955). The app-token step is gated on ``vars.RELEASE_APP_ID`` + and marked ``continue-on-error`` so an unconfigured app falls back to the + existing chain rather than blocking a release. + """ + workflow = yaml.safe_load((ROOT / workflow_path).read_text(encoding="utf-8")) + steps = workflow["jobs"][job]["steps"] + + minters = [ + s for s in steps if str(s.get("uses", "")).startswith("actions/create-github-app-token") + ] + assert len(minters) == 1, steps + minter = minters[0] + assert minter["id"] == "app-token" + assert minter["continue-on-error"] is True + assert "vars.RELEASE_APP_ID" in str(minter["if"]) + + # Whatever consumes the credential must try the app token first. + consumers = [ + value + for step in steps + for value in list(step.get("with", {}).values()) + list(step.get("env", {}).values()) + if "RELEASE_PLEASE_TOKEN" in str(value) + ] + assert consumers, "expected a step consuming the release credential" + for value in consumers: + assert str(value).index("steps.app-token.outputs.token") < str(value).index( + "secrets.RELEASE_PLEASE_TOKEN" + ), value