From: Jakub Narebski <jnareb@gmail.com>
Date: Wed, 14 Oct 2009 08:23:14 GMT
Subject: Re: [PATCH] gitweb: fix esc_param
Message-ID: <200910141023.15475.jnareb@gmail.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