From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id D04191FF16F for ; Thu, 16 Jan 2025 08:37:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 47C9B199DF; Thu, 16 Jan 2025 08:37:00 +0100 (CET) Message-ID: <146d00c4-8e99-42d0-ba4c-5663cce96c7f@proxmox.com> Date: Thu, 16 Jan 2025 08:36:56 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Thomas Lamprecht , Proxmox VE development discussion References: <20250108084558.390324-1-d.csapak@proxmox.com> <20250108084558.390324-3-d.csapak@proxmox.com> <4d7a8ece-96dd-4742-a0f4-011e54258d4c@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <4d7a8ece-96dd-4742-a0f4-011e54258d4c@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.016 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501' X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 >> --- >> 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