Skip to content

Commit

Permalink
refs: enforce name conventions for root refs
Browse files Browse the repository at this point in the history
We recently taught check_refname_format() to insist that any ref outside
of "refs/" match the is_root_ref_syntax() rules. This can reduce the
chance of accidentally reading or writing a non-ref file in ".git" when
the files backend is in use.

For case-sensitive filesystems, this should mostly work. We do not
generally create all-caps files under .git/ unless they are meant to be
root refs.

But for case-insensitive filesystems, it's less clear. Asking to write
to "CONFIG" would try to access the actual ".git/config" file (though
fortunately this does not work, as we refuse to write anything we can't
parse as a ref).

We already have functions that catalog the pattern of allowable names
(i.e., ending in "_HEAD" or one of a set of historical exceptions). So
even if we allowed creating such a ref, we'd skip over it while
iterating over the root refs.

Let's teach check_refname_format() to use those functions to further
restrict what it will allow at the root level. That should make things
safer and more consistent with ref iteration. Note that while
is_root_ref_syntax() covers the syntax for both regular root refs and
pseudo-refs (FETCH_HEAD and MERGE_HEAD), the is_root_ref() function does
not include pseudo-refs. So we have to check the two classes separately.

This patch doesn't touch refname_is_safe(), which generally tries to be
a bit more loose (e.g., to allow deletion of bogus names). I've left it
loose here, though arguably it would benefit from some of the same
protection (you wouldn't want to delete ".git/config" either, though
again, we'd refuse to delete something we can't parse).

[the mass test update is ugly; split it out?]
  • Loading branch information
peff committed Nov 27, 2024
1 parent 6477411 commit 5898975
Show file tree
Hide file tree
Showing 9 changed files with 93 additions and 82 deletions.
3 changes: 2 additions & 1 deletion refs.c
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ static int check_or_sanitize_refname(const char *refname, int flags,

parse_worktree_ref(refname, NULL, NULL, &bare_ref);
if (!starts_with(bare_ref, "refs/") &&
!is_root_ref_syntax(bare_ref))
!is_pseudo_ref(bare_ref) &&
!is_root_ref(bare_ref))
return -1;
}

Expand Down
90 changes: 45 additions & 45 deletions t/t1400-update-ref.sh
Original file line number Diff line number Diff line change
Expand Up @@ -492,58 +492,58 @@ test_expect_success 'git cat-file blob main@{2005-05-26 23:42}:F (expect OTHER)'
# Test adding and deleting pseudorefs

test_expect_success 'given old value for missing pseudoref, do not create' '
test_must_fail git update-ref PSEUDOREF $A $B 2>err &&
test_must_fail git rev-parse PSEUDOREF &&
test_must_fail git update-ref PSEUDO_HEAD $A $B 2>err &&
test_must_fail git rev-parse PSEUDO_HEAD &&
test_grep "unable to resolve reference" err
'

test_expect_success 'create pseudoref' '
git update-ref PSEUDOREF $A &&
test $A = $(git show-ref -s --verify PSEUDOREF)
git update-ref PSEUDO_HEAD $A &&
test $A = $(git show-ref -s --verify PSEUDO_HEAD)
'

test_expect_success 'overwrite pseudoref with no old value given' '
git update-ref PSEUDOREF $B &&
test $B = $(git show-ref -s --verify PSEUDOREF)
git update-ref PSEUDO_HEAD $B &&
test $B = $(git show-ref -s --verify PSEUDO_HEAD)
'

test_expect_success 'overwrite pseudoref with correct old value' '
git update-ref PSEUDOREF $C $B &&
test $C = $(git show-ref -s --verify PSEUDOREF)
git update-ref PSEUDO_HEAD $C $B &&
test $C = $(git show-ref -s --verify PSEUDO_HEAD)
'

test_expect_success 'do not overwrite pseudoref with wrong old value' '
test_must_fail git update-ref PSEUDOREF $D $E 2>err &&
test $C = $(git show-ref -s --verify PSEUDOREF) &&
test_must_fail git update-ref PSEUDO_HEAD $D $E 2>err &&
test $C = $(git show-ref -s --verify PSEUDO_HEAD) &&
test_grep "cannot lock ref.*expected" err
'

