# Invalid search parameter in webinterface 9 messages from 2009-04-29 to 2009-04-30. Participants: Olaf Hering, Michael J Gruber, Jakub Narebski, J.H.. Thread: https://gitlist.dev/t/19118 ## Olaf Hering, 2009-04-29 09:12 Subject: Invalid search parameter in webinterface Message-ID: <337ECD47-D343-40F2-9E41-9E7D8531C5DF@aepfle.de> URL: https://gitlist.dev/e/337ECD47-D343-40F2-9E41-9E7D8531C5DF%40aepfle.de ``` 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, 2009-04-29 11:44 Subject: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <1241005459-17311-1-git-send-email-git@drmicha.warpmail.net> URL: https://gitlist.dev/e/1241005459-17311-1-git-send-email-git%40drmicha.warpmail.net In-Reply-To: <337ECD47-D343-40F2-9E41-9E7D8531C5DF@aepfle.de> ``` 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 --- 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 .= " ⋅ " . -- 1.6.3.rc3 ``` ## Jakub Narebski, 2009-04-29 12:28 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <200904291428.46244.jnareb@gmail.com> URL: https://gitlist.dev/e/200904291428.46244.jnareb%40gmail.com In-Reply-To: <1241005459-17311-1-git-send-email-git@drmicha.warpmail.net> ``` On Wed, 29 April 2009, Michael J Gruber wrote: > 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 > --- > Olaf Hering 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. > 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 .= " ⋅ " . 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, 2009-04-29 12:52 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <0626233F-50D5-4F7E-9009-506FEA77571D@aepfle.de> URL: https://gitlist.dev/e/0626233F-50D5-4F7E-9009-506FEA77571D%40aepfle.de In-Reply-To: <200904291428.46244.jnareb@gmail.com> ``` 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, 2009-04-29 13:14 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <200904291514.11331.jnareb@gmail.com> URL: https://gitlist.dev/e/200904291514.11331.jnareb%40gmail.com In-Reply-To: <0626233F-50D5-4F7E-9009-506FEA77571D@aepfle.de> ``` 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, 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, 2009-04-29 19:36 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <200904292136.36987.jnareb@gmail.com> URL: https://gitlist.dev/e/200904292136.36987.jnareb%40gmail.com In-Reply-To: <200904291514.11331.jnareb@gmail.com> ``` 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. -- Jakub Narebski Poland ``` ## Michael J Gruber, 2009-04-29 20:07 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <49F8B36B.6040505@drmicha.warpmail.net> URL: https://gitlist.dev/e/49F8B36B.6040505%40drmicha.warpmail.net In-Reply-To: <200904292136.36987.jnareb@gmail.com> ``` Jakub Narebski venit, vidit, dixit 29.04.2009 21:36: > 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! > >> 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., 2009-04-29 23:11 Subject: Re: [PATCH] gitweb: escape searchtext and parameters for replay Message-ID: <49F8DEB4.40909@eaglescrag.net> URL: https://gitlist.dev/e/49F8DEB4.40909%40eaglescrag.net In-Reply-To: <200904292136.36987.jnareb@gmail.com> ``` Jakub Narebski wrote: > 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, 2009-04-30 10:17 Subject: [PATCH (backport for warthog9/gitweb.git)] gitweb: Separate search regexp from search text Message-ID: <200904301217.27154.jnareb@gmail.com> URL: https://gitlist.dev/e/200904301217.27154.jnareb%40gmail.com In-Reply-To: <49F8DEB4.40909@eaglescrag.net> ``` 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 --- 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... [...] > > ... 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)) . "
"); 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 ```