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

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

[Martin Atukunda]

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
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

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

[Junio C Hamano]

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.

-- >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>
---

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

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

[Martin Atukunda]

On Tuesday 13 December 2005 14:13, Junio C Hamano wrote:
> 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.

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

[Petr Baudis]

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...
> 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.

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

[Junio C Hamano]

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?

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

[Alex Riesen]

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...

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

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> 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.

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

[Junio C Hamano]

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?

-- >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
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

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

[Petr Baudis]

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...
> 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.