test_expect_success 'delete pseudoref' '
git update-ref -d PSEUDOREF &&
test_must_fail git show-ref -s --verify PSEUDOREF
git update-ref -d PSEUDO_HEAD &&
test_must_fail git show-ref -s --verify PSEUDO_HEAD
'

test_expect_success 'do not delete pseudoref with wrong old value' '
git update-ref PSEUDOREF $A &&
test_must_fail git update-ref -d PSEUDOREF $B 2>err &&
test $A = $(git show-ref -s --verify PSEUDOREF) &&
git update-ref PSEUDO_HEAD $A &&
test_must_fail git update-ref -d PSEUDO_HEAD $B 2>err &&
test $A = $(git show-ref -s --verify PSEUDO_HEAD) &&
test_grep "cannot lock ref.*expected" err
'

test_expect_success 'delete pseudoref with correct old value' '
git update-ref -d PSEUDOREF $A &&
test_must_fail git show-ref -s --verify PSEUDOREF
git update-ref -d PSEUDO_HEAD $A &&
test_must_fail git show-ref -s --verify PSEUDO_HEAD
'

test_expect_success 'create pseudoref with old OID zero' '
git update-ref PSEUDOREF $A $Z &&
test $A = $(git show-ref -s --verify PSEUDOREF)
git update-ref PSEUDO_HEAD $A $Z &&
test $A = $(git show-ref -s --verify PSEUDO_HEAD)
'

test_expect_success 'do not overwrite pseudoref with old OID zero' '
test_when_finished git update-ref -d PSEUDOREF &&
test_must_fail git update-ref PSEUDOREF $B $Z 2>err &&
test $A = $(git show-ref -s --verify PSEUDOREF) &&
test_when_finished git update-ref -d PSEUDO_HEAD &&
test_must_fail git update-ref PSEUDO_HEAD $B $Z 2>err &&
test $A = $(git show-ref -s --verify PSEUDO_HEAD) &&
test_grep "already exists" err
'

Expand Down Expand Up @@ -812,13 +812,13 @@ test_expect_success 'stdin delete ref fails with zero old value' '
'

test_expect_success 'stdin update symref works option no-deref' '
git symbolic-ref TESTSYMREF $b &&
git symbolic-ref TESTSYMREF_HEAD $b &&
cat >stdin <<-EOF &&
option no-deref
update TESTSYMREF $a $b
update TESTSYMREF_HEAD $a $b
EOF
git update-ref --stdin <stdin &&
git rev-parse TESTSYMREF >expect &&
git rev-parse TESTSYMREF_HEAD >expect &&
git rev-parse $a >actual &&
test_cmp expect actual &&
git rev-parse $m~1 >expect &&
Expand All @@ -827,27 +827,27 @@ test_expect_success 'stdin update symref works option no-deref' '
'

test_expect_success 'stdin delete symref works option no-deref' '
git symbolic-ref TESTSYMREF $b &&
git symbolic-ref TESTSYMREF_HEAD $b &&
cat >stdin <<-EOF &&
option no-deref
delete TESTSYMREF $b
delete TESTSYMREF_HEAD $b
EOF
git update-ref --stdin <stdin &&
test_must_fail git rev-parse --verify -q TESTSYMREF &&
test_must_fail git rev-parse --verify -q TESTSYMREF_HEAD &&
git rev-parse $m~1 >expect &&
git rev-parse $b >actual &&
test_cmp expect actual
'

test_expect_success 'stdin update symref works flag --no-deref' '
git symbolic-ref TESTSYMREFONE $b &&
git symbolic-ref TESTSYMREFTWO $b &&
git symbolic-ref TESTSYMREFONE_HEAD $b &&
git symbolic-ref TESTSYMREFTWO_HEAD $b &&
cat >stdin <<-EOF &&
update TESTSYMREFONE $a $b
update TESTSYMREFTWO $a $b
update TESTSYMREFONE_HEAD $a $b
update TESTSYMREFTWO_HEAD $a $b
EOF
git update-ref --no-deref --stdin <stdin &&
git rev-parse TESTSYMREFONE TESTSYMREFTWO >expect &&
git rev-parse TESTSYMREFONE_HEAD TESTSYMREFTWO_HEAD >expect &&
git rev-parse $a $a >actual &&
test_cmp expect actual &&
git rev-parse $m~1 >expect &&
Expand All @@ -856,15 +856,15 @@ test_expect_success 'stdin update symref works flag --no-deref' '
'

