From aa967a39428f7442750cd7bd3871bd6700600949 Mon Sep 17 00:00:00 2001 From: ch4r10t33r Date: Tue, 12 May 2026 10:43:21 +0100 Subject: [PATCH 1/2] fix(security): webhook SSRF guard and redact secrets in Config logs - Validate context.callbackUrl before persisting (https only, no URL credentials, block private/reserved IPs; DNS-resolve hosts and check all A/AAAA targets). Optional RELAYX_CALLBACK_ALLOW_LOOPBACK and RELAYX_CALLBACK_SKIP_SSRF_CHECKS for dev/test. - Webhook POST uses a shared reqwest client with redirects disabled and connect/send timeouts (issue #30). - Remove derived Debug from Config; implement fmt::Debug with redacted relayer_private_key and sentry_dsn; add log_summary_for_tracing for startup logging (issue #31). - Enable tokio net for lookup_host; document env vars in README. Fixes #30. Fixes #31. --- Cargo.toml | 2 +- README.md | 15 ++++ src/config.rs | 69 +++++++++++++- src/main.rs | 2 +- src/methods/send_tx/shared.rs | 7 ++ src/utils/callback.rs | 31 +++++-- src/utils/callback_security.rs | 158 +++++++++++++++++++++++++++++++++ src/utils/mod.rs | 1 + 8 files changed, 272 insertions(+), 13 deletions(-) create mode 100644 src/utils/callback_security.rs diff --git a/Cargo.toml b/Cargo.toml index a6a525b..1775460 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,7 +18,7 @@ rocksdb = "0.21" sentry = { version = "0.32", features = ["panic", "log"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -tokio = { version = "1.0", features = ["macros", "rt-multi-thread", "time"] } +tokio = { version = "1.0", features = ["macros", "rt-multi-thread", "net", "time"] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } url = "2.5" diff --git a/README.md b/README.md index ff53bcc..130f455 100644 --- a/README.md +++ b/README.md @@ -484,6 +484,21 @@ Callbacks fire on all terminal states: Callback failures are logged and silently dropped; they never affect the relay flow. +### Callback URL safety (SSRF) + +Before a job is stored, `callbackUrl` is validated (`src/utils/callback_security.rs`): + +- **HTTPS only** (no `http://` or exotic schemes). +- No **userinfo** (`https://user:pass@…` is rejected). +- Host must not be a **reserved / non-public** IP (private, loopback, link-local, documentation, unspecified, IPv6 ULA, etc.). Domain names are **DNS-resolved**; every resolved address must be allowed. + +Outbound webhook HTTP uses **no redirects** and bounded timeouts (`src/utils/callback.rs`). + +| Environment variable | Purpose | +|----------------------|---------| +| `RELAYX_CALLBACK_ALLOW_LOOPBACK=true` | Allow `127.0.0.1` / `::1` as callback targets (local development only). | +| `RELAYX_CALLBACK_SKIP_SSRF_CHECKS=true` | **Dangerous:** skip host/IP checks (parse-only). For isolated tests only. | + --- ## Error Codes diff --git a/src/config.rs b/src/config.rs index 46db844..0a17e32 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,11 +1,11 @@ -use std::{fs, path::PathBuf, sync::OnceLock}; +use std::{fmt, fs, path::PathBuf, sync::OnceLock}; use clap::Parser; use serde::{Deserialize, Serialize}; use crate::types::TokenDetails; -#[derive(Parser, Debug, Clone, Serialize, Deserialize)] +#[derive(Parser, Clone, Serialize, Deserialize)] #[command(name = "relayx")] #[command(about = "A modular relayer service with JSON-RPC endpoints")] pub struct Config { @@ -75,6 +75,42 @@ pub struct Config { pub disable_multichain: bool, } +impl fmt::Debug for Config { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Config") + .field("rpc_host", &self.rpc_host) + .field("rpc_port", &self.rpc_port) + .field("db_path", &self.db_path) + .field("relayers", &self.relayers) + .field("max_concurrent_requests", &self.max_concurrent_requests) + .field("request_timeout", &self.request_timeout) + .field("config_path", &self.config_path) + .field("http_address", &self.http_address) + .field("http_port", &self.http_port) + .field("http_cors", &self.http_cors) + .field("log_level", &self.log_level) + .field( + "relayer_private_key", + &self + .relayer_private_key + .as_ref() + .map(|_| "") + .unwrap_or(""), + ) + .field("disable_simulation", &self.disable_simulation) + .field( + "sentry_dsn", + &self + .sentry_dsn + .as_ref() + .map(|_| "") + .unwrap_or(""), + ) + .field("disable_multichain", &self.disable_multichain) + .finish() + } +} + impl Config { /// Cached parsed JSON config (loaded once globally) fn get_json_config(&self) -> Option<&'static serde_json::Value> { @@ -99,6 +135,35 @@ impl Config { Some(value) } } + + /// Short, non-secret configuration summary for tracing at startup. + pub fn log_summary_for_tracing(&self) -> String { + format!( + "http={}:{} db_path={:?} config_path={:?} disable_simulation={} disable_multichain={} relayer_private_key={} sentry_cli_dsn={} sentry_effective_dsn={}", + self.get_http_address(), + self.get_http_port(), + self.db_path, + self.config_path, + self.disable_simulation, + self.disable_multichain, + if self.relayer_private_key.as_ref().is_some_and(|s| !s.is_empty()) { + "" + } else { + "" + }, + if self.sentry_dsn.as_ref().is_some_and(|s| !s.is_empty()) { + "" + } else { + "" + }, + if self.get_sentry_dsn().is_some() { + "" + } else { + "" + }, + ) + } + /// Parse relayers string into a vector of addresses pub fn get_relayer_addresses(&self) -> Vec { if self.relayers.is_empty() { diff --git a/src/main.rs b/src/main.rs index 4405641..b1d38ad 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,7 +62,7 @@ async fn main() -> Result<()> { }; tracing::info!("Starting RelayX service"); - tracing::debug!("Configuration: {:?}", config); + tracing::debug!(summary = %config.log_summary_for_tracing(), "loaded configuration"); tracing::info!("Log level set to: {}", filter_str); // Initialize storage diff --git a/src/methods/send_tx/shared.rs b/src/methods/send_tx/shared.rs index 9dd9590..a1d6ec9 100644 --- a/src/methods/send_tx/shared.rs +++ b/src/methods/send_tx/shared.rs @@ -12,6 +12,7 @@ use crate::{ }, utils::{ callback::fire_callback, + callback_security, errors::rpc_errors::{ insufficient_balance_error, invalid_params_error, quote_expired_error, simulation_failed_error, transaction_too_large_error, unsupported_capability_error, @@ -175,6 +176,12 @@ pub async fn process_single_transaction( .and_then(|v| v.as_str()) .map(|s| s.to_string()); + if let Some(ref u) = callback_url { + callback_security::validate_outbound_webhook_url(u) + .await + .map_err(|_| invalid_params_error())?; + } + let internal_id = Uuid::new_v4(); let relayer_request = RelayerRequest { diff --git a/src/utils/callback.rs b/src/utils/callback.rs index 7056a26..3430a0f 100644 --- a/src/utils/callback.rs +++ b/src/utils/callback.rs @@ -1,16 +1,34 @@ +use std::sync::OnceLock; + +use serde::Serialize; + use crate::{RelayerRequest, SpecStatusResponse}; +fn webhook_http_client() -> &'static reqwest::Client { + static CLIENT: OnceLock = OnceLock::new(); + CLIENT.get_or_init(|| { + reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .timeout(std::time::Duration::from_secs(30)) + .connect_timeout(std::time::Duration::from_secs(10)) + .build() + .expect("reqwest webhook client builder") + }) +} + /// POST the final status payload to the callback URL registered for a request. /// /// The payload mirrors the `relayer_getStatus` response, with `taskId` added at the top level. +/// The HTTP client does not follow redirects (mitigates SSRF redirect chains; issue #30). +/// /// Failures are logged and silently swallowed — a failed callback never affects the relay flow. pub async fn fire_callback(req: &RelayerRequest, status: &SpecStatusResponse) { let url = match &req.callback_url { - Some(u) => u.clone(), - None => return, + Some(u) if !u.is_empty() => u.clone(), + _ => return, }; - #[derive(serde::Serialize)] + #[derive(Serialize)] struct CallbackPayload<'a> { #[serde(rename = "taskId")] task_id: &'a str, @@ -23,12 +41,7 @@ pub async fn fire_callback(req: &RelayerRequest, status: &SpecStatusResponse) { status, }; - match reqwest::Client::new() - .post(&url) - .json(&payload) - .send() - .await - { + match webhook_http_client().post(&url).json(&payload).send().await { Ok(resp) => { tracing::info!( "Callback delivered for task_id {} → {} (HTTP {})", diff --git a/src/utils/callback_security.rs b/src/utils/callback_security.rs new file mode 100644 index 0000000..610c934 --- /dev/null +++ b/src/utils/callback_security.rs @@ -0,0 +1,158 @@ +//! Guardrails for outbound status webhooks (`context.callbackUrl`). +//! +//! Mitigates SSRF (issue #30): restrict schemes, forbid URL credentials, block +//! non-public/reserved destinations by default, and resolve hostnames to ensure no +//! resolved address is disallowed. + +use std::net::IpAddr; + +use url::Url; + +fn ssrf_checks_disabled() -> bool { + matches!( + std::env::var("RELAYX_CALLBACK_SKIP_SSRF_CHECKS") + .map(|v| { matches!(v.to_ascii_lowercase().as_str(), "1" | "true" | "yes" | "on") }) + .unwrap_or(false), + true + ) +} + +fn allow_loopback_callback_targets() -> bool { + matches!( + std::env::var("RELAYX_CALLBACK_ALLOW_LOOPBACK") + .map(|v| { matches!(v.to_ascii_lowercase().as_str(), "1" | "true" | "yes" | "on") }) + .unwrap_or(false), + true + ) +} + +/// True when this IP must not be used as a webhook target (strict default). +fn is_blocked_ip(ip: IpAddr) -> bool { + if allow_loopback_callback_targets() && ip.is_loopback() { + return false; + } + match ip { + IpAddr::V4(v) => { + v.is_private() + || v.is_loopback() + || v.is_link_local() + || v.is_broadcast() + || v.is_documentation() + || v.is_unspecified() + } + IpAddr::V6(v) => { + v.is_loopback() + || v.is_unique_local() + || v.is_unicast_link_local() + || v.is_multicast() + || v.is_unspecified() + } + } +} + +/// Validate a client-supplied webhook URL before persisting the relay job. +/// +/// Policy (unless `RELAYX_CALLBACK_SKIP_SSRF_CHECKS` is set): +/// - Only `https` URLs (no `http`, `file`, `gopher`, etc.). +/// - No username/password embedded in the URL. +/// - Literal IP hosts must not be loopback, private, link-local, documentation, etc. +/// - Domain hosts are resolved with [`tokio::net::lookup_host`]; every resolved address +/// must pass the same IP rules. +/// +/// Set `RELAYX_CALLBACK_ALLOW_LOOPBACK=true` to permit loopback targets (local dev only). +pub async fn validate_outbound_webhook_url(raw: &str) -> Result<(), String> { + if raw.len() > 2048 { + return Err("callback URL exceeds maximum length".into()); + } + + if ssrf_checks_disabled() { + Url::parse(raw).map_err(|e| format!("invalid URL: {e}"))?; + return Ok(()); + } + + let url = Url::parse(raw).map_err(|e| format!("invalid URL: {e}"))?; + + if !url.username().is_empty() || url.password().is_some() { + return Err("callback URL must not contain credentials".into()); + } + + if url.scheme() != "https" { + return Err("only https callback URLs are allowed".into()); + } + + let host = url.host_str().ok_or("callback URL is missing a host")?; + let port = url.port_or_known_default().unwrap_or(443); + + match url.host() { + Some(url::Host::Ipv4(ip)) => { + if is_blocked_ip(IpAddr::V4(ip)) { + return Err("callback host IP is not an allowed public address".into()); + } + } + Some(url::Host::Ipv6(ip)) => { + if is_blocked_ip(IpAddr::V6(ip)) { + return Err("callback host IP is not an allowed public address".into()); + } + } + Some(url::Host::Domain(_)) => { + let mut found = false; + for sa in tokio::net::lookup_host((host, port)) + .await + .map_err(|e| format!("DNS lookup failed for callback host: {e}"))? + { + found = true; + if is_blocked_ip(sa.ip()) { + return Err(format!( + "callback host resolves to a disallowed address ({})", + sa.ip() + )); + } + } + if !found { + return Err("callback host resolved to no addresses".into()); + } + } + None => return Err("callback URL has an invalid host".into()), + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn blocked_ipv4_detection() { + assert!(is_blocked_ip(IpAddr::V4("127.0.0.1".parse().unwrap()))); + assert!(is_blocked_ip(IpAddr::V4("10.0.0.1".parse().unwrap()))); + assert!(is_blocked_ip(IpAddr::V4( + "169.254.169.254".parse().unwrap() + ))); + assert!(!is_blocked_ip(IpAddr::V4("8.8.8.8".parse().unwrap()))); + } + + #[tokio::test] + async fn rejects_https_with_literal_private_ip() { + let err = validate_outbound_webhook_url("https://10.0.0.1/webhook") + .await + .unwrap_err(); + assert!(err.contains("not an allowed public")); + } + + #[tokio::test] + async fn rejects_non_https_scheme() { + let err = validate_outbound_webhook_url("http://8.8.8.8/webhook") + .await + .unwrap_err(); + assert!(err.contains("only https")); + } + + #[tokio::test] + async fn rejects_credentials_in_userinfo() { + let err = validate_outbound_webhook_url("https://user:pass@8.8.8.8/hook") + .await + .unwrap_err(); + assert!(err.contains("credentials")); + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 36d8472..6d3c825 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,4 +1,5 @@ pub mod callback; +pub mod callback_security; pub mod errors; pub mod hex; pub mod misc; From e18d4e9aa504b78e64252d46bceaef7fc51dfc12 Mon Sep 17 00:00:00 2001 From: ch4r10t33r Date: Tue, 12 May 2026 10:59:05 +0100 Subject: [PATCH 2/2] fix(ci): satisfy clippy on callback env flags; add minimal CircleCI config - Replace redundant matches!(bool, true) with a small env_truthy helper (clippy::redundant_pattern_matching under -D warnings). - Add .circleci/config.yml noop job so the linked CircleCI project stops failing with "no configuration found" until the app is disabled. --- .circleci/config.yml | 20 ++++++++++++++++++++ src/utils/callback_security.rs | 20 ++++++++------------ 2 files changed, 28 insertions(+), 12 deletions(-) create mode 100644 .circleci/config.yml diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 0000000..e33286d --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,20 @@ +# Legacy CircleCI project hook: relayx CI runs on GitHub Actions (.github/workflows/). +# This minimal pipeline keeps the CircleCI app from failing with "no configuration found" +# until the project is disabled in the CircleCI UI. +version: 2.1 + +jobs: + noop: + docker: + - image: cimg/base:2024.02 + resource_class: small + steps: + - checkout + - run: + name: Skip (use GitHub Actions) + command: echo "Primary CI is GitHub Actions; see .github/workflows/" + +workflows: + default: + jobs: + - noop diff --git a/src/utils/callback_security.rs b/src/utils/callback_security.rs index 610c934..2537d6b 100644 --- a/src/utils/callback_security.rs +++ b/src/utils/callback_security.rs @@ -8,22 +8,18 @@ use std::net::IpAddr; use url::Url; +fn env_truthy(name: &str) -> bool { + std::env::var(name) + .map(|v| matches!(v.to_ascii_lowercase().as_str(), "1" | "true" | "yes" | "on")) + .unwrap_or(false) +} + fn ssrf_checks_disabled() -> bool { - matches!( - std::env::var("RELAYX_CALLBACK_SKIP_SSRF_CHECKS") - .map(|v| { matches!(v.to_ascii_lowercase().as_str(), "1" | "true" | "yes" | "on") }) - .unwrap_or(false), - true - ) + env_truthy("RELAYX_CALLBACK_SKIP_SSRF_CHECKS") } fn allow_loopback_callback_targets() -> bool { - matches!( - std::env::var("RELAYX_CALLBACK_ALLOW_LOOPBACK") - .map(|v| { matches!(v.to_ascii_lowercase().as_str(), "1" | "true" | "yes" | "on") }) - .unwrap_or(false), - true - ) + env_truthy("RELAYX_CALLBACK_ALLOW_LOOPBACK") } /// True when this IP must not be used as a webhook target (strict default).