From 20f51a9bd436904afc32adb99cc83a0b7372a3fc Mon Sep 17 00:00:00 2001 From: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 09:29:01 -0700 Subject: [PATCH 1/6] fix: bind marketplace credentials to their origin Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- bb-cli/src/bb/agents_install.rs | 20 ++ bb-cli/src/bb/skills_api.rs | 582 +++++++++++++++++++++++++++++--- bb-cli/src/bb/skills_install.rs | 28 ++ 3 files changed, 578 insertions(+), 52 deletions(-) diff --git a/bb-cli/src/bb/agents_install.rs b/bb-cli/src/bb/agents_install.rs index e206b53f2..bfd70f800 100644 --- a/bb-cli/src/bb/agents_install.rs +++ b/bb-cli/src/bb/agents_install.rs @@ -777,6 +777,26 @@ impl Drop for AgentLock { #[cfg(test)] mod tests { use super::*; + use crate::bb::agents_models::AgentInstallArtifact; + use crate::bb::skills_api::MarketplaceClient; + + #[test] + fn agent_install_download_rejects_malformed_artifact_url() { + let artifact: AgentInstallArtifact = serde_json::from_value(serde_json::json!({ + "id": "artifact", + "download_url": "data:text/plain,secret", + "sha256": "unused", + "size_bytes": 0, + "media_type": "application/zip" + })) + .expect("parse artifact"); + let client = MarketplaceClient::unauthenticated_for_test("http://127.0.0.1:1"); + let error = client + .download(&artifact.download_url) + .expect_err("agent install URL must fail safely"); + + assert!(format!("{error:#}").contains("artifact URL must be an absolute HTTP(S) URL")); + } fn temp_paths(slug: &str) -> (tempfile::TempDir, AgentPaths) { let root = tempfile::tempdir().unwrap(); diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index a1c22f647..1105049e4 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -3,9 +3,11 @@ use anyhow::{Context, Result}; use reqwest::blocking::Client; use reqwest::header::{HeaderMap, HeaderValue, ACCEPT, CONTENT_TYPE}; +use reqwest::redirect::Policy; use reqwest::StatusCode; use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; +use url::Url; use super::agents_models::{ AgentCatalogPage, AgentDetail, AgentInstallPlan, AgentInstallPlanRequest, @@ -113,20 +115,25 @@ pub fn failure_info(error: &anyhow::Error) -> (i32, Value) { #[derive(Debug)] pub struct MarketplaceClient { - base_url: String, + base_url: Url, client: Client, + authenticated_artifact_client: Client, + artifact_client: Client, has_auth: bool, style: Style, } impl MarketplaceClient { pub fn new(config: &SkillsConfig) -> Result { + let service_url = kgoose_service_url(&config.kgoose_base_url, &config.kgoose_service_path); + let base_url = parse_http_url(&service_url, "marketplace service URL")?; + let marketplace_origin = base_url.clone(); let mut headers = HeaderMap::new(); headers.insert(ACCEPT, HeaderValue::from_static("application/json")); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); let session_credential = stored_session_credential_header_value( &config.profile, - &kgoose_service_url(&config.kgoose_base_url, &config.kgoose_service_path), + &service_url, config.bb_home.clone(), )?; if let Some(session_credential) = session_credential.as_deref() { @@ -144,11 +151,21 @@ impl MarketplaceClient { ); } Ok(Self { - base_url: kgoose_service_url(&config.kgoose_base_url, &config.kgoose_service_path), + base_url, client: Client::builder() - .default_headers(headers) + .default_headers(headers.clone()) + .redirect(same_origin_redirect_policy(marketplace_origin)) .build() .context("build marketplace HTTP client")?, + authenticated_artifact_client: Client::builder() + .default_headers(headers) + .redirect(Policy::none()) + .build() + .context("build authenticated artifact HTTP client")?, + artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .context("build artifact HTTP client")?, has_auth: session_credential.is_some(), style: config.style, }) @@ -158,6 +175,28 @@ impl MarketplaceClient { self.has_auth } + #[cfg(test)] + pub(crate) fn unauthenticated_for_test(base_url: &str) -> Self { + let base_url = Url::parse(base_url).expect("parse test marketplace URL"); + Self { + base_url: base_url.clone(), + client: Client::builder() + .redirect(same_origin_redirect_policy(base_url)) + .build() + .expect("build test marketplace client"), + authenticated_artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .expect("build test authenticated artifact client"), + artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .expect("build test artifact client"), + has_auth: false, + style: Style::new(true, true, false), + } + } + pub fn get_json(&self, path: &str) -> Result where T: for<'de> Deserialize<'de>, @@ -165,7 +204,7 @@ impl MarketplaceClient { self.style.verbose(&format!("GET {path}")); let response = self .client - .get(self.url(path)) + .get(self.url(path)?) .send() .map_err(|err| network_failure("GET", path, err))?; let status = response.status(); @@ -174,7 +213,7 @@ impl MarketplaceClient { .with_context(|| format!("read GET {path} response"))?; self.style .verbose(&format!("GET {path} -> {status} ({} bytes)", body.len())); - self.ensure_success("GET", path, status, body.as_bytes())?; + self.ensure_success("GET", path, status, body.as_bytes(), true)?; serde_json::from_str(&body).with_context(|| format!("deserialize GET {path} response")) } @@ -186,7 +225,7 @@ impl MarketplaceClient { self.style.verbose(&format!("POST {path}")); let response = self .client - .post(self.url(path)) + .post(self.url(path)?) .json(body) .send() .map_err(|err| network_failure("POST", path, err))?; @@ -196,7 +235,7 @@ impl MarketplaceClient { .with_context(|| format!("read POST {path} response"))?; self.style .verbose(&format!("POST {path} -> {status} ({} bytes)", body.len())); - self.ensure_success("POST", path, status, body.as_bytes())?; + self.ensure_success("POST", path, status, body.as_bytes(), true)?; serde_json::from_str(&body).with_context(|| format!("deserialize POST {path} response")) } @@ -205,50 +244,91 @@ impl MarketplaceClient { self.style.verbose(&format!("GET {path}")); let response = self .client - .get(self.url(path)) + .get(self.url(path)?) .send() .map_err(|err| network_failure("GET", path, err))?; let status = response.status(); let bytes = response .bytes() .with_context(|| format!("read GET {path} response"))?; - self.ensure_success("GET", path, status, &bytes)?; + self.ensure_success("GET", path, status, &bytes, true)?; Ok(bytes.to_vec()) } pub fn download(&self, path_or_url: &str) -> Result { - let url = if path_or_url.starts_with("http://") || path_or_url.starts_with("https://") { - path_or_url.to_string() - } else { - self.url(path_or_url) - }; + let mut url = self.artifact_url(path_or_url)?; + let mut authenticated = same_origin(&url, &self.base_url); self.style.verbose(&format!("GET {path_or_url} (artifact)")); - let response = self - .client - .get(&url) - .send() - .map_err(|err| network_failure("GET", path_or_url, err))?; - let status = response.status(); - let headers = response.headers().clone(); - let bytes = response - .bytes() - .with_context(|| format!("read GET {path_or_url} response"))?; - self.style.verbose(&format!( - "GET {path_or_url} -> {status} ({} bytes)", - bytes.len() - )); - self.ensure_success("GET", path_or_url, status, &bytes)?; - Ok(DownloadedArtifact { - bytes: bytes.to_vec(), - header_sha256: headers - .get("X-Artifact-SHA256") - .and_then(|value| value.to_str().ok()) - .map(ToOwned::to_owned), - header_size: headers - .get("X-Artifact-Size") - .and_then(|value| value.to_str().ok()) - .and_then(|value| value.parse::().ok()), - }) + + for redirects in 0..=10 { + let client = if authenticated { + &self.authenticated_artifact_client + } else { + &self.artifact_client + }; + let response = client + .get(url.clone()) + .send() + .map_err(|err| network_failure("GET", path_or_url, err))?; + let status = response.status(); + if is_redirect(status) { + if redirects == 10 { + return Err(failure( + exit_codes::NETWORK, + "too_many_redirects", + format!("GET {path_or_url} failed: too many redirects"), + )); + } + let location = response + .headers() + .get(reqwest::header::LOCATION) + .ok_or_else(|| { + failure( + exit_codes::NETWORK, + "invalid_redirect", + format!("GET {path_or_url} failed: redirect response omitted Location"), + ) + })?; + let location = location.to_str().map_err(|_| { + failure( + exit_codes::NETWORK, + "invalid_redirect", + format!("GET {path_or_url} failed: redirect Location is not valid text"), + ) + })?; + url = url.join(location).with_context(|| { + format!("resolve artifact redirect `{location}` from `{url}`") + })?; + ensure_http_url(&url, "artifact redirect URL")?; + // Once a chain leaves the marketplace origin it remains unauthenticated, + // even if a later redirect points back to the marketplace. + authenticated = authenticated && same_origin(&url, &self.base_url); + continue; + } + + let headers = response.headers().clone(); + let bytes = response + .bytes() + .with_context(|| format!("read GET {path_or_url} response"))?; + self.style.verbose(&format!( + "GET {path_or_url} -> {status} ({} bytes)", + bytes.len() + )); + self.ensure_success("GET", path_or_url, status, &bytes, authenticated)?; + return Ok(DownloadedArtifact { + bytes: bytes.to_vec(), + header_sha256: headers + .get("X-Artifact-SHA256") + .and_then(|value| value.to_str().ok()) + .map(ToOwned::to_owned), + header_size: headers + .get("X-Artifact-Size") + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.parse::().ok()), + }); + } + + unreachable!("redirect loop returns within its fixed bound") } /// Lists skills, following pagination so large catalogs are not silently @@ -299,12 +379,28 @@ impl MarketplaceClient { AgentMarketplace { client: self } } - fn url(&self, path: &str) -> String { - if path.starts_with('/') { - format!("{}{}", self.base_url, path) - } else { - format!("{}/{}", self.base_url, path) - } + fn url(&self, path: &str) -> Result { + let separator = if path.starts_with('/') { "" } else { "/" }; + let value = format!( + "{}{}{}", + self.base_url.as_str().trim_end_matches('/'), + separator, + path + ); + parse_http_url(&value, "marketplace URL") + .with_context(|| format!("resolve marketplace path `{path}`")) + } + + fn artifact_url(&self, path_or_url: &str) -> Result { + let url = match Url::parse(path_or_url) { + Ok(url) => url, + Err(url::ParseError::RelativeUrlWithoutBase) => self.url(path_or_url)?, + Err(error) => { + return Err(error).with_context(|| format!("parse artifact URL `{path_or_url}`")) + } + }; + ensure_http_url(&url, "artifact URL")?; + Ok(url) } fn ensure_success( @@ -313,6 +409,7 @@ impl MarketplaceClient { path: &str, status: StatusCode, body: &[u8], + marketplace_request: bool, ) -> Result<()> { if status.is_success() { return Ok(()); @@ -320,7 +417,9 @@ impl MarketplaceClient { let mut message = format_http_error(method, path, status, body); let exit_code = match status.as_u16() { 401 => { - message.push_str(if self.has_auth { + message.push_str(if !marketplace_request { + "\nhint: the artifact host rejected the request (401); no marketplace credential was sent, so `bb auth login` will not help" + } else if self.has_auth { "\nhint: the marketplace rejected your credentials; run `bb auth login` to refresh your session" } else { "\nhint: no credentials are configured; run `bb auth login` first" @@ -328,9 +427,11 @@ impl MarketplaceClient { exit_codes::AUTH_REQUIRED } 403 => { - message.push_str( - "\nhint: your credentials lack the required scope; run `bb auth login` with an authorized account", - ); + message.push_str(if marketplace_request { + "\nhint: your credentials lack the required scope; run `bb auth login` with an authorized account" + } else { + "\nhint: the artifact host denied access (403); no marketplace credential was sent, so `bb auth login` will not help" + }); exit_codes::FORBIDDEN } 422 => exit_codes::PLAN_BLOCKED, @@ -452,6 +553,48 @@ fn invalid_agent_operation(error: AgentOperationError) -> anyhow::Error { pub const LIST_PAGE_LIMIT: u32 = 5000; +fn parse_http_url(value: &str, label: &str) -> Result { + let url = Url::parse(value).with_context(|| format!("parse {label} `{value}`"))?; + ensure_http_url(&url, label)?; + Ok(url) +} + +fn ensure_http_url(url: &Url, label: &str) -> Result<()> { + if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() { + anyhow::bail!("{label} must be an absolute HTTP(S) URL: `{url}`"); + } + Ok(()) +} + +fn same_origin(left: &Url, right: &Url) -> bool { + left.scheme() == right.scheme() + && left.host_str() == right.host_str() + && left.port_or_known_default() == right.port_or_known_default() +} + +fn is_redirect(status: StatusCode) -> bool { + matches!( + status, + StatusCode::MOVED_PERMANENTLY + | StatusCode::FOUND + | StatusCode::SEE_OTHER + | StatusCode::TEMPORARY_REDIRECT + | StatusCode::PERMANENT_REDIRECT + ) +} + +fn same_origin_redirect_policy(origin: Url) -> Policy { + Policy::custom(move |attempt| { + if attempt.previous().len() > 10 { + attempt.error("too many redirects") + } else if same_origin(attempt.url(), &origin) { + attempt.follow() + } else { + attempt.error("refusing authenticated cross-origin redirect") + } + }) +} + fn network_failure(method: &str, path: &str, err: reqwest::Error) -> anyhow::Error { anyhow::Error::new(CliFailure::new( exit_codes::NETWORK, @@ -612,6 +755,134 @@ mod tests { type RecordedRequest = (String, String, Value); + #[derive(Clone, Debug)] + struct ArtifactRequest { + path: String, + headers: HeaderMap, + } + + struct ArtifactServer { + base_url: String, + requests: Arc>>, + handle: thread::JoinHandle<()>, + } + + impl ArtifactServer { + fn start(responses: Vec) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind artifact server"); + Self::start_with_listener(listener, responses) + } + + fn start_with_listener(listener: TcpListener, responses: Vec) -> Self { + let base_url = format!("http://{}", listener.local_addr().expect("server address")); + let requests = Arc::new(Mutex::new(Vec::new())); + let thread_requests = Arc::clone(&requests); + let handle = thread::spawn(move || { + for response in responses { + let (stream, _) = listener.accept().expect("accept artifact request"); + record_and_respond_raw(stream, &thread_requests, &response); + } + }); + Self { + base_url, + requests, + handle, + } + } + + fn finish(self) -> Vec { + self.handle.join().expect("join artifact server"); + self.requests.lock().expect("lock requests").clone() + } + } + + fn artifact_response(body: &[u8]) -> String { + format!( + "HTTP/1.1 200 OK\r\nX-Artifact-SHA256: test-sha\r\nX-Artifact-Size: {}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body.len(), + String::from_utf8_lossy(body) + ) + } + + fn redirect_response(location: &str) -> String { + format!( + "HTTP/1.1 302 Found\r\nLocation: {location}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + ) + } + + fn status_response(status_line: &str) -> String { + format!("HTTP/1.1 {status_line}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + } + + fn record_and_respond_raw( + stream: TcpStream, + requests: &Arc>>, + response: &str, + ) { + let mut reader = BufReader::new(stream.try_clone().expect("clone artifact stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read request line"); + let path = request_line + .split_whitespace() + .nth(1) + .expect("request path") + .to_string(); + let mut headers = HeaderMap::new(); + loop { + let mut line = String::new(); + reader.read_line(&mut line).expect("read request header"); + if line == "\r\n" { + break; + } + if let Some((name, value)) = line.split_once(':') { + headers.insert( + reqwest::header::HeaderName::from_bytes(name.as_bytes()) + .expect("valid header name"), + HeaderValue::from_str(value.trim()).expect("valid header value"), + ); + } + } + requests + .lock() + .expect("lock requests") + .push(ArtifactRequest { path, headers }); + let mut stream = stream; + stream + .write_all(response.as_bytes()) + .expect("write artifact response"); + } + + fn authenticated_client(base_url: &str) -> MarketplaceClient { + let base_url = Url::parse(base_url).expect("parse marketplace URL"); + let mut headers = HeaderMap::new(); + headers.insert( + SESSION_CREDENTIAL_HEADER, + HeaderValue::from_static("secret-session"), + ); + MarketplaceClient { + base_url: base_url.clone(), + client: Client::builder() + .default_headers(headers.clone()) + .redirect(same_origin_redirect_policy(base_url)) + .build() + .expect("build API client"), + authenticated_artifact_client: Client::builder() + .default_headers(headers) + .redirect(Policy::none()) + .build() + .expect("build authenticated artifact client"), + artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .expect("build artifact client"), + has_auth: true, + style: Style::new(true, true, false), + } + } + struct TestServer { base_url: String, requests: Arc>>, @@ -639,8 +910,16 @@ mod tests { fn client(&self) -> MarketplaceClient { MarketplaceClient { - base_url: self.base_url.clone(), + base_url: Url::parse(&self.base_url).expect("parse test server URL"), client: Client::new(), + authenticated_artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .expect("build test authenticated artifact client"), + artifact_client: Client::builder() + .redirect(Policy::none()) + .build() + .expect("build test artifact client"), has_auth: false, style: Style::new(true, true, false), } @@ -1036,6 +1315,205 @@ mod tests { server.finish(); } + #[test] + fn authenticated_api_client_refuses_cross_origin_redirect() { + let destination = ArtifactServer::start(Vec::new()); + let marketplace = ArtifactServer::start(vec![redirect_response(&format!( + "{}/catalog", + destination.base_url + ))]); + let client = authenticated_client(&marketplace.base_url); + + let error = client + .get_json::("/catalog") + .expect_err("authenticated API redirect must fail"); + + let (exit_code, payload) = failure_info(&error); + assert_eq!(exit_code, exit_codes::NETWORK); + assert_eq!(payload["error"]["code"], "server_unreachable"); + assert!(format!("{error:#}").contains("redirect")); + let marketplace_requests = marketplace.finish(); + assert_eq!(marketplace_requests.len(), 1); + assert!(marketplace_requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .is_some()); + assert!(destination.finish().is_empty()); + } + + #[test] + fn download_authenticates_same_origin_and_preserves_verification_headers() { + let server = ArtifactServer::start(vec![artifact_response(b"artifact")]); + let client = authenticated_client(&server.base_url); + + let download = client.download("/artifact.zip").expect("download artifact"); + + assert_eq!(download.bytes, b"artifact"); + assert_eq!(download.header_sha256.as_deref(), Some("test-sha")); + assert_eq!(download.header_size, Some(8)); + let requests = server.finish(); + assert_eq!(requests[0].path, "/artifact.zip"); + assert_eq!( + requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .and_then(|value| value.to_str().ok()), + Some("secret-session") + ); + } + + #[test] + fn download_uses_no_credential_for_cross_origin_initial_url() { + let marketplace = ArtifactServer::start(Vec::new()); + let artifact = ArtifactServer::start(vec![artifact_response(b"artifact")]); + let client = authenticated_client(&marketplace.base_url); + + client + .download(&format!("{}/artifact.zip", artifact.base_url)) + .expect("download cross-origin artifact"); + + let requests = artifact.finish(); + assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); + marketplace.finish(); + } + + #[test] + fn download_cross_origin_401_does_not_suggest_marketplace_login() { + let marketplace = ArtifactServer::start(Vec::new()); + let artifact = ArtifactServer::start(vec![status_response("401 Unauthorized")]); + let client = authenticated_client(&marketplace.base_url); + + let error = client + .download(&format!("{}/artifact.zip", artifact.base_url)) + .expect_err("cross-origin 401 must fail"); + + let (exit_code, _) = failure_info(&error); + assert_eq!(exit_code, exit_codes::AUTH_REQUIRED); + let rendered = format!("{error:#}"); + assert!(rendered.contains("artifact host")); + assert!(!rendered.contains("run `bb auth login`")); + artifact.finish(); + marketplace.finish(); + } + + #[test] + fn download_cross_origin_403_does_not_suggest_marketplace_login() { + let marketplace = ArtifactServer::start(Vec::new()); + let artifact = ArtifactServer::start(vec![status_response("403 Forbidden")]); + let client = authenticated_client(&marketplace.base_url); + + let error = client + .download(&format!("{}/artifact.zip", artifact.base_url)) + .expect_err("cross-origin 403 must fail"); + + let (exit_code, _) = failure_info(&error); + assert_eq!(exit_code, exit_codes::FORBIDDEN); + let rendered = format!("{error:#}"); + assert!(rendered.contains("artifact host")); + assert!(!rendered.contains("run `bb auth login`")); + artifact.finish(); + marketplace.finish(); + } + + #[test] + fn download_same_origin_401_keeps_marketplace_login_hint() { + let server = ArtifactServer::start(vec![status_response("401 Unauthorized")]); + let client = authenticated_client(&server.base_url); + + let error = client + .download("/artifact.zip") + .expect_err("same-origin 401 must fail"); + + let (exit_code, _) = failure_info(&error); + assert_eq!(exit_code, exit_codes::AUTH_REQUIRED); + assert!(format!("{error:#}").contains("run `bb auth login`")); + server.finish(); + } + + #[test] + fn download_keeps_credential_across_same_origin_redirect() { + let server = ArtifactServer::start(vec![ + redirect_response("/final.zip"), + artifact_response(b"artifact"), + ]); + let client = authenticated_client(&server.base_url); + + client + .download("/redirect") + .expect("follow same-origin artifact redirect"); + + let requests = server.finish(); + assert_eq!(requests.len(), 2); + assert!(requests + .iter() + .all(|request| request.headers.get(SESSION_CREDENTIAL_HEADER).is_some())); + } + + #[test] + fn download_drops_credential_on_cross_origin_redirect() { + let destination = ArtifactServer::start(vec![artifact_response(b"artifact")]); + let marketplace = ArtifactServer::start(vec![redirect_response(&format!( + "{}/artifact.zip", + destination.base_url + ))]); + let client = authenticated_client(&marketplace.base_url); + + client + .download("/redirect") + .expect("follow cross-origin artifact redirect"); + + let marketplace_requests = marketplace.finish(); + assert!(marketplace_requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .is_some()); + let destination_requests = destination.finish(); + assert!(destination_requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .is_none()); + } + + #[test] + fn download_never_restores_credential_after_cross_origin_redirect() { + let marketplace_listener = TcpListener::bind("127.0.0.1:0").expect("bind marketplace"); + let marketplace_url = format!( + "http://{}", + marketplace_listener + .local_addr() + .expect("marketplace address") + ); + let cross_origin = ArtifactServer::start(vec![redirect_response(&format!( + "{marketplace_url}/final.zip" + ))]); + let marketplace = ArtifactServer::start_with_listener( + marketplace_listener, + vec![ + redirect_response(&format!("{}/bounce", cross_origin.base_url)), + artifact_response(b"artifact"), + ], + ); + let client = authenticated_client(&marketplace.base_url); + client.download("/start").expect("download redirect chain"); + + let requests = marketplace.finish(); + assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_some()); + assert!(requests[1].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); + cross_origin.finish(); + } + + #[test] + fn download_rejects_malformed_and_unsafe_urls_without_requesting() { + let marketplace = ArtifactServer::start(Vec::new()); + let client = authenticated_client(&marketplace.base_url); + + for url in ["ftp://example.com/artifact", "http://[::1"] { + let error = client.download(url).expect_err("unsafe URL must fail"); + assert!(format!("{error:#}").contains("artifact URL")); + } + marketplace.finish(); + } + #[test] fn failure_info_defaults_to_general_exit_code() { let error = anyhow::anyhow!("boom"); diff --git a/bb-cli/src/bb/skills_install.rs b/bb-cli/src/bb/skills_install.rs index a141ae15d..778b4c36c 100644 --- a/bb-cli/src/bb/skills_install.rs +++ b/bb-cli/src/bb/skills_install.rs @@ -964,6 +964,34 @@ pub fn find_orphaned_work_dirs(root: &Path) -> Vec { #[cfg(test)] mod tests { use super::*; + use crate::bb::skills_api::MarketplaceClient; + + #[test] + fn skill_install_download_rejects_malformed_artifact_url() { + let operation: InstallOperation = serde_json::from_value(serde_json::json!({ + "action": "install", + "reason": "test", + "skill": { + "slug": "demo", + "version_id": "v1", + "content_sha256": "content" + }, + "artifact": { + "id": "artifact", + "download_url": "ftp://example.com/demo.zip", + "sha256": "unused", + "size_bytes": 0 + }, + "installed_via": "explicit" + })) + .expect("parse operation"); + let client = MarketplaceClient::unauthenticated_for_test("http://127.0.0.1:1"); + let error = client + .download(&operation.artifact.as_ref().expect("artifact").download_url) + .expect_err("skill install URL must fail safely"); + + assert!(format!("{error:#}").contains("artifact URL must be an absolute HTTP(S) URL")); + } #[test] fn iso8601_formats_known_timestamps() { From 53e2d77e13df46d1e414bbc0d527b73bb5077bde Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 16:23:36 +1000 Subject: [PATCH 2/6] fix: bind kgoose credentials to their origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HttpKgooseClient` installed `X-BB-Session-Credential` and `x-forwarded-identity-token` as default headers on a client built with reqwest's default redirect policy, which follows up to ten hops across hosts. reqwest only strips the headers it knows are sensitive (`Authorization`, `Cookie`, ...) when a redirect leaves the origin, so both credentials rode every hop. That was not hypothetical for this client: `post_json` checked the final URL for `cloudflareaccess.com` precisely because kgoose is known to redirect there when WARP is off, and that check ran after the response came back — i.e. after the credential had already been sent to the Cloudflare Access host. The client now follows redirects only within the configured base origin, matching the marketplace client. Origin binding helpers move out of `skills_api` into `http_origin` so both clients share one implementation of the origin check and the hop limit. The WARP diagnostic moves to the refusal path, where it now happens: the redirect policy records the target it declined, so the error names the host and states that credentials were withheld. The post-response check stays as a backstop for a base URL that is itself a Cloudflare Access host. Signed-off-by: Matt Toohey --- bb-cli/src/bb/skills_api.rs | 38 +--- bb-cli/src/http_origin.rs | 117 ++++++++++++ bb-cli/src/kgoose.rs | 357 ++++++++++++++++++++++++++++++++++-- bb-cli/src/lib.rs | 1 + 4 files changed, 467 insertions(+), 46 deletions(-) create mode 100644 bb-cli/src/http_origin.rs diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index 1105049e4..818b2d953 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -19,6 +19,9 @@ use super::auth_storage::stored_session_credential_header_value; use super::display::Style; use super::skills_config::{kgoose_service_url, SkillsConfig}; use super::skills_models::{BundlePage, BundleSummary, SkillPage, SkillSummary}; +use crate::http_origin::{ + ensure_http_url, parse_http_url, same_origin, same_origin_redirect_policy, MAX_REDIRECTS, +}; /// Documented `bb skills` exit codes (see `bb skills --help`). pub mod exit_codes { @@ -260,7 +263,7 @@ impl MarketplaceClient { let mut authenticated = same_origin(&url, &self.base_url); self.style.verbose(&format!("GET {path_or_url} (artifact)")); - for redirects in 0..=10 { + for redirects in 0..=MAX_REDIRECTS { let client = if authenticated { &self.authenticated_artifact_client } else { @@ -272,7 +275,7 @@ impl MarketplaceClient { .map_err(|err| network_failure("GET", path_or_url, err))?; let status = response.status(); if is_redirect(status) { - if redirects == 10 { + if redirects == MAX_REDIRECTS { return Err(failure( exit_codes::NETWORK, "too_many_redirects", @@ -553,25 +556,6 @@ fn invalid_agent_operation(error: AgentOperationError) -> anyhow::Error { pub const LIST_PAGE_LIMIT: u32 = 5000; -fn parse_http_url(value: &str, label: &str) -> Result { - let url = Url::parse(value).with_context(|| format!("parse {label} `{value}`"))?; - ensure_http_url(&url, label)?; - Ok(url) -} - -fn ensure_http_url(url: &Url, label: &str) -> Result<()> { - if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() { - anyhow::bail!("{label} must be an absolute HTTP(S) URL: `{url}`"); - } - Ok(()) -} - -fn same_origin(left: &Url, right: &Url) -> bool { - left.scheme() == right.scheme() - && left.host_str() == right.host_str() - && left.port_or_known_default() == right.port_or_known_default() -} - fn is_redirect(status: StatusCode) -> bool { matches!( status, @@ -583,18 +567,6 @@ fn is_redirect(status: StatusCode) -> bool { ) } -fn same_origin_redirect_policy(origin: Url) -> Policy { - Policy::custom(move |attempt| { - if attempt.previous().len() > 10 { - attempt.error("too many redirects") - } else if same_origin(attempt.url(), &origin) { - attempt.follow() - } else { - attempt.error("refusing authenticated cross-origin redirect") - } - }) -} - fn network_failure(method: &str, path: &str, err: reqwest::Error) -> anyhow::Error { anyhow::Error::new(CliFailure::new( exit_codes::NETWORK, diff --git a/bb-cli/src/http_origin.rs b/bb-cli/src/http_origin.rs new file mode 100644 index 000000000..83de86d2c --- /dev/null +++ b/bb-cli/src/http_origin.rs @@ -0,0 +1,117 @@ +//! Origin binding for the HTTP clients that carry BuilderBot credentials. +//! +//! Our authenticated clients install credentials as *default headers*, which +//! reqwest replays on every redirect hop. reqwest only strips the headers it +//! knows are sensitive (`Authorization`, `Cookie`, ...) when a redirect leaves +//! the origin, so custom headers such as `X-BB-Session-Credential` would +//! otherwise follow a redirect to any host the server names. Every client that +//! sends a credential therefore binds it to the origin it was issued for. + +use anyhow::{Context, Result}; +use reqwest::redirect::Policy; +use url::Url; + +/// Redirect hops an authenticated client follows before giving up. Matches +/// reqwest's own default limit. +pub const MAX_REDIRECTS: usize = 10; + +pub fn parse_http_url(value: &str, label: &str) -> Result { + let url = Url::parse(value).with_context(|| format!("parse {label} `{value}`"))?; + ensure_http_url(&url, label)?; + Ok(url) +} + +pub fn ensure_http_url(url: &Url, label: &str) -> Result<()> { + if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() { + anyhow::bail!("{label} must be an absolute HTTP(S) URL: `{url}`"); + } + Ok(()) +} + +pub fn same_origin(left: &Url, right: &Url) -> bool { + left.scheme() == right.scheme() + && left.host_str() == right.host_str() + && left.port_or_known_default() == right.port_or_known_default() +} + +/// Names an origin the way our diagnostics should: scheme, host, and port only, +/// never the path or query, which can carry request-specific data. +pub fn origin_label(url: &Url) -> String { + url.origin().ascii_serialization() +} + +/// Follows redirects only within `origin`, so a credential attached as a +/// default header can never leave the host it was issued for. +pub fn same_origin_redirect_policy(origin: Url) -> Policy { + same_origin_redirect_policy_with(origin, |_| { + "refusing authenticated cross-origin redirect".to_string() + }) +} + +/// [`same_origin_redirect_policy`] with a caller-supplied description of the +/// refusal. `describe_refusal` receives the redirect target so the caller can +/// explain the specific hop, which reqwest's redirect error cannot: it reports +/// the URL the redirect came *from*, not the one we declined to follow. +pub fn same_origin_redirect_policy_with(origin: Url, describe_refusal: F) -> Policy +where + F: Fn(&Url) -> String + Send + Sync + 'static, +{ + Policy::custom(move |attempt| { + if attempt.previous().len() > MAX_REDIRECTS { + attempt.error("too many redirects") + } else if same_origin(attempt.url(), &origin) { + attempt.follow() + } else { + let refusal = describe_refusal(attempt.url()); + attempt.error(refusal) + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn url(value: &str) -> Url { + Url::parse(value).expect("parse test URL") + } + + #[test] + fn same_origin_compares_scheme_host_and_effective_port() { + assert!(same_origin( + &url("https://kgoose.sqprod.co/a"), + &url("https://kgoose.sqprod.co:443/b") + )); + assert!(!same_origin( + &url("http://kgoose.sqprod.co/a"), + &url("https://kgoose.sqprod.co/a") + )); + assert!(!same_origin( + &url("https://kgoose.sqprod.co/a"), + &url("https://evil.example.com/a") + )); + assert!(!same_origin( + &url("http://127.0.0.1:1234/a"), + &url("http://127.0.0.1:4321/a") + )); + } + + #[test] + fn origin_label_omits_path_and_query() { + assert_eq!( + origin_label(&url("https://kgoose.sqprod.co/v3/call-tool?token=secret")), + "https://kgoose.sqprod.co" + ); + assert_eq!( + origin_label(&url("http://127.0.0.1:8080/v3")), + "http://127.0.0.1:8080" + ); + } + + #[test] + fn ensure_http_url_rejects_non_http_schemes_and_hostless_urls() { + assert!(ensure_http_url(&url("https://example.com"), "test URL").is_ok()); + assert!(ensure_http_url(&url("file:///etc/passwd"), "test URL").is_err()); + assert!(parse_http_url("not a url", "test URL").is_err()); + } +} diff --git a/bb-cli/src/kgoose.rs b/bb-cli/src/kgoose.rs index aa88d95fc..3f6d02814 100644 --- a/bb-cli/src/kgoose.rs +++ b/bb-cli/src/kgoose.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; use std::env; +use std::sync::{Arc, Mutex, PoisonError}; use std::time::Duration; use anyhow::{Context, Result}; @@ -7,9 +8,11 @@ use reqwest::blocking::{Client, ClientBuilder}; use reqwest::header::{HeaderMap, HeaderName, HeaderValue, ACCEPT, CONTENT_TYPE}; use serde::de::DeserializeOwned; use serde::Serialize; +use url::Url; use crate::bb::auth::SESSION_CREDENTIAL_HEADER; use crate::bb::skills_config::normalize_kgoose_service_path; +use crate::http_origin::{origin_label, parse_http_url, same_origin_redirect_policy_with}; pub use crate::proto::squareup::cash::kgoose::api::v3::{ CallToolRequest, CallToolResponse, ExtensionInfo, ListExtensionsRequest, ListExtensionsResponse, ListToolsRequest, ListToolsResponse, Source, ToolConfig, @@ -20,6 +23,8 @@ pub const DEFAULT_KGOOSE_BASE_URL: &str = "https://kgoose.sqprod.co"; pub const DEFAULT_KGOOSE_TIMEOUT_SECS: f64 = 600.0; const STS_ACCESS_TOKEN_ENV_VAR: &str = "STS_ACCESS_TOKEN"; const KGOOSE_DEBUG_ENV_VAR: &str = "KGOOSE_DEBUG"; +/// Registrable domain Cloudflare Access redirects to when WARP is off. +const CLOUDFLARE_ACCESS_DOMAIN: &str = "cloudflareaccess.com"; #[derive(Debug, Clone, PartialEq)] pub struct KgooseConfig { @@ -106,7 +111,8 @@ impl HttpKgooseClient { T: DeserializeOwned, B: Serialize + ?Sized, { - let client = build_http_client(config)?; + let base_url = parse_http_url(&config.base_url, "kgoose base URL")?; + let (client, refused_redirect) = build_http_client(config, &base_url)?; let service_path = normalize_kgoose_service_path(&config.service_path)?; let request_path = format!( "{}/{}", @@ -122,13 +128,20 @@ impl HttpKgooseClient { option_for_debug(config.goosemcp_playpen.as_deref()) )); - let response = client - .post(&url) - .json(body) - .send() - .with_context(|| format!("POST {request_path}"))?; + let response = match client.post(&url).json(body).send() { + Ok(response) => response, + Err(err) => { + // The slot is only filled when our redirect policy refused a + // hop, so a recorded target means the credential stayed home. + if let Some(target) = refused_redirect.take() { + return Err(cross_origin_redirect_error(&base_url, &target)); + } + return Err(err).with_context(|| format!("POST {request_path}")); + } + }; let status = response.status(); + let served_by_cloudflare_access = is_cloudflare_access(response.url()); let final_url = response.url().to_string(); let response_body = response .text() @@ -139,9 +152,12 @@ impl HttpKgooseClient { response_body.len() )); - // Check for Cloudflare Access redirect (indicates VPN is off) - // Note: Cloudflare returns 200 OK with an HTML login page, not an error status - if final_url.contains("cloudflareaccess.com") { + // Backstop for a Cloudflare Access login page (indicates VPN is off). + // A redirect to Cloudflare Access is refused before the credential is + // sent, so this only fires when the configured base URL is itself a + // Cloudflare Access host. Note that Cloudflare returns 200 OK with an + // HTML login page, not an error status. + if served_by_cloudflare_access { anyhow::bail!( "Cannot connect to kgoose - received Cloudflare Access redirect.\n\ This usually means you need to connect to the corporate VPN (WARP).\n\ @@ -159,7 +175,51 @@ impl HttpKgooseClient { } } -fn build_http_client(config: &KgooseConfig) -> Result { +/// Records the redirect target the client refused to follow, so `post_json` can +/// name the host it declined to send credentials to. +#[derive(Clone, Debug, Default)] +struct RefusedRedirect(Arc>>); + +impl RefusedRedirect { + fn record(&self, url: &Url) { + *self.0.lock().unwrap_or_else(PoisonError::into_inner) = Some(url.clone()); + } + + fn take(&self) -> Option { + self.0.lock().unwrap_or_else(PoisonError::into_inner).take() + } +} + +fn is_cloudflare_access(url: &Url) -> bool { + url.host_str().is_some_and(|host| { + host == CLOUDFLARE_ACCESS_DOMAIN || host.ends_with(&format!(".{CLOUDFLARE_ACCESS_DOMAIN}")) + }) +} + +fn cross_origin_redirect_error(base_url: &Url, target: &Url) -> anyhow::Error { + let base_origin = origin_label(base_url); + let target_origin = origin_label(target); + if is_cloudflare_access(target) { + anyhow::anyhow!( + "Cannot connect to kgoose - {base_origin} redirected to Cloudflare Access \ + ({target_origin}).\n\ + This usually means you need to connect to the corporate VPN (WARP).\n\ + Please enable WARP and try again.\n\ + Your kgoose credentials were not sent to {target_origin}." + ) + } else { + anyhow::anyhow!( + "Cannot connect to kgoose - {base_origin} redirected to {target_origin}, a different \ + origin.\n\ + Your kgoose credentials were not sent to {target_origin}; point KGOOSE_BASE_URL at \ + {target_origin} directly if that host is the intended service." + ) + } +} + +/// Builds the request client alongside the slot that records a refused +/// cross-origin redirect. +fn build_http_client(config: &KgooseConfig, base_url: &Url) -> Result<(Client, RefusedRedirect)> { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); headers.insert(ACCEPT, HeaderValue::from_static("application/json")); @@ -211,11 +271,29 @@ fn build_http_client(config: &KgooseConfig) -> Result { .join(",") )); - ClientBuilder::new() + // The session credential and identity token ride on every request as + // default headers, so redirects stay inside the configured origin: reqwest + // would otherwise replay them to whatever host a redirect names, which is + // exactly what happens when Cloudflare Access bounces us off-origin. + let refused_redirect = RefusedRedirect::default(); + let redirect_policy = same_origin_redirect_policy_with(base_url.clone(), { + let refused_redirect = refused_redirect.clone(); + move |target| { + refused_redirect.record(target); + format!( + "refusing to send kgoose credentials across a redirect to {}", + origin_label(target) + ) + } + }); + + let client = ClientBuilder::new() .default_headers(headers) + .redirect(redirect_policy) .timeout(config.timeout()) .build() - .context("build HTTP client") + .context("build HTTP client")?; + Ok((client, refused_redirect)) } fn truncate(value: &str, max_len: usize) -> String { @@ -249,9 +327,262 @@ fn option_for_debug(value: Option<&str>) -> &str { #[cfg(test)] mod tests { - use super::{CallToolResponse, ListExtensionsResponse, ListToolsResponse}; + use std::io::{self, BufRead, BufReader, Read, Write}; + use std::net::{TcpListener, TcpStream}; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::thread; + + use super::*; + use crate::bb::skills_config::DEFAULT_KGOOSE_SERVICE_PATH; use crate::proto::squareup::cash::kgoose::api::v3::user_content; + const TEST_SESSION_CREDENTIAL: &str = "test-session-credential"; + + #[derive(Clone, Debug)] + struct RecordedRequest { + path: String, + headers: HeaderMap, + } + + impl RecordedRequest { + fn session_credential(&self) -> Option<&str> { + self.headers + .get(SESSION_CREDENTIAL_HEADER) + .and_then(|value| value.to_str().ok()) + } + } + + /// Minimal HTTP server for the redirect tests. It accepts non-blocking and + /// stops on drop, so a test that expects *no* request cannot hang waiting + /// for one that never arrives. + struct TestServer { + base_url: String, + requests: Arc>>, + stop: Arc, + handle: Option>, + } + + impl TestServer { + fn start(responses: Vec) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind test server"); + listener + .set_nonblocking(true) + .expect("set listener non-blocking"); + let base_url = format!("http://{}", listener.local_addr().expect("server address")); + let requests = Arc::new(Mutex::new(Vec::new())); + let stop = Arc::new(AtomicBool::new(false)); + let thread_requests = Arc::clone(&requests); + let thread_stop = Arc::clone(&stop); + let handle = thread::spawn(move || { + let mut responses = responses.into_iter(); + while !thread_stop.load(Ordering::Relaxed) { + match listener.accept() { + Ok((stream, _)) => match responses.next() { + Some(response) => { + record_and_respond(stream, &thread_requests, &response) + } + None => break, + }, + Err(err) if err.kind() == io::ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(5)); + } + Err(_) => break, + } + } + }); + Self { + base_url, + requests, + stop, + handle: Some(handle), + } + } + + fn requests(&self) -> Vec { + self.requests.lock().expect("lock requests").clone() + } + } + + impl Drop for TestServer { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(handle) = self.handle.take() { + let _ = handle.join(); + } + } + } + + fn record_and_respond( + stream: TcpStream, + requests: &Arc>>, + response: &str, + ) { + stream + .set_nonblocking(false) + .expect("set stream blocking(false)"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("set stream read timeout"); + let mut reader = BufReader::new(stream.try_clone().expect("clone test stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read request line"); + let path = request_line + .split_whitespace() + .nth(1) + .expect("request path") + .to_string(); + let mut headers = HeaderMap::new(); + let mut content_length = 0usize; + loop { + let mut line = String::new(); + reader.read_line(&mut line).expect("read request header"); + if line == "\r\n" || line.is_empty() { + break; + } + if let Some((name, value)) = line.split_once(':') { + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().expect("content length"); + } + headers.insert( + HeaderName::from_bytes(name.as_bytes()).expect("valid header name"), + HeaderValue::from_str(value.trim()).expect("valid header value"), + ); + } + } + let mut body = vec![0; content_length]; + reader.read_exact(&mut body).expect("read request body"); + requests + .lock() + .expect("lock requests") + .push(RecordedRequest { path, headers }); + let mut stream = stream; + stream + .write_all(response.as_bytes()) + .expect("write test response"); + } + + fn json_response(body: &str) -> String { + format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ) + } + + /// 307 keeps the method and body, so the retried hop is another authenticated POST. + fn redirect_response(location: &str) -> String { + format!( + "HTTP/1.1 307 Temporary Redirect\r\nLocation: {location}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + ) + } + + fn test_config(base_url: &str) -> KgooseConfig { + KgooseConfig { + base_url: base_url.to_string(), + service_path: DEFAULT_KGOOSE_SERVICE_PATH.to_string(), + playpen: None, + goosemcp_playpen: None, + timeout_secs: 5.0, + session_credential: Some(TEST_SESSION_CREDENTIAL.to_string()), + } + } + + fn list_extensions(config: &KgooseConfig) -> Result { + HttpKgooseClient.post_json(config, LIST_EXTENSIONS_PATH, &ListExtensionsRequest {}) + } + + #[test] + fn post_json_keeps_credential_across_same_origin_redirect() { + let server = TestServer::start(vec![ + redirect_response("/cash-app/goose/v3/list-extensions?retry=1"), + json_response("{}"), + ]); + + let response = + list_extensions(&test_config(&server.base_url)).expect("follow same-origin redirect"); + + assert!(response.extensions.is_empty()); + let requests = server.requests(); + assert_eq!(requests.len(), 2); + assert_eq!( + requests[1].path, + "/cash-app/goose/v3/list-extensions?retry=1" + ); + for request in &requests { + assert_eq!( + request.session_credential(), + Some(TEST_SESSION_CREDENTIAL), + "same-origin hop {} lost the credential", + request.path + ); + } + } + + #[test] + fn post_json_refuses_cross_origin_redirect_without_sending_credential() { + // Queue a response the destination would happily serve, so the empty + // request log below reflects a refusal rather than a dead listener. + let destination = TestServer::start(vec![json_response("{}")]); + let kgoose = TestServer::start(vec![redirect_response(&format!( + "{}/cash-app/goose/v3/list-extensions", + destination.base_url + ))]); + + let error = + list_extensions(&test_config(&kgoose.base_url)).expect_err("refuse cross-origin hop"); + + let message = format!("{error:#}"); + assert!(message.contains(&destination.base_url), "{message}"); + assert!(message.contains("credentials were not sent"), "{message}"); + assert_eq!(kgoose.requests().len(), 1); + assert!( + destination.requests().is_empty(), + "credential-bearing request reached the redirect target" + ); + } + + #[test] + fn post_json_reports_cloudflare_access_redirect_as_vpn_hint() { + let server = TestServer::start(vec![redirect_response( + "https://block.cloudflareaccess.com/cdn-cgi/access/login/kgoose.sqprod.co", + )]); + + let error = list_extensions(&test_config(&server.base_url)) + .expect_err("refuse Cloudflare Access hop"); + + let message = format!("{error:#}"); + assert!(message.contains("WARP"), "{message}"); + assert!( + message.contains("https://block.cloudflareaccess.com"), + "{message}" + ); + assert!(message.contains("credentials were not sent"), "{message}"); + } + + #[test] + fn cloudflare_access_detection_requires_the_real_domain() { + for host in [ + "https://block.cloudflareaccess.com/login", + "https://cloudflareaccess.com/login", + ] { + assert!( + is_cloudflare_access(&Url::parse(host).expect("parse URL")), + "{host}" + ); + } + for host in [ + "https://notcloudflareaccess.com/login", + "https://cloudflareaccess.com.evil.example/login", + "https://kgoose.sqprod.co/cash-app/goose", + ] { + assert!( + !is_cloudflare_access(&Url::parse(host).expect("parse URL")), + "{host}" + ); + } + } + #[test] fn list_tools_response_deserializes_generated_proto_shape() { let response: ListToolsResponse = serde_json::from_str( diff --git a/bb-cli/src/lib.rs b/bb-cli/src/lib.rs index c331fda34..32ee837e6 100644 --- a/bb-cli/src/lib.rs +++ b/bb-cli/src/lib.rs @@ -2,6 +2,7 @@ mod appkit; mod bb; mod catalog; mod cli; +mod http_origin; mod kgoose; mod proto; mod runtime; From ca79089daefe29acf45c7d3f607cdca0b9d07052 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 16:33:03 +1000 Subject: [PATCH 3/6] test(bb-cli): fail instead of hang when a queued response goes unrequested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The marketplace test servers spawned a thread that looped `accept()` once per queued response, and `finish()` joined it. A client that made fewer requests than the test queued responses left that thread parked in `accept()` forever, so `finish()` never returned. That is exactly the shape a regression in the redirect tests produces: `download_keeps_credential_across_same_origin_redirect` queues two responses, so a change that stopped following same-origin redirects would hang CI rather than report a failed assertion. The servers now use the non-blocking, stop-on-drop accept loop the kgoose redirect tests already use, and expose `requests()` instead of a joining `finish()`. Requests are recorded before their response is written, so every hop the client saw completed is already recorded when the client call returns — the assertions do not need the join. Accepted streams also get a read timeout, since a connection that never sends a request would otherwise park the serving thread in `read_line`. That loop now lives in `test_server` and both modules share it, rather than two copies of the same concurrency code. Call sites that only used `finish()` for teardown now assert what they were implicitly relying on: the credential-bearing origin received no request. Signed-off-by: Matt Toohey --- bb-cli/src/bb/skills_api.rs | 74 ++++++++++++++++++------------------- bb-cli/src/kgoose.rs | 57 ++++++---------------------- bb-cli/src/lib.rs | 2 + bb-cli/src/test_server.rs | 73 ++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 84 deletions(-) create mode 100644 bb-cli/src/test_server.rs diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index 818b2d953..1d0fa04e3 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -720,9 +720,9 @@ mod tests { use std::io::{BufRead, BufReader, Read, Write}; use std::net::{TcpListener, TcpStream}; use std::sync::{Arc, Mutex}; - use std::thread; use super::*; + use crate::test_server::{prepare_stream, ServerThread}; use serde_json::json; type RecordedRequest = (String, String, Value); @@ -736,7 +736,7 @@ mod tests { struct ArtifactServer { base_url: String, requests: Arc>>, - handle: thread::JoinHandle<()>, + _thread: ServerThread, } impl ArtifactServer { @@ -749,21 +749,20 @@ mod tests { let base_url = format!("http://{}", listener.local_addr().expect("server address")); let requests = Arc::new(Mutex::new(Vec::new())); let thread_requests = Arc::clone(&requests); - let handle = thread::spawn(move || { - for response in responses { - let (stream, _) = listener.accept().expect("accept artifact request"); - record_and_respond_raw(stream, &thread_requests, &response); - } + let thread = ServerThread::spawn(listener, responses, move |stream, response| { + record_and_respond_raw(stream, &thread_requests, &response); }); Self { base_url, requests, - handle, + _thread: thread, } } - fn finish(self) -> Vec { - self.handle.join().expect("join artifact server"); + /// Requests recorded so far. Each request is recorded before its + /// response is written, so every hop the client saw completed is + /// already here by the time the client call returns. + fn requests(&self) -> Vec { self.requests.lock().expect("lock requests").clone() } } @@ -792,6 +791,7 @@ mod tests { requests: &Arc>>, response: &str, ) { + prepare_stream(&stream); let mut reader = BufReader::new(stream.try_clone().expect("clone artifact stream")); let mut request_line = String::new(); reader @@ -858,7 +858,7 @@ mod tests { struct TestServer { base_url: String, requests: Arc>>, - handle: thread::JoinHandle<()>, + _thread: ServerThread, } impl TestServer { @@ -867,16 +867,13 @@ mod tests { let base_url = format!("http://{}", listener.local_addr().expect("server address")); let requests = Arc::new(Mutex::new(Vec::new())); let thread_requests = Arc::clone(&requests); - let handle = thread::spawn(move || { - for response in responses { - let (stream, _) = listener.accept().expect("accept client request"); - record_and_respond(stream, &thread_requests, response); - } + let thread = ServerThread::spawn(listener, responses, move |stream, response| { + record_and_respond(stream, &thread_requests, response); }); Self { base_url, requests, - handle, + _thread: thread, } } @@ -897,8 +894,8 @@ mod tests { } } - fn finish(self) -> Vec { - self.handle.join().expect("join test server"); + /// Requests recorded so far; see [`ArtifactServer::requests`]. + fn requests(&self) -> Vec { self.requests.lock().expect("lock requests").clone() } } @@ -908,6 +905,7 @@ mod tests { requests: &Arc>>, response: Value, ) { + prepare_stream(&stream); let mut reader = BufReader::new(stream.try_clone().expect("clone test stream")); let mut request_line = String::new(); reader @@ -1148,7 +1146,7 @@ mod tests { "application/zip" ); - let requests = server.finish(); + let requests = server.requests(); assert_eq!(requests[0].0, "GET"); assert_eq!( requests[0].1, @@ -1212,7 +1210,6 @@ mod tests { let (exit_code, payload) = failure_info(&error); assert_eq!(exit_code, exit_codes::VERIFICATION); assert_eq!(payload["error"]["code"], "invalid_agent_operation_kind"); - server.finish(); } } @@ -1249,7 +1246,6 @@ mod tests { payload["error"]["message"], "requested version `agent-v1` but the server resolved `agent-v2`; the marketplace currently serves only the latest stable version" ); - server.finish(); } #[test] @@ -1284,7 +1280,6 @@ mod tests { assert_eq!(resolution.plan.version_id, "agent-v1"); assert!(resolution.artifact.is_none()); assert_eq!(resolution.installed_via, "explicit"); - server.finish(); } #[test] @@ -1304,13 +1299,13 @@ mod tests { assert_eq!(exit_code, exit_codes::NETWORK); assert_eq!(payload["error"]["code"], "server_unreachable"); assert!(format!("{error:#}").contains("redirect")); - let marketplace_requests = marketplace.finish(); + let marketplace_requests = marketplace.requests(); assert_eq!(marketplace_requests.len(), 1); assert!(marketplace_requests[0] .headers .get(SESSION_CREDENTIAL_HEADER) .is_some()); - assert!(destination.finish().is_empty()); + assert!(destination.requests().is_empty()); } #[test] @@ -1323,7 +1318,7 @@ mod tests { assert_eq!(download.bytes, b"artifact"); assert_eq!(download.header_sha256.as_deref(), Some("test-sha")); assert_eq!(download.header_size, Some(8)); - let requests = server.finish(); + let requests = server.requests(); assert_eq!(requests[0].path, "/artifact.zip"); assert_eq!( requests[0] @@ -1344,9 +1339,9 @@ mod tests { .download(&format!("{}/artifact.zip", artifact.base_url)) .expect("download cross-origin artifact"); - let requests = artifact.finish(); + let requests = artifact.requests(); assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); - marketplace.finish(); + assert!(marketplace.requests().is_empty()); } #[test] @@ -1364,8 +1359,8 @@ mod tests { let rendered = format!("{error:#}"); assert!(rendered.contains("artifact host")); assert!(!rendered.contains("run `bb auth login`")); - artifact.finish(); - marketplace.finish(); + assert_eq!(artifact.requests().len(), 1); + assert!(marketplace.requests().is_empty()); } #[test] @@ -1383,8 +1378,8 @@ mod tests { let rendered = format!("{error:#}"); assert!(rendered.contains("artifact host")); assert!(!rendered.contains("run `bb auth login`")); - artifact.finish(); - marketplace.finish(); + assert_eq!(artifact.requests().len(), 1); + assert!(marketplace.requests().is_empty()); } #[test] @@ -1399,7 +1394,7 @@ mod tests { let (exit_code, _) = failure_info(&error); assert_eq!(exit_code, exit_codes::AUTH_REQUIRED); assert!(format!("{error:#}").contains("run `bb auth login`")); - server.finish(); + assert_eq!(server.requests().len(), 1); } #[test] @@ -1414,7 +1409,7 @@ mod tests { .download("/redirect") .expect("follow same-origin artifact redirect"); - let requests = server.finish(); + let requests = server.requests(); assert_eq!(requests.len(), 2); assert!(requests .iter() @@ -1434,12 +1429,12 @@ mod tests { .download("/redirect") .expect("follow cross-origin artifact redirect"); - let marketplace_requests = marketplace.finish(); + let marketplace_requests = marketplace.requests(); assert!(marketplace_requests[0] .headers .get(SESSION_CREDENTIAL_HEADER) .is_some()); - let destination_requests = destination.finish(); + let destination_requests = destination.requests(); assert!(destination_requests[0] .headers .get(SESSION_CREDENTIAL_HEADER) @@ -1468,10 +1463,11 @@ mod tests { let client = authenticated_client(&marketplace.base_url); client.download("/start").expect("download redirect chain"); - let requests = marketplace.finish(); + let requests = marketplace.requests(); + assert_eq!(requests.len(), 2); assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_some()); assert!(requests[1].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); - cross_origin.finish(); + assert_eq!(cross_origin.requests().len(), 1); } #[test] @@ -1483,7 +1479,7 @@ mod tests { let error = client.download(url).expect_err("unsafe URL must fail"); assert!(format!("{error:#}").contains("artifact URL")); } - marketplace.finish(); + assert!(marketplace.requests().is_empty()); } #[test] diff --git a/bb-cli/src/kgoose.rs b/bb-cli/src/kgoose.rs index 3f6d02814..f1c21bf86 100644 --- a/bb-cli/src/kgoose.rs +++ b/bb-cli/src/kgoose.rs @@ -327,14 +327,13 @@ fn option_for_debug(value: Option<&str>) -> &str { #[cfg(test)] mod tests { - use std::io::{self, BufRead, BufReader, Read, Write}; + use std::io::{BufRead, BufReader, Read, Write}; use std::net::{TcpListener, TcpStream}; - use std::sync::atomic::{AtomicBool, Ordering}; - use std::thread; use super::*; use crate::bb::skills_config::DEFAULT_KGOOSE_SERVICE_PATH; use crate::proto::squareup::cash::kgoose::api::v3::user_content; + use crate::test_server::{prepare_stream, ServerThread}; const TEST_SESSION_CREDENTIAL: &str = "test-session-credential"; @@ -352,77 +351,45 @@ mod tests { } } - /// Minimal HTTP server for the redirect tests. It accepts non-blocking and - /// stops on drop, so a test that expects *no* request cannot hang waiting + /// Minimal HTTP server for the redirect tests, over the shared + /// [`ServerThread`] so a test that expects *no* request cannot hang waiting /// for one that never arrives. struct TestServer { base_url: String, requests: Arc>>, - stop: Arc, - handle: Option>, + _thread: ServerThread, } impl TestServer { fn start(responses: Vec) -> Self { let listener = TcpListener::bind("127.0.0.1:0").expect("bind test server"); - listener - .set_nonblocking(true) - .expect("set listener non-blocking"); let base_url = format!("http://{}", listener.local_addr().expect("server address")); let requests = Arc::new(Mutex::new(Vec::new())); - let stop = Arc::new(AtomicBool::new(false)); let thread_requests = Arc::clone(&requests); - let thread_stop = Arc::clone(&stop); - let handle = thread::spawn(move || { - let mut responses = responses.into_iter(); - while !thread_stop.load(Ordering::Relaxed) { - match listener.accept() { - Ok((stream, _)) => match responses.next() { - Some(response) => { - record_and_respond(stream, &thread_requests, &response) - } - None => break, - }, - Err(err) if err.kind() == io::ErrorKind::WouldBlock => { - thread::sleep(Duration::from_millis(5)); - } - Err(_) => break, - } - } + let thread = ServerThread::spawn(listener, responses, move |stream, response| { + record_and_respond(stream, &thread_requests, &response); }); Self { base_url, requests, - stop, - handle: Some(handle), + _thread: thread, } } + /// Requests recorded so far. Each request is recorded before its + /// response is written, so every hop the client saw completed is + /// already here by the time the client call returns. fn requests(&self) -> Vec { self.requests.lock().expect("lock requests").clone() } } - impl Drop for TestServer { - fn drop(&mut self) { - self.stop.store(true, Ordering::Relaxed); - if let Some(handle) = self.handle.take() { - let _ = handle.join(); - } - } - } - fn record_and_respond( stream: TcpStream, requests: &Arc>>, response: &str, ) { - stream - .set_nonblocking(false) - .expect("set stream blocking(false)"); - stream - .set_read_timeout(Some(Duration::from_secs(5))) - .expect("set stream read timeout"); + prepare_stream(&stream); let mut reader = BufReader::new(stream.try_clone().expect("clone test stream")); let mut request_line = String::new(); reader diff --git a/bb-cli/src/lib.rs b/bb-cli/src/lib.rs index 32ee837e6..b665ba733 100644 --- a/bb-cli/src/lib.rs +++ b/bb-cli/src/lib.rs @@ -6,6 +6,8 @@ mod http_origin; mod kgoose; mod proto; mod runtime; +#[cfg(test)] +mod test_server; pub use bb::agents_models; pub use bb::skills_api::{AgentMarketplace, MarketplaceClient}; diff --git a/bb-cli/src/test_server.rs b/bb-cli/src/test_server.rs new file mode 100644 index 000000000..1f7c368a8 --- /dev/null +++ b/bb-cli/src/test_server.rs @@ -0,0 +1,73 @@ +//! Socket plumbing shared by the unit tests that drive a real HTTP client +//! through redirects. The clients under test decide per hop whether to send a +//! credential, so those tests need an actual listener rather than a mock. + +use std::io; +use std::net::{TcpListener, TcpStream}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; +use std::thread; +use std::time::Duration; + +/// Serves queued responses in order, one per connection. It accepts +/// non-blocking and stops on drop, so a test that queues more responses than +/// the client requests — the shape a dropped redirect hop produces — fails its +/// assertions instead of blocking forever in `accept()`. +pub struct ServerThread { + stop: Arc, + handle: Option>, +} + +impl ServerThread { + pub fn spawn( + listener: TcpListener, + responses: Vec, + respond: impl Fn(TcpStream, R) + Send + 'static, + ) -> Self { + listener + .set_nonblocking(true) + .expect("set listener non-blocking"); + let stop = Arc::new(AtomicBool::new(false)); + let thread_stop = Arc::clone(&stop); + let handle = thread::spawn(move || { + let mut responses = responses.into_iter(); + while !thread_stop.load(Ordering::Relaxed) { + match listener.accept() { + Ok((stream, _)) => match responses.next() { + Some(response) => respond(stream, response), + None => break, + }, + Err(err) if err.kind() == io::ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(5)); + } + Err(_) => break, + } + } + }); + Self { + stop, + handle: Some(handle), + } + } +} + +impl Drop for ServerThread { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(handle) = self.handle.take() { + let _ = handle.join(); + } + } +} + +/// Accepted streams can inherit the listener's non-blocking flag, and a client +/// that connects without sending a request would otherwise park the serving +/// thread in `read_line` forever. +pub fn prepare_stream(stream: &TcpStream) { + stream + .set_nonblocking(false) + .expect("set stream blocking(false)"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("set stream read timeout"); +} From f671b371713bf84abbd5b4998d7f5cce8bb30ec3 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 16:38:35 +1000 Subject: [PATCH 4/6] test(bb-cli): record requests no test queued a response for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ServerThread` dropped a connection that arrived after its response queue was drained: it accepted the stream, found no response, and broke out of the loop without handing the request to the caller's recording closure. Requests are only logged inside those closures, so a request no test expected left no trace. That made every "this host was never contacted" assertion vacuous. `authenticated_api_client_refuses_cross_origin_redirect` asserted the redirect target's log was empty while the target had no queued responses, so the line passed whether or not the client sent the credential across origins — the very thing the test exists to catch. Four sibling assertions on the marketplace log had the same hole. Connections past the end of the queue are now handed to `respond` with `None`, and both modules' closures serve a 500 in that case, so the request is recorded either way. Verified by pointing the test client at `Policy::limited(10)`: the redirect-target assertion now fails where it previously passed. The assertions also say what they mean when they fire, rather than reporting a bare `is_empty()`. The kgoose refusal test queued a response to work around this; the comment explaining that now describes what the queued response is still for, which is making a followed hop fail on `expect_err` rather than on an unserved request. Signed-off-by: Matt Toohey --- bb-cli/src/bb/skills_api.rs | 33 ++++++++++++++++++++++++++++----- bb-cli/src/kgoose.rs | 13 +++++++++++-- bb-cli/src/test_server.rs | 13 ++++++++----- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index 1d0fa04e3..a5d5cbf69 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -750,6 +750,7 @@ mod tests { let requests = Arc::new(Mutex::new(Vec::new())); let thread_requests = Arc::clone(&requests); let thread = ServerThread::spawn(listener, responses, move |stream, response| { + let response = response.unwrap_or_else(unexpected_request_response); record_and_respond_raw(stream, &thread_requests, &response); }); Self { @@ -786,6 +787,12 @@ mod tests { format!("HTTP/1.1 {status_line}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") } + /// Served for a request the test queued no response for, so that request is + /// still recorded rather than dropped. + fn unexpected_request_response() -> String { + status_response("500 Internal Server Error") + } + fn record_and_respond_raw( stream: TcpStream, requests: &Arc>>, @@ -868,6 +875,7 @@ mod tests { let requests = Arc::new(Mutex::new(Vec::new())); let thread_requests = Arc::clone(&requests); let thread = ServerThread::spawn(listener, responses, move |stream, response| { + let response = response.unwrap_or_else(|| json!({"error": "unexpected request"})); record_and_respond(stream, &thread_requests, response); }); Self { @@ -1305,7 +1313,10 @@ mod tests { .headers .get(SESSION_CREDENTIAL_HEADER) .is_some()); - assert!(destination.requests().is_empty()); + assert!( + destination.requests().is_empty(), + "credential-bearing request reached the redirect target" + ); } #[test] @@ -1341,7 +1352,10 @@ mod tests { let requests = artifact.requests(); assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); - assert!(marketplace.requests().is_empty()); + assert!( + marketplace.requests().is_empty(), + "cross-origin artifact URL was fetched through the marketplace" + ); } #[test] @@ -1360,7 +1374,10 @@ mod tests { assert!(rendered.contains("artifact host")); assert!(!rendered.contains("run `bb auth login`")); assert_eq!(artifact.requests().len(), 1); - assert!(marketplace.requests().is_empty()); + assert!( + marketplace.requests().is_empty(), + "artifact host failure was retried against the marketplace" + ); } #[test] @@ -1379,7 +1396,10 @@ mod tests { assert!(rendered.contains("artifact host")); assert!(!rendered.contains("run `bb auth login`")); assert_eq!(artifact.requests().len(), 1); - assert!(marketplace.requests().is_empty()); + assert!( + marketplace.requests().is_empty(), + "artifact host failure was retried against the marketplace" + ); } #[test] @@ -1479,7 +1499,10 @@ mod tests { let error = client.download(url).expect_err("unsafe URL must fail"); assert!(format!("{error:#}").contains("artifact URL")); } - assert!(marketplace.requests().is_empty()); + assert!( + marketplace.requests().is_empty(), + "rejected URL still produced a request" + ); } #[test] diff --git a/bb-cli/src/kgoose.rs b/bb-cli/src/kgoose.rs index f1c21bf86..ed0694c4f 100644 --- a/bb-cli/src/kgoose.rs +++ b/bb-cli/src/kgoose.rs @@ -367,6 +367,7 @@ mod tests { let requests = Arc::new(Mutex::new(Vec::new())); let thread_requests = Arc::clone(&requests); let thread = ServerThread::spawn(listener, responses, move |stream, response| { + let response = response.unwrap_or_else(unexpected_request_response); record_and_respond(stream, &thread_requests, &response); }); Self { @@ -437,6 +438,13 @@ mod tests { ) } + /// Served for a request the test queued no response for, so that request is + /// still recorded rather than dropped. + fn unexpected_request_response() -> String { + "HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + .to_string() + } + /// 307 keeps the method and body, so the retried hop is another authenticated POST. fn redirect_response(location: &str) -> String { format!( @@ -488,8 +496,9 @@ mod tests { #[test] fn post_json_refuses_cross_origin_redirect_without_sending_credential() { - // Queue a response the destination would happily serve, so the empty - // request log below reflects a refusal rather than a dead listener. + // The destination serves what it would serve in the attack it stands in + // for, so a client that followed the hop fails on `expect_err` rather + // than on some incidental error from an unserved request. let destination = TestServer::start(vec![json_response("{}")]); let kgoose = TestServer::start(vec![redirect_response(&format!( "{}/cash-app/goose/v3/list-extensions", diff --git a/bb-cli/src/test_server.rs b/bb-cli/src/test_server.rs index 1f7c368a8..bbe433362 100644 --- a/bb-cli/src/test_server.rs +++ b/bb-cli/src/test_server.rs @@ -13,6 +13,12 @@ use std::time::Duration; /// non-blocking and stops on drop, so a test that queues more responses than /// the client requests — the shape a dropped redirect hop produces — fails its /// assertions instead of blocking forever in `accept()`. +/// +/// A connection that arrives after the queue is drained is still handed to +/// `respond`, with `None` in place of a response, so the request lands in the +/// caller's log. Otherwise a request no test expected would go unrecorded, and +/// `assert!(server.requests().is_empty())` — the way these tests state "the +/// client never contacted this host" — could not fail. pub struct ServerThread { stop: Arc, handle: Option>, @@ -22,7 +28,7 @@ impl ServerThread { pub fn spawn( listener: TcpListener, responses: Vec, - respond: impl Fn(TcpStream, R) + Send + 'static, + respond: impl Fn(TcpStream, Option) + Send + 'static, ) -> Self { listener .set_nonblocking(true) @@ -33,10 +39,7 @@ impl ServerThread { let mut responses = responses.into_iter(); while !thread_stop.load(Ordering::Relaxed) { match listener.accept() { - Ok((stream, _)) => match responses.next() { - Some(response) => respond(stream, response), - None => break, - }, + Ok((stream, _)) => respond(stream, responses.next()), Err(err) if err.kind() == io::ErrorKind::WouldBlock => { thread::sleep(Duration::from_millis(5)); } From 5995a2863958391e0a7dcc4f6b7fa9d4c8e7c158 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 16:46:51 +1000 Subject: [PATCH 5/6] test(bb-cli): drive artifact URL rejection through the install entry points MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent_install_download_rejects_malformed_artifact_url` and `skill_install_download_rejects_malformed_artifact_url` ran no install code despite their names: each deserialized an artifact model and then called `MarketplaceClient::download` directly, which `download_rejects_malformed_and_unsafe_urls_without_requesting` in `skills_api.rs` already covers. All they added over that test was serde coverage for the two artifact models, so they would keep passing even if `install_agent`/`install_skill` stopped calling `download` safely, swallowed the refusal, or left a partly staged install behind. Both are replaced by e2e tests that drive a marketplace-supplied non-HTTP(S) `download_url` through `bb agents install` and `bb skills install`, the way `bb_e2e.rs` already drives other artifact failures. The plan is served by the mock marketplace, so the artifact models are still deserialized from real plan JSON. Each test pins the refusal reaching the caller (exit code and message), the recorded requests stopping before the artifact fetch, and nothing being left on disk: no agent document, state record, or held lock; no package, staging directory, target link, or persisted download. Verified by deleting the `ensure_http_url` call in `artifact_url`: both tests fail, where the deleted unit tests were the only thing standing between that regression and a green suite for the install paths. `unauthenticated_for_test` goes with them — it was added for those two tests and has no other caller. Signed-off-by: Matt Toohey --- bb-cli/src/bb/agents_install.rs | 20 ---- bb-cli/src/bb/skills_api.rs | 22 ---- bb-cli/src/bb/skills_install.rs | 28 ----- bb-cli/tests/bb_e2e.rs | 196 ++++++++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 70 deletions(-) diff --git a/bb-cli/src/bb/agents_install.rs b/bb-cli/src/bb/agents_install.rs index bfd70f800..e206b53f2 100644 --- a/bb-cli/src/bb/agents_install.rs +++ b/bb-cli/src/bb/agents_install.rs @@ -777,26 +777,6 @@ impl Drop for AgentLock { #[cfg(test)] mod tests { use super::*; - use crate::bb::agents_models::AgentInstallArtifact; - use crate::bb::skills_api::MarketplaceClient; - - #[test] - fn agent_install_download_rejects_malformed_artifact_url() { - let artifact: AgentInstallArtifact = serde_json::from_value(serde_json::json!({ - "id": "artifact", - "download_url": "data:text/plain,secret", - "sha256": "unused", - "size_bytes": 0, - "media_type": "application/zip" - })) - .expect("parse artifact"); - let client = MarketplaceClient::unauthenticated_for_test("http://127.0.0.1:1"); - let error = client - .download(&artifact.download_url) - .expect_err("agent install URL must fail safely"); - - assert!(format!("{error:#}").contains("artifact URL must be an absolute HTTP(S) URL")); - } fn temp_paths(slug: &str) -> (tempfile::TempDir, AgentPaths) { let root = tempfile::tempdir().unwrap(); diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index a5d5cbf69..78eb56a88 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -178,28 +178,6 @@ impl MarketplaceClient { self.has_auth } - #[cfg(test)] - pub(crate) fn unauthenticated_for_test(base_url: &str) -> Self { - let base_url = Url::parse(base_url).expect("parse test marketplace URL"); - Self { - base_url: base_url.clone(), - client: Client::builder() - .redirect(same_origin_redirect_policy(base_url)) - .build() - .expect("build test marketplace client"), - authenticated_artifact_client: Client::builder() - .redirect(Policy::none()) - .build() - .expect("build test authenticated artifact client"), - artifact_client: Client::builder() - .redirect(Policy::none()) - .build() - .expect("build test artifact client"), - has_auth: false, - style: Style::new(true, true, false), - } - } - pub fn get_json(&self, path: &str) -> Result where T: for<'de> Deserialize<'de>, diff --git a/bb-cli/src/bb/skills_install.rs b/bb-cli/src/bb/skills_install.rs index 778b4c36c..a141ae15d 100644 --- a/bb-cli/src/bb/skills_install.rs +++ b/bb-cli/src/bb/skills_install.rs @@ -964,34 +964,6 @@ pub fn find_orphaned_work_dirs(root: &Path) -> Vec { #[cfg(test)] mod tests { use super::*; - use crate::bb::skills_api::MarketplaceClient; - - #[test] - fn skill_install_download_rejects_malformed_artifact_url() { - let operation: InstallOperation = serde_json::from_value(serde_json::json!({ - "action": "install", - "reason": "test", - "skill": { - "slug": "demo", - "version_id": "v1", - "content_sha256": "content" - }, - "artifact": { - "id": "artifact", - "download_url": "ftp://example.com/demo.zip", - "sha256": "unused", - "size_bytes": 0 - }, - "installed_via": "explicit" - })) - .expect("parse operation"); - let client = MarketplaceClient::unauthenticated_for_test("http://127.0.0.1:1"); - let error = client - .download(&operation.artifact.as_ref().expect("artifact").download_url) - .expect_err("skill install URL must fail safely"); - - assert!(format!("{error:#}").contains("artifact URL must be an absolute HTTP(S) URL")); - } #[test] fn iso8601_formats_known_timestamps() { diff --git a/bb-cli/tests/bb_e2e.rs b/bb-cli/tests/bb_e2e.rs index 123580155..ae70201f0 100644 --- a/bb-cli/tests/bb_e2e.rs +++ b/bb-cli/tests/bb_e2e.rs @@ -260,6 +260,26 @@ fn snapshot_agent_target(path: &Path) -> (bool, bool, Option>) { (file_type.is_dir(), file_type.is_symlink(), bytes) } +/// Entry names in `path`, sorted, or empty when the directory does not exist. +/// A failed install must leave nothing behind, including the staging and backup +/// entries an `exists()` check on the final path would miss. +fn sorted_dir_entries(path: &Path) -> Vec { + let Ok(entries) = fs::read_dir(path) else { + return Vec::new(); + }; + let mut names = entries + .map(|entry| { + entry + .expect("read directory entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .collect::>(); + names.sort(); + names +} + fn assert_agent_pair_unchanged( target: &Path, state: &Path, @@ -1050,6 +1070,101 @@ fn bb_agents_preserve_managed_pairs_for_failure_envelopes() { fs::remove_dir_all(sandbox).expect("remove failure sandbox"); } +/// The download URL is marketplace-supplied, so a plan can name a scheme that +/// would take the request somewhere the marketplace client cannot reach safely. +/// `bb agents install` must refuse it before opening a connection, surface the +/// refusal, and leave no half-installed agent behind. +#[test] +fn bb_agents_install_refuses_non_http_artifact_url_before_requesting_it() { + let sandbox = temp_test_dir("bb-agents-artifact-url"); + let bb_home = sandbox.join("bb-home"); + let home = sandbox.join("home"); + write_bb_org_config(&bb_home, "test"); + + let server = MockServer::start(vec![ + MockResponse::json(marketplace_agent_detail( + "release-notes", + "agent-v1", + "content-v1", + )), + agent_install_plan( + "release-notes", + "agent-v1", + "content-v1", + "install", + Some(json!({ + "id": "art_agent-v1", + "download_url": "data:text/plain,secret", + "sha256": "unused", + "size_bytes": 0, + "media_type": "application/zip" + })), + ), + MockResponse::json(marketplace_agent_version( + "release-notes", + "agent-v1", + "content-v1", + )), + ]); + let output = bb_command() + .env("BB_HOME", &bb_home) + .env("HOME", &home) + .env("KGOOSE_BASE_URL", &server.base_url) + .args(["agents", "install", "release-notes", "--json"]) + .output() + .expect("run bb agents install"); + let requests = server.finish(); + let (stdout, stderr) = output_text(&output); + + assert!(stdout.is_empty(), "stdout was: {stdout}"); + assert_eq!(output.status.code(), Some(1), "stderr was: {stderr}"); + let error = parse_stderr_error(&stderr); + assert_eq!(error["error"]["code"], "cli_error"); + assert!( + error["error"]["message"] + .as_str() + .expect("error message string") + .contains("artifact URL must be an absolute HTTP(S) URL"), + "install must name the refused URL; stderr was: {stderr}" + ); + assert_eq!( + requests + .iter() + .map(|request| request.path.as_str()) + .collect::>(), + [ + "/api/goose/v1/marketplace/agents/release-notes", + "/api/goose/v1/marketplace/install-plan", + "/api/goose/v1/marketplace/agents/release-notes/versions/agent-v1" + ], + "install must stop at the refused artifact URL" + ); + + let target = agent_target(&home, "release-notes"); + assert!( + !target.exists(), + "refused install wrote {}", + target.display() + ); + assert_eq!( + sorted_dir_entries(target.parent().expect("agents dir")), + Vec::::new(), + "refused install left staged files beside the agent document" + ); + assert_eq!( + sorted_dir_entries(&bb_home.join("agents").join("installed")), + Vec::::new(), + "refused install left an install record" + ); + assert_eq!( + sorted_dir_entries(&bb_home.join("agents").join("locks")), + Vec::::new(), + "refused install held its lock" + ); + + fs::remove_dir_all(sandbox).expect("remove artifact URL sandbox"); +} + /// Server capabilities pointing the `agents` target at a directory we control, /// so installs link into the test sandbox instead of the real home directory. fn capabilities_response(agents_dir: &Path) -> MockResponse { @@ -2945,6 +3060,87 @@ fn bb_skills_install_surfaces_artifact_error_envelope() { fs::remove_dir_all(temp).expect("remove temp dir"); } +/// Skill counterpart to +/// `bb_agents_install_refuses_non_http_artifact_url_before_requesting_it`: the +/// plan names the download URL, so `bb skills install` must refuse a non-HTTP(S) +/// one before opening a connection and leave no package or staging directory. +#[test] +fn bb_skills_install_refuses_non_http_artifact_url_before_requesting_it() { + let zip_bytes = skill_zip(&[("SKILL.md", "# BuilderBot Tools\n")]); + let artifact_sha = sha256_hex(&zip_bytes); + let temp = temp_test_dir("bb-skills-artifact-url"); + let bb_home = temp.join("bb-home"); + write_bb_org_config(&bb_home, "test"); + let agents_dir = temp.join("agents-skills"); + let packages_dir = temp.join("skills-home/packages"); + let mut plan = marketplace_install_plan(&zip_bytes, &artifact_sha, zip_bytes.len()); + plan["operations"][0]["artifact"]["download_url"] = json!("ftp://example.com/artifact.zip"); + let server = MockServer::start(vec![ + capabilities_response(&agents_dir), + MockResponse::json(plan), + skill_detail_response(), + ]); + + let output = bb_command() + .env("BB_HOME", &bb_home) + .env("BB_SKILLS_HOME", temp.join("skills-home")) + .env("BB_SKILLS_PACKAGES_DIR", &packages_dir) + .env("KGOOSE_BASE_URL", &server.base_url) + .args([ + "skills", + "install", + "builderbot-tools", + "--target", + "agents", + "--yes", + "--json", + ]) + .output() + .expect("run bb skills install"); + let requests = server.finish(); + let (stdout, stderr) = output_text(&output); + + assert!(stdout.is_empty(), "stdout was: {stdout}"); + assert_eq!(output.status.code(), Some(1), "stderr was: {stderr}"); + let payload = parse_stderr_error(&stderr); + assert_eq!(payload["error"]["code"], json!("cli_error")); + assert!( + payload["error"]["message"] + .as_str() + .expect("error message string") + .contains("artifact URL must be an absolute HTTP(S) URL"), + "install must name the refused URL; stderr was: {stderr}" + ); + assert_eq!( + requests + .iter() + .map(|request| request.path.as_str()) + .collect::>(), + [ + "/api/goose/v1/marketplace/capabilities", + "/api/goose/v1/marketplace/install-plan", + "/api/goose/v1/marketplace/skills/builderbot-tools" + ], + "install must stop at the refused artifact URL" + ); + assert_eq!( + sorted_dir_entries(&packages_dir), + Vec::::new(), + "refused install left a package or staging directory" + ); + assert_eq!( + sorted_dir_entries(&agents_dir), + Vec::::new(), + "refused install linked into the target" + ); + assert_eq!( + sorted_dir_entries(&temp.join("skills-home/downloads")), + Vec::::new(), + "refused install persisted an artifact" + ); + fs::remove_dir_all(temp).expect("remove temp dir"); +} + #[test] fn bb_skills_install_refuses_checksum_mismatch() { let good_zip = skill_zip(&[("SKILL.md", "# BuilderBot Tools\n")]); From 2a6d678bcc0d2b0cffdd825526636cf2326757ee Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 18:31:08 +1000 Subject: [PATCH 6/6] revert: fix: bind kgoose credentials to their origin This reverts commit 53e2d77e13df46d1e414bbc0d527b73bb5077bde. HttpKgooseClient goes back to reqwest's default redirect policy with the post-response Cloudflare Access check, the origin-binding helpers move from http_origin back into skills_api as the marketplace client's private helpers, and http_origin is deleted. The kgoose redirect tests added by the reverted commit are removed with the behavior they pinned; resolving them against the later test-server refactors keeps the shared test_server module, which the marketplace tests still use. Signed-off-by: Matt Toohey --- bb-cli/src/bb/skills_api.rs | 38 +++- bb-cli/src/http_origin.rs | 117 ------------- bb-cli/src/kgoose.rs | 333 ++---------------------------------- bb-cli/src/lib.rs | 1 - 4 files changed, 46 insertions(+), 443 deletions(-) delete mode 100644 bb-cli/src/http_origin.rs diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index 78eb56a88..3e4964954 100644 --- a/bb-cli/src/bb/skills_api.rs +++ b/bb-cli/src/bb/skills_api.rs @@ -19,9 +19,6 @@ use super::auth_storage::stored_session_credential_header_value; use super::display::Style; use super::skills_config::{kgoose_service_url, SkillsConfig}; use super::skills_models::{BundlePage, BundleSummary, SkillPage, SkillSummary}; -use crate::http_origin::{ - ensure_http_url, parse_http_url, same_origin, same_origin_redirect_policy, MAX_REDIRECTS, -}; /// Documented `bb skills` exit codes (see `bb skills --help`). pub mod exit_codes { @@ -241,7 +238,7 @@ impl MarketplaceClient { let mut authenticated = same_origin(&url, &self.base_url); self.style.verbose(&format!("GET {path_or_url} (artifact)")); - for redirects in 0..=MAX_REDIRECTS { + for redirects in 0..=10 { let client = if authenticated { &self.authenticated_artifact_client } else { @@ -253,7 +250,7 @@ impl MarketplaceClient { .map_err(|err| network_failure("GET", path_or_url, err))?; let status = response.status(); if is_redirect(status) { - if redirects == MAX_REDIRECTS { + if redirects == 10 { return Err(failure( exit_codes::NETWORK, "too_many_redirects", @@ -534,6 +531,25 @@ fn invalid_agent_operation(error: AgentOperationError) -> anyhow::Error { pub const LIST_PAGE_LIMIT: u32 = 5000; +fn parse_http_url(value: &str, label: &str) -> Result { + let url = Url::parse(value).with_context(|| format!("parse {label} `{value}`"))?; + ensure_http_url(&url, label)?; + Ok(url) +} + +fn ensure_http_url(url: &Url, label: &str) -> Result<()> { + if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() { + anyhow::bail!("{label} must be an absolute HTTP(S) URL: `{url}`"); + } + Ok(()) +} + +fn same_origin(left: &Url, right: &Url) -> bool { + left.scheme() == right.scheme() + && left.host_str() == right.host_str() + && left.port_or_known_default() == right.port_or_known_default() +} + fn is_redirect(status: StatusCode) -> bool { matches!( status, @@ -545,6 +561,18 @@ fn is_redirect(status: StatusCode) -> bool { ) } +fn same_origin_redirect_policy(origin: Url) -> Policy { + Policy::custom(move |attempt| { + if attempt.previous().len() > 10 { + attempt.error("too many redirects") + } else if same_origin(attempt.url(), &origin) { + attempt.follow() + } else { + attempt.error("refusing authenticated cross-origin redirect") + } + }) +} + fn network_failure(method: &str, path: &str, err: reqwest::Error) -> anyhow::Error { anyhow::Error::new(CliFailure::new( exit_codes::NETWORK, diff --git a/bb-cli/src/http_origin.rs b/bb-cli/src/http_origin.rs deleted file mode 100644 index 83de86d2c..000000000 --- a/bb-cli/src/http_origin.rs +++ /dev/null @@ -1,117 +0,0 @@ -//! Origin binding for the HTTP clients that carry BuilderBot credentials. -//! -//! Our authenticated clients install credentials as *default headers*, which -//! reqwest replays on every redirect hop. reqwest only strips the headers it -//! knows are sensitive (`Authorization`, `Cookie`, ...) when a redirect leaves -//! the origin, so custom headers such as `X-BB-Session-Credential` would -//! otherwise follow a redirect to any host the server names. Every client that -//! sends a credential therefore binds it to the origin it was issued for. - -use anyhow::{Context, Result}; -use reqwest::redirect::Policy; -use url::Url; - -/// Redirect hops an authenticated client follows before giving up. Matches -/// reqwest's own default limit. -pub const MAX_REDIRECTS: usize = 10; - -pub fn parse_http_url(value: &str, label: &str) -> Result { - let url = Url::parse(value).with_context(|| format!("parse {label} `{value}`"))?; - ensure_http_url(&url, label)?; - Ok(url) -} - -pub fn ensure_http_url(url: &Url, label: &str) -> Result<()> { - if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() { - anyhow::bail!("{label} must be an absolute HTTP(S) URL: `{url}`"); - } - Ok(()) -} - -pub fn same_origin(left: &Url, right: &Url) -> bool { - left.scheme() == right.scheme() - && left.host_str() == right.host_str() - && left.port_or_known_default() == right.port_or_known_default() -} - -/// Names an origin the way our diagnostics should: scheme, host, and port only, -/// never the path or query, which can carry request-specific data. -pub fn origin_label(url: &Url) -> String { - url.origin().ascii_serialization() -} - -/// Follows redirects only within `origin`, so a credential attached as a -/// default header can never leave the host it was issued for. -pub fn same_origin_redirect_policy(origin: Url) -> Policy { - same_origin_redirect_policy_with(origin, |_| { - "refusing authenticated cross-origin redirect".to_string() - }) -} - -/// [`same_origin_redirect_policy`] with a caller-supplied description of the -/// refusal. `describe_refusal` receives the redirect target so the caller can -/// explain the specific hop, which reqwest's redirect error cannot: it reports -/// the URL the redirect came *from*, not the one we declined to follow. -pub fn same_origin_redirect_policy_with(origin: Url, describe_refusal: F) -> Policy -where - F: Fn(&Url) -> String + Send + Sync + 'static, -{ - Policy::custom(move |attempt| { - if attempt.previous().len() > MAX_REDIRECTS { - attempt.error("too many redirects") - } else if same_origin(attempt.url(), &origin) { - attempt.follow() - } else { - let refusal = describe_refusal(attempt.url()); - attempt.error(refusal) - } - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn url(value: &str) -> Url { - Url::parse(value).expect("parse test URL") - } - - #[test] - fn same_origin_compares_scheme_host_and_effective_port() { - assert!(same_origin( - &url("https://kgoose.sqprod.co/a"), - &url("https://kgoose.sqprod.co:443/b") - )); - assert!(!same_origin( - &url("http://kgoose.sqprod.co/a"), - &url("https://kgoose.sqprod.co/a") - )); - assert!(!same_origin( - &url("https://kgoose.sqprod.co/a"), - &url("https://evil.example.com/a") - )); - assert!(!same_origin( - &url("http://127.0.0.1:1234/a"), - &url("http://127.0.0.1:4321/a") - )); - } - - #[test] - fn origin_label_omits_path_and_query() { - assert_eq!( - origin_label(&url("https://kgoose.sqprod.co/v3/call-tool?token=secret")), - "https://kgoose.sqprod.co" - ); - assert_eq!( - origin_label(&url("http://127.0.0.1:8080/v3")), - "http://127.0.0.1:8080" - ); - } - - #[test] - fn ensure_http_url_rejects_non_http_schemes_and_hostless_urls() { - assert!(ensure_http_url(&url("https://example.com"), "test URL").is_ok()); - assert!(ensure_http_url(&url("file:///etc/passwd"), "test URL").is_err()); - assert!(parse_http_url("not a url", "test URL").is_err()); - } -} diff --git a/bb-cli/src/kgoose.rs b/bb-cli/src/kgoose.rs index ed0694c4f..aa88d95fc 100644 --- a/bb-cli/src/kgoose.rs +++ b/bb-cli/src/kgoose.rs @@ -1,6 +1,5 @@ use std::collections::BTreeMap; use std::env; -use std::sync::{Arc, Mutex, PoisonError}; use std::time::Duration; use anyhow::{Context, Result}; @@ -8,11 +7,9 @@ use reqwest::blocking::{Client, ClientBuilder}; use reqwest::header::{HeaderMap, HeaderName, HeaderValue, ACCEPT, CONTENT_TYPE}; use serde::de::DeserializeOwned; use serde::Serialize; -use url::Url; use crate::bb::auth::SESSION_CREDENTIAL_HEADER; use crate::bb::skills_config::normalize_kgoose_service_path; -use crate::http_origin::{origin_label, parse_http_url, same_origin_redirect_policy_with}; pub use crate::proto::squareup::cash::kgoose::api::v3::{ CallToolRequest, CallToolResponse, ExtensionInfo, ListExtensionsRequest, ListExtensionsResponse, ListToolsRequest, ListToolsResponse, Source, ToolConfig, @@ -23,8 +20,6 @@ pub const DEFAULT_KGOOSE_BASE_URL: &str = "https://kgoose.sqprod.co"; pub const DEFAULT_KGOOSE_TIMEOUT_SECS: f64 = 600.0; const STS_ACCESS_TOKEN_ENV_VAR: &str = "STS_ACCESS_TOKEN"; const KGOOSE_DEBUG_ENV_VAR: &str = "KGOOSE_DEBUG"; -/// Registrable domain Cloudflare Access redirects to when WARP is off. -const CLOUDFLARE_ACCESS_DOMAIN: &str = "cloudflareaccess.com"; #[derive(Debug, Clone, PartialEq)] pub struct KgooseConfig { @@ -111,8 +106,7 @@ impl HttpKgooseClient { T: DeserializeOwned, B: Serialize + ?Sized, { - let base_url = parse_http_url(&config.base_url, "kgoose base URL")?; - let (client, refused_redirect) = build_http_client(config, &base_url)?; + let client = build_http_client(config)?; let service_path = normalize_kgoose_service_path(&config.service_path)?; let request_path = format!( "{}/{}", @@ -128,20 +122,13 @@ impl HttpKgooseClient { option_for_debug(config.goosemcp_playpen.as_deref()) )); - let response = match client.post(&url).json(body).send() { - Ok(response) => response, - Err(err) => { - // The slot is only filled when our redirect policy refused a - // hop, so a recorded target means the credential stayed home. - if let Some(target) = refused_redirect.take() { - return Err(cross_origin_redirect_error(&base_url, &target)); - } - return Err(err).with_context(|| format!("POST {request_path}")); - } - }; + let response = client + .post(&url) + .json(body) + .send() + .with_context(|| format!("POST {request_path}"))?; let status = response.status(); - let served_by_cloudflare_access = is_cloudflare_access(response.url()); let final_url = response.url().to_string(); let response_body = response .text() @@ -152,12 +139,9 @@ impl HttpKgooseClient { response_body.len() )); - // Backstop for a Cloudflare Access login page (indicates VPN is off). - // A redirect to Cloudflare Access is refused before the credential is - // sent, so this only fires when the configured base URL is itself a - // Cloudflare Access host. Note that Cloudflare returns 200 OK with an - // HTML login page, not an error status. - if served_by_cloudflare_access { + // Check for Cloudflare Access redirect (indicates VPN is off) + // Note: Cloudflare returns 200 OK with an HTML login page, not an error status + if final_url.contains("cloudflareaccess.com") { anyhow::bail!( "Cannot connect to kgoose - received Cloudflare Access redirect.\n\ This usually means you need to connect to the corporate VPN (WARP).\n\ @@ -175,51 +159,7 @@ impl HttpKgooseClient { } } -/// Records the redirect target the client refused to follow, so `post_json` can -/// name the host it declined to send credentials to. -#[derive(Clone, Debug, Default)] -struct RefusedRedirect(Arc>>); - -impl RefusedRedirect { - fn record(&self, url: &Url) { - *self.0.lock().unwrap_or_else(PoisonError::into_inner) = Some(url.clone()); - } - - fn take(&self) -> Option { - self.0.lock().unwrap_or_else(PoisonError::into_inner).take() - } -} - -fn is_cloudflare_access(url: &Url) -> bool { - url.host_str().is_some_and(|host| { - host == CLOUDFLARE_ACCESS_DOMAIN || host.ends_with(&format!(".{CLOUDFLARE_ACCESS_DOMAIN}")) - }) -} - -fn cross_origin_redirect_error(base_url: &Url, target: &Url) -> anyhow::Error { - let base_origin = origin_label(base_url); - let target_origin = origin_label(target); - if is_cloudflare_access(target) { - anyhow::anyhow!( - "Cannot connect to kgoose - {base_origin} redirected to Cloudflare Access \ - ({target_origin}).\n\ - This usually means you need to connect to the corporate VPN (WARP).\n\ - Please enable WARP and try again.\n\ - Your kgoose credentials were not sent to {target_origin}." - ) - } else { - anyhow::anyhow!( - "Cannot connect to kgoose - {base_origin} redirected to {target_origin}, a different \ - origin.\n\ - Your kgoose credentials were not sent to {target_origin}; point KGOOSE_BASE_URL at \ - {target_origin} directly if that host is the intended service." - ) - } -} - -/// Builds the request client alongside the slot that records a refused -/// cross-origin redirect. -fn build_http_client(config: &KgooseConfig, base_url: &Url) -> Result<(Client, RefusedRedirect)> { +fn build_http_client(config: &KgooseConfig) -> Result { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); headers.insert(ACCEPT, HeaderValue::from_static("application/json")); @@ -271,29 +211,11 @@ fn build_http_client(config: &KgooseConfig, base_url: &Url) -> Result<(Client, R .join(",") )); - // The session credential and identity token ride on every request as - // default headers, so redirects stay inside the configured origin: reqwest - // would otherwise replay them to whatever host a redirect names, which is - // exactly what happens when Cloudflare Access bounces us off-origin. - let refused_redirect = RefusedRedirect::default(); - let redirect_policy = same_origin_redirect_policy_with(base_url.clone(), { - let refused_redirect = refused_redirect.clone(); - move |target| { - refused_redirect.record(target); - format!( - "refusing to send kgoose credentials across a redirect to {}", - origin_label(target) - ) - } - }); - - let client = ClientBuilder::new() + ClientBuilder::new() .default_headers(headers) - .redirect(redirect_policy) .timeout(config.timeout()) .build() - .context("build HTTP client")?; - Ok((client, refused_redirect)) + .context("build HTTP client") } fn truncate(value: &str, max_len: usize) -> String { @@ -327,237 +249,8 @@ fn option_for_debug(value: Option<&str>) -> &str { #[cfg(test)] mod tests { - use std::io::{BufRead, BufReader, Read, Write}; - use std::net::{TcpListener, TcpStream}; - - use super::*; - use crate::bb::skills_config::DEFAULT_KGOOSE_SERVICE_PATH; + use super::{CallToolResponse, ListExtensionsResponse, ListToolsResponse}; use crate::proto::squareup::cash::kgoose::api::v3::user_content; - use crate::test_server::{prepare_stream, ServerThread}; - - const TEST_SESSION_CREDENTIAL: &str = "test-session-credential"; - - #[derive(Clone, Debug)] - struct RecordedRequest { - path: String, - headers: HeaderMap, - } - - impl RecordedRequest { - fn session_credential(&self) -> Option<&str> { - self.headers - .get(SESSION_CREDENTIAL_HEADER) - .and_then(|value| value.to_str().ok()) - } - } - - /// Minimal HTTP server for the redirect tests, over the shared - /// [`ServerThread`] so a test that expects *no* request cannot hang waiting - /// for one that never arrives. - struct TestServer { - base_url: String, - requests: Arc>>, - _thread: ServerThread, - } - - impl TestServer { - fn start(responses: Vec) -> Self { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind test server"); - let base_url = format!("http://{}", listener.local_addr().expect("server address")); - let requests = Arc::new(Mutex::new(Vec::new())); - let thread_requests = Arc::clone(&requests); - let thread = ServerThread::spawn(listener, responses, move |stream, response| { - let response = response.unwrap_or_else(unexpected_request_response); - record_and_respond(stream, &thread_requests, &response); - }); - Self { - base_url, - requests, - _thread: thread, - } - } - - /// Requests recorded so far. Each request is recorded before its - /// response is written, so every hop the client saw completed is - /// already here by the time the client call returns. - fn requests(&self) -> Vec { - self.requests.lock().expect("lock requests").clone() - } - } - - fn record_and_respond( - stream: TcpStream, - requests: &Arc>>, - response: &str, - ) { - prepare_stream(&stream); - let mut reader = BufReader::new(stream.try_clone().expect("clone test stream")); - let mut request_line = String::new(); - reader - .read_line(&mut request_line) - .expect("read request line"); - let path = request_line - .split_whitespace() - .nth(1) - .expect("request path") - .to_string(); - let mut headers = HeaderMap::new(); - let mut content_length = 0usize; - loop { - let mut line = String::new(); - reader.read_line(&mut line).expect("read request header"); - if line == "\r\n" || line.is_empty() { - break; - } - if let Some((name, value)) = line.split_once(':') { - if name.eq_ignore_ascii_case("content-length") { - content_length = value.trim().parse().expect("content length"); - } - headers.insert( - HeaderName::from_bytes(name.as_bytes()).expect("valid header name"), - HeaderValue::from_str(value.trim()).expect("valid header value"), - ); - } - } - let mut body = vec![0; content_length]; - reader.read_exact(&mut body).expect("read request body"); - requests - .lock() - .expect("lock requests") - .push(RecordedRequest { path, headers }); - let mut stream = stream; - stream - .write_all(response.as_bytes()) - .expect("write test response"); - } - - fn json_response(body: &str) -> String { - format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", - body.len() - ) - } - - /// Served for a request the test queued no response for, so that request is - /// still recorded rather than dropped. - fn unexpected_request_response() -> String { - "HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" - .to_string() - } - - /// 307 keeps the method and body, so the retried hop is another authenticated POST. - fn redirect_response(location: &str) -> String { - format!( - "HTTP/1.1 307 Temporary Redirect\r\nLocation: {location}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" - ) - } - - fn test_config(base_url: &str) -> KgooseConfig { - KgooseConfig { - base_url: base_url.to_string(), - service_path: DEFAULT_KGOOSE_SERVICE_PATH.to_string(), - playpen: None, - goosemcp_playpen: None, - timeout_secs: 5.0, - session_credential: Some(TEST_SESSION_CREDENTIAL.to_string()), - } - } - - fn list_extensions(config: &KgooseConfig) -> Result { - HttpKgooseClient.post_json(config, LIST_EXTENSIONS_PATH, &ListExtensionsRequest {}) - } - - #[test] - fn post_json_keeps_credential_across_same_origin_redirect() { - let server = TestServer::start(vec![ - redirect_response("/cash-app/goose/v3/list-extensions?retry=1"), - json_response("{}"), - ]); - - let response = - list_extensions(&test_config(&server.base_url)).expect("follow same-origin redirect"); - - assert!(response.extensions.is_empty()); - let requests = server.requests(); - assert_eq!(requests.len(), 2); - assert_eq!( - requests[1].path, - "/cash-app/goose/v3/list-extensions?retry=1" - ); - for request in &requests { - assert_eq!( - request.session_credential(), - Some(TEST_SESSION_CREDENTIAL), - "same-origin hop {} lost the credential", - request.path - ); - } - } - - #[test] - fn post_json_refuses_cross_origin_redirect_without_sending_credential() { - // The destination serves what it would serve in the attack it stands in - // for, so a client that followed the hop fails on `expect_err` rather - // than on some incidental error from an unserved request. - let destination = TestServer::start(vec![json_response("{}")]); - let kgoose = TestServer::start(vec![redirect_response(&format!( - "{}/cash-app/goose/v3/list-extensions", - destination.base_url - ))]); - - let error = - list_extensions(&test_config(&kgoose.base_url)).expect_err("refuse cross-origin hop"); - - let message = format!("{error:#}"); - assert!(message.contains(&destination.base_url), "{message}"); - assert!(message.contains("credentials were not sent"), "{message}"); - assert_eq!(kgoose.requests().len(), 1); - assert!( - destination.requests().is_empty(), - "credential-bearing request reached the redirect target" - ); - } - - #[test] - fn post_json_reports_cloudflare_access_redirect_as_vpn_hint() { - let server = TestServer::start(vec![redirect_response( - "https://block.cloudflareaccess.com/cdn-cgi/access/login/kgoose.sqprod.co", - )]); - - let error = list_extensions(&test_config(&server.base_url)) - .expect_err("refuse Cloudflare Access hop"); - - let message = format!("{error:#}"); - assert!(message.contains("WARP"), "{message}"); - assert!( - message.contains("https://block.cloudflareaccess.com"), - "{message}" - ); - assert!(message.contains("credentials were not sent"), "{message}"); - } - - #[test] - fn cloudflare_access_detection_requires_the_real_domain() { - for host in [ - "https://block.cloudflareaccess.com/login", - "https://cloudflareaccess.com/login", - ] { - assert!( - is_cloudflare_access(&Url::parse(host).expect("parse URL")), - "{host}" - ); - } - for host in [ - "https://notcloudflareaccess.com/login", - "https://cloudflareaccess.com.evil.example/login", - "https://kgoose.sqprod.co/cash-app/goose", - ] { - assert!( - !is_cloudflare_access(&Url::parse(host).expect("parse URL")), - "{host}" - ); - } - } #[test] fn list_tools_response_deserializes_generated_proto_shape() { diff --git a/bb-cli/src/lib.rs b/bb-cli/src/lib.rs index b665ba733..2b2f3ebee 100644 --- a/bb-cli/src/lib.rs +++ b/bb-cli/src/lib.rs @@ -2,7 +2,6 @@ mod appkit; mod bb; mod catalog; mod cli; -mod http_origin; mod kgoose; mod proto; mod runtime;