threads / patch / 2820

patch[COGITO] make cg-tag use git-check-ref-format

Subject: [PATCH] [COGITO] make cg-tag use git-check-ref-format

## tl;dr

9 messages between Dec 13, 2005 and Dec 16, 2005. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Martin Atukunda· Dec 13, 2005, 10:54 UTC · lore

The egrep pattern used by cg-tag is too restrictive. While it will prevent control characters from being specified as a tag name, it will also reject nearly anything written in a non-English language, as noted by -hpa

Signed-off-by: Martin Atukunda <matlads@dsmagic.com>
---
 cg-tag |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
187670279068e7177149765a7f736cc565a1fe19
Show changes to cg-tag +1 −1
diff --git a/cg-tag b/cg-tag
index da4f2d5..73616b8 100755
--- a/cg-tag
+++ b/cg-tag
@@ -54,7 +54,7 @@ id=$(cg-object-id -n "$id") || exit 1
 type=$(git-cat-file -t "$id")
 id=${id% *}
 
-(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
+git-check-ref-format $name || \
 	die "name contains invalid characters"
 
 mkdir -p $_git/refs/tags

-- 
0.99.9.GIT
Junio C Hamano· Dec 13, 2005, 11:13 UTC · re: Martin Atukunda · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Martin Atukunda <matlads@dsmagic.com> writes:
Show 7 quoted lines
> The egrep pattern used by cg-tag is too restrictive. While it will prevent
> control characters from being specified as a tag name, it will also reject
> nearly anything written in a non-English language, as noted by -hpa
>...
> -(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
> +git-check-ref-format $name || \
>  	die "name contains invalid characters"
Perhaps you meant to say:
	git-check-ref-format "$name"

instead; after all you are dealing with potentially garbage input from the user here.

While you are at it, you might want to also quote $_git/refs/tags immediately follows the part that you patched, and there is another.

-- >8 -- [PATCH] cg-tag: shell variable quoting.

Scripts sometimes tend to be loose in variable quoting, and often they are justifiable (e.g. the variables are already validated before used unquoted); but when checking the input, we should try to be strict.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
Show changes to cg-tag +5 −5
diff --git a/cg-tag b/cg-tag
index da4f2d5..1efb50d 100755
--- a/cg-tag
+++ b/cg-tag
@@ -28,7 +28,7 @@
 
 USAGE="cg-tag [-d DESCRIPTION] [-s [-k KEYNAME]] TAG_NAME [OBJECT_ID]"
 
-. ${COGITO_LIB}cg-Xlib || exit 1
+. "${COGITO_LIB}cg-Xlib" || exit 1
 
 sign=
 keyname=
@@ -54,10 +54,10 @@ id=$(cg-object-id -n "$id") || exit 1
 type=$(git-cat-file -t "$id")
 id=${id% *}
 
-(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
-	die "name contains invalid characters"
+git-check-ref-format "$name" ||
+	die "name $name contains invalid characters"
 
-mkdir -p $_git/refs/tags
+mkdir -p "$_git/refs/tags"
 
 [ -s "$_git/refs/tags/$name" ] && die "tag already exists ($name)"
 [ "$id" ] || id="$(cat "$_git/$(git-symbolic-ref HEAD)")"
@@ -83,7 +83,7 @@ if [ "$sign" ]; then
 	fi
 	cat "$tagdir/tag.asc" >>"$tagdir/tag"
 fi
-if ! git-mktag <"$tagdir/tag" >$_git/refs/tags/$name; then
+if ! git-mktag <"$tagdir/tag" >"$_git/refs/tags/$name"; then
 	rm -rf "$tagdir"
 	die "error creating tag"
 fi
Martin Atukunda· Dec 13, 2005, 11:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

On Tuesday 13 December 2005 14:13, Junio C Hamano wrote:
Show 13 quoted lines
> Martin Atukunda <matlads@dsmagic.com> writes:
> > The egrep pattern used by cg-tag is too restrictive. While it will
> > prevent control characters from being specified as a tag name, it will
> > also reject nearly anything written in a non-English language, as noted
> > by -hpa ...
> > -(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
> > +git-check-ref-format $name || \
> >  	die "name contains invalid characters"
>
> Perhaps you meant to say:
>
> 	git-check-ref-format "$name"
>

Yes. i've just finished preparing a new patch with this exact change. But your patch is much better.

- Martin -
-- 
Due to a shortage of devoted followers, the production of great leaders has 
been discontinued.
Petr Baudis· Dec 13, 2005, 17:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Dear diary, on Tue, Dec 13, 2005 at 12:13:12PM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 20 quoted lines
> Martin Atukunda <matlads@dsmagic.com> writes:
> 
> > The egrep pattern used by cg-tag is too restrictive. While it will prevent
> > control characters from being specified as a tag name, it will also reject
> > nearly anything written in a non-English language, as noted by -hpa
> >...
> > -(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
> > +git-check-ref-format $name || \
> >  	die "name contains invalid characters"
> 
> Perhaps you meant to say:
> 
> 	git-check-ref-format "$name"
> 
> instead; after all you are dealing with potentially garbage
> input from the user here.
> 
> While you are at it, you might want to also quote $_git/refs/tags
> immediately follows the part that you patched, and there is
> another.

Thank you both for the patch, but I'd be much more comfortable if at least quotes (both ' and "), backslashes, ? and * would be prohibited in the names as well. Any chance of also implementing this policy upstream? Taken to the extreme, using such a names for tags might be perceived as a possible security vulnerability wrt. the less shell-savy users. ;-)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.
Junio C Hamano· Dec 13, 2005, 18:41 UTC · re: Petr Baudis · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Petr Baudis <pasky@suse.cz> writes:
> Thank you both for the patch, but I'd be much more comfortable if at
> least quotes (both ' and "), backslashes, ? and * would be prohibited in
> the names as well.
I second that, and thanks for pointing it out.  Any objections?
Alex Riesen· Dec 15, 2005, 22:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Junio C Hamano, Tue, Dec 13, 2005 19:41:27 +0100:
Show 6 quoted lines
> 
> > Thank you both for the patch, but I'd be much more comfortable if at
> > least quotes (both ' and "), backslashes, ? and * would be prohibited in
> > the names as well.
> 
> I second that, and thanks for pointing it out.  Any objections?

Just as a warning, perhaps? It's not like git is anywhere limited in this respect...

Junio C Hamano· Dec 15, 2005, 23:38 UTC · re: Alex Riesen · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Alex Riesen <raa.lkml@gmail.com> writes:
Show 10 quoted lines
> Junio C Hamano, Tue, Dec 13, 2005 19:41:27 +0100:
>> 
>> > Thank you both for the patch, but I'd be much more comfortable if at
>> > least quotes (both ' and "), backslashes, ? and * would be prohibited in
>> > the names as well.
>> 
>> I second that, and thanks for pointing it out.  Any objections?
>
> Just as a warning, perhaps? It's not like git is anywhere limited in
> this respect...
Yeah, after thinking about it a bit more, I changed my mind.

The wildcard letters like ? and * I understand and sympathetic about somewhat. Something like this:

        name="*.sh" ;# this also comes from the end user
        echo $name

ends up showing every shell script in the current directory, and not literal '*.sh'.

However, I do not think covering five characters '"\?* gives us anything, and sends a strong message that we do not know our shell programming to whoever is reading our code. For one thing, the user can still say "foo[a-z]bar" to confuse you, so you also need to forbid [].

The thing is, if you start to care about single and double quotes, then what you are doing carelessly is not something simple like this:

	name='frotz'\''nitfol"filfre\xyzzy' ;# this comes from the end user.
	echo $name ;# and this prints just fine.

For quotes to matter, you must be doing an "eval" carelessly, and "eval" and careless should never go together.

        # do not try this in your repository without echo
	name="foo; echo rm -fr ."
        eval "git-rev-parse $name"

You end up needing to forbid a lot more than the quoting and wildcard, if you want to keep your shell scripts loose and lazy; which may be a worthy goal in itself but pretty much defeats the initial discussion of "why do we allow only these characters in tags".

So in short, I am somewhat negative about the idea of adding more "forbidden letters". Let's make sure our scripts are careful where safety matters.

Note that this does not forbid Porcelains to enforce additional restrictions on their own.

Junio C Hamano· Dec 16, 2005, 02:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Junio C Hamano <junkio@cox.net> writes:
Show 8 quoted lines
> The wildcard letters like ? and * I understand and sympathetic
> about somewhat.  Something like this:
>
>         name="*.sh" ;# this also comes from the end user
>         echo $name
>
> ends up showing every shell script in the current directory,
> and not literal '*.sh'.
So why don't we do this?
-- >8 --
Subject: [PATCH] Forbid pattern maching characters in refnames.
by marking '?', '*', and '[' as bad_ref_char().
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 Documentation/git-check-ref-format.txt |    8 +++++---
 refs.c                                 |    4 +++-
 2 files changed, 8 insertions(+), 4 deletions(-)
d04ec25a77249095b6d2af5a08fe131351f2d86d
Show changes to 2 files +8 −4

Documentation/git-check-ref-format.txt, refs.c

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 636e951..f7f84c6 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -26,13 +26,15 @@ imposes the following rules on how refs 
 
 . It cannot have ASCII control character (i.e. bytes whose
   values are lower than \040, or \177 `DEL`), space, tilde `~`,
-  caret `{caret}`, or colon `:` anywhere;
+  caret `{caret}`, colon `:`, question-mark `?`, asterisk `*`,
+  or open bracket `[` anywhere;
 
 . It cannot end with a slash `/`.
 
 These rules makes it easy for shell script based tools to parse
-refnames, and also avoids ambiguities in certain refname
-expressions (see gitlink:git-rev-parse[1]).  Namely:
+refnames, pathname expansion by the shell when a refname is used
+unquoted (by mistake), and also avoids ambiguities in certain
+refname expressions (see gitlink:git-rev-parse[1]).  Namely:
 
 . double-dot `..` are often used as in `ref1..ref2`, and in some
   context this notation means `{caret}ref1 ref2` (i.e. not in
diff --git a/refs.c b/refs.c
index b8fcb98..0d63c1f 100644
--- a/refs.c
+++ b/refs.c
@@ -313,7 +313,9 @@ int write_ref_sha1(const char *ref, int 
 static inline int bad_ref_char(int ch)
 {
 	return (((unsigned) ch) <= ' ' ||
-		ch == '~' || ch == '^' || ch == ':');
+		ch == '~' || ch == '^' || ch == ':' ||
+		/* 2.13 Pattern Matching Notation */
+		ch == '?' || ch == '*' || ch == '[');
 }
 
 int check_ref_format(const char *ref)

-- 
0.99.9.GIT
Petr Baudis· Dec 16, 2005, 09:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format

Dear diary, on Fri, Dec 16, 2005 at 03:17:19AM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 12 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
> 
> > The wildcard letters like ? and * I understand and sympathetic
> > about somewhat.  Something like this:
> >
> >         name="*.sh" ;# this also comes from the end user
> >         echo $name
> >
> > ends up showing every shell script in the current directory,
> > and not literal '*.sh'.
> 
> So why don't we do this?
I'm all for it, and now I also agree that forbidding \'" is pointless.
Thanks,
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

← back to recent threads