Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow nesting at the root of the router #3051

Merged
merged 1 commit into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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