Set cookies even on error responses.

Fixes #1213.

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"
 

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

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

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")]);
+    }
+}

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