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.csa...@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;
}
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
