diff --git a/CHANGES.md b/CHANGES.md index 15d3c81ce..3419ce818 100644 --- a/CHANGES.md +++ b/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 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/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!(), + } + } } 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| { 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(" }")); + } }