[PATCH] gitweb: fix esc_param

[PATCH] gitweb: fix esc_param

[Giuseppe Bilotta]

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

Re: [PATCH] gitweb: fix esc_param

[Stephen Boyd]

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.

Re: [PATCH] gitweb: fix esc_param

[Giuseppe Bilotta]

On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd <bebarino@gmail.com> 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 <giuseppe.bilotta@gmail.com>

(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

Re: [PATCH] gitweb: fix esc_param

[Stephen Boyd]

Giuseppe Bilotta wrote:
> On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd <bebarino@gmail.com> 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 .= " <span class=\"$class\" title=\"$ref\">" .
+                       my $title_ref = esc_html($ref);
+                       $markers .= " <span class=\"$class\" title=\"$title_ref\">" .
                                $link . "</span>";
                }
        }

Re: [PATCH] gitweb: fix esc_param

[Jakub Narebski]

On Wed, 14 Oct 2009, Stephen Boyd wrote:
> Giuseppe Bilotta wrote:
>> On Wed, Oct 14, 2009 at 3:13 AM, Stephen Boyd <bebarino@gmail.com> 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 '<b>' is valid branch name).

>  
> -                       $markers .= " <span class=\"$class\" title=\"$ref\">" .
> +                       my $title_ref = esc_html($ref);
> +                       $markers .= " <span class=\"$class\" title=\"$title_ref\">" .

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 . "</span>";
>                 }
>         }

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: fix esc_param

[Jakub Narebski]

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

Re: [PATCH] gitweb: fix esc_param

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> ...
> Finally, remove an unnecessary escaping of the + sign.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Thanks.