[pve-devel] [PATCH http-server 0/1] fix #4344: ignore unused multipart headers

[pve-devel] [PATCH http-server 0/1] fix #4344: ignore unused multipart headers

[John Hollowell]

This fixes an issue where an upload request without a Content-Type in
the file's multipart part would prevent the upload and throw
missleading errors. This patch removes the requirement and ignores
all multipart headers once the needed information has been extracted.

I have tested these changes against a 7.2-11 server and both a previously
broken upload method (without the Content-Type) and using the webUI in
Chrome (which includes a Content-Type) correctly uploads the file.

John Hollowell (1):
  fix #4344: http-server: ignore unused multipart headers

 src/PVE/APIServer/AnyEvent.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--
2.30.2

[pve-devel] [PATCH http-server 1/1] fix #4344: http-server: ignore unused multipart headers

[John Hollowell]

Signed-off-by: John Hollowell <jhollowe@johnhollowell.com>
---
 src/PVE/APIServer/AnyEvent.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index f397a8c..d958642 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1215,15 +1215,15 @@ sub file_upload_multipart {
 	    $extract_form_disposition->('checksum');

 	    if ($hdl->{rbuf} =~
-		s/^${delim_re}
-		Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"${newline_re}
-		Content-Type:\ \S*\s+
-		//sxx
+		s/^${delim_re}Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"//sxx
 	    ) {
 		assert_form_disposition($1);
 		die "wrong field name '$2' for file upload, expected 'filename'" if $2 ne "filename";
 		$rstate->{phase} = 2;
 		$rstate->{params}->{filename} = trim($3);
+
+		# remove any remaining multipart "headers" like Content-Type
+		$hdl->{rbuf} =~ s/^.*?${newline_re}{2}//s
 	    }
 	}

--
2.30.2

Re: [pve-devel] [PATCH http-server 1/1] fix #4344: http-server: ignore unused multipart headers

[Matthias Heiserer]

Thanks for the patch! I must have overlooked that in my original patch. 
Didn't think to test with proxmoxer :)

In case you haven't do so already, please submit the contributor license 
agreement. We require that in order to use your patch.
https://www.proxmox.com/en/proxmox-ve/get-involved

On 14.11.2022 00:48, John Hollowell wrote:
> Signed-off-by: John Hollowell <jhollowe@johnhollowell.com>
> ---
>   src/PVE/APIServer/AnyEvent.pm | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> index f397a8c..d958642 100644
> --- a/src/PVE/APIServer/AnyEvent.pm
> +++ b/src/PVE/APIServer/AnyEvent.pm
> @@ -1215,15 +1215,15 @@ sub file_upload_multipart {
>   	    $extract_form_disposition->('checksum');
> 
>   	    if ($hdl->{rbuf} =~
> -		s/^${delim_re}
> -		Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"${newline_re}
> -		Content-Type:\ \S*\s+
> -		//sxx
> +		s/^${delim_re}Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"//sxx
We should drop the xx and escaping of spaces, it's not needed for the 
single line.
>   	    ) {
>   		assert_form_disposition($1);
>   		die "wrong field name '$2' for file upload, expected 'filename'" if $2 ne "filename";
>   		$rstate->{phase} = 2;
>   		$rstate->{params}->{filename} = trim($3);
> +
> +		# remove any remaining multipart "headers" like Content-Type
> +		$hdl->{rbuf} =~ s/^.*?${newline_re}{2}//s
I'm thinking of whether it would be better to include this line in the 
other one, or not. Probably more clearer the way it is now.
>   	    }
>   	}
> 
> --
> 2.30.2
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> Reviewed-by: Matthias Heiserer <m.heiserer@proxmox.com>
Tested-by: Matthias Heiserer <m.heiserer@proxmox.com>

Re: [pve-devel] [PATCH http-server 1/1] fix #4344: http-server: ignore unused multipart headers

[Matthias Heiserer]

(Citing the accidental off-list response)
>>
>> We should drop the xx and escaping of spaces, it's not needed for the
>> single line.
>>
> 
>  I think this would still be needed to support weird filenames? idk, I'm
> not familiar with all of perl's regex flags.

AFAIUI the /x and /xx is in case you want to use whitespace/newlines as 
separators in the regex, rather than actually matching it:
https://perldoc.perl.org/perlre#/x-and-/xx

Instead of e.g. `Disposition:\ (.*?);\ name=`
we can then use `Disposition: (.*?); name=`

>>  I'm thinking of whether it would be better to include this line in the
>> 
>> other one, or not. Probably more clearer the way it is now.
> 
> 
> I think, while this is more lines, it is a better signpost that this is the
> end of the multipart stuff. And if any future
> data needs to be taken from the headers, it will not need to change.

Agree, let's keep it that way.