Re: [PATCH] gitweb: escape searchtext and parameters for replay

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 29, 2009, 12:28 UTC
Message-ID: <200904291428.46244.jnareb@gmail.com>
In-Reply-To: <1241005459-17311-1-git-send-email-git@drmicha.warpmail.net>

On Wed, 29 April 2009, Michael J Gruber wrote:

Show 10 quoted lines

> Search texts may very likely include characters like '@' when grepping
> for author names. Currently, gitweb produces first/prev/next links with
> incorrectly escaped characters.
> 
> Make gitweb escape searchtext and parameters which are reused in href()
> when replay is set. (cgi params are de-escaped when put into the
> parameter dictionary and need to be re-escaped when reused.)
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---

Show 12 quoted lines

> Olaf Hering <olaf@aepfle.de> wrote:
>
>> An 'author' search string like "torvalds@linux" at
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git
>> generates a 'next' link due to the huge number of commits.
>>
>> This link has an incorrect escaping for the @ sign.
>> The backslash does not work, it generates an error:
>>
>> 403 Forbidden - Invalid search parameter
>>
>> It should be s=torvalds%40linux instead of s=torvalds\@linux

If you by hand edit URL changing '\@' to simply '@', does changed gitweb URL works correctly?

>
> Maybe something like this? Highly untested!

No, the problem is not with lack of URI escaping in 's' parameter ('@' character is not forbidden for query string; it has special meaning and has to be escaped only in the hostname part), but with passing value _quoted for Perl_ (quotemeta) to href() subroutine.

I'll try to come with the solution soon.

Show 25 quoted lines

>  gitweb/gitweb.perl |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3f99361..e1b09f8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -848,7 +848,7 @@ sub href (%) {
>  	if ($params{-replay}) {
>  		while (my ($name, $symbol) = each %cgi_param_mapping) {
>  			if (!exists $params{$name}) {
> -				$params{$name} = $input_params{$name};
> +				$params{$name} = esc_url($input_params{$name});
>  			}
>  		}
>  	}
> @@ -5775,7 +5775,7 @@ sub git_search {
>  		if ($page > 0) {
>  			$paging_nav .=
>  				$cgi->a({-href => href(action=>"search", hash=>$hash,
> -				                       searchtext=>$searchtext,
> +				                       searchtext=>esc_url($searchtext),
>  				                       searchtype=>$searchtype)},
>  				        "first");
>  			$paging_nav .= " &sdot; " .

This would result in double URI escaping, as at the end of href() subroutine we have:

	push @result, $symbol . "=" . esc_param($params{$name});

   [...]

   $href .= "?" . join(';', @result) if scalar @result;

   return $href;

Note that gitweb uses esc_param() and not esc_url() here; the rules are slightly different (you need to quote/escape ';', '?', '=' etc. for query parameter).

-- 
Jakub Narebski
Poland