From 59be0c65c6b4766355d8bf08de27531d03730bdd Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Sun, 5 Dec 2021 07:39:18 +0300 Subject: [PATCH] disallow query or fragements in `url_for` constructions (#2430) Co-authored-by: Rob Ede --- CHANGES.md | 2 ++ src/error/mod.rs | 8 +++--- src/request.rs | 46 +++++++++++++++++------------ src/rmap.rs | 75 +++++++++++++++++++++++++++++++++++++++--------- 4 files changed, 96 insertions(+), 35 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 78aa729df..1b108fee6 100644 --- a/CHANGES.md +++ b/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 diff --git a/src/error/mod.rs b/src/error/mod.rs index 3ccd5bba6..46d0dccc6 100644 --- a/src/error/mod.rs +++ b/src/error/mod.rs @@ -29,15 +29,15 @@ pub type Result = std::result::Result; #[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), } diff --git a/src/request.rs b/src/request.rs index 0027f9b4b..58222da47 100644 --- a/src/request.rs +++ b/src/request.rs @@ -100,7 +100,7 @@ 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() @@ -108,18 +108,22 @@ impl HttpRequest { /// 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 { &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(&self, name: &str, elements: U) -> Result 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() } diff --git a/src/rmap.rs b/src/rmap.rs index 8466eda28..432eaf83c 100644 --- a/src/rmap.rs +++ b/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(""));