threads / patch / 30827

patchsymbolic-ref: check format of given reference

Subject: [PATCH] symbolic-ref: check format of given reference

## tl;dr

8 messages between Jun 17, 2012 and Jun 19, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Michael Schubert· Jun 17, 2012, 20:26 UTC · lore

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
    >
    >
    > '
Fix this by checking the given reference with check_refname_format().
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
This was discussed earlier this year:
http://thread.gmane.org/gmane.comp.version-control.git/189715

What about pointing at non-existing references? Should this still be allowed?

Additionally, I had to reindent two lines to make git-am happy (indent with spaces).

 builtin/symbolic-ref.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to builtin/symbolic-ref.c +5 −3
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 801d62e..22362e0 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -43,16 +43,18 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options,
-			     git_symbolic_ref_usage, 0);
-	if (msg &&!*msg)
+				git_symbolic_ref_usage, 0);
+	if (msg && !*msg)
 		die("Refusing to perform update with empty message");
 	switch (argc) {
 	case 1:
 		check_symref(argv[0], quiet);
 		break;
 	case 2:
+		if (check_refname_format(argv[1], 0))
+			die("No valid reference format: '%s'", argv[1]);
 		if (!strcmp(argv[0], "HEAD") &&
-		    prefixcmp(argv[1], "refs/"))
+			prefixcmp(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");
 		create_symref(argv[0], argv[1], msg);
 		break;

-- 
1.7.11.rc3.11.g7dba3f7.dirty
Junio C Hamano· Jun 17, 2012, 20:55 UTC · re: Michael Schubert · lore

Re: [PATCH] symbolic-ref: check format of given reference

Michael Schubert <mschub@elegosoft.com> writes:
Show 6 quoted lines
> This was discussed earlier this year:
>
> http://thread.gmane.org/gmane.comp.version-control.git/189715
>
> What about pointing at non-existing references? Should this
> still be allowed?

How else would you reimplement "checkout --orphan" in your own Porcelain using symbolic-ref?

>
> Additionally, I had to reindent two lines to make git-am happy
> (indent with spaces).

I doubt that it is needed; the '-' lines show runs of HT followed by fewer than 8 SP, which should not trigger "indent with spaces".

Show 29 quoted lines
>  builtin/symbolic-ref.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index 801d62e..22362e0 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -43,16 +43,18 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  	argc = parse_options(argc, argv, prefix, options,
> -			     git_symbolic_ref_usage, 0);
> -	if (msg &&!*msg)
> +				git_symbolic_ref_usage, 0);
> +	if (msg && !*msg)
>  		die("Refusing to perform update with empty message");
>  	switch (argc) {
>  	case 1:
>  		check_symref(argv[0], quiet);
>  		break;
>  	case 2:
> +		if (check_refname_format(argv[1], 0))
> +			die("No valid reference format: '%s'", argv[1]);
>  		if (!strcmp(argv[0], "HEAD") &&
> -		    prefixcmp(argv[1], "refs/"))
> +			prefixcmp(argv[1], "refs/"))
>  			die("Refusing to point HEAD outside of refs/");
>  		create_symref(argv[0], argv[1], msg);
>  		break;
Michael Schubert· Jun 18, 2012, 12:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: check format of given reference

On 06/17/2012 10:55 PM, Junio C Hamano wrote:
Show 11 quoted lines
> Michael Schubert <mschub@elegosoft.com> writes:
> 
>> This was discussed earlier this year:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/189715
>>
>> What about pointing at non-existing references? Should this
>> still be allowed?
> 
> How else would you reimplement "checkout --orphan" in your own
> Porcelain using symbolic-ref?
Forgot about that.
Show 6 quoted lines
>>
>> Additionally, I had to reindent two lines to make git-am happy
>> (indent with spaces).
> 
> I doubt that it is needed; the '-' lines show runs of HT followed by
> fewer than 8 SP, which should not trigger "indent with spaces".

I've only noticed because git-am was telling me when I tried to apply the patch.? Am I missing something?

Junio C Hamano· Jun 18, 2012, 16:39 UTC · re: Michael Schubert · lore

Re: [PATCH] symbolic-ref: check format of given reference

Michael Schubert <mschub@elegosoft.com> writes:
Show 8 quoted lines
>>> Additionally, I had to reindent two lines to make git-am happy
>>> (indent with spaces).
>> 
>> I doubt that it is needed; the '-' lines show runs of HT followed by
>> fewer than 8 SP, which should not trigger "indent with spaces".
>
> I've only noticed because git-am was telling me when I tried to
> apply the patch.? Am I missing something?
Perhaps, but I cannot tell exactly what you are doing wrong.

If you didn't touch lines you did not have to in a way to break indentation and cause "indent with spaces", "am" would not have complained (it only looks at "+" lines).

Attached is a patch based on your patch but removes the unnecessary re-indentation part, and "git am" happily applies it to my tree without complaining. Does it apply for you (obviously to a revision without your patch) cleanly without complaint? Otherwise it could be that whitespace categories that are specified for the file in your local attributes file may be different from mine (i.e. an empty set).

-- >8 --
From: Michael Schubert <mschub@elegosoft.com>
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
    >
    >
    > '
Fix this by checking the given reference with check_refname_format().
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/symbolic-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to builtin/symbolic-ref.c +3 −1
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 801d62e..a529541 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options,
 			     git_symbolic_ref_usage, 0);
