From f54bca937ca46d8fc18cc7786c66dc9146e63619 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 6 Nov 2024 11:25:10 -0800 Subject: [PATCH] add `recommended_status_code()` to dropshot errors As I proposed in [this comment][1]. This is intended to be used by user code that constructs its own error type from Dropshot's errors, to make it easier to get the same status codes that are used by `HttpError` when the custom user error type just structures the response differently, or only wishes to override a small subset of Dropshot errors. Perhaps this should be a trait eventually --- I'm kinda on the fence about this. We may also want to do a similar "recommended headers" thing, since some error conditions are supposed to return specific headers, like which methods are allowed for a Method Not Allowed error code, or the desired protocol to upgrade to for a 426 Upgrade Required error. While I was here, I also changed some error variants to return more correct status codes --- a bunch of stuff currently just returns 400 Bad Request when it should really return a more specific error like 426 Upgrade Required etc. [1]: https://github.com/oxidecomputer/dropshot/pull/1164#discussion_r1830220270 --- dropshot/src/error.rs | 46 +++++++++++++++---- dropshot/src/extractor/body.rs | 81 +++++++++++++++++++++++++++++++-- dropshot/src/extractor/mod.rs | 18 ++++++++ dropshot/src/extractor/query.rs | 17 ++++++- dropshot/src/http_util.rs | 17 ++++++- dropshot/src/router.rs | 31 +++++++------ dropshot/src/websocket.rs | 59 +++++++++++++++++++++--- 7 files changed, 233 insertions(+), 36 deletions(-) diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 567c555c..3e7816e2 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -68,6 +68,31 @@ pub enum ServerError { Response(ResponseError), } +impl From for HttpError { + fn from(error: ServerError) -> Self { + match error { + ServerError::Route(e) => e.into(), + ServerError::Extractor(e) => e.into(), + ServerError::Response(e) => e.into(), + } + } +} + +impl ServerError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` + /// implementation for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + Self::Route(e) => e.recommended_status_code(), + Self::Extractor(e) => e.recommended_status_code(), + Self::Response(e) => e.recommended_status_code(), + } + } +} + #[derive(Debug, thiserror::Error)] pub enum ResponseError { #[error(transparent)] @@ -88,22 +113,23 @@ pub enum ResponseError { }, } -impl From for HttpError { - fn from(error: ServerError) -> Self { - match error { - ServerError::Route(e) => e.into(), - ServerError::Extractor(e) => e.into(), - ServerError::Response(e) => e.into(), - } - } -} - impl From for HttpError { fn from(error: ResponseError) -> Self { HttpError::for_internal_error(error.to_string()) } } +impl ResponseError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` + /// implementation for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + http::StatusCode::INTERNAL_SERVER_ERROR + } +} + /// Trait implemented by errors which can be converted into an HTTP response. /// /// In order to be returned as an error from a Dropshot endpoint handler, a type diff --git a/dropshot/src/extractor/body.rs b/dropshot/src/extractor/body.rs index 651618d4..11ada181 100644 --- a/dropshot/src/extractor/body.rs +++ b/dropshot/src/extractor/body.rs @@ -68,8 +68,31 @@ pub enum MultipartBodyError { } impl From for HttpError { - fn from(value: MultipartBodyError) -> Self { - HttpError::for_bad_request(None, value.to_string()) + fn from(error: MultipartBodyError) -> Self { + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) + } +} + +impl MultipartBodyError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` implementation + /// for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + // Invalid or unsupported content-type headers should return 415 + // Unsupported Media Type + Self::MissingContentType | Self::InvalidContentType(_) => { + http::StatusCode::UNSUPPORTED_MEDIA_TYPE + } + // Everything else gets a generic `400 Bad Request`. + _ => http::StatusCode::BAD_REQUEST, + } } } @@ -150,7 +173,36 @@ pub enum TypedBodyError { impl From for HttpError { fn from(error: TypedBodyError) -> Self { - HttpError::for_bad_request(None, error.to_string()) + match error { + TypedBodyError::StreamingBody(e) => e.into(), + _ => HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ), + } + } +} + +impl TypedBodyError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` implementation + /// for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + // Invalid or unsupported content-type headers should return 415 + // Unsupported Media Type + Self::InvalidContentType(_) + | Self::UnsupportedMimeType(_) + | Self::UnexpectedMimeType { .. } => { + http::StatusCode::UNSUPPORTED_MEDIA_TYPE + } + Self::StreamingBody(e) => e.recommended_status_code(), + // Everything else gets a generic `400 Bad Request`. + _ => http::StatusCode::BAD_REQUEST, + } } } @@ -312,7 +364,28 @@ pub enum StreamingBodyError { impl From for HttpError { fn from(error: StreamingBodyError) -> Self { - HttpError::for_bad_request(None, error.to_string()) + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) + } +} + +impl StreamingBodyError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` implementation + /// for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + // If the max body size was exceeded, return 413 Payload Too Large + // (nee Content Too Large). + Self::MaxSizeExceeded(_) => http::StatusCode::PAYLOAD_TOO_LARGE, + // Everything else gets a generic `400 Bad Request`. + _ => http::StatusCode::BAD_REQUEST, + } } } diff --git a/dropshot/src/extractor/mod.rs b/dropshot/src/extractor/mod.rs index c9d0e984..2a240a64 100644 --- a/dropshot/src/extractor/mod.rs +++ b/dropshot/src/extractor/mod.rs @@ -75,3 +75,21 @@ impl From for HttpError { } } } + +impl ExtractorError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` + /// implementation for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + Self::MultipartBody(e) => e.recommended_status_code(), + Self::StreamingBody(e) => e.recommended_status_code(), + Self::TypedBody(e) => e.recommended_status_code(), + Self::PathParams(e) => e.recommended_status_code(), + Self::QueryParams(e) => e.recommended_status_code(), + Self::Websocket(e) => e.recommended_status_code(), + } + } +} diff --git a/dropshot/src/extractor/query.rs b/dropshot/src/extractor/query.rs index 900396e0..1895541a 100644 --- a/dropshot/src/extractor/query.rs +++ b/dropshot/src/extractor/query.rs @@ -40,7 +40,22 @@ pub struct QueryError(#[from] serde_urlencoded::de::Error); impl From for HttpError { fn from(error: QueryError) -> Self { - HttpError::for_bad_request(None, error.to_string()) + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) + } +} + +impl QueryError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` implementation + /// for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + http::StatusCode::BAD_REQUEST } } diff --git a/dropshot/src/http_util.rs b/dropshot/src/http_util.rs index d65f14cb..37c35c06 100644 --- a/dropshot/src/http_util.rs +++ b/dropshot/src/http_util.rs @@ -55,7 +55,22 @@ pub struct PathError(String); impl From for HttpError { fn from(error: PathError) -> Self { - HttpError::for_bad_request(None, error.to_string()) + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) + } +} + +impl PathError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` implementation + /// for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + http::StatusCode::BAD_REQUEST } } diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index af7a9816..49908817 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -236,25 +236,30 @@ impl RouterError { pub fn is_not_found(&self) -> bool { matches!(self, Self::NotFound(_)) } + + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` + /// implementation for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + Self::InvalidPath(_) => http::StatusCode::BAD_REQUEST, + Self::NotFound(_) => http::StatusCode::NOT_FOUND, + Self::MethodNotAllowed => http::StatusCode::METHOD_NOT_ALLOWED, + } + } } impl From for HttpError { fn from(error: RouterError) -> Self { - match error { - RouterError::InvalidPath(_) => { - HttpError::for_bad_request(None, error.to_string()) - } - RouterError::NotFound(s) => { - HttpError::for_not_found(None, s.to_string()) - } - RouterError::MethodNotAllowed => HttpError::for_status( - None, - http::StatusCode::METHOD_NOT_ALLOWED, - ), - } + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) } } - impl HttpRouter { /// Returns a new `HttpRouter` with no routes configured. pub fn new() -> Self { diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index e5d2a459..07407332 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -93,15 +93,59 @@ pub enum WebsocketUpgradeError { NoUpgradeHeader, #[error("unexpected protocol for upgrade")] NotWebsocket, - #[error("missing or invalid websocket version")] - BadVersion, + #[error("missing websocket version header")] + NoVersion, + #[error("unsupported websocket version")] + WrongVersion, #[error("missing websocket key")] NoKey, } impl From for HttpError { - fn from(e: WebsocketUpgradeError) -> Self { - HttpError::for_bad_request(None, e.to_string()) + fn from(error: WebsocketUpgradeError) -> Self { + HttpError::for_client_error( + None, + error.recommended_status_code(), + error.to_string(), + ) + } +} + +impl WebsocketUpgradeError { + /// Returns the recommended status code for this error. + /// + /// This can be used when constructing a HTTP response for this error. These + /// are the status codes used by the `From` + /// implementation for [`HttpError`]. + pub fn recommended_status_code(&self) -> http::StatusCode { + match self { + // TODO(eliza): in this case, the response should also include an + // `Upgrade: websocket` header to indicate what protocol the client + // must upgrade to. We should eventually figure out an API for + // errors to indicate "recommended headers" as well as recommended + // statuses... + Self::NoUpgradeHeader | Self::NotWebsocket => { + http::StatusCode::UPGRADE_REQUIRED + } + // Per RFC 6455 § 4.2.2, if the client has requested an unsupported + // websocket version, the server should respond with a 426 Upgrade + // Required status code: + // https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.2 + // + // Again, we should also include a header (Sec-WebSocket-Version) + // indicating the supported cversions, but we gotta figure that out + // later... + Self::WrongVersion => http::StatusCode::UPGRADE_REQUIRED, + // Note that we differentiate between "missing version header" and + // "wrong version" because RFC 6455 § 4.2.1 kind of vaguely implies + // that if any of the expected websocket headers are not present, + // the server responds with a 400, but if the version header is + // present but has the wrong value, that's an 426 Upgrade Required: + // https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.1 + Self::NoVersion => http::StatusCode::BAD_REQUEST, + // Similarly, a missing `Sec-Websocket-Key` also gets Bad Request'd. + Self::NoKey => http::StatusCode::BAD_REQUEST, + } } } @@ -143,10 +187,11 @@ impl ExclusiveExtractor for WebsocketUpgrade { if request .headers() .get(header::SEC_WEBSOCKET_VERSION) - .map(|v| v.as_bytes()) - != Some(b"13") + .ok_or(WebsocketUpgradeError::NoVersion)? + .as_bytes() + != b"13" { - return Err(WebsocketUpgradeError::BadVersion.into()); + return Err(WebsocketUpgradeError::WrongVersion.into()); } let accept_key = request