refs: check refnames as fully qualified when resolving

Most code paths for resolving refs end up in refs_resolve_ref_unsafe(),
which checks the names using check_refname_format(). The names we have
at this stage are always full refnames, but we pass the ALLOW_ONELEVEL
flag so that the function allows root refs like MERGE_HEAD. We should
instead pass the FULLY_QUALIFIED flag, which lets check_refname_format()
do some extra syntactic checks on those ref names.

With this patch we'll refuse to read anything outside of refs/ that does
not match the usual root ref syntax (all caps plus underscore). This
should not be a loss of functionality (since such refs cannot be written
as of the previous commit), but may protect us from mischief. For
example, you can ask for silly things that look vaguely like object ids,
like "rr-cache/<sha1>/postimage", "info/refs", or
"objects/info/commit-graphs/commit-graph-chain". It's doubtful you can
really do anything _too_ terrible there, but it seems like peeking at
random files in .git in response to possibly untrusted input is
something we should avoid.

Our tests are a bit tricky. We'll try to read a ref with an invalid name
and make sure it fails. Ideally we could realize that the failure is
caused by the invalid name, and not that the ref doesn't exist. But the
"bad name" error is not passed up the stack from the ref code, so the
two look the same. The best we can do is create the ref and confirm that
we don't accidentally read it. But there's a problem: we don't allow
writing refs with invalid names either! For the files backend, we can
hack around this by munging the filesystem manually. That means with the
files backend, the new test fails without this patch and succeeds with
it. But for other backends like reftable, the test would have passed
anyway (because the invalid ref does not exist).

This is probably OK for now. At least one backend is performing the full
test, and other backends won't erroneously fail. In the long run it
might be nice to surface the error via stderr, but that can come later.

The second test here, for "main-worktree/bad", actually succeeds even
without this patch. The worktree-ref code enforces the root-ref syntax
itself via is_current_worktree_ref(), so the extra checks for this in
check_refname_format() are redundant (though the parsing there is still
necessary so we know _not_ to reject "main-worktree/HEAD"). But either
way it's good to have a test which makes sure this remains the case.

Signed-off-by: Jeff King <peff@peff.net>

Author: peff

refs.c

@@ -2058,7 +2058,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 
 	*flags = 0;
 
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+	if (check_refname_format(refname, REFNAME_FULLY_QUALIFIED)) {
 		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 		    !refname_is_safe(refname))
 			return NULL;
@@ -2117,7 +2117,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 			oidclr(oid, refs->repo->hash_algo);
 			return refname;
 		}
-		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (check_refname_format(refname, REFNAME_FULLY_QUALIFIED)) {
 			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 			    !refname_is_safe(refname))
 				return NULL;

t/t1430-bad-ref-name.sh

@@ -399,4 +399,14 @@ test_expect_success 'update-ref refuses non-underscore punctuation outside of re
 	test_grep "refusing to update ref with bad name" err
 '
 
+test_expect_success 'rev-parse refuses non-root-ref outside of refs/' '
+	git rev-parse HEAD >.git/bad &&
+	test_must_fail git rev-parse --verify bad
+'
+
+test_expect_success 'rev-parse recognizes non-root-ref via worktree' '
+	git rev-parse HEAD >.git/bad &&
+	test_must_fail git rev-parse --verify main-worktree/bad
+'
+
 test_done