disallow query or fragements in url_for constructions (#2430)

Co-authored-by: Rob Ede <robjtede@icloud.com>

CHANGES.md

@@ -12,12 +12,14 @@
 * Rename `Accept::{mime_precedence => ranked}`. [#2480]
 * Rename `Accept::{mime_preference => preference}`. [#2480]
 * Un-deprecate `App::data_factory`. [#2484]
+* `HttpRequest::url_for` no longer constructs URLs with query or fragment components. [#2430]
 
 ### Fixed
 * Accept wildcard `*` items in `AcceptLanguage`. [#2480]
 * Re-exports `dev::{BodySize, MessageBody, SizedStream}`. They are exposed through the `body` module. [#2468]
 * Typed headers containing lists that require one or more items now enforce this minimum. [#2482]
 
+[#2430]: https://github.com/actix/actix-web/pull/2430
 [#2468]: https://github.com/actix/actix-web/pull/2468
 [#2480]: https://github.com/actix/actix-web/pull/2480
 [#2482]: https://github.com/actix/actix-web/pull/2482

src/error/mod.rs

@@ -29,15 +29,15 @@ pub type Result<T, E = Error> = std::result::Result<T, E>;
 #[derive(Debug, PartialEq, Display, Error, From)]
 #[non_exhaustive]
 pub enum UrlGenerationError {
-    /// Resource not found
+    /// Resource not found.
     #[display(fmt = "Resource not found")]
     ResourceNotFound,
 
-    /// Not all path pattern covered
-    #[display(fmt = "Not all path pattern covered")]
+    /// Not all URL parameters covered.
+    #[display(fmt = "Not all URL parameters covered")]
     NotEnoughElements,
 
-    /// URL parse error
+    /// URL parse error.
     #[display(fmt = "{}", _0)]
     ParseError(UrlParseError),
 }

src/request.rs

@@ -100,26 +100,30 @@ impl HttpRequest {
         &self.head().headers
     }
 
-    /// The target path of this Request.
+    /// The target path of this request.
     #[inline]
     pub fn path(&self) -> &str {
         self.head().uri.path()
     }
 
     /// The query string in the URL.
     ///
-    /// E.g., id=10
+    /// Example: `id=10`
     #[inline]
     pub fn query_string(&self) -> &str {
         self.uri().query().unwrap_or_default()
     }
 
-    /// Get a reference to the Path parameters.
+    /// Returns a reference to the URL parameters container.
     ///
-    /// Params is a container for url parameters.
-    /// A variable segment 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.
+    /// 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 parameter.
+    ///
+    /// # Percent Encoding and URL Parameters
+    /// Because each URL parameter 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 parameter 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
@@ -161,23 +165,29 @@ impl HttpRequest {
         self.head().extensions_mut()
     }
 
-    /// Generate url for named resource
+    /// Generates URL for a 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
+    /// a URL path context is escaped using percent-encoding.
     ///
+    /// # Examples
     /// ```
     /// # use actix_web::{web, App, HttpRequest, HttpResponse};
-    /// #
     /// fn index(req: HttpRequest) -> HttpResponse {
-    ///     let url = req.url_for("foo", &["1", "2", "3"]); // <- generate url for "foo" resource
+    ///     let url = req.url_for("foo", &["1", "2", "3"]); // <- generate URL for "foo" resource
     ///     HttpResponse::Ok().into()
     /// }
     ///
-    /// fn main() {
-    ///     let app = App::new()
-    ///         .service(web::resource("/test/{one}/{two}/{three}")
-    ///              .name("foo")  // <- set resource name, then it could be used in `url_for`
-    ///              .route(web::get().to(|| HttpResponse::Ok()))
-    ///         );
-    /// }
+    /// let app = App::new()
+    ///     .service(web::resource("/test/{one}/{two}/{three}")
+    ///          .name("foo")  // <- set resource name so it can be used in `url_for`
+    ///          .route(web::get().to(|| HttpResponse::Ok()))
+    ///     );
     /// ```
     pub fn url_for<U, I>(&self, name: &str, elements: U) -> Result<url::Url, UrlGenerationError>
     where
@@ -196,8 +206,8 @@ impl HttpRequest {
         self.url_for(name, &NO_PARAMS)
     }
 
-    #[inline]
     /// Get a reference to a `ResourceMap` of current application.
+    #[inline]
     pub fn resource_map(&self) -> &ResourceMap {
         self.app_state().rmap()
     }

src/rmap.rs

@@ -1,12 +1,14 @@
-use std::cell::RefCell;
-use std::rc::{Rc, Weak};
+use std::{
+    borrow::Cow,
+    cell::RefCell,
+    rc::{Rc, Weak},
+};
 
 use actix_router::ResourceDef;
 use ahash::AHashMap;
 use url::Url;
 
-use crate::error::UrlGenerationError;
-use crate::request::HttpRequest;
+use crate::{error::UrlGenerationError, request::HttpRequest};
 
 #[derive(Clone, Debug)]
 pub struct ResourceMap {
@@ -102,17 +104,28 @@ impl ResourceMap {
             })
             .ok_or(UrlGenerationError::NotEnoughElements)?;
 
-        if path.starts_with('/') {
+        let (base, path): (Cow<'_, _>, _) = if path.starts_with('/') {
+            // build full URL from connection info parts and resource path
             let conn = req.connection_info();
-            Ok(Url::parse(&format!(
-                "{}://{}{}",
-                conn.scheme(),
-                conn.host(),
-                path
-            ))?)
+            let base = format!("{}://{}", conn.scheme(), conn.host());
+            (Cow::Owned(base), path.as_str())
         } else {
-            Ok(Url::parse(&path)?)
-        }
+            // external resource; third slash would be the root slash in the path
+            let third_slash_index = path
+                .char_indices()
+                .filter_map(|(i, c)| (c == '/').then(|| i))
+                .nth(2)
+                .unwrap_or_else(|| path.len());
+
+            (
+                Cow::Borrowed(&path[..third_slash_index]),
+                &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 +419,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(""));