Repository navigation
Conversation
Avoid overwriting a verified default-branch checkout with stale activation config. Retain snapshot restoration for other checkouts and cover concurrent base updates with a real-Git regression. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if [ -n "${DEFAULT_BRANCH:-}" ] && | ||
| HEAD_SHA=$(git -C "${WORKSPACE}" rev-parse --verify 'HEAD^{commit}' 2>/dev/null) && | ||
| BASE_SHA=$(git -C "${WORKSPACE}" rev-parse --verify "refs/remotes/origin/${DEFAULT_BRANCH}^{commit}" 2>/dev/null) && | ||
| [ "${HEAD_SHA}" = "${BASE_SHA}" ]; then | ||
| echo "Keeping agent config folders from the checked-out trusted base commit ${HEAD_SHA}" | ||
| FOLDERS=() | ||
| ROOT_FILES=() | ||
| fi |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No additional blocking issues found in the trusted-base restore guard or its regression coverage.
Notes
I checked the exact-commit bypass, the PR-head and missing-ref fallback paths, and the new real-Git test. The change stays narrowly scoped to the one case it is trying to preserve and keeps the previous restoration behavior everywhere else.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 33.1 AIC · ⌖ 5.55 AIC · ⊞ 19.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes due to a confirmed, reproducible security hole in the fix itself.
📋 Key Themes & Highlights
Key Themes
- Security regression in the new guard (critical): The base-ref lookup uses
git rev-parse --verifyon arefs/remotes/origin/<branch>^{commit}expression, which does not require an exact ref-path match — it falls back through git's standard ref disambiguation order, includingrefs/heads/<name>. A fork PR branch literally namedrefs/remotes/origin/mainbecomesrefs/heads/refs/remotes/origin/mainafter the agent-job checkout (git checkout -B <headRef> ...), and in setups without a real localrefs/remotes/origin/<default>tracking ref (pull_requesttriggers viaactions/checkout's shallowrefs/pull/N/mergefetch, or non-default-branch bases), this resolves to the attacker's own commit. SinceHEAD_SHAthen equals this spoofedBASE_SHA, the script treats the fork PR as trusted and skips config restoration entirely — the exact class of attack (.github/,.claude/,AGENTS.mdinjection) this PR exists to prevent. I reproduced this locally in a throwaway repo and confirmed the vulnerable match, then confirmedgit show-ref --verify --hashon the exact ref path correctly rejects it (no fallback). See inline comment for the concrete fix and a suggested regression test to add to the existing table-drivenTestRestoreConfigWhenBaseAdvancesinpr_test.go. - This matches a finding GitHub Advanced Security already flagged on this PR (comment id 4276470596) that has not yet been addressed in the current diff.
Positive Highlights
- ✅ Good threat-model framing in the script comment: explicitly calling out that an ancestry relationship alone must never bypass restoration, and that only an exact commit match should.
- ✅ The new
TestRestoreConfigWhenBaseAdvancestable test is well structured (real Git repos, four realistic modes: trusted base, PR head, missing base ref, missing default-branch input) and gives good coverage of the intended semantics — it just doesn't yet cover the ref-namespace-confusion case above. - ✅ Docs updates in
copilot.mdandsafe-outputs-pull-requests.mdclearly explain the new trusted-checkout exception and its boundaries for users.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 63 AIC · ⌖ 14.9 AIC · ⊞ 11.1K
Comment /matt to run again
| echo "Keeping agent config folders from the checked-out trusted base commit ${HEAD_SHA}" | ||
| FOLDERS=() | ||
| ROOT_FILES=() | ||
| fi |
There was a problem hiding this comment.
[/diagnosing-bugs] 🔴 Critical: git rev-parse --verify "refs/remotes/origin/${DEFAULT_BRANCH}^{commit}" does not do an exact-path ref lookup — it falls back through git's standard ref search order (including refs/heads/<name>). This reopens the exact attack this PR is meant to close.
💡 Exploit and fix
A fork PR can name its branch refs/remotes/origin/main. git checkout -B <headRef> origin/pr-head then creates a local branch literally at refs/heads/refs/remotes/origin/main. When the workspace has no real refs/remotes/origin/<default> (e.g. pull_request runs where actions/checkout only fetched refs/pull/N/merge, or the PR base isn't the default branch), rev-parse resolves BASE_SHA to the attacker's commit, which equals HEAD_SHA — so FOLDERS/ROOT_FILES are emptied and restoration is skipped, leaving attacker-controlled .github/, .claude/, skills, and AGENTS.md in place.
I reproduced this locally: creating a branch named refs/remotes/origin/main and checking it out causes the current script to treat the attacker commit as the trusted base and skip restoration.
Fix: replace the rev-parse call with an exact-path lookup that can't fall back to other namespaces:
BASE_SHA=$(git -C "${WORKSPACE}" show-ref --verify --hash "refs/remotes/origin/${DEFAULT_BRANCH}" 2>/dev/null)I verified this rejects the attacker-branch scenario (exits non-zero, no match) while still resolving a genuine remote-tracking ref. Please add a regression test for this exact scenario (PR head named refs/remotes/origin/<default-branch>) alongside the existing table-driven cases in pr_test.go.
@copilot please address this.
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive bypass has unresolved checkout and restoration edge cases that require human review.
4 open findings
What changed in this PR
Adds a trusted-default-branch exception to agent configuration restoration, aiming to prevent stale activation snapshots from causing unintended PR reverts (#55318).
Changes:
- Preserve checkout configuration when HEAD exactly matches the local default-branch ref.
- Add real-Git regressions for advancing bases and restoration fallbacks.
- Document the exception while retaining
.mcp.jsoncleanup.
| File | Description |
|---|---|
pkg/workflow/pr_test.go |
Adds advancing-base and fallback regression coverage. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Explains newer-base configuration preservation. |
docs/src/content/docs/engines/copilot.md |
Documents the engine-restoration exception. |
actions/setup/sh/restore_base_github_folders.sh |
Adds the exact-default-branch-match bypass. |
🧠 Review effort: Max
| # with the old snapshot would turn concurrent base changes into agent-authored | ||
| # reverts. Only an exact commit match is sufficient; a PR head can also descend | ||
| # from the base. Missing refs (including checkout-disabled runs) retain restoration. | ||
| if [ -n "${DEFAULT_BRANCH:-}" ] && |
| # reverts. Only an exact commit match is sufficient; a PR head can also descend | ||
| # from the base. Missing refs (including checkout-disabled runs) retain restoration. | ||
| if [ -n "${DEFAULT_BRANCH:-}" ] && | ||
| HEAD_SHA=$(git -C "${WORKSPACE}" rev-parse --verify 'HEAD^{commit}' 2>/dev/null) && |
| if [ -n "${DEFAULT_BRANCH:-}" ] && | ||
| HEAD_SHA=$(git -C "${WORKSPACE}" rev-parse --verify 'HEAD^{commit}' 2>/dev/null) && | ||
| BASE_SHA=$(git -C "${WORKSPACE}" rev-parse --verify "refs/remotes/origin/${DEFAULT_BRANCH}^{commit}" 2>/dev/null) && | ||
| [ "${HEAD_SHA}" = "${BASE_SHA}" ]; then |
| BASE_SHA=$(git -C "${WORKSPACE}" rev-parse --verify "refs/remotes/origin/${DEFAULT_BRANCH}^{commit}" 2>/dev/null) && | ||
| [ "${HEAD_SHA}" = "${BASE_SHA}" ]; then | ||
| echo "Keeping agent config folders from the checked-out trusted base commit ${HEAD_SHA}" | ||
| FOLDERS=() |
|
@copilot address the following outstanding work in one pass:
|
Summary
Fixes #55318.
DEFAULT_BRANCHenvironment variable..mcp.jsonremoval remains unchanged.Validation
make buildpassed.shellcheck --severity=warning actions/setup/sh/restore_base_github_folders.shpassed.make agent-report-progresspassed. Its initial workflow-drift failure was eliminated by reusing the existing environment variable rather than changing generated YAML.make agent-report-progress-no-testpassed with modern Bash and an isolated TMPDIR: Go/custom lint, shell lint, schema freshness, and all 344 workflow locks in sync.make fmtran Go formatting successfully but hit a pre-existing JavaScript formatter failure (convertToJson is not definedintestdata/copilot_sdk_web_fetch_contract.json); unrelated formatting edits were removed.This fix does not touch the trusted PR bundle baseline changes in #68717 or validated transport-byte changes in #68732.