Skip to content

Commit 71287a8

Browse files
authored
fix: remove api keys from RPC error messages (#5067)
1 parent fbf7cfa commit 71287a8

File tree

5 files changed

+172
-16
lines changed

5 files changed

+172
-16
lines changed

.changeset/brave-scissors-train.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@nomicfoundation/edr": patch
3+
---
4+
5+
fix: remove api keys from RPC error messages

Cargo.lock

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/edr_eth/Cargo.toml

+8-5
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,17 @@ version = "0.2.0-dev"
44
edition = "2021"
55

66
[dependencies]
7+
anyhow = "1.0.75"
78
alloy-primitives = { version = "0.6", default-features = false, features = ["rand", "rlp"] }
89
alloy-rlp = { version = "0.3", default-features = false, features = ["derive"] }
9-
futures = {version = "0.3.28", default-features = false}
10+
futures = { version = "0.3.28", default-features = false }
1011
hash-db = { version = "0.15.2", default-features = false }
1112
hash256-std-hasher = { version = "0.15.2", default-features = false }
1213
hex = { version = "0.4.3", default-features = false, features = ["alloc"] }
13-
hyper = {version = "0.14.27", default-features = false}
14+
hyper = { version = "0.14.27", default-features = false }
1415
itertools = { version = "0.10.5", default-features = false, features = ["use_alloc"] }
15-
k256 = { version = "0.13.1", default-features = false, features = ["arithmetic", "ecdsa", "pkcs8",] }
16+
k256 = { version = "0.13.1", default-features = false, features = ["arithmetic", "ecdsa", "pkcs8", ] }
17+
lazy_static = { version = "1.4.0", default-features = false }
1618
log = { version = "0.4.17", default-features = false }
1719
reqwest = { version = "0.11", features = ["blocking", "json"] }
1820
reqwest-middleware = { version = "0.2.4", default-features = false }
@@ -26,8 +28,9 @@ thiserror = { version = "1.0.37", default-features = false }
2628
tokio = { version = "1.21.2", default-features = false, features = ["fs", "sync"] }
2729
tracing = { version = "0.1.37", features = ["attributes", "std"], optional = true }
2830
triehash = { version = "0.8.4", default-features = false }
29-
uuid = { version = "1.4.1", default-features = false, features = ["v4"]}
31+
uuid = { version = "1.4.1", default-features = false, features = ["v4"] }
3032
url = "2.4.1"
33+
regex = "1.10.0"
3134

3235
[dev-dependencies]
3336
anyhow = "1.0.75"
@@ -40,7 +43,7 @@ edr_test_utils = { version = "0.2.0-dev", path = "../edr_test_utils" }
4043
serial_test = "2.0.0"
4144
tempfile = { version = "3.7.1", default-features = false }
4245
tokio = { version = "1.23.0", features = ["macros"] }
43-
walkdir = { version = "2.3.3", default-features = false }
46+
walkdir = { version = "2.3.3", default-features = false }
4447

4548
[features]
4649
default = ["std"]

crates/edr_eth/src/remote/client.rs

+20-11
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
mod reqwest_error;
2+
13
use std::{
24
collections::{HashMap, VecDeque},
35
fmt::Debug,
@@ -29,6 +31,7 @@ use super::{
2931
request_methods::RequestMethod,
3032
BlockSpec, PreEip1898BlockSpec,
3133
};
34+
pub use crate::remote::client::reqwest_error::{MiddlewareError, ReqwestError};
3235
use crate::{
3336
block::{block_time, is_safe_block_number, IsSafeBlockNumberArgs},
3437
log::FilterLog,
@@ -59,15 +62,15 @@ const MAX_RETRIES: u32 = 9;
5962
pub enum RpcClientError {
6063
/// The message could not be sent to the remote node
6164
#[error(transparent)]
62-
FailedToSend(reqwest_middleware::Error),
65+
FailedToSend(MiddlewareError),
6366

6467
/// The remote node failed to reply with the body of the response
6568
#[error("The response text was corrupted: {0}.")]
66-
CorruptedResponse(reqwest::Error),
69+
CorruptedResponse(ReqwestError),
6770

6871
/// The server returned an error code.
6972
#[error("The Http server returned error status code: {0}")]
70-
HttpStatus(reqwest::Error),
73+
HttpStatus(ReqwestError),
7174

7275
/// The request cannot be serialized as JSON.
7376
#[error(transparent)]
@@ -484,12 +487,12 @@ impl RpcClient {
484487
.body(request_body.to_json_string())
485488
.send()
486489
.await
487-
.map_err(RpcClientError::FailedToSend)?
490+
.map_err(|err| RpcClientError::FailedToSend(err.into()))?
488491
.error_for_status()
489-
.map_err(RpcClientError::HttpStatus)?
492+
.map_err(|err| RpcClientError::HttpStatus(err.into()))?
490493
.text()
491494
.await
492-
.map_err(RpcClientError::CorruptedResponse)
495+
.map_err(|err| RpcClientError::CorruptedResponse(err.into()))
493496
}
494497

495498
#[cfg_attr(feature = "tracing", tracing::instrument(level = "trace", skip_all))]
@@ -1203,7 +1206,7 @@ mod tests {
12031206

12041207
if let RpcClientError::HttpStatus(error) = error {
12051208
assert_eq!(
1206-
error.status(),
1209+
reqwest::Error::from(error).status(),
12071210
Some(StatusCode::from_u16(STATUS_CODE).unwrap())
12081211
);
12091212
} else {
@@ -1245,20 +1248,26 @@ mod tests {
12451248

12461249
#[tokio::test]
12471250
async fn call_bad_api_key() {
1248-
let alchemy_url = "https://eth-mainnet.g.alchemy.com/v2/abcdefg";
1251+
let api_key = "invalid-api-key";
1252+
let alchemy_url = format!("https://eth-mainnet.g.alchemy.com/v2/{api_key}");
12491253

12501254
let hash = B256::from_str(
12511255
"0xc008e9f9bb92057dd0035496fbf4fb54f66b4b18b370928e46d6603933022222",
12521256
)
12531257
.expect("failed to parse hash from string");
12541258

1255-
let error = TestRpcClient::new(alchemy_url)
1259+
let error = TestRpcClient::new(&alchemy_url)
12561260
.call::<Option<eth::Transaction>>(RequestMethod::GetTransactionByHash(hash))
12571261
.await
12581262
.expect_err("should have failed to interpret response as a Transaction");
12591263

1264+
assert!(!error.to_string().contains(api_key));
1265+
12601266
if let RpcClientError::HttpStatus(error) = error {
1261-
assert_eq!(error.status(), Some(StatusCode::from_u16(401).unwrap()));
1267+
assert_eq!(
1268+
reqwest::Error::from(error).status(),
1269+
Some(StatusCode::from_u16(401).unwrap())
1270+
);
12621271
} else {
12631272
unreachable!("Invalid error: {error}");
12641273
}
@@ -1279,7 +1288,7 @@ mod tests {
12791288
.expect_err("should have failed to connect due to a garbage domain name");
12801289

12811290
if let RpcClientError::FailedToSend(error) = error {
1282-
assert!(error.to_string().contains(&format!("error sending request for url ({alchemy_url}): error trying to connect: dns error: ")));
1291+
assert!(error.to_string().contains("dns error"));
12831292
} else {
12841293
unreachable!("Invalid error: {error}");
12851294
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
use std::error::Error;
2+
3+
use lazy_static::lazy_static;
4+
use regex::{Captures, Regex, Replacer};
5+
use url::Url;
6+
7+
/// Custom wrapper for `reqwest::Error` to avoid displaying the url in error
8+
/// message that could contain sensitive information such as API keys.
9+
#[derive(thiserror::Error, Debug)]
10+
pub struct ReqwestError(#[from] reqwest::Error);
11+
12+
impl From<ReqwestError> for reqwest::Error {
13+
fn from(value: ReqwestError) -> Self {
14+
value.0
15+
}
16+
}
17+
18+
// Matches the `Display` implementation for `reqwest::Error` except where noted
19+
impl std::fmt::Display for ReqwestError {
20+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
21+
if self.0.is_builder() {
22+
f.write_str("builder self")?;
23+
} else if self.0.is_request() {
24+
f.write_str("self sending request")?;
25+
} else if self.0.is_body() {
26+
f.write_str("request or response body self")?;
27+
} else if self.0.is_decode() {
28+
f.write_str("self decoding response body")?;
29+
} else if self.0.is_redirect() {
30+
f.write_str("self following redirect")?;
31+
} else if self.0.is_status() {
32+
let code = self.0.status().expect("Error is status");
33+
let prefix = if code.is_client_error() {
34+
"HTTP status client self"
35+
} else {
36+
debug_assert!(code.is_server_error());
37+
"HTTP status server self"
38+
};
39+
write!(f, "{prefix} ({code})")?;
40+
} else {
41+
// It might be an upgrade, but `reqwest` doesn't expose checking that on the
42+
// self type.
43+
f.write_str("unknown self")?;
44+
}
45+
46+
// This is changed from the original code
47+
if let Some(host) = self.0.url().and_then(|url| url.host_str()) {
48+
write!(f, " for host ({host})")?;
49+
}
50+
51+
if let Some(e) = self.0.source() {
52+
write!(f, ": {e}")?;
53+
}
54+
55+
Ok(())
56+
}
57+
}
58+
59+
/// Custom wrapper for `reqwest_middleware::Error` to avoid displaying the url
60+
/// in error message that could contain sensitive information such as API keys.
61+
#[derive(thiserror::Error, Debug)]
62+
pub enum MiddlewareError {
63+
/// There was an error running some middleware
64+
Middleware(#[from] anyhow::Error),
65+
/// Error from the underlying reqwest client
66+
Reqwest(#[from] ReqwestError),
67+
}
68+
69+
impl From<reqwest_middleware::Error> for MiddlewareError {
70+
fn from(value: reqwest_middleware::Error) -> Self {
71+
match value {
72+
reqwest_middleware::Error::Middleware(middleware) => {
73+
MiddlewareError::Middleware(middleware)
74+
}
75+
reqwest_middleware::Error::Reqwest(reqwest) => MiddlewareError::Reqwest(reqwest.into()),
76+
}
77+
}
78+
}
79+
80+
impl std::fmt::Display for MiddlewareError {
81+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
82+
match self {
83+
MiddlewareError::Middleware(e) => {
84+
let s = e.to_string();
85+
let replaced = URL_REGEX.replace_all(&s, UrlReplacer);
86+
f.write_str(&replaced)
87+
}
88+
MiddlewareError::Reqwest(e) => e.fmt(f),
89+
}
90+
}
91+
}
92+
93+
lazy_static! {
94+
static ref URL_REGEX: Regex = Regex::new(r"(http(s)?:\/\/.)?(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)").expect("Test checks panic");
95+
}
96+
97+
/// Replaces urls in strings with the host part only.
98+
struct UrlReplacer;
99+
100+
impl Replacer for UrlReplacer {
101+
fn replace_append(&mut self, caps: &Captures<'_>, dst: &mut String) {
102+
if let Some(host) = caps.get(0).and_then(|url| {
103+
Url::parse(url.as_str())
104+
.ok()
105+
.and_then(|url| url.host_str().map(ToString::to_string))
106+
}) {
107+
dst.push_str(&host);
108+
} else {
109+
dst.push_str("<unknown host>");
110+
}
111+
}
112+
}
113+
114+
#[cfg(test)]
115+
mod tests {
116+
use super::*;
117+
118+
#[test]
119+
fn test_display_middleware_error() -> anyhow::Result<()> {
120+
let error = MiddlewareError::Middleware(anyhow::anyhow!(
121+
"Some middleware error occurred for url: http://subdomain.example.com:1234/secret something else"
122+
));
123+
assert_eq!(
124+
error.to_string(),
125+
"Some middleware error occurred for url: subdomain.example.com something else"
126+
);
127+
128+
let error = MiddlewareError::Middleware(anyhow::anyhow!(
129+
"Some middleware error occurred for url: https://subdomain.example.com/path?query=secret something else"
130+
));
131+
assert_eq!(
132+
error.to_string(),
133+
"Some middleware error occurred for url: subdomain.example.com something else"
134+
);
135+
136+
Ok(())
137+
}
138+
}

0 commit comments

Comments
 (0)