From 37c76a39ab3ab43c46af458ebfd08a0d3d97adf1 Mon Sep 17 00:00:00 2001
From: Matt Gathu <mattgathu@gmail.com>
Date: Fri, 25 Sep 2020 15:50:37 +0200
Subject: [PATCH] Fix Multipart consuming payload before header checks (#1704)

* Fix Multipart consuming payload before header checks

What
--
Split up logic in the constructor into two functions:

- **from_boundary:** build Multipart from boundary and stream
- **from_error:** build Multipart for MultipartError

Also we make the `boundary`, `from_boundary`, `from_error`  methods public within the crate so that we can use them in the extractor.

The extractor is then able to perform header checks and only consume the
payload if the checks pass.

* Add tests

* Add payload consumption test

Co-authored-by: Rob Ede <robjtede@icloud.com>
---
 actix-multipart/CHANGES.md       |  1 +
 actix-multipart/src/extractor.rs |  5 +-
 actix-multipart/src/server.rs    | 81 +++++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/actix-multipart/CHANGES.md b/actix-multipart/CHANGES.md
index b2505302..446ca5ad 100644
--- a/actix-multipart/CHANGES.md
+++ b/actix-multipart/CHANGES.md
@@ -1,6 +1,7 @@
 # Changes
 
 ## Unreleased - 2020-xx-xx
+* Fix multipart consuming payload before header checks #1513
 
 
 ## 3.0.0 - 2020-09-11
diff --git a/actix-multipart/src/extractor.rs b/actix-multipart/src/extractor.rs
index 4e4caee0..6aaa415c 100644
--- a/actix-multipart/src/extractor.rs
+++ b/actix-multipart/src/extractor.rs
@@ -36,6 +36,9 @@ impl FromRequest for Multipart {
 
     #[inline]
     fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future {
-        ok(Multipart::new(req.headers(), payload.take()))
+        ok(match Multipart::boundary(req.headers()) {
+            Ok(boundary) => Multipart::from_boundary(boundary, payload.take()),
+            Err(err) => Multipart::from_error(err),
+        })
     }
 }
diff --git a/actix-multipart/src/server.rs b/actix-multipart/src/server.rs
index 1507959b..b9ebf97c 100644
--- a/actix-multipart/src/server.rs
+++ b/actix-multipart/src/server.rs
@@ -64,26 +64,13 @@ impl Multipart {
         S: Stream<Item = Result<Bytes, PayloadError>> + Unpin + 'static,
     {
         match Self::boundary(headers) {
-            Ok(boundary) => Multipart {
-                error: None,
-                safety: Safety::new(),
-                inner: Some(Rc::new(RefCell::new(InnerMultipart {
-                    boundary,
-                    payload: PayloadRef::new(PayloadBuffer::new(Box::new(stream))),
-                    state: InnerState::FirstBoundary,
-                    item: InnerMultipartItem::None,
-                }))),
-            },
-            Err(err) => Multipart {
-                error: Some(err),
-                safety: Safety::new(),
-                inner: None,
-            },
+            Ok(boundary) => Multipart::from_boundary(boundary, stream),
+            Err(err) => Multipart::from_error(err),
         }
     }
 
     /// Extract boundary info from headers.
-    fn boundary(headers: &HeaderMap) -> Result<String, MultipartError> {
+    pub(crate) fn boundary(headers: &HeaderMap) -> Result<String, MultipartError> {
         if let Some(content_type) = headers.get(&header::CONTENT_TYPE) {
             if let Ok(content_type) = content_type.to_str() {
                 if let Ok(ct) = content_type.parse::<mime::Mime>() {
@@ -102,6 +89,32 @@ impl Multipart {
             Err(MultipartError::NoContentType)
         }
     }
+
+    /// Create multipart instance for given boundary and stream
+    pub(crate) fn from_boundary<S>(boundary: String, stream: S) -> Multipart
+    where
+        S: Stream<Item = Result<Bytes, PayloadError>> + Unpin + 'static,
+    {
+        Multipart {
+            error: None,
+            safety: Safety::new(),
+            inner: Some(Rc::new(RefCell::new(InnerMultipart {
+                boundary,
+                payload: PayloadRef::new(PayloadBuffer::new(Box::new(stream))),
+                state: InnerState::FirstBoundary,
+                item: InnerMultipartItem::None,
+            }))),
+        }
+    }
+
+    /// Create Multipart instance from MultipartError
+    pub(crate) fn from_error(err: MultipartError) -> Multipart {
+        Multipart {
+            error: Some(err),
+            safety: Safety::new(),
+            inner: None,
+        }
+    }
 }
 
 impl Stream for Multipart {
@@ -815,6 +828,8 @@ mod tests {
     use actix_http::h1::Payload;
     use actix_utils::mpsc;
     use actix_web::http::header::{DispositionParam, DispositionType};
+    use actix_web::test::TestRequest;
+    use actix_web::FromRequest;
     use bytes::Bytes;
     use futures_util::future::lazy;
 
@@ -1151,4 +1166,38 @@ mod tests {
         );
         assert_eq!(payload.buf.len(), 0);
     }
+
+    #[actix_rt::test]
+    async fn test_multipart_from_error() {
+        let err = MultipartError::NoContentType;
+        let mut multipart = Multipart::from_error(err);
+        assert!(multipart.next().await.unwrap().is_err())
+    }
+
+    #[actix_rt::test]
+    async fn test_multipart_from_boundary() {
+        let (_, payload) = create_stream();
+        let (_, headers) = create_simple_request_with_header();
+        let boundary = Multipart::boundary(&headers);
+        assert!(boundary.is_ok());
+        let _ = Multipart::from_boundary(boundary.unwrap(), payload);
+    }
+
+    #[actix_rt::test]
+    async fn test_multipart_payload_consumption() {
+        // with sample payload and HttpRequest with no headers
+        let (_, inner_payload) = Payload::create(false);
+        let mut payload = actix_web::dev::Payload::from(inner_payload);
+        let req = TestRequest::default().to_http_request();
+
+        // multipart should generate an error
+        let mut mp = Multipart::from_request(&req, &mut payload).await.unwrap();
+        assert!(mp.next().await.unwrap().is_err());
+
+        // and should not consume the payload
+        match payload {
+            actix_web::dev::Payload::H1(_) => {} //expected
+            _ => unreachable!(),
+        }
+    }
 }