From 9b16cbf254738c812a86f696cfb015208a4f3202 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Mon, 2 Jun 2025 13:14:53 -0500 Subject: [PATCH] Fix url encoding query and implement FromQueryArgument for Option (#4213) * Fix url encoding queries * handle percent encoding hash fragments * implement fromqueryargument for Option * fix tests * fix typo --- Cargo.lock | 6 +- Cargo.toml | 2 +- packages/asset-resolver/Cargo.toml | 2 +- packages/asset-resolver/src/lib.rs | 3 +- packages/desktop/Cargo.toml | 2 +- packages/router-macro/src/hash.rs | 2 +- packages/router-macro/src/lib.rs | 14 ++- packages/router-macro/src/query.rs | 28 +++--- packages/router-macro/src/segment.rs | 2 +- packages/router/Cargo.toml | 2 +- packages/router/src/lib.rs | 33 ++++++- packages/router/src/routable.rs | 139 ++++++++++++++++++++++++++- packages/router/tests/parsing.rs | 89 ++++++++++++++++- 13 files changed, 294 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8e6db4ae3..aa47f7c12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3675,8 +3675,8 @@ dependencies = [ "ndk", "ndk-context", "ndk-sys 0.6.0+11769913", + "percent-encoding", "thiserror 2.0.12", - "urlencoding", ] [[package]] @@ -3985,6 +3985,7 @@ dependencies = [ "ndk-sys 0.6.0+11769913", "objc", "objc_id", + "percent-encoding", "rand 0.8.5", "reqwest 0.12.15", "rfd", @@ -4000,7 +4001,6 @@ dependencies = [ "tokio-tungstenite", "tracing", "tray-icon", - "urlencoding", "webbrowser", "wry", ] @@ -4447,12 +4447,12 @@ dependencies = [ "dioxus-router", "dioxus-router-macro", "dioxus-ssr", + "percent-encoding", "rustversion", "serde", "tokio", "tracing", "url", - "urlencoding", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d8156f360..874c6f83e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -316,7 +316,7 @@ tao = { version = "0.33.0", features = ["rwh_05"] } webbrowser = "1.0.3" infer = "0.19.0" dunce = "1.0.5" -urlencoding = "2.1.3" +percent-encoding = "2.3.1" global-hotkey = "0.7.0" rfd = { version = "0.15.2", default-features = false } muda = "0.16.1" diff --git a/packages/asset-resolver/Cargo.toml b/packages/asset-resolver/Cargo.toml index 44c25809b..784a87eec 100644 --- a/packages/asset-resolver/Cargo.toml +++ b/packages/asset-resolver/Cargo.toml @@ -12,7 +12,7 @@ rust-version = "1.79.0" [dependencies] http = { workspace = true } -urlencoding = { workspace = true } +percent-encoding = { workspace = true } infer = { workspace = true } thiserror = { workspace = true } dioxus-cli-config = { workspace = true } diff --git a/packages/asset-resolver/src/lib.rs b/packages/asset-resolver/src/lib.rs index ca1759dec..47225fbff 100644 --- a/packages/asset-resolver/src/lib.rs +++ b/packages/asset-resolver/src/lib.rs @@ -18,7 +18,8 @@ pub fn serve_asset_from_raw_path(path: &str) -> Result>, AssetS // If the user provided a custom asset handler, then call it and return the response if the request was handled. // The path is the first part of the URI, so we need to trim the leading slash. let mut uri_path = PathBuf::from( - urlencoding::decode(path) + percent_encoding::percent_decode_str(path) + .decode_utf8() .expect("expected URL to be UTF-8 encoded") .as_ref(), ); diff --git a/packages/desktop/Cargo.toml b/packages/desktop/Cargo.toml index da079ea1e..f2845c4d1 100644 --- a/packages/desktop/Cargo.toml +++ b/packages/desktop/Cargo.toml @@ -44,7 +44,7 @@ slab = { workspace = true } rustc-hash = { workspace = true } dioxus-hooks = { workspace = true } futures-util = { workspace = true } -urlencoding = { workspace = true } +percent-encoding = { workspace = true } async-trait = { workspace = true } tao = { workspace = true, features = ["rwh_05"] } dioxus-history = { workspace = true } diff --git a/packages/router-macro/src/hash.rs b/packages/router-macro/src/hash.rs index c199a0ac2..367c5ef36 100644 --- a/packages/router-macro/src/hash.rs +++ b/packages/router-macro/src/hash.rs @@ -28,7 +28,7 @@ impl HashFragment { { let __hash = #ident.to_string(); if !__hash.is_empty() { - write!(f, "#{}", __hash)?; + write!(f, "#{}", dioxus_router::exports::percent_encoding::utf8_percent_encode(&__hash, dioxus_router::exports::FRAGMENT_ASCII_SET))?; } } } diff --git a/packages/router-macro/src/lib.rs b/packages/router-macro/src/lib.rs index 8c0b4f906..c17c269c0 100644 --- a/packages/router-macro/src/lib.rs +++ b/packages/router-macro/src/lib.rs @@ -531,9 +531,17 @@ impl RouteEnum { // Remove any trailing slashes. We parse /route/ and /route in the same way // Note: we don't use trim because it includes more code let route = route.strip_suffix('/').unwrap_or(route); - let query = dioxus_router::exports::urlencoding::decode(query).unwrap_or(query.into()); - let hash = dioxus_router::exports::urlencoding::decode(hash).unwrap_or(hash.into()); - let mut segments = route.split('/').map(|s| dioxus_router::exports::urlencoding::decode(s).unwrap_or(s.into())); + let query = dioxus_router::exports::percent_encoding::percent_decode_str(query) + .decode_utf8() + .unwrap_or(query.into()); + let hash = dioxus_router::exports::percent_encoding::percent_decode_str(hash) + .decode_utf8() + .unwrap_or(hash.into()); + let mut segments = route.split('/').map(|s| { + dioxus_router::exports::percent_encoding::percent_decode_str(s) + .decode_utf8() + .unwrap_or(s.into()) + }); // skip the first empty segment if s.starts_with('/') { let _ = segments.next(); diff --git a/packages/router-macro/src/query.rs b/packages/router-macro/src/query.rs index 7cddf5c57..ae6f17f29 100644 --- a/packages/router-macro/src/query.rs +++ b/packages/router-macro/src/query.rs @@ -39,13 +39,8 @@ impl QuerySegment { QuerySegment::Segments(segments) => { let mut tokens = TokenStream2::new(); tokens.extend(quote! { write!(f, "?")?; }); - let mut segments_iter = segments.iter(); - if let Some(first_segment) = segments_iter.next() { - tokens.extend(first_segment.write()); - } - for segment in segments_iter { - tokens.extend(quote! { write!(f, "&")?; }); - tokens.extend(segment.write()); + for (i, segment) in segments.iter().enumerate() { + tokens.extend(segment.write(i == segments.len() - 1)); } tokens } @@ -133,7 +128,7 @@ impl FullQuerySegment { quote! { { let as_string = #ident.to_string(); - write!(f, "?{}", dioxus_router::exports::urlencoding::encode(&as_string))?; + write!(f, "?{}", dioxus_router::exports::percent_encoding::utf8_percent_encode(&as_string, dioxus_router::exports::QUERY_ASCII_SET))?; } } } @@ -151,18 +146,27 @@ impl QueryArgument { let ty = &self.ty; quote! { let #ident = match split_query.get(stringify!(#ident)) { - Some(query_argument) => <#ty as dioxus_router::routable::FromQueryArgument>::from_query_argument(query_argument).unwrap_or_default(), + Some(query_argument) => { + use dioxus_router::routable::FromQueryArgument; + <#ty>::from_query_argument(query_argument).unwrap_or_default() + }, None => <#ty as Default>::default(), }; } } - pub fn write(&self) -> TokenStream2 { + pub fn write(&self, trailing: bool) -> TokenStream2 { let ident = &self.ident; + let write_ampersand = if !trailing { + quote! { if !as_string.is_empty() { write!(f, "&")?; } } + } else { + quote! {} + }; quote! { { - let as_string = #ident.to_string(); - write!(f, "{}={}", stringify!(#ident), dioxus_router::exports::urlencoding::encode(&as_string))?; + let as_string = dioxus_router::routable::DisplayQueryArgument::new(stringify!(#ident), #ident).to_string(); + write!(f, "{}", dioxus_router::exports::percent_encoding::utf8_percent_encode(&as_string, dioxus_router::exports::QUERY_ASCII_SET))?; + #write_ampersand } } } diff --git a/packages/router-macro/src/segment.rs b/packages/router-macro/src/segment.rs index 1c181e9e5..06f57f707 100644 --- a/packages/router-macro/src/segment.rs +++ b/packages/router-macro/src/segment.rs @@ -27,7 +27,7 @@ impl RouteSegment { Self::Dynamic(ident, _) => quote! { { let as_string = #ident.to_string(); - write!(f, "/{}", dioxus_router::exports::urlencoding::encode(&as_string))?; + write!(f, "/{}", dioxus_router::exports::percent_encoding::utf8_percent_encode(&as_string, dioxus_router::exports::PATH_ASCII_SET))?; } }, Self::CatchAll(ident, _) => quote! { #ident.display_route_segments(f)?; }, diff --git a/packages/router/Cargo.toml b/packages/router/Cargo.toml index 004b16d5a..de9f55591 100644 --- a/packages/router/Cargo.toml +++ b/packages/router/Cargo.toml @@ -15,7 +15,7 @@ dioxus-history = { workspace = true } dioxus-router-macro = { workspace = true } dioxus-fullstack-hooks = { workspace = true, optional = true } tracing = { workspace = true } -urlencoding = { workspace = true } +percent-encoding = { workspace = true } url = { workspace = true } dioxus-cli-config = { workspace = true } rustversion = { workspace = true } diff --git a/packages/router/src/lib.rs b/packages/router/src/lib.rs index eda92422e..470a57939 100644 --- a/packages/router/src/lib.rs +++ b/packages/router/src/lib.rs @@ -89,5 +89,36 @@ mod utils { #[doc(hidden)] pub mod exports { - pub use urlencoding; + pub use crate::query_sets::*; + pub use percent_encoding; +} + +pub(crate) mod query_sets { + //! Url percent encode sets defined [here](https://url.spec.whatwg.org/#percent-encoded-bytes) + + use percent_encoding::AsciiSet; + + /// The ASCII set that must be escaped in query strings. + pub const QUERY_ASCII_SET: &AsciiSet = &percent_encoding::CONTROLS + .add(b' ') + .add(b'"') + .add(b'#') + .add(b'<') + .add(b'>'); + + /// The ASCII set that must be escaped in path segments. + pub const PATH_ASCII_SET: &AsciiSet = &QUERY_ASCII_SET + .add(b'?') + .add(b'^') + .add(b'`') + .add(b'{') + .add(b'}'); + + /// The ASCII set that must be escaped in hash fragments. + pub const FRAGMENT_ASCII_SET: &AsciiSet = &percent_encoding::CONTROLS + .add(b' ') + .add(b'"') + .add(b'<') + .add(b'>') + .add(b'`'); } diff --git a/packages/router/src/routable.rs b/packages/router/src/routable.rs index 34017cd8e..c6e5ac3cf 100644 --- a/packages/router/src/routable.rs +++ b/packages/router/src/routable.rs @@ -123,7 +123,7 @@ impl From<&'a str>> FromQuery for T { /// } /// } /// -/// // We also need to implement Display for CustomQuery which will be used to format the query string into the URL +/// // We also need to implement Display for CustomQuery so that ToQueryArgument is implemented automatically /// impl std::fmt::Display for CustomQuery { /// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { /// write!(f, "{}", self.count) @@ -143,7 +143,7 @@ impl From<&'a str>> FromQuery for T { note = "FromQueryArgument is automatically implemented for types that implement `FromStr` and `Default`. You need to either implement FromStr and Default or implement FromQueryArgument manually." ) )] -pub trait FromQueryArgument: Default { +pub trait FromQueryArgument

: Default { /// The error that can occur when parsing a query argument. type Err; @@ -168,6 +168,138 @@ where } } +/// A marker type for `Option` to implement `FromQueryArgument`. +pub struct OptionMarker; + +impl FromQueryArgument for Option +where + ::Err: Display, +{ + type Err = ::Err; + + fn from_query_argument(argument: &str) -> Result { + match T::from_str(argument) { + Ok(result) => Ok(Some(result)), + Err(err) => { + tracing::error!("Failed to parse query argument: {}", err); + Err(err) + } + } + } +} + +/// Something that can be formatted as a query argument. This trait must be implemented for any type that is used as a query argument like `#[route("/?:query")]`. +/// +/// **This trait is automatically implemented for any types that implement [`Display`].** +/// +/// ```rust +/// use dioxus::prelude::*; +/// +/// #[derive(Routable, Clone, PartialEq, Debug)] +/// enum Route { +/// // FromQuerySegment must be implemented for any types you use in the query segment +/// // When you don't spread the query, you can parse multiple values form the query +/// // This url will be in the format `/?query=123&other=456` +/// #[route("/?:query&:other")] +/// Home { +/// query: CustomQuery, +/// other: i32, +/// }, +/// } +/// +/// // We can derive Default for CustomQuery +/// // If the router fails to parse the query value, it will use the default value instead +/// #[derive(Default, Clone, PartialEq, Debug)] +/// struct CustomQuery { +/// count: i32, +/// } +/// +/// // We implement FromStr for CustomQuery so that FromQuerySegment is implemented automatically +/// impl std::str::FromStr for CustomQuery { +/// type Err = ::Err; +/// +/// fn from_str(query: &str) -> Result { +/// Ok(CustomQuery { +/// count: query.parse()?, +/// }) +/// } +/// } +/// +/// // We also need to implement Display for CustomQuery so that ToQueryArgument is implemented automatically +/// impl std::fmt::Display for CustomQuery { +/// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +/// write!(f, "{}", self.count) +/// } +/// } +/// +/// # #[component] +/// # fn Home(query: CustomQuery, other: i32) -> Element { +/// # unimplemented!() +/// # } +/// ``` +pub trait ToQueryArgument { + /// Display the query argument as a string. + fn display_query_argument( + &self, + query_name: &str, + f: &mut std::fmt::Formatter<'_>, + ) -> std::fmt::Result; +} + +impl ToQueryArgument for T +where + T: Display, +{ + fn display_query_argument( + &self, + query_name: &str, + f: &mut std::fmt::Formatter<'_>, + ) -> std::fmt::Result { + write!(f, "{}={}", query_name, self) + } +} + +impl ToQueryArgument for Option { + fn display_query_argument( + &self, + query_name: &str, + f: &mut std::fmt::Formatter<'_>, + ) -> std::fmt::Result { + if let Some(value) = self { + write!(f, "{}={}", query_name, value) + } else { + Ok(()) + } + } +} + +/// A type that implements [`ToQueryArgument`] along with the query name. This type implements Display and can be used to format the query argument into a string. +pub struct DisplayQueryArgument<'a, T, M = ()> { + /// The query name. + query_name: &'a str, + /// The value to format. + value: &'a T, + /// The `ToQueryArgument` marker type, which can be used to differentiate between different types of query arguments. + _marker: std::marker::PhantomData, +} + +impl<'a, T, M> DisplayQueryArgument<'a, T, M> { + /// Create a new `DisplayQueryArgument`. + pub fn new(query_name: &'a str, value: &'a T) -> Self { + Self { + query_name, + value, + _marker: std::marker::PhantomData, + } + } +} + +impl, M> Display for DisplayQueryArgument<'_, T, M> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.value.display_query_argument(self.query_name, f) + } +} + /// Something that can be created from an entire hash fragment. This must be implemented for any type that is used as a hash fragment like `#[route("/#:hash_fragment")]`. /// /// @@ -406,7 +538,8 @@ where for segment in self { write!(f, "/")?; let segment = segment.to_string(); - let encoded = urlencoding::encode(&segment); + let encoded = + percent_encoding::utf8_percent_encode(&segment, crate::query_sets::PATH_ASCII_SET); write!(f, "{}", encoded)?; } Ok(()) diff --git a/packages/router/tests/parsing.rs b/packages/router/tests/parsing.rs index 05dfb6537..7825d8fcd 100644 --- a/packages/router/tests/parsing.rs +++ b/packages/router/tests/parsing.rs @@ -1,5 +1,8 @@ use dioxus::prelude::*; -use std::str::FromStr; +use std::{ + fmt::{self, Display}, + str::FromStr, +}; #[component] fn Root() -> Element { @@ -75,3 +78,87 @@ fn without_trailing_slashes_parse() { RouteWithoutTrailingSlash::Dynamic { id: 123 } ); } + +// Regression test for https://github.com/DioxusLabs/dioxus/issues/2984 +#[test] +fn query_segments_parse() { + #[derive(Debug, Clone, PartialEq)] + enum Query { + Id(u64), + } + + impl From<&str> for Query { + fn from(_: &str) -> Self { + // e.g. split query on `&` and split pairs on `=` + Query::Id(10) + } + } + + impl Display for Query { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "id=10") + } + } + + #[component] + fn Index(query: Query) -> Element { + rsx! { + h1 { "Index" } + } + } + + #[derive(Debug, Clone, PartialEq, Routable)] + enum Route { + #[route("/?:..query")] + Index { query: Query }, + } + + let route = Route::Index { + query: Query::Id(10), + }; + assert_eq!(route.to_string(), "/?id=10"); + let parsed_route = "/?id=10".parse::().unwrap(); + assert_eq!(parsed_route, route); +} + +#[test] +fn optional_query_segments_parse() { + #[derive(Debug, Clone, PartialEq, Routable)] + enum Route { + #[route("/?:query&:other")] + Index { query: Option, other: u64 }, + } + + #[component] + fn Index(query: Option, other: u64) -> Element { + rsx! { + h1 { "Index" } + } + } + + let route = Route::Index { + query: Some(10), + other: 20, + }; + assert_eq!(route.to_string(), "/?query=10&other=20"); + let parsed_route = "/?query=10&other=20".parse::().unwrap(); + assert_eq!(parsed_route, route); + + let route_without_query = Route::Index { + query: None, + other: 20, + }; + assert_eq!(route_without_query.to_string(), "/?other=20"); + let parsed_route_without_query = "/?other=20".parse::().unwrap(); + assert_eq!(parsed_route_without_query, route_without_query); + let route_without_query_and_other = Route::Index { + query: None, + other: 0, + }; + assert_eq!(route_without_query_and_other.to_string(), "/?other=0"); + let parsed_route_without_query_and_other = "/".parse::().unwrap(); + assert_eq!( + parsed_route_without_query_and_other, + route_without_query_and_other + ); +}