threads / discuss / 19118

Invalid search parameter in webinterface

Subject: Invalid search parameter in webinterface

## tl;dr

9 messages between Apr 29, 2009 and Apr 30, 2009.

replies: 8people: 4as markdown or json

Olaf Hering· Apr 29, 2009, 09:12 UTC · lore
Hello.

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
Olaf
Michael J Gruber· Apr 29, 2009, 11:44 UTC · re: Olaf Hering · lore

[PATCH] gitweb: escape searchtext and parameters for replay

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>
---
Maybe something like this? Highly untested!

Cheers, Michael

 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; " .

-- 
1.6.3.rc3
Jakub Narebski· Apr 29, 2009, 12:28 UTC · re: Michael J Gruber · lore

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

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
Olaf Hering· Apr 29, 2009, 12:52 UTC · re: Jakub Narebski · lore

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

Am 29.04.2009 um 14:28 schrieb Jakub Narebski:
> On Wed, 29 April 2009, Michael J Gruber wrote:
>>> 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?

I tried akpm@osdl.org, and @ and . was escaped with a backslash. Removing both, and using the plain mail address worked.

Olaf
Jakub Narebski· Apr 29, 2009, 13:14 UTC · re: Olaf Hering · lore

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

On Wed, 29 Apr 2009, Olaf Hering wrote:
Show 10 quoted lines
> Am 29.04.2009 um 14:28 schrieb Jakub Narebski: 
>> On Wed, 29 April 2009, Michael J Gruber wrote:
> 
>>>> 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?
> 
> I tried akpm@osdl.org, and @ and . was escaped with a backslash.
> Removing both, and using the plain mail address worked.

This problem was fixed in 7e431ef (gitweb: Separate search regexp from search text), but this fix is present in git 1.5.2 and later, and kernel.org gitweb is a fork (adding caching mechanism) of 1.4.5-rc0, according to info in git.kernel.org HTML source, and it does not have this fix.

  http://git.kernel.org/?p=git/warthog9/gitweb.git;a=summary
I have CC-ed J.H. on this.
-- 
Jakub Narebski
Poland
Jakub Narebski· Apr 29, 2009, 19:36 UTC · re: Jakub Narebski · lore

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

On Wed, 29 Apr 2009, Jakub Narebski wrote:
Show 14 quoted lines
> On Wed, 29 Apr 2009, Olaf Hering wrote:
>> Am 29.04.2009 um 14:28 schrieb Jakub Narebski: 
>>> On Wed, 29 April 2009, Michael J Gruber wrote:
>>>>> 
>>>>> 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?
>> 
>> I tried akpm@osdl.org, and @ and . was escaped with a backslash.
>> Removing both, and using the plain mail address worked.
> 
> This problem was fixed in 7e431ef (gitweb: Separate search regexp from 
> search text), but this fix is present in git 1.5.2 and later,

I have just checked that current gitweb (1.6.2.rc1.20.g8c5b) does not have this bug...

Show 7 quoted lines
> and kernel.org gitweb is a fork (adding caching mechanism) of
> 1.4.5-rc0 (according to info in git.kernel.org HTML source), and
> it does not have this fix.
> 
>   http://git.kernel.org/?p=git/warthog9/gitweb.git;a=summary
> 
> I have CC-ed J.H. on this.

... so J.H., or Lea Wiemann, or whoever manages gitweb on kernel.org should backport this fix to kernel.org's fork of gitweb sources.

-- 
Jakub Narebski
Poland
Michael J Gruber· Apr 29, 2009, 20:07 UTC · re: Jakub Narebski · lore

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

Jakub Narebski venit, vidit, dixit 29.04.2009 21:36:
Show 18 quoted lines
> On Wed, 29 Apr 2009, Jakub Narebski wrote:
>> On Wed, 29 Apr 2009, Olaf Hering wrote:
>>> Am 29.04.2009 um 14:28 schrieb Jakub Narebski: 
>>>> On Wed, 29 April 2009, Michael J Gruber wrote:
>>>>>>
>>>>>> 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?
>>>
>>> I tried akpm@osdl.org, and @ and . was escaped with a backslash.
>>> Removing both, and using the plain mail address worked.
>>
>> This problem was fixed in 7e431ef (gitweb: Separate search regexp from 
>> search text), but this fix is present in git 1.5.2 and later,
> 
> I have just checked that current gitweb (1.6.2.rc1.20.g8c5b)
> does not have this bug...
Yes, I should have tested before suggesting a patch blindly. Sorry!
Show 12 quoted lines
> 
>> and kernel.org gitweb is a fork (adding caching mechanism) of
>> 1.4.5-rc0 (according to info in git.kernel.org HTML source), and
>> it does not have this fix.
>>
>>   http://git.kernel.org/?p=git/warthog9/gitweb.git;a=summary
>>
>> I have CC-ed J.H. on this.
> 
> ... so J.H., or Lea Wiemann, or whoever manages gitweb on kernel.org
> should backport this fix to kernel.org's fork of gitweb sources.
>
J.H.· Apr 29, 2009, 23:11 UTC · re: Jakub Narebski · lore

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

