Skip to content

Commit bf02a40

Browse files
Phlogistiqueclaudegithub-actions
authored
Trust only our own comments for the state marker (#51)
`read_state_marker` accepted a marker from *any* comment, so on a public repo anyone able to comment could plant one; if its `base=` matched, the resume would merge an attacker-chosen commit (fork-pushed objects are reachable by hash in the repo network) into the branch and push it with the action's token. Benign variant: a quote-reply of an old conflict comment resurrects a stale marker, since HTML comments survive quoting and the newest marker wins. Fix: filter comments to `viewerDidAuthor` — those posted with the same token the action runs under — which needs no configured identity. The resume test's gh mock rejects comment queries without that filter. Caveat: if the repo switches tokens (e.g. `GITHUB_TOKEN` → App) while a PR sits in conflict, the old marker is no longer "ours" and the resume takes the safe abandon path. Also rejects markers with missing fields instead of passing empty values to git (a marker missing `squash=` used to crash on `update-ref` and strand the PR under the label); new scenario E covers it. Stacked on #50 (same function). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JHvKryT4QUpHYdNq9YEQxX --- _Generated by [Claude Code](https://claude.ai/code/session_01JHvKryT4QUpHYdNq9YEQxX)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: github-actions <github-actions@github.com>
1 parent 9c0543e commit bf02a40

2 files changed

Lines changed: 28 additions & 2 deletions

File tree

tests/test_conflict_resolution_resume.sh

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,11 @@ echo "gh $*" >> "$CALLS"
3333
if [[ "$1 $2" == "pr view" ]]; then
3434
case "$*" in
3535
*--json\ labels*) printf '%s\n' "${MOCK_LABELS:-}";;
36-
*--json\ comments*) cat "${MOCK_COMMENTS_FILE:-/dev/null}";;
36+
*--json\ comments*)
37+
# The comments file stands for our own comments only, so the query
38+
# must restrict itself to those.
39+
[[ "$*" == *viewerDidAuthor* ]] || { echo "comments query must filter by viewerDidAuthor" >&2; exit 1; }
40+
cat "${MOCK_COMMENTS_FILE:-/dev/null}";;
3741
*) echo "unhandled pr view: $*" >&2; exit 1;;
3842
esac
3943
elif [[ "$1 $2" == "pr comment" ]]; then
@@ -167,5 +171,21 @@ grep -q -- "--base" "$CALLS" && fail "D: base must NOT be edited"
167171
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "D: child was pushed"
168172
ok "D: missing target detected, no branch mutation, label removed"
169173

174+
# ---------------------------------------------------------------------------
175+
echo "### Scenario E: malformed state marker -> internal error, die without touching the PR"
176+
setup_repo
177+
MOCK_LABELS="autorestack-needs-conflict-resolution"
178+
PR_BASE="parent"
179+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
180+
{ echo "### conflict"; echo; echo '<!-- autorestack-state: base=parent target=main -->'; } > "$MOCK_COMMENTS_FILE"
181+
run_resume
182+
183+
grep -q "EXIT=1" "$WORK/out.log" || fail "E: run must die on a malformed marker"
184+
grep -q "remove-label" "$CALLS" && fail "E: label must stay on"
185+
grep -q "gh pr comment" "$CALLS" && fail "E: no PR comment for an internal error"
186+
grep -q -- "--base" "$CALLS" && fail "E: base must NOT be edited"
187+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "E: child was pushed"
188+
ok "E: malformed marker dies, PR untouched, label kept"
189+
170190
echo
171191
echo "All conflict-resume tests passed 🎉 ($PASS scenarios)"

update-pr-stack.sh

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ format_state_marker() {
4444
read_state_marker() {
4545
local PR_NUMBER="$1"
4646
local BODIES
47-
if ! BODIES=$(gh pr view "$PR_NUMBER" --json comments --jq '.comments[].body'); then
47+
if ! BODIES=$(gh pr view "$PR_NUMBER" --json comments \
48+
--jq '.comments[] | select(.viewerDidAuthor) | .body'); then
4849
echo "Error: could not read comments of PR #$PR_NUMBER" >&2
4950
exit 1
5051
fi
@@ -307,6 +308,11 @@ continue_after_resolution() {
307308
read -r OLD_BASE NEW_TARGET SQUASH_HASH < <(parse_state_marker "$MARKER")
308309
echo "Recorded state: base=$OLD_BASE target=$NEW_TARGET squash=$SQUASH_HASH"
309310

311+
if [[ -z "$OLD_BASE" || -z "$NEW_TARGET" || -z "$SQUASH_HASH" ]]; then
312+
echo "Error: malformed state marker on $PR_BRANCH: $MARKER" >&2
313+
exit 1
314+
fi
315+
310316
# The PR was left based on the merged parent branch. If the payload shows a
311317
# different base, a human retargeted the PR; the recorded target is stale,
312318
# so step back before any mutation.

0 commit comments

Comments
 (0)