rwf2
/
Rocket
mirror of https://github.com/rwf2/Rocket.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make trailing slashes significant during routing. 

This commit modifies request routing in a backwards incompatible manner.
The change is summarized as: trailing slashes are now significant and
never transparently disregarded. This has the following implications,
all representing behavior that differs from that before this change:

  * Route URIs with trailing slashes (`/foo/`, `/<a>/`) are legal.
  * A request `/foo/` is routed to route `/foo/` but not `/foo`.
  * Similarly, a request `/bar/` is routed to `/<a>/` but not `/<a>`.
  * A request `/bar/foo` is not routed to `/<a>/<b>/<c..>`.

A new `AdHoc::uri_normalizer()` fairing was added that recovers the
previous behavior.

In addition to the above, the `Options::NormalizeDirs` `FileServer`
option is now enabled by default to remain consistent with the above
changes and reduce breaking changes at the `FileServer` level.
This commit is contained in:
Sergio Benitez 2023-04-10 10:48:30 -07:00
parent 908a918e8b
commit 51ed332127
13 changed files with 179 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
core/codegen/src/attribute/route/parse.rs
View File

@ -77,11 +77,9 @@ impl FromMeta for RouteUri {
.help("expected URI in origin form: \"/path/<param>\"")
})?;
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))

2
core/codegen/src/bang/uri_parsing.rs
View File

@ -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;

3
core/codegen/tests/route.rs
View File

@ -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");

26
core/codegen/tests/ui-fail-nightly/route-path-bad-syntax.stderr
View File

@ -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 `<name>`?
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/"

23
core/codegen/tests/ui-fail-stable/route-path-bad-syntax.stderr
View File

@ -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/")]
| ^^^^^^^^^

78
core/lib/src/fairing/ad_hoc.rs
View File

@ -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 `/<a>` 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("/<param>")]
/// 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("/<param>")]
/// 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]

52
core/lib/src/fs/server.rs
View File

@ -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/<path..>` will be handled by
/// returning the contents of `/static/<path..>`. Requests for _directories_ at
/// Requests for files at `/public/<path..>` will be handled by returning the
/// contents of `/static/<path..>`. Requests for _directories_ at
/// `/public/<directory>` will be handled by returning the contents of
/// `/static/<directory>/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
}
}

25
core/lib/src/router/collider.rs
View File

