Skip to content

Commit

Permalink
Fix rwf2#1224 by searching routes after failed match
Browse files Browse the repository at this point in the history
  • Loading branch information
jespersm committed Sep 20, 2023
1 parent 26a3f00 commit 7251c5b
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 9 deletions.
9 changes: 6 additions & 3 deletions core/codegen/tests/route-format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ fn test_formats() {
assert_eq!(response.into_string().unwrap(), "plain");

let response = client.put("/").header(ContentType::HTML).dispatch();
assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::MethodNotAllowed);
}

// Test custom formats.
Expand Down Expand Up @@ -109,9 +109,12 @@ fn test_custom_formats() {
let response = client.get("/").dispatch();
assert_eq!(response.into_string().unwrap(), "get_foo");

let response = client.get("/").header(Accept::JPEG).dispatch();
assert_eq!(response.status(), Status::NotAcceptable); // Route can't produce JPEG

let response = client.put("/").header(ContentType::HTML).dispatch();
assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::UnsupportedMediaType); // Route expects "bar/baz"

let response = client.post("/").header(ContentType::HTML).dispatch();
assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::UnsupportedMediaType); // Route expects "foo"
}
4 changes: 2 additions & 2 deletions core/codegen/tests/route.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ fn test_full_route() {
assert_eq!(response.status(), Status::NotFound);

let response = client.post(format!("/1{}", uri)).body(simple).dispatch();
assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::UnsupportedMediaType);

let response = client
.post(format!("/1{}", uri))
Expand All @@ -117,7 +117,7 @@ fn test_full_route() {
sky, name.percent_decode().unwrap(), "A A", "inside", path, simple, expected_uri));

let response = client.post(format!("/2{}", uri)).body(simple).dispatch();
assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::UnsupportedMediaType);

let response = client
.post(format!("/2{}", uri))
Expand Down
4 changes: 2 additions & 2 deletions core/lib/src/router/matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl Catcher {
}
}

fn paths_match(route: &Route, req: &Request<'_>) -> bool {
pub(crate) fn paths_match(route: &Route, req: &Request<'_>) -> bool {
trace!("checking path match: route {} vs. request {}", route, req);
let route_segments = &route.uri.metadata.uri_segments;
let req_segments = req.uri().path().segments();
Expand Down Expand Up @@ -170,7 +170,7 @@ fn paths_match(route: &Route, req: &Request<'_>) -> bool {
true
}

fn queries_match(route: &Route, req: &Request<'_>) -> bool {
pub(crate) fn queries_match(route: &Route, req: &Request<'_>) -> bool {
trace!("checking query match: route {} vs. request {}", route, req);
if matches!(route.uri.metadata.query_color, None | Some(Color::Wild)) {
return true;
Expand Down
43 changes: 43 additions & 0 deletions core/lib/src/router/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,33 @@ impl Router {
.flat_map(move |routes| routes.iter().filter(move |r| r.matches(req)))
}

pub(crate) fn matches_except_formats<'r, 'a: 'r>(
&'a self,
req: &'r Request<'r>
) -> bool {
self.routes.get(&req.method())
.into_iter()
.flatten()
.any(|route| super::matcher::paths_match(route, req) && super::matcher::queries_match(route, req))
}

const ALL_METHODS: &[Method] = &[
Method::Get, Method::Put, Method::Post, Method::Delete, Method::Options,
Method::Head, Method::Trace, Method::Connect, Method::Patch,
];

pub(crate) fn matches_except_method<'r, 'a: 'r>(
&'a self,
req: &'r Request<'r>
) -> bool {
Self::ALL_METHODS
.iter()
.filter(|method| *method != &req.method())
.filter_map(|method| self.routes.get(method))
.flatten()
.any(|route| super::matcher::paths_match(route, req))
}

// For many catchers, using aho-corasick or similar should be much faster.
pub fn catch<'r>(&self, status: Status, req: &'r Request<'r>) -> Option<&Catcher> {
// Note that catchers are presorted by descending base length.
Expand Down Expand Up @@ -368,6 +395,22 @@ mod test {
assert!(route(&router, Get, "/prefi/").is_none());
}

fn has_mismatched_method<'a>(router: &'a Router, method: Method, uri: &'a str) -> bool {
let client = Client::debug_with(vec![]).expect("client");
let request = client.req(method, Origin::parse(uri).unwrap());
router.matches_except_method(&request)
}

#[test]
fn test_bad_method_routing() {
let router = router_with_routes(&["/hello"]);
assert!(route(&router, Put, "/hello").is_none());
assert!(has_mismatched_method(&router, Put, "/hello"));
assert!(has_mismatched_method(&router, Post, "/hello"));

assert!(! has_mismatched_method(&router, Get, "/hello"));
}

/// Asserts that `$to` routes to `$want` given `$routes` are present.
macro_rules! assert_ranked_match {
($routes:expr, $to:expr => $want:expr) => ({
Expand Down
10 changes: 10 additions & 0 deletions core/lib/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,16 @@ impl Rocket<Orbit> {
}

error_!("No matching routes for {}.", request);
if request.route().is_none() {
// We failed to find a route which matches on path, query AND formats
if self.router.matches_except_formats(request) {
// Tailor the error code to the interpretation of the request in question.
status = if request.method().supports_payload() { Status::UnsupportedMediaType } else { Status::NotAcceptable };
} else if self.router.matches_except_method(request) {
// Found a more suitable error code from simple route paths implemented on different methods
status = Status::MethodNotAllowed;
}
}
Outcome::Forward((data, status))
}

Expand Down
2 changes: 1 addition & 1 deletion core/lib/tests/form_method-issue-45.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ mod tests {
.body("_method=patch&form_data=Form+data")
.dispatch();

assert_eq!(response.status(), Status::NotFound);
assert_eq!(response.status(), Status::MethodNotAllowed);
}
}
2 changes: 1 addition & 1 deletion core/lib/tests/precise-content-type-matching.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ mod tests {
let body: Option<&'static str> = $body;
match body {
Some(string) => assert_eq!(body_str, Some(string.to_string())),
None => assert_eq!(status, Status::NotFound)
None => assert_eq!(status, Status::UnsupportedMediaType)
}
)
}
Expand Down
9 changes: 9 additions & 0 deletions examples/error-handling/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,15 @@ fn test_hello_invalid_age() {
}
}

#[test]
fn test_method_not_allowed() {
let client = Client::tracked(super::rocket()).unwrap();
let (name, age) = ("Pat", 86);
let request = client.post(format!("/hello/{}/{}", name, age)).body("body");
let response = request.dispatch();
assert_eq!(response.status(), Status::MethodNotAllowed);
}

#[test]
fn test_hello_sergio() {
let client = Client::tracked(super::rocket()).unwrap();
Expand Down

0 comments on commit 7251c5b

Please sign in to comment.