gitweb: Handle invalid regexp in regexp search

When using regexp search ('sr' parameter / $search_use_regexp variable is true), check first that regexp is valid.

Quoted 2 lines · Jakub Narebski

When using regexp search ('sr' parameter / $search_use_regexp variable is true), check first that regexp is valid.

Thanks.

How old is this bug? Should it go to older maitenance tracks like 1.7.6?

From what I examined this bug is from the very beginning when gitweb

When using regexp search ('sr' parameter / $search_use_regexp variable is true), check first that regexp is valid.

Without this patch we would get an error from Perl during search (if searching is performed by gitweb), or highlighting matches substring (if applicable), if user provided invalid regexp... which means broken HTML, with error page (including HTTP headers) generated after gitweb already produced some output.

Add test that illustrates such error: for example for regexp "*\.git" we would get the following error:

Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/ at /var/www/cgi-bin/gitweb.cgi line 3084.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
See "Re: gitweb: (potential) problems with new installation"
http://thread.gmane.org/gmane.comp.version-control.git/191746

Quoted 12 lines · Jakub Narebski

When using regexp search ('sr' parameter / $search_use_regexp variable is true), check first that regexp is valid.

Without this patch we would get an error from Perl during search (if searching is performed by gitweb), or highlighting matches substring (if applicable), if user provided invalid regexp... which means broken HTML, with error page (including HTTP headers) generated after gitweb already produced some output.

Add test that illustrates such error: for example for regexp "*\.git" we would get the following error:

Quoted 8 lines

Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/ at /var/www/cgi-bin/gitweb.cgi line 3084.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
See "Re: gitweb: (potential) problems with new installation"
http://thread.gmane.org/gmane.comp.version-control.git/191746

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

If you have a leading '*' or '+', in the non-regex case, then you still get the above complaint (and xml error page etc.), although the line number has changed slightly from that given above.

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
I think this bug was here from the very beginning of adding project search, i.e. from v1.6.0.2-446-g0d1d154 (gitweb: Support for simple project search form, 2008-10-03) which was present since 1.6.1

Quoted 3 lines · Ramsay Jones

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

Quoted 3 lines

If you have a leading '*' or '+', in the non-regex case, then you still get the above complaint (and xml error page etc.), although the line number has changed slightly from that given above.

Ramsay, please provide those line number in the future, together with line and if possible some context.

The line is different because it is different bug: this is about not using quotemeta'ed string for search for fixed-string search.

 gitweb/gitweb.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

gitweb/gitweb.perl +11 -11

index 22ad279..7398be1 100755
@@ -3072,16 +3072,16 @@ sub filter_forks_from_projects_list {
 # for 'descr_long' and 'ctags' to be filled
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
-	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $tagfilter = $opts{'tagfilter'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	# searching projects require filling to be run before it;
 	fill_project_list_info($projlist,
-	                       $tagfilter  ? 'ctags' : (),
-	                       $searchtext ? ('path', 'descr') : ());
+	                       $tagfilter ? 'ctags' : (),
+	                       $search_re ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {

@@ -3092,10 +3092,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;

@@ -5498,9 +5498,9 @@ sub git_project_list_body {
 		if ($check_forks);
 	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);
+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
 	# fill the rest
 	@projects = fill_project_list_info(\@projects);

Quoted 13 lines · Jakub Narebski

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
I think this bug was here from the very beginning of adding project search, i.e. from v1.6.0.2-446-g0d1d154 (gitweb: Support for simple project search form, 2008-10-03) which was present since 1.6.1

On Fri, 2 Mar 2012, Ramsay Jones wrote:

Quoted 3 lines

Quoted 3 lines

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

This patch depends on the more recent changes than the regexp fix, no? I was hoping that we could merge the earlier fix for the regexp case to older maintenance tracks later, but if we were going to do so, we would want to do the same for a fix for fixed-string case.

Quoted 15 lines · Junio C Hamano

Quoted 13 lines · Jakub Narebski

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
I think this bug was here from the very beginning of adding project search, i.e. from v1.6.0.2-446-g0d1d154 (gitweb: Support for simple project search form, 2008-10-03) which was present since 1.6.1

On Fri, 2 Mar 2012, Ramsay Jones wrote:

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

This patch depends on the more recent changes than the regexp fix, no? I was hoping that we could merge the earlier fix for the regexp case to older maintenance tracks later, but if we were going to do so, we would want to do the same for a fix for fixed-string case.

The regexp and non-regexp bugs and fixes are different.

The non-regexp project search bug was using $searchtext instead of $search_regexp as search regexp in gitweb. The bug was present from the very addition of project search, namely commit 0d1d154 (gitweb:
Support for simple project search form, 2008-10-03), which was present in v1.5.1 if I have checked correctly. Unfortunately the fix affects code that was changed recently in a1e1b2d (gitweb: improve usability of projects search form, 2012-01-31); I'll try to come up with equivalent patch to 'maint' soon (if the current one does not apply, and I guess it doesn't).

And here is the patch for maint
-->8-- -------------------------------------------------------- -->8--
Subject: gitweb: Fix fixed string (non-regexp) project search

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
 gitweb/gitweb.perl |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

gitweb/gitweb.perl +0 -0

index d5dbd64..e248792 100755
@@ -5290,9 +5290,11 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);

Quoted 25 lines · Jakub Narebski

And here is the patch for maint
-->8-- -------------------------------------------------------- -->8--
Subject: gitweb: Fix fixed string (non-regexp) project search

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
 gitweb/gitweb.perl |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

gitweb/gitweb.perl +0 -3

index d5dbd64..e248792 100755
@@ -5290,9 +5290,11 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);
 	# searching projects require filling to be run before it
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);

