[pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'

[Dominik Csapak]

On 1/15/25 17:19, Thomas Lamprecht wrote:
> Am 08.01.25 um 09:45 schrieb Dominik Csapak:
>> The http status code 501 is meant to be 'Not Implemented'[0] but that
>> clearly does not fit here as the default error when we encounter a
>> problem during handling an api request or upload.
> 
> Not sure about the clearly; 501 is not a 404 like error but one where
> some functionality is not implemented.
> 
> So if the error stems from an side effect of the actual code handling
> the request switching over to 500 seems OK, but if it's a error from
> some header flag not being supported then 501 seems alright to me,
> I looked into a few hunks inline with more comments.
> 
>>
>> So instead use '500' (HTTP_INTERNAL_SERVER_ERROR) which we already use
>> in other places where it fits.
>>
>> 0: https://datatracker.ietf.org/doc/html/rfc9110#name-501-not-implemented
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   src/PVE/APIServer/AnyEvent.pm | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
>> index bd76488..3b96d2a 100644
>> --- a/src/PVE/APIServer/AnyEvent.pm
>> +++ b/src/PVE/APIServer/AnyEvent.pm
>> @@ -504,7 +504,7 @@ sub send_file_start {
>>   	    $self->response($reqstate, $resp, $mtime, $nocomp);
>>   	};
>>   	if (my $err = $@) {
>> -	    $self->error($reqstate, 501, $err);
>> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>>   	}
>>       };
>>   
>> @@ -1020,7 +1020,7 @@ sub handle_api2_request {
>>   	$self->response($reqstate, $resp, undef, $nocomp, $delay);
>>       };
>>       if (my $err = $@) {
>> -	$self->error($reqstate, 501, $err);
>> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>>       }
>>   }
>>   
>> @@ -1214,7 +1214,7 @@ sub handle_request {
>>   	die "no such file '$path'\n";
>>       };
>>       if (my $err = $@) {
>> -	$self->error($reqstate, 501, $err);
>> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>>       }
>>   }
>>   
>> @@ -1304,7 +1304,7 @@ sub file_upload_multipart {
>>       };
>>       if (my $err = $@) {
>>   	syslog('err', $err);
>> -	$self->error($reqstate, 501, $err);
>> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>>       }
>>   }
>>   
>> @@ -1402,10 +1402,10 @@ sub process_header {
>>       my $te  = $request->header('Transfer-Encoding');
>>       if ($te && lc($te) eq 'chunked') {
>>   	# Handle chunked transfer encoding
>> -	$self->error($reqstate, 501, "chunked transfer encoding not supported");
>> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "chunked transfer encoding not supported");
>>   	return 0;
>>       } elsif ($te) {
>> -	$self->error($reqstate, 501, "Unknown transfer encoding '$te'");
>> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "Unknown transfer encoding '$te'");
> 
> both above seem to fulfill the "server does not support the functionality
> required to fulfill the request" part of the 501 Not implemented error
> though?
> 
> While it follows "This is the appropriate response when the server does not
> recognize the request method and is not capable of supporting it for any
> resource", this rather reads as example to me, but not deep into the HTTP
> lore as of now, just not 100$ sure this counts as unexpected condition, as
> I can trigger it quite expectedly.

forgot that i talked with fabian off-list about this too, and
we said that the first 4 instances (where we simply pass through the error)
is fine, but for the last 4 (like you mentioned here) we should keep the 501
since we actually have not implemented some part of the request

I misunderstood the 501 error at first, thinking it's about the path of the request only,
but it's actually for any part of the request, so here the 'transfer-encoding' above
as well as the 'unexpected content' and 'data too large' below would qualify for a 501 error
IMO (though I'm fine with either of those be a 500 too)

So if it's fine with you, I'd send a new version with just the first 4 occurrences replaced.

> 
>>   	return 0;
>>       }
>>   
>> @@ -1574,7 +1574,7 @@ sub authenticate_and_handle_request {
>>       if ($len) {
>>   
>>   	if (!($method eq 'PUT' || $method eq 'POST')) {
>> -	    $self->error($reqstate, 501, "Unexpected content for method '$method'");
>> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "Unexpected content for method '$method'");
> 
> not 100% sure here either, one could support a body for GET, but tbh. I'd
> be fine with 500 here, it's even less of a a clear cut.
> 
>>   	    return;
>>   	}
>>   
>> @@ -1624,7 +1624,7 @@ sub authenticate_and_handle_request {
>>   	}
>>   
>>   	if ($len > $limit_max_post) {
>> -	    $self->error($reqstate, 501, "for data too large");
>> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "for data too large");
> 
> 501 could be OK here, we explicitly do not implement handling bigger
> data.
> 
>>   	    return;
>>   	}
>>   
>