Repository navigation
Allow semicolons in branch names - #1789
rolandsusans wants to merge 2 commits into
Conversation
Semicolons are valid per git-check-ref-format but rejected by the branch name whitelist, so a PR whose head ref contains one cannot be reviewed at all. setupBranch validates both the head and base ref in tag mode before the prompt is built, so this fails a review-only run that never creates a branch. Follows the same reasoning as the existing entries: the characters carry no injection risk because all git calls use execFileSync rather than shell interpolation.
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
validateBranchName() now accepts ;, but the branch is also interpolated unquoted into Bash(${GIT_PUSH_WRAPPER} origin ${branchName}) in create-prompt/index.ts. A branch such as foo;/usr/bin/env is valid under this validator and turns that push instruction into a second shell command, so the execFileSync safety argument does not cover the Bash path. Please quote/escape the branch there (or keep ; rejected) and add a prompt-generation regression test.
Raised in review, and the concern is correct. The execFileSync argument covers the git operations but not the prompt path: the branch is interpolated into a suggested `Bash(<wrapper> origin <branch>)`, the live grant in modes/tag/index.ts is `Bash(<wrapper>:*)` which prefix-matches, so anything appended to that command is permitted and the Bash tool runs it through a shell. `validateBranchName` was what stood between a `;` in a branch name and a second command. Single-quoting closes it at the interpolation site. A quote character cannot itself appear in a valid branch name — it is outside the whitelist and the separate metacharacter guard — so there is nothing to escape past. Regression test asserts the quoted form is emitted and the bare form is not.
|
Thanks for the look. I went digging, and it turns out neither of us had the permission model right. Claude Code doesn't treat I pushed the quoting fix anyway ( One thing that might be useful either way: If you'd rather keep |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed d85e9a1. The accepted semicolon is now single-quoted in the generated push instruction, and the validator still excludes single quotes, so the accepted branch-name surface cannot break out of that quoting. The prompt regression covers the semicolon case directly, alongside the validator coverage. My earlier concern is resolved; no blocker from me.
Problem
validateBranchNamerejects semicolons, but they are valid pergit-check-ref-format:The consequence is larger than a naming inconvenience, because
setupBranchvalidates the PR's own head and base refs, not just a branch the action would create:prepareTagModecallssetupBranchunconditionally, before the prompt is built and before the model starts. So a repository whose tickets produce branch names likeFIDA-3971;-adjust-mappingcannot run the action at all on those PRs — including a pure review-only run that never creates a branch, never commits, and never pushes. It fails in the prepare step with:There is no input that skips it, and the head ref belongs to the repository rather than to the action.
We hit this on 16 runs in a month in one repo, whose Jira integration puts the ticket id and title into the branch name with a separator the tracker chooses.
Change
Adds
;to the whitelist, following the same reasoning already recorded for#,+,,,@and():That reasoning turned out to be incomplete, and review caught it. It covers the git operations but not the prompt, where the branch is interpolated into a suggested
Bash(<wrapper> origin <branch>)and the live grant prefix-matches — so a semicolon there would have become a second command. The second commit single-quotes it at that site; see the discussion below.The error message gains
semicolons (;)alongside the other named characters, and a comment records the reasoning in the same style as its neighbours.Verification
Two cases added to
test/validate-branch-name.test.ts, matching the shape of the existing parentheses test, plus a prompt-generation regression test for the quoting.Checked that the change does not weaken anything else: the separate metacharacter guard still rejects
~ ^ : ? * [ ] \, spaces and control characters, and the follow-on guards still reject a leading dash, a leading or trailing period, a trailing slash,//,..,.lock,@{, and a bare@. A name likesemi;colon~tildeis still rejected, on the tilde.I was not able to run
bun testlocally (no Bun on this machine), so CI is the first real run of the new cases.