Jakub Narebski wrote:
Show 25 quoted lines
> On Wed, 29 Apr 2009, Jakub Narebski wrote:
>> On Wed, 29 Apr 2009, Olaf Hering wrote:
>>> Am 29.04.2009 um 14:28 schrieb Jakub Narebski: 
>>>> On Wed, 29 April 2009, Michael J Gruber wrote:
>>>>>> 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?
>>> I tried akpm@osdl.org, and @ and . was escaped with a backslash.
>>> Removing both, and using the plain mail address worked.
>> This problem was fixed in 7e431ef (gitweb: Separate search regexp from 
>> search text), but this fix is present in git 1.5.2 and later,
> 
> I have just checked that current gitweb (1.6.2.rc1.20.g8c5b)
> does not have this bug...
> 
>> and kernel.org gitweb is a fork (adding caching mechanism) of
>> 1.4.5-rc0 (according to info in git.kernel.org HTML source), and
>> it does not have this fix.
>>
>>   http://git.kernel.org/?p=git/warthog9/gitweb.git;a=summary
>>
>> I have CC-ed J.H. on this.
> 
> ... so J.H., or Lea Wiemann, or whoever manages gitweb on kernel.org
> should backport this fix to kernel.org's fork of gitweb sources.

Patches, as always, are welcome - I've got a few queued up already, but if I have to do it I won't be getting back around to it for a bit (there are other things on my priority list right now).

- John "Warthog9" Hawley
Jakub Narebski· Apr 30, 2009, 10:17 UTC · re: J.H. · lore

[PATCH (backport for warthog9/gitweb.git)] gitweb: Separate search regexp from search text

Separate search text, which is saved in $searchtext global variable, and is used in links, as default value for the textfield in search form, and for pickaxe search, from search regexp, which is saved in $search_regexp global variable, and is used as parameter to --grep, --committer or --author options to git-rev-list, and for searching commit body in gitweb. For now $search_regexp is unconditionally equal to quotemeta($searchtext), meaning that we always search for fixed string.

This fixes bug where 'next page' links for 'search' view didn't work for searchtext containing quotable characters, like `@'.

(manually cherry picked from 7e431ef9ab933d7ff899a999e40627ab49774f3a)

Olaf Hering noticed that this bug was still present, and this bugfix was not ported to kernel.org's fork of gitweb.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Thu, 30 Apr 2009, J.H. wrote:
> Jakub Narebski wrote:
> > I have just checked that current gitweb (1.6.2.rc1.20.g8c5b)
> > does not have this bug...
[...]
Show 6 quoted lines
> > ... so J.H., or Lea Wiemann, or whoever manages gitweb on kernel.org
> > should backport this fix to kernel.org's fork of gitweb sources.
> 
> Patches, as always, are welcome - I've got a few queued up already, but 
> if I have to do it I won't be getting back around to it for a bit (there 
> are other things on my priority list right now).
Here you have backport of 7e431ef (gitweb: Separate search regexp from 
search text) on top of 'master' branch of kernel.org's fork of gitweb:
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git

The only difference is that git_search_grep_body is in separate gitweb/gitweb/search.pm file in kernel.org's fork. The change is otherwise identical.

Not tested!
 gitweb/gitweb.perl      |    5 +++--
 gitweb/gitweb/search.pm |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 984161c..aee9239 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -231,6 +231,7 @@ if (defined $page) {
 }
 
 our $searchtext = $cgi->param('s');
+our $search_regexp;
 if (defined $searchtext) {
 	if ($searchtext =~ m/[^a-zA-Z0-9_\.\/\-\+\:\@ ]/) {
 		die_error(undef, "Invalid search parameter");
@@ -238,7 +239,7 @@ if (defined $searchtext) {
 	if (length($searchtext) < 2) {
 		die_error(undef, "At least two characters are required for search parameter");
 	}
-	$searchtext = quotemeta $searchtext;
+	$search_regexp = quotemeta $searchtext;
 }
 
 our $searchtype = $cgi->param('st');
@@ -1702,7 +1703,7 @@ sub git_search {
 		} elsif ($searchtype eq 'committer') {
 			$greptype = "--committer=";
 		}
-		$greptype .= $searchtext;
+		$greptype .= $search_regexp;
 		my @commitlist = parse_commits($hash, 101, (100 * $page), $greptype);
 
 		my $paging_nav = '';
diff --git a/gitweb/gitweb/search.pm b/gitweb/gitweb/search.pm
index f1aea0a..a381f4e 100644
--- a/gitweb/gitweb/search.pm
+++ b/gitweb/gitweb/search.pm
@@ -33,7 +33,7 @@ sub git_search_grep_body {
 				esc_html(chop_str($co{'title'}, 50)) . "<br/>");
 		my $comment = $co{'comment'};
 		foreach my $line (@$comment) {
-			if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
+			if ($line =~ m/^(.*)($search_regexp)(.*)$/i) {
 				my $lead = esc_html($1) || "";
 				$lead = chop_str($lead, 30, 10);
 				my $match = esc_html($2) || "";

-- 
1.6.2

← back to recent threads