Merge pull request #95 from actix/master

-

CHANGES.md

@@ -1,7 +1,12 @@
 # Changes
 
 ## Unreleased - 2020-xx-xx
+### Changed
+* Add `TrailingSlash::MergeOnly` behaviour to `NormalizePath`, which allow `NormalizePath`
+  to keep the trailing slash's existance as it is. [#1695]
+* Fix `ResourceMap` recursive references when printing/debugging. [#1708]
 
+[#1708]: https://github.com/actix/actix-web/pull/1708
 
 ## 3.0.2 - 2020-09-15
 ### Fixed

MIGRATION.md

@@ -48,7 +48,7 @@
 
 * `middleware::NormalizePath` can now also be configured to trim trailing slashes instead of always keeping one.
   It will need `middleware::normalize::TrailingSlash` when being constructed with `NormalizePath::new(...)`,
-  or for an easier migration you can replace `wrap(middleware::NormalizePath)` with `wrap(middleware::NormalizePath::default())`.
+  or for an easier migration you can replace `wrap(middleware::NormalizePath)` with `wrap(middleware::NormalizePath::new(TrailingSlash::MergeOnly))`.
 
 * `HttpServer::maxconn` is renamed to the more expressive `HttpServer::max_connections`.
 

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

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),
+        })
     }
 }

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 {
+            Ok(boundary) => Multipart::from_boundary(boundary, stream),
-                error: None,
+            Err(err) => Multipart::from_error(err),
-                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,
-            },
         }
     }
 
     /// 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!(),
+        }
+    }
 }

src/middleware/normalize.rs

@@ -17,6 +17,10 @@ pub enum TrailingSlash {
     /// Always add a trailing slash to the end of the path.
     /// This will require all routes to end in a trailing slash for them to be accessible.
     Always,
+    /// Only merge any present multiple trailing slashes.
+    ///
+    /// Note: This option provides the best compatibility with the v2 version of this middlware.
+    MergeOnly,
     /// Trim trailing slashes from the end of the path.
     Trim,
 }