test_expect_success 'stdin delete symref works flag --no-deref' '
git symbolic-ref TESTSYMREFONE $b &&
git symbolic-ref TESTSYMREFTWO $b &&
git symbolic-ref TESTSYMREFONE_HEAD $b &&
git symbolic-ref TESTSYMREFTWO_HEAD $b &&
cat >stdin <<-EOF &&
delete TESTSYMREFONE $b
delete TESTSYMREFTWO $b
delete TESTSYMREFONE_HEAD $b
delete TESTSYMREFTWO_HEAD $b
EOF
git update-ref --no-deref --stdin <stdin &&
test_must_fail git rev-parse --verify -q TESTSYMREFONE &&
test_must_fail git rev-parse --verify -q TESTSYMREFTWO &&
test_must_fail git rev-parse --verify -q TESTSYMREFONE_HEAD &&
test_must_fail git rev-parse --verify -q TESTSYMREFTWO_HEAD &&
git rev-parse $m~1 >expect &&
git rev-parse $b >actual &&
test_cmp expect actual
Expand Down Expand Up @@ -1224,10 +1224,10 @@ test_expect_success 'stdin -z delete ref fails with zero old value' '
'

test_expect_success 'stdin -z update symref works option no-deref' '
git symbolic-ref TESTSYMREF $b &&
printf $F "option no-deref" "update TESTSYMREF" "$a" "$b" >stdin &&
git symbolic-ref TESTSYMREF_HEAD $b &&
printf $F "option no-deref" "update TESTSYMREF_HEAD" "$a" "$b" >stdin &&
git update-ref -z --stdin <stdin &&
git rev-parse TESTSYMREF >expect &&
git rev-parse TESTSYMREF_HEAD >expect &&
git rev-parse $a >actual &&
test_cmp expect actual &&
git rev-parse $m~1 >expect &&
Expand All @@ -1236,10 +1236,10 @@ test_expect_success 'stdin -z update symref works option no-deref' '
'

test_expect_success 'stdin -z delete symref works option no-deref' '
git symbolic-ref TESTSYMREF $b &&
printf $F "option no-deref" "delete TESTSYMREF" "$b" >stdin &&
git symbolic-ref TESTSYMREF_HEAD $b &&
printf $F "option no-deref" "delete TESTSYMREF_HEAD" "$b" >stdin &&
git update-ref -z --stdin <stdin &&
test_must_fail git rev-parse --verify -q TESTSYMREF &&
test_must_fail git rev-parse --verify -q TESTSYMREF_HEAD &&
git rev-parse $m~1 >expect &&
git rev-parse $b >actual &&
test_cmp expect actual
Expand Down
34 changes: 17 additions & 17 deletions t/t1401-symbolic-ref.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,24 +46,24 @@ test_expect_success 'HEAD cannot be removed' '
reset_to_sane

test_expect_success 'symbolic-ref can be deleted' '
git symbolic-ref NOTHEAD refs/heads/foo &&
git symbolic-ref -d NOTHEAD &&
git symbolic-ref NOT_HEAD refs/heads/foo &&
git symbolic-ref -d NOT_HEAD &&
git rev-parse refs/heads/foo &&
test_must_fail git symbolic-ref NOTHEAD
test_must_fail git symbolic-ref NOT_HEAD
'
reset_to_sane

test_expect_success 'symbolic-ref can delete dangling symref' '
git symbolic-ref NOTHEAD refs/heads/missing &&
git symbolic-ref -d NOTHEAD &&
git symbolic-ref NOT_HEAD refs/heads/missing &&
git symbolic-ref -d NOT_HEAD &&
test_must_fail git rev-parse refs/heads/missing &&
test_must_fail git symbolic-ref NOTHEAD
test_must_fail git symbolic-ref NOT_HEAD
'
reset_to_sane

