Cogito limitation in tag names?
From cg-tag in 0.16:
(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
die "name contains invalid characters"
WHY? I can see rejecting control characters, but the above will reject pretty much anything written in a non-English language, which is rude to say the least...
-hpa
PATCH 1/1 make cg-tag use git-check-ref-format
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
---
cg-tag | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
187670279068e7177149765a7f736cc565a1fe19
cg-tag +1 -1
@@ -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
Signature · 1 line
0.99.9.GIT
Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format
Review
Quoted 7 lines · 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
...(quoted diff) +1
-1-(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.
---
cg-tag +5 -5
@@ -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
Quoted 11 lines · Junio C Hamano
Review
Quoted 7 lines · 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 ...
(quoted diff) +1
-1-(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \ +git-check-ref-format $name || \ die "name contains invalid characters"Perhaps you meant to say:
Quoted 2 lines
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 -
Signature · 2 lines
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
In reply to Junio C HamanoDear diary, on Tue, Dec 13, 2005 at 12:13:12PM CET, I got a letter where Junio C Hamano <[email protected]> said that...
Quoted 12 lines
Review
Quoted 7 lines · 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
...(quoted diff) +1
-1-(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \ +git-check-ref-format $name || \ die "name contains invalid characters"Perhaps you meant to say:
Quoted 8 lines
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. ;-)
Signature · 4 lines
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
Quoted 3 lines · Petr Baudis
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
Junio C Hamano, Tue, Dec 13, 2005 19:41:27 +0100:
Quoted 6 lines
Quoted 4 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...
Re: [PATCH] [COGITO] make cg-tag use git-check-ref-format
Quoted 10 lines · Alex Riesen
Junio C Hamano, Tue, Dec 13, 2005 19:41:27 +0100:
Quoted 6 lines
Quoted 4 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...
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
Quoted 8 lines · Junio C Hamano
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().
---
Documentation/git-check-ref-format.txt | 8 +++++--- refs.c | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
d04ec25a77249095b6d2af5a08fe131351f2d86d
Documentation/git-check-ref-format.txt +5 -3
@@ -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
refs.c +3 -1
@@ -313,7 +313,9 @@ int write_ref_sha1(const char *ref, intstatic 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)
Signature · 1 line
0.99.9.GIT
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 <[email protected]> said that...
Quoted 5 lines
Quoted 3 lines · Junio C Hamano
The wildcard letters like ? and * I understand and sympathetic about somewhat. Something like this:
Quoted 7 lines
Quoted 5 lines
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,
Signature · 4 lines
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.
Probably it can safely be updated to use check-ref-format, and I suspect that the only reason why it does not do so is because check-ref-format came later.