From 3b2cffceafa07ecbe92695a8096ea0743eafd640 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 12 Apr 2026 11:48:54 -0400 Subject: [PATCH 1/3] refactor(http): centralize reqwest behind fabro-http Add the shared fabro-http transport crate and route hand-written HTTP client construction through it. Use FABRO_HTTP_PROXY_POLICY for test no-proxy defaults, remove direct reqwest deps from ordinary crates, and add clippy bans for raw reqwest entrypoints. --- Cargo.lock | 39 +- Cargo.toml | 2 + clippy.toml | 8 + lib/crates/fabro-agent/Cargo.toml | 2 +- lib/crates/fabro-agent/src/tools.rs | 8 +- lib/crates/fabro-api/src/lib.rs | 2 + lib/crates/fabro-cli/Cargo.toml | 2 +- lib/crates/fabro-cli/src/commands/exec.rs | 2 +- lib/crates/fabro-cli/src/commands/install.rs | 2 +- lib/crates/fabro-cli/src/commands/model.rs | 4 +- lib/crates/fabro-cli/src/commands/upgrade.rs | 6 +- lib/crates/fabro-cli/src/server_client.rs | 30 +- lib/crates/fabro-cli/src/user_config.rs | 10 +- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 18 +- lib/crates/fabro-cli/tests/it/cmd/doctor.rs | 3 +- lib/crates/fabro-cli/tests/it/cmd/exec.rs | 12 +- lib/crates/fabro-cli/tests/it/cmd/pr_view.rs | 7 +- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 13 +- .../fabro-cli/tests/it/cmd/server_start.rs | 2 + lib/crates/fabro-cli/tests/it/cmd/support.rs | 6 +- lib/crates/fabro-cli/tests/it/scenario/mod.rs | 6 +- lib/crates/fabro-cli/tests/it/support/mod.rs | 3 +- lib/crates/fabro-cli/tests/it/workflow/mod.rs | 6 +- lib/crates/fabro-devcontainer/Cargo.toml | 2 +- lib/crates/fabro-devcontainer/src/features.rs | 5 +- lib/crates/fabro-github/Cargo.toml | 2 +- lib/crates/fabro-github/src/lib.rs | 59 +-- lib/crates/fabro-hooks/Cargo.toml | 2 +- lib/crates/fabro-hooks/src/executor.rs | 18 +- lib/crates/fabro-http/Cargo.toml | 20 + lib/crates/fabro-http/src/lib.rs | 351 ++++++++++++++++++ lib/crates/fabro-llm/Cargo.toml | 2 +- .../fabro-llm/src/providers/anthropic.rs | 4 +- lib/crates/fabro-llm/src/providers/common.rs | 14 +- .../fabro-llm/src/providers/fabro_server.rs | 12 +- lib/crates/fabro-llm/src/providers/gemini.rs | 14 +- .../fabro-llm/src/providers/http_api.rs | 8 +- lib/crates/fabro-llm/src/providers/openai.rs | 6 +- .../src/providers/openai_compatible.rs | 16 +- lib/crates/fabro-mcp/src/client.rs | 2 + lib/crates/fabro-oauth/Cargo.toml | 2 +- lib/crates/fabro-oauth/src/lib.rs | 10 +- lib/crates/fabro-server/Cargo.toml | 2 +- lib/crates/fabro-server/src/diagnostics.rs | 46 ++- .../fabro-server/src/github_webhooks.rs | 6 +- lib/crates/fabro-server/src/run_manifest.rs | 2 +- lib/crates/fabro-server/src/server.rs | 32 +- lib/crates/fabro-server/src/web_auth.rs | 26 +- lib/crates/fabro-server/tests/it/api/mtls.rs | 8 +- lib/crates/fabro-slack/Cargo.toml | 2 +- lib/crates/fabro-slack/src/client.rs | 7 +- lib/crates/fabro-slack/src/connection.rs | 2 +- lib/crates/fabro-telemetry/Cargo.toml | 2 +- lib/crates/fabro-telemetry/src/sender.rs | 5 +- lib/crates/fabro-test/Cargo.toml | 2 +- lib/crates/fabro-test/src/lib.rs | 8 +- lib/crates/fabro-tracker/Cargo.toml | 4 +- lib/crates/fabro-tracker/src/github.rs | 12 +- lib/crates/fabro-tracker/src/lib.rs | 2 +- lib/crates/fabro-tracker/src/linear.rs | 14 +- lib/crates/fabro-workflow/Cargo.toml | 2 +- .../fabro-workflow/src/pipeline/initialize.rs | 2 +- .../tests/it/daytona_integration.rs | 2 +- test/twin/github/Cargo.toml | 2 +- test/twin/github/src/handlers/branches.rs | 2 +- test/twin/github/src/handlers/graphql.rs | 4 +- test/twin/github/src/handlers/pulls.rs | 4 +- test/twin/github/src/test_support.rs | 4 +- test/twin/openai/Cargo.toml | 2 +- test/twin/openai/tests/common/mod.rs | 43 ++- test/twin/openai/tests/failure_modes.rs | 4 +- .../twin/openai/tests/live_openai_contract.rs | 12 +- .../openai/tests/tool_and_schema_contract.rs | 2 +- 73 files changed, 740 insertions(+), 269 deletions(-) create mode 100644 lib/crates/fabro-http/Cargo.toml create mode 100644 lib/crates/fabro-http/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index c3f35a61c..53092e2b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1481,6 +1481,7 @@ dependencies = [ "clap", "dirs", "fabro-config", + "fabro-http", "fabro-llm", "fabro-macros", "fabro-mcp", @@ -1495,7 +1496,6 @@ dependencies = [ "jsonschema", "libc", "paste", - "reqwest 0.13.2", "serde", "serde_json", "shell-escape", @@ -1567,6 +1567,7 @@ dependencies = [ "fabro-github", "fabro-graphviz", "fabro-hooks", + "fabro-http", "fabro-interview", "fabro-llm", "fabro-macros", @@ -1597,7 +1598,6 @@ dependencies = [ "progenitor-client", "rand 0.8.5", "regex", - "reqwest 0.13.2", "rustls", "rustls-pemfile", "scopeguard", @@ -1657,9 +1657,9 @@ dependencies = [ name = "fabro-devcontainer" version = "0.176.2" dependencies = [ + "fabro-http", "fabro-util", "insta", - "reqwest 0.13.2", "serde", "serde_json", "serde_yaml", @@ -1675,10 +1675,10 @@ version = "0.176.2" dependencies = [ "base64", "chrono", + "fabro-http", "fabro-macros", "fabro-test", "jsonwebtoken", - "reqwest 0.13.2", "serde", "serde_json", "tokio", @@ -1704,6 +1704,7 @@ dependencies = [ "async-trait", "fabro-agent", "fabro-config", + "fabro-http", "fabro-llm", "fabro-model", "fabro-template", @@ -1711,7 +1712,6 @@ dependencies = [ "fabro-util", "httpmock", "regex", - "reqwest 0.13.2", "serde", "serde_json", "tokio", @@ -1720,6 +1720,15 @@ dependencies = [ "tracing", ] +[[package]] +name = "fabro-http" +version = "0.176.2" +dependencies = [ + "http", + "reqwest 0.13.2", + "thiserror 2.0.18", +] + [[package]] name = "fabro-interview" version = "0.176.2" @@ -1742,6 +1751,7 @@ dependencies = [ "async-trait", "base64", "bytes", + "fabro-http", "fabro-macros", "fabro-model", "fabro-test", @@ -1751,7 +1761,6 @@ dependencies = [ "httpmock", "insta", "rand 0.8.5", - "reqwest 0.13.2", "serde", "serde_json", "thiserror 2.0.18", @@ -1802,11 +1811,11 @@ version = "0.176.2" dependencies = [ "axum", "base64", + "fabro-http", "hex", "httpmock", "open", "rand 0.8.5", - "reqwest 0.13.2", "serde", "serde_json", "sha2", @@ -1891,6 +1900,7 @@ dependencies = [ "fabro-github", "fabro-graphviz", "fabro-hooks", + "fabro-http", "fabro-interview", "fabro-llm", "fabro-model", @@ -1917,7 +1927,6 @@ dependencies = [ "openapiv3", "rand 0.8.5", "regex", - "reqwest 0.13.2", "rustls", "rustls-pemfile", "rustls-pki-types", @@ -1947,10 +1956,10 @@ dependencies = [ name = "fabro-slack" version = "0.176.2" dependencies = [ + "fabro-http", "fabro-interview", "fabro-workflow", "futures-util", - "reqwest 0.13.2", "rustls", "serde", "serde_json", @@ -2000,6 +2009,7 @@ dependencies = [ "chrono", "dirs", "exec", + "fabro-http", "fabro-util", "fork", "git2", @@ -2007,7 +2017,6 @@ dependencies = [ "mac_address", "md5", "regex", - "reqwest 0.13.2", "sentry", "serde", "serde_json", @@ -2035,11 +2044,11 @@ dependencies = [ "assert_cmd", "axum", "fabro-config", + "fabro-http", "fabro-proc", "fabro-types", "insta", "regex", - "reqwest 0.13.2", "serde", "serde_json", "tempfile", @@ -2055,8 +2064,8 @@ version = "0.176.2" dependencies = [ "async-trait", "fabro-github", + "fabro-http", "httpmock", - "reqwest 0.13.2", "serde_json", "tokio", "tracing", @@ -2131,6 +2140,7 @@ dependencies = [ "fabro-github", "fabro-graphviz", "fabro-hooks", + "fabro-http", "fabro-interview", "fabro-llm", "fabro-macros", @@ -2153,7 +2163,6 @@ dependencies = [ "predicates", "rand 0.8.5", "regex", - "reqwest 0.13.2", "scopeguard", "serde", "serde_json", @@ -6685,8 +6694,8 @@ dependencies = [ "axum", "base64", "chrono", + "fabro-http", "jsonwebtoken", - "reqwest 0.13.2", "serde", "serde_json", "tempfile", @@ -6703,9 +6712,9 @@ dependencies = [ "anyhow", "async-stream", "axum", + "fabro-http", "futures-util", "http", - "reqwest 0.13.2", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index f39bac0ba..192b60d63 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,6 +74,7 @@ object_store = { version = "0.12.5", features = ["aws"] } rust-embed = "8" percent-encoding = "2" minijinja = "2" +fabro-http = { path = "lib/crates/fabro-http" } [workspace.lints.rust] unsafe_code = "deny" @@ -99,6 +100,7 @@ print_stderr = "warn" dbg_macro = "warn" empty_drop = "warn" empty_structs_with_brackets = "warn" +disallowed_methods = "deny" exit = "warn" get_unwrap = "warn" rc_buffer = "warn" diff --git a/clippy.toml b/clippy.toml index ecc515b4f..b5e5cb0b4 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,2 +1,10 @@ absolute-paths-max-segments = 2 absolute-paths-allowed-crates = ["std", "core", "alloc"] + +disallowed-methods = [ + { path = "reqwest::Client::new", reason = "Use fabro_http::http_client() or fabro_http::test_http_client()", allow-invalid = true }, + { path = "reqwest::Client::builder", reason = "Use fabro_http::HttpClientBuilder::new()", allow-invalid = true }, + { path = "reqwest::blocking::Client::new", reason = "Use fabro_http::blocking_http_client() or fabro_http::blocking_test_http_client()", allow-invalid = true }, + { path = "reqwest::blocking::Client::builder", reason = "Use fabro_http::BlockingHttpClientBuilder::new()", allow-invalid = true }, + { path = "reqwest::get", reason = "Build a fabro_http client and send the request explicitly", allow-invalid = true }, +] diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index 917cd93d8..4fbe1079c 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -31,6 +31,7 @@ fabro-model = { path = "../fabro-model" } fabro-mcp = { path = "../fabro-mcp" } fabro-sandbox = { path = "../fabro-sandbox" } fabro-util = { path = "../fabro-util" } +fabro-http.workspace = true thiserror.workspace = true serde.workspace = true serde_json.workspace = true @@ -40,7 +41,6 @@ futures.workspace = true async-trait.workspace = true jsonschema.workspace = true chrono.workspace = true -reqwest.workspace = true tokio-util.workspace = true tracing.workspace = true dirs = "6" diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index a620f8214..c3b1a8bc5 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -467,7 +467,7 @@ pub(crate) fn make_web_search_tool() -> RegisteredTool { fn make_web_search_tool_with_api_key(api_key: Option) -> RegisteredTool { use std::sync::OnceLock; - static CLIENT: OnceLock = OnceLock::new(); + static CLIENT: OnceLock = OnceLock::new(); RegisteredTool { definition: ToolDefinition { @@ -490,7 +490,11 @@ fn make_web_search_tool_with_api_key(api_key: Option) -> RegisteredTool })?; let query = required_str(&args, "query")?; - let client = CLIENT.get_or_init(reqwest::Client::new).clone(); + let client = CLIENT + .get_or_init(|| { + fabro_http::http_client().expect("Brave Search HTTP client should build") + }) + .clone(); let count = args .get("max_results") .and_then(serde_json::Value::as_u64) diff --git a/lib/crates/fabro-api/src/lib.rs b/lib/crates/fabro-api/src/lib.rs index aedc1e622..8c05995fd 100644 --- a/lib/crates/fabro-api/src/lib.rs +++ b/lib/crates/fabro-api/src/lib.rs @@ -2,6 +2,8 @@ clippy::absolute_paths, clippy::all, clippy::derivable_impls, + clippy::disallowed_methods, + clippy::disallowed_types, clippy::needless_lifetimes, unreachable_pub, unused_imports diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index e294575d7..e716ae3d2 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -41,6 +41,7 @@ fabro-telemetry = { path = "../fabro-telemetry" } fabro-store = { path = "../fabro-store" } fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } +fabro-http.workspace = true clap.workspace = true clap_complete.workspace = true cli-table.workspace = true @@ -60,7 +61,6 @@ toml.workspace = true futures.workspace = true regex.workspace = true semver.workspace = true -reqwest.workspace = true progenitor-client = "0.13" async-trait.workspace = true jsonwebtoken.workspace = true diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index ca9a9be67..cf87fedab 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -198,7 +198,7 @@ pub(crate) async fn execute( user_config::build_server_client(tls.as_ref())?, ), user_config::ServerTarget::UnixSocket(path) => { - let http_client = reqwest::ClientBuilder::new() + let http_client = fabro_http::HttpClientBuilder::new() .unix_socket(path.as_path()) .no_proxy() .build()?; diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 2ccbfcd78..e298a4449 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -596,7 +596,7 @@ async fn setup_github_app( " {}", s.dim.apply_to("Exchanging code with GitHub...") ); - let client = reqwest::Client::new(); + let client = fabro_http::http_client()?; let resp = client .post(format!( "https://api.github.com/app-manifests/{code}/conversions" diff --git a/lib/crates/fabro-cli/src/commands/model.rs b/lib/crates/fabro-cli/src/commands/model.rs index 2b35c67f9..7ba873053 100644 --- a/lib/crates/fabro-cli/src/commands/model.rs +++ b/lib/crates/fabro-cli/src/commands/model.rs @@ -424,8 +424,8 @@ mod tests { use super::*; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } fn test_api_client(api_url: &str) -> fabro_api::Client { diff --git a/lib/crates/fabro-cli/src/commands/upgrade.rs b/lib/crates/fabro-cli/src/commands/upgrade.rs index 451bc1fd0..140dea3e9 100644 --- a/lib/crates/fabro-cli/src/commands/upgrade.rs +++ b/lib/crates/fabro-cli/src/commands/upgrade.rs @@ -19,11 +19,11 @@ const GITHUB_REPO: &str = "fabro-sh/fabro"; enum Backend { Gh, - Http(reqwest::Client), + Http(fabro_http::HttpClient), } -fn http_client() -> Result { - reqwest::Client::builder() +fn http_client() -> Result { + fabro_http::HttpClientBuilder::new() .user_agent("fabro-cli") .build() .context("failed to build HTTP client") diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 9cc115c46..bd9594add 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -6,14 +6,14 @@ use std::time::Duration; use anyhow::{Context as _, Result, anyhow, bail}; use bytes::Bytes; use fabro_api::types; +use fabro_http::header::{CONTENT_LENGTH, CONTENT_TYPE}; +use fabro_http::multipart::{Form, Part}; use fabro_server::bind::Bind; use fabro_store::{EventEnvelope, RunSummary, StageId}; use fabro_types::settings::SettingsLayer; use fabro_types::{RunBlobId, RunEvent, RunId}; use fabro_workflow::artifact_snapshot::CapturedArtifactInfo; use futures::StreamExt; -use reqwest::header::{CONTENT_LENGTH, CONTENT_TYPE}; -use reqwest::multipart::{Form, Part}; use serde::Serialize; use serde::de::DeserializeOwned; use tokio::fs::File; @@ -28,7 +28,7 @@ use crate::{sse, user_config}; #[derive(Clone)] pub(crate) struct ServerStoreClient { client: fabro_api::Client, - http_client: reqwest::Client, + http_client: fabro_http::HttpClient, base_url: String, } @@ -174,7 +174,7 @@ fn normalize_remote_server_target(api_url: &str) -> String { .to_string() } -fn build_unix_socket_http_client(path: &Path) -> Result { +fn build_unix_socket_http_client(path: &Path) -> Result { cli_http_client_builder() .unix_socket(path) .no_proxy() @@ -182,7 +182,7 @@ fn build_unix_socket_http_client(path: &Path) -> Result { .context("Failed to build Unix-socket HTTP client for fabro server") } -fn unix_socket_api_client_bundle(http_client: reqwest::Client) -> ServerStoreClient { +fn unix_socket_api_client_bundle(http_client: fabro_http::HttpClient) -> ServerStoreClient { let base_url = "http://fabro".to_string(); let client = fabro_api::Client::new_with_client(&base_url, http_client.clone()); ServerStoreClient { @@ -204,7 +204,7 @@ async fn connect_unix_socket_api_client_bundle(path: &Path) -> Result Result<()> { +async fn check_server_ready(http_client: &fabro_http::HttpClient) -> Result<()> { match http_client.get("http://fabro/health").send().await { Ok(response) if response.status().is_success() => Ok(()), Ok(response) => bail!("server health check returned status {}", response.status()), @@ -212,7 +212,7 @@ async fn check_server_ready(http_client: &reqwest::Client) -> Result<()> { } } -async fn wait_for_server_ready(http_client: &reqwest::Client) -> Result<()> { +async fn wait_for_server_ready(http_client: &fabro_http::HttpClient) -> Result<()> { let deadline = std::time::Instant::now() + Duration::from_secs(5); let mut last_error = None; @@ -268,7 +268,7 @@ impl ServerStoreClient { } #[allow(dead_code)] - pub(crate) fn http_client(&self) -> &reqwest::Client { + pub(crate) fn http_client(&self) -> &fabro_http::HttpClient { &self.http_client } @@ -584,8 +584,8 @@ impl ServerStoreClient { Ok(bytes) } - fn stage_artifacts_url(&self, run_id: &RunId, stage_id: &StageId) -> Result { - let mut url = reqwest::Url::parse(&self.base_url) + fn stage_artifacts_url(&self, run_id: &RunId, stage_id: &StageId) -> Result { + let mut url = fabro_http::Url::parse(&self.base_url) .with_context(|| format!("invalid server base URL {}", self.base_url))?; url.path_segments_mut() .map_err(|()| anyhow!("server base URL cannot accept path segments"))? @@ -620,7 +620,7 @@ impl ServerStoreClient { .await .with_context(|| format!("failed to stat artifact {}", path.display()))? .len(); - let body = reqwest::Body::wrap_stream(ReaderStream::new(file)); + let body = fabro_http::Body::wrap_stream(ReaderStream::new(file)); let response = self .http_client @@ -670,7 +670,7 @@ impl ServerStoreClient { file_parts.push(( part_name, Part::stream_with_length( - reqwest::Body::wrap_stream(ReaderStream::new(file)), + fabro_http::Body::wrap_stream(ReaderStream::new(file)), content_length, ) .file_name(artifact.path.clone()), @@ -819,7 +819,7 @@ where } } -async fn ensure_raw_response_success(response: reqwest::Response) -> Result<()> { +async fn ensure_raw_response_success(response: fabro_http::Response) -> Result<()> { if response.status().is_success() { return Ok(()); } @@ -851,10 +851,10 @@ where { match err { progenitor_client::Error::ErrorResponse(response) => { - response.status() == reqwest::StatusCode::NOT_FOUND + response.status() == fabro_http::StatusCode::NOT_FOUND } progenitor_client::Error::UnexpectedResponse(response) => { - response.status() == reqwest::StatusCode::NOT_FOUND + response.status() == fabro_http::StatusCode::NOT_FOUND } _ => false, } diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 317a9b332..316f45919 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -193,13 +193,13 @@ pub(crate) fn exec_server_target( Ok(target) } -pub(crate) fn cli_http_client_builder() -> reqwest::ClientBuilder { - reqwest::Client::builder().user_agent(format!("fabro-cli/{FABRO_VERSION}")) +pub(crate) fn cli_http_client_builder() -> fabro_http::HttpClientBuilder { + fabro_http::HttpClientBuilder::new().user_agent(format!("fabro-cli/{FABRO_VERSION}")) } pub(crate) fn build_server_client( tls: Option<&ClientTlsSettings>, -) -> anyhow::Result { +) -> anyhow::Result { let Some(tls) = tls else { return Ok(cli_http_client_builder().build()?); }; @@ -216,8 +216,8 @@ pub(crate) fn build_server_client( identity_pem.push(b'\n'); identity_pem.extend_from_slice(&key_pem); - let identity = reqwest::Identity::from_pem(&identity_pem)?; - let ca_cert = reqwest::Certificate::from_pem(&ca_pem)?; + let identity = fabro_http::Identity::from_pem(&identity_pem)?; + let ca_cert = fabro_http::Certificate::from_pem(&ca_pem)?; let client = cli_http_client_builder() .use_rustls_tls() diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index ee65af4f4..8d2a9c1d0 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -14,11 +14,11 @@ use crate::support::{example_fixture, fabro_json_snapshot, run_output_filters, u const SHARED_DAEMON_TIMEOUT: Duration = Duration::from_secs(30); -fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { +fn server_endpoint(storage_dir: &Path) -> (fabro_http::HttpClient, String) { let target = server_target(storage_dir); if target.starts_with('/') { ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(target) .no_proxy() .build() @@ -27,7 +27,7 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { ) } else { ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .no_proxy() .build() .expect("test TCP HTTP client should build"), @@ -36,7 +36,11 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { } } -async fn wait_for_server_question(client: &reqwest::Client, base_url: &str, run_id: &str) -> Value { +async fn wait_for_server_question( + client: &fabro_http::HttpClient, + base_url: &str, + run_id: &str, +) -> Value { let deadline = std::time::Instant::now() + SHARED_DAEMON_TIMEOUT; loop { let response = client @@ -216,7 +220,9 @@ fn attach_before_completion_streams_to_finished_state() { attach_cmd.current_dir(&context.temp_dir); attach_cmd.env("NO_COLOR", "1"); attach_cmd.env("HOME", &context.home_dir); - attach_cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + attach_cmd + .env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); attach_cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); attach_cmd.env("FABRO_TEST_IN_MEMORY_STORE", "1"); attach_cmd.args(["attach", &run_id]); @@ -818,7 +824,7 @@ fn attach_json_errors_without_prompting_for_human_input() { .send() .await .expect("answer submission should succeed"); - assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT); + assert_eq!(response.status(), fabro_http::StatusCode::NO_CONTENT); }); wait_for_status(&run.run_dir, &["succeeded"]); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs index f5ea877c5..553af0aeb 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs @@ -69,7 +69,8 @@ async fn twin_doctor() { cmd.env_clear(); cmd.env("NO_COLOR", "1"); cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.env("FABRO_STORAGE_DIR", &context.storage_dir); cmd.env( "PATH", diff --git a/lib/crates/fabro-cli/tests/it/cmd/exec.rs b/lib/crates/fabro-cli/tests/it/cmd/exec.rs index c82db1081..4cc2ab30b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/exec.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/exec.rs @@ -108,7 +108,8 @@ fn exec_uses_user_config_defaults() { cmd.env_clear(); cmd.env("HOME", &context.home_dir); cmd.env("FABRO_STORAGE_DIR", &context.storage_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); fabro_snapshot!(context.filters(), cmd, @" success: false @@ -133,7 +134,8 @@ fn exec_server_target_uses_remote_transport_instead_of_local_api_key_resolution( let mut cmd = context.exec_cmd(); cmd.env_clear(); cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.args([ "--server", &format!("{}/api/v1", server.base_url()), @@ -175,7 +177,8 @@ fn exec_configured_server_target_alone_does_not_reroute_exec() { let mut cmd = context.exec_cmd(); cmd.env_clear(); cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.args([ "--provider", "openai", @@ -222,7 +225,8 @@ fn exec_cli_server_target_overrides_configured_server_target() { let mut cmd = context.exec_cmd(); cmd.env_clear(); cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.args([ "--server", &format!("{}/api/v1", cli_server.base_url()), diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs index a4b6f295e..81267fbef 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs @@ -72,7 +72,7 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() { serde_json::from_str(&std::fs::read_to_string(record_path).unwrap()).unwrap(); let (client, base_url) = match record.bind { Bind::Unix(path) => ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(path) .no_proxy() .build() @@ -80,7 +80,10 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() { "http://fabro".to_string(), ), Bind::Tcp(addr) => ( - reqwest::ClientBuilder::new().no_proxy().build().unwrap(), + fabro_http::HttpClientBuilder::new() + .no_proxy() + .build() + .unwrap(), format!("http://{addr}"), ), }; diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 0b2fea0b0..e806de311 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -46,7 +46,8 @@ fn spawn_worker_process( cmd.current_dir(&context.temp_dir); cmd.env("NO_COLOR", "1"); cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); cmd.env("FABRO_TEST_IN_MEMORY_STORE", "1"); cmd.args([ @@ -98,11 +99,11 @@ fn child_output(mut child: Child, status: ExitStatus) -> Output { } } -fn server_endpoint(storage_dir: &std::path::Path) -> (reqwest::Client, String) { +fn server_endpoint(storage_dir: &std::path::Path) -> (fabro_http::HttpClient, String) { let target = server_target(storage_dir); if target.starts_with('/') { ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(target) .no_proxy() .build() @@ -111,7 +112,7 @@ fn server_endpoint(storage_dir: &std::path::Path) -> (reqwest::Client, String) { ) } else { ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .no_proxy() .build() .expect("test TCP HTTP client should build"), @@ -121,7 +122,7 @@ fn server_endpoint(storage_dir: &std::path::Path) -> (reqwest::Client, String) { } async fn wait_for_server_question( - client: &reqwest::Client, + client: &fabro_http::HttpClient, base_url: &str, run_id: &str, ) -> serde_json::Value { @@ -608,7 +609,7 @@ fn detached_run_answers_pending_question_without_interview_scratch_files() { .send() .await .expect("answer submission should succeed"); - assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT); + assert_eq!(response.status(), fabro_http::StatusCode::NO_CONTENT); question_id }); diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index da04149fa..8525a9e4c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -350,6 +350,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { .env("HOME", home_dir) .env("FABRO_CONFIG", config_path) .env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled") .args(["ps", "-a", "--json"]) .output() .expect("ps command should execute") @@ -442,6 +443,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { .env("NO_COLOR", "1") .env("FABRO_CONFIG", &config_path) .env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled") .args(["server", "stop", "--timeout", "0"]) .output() .expect("server stop should execute"); diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index 5c78df473..9f2b4fefe 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -581,14 +581,14 @@ struct TestServerRecord { bind: Bind, } -fn server_endpoint(storage_dir: &Path) -> Option<(reqwest::Client, String)> { +fn server_endpoint(storage_dir: &Path) -> Option<(fabro_http::HttpClient, String)> { let record_path = Storage::new(storage_dir).server_state().record_path(); let record = std::fs::read_to_string(record_path) .ok() .and_then(|content| serde_json::from_str::(&content).ok())?; match record.bind { Bind::Unix(path) if path.exists() => Some(( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(path) .no_proxy() .build() @@ -597,7 +597,7 @@ fn server_endpoint(storage_dir: &Path) -> Option<(reqwest::Client, String)> { )), Bind::Unix(_) => None, Bind::Tcp(addr) => Some(( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .no_proxy() .build() .expect("test TCP HTTP client should build"), diff --git a/lib/crates/fabro-cli/tests/it/scenario/mod.rs b/lib/crates/fabro-cli/tests/it/scenario/mod.rs index 66da396dd..f54e1bd8c 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/mod.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/mod.rs @@ -41,7 +41,7 @@ struct TestServerRecord { bind: Bind, } -fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { +fn server_endpoint(storage_dir: &Path) -> (fabro_http::HttpClient, String) { let record_path = Storage::new(storage_dir).server_state().record_path(); let record: TestServerRecord = serde_json::from_str( &std::fs::read_to_string(record_path).expect("server record should exist"), @@ -49,7 +49,7 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { .expect("server record should parse"); match record.bind { Bind::Unix(path) => ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(path) .no_proxy() .build() @@ -57,7 +57,7 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { "http://fabro".to_string(), ), Bind::Tcp(addr) => ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .no_proxy() .build() .expect("test TCP HTTP client should build"), diff --git a/lib/crates/fabro-cli/tests/it/support/mod.rs b/lib/crates/fabro-cli/tests/it/support/mod.rs index 5fcd359e0..2907f43ac 100644 --- a/lib/crates/fabro-cli/tests/it/support/mod.rs +++ b/lib/crates/fabro-cli/tests/it/support/mod.rs @@ -91,7 +91,8 @@ impl LightweightCli { } cmd.env("HOME", self.home_dir.path()); cmd.env("NO_COLOR", "1"); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.current_dir(self.home_dir.path()); cmd } diff --git a/lib/crates/fabro-cli/tests/it/workflow/mod.rs b/lib/crates/fabro-cli/tests/it/workflow/mod.rs index d81c80ee9..185c35a41 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/mod.rs @@ -110,7 +110,7 @@ struct TestServerRecord { bind: Bind, } -fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { +fn server_endpoint(storage_dir: &Path) -> (fabro_http::HttpClient, String) { let record_path = Storage::new(storage_dir).server_state().record_path(); let record: TestServerRecord = serde_json::from_str( &std::fs::read_to_string(record_path).expect("server record should exist"), @@ -118,7 +118,7 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { .expect("server record should parse"); match record.bind { Bind::Unix(path) => ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .unix_socket(path) .no_proxy() .build() @@ -126,7 +126,7 @@ fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { "http://fabro".to_string(), ), Bind::Tcp(addr) => ( - reqwest::ClientBuilder::new() + fabro_http::HttpClientBuilder::new() .no_proxy() .build() .expect("test TCP HTTP client should build"), diff --git a/lib/crates/fabro-devcontainer/Cargo.toml b/lib/crates/fabro-devcontainer/Cargo.toml index 09045f6d3..89a26c394 100644 --- a/lib/crates/fabro-devcontainer/Cargo.toml +++ b/lib/crates/fabro-devcontainer/Cargo.toml @@ -14,13 +14,13 @@ workspace = true [dependencies] fabro-util = { path = "../fabro-util" } +fabro-http.workspace = true serde = { workspace = true } serde_json = { workspace = true } serde_yaml = "0.9" thiserror = { workspace = true } tracing = { workspace = true } tokio = { workspace = true } -reqwest = { workspace = true } [dev-dependencies] insta = { workspace = true } diff --git a/lib/crates/fabro-devcontainer/src/features.rs b/lib/crates/fabro-devcontainer/src/features.rs index 6d56ce254..bd88e1ed7 100644 --- a/lib/crates/fabro-devcontainer/src/features.rs +++ b/lib/crates/fabro-devcontainer/src/features.rs @@ -248,7 +248,10 @@ async fn fetch_feature_https( info!(feature_id, "downloading feature from HTTPS"); - let response = reqwest::get(feature_id) + let response = fabro_http::http_client() + .map_err(|e| DevcontainerError::Feature(format!("failed to build HTTP client: {e}")))? + .get(feature_id) + .send() .await .map_err(|e| DevcontainerError::Feature(format!("failed to download {feature_id}: {e}")))?; diff --git a/lib/crates/fabro-github/Cargo.toml b/lib/crates/fabro-github/Cargo.toml index 830eb7eb0..472206c04 100644 --- a/lib/crates/fabro-github/Cargo.toml +++ b/lib/crates/fabro-github/Cargo.toml @@ -15,7 +15,7 @@ workspace = true [dependencies] serde.workspace = true serde_json.workspace = true -reqwest.workspace = true +fabro-http.workspace = true jsonwebtoken.workspace = true chrono.workspace = true tracing.workspace = true diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 286b185db..825c77751 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -10,6 +10,10 @@ pub fn github_api_base_url() -> String { std::env::var("GITHUB_BASE_URL").unwrap_or_else(|_| GITHUB_API_BASE_URL.to_string()) } +fn http_client() -> Result { + fabro_http::http_client().map_err(|err| err.to_string()) +} + /// Detailed information about a pull request from the GitHub API. #[derive(Debug, Clone, Deserialize, Serialize)] pub struct PullRequestDetail { @@ -186,7 +190,7 @@ impl HttpResponse { /// Abstract HTTP client for GitHub API calls. /// -/// Implemented for `reqwest::Client` in production; tests use a mock +/// Implemented for `fabro_http::HttpClient` in production; tests use a mock /// to avoid TCP/process overhead. pub trait HttpClient: Send + Sync { fn request( @@ -198,7 +202,7 @@ pub trait HttpClient: Send + Sync { ) -> impl std::future::Future> + Send; } -impl HttpClient for reqwest::Client { +impl HttpClient for fabro_http::HttpClient { async fn request( &self, method: HttpMethod, @@ -474,7 +478,7 @@ pub async fn create_pull_request( node_id: String, } - let client = reqwest::Client::new(); + let client = http_client()?; let token = creds .resolve_bearer_token( &client, @@ -571,7 +575,7 @@ pub async fn enable_auto_merge( merge_method: AutoMergeMethod, base_url: &str, ) -> Result<(), String> { - let client = reqwest::Client::new(); + let client = http_client()?; let token = creds .resolve_bearer_token( &client, @@ -697,15 +701,8 @@ pub async fn branch_exists( branch: &str, base_url: &str, ) -> Result { - branch_exists_with_client( - &reqwest::Client::new(), - creds, - owner, - repo, - branch, - base_url, - ) - .await + let client = http_client()?; + branch_exists_with_client(&client, creds, owner, repo, branch, base_url).await } async fn branch_exists_with_client( @@ -854,7 +851,7 @@ pub async fn resolve_clone_credentials( let token = match creds { GitHubCredentials::Token(token) => token.clone(), GitHubCredentials::App(_) => { - let client = reqwest::Client::new(); + let client = http_client()?; creds .resolve_bearer_token( &client, @@ -902,15 +899,8 @@ pub async fn get_pull_request( number: u64, base_url: &str, ) -> Result { - get_pull_request_with_client( - &reqwest::Client::new(), - creds, - owner, - repo, - number, - base_url, - ) - .await + let client = http_client()?; + get_pull_request_with_client(&client, creds, owner, repo, number, base_url).await } async fn get_pull_request_with_client( @@ -974,16 +964,8 @@ pub async fn merge_pull_request( method: &str, base_url: &str, ) -> Result<(), String> { - merge_pull_request_with_client( - &reqwest::Client::new(), - creds, - owner, - repo, - number, - method, - base_url, - ) - .await + let client = http_client()?; + merge_pull_request_with_client(&client, creds, owner, repo, number, method, base_url).await } #[allow(clippy::too_many_arguments)] @@ -1045,15 +1027,8 @@ pub async fn close_pull_request( number: u64, base_url: &str, ) -> Result<(), String> { - close_pull_request_with_client( - &reqwest::Client::new(), - creds, - owner, - repo, - number, - base_url, - ) - .await + let client = http_client()?; + close_pull_request_with_client(&client, creds, owner, repo, number, base_url).await } async fn close_pull_request_with_client( diff --git a/lib/crates/fabro-hooks/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index f77c65eda..eed88ed19 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -20,12 +20,12 @@ fabro-model = { path = "../fabro-model" } fabro-template = { path = "../fabro-template" } fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } +fabro-http.workspace = true serde.workspace = true serde_json.workspace = true tokio.workspace = true async-trait.workspace = true regex.workspace = true -reqwest.workspace = true tracing.workspace = true tokio-util.workspace = true diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 807d69ef7..c5722a143 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -438,11 +438,11 @@ impl HookExecutorImpl { } /// Build a reqwest client for the given TLS mode. - fn build_http_client(tls: TlsMode) -> reqwest::Client { + fn build_http_client(tls: TlsMode) -> fabro_http::HttpClient { let accept_invalid = matches!(tls, TlsMode::NoVerify | TlsMode::Off); #[cfg(test)] { - reqwest::Client::builder() + fabro_http::HttpClientBuilder::new() .danger_accept_invalid_certs(accept_invalid) .no_proxy() .build() @@ -450,7 +450,7 @@ impl HookExecutorImpl { } #[cfg(not(test))] { - reqwest::Client::builder() + fabro_http::HttpClientBuilder::new() .danger_accept_invalid_certs(accept_invalid) .build() .unwrap_or_default() @@ -461,7 +461,7 @@ impl HookExecutorImpl { /// Fail-open: non-2xx and connection errors return `Proceed`. #[allow(clippy::too_many_arguments)] async fn execute_http( - client: &reqwest::Client, + client: &fabro_http::HttpClient, url: &str, headers: Option<&HashMap>, allowed_env_vars: &[String], @@ -560,9 +560,9 @@ impl HookExecutorImpl { /// Cached HTTP clients keyed by TLS mode. struct HttpClientCache { - verify: reqwest::Client, - no_verify: reqwest::Client, - off: reqwest::Client, + verify: fabro_http::HttpClient, + no_verify: fabro_http::HttpClient, + off: fabro_http::HttpClient, } impl HttpClientCache { @@ -574,7 +574,7 @@ impl HttpClientCache { } } - fn get(&self, tls: TlsMode) -> &reqwest::Client { + fn get(&self, tls: TlsMode) -> &fabro_http::HttpClient { match tls { TlsMode::Verify => &self.verify, TlsMode::NoVerify => &self.no_verify, @@ -704,7 +704,7 @@ mod tests { )) } - fn test_http_client() -> reqwest::Client { + fn test_http_client() -> fabro_http::HttpClient { HookExecutorImpl::build_http_client(TlsMode::Off) } diff --git a/lib/crates/fabro-http/Cargo.toml b/lib/crates/fabro-http/Cargo.toml new file mode 100644 index 000000000..ed79e54a4 --- /dev/null +++ b/lib/crates/fabro-http/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "fabro-http" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Shared HTTP client utilities and reqwest wrappers for Fabro" + +[lib] +doctest = false + +[lints] +workspace = true + +[dependencies] +reqwest = { workspace = true, features = ["blocking"] } +thiserror.workspace = true + +[dev-dependencies] +http = "1" diff --git a/lib/crates/fabro-http/src/lib.rs b/lib/crates/fabro-http/src/lib.rs new file mode 100644 index 000000000..1a560cbeb --- /dev/null +++ b/lib/crates/fabro-http/src/lib.rs @@ -0,0 +1,351 @@ +#![allow( + clippy::absolute_paths, + clippy::disallowed_methods, + clippy::disallowed_types +)] + +use std::path::Path; +use std::time::Duration; + +pub use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; +pub use reqwest::tls::{Certificate, Identity}; +pub use reqwest::{ + Body, Method, RequestBuilder, Response, StatusCode, Url, header, multipart, tls, +}; + +pub type HttpClient = reqwest::Client; +pub type BlockingHttpClient = reqwest::blocking::Client; +pub type BlockingRequestBuilder = reqwest::blocking::RequestBuilder; +pub type BlockingResponse = reqwest::blocking::Response; +pub type Proxy = reqwest::Proxy; + +pub const HTTP_PROXY_POLICY_ENV: &str = "FABRO_HTTP_PROXY_POLICY"; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ProxyPolicy { + System, + Disabled, +} + +impl ProxyPolicy { + fn parse(value: &str) -> Result { + match value.to_ascii_lowercase().as_str() { + "system" => Ok(Self::System), + "disabled" => Ok(Self::Disabled), + _ => Err(HttpClientBuildError::InvalidProxyPolicy(value.to_string())), + } + } + + fn resolve(explicit: Option) -> Result { + if let Some(policy) = explicit { + return Ok(policy); + } + + match std::env::var(HTTP_PROXY_POLICY_ENV) { + Ok(value) => Self::parse(&value), + Err(std::env::VarError::NotPresent) => Ok(Self::System), + Err(std::env::VarError::NotUnicode(value)) => Err( + HttpClientBuildError::InvalidProxyPolicy(value.to_string_lossy().into_owned()), + ), + } + } +} + +#[derive(Debug, thiserror::Error)] +pub enum HttpClientBuildError { + #[error("invalid {HTTP_PROXY_POLICY_ENV} value `{0}`; expected `system` or `disabled`")] + InvalidProxyPolicy(String), + + #[error(transparent)] + Reqwest(#[from] reqwest::Error), +} + +#[derive(Default)] +pub struct HttpClientBuilder { + inner: reqwest::ClientBuilder, + proxy_policy: Option, +} + +impl HttpClientBuilder { + #[must_use] + pub fn new() -> Self { + Self { + inner: reqwest::Client::builder(), + proxy_policy: None, + } + } + + #[must_use] + pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { + self.proxy_policy = Some(proxy_policy); + self + } + + #[must_use] + pub fn no_proxy(mut self) -> Self { + self.inner = self.inner.no_proxy(); + self + } + + #[must_use] + pub fn proxy(mut self, proxy: Proxy) -> Self { + self.inner = self.inner.proxy(proxy); + self + } + + #[must_use] + pub fn user_agent(mut self, value: impl Into) -> Self { + self.inner = self.inner.user_agent(value.into()); + self + } + + #[must_use] + pub fn default_headers(mut self, headers: HeaderMap) -> Self { + self.inner = self.inner.default_headers(headers); + self + } + + #[must_use] + pub fn connect_timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.connect_timeout(timeout); + self + } + + #[must_use] + pub fn timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.timeout(timeout); + self + } + + #[must_use] + pub fn read_timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.read_timeout(timeout); + self + } + + #[must_use] + pub fn use_rustls_tls(mut self) -> Self { + self.inner = self.inner.use_rustls_tls(); + self + } + + #[must_use] + pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { + self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); + self + } + + #[must_use] + pub fn add_root_certificate(mut self, cert: Certificate) -> Self { + self.inner = self.inner.add_root_certificate(cert); + self + } + + #[must_use] + pub fn identity(mut self, identity: Identity) -> Self { + self.inner = self.inner.identity(identity); + self + } + + #[cfg(unix)] + #[must_use] + pub fn unix_socket

(mut self, path: P) -> Self + where + P: AsRef, + { + self.inner = self.inner.unix_socket(path.as_ref()); + self + } + + pub fn build(self) -> Result { + let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; + let inner = match proxy_policy { + ProxyPolicy::System => self.inner, + ProxyPolicy::Disabled => self.inner.no_proxy(), + }; + inner.build().map_err(Into::into) + } +} + +pub fn http_client() -> Result { + HttpClientBuilder::new().build() +} + +pub fn test_http_client() -> Result { + HttpClientBuilder::new() + .proxy_policy(ProxyPolicy::Disabled) + .build() +} + +#[derive(Default)] +pub struct BlockingHttpClientBuilder { + inner: reqwest::blocking::ClientBuilder, + proxy_policy: Option, +} + +impl BlockingHttpClientBuilder { + #[must_use] + pub fn new() -> Self { + Self { + inner: reqwest::blocking::Client::builder(), + proxy_policy: None, + } + } + + #[must_use] + pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { + self.proxy_policy = Some(proxy_policy); + self + } + + #[must_use] + pub fn no_proxy(mut self) -> Self { + self.inner = self.inner.no_proxy(); + self + } + + #[must_use] + pub fn proxy(mut self, proxy: Proxy) -> Self { + self.inner = self.inner.proxy(proxy); + self + } + + #[must_use] + pub fn user_agent(mut self, value: impl Into) -> Self { + self.inner = self.inner.user_agent(value.into()); + self + } + + #[must_use] + pub fn default_headers(mut self, headers: HeaderMap) -> Self { + self.inner = self.inner.default_headers(headers); + self + } + + #[must_use] + pub fn connect_timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.connect_timeout(timeout); + self + } + + #[must_use] + pub fn timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.timeout(timeout); + self + } + + #[must_use] + pub fn use_rustls_tls(mut self) -> Self { + self.inner = self.inner.use_rustls_tls(); + self + } + + #[must_use] + pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { + self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); + self + } + + #[must_use] + pub fn add_root_certificate(mut self, cert: Certificate) -> Self { + self.inner = self.inner.add_root_certificate(cert); + self + } + + #[must_use] + pub fn identity(mut self, identity: Identity) -> Self { + self.inner = self.inner.identity(identity); + self + } + + #[cfg(unix)] + #[must_use] + pub fn unix_socket

(mut self, path: P) -> Self + where + P: AsRef, + { + self.inner = self.inner.unix_socket(path.as_ref()); + self + } + + pub fn build(self) -> Result { + let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; + let inner = match proxy_policy { + ProxyPolicy::System => self.inner, + ProxyPolicy::Disabled => self.inner.no_proxy(), + }; + inner.build().map_err(Into::into) + } +} + +pub fn blocking_http_client() -> Result { + BlockingHttpClientBuilder::new().build() +} + +pub fn blocking_test_http_client() -> Result { + BlockingHttpClientBuilder::new() + .proxy_policy(ProxyPolicy::Disabled) + .build() +} + +#[cfg(test)] +mod tests { + use super::*; + + struct EnvGuard { + key: &'static str, + original: Option, + } + + impl EnvGuard { + fn set(key: &'static str, value: Option<&str>) -> Self { + let original = std::env::var_os(key); + match value { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + Self { key, original } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.original.as_ref() { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } + + #[test] + fn proxy_policy_defaults_to_system() { + let _guard = EnvGuard::set(HTTP_PROXY_POLICY_ENV, None); + assert_eq!(ProxyPolicy::resolve(None).unwrap(), ProxyPolicy::System); + } + + #[test] + fn proxy_policy_reads_disabled_from_env() { + let _guard = EnvGuard::set(HTTP_PROXY_POLICY_ENV, Some("disabled")); + assert_eq!(ProxyPolicy::resolve(None).unwrap(), ProxyPolicy::Disabled); + } + + #[test] + fn proxy_policy_rejects_invalid_env_values() { + let _guard = EnvGuard::set(HTTP_PROXY_POLICY_ENV, Some("bogus")); + let error = ProxyPolicy::resolve(None).unwrap_err(); + assert!( + error + .to_string() + .contains("expected `system` or `disabled`") + ); + } + + #[test] + fn explicit_proxy_policy_overrides_env() { + let _guard = EnvGuard::set(HTTP_PROXY_POLICY_ENV, Some("system")); + assert_eq!( + ProxyPolicy::resolve(Some(ProxyPolicy::Disabled)).unwrap(), + ProxyPolicy::Disabled + ); + } +} diff --git a/lib/crates/fabro-llm/Cargo.toml b/lib/crates/fabro-llm/Cargo.toml index 7c62ca633..676715b7e 100644 --- a/lib/crates/fabro-llm/Cargo.toml +++ b/lib/crates/fabro-llm/Cargo.toml @@ -27,11 +27,11 @@ rand.workspace = true futures.workspace = true tokio-stream.workspace = true async-trait.workspace = true -reqwest.workspace = true base64.workspace = true bytes.workspace = true tokio-util.workspace = true tracing.workspace = true +fabro-http.workspace = true fabro-model = { path = "../fabro-model" } fabro-util = { path = "../fabro-util" } diff --git a/lib/crates/fabro-llm/src/providers/anthropic.rs b/lib/crates/fabro-llm/src/providers/anthropic.rs index d0ab4a060..aa96e021a 100644 --- a/lib/crates/fabro-llm/src/providers/anthropic.rs +++ b/lib/crates/fabro-llm/src/providers/anthropic.rs @@ -987,7 +987,7 @@ struct SseReaderState { impl SseReaderState { fn new( - http_resp: reqwest::Response, + http_resp: fabro_http::Response, rate_limit: Option, json_schema_mode: bool, stream_read_timeout: Option, @@ -1078,7 +1078,7 @@ fn build_api_request( adapter: &Adapter, request: &Request, stream: bool, -) -> (ApiRequest, reqwest::RequestBuilder) { +) -> (ApiRequest, fabro_http::RequestBuilder) { let (system, other_messages) = extract_system_prompt(&request.messages); let mut api_messages = translate_messages(&other_messages); diff --git a/lib/crates/fabro-llm/src/providers/common.rs b/lib/crates/fabro-llm/src/providers/common.rs index 72e1e7cb3..2a227476e 100644 --- a/lib/crates/fabro-llm/src/providers/common.rs +++ b/lib/crates/fabro-llm/src/providers/common.rs @@ -1,6 +1,6 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; -use reqwest::header::HeaderMap; +use fabro_http::HeaderMap; use tokio::time; use tracing::warn; @@ -169,7 +169,7 @@ pub fn parse_rate_limit_headers(headers: &HeaderMap) -> Option { /// Returns `Error::Network` on connection failure or `Error::Provider` on /// non-success status. pub async fn send_and_read_response( - request: reqwest::RequestBuilder, + request: fabro_http::RequestBuilder, provider: &str, error_code_field: &str, ) -> Result<(String, HeaderMap), Error> { @@ -209,18 +209,18 @@ pub async fn send_and_read_response( /// Shared line reader for SSE streams. /// -/// Buffers bytes from a `reqwest::Response` and splits them by a configurable -/// delimiter (e.g. `"\n"` for Gemini/OpenAI-compatible, `"\n\n"` for -/// Anthropic/OpenAI SSE event blocks). +/// Buffers bytes from a `fabro_http::Response` and splits them by a +/// configurable delimiter (e.g. `"\n"` for Gemini/OpenAI-compatible, `"\n\n"` +/// for Anthropic/OpenAI SSE event blocks). pub struct LineReader { - response: reqwest::Response, + response: fabro_http::Response, buffer: String, stream_read_timeout: Option, } impl LineReader { pub fn new( - response: reqwest::Response, + response: fabro_http::Response, stream_read_timeout: Option, ) -> Self { Self { diff --git a/lib/crates/fabro-llm/src/providers/fabro_server.rs b/lib/crates/fabro-llm/src/providers/fabro_server.rs index 513a2512a..e7f5bcb51 100644 --- a/lib/crates/fabro-llm/src/providers/fabro_server.rs +++ b/lib/crates/fabro-llm/src/providers/fabro_server.rs @@ -10,14 +10,14 @@ use crate::types::{FinishReason, Message, Request, Response, StreamEvent, TokenC /// `/completions` endpoint, delegating to whatever real provider the server /// is configured with. pub struct Adapter { - client: reqwest::Client, + client: fabro_http::HttpClient, base_url: String, provider_name: String, } impl Adapter { pub fn new( - client: reqwest::Client, + client: fabro_http::HttpClient, base_url: impl Into, provider_name: impl Into, ) -> Self { @@ -74,11 +74,11 @@ fn build_body(request: &Request, stream: bool) -> Result Result { +) -> Result { let http_resp = client.post(url).json(body).send().await.map_err(|e| { if e.is_timeout() { Error::request_timeout(e.to_string(), e) @@ -228,8 +228,8 @@ mod tests { use crate::error::ProviderErrorKind; use crate::types::Message; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } fn make_request() -> Request { diff --git a/lib/crates/fabro-llm/src/providers/gemini.rs b/lib/crates/fabro-llm/src/providers/gemini.rs index f77e43970..e66d81f2c 100644 --- a/lib/crates/fabro-llm/src/providers/gemini.rs +++ b/lib/crates/fabro-llm/src/providers/gemini.rs @@ -1,7 +1,7 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use fabro_http::HeaderMap; use futures::stream; -use reqwest::header::HeaderMap; use crate::error::{ Error, ProviderErrorDetail, ProviderErrorKind, error_from_grpc_status, error_from_status_code, @@ -512,7 +512,7 @@ fn parse_usage(metadata: Option<&UsageMetadata>) -> TokenCounts { /// Like `send_and_read_response` but uses gRPC status code mapping when /// available. async fn send_gemini_response( - request: reqwest::RequestBuilder, + request: fabro_http::RequestBuilder, ) -> Result<(String, HeaderMap), Error> { let http_resp = request.send().await.map_err(|e| { if e.is_timeout() { @@ -567,13 +567,13 @@ fn gemini_error( } } -/// Send an HTTP request for streaming and return the `reqwest::Response`. +/// Send an HTTP request for streaming and return the `fabro_http::Response`. /// /// Checks for HTTP errors before returning. On error, reads the body and /// maps it to `Error` using gRPC status code mapping when available. async fn send_streaming_request( - request: reqwest::RequestBuilder, -) -> Result { + request: fabro_http::RequestBuilder, +) -> Result { let http_resp = request .send() .await @@ -596,7 +596,7 @@ async fn send_streaming_request( /// Process a stream of SSE chunks from the Gemini `streamGenerateContent` /// endpoint and yield `StreamEvent` values. fn process_sse_stream( - http_resp: reqwest::Response, + http_resp: fabro_http::Response, model: String, rate_limit: Option, stream_read_timeout: Option, @@ -712,7 +712,7 @@ struct SseStreamState { impl SseStreamState { fn new( - http_resp: reqwest::Response, + http_resp: fabro_http::Response, model: String, rate_limit: Option, stream_read_timeout: Option, diff --git a/lib/crates/fabro-llm/src/providers/http_api.rs b/lib/crates/fabro-llm/src/providers/http_api.rs index 539c554e8..7c831dd6e 100644 --- a/lib/crates/fabro-llm/src/providers/http_api.rs +++ b/lib/crates/fabro-llm/src/providers/http_api.rs @@ -12,16 +12,16 @@ pub struct HttpApi { pub(crate) api_key: String, pub(crate) base_url: String, pub(crate) default_headers: HashMap, - pub(crate) client: reqwest::Client, + pub(crate) client: fabro_http::HttpClient, pub(crate) request_timeout: Option, pub(crate) stream_read_timeout: Option, } impl HttpApi { - fn build_client(timeout: AdapterTimeout) -> reqwest::Client { + fn build_client(timeout: AdapterTimeout) -> fabro_http::HttpClient { #[cfg(test)] { - reqwest::Client::builder() + fabro_http::HttpClientBuilder::new() .connect_timeout(Duration::from_secs_f64(timeout.connect)) .no_proxy() .build() @@ -29,7 +29,7 @@ impl HttpApi { } #[cfg(not(test))] { - reqwest::Client::builder() + fabro_http::HttpClientBuilder::new() .connect_timeout(Duration::from_secs_f64(timeout.connect)) .build() .unwrap_or_default() diff --git a/lib/crates/fabro-llm/src/providers/openai.rs b/lib/crates/fabro-llm/src/providers/openai.rs index 8476676d0..23d5d020d 100644 --- a/lib/crates/fabro-llm/src/providers/openai.rs +++ b/lib/crates/fabro-llm/src/providers/openai.rs @@ -80,9 +80,9 @@ impl Adapter { } } - /// Build a `reqwest::RequestBuilder` with default headers, org/project + /// Build a `fabro_http::RequestBuilder` with default headers, org/project /// headers, and auth. - fn build_request(&self, url: &str) -> reqwest::RequestBuilder { + fn build_request(&self, url: &str) -> fabro_http::RequestBuilder { let mut req = self.http.client.post(url); // Apply default_headers first so adapter-specific headers can override for (key, value) in &self.http.default_headers { @@ -1597,7 +1597,7 @@ mod tests { fn empty_sse_state() -> SseStreamState { let http_resp = http::Response::builder().status(200).body("").unwrap(); - let response = reqwest::Response::from(http_resp); + let response = fabro_http::Response::from(http_resp); SseStreamState { line_reader: LineReader::new(response, None), model: String::new(), diff --git a/lib/crates/fabro-llm/src/providers/openai_compatible.rs b/lib/crates/fabro-llm/src/providers/openai_compatible.rs index 10d39632c..354b83b38 100644 --- a/lib/crates/fabro-llm/src/providers/openai_compatible.rs +++ b/lib/crates/fabro-llm/src/providers/openai_compatible.rs @@ -54,8 +54,8 @@ impl Adapter { } } - /// Build a `reqwest::RequestBuilder` with default headers and auth. - fn build_request(&self, url: &str) -> reqwest::RequestBuilder { + /// Build a `fabro_http::RequestBuilder` with default headers and auth. + fn build_request(&self, url: &str) -> fabro_http::RequestBuilder { let mut req = self.http.client.post(url); // Apply default_headers first so adapter-specific headers can override for (key, value) in &self.http.default_headers { @@ -688,7 +688,7 @@ struct StreamState { impl StreamState { fn new( - response: reqwest::Response, + response: fabro_http::Response, provider_name: String, model: String, rate_limit: Option, @@ -969,7 +969,7 @@ mod tests { #[test] fn stream_state_process_text_chunks() { let http_resp = - reqwest::Response::from(http::Response::builder().status(200).body("").unwrap()); + fabro_http::Response::from(http::Response::builder().status(200).body("").unwrap()); let mut state = StreamState::new( http_resp, "test".into(), @@ -1001,7 +1001,7 @@ mod tests { #[test] fn stream_state_process_tool_call_chunks() { let http_resp = - reqwest::Response::from(http::Response::builder().status(200).body("").unwrap()); + fabro_http::Response::from(http::Response::builder().status(200).body("").unwrap()); let mut state = StreamState::new( http_resp, "test".into(), @@ -1032,7 +1032,7 @@ mod tests { #[test] fn stream_state_finish_events_text_only() { let http_resp = - reqwest::Response::from(http::Response::builder().status(200).body("").unwrap()); + fabro_http::Response::from(http::Response::builder().status(200).body("").unwrap()); let mut state = StreamState::new( http_resp, "test-provider".into(), @@ -1075,7 +1075,7 @@ mod tests { #[test] fn stream_state_finish_events_with_tool_calls() { let http_resp = - reqwest::Response::from(http::Response::builder().status(200).body("").unwrap()); + fabro_http::Response::from(http::Response::builder().status(200).body("").unwrap()); let mut state = StreamState::new( http_resp, "test".into(), @@ -1120,7 +1120,7 @@ mod tests { #[test] fn stream_state_uses_request_model_as_fallback() { let http_resp = - reqwest::Response::from(http::Response::builder().status(200).body("").unwrap()); + fabro_http::Response::from(http::Response::builder().status(200).body("").unwrap()); let mut state = StreamState::new( http_resp, "test".into(), diff --git a/lib/crates/fabro-mcp/src/client.rs b/lib/crates/fabro-mcp/src/client.rs index 81bbeb2b9..b282659a7 100644 --- a/lib/crates/fabro-mcp/src/client.rs +++ b/lib/crates/fabro-mcp/src/client.rs @@ -1,3 +1,5 @@ +#![allow(clippy::disallowed_methods, clippy::disallowed_types)] + use std::process::Stdio; use std::sync::Arc; use std::time::Duration; diff --git a/lib/crates/fabro-oauth/Cargo.toml b/lib/crates/fabro-oauth/Cargo.toml index ff052cf5b..a1db69c3b 100644 --- a/lib/crates/fabro-oauth/Cargo.toml +++ b/lib/crates/fabro-oauth/Cargo.toml @@ -15,7 +15,7 @@ workspace = true [dependencies] serde.workspace = true serde_json.workspace = true -reqwest.workspace = true +fabro-http.workspace = true sha2.workspace = true base64.workspace = true rand.workspace = true diff --git a/lib/crates/fabro-oauth/src/lib.rs b/lib/crates/fabro-oauth/src/lib.rs index e91ee0d10..e1315293d 100644 --- a/lib/crates/fabro-oauth/src/lib.rs +++ b/lib/crates/fabro-oauth/src/lib.rs @@ -107,7 +107,7 @@ pub struct TokenResponse { // --------------------------------------------------------------------------- pub async fn exchange_code_for_tokens( - client: &reqwest::Client, + client: &fabro_http::HttpClient, issuer: &str, client_id: &str, code: &str, @@ -154,7 +154,7 @@ pub async fn exchange_code_for_tokens( // --------------------------------------------------------------------------- pub async fn refresh_access_token( - client: &reqwest::Client, + client: &fabro_http::HttpClient, issuer: &str, client_id: &str, refresh_token: &str, @@ -394,7 +394,7 @@ pub async fn run_browser_flow( .map_err(|_| "Did not receive authorization code".to_string())? .map_err(|e| format!("Authorization failed: {e}"))?; - let client = reqwest::Client::new(); + let client = fabro_http::http_client().map_err(|e| e.to_string())?; exchange_code_for_tokens( &client, issuer, @@ -414,8 +414,8 @@ pub async fn run_browser_flow( mod tests { use super::*; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } // ----------------------------------------------------------------------- diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index 843a14343..d45c14fcd 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -32,6 +32,7 @@ fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fabro-api = { path = "../fabro-api" } fabro-store = { path = "../fabro-store" } +fabro-http.workspace = true chrono.workspace = true futures-util.workspace = true axum.workspace = true @@ -65,7 +66,6 @@ uuid.workspace = true hmac.workspace = true sha2.workspace = true hex.workspace = true -reqwest.workspace = true rand.workspace = true bytes = "1" tempfile = "3" diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 89ea73d67..abcc9f0c1 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -321,7 +321,18 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = reqwest::Client::new(); + let http = match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + return CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "client error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some(err.to_string()), + }; + } + }; let probe = timeout( Duration::from_secs(15), http.get(format!("{}/user", fabro_github::github_api_base_url())) @@ -340,7 +351,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { details: Vec::new(), remediation: None, }, - Ok(Ok(response)) if response.status() == reqwest::StatusCode::UNAUTHORIZED => { + Ok(Ok(response)) if response.status() == fabro_http::StatusCode::UNAUTHORIZED => { CheckResult { name: "GitHub CLI".to_string(), status: CheckStatus::Error, @@ -461,7 +472,18 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = reqwest::Client::new(); + let http = match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + return CheckResult { + name: "GitHub App".to_string(), + status: CheckStatus::Error, + summary: "client error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some(err.to_string()), + }; + } + }; let auth_result = timeout( Duration::from_secs(15), fabro_github::get_authenticated_app(&http, &jwt, &fabro_github::github_api_base_url()), @@ -523,9 +545,21 @@ async fn check_brave_search(state: &AppState) -> CheckResult { }; }; - let probe = timeout(Duration::from_secs(15), async { - reqwest::Client::new() - .get("https://api.search.brave.com/res/v1/web/search?q=test&count=1") + let http = match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + return CheckResult { + name: "Brave Search".to_string(), + status: CheckStatus::Warning, + summary: "client error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some(err.to_string()), + }; + } + }; + + let probe = timeout(Duration::from_secs(15), async move { + http.get("https://api.search.brave.com/res/v1/web/search?q=test&count=1") .header("X-Subscription-Token", api_key) .send() .await diff --git a/lib/crates/fabro-server/src/github_webhooks.rs b/lib/crates/fabro-server/src/github_webhooks.rs index 698abac4c..ca556dc99 100644 --- a/lib/crates/fabro-server/src/github_webhooks.rs +++ b/lib/crates/fabro-server/src/github_webhooks.rs @@ -255,7 +255,7 @@ async fn update_github_app_webhook( let jwt = fabro_github::sign_app_jwt(app_id, private_key_pem).map_err(|e| anyhow::anyhow!(e))?; - let client = reqwest::Client::new(); + let client = fabro_http::http_client()?; let body = serde_json::json!({ "url": webhook_url, "content_type": "json", @@ -287,8 +287,8 @@ mod tests { use super::*; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } // ----------------------------------------------------------------------- diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 52c1b47fa..9d291ae56 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -769,7 +769,7 @@ async fn mint_github_token( }; let jwt = fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) .map_err(|err| anyhow!("{err}"))?; - let client = reqwest::Client::new(); + let client = fabro_http::http_client()?; let perms_json = serde_json::to_value(permissions)?; fabro_github::create_installation_access_token_with_permissions( &client, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index a1f32746d..cf148cc63 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1708,7 +1708,7 @@ async fn get_github_repo( let settings = state.server_settings(); let github_settings = &settings.integrations.github; let base_url = fabro_github::github_api_base_url(); - let mut client = None; + let mut client: Option = None; let token = match github_settings.strategy { GithubIntegrationStrategy::App => { let Some(app_id) = github_settings.app_id.as_ref() else { @@ -1754,9 +1754,18 @@ async fn get_github_repo( None => format!("https://github.com/organizations/{owner}/settings/installations"), }; - let client_ref = client.get_or_insert_with(reqwest::Client::new); + if client.is_none() { + client = Some(match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) + .into_response(); + } + }); + } + let client_ref = client.as_ref().expect("client initialized above"); let installed = match fabro_github::check_app_installed( - &*client_ref, + client_ref, &jwt, &owner, &name, @@ -1787,7 +1796,7 @@ async fn get_github_repo( } match fabro_github::create_installation_access_token_with_permissions( - &*client_ref, + client_ref, &jwt, &owner, &name, @@ -1816,7 +1825,16 @@ async fn get_github_repo( }, }; - let client = client.unwrap_or_else(reqwest::Client::new); + let client = match client { + Some(client) => client, + None => match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) + .into_response(); + } + }, + }; let repo_response = match client .get(format!("{base_url}/repos/{owner}/{name}")) .header("Authorization", format!("Bearer {token}")) @@ -1830,7 +1848,7 @@ async fn get_github_repo( if github_settings.strategy == GithubIntegrationStrategy::GhCli && matches!( response.status(), - reqwest::StatusCode::FORBIDDEN | reqwest::StatusCode::NOT_FOUND + fabro_http::StatusCode::FORBIDDEN | fabro_http::StatusCode::NOT_FOUND ) => { return ( @@ -1849,7 +1867,7 @@ async fn get_github_repo( } Ok(response) if github_settings.strategy == GithubIntegrationStrategy::GhCli - && response.status() == reqwest::StatusCode::UNAUTHORIZED => + && response.status() == fabro_http::StatusCode::UNAUTHORIZED => { return ApiError::new( StatusCode::SERVICE_UNAVAILABLE, diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index ae424c03d..cafb99e38 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -207,7 +207,7 @@ async fn login_github(State(state): State>) -> Response { let state_token = format!("fabro-{}", ulid::Ulid::new()); let authorize_url = - reqwest::Url::parse_with_params("https://github.com/login/oauth/authorize", &[ + fabro_http::Url::parse_with_params("https://github.com/login/oauth/authorize", &[ ("client_id", client_id.as_str()), ("redirect_uri", &format!("{web_url}/auth/callback/github")), ("scope", "read:user user:email"), @@ -286,7 +286,16 @@ async fn callback_github( } }; - let http = reqwest::Client::new(); + let http = match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + error!(error = %err, "OAuth callback failed: could not build GitHub HTTP client"); + return json_response( + StatusCode::SERVICE_UNAVAILABLE, + json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), + ); + } + }; let token = match http .post("https://github.com/login/oauth/access_token") .header(header::ACCEPT, "application/json") @@ -522,10 +531,19 @@ async fn setup_register( .get(header::ORIGIN) .or_else(|| headers.get(header::REFERER)) .and_then(|v| v.to_str().ok()) - .and_then(|s| reqwest::Url::parse(s).ok()) + .and_then(|s| fabro_http::Url::parse(s).ok()) .map(|url| format!("{}://{}", url.scheme(), url.authority())); - let http = reqwest::Client::new(); + let http = match fabro_http::http_client() { + Ok(http) => http, + Err(err) => { + error!(error = %err, "Setup register failed: could not build GitHub HTTP client"); + return json_response( + StatusCode::SERVICE_UNAVAILABLE, + json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), + ); + } + }; let response = match http .post(format!( "https://api.github.com/app-manifests/{}/conversions", diff --git a/lib/crates/fabro-server/tests/it/api/mtls.rs b/lib/crates/fabro-server/tests/it/api/mtls.rs index 34e353eee..7353c1589 100644 --- a/lib/crates/fabro-server/tests/it/api/mtls.rs +++ b/lib/crates/fabro-server/tests/it/api/mtls.rs @@ -62,11 +62,11 @@ fn build_client( ca_cert_path: &Path, client_cert_path: Option<&Path>, client_key_path: Option<&Path>, -) -> reqwest::Client { +) -> fabro_http::HttpClient { let ca_pem = std::fs::read(ca_cert_path).unwrap(); - let ca_cert = reqwest::tls::Certificate::from_pem(&ca_pem).unwrap(); + let ca_cert = fabro_http::tls::Certificate::from_pem(&ca_pem).unwrap(); - let mut builder = reqwest::Client::builder() + let mut builder = fabro_http::HttpClientBuilder::new() .add_root_certificate(ca_cert) .no_proxy() .use_rustls_tls(); @@ -76,7 +76,7 @@ fn build_client( let key_pem = std::fs::read(key_path).unwrap(); let mut identity_pem = cert_pem; identity_pem.extend_from_slice(&key_pem); - let identity = reqwest::tls::Identity::from_pem(&identity_pem).unwrap(); + let identity = fabro_http::tls::Identity::from_pem(&identity_pem).unwrap(); builder = builder.identity(identity); } diff --git a/lib/crates/fabro-slack/Cargo.toml b/lib/crates/fabro-slack/Cargo.toml index 681a683f5..5134ed76b 100644 --- a/lib/crates/fabro-slack/Cargo.toml +++ b/lib/crates/fabro-slack/Cargo.toml @@ -15,13 +15,13 @@ workspace = true [dependencies] fabro-interview = { path = "../fabro-interview" } fabro-workflow = { path = "../fabro-workflow" } +fabro-http.workspace = true futures-util.workspace = true serde.workspace = true serde_json.workspace = true tokio.workspace = true thiserror.workspace = true tokio-tungstenite.workspace = true -reqwest.workspace = true tracing.workspace = true [dev-dependencies] diff --git a/lib/crates/fabro-slack/src/client.rs b/lib/crates/fabro-slack/src/client.rs index 1fcd0d374..b3b38b493 100644 --- a/lib/crates/fabro-slack/src/client.rs +++ b/lib/crates/fabro-slack/src/client.rs @@ -1,4 +1,3 @@ -use reqwest::Client; use serde_json::{Value, json}; use tracing::debug; @@ -14,7 +13,7 @@ pub struct PostedMessage { pub struct SlackClient { bot_token: String, api_base: String, - http: Client, + http: fabro_http::HttpClient, } impl SlackClient { @@ -24,11 +23,11 @@ impl SlackClient { Self { bot_token, api_base, - http: Client::new(), + http: fabro_http::http_client().expect("Slack HTTP client should build"), } } - pub fn http(&self) -> &Client { + pub fn http(&self) -> &fabro_http::HttpClient { &self.http } diff --git a/lib/crates/fabro-slack/src/connection.rs b/lib/crates/fabro-slack/src/connection.rs index ea344c9b4..567ea5b91 100644 --- a/lib/crates/fabro-slack/src/connection.rs +++ b/lib/crates/fabro-slack/src/connection.rs @@ -57,7 +57,7 @@ pub fn process_message( /// Fetch a WebSocket URL from Slack's `apps.connections.open` endpoint. pub async fn open_socket_url( - http: &reqwest::Client, + http: &fabro_http::HttpClient, app_token: &str, ) -> Result { let resp = http diff --git a/lib/crates/fabro-telemetry/Cargo.toml b/lib/crates/fabro-telemetry/Cargo.toml index 68d29bd76..8eb26a92e 100644 --- a/lib/crates/fabro-telemetry/Cargo.toml +++ b/lib/crates/fabro-telemetry/Cargo.toml @@ -18,13 +18,13 @@ base64.workspace = true chrono.workspace = true dirs.workspace = true exec.workspace = true +fabro-http.workspace = true fabro-util = { path = "../fabro-util" } fork.workspace = true git2.workspace = true mac_address.workspace = true md5.workspace = true regex.workspace = true -reqwest = { workspace = true, features = ["blocking"] } sentry.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/lib/crates/fabro-telemetry/src/sender.rs b/lib/crates/fabro-telemetry/src/sender.rs index f1181561b..8f97db039 100644 --- a/lib/crates/fabro-telemetry/src/sender.rs +++ b/lib/crates/fabro-telemetry/src/sender.rs @@ -2,7 +2,6 @@ use std::path::Path; use base64::Engine; use base64::engine::general_purpose::STANDARD; -use reqwest::blocking::Client as BlockingClient; use uuid::Uuid; use crate::event::Track; @@ -102,7 +101,7 @@ pub fn upload_blocking(tracks: &[Track]) -> anyhow::Result<()> { let auth = STANDARD.encode(format!("{write_key}:")); - let resp = BlockingClient::new() + let resp = fabro_http::blocking_http_client()? .post(format!("{SEGMENT_BASE_URL}/v1/batch")) .header("Authorization", format!("Basic {auth}")) .json(&payload) @@ -130,7 +129,7 @@ pub async fn upload(path: &Path) -> anyhow::Result<()> { let auth = STANDARD.encode(format!("{write_key}:")); - let resp = reqwest::Client::new() + let resp = fabro_http::http_client()? .post(format!("{SEGMENT_BASE_URL}/v1/batch")) .header("Authorization", format!("Basic {auth}")) .json(&payload) diff --git a/lib/crates/fabro-test/Cargo.toml b/lib/crates/fabro-test/Cargo.toml index 6d648f3fe..3ba0f9d07 100644 --- a/lib/crates/fabro-test/Cargo.toml +++ b/lib/crates/fabro-test/Cargo.toml @@ -18,9 +18,9 @@ axum = { workspace = true } fabro-config = { path = "../fabro-config" } fabro-proc = { path = "../fabro-proc" } fabro-types = { path = "../fabro-types" } +fabro-http.workspace = true insta = { workspace = true, features = ["filters"] } regex = { workspace = true } -reqwest = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } tempfile = "3" diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 6d1ae6d70..59996712f 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -605,6 +605,7 @@ fn ensure_server_running(fabro_bin: &Path, server: &ServerPaths, config_path: &P let output = std::process::Command::new(fabro_bin) .env("NO_COLOR", "1") .env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled") .env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64") .env(TEST_IN_MEMORY_STORE_ENV, "1") .env("FABRO_HOME", &server.root) @@ -930,7 +931,8 @@ impl TestContext { } cmd.env("NO_COLOR", "1"); cmd.env("HOME", &self.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); cmd.env(TEST_IN_MEMORY_STORE_ENV, "1"); cmd @@ -1385,8 +1387,8 @@ pub struct TwinGitHub { server: twin_github::TestServer, } -pub fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() +pub fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } impl TwinGitHub { diff --git a/lib/crates/fabro-tracker/Cargo.toml b/lib/crates/fabro-tracker/Cargo.toml index 1587306d9..bb1c60e0c 100644 --- a/lib/crates/fabro-tracker/Cargo.toml +++ b/lib/crates/fabro-tracker/Cargo.toml @@ -15,11 +15,11 @@ workspace = true [dependencies] fabro-github = { path = "../fabro-github" } async-trait.workspace = true +fabro-http.workspace = true serde_json.workspace = true -reqwest.workspace = true tracing.workspace = true tokio = { workspace = true } [dev-dependencies] httpmock = "0.8" -tokio = { workspace = true, features = ["test-util", "macros"] } \ No newline at end of file +tokio = { workspace = true, features = ["test-util", "macros"] } diff --git a/lib/crates/fabro-tracker/src/github.rs b/lib/crates/fabro-tracker/src/github.rs index adba5995f..10aa4de9e 100644 --- a/lib/crates/fabro-tracker/src/github.rs +++ b/lib/crates/fabro-tracker/src/github.rs @@ -8,7 +8,7 @@ use crate::{Issue, Tracker, execute_graphql_request}; /// Execute a GitHub GraphQL request and return the response JSON. async fn execute_github_graphql( - client: &reqwest::Client, + client: &fabro_http::HttpClient, token: &str, endpoint: &str, query: &str, @@ -30,7 +30,7 @@ async fn execute_github_graphql( /// Scoped to a single project board identified by `project_number`. pub struct GitHubTracker { creds: GitHubCredentials, - client: reqwest::Client, + client: fabro_http::HttpClient, owner: String, repo: String, project_number: u64, @@ -41,7 +41,7 @@ pub struct GitHubTracker { impl GitHubTracker { pub fn new( creds: GitHubCredentials, - client: reqwest::Client, + client: fabro_http::HttpClient, owner: String, repo: String, project_number: u64, @@ -206,7 +206,7 @@ fn normalize_github_item(item: &serde_json::Value) -> Option { /// Fetch one page of project items. Returns (items, has_next_page, end_cursor). async fn fetch_project_items_page( - client: &reqwest::Client, + client: &fabro_http::HttpClient, token: &str, graphql_url: &str, project_node_id: &str, @@ -502,8 +502,8 @@ mod tests { use super::*; use crate::Issue; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } fn test_rsa_key() -> String { diff --git a/lib/crates/fabro-tracker/src/lib.rs b/lib/crates/fabro-tracker/src/lib.rs index f31846652..ca202caf2 100644 --- a/lib/crates/fabro-tracker/src/lib.rs +++ b/lib/crates/fabro-tracker/src/lib.rs @@ -12,7 +12,7 @@ pub use linear::{LINEAR_API_ENDPOINT, LinearOptions, LinearTracker}; /// as the `Authorization` value, and returns the parsed JSON response. /// Provider-specific error messages use `provider` as a label. pub(crate) async fn execute_graphql_request( - client: &reqwest::Client, + client: &fabro_http::HttpClient, endpoint: &str, auth_header: &str, provider: &str, diff --git a/lib/crates/fabro-tracker/src/linear.rs b/lib/crates/fabro-tracker/src/linear.rs index 6e11d5c11..cbe6fa0dc 100644 --- a/lib/crates/fabro-tracker/src/linear.rs +++ b/lib/crates/fabro-tracker/src/linear.rs @@ -128,7 +128,7 @@ fn normalize_issue(node: &Value) -> Result { } async fn execute_graphql( - client: &reqwest::Client, + client: &fabro_http::HttpClient, config: &LinearOptions, query: &str, variables: Value, @@ -155,12 +155,16 @@ fn extract_issues(response: &Value) -> Result, String> { /// A `Tracker` implementation backed by Linear. pub struct LinearTracker { config: LinearOptions, - client: reqwest::Client, + client: fabro_http::HttpClient, project_slug: String, } impl LinearTracker { - pub fn new(config: LinearOptions, client: reqwest::Client, project_slug: String) -> Self { + pub fn new( + config: LinearOptions, + client: fabro_http::HttpClient, + project_slug: String, + ) -> Self { Self { config, client, @@ -355,8 +359,8 @@ impl Tracker for LinearTracker { #[cfg(test)] mod tests { use super::*; - fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() + fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } fn mock_config(server_url: &str) -> LinearOptions { diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index f4cc27c9c..f0714b65c 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -37,6 +37,7 @@ fabro-retro = { path = "../fabro-retro" } fabro-core = { path = "../fabro-core" } fabro-store = { path = "../fabro-store" } fabro-types = { path = "../fabro-types" } +fabro-http.workspace = true thiserror.workspace = true serde.workspace = true serde_json.workspace = true @@ -61,7 +62,6 @@ git2.workspace = true tokio-util.workspace = true tracing.workspace = true walkdir.workspace = true -reqwest.workspace = true tempfile = "3" toml.workspace = true [dev-dependencies] diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 0562ecf94..32eece8e7 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -218,7 +218,7 @@ async fn mint_github_token( }; let jwt = fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) .map_err(|e| Error::engine(e.clone()))?; - let client = reqwest::Client::new(); + let client = fabro_http::http_client().map_err(|e| Error::engine(e.to_string()))?; let perms_json = serde_json::to_value(permissions).map_err(|e| Error::engine(e.to_string()))?; fabro_github::create_installation_access_token_with_permissions( &client, diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 0efbd7910..021df0198 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -1708,7 +1708,7 @@ async fn daytona_toolbox_idle_diagnostic() { // Diagnose with raw HTTP calls let api_key = std::env::var("DAYTONA_API_KEY").unwrap_or_default(); - let client = reqwest::Client::builder() + let client = fabro_http::HttpClientBuilder::new() .timeout(std::time::Duration::from_secs(15)) .build() .unwrap(); diff --git a/test/twin/github/Cargo.toml b/test/twin/github/Cargo.toml index bd7bcd2a7..92d32e19d 100644 --- a/test/twin/github/Cargo.toml +++ b/test/twin/github/Cargo.toml @@ -16,6 +16,7 @@ workspace = true axum = { workspace = true } base64 = { workspace = true } chrono = { workspace = true } +fabro-http.workspace = true jsonwebtoken = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } @@ -26,5 +27,4 @@ tracing-subscriber = { workspace = true } uuid = { workspace = true } [dev-dependencies] -reqwest = { workspace = true } tokio = { workspace = true, features = ["test-util", "macros"] } diff --git a/test/twin/github/src/handlers/branches.rs b/test/twin/github/src/handlers/branches.rs index 46b1aacc2..5eca78d4f 100644 --- a/test/twin/github/src/handlers/branches.rs +++ b/test/twin/github/src/handlers/branches.rs @@ -96,7 +96,7 @@ mod tests { use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( - client: &reqwest::Client, + client: &fabro_http::HttpClient, jwt: &str, owner: &str, repo: &str, diff --git a/test/twin/github/src/handlers/graphql.rs b/test/twin/github/src/handlers/graphql.rs index d4f4985d6..1794c4065 100644 --- a/test/twin/github/src/handlers/graphql.rs +++ b/test/twin/github/src/handlers/graphql.rs @@ -620,7 +620,7 @@ mod tests { use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( - client: &reqwest::Client, + client: &fabro_http::HttpClient, jwt: &str, owner: &str, repo: &str, @@ -663,7 +663,7 @@ mod tests { async fn setup_with_token( state: &mut AppState, pem: &str, - ) -> (TestServer, reqwest::Client, String) { + ) -> (TestServer, fabro_http::HttpClient, String) { state.register_app(AppOptions { app_id: "100".to_string(), slug: "test-app".to_string(), diff --git a/test/twin/github/src/handlers/pulls.rs b/test/twin/github/src/handlers/pulls.rs index 79041a121..5dcbcbb76 100644 --- a/test/twin/github/src/handlers/pulls.rs +++ b/test/twin/github/src/handlers/pulls.rs @@ -306,7 +306,7 @@ mod tests { use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( - client: &reqwest::Client, + client: &fabro_http::HttpClient, jwt: &str, owner: &str, repo: &str, @@ -342,7 +342,7 @@ mod tests { async fn setup_and_get_token( state: &mut AppState, pem: &str, - ) -> (TestServer, reqwest::Client, String) { + ) -> (TestServer, fabro_http::HttpClient, String) { state.register_app(AppOptions { app_id: "100".to_string(), slug: "test-app".to_string(), diff --git a/test/twin/github/src/test_support.rs b/test/twin/github/src/test_support.rs index 1fec7d85b..b25b36bbe 100644 --- a/test/twin/github/src/test_support.rs +++ b/test/twin/github/src/test_support.rs @@ -1,6 +1,6 @@ #[cfg(test)] -pub fn test_http_client() -> reqwest::Client { - reqwest::Client::builder().no_proxy().build().unwrap() +pub fn test_http_client() -> fabro_http::HttpClient { + fabro_http::test_http_client().unwrap() } #[cfg(test)] diff --git a/test/twin/openai/Cargo.toml b/test/twin/openai/Cargo.toml index 1e67bc939..d66836c33 100644 --- a/test/twin/openai/Cargo.toml +++ b/test/twin/openai/Cargo.toml @@ -16,9 +16,9 @@ workspace = true anyhow.workspace = true async-stream = "0.3" axum = { workspace = true, features = ["macros"] } +fabro-http.workspace = true futures-util.workspace = true http = "1" -reqwest.workspace = true serde.workspace = true serde_json.workspace = true tokio.workspace = true diff --git a/test/twin/openai/tests/common/mod.rs b/test/twin/openai/tests/common/mod.rs index f9dff9743..cca6a5898 100644 --- a/test/twin/openai/tests/common/mod.rs +++ b/test/twin/openai/tests/common/mod.rs @@ -6,9 +6,9 @@ use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant}; use anyhow::Result; +use fabro_http::{HttpClient as Client, HttpClientBuilder}; +use fabro_http::header::AUTHORIZATION; use futures_util::StreamExt; -use reqwest::Client; -use reqwest::header::AUTHORIZATION; use serde_json::Value; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpListener, TcpStream}; @@ -31,7 +31,7 @@ pub struct ApiClient { } pub struct RecordedResponse { - pub status: reqwest::StatusCode, + pub status: fabro_http::StatusCode, pub headers: HashMap, pub body: Vec, } @@ -43,7 +43,7 @@ pub struct RawStreamResponse { } pub struct TimedStreamResponse { - pub status: reqwest::StatusCode, + pub status: fabro_http::StatusCode, pub first_event_elapsed: Duration, pub chunks: Vec, } @@ -64,7 +64,7 @@ pub struct ParsedSseEvent { static NEXT_BEARER_TOKEN: AtomicU64 = AtomicU64::new(1); pub fn test_http_client() -> Result { - Client::builder().no_proxy().build().map_err(Into::into) + fabro_http::test_http_client().map_err(Into::into) } pub async fn spawn_server() -> Result { @@ -95,8 +95,8 @@ fn authorization_header_value(bearer_token: &str) -> String { } fn build_authenticated_client(bearer_token: &str) -> Result { - Client::builder() - .no_proxy() + HttpClientBuilder::new() + .proxy_policy(fabro_http::ProxyPolicy::Disabled) .default_headers( [( AUTHORIZATION, @@ -120,8 +120,8 @@ impl ApiClient { ) -> Result { Ok(Self { base_url: base_url.into(), - client: Client::builder() - .no_proxy() + client: HttpClientBuilder::new() + .proxy_policy(fabro_http::ProxyPolicy::Disabled) .timeout(Duration::from_secs(30)) .build()?, bearer_token, @@ -146,7 +146,7 @@ impl ApiClient { } } - pub async fn post_json(&self, path: &str, body: &Value) -> reqwest::Response { + pub async fn post_json(&self, path: &str, body: &Value) -> fabro_http::Response { self.post(path) .json(body) .send() @@ -168,15 +168,15 @@ impl ApiClient { .await } - pub fn post(&self, path: &str) -> reqwest::RequestBuilder { + pub fn post(&self, path: &str) -> fabro_http::RequestBuilder { self.request(self.client.post(format!("{}{}", self.base_url, path))) } - pub fn get(&self, path: &str) -> reqwest::RequestBuilder { + pub fn get(&self, path: &str) -> fabro_http::RequestBuilder { self.request(self.client.get(format!("{}{}", self.base_url, path))) } - fn request(&self, mut request: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + fn request(&self, mut request: fabro_http::RequestBuilder) -> fabro_http::RequestBuilder { if let Some(token) = &self.bearer_token { request = request.bearer_auth(token); } @@ -224,7 +224,7 @@ impl TestServer { } impl TestServer { - pub async fn post_responses(&self, body: Value) -> reqwest::Response { + pub async fn post_responses(&self, body: Value) -> fabro_http::Response { self.auth_client .post(format!("{}/v1/responses", self.base_url)) .json(&body) @@ -238,7 +238,7 @@ impl TestServer { body: Value, org: Option<&str>, project: Option<&str>, - ) -> reqwest::Response { + ) -> fabro_http::Response { let mut request = self .auth_client .post(format!("{}/v1/responses", self.base_url)); @@ -258,7 +258,10 @@ impl TestServer { .expect("request should complete") } - pub async fn post_responses_stream(&self, body: Value) -> (reqwest::StatusCode, Vec) { + pub async fn post_responses_stream( + &self, + body: Value, + ) -> (fabro_http::StatusCode, Vec) { let response = self .auth_client .post(format!("{}/v1/responses", self.base_url)) @@ -280,7 +283,7 @@ impl TestServer { (status, chunks) } - pub async fn post_chat(&self, body: Value) -> reqwest::Response { + pub async fn post_chat(&self, body: Value) -> fabro_http::Response { self.auth_client .post(format!("{}/v1/chat/completions", self.base_url)) .json(&body) @@ -289,7 +292,7 @@ impl TestServer { .expect("request should complete") } - pub async fn post_chat_stream(&self, body: Value) -> (reqwest::StatusCode, Vec) { + pub async fn post_chat_stream(&self, body: Value) -> (fabro_http::StatusCode, Vec) { let response = self.post_chat(body).await; let status = response.status(); let mut stream = response.bytes_stream(); @@ -308,7 +311,7 @@ impl TestServer { &self, body: Value, authorization: Option<&str>, - ) -> reqwest::Response { + ) -> fabro_http::Response { let mut request = self .client .post(format!("{}/v1/chat/completions", self.base_url)); @@ -491,7 +494,7 @@ fn parse_sse_block(block: &str) -> Result { }) } -pub async fn record_response(response: reqwest::Response) -> RecordedResponse { +pub async fn record_response(response: fabro_http::Response) -> RecordedResponse { let status = response.status(); let headers = response .headers() diff --git a/test/twin/openai/tests/failure_modes.rs b/test/twin/openai/tests/failure_modes.rs index c1b967e2e..0aa67b58d 100644 --- a/test/twin/openai/tests/failure_modes.rs +++ b/test/twin/openai/tests/failure_modes.rs @@ -2,7 +2,7 @@ mod common; use std::time::Duration; -use reqwest::header::AUTHORIZATION; +use fabro_http::header::AUTHORIZATION; use serde_json::json; #[tokio::test] @@ -106,7 +106,7 @@ async fn scripted_hang_times_out_client_side() { })) .await; - let client = reqwest::Client::builder() + let client = fabro_http::HttpClientBuilder::new() .no_proxy() .timeout(Duration::from_millis(150)) .default_headers( diff --git a/test/twin/openai/tests/live_openai_contract.rs b/test/twin/openai/tests/live_openai_contract.rs index 9de99d204..ac971d9ab 100644 --- a/test/twin/openai/tests/live_openai_contract.rs +++ b/test/twin/openai/tests/live_openai_contract.rs @@ -270,7 +270,7 @@ async fn probe_surface_availability( ) -> Result { let response = client.post_json_recorded(path, body).await; - if response.status == reqwest::StatusCode::OK { + if response.status == fabro_http::StatusCode::OK { return Ok(SurfaceAvailability::Available); } @@ -1312,7 +1312,7 @@ async fn enqueue_responses_continuation_scenarios(server: &common::TestServer, m async fn post_json_ok(client: &common::ApiClient, path: &str, body: &Value) -> Result { let response = client.post_json_recorded(path, body).await; ensure!( - response.status == reqwest::StatusCode::OK, + response.status == fabro_http::StatusCode::OK, "{} returned {}: {}", path, response.status, @@ -1328,7 +1328,7 @@ async fn post_sse_ok( ) -> Result { let response = client.post_json_recorded(path, body).await; ensure!( - response.status == reqwest::StatusCode::OK, + response.status == fabro_http::StatusCode::OK, "{} returned {}: {}", path, response.status, @@ -1362,14 +1362,14 @@ fn classify_live_access_blocker(response: &common::RecordedResponse) -> Option { Some("missing required OpenAI API scope or role".to_owned()) } - reqwest::StatusCode::TOO_MANY_REQUESTS if error_type == Some("insufficient_quota") => { + fabro_http::StatusCode::TOO_MANY_REQUESTS if error_type == Some("insufficient_quota") => { Some("account has insufficient quota for live API calls".to_owned()) } _ => None, @@ -1391,7 +1391,7 @@ fn ensure_matching_status( summarize_recorded_body(live) ); ensure!( - local.status == reqwest::StatusCode::OK, + local.status == fabro_http::StatusCode::OK, "{} returned non-OK responses: local={} body={} live={} body={}", label, local.status, diff --git a/test/twin/openai/tests/tool_and_schema_contract.rs b/test/twin/openai/tests/tool_and_schema_contract.rs index 81d15ff90..4839db86b 100644 --- a/test/twin/openai/tests/tool_and_schema_contract.rs +++ b/test/twin/openai/tests/tool_and_schema_contract.rs @@ -1,6 +1,6 @@ mod common; -use reqwest::header::AUTHORIZATION; +use fabro_http::header::AUTHORIZATION; use serde_json::json; #[tokio::test] From e8ad12fc3078d48877c54ff5f4d575f641c1a75c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 12 Apr 2026 12:20:55 -0400 Subject: [PATCH 2/3] fix: simplify fabro-http crate and fix correctness issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace unwrap_or_default() with expect() in hooks/llm HTTP client builders — Default silently discards all config (timeouts, TLS, proxy) - Route fabro-mcp through fabro_http instead of raw reqwest, respecting FABRO_HTTP_PROXY_POLICY for MCP HTTP transport connections - Deduplicate HttpClientBuilder / BlockingHttpClientBuilder via macro - Extract helpers for repeated http_client error handling in diagnostics and web_auth - Remove duplicate test_http_client() in fabro-cli and fabro-llm Co-Authored-By: Claude Opus 4.6 (1M context) --- Cargo.lock | 2 +- lib/crates/fabro-cli/src/commands/model.rs | 6 +- lib/crates/fabro-hooks/src/executor.rs | 6 +- lib/crates/fabro-http/src/lib.rs | 317 +++++++----------- .../fabro-llm/src/providers/fabro_server.rs | 40 ++- .../fabro-llm/src/providers/http_api.rs | 4 +- lib/crates/fabro-mcp/Cargo.toml | 2 +- lib/crates/fabro-mcp/src/client.rs | 8 +- lib/crates/fabro-server/src/diagnostics.rs | 49 ++- lib/crates/fabro-server/src/server.rs | 23 +- lib/crates/fabro-server/src/web_auth.rs | 30 +- test/twin/openai/tests/common/mod.rs | 2 +- 12 files changed, 203 insertions(+), 286 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 53092e2b8..11cd5bb62 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1786,9 +1786,9 @@ version = "0.176.2" dependencies = [ "anyhow", "fabro-config", + "fabro-http", "fabro-types", "futures", - "reqwest 0.13.2", "rmcp", "serde", "serde_json", diff --git a/lib/crates/fabro-cli/src/commands/model.rs b/lib/crates/fabro-cli/src/commands/model.rs index 7ba873053..f4b25506c 100644 --- a/lib/crates/fabro-cli/src/commands/model.rs +++ b/lib/crates/fabro-cli/src/commands/model.rs @@ -424,12 +424,8 @@ mod tests { use super::*; - fn test_http_client() -> fabro_http::HttpClient { - fabro_http::test_http_client().unwrap() - } - fn test_api_client(api_url: &str) -> fabro_api::Client { - fabro_api::Client::new_with_client(api_url, test_http_client()) + fabro_api::Client::new_with_client(api_url, fabro_test::test_http_client()) } fn test_model_json(id: &str, provider: Provider) -> serde_json::Value { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index c5722a143..b7cba714f 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -437,7 +437,7 @@ impl HookExecutorImpl { .await } - /// Build a reqwest client for the given TLS mode. + /// Build an HTTP client for the given TLS mode. fn build_http_client(tls: TlsMode) -> fabro_http::HttpClient { let accept_invalid = matches!(tls, TlsMode::NoVerify | TlsMode::Off); #[cfg(test)] @@ -446,14 +446,14 @@ impl HookExecutorImpl { .danger_accept_invalid_certs(accept_invalid) .no_proxy() .build() - .unwrap_or_default() + .expect("hook HTTP client should build") } #[cfg(not(test))] { fabro_http::HttpClientBuilder::new() .danger_accept_invalid_certs(accept_invalid) .build() - .unwrap_or_default() + .expect("hook HTTP client should build") } } diff --git a/lib/crates/fabro-http/src/lib.rs b/lib/crates/fabro-http/src/lib.rs index 1a560cbeb..f439b3814 100644 --- a/lib/crates/fabro-http/src/lib.rs +++ b/lib/crates/fabro-http/src/lib.rs @@ -60,113 +60,135 @@ pub enum HttpClientBuildError { Reqwest(#[from] reqwest::Error), } -#[derive(Default)] -pub struct HttpClientBuilder { - inner: reqwest::ClientBuilder, - proxy_policy: Option, +/// Generates a proxy-policy-aware builder that wraps a reqwest client builder. +macro_rules! define_builder { + ($builder_name:ident, $inner_builder:ty, $inner_new:expr, $client_type:ty) => { + #[derive(Default)] + pub struct $builder_name { + inner: $inner_builder, + proxy_policy: Option, + } + + impl $builder_name { + #[must_use] + pub fn new() -> Self { + Self { + inner: $inner_new, + proxy_policy: None, + } + } + + #[must_use] + pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { + self.proxy_policy = Some(proxy_policy); + self + } + + #[must_use] + pub fn no_proxy(mut self) -> Self { + self.inner = self.inner.no_proxy(); + self + } + + #[must_use] + pub fn proxy(mut self, proxy: Proxy) -> Self { + self.inner = self.inner.proxy(proxy); + self + } + + #[must_use] + pub fn user_agent(mut self, value: impl Into) -> Self { + self.inner = self.inner.user_agent(value.into()); + self + } + + #[must_use] + pub fn default_headers(mut self, headers: HeaderMap) -> Self { + self.inner = self.inner.default_headers(headers); + self + } + + #[must_use] + pub fn connect_timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.connect_timeout(timeout); + self + } + + #[must_use] + pub fn timeout(mut self, timeout: Duration) -> Self { + self.inner = self.inner.timeout(timeout); + self + } + + #[must_use] + pub fn use_rustls_tls(mut self) -> Self { + self.inner = self.inner.use_rustls_tls(); + self + } + + #[must_use] + pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { + self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); + self + } + + #[must_use] + pub fn add_root_certificate(mut self, cert: Certificate) -> Self { + self.inner = self.inner.add_root_certificate(cert); + self + } + + #[must_use] + pub fn identity(mut self, identity: Identity) -> Self { + self.inner = self.inner.identity(identity); + self + } + + #[cfg(unix)] + #[must_use] + pub fn unix_socket

(mut self, path: P) -> Self + where + P: AsRef, + { + self.inner = self.inner.unix_socket(path.as_ref()); + self + } + + pub fn build(self) -> Result<$client_type, HttpClientBuildError> { + let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; + let inner = match proxy_policy { + ProxyPolicy::System => self.inner, + ProxyPolicy::Disabled => self.inner.no_proxy(), + }; + inner.build().map_err(Into::into) + } + } + }; } +define_builder!( + HttpClientBuilder, + reqwest::ClientBuilder, + reqwest::Client::builder(), + HttpClient +); + +// `read_timeout` is only available on the async builder. impl HttpClientBuilder { - #[must_use] - pub fn new() -> Self { - Self { - inner: reqwest::Client::builder(), - proxy_policy: None, - } - } - - #[must_use] - pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { - self.proxy_policy = Some(proxy_policy); - self - } - - #[must_use] - pub fn no_proxy(mut self) -> Self { - self.inner = self.inner.no_proxy(); - self - } - - #[must_use] - pub fn proxy(mut self, proxy: Proxy) -> Self { - self.inner = self.inner.proxy(proxy); - self - } - - #[must_use] - pub fn user_agent(mut self, value: impl Into) -> Self { - self.inner = self.inner.user_agent(value.into()); - self - } - - #[must_use] - pub fn default_headers(mut self, headers: HeaderMap) -> Self { - self.inner = self.inner.default_headers(headers); - self - } - - #[must_use] - pub fn connect_timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.connect_timeout(timeout); - self - } - - #[must_use] - pub fn timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.timeout(timeout); - self - } - #[must_use] pub fn read_timeout(mut self, timeout: Duration) -> Self { self.inner = self.inner.read_timeout(timeout); self } - - #[must_use] - pub fn use_rustls_tls(mut self) -> Self { - self.inner = self.inner.use_rustls_tls(); - self - } - - #[must_use] - pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { - self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); - self - } - - #[must_use] - pub fn add_root_certificate(mut self, cert: Certificate) -> Self { - self.inner = self.inner.add_root_certificate(cert); - self - } - - #[must_use] - pub fn identity(mut self, identity: Identity) -> Self { - self.inner = self.inner.identity(identity); - self - } - - #[cfg(unix)] - #[must_use] - pub fn unix_socket

(mut self, path: P) -> Self - where - P: AsRef, - { - self.inner = self.inner.unix_socket(path.as_ref()); - self - } - - pub fn build(self) -> Result { - let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; - let inner = match proxy_policy { - ProxyPolicy::System => self.inner, - ProxyPolicy::Disabled => self.inner.no_proxy(), - }; - inner.build().map_err(Into::into) - } } +define_builder!( + BlockingHttpClientBuilder, + reqwest::blocking::ClientBuilder, + reqwest::blocking::Client::builder(), + BlockingHttpClient +); + pub fn http_client() -> Result { HttpClientBuilder::new().build() } @@ -177,107 +199,6 @@ pub fn test_http_client() -> Result { .build() } -#[derive(Default)] -pub struct BlockingHttpClientBuilder { - inner: reqwest::blocking::ClientBuilder, - proxy_policy: Option, -} - -impl BlockingHttpClientBuilder { - #[must_use] - pub fn new() -> Self { - Self { - inner: reqwest::blocking::Client::builder(), - proxy_policy: None, - } - } - - #[must_use] - pub fn proxy_policy(mut self, proxy_policy: ProxyPolicy) -> Self { - self.proxy_policy = Some(proxy_policy); - self - } - - #[must_use] - pub fn no_proxy(mut self) -> Self { - self.inner = self.inner.no_proxy(); - self - } - - #[must_use] - pub fn proxy(mut self, proxy: Proxy) -> Self { - self.inner = self.inner.proxy(proxy); - self - } - - #[must_use] - pub fn user_agent(mut self, value: impl Into) -> Self { - self.inner = self.inner.user_agent(value.into()); - self - } - - #[must_use] - pub fn default_headers(mut self, headers: HeaderMap) -> Self { - self.inner = self.inner.default_headers(headers); - self - } - - #[must_use] - pub fn connect_timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.connect_timeout(timeout); - self - } - - #[must_use] - pub fn timeout(mut self, timeout: Duration) -> Self { - self.inner = self.inner.timeout(timeout); - self - } - - #[must_use] - pub fn use_rustls_tls(mut self) -> Self { - self.inner = self.inner.use_rustls_tls(); - self - } - - #[must_use] - pub fn danger_accept_invalid_certs(mut self, accept_invalid_certs: bool) -> Self { - self.inner = self.inner.danger_accept_invalid_certs(accept_invalid_certs); - self - } - - #[must_use] - pub fn add_root_certificate(mut self, cert: Certificate) -> Self { - self.inner = self.inner.add_root_certificate(cert); - self - } - - #[must_use] - pub fn identity(mut self, identity: Identity) -> Self { - self.inner = self.inner.identity(identity); - self - } - - #[cfg(unix)] - #[must_use] - pub fn unix_socket

(mut self, path: P) -> Self - where - P: AsRef, - { - self.inner = self.inner.unix_socket(path.as_ref()); - self - } - - pub fn build(self) -> Result { - let proxy_policy = ProxyPolicy::resolve(self.proxy_policy)?; - let inner = match proxy_policy { - ProxyPolicy::System => self.inner, - ProxyPolicy::Disabled => self.inner.no_proxy(), - }; - inner.build().map_err(Into::into) - } -} - pub fn blocking_http_client() -> Result { BlockingHttpClientBuilder::new().build() } diff --git a/lib/crates/fabro-llm/src/providers/fabro_server.rs b/lib/crates/fabro-llm/src/providers/fabro_server.rs index e7f5bcb51..da9c643e8 100644 --- a/lib/crates/fabro-llm/src/providers/fabro_server.rs +++ b/lib/crates/fabro-llm/src/providers/fabro_server.rs @@ -228,10 +228,6 @@ mod tests { use crate::error::ProviderErrorKind; use crate::types::Message; - fn test_http_client() -> fabro_http::HttpClient { - fabro_http::test_http_client().unwrap() - } - fn make_request() -> Request { Request { model: "test-model".to_string(), @@ -273,7 +269,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ .body(sse_body); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let mut stream = adapter.stream(&make_request()).await.unwrap(); @@ -326,7 +326,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ .json_body(response_json); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let response = adapter.complete(&make_request()).await.unwrap(); @@ -349,7 +353,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ then.status(502).body("Bad Gateway"); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let err = adapter.complete(&make_request()).await.unwrap_err(); match &err { @@ -370,7 +378,11 @@ data: {\"type\":\"text_delta\",\"delta\":\" world\",\"text_id\":null}\n\ then.status(502).body("Bad Gateway"); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let result = adapter.stream(&make_request()).await; let Err(err) = result else { @@ -404,7 +416,11 @@ data: {\"type\":\"stream_start\"}\n\ .body(sse_body); }); - let adapter = Adapter::new(test_http_client(), server.base_url(), "test-provider"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + server.base_url(), + "test-provider", + ); let mut stream = adapter.stream(&make_request()).await.unwrap(); @@ -450,7 +466,11 @@ data: {\"type\":\"stream_start\"}\n\ #[test] fn adapter_name() { - let adapter = Adapter::new(test_http_client(), "http://localhost", "anthropic"); + let adapter = Adapter::new( + fabro_test::test_http_client(), + "http://localhost", + "anthropic", + ); assert_eq!(adapter.name(), "anthropic"); } } diff --git a/lib/crates/fabro-llm/src/providers/http_api.rs b/lib/crates/fabro-llm/src/providers/http_api.rs index 7c831dd6e..809f605c5 100644 --- a/lib/crates/fabro-llm/src/providers/http_api.rs +++ b/lib/crates/fabro-llm/src/providers/http_api.rs @@ -25,14 +25,14 @@ impl HttpApi { .connect_timeout(Duration::from_secs_f64(timeout.connect)) .no_proxy() .build() - .unwrap_or_default() + .expect("LLM HTTP client should build") } #[cfg(not(test))] { fabro_http::HttpClientBuilder::new() .connect_timeout(Duration::from_secs_f64(timeout.connect)) .build() - .unwrap_or_default() + .expect("LLM HTTP client should build") } } diff --git a/lib/crates/fabro-mcp/Cargo.toml b/lib/crates/fabro-mcp/Cargo.toml index 5ce17655a..03bd6455a 100644 --- a/lib/crates/fabro-mcp/Cargo.toml +++ b/lib/crates/fabro-mcp/Cargo.toml @@ -15,12 +15,12 @@ workspace = true [dependencies] anyhow.workspace = true fabro-config = { path = "../fabro-config" } +fabro-http.workspace = true fabro-types = { path = "../fabro-types" } serde.workspace = true serde_json.workspace = true tokio.workspace = true futures.workspace = true -reqwest.workspace = true tracing.workspace = true rmcp = { workspace = true, features = [ "client", diff --git a/lib/crates/fabro-mcp/src/client.rs b/lib/crates/fabro-mcp/src/client.rs index b282659a7..1c2cfde0f 100644 --- a/lib/crates/fabro-mcp/src/client.rs +++ b/lib/crates/fabro-mcp/src/client.rs @@ -1,11 +1,9 @@ -#![allow(clippy::disallowed_methods, clippy::disallowed_types)] - use std::process::Stdio; use std::sync::Arc; use std::time::Duration; use anyhow::{Result, anyhow}; -use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; +use fabro_http::{HeaderMap, HeaderName, HeaderValue}; use rmcp::model::{CallToolRequestParams, CallToolResult}; use rmcp::service::{RoleClient, RunningService, serve_client}; use rmcp::transport::StreamableHttpClientTransport; @@ -28,7 +26,7 @@ enum ClientState { enum PendingTransport { Stdio(TokioChildProcess), - Http(StreamableHttpClientTransport), + Http(StreamableHttpClientTransport), } /// MCP client wrapping the rmcp SDK. Handles stdio and HTTP transports. @@ -68,7 +66,7 @@ impl McpClient { McpTransport::Http { url, headers } => { let http_config = StreamableHttpClientTransportConfig::with_uri(url.clone()); - let mut builder = reqwest::Client::builder(); + let mut builder = fabro_http::HttpClientBuilder::new(); if !headers.is_empty() { let mut header_map = HeaderMap::new(); for (key, value) in headers { diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index abcc9f0c1..649f8a5d5 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -19,6 +19,19 @@ use tokio::time::timeout; use crate::server::AppState; +fn http_client_or_check( + name: &str, + status: CheckStatus, +) -> Result { + fabro_http::http_client().map_err(|err| CheckResult { + name: name.to_string(), + status, + summary: "client error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some(err.to_string()), + }) +} + #[derive(Debug, Serialize)] pub struct DiagnosticsReport { pub version: String, @@ -321,17 +334,9 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("GitHub CLI", CheckStatus::Error) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "GitHub CLI".to_string(), - status: CheckStatus::Error, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let probe = timeout( Duration::from_secs(15), @@ -472,17 +477,9 @@ async fn check_github_app(state: &AppState) -> CheckResult { } }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("GitHub App", CheckStatus::Error) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "GitHub App".to_string(), - status: CheckStatus::Error, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let auth_result = timeout( Duration::from_secs(15), @@ -545,17 +542,9 @@ async fn check_brave_search(state: &AppState) -> CheckResult { }; }; - let http = match fabro_http::http_client() { + let http = match http_client_or_check("Brave Search", CheckStatus::Warning) { Ok(http) => http, - Err(err) => { - return CheckResult { - name: "Brave Search".to_string(), - status: CheckStatus::Warning, - summary: "client error".to_string(), - details: vec![CheckDetail::new(err.to_string())], - remediation: Some(err.to_string()), - }; - } + Err(result) => return result, }; let probe = timeout(Duration::from_secs(15), async move { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index cf148cc63..08aee15b7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1764,20 +1764,15 @@ async fn get_github_repo( }); } let client_ref = client.as_ref().expect("client initialized above"); - let installed = match fabro_github::check_app_installed( - client_ref, - &jwt, - &owner, - &name, - &base_url, - ) - .await - { - Ok(installed) => installed, - Err(err) => { - return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); - } - }; + let installed = + match fabro_github::check_app_installed(client_ref, &jwt, &owner, &name, &base_url) + .await + { + Ok(installed) => installed, + Err(err) => { + return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); + } + }; if !installed { return ( diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index cafb99e38..470b073a8 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -15,6 +15,16 @@ use tracing::{debug, error, info, warn}; use crate::server::AppState; +fn github_http_client(context: &str) -> Result { + fabro_http::http_client().map_err(|err| { + error!(error = %err, "{context}: could not build GitHub HTTP client"); + json_response( + StatusCode::SERVICE_UNAVAILABLE, + json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), + ) + }) +} + pub const SESSION_COOKIE_NAME: &str = "__fabro_session"; const OAUTH_STATE_COOKIE_NAME: &str = "fabro_oauth_state"; @@ -286,15 +296,9 @@ async fn callback_github( } }; - let http = match fabro_http::http_client() { + let http = match github_http_client("OAuth callback failed") { Ok(http) => http, - Err(err) => { - error!(error = %err, "OAuth callback failed: could not build GitHub HTTP client"); - return json_response( - StatusCode::SERVICE_UNAVAILABLE, - json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), - ); - } + Err(response) => return response, }; let token = match http .post("https://github.com/login/oauth/access_token") @@ -534,15 +538,9 @@ async fn setup_register( .and_then(|s| fabro_http::Url::parse(s).ok()) .map(|url| format!("{}://{}", url.scheme(), url.authority())); - let http = match fabro_http::http_client() { + let http = match github_http_client("Setup register failed") { Ok(http) => http, - Err(err) => { - error!(error = %err, "Setup register failed: could not build GitHub HTTP client"); - return json_response( - StatusCode::SERVICE_UNAVAILABLE, - json!({"error": format!("Failed to build GitHub HTTP client: {err}")}), - ); - } + Err(response) => return response, }; let response = match http .post(format!( diff --git a/test/twin/openai/tests/common/mod.rs b/test/twin/openai/tests/common/mod.rs index cca6a5898..af1f7c93e 100644 --- a/test/twin/openai/tests/common/mod.rs +++ b/test/twin/openai/tests/common/mod.rs @@ -6,8 +6,8 @@ use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant}; use anyhow::Result; -use fabro_http::{HttpClient as Client, HttpClientBuilder}; use fabro_http::header::AUTHORIZATION; +use fabro_http::{HttpClient as Client, HttpClientBuilder}; use futures_util::StreamExt; use serde_json::Value; use tokio::io::{AsyncReadExt, AsyncWriteExt}; From cb9a82b8d0f3b8ecc516d34eab1e440427026627 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 12 Apr 2026 11:13:47 -0400 Subject: [PATCH 3/3] feat: add typed secrets metadata and API --- docs/api-reference/fabro-api.yaml | 69 ++-- lib/crates/fabro-cli/src/args.rs | 14 +- lib/crates/fabro-cli/src/commands/install.rs | 16 +- .../fabro-cli/src/commands/provider/login.rs | 10 +- .../fabro-cli/src/commands/secret/list.rs | 8 +- .../fabro-cli/src/commands/secret/rm.rs | 8 +- .../fabro-cli/src/commands/secret/set.rs | 19 +- lib/crates/fabro-cli/tests/it/cmd/secret.rs | 36 +- .../fabro-cli/tests/it/cmd/secret_list.rs | 1 + .../fabro-cli/tests/it/cmd/secret_set.rs | 14 +- lib/crates/fabro-server/src/demo/mod.rs | 31 +- lib/crates/fabro-server/src/secret_store.rs | 309 ++++++++++++++++-- lib/crates/fabro-server/src/server.rs | 136 ++++++-- lib/crates/fabro-server/src/web_auth.rs | 3 +- .../src/.openapi-generator/FILES | 4 +- .../fabro-api-client/src/api/secrets-api.ts | 206 ++++++------ .../src/models/create-secret-request.ts | 40 +++ .../src/models/delete-secret-request.ts | 26 ++ .../fabro-api-client/src/models/index.ts | 4 +- .../src/models/secret-metadata.ts | 12 +- .../src/models/secret-type.ts | 29 ++ 21 files changed, 771 insertions(+), 224 deletions(-) create mode 100644 lib/packages/fabro-api-client/src/models/create-secret-request.ts create mode 100644 lib/packages/fabro-api-client/src/models/delete-secret-request.ts create mode 100644 lib/packages/fabro-api-client/src/models/secret-type.ts diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml index f523b75a0..a635bbc55 100644 --- a/docs/api-reference/fabro-api.yaml +++ b/docs/api-reference/fabro-api.yaml @@ -1327,24 +1327,16 @@ paths: application/json: schema: $ref: "#/components/schemas/SecretListResponse" - - /api/v1/secrets/{name}: - put: - operationId: setSecret + post: + operationId: createSecret tags: [Secrets] summary: Store or update a secret - parameters: - - name: name - in: path - required: true - schema: - type: string requestBody: required: true content: application/json: schema: - $ref: "#/components/schemas/SetSecretRequest" + $ref: "#/components/schemas/CreateSecretRequest" responses: "200": description: Secret stored @@ -1359,20 +1351,20 @@ paths: schema: $ref: "#/components/schemas/ErrorResponse" delete: - operationId: deleteSecret + operationId: deleteSecretByName tags: [Secrets] summary: Delete a stored secret - parameters: - - name: name - in: path - required: true - schema: - type: string + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/DeleteSecretRequest" responses: "204": description: Secret deleted "400": - description: Invalid secret name + description: Invalid secret name or request body content: application/json: schema: @@ -4329,28 +4321,61 @@ components: description: Server version string. example: "0.176.2" - SetSecretRequest: - description: Request to store a secret value. + SecretType: + description: The way a secret is consumed by the sandbox. + type: string + enum: + - environment + - file + + CreateSecretRequest: + description: Request to store or update a secret. type: object required: + - name - value + - type properties: + name: + type: string + description: Secret name or destination path for file secrets. value: type: string description: The secret value to store. + type: + $ref: "#/components/schemas/SecretType" + description: + type: string + description: Optional operator-facing description of the secret. + + DeleteSecretRequest: + description: Request to delete a secret by name. + type: object + required: + - name + properties: + name: + type: string + description: Secret name or destination path for file secrets. SecretMetadata: description: Metadata for a stored secret (value is never exposed). type: object required: - name + - type - created_at - updated_at properties: name: type: string - description: Secret key name. + description: Secret key name or destination path. example: ANTHROPIC_API_KEY + type: + $ref: "#/components/schemas/SecretType" + description: + type: string + description: Optional operator-facing description of the secret. created_at: type: string format: date-time diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 216d0e532..e2b348c23 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -489,12 +489,22 @@ pub(crate) struct SecretRmArgs { pub(crate) key: String, } +#[derive(Clone, Copy, Debug, ValueEnum)] +pub(crate) enum SecretTypeArg { + Environment, + File, +} + #[derive(Args)] pub(crate) struct SecretSetArgs { /// Name of the secret - pub(crate) key: String, + pub(crate) key: String, /// Value to store - pub(crate) value: String, + pub(crate) value: String, + #[arg(long, value_enum, default_value = "environment")] + pub(crate) r#type: SecretTypeArg, + #[arg(long)] + pub(crate) description: Option, } #[derive(Debug, Args)] diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index e298a4449..4a0b13fb0 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -12,11 +12,11 @@ use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use dialoguer::console::Term; use dialoguer::theme::ColorfulTheme; use dialoguer::{MultiSelect, Select}; -use fabro_api::types::SetSecretRequest; +use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; use fabro_config::user::SETTINGS_CONFIG_FILENAME; use fabro_config::{Storage, legacy_env}; use fabro_model::Provider; -use fabro_server::secret_store::SecretStore; +use fabro_server::secret_store::{SecretStore, SecretType}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; use rand::Rng; @@ -687,10 +687,12 @@ async fn persist_install_secrets( let client = server_client::connect_api_client(storage_dir).await?; for (name, value) in secrets { client - .set_secret() - .name(name.clone()) - .body(SetSecretRequest { - value: value.clone(), + .create_secret() + .body(CreateSecretRequest { + name: name.clone(), + value: value.clone(), + type_: ApiSecretType::Environment, + description: None, }) .send() .await?; @@ -700,7 +702,7 @@ async fn persist_install_secrets( let mut store = SecretStore::load(Storage::new(storage_dir).secrets_path())?; for (name, value) in secrets { - store.set(name, value)?; + store.set(name, value, SecretType::Environment, None)?; } Ok(()) } diff --git a/lib/crates/fabro-cli/src/commands/provider/login.rs b/lib/crates/fabro-cli/src/commands/provider/login.rs index f0ca944ae..49189bd94 100644 --- a/lib/crates/fabro-cli/src/commands/provider/login.rs +++ b/lib/crates/fabro-cli/src/commands/provider/login.rs @@ -46,9 +46,13 @@ pub(super) async fn login_command( for (name, value) in env_pairs { server .api() - .set_secret() - .name(name.clone()) - .body(types::SetSecretRequest { value }) + .create_secret() + .body(types::CreateSecretRequest { + name: name.clone(), + value, + type_: types::SecretType::Environment, + description: None, + }) .send() .await?; fabro_util::printerr!(printer, " {} Saved {}", s.green.apply_to("✔"), name); diff --git a/lib/crates/fabro-cli/src/commands/secret/list.rs b/lib/crates/fabro-cli/src/commands/secret/list.rs index 88a1d63ae..6fe367089 100644 --- a/lib/crates/fabro-cli/src/commands/secret/list.rs +++ b/lib/crates/fabro-cli/src/commands/secret/list.rs @@ -24,7 +24,13 @@ pub(super) async fn list_command( } let _ = args; for secret in secrets { - fabro_util::printout!(printer, "{}\t{}", secret.name, secret.updated_at); + fabro_util::printout!( + printer, + "{}\t{}\t{}", + secret.name, + secret.type_, + secret.updated_at + ); } Ok(()) } diff --git a/lib/crates/fabro-cli/src/commands/secret/rm.rs b/lib/crates/fabro-cli/src/commands/secret/rm.rs index 742a2e949..62a97fe73 100644 --- a/lib/crates/fabro-cli/src/commands/secret/rm.rs +++ b/lib/crates/fabro-cli/src/commands/secret/rm.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use fabro_api::Client; +use fabro_api::{Client, types}; use fabro_util::printer::Printer; use crate::args::{GlobalArgs, SecretRmArgs}; @@ -13,8 +13,10 @@ pub(super) async fn rm_command( printer: Printer, ) -> Result<()> { client - .delete_secret() - .name(args.key.clone()) + .delete_secret_by_name() + .body(types::DeleteSecretRequest { + name: args.key.clone(), + }) .send() .await .map_err(server_client::map_api_error)?; diff --git a/lib/crates/fabro-cli/src/commands/secret/set.rs b/lib/crates/fabro-cli/src/commands/secret/set.rs index bd9ec5d5a..c70b8ad2b 100644 --- a/lib/crates/fabro-cli/src/commands/secret/set.rs +++ b/lib/crates/fabro-cli/src/commands/secret/set.rs @@ -2,10 +2,17 @@ use anyhow::Result; use fabro_api::{Client, types}; use fabro_util::printer::Printer; -use crate::args::{GlobalArgs, SecretSetArgs}; +use crate::args::{GlobalArgs, SecretSetArgs, SecretTypeArg}; use crate::server_client; use crate::shared::print_json_pretty; +fn api_secret_type(secret_type: SecretTypeArg) -> types::SecretType { + match secret_type { + SecretTypeArg::Environment => types::SecretType::Environment, + SecretTypeArg::File => types::SecretType::File, + } +} + pub(super) async fn set_command( client: &Client, args: &SecretSetArgs, @@ -13,10 +20,12 @@ pub(super) async fn set_command( printer: Printer, ) -> Result<()> { let meta = client - .set_secret() - .name(args.key.clone()) - .body(types::SetSecretRequest { - value: args.value.clone(), + .create_secret() + .body(types::CreateSecretRequest { + name: args.key.clone(), + value: args.value.clone(), + type_: api_secret_type(args.r#type), + description: args.description.clone(), }) .send() .await diff --git a/lib/crates/fabro-cli/tests/it/cmd/secret.rs b/lib/crates/fabro-cli/tests/it/cmd/secret.rs index b18b19f0c..db64fb1ad 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/secret.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/secret.rs @@ -47,7 +47,7 @@ fn test_secret_lifecycle() { // 2. list -> contains FOO secret(&["list"]) .success() - .stdout(predicates::str::contains("FOO")); + .stdout(predicates::str::contains("FOO\tenvironment")); // 3. update FOO secret(&["set", "FOO", "updated"]).success(); @@ -90,7 +90,7 @@ fn test_secret_list_alias_ls() { .args(["ls"]) .assert() .success() - .stdout(predicates::str::contains("X")); + .stdout(predicates::str::contains("X\tenvironment")); } #[test] @@ -122,6 +122,36 @@ fn test_secret_value_with_equals() { .args(["list"]) .assert() .success() - .stdout(predicates::str::contains("URL")) + .stdout(predicates::str::contains("URL\tenvironment")) .stdout(predicates::str::contains("https://x.com?a=1&b=2").not()); } + +#[test] +fn test_file_secret_lifecycle() { + let context = test_context!(); + + let secret = + |args: &[&str]| -> assert_cmd::assert::Assert { context.secret().args(args).assert() }; + + secret(&[ + "set", + "/tmp/test.pem", + "pem-data", + "--type", + "file", + "--description", + "Test certificate", + ]) + .success(); + + secret(&["list"]) + .success() + .stdout(predicates::str::contains("/tmp/test.pem\tfile")) + .stdout(predicates::str::contains("pem-data").not()); + + secret(&["rm", "/tmp/test.pem"]).success(); + + secret(&["list"]) + .success() + .stdout(predicates::str::contains("/tmp/test.pem").not()); +} diff --git a/lib/crates/fabro-cli/tests/it/cmd/secret_list.rs b/lib/crates/fabro-cli/tests/it/cmd/secret_list.rs index 27dc86c5a..531ca77e3 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/secret_list.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/secret_list.rs @@ -47,6 +47,7 @@ fn secret_list_json_returns_metadata_only() { .iter() .find(|entry| entry["name"] == "ANTHROPIC_API_KEY") .expect("secret list should include the saved key"); + assert_eq!(entry["type"], "environment"); assert!(entry.get("updated_at").is_some()); assert!(entry.get("value").is_none()); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/secret_set.rs b/lib/crates/fabro-cli/tests/it/cmd/secret_set.rs index 3d1a49c6a..cc71d2691 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/secret_set.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/secret_set.rs @@ -18,12 +18,14 @@ fn help() { Value to store Options: - --json Output as JSON [env: FABRO_JSON=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --type [default: environment] [possible values: environment, file] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --description + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index 92e5d7ebe..48f5d77e7 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -11,7 +11,7 @@ use axum::extract::{Path, Query, State}; use axum::http::StatusCode; use axum::response::sse::{Event, Sse}; use axum::response::{IntoResponse, Response}; -use fabro_api::types::RunArtifactListResponse; +use fabro_api::types::{CreateSecretRequest, DeleteSecretRequest, RunArtifactListResponse}; use serde_json::json; use crate::error::ApiError; @@ -316,11 +316,13 @@ pub(crate) async fn list_secrets( "data": [ { "name": "OPENAI_API_KEY", + "type": "environment", "created_at": "2026-04-05T12:00:00Z", "updated_at": "2026-04-05T12:00:00Z" }, { "name": "GITHUB_APP_PRIVATE_KEY", + "type": "environment", "created_at": "2026-04-05T12:05:00Z", "updated_at": "2026-04-05T12:05:00Z" } @@ -330,26 +332,27 @@ pub(crate) async fn list_secrets( .into_response() } -pub(crate) async fn set_secret( +pub(crate) async fn create_secret( _auth: AuthenticatedService, State(_state): State>, - Path(name): Path, + Json(body): Json, ) -> Response { - ( - StatusCode::OK, - Json(json!({ - "name": name, - "created_at": "2026-04-05T12:00:00Z", - "updated_at": "2026-04-05T12:00:00Z" - })), - ) - .into_response() + let mut payload = serde_json::Map::new(); + payload.insert("name".to_string(), json!(body.name)); + payload.insert("type".to_string(), json!(body.type_)); + if let Some(description) = body.description { + payload.insert("description".to_string(), json!(description)); + } + payload.insert("created_at".to_string(), json!("2026-04-05T12:00:00Z")); + payload.insert("updated_at".to_string(), json!("2026-04-05T12:00:00Z")); + + (StatusCode::OK, Json(serde_json::Value::Object(payload))).into_response() } -pub(crate) async fn delete_secret( +pub(crate) async fn delete_secret_by_name( _auth: AuthenticatedService, State(_state): State>, - Path(_name): Path, + Json(_body): Json, ) -> Response { StatusCode::NO_CONTENT.into_response() } diff --git a/lib/crates/fabro-server/src/secret_store.rs b/lib/crates/fabro-server/src/secret_store.rs index d87c53014..b599dac77 100644 --- a/lib/crates/fabro-server/src/secret_store.rs +++ b/lib/crates/fabro-server/src/secret_store.rs @@ -1,19 +1,35 @@ use std::collections::HashMap; use std::fmt; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum SecretType { + #[default] + Environment, + File, +} #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct SecretEntry { - pub value: String, - pub created_at: String, - pub updated_at: String, + pub value: String, + #[serde(rename = "type", default)] + pub secret_type: SecretType, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub description: Option, + pub created_at: String, + pub updated_at: String, } #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct SecretMetadata { - pub name: String, - pub created_at: String, - pub updated_at: String, + pub name: String, + #[serde(rename = "type")] + pub secret_type: SecretType, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub description: Option, + pub created_at: String, + pub updated_at: String, } #[derive(Debug)] @@ -66,16 +82,31 @@ impl SecretStore { Ok(Self { path, entries }) } - pub fn set(&mut self, name: &str, value: &str) -> Result { - Self::validate_name(name)?; + pub fn set( + &mut self, + name: &str, + value: &str, + secret_type: SecretType, + description: Option<&str>, + ) -> Result { + Self::validate_name(name, secret_type)?; let now = chrono::Utc::now().to_rfc3339(); - let created_at = self - .entries - .get(name) - .map_or_else(|| now.clone(), |entry| entry.created_at.clone()); + let (created_at, description) = self.entries.get(name).map_or_else( + || (now.clone(), description.map(str::to_string)), + |entry| { + ( + entry.created_at.clone(), + description + .map(str::to_string) + .or_else(|| entry.description.clone()), + ) + }, + ); let entry = SecretEntry { - value: value.to_string(), + value: value.to_string(), + secret_type, + description: description.clone(), created_at: created_at.clone(), updated_at: now.clone(), }; @@ -84,13 +115,14 @@ impl SecretStore { Ok(SecretMetadata { name: name.to_string(), + secret_type, + description, created_at, updated_at: now, }) } pub fn remove(&mut self, name: &str) -> Result<(), SecretStoreError> { - Self::validate_name(name)?; if self.entries.remove(name).is_none() { return Err(SecretStoreError::NotFound(name.to_string())); } @@ -103,9 +135,11 @@ impl SecretStore { .entries .iter() .map(|(name, entry)| SecretMetadata { - name: name.clone(), - created_at: entry.created_at.clone(), - updated_at: entry.updated_at.clone(), + name: name.clone(), + secret_type: entry.secret_type, + description: entry.description.clone(), + created_at: entry.created_at.clone(), + updated_at: entry.updated_at.clone(), }) .collect::>(); data.sort_by(|a, b| a.name.cmp(&b.name)); @@ -119,11 +153,30 @@ impl SecretStore { pub fn snapshot(&self) -> HashMap { self.entries .iter() + .filter(|(_, entry)| entry.secret_type == SecretType::Environment) .map(|(name, entry)| (name.clone(), entry.value.clone())) .collect() } - pub fn validate_name(name: &str) -> Result<(), SecretStoreError> { + pub fn file_secrets(&self) -> Vec<(String, String)> { + let mut data = self + .entries + .iter() + .filter(|(_, entry)| entry.secret_type == SecretType::File) + .map(|(name, entry)| (name.clone(), entry.value.clone())) + .collect::>(); + data.sort_by(|a, b| a.0.cmp(&b.0)); + data + } + + pub fn validate_name(name: &str, secret_type: SecretType) -> Result<(), SecretStoreError> { + match secret_type { + SecretType::Environment => Self::validate_env_name(name), + SecretType::File => Self::validate_file_name(name), + } + } + + fn validate_env_name(name: &str) -> Result<(), SecretStoreError> { let mut chars = name.chars(); match chars.next() { Some(first) if first.is_ascii_alphabetic() || first == '_' => {} @@ -137,6 +190,26 @@ impl SecretStore { } } + fn validate_file_name(name: &str) -> Result<(), SecretStoreError> { + if !name.starts_with('/') || name.ends_with('/') || name.contains('\0') { + return Err(SecretStoreError::InvalidName(name.to_string())); + } + + let path = Path::new(name); + if !path.is_absolute() { + return Err(SecretStoreError::InvalidName(name.to_string())); + } + + if path + .components() + .any(|component| matches!(component, Component::ParentDir)) + { + return Err(SecretStoreError::InvalidName(name.to_string())); + } + + Ok(()) + } + fn write_atomic(&self) -> Result<(), SecretStoreError> { let parent = self .path @@ -188,9 +261,13 @@ mod tests { let path = dir.path().join("secrets.json"); let mut store = SecretStore::load(path.clone()).unwrap(); - let meta = store.set("OPENAI_API_KEY", "secret").unwrap(); + let meta = store + .set("OPENAI_API_KEY", "secret", SecretType::Environment, None) + .unwrap(); assert_eq!(meta.name, "OPENAI_API_KEY"); + assert_eq!(meta.secret_type, SecretType::Environment); + assert_eq!(meta.description, None); assert_eq!(store.get("OPENAI_API_KEY"), Some("secret")); assert!(path.exists()); } @@ -201,8 +278,12 @@ mod tests { let path = dir.path().join("secrets.json"); let mut store = SecretStore::load(path).unwrap(); - let first = store.set("OPENAI_API_KEY", "first").unwrap(); - let second = store.set("OPENAI_API_KEY", "second").unwrap(); + let first = store + .set("OPENAI_API_KEY", "first", SecretType::Environment, None) + .unwrap(); + let second = store + .set("OPENAI_API_KEY", "second", SecretType::Environment, None) + .unwrap(); assert_eq!(first.created_at, second.created_at); assert_eq!(store.get("OPENAI_API_KEY"), Some("second")); @@ -213,7 +294,9 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("secrets.json"); let mut store = SecretStore::load(path.clone()).unwrap(); - store.set("OPENAI_API_KEY", "secret").unwrap(); + store + .set("OPENAI_API_KEY", "secret", SecretType::Environment, None) + .unwrap(); store.remove("OPENAI_API_KEY").unwrap(); @@ -234,8 +317,12 @@ mod tests { fn list_returns_sorted_metadata_without_values() { let dir = tempfile::tempdir().unwrap(); let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - store.set("Z_KEY", "z").unwrap(); - store.set("A_KEY", "a").unwrap(); + store + .set("Z_KEY", "z", SecretType::Environment, None) + .unwrap(); + store + .set("A_KEY", "a", SecretType::Environment, None) + .unwrap(); let listed = store.list(); @@ -252,7 +339,177 @@ mod tests { fn invalid_names_are_rejected() { let dir = tempfile::tempdir().unwrap(); let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - let error = store.set("NOT-VALID", "secret").unwrap_err(); + let error = store + .set("NOT-VALID", "secret", SecretType::Environment, None) + .unwrap_err(); assert_eq!(error.to_string(), "invalid secret name: NOT-VALID"); } + + #[test] + fn set_file_secret_stores_type() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secrets.json"); + let mut store = SecretStore::load(path.clone()).unwrap(); + + let meta = store + .set("/root/.ssh/id_rsa", "secret", SecretType::File, None) + .unwrap(); + + assert_eq!(meta.name, "/root/.ssh/id_rsa"); + assert_eq!(meta.secret_type, SecretType::File); + + let reloaded = SecretStore::load(path).unwrap(); + let listed = reloaded.list(); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].secret_type, SecretType::File); + } + + #[test] + fn set_file_secret_validates_absolute_path() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + + let error = store + .set("relative/path", "secret", SecretType::File, None) + .unwrap_err(); + + assert_eq!(error.to_string(), "invalid secret name: relative/path"); + } + + #[test] + fn set_file_secret_rejects_traversal() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + + let error = store + .set("/root/../id_rsa", "secret", SecretType::File, None) + .unwrap_err(); + + assert_eq!(error.to_string(), "invalid secret name: /root/../id_rsa"); + } + + #[test] + fn set_env_secret_rejects_path_names() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + + let error = store + .set("/foo/bar", "secret", SecretType::Environment, None) + .unwrap_err(); + + assert_eq!(error.to_string(), "invalid secret name: /foo/bar"); + } + + #[test] + fn snapshot_excludes_file_secrets() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + store + .set("OPENAI_API_KEY", "env", SecretType::Environment, None) + .unwrap(); + store + .set("/tmp/test.pem", "file", SecretType::File, None) + .unwrap(); + + let snapshot = store.snapshot(); + + assert_eq!(snapshot.get("OPENAI_API_KEY"), Some(&"env".to_string())); + assert!(!snapshot.contains_key("/tmp/test.pem")); + } + + #[test] + fn snapshot_includes_only_env_secrets() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + store + .set("ANTHROPIC_API_KEY", "a", SecretType::Environment, None) + .unwrap(); + store + .set("OPENAI_API_KEY", "b", SecretType::Environment, None) + .unwrap(); + + let snapshot = store.snapshot(); + + assert_eq!(snapshot.len(), 2); + assert_eq!(snapshot.get("ANTHROPIC_API_KEY"), Some(&"a".to_string())); + assert_eq!(snapshot.get("OPENAI_API_KEY"), Some(&"b".to_string())); + } + + #[test] + fn file_secrets_returns_only_files() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + store + .set("OPENAI_API_KEY", "env", SecretType::Environment, None) + .unwrap(); + store + .set("/tmp/test.pem", "file", SecretType::File, None) + .unwrap(); + + let files = store.file_secrets(); + + assert_eq!(files, vec![( + "/tmp/test.pem".to_string(), + "file".to_string() + )]); + } + + #[test] + fn description_round_trips() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secrets.json"); + let mut store = SecretStore::load(path.clone()).unwrap(); + + let meta = store + .set( + "/tmp/test.pem", + "file", + SecretType::File, + Some("Test certificate"), + ) + .unwrap(); + + assert_eq!(meta.description.as_deref(), Some("Test certificate")); + + let reloaded = SecretStore::load(path).unwrap(); + let listed = reloaded.list(); + assert_eq!(listed[0].description.as_deref(), Some("Test certificate")); + } + + #[test] + fn legacy_json_defaults_to_environment() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secrets.json"); + std::fs::write( + &path, + r#"{ + "OPENAI_API_KEY": { + "value": "secret", + "created_at": "2026-04-12T00:00:00Z", + "updated_at": "2026-04-12T00:00:00Z" + } +}"#, + ) + .unwrap(); + + let store = SecretStore::load(path).unwrap(); + let listed = store.list(); + + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].secret_type, SecretType::Environment); + assert_eq!(listed[0].description, None); + } + + #[test] + fn remove_allows_file_path_names() { + let dir = tempfile::tempdir().unwrap(); + let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + store + .set("/tmp/test.pem", "file", SecretType::File, None) + .unwrap(); + + store.remove("/tmp/test.pem").unwrap(); + + assert!(store.list().is_empty()); + } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 08aee15b7..b9af91fc2 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -15,7 +15,7 @@ use axum::http::{HeaderMap, HeaderValue, Method, StatusCode, header}; use axum::middleware::{self, Next}; use axum::response::sse::{Event, KeepAlive, Sse}; use axum::response::{IntoResponse, Response}; -use axum::routing::{get, post, put}; +use axum::routing::{get, post}; use axum::{Json, Router}; use axum_extra::extract::cookie::Key; use base64::Engine as _; @@ -26,16 +26,16 @@ pub use fabro_api::types::{ ArtifactEntry, ArtifactListResponse, BilledTokenCounts as ApiBilledTokenCounts, BillingByModel, BillingStageRef, CompletionContentPart, CompletionMessage, CompletionMessageRole, CompletionResponse, CompletionToolChoiceMode, CompletionUsage, CreateCompletionRequest, - DiskUsageResponse, DiskUsageRunRow, DiskUsageSummaryRow, EventEnvelope as ApiEventEnvelope, - ModelReference, PaginatedEventList, PaginatedRunList, PaginationMeta, PreflightResponse, - PreviewUrlRequest, PreviewUrlResponse, PruneRunEntry, PruneRunsRequest, PruneRunsResponse, - QuestionType as ApiQuestionType, RenderWorkflowGraphDirection, RenderWorkflowGraphFormat, - RenderWorkflowGraphRequest, RunArtifactEntry, RunArtifactListResponse, RunBilling, - RunBillingStage, RunBillingTotals, RunControlAction as ApiRunControlAction, RunError, - RunManifest, RunStatus, RunStatusResponse, SandboxFileEntry, SandboxFileListResponse, - ServerSettings, SetSecretRequest, SshAccessRequest, SshAccessResponse, StartRunRequest, - StatusReason as ApiStatusReason, SubmitAnswerRequest, SystemInfoResponse, SystemRunCounts, - WriteBlobResponse, + CreateSecretRequest, DeleteSecretRequest, DiskUsageResponse, DiskUsageRunRow, + DiskUsageSummaryRow, EventEnvelope as ApiEventEnvelope, ModelReference, PaginatedEventList, + PaginatedRunList, PaginationMeta, PreflightResponse, PreviewUrlRequest, PreviewUrlResponse, + PruneRunEntry, PruneRunsRequest, PruneRunsResponse, QuestionType as ApiQuestionType, + RenderWorkflowGraphDirection, RenderWorkflowGraphFormat, RenderWorkflowGraphRequest, + RunArtifactEntry, RunArtifactListResponse, RunBilling, RunBillingStage, RunBillingTotals, + RunControlAction as ApiRunControlAction, RunError, RunManifest, RunStatus, RunStatusResponse, + SandboxFileEntry, SandboxFileListResponse, SecretType as ApiSecretType, ServerSettings, + SshAccessRequest, SshAccessResponse, StartRunRequest, StatusReason as ApiStatusReason, + SubmitAnswerRequest, SystemInfoResponse, SystemRunCounts, WriteBlobResponse, }; use fabro_config::{Storage, resolve_server_from_file}; use fabro_graphviz::render::GraphFormat; @@ -112,7 +112,7 @@ use crate::error::ApiError; use crate::jwt_auth::{ AuthMode, AuthenticatedService, AuthenticatedSubject, authenticate_service_parts, }; -use crate::secret_store::{SecretStore, SecretStoreError}; +use crate::secret_store::{SecretStore, SecretStoreError, SecretType as StoreSecretType}; use crate::{demo, diagnostics, run_manifest, settings_view, static_files, web_auth}; pub fn default_page_limit() -> u32 { @@ -1027,10 +1027,11 @@ fn demo_routes() -> Router> { .route("/insights/history", get(demo::list_query_history)) .route("/models", get(list_models)) .route("/models/{id}/test", post(test_model)) - .route("/secrets", get(demo::list_secrets)) .route( - "/secrets/{name}", - put(demo::set_secret).delete(demo::delete_secret), + "/secrets", + get(demo::list_secrets) + .post(demo::create_secret) + .delete(demo::delete_secret_by_name), ) .route("/repos/github/{owner}/{name}", get(demo::get_github_repo)) .route("/health/diagnostics", post(demo::run_diagnostics)) @@ -1106,8 +1107,12 @@ fn real_routes() -> Router> { .route("/insights/history", get(not_implemented)) .route("/models", get(list_models)) .route("/models/{id}/test", post(test_model)) - .route("/secrets", get(list_secrets)) - .route("/secrets/{name}", put(set_secret).delete(delete_secret)) + .route( + "/secrets", + get(list_secrets) + .post(create_secret) + .delete(delete_secret_by_name), + ) .route("/repos/github/{owner}/{name}", get(get_github_repo)) .route("/health/diagnostics", post(run_diagnostics)) .route("/completions", post(create_completion)) @@ -1621,16 +1626,26 @@ async fn list_secrets(_auth: AuthenticatedService, State(state): State StoreSecretType { + match secret_type { + ApiSecretType::Environment => StoreSecretType::Environment, + ApiSecretType::File => StoreSecretType::File, + } +} + +async fn create_secret( _auth: AuthenticatedService, State(state): State>, - Path(name): Path, - Json(body): Json, + Json(body): Json, ) -> Response { + let secret_type = secret_type_from_api(body.type_); + let name = body.name; + let value = body.value; + let description = body.description; let state_for_write = Arc::clone(&state); let result = spawn_blocking(move || { let mut store = state_for_write.secret_store.blocking_write(); - store.set(&name, &body.value) + store.set(&name, &value, secret_type, description.as_deref()) }) .await; @@ -1658,11 +1673,12 @@ async fn set_secret( } } -async fn delete_secret( +async fn delete_secret_by_name( _auth: AuthenticatedService, State(state): State>, - Path(name): Path, + Json(body): Json, ) -> Response { + let name = body.name; let state_for_write = Arc::clone(&state); let result = spawn_blocking(move || { let mut store = state_for_write.secret_store.blocking_write(); @@ -6199,6 +6215,80 @@ mod tests { format!("/api/v1{path}") } + #[tokio::test] + async fn create_secret_stores_file_secret_and_excludes_it_from_snapshot() { + let state = create_app_state(); + let app = build_router(Arc::clone(&state), AuthMode::Disabled); + + let req = Request::builder() + .method("POST") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "/tmp/test.pem", + "value": "pem-data", + "type": "file", + "description": "Test certificate", + })) + .unwrap(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + + let body = body_json(response.into_body()).await; + assert_eq!(body["name"], "/tmp/test.pem"); + assert_eq!(body["type"], "file"); + assert_eq!(body["description"], "Test certificate"); + + let store = state.secret_store.read().await; + assert!(!store.snapshot().contains_key("/tmp/test.pem")); + assert_eq!(store.file_secrets(), vec![( + "/tmp/test.pem".to_string(), + "pem-data".to_string() + )]); + } + + #[tokio::test] + async fn delete_secret_by_name_removes_file_secret() { + let state = create_app_state(); + let app = build_router(Arc::clone(&state), AuthMode::Disabled); + + let create_req = Request::builder() + .method("POST") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "/tmp/test.pem", + "value": "pem-data", + "type": "file", + })) + .unwrap(), + )) + .unwrap(); + let create_response = app.clone().oneshot(create_req).await.unwrap(); + assert_eq!(create_response.status(), StatusCode::OK); + + let delete_req = Request::builder() + .method("DELETE") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "/tmp/test.pem", + })) + .unwrap(), + )) + .unwrap(); + + let delete_response = app.oneshot(delete_req).await.unwrap(); + assert_eq!(delete_response.status(), StatusCode::NO_CONTENT); + assert!(state.secret_store.read().await.list().is_empty()); + } + #[tokio::test] async fn subprocess_answer_transport_cancel_run_enqueues_cancel_message() { let (control_tx, mut control_rx) = tokio::sync::mpsc::channel(1); diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 470b073a8..1d53f1433 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -13,6 +13,7 @@ use serde::{Deserialize, Serialize}; use serde_json::json; use tracing::{debug, error, info, warn}; +use crate::secret_store::SecretType; use crate::server::AppState; fn github_http_client(context: &str) -> Result { @@ -653,7 +654,7 @@ async fn setup_register( { let mut store = state.secret_store.write().await; for (name, value) in secret_updates { - if let Err(err) = store.set(name, &value) { + if let Err(err) = store.set(name, &value, SecretType::Environment, None) { error!(error = %err, secret = name, "Setup register failed: could not save secret"); return json_response( StatusCode::INTERNAL_SERVER_ERROR, diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 7d9b4f276..9d0a8e976 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -42,6 +42,8 @@ models/completion-tool-choice.ts models/completion-tool-definition.ts models/completion-usage.ts models/create-completion-request.ts +models/create-secret-request.ts +models/delete-secret-request.ts models/diagnostics-check.ts models/diagnostics-detail.ts models/diagnostics-report.ts @@ -142,7 +144,7 @@ models/save-query-request.ts models/saved-query.ts models/secret-list-response.ts models/secret-metadata.ts -models/set-secret-request.ts +models/secret-type.ts models/ssh-access-request.ts models/ssh-access-response.ts models/stage-status.ts diff --git a/lib/packages/fabro-api-client/src/api/secrets-api.ts b/lib/packages/fabro-api-client/src/api/secrets-api.ts index 7513f8277..c5c7a3b21 100644 --- a/lib/packages/fabro-api-client/src/api/secrets-api.ts +++ b/lib/packages/fabro-api-client/src/api/secrets-api.ts @@ -22,13 +22,15 @@ import { DUMMY_BASE_URL, assertParamExists, setApiKeyToObject, setBasicAuthToObj // @ts-ignore import { BASE_PATH, COLLECTION_FORMATS, type RequestArgs, BaseAPI, RequiredError, operationServerMap } from '../base'; // @ts-ignore +import type { CreateSecretRequest } from '../models'; +// @ts-ignore +import type { DeleteSecretRequest } from '../models'; +// @ts-ignore import type { ErrorResponse } from '../models'; // @ts-ignore import type { SecretListResponse } from '../models'; // @ts-ignore import type { SecretMetadata } from '../models'; -// @ts-ignore -import type { SetSecretRequest } from '../models'; /** * SecretsApi - axios parameter creator */ @@ -36,16 +38,57 @@ export const SecretsApiAxiosParamCreator = function (configuration?: Configurati return { /** * - * @summary Delete a stored secret - * @param {string} name + * @summary Store or update a secret + * @param {CreateSecretRequest} createSecretRequest * @param {*} [options] Override http request option. * @throws {RequiredError} */ - deleteSecret: async (name: string, options: RawAxiosRequestConfig = {}): Promise => { - // verify required parameter 'name' is not null or undefined - assertParamExists('deleteSecret', 'name', name) - const localVarPath = `/api/v1/secrets/{name}` - .replace(`{${"name"}}`, encodeURIComponent(String(name))); + createSecret: async (createSecretRequest: CreateSecretRequest, options: RawAxiosRequestConfig = {}): Promise => { + // verify required parameter 'createSecretRequest' is not null or undefined + assertParamExists('createSecret', 'createSecretRequest', createSecretRequest) + const localVarPath = `/api/v1/secrets`; + // use dummy base URL string because the URL constructor only accepts absolute URLs. + const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL); + let baseOptions; + if (configuration) { + baseOptions = configuration.baseOptions; + } + + const localVarRequestOptions = { method: 'POST', ...baseOptions, ...options}; + const localVarHeaderParameter = {} as any; + const localVarQueryParameter = {} as any; + + // authentication mTLS required + await setApiKeyToObject(localVarHeaderParameter, "X-mTLS-Client-CN", configuration) + + // authentication BearerAuth required + // http bearer authentication required + await setBearerAuthToObject(localVarHeaderParameter, configuration) + + localVarHeaderParameter['Content-Type'] = 'application/json'; + localVarHeaderParameter['Accept'] = 'application/json'; + + setSearchParams(localVarUrlObj, localVarQueryParameter); + let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; + localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; + localVarRequestOptions.data = serializeDataIfNeeded(createSecretRequest, localVarRequestOptions, configuration) + + return { + url: toPathString(localVarUrlObj), + options: localVarRequestOptions, + }; + }, + /** + * + * @summary Delete a stored secret + * @param {DeleteSecretRequest} deleteSecretRequest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + deleteSecretByName: async (deleteSecretRequest: DeleteSecretRequest, options: RawAxiosRequestConfig = {}): Promise => { + // verify required parameter 'deleteSecretRequest' is not null or undefined + assertParamExists('deleteSecretByName', 'deleteSecretRequest', deleteSecretRequest) + const localVarPath = `/api/v1/secrets`; // use dummy base URL string because the URL constructor only accepts absolute URLs. const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL); let baseOptions; @@ -64,11 +107,13 @@ export const SecretsApiAxiosParamCreator = function (configuration?: Configurati // http bearer authentication required await setBearerAuthToObject(localVarHeaderParameter, configuration) + localVarHeaderParameter['Content-Type'] = 'application/json'; localVarHeaderParameter['Accept'] = 'application/json'; setSearchParams(localVarUrlObj, localVarQueryParameter); let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; + localVarRequestOptions.data = serializeDataIfNeeded(deleteSecretRequest, localVarRequestOptions, configuration) return { url: toPathString(localVarUrlObj), @@ -107,52 +152,6 @@ export const SecretsApiAxiosParamCreator = function (configuration?: Configurati let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; - return { - url: toPathString(localVarUrlObj), - options: localVarRequestOptions, - }; - }, - /** - * - * @summary Store or update a secret - * @param {string} name - * @param {SetSecretRequest} setSecretRequest - * @param {*} [options] Override http request option. - * @throws {RequiredError} - */ - setSecret: async (name: string, setSecretRequest: SetSecretRequest, options: RawAxiosRequestConfig = {}): Promise => { - // verify required parameter 'name' is not null or undefined - assertParamExists('setSecret', 'name', name) - // verify required parameter 'setSecretRequest' is not null or undefined - assertParamExists('setSecret', 'setSecretRequest', setSecretRequest) - const localVarPath = `/api/v1/secrets/{name}` - .replace(`{${"name"}}`, encodeURIComponent(String(name))); - // use dummy base URL string because the URL constructor only accepts absolute URLs. - const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL); - let baseOptions; - if (configuration) { - baseOptions = configuration.baseOptions; - } - - const localVarRequestOptions = { method: 'PUT', ...baseOptions, ...options}; - const localVarHeaderParameter = {} as any; - const localVarQueryParameter = {} as any; - - // authentication mTLS required - await setApiKeyToObject(localVarHeaderParameter, "X-mTLS-Client-CN", configuration) - - // authentication BearerAuth required - // http bearer authentication required - await setBearerAuthToObject(localVarHeaderParameter, configuration) - - localVarHeaderParameter['Content-Type'] = 'application/json'; - localVarHeaderParameter['Accept'] = 'application/json'; - - setSearchParams(localVarUrlObj, localVarQueryParameter); - let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; - localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; - localVarRequestOptions.data = serializeDataIfNeeded(setSecretRequest, localVarRequestOptions, configuration) - return { url: toPathString(localVarUrlObj), options: localVarRequestOptions, @@ -169,15 +168,28 @@ export const SecretsApiFp = function(configuration?: Configuration) { return { /** * - * @summary Delete a stored secret - * @param {string} name + * @summary Store or update a secret + * @param {CreateSecretRequest} createSecretRequest * @param {*} [options] Override http request option. * @throws {RequiredError} */ - async deleteSecret(name: string, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { - const localVarAxiosArgs = await localVarAxiosParamCreator.deleteSecret(name, options); + async createSecret(createSecretRequest: CreateSecretRequest, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { + const localVarAxiosArgs = await localVarAxiosParamCreator.createSecret(createSecretRequest, options); const localVarOperationServerIndex = configuration?.serverIndex ?? 0; - const localVarOperationServerBasePath = operationServerMap['SecretsApi.deleteSecret']?.[localVarOperationServerIndex]?.url; + const localVarOperationServerBasePath = operationServerMap['SecretsApi.createSecret']?.[localVarOperationServerIndex]?.url; + return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); + }, + /** + * + * @summary Delete a stored secret + * @param {DeleteSecretRequest} deleteSecretRequest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + async deleteSecretByName(deleteSecretRequest: DeleteSecretRequest, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { + const localVarAxiosArgs = await localVarAxiosParamCreator.deleteSecretByName(deleteSecretRequest, options); + const localVarOperationServerIndex = configuration?.serverIndex ?? 0; + const localVarOperationServerBasePath = operationServerMap['SecretsApi.deleteSecretByName']?.[localVarOperationServerIndex]?.url; return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, /** @@ -192,20 +204,6 @@ export const SecretsApiFp = function(configuration?: Configuration) { const localVarOperationServerBasePath = operationServerMap['SecretsApi.listSecrets']?.[localVarOperationServerIndex]?.url; return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, - /** - * - * @summary Store or update a secret - * @param {string} name - * @param {SetSecretRequest} setSecretRequest - * @param {*} [options] Override http request option. - * @throws {RequiredError} - */ - async setSecret(name: string, setSecretRequest: SetSecretRequest, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { - const localVarAxiosArgs = await localVarAxiosParamCreator.setSecret(name, setSecretRequest, options); - const localVarOperationServerIndex = configuration?.serverIndex ?? 0; - const localVarOperationServerBasePath = operationServerMap['SecretsApi.setSecret']?.[localVarOperationServerIndex]?.url; - return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); - }, } }; @@ -217,13 +215,23 @@ export const SecretsApiFactory = function (configuration?: Configuration, basePa return { /** * - * @summary Delete a stored secret - * @param {string} name + * @summary Store or update a secret + * @param {CreateSecretRequest} createSecretRequest * @param {*} [options] Override http request option. * @throws {RequiredError} */ - deleteSecret(name: string, options?: RawAxiosRequestConfig): AxiosPromise { - return localVarFp.deleteSecret(name, options).then((request) => request(axios, basePath)); + createSecret(createSecretRequest: CreateSecretRequest, options?: RawAxiosRequestConfig): AxiosPromise { + return localVarFp.createSecret(createSecretRequest, options).then((request) => request(axios, basePath)); + }, + /** + * + * @summary Delete a stored secret + * @param {DeleteSecretRequest} deleteSecretRequest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + deleteSecretByName(deleteSecretRequest: DeleteSecretRequest, options?: RawAxiosRequestConfig): AxiosPromise { + return localVarFp.deleteSecretByName(deleteSecretRequest, options).then((request) => request(axios, basePath)); }, /** * Returns stored secret names and timestamps. Secret values are never exposed. @@ -234,17 +242,6 @@ export const SecretsApiFactory = function (configuration?: Configuration, basePa listSecrets(options?: RawAxiosRequestConfig): AxiosPromise { return localVarFp.listSecrets(options).then((request) => request(axios, basePath)); }, - /** - * - * @summary Store or update a secret - * @param {string} name - * @param {SetSecretRequest} setSecretRequest - * @param {*} [options] Override http request option. - * @throws {RequiredError} - */ - setSecret(name: string, setSecretRequest: SetSecretRequest, options?: RawAxiosRequestConfig): AxiosPromise { - return localVarFp.setSecret(name, setSecretRequest, options).then((request) => request(axios, basePath)); - }, }; }; @@ -254,13 +251,24 @@ export const SecretsApiFactory = function (configuration?: Configuration, basePa export class SecretsApi extends BaseAPI { /** * - * @summary Delete a stored secret - * @param {string} name + * @summary Store or update a secret + * @param {CreateSecretRequest} createSecretRequest * @param {*} [options] Override http request option. * @throws {RequiredError} */ - public deleteSecret(name: string, options?: RawAxiosRequestConfig) { - return SecretsApiFp(this.configuration).deleteSecret(name, options).then((request) => request(this.axios, this.basePath)); + public createSecret(createSecretRequest: CreateSecretRequest, options?: RawAxiosRequestConfig) { + return SecretsApiFp(this.configuration).createSecret(createSecretRequest, options).then((request) => request(this.axios, this.basePath)); + } + + /** + * + * @summary Delete a stored secret + * @param {DeleteSecretRequest} deleteSecretRequest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + public deleteSecretByName(deleteSecretRequest: DeleteSecretRequest, options?: RawAxiosRequestConfig) { + return SecretsApiFp(this.configuration).deleteSecretByName(deleteSecretRequest, options).then((request) => request(this.axios, this.basePath)); } /** @@ -272,17 +280,5 @@ export class SecretsApi extends BaseAPI { public listSecrets(options?: RawAxiosRequestConfig) { return SecretsApiFp(this.configuration).listSecrets(options).then((request) => request(this.axios, this.basePath)); } - - /** - * - * @summary Store or update a secret - * @param {string} name - * @param {SetSecretRequest} setSecretRequest - * @param {*} [options] Override http request option. - * @throws {RequiredError} - */ - public setSecret(name: string, setSecretRequest: SetSecretRequest, options?: RawAxiosRequestConfig) { - return SecretsApiFp(this.configuration).setSecret(name, setSecretRequest, options).then((request) => request(this.axios, this.basePath)); - } } diff --git a/lib/packages/fabro-api-client/src/models/create-secret-request.ts b/lib/packages/fabro-api-client/src/models/create-secret-request.ts new file mode 100644 index 000000000..d338df5b6 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/create-secret-request.ts @@ -0,0 +1,40 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +// May contain unused imports in some cases +// @ts-ignore +import type { SecretType } from './secret-type'; + +/** + * Request to store or update a secret. + */ +export interface CreateSecretRequest { + /** + * Secret name or destination path for file secrets. + */ + 'name': string; + /** + * The secret value to store. + */ + 'value': string; + 'type': SecretType; + /** + * Optional operator-facing description of the secret. + */ + 'description'?: string; +} + + + diff --git a/lib/packages/fabro-api-client/src/models/delete-secret-request.ts b/lib/packages/fabro-api-client/src/models/delete-secret-request.ts new file mode 100644 index 000000000..8f9d68211 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/delete-secret-request.ts @@ -0,0 +1,26 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * Request to delete a secret by name. + */ +export interface DeleteSecretRequest { + /** + * Secret name or destination path for file secrets. + */ + 'name': string; +} + diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index eade91d3c..dfcd3f3ae 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -24,6 +24,8 @@ export * from './completion-tool-choice'; export * from './completion-tool-definition'; export * from './completion-usage'; export * from './create-completion-request'; +export * from './create-secret-request'; +export * from './delete-secret-request'; export * from './diagnostics-check'; export * from './diagnostics-detail'; export * from './diagnostics-report'; @@ -123,7 +125,7 @@ export * from './save-query-request'; export * from './saved-query'; export * from './secret-list-response'; export * from './secret-metadata'; -export * from './set-secret-request'; +export * from './secret-type'; export * from './ssh-access-request'; export * from './ssh-access-response'; export * from './stage-status'; diff --git a/lib/packages/fabro-api-client/src/models/secret-metadata.ts b/lib/packages/fabro-api-client/src/models/secret-metadata.ts index 55be6bf6e..404fba427 100644 --- a/lib/packages/fabro-api-client/src/models/secret-metadata.ts +++ b/lib/packages/fabro-api-client/src/models/secret-metadata.ts @@ -13,15 +13,23 @@ */ +// May contain unused imports in some cases +// @ts-ignore +import type { SecretType } from './secret-type'; /** * Metadata for a stored secret (value is never exposed). */ export interface SecretMetadata { /** - * Secret key name. + * Secret key name or destination path. */ 'name': string; + 'type': SecretType; + /** + * Optional operator-facing description of the secret. + */ + 'description'?: string; /** * When the secret was first stored. */ @@ -32,3 +40,5 @@ export interface SecretMetadata { 'updated_at': string; } + + diff --git a/lib/packages/fabro-api-client/src/models/secret-type.ts b/lib/packages/fabro-api-client/src/models/secret-type.ts new file mode 100644 index 000000000..caa89fdf4 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/secret-type.ts @@ -0,0 +1,29 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * The way a secret is consumed by the sandbox. + */ + +export const SecretType = { + ENVIRONMENT: 'environment', + FILE: 'file' +} as const; + +export type SecretType = typeof SecretType[keyof typeof SecretType]; + + +