git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] gitweb: standarize HTTP status codes

From
LWLea Wiemann <lewiemann@gmail.com>
Date
Jun 19, 2008, 19:08 UTC
Message-ID
<485AAEB9.2080100@gmail.com>
In-Reply-To
<m3y752melj.fsf_-_@localhost.localdomain>
Jakub Narebski wrote:
Show 5 quoted lines
> Lea Wiemann <lewiemann@gmail.com> writes:
>> For convenience the die_error function now only takes the status code
>> without reason as first parameter (e.g. 404 instead of "404 Not Found")
> 
> _Whose_ convenience?
The developer's convenience of course.  It's plain redundant.
Show 6 quoted lines
>  * I don't think that RFC 2616 allows blanket replacing reason phrase
>    by generic "Error",
>  * Test::WWW::Mechanize displays both HTTP error status code and
>    reason phrase when get_ok(...) fails:
>  * From the point of view of someone examinimg gitweb.perl code, 400,
>    403, 404, 500 are _magic numbers_;

I think we're really arguing about the color of the bikeshed here. IMO we're not stretching RFC 2616 too much by putting "Error" there (since reason codes don't matter on a technical level), and the status codes make enough sense to me (and I'm not even a web developer) that I'm not concerned about readability.

I don't think your constants a la HTTP_INVALID are a good idea (I remember the status codes in a year, but maybe not the constants); die_error could figure out the right reason code using a hash. Either way will be fine with me though.

Look, I really don't care about this and I think it's plain irrelevant -- do you mind fixing the patch with whatever solution you prefer and resend it? I'd rather spend my time on getting caching implemented, but if you insist on me changing this myself, please let me know.

>> Also documented status code conventions in die_error.
> 
> Putting copy of those status code conventions in the commit message,
> and not only in the comment section, would be also good idea.
*shrugs*  I'd rather not duplicate, and it's in the code anyway.
Show 6 quoted lines
>> -		die_error(undef, "No such project");
>> +		die_error(404, "No such project");
> 
> Here is one thing worth considering: if project exists, but is
> excluded due to either $export_ok or $strict_export being set,
> should we use '404 Not Found' or '403 Forbidden'?

No, because (a) we'd have add code to check this (so it should be in a separate patch), and (b) I don't think we even want to return 403, since (as you say) it would reveal the existence of a project on the server. Project names can contain sensitive/confidential information though.

Show 6 quoted lines
>> -		die_error(undef, "At least two characters are required for search parameter");
>> +		die_error(403, "At least two characters are required for search parameter");
> 
> Should gitweb use there '403 Forbidden', or '400 Bad Request'?
> This is failing static validation of CGI parameters, not a matter of
> some permissions...

I used 403 in the sense of "sorry, we don't have shorter search strings activated for performance reasons". The '2' could even become configurable. 400 is fine too, though, I don't care.

> *All* "open ... or die_error" constructs should use '500 Internal
> Server Error', as the only errors that can be detected on open are
> very serious, server related ones:
Right, thanks for pointing this out.
Show 11 quoted lines
>> -	my ($have_blame) = gitweb_check_feature('blame');
>> -	if (!$have_blame) {
>> -		die_error('403 Permission denied', "Permission denied");
>> -	}
>> +	gitweb_check_feature('blame')
>> +	    or die_error(403, "Blame not allowed");
> 
> That is a bit separate change, i.e. better explanation of an error
> (although I'd say rather "Blame view not allowed").
> 
> But what about security reasons?

Separate change: I don't think this has to be a separate patch. ;-) "Blame view not allowed": Fine with me. Security concerns: I don't see any, and anyway you can probably infer from getting 403 on a=blame that it's disabled.

>> -	die_error(undef, "Couldn't find base commit") unless ($hash_base);
>> +	die_error(400, "Couldn't find base commit") unless ($hash_base);
> 
> Wound't it be '404 Not Found', as the explanation suggests?
Yup, right.
Show 5 quoted lines
>> -	close $fd or die_error(undef, "Reading tree failed");
>> +	close $fd or die_error(500, "Reading tree failed");
> 
> Not O.K.  Barring errors in gitweb code this might happen when
> [X Y Z].  All those are clearly 4xx _client_ errors,

I haven't verified that, so until we have better error handling I prefer 500, but I really won't bother objecting to 404. (Same goes for all other occurrences of 500 you quoted, which I've snipped.) FWIW I'm assuming that once gitweb uses the new API, that error handling code will go away anyway.

> One should examine URL for mistakes, not contact server admin here...
I don't think that'll be a practical concern. ;-)
>> -		die_error('404 Not Found', "Not enough information to find object");
>> +		die_error(400, "Not enough information to find object");
> 
> I'm not sure if it should be '400 Bad Request' or '404 Not Found' here.
It's about missing CGI parameters, so 400 should be fine.
Show 6 quoted lines
>>  			@difftree
>> -				or die_error('404 Not Found', "Blob diff not found");
>> +				or die_error(404, "Blob diff not found");
> 
> This, if I am not mistaken, can happen only if path limiters doesn'
> catch anything.
Your sentence doesn't quite parse -- why is 404 wrong?
>> -			die_error('404 Not Found', "Ambiguous blob diff specification");
>> +			die_error(400, "Ambiguous blob diff specification");
> 
> Or perhaps '404 Not Dound' here?
Either way is fine.
Show 5 quoted lines
>> -		die_error('404 Not Found', "Missing one of the blob diff parameters")
>> +		die_error(400, "Missing one of the blob diff parameters")
>>  			unless %diffinfo;
> 
> The "unless %diffinfo" makes it look more like '404 Not Found'...

Looking at the preceding code (the if statement) I believe diffinfo being false doesn't indicate 404 but actually a missing CGI parameter (as the error message states).

Show 5 quoted lines
>>  	if (!defined $ftype) {
>> -		die_error(undef, "Unknown type of object");
>> +		die_error(500, "Unknown type of object");
> 
> Errr... shouldn't be '400 Bad Request' here, per convention?
Nope, we didn't get *anything* back, so something weird happened.  500.
-- Lea
Previous: Jakub NarebskiNext: Lea Wiemann
Message 20 of 25 in “gitweb: return correct HTTP status codes”
  1. gitweb: return correct HTTP status codesLea Wiemann, Jun 15, 2008
  2. Jakub NarebskiJun 15, 2008
  3. Lea WiemannJun 16, 2008
  4. Jakub NarebskiJun 16, 2008
  5. Lea WiemannJun 16, 2008
  6. Jakub NarebskiJun 16, 2008
  7. Lea WiemannJun 17, 2008
  8. Junio C HamanoJun 16, 2008
  9. Lea WiemannJun 17, 2008
  10. Jakub NarebskiJun 17, 2008
  11. Lea WiemannJun 17, 2008
  12. Jakub NarebskiJun 17, 2008
  13. Lea WiemannJun 17, 2008
  14. Jakub NarebskiJun 18, 2008
  15. Lea WiemannJun 18, 2008
  16. Jakub NarebskiJun 18, 2008
  17. Jakub NarebskiJun 16, 2008
  18. gitweb: standarize HTTP status codesLea Wiemann, Jun 18, 2008
  19. Jakub NarebskiJun 19, 2008
  20. Lea WiemannJun 19, 2008
  21. gitweb: standarize HTTP status codesLea Wiemann, Jun 19, 2008
  22. gitweb: standarize HTTP status codesLea Wiemann, Jun 19, 2008
  23. Jakub NarebskiJun 19, 2008
  24. Junio C HamanoJun 20, 2008
  25. Jakub NarebskiJun 19, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.