threads / rfc / 29309

RFC patchgitweb: Fix actionless dispatch for non-existent objects

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

## tl;dr

3 messages between Jan 7, 2012 and Jan 9, 2012. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jakub Narebski· Jan 7, 2012, 10:47 UTC · lore
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(-)
Show changes to 2 files +11 −1

gitweb/gitweb.perl, t/t9500-gitweb-standalone-no-errors.sh

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"'
Junio C Hamano· Jan 9, 2012, 21:30 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 8 quoted lines
> 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.
Jakub Narebski· Jan 9, 2012, 22:05 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 10 quoted lines
> 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".
Show 10 quoted lines
> 
> 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

← back to recent threads