Don't normalize URIs with no valid path (#2246)

CHANGES.md

@@ -10,12 +10,14 @@
 * Update `language-tags` to `0.3`.
 * `ServiceResponse::take_body`. [#2201]
 * `ServiceResponse::map_body` closure receives and returns `B` instead of `ResponseBody<B>` types. [#2201]
+* `middleware::normalize` now will not try to normalize URIs with no valid path [#2246]
 
 ### Removed
 * `HttpResponse::take_body` and old `HttpResponse::into_body` method that casted body type. [#2201]
 
 [#2200]: https://github.com/actix/actix-web/pull/2200
 [#2201]: https://github.com/actix/actix-web/pull/2201
+[#2246]: https://github.com/actix/actix-web/pull/2246
 
 
 ## 4.0.0-beta.6 - 2021-04-17

src/middleware/normalize.rs

@@ -137,53 +137,57 @@ where
 
         let original_path = head.uri.path();
 
-        // Either adds a string to the end (duplicates will be removed anyways) or trims all slashes from the end
+        // An empty path here means that the URI has no valid path. We skip normalization in this
-        let path = match self.trailing_slash_behavior {
+        // case, because adding a path can make the URI invalid
-            TrailingSlash::Always => original_path.to_string() + "/",
+        if !original_path.is_empty() {
-            TrailingSlash::MergeOnly => original_path.to_string(),
+            // Either adds a string to the end (duplicates will be removed anyways) or trims all
-            TrailingSlash::Trim => original_path.trim_end_matches('/').to_string(),
+            // slashes from the end
-        };
+            let path = match self.trailing_slash_behavior {
-
+                TrailingSlash::Always => format!("{}/", original_path),
-        // normalize multiple /'s to one /
+                TrailingSlash::MergeOnly => original_path.to_string(),
-        let path = self.merge_slash.replace_all(&path, "/");
+                TrailingSlash::Trim => original_path.trim_end_matches('/').to_string(),
-
-        // Ensure root paths are still resolvable. If resulting path is blank after previous step
-        // it means the path was one or more slashes. Reduce to single slash.
-        let path = if path.is_empty() { "/" } else { path.as_ref() };
-
-        // 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 query = parts.path_and_query.as_ref().and_then(|pq| pq.query());
-
-            let path = if let Some(q) = query {
-                Bytes::from(format!("{}?{}", path, q))
-            } else {
-                Bytes::copy_from_slice(path.as_bytes())
             };
-            parts.path_and_query = Some(PathAndQuery::from_maybe_shared(path).unwrap());
 
-            let uri = Uri::from_parts(parts).unwrap();
+            // normalize multiple /'s to one /
-            req.match_info_mut().get_mut().update(&uri);
+            let path = self.merge_slash.replace_all(&path, "/");
-            req.head_mut().uri = uri;
+
+            // Ensure root paths are still resolvable. If resulting path is blank after previous
+            // step it means the path was one or more slashes. Reduce to single slash.
+            let path = if path.is_empty() { "/" } else { path.as_ref() };
+
+            // 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 query = parts.path_and_query.as_ref().and_then(|pq| pq.query());
+
+                let path = match query {
+                    Some(q) => Bytes::from(format!("{}?{}", path, q)),
+                    None => Bytes::copy_from_slice(path.as_bytes()),
+                };
+                parts.path_and_query = Some(PathAndQuery::from_maybe_shared(path).unwrap());
+
+                let uri = Uri::from_parts(parts).unwrap();
+                req.match_info_mut().get_mut().update(&uri);
+                req.head_mut().uri = uri;
+            }
         }
-
         self.service.call(req)
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use actix_http::StatusCode;
     use actix_service::IntoService;
 
     use super::*;
@@ -387,6 +391,22 @@ mod tests {
         }
     }
 
+    #[actix_rt::test]
+    async fn no_path() {
+        let app = init_service(
+            App::new()
+                .wrap(NormalizePath::default())
+                .service(web::resource("/").to(HttpResponse::Ok)),
+        )
+        .await;
+
+        // This URI will be interpreted as an authority form, i.e. there is no path nor scheme
+        // (https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3)
+        let req = TestRequest::with_uri("eh").to_request();
+        let res = call_service(&app, req).await;
+        assert_eq!(res.status(), StatusCode::NOT_FOUND);
+    }
+
     #[actix_rt::test]
     async fn test_in_place_normalization() {
         let srv = |req: ServiceRequest| {