add recommended_status_code() to dropshot errors

hawkw · hawkw · commit f54bca937ca4 · 2024-11-06T11:25:10.000-08:00
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]: #1164 (comment)
diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs
@@ -68,6 +68,31 @@ pub enum ServerError {
     Response(ResponseError),
 }
 
+impl From<ServerError> 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<ServerError>`
+    /// 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<ServerError> 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<ResponseError> 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<ResponseError>`
+    /// 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
@@ -68,8 +68,31 @@ pub enum MultipartBodyError {
 }
 
 impl From<MultipartBodyError> 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<MultipartBodyError>` 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<TypedBodyError> 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<TypedBodyError>` 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<StreamingBodyError> 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<StreamingBodyError>` 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
@@ -75,3 +75,21 @@ impl From<ExtractorError> 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<ExtractorError>`
+    /// 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
@@ -40,7 +40,22 @@ pub struct QueryError(#[from] serde_urlencoded::de::Error);
 
 impl From<QueryError> 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<QueryError>` 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
@@ -55,7 +55,22 @@ pub struct PathError(String);
 
 impl From<PathError> 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<PathError>` 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
@@ -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<RouterError>`
+    /// 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<RouterError> 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<Context: ServerContext> HttpRouter<Context> {
     /// Returns a new `HttpRouter` with no routes configured.
     pub fn new() -> Self {
diff --git 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<WebsocketUpgradeError> 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<WebsocketUpgradeError>`
+    /// 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
