From 3a44b1b28e6c22b1d316409269999dc8e8395444 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Mon, 10 Apr 2023 12:31:17 -0700 Subject: [PATCH] Hide 'RouteUri' fields to ensure URI coherence. Prior to this commit, several `RouteUri` fields were public, allowing those values to be changed at will. These changes were at times not reflected by the rest of the library, meaning that the values in the route URI structure for a route became incoherent with the reflected values. This commit makes all fields private, forcing all changes to go through methods that can ensure coherence. All values remain accessible via getter methods. --- benchmarks/src/routing.rs | 4 +- core/http/src/raw_str.rs | 7 ++ core/http/src/uri/origin.rs | 2 +- core/http/src/uri/path_query.rs | 6 + core/http/src/uri/uri.rs | 10 +- core/lib/fuzz/targets/collision-matching.rs | 6 +- core/lib/src/route/route.rs | 18 ++- core/lib/src/route/uri.rs | 128 +++++++------------- core/lib/tests/route_guard.rs | 2 +- 9 files changed, 89 insertions(+), 94 deletions(-) diff --git a/benchmarks/src/routing.rs b/benchmarks/src/routing.rs index 1a6dafcd..8f842896 100644 --- a/benchmarks/src/routing.rs +++ b/benchmarks/src/routing.rs @@ -45,13 +45,13 @@ fn generate_matching_requests<'c>(client: &'c Client, routes: &[Route]) -> Vec(client: &'c Client, route: &Route) -> LocalRequest<'c> { - let path = route.uri.uri.path() + let path = route.uri.path() .raw_segments() .map(staticify_segment) .collect::>() .join("/"); - let query = route.uri.uri.query() + let query = route.uri.query() .map(|q| q.raw_segments()) .into_iter() .flatten() diff --git a/core/http/src/raw_str.rs b/core/http/src/raw_str.rs index 1aa37b2b..372453e8 100644 --- a/core/http/src/raw_str.rs +++ b/core/http/src/raw_str.rs @@ -1009,6 +1009,13 @@ impl AsRef for RawStr { } } +impl AsRef for RawStr { + #[inline(always)] + fn as_ref(&self) -> &std::ffi::OsStr { + self.as_str().as_ref() + } +} + impl AsRef for str { #[inline(always)] fn as_ref(&self) -> &RawStr { diff --git a/core/http/src/uri/origin.rs b/core/http/src/uri/origin.rs index 716dd163..ebe5e444 100644 --- a/core/http/src/uri/origin.rs +++ b/core/http/src/uri/origin.rs @@ -548,7 +548,7 @@ impl<'a> Origin<'a> { impl_serde!(Origin<'a>, "an origin-form URI"); -impl_traits!(Origin, path, query); +impl_traits!(Origin [parse_route], path, query); impl std::fmt::Display for Origin<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { diff --git a/core/http/src/uri/path_query.rs b/core/http/src/uri/path_query.rs index 2b9ea23d..16733bc7 100644 --- a/core/http/src/uri/path_query.rs +++ b/core/http/src/uri/path_query.rs @@ -416,6 +416,12 @@ macro_rules! impl_traits { } } + impl AsRef for $T<'_> { + fn as_ref(&self) -> &std::ffi::OsStr { + self.raw().as_ref() + } + } + impl std::fmt::Display for $T<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self.raw()) diff --git a/core/http/src/uri/uri.rs b/core/http/src/uri/uri.rs index 57e992ea..14247670 100644 --- a/core/http/src/uri/uri.rs +++ b/core/http/src/uri/uri.rs @@ -391,7 +391,10 @@ macro_rules! impl_serde { /// Implements traits from `impl_base_traits` and IntoOwned for a URI. macro_rules! impl_traits { ($T:ident, $($field:ident),* $(,)?) => { - impl_base_traits!($T, $($field),*); + impl_traits!($T [parse], $($field),*); + }; + ($T:ident [$partial_eq_parse:ident], $($field:ident),* $(,)?) => { + impl_base_traits!($T [$partial_eq_parse], $($field),*); impl crate::ext::IntoOwned for $T<'_> { type Owned = $T<'static>; @@ -409,6 +412,9 @@ macro_rules! impl_traits { /// Implements PartialEq, Eq, Hash, and TryFrom. macro_rules! impl_base_traits { ($T:ident, $($field:ident),* $(,)?) => { + impl_base_traits!($T [parse], $($field),*); + }; + ($T:ident [$partial_eq_parse:ident], $($field:ident),* $(,)?) => { impl std::convert::TryFrom for $T<'static> { type Error = Error<'static>; @@ -442,7 +448,7 @@ macro_rules! impl_base_traits { impl PartialEq for $T<'_> { fn eq(&self, string: &str) -> bool { - $T::parse(string).map_or(false, |v| &v == self) + $T::$partial_eq_parse(string).map_or(false, |v| &v == self) } } diff --git a/core/lib/fuzz/targets/collision-matching.rs b/core/lib/fuzz/targets/collision-matching.rs index b38a2875..ad350eac 100644 --- a/core/lib/fuzz/targets/collision-matching.rs +++ b/core/lib/fuzz/targets/collision-matching.rs @@ -26,8 +26,8 @@ impl std::fmt::Debug for ArbitraryRouteData<'_> { f.debug_struct("ArbitraryRouteData") .field("method", &self.method.0) .field("base", &self.uri.0.base()) - .field("path", &self.uri.0.unmounted_origin.to_string()) - .field("uri", &self.uri.0.uri.to_string()) + .field("unmounted", &self.uri.0.unmounted().to_string()) + .field("uri", &self.uri.0.to_string()) .field("format", &self.format.as_ref().map(|v| v.0.to_string())) .finish() } @@ -59,7 +59,7 @@ impl<'c, 'a: 'c> ArbitraryRequestData<'a> { impl<'a> ArbitraryRouteData<'a> { fn into_route(self) -> Route { - let mut r = Route::ranked(0, self.method.0, self.uri.0.as_str(), dummy_handler); + let mut r = Route::ranked(0, self.method.0, &self.uri.0.to_string(), dummy_handler); r.format = self.format.map(|f| f.0); r } diff --git a/core/lib/src/route/route.rs b/core/lib/src/route/route.rs index 994cb058..944b56e4 100644 --- a/core/lib/src/route/route.rs +++ b/core/lib/src/route/route.rs @@ -200,6 +200,11 @@ impl Route { /// /// Panics if `path` is not a valid Rocket route URI. /// + /// A valid route URI is any valid [`Origin`](uri::Origin) URI that is + /// normalized, that is, does not contain any empty segments except for an + /// optional trailing slash. Unlike a strict `Origin`, route URIs are also + /// allowed to contain any UTF-8 characters. + /// /// # Example /// /// ```rust @@ -207,7 +212,7 @@ impl Route { /// use rocket::http::Method; /// # use rocket::route::dummy_handler as handler; /// - /// // this is a rank 1 route matching requests to `GET /` + /// // this is a route matching requests to `GET /` /// let index = Route::new(Method::Get, "/", handler); /// assert_eq!(index.rank, -9); /// assert_eq!(index.method, Method::Get); @@ -226,6 +231,11 @@ impl Route { /// /// Panics if `path` is not a valid Rocket route URI. /// + /// A valid route URI is any valid [`Origin`](uri::Origin) URI that is + /// normalized, that is, does not contain any empty segments except for an + /// optional trailing slash. Unlike a strict `Origin`, route URIs are also + /// allowed to contain any UTF-8 characters. + /// /// # Example /// /// ```rust @@ -275,12 +285,12 @@ impl Route { /// /// let index = Route::new(Method::Get, "/foo/bar", handler); /// assert_eq!(index.uri.base(), "/"); - /// assert_eq!(index.uri.unmounted_origin.path(), "/foo/bar"); + /// assert_eq!(index.uri.unmounted().path(), "/foo/bar"); /// assert_eq!(index.uri.path(), "/foo/bar"); /// /// let index = index.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); /// assert_eq!(index.uri.base(), "/boo"); - /// assert_eq!(index.uri.unmounted_origin.path(), "/foo/bar"); + /// assert_eq!(index.uri.unmounted().path(), "/foo/bar"); /// assert_eq!(index.uri.path(), "/boo/foo/bar"); /// ``` pub fn map_base<'a, F>(mut self, mapper: F) -> Result> @@ -309,7 +319,7 @@ impl fmt::Display for Route { write!(f, "{}", Paint::blue(self.uri.base()).underline())?; } - write!(f, "{}", Paint::blue(&self.uri.unmounted_origin))?; + write!(f, "{}", Paint::blue(&self.uri.unmounted()))?; if self.rank > 1 { write!(f, " [{}]", Paint::default(&self.rank).bold())?; diff --git a/core/lib/src/route/uri.rs b/core/lib/src/route/uri.rs index b3b8b19a..9fc073b0 100644 --- a/core/lib/src/route/uri.rs +++ b/core/lib/src/route/uri.rs @@ -1,7 +1,6 @@ use std::fmt; -use std::borrow::Cow; -use crate::http::uri::{self, Origin}; +use crate::http::uri::{self, Origin, Path}; use crate::http::ext::IntoOwned; use crate::form::ValueField; use crate::route::Segment; @@ -53,16 +52,14 @@ use crate::route::Segment; /// /// [`Rocket::mount()`]: crate::Rocket::mount() /// [`Route::new()`]: crate::Route::new() -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct RouteUri<'a> { - /// The source string for this URI. - source: Cow<'a, str>, /// The mount point. - pub base: Origin<'a>, + pub(crate) base: Origin<'a>, /// The URI _without_ the `base` mount point. - pub unmounted_origin: Origin<'a>, + pub(crate) unmounted_origin: Origin<'a>, /// The URI _with_ the base mount point. This is the canonical route URI. - pub uri: Origin<'a>, + pub(crate) uri: Origin<'a>, /// Cached metadata about this URI. pub(crate) metadata: Metadata, } @@ -98,6 +95,14 @@ type Result> = std::result::Result; impl<'a> RouteUri<'a> { /// Create a new `RouteUri`. /// + /// Panics if `base` or `uri` cannot be parsed as `Origin`s. + #[track_caller] + pub(crate) fn new(base: &str, uri: &str) -> RouteUri<'static> { + Self::try_new(base, uri).expect("Expected valid URIs") + } + + /// Creates a new `RouteUri` from a `base` mount point and a route `uri`. + /// /// This is a fallible variant of [`RouteUri::new`] which returns an `Err` /// if `base` or `uri` cannot be parsed as [`Origin`]s. /// INTERNAL! @@ -129,21 +134,15 @@ impl<'a> RouteUri<'a> { .into_normalized() .into_owned(); - let source = uri.to_string().into(); let metadata = Metadata::from(&base, &uri); - Ok(RouteUri { source, base, unmounted_origin: origin, uri, metadata }) + Ok(RouteUri { base, unmounted_origin: origin, uri, metadata }) } - /// Create a new `RouteUri`. + /// Returns the complete route URI. /// - /// Panics if `base` or `uri` cannot be parsed as `Origin`s. - #[track_caller] - pub(crate) fn new(base: &str, uri: &str) -> RouteUri<'static> { - Self::try_new(base, uri).expect("Expected valid URIs") - } - - /// The path of the base mount point of this route URI as an `&str`. + /// **Note:** `RouteURI` derefs to the `Origin` returned by this method, so + /// this method should rarely be called directly. /// /// # Example /// @@ -152,17 +151,19 @@ impl<'a> RouteUri<'a> { /// use rocket::http::Method; /// # use rocket::route::dummy_handler as handler; /// - /// let index = Route::new(Method::Get, "/foo/bar?a=1", handler); - /// assert_eq!(index.uri.base(), "/"); - /// let index = index.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); - /// assert_eq!(index.uri.base(), "/boo"); + /// let route = Route::new(Method::Get, "/foo/bar?a=1", handler); + /// + /// // Use `inner()` directly: + /// assert_eq!(route.uri.inner().query().unwrap(), "a=1"); + /// + /// // Use the deref implementation. This is preferred: + /// assert_eq!(route.uri.query().unwrap(), "a=1"); /// ``` - #[inline(always)] - pub fn base(&self) -> &str { - self.base.path().as_str() + pub fn inner(&self) -> &Origin<'a> { + &self.uri } - /// The path part of this route URI as an `&str`. + /// The base mount point of this route URI. /// /// # Example /// @@ -171,17 +172,18 @@ impl<'a> RouteUri<'a> { /// use rocket::http::Method; /// # use rocket::route::dummy_handler as handler; /// - /// let index = Route::new(Method::Get, "/foo/bar?a=1", handler); - /// assert_eq!(index.uri.path(), "/foo/bar"); - /// let index = index.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); - /// assert_eq!(index.uri.path(), "/boo/foo/bar"); + /// let route = Route::new(Method::Get, "/foo/bar?a=1", handler); + /// assert_eq!(route.uri.base(), "/"); + /// + /// let route = route.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); + /// assert_eq!(route.uri.base(), "/boo"); /// ``` #[inline(always)] - pub fn path(&self) -> &str { - self.uri.path().as_str() + pub fn base(&self) -> Path<'_> { + self.base.path() } - /// The query part of this route URI, if there is one. + /// The route URI _without_ the base mount point. /// /// # Example /// @@ -190,41 +192,16 @@ impl<'a> RouteUri<'a> { /// use rocket::http::Method; /// # use rocket::route::dummy_handler as handler; /// - /// let index = Route::new(Method::Get, "/foo/bar", handler); - /// assert!(index.uri.query().is_none()); + /// let route = Route::new(Method::Get, "/foo/bar?a=1", handler); + /// let route = route.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); /// - /// // Normalization clears the empty '?'. - /// let index = Route::new(Method::Get, "/foo/bar?", handler); - /// assert_eq!(index.uri.query().unwrap(), ""); - /// - /// let index = Route::new(Method::Get, "/foo/bar?a=1", handler); - /// assert_eq!(index.uri.query().unwrap(), "a=1"); - /// - /// let index = index.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); - /// assert_eq!(index.uri.query().unwrap(), "a=1"); + /// assert_eq!(route.uri, "/boo/foo/bar?a=1"); + /// assert_eq!(route.uri.base(), "/boo"); + /// assert_eq!(route.uri.unmounted(), "/foo/bar?a=1"); /// ``` #[inline(always)] - pub fn query(&self) -> Option<&str> { - self.uri.query().map(|q| q.as_str()) - } - - /// The full URI as an `&str`. - /// - /// # Example - /// - /// ```rust - /// use rocket::Route; - /// use rocket::http::Method; - /// # use rocket::route::dummy_handler as handler; - /// - /// let index = Route::new(Method::Get, "/foo/bar?a=1", handler); - /// assert_eq!(index.uri.as_str(), "/foo/bar?a=1"); - /// let index = index.map_base(|base| format!("{}{}", "/boo", base)).unwrap(); - /// assert_eq!(index.uri.as_str(), "/boo/foo/bar?a=1"); - /// ``` - #[inline(always)] - pub fn as_str(&self) -> &str { - &self.source + pub fn unmounted(&self) -> &Origin<'a> { + &self.unmounted_origin } /// Get the default rank of a route with this URI. @@ -306,35 +283,24 @@ impl<'a> std::ops::Deref for RouteUri<'a> { type Target = Origin<'a>; fn deref(&self) -> &Self::Target { - &self.uri + self.inner() } } impl fmt::Display for RouteUri<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.uri.fmt(f) - } -} - -impl fmt::Debug for RouteUri<'_> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("RouteUri") - .field("base", &self.base) - .field("unmounted_origin", &self.unmounted_origin) - .field("origin", &self.uri) - .field("metadata", &self.metadata) - .finish() + self.inner().fmt(f) } } impl<'a, 'b> PartialEq> for RouteUri<'a> { - fn eq(&self, other: &Origin<'b>) -> bool { &self.uri == other } + fn eq(&self, other: &Origin<'b>) -> bool { self.inner() == other } } impl PartialEq for RouteUri<'_> { - fn eq(&self, other: &str) -> bool { self.as_str() == other } + fn eq(&self, other: &str) -> bool { self.inner() == other } } impl PartialEq<&str> for RouteUri<'_> { - fn eq(&self, other: &&str) -> bool { self.as_str() == *other } + fn eq(&self, other: &&str) -> bool { self.inner() == *other } } diff --git a/core/lib/tests/route_guard.rs b/core/lib/tests/route_guard.rs index 2a88b24f..117efddf 100644 --- a/core/lib/tests/route_guard.rs +++ b/core/lib/tests/route_guard.rs @@ -6,7 +6,7 @@ use rocket::Route; #[get("/")] fn files(route: &Route, path: PathBuf) -> String { - Path::new(route.uri.base()).join(path).normalized_str().to_string() + Path::new(&route.uri.base()).join(path).normalized_str().to_string() } mod route_guard_tests {