fix(git): reject tabs in ref names, git refuses them too - #1053
Conversation
There was a problem hiding this comment.
Change is correct. The carve-out removal is a strict narrowing over what git itself accepts, and all four call sites (create_tag, create_or_move_tag, remote name and branch name in push.rs) are real refnames, so nothing legitimate loses. The old condition already contained ch.is_control(), so collapsing to it is behaviour-preserving apart from the tab.
One nit inline on the new test's name.
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
004434a to
298051e
Compare
|
Renamed in 298051e to Verified the premise rather than taking it on trust, and it holds. git accepts a C1 byte in a refname and rejects an ASCII one: So the validator is deliberately stricter than git across the C1 range, which the existing doc comment already allows for ("not the full |
298051e to
a29844a
Compare
Closes #798.
Two parts of that issue's analysis are wrong, and the fix here is smaller than what it proposed. Checked both before writing any code.
parse_ls_remote_tagsis not broken.split_once('\t')keeps everything after the first tab, so a hypotheticalsha<TAB>refs/tags/foo<TAB>barline yieldsrefname = "refs/tags/foo\tbar"andstrip_prefix("refs/tags/")then gives"foo\tbar", which is correct. No parser change is needed.The scenario is unreachable anyway, because git rejects tabs in ref names. A tab is an ASCII control character and
check-ref-formatrefuses it:So the real defect is only in the validator.
ensure_safe_refname_fragmentcarved tabs out of its control-character check on the premise that "tabs are technically allowed in refs", which is not true. The effect was that a tab in atagTemplategot past our own check and failed later as an opaquefatal: '...' is not a valid tag namefrom a git subprocess, instead of the validator's actionable message.Removing the carve-out collapses the whole condition to
ch.is_control(), since\0,\n,\rand\x7fare all in Unicode category Cc and were already listed individually.Verification
rejects_tabandevery_rejected_control_char_is_one_git_also_refusesboth fail against the previous condition:The second test is the one that matters: for each character the validator rejects it shells out to
git check-ref-formatand asserts git refuses it too, so the validator can never drift into rejecting a name git would have accepted.\0is excluded from that loop because an interior NUL cannot be passed throughargvat all;rejects_null_bytestill covers it on our side.Second layer of a stack of four. Based on #1052, and #1054 sits on top.