@@ -33,7 +37,8 @@ impl Default for TrailingSlash {
 /// Performs following:
 ///
 /// - Merges multiple slashes into one.
-/// - Appends a trailing slash if one is not present, or removes one if present, depending on the supplied `TrailingSlash`.
+/// - Appends a trailing slash if one is not present, removes one if present, or keeps trailing
+///   slashes as-is, depending on the supplied `TrailingSlash` variant.
 ///
 /// ```rust
 /// use actix_web::{web, http, middleware, App, HttpResponse};
@@ -108,6 +113,7 @@ where
         // Either adds a string to the end (duplicates will be removed anyways) or trims all slashes from the end
         let path = match self.trailing_slash_behavior {
             TrailingSlash::Always => original_path.to_string() + "/",
+            TrailingSlash::MergeOnly => original_path.to_string(),
             TrailingSlash::Trim => original_path.trim_end_matches('/').to_string(),
         };
 
@@ -237,6 +243,38 @@ mod tests {
         assert!(res4.status().is_success());
     }
 
+    #[actix_rt::test]
+    async fn keep_trailing_slash_unchange() {
+        let mut app = init_service(
+            App::new()
+                .wrap(NormalizePath(TrailingSlash::MergeOnly))
+                .service(web::resource("/").to(HttpResponse::Ok))
+                .service(web::resource("/v1/something").to(HttpResponse::Ok))
+                .service(web::resource("/v1/").to(HttpResponse::Ok)),
+        )
+        .await;
+
+        let tests = vec![
+            ("/", true), // root paths should still work
+            ("/?query=test", true),
+            ("///", true),
+            ("/v1/something////", false),
+            ("/v1/something/", false),
+            ("//v1//something", true),
+            ("/v1/", true),
+            ("/v1", false),
+            ("/v1////", true),
+            ("//v1//", true),
+            ("///v1", false),
+        ];
+
+        for (path, success) in tests {
+            let req = TestRequest::with_uri(path).to_request();
+            let res = call_service(&mut app, req).await;
+            assert_eq!(res.status().is_success(), success);
+        }
+    }
+
     #[actix_rt::test]
     async fn test_in_place_normalization() {
         let srv = |req: ServiceRequest| {

src/rmap.rs

@@ -1,5 +1,5 @@
 use std::cell::RefCell;
-use std::rc::Rc;
+use std::rc::{Rc, Weak};
 
 use actix_router::ResourceDef;
 use fxhash::FxHashMap;
@@ -11,7 +11,7 @@ use crate::request::HttpRequest;
 #[derive(Clone, Debug)]
 pub struct ResourceMap {
     root: ResourceDef,
-    parent: RefCell<Option<Rc<ResourceMap>>>,
+    parent: RefCell<Weak<ResourceMap>>,
     named: FxHashMap<String, ResourceDef>,
     patterns: Vec<(ResourceDef, Option<Rc<ResourceMap>>)>,
 }
@@ -20,7 +20,7 @@ impl ResourceMap {
     pub fn new(root: ResourceDef) -> Self {
         ResourceMap {
             root,
-            parent: RefCell::new(None),
+            parent: RefCell::new(Weak::new()),
             named: FxHashMap::default(),
             patterns: Vec::new(),
         }
@@ -38,7 +38,7 @@ impl ResourceMap {
     pub(crate) fn finish(&self, current: Rc<ResourceMap>) {
         for (_, nested) in &self.patterns {
             if let Some(ref nested) = nested {
-                *nested.parent.borrow_mut() = Some(current.clone());
+                *nested.parent.borrow_mut() = Rc::downgrade(&current);
                 nested.finish(nested.clone());
             }
         }
@@ -210,7 +210,7 @@ impl ResourceMap {
         U: Iterator<Item = I>,
         I: AsRef<str>,
     {
-        if let Some(ref parent) = *self.parent.borrow() {
+        if let Some(ref parent) = self.parent.borrow().upgrade() {
             parent.fill_root(path, elements)?;
         }
         if self.root.resource_path(path, elements) {
@@ -230,7 +230,7 @@ impl ResourceMap {
         U: Iterator<Item = I>,
         I: AsRef<str>,
     {
-        if let Some(ref parent) = *self.parent.borrow() {
+        if let Some(ref parent) = self.parent.borrow().upgrade() {
             if let Some(pattern) = parent.named.get(name) {
                 self.fill_root(path, elements)?;
                 if pattern.resource_path(path, elements) {
@@ -367,4 +367,42 @@ mod tests {
         assert_eq!(root.match_name("/user/22/"), None);
         assert_eq!(root.match_name("/user/22/post/55"), Some("user_post"));
     }
+
+    #[test]
+    fn bug_fix_issue_1582_debug_print_exits() {
+        // ref: https://github.com/actix/actix-web/issues/1582
+        let mut root = ResourceMap::new(ResourceDef::root_prefix(""));
+
+        let mut user_map = ResourceMap::new(ResourceDef::root_prefix(""));
+        user_map.add(&mut ResourceDef::new("/"), None);
+        user_map.add(&mut ResourceDef::new("/profile"), None);
+        user_map.add(&mut ResourceDef::new("/article/{id}"), None);
+        user_map.add(&mut ResourceDef::new("/post/{post_id}"), None);
+        user_map.add(
+            &mut ResourceDef::new("/post/{post_id}/comment/{comment_id}"),
+            None,
+        );
+
+        root.add(
+            &mut ResourceDef::root_prefix("/user/{id}"),
+            Some(Rc::new(user_map)),
+        );
+
+        let root = Rc::new(root);
+        root.finish(Rc::clone(&root));
+
+        // check root has no parent
+        assert!(root.parent.borrow().upgrade().is_none());
+        // check child has parent reference
+        assert!(root.patterns[0].1.is_some());
+        // check child's parent root id matches root's root id
+        assert_eq!(
+            root.patterns[0].1.as_ref().unwrap().root.id(),
+            root.root.id()
+        );
+
+        let output = format!("{:?}", root);
+        assert!(output.starts_with("ResourceMap {"));
+        assert!(output.ends_with(" }"));
+    }
 }