From 60e7e52276ee60d46336563a91a8ef72be2ad699 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?LIU=20An=20=28=E5=8A=89=E5=AE=89=29?= Date: Fri, 25 Sep 2020 19:50:59 +0800 Subject: [PATCH 1/3] Add TrailingSlash::MergeOnly behavior (#1695) Co-authored-by: Rob Ede --- CHANGES.md | 3 +++ MIGRATION.md | 2 +- src/middleware/normalize.rs | 40 ++++++++++++++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 15d3c81ce..608c237b2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,9 @@ # 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] ## 3.0.2 - 2020-09-15 diff --git a/MIGRATION.md b/MIGRATION.md index 98b22ae4e..5c4650194 100644 --- a/MIGRATION.md +++ b/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`. diff --git a/src/middleware/normalize.rs b/src/middleware/normalize.rs index 8452c9083..e0ecd90dc 100644 --- a/src/middleware/normalize.rs +++ b/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| { From 37c76a39ab3ab43c46af458ebfd08a0d3d97adf1 Mon Sep 17 00:00:00 2001 From: Matt Gathu Date: Fri, 25 Sep 2020 15:50:37 +0200 Subject: [PATCH 2/3] 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 --- 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 b25053025..446ca5ad2 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 4e4caee01..6aaa415c4 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 1507959b8..b9ebf97cc 100644 --- a/actix-multipart/src/server.rs +++ b/actix-multipart/src/server.rs @@ -64,26 +64,13 @@ impl Multipart { S: Stream> + 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 { + pub(crate) fn boundary(headers: &HeaderMap) -> Result { 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::() { @@ -102,6 +89,32 @@ impl Multipart { Err(MultipartError::NoContentType) } } + + /// Create multipart instance for given boundary and stream + pub(crate) fn from_boundary(boundary: String, stream: S) -> Multipart + where + S: Stream> + 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!(), + } + } } From b4e02fe29a3399601a03e9fa429c9af0e85d1649 Mon Sep 17 00:00:00 2001 From: Matt Gathu Date: Fri, 25 Sep 2020 18:42:49 +0200 Subject: [PATCH 3/3] Fix cyclic references in ResourceMap (#1708) --- CHANGES.md | 2 ++ src/rmap.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 608c237b2..3419ce818 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,7 +4,9 @@ ### 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 diff --git a/src/rmap.rs b/src/rmap.rs index 5e79830ec..05c1f3f15 100644 --- a/src/rmap.rs +++ b/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>>, + parent: RefCell>, named: FxHashMap, patterns: Vec<(ResourceDef, Option>)>, } @@ -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) { for (_, nested) in &self.patterns { if let Some(ref nested) = nested { - *nested.parent.borrow_mut() = Some(current.clone()); + *nested.parent.borrow_mut() = Rc::downgrade(¤t); nested.finish(nested.clone()); } } @@ -210,7 +210,7 @@ impl ResourceMap { U: Iterator, I: AsRef, { - 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, I: AsRef, { - 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(" }")); + } }