[PATCH 7/7] fetch: do not consider peeled tags as advertised tips

From: Jeff King <peff@peff.net>
Date: Apr 13, 2019, 05:57 UTC
Message-ID: <20190413055736.GG19495@sigill.intra.peff.net>
In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net>

Our filter_refs() function accidentally considers the target of a peeled tag to be advertised by the server, even though upload-pack on the server side does not consider it so. This can result in the client making a bogus fetch to the server, which will end with the server complaining "not our ref". Whereas the correct behavior is for the client to notice that the server will not allow the request and error out immediately.

So as bugs go, this is not very serious (the outcome is the same either way -- the fetch fails). But it's worth making the logic here correct and consistent with other related cases (e.g., fetching an oid that the server did not mention at all).

The crux of the issue comes from fdb69d33c4 (fetch-pack: always allow fetching of literal SHA1s, 2017-05-15). After that, the strategy of filter_refs() is basically:

  - for each advertised ref, try to match it with a "sought" ref
    provided by the user. Skip any malformed refs (which includes
    peeled values like "refs/tags/foo^{}"), and place any unmatched
    items onto the unmatched list.

  - if there are unmatched sought refs, then put all of the advertised
    tips into an oidset, including the unmatched ones.

  - for each sought ref, see if it's in the oidset, in which case it's
    legal for us to ask the server for it

The problem is in the second step. Our list of unmatched refs includes the peeled refs, even though upload-pack does not allow them to be directly fetched. So the simplest fix would be to exclude them during that step.

However, we can observe that the unmatched list isn't used for anything else, and is freed at the end. We can just free those malformed refs immediately. That saves us having to check each ref a second time to see if it's malformed.

Note that this code only kicks in when "strict" is in effect. I.e., if we are using the v0 protocol and uploadpack.allowReachableSHA1InWant is not in effect. With v2, all oids are allowed, and we do not bother creating or consulting the oidset at all. To future-proof our test against the upcoming GIT_TEST_PROTOCOL_VERSION flag, we'll manually mark it as a v0-only test.

Signed-off-by: Jeff King <peff@peff.net>
---
We actually discussed this a while ago in:

  https://public-inbox.org/git/20180611044710.GB31642@sigill.intra.peff.net/

and the surrounding thread. I'm not convinced that upload-pack shouldn't be considering the peeled values as ref tips, but given that we have never done so, it's probably not worth making that change now. Clients couldn't rely on it without a capability marker, and all of this goes away in the v2 protocol anyway.

 fetch-pack.c          | 11 ++++++++---
 t/t5516-fetch-push.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index a181d3401d..bb8eac8126 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -573,9 +573,14 @@ static void filter_refs(struct fetch_pack_args *args,
 		next = ref->next;
 
 		if (starts_with(ref->name, "refs/") &&
-		    check_refname_format(ref->name, 0))
-			; /* trash */
-		else {
+		    check_refname_format(ref->name, 0)) {
+			/*
+			 * trash or a peeled value; do not even add it to
+			 * unmatched list
+			 */
+			free_one_ref(ref);
+			continue;
+		} else {
 			while (i < nr_sought) {
 				int cmp = strcmp(ref->name, sought[i]->name);
 				if (cmp < 0)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 98ef71b48c..4f065212b8 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1273,6 +1273,17 @@ test_expect_success 'fetch follows tags by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'peeled advertisements are not considered ref tips' '
+	mk_empty testrepo &&
+	git -C testrepo commit --allow-empty -m one &&
+	git -C testrepo commit --allow-empty -m two &&
+	git -C testrepo tag -m foo mytag HEAD^ &&
+	oid=$(git -C testrepo rev-parse mytag^{commit}) &&
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git fetch testrepo $oid 2>err &&
+	test_i18ngrep "Server does not allow request for unadvertised object" err
+'
+
 test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&

-- 
2.21.0.931.gd0bc72a411