From f5c0999fc06cb30692b222b3ef300f02f3cbdff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C2=96=C2=96=C2=96feyisaralawal?= <––––feyisaralawal01@gmail.com> Date: Sat, 26 Sep 2026 22:48:34 +0100 Subject: [PATCH] feat(api,webhooks,store,ci): implement multi-issue updates (#333, #332, #335, #331) Detailed explanation of changes across all four resolved issues: 1. Propagate per-request id through logging for cross-service traceability (Closes #333): - Audited request handling across crates/api and implemented `request_id_middleware` in crates/api/src/lib.rs. - For every incoming HTTP request, extracts the caller-supplied `X-Request-Id` header (if valid ASCII and non-empty) or generates a new UUIDv4. - Enters an instrumented tracing span `info_span!("request", request_id = %request_id)` wrapping downstream route handling, store calls, Horizon requests, and webhook dispatch so all log lines carry the correlation id automatically. - Attaches `x-request-id` to response headers so clients can reference request IDs when reporting issues. - Added `tracing-subscriber` dev-dependency and comprehensive tests in `crates/api/tests/request_id_tests.rs`: * `every_response_carries_an_x_request_id_header` * `a_caller_supplied_x_request_id_is_echoed_back_unchanged` * `log_output_for_a_request_consistently_carries_the_same_request_id_across_nested_spans` 2. Consolidate is_safe_url into comprehensive edge-case test suite (Closes #332): - Hardened `is_safe_url` in crates/webhooks/src/lib.rs against SSRF vectors across all IP encoding classes: * Dotted-decimal IPv4, loopback range (127.0.0.0/8), private (RFC 1918), carrier-grade NAT (100.64.0.0/10), link-local (169.254.0.0/16), broadcast (255.255.255.255), and unspecified (0.0.0.0/8). * Alternative representations including raw decimal integer (`2130706433`), hex integer (`0x7f000001`), hex-dotted (`0x7f.0.0.1`), and octal-dotted (`0177.0.0.1`). * IPv6 loopback (`::1`), unspecified (`::`), link-local (`fe80::/10`), unique-local (`fc00::/7`), IPv4-mapped IPv6 (`::ffff:x`), and IPv4-compatible IPv6 (`::x`). - Documented explicit DNS scope boundary: `is_safe_url` handles syntactic validation and IP literal filtering, while DNS resolution and DNS rebind defense are delegated to the HTTP client and egress network policies. - Organized tests into structured test modules by encoding class: * `test_standard_public_urls` * `test_ipv4_literal_forms` * `test_ipv6_forms` * `test_ipv4_mapped_and_compatible_ipv6_forms` * `test_link_local_addresses` * `test_unspecified_addresses` * `test_hostnames_and_dns_scope_boundary` * `test_invalid_and_malformed_urls` 3. Add migration-order regression test on fresh database (Closes #335): - Added `migrate_applies_cleanly_from_a_genuinely_empty_database` in crates/store/tests/store_tests.rs. - Dynamically provisions a fresh, isolated PostgreSQL database from the base instance rather than reusing an existing or pre-migrated schema. - Runs `Store::connect` and `Store::migrate` (`MIGRATOR.run`) to verify all 20 sequential migrations apply cleanly in order from scratch. - Asserts key database tables exist (`wallets`, `addresses`, `transactions`, `withdrawals`, `webhook_endpoints`, `webhook_deliveries`, `_sqlx_migrations`) and drops the temporary test database upon completion. 4. Add cargo-audit and cargo-deny result caching to speed up CI (Closes #331): - Updated `.github/workflows/ci.yml` for both `audit` and `deny` jobs. - Added `actions/cache@v4` steps caching tool binaries (`~/.cargo/bin/cargo-audit`) and advisory databases (`~/.cargo/advisory-db` for cargo-audit, `~/.cargo/advisory-dbs` for cargo-deny). - Configured daily rotating cache keys (`${{ runner.os }}-cargo-audit-${{ steps.cache-date.outputs.date }}` and `${{ runner.os }}-cargo-deny-${{ steps.cache-date.outputs.date }}`) with prefix restore keys to prevent cache drift and ensure advisory freshness. - Preserved active advisory fetching so incremental fetches occur fast against warm caches rather than downloading full databases from scratch on every CI run. --- .github/workflows/ci.yml | 23 +++ crates/api/Cargo.toml | 1 + crates/api/src/lib.rs | 44 +++- crates/api/tests/request_id_tests.rs | 166 +++++++++++++++ crates/store/tests/store_tests.rs | 69 +++++++ crates/webhooks/src/lib.rs | 289 +++++++++++++++++++-------- 6 files changed, 506 insertions(+), 86 deletions(-) create mode 100644 crates/api/tests/request_id_tests.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e3d285e..31dc1ab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -81,6 +81,18 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + - name: Set cache key date + id: cache-date + run: echo "date=$(date -u +'%Y-%m-%d')" >> $GITHUB_OUTPUT + - name: Cache cargo-audit binary and advisory DB + uses: actions/cache@v4 + with: + path: | + ~/.cargo/bin/cargo-audit + ~/.cargo/advisory-db + key: ${{ runner.os }}-cargo-audit-${{ steps.cache-date.outputs.date }} + restore-keys: | + ${{ runner.os }}-cargo-audit- # A prebuilt binary, deliberately not rustsec/audit-check: that action shells out to # `cargo install cargo-audit`, which picks up the 1.84.1 pin in rust-toolchain.toml and # fails, because a current cargo-audit needs rustc 1.88+. Installing a prebuilt binary @@ -98,6 +110,17 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + - name: Set cache key date + id: cache-date + run: echo "date=$(date -u +'%Y-%m-%d')" >> $GITHUB_OUTPUT + - name: Cache cargo-deny advisory DB + uses: actions/cache@v4 + with: + path: | + ~/.cargo/advisory-dbs + key: ${{ runner.os }}-cargo-deny-${{ steps.cache-date.outputs.date }} + restore-keys: | + ${{ runner.os }}-cargo-deny- - uses: EmbarkStudios/cargo-deny-action@v2 secret-scan: diff --git a/crates/api/Cargo.toml b/crates/api/Cargo.toml index 2f37fd0..0485e68 100644 --- a/crates/api/Cargo.toml +++ b/crates/api/Cargo.toml @@ -41,6 +41,7 @@ hex.workspace = true [dev-dependencies] tokio.workspace = true tower.workspace = true +tracing-subscriber.workspace = true sqlx.workspace = true stellar-base.workspace = true dotenvy = "0.15" diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index a41dbf8..d1858ae 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -18,10 +18,51 @@ pub mod submit_validation; pub use error::{ApiError, ApiResult, Envelope}; pub use state::AppState; -use axum::extract::DefaultBodyLimit; +use axum::extract::{DefaultBodyLimit, Request}; +use axum::http::header::HeaderName; +use axum::http::HeaderValue; +use axum::middleware::{self, Next}; +use axum::response::Response; use axum::routing::{delete, get, post}; use axum::Router; use tower_http::cors::{Any, CorsLayer}; +use tracing::Instrument; + +/// Canonical header name for request correlation IDs. +pub static REQUEST_ID_HEADER: HeaderName = HeaderName::from_static("x-request-id"); + +/// Extracted or generated request ID stored in request extensions. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct RequestId(pub String); + +/// Middleware that extracts or generates a request ID, enters an info span, and echoes it in responses. +pub async fn request_id_middleware(mut req: Request, next: Next) -> Response { + // Reuse caller-supplied X-Request-Id if non-empty and valid ASCII, else generate UUIDv4. + let request_id = req + .headers() + .get(&REQUEST_ID_HEADER) + .and_then(|v| v.to_str().ok()) + .map(str::trim) + .filter(|s| !s.is_empty() && HeaderValue::from_str(s).is_ok()) + .map(|s| s.to_string()) + .unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); + + // Record request ID in request extensions for handlers. + req.extensions_mut().insert(RequestId(request_id.clone())); + + // Open tracing span carrying the request ID for cross-service log correlation. + let span = tracing::info_span!("request", request_id = %request_id); + + // Run downstream middleware and routes within the span context. + let mut response = next.run(req).instrument(span).await; + + // Attach request ID header to response. + if let Ok(val) = HeaderValue::from_str(&request_id) { + response.headers_mut().insert(REQUEST_ID_HEADER.clone(), val); + } + + response +} /// Keep API request payloads bounded to a deliberate, documented ceiling. /// @@ -189,6 +230,7 @@ pub fn build_router(state: AppState) -> Router { // cleanly with `Router::layer` here. .layer(DefaultBodyLimit::max(REQUEST_BODY_LIMIT)) .layer(cors) + .layer(middleware::from_fn(request_id_middleware)) .with_state(state) } diff --git a/crates/api/tests/request_id_tests.rs b/crates/api/tests/request_id_tests.rs new file mode 100644 index 0000000..8eb04c2 --- /dev/null +++ b/crates/api/tests/request_id_tests.rs @@ -0,0 +1,166 @@ +//! Tests for per-request id propagation through middleware, response headers, and tracing spans. + +use axum::body::Body; +use axum::http::header::HeaderName; +use axum::http::{Request, StatusCode}; +use axum::routing::get; +use axum::Router; +use octo_api::{build_router, request_id_middleware, AppState, REQUEST_ID_HEADER}; +use octo_store::Store; +use octo_wallet_core::StellarNetwork; +use std::sync::{Arc, Mutex, Once}; +use tower::ServiceExt; +use tracing_subscriber::fmt::MakeWriter; +use uuid::Uuid; + +static LOAD_ENV: Once = Once::new(); + +fn database_url() -> Option { + LOAD_ENV.call_once(|| { + let _ = dotenvy::dotenv(); + }); + std::env::var("DATABASE_URL").ok() +} + +async fn test_state() -> Option { + let url = database_url()?; + let store = Store::connect(&url).await.ok()?; + let _ = store.migrate().await; + let master_key = [42u8; 32]; + Some(AppState::new( + store, + master_key, + StellarNetwork::Testnet, + "https://horizon-testnet.stellar.org".into(), + None, + octo_email::EmailSender::new_captured(), + )) +} + +async fn test_router() -> Router { + if let Some(state) = test_state().await { + build_router(state) + } else { + // Fallback router with identical middleware layer when database is unavailable. + Router::new() + .route("/health", get(|| async { "ok" })) + .layer(axum::middleware::from_fn(request_id_middleware)) + } +} + +#[derive(Clone)] +struct BufferWriter(Arc>>); + +impl std::io::Write for BufferWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +impl<'a> MakeWriter<'a> for BufferWriter { + type Writer = BufferWriter; + + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } +} + +#[tokio::test] +async fn every_response_carries_an_x_request_id_header() { + let app = test_router().await; + let req = Request::builder() + .uri("/health") + .body(Body::empty()) + .unwrap(); + + let resp = app.oneshot(req).await.expect("execute request"); + assert_eq!(resp.status(), StatusCode::OK); + + // Verify response carries X-Request-Id header. + let header_val = resp + .headers() + .get(&REQUEST_ID_HEADER) + .expect("X-Request-Id header present in response") + .to_str() + .expect("header is valid ASCII"); + + // Verify generated request id is a valid UUIDv4. + let parsed = Uuid::parse_str(header_val); + assert!(parsed.is_ok(), "generated request id must be a valid UUID"); + assert_eq!(parsed.unwrap().get_version_num(), 4); +} + +#[tokio::test] +async fn a_caller_supplied_x_request_id_is_echoed_back_unchanged() { + let app = test_router().await; + let custom_id = "client-trace-777-custom-id"; + let req = Request::builder() + .uri("/health") + .header(HeaderName::from_static("x-request-id"), custom_id) + .body(Body::empty()) + .unwrap(); + + let resp = app.oneshot(req).await.expect("execute request"); + assert_eq!(resp.status(), StatusCode::OK); + + // Verify caller-supplied request ID is preserved exactly. + let header_val = resp + .headers() + .get(&REQUEST_ID_HEADER) + .expect("X-Request-Id header present in response") + .to_str() + .expect("header is valid ASCII"); + assert_eq!(header_val, custom_id); +} + +#[tokio::test] +async fn log_output_for_a_request_consistently_carries_the_same_request_id_across_nested_spans() { + let log_buffer = Arc::new(Mutex::new(Vec::new())); + let subscriber = tracing_subscriber::fmt() + .with_writer(BufferWriter(log_buffer.clone())) + .with_ansi(false) + .finish(); + + // Register test subscriber for the current thread during test execution. + let _guard = tracing::subscriber::set_default(subscriber); + + // Handler with a nested child span to test span inheritance. + async fn nested_handler() -> &'static str { + let child_span = tracing::info_span!("horizon_dispatch", operation = "poll_status"); + let _enter = child_span.enter(); + tracing::info!("nested horizon call completed"); + "ok" + } + + let app = Router::new() + .route("/trace-test", get(nested_handler)) + .layer(axum::middleware::from_fn(request_id_middleware)); + + let custom_id = "trace-correlation-id-9988"; + let req = Request::builder() + .uri("/trace-test") + .header(HeaderName::from_static("x-request-id"), custom_id) + .body(Body::empty()) + .unwrap(); + + let resp = app.oneshot(req).await.expect("execute request"); + assert_eq!(resp.status(), StatusCode::OK); + + // Extract captured logs and assert request id is threaded through nested spans. + let logs = String::from_utf8(log_buffer.lock().unwrap().clone()).expect("valid utf8 logs"); + assert!( + logs.contains(custom_id), + "log output must contain the request id: {}", + logs + ); + assert!( + logs.contains("nested horizon call completed"), + "log output must contain nested span event: {}", + logs + ); +} diff --git a/crates/store/tests/store_tests.rs b/crates/store/tests/store_tests.rs index 9b047f6..461fcdb 100644 --- a/crates/store/tests/store_tests.rs +++ b/crates/store/tests/store_tests.rs @@ -1200,3 +1200,72 @@ async fn mark_polled_creates_and_updates_the_cursor_row() { "mark_polled must not fabricate a cursor position" ); } + +#[tokio::test] +async fn migrate_applies_cleanly_from_a_genuinely_empty_database() { + let Some(base_url) = database_url() else { + eprintln!("SKIPPED: DATABASE_URL is not set"); + return; + }; + + // Connect to base Postgres instance to provision an isolated empty database. + let base_pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect(&base_url) + .await + .expect("connect to base postgres"); + + // Create fresh empty database with random name. + let empty_db_name = format!("octo_empty_{}", Uuid::new_v4().simple()); + sqlx::query(&format!("CREATE DATABASE \"{empty_db_name}\"")) + .execute(&base_pool) + .await + .expect("create empty test database"); + + // Format connection URL targeting the newly created database. + let (prefix, query) = match base_url.rsplit_once('/') { + Some((p, rest)) => match rest.split_once('?') { + Some((_, q)) => (p, format!("?{}", q)), + None => (p, String::new()), + }, + None => panic!("invalid DATABASE_URL format"), + }; + let empty_db_url = format!("{prefix}/{empty_db_name}{query}"); + + // Connect Store handle and run all migrations from empty state. + let store = Store::connect(&empty_db_url) + .await + .expect("connect to empty database"); + store + .migrate() + .await + .expect("migrations must apply cleanly from empty database"); + + // Sanity check that core tables were created by the migrations. + let rows: Vec<(String,)> = sqlx::query_as( + "SELECT table_name FROM information_schema.tables WHERE table_schema = 'public'", + ) + .fetch_all(store.pool()) + .await + .expect("query tables"); + + let tables: std::collections::HashSet = rows.into_iter().map(|r| r.0).collect(); + assert!(tables.contains("wallets"), "wallets table must exist"); + assert!(tables.contains("addresses"), "addresses table must exist"); + assert!(tables.contains("transactions"), "transactions table must exist"); + assert!(tables.contains("withdrawals"), "withdrawals table must exist"); + assert!(tables.contains("webhook_endpoints"), "webhook_endpoints table must exist"); + assert!(tables.contains("webhook_deliveries"), "webhook_deliveries table must exist"); + assert!(tables.contains("_sqlx_migrations"), "_sqlx_migrations table must exist"); + + // Close connections to empty database. + drop(store); + + // Drop temporary test database to clean up resources. + let _ = sqlx::query(&format!( + "DROP DATABASE IF EXISTS \"{empty_db_name}\" WITH (FORCE)" + )) + .execute(&base_pool) + .await; +} + diff --git a/crates/webhooks/src/lib.rs b/crates/webhooks/src/lib.rs index 40fc3c2..f96376f 100644 --- a/crates/webhooks/src/lib.rs +++ b/crates/webhooks/src/lib.rs @@ -188,29 +188,31 @@ impl WebhookSender { /// Reject obviously-internal webhook targets (defense-in-depth against SSRF). Only `http`/`https` /// to non-loopback, non-private hosts are allowed. +/// +/// Scope boundary: `is_safe_url` inspects the URL syntactically and validates literal +/// IP addresses and local domain patterns. It deliberately does not perform asynchronous DNS +/// lookups to resolve hostnames to IP addresses; full DNS resolution and rebinding protections +/// are delegated to the HTTP client and egress network policies. pub fn is_safe_url(url: &str) -> bool { let lower = url.to_ascii_lowercase(); if !(lower.starts_with("http://") || lower.starts_with("https://")) { return false; } - // Dev/test escape hatch: allow loopback/private targets only when explicitly opted in. Never - // set this in production. + // Dev/test escape hatch: allow loopback/private targets only when explicitly opted in. let allow_local = std::env::var("OCTO_ALLOW_LOCAL_WEBHOOKS").as_deref() == Ok("1"); if allow_local { return true; } - // Extract host between scheme and the next '/' or ':'. + // Extract host between scheme and the next delimiter. let after_scheme = match lower.split_once("://") { Some((_, rest)) => rest, None => return false, }; - // A bracketed IPv6 literal must be extracted before splitting on ':', otherwise - // "[fe80::1]/hook" is cut at the first colon and every check below sees "fe80" — matching - // nothing, so link-local IPv6 was silently allowed through. + // Extract bracketed IPv6 literal before splitting on port colons. let host: &str = if let Some(rest) = after_scheme.strip_prefix('[') { match rest.split_once(']') { Some((inside, _)) => inside, - None => return false, // malformed bracketed host + None => return false, } } else { after_scheme @@ -223,118 +225,235 @@ pub fn is_safe_url(url: &str) -> bool { return false; } - // IPv6 private / non-routable ranges. `host` here is already unbracketed and lowercase. - // - ::1 loopback - // - fe80::/10 link-local (fe80..febf) - // - fc00::/7 unique local (fc00..fdff) - // - ::ffff:x IPv4-mapped — defer to the IPv4 rules below by unwrapping it - if host.contains(':') { - if let Some(v4) = host.rsplit_once(':').map(|(_, tail)| tail) { - // IPv4-mapped form like ::ffff:127.0.0.1 — re-check the embedded IPv4 literal. - if v4.contains('.') { - return is_safe_url(&format!("http://{v4}")); - } - } - let first_group = host.split(':').next().unwrap_or(""); - let is_link_local = first_group.starts_with("fe8") - || first_group.starts_with("fe9") - || first_group.starts_with("fea") - || first_group.starts_with("feb"); - let is_unique_local = first_group.starts_with("fc") || first_group.starts_with("fd"); - if is_link_local || is_unique_local { - return false; - } - } - // Block loopback, link-local, metadata, and common private ranges. - let blocked_exact = [ - "localhost", - "127.0.0.1", - "0.0.0.0", - "::1", - "169.254.169.254", - ]; - if blocked_exact.contains(&host) { - return false; - } - if host.starts_with("10.") - || host.starts_with("192.168.") - || host.starts_with("169.254.") + // Block well-known local hostnames and mDNS domains. + if host == "localhost" + || host.ends_with(".localhost") + || host == "local" || host.ends_with(".local") { return false; } - // 172.16.0.0/12 - if let Some(rest) = host.strip_prefix("172.") { - if let Some(second) = rest.split('.').next() { - if let Ok(n) = second.parse::() { - if (16..=31).contains(&n) { - return false; - } - } + + // Check if host is a valid IPv6 literal. + if let Ok(v6) = host.parse::() { + return !is_unsafe_ipv6(v6); + } + + // Check if host matches lenient IPv4 representations. + if let Some(v4) = parse_ipv4_lenient(host) { + return !is_unsafe_ipv4(v4); + } + + true +} + +/// Helper to parse IPv4 addresses in dotted-decimal, octal, hex, or raw integer representations. +fn parse_ipv4_lenient(s: &str) -> Option { + // Raw integer IPv4 representation (e.g. 2130706433 or 0). + if let Ok(num) = s.parse::() { + return Some(std::net::Ipv4Addr::from(num)); + } + // Raw hex integer IPv4 representation (e.g. 0x7f000001). + if let Some(hex_str) = s.strip_prefix("0x").or_else(|| s.strip_prefix("0X")) { + if let Ok(num) = u32::from_str_radix(hex_str, 16) { + return Some(std::net::Ipv4Addr::from(num)); } } - // IPv4 100.64.0.0/10 (carrier-grade NAT) - if host.starts_with("100.") { - if let Some(rest) = host.strip_prefix("100.") { - if let Some(second) = rest.split('.').next() { - if let Ok(n) = second.parse::() { - if (64..=127).contains(&n) { - return false; - } - } + // Dotted 4-octet representation with potential decimal, octal, or hex segments. + let parts: Vec<&str> = s.split('.').collect(); + if parts.len() == 4 { + let mut octets = [0u8; 4]; + for (i, part) in parts.iter().enumerate() { + let val = if let Some(hex) = part.strip_prefix("0x").or_else(|| part.strip_prefix("0X")) { + u32::from_str_radix(hex, 16).ok()? + } else if part.len() > 1 && part.starts_with('0') { + u32::from_str_radix(part, 8).ok()? + } else { + part.parse::().ok()? + }; + if val > 255 { + return None; } + octets[i] = val as u8; } + return Some(std::net::Ipv4Addr::from(octets)); } - // IPv6 checks (loopback, link-local, unique-local) - if host.contains(":") { - if host == "::1" || host == "::" { - return false; - } - if host.starts_with("fe80:") { - return false; + None +} + +/// Returns true if an IPv4 address is in a private, loopback, link-local, unspecified, or broadcast range. +fn is_unsafe_ipv4(ip: std::net::Ipv4Addr) -> bool { + let octets = ip.octets(); + // 0.0.0.0/8 (unspecified / this network) + octets[0] == 0 + // 127.0.0.0/8 (loopback) + || octets[0] == 127 + // 10.0.0.0/8 (private) + || octets[0] == 10 + // 172.16.0.0/12 (private: 172.16.x.x - 172.31.x.x) + || (octets[0] == 172 && (16..=31).contains(&octets[1])) + // 192.168.0.0/16 (private) + || (octets[0] == 192 && octets[1] == 168) + // 169.254.0.0/16 (link-local, cloud metadata) + || (octets[0] == 169 && octets[1] == 254) + // 100.64.0.0/10 (carrier-grade NAT) + || (octets[0] == 100 && (64..=127).contains(&octets[1])) + // Broadcast 255.255.255.255 + || ip.is_broadcast() +} + +/// Returns true if an IPv6 address is in a private, loopback, link-local, unspecified, or mapped unsafe range. +fn is_unsafe_ipv6(ip: std::net::Ipv6Addr) -> bool { + // Loopback ::1 + if ip.is_loopback() { + return true; + } + // Unspecified :: + if ip.is_unspecified() { + return true; + } + // IPv4-mapped IPv6 address (e.g. ::ffff:127.0.0.1 or ::ffff:7f00:1) + if let Some(v4) = ip.to_ipv4_mapped() { + if is_unsafe_ipv4(v4) { + return true; } - if host.starts_with("fc") || host.starts_with("fd") { - return false; + } + // IPv4-compatible IPv6 address (deprecated, e.g. ::127.0.0.1) + if let Some(v4) = ip.to_ipv4() { + if is_unsafe_ipv4(v4) { + return true; } } - true + let segs = ip.segments(); + // Link-local: fe80::/10 (fe80..febf) + if (segs[0] & 0xffc0) == 0xfe80 { + return true; + } + // Unique-local: fc00::/7 (fc00..fdff) + if (segs[0] & 0xfe00) == 0xfc00 { + return true; + } + false } #[cfg(test)] mod tests { use super::is_safe_url; + // --- Standard Public URLs --- #[test] - fn allows_public_https() { + fn test_standard_public_urls() { assert!(is_safe_url("https://api.customer.com/webhooks")); assert!(is_safe_url("http://example.org:8080/hook")); + assert!(is_safe_url("http://172.15.0.1/x")); + assert!(is_safe_url("http://172.32.0.1/x")); + assert!(is_safe_url("https://93.184.216.34/webhook")); } + // --- IPv4 Literal Forms (Dotted, Decimal, Hex, Octal) --- #[test] - fn blocks_internal_targets() { - assert!(!is_safe_url("http://localhost/hook")); + fn test_ipv4_literal_forms() { + // Standard dotted decimal loopback assert!(!is_safe_url("http://127.0.0.1:9000")); - assert!(!is_safe_url("http://169.254.169.254/latest/meta-data")); + assert!(!is_safe_url("http://127.0.0.2/hook")); + assert!(!is_safe_url("http://127.1.2.3/hook")); + + // Private ranges (RFC 1918) assert!(!is_safe_url("http://10.0.0.5/x")); assert!(!is_safe_url("http://192.168.1.10/x")); assert!(!is_safe_url("http://172.16.5.5/x")); - assert!(!is_safe_url("http://db.internal.local/x")); - assert!(!is_safe_url("ftp://example.com")); - assert!(!is_safe_url("not-a-url")); - } + assert!(!is_safe_url("http://172.31.255.255/x")); - #[test] - fn allows_172_outside_private_block() { - assert!(is_safe_url("http://172.15.0.1/x")); - assert!(is_safe_url("http://172.32.0.1/x")); + // Carrier-grade NAT (100.64.0.0/10) + assert!(!is_safe_url("http://100.64.5.5/x")); + assert!(!is_safe_url("http://100.127.255.255/x")); + + // Alternative representations (decimal integer, hex, octal) + assert!(!is_safe_url("http://2130706433/hook")); + assert!(!is_safe_url("http://0x7f000001/hook")); + assert!(!is_safe_url("http://0x7f.0.0.1/hook")); + assert!(!is_safe_url("http://0177.0.0.1/hook")); } + // --- IPv6 Forms (Loopback, Unique Local) --- #[test] - fn blocks_ipv6_and_shared_address() { + fn test_ipv6_forms() { + // Loopback assert!(!is_safe_url("http://[::1]/hook")); - assert!(!is_safe_url("http://[fe80::1]/hook")); + assert!(!is_safe_url("http://[0:0:0:0:0:0:0:1]/hook")); + + // Unique local (fc00::/7) assert!(!is_safe_url("http://[fc00::1]/hook")); assert!(!is_safe_url("http://[fd00::1]/hook")); - assert!(!is_safe_url("http://100.64.5.5/x")); + assert!(!is_safe_url("http://[fd12:3456:789a::1]/hook")); + } + + // --- IPv4-Mapped and IPv4-Compatible IPv6 Forms --- + #[test] + fn test_ipv4_mapped_and_compatible_ipv6_forms() { + // IPv4-mapped with dotted decimal + assert!(!is_safe_url("http://[::ffff:127.0.0.1]/hook")); + assert!(!is_safe_url("http://[::ffff:10.0.0.1]/hook")); + assert!(!is_safe_url("http://[::ffff:192.168.1.1]/hook")); + assert!(!is_safe_url("http://[::ffff:169.254.169.254]/hook")); + + // IPv4-mapped with hex representation (7f00:1 == 127.0.0.1) + assert!(!is_safe_url("http://[::ffff:7f00:1]/hook")); + + // IPv4-compatible + assert!(!is_safe_url("http://[::127.0.0.1]/hook")); + assert!(!is_safe_url("http://[::10.0.0.1]/hook")); + } + + // --- Link-Local Addresses (IPv4 and IPv6) --- + #[test] + fn test_link_local_addresses() { + // IPv4 link-local (169.254.0.0/16 including AWS/cloud metadata) + assert!(!is_safe_url("http://169.254.169.254/latest/meta-data")); + assert!(!is_safe_url("http://169.254.1.1/x")); + + // IPv6 link-local (fe80::/10) + assert!(!is_safe_url("http://[fe80::1]/hook")); + assert!(!is_safe_url("http://[febf::ffff]/hook")); + } + + // --- Unspecified Addresses (0.0.0.0 and ::) --- + #[test] + fn test_unspecified_addresses() { + // IPv4 0.0.0.0 + assert!(!is_safe_url("http://0.0.0.0/hook")); + assert!(!is_safe_url("http://0.0.0.0:8080/hook")); + assert!(!is_safe_url("http://0/hook")); + + // IPv6 :: + assert!(!is_safe_url("http://[::]/hook")); + assert!(!is_safe_url("http://[0:0:0:0:0:0:0:0]/hook")); + } + + // --- Hostnames, Local Domains, and DNS Boundary Scope --- + #[test] + fn test_hostnames_and_dns_scope_boundary() { + // Obvious local hostnames and mDNS domains are blocked syntactically + assert!(!is_safe_url("http://localhost/hook")); + assert!(!is_safe_url("http://localhost:3000/hook")); + assert!(!is_safe_url("http://app.localhost/hook")); + assert!(!is_safe_url("http://db.internal.local/x")); + assert!(!is_safe_url("http://service.local/webhook")); + + // Scope boundary: arbitrary hostnames (e.g., custom domains that might resolve + // to private IPs via DNS) are permitted by syntactic validation; DNS resolution + // and rebind protection are explicitly the responsibility of the HTTP client. + assert!(is_safe_url("https://internal-service.example.com/webhook")); + assert!(is_safe_url("https://webhook.acme-corp.com/events")); + } + + // --- Invalid and Malformed URLs --- + #[test] + fn test_invalid_and_malformed_urls() { + assert!(!is_safe_url("ftp://example.com")); + assert!(!is_safe_url("javascript:alert(1)")); + assert!(!is_safe_url("not-a-url")); + assert!(!is_safe_url("http:///empty-host")); + assert!(!is_safe_url("http://[invalid-ipv6]/hook")); } }