fix(security): scope cross-org PAT git credential in tests-runner + coverage-floor#119
Merged
Merged
Conversation
…behind opt-in tests-runner.yml and coverage-floor.yml wrote AUTOMERGE_PAT into a GLOBAL git insteadOf rewrite before running uv sync + pytest — both execute PR-controlled code on same-repo pull_request events, so any PR could read the cross-org PAT via git's global url config and exfiltrate it (found by codex review on wxa-secrets#28, 2026-07-16). Changes to both reusables' python paths: - Credential now lives in a scoped throwaway config file under $RUNNER_TEMP (exposed via GIT_CONFIG_GLOBAL), never in global config, and is deleted before the test invocation in every install branch. - Test invocation becomes `uv run --no-sync pytest` so tests can never trigger a credential-needing re-resolve. - New `use_pat_for_git_deps` input (default false): pull_request runs no longer materialize the PAT at all unless the caller explicitly opts in (repos with private git-URL deps, e.g. wxa_sanctions -> wxa-secrets). push:main runs (already-reviewed code) keep materializing it whenever the secret is forwarded, so coverage-floor's seed-PR flow is unaffected. - coverage-floor gets an always() scrub step so no later step (including the third-party sticky-comment action) can read the file on early-exit paths. Residual, documented: repos that opt in still expose the PAT to PR- controlled build hooks DURING dependency resolution — unavoidable while uv needs the credential to clone. Opt-in scopes that exposure to the two repos that need it instead of the whole fleet. selftest: test_workflow_guards.py gains a repo-wide "no global git config writes" guard and per-workflow gating/scrub/ordering assertions. Auto-merge rationale: manual merge — PRs to ci-workflows are always manual per policy, and this is secrets-handling + workflow surface. Co-Authored-By: Claude Fable 5 <[email protected]>
…eference (codex P1) Codex round-1 P1: on same-repo PRs the CALLER workflow is itself PR-controlled, so use_pat_for_git_deps is not a boundary against actors with push access — they can flip the input (or add any workflow) and read any repo-visible secret. That is GitHub's baseline for same-repo pull_request runs; no workflow input can prevent it. Response: - Reframe input/step docs: the gate defends the no-workflow-change surface (poisoned lockfiles / dep build hooks / test-code changes riding otherwise-legitimate PRs), not push-access actors. - New optional GIT_DEPS_PAT secret, preferred over AUTOMERGE_PAT for the insteadOf rewrite: a fine-grained read-only PAT scoped to the dep repos makes theft in the residual window nearly worthless. Seed-PR flow keeps using AUTOMERGE_PAT (needs write scope). - Selftest asserts the preference expression. Co-Authored-By: Claude Fable 5 <[email protected]>
"Not a pull_request" treated schedule / workflow_dispatch / feature- branch pushes as trusted and auto-materialized the PAT on unreviewed code. Flip the gate to an allowlist: auto-trust ONLY a push to the default branch; every other event requires use_pat_for_git_deps. Selftest now asserts the allowlist expression verbatim so a denylist regression trips CI. Co-Authored-By: Claude Fable 5 <[email protected]>
…d-4 P2) The global first-scrub < last-invocation comparison stayed green if a single branch's scrub moved below its own pytest. Each test invocation (line-anchored, so comments don't count) must now have a scrub AFTER the last install command preceding it. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Coverage Floor — mode:
|
…the window The prior wording said the opt-in "defends the no-workflow-change surface: poisoned lockfiles / dependency build hooks". That is backwards for an opted-in repo: build hooks and poisoned-lockfile installs run DURING `uv sync`, while the credential is present, and can read it. Only the post-install test phase is protected by the scrub. Reword the input descriptions + step comments: the real fleet protection is the default of forwarding nothing; opting in accepts a residual install-phase window bounded by allowlist gating + the least-privilege GIT_DEPS_PAT. Docs-only; selftest guards unaffected (11/11 pass). Co-Authored-By: Claude Fable 5 <[email protected]>
Input descriptions claimed "only the post-install test phase is protected by the scrub" — true for the default path, but a caller test_command runs install+tests as one command BEFORE the scrub, so the credential is present during those tests too (the code comments already say this; the user-facing input descriptions now do too). Co-Authored-By: Claude Fable 5 <[email protected]>
The security-model step comments promised deletion "before the test invocation" unconditionally, contradicting the input description + code branch that acknowledge a caller test_command runs install+tests together (credential persists through it). Qualify both step comments to match. Co-Authored-By: Claude Fable 5 <[email protected]>
…_command Comprehensive sweep of the last comment/message locations that still promised an unconditional pre-test scrub (install-step comments, the "configured insteadOf" echo lines, and the selftest docstrings/messages). All now say "before the DEFAULT test invocation" and note that a caller test_command runs install+tests as one command and is scrubbed only afterward. No logic change; 11/11 selftests pass. Co-Authored-By: Claude Fable 5 <[email protected]>
topcoder1
marked this pull request as ready for review
July 17, 2026 04:34
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.") |
|
No issues found. Credential expression, per-branch scrub ordering, and |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
tests-runner.ymlandcoverage-floor.ymlwroteAUTOMERGE_PATinto a global gitinsteadOfrewrite, then ranuv sync(build hooks) andpytest— both PR-controlled code on same-repopull_requestevents. Any PR could read the cross-org PAT viagit config --get-regexp --global urland exfiltrate it. Found by codex review on wxa-secrets#28 (2026-07-16).The fix (both reusables' Python paths)
insteadOfrewrite goes into a throwaway$RUNNER_TEMP/cross-org-gitconfigfile exposed to the install step viaGIT_CONFIG_GLOBAL, and is deleted before the test invocation in every install branch. A belt-and-suspendersalways()scrub step in coverage-floor catches early-exit paths (so even the third-party sticky-comment action can't read it).uv run --no-sync pytestso the test invocation can never trigger a credential-needing re-resolve after the scrub.use_pat_for_git_depsinput (defaultfalse). The credential auto-materializes only on a push to the default branch (already-reviewed code); PRs, branch pushes,schedule, andworkflow_dispatchall require the explicit input. coverage-floor's push:main seed-PR flow is unaffected.GIT_DEPS_PAT(fine-grained, read-only, scoped to just the dep repos) is preferred overAUTOMERGE_PATfor the rewrite, so theft in the residual dependency-resolution window yields read access to a couple of repos rather than the fleet automerge credential.Honest boundary (documented in the input description)
use_pat_for_git_depsis not a defense against actors with push access — on same-repo PRs the caller workflow is itself PR-controlled, so such an actor can flip the input or add a workflow and read any repo-visible secret (GitHub's baseline; no input changes it). The gate defends the no-workflow-change surface: poisoned lockfiles, dependency build hooks, and test-code changes riding otherwise-legitimate PRs (dependabot bumps, generated PRs).Verification
selftest/test_workflow_guards.py: repo-wide "nogit config --globalwrites" guard + per-workflow assertions for the allowlist gate,GIT_DEPS_PATpreference, scoped file, and per-invocation local scrub-ordering (eachpytesthas a scrub after its own install, line-anchored). 11/11 pass.actionlint: no new findings (5 pre-existing SC2006/SC2016 style notes on lines this PR doesn't touch).test_commandscrub exception across every comment/message/docstring (P2s). Ended on two consecutive clean rounds. Codex rounds: 11.Auto-merge rationale
Manual merge — PRs to
ci-workflowsare always manual per policy, and this is secrets-handling + workflow surface. Reviewer should sanity-check the twoCROSS_ORG_PAT:gating expressions and the scrub placement.🤖 Generated with Claude Code