test_expect_success 'symbolic-ref fails to delete missing FOO' '
echo "fatal: Cannot delete FOO, not a symbolic ref" >expect &&
test_must_fail git symbolic-ref -d FOO >actual 2>&1 &&
test_expect_success 'symbolic-ref fails to delete missing ref' '
echo "fatal: Cannot delete refs/heads/does-not-exist, not a symbolic ref" >expect &&
test_must_fail git symbolic-ref -d refs/heads/does-not-exist >actual 2>&1 &&
test_cmp expect actual
'
reset_to_sane
Expand Down Expand Up @@ -191,34 +191,34 @@ test_expect_success 'symbolic-ref pointing at another' '

test_expect_success 'symbolic-ref --short handles complex utf8 case' '
name="测试-加-增加-加-增加" &&
git symbolic-ref TEST_SYMREF "refs/heads/$name" &&
git symbolic-ref TEST_SYMREF_HEAD "refs/heads/$name" &&
# In the real world, we saw problems with this case only
# when the locale includes UTF-8. Set it here to try to make things as
# hard as possible for us to pass, but in practice we should do the
# right thing regardless (and of course some platforms may not even
# have this locale).
LC_ALL=en_US.UTF-8 git symbolic-ref --short TEST_SYMREF >actual &&
LC_ALL=en_US.UTF-8 git symbolic-ref --short TEST_SYMREF_HEAD >actual &&
echo "$name" >expect &&
test_cmp expect actual
'

test_expect_success 'symbolic-ref --short handles name with suffix' '
git symbolic-ref TEST_SYMREF "refs/remotes/origin/HEAD" &&
git symbolic-ref --short TEST_SYMREF >actual &&
git symbolic-ref TEST_SYMREF_HEAD "refs/remotes/origin/HEAD" &&
git symbolic-ref --short TEST_SYMREF_HEAD >actual &&
echo "origin" >expect &&
test_cmp expect actual
'

test_expect_success 'symbolic-ref --short handles almost-matching name' '
git symbolic-ref TEST_SYMREF "refs/headsXfoo" &&
git symbolic-ref --short TEST_SYMREF >actual &&
git symbolic-ref TEST_SYMREF_HEAD "refs/headsXfoo" &&
git symbolic-ref --short TEST_SYMREF_HEAD >actual &&
echo "headsXfoo" >expect &&
test_cmp expect actual
'

test_expect_success 'symbolic-ref --short handles name with percent' '
git symbolic-ref TEST_SYMREF "refs/heads/%foo" &&
git symbolic-ref --short TEST_SYMREF >actual &&
git symbolic-ref TEST_SYMREF_HEAD "refs/heads/%foo" &&
git symbolic-ref --short TEST_SYMREF_HEAD >actual &&
echo "%foo" >expect &&
test_cmp expect actual
'
Expand Down
16 changes: 8 additions & 8 deletions t/t1405-main-ref-store.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,21 @@ test_expect_success 'setup' '
test_commit one
'

test_expect_success 'create_symref(FOO, refs/heads/main)' '
$RUN create-symref FOO refs/heads/main nothing &&
test_expect_success 'create_symref(FOO_HEAD, refs/heads/main)' '
$RUN create-symref FOO_HEAD refs/heads/main nothing &&
echo refs/heads/main >expected &&
git symbolic-ref FOO >actual &&
git symbolic-ref FOO_HEAD >actual &&
test_cmp expected actual
'

test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
test_expect_success 'delete_refs(FOO_HEAD, refs/tags/new-tag)' '
git tag -a -m new-tag new-tag HEAD &&
git rev-parse FOO -- &&
git rev-parse FOO_HEAD -- &&
git rev-parse refs/tags/new-tag -- &&
m=$(git rev-parse main) &&
$RUN delete-refs REF_NO_DEREF nothing FOO refs/tags/new-tag &&
test_must_fail git rev-parse --symbolic-full-name FOO &&
test_must_fail git rev-parse FOO -- &&
$RUN delete-refs REF_NO_DEREF nothing FOO_HEAD refs/tags/new-tag &&
test_must_fail git rev-parse --symbolic-full-name FOO_HEAD &&
test_must_fail git rev-parse FOO_HEAD -- &&
test_must_fail git rev-parse refs/tags/new-tag --
'

