From 401c7a324a905235f3dd0ef04c2db6161ec498e1 Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Wed, 9 Mar 2022 11:25:06 +0100 Subject: [PATCH] Fix routing for opaque nested services (#842) * Fix routing for opaque nested services * Also test `MatchedPath` * clean up and more comments --- axum/CHANGELOG.md | 4 ++ axum/src/routing/mod.rs | 4 +- axum/src/routing/strip_prefix.rs | 113 +++++++++++++++++++------------ axum/src/routing/tests/nest.rs | 77 ++++++++++++++++++--- 4 files changed, 141 insertions(+), 57 deletions(-) diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index a7ccf4ff..53e51f9d 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -70,6 +70,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 requests ([#734]) - **fixed:** Fix wrong `content-length` for `HEAD` requests to endpoints that returns chunked responses ([#755]) +- **fixed:** Fixed several routing bugs related to nested "opaque" tower services (i.e. + non-`Router` services) ([#841] and [#842]) - **changed:** Update to tokio-tungstenite 0.17 ([#791]) [#644]: https://github.com/tokio-rs/axum/pull/644 @@ -89,6 +91,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#819]: https://github.com/tokio-rs/axum/pull/819 [#823]: https://github.com/tokio-rs/axum/pull/823 [#824]: https://github.com/tokio-rs/axum/pull/824 +[#841]: https://github.com/tokio-rs/axum/pull/841 +[#842]: https://github.com/tokio-rs/axum/pull/842 # 0.4.4 (13. January, 2022) diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs index de24fc8b..8f38f0a1 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -237,8 +237,8 @@ where } // otherwise we add a wildcard route to the service Err(svc) => { - let path = if path == "/" { - format!("/*{}", NEST_TAIL_PARAM) + let path = if path.ends_with('/') { + format!("{}*{}", path, NEST_TAIL_PARAM) } else { format!("{}/*{}", path, NEST_TAIL_PARAM) }; diff --git a/axum/src/routing/strip_prefix.rs b/axum/src/routing/strip_prefix.rs index 42b41ace..25744843 100644 --- a/axum/src/routing/strip_prefix.rs +++ b/axum/src/routing/strip_prefix.rs @@ -42,62 +42,78 @@ where } fn strip_prefix(uri: &Uri, prefix: &str) -> Option { - let path_and_query: http::uri::PathAndQuery = if let Some(path_and_query) = uri.path_and_query() - { - // Check whether the prefix matches the path and if so how long the matching prefix is. - // - // # Examples - // - // prefix = /api - // uri = /api/users - // ^^^^ this much is matched and the length is 4. Thus if we chop off the first 4 - // characters we get the remainder - // - // prefix = /api/:version - // uri = /api/v0/users - // ^^^^^^^ this much is matched and the length is 7. - let mut matching_prefix_length = Some(0); - for item in zip_longest(segments(path_and_query.path()), segments(prefix)) { - // count the `/` - *matching_prefix_length.as_mut().unwrap() += 1; + let path_and_query = uri.path_and_query()?; - match item { - Item::Both(path_segment, prefix_segment) => { - if prefix_segment.starts_with(':') || path_segment == prefix_segment { - *matching_prefix_length.as_mut().unwrap() += path_segment.len(); - } else { - matching_prefix_length = None; - break; - } - } - Item::First(_) => { + // Check whether the prefix matches the path and if so how long the matching prefix is. + // + // For example: + // + // prefix = /api + // path = /api/users + // ^^^^ this much is matched and the length is 4. Thus if we chop off the first 4 + // characters we get the remainder + // + // prefix = /api/:version + // path = /api/v0/users + // ^^^^^^^ this much is matched and the length is 7. + let mut matching_prefix_length = Some(0); + for item in zip_longest(segments(path_and_query.path()), segments(prefix)) { + // count the `/` + *matching_prefix_length.as_mut().unwrap() += 1; + + match item { + Item::Both(path_segment, prefix_segment) => { + if prefix_segment.starts_with(':') || path_segment == prefix_segment { + // the prefix segment is either a param, which matches anything, or + // it actually matches the path segment + *matching_prefix_length.as_mut().unwrap() += path_segment.len(); + } else if prefix_segment.is_empty() { + // the prefix ended in a `/` so we got a match. + // + // For example: + // + // prefix = /foo/ + // path = /foo/bar + // + // The prefix matches and the new path should be `/bar` break; - } - Item::Second(_) => { + } else { + // the prefix segment didn't match so there is no match matching_prefix_length = None; break; } } + // the path had more segments than the prefix but we got a match. + // + // For example: + // + // prefix = /foo + // path = /foo/bar + Item::First(_) => { + break; + } + // the prefix had more segments than the path so there is no match + Item::Second(_) => { + matching_prefix_length = None; + break; + } } + } - let after_prefix = if let Some(idx) = matching_prefix_length { - uri.path().split_at(idx).1 - } else { - return None; - }; + // if the prefix matches it will always do so up until a `/`, it cannot match only + // part of a segment. Therefore this will always be at a char boundary and `split_at` wont + // panic + let after_prefix = uri.path().split_at(matching_prefix_length?).1; - match (after_prefix.starts_with('/'), path_and_query.query()) { - (true, None) => after_prefix.parse().unwrap(), - (true, Some(query)) => format!("{}?{}", after_prefix, query).parse().unwrap(), - (false, None) => format!("/{}", after_prefix).parse().unwrap(), - (false, Some(query)) => format!("/{}?{}", after_prefix, query).parse().unwrap(), - } - } else { - return None; + let new_path_and_query = match (after_prefix.starts_with('/'), path_and_query.query()) { + (true, None) => after_prefix.parse().unwrap(), + (true, Some(query)) => format!("{}?{}", after_prefix, query).parse().unwrap(), + (false, None) => format!("/{}", after_prefix).parse().unwrap(), + (false, Some(query)) => format!("/{}?{}", after_prefix, query).parse().unwrap(), }; let mut parts = uri.clone().into_parts(); - parts.path_and_query = Some(path_and_query); + parts.path_and_query = Some(new_path_and_query); Some(Uri::from_parts(parts).unwrap()) } @@ -183,7 +199,7 @@ mod tests { single_segment_root_prefix, uri = "/a", prefix = "/", - expected = None, + expected = Some("/a"), ); test!( @@ -334,6 +350,13 @@ mod tests { expected = Some("/"), ); + test!( + param_13, + uri = "/a/a", + prefix = "/a/", + expected = Some("/a"), + ); + #[quickcheck] fn does_not_panic(uri_and_prefix: UriAndPrefix) -> bool { let UriAndPrefix { uri, prefix } = uri_and_prefix; diff --git a/axum/src/routing/tests/nest.rs b/axum/src/routing/tests/nest.rs index a311186c..31149c94 100644 --- a/axum/src/routing/tests/nest.rs +++ b/axum/src/routing/tests/nest.rs @@ -1,8 +1,10 @@ -use tower_http::services::ServeDir; - use super::*; -use crate::{body::boxed, extract::Extension}; +use crate::{ + body::boxed, + extract::{Extension, MatchedPath}, +}; use std::collections::HashMap; +use tower_http::services::ServeDir; #[tokio::test] async fn nesting_apps() { @@ -369,19 +371,59 @@ macro_rules! nested_route_test { $name:ident, nest = $nested_path:literal, route = $route_path:literal, - expected = $expected_path:literal $(,)? + expected = $expected_path:literal, + opaque_redirect = $opaque_redirect:expr $(,)? ) => { #[tokio::test] async fn $name() { - let inner = Router::new().route($route_path, get(|| async {})); + let inner = Router::new().route( + $route_path, + get(|matched: MatchedPath| async move { matched.as_str().to_owned() }), + ); let app = Router::new().nest($nested_path, inner); let client = TestClient::new(app); - assert_eq!( - client.get($expected_path).send().await.status(), - StatusCode::OK - ); + let res = client.get($expected_path).send().await; + let status = res.status(); + let matched_path = res.text().await; + assert_eq!(status, StatusCode::OK, "Router"); + + let inner = Router::new() + .route( + $route_path, + get(|matched: MatchedPath| async move { matched.as_str().to_owned() }), + ) + .boxed_clone(); + let app = Router::new().nest($nested_path, inner); + let client = TestClient::new(app); + let res = client.get($expected_path).send().await; + if $opaque_redirect { + assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT, "opaque"); + + let location = res.headers()[http::header::LOCATION].to_str().unwrap(); + let res = client.get(location).send().await; + assert_eq!(res.status(), StatusCode::OK, "opaque with redirect"); + assert_eq!(res.text().await, location); + } else { + assert_eq!(res.status(), StatusCode::OK, "opaque"); + assert_eq!(res.text().await, matched_path); + } } }; + + ( + $name:ident, + nest = $nested_path:literal, + route = $route_path:literal, + expected = $expected_path:literal $(,)? + ) => { + nested_route_test!( + $name, + nest = $nested_path, + route = $route_path, + expected = $expected_path, + opaque_redirect = false, + ); + }; } // test cases taken from https://github.com/tokio-rs/axum/issues/714#issuecomment-1058144460 @@ -391,7 +433,22 @@ nested_route_test!(nest_3, nest = "", route = "/a/", expected = "/a/"); nested_route_test!(nest_4, nest = "/", route = "/", expected = "/"); nested_route_test!(nest_5, nest = "/", route = "/a", expected = "/a"); nested_route_test!(nest_6, nest = "/", route = "/a/", expected = "/a/"); -nested_route_test!(nest_7, nest = "/a", route = "/", expected = "/a"); + +// This case is different for opaque services. +// +// The internal route becomes `/a/*__private__axum_nest_tail_param` which, according to matchit +// doesn't match `/a`. However matchit detects that a route for `/a/` exists and so it issues a +// redirect to `/a/`, which ends up calling the inner route as expected. +// +// So while the behavior isn't identical, the outcome is the same +nested_route_test!( + nest_7, + nest = "/a", + route = "/", + expected = "/a", + opaque_redirect = true, +); + nested_route_test!(nest_8, nest = "/a", route = "/a", expected = "/a/a"); nested_route_test!(nest_9, nest = "/a", route = "/a/", expected = "/a/a/"); nested_route_test!(nest_11, nest = "/a/", route = "/", expected = "/a/");