From f35e3c4aca6f19014fa9bac43d831bcc191c0fef Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Thu, 23 Jan 2020 21:07:11 -0800 Subject: [PATCH] Set cookies even on error responses. Fixes #1213. --- core/http/Cargo.toml | 2 +- core/http/src/cookies.rs | 11 ++++++ core/lib/src/rocket.rs | 39 +++++++++++++-------- core/lib/tests/catcher-cookies-1213.rs | 47 ++++++++++++++++++++++++++ site/guide/4-requests.md | 24 ++++++++----- 5 files changed, 99 insertions(+), 24 deletions(-) create mode 100644 core/lib/tests/catcher-cookies-1213.rs diff --git a/core/http/Cargo.toml b/core/http/Cargo.toml index bfea17d2..e616fb8b 100644 --- a/core/http/Cargo.toml +++ b/core/http/Cargo.toml @@ -27,7 +27,7 @@ time = "0.2.4" indexmap = "1.0" rustls = { version = "0.16", optional = true } state = "0.4" -cookie = { version = "0.13", features = ["percent-encode"] } +cookie = { version = "0.13.1", features = ["percent-encode"] } pear = "0.1" unicode-xid = "0.2" diff --git a/core/http/src/cookies.rs b/core/http/src/cookies.rs index 1d99217d..ad5e2ce6 100644 --- a/core/http/src/cookies.rs +++ b/core/http/src/cookies.rs @@ -236,6 +236,17 @@ impl<'a> Cookies<'a> { } } + /// Removes all delta cookies. + /// WARNING: This is unstable! Do not use this method outside of Rocket! + #[inline] + #[doc(hidden)] + pub fn reset_delta(&mut self) { + match *self { + Cookies::Jarred(ref mut jar, _) => jar.reset_delta(), + Cookies::Empty(ref mut jar) => jar.reset_delta() + } + } + /// Returns an iterator over all of the cookies present in this collection. /// /// # Example diff --git a/core/lib/src/rocket.rs b/core/lib/src/rocket.rs index 96c25990..88ca38e8 100644 --- a/core/lib/src/rocket.rs +++ b/core/lib/src/rocket.rs @@ -239,15 +239,8 @@ impl Rocket { request: &'r Request<'s>, data: Data ) -> Response<'r> { - match self.route(request, data) { - Outcome::Success(mut response) => { - // A user's route responded! Set the cookies. - for cookie in request.cookies().delta() { - response.adjoin_header(cookie); - } - - response - } + let mut response = match self.route(request, data) { + Outcome::Success(response) => response, Outcome::Forward(data) => { // There was no matching route. Autohandle `HEAD` requests. if request.method() == Method::Head { @@ -255,14 +248,24 @@ impl Rocket { // Dispatch the request again with Method `GET`. request._set_method(Method::Get); - self.route_and_process(request, data) + + // Return early so we don't set cookies twice. + return self.route_and_process(request, data); } else { // No match was found and it can't be autohandled. 404. self.handle_error(Status::NotFound, request) } } - Outcome::Failure(status) => self.handle_error(status, request) + Outcome::Failure(status) => self.handle_error(status, request), + }; + + // Set the cookies. Note that error responses will only include cookies + // set by the error handler. See `handle_error` for more. + for cookie in request.cookies().delta() { + response.adjoin_header(cookie); } + + response } /// Tries to find a `Responder` for a given `request`. It does this by @@ -306,10 +309,11 @@ impl Rocket { } // Finds the error catcher for the status `status` and executes it for the - // given request `req`. If a user has registered a catcher for `status`, the - // catcher is called. If the catcher fails to return a good response, the - // 500 catcher is executed. If there is no registered catcher for `status`, - // the default catcher is used. + // given request `req`; the cookies in `req` are reset to their original + // state before invoking the error handler. If a user has registered a + // catcher for `status`, the catcher is called. If the catcher fails to + // return a good response, the 500 catcher is executed. If there is no + // registered catcher for `status`, the default catcher is used. pub(crate) fn handle_error<'r>( &self, status: Status, @@ -317,6 +321,11 @@ impl Rocket { ) -> Response<'r> { warn_!("Responding with {} catcher.", Paint::red(&status)); + // For now, we reset the delta state to prevent any modifications from + // earlier, unsuccessful paths from being reflected in error response. + // We may wish to relax this in the future. + req.cookies().reset_delta(); + // Try to get the active catcher but fallback to user's 500 catcher. let catcher = self.catchers.get(&status.code).unwrap_or_else(|| { error_!("No catcher found for {}. Using 500 catcher.", status); diff --git a/core/lib/tests/catcher-cookies-1213.rs b/core/lib/tests/catcher-cookies-1213.rs new file mode 100644 index 00000000..f9f66ce3 --- /dev/null +++ b/core/lib/tests/catcher-cookies-1213.rs @@ -0,0 +1,47 @@ +#![feature(proc_macro_hygiene)] + +#[macro_use] extern crate rocket; + +use rocket::request::Request; +use rocket::http::{Cookie, Cookies}; + +#[catch(404)] +fn not_found(request: &Request) -> &'static str { + request.cookies().add(Cookie::new("not_found", "hi")); + "404 - Not Found" +} + +#[get("/")] +fn index(mut cookies: Cookies) -> &'static str { + cookies.add(Cookie::new("index", "hi")); + "Hello, world!" +} + +mod tests { + use super::*; + use rocket::local::Client; + use rocket::fairing::AdHoc; + + #[test] + fn error_catcher_sets_cookies() { + let rocket = rocket::ignite() + .mount("/", routes![index]) + .register(catchers![not_found]) + .attach(AdHoc::on_request("Add Fairing Cookie", |req, _| { + req.cookies().add(Cookie::new("fairing", "hi")); + })); + + let client = Client::new(rocket).unwrap(); + + // Check that the index returns the `index` and `fairing` cookie. + let response = client.get("/").dispatch(); + let cookies = response.cookies(); + assert_eq!(cookies.len(), 2); + assert!(cookies.iter().find(|c| c.name() == "index").is_some()); + assert!(cookies.iter().find(|c| c.name() == "fairing").is_some()); + + // Check that the catcher returns only the `not_found` cookie. + let response = client.get("/not-existent").dispatch(); + assert_eq!(response.cookies(), vec![Cookie::new("not_found", "hi")]); + } +} diff --git a/site/guide/4-requests.md b/site/guide/4-requests.md index f26fd3d8..c1916b52 100644 --- a/site/guide/4-requests.md +++ b/site/guide/4-requests.md @@ -918,13 +918,20 @@ Routing may fail for a variety of reasons. These include: * No routes matched. If any of these conditions occur, Rocket returns an error to the client. To do -so, Rocket invokes the _catcher_ corresponding to the error's status code. A -catcher is like a route, except it only handles errors. Rocket provides default -catchers for all of the standard HTTP error codes. To override a default -catcher, or declare a catcher for a custom status code, use the [`catch`] -attribute, which takes a single integer corresponding to the HTTP status code to -catch. For instance, to declare a catcher for `404 Not Found` errors, you'd -write: +so, Rocket invokes the _catcher_ corresponding to the error's status code. +Catchers are similar to routes except in that: + + 1. Catchers are only invoked on error conditions. + 2. Catchers are declared with the `catch` attribute. + 3. Catchers are _registered_ with [`register()`] instead of [`mount()`]. + 4. Any modifications to cookies are cleared before a catcher is invoked. + 5. Error catchers cannot invoke guards of any sort. + +Rocket provides default catchers for all of the standard HTTP error codes. To +override a default catcher, or declare a catcher for a custom status code, use +the [`catch`] attribute, which takes a single integer corresponding to the HTTP +status code to catch. For instance, to declare a catcher for `404 Not Found` +errors, you'd write: ```rust #[catch(404)] @@ -952,10 +959,11 @@ rocket::ignite().register(catchers![not_found]) ``` Unlike route request handlers, catchers take exactly zero or one parameter. If -the catcher takes a parameter, it must be of type [`&Request`] The [error +the catcher takes a parameter, it must be of type [`&Request`]. The [error catcher example](@example/errors) on GitHub illustrates their use in full. [`catch`]: @api/rocket_codegen/attr.catch.html [`register()`]: @api/rocket/struct.Rocket.html#method.register +[`mount()`]: @api/rocket/struct.Rocket.html#method.mount [`catchers!`]: @api/rocket_codegen/macro.catchers.html [`&Request`]: @api/rocket/struct.Request.html