diff --git a/CHANGES.md b/CHANGES.md index 3b4b8dc0b..3c21c301d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,6 +6,10 @@ * Fix actix_http::h1::dispatcher so it returns when HW_BUFFER_SIZE is reached. Should reduce peak memory consumption during large uploads. [#1550] +### Fixed + +* `NormalizePath` improved consistency when path needs slashes added _and_ removed. + ## [3.0.0-alpha.3] - 2020-05-21 ### Added diff --git a/src/middleware/normalize.rs b/src/middleware/normalize.rs index d23f03445..3d5d30c80 100644 --- a/src/middleware/normalize.rs +++ b/src/middleware/normalize.rs @@ -16,6 +16,7 @@ use crate::Error; /// Performs following: /// /// - Merges multiple slashes into one. +/// - Appends a trailing slash if one is not present. /// /// ```rust /// use actix_web::{web, http, middleware, App, HttpResponse}; @@ -75,14 +76,26 @@ where fn call(&mut self, mut req: ServiceRequest) -> Self::Future { let head = req.head_mut(); + let original_path = head.uri.path(); + // always add trailing slash, might be an extra one - let path = head.uri.path().to_string() + "/"; - let original_len = path.len(); + let path = original_path.to_string() + "/"; // normalize multiple /'s to one / let path = self.merge_slash.replace_all(&path, "/"); - if original_len != path.len() { + // Check whether the path has been changed + // + // This check was previously implemented as string length comparison + // + // That approach fails when a trailing slash is added, + // and a duplicate slash is removed, + // since the length of the strings remains the same + // + // For example, the path "/v1//s" will be normalized to "/v1/s/" + // Both of the paths have the same length, + // so the change can not be deduced from the length comparison + if path != original_path { let mut parts = head.uri.clone().into_parts(); let pq = parts.path_and_query.as_ref().unwrap(); @@ -131,6 +144,10 @@ mod tests { let req3 = TestRequest::with_uri("//v1//////something").to_request(); let res3 = call_service(&mut app, req3).await; assert!(res3.status().is_success()); + + let req4 = TestRequest::with_uri("/v1//something").to_request(); + let res4 = call_service(&mut app, req4).await; + assert!(res4.status().is_success()); } #[actix_rt::test] @@ -156,6 +173,10 @@ mod tests { let req3 = TestRequest::with_uri("//v1///something").to_srv_request(); let res3 = normalize.call(req3).await.unwrap(); assert!(res3.status().is_success()); + + let req4 = TestRequest::with_uri("/v1//something").to_srv_request(); + let res4 = normalize.call(req4).await.unwrap(); + assert!(res4.status().is_success()); } #[actix_rt::test] @@ -178,11 +199,11 @@ mod tests { } #[actix_rt::test] - async fn should_normalize_nothing_notrail() { + async fn should_normalize_notrail() { const URI: &str = "/v1/something"; let srv = |req: ServiceRequest| { - assert_eq!(URI, req.path()); + assert_eq!(URI.to_string() + "/", req.path()); ok(req.into_response(HttpResponse::Ok().finish())) };