From 5cb70ec58ca9c38f7b54d6a7a2e84b3a674e76ba Mon Sep 17 00:00:00 2001 From: Matthew Pomes Date: Mon, 23 May 2022 22:50:20 -0700 Subject: [PATCH] Salvage everything viable from bad requests. Co-authored-by: Sergio Benitez --- core/lib/src/request/request.rs | 93 +++++++++++++++++++-------------- core/lib/src/server.rs | 13 +++-- 2 files changed, 59 insertions(+), 47 deletions(-) diff --git a/core/lib/src/request/request.rs b/core/lib/src/request/request.rs index a2b26390..c51b8c41 100644 --- a/core/lib/src/request/request.rs +++ b/core/lib/src/request/request.rs @@ -968,33 +968,47 @@ impl<'r> Request<'r> { rocket: &'r Rocket, hyper: &'r hyper::request::Parts, connection: Option, - ) -> Result, Error<'r>> { + ) -> Result, BadRequest<'r>> { + // Keep track of parsing errors; emit a `BadRequest` if any exist. + let mut errors = vec![]; + // Ensure that the method is known. TODO: Allow made-up methods? let method = Method::from_hyp(&hyper.method) - .ok_or(Error::BadMethod(&hyper.method))?; + .unwrap_or_else(|| { + errors.push(Kind::BadMethod(&hyper.method)); + Method::Get + }); // TODO: Keep around not just the path/query, but the rest, if there? - let uri = hyper.uri.path_and_query().ok_or(Error::InvalidUri(&hyper.uri))?; + let uri = hyper.uri.path_and_query() + .map(|uri| { + // In debug, make sure we agree with Hyper about URI validity. + // If we disagree, log a warning but continue anyway; if this is + // a security issue with Hyper, there isn't much we can do. + #[cfg(debug_assertions)] + if Origin::parse(uri.as_str()).is_err() { + warn!("Hyper/Rocket URI validity discord: {:?}", uri.as_str()); + info_!("Hyper believes the URI is valid while Rocket disagrees."); + info_!("This is likely a Hyper bug with potential security implications."); + warn_!("Please report this warning to Rocket's GitHub issue tracker."); + } - // In debug, make sure we agree with Hyper that the URI is valid. If we - // disagree, print a warning but continue anyway seeing as if this is a - // security issue with Hyper, there isn't much we can do. - #[cfg(debug_assertions)] - if Origin::parse(uri.as_str()).is_err() { - warn!("Hyper/Rocket URI validity discord: {:?}", uri.as_str()); - info_!("Hyper believes the URI is valid while Rocket disagrees."); - info_!("This is likely a Hyper bug with potential security implications."); - warn_!("Please report this warning to Rocket's GitHub issue tracker."); - } + Origin::new(uri.path(), uri.query().map(Cow::Borrowed)) + }) + .unwrap_or_else(|| { + errors.push(Kind::InvalidUri(&hyper.uri)); + Origin::ROOT + }); - // Construct the request object. - let uri = Origin::new(uri.path(), uri.query().map(Cow::Borrowed)); + // Construct the request object; fill in metadata and headers next. let mut request = Request::new(rocket, method, uri); + + // Set the passed in connection metadata. if let Some(connection) = connection { request.connection = connection; } - // Determine the host. On HTTP < 2, use the `HOST` header. Otherwise, + // Determine + set host. On HTTP < 2, use the `HOST` header. Otherwise, // use the `:authority` pseudo-header which hyper makes part of the URI. request.state.host = if hyper.version < hyper::Version::HTTP_2 { hyper.headers.get("host").and_then(|h| Host::parse_bytes(h.as_bytes()).ok()) @@ -1031,32 +1045,32 @@ impl<'r> Request<'r> { request.add_header(Header::new(name.as_str(), value)); } - Ok(request) - } -} - -#[derive(Debug)] -pub(crate) enum Error<'r> { - InvalidUri(&'r hyper::Uri), - UriParse(crate::http::uri::Error<'r>), - BadMethod(&'r hyper::Method), -} - -impl fmt::Display for Error<'_> { - /// Pretty prints a Request. This is primarily used by Rocket's logging - /// infrastructure. - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Error::InvalidUri(u) => write!(f, "invalid origin URI: {}", u), - Error::UriParse(u) => write!(f, "URI `{}` failed to parse as origin", u), - Error::BadMethod(m) => write!(f, "invalid or unrecognized method: {}", m), + if errors.is_empty() { + Ok(request) + } else { + Err(BadRequest { request, errors }) } } } -impl<'r> From> for Error<'r> { - fn from(uri_parse: crate::http::uri::Error<'r>) -> Self { - Error::UriParse(uri_parse) +#[derive(Debug)] +pub(crate) struct BadRequest<'r> { + pub request: Request<'r>, + pub errors: Vec>, +} + +#[derive(Debug)] +pub(crate) enum Kind<'r> { + InvalidUri(&'r hyper::Uri), + BadMethod(&'r hyper::Method), +} + +impl fmt::Display for Kind<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Kind::InvalidUri(u) => write!(f, "invalid origin URI: {}", u), + Kind::BadMethod(m) => write!(f, "invalid or unrecognized method: {}", m), + } } } @@ -1073,8 +1087,7 @@ impl fmt::Debug for Request<'_> { } impl fmt::Display for Request<'_> { - /// Pretty prints a Request. This is primarily used by Rocket's logging - /// infrastructure. + /// Pretty prints a Request. Primarily used by Rocket's logging. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{} {}", Paint::green(self.method()), Paint::blue(&self.uri))?; diff --git a/core/lib/src/server.rs b/core/lib/src/server.rs index 45357cdc..5a71c5bb 100644 --- a/core/lib/src/server.rs +++ b/core/lib/src/server.rs @@ -15,7 +15,7 @@ use crate::error::{Error, ErrorKind}; use crate::ext::{AsyncReadExt, CancellableListener, CancellableIo}; use crate::request::ConnectionMeta; -use crate::http::{uri::Origin, hyper, Method, Status, Header}; +use crate::http::{hyper, Method, Status, Header}; use crate::http::private::{TcpListener, Listener, Connection, Incoming}; // A token returned to force the execution of one method before another. @@ -83,12 +83,11 @@ async fn hyper_service_fn( rocket.send_response(response, tx).await; }, Err(e) => { - // TODO: We don't have a request to pass in, so we fabricate - // one. This is weird. Instead, let the user know that we failed - // to parse a request (a special handler?). - error!("Bad incoming request: {}", e); - let dummy = Request::new(&rocket, Method::Get, Origin::ROOT); - let response = rocket.handle_error(Status::BadRequest, &dummy).await; + warn!("Bad incoming HTTP request."); + e.errors.iter().for_each(|e| warn_!("Error: {}.", e)); + warn_!("Dispatching salvaged request to catcher: {}.", e.request); + + let response = rocket.handle_error(Status::BadRequest, &e.request).await; rocket.send_response(response, tx).await; } }