From 681c3edb8721142187c10c9764d9b52eeaeca188 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 11 Apr 2025 20:32:41 +0200 Subject: [PATCH 1/4] Keep arguments in redirections --- src/web/builds.rs | 6 +++--- src/web/crate_details.rs | 40 ++++++++++++++++++++++--------------- src/web/error.rs | 34 ++++++++++++++++++++++++------- src/web/features.rs | 4 ++-- src/web/rustdoc.rs | 43 ++++++++++++++++++++++++++-------------- src/web/source.rs | 16 ++++++++++----- src/web/status.rs | 4 ++-- 7 files changed, 97 insertions(+), 50 deletions(-) diff --git a/src/web/builds.rs b/src/web/builds.rs index b3fc93618..d01756ef6 100644 --- a/src/web/builds.rs +++ b/src/web/builds.rs @@ -10,7 +10,7 @@ use crate::{ impl_axum_webpage, web::{ MetaData, ReqVersion, - error::AxumResult, + error::{AxumResult, EscapedURI}, extractors::{DbConnection, Path}, filters, match_version, page::templates::{RenderRegular, RenderSolid}, @@ -69,7 +69,7 @@ pub(crate) async fn build_list_handler( .assume_exact_name()? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{name}/{version}/builds"), + EscapedURI::new(&format!("/crate/{name}/{version}/builds"), None), CachePolicy::ForeverInCdn, ) })? @@ -93,7 +93,7 @@ pub(crate) async fn build_list_json_handler( .assume_exact_name()? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{name}/{version}/builds.json"), + EscapedURI::new(&format!("/crate/{name}/{version}/builds.json"), None), CachePolicy::ForeverInCdn, ) })? diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 517e2332c..2fa605450 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -10,8 +10,7 @@ use crate::{ web::{ MatchedRelease, ReqVersion, cache::CachePolicy, - encode_url_path, - error::{AxumNope, AxumResult}, + error::{AxumNope, AxumResult, EscapedURI}, extractors::{DbConnection, Path}, page::templates::{RenderRegular, RenderSolid, filters}, rustdoc::RustdocHtmlParams, @@ -475,7 +474,10 @@ pub(crate) async fn crate_details_handler( ) -> AxumResult { let req_version = params.version.ok_or_else(|| { AxumNope::Redirect( - format!("/crate/{}/{}", ¶ms.name, ReqVersion::Latest), + EscapedURI::new( + &format!("/crate/{}/{}", ¶ms.name, ReqVersion::Latest), + None, + ), CachePolicy::ForeverInCdn, ) })?; @@ -485,7 +487,7 @@ pub(crate) async fn crate_details_handler( .assume_exact_name()? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{}/{}", ¶ms.name, version), + EscapedURI::new(&format!("/crate/{}/{}", ¶ms.name, version), None), CachePolicy::ForeverInCdn, ) })?; @@ -694,23 +696,29 @@ pub(crate) async fn get_all_platforms_inner( .await? .into_exactly_named_or_else(|corrected_name, req_version| { AxumNope::Redirect( - encode_url_path(&format!( - "/platforms/{}/{}/{}", - corrected_name, - req_version, - req_path.join("/") - )), + EscapedURI::new( + &format!( + "/platforms/{}/{}/{}", + corrected_name, + req_version, + req_path.join("/") + ), + None, + ), CachePolicy::NoCaching, ) })? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - encode_url_path(&format!( - "/platforms/{}/{}/{}", - ¶ms.name, - version, - req_path.join("/") - )), + EscapedURI::new( + &format!( + "/platforms/{}/{}/{}", + ¶ms.name, + version, + req_path.join("/") + ), + None, + ), CachePolicy::ForeverInCdn, ) })?; diff --git a/src/web/error.rs b/src/web/error.rs index ce254ef16..8b06b9604 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -14,6 +14,24 @@ use tracing::error; use super::AxumErrorPage; +#[derive(Debug)] +pub struct EscapedURI(String); + +impl EscapedURI { + pub fn new(path: &str, query: Option<&str>) -> Self { + let mut path = encode_url_path(path); + if let Some(query) = query { + path.push('?'); + path.push_str(query); + } + Self(path) + } + + pub fn as_str(&self) -> &str { + self.0.as_str() + } +} + #[derive(Debug, thiserror::Error)] pub enum AxumNope { #[error("Requested resource not found")] @@ -35,7 +53,7 @@ pub enum AxumNope { #[error("bad request")] BadRequest(anyhow::Error), #[error("redirect")] - Redirect(String, CachePolicy), + Redirect(EscapedURI, CachePolicy), } // FUTURE: Ideally, the split between the 3 kinds of responses would @@ -118,8 +136,8 @@ struct ErrorInfo { pub status: StatusCode, } -fn redirect_with_policy(target: String, cache_policy: CachePolicy) -> AxumResponse { - match super::axum_cached_redirect(encode_url_path(&target), cache_policy) { +fn redirect_with_policy(target: EscapedURI, cache_policy: CachePolicy) -> AxumResponse { + match super::axum_cached_redirect(target.0, cache_policy) { Ok(response) => response.into_response(), Err(err) => AxumNope::InternalError(err).into_response(), } @@ -215,16 +233,18 @@ pub(crate) type JsonAxumResult = Result; #[cfg(test)] mod tests { - use super::{AxumNope, IntoResponse}; + use super::{AxumNope, EscapedURI, IntoResponse}; use crate::test::{AxumResponseTestExt, AxumRouterTestExt, async_wrapper}; use crate::web::cache::CachePolicy; use kuchikiki::traits::TendrilSink; #[test] fn test_redirect_error_encodes_url_path() { - let response = - AxumNope::Redirect("/something>".into(), CachePolicy::ForeverInCdnAndBrowser) - .into_response(); + let response = AxumNope::Redirect( + EscapedURI::new("/something>", None), + CachePolicy::ForeverInCdnAndBrowser, + ) + .into_response(); assert_eq!(response.status(), 302); assert_eq!(response.headers().get("Location").unwrap(), "/something%3E"); diff --git a/src/web/features.rs b/src/web/features.rs index 709c3352b..2b961b40e 100644 --- a/src/web/features.rs +++ b/src/web/features.rs @@ -4,7 +4,7 @@ use crate::{ web::{ MetaData, ReqVersion, cache::CachePolicy, - error::{AxumNope, AxumResult}, + error::{AxumNope, AxumResult, EscapedURI}, extractors::{DbConnection, Path}, filters, headers::CanonicalUrl, @@ -127,7 +127,7 @@ pub(crate) async fn build_features_handler( .assume_exact_name()? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{}/{}/features", &name, version), + EscapedURI::new(&format!("/crate/{}/{}/features", &name, version), None), CachePolicy::ForeverInCdn, ) })? diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index a019bc65a..fa17ab233 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -11,7 +11,7 @@ use crate::{ crate_details::CrateDetails, csp::Csp, encode_url_path, - error::{AxumNope, AxumResult}, + error::{AxumNope, AxumResult, EscapedURI}, extractors::{DbConnection, Path}, file::File, match_version, @@ -362,6 +362,13 @@ pub(crate) async fn rustdoc_html_server_handler( // Pages generated by Rustdoc are not ready to be served with a CSP yet. csp.suppress(true); + fn get_query_and_fragment(uri: &Uri) -> Option<&str> { + // We cannot extract the fragment (`#`) with current `Uri` API so forced to do ugly + // things like that... + uri.path_and_query() + .and_then(|path_and_query| path_and_query.as_str().splitn(2, '?').nth(1)) + } + // Convenience function to allow for easy redirection #[instrument] fn redirect( @@ -369,11 +376,12 @@ pub(crate) async fn rustdoc_html_server_handler( vers: &Version, path: &[&str], cache_policy: CachePolicy, + uri: &Uri, ) -> AxumResult { trace!("redirect"); - // Format and parse the redirect url + let query = get_query_and_fragment(&uri); Ok(axum_cached_redirect( - encode_url_path(&format!("/{}/{}/{}", name, vers, path.join("/"))), + EscapedURI::new(&format!("/{}/{}/{}", name, vers, path.join("/")), query).as_str(), cache_policy, )? .into_response()) @@ -391,24 +399,21 @@ pub(crate) async fn rustdoc_html_server_handler( let matched_release = match_version(&mut conn, ¶ms.name, ¶ms.version) .await? .into_exactly_named_or_else(|corrected_name, req_version| { + let query = get_query_and_fragment(&uri); AxumNope::Redirect( - encode_url_path(&format!( - "/{}/{}/{}", - corrected_name, - req_version, - req_path.join("/") - )), + EscapedURI::new( + &format!("/{}/{}/{}", corrected_name, req_version, req_path.join("/")), + query, + ), CachePolicy::NoCaching, ) })? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - encode_url_path(&format!( - "/{}/{}/{}", - ¶ms.name, - version, - req_path.join("/") - )), + EscapedURI::new( + &format!("/{}/{}/{}", ¶ms.name, version, req_path.join("/")), + None, + ), CachePolicy::ForeverInCdn, ) })?; @@ -439,6 +444,7 @@ pub(crate) async fn rustdoc_html_server_handler( &krate.version, &req_path[1..], CachePolicy::ForeverInCdn, + &uri, ); } @@ -494,6 +500,7 @@ pub(crate) async fn rustdoc_html_server_handler( &krate.version, &req_path, CachePolicy::ForeverInCdn, + &uri, ); } } @@ -2004,6 +2011,12 @@ mod test { "/foo-ab/0.0.1/foo_ab/", ) .await?; + // `-` becomes `_` but we keep the query arguments. + web.assert_redirect_unchecked( + "/foo-ab/0.0.1/foo_ab/?search=a", + "/foo_ab/0.0.1/foo_ab/?search=a", + ) + .await?; Ok(()) }) } diff --git a/src/web/source.rs b/src/web/source.rs index f8192cf03..730f43725 100644 --- a/src/web/source.rs +++ b/src/web/source.rs @@ -7,7 +7,7 @@ use crate::{ web::{ MetaData, ReqVersion, cache::CachePolicy, - error::AxumNope, + error::{AxumNope, EscapedURI}, extractors::Path, file::File as DbFile, headers::CanonicalUrl, @@ -203,16 +203,22 @@ pub(crate) async fn source_browser_handler( .await? .into_exactly_named_or_else(|corrected_name, req_version| { AxumNope::Redirect( - format!( - "/crate/{corrected_name}/{req_version}/source/{}", - params.path + EscapedURI::new( + &format!( + "/crate/{corrected_name}/{req_version}/source/{}", + params.path + ), + None, ), CachePolicy::NoCaching, ) })? .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{}/{version}/source/{}", params.name, params.path), + EscapedURI::new( + &format!("/crate/{}/{version}/source/{}", params.name, params.path), + None, + ), CachePolicy::ForeverInCdn, ) })? diff --git a/src/web/status.rs b/src/web/status.rs index efda38af3..563fc1e64 100644 --- a/src/web/status.rs +++ b/src/web/status.rs @@ -1,7 +1,7 @@ use super::{cache::CachePolicy, error::AxumNope}; use crate::web::{ ReqVersion, - error::AxumResult, + error::{AxumResult, EscapedURI}, extractors::{DbConnection, Path}, match_version, }; @@ -28,7 +28,7 @@ pub(crate) async fn status_handler( let version = matched_release .into_canonical_req_version_or_else(|version| { AxumNope::Redirect( - format!("/crate/{name}/{version}/status.json"), + EscapedURI::new(&format!("/crate/{name}/{version}/status.json"), None), CachePolicy::NoCaching, ) })? From e78f44e8ed3cc99995fad873a940586f07c3ada4 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 11 Apr 2025 21:34:42 +0200 Subject: [PATCH 2/4] Fix clippy lints --- src/web/rustdoc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index fa17ab233..c2c670202 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -366,7 +366,7 @@ pub(crate) async fn rustdoc_html_server_handler( // We cannot extract the fragment (`#`) with current `Uri` API so forced to do ugly // things like that... uri.path_and_query() - .and_then(|path_and_query| path_and_query.as_str().splitn(2, '?').nth(1)) + .and_then(|path_and_query| path_and_query.as_str().split_once('?').map(|s| s.1)) } // Convenience function to allow for easy redirection @@ -379,7 +379,7 @@ pub(crate) async fn rustdoc_html_server_handler( uri: &Uri, ) -> AxumResult { trace!("redirect"); - let query = get_query_and_fragment(&uri); + let query = get_query_and_fragment(uri); Ok(axum_cached_redirect( EscapedURI::new(&format!("/{}/{}/{}", name, vers, path.join("/")), query).as_str(), cache_policy, From f085c5c13962bd48e5e081d2b5cf0153259e0857 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 12 Apr 2025 11:33:53 +0200 Subject: [PATCH 3/4] Remove `get_query_and_fragment` function and only get `query` --- src/web/rustdoc.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index c2c670202..3f2772b2e 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -362,13 +362,6 @@ pub(crate) async fn rustdoc_html_server_handler( // Pages generated by Rustdoc are not ready to be served with a CSP yet. csp.suppress(true); - fn get_query_and_fragment(uri: &Uri) -> Option<&str> { - // We cannot extract the fragment (`#`) with current `Uri` API so forced to do ugly - // things like that... - uri.path_and_query() - .and_then(|path_and_query| path_and_query.as_str().split_once('?').map(|s| s.1)) - } - // Convenience function to allow for easy redirection #[instrument] fn redirect( @@ -379,9 +372,12 @@ pub(crate) async fn rustdoc_html_server_handler( uri: &Uri, ) -> AxumResult { trace!("redirect"); - let query = get_query_and_fragment(uri); Ok(axum_cached_redirect( - EscapedURI::new(&format!("/{}/{}/{}", name, vers, path.join("/")), query).as_str(), + EscapedURI::new( + &format!("/{}/{}/{}", name, vers, path.join("/")), + uri.query(), + ) + .as_str(), cache_policy, )? .into_response()) @@ -399,11 +395,10 @@ pub(crate) async fn rustdoc_html_server_handler( let matched_release = match_version(&mut conn, ¶ms.name, ¶ms.version) .await? .into_exactly_named_or_else(|corrected_name, req_version| { - let query = get_query_and_fragment(&uri); AxumNope::Redirect( EscapedURI::new( &format!("/{}/{}/{}", corrected_name, req_version, req_path.join("/")), - query, + uri.query(), ), CachePolicy::NoCaching, ) From a9f794db21ba45de42cc7ffaf7f8c6f34226d0bf Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 15 Apr 2025 00:47:00 +0200 Subject: [PATCH 4/4] Add missing redirection in `rustdoc_redirector_handler` --- src/web/rustdoc.rs | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index 3f2772b2e..a725e706a 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -253,7 +253,11 @@ pub(crate) async fn rustdoc_redirector_handler( .into_response()) } else { Ok(axum_cached_redirect( - format!("/crate/{crate_name}/{}", matched_release.req_version), + EscapedURI::new( + &format!("/crate/{crate_name}/{}", matched_release.req_version), + uri.query(), + ) + .as_str(), CachePolicy::ForeverInCdn, )? .into_response()) @@ -2983,4 +2987,26 @@ mod test { Ok(()) }) } + + #[test_case(true)] + #[test_case(false)] + fn test_redirect_with_query_args(archive_storage: bool) { + async_wrapper(|env| async move { + env.fake_release() + .await + .name("fake") + .version("0.0.1") + .archive_storage(archive_storage) + .rustdoc_file("fake/index.html") + .binary(true) // binary => rustdoc_status = false + .create() + .await?; + + let web = env.web_app().await; + web.assert_redirect("/fake?a=b", "/crate/fake/latest?a=b") + .await?; + + Ok(()) + }); + } }