[PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects

[PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects

[Jakub Narebski]

When gitweb URL does not provide action explicitly, e.g.

  http://git.example.org/repo.git/branch

dispatch() tries to guess action (view to be used) based on remaining
parameters.  Among others it is based on the type of requested object,
which gave problems when asking for non-existent branch or file (for
example misspelt name).

Now undefined $action from dispatch() should not result in problems.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm not sure if this is the way to fix it, by erroring-out in
dispatch() and leaving $action undefined.

Testsuite passes, but I have not examined output intensively.

 gitweb/gitweb.perl                     |    4 +++-
 t/t9500-gitweb-standalone-no-errors.sh |    8 ++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f884dfe..e2e04df 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1123,8 +1123,10 @@ sub dispatch {
 	if (!defined $action) {
 		if (defined $hash) {
 			$action = git_get_type($hash);
+			$action or die_error(404, "Object does not exist");
 		} elsif (defined $hash_base && defined $file_name) {
 			$action = git_get_type("$hash_base:$file_name");
+			$action or die_error(404, "File or directory does not exist");
 		} elsif (defined $project) {
 			$action = 'summary';
 		} else {
@@ -2391,7 +2393,7 @@ sub get_feed_info {
 	return unless (defined $project);
 	# some views should link to OPML, or to generic project feed,
 	# or don't have specific feed yet (so they should use generic)
-	return if ($action =~ /^(?:tags|heads|forks|tag|search)$/x);
+	return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
 
 	my $branch;
 	# branches refs uses 'refs/heads/' prefix (fullname) to differentiate
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index ab24917..0f771c6 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -475,6 +475,14 @@ test_expect_success \
 	'gitweb_run "" "/.git/master:foo/"'
 
 test_expect_success \
+	'path_info: project/branch (non-existent)' \
+	'gitweb_run "" "/.git/non-existent"'
+
+test_expect_success \
+	'path_info: project/branch:filename (non-existent branch)' \
+	'gitweb_run "" "/.git/non-existent:non-existent"'
+
+test_expect_success \
 	'path_info: project/branch:file (non-existent)' \
 	'gitweb_run "" "/.git/master:non-existent"'
 

Re: [PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> When gitweb URL does not provide action explicitly, e.g.
>
>   http://git.example.org/repo.git/branch
>
> dispatch() tries to guess action (view to be used) based on remaining
> parameters.  Among others it is based on the type of requested object,
> which gave problems when asking for non-existent branch or file (for
> example misspelt name).

Ok. "gave problems" is a bit unclear to see why explicitly calling
die_error() is an improvement, though. What is the nature of the
"problems"? Giving a server error 500 because later codepaths tried to
call an undefined subroutine?

> Now undefined $action from dispatch() should not result in problems.

Again, unspecified "problems" here. I'd like this sentence to end with
"should not result in X but gives an explicit '404 not found' error".

Thanks.

Re: [PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > When gitweb URL does not provide action explicitly, e.g.
> >
> >   http://git.example.org/repo.git/branch
> >
> > dispatch() tries to guess action (view to be used) based on remaining
> > parameters.  Among others it is based on the type of requested object,
> > which gave problems when asking for non-existent branch or file (for
> > example misspelt name)

                          , because git_get_type() returns undef when
requested object does not exist, and $action was left undefined.

This resulted in Perl generating the "Use of unitialized value" warnings,
which made it in error.log.  Additionally gitweb returned "400 Bad Request"
error instead of more informative "404 Not Found".
 
> 
> Ok. "gave problems" is a bit unclear to see why explicitly calling
> die_error() is an improvement, though. What is the nature of the
> "problems"? Giving a server error 500 because later codepaths tried to
> call an undefined subroutine?
> 
> > Now undefined $action from dispatch() should not result in problems.
> 
> Again, unspecified "problems" here. I'd like this sentence to end with
> "should not result in X but gives an explicit '404 not found' error".

This is about second chunk of change to gitweb/gitweb.perl, which is
responsible about silencing this warning:

   gitweb.perl: Use of uninitialized value in pattern match (m//) at ../gitweb.perl line 2397.

It was present even with the '$action or die_error(404,...)' short-circut,
as this was in the part responsible for generating page header, which is
the same for normal page and for error page.

Perhaps better solution would be to set action to 'object' if requested
object is not found (a valid action in itself).  Hmmm...

-- 
Jakub Narebski
Poland