From 69cf113ef34654c33951c39f7064422a0eab6fbf Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 10:44:49 -0800 Subject: [PATCH 1/8] Add headers to `HttpError` --- dropshot/src/error.rs | 104 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 103 insertions(+), 1 deletion(-) diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 5cca098f..7109098a 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -118,6 +118,9 @@ pub struct HttpError { pub external_message: String, /// Error message recorded in the log for this error pub internal_message: String, + /// Headers which should be added to error responses generated from this + /// error. + pub headers: Option>, } /// Body of an HTTP response for an `HttpError`. This type can be used to @@ -212,6 +215,7 @@ impl HttpError { error_code, internal_message: message.clone(), external_message: message, + headers: None, } } @@ -227,6 +231,7 @@ impl HttpError { .unwrap() .to_string(), internal_message, + headers: None, } } @@ -245,6 +250,7 @@ impl HttpError { .unwrap() .to_string(), internal_message, + headers: None, } } @@ -290,15 +296,111 @@ impl HttpError { error_code, internal_message, external_message, + headers: None, } } + /// Mutably borrow the `http::HeaderMap` + pub fn headers_mut(&mut self) -> Option<&mut http::HeaderMap> { + self.headers.as_deref_mut() + } + + /// Adds a header to the [`http::HeaderMap`] of headers to add to responses + /// generated from this error. + /// + /// If this error does not already have a header map (`self.header_map` is + /// `None`), this method creates one. + /// + /// # Returns + /// - [`Ok`]`(&mut Self)` if the provided `name` and `value` are a valid + /// header name and value, respectively. + /// - [`Err`]`(`[`http::Error`]`)` if the header name or value is invalid, + /// or the `HeaderMap` is full. + pub fn set_header( + &mut self, + name: K, + value: V, + ) -> Result<&mut Self, http::Error> + where + http::HeaderName: TryFrom, + >::Error: Into, + http::HeaderValue: TryFrom, + >::Error: Into, + { + let name = >::try_from(name) + .map_err(Into::into)?; + let value = >::try_from(value) + .map_err(Into::into)?; + self.headers + .get_or_insert_with(|| Box::new(http::HeaderMap::default())) + .try_append(name, value)?; + Ok(self) + } + + /// Adds a header to the [`http::HeaderMap`] of headers to add to responses + /// generated from this error, taking the error by value. + /// + /// If this error does not already have a header map (`self.header_map` is + /// `None`), this method creates one. + /// + /// Unlike [`HttpError::set_header`], this method takes `self` by value, + /// allowing it to be chained to form an expression that returns an + /// `HttpError`. However, because this takes `self` by value, returning an + /// error for an invalid header name or value will discard the `HttpError`. + /// To avoid this, use [`HttpError::set_header`] instead. + /// + /// # Returns + /// + /// - [`Ok`]`(`Self`)` if the provided `name` and `value` are a valid + /// header name and value, respectively. + /// - [`Err`]`(`[`http::Error`]`)` if the header name or value is invalid, + /// or the `HeaderMap` is full. + pub fn with_header( + mut self, + name: K, + value: V, + ) -> Result + where + http::HeaderName: TryFrom, + >::Error: Into, + http::HeaderValue: TryFrom, + >::Error: Into, + { + let name = >::try_from(name) + .map_err(Into::into)?; + let value = >::try_from(value) + .map_err(Into::into)?; + self.headers + .get_or_insert_with(|| Box::new(http::HeaderMap::default())) + .try_append(name, value)?; + Ok(self) + } + /// Generates an HTTP response for the given `HttpError`, using `request_id` /// for the response's request id. pub fn into_response( self, request_id: &str, ) -> hyper::Response { + let mut builder = hyper::Response::builder(); + + // Set the builder's initial `HeaderMap` to the headers recommended by + // the error, if any exist. Replacing the initial `HeaderMap` is more + // efficient than inserting each header from `self.headers` into the + // builder one-by-one, as we can just reuse the existing header map. + // It's fine to clobber the builder's header map, as we just created the + // builder and the header map is empty. + if let Some(headers) = self.headers { + let builder_headers = builder + .headers_mut() + // `headers_mut()` returns `None` in the case that the builder is in + // a failed state due to setting an invalid header or status code. + // However, we *just* constructed this builder, so it cannot be in + // an error state, as we haven't called any methods on it yet. + .expect("a newly created response builder cannot have failed"); + *builder_headers = *headers; + } + // TODO-hardening: consider handling the operational errors that the // Serde serialization fails or the response construction fails. In // those cases, we should probably try to report this as a serious @@ -307,7 +409,7 @@ impl HttpError { // there's only one possible set of input and we can test it. We'll // probably have to use unwrap() there and make sure we've tested that // code at least once!) - hyper::Response::builder() + builder .status(self.status_code.as_status()) .header( http::header::CONTENT_TYPE, From 9cead092e6b5fab060f7107458588781fcda2e2a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 10:56:46 -0800 Subject: [PATCH 2/8] Add headers to HttpError, emit Allow headers For example: ```console eliza@noctis ~/Code/dropshot $ cargo run --example basic & Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s Running `target/debug/examples/basic` Dec 04 18:51:47.826 INFO listening, local_addr: 127.0.0.1:41751 eliza@noctis ~/Code/dropshot $ curl -v http://localhost:41751/counter -X DELETE * Host localhost:41751 was resolved. * IPv6: ::1 * IPv4: 127.0.0.1 * Trying [::1]:41751... * connect to ::1 port 41751 from ::1 port 53992 failed: Connection refused * Trying 127.0.0.1:41751... * Connected to localhost (127.0.0.1) port 41751 * using HTTP/1.x > DELETE /counter HTTP/1.1 > Host: localhost:41751 > User-Agent: curl/8.11.0 > Accept: */* > * Request completely sent off Dec 04 18:59:14.913 INFO accepted connection, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751 Dec 04 18:59:14.917 INFO request completed, error_message_external: Method Not Allowed, error_message_internal: Method Not Allowed, latency_us: 834, response_code: 405, uri: /counter, method: DELETE, req_id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751 < HTTP/1.1 405 Method Not Allowed < allow: GET < allow: PUT < content-type: application/json < x-request-id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793 < content-length: 93 < date: Wed, 04 Dec 2024 18:59:14 GMT < { "request_id": "e1d14d57-ca67-49cc-84b0-d1dc1e0e5793", "message": "Method Not Allowed" * Connection #0 to host localhost left intact }% ``` --- CHANGELOG.adoc | 31 +++++++++++++++++++++++++++++++ dropshot/src/error.rs | 8 ++++++++ dropshot/src/router.rs | 15 +++++++++++++-- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index ffb4b4ab..8c410916 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -85,6 +85,37 @@ Additionally, `ErrorStatusCode` provides an `as_client_error` method that returns a `ClientErrorStatusCode` if the status code is a client error, or an error. +* `HttpError` now contains an `Option>` of headers to add +to error responses constructed for the error. ++ +Code which constructs `HttpError` literals must now initialize this field. For +example, this will no longer compile: ++ +```rust +let err = dropshot::HttpError { + status_code: ErrorStatusCode::INTERNAL_SERVER_ERROR, + error_code: None + internal_message: "something bad happened".to_string(), + external_message: "Internal Error".to_string(), +}; +``` ++ +Instead, write: ++ +```rust +let err = dropshot::HttpError { + status_code: ErrorStatusCode::INTERNAL_SERVER_ERROR, + error_code: None + internal_message: "something bad happened".to_string(), + external_message: "Internal Error".to_string(), + headers: None, // <-- Add this +}; +``` ++ +Similarly, code which destructures `HttpError`s, such as in a `match` or `let` +expression, must now either destructure the `headers` field or add `..` to +ignore it. + === Other notable changes * Endpoint handler functions may now return any error type that implements the diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 7109098a..7d46e5ca 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -120,6 +120,14 @@ pub struct HttpError { pub internal_message: String, /// Headers which should be added to error responses generated from this /// error. + /// + /// The [`http::HeaderMap`] is boxed to reduce the size of `HttpError`s + /// which don't contain a `HeaderMap` (the common case). Unlike containers + /// such as `Vec` and `HashMap`, an empty [`http::HeaderMap`] contains a + /// fairly large number of fields, rather than being a pointer to a heap + /// allocation containing the actual data. Thus, we store an + /// `Option>` here, so that the empty case is just a null + /// pointer. pub headers: Option>, } diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 8211fffe..a840234c 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -532,10 +532,21 @@ impl HttpRouter { if node.methods.values().any(|handlers| { find_handler_matching_version(handlers, version).is_some() }) { - Err(HttpError::for_client_error_with_status( + let mut err = HttpError::for_client_error_with_status( None, ClientErrorStatusCode::METHOD_NOT_ALLOWED, - )) + ); + + // Add `Allow` headers for the methods that *are* acceptable for + // this path. + if let Some(hdrs) = err.headers_mut() { + hdrs.reserve(node.methods.len()); + } + for allowed in node.methods.keys() { + err.set_header(http::header::ALLOW, allowed) + .expect("method should be a valid allow header"); + } + Err(err) } else { Err(HttpError::for_not_found( None, From 8ff6edc10a7c3100ec6dae473eb080343ba89440 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 11:07:44 -0800 Subject: [PATCH 3/8] naming/API tweaks --- dropshot/src/error.rs | 11 +++++++++-- dropshot/src/router.rs | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 7d46e5ca..bf277a85 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -308,11 +308,18 @@ impl HttpError { } } - /// Mutably borrow the `http::HeaderMap` + /// Mutably borrow the `http::HeaderMap` associated with this error, if one + /// exists. pub fn headers_mut(&mut self) -> Option<&mut http::HeaderMap> { self.headers.as_deref_mut() } + /// Borrow the `http::HeaderMap` associated with this error, if one + /// exists. + pub fn headers_ref(&self) -> Option<&http::HeaderMap> { + self.headers.as_deref() + } + /// Adds a header to the [`http::HeaderMap`] of headers to add to responses /// generated from this error. /// @@ -324,7 +331,7 @@ impl HttpError { /// header name and value, respectively. /// - [`Err`]`(`[`http::Error`]`)` if the header name or value is invalid, /// or the `HeaderMap` is full. - pub fn set_header( + pub fn add_header( &mut self, name: K, value: V, diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index a840234c..a06c6f4f 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -543,7 +543,7 @@ impl HttpRouter { hdrs.reserve(node.methods.len()); } for allowed in node.methods.keys() { - err.set_header(http::header::ALLOW, allowed) + err.add_header(http::header::ALLOW, allowed) .expect("method should be a valid allow header"); } Err(err) From 69bfd56b981a06e9d99aeb631c1084b0f2f0a2b4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 12:59:46 -0800 Subject: [PATCH 4/8] Copyedits from @ahl Co-authored-by: Adam Leventhal --- dropshot/src/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index bf277a85..59420111 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -118,7 +118,7 @@ pub struct HttpError { pub external_message: String, /// Error message recorded in the log for this error pub internal_message: String, - /// Headers which should be added to error responses generated from this + /// Headers that will be added to responses generated from this /// error. /// /// The [`http::HeaderMap`] is boxed to reduce the size of `HttpError`s @@ -399,7 +399,7 @@ impl HttpError { ) -> hyper::Response { let mut builder = hyper::Response::builder(); - // Set the builder's initial `HeaderMap` to the headers recommended by + // Set the builder's initial `HeaderMap` to the headers specified by // the error, if any exist. Replacing the initial `HeaderMap` is more // efficient than inserting each header from `self.headers` into the // builder one-by-one, as we can just reuse the existing header map. From 82d4f91c05cfa111952f08099011684416d72939 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 13:03:58 -0800 Subject: [PATCH 5/8] cite RFC9110 --- dropshot/src/router.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index a06c6f4f..000cc83a 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -538,7 +538,13 @@ impl HttpRouter { ); // Add `Allow` headers for the methods that *are* acceptable for - // this path. + // this path, as specified in ยง 15.5.0 RFC9110, which states: + // + // > The origin server MUST generate an Allow header field in a + // > 405 response containing a list of the target resource's + // > currently supported methods. + // + // See: https://httpwg.org/specs/rfc9110.html#status.405 if let Some(hdrs) = err.headers_mut() { hdrs.reserve(node.methods.len()); } From a34b66c1a2c2293a23cfa4ac3f3bd54effffe053 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 13:47:31 -0800 Subject: [PATCH 6/8] docs minimalism --- CHANGELOG.adoc | 28 +--------------------------- dropshot/src/error.rs | 13 +++++-------- 2 files changed, 6 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 8c410916..c39f17ae 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -88,33 +88,7 @@ error. * `HttpError` now contains an `Option>` of headers to add to error responses constructed for the error. + -Code which constructs `HttpError` literals must now initialize this field. For -example, this will no longer compile: -+ -```rust -let err = dropshot::HttpError { - status_code: ErrorStatusCode::INTERNAL_SERVER_ERROR, - error_code: None - internal_message: "something bad happened".to_string(), - external_message: "Internal Error".to_string(), -}; -``` -+ -Instead, write: -+ -```rust -let err = dropshot::HttpError { - status_code: ErrorStatusCode::INTERNAL_SERVER_ERROR, - error_code: None - internal_message: "something bad happened".to_string(), - external_message: "Internal Error".to_string(), - headers: None, // <-- Add this -}; -``` -+ -Similarly, code which destructures `HttpError`s, such as in a `match` or `let` -expression, must now either destructure the `headers` field or add `..` to -ignore it. +Code which constructs `HttpError` literals must now initialize this field. === Other notable changes diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 59420111..04a0ad0d 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -120,14 +120,11 @@ pub struct HttpError { pub internal_message: String, /// Headers that will be added to responses generated from this /// error. - /// - /// The [`http::HeaderMap`] is boxed to reduce the size of `HttpError`s - /// which don't contain a `HeaderMap` (the common case). Unlike containers - /// such as `Vec` and `HashMap`, an empty [`http::HeaderMap`] contains a - /// fairly large number of fields, rather than being a pointer to a heap - /// allocation containing the actual data. Thus, we store an - /// `Option>` here, so that the empty case is just a null - /// pointer. + // This is boxed in obeisance to Clippy's `result_large_err` lint + // (https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err). + // While we could allow that lint for Dropshot, `HttpError` will also be a + // common return type for consumers of Dropshot, so let's not force users to + // also allow the lint. pub headers: Option>, } From d69f141d3dbd210cfb91a92f5a8cf9a4e55562a1 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 13:50:01 -0800 Subject: [PATCH 7/8] make the API more like `HttpResponseHeaders` --- dropshot/src/error.rs | 22 ++++++---------------- dropshot/src/router.rs | 2 +- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/dropshot/src/error.rs b/dropshot/src/error.rs index 04a0ad0d..03ed3ab6 100644 --- a/dropshot/src/error.rs +++ b/dropshot/src/error.rs @@ -305,16 +305,10 @@ impl HttpError { } } - /// Mutably borrow the `http::HeaderMap` associated with this error, if one - /// exists. - pub fn headers_mut(&mut self) -> Option<&mut http::HeaderMap> { - self.headers.as_deref_mut() - } - - /// Borrow the `http::HeaderMap` associated with this error, if one - /// exists. - pub fn headers_ref(&self) -> Option<&http::HeaderMap> { - self.headers.as_deref() + /// Mutably borrow the `http::HeaderMap` associated with this error. If + /// there is no header map for this error, this method creates one. + pub fn headers_mut(&mut self) -> &mut http::HeaderMap { + self.headers.get_or_insert_with(|| Box::new(http::HeaderMap::new())) } /// Adds a header to the [`http::HeaderMap`] of headers to add to responses @@ -343,9 +337,7 @@ impl HttpError { .map_err(Into::into)?; let value = >::try_from(value) .map_err(Into::into)?; - self.headers - .get_or_insert_with(|| Box::new(http::HeaderMap::default())) - .try_append(name, value)?; + self.headers_mut().try_append(name, value)?; Ok(self) } @@ -382,9 +374,7 @@ impl HttpError { .map_err(Into::into)?; let value = >::try_from(value) .map_err(Into::into)?; - self.headers - .get_or_insert_with(|| Box::new(http::HeaderMap::default())) - .try_append(name, value)?; + self.headers_mut().try_append(name, value)?; Ok(self) } diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 000cc83a..e624a221 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -545,7 +545,7 @@ impl HttpRouter { // > currently supported methods. // // See: https://httpwg.org/specs/rfc9110.html#status.405 - if let Some(hdrs) = err.headers_mut() { + if let Some(hdrs) = err.headers.as_deref_mut() { hdrs.reserve(node.methods.len()); } for allowed in node.methods.keys() { From 5f6060a481d3f243e632962ce7923a63404a6367 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Dec 2024 14:14:25 -0800 Subject: [PATCH 8/8] Update CHANGELOG.adoc Co-authored-by: Adam Leventhal --- CHANGELOG.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index c39f17ae..c3c27449 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -88,7 +88,7 @@ error. * `HttpError` now contains an `Option>` of headers to add to error responses constructed for the error. + -Code which constructs `HttpError` literals must now initialize this field. +Code that constructs `HttpError` literals must now initialize this field. === Other notable changes