Skip to content

Commit

Permalink
feat(error): preserve InvalidUri details
Browse files Browse the repository at this point in the history
This branch changes the representation of the `Parse::Uri` error kind to
preserve information provided by the `http` crate's `uri::InvalidUri`
and `uri::InvalidUriParts` errors. A new `InvalidUri` enum is added that
holds one of those two error types, or a custom message (since `hyper`
currently returns `Parse::Uri` errors that didn't come from an inner
`http` error in some cases). The new enum has a custom `Display` and
`Debug` implementation to reduce repetition of the string "invalid URI"
in its formatted output.

This is _not_ stored as the error's `cause` currently in order to avoid
exposing the `http` crate's error types in the public API. However, when
`http` 1.0 is released, we can simplify this code significantly by
storing the error as a cause and exposing it in the source chain.

Closes #3043
  • Loading branch information
hawkw committed Nov 4, 2022
1 parent 75aac9f commit c3e4bb3
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 9 deletions.
49 changes: 43 additions & 6 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub(super) enum Parse {
Version,
#[cfg(feature = "http1")]
VersionH2,
Uri,
Uri(InvalidUri),
#[cfg_attr(not(all(feature = "http1", feature = "server")), allow(unused))]
UriTooLong,
Header(Header),
Expand Down Expand Up @@ -123,6 +123,14 @@ pub(super) enum User {
#[derive(Debug)]
pub(super) struct TimedOut;

pub(super) enum InvalidUri {
Uri(http::uri::InvalidUri),
Parts(http::uri::InvalidUriParts),

#[cfg(feature = "server")]
Other(&'static str),
}

impl Error {
/// Returns true if this was an HTTP parse error.
pub fn is_parse(&self) -> bool {
Expand Down Expand Up @@ -343,7 +351,7 @@ impl Error {
Kind::Parse(Parse::Version) => "invalid HTTP version parsed",
#[cfg(feature = "http1")]
Kind::Parse(Parse::VersionH2) => "invalid HTTP version parsed (found HTTP2 preface)",
Kind::Parse(Parse::Uri) => "invalid URI",
Kind::Parse(Parse::Uri(_)) => "invalid URI",
Kind::Parse(Parse::UriTooLong) => "URI too long",
Kind::Parse(Parse::Header(Header::Token)) => "invalid HTTP header parsed",
#[cfg(feature = "http1")]
Expand Down Expand Up @@ -420,6 +428,11 @@ impl fmt::Debug for Error {

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// handle "invalid URI" parse errors separately, since the inner error
// is not stored as a cause.
if let Kind::Parse(Parse::Uri(ref error)) = self.inner.kind {
return fmt::Display::fmt(error, f);
}
if let Some(ref cause) = self.inner.cause {
write!(f, "{}: {}", self.description(), cause)
} else {
Expand Down Expand Up @@ -487,14 +500,14 @@ impl From<http::status::InvalidStatusCode> for Parse {
}

impl From<http::uri::InvalidUri> for Parse {
fn from(_: http::uri::InvalidUri) -> Parse {
Parse::Uri
fn from(inner: http::uri::InvalidUri) -> Parse {
Parse::Uri(InvalidUri::Uri(inner))
}
}

impl From<http::uri::InvalidUriParts> for Parse {
fn from(_: http::uri::InvalidUriParts) -> Parse {
Parse::Uri
fn from(inner: http::uri::InvalidUriParts) -> Parse {
Parse::Uri(InvalidUri::Parts(inner))
}
}

Expand All @@ -513,6 +526,30 @@ impl fmt::Display for TimedOut {

impl StdError for TimedOut {}

// === impl InvalidUri ===

impl fmt::Display for InvalidUri {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
#[cfg(feature = "server")]
Self::Other(msg) => write!(f, "invalid URI: {}", msg),
Self::Uri(e) => fmt::Display::fmt(e, f),
Self::Parts(e) => fmt::Display::fmt(e, f),
}
}
}

impl fmt::Debug for InvalidUri {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
#[cfg(feature = "server")]
Self::Other(msg) => f.debug_tuple("InvalidUri").field(msg).finish(),
Self::Uri(e) => fmt::Debug::fmt(e, f),
Self::Parts(e) => fmt::Debug::fmt(e, f),
}
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
6 changes: 3 additions & 3 deletions src/proto/h1/role.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use tracing::{debug, error, trace, trace_span, warn};
use crate::body::DecodedLength;
#[cfg(feature = "server")]
use crate::common::date;
use crate::error::Parse;
use crate::error::{InvalidUri, Parse};
use crate::ext::HeaderCaseMap;
#[cfg(feature = "ffi")]
use crate::ext::OriginalHeaderOrder;
Expand Down Expand Up @@ -182,7 +182,7 @@ impl Http1Transaction for Server {
Parse::Method
} else {
debug_assert!(req.path.is_none());
Parse::Uri
Parse::Uri(InvalidUri::Other("invalid token"))
}
}
other => other.into(),
Expand Down Expand Up @@ -445,7 +445,7 @@ impl Http1Transaction for Server {
let status = match *err.kind() {
Kind::Parse(Parse::Method)
| Kind::Parse(Parse::Header(_))
| Kind::Parse(Parse::Uri)
| Kind::Parse(Parse::Uri(_))
| Kind::Parse(Parse::Version) => StatusCode::BAD_REQUEST,
Kind::Parse(Parse::TooLarge) => StatusCode::REQUEST_HEADER_FIELDS_TOO_LARGE,
Kind::Parse(Parse::UriTooLong) => StatusCode::URI_TOO_LONG,
Expand Down

0 comments on commit c3e4bb3

Please sign in to comment.