From 3a1d6718946a0a3830ffff3a7b15faf9d48182e5 Mon Sep 17 00:00:00 2001 From: Lukas Abfalterer Date: Sun, 4 Feb 2018 11:07:28 +0100 Subject: [PATCH] Fix interactions between fairings and auto-HEAD responses. Fixes #546. --- lib/src/fairing/mod.rs | 14 ++++ lib/src/request/request.rs | 18 +++-- lib/src/rocket.rs | 57 +++++++++------- .../fairing_before_head_strip-issue-546.rs | 67 +++++++++++++++++++ 4 files changed, 126 insertions(+), 30 deletions(-) create mode 100644 lib/tests/fairing_before_head_strip-issue-546.rs diff --git a/lib/src/fairing/mod.rs b/lib/src/fairing/mod.rs index a2c2e2b7..6989226c 100644 --- a/lib/src/fairing/mod.rs +++ b/lib/src/fairing/mod.rs @@ -48,6 +48,15 @@ //! Furthermore, a `Fairing` should take care to act locally so that the actions //! of other `Fairings` are not jeopardized. For instance, unless it is made //! abundantly clear, a fairing should not rewrite every request. +//! +//! ## Attention +//! +//! If Rocket receives a `HEAD` request and no appropriate Route is provided, +//! Rocket tries to fullfil this request as it were a `GET` request (Autohandling +//! `HEAD` requests). _Beware_ the request method is set to `GET` on the request which is +//! provided in the [`on_response`](/rocket/fairing/trait.Fairing.html#method.on_response) +//! method. + use {Rocket, Request, Response, Data}; mod fairings; @@ -336,6 +345,11 @@ pub trait Fairing: Send + Sync + 'static { /// this fairing. The `&Request` parameter is the request that was routed, /// and the `&mut Response` parameter is the resulting response. /// + /// ## Note + /// + /// The body of a `HEAD` request will be stripped off _after_ all response + /// `Fairings` have been performed. + /// /// ## Default Implementation /// /// The default implementation of this method does nothing. diff --git a/lib/src/request/request.rs b/lib/src/request/request.rs index 176b18f8..01cbaab6 100644 --- a/lib/src/request/request.rs +++ b/lib/src/request/request.rs @@ -37,7 +37,7 @@ struct RequestState<'r> { /// data. This includes the HTTP method, URI, cookies, headers, and more. #[derive(Clone)] pub struct Request<'r> { - method: Method, + method: Cell, uri: Uri<'r>, headers: HeaderMap<'r>, remote: Option, @@ -53,7 +53,7 @@ impl<'r> Request<'r> { method: Method, uri: U) -> Request<'r> { Request { - method: method, + method: Cell::new(method), uri: uri.into(), headers: HeaderMap::new(), remote: None, @@ -91,7 +91,7 @@ impl<'r> Request<'r> { /// ``` #[inline(always)] pub fn method(&self) -> Method { - self.method + self.method.get() } /// Set the method of `self`. @@ -111,7 +111,7 @@ impl<'r> Request<'r> { /// ``` #[inline(always)] pub fn set_method(&mut self, method: Method) { - self.method = method; + self.method.set(method); } /// Borrow the URI from `self`, which is guaranteed to be an absolute URI. @@ -446,7 +446,7 @@ impl<'r> Request<'r> { /// ``` pub fn format(&self) -> Option<&MediaType> { static ANY: MediaType = MediaType::Any; - if self.method.supports_payload() { + if self.method().supports_payload() { self.content_type().map(|ct| ct.media_type()) } else { // FIXME: Should we be using `accept_first` or `preferred`? Or @@ -616,6 +616,12 @@ impl<'r> Request<'r> { *self.state.params.borrow_mut() = route.get_param_indexes(self.uri()); } + /// Set the method of `self`, even if self is a shared reference + #[inline(always)] + pub(crate) fn _set_method(&self, method: Method) { + self.method.set(method); + } + /// Replace all of the cookies in `self` with those in `jar`. #[inline] pub(crate) fn set_cookies(&mut self, jar: CookieJar) { @@ -701,7 +707,7 @@ impl<'r> fmt::Display for Request<'r> { /// Pretty prints a Request. This is primarily used by Rocket's logging /// infrastructure. fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{} {}", Paint::green(&self.method), Paint::blue(&self.uri))?; + write!(f, "{} {}", Paint::green(self.method()), Paint::blue(&self.uri))?; // Print the requests media type when the route specifies a format. if let Some(media_type) = self.format() { diff --git a/lib/src/rocket.rs b/lib/src/rocket.rs index a031098e..632a5a42 100644 --- a/lib/src/rocket.rs +++ b/lib/src/rocket.rs @@ -193,20 +193,46 @@ impl Rocket { } } - #[inline] + #[inline(always)] pub(crate) fn dispatch<'s, 'r>( &'s self, request: &'r mut Request<'s>, - data: Data, + data: Data ) -> Response<'r> { info!("{}:", request); // Do a bit of preprocessing before routing; run the attached fairings. self.preprocess_request(request, &data); + + // Run the request fairings. self.fairings.handle_request(request, &data); + // Check if the request is a `HEAD` request. + let was_head_request = request.method() == Method::Head; + + // Route the request and run the user's handlers. + let mut response = self.route_and_process(request, data); + + // Add the 'rocket' server header to the response and run fairings. + // TODO: If removing Hyper, write out `Date` header too. + response.set_header(Header::new("Server", "Rocket")); + self.fairings.handle_response(request, &mut response); + + // Strip the body if this is a `HEAD` request. + if was_head_request { + response.strip_body(); + } + + response + } + + fn route_and_process<'s, 'r>( + &'s self, + request: &'r Request<'s>, + data: Data + ) -> Response<'r> { // Route the request to get a response. - let mut response = match self.route(request, data) { + let response = match self.route(request, data) { Outcome::Success(mut response) => { // A user's route responded! Set the cookies. for cookie in request.cookies().delta() { @@ -216,21 +242,14 @@ impl Rocket { response } Outcome::Forward(data) => { - // Rust thinks `request` is still borrowed here, but it's - // obviously not (data has nothing to do with it), so we - // convince it to give us another mutable reference. - // TODO: Use something that is well defined, like UnsafeCell. - // But that causes variance issues...so wait for NLL. - let request: &'r mut Request<'s> = - unsafe { (&mut *(request as *const _ as *mut _)) }; // There was no matching route. Autohandle `HEAD` requests. if request.method() == Method::Head { info_!("Autohandling {} request.", Paint::white("HEAD")); - request.set_method(Method::Get); - let mut response = self.dispatch(request, data); - response.strip_body(); - response + + // Dispatch the request again with Method `GET`. + request._set_method(Method::Get); + self.route_and_process(request, data) } else { self.handle_error(Status::NotFound, request) } @@ -238,16 +257,6 @@ impl Rocket { Outcome::Failure(status) => self.handle_error(status, request), }; - // Strip the body if this is a `HEAD` request. - if request.method() == Method::Head { - response.strip_body(); - } - - // Add the 'rocket' server header to the response and run fairings. - // TODO: If removing Hyper, write out `Date` header too. - response.set_header(Header::new("Server", "Rocket")); - self.fairings.handle_response(request, &mut response); - response } diff --git a/lib/tests/fairing_before_head_strip-issue-546.rs b/lib/tests/fairing_before_head_strip-issue-546.rs new file mode 100644 index 00000000..7363c5d0 --- /dev/null +++ b/lib/tests/fairing_before_head_strip-issue-546.rs @@ -0,0 +1,67 @@ +#![feature(plugin, decl_macro)] +#![plugin(rocket_codegen)] + +extern crate rocket; + +const RESPONSE_STRING: &'static str = "{'test': 'dont strip before fairing' }"; + +#[head("/")] +fn index() -> &'static str { + RESPONSE_STRING +} + +#[get("/")] +fn auto() -> &'static str { + RESPONSE_STRING +} + +mod fairing_before_head_strip { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + + use rocket::fairing::AdHoc; + use rocket::http::Method; + use rocket::local::Client; + use rocket::http::Status; + use rocket::State; + + #[derive(Default)] + struct Counter { + get: AtomicUsize, + } + + #[test] + fn not_empty_before_fairing() { + let rocket = rocket::ignite() + .mount("/", routes![index]) + .attach(AdHoc::on_response(|req, res| { + assert_eq!(req.method(), Method::Head); + assert_eq!(res.body_string(), Some(RESPONSE_STRING.into())); + })); + + let client = Client::new(rocket).unwrap(); + let response = client.head("/").dispatch(); + assert_eq!(response.status(), Status::Ok); + } + + #[test] + fn not_empty_before_fairing_autohandeled() { + let counter = Counter::default(); + let rocket = rocket::ignite() + .mount("/", routes![auto]) + .manage(counter) + .attach(AdHoc::on_request(|req, _| { + let c = req.guard::>().unwrap(); + + assert_eq!(c.get.fetch_add(1, Ordering::Release), 0); + })) + .attach(AdHoc::on_response(|req, res| { + assert_eq!(req.method(), Method::Get); + assert_eq!(res.body_string(), Some(RESPONSE_STRING.into())); + })); + + let client = Client::new(rocket).unwrap(); + let response = client.head("/").dispatch(); + assert_eq!(response.status(), Status::Ok); + } +}