picatz / picatz/flowstate

tools/hooks/mergeguard: two merge shapes reach the tool unblocked — `merge` before `gh`, and `gh api` wrapped in `eval`

Open
#1,973 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

mergeUsesShellExpansion's text heuristics have two order- and vocabulary-dependent gaps that let a merge-capable command through. Both predate #1972's false-positive fix and neither is caused or widened by it; they were found while establishing that fix's baseline.

The gaps

1. The word merge before the gh token. ghExpansionMergeText is (?s)\bgh\b.*\bmerge\b, which requires gh to appear first. A command that assigns the word earlier is not matched:

X=merge; gh api -X PUT repos/o/r/pulls/1/$X

ghDynamicPRMergeText and dynamicGHExecutable do not cover it either: both want pr-ish or p… m… shapes that gh api -X PUT does not have. Verified allowed on 612983c.

2. gh api inside eval or bash -c. nestedEvaluation gates on ghMergeText, which is (?s)\bgh\b.*\bpr\b.*\bmerge\b — it requires the literal word pr. gh api never carries it, so the nested-evaluation path never fires for the REST spelling of a merge:

eval 'gh api -X PUT repos/o/r/pulls/1/merge'
bash -c 'gh api -X PUT repos/o/r/pulls/1/merge'

Both verified allowed on 612983c. The quoting also collapses the inner command into a single word, so the tokenizer sees no gh invocation at all.

The common cause is that every predicate here was written against gh pr merge, the CLI spelling. gh api -X PUT .../merge is the same operation over REST and satisfies none of the pr-shaped patterns.

Evidence

Computed by evaluating the checked-in predicates directly, and confirmed against the built hook:

command expMrg dynPR dynGH nested result on 612983c
X=merge; gh api -X PUT .../$X false false false false ALLOWED
eval 'gh api -X PUT .../merge' true false false false ALLOWED
bash -c 'gh api -X PUT .../merge' true false false false ALLOWED
gh api -X PUT .../merge --jq .$f true false false false denied
gh pr merge 1 -R o/r --auto $X true true false false denied

The third column pair is what shows the cause: the two allowed rows fail for different reasons — the first because of ordering, the next two because nested needs pr.

Why this is bounded, and still worth closing

This hook is one control among several, and it is not the merge gate. A merge still needs shipcheck to pass on an unchanged head with an exact-head independent review, and the pinned-head precondition (--match-head-commit, or expectedHeadSha through the GitHub tool) is enforced separately by mergeHeadPinned. Neither gap grants an approval or hides a review. What they do is let a merge-capable command run without the hook's last-local-action check, which is precisely the control .agents/ship.md describes as "independent of the model's summary".

Acceptance

Close both without reintroducing #1972's false positive:

  1. Make the merge-text predicates order-independent, or match on the merge capability rather than on word order: a gh api invocation carrying a method-changing flag (-X, --method, or a body flag) and a URL that ends in /merge is a merge regardless of where the word sits. provableGHAPIRead in main.go already classifies the read side of exactly this and can be inverted for the write side.
  2. Drop the pr requirement from the nested-evaluation gate, or tokenize one level into eval and bash -c strings so the inner command is analyzed as a command rather than as an opaque word.
  3. Keep #1972's regression test passing, and flip the three cases currently pinned as known gaps in TestReadOnlyGHAPIIsNotAMerge into the denied set as each is closed — they are pinned there with a pointer to this issue so the expectation has an obvious home.

A regression test per shape, in both directions, and no change to the existing cases in TestMergeShellExpansionIsRejected.

Correction to #1972

#1972's "Why the obvious narrowing is wrong" section argued against segment-scoping ghExpansionMergeText on the grounds that X=merge; gh api -X PUT repos/o/r/pulls/1/$X would stop matching. That reasoning was wrong: as gap 1 above shows, the command does not match today either. The conclusion still held for a different reason — the exemption that shipped is about provable read capability rather than about segment distance — but the stated justification was not accurate, and #1972 has been corrected.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in main.go with the merge-text predicates, nestedEvaluation, and provableGHAPIRead. Run TestReadOnlyGHAPIIsNotAMerge and TestMergeShellExpansionIsRejected, then add regression coverage for both command shapes in both directions. Done means the three known gaps are denied without changing the existing regression cases or reintroducing #1972's false positive.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, shell
Domain
security, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.