gitweb/gitweb.perl sub git_project_list_body { · line 5297

Quoted 5 lines

gitweb/gitweb.perl +5 -0

+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);

Hmph, didn't you already call fill_project_list_info(\@projects) before search_projects_list() already?

Quoted 10 lines · Junio C Hamano

Quoted 1 line · Jakub Narebski

And here is the patch for maint

...

gitweb/gitweb.perl sub git_project_list_body { · line 5297

Quoted 3 lines

gitweb/gitweb.perl +3 -0

+		if ($tagfilter || $search_regexp);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);

Hmph, didn't you already call fill_project_list_info(\@projects) before search_projects_list() already?

True. Sorry about that.

Can you fix that, or should I resend?

Quoted 6 lines · Jakub Narebski

Quoted 2 lines

Hmph, didn't you already call fill_project_list_info(\@projects) before search_projects_list() already?

True. Sorry about that.

Can you fix that, or should I resend?

Could you check the following two diffs?

$ git show debd1c2

This is jn/maint-do-not-match-with-unsanitized-searchtext that should be merged to maintenance track that lack the lazy filling.

And then

$ git show --first-parent d4b52c2

This is how the above was merged to 'pu' and the conflict resolution should be the same when we merge it to 'master'. As our @projects may still be only sparsely filled when search_projects_list() returns, we do call fill_project_list_info(\@projects) ourselves with the lazy filling codebase.

Quoted 18 lines · Junio C Hamano

Quoted 6 lines · Jakub Narebski

Quoted 2 lines

Hmph, didn't you already call fill_project_list_info(\@projects) before search_projects_list() already?

True. Sorry about that.

Can you fix that, or should I resend?

Could you check the following two diffs?

$ git show debd1c2

This is jn/maint-do-not-match-with-unsanitized-searchtext that should be merged to maintenance track that lack the lazy filling.

And then

Quoted 7 lines

$ git show --first-parent d4b52c2

This is how the above was merged to 'pu' and the conflict resolution should be the same when we merge it to 'master'. As our @projects may still be only sparsely filled when search_projects_list() returns, we do call fill_project_list_info(\@projects) ourselves with the lazy filling codebase.

The latter is now

$ git show --first-parent 657c6d0

on today's 'pu'.

Thanks.

Quoted 15 lines · Junio C Hamano

Quoted 13 lines · Jakub Narebski

Use $search_regexp, where regex metacharacters are quoted, for searching projects list, rather than $searchtext, which contains original search term.

Reported-by Ramsay Jones <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
I think this bug was here from the very beginning of adding project search, i.e. from v1.6.0.2-446-g0d1d154 (gitweb: Support for simple project search form, 2008-10-03) which was present since 1.6.1

On Fri, 2 Mar 2012, Ramsay Jones wrote:

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

This patch depends on the more recent changes than the regexp fix, no? I was hoping that we could merge the earlier fix for the regexp case to older maintenance tracks later, but if we were going to do so, we would want to do the same for a fix for fixed-string case.

The regexp and non-regexp bugs and fixes are different.

The regexp "bug" was just us forgetting that regexp is provided by user input, and should be validated. The bug as reported by Ramsay was here from the very beginning, i.e. commit 0e55991 (gitweb: Clearly distinguish regexp / exact match searches, 2008-02-26), which was present in v1.5.1 if I have checked correctly. The fix is about adding new code and should apply cleanly to 'maint' and even to older versions; the only trouble with older version might be whitespace issue related to refactoring code into subroutines.

The non-regexp project search bug was using $searchtext instead of $search_regexp as search regexp in gitweb. The bug was present from the very addition of project search, namely commit 0d1d154 (gitweb:
Support for simple project search form, 2008-10-03), which was present in v1.5.1 if I have checked correctly. Unfortunately the fix affects code that was changed recently in a1e1b2d (gitweb: improve usability of projects search form, 2012-01-31); I'll try to come up with equivalent patch to 'maint' soon (if the current one does not apply, and I guess it doesn't).

.... The fix is about adding new code and should
apply cleanly to 'maint' and even to older versions; the only trouble with older version might be whitespace issue related to refactoring code into subroutines.

Quoted 4 lines · Jakub Narebski

.... The fix is about adding new code and should
apply cleanly to 'maint' and even to older versions; the only trouble with older version might be whitespace issue related to refactoring code into subroutines.

OK, so the global $searchtext is what came from form submit from the end user, while the global $search_regexp is what the code should be using for matching throughout the program, prepared by eval-and-validate-params.

Here is a hand-ported version of your patch that should apply to 1.7.6.6; does it look sane?

 gitweb/gitweb.perl |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

gitweb/gitweb.perl +5 -5

index 50a835a..d1698b7 100755
@@ -2905,10 +2905,10 @@ sub filter_forks_from_projects_list {
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
 	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	my @projects;
  PROJECT:

@@ -2920,10 +2920,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;

Quoted 6 lines

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

If you have a leading '*' or '+', in the non-regex case, then you still get the above complaint (and xml error page etc.), although the line number has changed slightly from that given above.

Ramsay, please provide those line number in the future, together with line and if possible some context.

 gitweb/gitweb.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

Quoted 7 lines · Jakub Narebski

Quoted 6 lines

This patch solves the problem for me when using a regex search (re checkbox checked), but *not* for a non-regex search.

If you have a leading '*' or '+', in the non-regex case, then you still get the above complaint (and xml error page etc.), although the line number has changed slightly from that given above.

Quoted 2 lines

Ramsay, please provide those line number in the future, together with line and if possible some context.

Yeah, sorry about that; when I wrote that I didn't have the information readily available (I would have had to shutdown Windows, boot Linux, ...) and I was about to go out.

So, the choice was to wait about 24hrs to report
with full info, or provide the feedback earlier; I chose the latter. ;-)