Junio C Hamano [off-list ref] writes:
From: Michael Schubert <redacted>
Date: Sun, 17 Jun 2012 22:26:37 +0200
Subject: [PATCH] symbolic-ref: check format of given reference
Currently, it's possible to update HEAD with a nonsense reference since
no strict validation is performed. Example:
$ git symbolic-ref HEAD 'refs/heads/master
>
>
> '
It would be nice to add a new test or two to t1401. 1401.3 was
already trying to catch a malformed reference with this test:
test_must_fail git symbolic-ref HEAD foo
and it did trigger thanks to the prefixcmp(argv[1], "refs/") test we
already have. Probably something like
git symbolic-ref HEAD "refs/heads/.foo"
git symbolic-ref HEAD "refs/heads/-foo"
would be a good start.
To make the latter _correctly_ work requires a bit of work, though.
We should make sure all the check_refname_format() callers pass the
full path to a ref, get rid of ALLOW_ONELEVEL, and redo commits like
6348624 (disallow branch names that start with a hyphen, 2010-09-14)
and 4f0accd (tag: disallow '-' as tag name, 2011-05-10).
For that matter, shouldn't symbolic-ref be forbidden to point
outside refs/heads/, not just restricted in refs/ like the current
code does?
On Mon, Jun 18, 2012 at 10:10:14AM -0700, Junio C Hamano wrote:
For that matter, shouldn't symbolic-ref be forbidden to point
outside refs/heads/, not just restricted in refs/ like the current
code does?
We tried that already but reverted it due to topgit. See:
commit e9cc02f0e41fd5d2f51e3c3f2b4f8cfa9e434432
Author: Jeff King [off-list ref]
Date: Fri Feb 13 13:26:09 2009 -0500
symbolic-ref: allow refs/<whatever> in HEAD
Commit afe5d3d5 introduced a safety valve to symbolic-ref to
disallow installing an invalid HEAD. It was accompanied by
b229d18a, which changed validate_headref to require that
HEAD contain a pointer to refs/heads/ instead of just refs/.
Therefore, the safety valve also checked for refs/heads/.
As it turns out, topgit is using refs/top-bases/ in HEAD,
leading us to re-loosen (at least temporarily) the
validate_headref check made in b229d18a. This patch does the
corresponding loosening for the symbolic-ref safety valve,
so that the two are in agreement once more.
-Peff
On Tue, Jun 19, 2012 at 10:47:12AM -0400, Jeff King wrote:
On Mon, Jun 18, 2012 at 10:10:14AM -0700, Junio C Hamano wrote:
quoted
For that matter, shouldn't symbolic-ref be forbidden to point
outside refs/heads/, not just restricted in refs/ like the current
code does?
We tried that already but reverted it due to topgit. See:
commit e9cc02f0e41fd5d2f51e3c3f2b4f8cfa9e434432
Author: Jeff King [off-list ref]
Date: Fri Feb 13 13:26:09 2009 -0500
symbolic-ref: allow refs/<whatever> in HEAD
Commit afe5d3d5 introduced a safety valve to symbolic-ref to
disallow installing an invalid HEAD. It was accompanied by
b229d18a, which changed validate_headref to require that
HEAD contain a pointer to refs/heads/ instead of just refs/.
Therefore, the safety valve also checked for refs/heads/.
As it turns out, topgit is using refs/top-bases/ in HEAD,
leading us to re-loosen (at least temporarily) the
validate_headref check made in b229d18a. This patch does the
corresponding loosening for the symbolic-ref safety valve,
so that the two are in agreement once more.
The "at least temporarily" in that commit message merited a little
investigation. There was some discussion of changing topgit to record
its information using a different scheme, but there was no clear
outcome:
http://thread.gmane.org/gmane.comp.version-control.git/109581
So if somebody wanted to re-tighten this check, they would want
to at least check topgit's current behavior, and see which versions of
it we would be breaking. I tend to think it is not worth the effort.
-Peff