From d397900e1a2705548ef82f06cd2460fa9e977281 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Mon, 1 Nov 2021 01:59:16 +0300
Subject: [PATCH] fix url_for parser

---
 src/error/mod.rs |  4 ++--
 src/request.rs   | 24 +++++++++++++++----
 src/rmap.rs      | 61 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/src/error/mod.rs b/src/error/mod.rs
index 3ccd5bba6..ca84a427b 100644
--- a/src/error/mod.rs
+++ b/src/error/mod.rs
@@ -33,8 +33,8 @@ pub enum UrlGenerationError {
     #[display(fmt = "Resource not found")]
     ResourceNotFound,
 
-    /// Not all path pattern covered
-    #[display(fmt = "Not all path pattern covered")]
+    /// Not all url paramters covered
+    #[display(fmt = "Not all url parameters covered")]
     NotEnoughElements,
 
     /// URL parse error
diff --git a/src/request.rs b/src/request.rs
index 0027f9b4b..2a0178ccd 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -114,12 +114,20 @@ impl HttpRequest {
         self.uri().query().unwrap_or_default()
     }
 
-    /// Get a reference to the Path parameters.
+    /// Get a reference to url parameters container
     ///
-    /// Params is a container for url parameters.
-    /// A variable segment is specified in the form `{identifier}`,
+    /// A url parameter is specified in the form `{identifier}`,
     /// where the identifier can be used later in a request handler to
-    /// access the matched value for that segment.
+    /// access the matched value for that parameter.
+    ///
+    ///
+    /// # Percent Enconding and Url Paramters
+    ///
+    /// Because each url paramter is able to capture multiple path segments,
+    /// both `["%2F", "%25"]` found in the request uri are not decoded into `["/", "%"]`
+    /// in order to preserve path segment boundaries.
+    /// If a url paramter is expected to contain these characters, then it is on the user to decode them.
+    ///
     #[inline]
     pub fn match_info(&self) -> &Path<Url> {
         &self.inner.path
@@ -163,6 +171,14 @@ impl HttpRequest {
 
     /// Generate url for named resource
     ///
+    /// This substitutes in sequence all url parameters that appear in the resource itself
+    /// and in parent [scopes](crate::web::scope), if any.
+    ///
+    /// It is worth noting that the characters `['/', '%']` are not escaped and therefore a single
+    /// url parameter may expand into multiple path segments and `elements`
+    /// can be percent-encoded beforehand without worrying about double encoding.
+    /// Any other character that is not valid in url ath context is escaped using percent-encoding.
+    ///
     /// ```
     /// # use actix_web::{web, App, HttpRequest, HttpResponse};
     /// #
diff --git a/src/rmap.rs b/src/rmap.rs
index 8466eda28..a7f1884bb 100644
--- a/src/rmap.rs
+++ b/src/rmap.rs
@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::cell::RefCell;
 use std::rc::{Rc, Weak};
 
@@ -102,17 +103,23 @@ impl ResourceMap {
             })
             .ok_or(UrlGenerationError::NotEnoughElements)?;
 
-        if path.starts_with('/') {
+        let (base, path): (Cow<'_, _>, _) = if path.starts_with('/') {
             let conn = req.connection_info();
-            Ok(Url::parse(&format!(
-                "{}://{}{}",
-                conn.scheme(),
-                conn.host(),
-                path
-            ))?)
+            let base = format!("{}://{}", conn.scheme(), conn.host(),).into();
+            (base, path.as_str())
         } else {
-            Ok(Url::parse(&path)?)
-        }
+            // external resource
+            let third_slash_index = path
+                .char_indices()
+                .filter_map(|(i, c)| (c == '/').then(|| i))
+                .nth(2)
+                .unwrap_or(path.len());
+            (path[..third_slash_index].into(), &path[third_slash_index..])
+        };
+
+        let mut url = Url::parse(&base)?;
+        url.set_path(path);
+        Ok(url)
     }
 
     pub fn has_resource(&self, path: &str) -> bool {
@@ -406,6 +413,42 @@ mod tests {
         assert!(rmap.url_for(&req, "missing", &["u123"]).is_err());
     }
 
+    #[test]
+    fn url_for_parser() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut rdef_1 = ResourceDef::new("/{var}");
+        rdef_1.set_name("internal");
+
+        let mut rdef_2 = ResourceDef::new("http://host.dom/{var}");
+        rdef_2.set_name("external.1");
+
+        let mut rdef_3 = ResourceDef::new("{var}");
+        rdef_3.set_name("external.2");
+
+        root.add(&mut rdef_1, None);
+        root.add(&mut rdef_2, None);
+        root.add(&mut rdef_3, None);
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        let mut req = crate::test::TestRequest::default();
+        req.set_server_hostname("localhost:8888");
+        let req = req.to_http_request();
+
+        const INPUT: &[&str] = &["a/../quick brown%20fox/%nan?query#frag"];
+        const OUTPUT: &str = "/quick%20brown%20fox/%nan%3Fquery%23frag";
+
+        let url = rmap.url_for(&req, "internal", INPUT).unwrap();
+        assert_eq!(url.path(), OUTPUT);
+
+        let url = rmap.url_for(&req, "external.1", INPUT).unwrap();
+        assert_eq!(url.path(), OUTPUT);
+
+        assert!(rmap.url_for(&req, "external.2", INPUT).is_err());
+        assert!(rmap.url_for(&req, "external.2", &[""]).is_err());
+    }
+
     #[test]
     fn external_resource_with_no_name() {
         let mut root = ResourceMap::new(ResourceDef::prefix(""));