@ -57,10 +57,7 @@ impl Collide for RouteUri<'_> {
}
}
// Check for `/a/<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/<a>", "/foo/<a>/");
assert_no_collision!("/foo/<a>", "/<b>/<a>/");
assert_no_collision!("/<b>/<a>", "/<b>/<a>/");
assert_no_collision!("/a/", "/<a>/<b>/<c..>");
assert_no_collision!("/a", "/a/<a..>");
assert_no_collision!("/<a>", "/a/<a..>");
assert_no_collision!("/a/b", "/<a>/<b>/<c..>");
assert_no_collision!("/a/<b>", "/<a>/<b>/<c..>");
assert_no_collision!("/<a>/b", "/<a>/<b>/<c..>");
assert_no_collision!("/hi/<a..>", "/hi");
assert_no_collision!(ranked "/<a>", "/");
assert_no_collision!(ranked "/a/", "/<a>/");
assert_no_collision!(ranked "/hello/<a>", "/hello/");
assert_no_collision!(ranked "/", "/?a");
assert_no_collision!(ranked "/", "/?<a>");
@ -227,10 +240,9 @@ mod tests {
assert_collision!("/<a..>", "/foo");
assert_collision!("/", "/<a..>");
assert_collision!("/a", "/a/<a..>");
assert_collision!("/a/", "/a/<a..>");
assert_collision!("/<a>/", "/a/<a..>");
assert_collision!("/<a>", "/a/<a..>");
assert_collision!("/<a>/bar/", "/a/<a..>");
assert_collision!("/<a>", "/b");
assert_collision!("/hello/<name>", "/hello/bob");
@ -244,7 +256,6 @@ mod tests {
assert_collision!("/", "/<_..>");
assert_collision!("/a/b/<a..>", "/a/<b..>");
assert_collision!("/a/b/<a..>", "/a/<b>/<b..>");
assert_collision!("/hi/<a..>", "/hi");
assert_collision!("/hi/<a..>", "/hi/");
assert_collision!("/<a..>", "//////");

27
core/lib/src/router/matcher.rs
View File

@ -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 (<a..>).
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/<b..>`.
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 (<a..>).
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?<c>"));
assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?<c>"));
assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?<c..>"));
assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?c=foo&<c..>"));
assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?d=z&<c..>"));
@ -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", "/<a>/"));
assert!(!req_matches_route("/a/", "/<a>"));
assert!(!req_matches_route("/a/b", "/<a>/b/"));
assert!(!req_matches_route("/a/b", "/<a>/<b>/"));
assert!(!req_matches_route("/a/b/c", "/a/b?<c>"));
assert!(!req_matches_route("/a?b=c", "/a/b?<c>"));
assert!(!req_matches_route("/?b=c", "/a/b?<c>"));
assert!(!req_matches_route("/?b=c", "/a?<c>"));
assert!(!req_matches_route("/a/", "/<a>/<b>/<c..>"));
assert!(!req_matches_route("/a/b", "/<a>/<b>/<c..>"));
assert!(!req_matches_route("/a/b?c=foo&d=z", "/a/b?a=b&<c..>"));
assert!(!req_matches_route("/a/b?c=foo&d=z", "/a/b?d=b&<c..>"));
assert!(!req_matches_route("/a/b", "/a/b?c"));

21
core/lib/src/router/router.rs
View File

@ -170,13 +170,7 @@ mod test {
assert!(rankless_route_collisions(&["/", "/<a..>"]));
assert!(rankless_route_collisions(&["/a/<_>", "/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/<b..>", "/a/d"]));
assert!(rankless_route_collisions(&["/a/<_..>", "/<_>"]));
assert!(rankless_route_collisions(&["/a/<_..>", "/a"]));
assert!(rankless_route_collisions(&["/<a>", "/a/<a..>"]));
assert!(rankless_route_collisions(&["/<_>", "/<_>"]));
assert!(rankless_route_collisions(&["/a/<_>", "/a/b"]));
@ -235,6 +229,13 @@ mod test {
assert!(!rankless_route_collisions(&["/a/d/<b..>", "/a/b/c"]));
assert!(!rankless_route_collisions(&["/a/<_>", "/a"]));
assert!(!rankless_route_collisions(&["/a/<_>", "/<_>"]));
assert!(!rankless_route_collisions(&["/a/<b>/<c..>", "/a/<c>"]));
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>", "/a/<a..>"]));
assert!(!rankless_route_collisions(&["/a/d/<b..>", "/a/d"]));
}
#[test]
@ -259,11 +260,11 @@ mod test {
assert!(!default_rank_route_collisions(&["/<a..>", "/hello"]));
assert!(!default_rank_route_collisions(&["/<a>", "/a/<path..>"]));
assert!(!default_rank_route_collisions(&["/a/<b>/c", "/<d>/<c..>"]));
assert!(!default_rank_route_collisions(&["/a/<b>/<c..>", "/a/<c>"]));
}
#[test]
fn test_collision_when_ranked() {
assert!(default_rank_route_collisions(&["/a/<b>/<c..>", "/a/<c>"]));
assert!(default_rank_route_collisions(&["/<a>/b", "/a/<b>"]));
}
@ -329,7 +330,7 @@ mod test {
assert!(route(&router, Get, "/a/b/c/d/e/f").is_some());
let router = router_with_routes(&["/foo/<a..>"]);
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/<foo..>"), (0, "/hi/<foo>")],
expect: (1, "/hi/<foo..>")
expect: (0, "/hi/<foo>"), (1, "/hi/<foo..>")
);
}

16
core/lib/tests/file_server.rs
View File

@ -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/"));
}

11
examples/hello/src/main.rs
View File

@ -74,8 +74,19 @@ fn hello(lang: Option<Lang>, 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);
}
})))
}

2
examples/static-files/src/tests.rs
View File

@ -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);