Fix NormalizePath trailing slash behavior (#1548)

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

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()))
         };