Skip to content

Commit

Permalink
Disallow nesting at the root of the router (#3051)
Browse files Browse the repository at this point in the history
  • Loading branch information
jplatte authored Nov 26, 2024
1 parent fb1b8f1 commit 11806fb
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 126 deletions.
4 changes: 2 additions & 2 deletions axum/src/extract/matched_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,14 +324,14 @@ mod tests {
"/{*path}",
any(|req: Request| {
Router::new()
.nest("/", Router::new().route("/foo", get(|| async {})))
.nest("/foo", Router::new().route("/bar", get(|| async {})))
.oneshot(req)
}),
);

let client = TestClient::new(app);

let res = client.get("/foo").await;
let res = client.get("/foo/bar").await;
assert_eq!(res.status(), StatusCode::OK);
}

Expand Down
36 changes: 0 additions & 36 deletions axum/src/extract/nested_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,42 +191,6 @@ mod tests {
assert_eq!(res.status(), StatusCode::OK);
}

#[crate::test]
async fn nested_at_root() {
let api = Router::new().route(
"/users",
get(|nested_path: NestedPath| {
assert_eq!(nested_path.as_str(), "/");
async {}
}),
);

let app = Router::new().nest("/", api);

let client = TestClient::new(app);

let res = client.get("/users").await;
assert_eq!(res.status(), StatusCode::OK);
}

#[crate::test]
async fn deeply_nested_from_root() {
let api = Router::new().route(
"/users",
get(|nested_path: NestedPath| {
assert_eq!(nested_path.as_str(), "/api");
async {}
}),
);

let app = Router::new().nest("/", Router::new().nest("/api", api));

let client = TestClient::new(app);

let res = client.get("/api/users").await;
assert_eq!(res.status(), StatusCode::OK);
}

