Turn non-200 HTTP responses into an error instead of trying to parse …

…the body as a JSON-RPC response (#1361)

* Turn non-200 HTTP responses into an error instead of trying to parse the body as a JSON-RPC response

* `Url::authority(&self)` requires `url` >=2.4.1

* Feature guard status code in `HttpRequestFailed` error variant

When the `reqwest` feature is disabled, this now falls back to `NonZeroU16`,
even though that variant is never actually created.

A better solution would be to feature-guard the error variant itself,
but thats not supported by `flex-error` at the moment.

* Add changelog entry

.changelog/unreleased/improvements/1359-hande-http-error.md

@@ -0,0 +1,3 @@
+- `[tendermint-rpc]` Turn non-200 HTTP response into an error
+  instead of trying to parse the body as a JSON-RPC response
+  ([\#1359](https://github.com/informalsystems/tendermint-rs/issues/1359))

rpc/Cargo.toml

@@ -71,7 +71,7 @@ thiserror = { version = "1", default-features = false }
 time = { version = "0.3", default-features = false, features = ["macros", "parsing"] }
 uuid = { version = "0.8", default-features = false }
 subtle-encoding = { version = "0.5", default-features = false, features = ["bech32-preview"] }
-url = { version = "2.2", default-features = false }
+url = { version = "2.4.1", default-features = false }
 walkdir = { version = "2.3", default-features = false }
 flex-error = { version = "0.4.4", default-features = false }
 subtle = { version = "2", default-features = false }

rpc/src/client/transport/http.rs

@@ -160,7 +160,7 @@ impl HttpClient {
     {
         let request_body = request.into_json();
 
-        tracing::trace!("outgoing request: {}", request_body);
+        tracing::debug!(url = %self.url, body = %request_body, "outgoing request");
 
         let mut builder = self
             .inner
@@ -182,11 +182,23 @@ impl HttpClient {
     {
         let request = self.build_request(request)?;
         let response = self.inner.execute(request).await.map_err(Error::http)?;
+        let response_status = response.status();
         let response_body = response.bytes().await.map_err(Error::http)?;
-        tracing::trace!(
-            "incoming response: {}",
-            String::from_utf8_lossy(&response_body)
+
+        tracing::debug!(
+            status = %response_status,
+            body = %String::from_utf8_lossy(&response_body),
+            "incoming response"
         );
+
+        // Successful JSON-RPC requests are expected to return a 200 OK HTTP status.
+        // Otherwise, this means that the HTTP request failed as a whole,
+        // as opposed to the JSON-RPC request returning an error,
+        // and we cannot expect the response body to be a valid JSON-RPC response.
+        if response_status != reqwest::StatusCode::OK {
+            return Err(Error::http_request_failed(response_status));
+        }
+
         R::Response::from_string(&response_body).map(Into::into)
     }
 }

rpc/src/error.rs

@@ -12,6 +12,12 @@ type ReqwestError = flex_error::DisplayOnly<reqwest::Error>;
 #[cfg(not(feature = "reqwest"))]
 type ReqwestError = flex_error::NoSource;
 
+#[cfg(feature = "reqwest")]
+type HttpStatusCode = reqwest::StatusCode;
+
+#[cfg(not(feature = "reqwest"))]
+type HttpStatusCode = core::num::NonZeroU16;
+
 #[cfg(feature = "tokio")]
 type JoinError = flex_error::DisplayOnly<tokio::task::JoinError>;
 
@@ -77,6 +83,14 @@ define_error! {
                 format_args!("method not found: {}", e.method)
             },
 
+        HttpRequestFailed
+            {
+                status: HttpStatusCode,
+            }
+            | e | {
+                format_args!("HTTP request failed with non-200 status code: {}", e.status)
+            },
+
         Parse
             {
                 reason: String