fix(ci): scope the release credential and stop persisting it to disk (#3062)
## 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 <tejas@Tejass-MacBook-Pro.local>
This commit is contained in:
Tejas Chopra
@@ -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}"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user