diff --git a/core/codegen/src/attribute/route/parse.rs b/core/codegen/src/attribute/route/parse.rs index 88895161..59cbd4e3 100644 --- a/core/codegen/src/attribute/route/parse.rs +++ b/core/codegen/src/attribute/route/parse.rs @@ -77,11 +77,9 @@ impl FromMeta for RouteUri { .help("expected URI in origin form: \"/path/\"") })?; - if !origin.is_normalized_nontrailing() { - let normalized = origin.clone().into_normalized_nontrailing(); + if !origin.is_normalized() { + let normalized = origin.clone().into_normalized(); let span = origin.path().find("//") - .or_else(|| origin.has_trailing_slash() - .then_some(origin.path().len() - 1)) .or_else(|| origin.query() .and_then(|q| q.find("&&")) .map(|i| origin.path().len() + 1 + i)) diff --git a/core/codegen/src/bang/uri_parsing.rs b/core/codegen/src/bang/uri_parsing.rs index e6ae1e23..d7c772dc 100644 --- a/core/codegen/src/bang/uri_parsing.rs +++ b/core/codegen/src/bang/uri_parsing.rs @@ -309,7 +309,7 @@ impl Parse for InternalUriParams { // Validation should always succeed since this macro can only be called // if the route attribute succeeded, implying a valid route URI. let route_uri = Origin::parse_route(&route_uri_str) - .map(|o| o.into_normalized_nontrailing().into_owned()) + .map(|o| o.into_normalized().into_owned()) .map_err(|_| input.error("internal error: invalid route URI"))?; let content; diff --git a/core/codegen/tests/route.rs b/core/codegen/tests/route.rs index 6fe269aa..d8e45094 100644 --- a/core/codegen/tests/route.rs +++ b/core/codegen/tests/route.rs @@ -338,8 +338,9 @@ fn test_inclusive_segments() { assert_eq!(get("//a/"), "empty+a/"); assert_eq!(get("//a//"), "empty+a/"); assert_eq!(get("//a//c/d"), "empty+a/c/d"); + assert_eq!(get("//a/b"), "empty+a/b"); - assert_eq!(get("//a/b"), "nonempty+"); + assert_eq!(get("//a/b/"), "nonempty+"); assert_eq!(get("//a/b/c"), "nonempty+c"); assert_eq!(get("//a/b//c"), "nonempty+c"); assert_eq!(get("//a//b////c"), "nonempty+c"); diff --git a/core/codegen/tests/ui-fail-nightly/route-path-bad-syntax.stderr b/core/codegen/tests/ui-fail-nightly/route-path-bad-syntax.stderr index f322538b..a592cc86 100644 --- a/core/codegen/tests/ui-fail-nightly/route-path-bad-syntax.stderr +++ b/core/codegen/tests/ui-fail-nightly/route-path-bad-syntax.stderr @@ -52,7 +52,7 @@ error: route URIs cannot contain empty segments 23 | #[get("/a/b//")] | ^^ | - = note: expected "/a/b", found "/a/b//" + = note: expected "/a/b/", found "/a/b//" error: unused parameter --> tests/ui-fail-nightly/route-path-bad-syntax.rs:42:10 @@ -240,27 +240,3 @@ warning: `segment` starts with `<` but does not end with `>` | ^^^^^^^^ | = help: perhaps you meant the dynamic parameter ``? - -error: route URIs cannot contain empty segments - --> tests/ui-fail-nightly/route-path-bad-syntax.rs:107:10 - | -107 | #[get("/a/")] - | ^^ - | - = note: expected "/a", found "/a/" - -error: route URIs cannot contain empty segments - --> tests/ui-fail-nightly/route-path-bad-syntax.rs:110:12 - | -110 | #[get("/a/b/")] - | ^^ - | - = note: expected "/a/b", found "/a/b/" - -error: route URIs cannot contain empty segments - --> tests/ui-fail-nightly/route-path-bad-syntax.rs:113:14 - | -113 | #[get("/a/b/c/")] - | ^^ - | - = note: expected "/a/b/c", found "/a/b/c/" diff --git a/core/codegen/tests/ui-fail-stable/route-path-bad-syntax.stderr b/core/codegen/tests/ui-fail-stable/route-path-bad-syntax.stderr index bc4b8ccc..7263aa20 100644 --- a/core/codegen/tests/ui-fail-stable/route-path-bad-syntax.stderr +++ b/core/codegen/tests/ui-fail-stable/route-path-bad-syntax.stderr @@ -41,7 +41,7 @@ error: route URIs cannot contain empty segments | ^^^^^^^^^ error: route URIs cannot contain empty segments - --- note: expected "/a/b", found "/a/b//" + --- note: expected "/a/b/", found "/a/b//" --> tests/ui-fail-stable/route-path-bad-syntax.rs:23:7 | 23 | #[get("/a/b//")] @@ -180,24 +180,3 @@ error: parameters cannot be empty | 93 | #[get("/<>")] | ^^^^^ - -error: route URIs cannot contain empty segments - --- note: expected "/a", found "/a/" - --> tests/ui-fail-stable/route-path-bad-syntax.rs:107:7 - | -107 | #[get("/a/")] - | ^^^^^ - -error: route URIs cannot contain empty segments - --- note: expected "/a/b", found "/a/b/" - --> tests/ui-fail-stable/route-path-bad-syntax.rs:110:7 - | -110 | #[get("/a/b/")] - | ^^^^^^^ - -error: route URIs cannot contain empty segments - --- note: expected "/a/b/c", found "/a/b/c/" - --> tests/ui-fail-stable/route-path-bad-syntax.rs:113:7 - | -113 | #[get("/a/b/c/")] - | ^^^^^^^^^ diff --git a/core/lib/src/fairing/ad_hoc.rs b/core/lib/src/fairing/ad_hoc.rs index f4ea6e1b..4a4d75ee 100644 --- a/core/lib/src/fairing/ad_hoc.rs +++ b/core/lib/src/fairing/ad_hoc.rs @@ -242,6 +242,84 @@ impl AdHoc { Ok(rocket.manage(app_config)) }) } + + /// Constructs an `AdHoc` request fairing that strips trailing slashes from + /// all URIs in all incoming requests. + /// + /// The fairing returned by this method is intended largely for applications + /// that migrated from Rocket v0.4 to Rocket v0.5. In Rocket v0.4, requests + /// with a trailing slash in the URI were treated as if the trailing slash + /// were not present. For example, the request URI `/foo/` would match the + /// route `/` with `a = foo`. If the application depended on this + /// behavior, say by using URIs with previously innocuous trailing slashes + /// in an external application, requests will not be routed as expected. + /// + /// This fairing resolves this issue by stripping a trailing slash, if any, + /// in all incoming URIs. When it does so, it logs a warning. It is + /// recommended to use this fairing as a stop-gap measure instead of a + /// permanent resolution, if possible. + // + /// # Example + /// + /// With the fairing attached, request URIs have a trailing slash stripped: + /// + /// ```rust + /// # #[macro_use] extern crate rocket; + /// use rocket::local::blocking::Client; + /// use rocket::fairing::AdHoc; + /// + /// #[get("/")] + /// fn foo(param: &str) -> &str { + /// param + /// } + /// + /// #[launch] + /// fn rocket() -> _ { + /// rocket::build() + /// .mount("/", routes![foo]) + /// .attach(AdHoc::uri_normalizer()) + /// } + /// + /// # let client = Client::debug(rocket()).unwrap(); + /// let response = client.get("/bar/").dispatch(); + /// assert_eq!(response.into_string().unwrap(), "bar"); + /// ``` + /// + /// Without it, request URIs are unchanged and routed normally: + /// + /// ```rust + /// # #[macro_use] extern crate rocket; + /// use rocket::local::blocking::Client; + /// use rocket::fairing::AdHoc; + /// + /// #[get("/")] + /// fn foo(param: &str) -> &str { + /// param + /// } + /// + /// #[launch] + /// fn rocket() -> _ { + /// rocket::build().mount("/", routes![foo]) + /// } + /// + /// # let client = Client::debug(rocket()).unwrap(); + /// let response = client.get("/bar/").dispatch(); + /// assert!(response.status().class().is_client_error()); + /// + /// let response = client.get("/bar").dispatch(); + /// assert_eq!(response.into_string().unwrap(), "bar"); + /// ``` + #[deprecated(since = "0.6", note = "routing from Rocket v0.5 is now standard")] + pub fn uri_normalizer() -> AdHoc { + AdHoc::on_request("URI Normalizer", |req, _| Box::pin(async move { + if !req.uri().is_normalized_nontrailing() { + let normal = req.uri().clone().into_normalized_nontrailing(); + warn!("Incoming request URI was normalized for compatibility."); + info_!("{} -> {}", req.uri(), normal); + req.set_uri(normal); + } + })) + } } #[crate::async_trait] diff --git a/core/lib/src/fs/server.rs b/core/lib/src/fs/server.rs index e806a932..267c4191 100644 --- a/core/lib/src/fs/server.rs +++ b/core/lib/src/fs/server.rs @@ -24,9 +24,8 @@ use crate::fs::NamedFile; /// /// # Example /// -/// To serve files from the `/static` directory on the local file system at the -/// `/public` path, allowing `index.html` files to be used to respond to -/// requests for a directory (the default), you might write the following: +/// Serve files from the `/static` directory on the local file system at the +/// `/public` path with the [default options](#impl-Default): /// /// ```rust,no_run /// # #[macro_use] extern crate rocket; @@ -38,18 +37,18 @@ use crate::fs::NamedFile; /// } /// ``` /// -/// With this, requests for files at `/public/` will be handled by -/// returning the contents of `/static/`. Requests for _directories_ at +/// Requests for files at `/public/` will be handled by returning the +/// contents of `/static/`. Requests for _directories_ at /// `/public/` will be handled by returning the contents of /// `/static//index.html`. /// /// ## Relative Paths /// /// In the example above, `/static` is an absolute path. If your static files -/// are stored relative to your crate and your project is managed by Rocket, use -/// the [`relative!`] macro to obtain a path that is relative to your -/// crate's root. For example, to serve files in the `static` subdirectory of -/// your crate at `/`, you might write: +/// are stored relative to your crate and your project is managed by Cargo, use +/// the [`relative!`] macro to obtain a path that is relative to your crate's +/// root. For example, to serve files in the `static` subdirectory of your crate +/// at `/`, you might write: /// /// ```rust,no_run /// # #[macro_use] extern crate rocket; @@ -263,8 +262,8 @@ pub struct Options(u8); impl Options { /// All options disabled. /// - /// This is different than [`Options::default()`](#impl-Default), which - /// enables `Options::Index`. + /// Note that this is different than [`Options::default()`](#impl-Default), + /// which enables options. pub const None: Options = Options(0); /// Respond to requests for a directory with the `index.html` file in that @@ -289,14 +288,14 @@ impl Options { /// Normalizes directory requests by redirecting requests to directory paths /// without a trailing slash to ones with a trailing slash. /// + /// **Enabled by default.** + /// /// When enabled, the [`FileServer`] handler will respond to requests for a /// directory without a trailing `/` with a permanent redirect (308) to the /// same path with a trailing `/`. This ensures relative URLs within any /// document served from that directory will be interpreted relative to that /// directory rather than its parent. /// - /// **Disabled by default.** - /// /// # Example /// /// Given the following directory structure... @@ -308,14 +307,21 @@ impl Options { /// └── index.html /// ``` /// - /// ...with `FileServer::from("static")`, both requests to `/foo` and - /// `/foo/` will serve `static/foo/index.html`. If `index.html` references - /// `cat.jpeg` as a relative URL, the browser will request `/cat.jpeg` - /// (`static/cat.jpeg`) when the request for `/foo` was handled and - /// `/foo/cat.jpeg` (`static/foo/cat.jpeg`) if `/foo/` was handled. As a - /// result, the request in the former case will fail. To avoid this, - /// `NormalizeDirs` will redirect requests to `/foo` to `/foo/` if the file - /// that would be served is a directory. + /// And the following server: + /// + /// ```text + /// rocket.mount("/", FileServer::from("static")) + /// ``` + /// + /// ...requests to `example.com/foo` will be redirected to + /// `example.com/foo/`. If `index.html` references `cat.jpeg` as a relative + /// URL, the browser will resolve the URL to `example.com/foo/cat.jpeg`, + /// which in-turn Rocket will match to `/static/foo/cat.jpg`. + /// + /// Without this option, requests to `example.com/foo` would not be + /// redirected. `index.html` would be rendered, and the relative link to + /// `cat.jpeg` would be resolved by the browser as `example.com/cat.jpeg`. + /// Rocket would thus try to find `/static/cat.jpeg`, which does not exist. pub const NormalizeDirs: Options = Options(1 << 2); /// Allow serving a file instead of a directory. @@ -380,10 +386,10 @@ impl Options { } } -/// The default set of options: `Options::Index`. +/// The default set of options: `Options::Index | Options:NormalizeDirs`. impl Default for Options { fn default() -> Self { - Options::Index + Options::Index | Options::NormalizeDirs } } diff --git a/core/lib/src/router/collider.rs b/core/lib/src/router/collider.rs index 13840774..7968bd08 100644 --- a/core/lib/src/router/collider.rs +++ b/core/lib/src/router/collider.rs @@ -57,10 +57,7 @@ impl Collide for RouteUri<'_> { } } - // Check for `/a/` vs. `/a`, which should collide. - a_segments.get(b_segments.len()).map_or(false, |s| s.dynamic_trail) - || b_segments.get(a_segments.len()).map_or(false, |s| s.dynamic_trail) - || a_segments.len() == b_segments.len() + a_segments.len() == b_segments.len() } } @@ -192,7 +189,23 @@ mod tests { assert_no_collision!("/a", "/aaa"); assert_no_collision!("/", "/a"); + assert_no_collision!("/foo", "/foo/"); + assert_no_collision!("/foo/bar", "/foo/"); + assert_no_collision!("/foo/bar", "/foo/bar/"); + assert_no_collision!("/foo/", "/foo//"); + assert_no_collision!("/foo/", "///"); + assert_no_collision!("//", "///"); + assert_no_collision!("/a/", "///"); + + assert_no_collision!("/a", "/a/"); + assert_no_collision!("/", "/a/"); + assert_no_collision!("/a/b", "///"); + assert_no_collision!("/a/", "///"); + assert_no_collision!("//b", "///"); + assert_no_collision!("/hi/", "/hi"); + assert_no_collision!(ranked "/", "/"); + assert_no_collision!(ranked "/a/", "//"); assert_no_collision!(ranked "/hello/", "/hello/"); assert_no_collision!(ranked "/", "/?a"); assert_no_collision!(ranked "/", "/?"); @@ -227,10 +240,9 @@ mod tests { assert_collision!("/", "/foo"); assert_collision!("/", "/"); - assert_collision!("/a", "/a/"); assert_collision!("/a/", "/a/"); assert_collision!("//", "/a/"); - assert_collision!("/", "/a/"); + assert_collision!("//bar/", "/a/"); assert_collision!("/", "/b"); assert_collision!("/hello/", "/hello/bob"); @@ -244,7 +256,6 @@ mod tests { assert_collision!("/", "/<_..>"); assert_collision!("/a/b/", "/a/"); assert_collision!("/a/b/", "/a//"); - assert_collision!("/hi/", "/hi"); assert_collision!("/hi/", "/hi/"); assert_collision!("/", "//////"); diff --git a/core/lib/src/router/matcher.rs b/core/lib/src/router/matcher.rs index b6f34978..2f2b89a3 100644 --- a/core/lib/src/router/matcher.rs +++ b/core/lib/src/router/matcher.rs @@ -43,20 +43,14 @@ fn paths_match(route: &Route, req: &Request<'_>) -> bool { let route_segments = &route.uri.metadata.uri_segments; let req_segments = req.uri().path().segments(); - // requests with longer paths only match if we have dynamic trail (). - if req_segments.num() > route_segments.len() { - if !route.uri.metadata.dynamic_trail { - return false; - } + // A route can never have more segments than a request. Recall that a + // trailing slash is considering a segment, albeit empty. + if route_segments.len() > req_segments.num() { + return false; } - // The last route segment can be trailing (`/<..>`), which is allowed to be - // empty in the request. That is, we want to match `GET /a` to `/a/`. - if route_segments.len() > req_segments.num() { - if route_segments.len() != req_segments.num() + 1 { - return false; - } - + // requests with longer paths only match if we have dynamic trail (). + if req_segments.num() > route_segments.len() { if !route.uri.metadata.dynamic_trail { return false; } @@ -151,7 +145,6 @@ mod tests { assert!(req_matches_route("/a/b?c", "/a/b?")); assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?")); assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?")); - assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?c=foo&")); assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?d=z&")); @@ -169,11 +162,19 @@ mod tests { assert!(!req_matches_route("/a/", "/a")); assert!(!req_matches_route("/a/b", "/a/b/")); + assert!(!req_matches_route("/a", "//")); + assert!(!req_matches_route("/a/", "/")); + assert!(!req_matches_route("/a/b", "//b/")); + assert!(!req_matches_route("/a/b", "///")); + assert!(!req_matches_route("/a/b/c", "/a/b?")); assert!(!req_matches_route("/a?b=c", "/a/b?")); assert!(!req_matches_route("/?b=c", "/a/b?")); assert!(!req_matches_route("/?b=c", "/a?")); + assert!(!req_matches_route("/a/", "///")); + assert!(!req_matches_route("/a/b", "///")); + assert!(!req_matches_route("/a/b?c=foo&d=z", "/a/b?a=b&")); assert!(!req_matches_route("/a/b?c=foo&d=z", "/a/b?d=b&")); assert!(!req_matches_route("/a/b", "/a/b?c")); diff --git a/core/lib/src/router/router.rs b/core/lib/src/router/router.rs index 88d3a293..5617f4fb 100644 --- a/core/lib/src/router/router.rs +++ b/core/lib/src/router/router.rs @@ -170,13 +170,7 @@ mod test { assert!(rankless_route_collisions(&["/", "/"])); assert!(rankless_route_collisions(&["/a/<_>", "/a/"])); assert!(rankless_route_collisions(&["/a/<_>", "/a/<_..>"])); - assert!(rankless_route_collisions(&["/<_>", "/a/<_..>"])); - assert!(rankless_route_collisions(&["/foo", "/foo/<_..>"])); assert!(rankless_route_collisions(&["/foo/bar/baz", "/foo/<_..>"])); - assert!(rankless_route_collisions(&["/a/d/", "/a/d"])); - assert!(rankless_route_collisions(&["/a/<_..>", "/<_>"])); - assert!(rankless_route_collisions(&["/a/<_..>", "/a"])); - assert!(rankless_route_collisions(&["/", "/a/"])); assert!(rankless_route_collisions(&["/<_>", "/<_>"])); assert!(rankless_route_collisions(&["/a/<_>", "/a/b"])); @@ -235,6 +229,13 @@ mod test { assert!(!rankless_route_collisions(&["/a/d/", "/a/b/c"])); assert!(!rankless_route_collisions(&["/a/<_>", "/a"])); assert!(!rankless_route_collisions(&["/a/<_>", "/<_>"])); + assert!(!rankless_route_collisions(&["/a//", "/a/"])); + assert!(!rankless_route_collisions(&["/<_>", "/a/<_..>"])); + assert!(!rankless_route_collisions(&["/foo", "/foo/<_..>"])); + assert!(!rankless_route_collisions(&["/a/<_..>", "/<_>"])); + assert!(!rankless_route_collisions(&["/a/<_..>", "/a"])); + assert!(!rankless_route_collisions(&["/", "/a/"])); + assert!(!rankless_route_collisions(&["/a/d/", "/a/d"])); } #[test] @@ -259,11 +260,11 @@ mod test { assert!(!default_rank_route_collisions(&["/", "/hello"])); assert!(!default_rank_route_collisions(&["/", "/a/"])); assert!(!default_rank_route_collisions(&["/a//c", "//"])); + assert!(!default_rank_route_collisions(&["/a//", "/a/"])); } #[test] fn test_collision_when_ranked() { - assert!(default_rank_route_collisions(&["/a//", "/a/"])); assert!(default_rank_route_collisions(&["//b", "/a/"])); } @@ -329,7 +330,7 @@ mod test { assert!(route(&router, Get, "/a/b/c/d/e/f").is_some()); let router = router_with_routes(&["/foo/"]); - assert!(route(&router, Get, "/foo").is_some()); + assert!(route(&router, Get, "/foo").is_none()); assert!(route(&router, Get, "/foo/").is_some()); assert!(route(&router, Get, "/foo///bar").is_some()); } @@ -497,9 +498,9 @@ mod test { ); assert_ranked_routing!( - to: "/hi", + to: "/hi/", with: [(1, "/hi/"), (0, "/hi/")], - expect: (1, "/hi/") + expect: (0, "/hi/"), (1, "/hi/") ); } diff --git a/core/lib/tests/file_server.rs b/core/lib/tests/file_server.rs index 1271d6ef..69416eda 100644 --- a/core/lib/tests/file_server.rs +++ b/core/lib/tests/file_server.rs @@ -172,17 +172,21 @@ fn test_redirection() { assert_eq!(response.status(), Status::Ok); // Root of route is also redirected. - let response = client.get("/no_index").dispatch(); + let response = client.get("/no_index/").dispatch(); assert_eq!(response.status(), Status::NotFound); - let response = client.get("/index").dispatch(); + let response = client.get("/index/").dispatch(); assert_eq!(response.status(), Status::Ok); - let response = client.get("/redir").dispatch(); + let response = client.get("/redir/inner").dispatch(); assert_eq!(response.status(), Status::PermanentRedirect); - assert_eq!(response.headers().get("Location").next(), Some("/redir/")); + assert_eq!(response.headers().get("Location").next(), Some("/redir/inner/")); - let response = client.get("/redir_index").dispatch(); + let response = client.get("/redir/other").dispatch(); assert_eq!(response.status(), Status::PermanentRedirect); - assert_eq!(response.headers().get("Location").next(), Some("/redir_index/")); + assert_eq!(response.headers().get("Location").next(), Some("/redir/other/")); + + let response = client.get("/redir_index/other").dispatch(); + assert_eq!(response.status(), Status::PermanentRedirect); + assert_eq!(response.headers().get("Location").next(), Some("/redir_index/other/")); } diff --git a/examples/hello/src/main.rs b/examples/hello/src/main.rs index 0f8c55cb..79b4588a 100644 --- a/examples/hello/src/main.rs +++ b/examples/hello/src/main.rs @@ -74,8 +74,19 @@ fn hello(lang: Option, opt: Options<'_>) -> String { #[launch] fn rocket() -> _ { + use rocket::fairing::AdHoc; + rocket::build() .mount("/", routes![hello]) .mount("/hello", routes![world, mir]) .mount("/wave", routes![wave]) + .attach(AdHoc::on_request("Compatibility Normalizer", |req, _| Box::pin(async move { + if !req.uri().is_normalized_nontrailing() { + let normal = req.uri().clone().into_normalized_nontrailing(); + warn!("Incoming request URI was normalized for compatibility."); + info_!("{} -> {}", req.uri(), normal); + req.set_uri(normal); + } + }))) + } diff --git a/examples/static-files/src/tests.rs b/examples/static-files/src/tests.rs index b437f77d..df096dc6 100644 --- a/examples/static-files/src/tests.rs +++ b/examples/static-files/src/tests.rs @@ -40,7 +40,6 @@ fn test_index_html() { #[test] fn test_hidden_index_html() { - test_query_file("/hidden", "static/hidden/index.html", Status::Ok); test_query_file("/hidden/", "static/hidden/index.html", Status::Ok); test_query_file("//hidden//", "static/hidden/index.html", Status::Ok); test_query_file("/second/hidden", "static/hidden/index.html", Status::Ok); @@ -65,6 +64,7 @@ fn test_icon_file() { #[test] fn test_invalid_path() { + test_query_file("/hidden", None, Status::PermanentRedirect); test_query_file("/thou_shalt_not_exist", None, Status::NotFound); test_query_file("/thou/shalt/not/exist", None, Status::NotFound); test_query_file("/thou/shalt/not/exist?a=b&c=d", None, Status::NotFound);