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

[Thomas Lamprecht]

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.

>  	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;
>  	}
>