From ed6fecfc5ab6a9ab4956e37a397c15cf11d7cbbe Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 13:30:45 -0400 Subject: [PATCH] fix(auth): harden loopback checks and align CSP tests Tighten CLI loopback target classification to use literal host checks, update explicit local TCP auth coverage to match the remote-target contract, and align server CSP assertions with the current external-script SPA bundle. Also enable reqwest cookies in fabro-http so package-scoped server tests compile without relying on workspace feature unification. --- lib/crates/fabro-cli/src/loopback_target.rs | 73 +++++++++++++++---- lib/crates/fabro-cli/tests/it/cmd/ps.rs | 63 +++++++++++++++- lib/crates/fabro-http/Cargo.toml | 2 +- lib/crates/fabro-server/src/csp.rs | 15 ++-- .../fabro-server/tests/it/api/routing.rs | 7 +- 5 files changed, 132 insertions(+), 28 deletions(-) diff --git a/lib/crates/fabro-cli/src/loopback_target.rs b/lib/crates/fabro-cli/src/loopback_target.rs index e9f115072..c41086322 100644 --- a/lib/crates/fabro-cli/src/loopback_target.rs +++ b/lib/crates/fabro-cli/src/loopback_target.rs @@ -32,29 +32,34 @@ pub(crate) fn is_loopback_or_unix_socket( } fn classify_http_target(api_url: &str) -> Result { - let url = fabro_http::Url::parse(user_config::normalized_http_base_url(api_url)).map_err( - |source| TargetSchemeError::InvalidUrl { + let normalized = user_config::normalized_http_base_url(api_url); + let url = + fabro_http::Url::parse(normalized).map_err(|source| TargetSchemeError::InvalidUrl { value: api_url.to_string(), reason: source.to_string(), - }, - )?; + })?; match url.scheme() { "https" => Ok(LoopbackClassification::Https), "http" => { - let Some(host) = url.host_str() else { + if url.host_str().is_none() { + return Err(TargetSchemeError::MissingHost { + value: api_url.to_string(), + }); + } + if !url.username().is_empty() || url.password().is_some() { + return Ok(LoopbackClassification::Rejected); + } + let Some(authority) = raw_authority(normalized) else { return Err(TargetSchemeError::MissingHost { value: api_url.to_string(), }); }; - let classification = host - .parse::() - .ok() - .filter(ip_is_loopback) - .map_or(LoopbackClassification::Rejected, |_| { - LoopbackClassification::LoopbackHttp - }); - Ok(classification) + Ok(if raw_host_is_loopback_literal(authority) { + LoopbackClassification::LoopbackHttp + } else { + LoopbackClassification::Rejected + }) } scheme => Err(TargetSchemeError::UnsupportedScheme { scheme: scheme.to_string(), @@ -62,6 +67,47 @@ fn classify_http_target(api_url: &str) -> Result Option<&str> { + let (_, remainder) = url.split_once("://")?; + let end = remainder + .find(|ch| ['/', '?', '#'].contains(&ch)) + .unwrap_or(remainder.len()); + Some(&remainder[..end]) +} + +fn raw_host_is_loopback_literal(authority: &str) -> bool { + if authority.contains('@') { + return false; + } + let Some(host) = raw_host(authority) else { + return false; + }; + match host.parse::().ok() { + Some(IpAddr::V4(ipv4)) => host.contains('.') && ipv4.is_loopback(), + Some(ip @ IpAddr::V6(_)) => ip_is_loopback(&ip), + None => false, + } +} + +fn raw_host(authority: &str) -> Option<&str> { + if authority.is_empty() { + return None; + } + if authority.starts_with('[') { + let end = authority.find(']')?; + let remainder = &authority[end + 1..]; + if !remainder.is_empty() && !remainder.starts_with(':') { + return None; + } + return Some(&authority[1..end]); + } + + let host = authority + .split_once(':') + .map_or(authority, |(host, _)| host); + if host.is_empty() { None } else { Some(host) } +} + fn ip_is_loopback(ip: &IpAddr) -> bool { match ip { IpAddr::V4(ipv4) => ipv4.is_loopback(), @@ -125,6 +171,7 @@ mod tests { let cases = [ "http://fabro.example.com", "http://127.0.0.1.evil.com", + "http://127.0.0.1:1@attacker.com", "http://localhost", "http://localhost.evil.com", "http://2130706433", diff --git a/lib/crates/fabro-cli/tests/it/cmd/ps.rs b/lib/crates/fabro-cli/tests/it/cmd/ps.rs index 3395082bd..8904431aa 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/ps.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/ps.rs @@ -2,7 +2,7 @@ use fabro_test::{fabro_snapshot, test_context}; use httpmock::MockServer; use serde_json::Value; -use super::support::{setup_completed_fast_dry_run, setup_created_fast_dry_run}; +use super::support::{local_dev_token, setup_completed_fast_dry_run, setup_created_fast_dry_run}; use crate::support::unique_run_id; #[test] @@ -36,7 +36,7 @@ fn help() { } #[test] -fn ps_accepts_local_tcp_server_target() { +fn ps_explicit_local_tcp_server_target_requires_explicit_auth() { let context = test_context!(); let storage_root = tempfile::tempdir_in("/tmp").unwrap(); let storage_dir = storage_root.path().join("storage"); @@ -77,9 +77,66 @@ fn ps_accepts_local_tcp_server_target() { .assert() .success(); + assert!( + !output.status.success(), + "ps against an explicit local TCP target should require explicit auth:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Authentication required."), + "explicit local TCP target should fail with an auth error:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn ps_explicit_local_tcp_server_target_accepts_explicit_dev_token() { + let context = test_context!(); + let storage_root = tempfile::tempdir_in("/tmp").unwrap(); + let storage_dir = storage_root.path().join("storage"); + std::fs::create_dir_all(&storage_dir).unwrap(); + + context + .command() + .env("FABRO_STORAGE_DIR", &storage_dir) + .args(["server", "start", "--bind", "127.0.0.1"]) + .assert() + .success(); + + let status_output = context + .command() + .env("FABRO_STORAGE_DIR", &storage_dir) + .args(["server", "status", "--json"]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let status_json: Value = serde_json::from_slice(&status_output).unwrap(); + let bind = status_json["bind"] + .as_str() + .expect("bind should be present"); + let token = local_dev_token(&storage_dir).expect("local dev token should exist"); + + let output = context + .command() + .env("FABRO_DEV_TOKEN", &token) + .args(["ps", "-a", "--json", "--server", &format!("http://{bind}")]) + .output() + .expect("ps should run"); + + context + .command() + .env("FABRO_STORAGE_DIR", &storage_dir) + .args(["server", "stop"]) + .assert() + .success(); + assert!( output.status.success(), - "ps against local TCP target failed:\nstdout:\n{}\nstderr:\n{}", + "ps against local TCP target with explicit auth failed:\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); diff --git a/lib/crates/fabro-http/Cargo.toml b/lib/crates/fabro-http/Cargo.toml index ed79e54a4..6629d9879 100644 --- a/lib/crates/fabro-http/Cargo.toml +++ b/lib/crates/fabro-http/Cargo.toml @@ -13,7 +13,7 @@ doctest = false workspace = true [dependencies] -reqwest = { workspace = true, features = ["blocking"] } +reqwest = { workspace = true, features = ["blocking", "cookies"] } thiserror.workspace = true [dev-dependencies] diff --git a/lib/crates/fabro-server/src/csp.rs b/lib/crates/fabro-server/src/csp.rs index eabd536bc..4b4bd3f1d 100644 --- a/lib/crates/fabro-server/src/csp.rs +++ b/lib/crates/fabro-server/src/csp.rs @@ -1,7 +1,7 @@ //! Content Security Policy generation. //! //! The policy is built once at server startup from the embedded SPA -//! `index.html` so inline `