-	if (msg &&!*msg)
+	if (msg && !*msg)
 		die("Refusing to perform update with empty message");
 	switch (argc) {
 	case 1:
 		check_symref(argv[0], quiet);
 		break;
 	case 2:
+		if (check_refname_format(argv[1], 0))
+			die("No valid reference format: '%s'", argv[1]);
 		if (!strcmp(argv[0], "HEAD") &&
 		    prefixcmp(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");

-- 
1.7.11
Junio C Hamano· Jun 18, 2012, 17:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: check format of given reference

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> From: Michael Schubert <mschub@elegosoft.com>
> 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?

Jeff King· Jun 19, 2012, 14:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: check format of given reference

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 <peff@peff.net>
    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
Jeff King· Jun 19, 2012, 14:52 UTC · re: Jeff King · lore

Re: [PATCH] symbolic-ref: check format of given reference

On Tue, Jun 19, 2012 at 10:47:12AM -0400, Jeff King wrote:
Show 25 quoted lines
> 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 <peff@peff.net>
>     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
Michael Schubert· Jun 19, 2012, 14:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: check format of given reference

On 06/18/2012 06:39 PM, Junio C Hamano wrote:
Show 12 quoted lines
> Michael Schubert <mschub@elegosoft.com> writes:
> 
>>>> Additionally, I had to reindent two lines to make git-am happy
>>>> (indent with spaces).
>>>
>>> I doubt that it is needed; the '-' lines show runs of HT followed by
>>> fewer than 8 SP, which should not trigger "indent with spaces".
>>
>> I've only noticed because git-am was telling me when I tried to
>> apply the patch.? Am I missing something?
> 
> Perhaps, but I cannot tell exactly what you are doing wrong.

Thunderbird replaced the tabs (but did not do that before). Sorry for the noise.

Show 8 quoted lines
> If you didn't touch lines you did not have to in a way to break
> indentation and cause "indent with spaces", "am" would not have
> complained (it only looks at "+" lines).
> 
> Attached is a patch based on your patch but removes the unnecessary
> re-indentation part, and "git am" happily applies it to my tree
> without complaining.  Does it apply for you (obviously to a revision
> without your patch) cleanly without complaint?
Works, Thanks.
Show 43 quoted lines
> -- >8 --
> From: Michael Schubert <mschub@elegosoft.com>
> 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
>     >
>     >
>     > '
> 
> Fix this by checking the given reference with check_refname_format().
> 
> Signed-off-by: Michael Schubert <mschub@elegosoft.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/symbolic-ref.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index 801d62e..a529541 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
>  	git_config(git_default_config, NULL);
>  	argc = parse_options(argc, argv, prefix, options,
>  			     git_symbolic_ref_usage, 0);
> -	if (msg &&!*msg)
> +	if (msg && !*msg)
>  		die("Refusing to perform update with empty message");
>  	switch (argc) {
>  	case 1:
>  		check_symref(argv[0], quiet);
>  		break;
>  	case 2:
> +		if (check_refname_format(argv[1], 0))
> +			die("No valid reference format: '%s'", argv[1]);
>  		if (!strcmp(argv[0], "HEAD") &&
>  		    prefixcmp(argv[1], "refs/"))
>  			die("Refusing to point HEAD outside of refs/");
>

← back to recent threads