Expand Down
10 changes: 5 additions & 5 deletions t/t1407-worktree-ref-store.sh
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' '
test_cmp expected actual
'

test_expect_success 'create_symref(FOO, refs/heads/main)' '
$RWT create-symref FOO refs/heads/main nothing &&
test_expect_success 'create_symref(FOO_HEAD, refs/heads/main)' '
$RWT create-symref FOO_HEAD refs/heads/main nothing &&
echo refs/heads/main >expected &&
git -C wt symbolic-ref FOO >actual &&
git -C wt symbolic-ref FOO_HEAD >actual &&
test_cmp expected actual &&
$RMAIN create-symref FOO refs/heads/wt-main nothing &&
$RMAIN create-symref FOO_HEAD refs/heads/wt-main nothing &&
echo refs/heads/wt-main >expected &&
git symbolic-ref FOO >actual &&
git symbolic-ref FOO_HEAD >actual &&
test_cmp expected actual
'

Expand Down
10 changes: 10 additions & 0 deletions t/t1430-bad-ref-name.sh
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,11 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '
test_grep "refusing to update ref with bad name" err
'

test_expect_success 'update-ref enforces root ref naming convention' '
test_must_fail git update-ref FOO_BAR HEAD 2>err &&
test_grep "refusing to update ref with bad name" err
'

test_expect_success 'rev-parse refuses non-pseudoref outside of refs/' '
git rev-parse HEAD >.git/bad &&
test_must_fail git rev-parse --verify bad
Expand All @@ -410,4 +415,9 @@ test_expect_success 'rev-parse recognizes non-pseudoref via worktree' '
test_must_fail git rev-parse --verify main-worktree/bad
'

test_expect_success 'rev-parse enforces root ref naming convention' '
git rev-parse HEAD >.git/BAD_NAME &&
test_must_fail git rev-parse --verify BAD_NAME
'

test_done
4 changes: 2 additions & 2 deletions t/t4013-diff-various.sh
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ test_expect_success setup '
# pseudo-ref to avoid impacting tests with --all.
commit=$(echo reverse |
git commit-tree -p master^2 -p master^1 master^{tree}) &&
git update-ref REVERSE $commit &&
git update-ref REVERSE_HEAD $commit &&
git config diff.renames false &&
Expand Down Expand Up @@ -307,7 +307,7 @@ diff-tree --cc --stat --summary master
diff-tree -c --stat --summary side
diff-tree --cc --stat --summary side
diff-tree --cc --shortstat master
diff-tree --cc --summary REVERSE
diff-tree --cc --summary REVERSE_HEAD
# improved by Timo's patch
diff-tree --cc --patch-with-stat master
# improved by Timo's patch
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
$ git diff-tree --cc --summary REVERSE
$ git diff-tree --cc --summary REVERSE_HEAD
2562325a7ee916efb2481da93073b82cec801cbc
create mode 100644 file1
delete mode 100644 file2
Expand Down
6 changes: 3 additions & 3 deletions t/t9300-fast-import.sh
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ test_expect_success 'B: fail on invalid blob sha1' '

test_expect_success 'B: accept branch name "TEMP_TAG"' '
cat >input <<-INPUT_END &&
commit TEMP_TAG
commit refs/tags/TEMP_TAG
committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
data <<COMMIT
tag base
Expand All @@ -389,9 +389,9 @@ test_expect_success 'B: accept branch name "TEMP_TAG"' '
INPUT_END
test_when_finished "rm -f .git/TEMP_TAG && git gc --prune=now" &&
test_when_finished "git tag -d TEMP_TAG && git gc --prune=now" &&
git fast-import <input &&
test $(test-tool ref-store main resolve-ref TEMP_TAG 0 | cut -f1 -d " " ) != "$ZERO_OID" &&
test $(test-tool ref-store main resolve-ref refs/tags/TEMP_TAG 0 | cut -f1 -d " " ) != "$ZERO_OID" &&
test $(git rev-parse main) = $(git rev-parse TEMP_TAG^)
'

Expand Down

0 comments on commit 5898975

Please sign in to comment.