# [PATCH] gitweb: fix esc_param 7 messages from 2009-10-13 to 2009-10-14. Participants: Giuseppe Bilotta, Stephen Boyd, Jakub Narebski, Junio C Hamano. Thread: https://gitlist.dev/t/21229 ## Giuseppe Bilotta, 2009-10-13 19:51 Subject: [PATCH] gitweb: fix esc_param Message-ID: <1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com> URL: https://gitlist.dev/e/1255463496-21617-1-git-send-email-giuseppe.bilotta%40gmail.com ``` The custom CGI escaping done in esc_param failed to escape UTF-8 properly. Fix by using CGI::escape on each sequence of matched characters instead of sprintf()ing a custom escaping for each byte. Additionally, the space -> + escape was being escaped due to greedy matching on the first substitution. Fix by adding space to the list of characters not handled on the first substitution. Finally, remove an unnecessary escaping of the + sign. --- gitweb/gitweb.perl | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) The issues with this routine were exposed by Stephen's "author as search link" patch. This should fix them. Since the idea of esc_param is to replicate CGI::escape except for the / character (if I read the comment correclty), a possible alternative would be to just use CGI::escape on the whole string and then undo the escaping for the / character. diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 6237865..6593e5c 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1115,8 +1115,7 @@ sub to_utf8 { # correct, but quoted slashes look too horrible in bookmarks sub esc_param { my $str = shift; - $str =~ s/([^A-Za-z0-9\-_.~()\/:@])/sprintf("%%%02X", ord($1))/eg; - $str =~ s/\+/%2B/g; + $str =~ s/([^A-Za-z0-9\-_.~()\/:@ ]+)/CGI::escape($1)/eg; $str =~ s/ /\+/g; return $str; } -- 1.6.3.rc1.192.gdbfcb ``` ## Stephen Boyd, 2009-10-14 01:13 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: <4AD525CA.8090102@gmail.com> URL: https://gitlist.dev/e/4AD525CA.8090102%40gmail.com In-Reply-To: <1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com> ``` Giuseppe Bilotta wrote: > The custom CGI escaping done in esc_param failed to escape UTF-8 > properly. Fix by using CGI::escape on each sequence of matched > characters instead of sprintf()ing a custom escaping for each byte. > > Additionally, the space -> + escape was being escaped due to greedy > matching on the first substitution. Fix by adding space to the > list of characters not handled on the first substitution. > > Finally, remove an unnecessary escaping of the + sign. > Signoff? This works great for my purposes. Thanks. ``` ## Giuseppe Bilotta, 2009-10-14 06:19 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: URL: https://gitlist.dev/e/cb7bb73a0910132319p27b4f362sf5bffa2afe6e5849%40mail.gmail.com In-Reply-To: <4AD525CA.8090102@gmail.com> ``` On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote: > Giuseppe Bilotta wrote: >> The custom CGI escaping done in esc_param failed to escape UTF-8 >> properly. Fix by using CGI::escape on each sequence of matched >> characters instead of sprintf()ing a custom escaping for each byte. >> >> Additionally, the space -> + escape was being escaped due to greedy >> matching on the first substitution. Fix by adding space to the >> list of characters not handled on the first substitution. >> >> Finally, remove an unnecessary escaping of the + sign. >> > > Signoff? Doh. Signed-off-by: Giuseppe Bilotta (If this isn't enough, let me know) > This works great for my purposes. Thanks. Can you also check if this fixes the branch name issue you mentioned in the other branch? (And/or do you have a repository exposing the problem if not?) -- Giuseppe "Oblomov" Bilotta ``` ## Stephen Boyd, 2009-10-14 06:29 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: <4AD56FD9.70602@gmail.com> URL: https://gitlist.dev/e/4AD56FD9.70602%40gmail.com In-Reply-To: ``` Giuseppe Bilotta wrote: > On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote: >> This works great for my purposes. Thanks. > > Can you also check if this fixes the branch name issue you mentioned > in the other branch? (And/or do you have a repository exposing the > problem if not?) (We're jumping back and forth between threads haha) Sorry, it doesn't. But this diff fixes the first part of the problem. There are still problems with the RSS/Atom feed names being mangled. The branch name I'm using is gitwéb, but I imagine any utf8 character will fail. I see the title and the actual text being mangled without this patch. diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4b21ad2..910c370 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1500,9 +1500,10 @@ sub format_ref_marker { -href => href( action=>$dest_action, hash=>$dest - )}, $name); + )}, esc_html($name)); - $markers .= " " . + my $title_ref = esc_html($ref); + $markers .= " " . $link . ""; } } ``` ## Jakub Narebski, 2009-10-14 08:23 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: <200910141023.15475.jnareb@gmail.com> URL: https://gitlist.dev/e/200910141023.15475.jnareb%40gmail.com In-Reply-To: <1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com> ``` On Tue, 13 Oct 2009, Giuseppe Bilotta wrote: > The custom CGI escaping done in esc_param failed to escape UTF-8 > properly. Fix by using CGI::escape on each sequence of matched > characters instead of sprintf()ing a custom escaping for each byte. Hmmm... I wonder if this bug isn't caused by failing to mark some input as utf8 using to_utf8() subroutine... or by using binmode $fd, ':utf8' on $fd from opening git-rev-list, after ensuring that it outputs utf8 by --encoding=utf8 (or is it only git-log that accepts that option?). > > Additionally, the space -> + escape was being escaped due to greedy > matching on the first substitution. Fix by adding space to the > list of characters not handled on the first substitution. Thanks. > > Finally, remove an unnecessary escaping of the + sign. Signoff? > --- > gitweb/gitweb.perl | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > The issues with this routine were exposed by Stephen's > "author as search link" patch. This should fix them. > > Since the idea of esc_param is to replicate CGI::escape except for the / > character (if I read the comment correclty), a possible alternative > would be to just use CGI::escape on the whole string and then undo the > escaping for the / character. Well, that and widely used but non-standard (well, not using percent encoding) escaping of space with '+'; CGI::escape(" ") is %20, not '+'. Se we would have to turn '%2F' into '/', and '%20' into '+'. > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 6237865..6593e5c 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -1115,8 +1115,7 @@ sub to_utf8 { > # correct, but quoted slashes look too horrible in bookmarks > sub esc_param { > my $str = shift; > - $str =~ s/([^A-Za-z0-9\-_.~()\/:@])/sprintf("%%%02X", ord($1))/eg; > - $str =~ s/\+/%2B/g; > + $str =~ s/([^A-Za-z0-9\-_.~()\/:@ ]+)/CGI::escape($1)/eg; > $str =~ s/ /\+/g; > return $str; > } > -- > 1.6.3.rc1.192.gdbfcb > > -- Jakub Narebski Poland ``` ## Jakub Narebski, 2009-10-14 09:03 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: <200910141103.19295.jnareb@gmail.com> URL: https://gitlist.dev/e/200910141103.19295.jnareb%40gmail.com In-Reply-To: <4AD56FD9.70602@gmail.com> ``` On Wed, 14 Oct 2009, Stephen Boyd wrote: > Giuseppe Bilotta wrote: >> On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd wrote: >> >>> This works great for my purposes. Thanks. >>> >> Can you also check if this fixes the branch name issue you mentioned >> in the other branch? (And/or do you have a repository exposing the >> problem if not?) > > (We're jumping back and forth between threads haha) > > Sorry, it doesn't. But this diff fixes the first part of the problem. > There are still problems with the RSS/Atom feed names being mangled. The > branch name I'm using is gitwéb, but I imagine any utf8 character will fail. That it is (probably) not because of lack of esc_html, but because of lack of to_utf8. Grrr... we really should convert to utf8 (usually just mark as utf8) on reading input, to avoid such kind of errors. > > I see the title and the actual text being mangled without this patch. > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 4b21ad2..910c370 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -1500,9 +1500,10 @@ sub format_ref_marker { > -href => href( > action=>$dest_action, > hash=>$dest > - )}, $name); > + )}, esc_html($name)); Hmmm... to_utf8 would be enough here, but for the fact that git-check-ref-format doesn't prohibit '<', '>' in ref names, so it is possible that somebody somewhere is using such strange ref names (e.g. branch named '' is valid branch name). > > - $markers .= " " . > + my $title_ref = esc_html($ref); > + $markers .= " " . Here it would be really useful to have esc_attr (which would do to_utf8 plus escaping of '"' and '%', quote and escape characters), although I guess that full HTML escaping done with esc_html wouldn't do anything bad. > $link . ""; > } > } -- Jakub Narebski Poland ``` ## Junio C Hamano, 2009-10-14 09:13 Subject: Re: [PATCH] gitweb: fix esc_param Message-ID: <7vpr8qe3s9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr8qe3s9.fsf%40alter.siamese.dyndns.org In-Reply-To: <1255463496-21617-1-git-send-email-giuseppe.bilotta@gmail.com> ``` Giuseppe Bilotta writes: > ... > Finally, remove an unnecessary escaping of the + sign. > > Signed-off-by: Giuseppe Bilotta Thanks. ```