diff --git a/bb-cli/src/bb/skills_api.rs b/bb-cli/src/bb/skills_api.rs index a1c22f647..3e4964954 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, }) @@ -165,7 +182,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 +191,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 +203,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 +213,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 +222,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 +357,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 +387,7 @@ impl MarketplaceClient { path: &str, status: StatusCode, body: &[u8], + marketplace_request: bool, ) -> Result<()> { if status.is_success() { return Ok(()); @@ -320,7 +395,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 +405,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 +531,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, @@ -605,17 +726,152 @@ 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); + #[derive(Clone, Debug)] + struct ArtifactRequest { + path: String, + headers: HeaderMap, + } + + struct ArtifactServer { + base_url: String, + requests: Arc>>, + _thread: ServerThread, + } + + 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 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 { + 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 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") + } + + /// 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>>, + response: &str, + ) { + prepare_stream(&stream); + 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>>, - handle: thread::JoinHandle<()>, + _thread: ServerThread, } impl TestServer { @@ -624,30 +880,36 @@ 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| { + let response = response.unwrap_or_else(|| json!({"error": "unexpected request"})); + record_and_respond(stream, &thread_requests, response); }); Self { base_url, requests, - handle, + _thread: thread, } } 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), } } - 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() } } @@ -657,6 +919,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 @@ -897,7 +1160,7 @@ mod tests { "application/zip" ); - let requests = server.finish(); + let requests = server.requests(); assert_eq!(requests[0].0, "GET"); assert_eq!( requests[0].1, @@ -961,7 +1224,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(); } } @@ -998,7 +1260,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] @@ -1033,7 +1294,221 @@ mod tests { assert_eq!(resolution.plan.version_id, "agent-v1"); assert!(resolution.artifact.is_none()); assert_eq!(resolution.installed_via, "explicit"); - 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.requests(); + assert_eq!(marketplace_requests.len(), 1); + assert!(marketplace_requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .is_some()); + assert!( + destination.requests().is_empty(), + "credential-bearing request reached the redirect target" + ); + } + + #[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.requests(); + 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.requests(); + assert!(requests[0].headers.get(SESSION_CREDENTIAL_HEADER).is_none()); + assert!( + marketplace.requests().is_empty(), + "cross-origin artifact URL was fetched through the marketplace" + ); + } + + #[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`")); + assert_eq!(artifact.requests().len(), 1); + assert!( + marketplace.requests().is_empty(), + "artifact host failure was retried against the marketplace" + ); + } + + #[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`")); + assert_eq!(artifact.requests().len(), 1); + assert!( + marketplace.requests().is_empty(), + "artifact host failure was retried against the marketplace" + ); + } + + #[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`")); + assert_eq!(server.requests().len(), 1); + } + + #[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.requests(); + 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.requests(); + assert!(marketplace_requests[0] + .headers + .get(SESSION_CREDENTIAL_HEADER) + .is_some()); + let destination_requests = destination.requests(); + 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.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()); + assert_eq!(cross_origin.requests().len(), 1); + } + + #[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")); + } + assert!( + marketplace.requests().is_empty(), + "rejected URL still produced a request" + ); } #[test] diff --git a/bb-cli/src/lib.rs b/bb-cli/src/lib.rs index c331fda34..2b2f3ebee 100644 --- a/bb-cli/src/lib.rs +++ b/bb-cli/src/lib.rs @@ -5,6 +5,8 @@ mod cli; 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..bbe433362 --- /dev/null +++ b/bb-cli/src/test_server.rs @@ -0,0 +1,76 @@ +//! 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()`. +/// +/// 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>, +} + +impl ServerThread { + pub fn spawn( + listener: TcpListener, + responses: Vec, + respond: impl Fn(TcpStream, Option) + 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, _)) => respond(stream, responses.next()), + 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"); +} 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")]);