#[crate::test]
async fn in_fallbacks() {
let api = Router::new().fallback(get(|nested_path: NestedPath| {
Expand Down
8 changes: 8 additions & 0 deletions axum/src/routing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ where
#[doc(alias = "scope")] // Some web frameworks like actix-web use this term
#[track_caller]
pub fn nest(self, path: &str, router: Router<S>) -> Self {
if path.is_empty() || path == "/" {
panic!("Nesting at the root is no longer supported. Use merge instead.");
}

let RouterInner {
path_router,
fallback_router,
Expand Down Expand Up @@ -227,6 +231,10 @@ where
T::Response: IntoResponse,
T::Future: Send + 'static,
{
if path.is_empty() || path == "/" {
panic!("Nesting at the root is no longer supported. Use fallback_service instead.");
}

tap_inner!(self, mut this => {
panic_on_err!(this.path_router.nest_service(path, service));
})
Expand Down
6 changes: 2 additions & 4 deletions axum/src/routing/path_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,10 +515,8 @@ impl fmt::Debug for Node {

#[track_caller]
fn validate_nest_path(v7_checks: bool, path: &str) -> &str {
if path.is_empty() {
// nesting at `""` and `"/"` should mean the same thing
return "/";
}
assert!(path.starts_with('/'));
assert!(path.len() > 1);

if path.split('/').any(|segment| {
segment.starts_with("{*") && segment.ends_with('}') && !segment.ends_with("}}")
Expand Down
4 changes: 2 additions & 2 deletions axum/src/routing/tests/fallback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,13 @@ async fn doesnt_panic_if_used_with_nested_router() {
async fn handler() {}

let routes_static =
Router::new().nest_service("/", crate::routing::get_service(handler.into_service()));
Router::new().nest_service("/foo", crate::routing::get_service(handler.into_service()));

let routes_all = Router::new().fallback_service(routes_static);

let client = TestClient::new(routes_all);

let res = client.get("/foobar").await;
let res = client.get("/foo/bar").await;
assert_eq!(res.status(), StatusCode::OK);
}

Expand Down
6 changes: 3 additions & 3 deletions axum/src/routing/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,21 +600,21 @@ async fn head_with_middleware_applied() {

let app = Router::new()
.nest(
"/",
"/foo",
Router::new().route("/", get(|| async { "Hello, World!" })),
)
.layer(CompressionLayer::new().compress_when(SizeAbove::new(0)));

let client = TestClient::new(app);

// send GET request
let res = client.get("/").header("accept-encoding", "gzip").await;
let res = client.get("/foo").header("accept-encoding", "gzip").await;
assert_eq!(res.headers()["transfer-encoding"], "chunked");
// cannot have `transfer-encoding: chunked` and `content-length`
assert!(!res.headers().contains_key("content-length"));

// send HEAD request
let res = client.head("/").header("accept-encoding", "gzip").await;
let res = client.head("/foo").header("accept-encoding", "gzip").await;
// no response body so no `transfer-encoding`
assert!(!res.headers().contains_key("transfer-encoding"));
// no content-length since we cannot know it since the response
Expand Down
112 changes: 33 additions & 79 deletions axum/src/routing/tests/nest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,74 +60,49 @@ async fn nesting_apps() {
#[crate::test]
async fn wrong_method_nest() {
let nested_app = Router::new().route("/", get(|| async {}));
let app = Router::new().nest("/", nested_app);
let app = Router::new().nest("/foo", nested_app);

let client = TestClient::new(app);

let res = client.get("/").await;
let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.post("/").await;
let res = client.post("/foo").await;
assert_eq!(res.status(), StatusCode::METHOD_NOT_ALLOWED);
assert_eq!(res.headers()[ALLOW], "GET,HEAD");

let res = client.patch("/foo").await;
let res = client.patch("/foo/bar").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}

#[crate::test]
async fn nesting_router_at_root() {
let nested = Router::new().route("/foo", get(|uri: Uri| async move { uri.to_string() }));
let app = Router::new().nest("/", nested);

let client = TestClient::new(app);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/foo");

let res = client.get("/foo/bar").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
#[test]
#[should_panic(expected = "Nesting at the root is no longer supported. Use merge instead.")]
fn nest_router_at_root() {
let nested = Router::new().route("/foo", get(|| async {}));
let _: Router = Router::new().nest("/", nested);
}

#[crate::test]
async fn nesting_router_at_empty_path() {
let nested = Router::new().route("/foo", get(|uri: Uri| async move { uri.to_string() }));
let app = Router::new().nest("", nested);

let client = TestClient::new(app);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/foo");

let res = client.get("/foo/bar").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
#[test]
#[should_panic(expected = "Nesting at the root is no longer supported. Use merge instead.")]
fn nest_router_at_empty_path() {
let nested = Router::new().route("/foo", get(|| async {}));
let _: Router = Router::new().nest("", nested);
}

#[crate::test]
async fn nesting_handler_at_root() {
let app = Router::new().nest_service("/", get(|uri: Uri| async move { uri.to_string() }));

let client = TestClient::new(app);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/");

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/foo");
#[test]
#[should_panic(
expected = "Nesting at the root is no longer supported. Use fallback_service instead."
)]
fn nest_service_at_root() {
let _: Router = Router::new().nest_service("/", get(|| async {}));
}

let res = client.get("/foo/bar").await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/foo/bar");
#[test]
#[should_panic(
expected = "Nesting at the root is no longer supported. Use fallback_service instead."
)]
fn nest_service_at_empty_path() {
let _: Router = Router::new().nest_service("", get(|| async {}));
}

#[crate::test]
Expand Down Expand Up @@ -227,21 +202,6 @@ async fn nested_multiple_routes() {
assert_eq!(client.get("/api/teams").await.text().await, "teams");
}

#[test]
#[should_panic = r#"Invalid route "/": Insertion failed due to conflict with previously registered route: /"#]
fn nested_service_at_root_with_other_routes() {
let _: Router = Router::new()
.nest_service("/", Router::new().route("/users", get(|| async {})))
.route("/", get(|| async {}));
}

#[test]
fn nested_at_root_with_other_routes() {
let _: Router = Router::new()
.nest("/", Router::new().route("/users", get(|| async {})))
.route("/", get(|| async {}));
}

#[crate::test]
async fn multiple_top_level_nests() {
let app = Router::new()
Expand Down Expand Up @@ -405,18 +365,12 @@ macro_rules! nested_route_test {
}

// test cases taken from https://github.com/tokio-rs/axum/issues/714#issuecomment-1058144460
nested_route_test!(nest_1, nest = "", route = "/", expected = "/");
nested_route_test!(nest_2, nest = "", route = "/a", expected = "/a");
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");
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/");
nested_route_test!(nest_12, nest = "/a/", route = "/a", expected = "/a/a");
nested_route_test!(nest_13, nest = "/a/", route = "/a/", expected = "/a/a/");
nested_route_test!(nest_1, nest = "/a", route = "/", expected = "/a");
nested_route_test!(nest_2, nest = "/a", route = "/a", expected = "/a/a");
nested_route_test!(nest_3, nest = "/a", route = "/a/", expected = "/a/a/");
nested_route_test!(nest_4, nest = "/a/", route = "/", expected = "/a/");
nested_route_test!(nest_5, nest = "/a/", route = "/a", expected = "/a/a");
nested_route_test!(nest_6, nest = "/a/", route = "/a/", expected = "/a/a/");

#[crate::test]
#[should_panic(
Expand Down

0 comments on commit 11806fb

Please sign in to comment.