Re: "git symbolic-ref" doesn't do a very good job

From

Jeff King <peff@peff.net>

Date

Aug 1, 2022, 18:15 UTC

Message-ID

<YugYNzQYWqDCmOqN@coredump.intra.peff.net>

In-Reply-To

<YugQqp4oN26OFOpt@coredump.intra.peff.net>

On Mon, Aug 01, 2022 at 01:43:06PM -0400, Jeff King wrote:

> I'd be in favor of (2), which is really just catching syntactically
> invalid crap, and shouldn't break anyone. Technically it's possible
> somebody could be using a symref pointing at arbitrary data for
> who-knows-what reason, and extracting it with "symbolic-ref", but that
> is getting beyond far-fetched, I think.

-- >8 --
From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH] symbolic-ref: refuse to set syntactically invalid target

You can feed absolute garbage to symbolic-ref as a target like:

  git symbolic-ref HEAD refs/heads/foo..bar

A few notes:

  - We use REFNAME_ALLOW_ONELEVEL here, which lets:

     git update-ref refs/heads/foo FETCH_HEAD

    continue to work. It's unclear whether anybody wants to do something
    so odd, but it does work now, so this is erring on the conservative
    side. There's a test to make sure we didn't accidentally break this,
    but don't take that test as an endorsement that it's a good idea, or
    something we might not change in the future.

  - The test in t4202-log.sh checks how we handle such an invalid ref on
    the reading side, so it has to be updated to touch the HEAD file
    directly.

  - We need to keep our HEAD-specific check for "does it start with
    refs/". The ALLOW_ONELEVEL flag means we won't be enforcing that for
    other refs, but HEAD is special here because of the checks in
    validate_headref().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/symbolic-ref.c  |  2 ++
 t/t1401-symbolic-ref.sh | 10 ++++++++++
 t/t4202-log.sh          |  4 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index e547a08d6c..1b0f10225f 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		if (!strcmp(argv[0], "HEAD") &&
 		    !starts_with(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");
+		if (check_refname_format(argv[1], REFNAME_ALLOW_ONELEVEL) < 0)
+			die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]);
 		ret = !!create_symref(argv[0], argv[1], msg);
 		break;
 	default:
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 9fb0b90f25..0c204089b8 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -165,4 +165,14 @@ test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '
+	test_must_fail git symbolic-ref refs/heads/invalid foo..bar
+'
+
+test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '
+	git symbolic-ref refs/heads/top-level FETCH_HEAD &&
+	git update-ref FETCH_HEAD HEAD &&
+	test_cmp_rev top-level HEAD
+'
+
 test_done
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 6e66352558..f0aaa1fa02 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -2112,9 +2112,9 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
 	test_i18ngrep broken stderr
 '
 
-test_expect_success 'log diagnoses bogus HEAD symref' '
+test_expect_success REFFILES 'log diagnoses bogus HEAD symref' '
 	git init empty &&
-	git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&
+	echo "ref: refs/heads/invalid.lock" > empty/.git/HEAD &&
 	test_must_fail git -C empty log 2>stderr &&
 	test_i18ngrep broken stderr &&
 	test_must_fail git -C empty log --default totally-bogus 2>stderr &&

-- 
2.37.1.804.g1775fa20e0