all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines
@ 2022-12-12 15:07 Matthias Heiserer
  2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 2/3] fix multipart upload: ignore additional headers Matthias Heiserer
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Matthias Heiserer @ 2022-12-12 15:07 UTC (permalink / raw)
  To: pve-devel

Currently, if a file starts with a newline, it gets removed
and the uploda succeeds (provided no hash is given).

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index f397a8c..545c122 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1217,7 +1217,7 @@ sub file_upload_multipart {
 	    if ($hdl->{rbuf} =~
 		s/^${delim_re}
 		Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"${newline_re}
-		Content-Type:\ \S*\s+
+		Content-Type:\ \S*${newline_re}{2}
 		//sxx
 	    ) {
 		assert_form_disposition($1);
-- 
2.30.2





^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pve-devel] [PATCH v3 http-server 2/3] fix multipart upload: ignore additional headers
  2022-12-12 15:07 [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Matthias Heiserer
@ 2022-12-12 15:07 ` Matthias Heiserer
  2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 3/3] multipart upload: don't require trailing newline Matthias Heiserer
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Matthias Heiserer @ 2022-12-12 15:07 UTC (permalink / raw)
  To: pve-devel

Reported in the forum: https://forum.proxmox.com/threads/image-upload-fails-after-upgrading-from-7-1-to-7-3.119051/#post-516517

When additional headers existed in the request body, the upload failed.
With this patch, all additional headers get ignored.

Example: The following upload would fail because no
headers were expected after Content-Disposition.

```
--EPIHyQJFC5ftgoXHMe8-Jc6E7FqA4oMb0QBfOTz
Content-Disposition: form-data; name="content"
Content-Type: text/plain; charset=ISO-8859-1

iso
```
would fail. These headers now also get ignored, as we don't use them.

Also, upload now works when the Content-Disposition header isn't the first, i.e.
```
--XVH95dt1-A3J8mWiLCmHCW4roSC7-gBntjATBy--
Content-Type: text/plain; charset=ISO-8859-1
Content-Disposition: form-data; name="content"
```

Fixed upload was tested using
* Curl
* GUI
* Apache HttpClient 5

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v2:
None

Changes from v1:
* wait for delimiter before handling data, as reported in forum
* squash ignore order of headers patch

 src/PVE/APIServer/AnyEvent.pm | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 545c122..2f8718f 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1201,11 +1201,19 @@ sub file_upload_multipart {
 	    $rstate->{phase} = 1;
 	}
 
+	my $remove_until_data = sub {
+	    my ($hdl) = @_;
+	    # remove any remaining multipart "headers" like Content-Type
+	    $hdl->{rbuf} =~ s/^.*?${newline_re}{2}//s;
+	};
+
 	my $extract_form_disposition = sub {
 	    my ($name) = @_;
-	    if ($hdl->{rbuf} =~ s/^${delim_re}Content-Disposition: (.*?); name="$name"(.*?)($delim_re)/$3/s) {
+	    if ($hdl->{rbuf} =~ s/^${delim_re}.*?Content-Disposition: (.*?); name="$name"(.*?${delim_re})/$2/s) {
 		assert_form_disposition($1);
-		$rstate->{params}->{$name} = trim($2);
+		$remove_until_data->($hdl);
+		$hdl->{rbuf} =~ s/^(.*?)(${delim_re})/$2/s;
+		$rstate->{params}->{$name} = trim($1);
 	    }
 	};
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pve-devel] [PATCH v3 http-server 3/3] multipart upload: don't require trailing newline
  2022-12-12 15:07 [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Matthias Heiserer
  2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 2/3] fix multipart upload: ignore additional headers Matthias Heiserer
@ 2022-12-12 15:07 ` Matthias Heiserer
  2022-12-12 16:05 ` [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Daniel Tschlatscher
  2022-12-13 12:25 ` [pve-devel] applied-series: " Thomas Lamprecht
  3 siblings, 0 replies; 5+ messages in thread
From: Matthias Heiserer @ 2022-12-12 15:07 UTC (permalink / raw)
  To: pve-devel

Allow upload without trailing newline.
This is not compliant with RFC 1521.

RFC 1521 mandates that the close-delimiter ends
in a newline:
'close-delimiter := "--" boundary "--" CRLF'
However, some software (e.g. postman) sends
their request without a trailing newline, which resulted
in failing uploads.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v2:
Reword commit message as I misunderstood the RFC

 src/PVE/APIServer/AnyEvent.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 2f8718f..3cd77fa 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1193,7 +1193,7 @@ sub file_upload_multipart {
 
 	my $newline_re = qr/\015?\012/;
 	my $delim_re = qr/--\Q$boundary\E${newline_re}/;
-	my $close_delim_re = qr/--\Q$boundary\E--${newline_re}/;
+	my $close_delim_re = qr/--\Q$boundary\E--/;
 
 	# Phase 0 - preserve boundary, but remove everything before
 	if ($rstate->{phase} == 0 && $hdl->{rbuf} =~ s/^.*?($delim_re)/$1/s) {
-- 
2.30.2





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines
  2022-12-12 15:07 [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Matthias Heiserer
  2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 2/3] fix multipart upload: ignore additional headers Matthias Heiserer
  2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 3/3] multipart upload: don't require trailing newline Matthias Heiserer
@ 2022-12-12 16:05 ` Daniel Tschlatscher
  2022-12-13 12:25 ` [pve-devel] applied-series: " Thomas Lamprecht
  3 siblings, 0 replies; 5+ messages in thread
From: Daniel Tschlatscher @ 2022-12-12 16:05 UTC (permalink / raw)
  To: pve-devel

Testing this series in the Browser, with curl and postman, I couldn't
find any issues anymore, more details below. Code looks good to me as well.


Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
Reviewed-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>


Browser/GUI:

✅ Uploading files with 0B, 1B, 1kB, 17kB, 1MB, 1GB, 10GB
✅ Uploading file with a SHA256 checksum

In curl and Postman:

✅ Changing the extension in the first boundary (error)
✅ Adding additional headers leading or trailing (ignored)
✅ Specifying no headers in first boundary (error)
✅ Inconsistent boundary parameter in the Content-Type header (error)
✅ Inconsistent boundary in the body (error)
✅ Whitespaces at the beginning of the file are not discarded
✅ Arbitrary input after the last boundary
✅ Nothing after last boundary
✅ Mixed \n and \r\n in body


On 12/12/22 16:07, Matthias Heiserer wrote:
> Currently, if a file starts with a newline, it gets removed
> and the uploda succeeds (provided no hash is given).
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> index f397a8c..545c122 100644
> --- a/src/PVE/APIServer/AnyEvent.pm
> +++ b/src/PVE/APIServer/AnyEvent.pm
> @@ -1217,7 +1217,7 @@ sub file_upload_multipart {
>  	    if ($hdl->{rbuf} =~
>  		s/^${delim_re}
>  		Content-Disposition:\ (.*?);\ name="(.*?)";\ filename="([^"]+)"${newline_re}
> -		Content-Type:\ \S*\s+
> +		Content-Type:\ \S*${newline_re}{2}
>  		//sxx
>  	    ) {
>  		assert_form_disposition($1);




^ permalink raw reply	[flat|nested] 5+ messages in thread

* [pve-devel] applied-series: [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines
  2022-12-12 15:07 [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Matthias Heiserer
                   ` (2 preceding siblings ...)
  2022-12-12 16:05 ` [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Daniel Tschlatscher
@ 2022-12-13 12:25 ` Thomas Lamprecht
  3 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2022-12-13 12:25 UTC (permalink / raw)
  To: Proxmox VE development discussion, Matthias Heiserer

Am 12/12/2022 um 16:07 schrieb Matthias Heiserer:
> Currently, if a file starts with a newline, it gets removed
> and the uploda succeeds (provided no hash is given).
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied series with Daniel's R-b and T-b, thanks to both of you!




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-12-13 12:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-12 15:07 [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Matthias Heiserer
2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 2/3] fix multipart upload: ignore additional headers Matthias Heiserer
2022-12-12 15:07 ` [pve-devel] [PATCH v3 http-server 3/3] multipart upload: don't require trailing newline Matthias Heiserer
2022-12-12 16:05 ` [pve-devel] [PATCH v3 http-server 1/3] multipart upload: fix upload of files starting with newlines Daniel Tschlatscher
2022-12-13 12:25 ` [pve-devel] applied-series: " Thomas Lamprecht

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal