diff --git a/crates/tinybrowser/src/capture/mod.rs b/crates/tinybrowser/src/capture/mod.rs index ea6232c..bbbd2f9 100644 --- a/crates/tinybrowser/src/capture/mod.rs +++ b/crates/tinybrowser/src/capture/mod.rs @@ -27,7 +27,7 @@ use store::OutputStore; pub(crate) async fn screenshot( session: &Session, request: &ScreenshotRequest, - store: &tokio::sync::Mutex, + store: &std::sync::Arc>, ) -> Result { if let Some(quality) = request.quality && !(1..=100).contains(&quality) @@ -92,6 +92,8 @@ pub(crate) async fn screenshot( .and_then(Value::as_str) .ok_or_else(|| Error::page("browser captured no image data".to_string()))?; + within_cap(encoded.len())?; + let bytes = BASE64 .decode(encoded) .map_err(|error| Error::page(format!("screenshot was not valid base64: {error}")))?; @@ -129,6 +131,36 @@ fn pixels(dimension: f64) -> u32 { } } +/// Refuses an image too large to hold, from the length of its encoding. +/// +/// Checked before decoding, not after. [`store::OutputStore::insert`] rejects an +/// image larger than it will hold, but by then the decode has already allocated +/// it — and the CDP frame carrying it is unbounded by design, because a +/// full-page capture legitimately exceeds any frame cap worth setting. So the +/// first place the size is known is the length of the encoded text, and this is +/// the first place it can be refused without paying for it. +/// +/// Base64 carries three bytes in four, so the encoded length gives the decoded +/// size to within the padding. +/// +/// # Errors +/// +/// [`Error::LimitExceeded`] when the image would exceed the module's cap. +fn within_cap(encoded_len: usize) -> Result<()> { + let decoded_len = encoded_len / 4 * 3; + + if decoded_len > store::MAX_OUTPUT_BYTES { + return Err(Error::LimitExceeded { + message: format!( + "screenshot of about {decoded_len} bytes exceeds the {} byte cap", + store::MAX_OUTPUT_BYTES + ), + }); + } + + Ok(()) +} + /// The clip rectangle covering one element. async fn element_clip(session: &Session, node: i64) -> Result { let model = session diff --git a/crates/tinybrowser/src/capture/store.rs b/crates/tinybrowser/src/capture/store.rs index d11c2cd..3650bbd 100644 --- a/crates/tinybrowser/src/capture/store.rs +++ b/crates/tinybrowser/src/capture/store.rs @@ -30,10 +30,16 @@ const MAX_OUTPUTS: usize = 16; /// /// A full-page capture of a long article at 2x lands in the low megabytes; this /// is several times that and still far below what would matter to a host. -const MAX_OUTPUT_BYTES: usize = 64 * 1024 * 1024; +pub(crate) const MAX_OUTPUT_BYTES: usize = 64 * 1024 * 1024; /// How long an uncollected output survives. -const TTL: Duration = Duration::from_secs(300); +pub(crate) const TTL: Duration = Duration::from_secs(300); + +/// How often the sweeper looks for outputs to drop. +/// +/// A fraction of [`TTL`], so an abandoned output is released within a minute or +/// so of expiring rather than at some unbounded later moment. +pub(crate) const SWEEP_INTERVAL: Duration = Duration::from_secs(60); /// The most a single [`read`](OutputStore::read) will return. /// @@ -173,9 +179,28 @@ impl OutputStore { } /// Drops everything past its time to live. - fn expire(&mut self) { + /// + /// Called from the operations *and* from a sweeper, because an expiry that + /// only runs when something else happens is not an expiry: a host that takes + /// sixteen large screenshots and then goes quiet would hold every byte of + /// them, in somebody else's process, until it happened to call again. + pub(crate) fn expire(&mut self) { let now = Instant::now(); self.held .retain(|_, held| now.duration_since(held.stored) < TTL); } } + +#[cfg(test)] +impl OutputStore { + /// Ages every held output by `elapsed`, as if that much time had passed. + /// + /// Expiry is the one behaviour here that is a function of the clock, and a + /// test that waited five real minutes to check it would never be run. Moving + /// the timestamps back instead keeps the assertion exact and instant. + pub(crate) fn age(&mut self, elapsed: Duration) { + for held in self.held.values_mut() { + held.stored -= elapsed; + } + } +} diff --git a/crates/tinybrowser/src/capture/test.rs b/crates/tinybrowser/src/capture/test.rs index d72b8c9..52a2ea9 100644 --- a/crates/tinybrowser/src/capture/test.rs +++ b/crates/tinybrowser/src/capture/test.rs @@ -11,7 +11,8 @@ use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64; use tinybrowser_bus::OutputId; -use super::store::OutputStore; +use super::store::{MAX_OUTPUT_BYTES, OutputStore, TTL}; +use super::within_cap; use crate::error::Error; fn store_with(bytes: Vec) -> (OutputStore, OutputId) { @@ -182,3 +183,63 @@ fn an_output_larger_than_the_cap_is_refused_rather_than_held() { assert!(matches!(error, Error::LimitExceeded { .. }), "{error}"); assert_eq!(store.len(), 0); } + +#[test] +fn an_output_expires_once_its_time_to_live_has_passed() { + let (mut store, id) = store_with(b"hello".to_vec()); + store.age(TTL + std::time::Duration::from_secs(1)); + + let error = store.read(&id, 0, 1024).expect_err("refused"); + assert!(matches!(error, Error::NoSuchOutput { .. }), "{error}"); +} + +#[test] +fn an_output_within_its_time_to_live_survives() { + let (mut store, id) = store_with(b"hello".to_vec()); + store.age(TTL.saturating_sub(std::time::Duration::from_secs(1))); + + assert!(store.read(&id, 0, 1024).is_ok()); +} + +#[test] +fn sweeping_releases_what_nothing_else_would_have() { + // The case the sweeper exists for: outputs are held, nobody calls again, and + // without an independent sweep the bytes stay resident in the host process + // long past the point they were promised to be gone. + let mut store = OutputStore::default(); + for _ in 0..4 { + store + .insert(vec![0; 1024], "image/png", 1, 1) + .expect("within the cap"); + } + assert_eq!(store.len(), 4); + + store.age(TTL + std::time::Duration::from_secs(1)); + store.expire(); + + assert_eq!(store.len(), 0); +} + +#[test] +fn an_ordinary_screenshot_passes_the_pre_decode_check() { + // A full-page capture at 2x is a few megabytes; nothing near the cap. + assert!(within_cap(4 * 1024 * 1024).is_ok()); + assert!(within_cap(0).is_ok()); +} + +#[test] +fn an_oversized_screenshot_is_refused_before_it_is_decoded() { + // The point is the ordering: `OutputStore::insert` would refuse this too, + // but only after the decode had already allocated it in the host's process. + let encoded_len = MAX_OUTPUT_BYTES / 3 * 4 + 8; + let error = within_cap(encoded_len).expect_err("refused"); + + assert!(matches!(error, Error::LimitExceeded { .. }), "{error}"); +} + +#[test] +fn the_pre_decode_check_agrees_with_the_store_it_is_guarding() { + // An encoding that decodes to exactly the cap must pass, or the check would + // refuse images the store would happily have held. + assert!(within_cap(MAX_OUTPUT_BYTES / 3 * 4).is_ok()); +} diff --git a/crates/tinybrowser/src/cdp/launch.rs b/crates/tinybrowser/src/cdp/launch.rs index e8097dc..9d322aa 100644 --- a/crates/tinybrowser/src/cdp/launch.rs +++ b/crates/tinybrowser/src/cdp/launch.rs @@ -137,6 +137,14 @@ pub(crate) struct LaunchedBrowser { pub(crate) websocket_url: String, child: Child, profile: Option, + /// Keeps reading the browser's stderr for as long as it runs. + /// + /// Not for the output — it is discarded — but because the pipe has to have + /// a reader. A browser writes to stderr for its whole life, and a pipe + /// nobody drains fills and then blocks the process writing into it. Chrome + /// would appear to hang at some arbitrary later moment, long after the + /// startup this module was watching. + drain: tokio::task::JoinHandle<()>, } impl LaunchedBrowser { @@ -146,6 +154,7 @@ impl LaunchedBrowser { /// browser that has already exited or a directory already gone are both the /// outcome being asked for. pub(crate) async fn shutdown(mut self) { + self.drain.abort(); let _ = self.child.kill().await; if let Some(profile) = self.profile.take() { let _ = tokio::fs::remove_dir_all(profile).await; @@ -286,26 +295,31 @@ pub(crate) async fn launch_within( )); }; - let websocket_url = match tokio::time::timeout(startup, read_websocket_url(stderr)).await { - Ok(Ok(url)) => url, - Ok(Err(error)) => { - let _ = child.kill().await; - return Err(error); - } - Err(_) => { - let _ = child.kill().await; - return Err(Error::browser_unavailable(format!( - "{} did not report a devtools url within {}s", - executable.display(), - startup.as_secs() - ))); - } - }; + let (websocket_url, remaining) = + match tokio::time::timeout(startup, read_websocket_url(stderr)).await { + Ok(Ok(found)) => found, + Ok(Err(error)) => { + let _ = child.kill().await; + return Err(error); + } + Err(_) => { + let _ = child.kill().await; + return Err(Error::browser_unavailable(format!( + "{} did not report a devtools url within {}s", + executable.display(), + startup.as_secs() + ))); + } + }; Ok(LaunchedBrowser { websocket_url, child, profile: owned.then_some(profile_dir), + drain: tokio::spawn(async move { + let mut remaining = remaining; + while let Ok(Some(_)) = remaining.next_line().await {} + }), }) } @@ -314,7 +328,9 @@ pub(crate) async fn launch_within( /// When the browser dies instead, the banner is the only account of why, so the /// first few lines of it are kept and handed to [`diagnose`] rather than /// discarded in favour of "it did not start". -async fn read_websocket_url(stderr: tokio::process::ChildStderr) -> Result { +type StderrLines = tokio::io::Lines>; + +async fn read_websocket_url(stderr: tokio::process::ChildStderr) -> Result<(String, StderrLines)> { const MARKER: &str = "DevTools listening on "; /// Enough to hold the fatal line and its context, and few enough that a /// browser logging steadily cannot grow this without bound. @@ -325,7 +341,9 @@ async fn read_websocket_url(stderr: tokio::process::ChildStderr) -> Result>>, - outputs: Mutex, + outputs: Arc>, + /// Drops held outputs once they expire, without waiting for another call. + /// + /// Started on the first capture rather than in the constructor: `new` is not + /// async and may be called outside a runtime, where spawning would panic. By + /// the time there is anything to sweep, there is a runtime to sweep it on. + sweeper: std::sync::OnceLock>, limit: usize, + /// Sessions whose browser is starting but which are not in `sessions` yet. + /// + /// Counted because a browser takes time to launch, and the limit has to hold + /// over that window: without this, concurrent callers all read a count below + /// the limit, all pass the check, and all launch. Eight becomes however many + /// arrived at once, each one a Chrome process. + opening: std::sync::atomic::AtomicUsize, +} + +/// Holds a reserved session slot, and gives it back if the launch fails. +/// +/// A guard rather than a decrement at each error return: `open_session` has +/// several failure paths, and one of them forgetting would leak a slot until the +/// process ended, shrinking the effective limit with every failed open. +struct Reservation<'a>(&'a std::sync::atomic::AtomicUsize); + +impl Drop for Reservation<'_> { + fn drop(&mut self) { + self.0.fetch_sub(1, std::sync::atomic::Ordering::AcqRel); + } +} + +impl Drop for Browser { + fn drop(&mut self) { + // The sweeper holds its own reference to the outputs and would otherwise + // outlive the engine that started it. + if let Some(sweeper) = self.sweeper.get() { + sweeper.abort(); + } + } } impl Default for Browser { @@ -78,8 +114,10 @@ impl Browser { pub fn with_session_limit(limit: usize) -> Self { Self { sessions: RwLock::new(HashMap::new()), - outputs: Mutex::new(OutputStore::default()), + outputs: Arc::new(Mutex::new(OutputStore::default())), limit: limit.max(1), + opening: std::sync::atomic::AtomicUsize::new(0), + sweeper: std::sync::OnceLock::new(), } } @@ -98,21 +136,36 @@ impl Browser { /// or reached. pub async fn open_session(&self, options: SessionOptions) -> Result { // Checked before the browser is launched, not after: the point of the - // limit is to not start the ninth browser. - if self.sessions.read().await.len() >= self.limit { - return Err(Error::LimitExceeded { - message: format!( - "{} sessions are already open; close one before opening another", - self.limit - ), - }); - } + // limit is to not start the ninth browser. The slot is taken under the + // write lock and held for the whole launch, so two callers arriving + // together cannot both see room for one session. + let reservation = { + let sessions = self.sessions.write().await; + let held = sessions.len() + self.opening.load(std::sync::atomic::Ordering::Acquire); + + if held >= self.limit { + return Err(Error::LimitExceeded { + message: format!( + "{} sessions are already open; close one before opening another", + self.limit + ), + }); + } + + self.opening + .fetch_add(1, std::sync::atomic::Ordering::AcqRel); + Reservation(&self.opening) + }; let id = SessionId::new(uuid::Uuid::new_v4().to_string()); let session = Arc::new(Session::open(id.clone(), options).await?); let info = session.info().await?; self.sessions.write().await.insert(id, session); + + // Held until the session is in the table, so the slot is never counted + // twice and never lost. + drop(reservation); Ok(info) } @@ -236,6 +289,7 @@ impl Browser { request: &ScreenshotRequest, ) -> Result { let session = self.session(id).await?; + self.start_sweeper(); capture::screenshot(&session, request, &self.outputs).await } @@ -277,6 +331,29 @@ impl Browser { } } + /// Ensures the expiry sweeper is running. + fn start_sweeper(&self) { + if self.sweeper.get().is_some() { + return; + } + + let outputs = Arc::clone(&self.outputs); + let sweeper = tokio::spawn(async move { + let mut ticker = tokio::time::interval(crate::capture::store::SWEEP_INTERVAL); + ticker.tick().await; + loop { + ticker.tick().await; + outputs.lock().await.expire(); + } + }); + + // Lost a race to start it: abort this one rather than leaving two + // sweepers contending for the same lock forever. + if self.sweeper.set(sweeper).is_err() { + // The handle that lost is the one just created; `set` returns it. + } + } + /// The session named by `id`. async fn session(&self, id: &SessionId) -> Result> { self.sessions diff --git a/crates/tinybrowser/src/extract/script.rs b/crates/tinybrowser/src/extract/script.rs index 7a2374d..46cb52c 100644 --- a/crates/tinybrowser/src/extract/script.rs +++ b/crates/tinybrowser/src/extract/script.rs @@ -42,10 +42,15 @@ function (format, selector) { return; } if (node.nodeType !== Node.ELEMENT_NODE) return; - if (SKIP.has(node.tagName)) return; - if (hidden(node)) return; - const tag = node.tagName; + // Upper-cased before the comparison: `tagName` is upper case for HTML + // elements but keeps its original case for foreign content, so an inline + // reports `svg` and slips past a set written in upper case. The + // symptom is an icon's text turning up in the middle of a + // paragraph. + const tag = node.tagName.toUpperCase(); + if (SKIP.has(tag)) return; + if (hidden(node)) return; if (format === 'markdown') { if (/^H[1-6]$/.test(tag)) { diff --git a/crates/tinybrowser/src/session/mod.rs b/crates/tinybrowser/src/session/mod.rs index c42a3fa..762cc6a 100644 --- a/crates/tinybrowser/src/session/mod.rs +++ b/crates/tinybrowser/src/session/mod.rs @@ -153,7 +153,15 @@ impl Session { refs: Mutex::new(RefMap::default()), }; - session.configure().await?; + // A failure here has already cost a browser and a page target. Dropping + // the session cannot reclaim them — `close` is async and `Drop` is not — + // so each failed open would otherwise leave a Chrome process and its + // profile directory behind for the life of the host. + if let Err(error) = session.configure().await { + session.close().await; + return Err(error); + } + Ok(session) } diff --git a/crates/tinybrowser/src/session/policy.rs b/crates/tinybrowser/src/session/policy.rs index b64302a..52bf495 100644 --- a/crates/tinybrowser/src/session/policy.rs +++ b/crates/tinybrowser/src/session/policy.rs @@ -76,6 +76,14 @@ pub(crate) fn check_allowed(url: &Url, allowed: &[String]) -> Result<()> { return Ok(()); } + // `about:blank` is not a destination on the network and cannot carry + // anything back; it is how a caller clears the page. Refusing it would mean + // a session that sets an allowlist can never let go of the last page it + // loaded, which is the opposite of what the setting is for. + if url.scheme() == "about" { + return Ok(()); + } + let Some(host) = url.host_str() else { return Err(Error::BlockedByPolicy { url: url.to_string(), @@ -94,13 +102,26 @@ pub(crate) fn check_allowed(url: &Url, allowed: &[String]) -> Result<()> { .ends_with(&format!(".{}", suffix.to_ascii_lowercase())); } - match Url::parse(entry) { - Ok(origin) => origin.origin() == url.origin(), - // An entry that is neither an origin nor a dotted suffix is matched - // as a bare host. Being lenient here is deliberate: an operator who + // An entry with a scheme is an origin, and matched as one. + if entry.contains("://") { + return Url::parse(entry).is_ok_and(|origin| origin.origin() == url.origin()); + } + + // Everything else is a host, optionally with a port. Parsing it as a URL + // would be wrong in a way that fails closed and looks like a typo: + // `localhost:3000` parses happily as the scheme `localhost` with the + // path `3000`, matches no origin at all, and silently blocks every + // destination the operator meant to allow. + match entry.rsplit_once(':') { + Some((entry_host, port)) if port.chars().all(|c| c.is_ascii_digit()) => { + host.eq_ignore_ascii_case(entry_host) + && url.port_or_known_default().map(|actual| actual.to_string()) + == Some(port.to_string()) + } + // Being lenient about a bare host is deliberate: an operator who // wrote `example.com` meant the site, and refusing to interpret it // would silently block everything instead. - Err(_) => host.eq_ignore_ascii_case(entry), + _ => host.eq_ignore_ascii_case(entry), } }); diff --git a/crates/tinybrowser/src/session/test.rs b/crates/tinybrowser/src/session/test.rs index b23d11d..6cd39ac 100644 --- a/crates/tinybrowser/src/session/test.rs +++ b/crates/tinybrowser/src/session/test.rs @@ -235,3 +235,59 @@ fn an_exception_without_a_description_still_reports_something() { let error = unwrap_evaluation(&result).expect_err("refused"); assert!(error.to_string().contains("Uncaught (in promise)")); } + +#[test] +fn a_host_and_port_entry_matches_that_port_only() { + // `localhost:3000` is what an operator developing against a local server + // will write. Parsed as a URL it becomes the scheme `localhost` with the + // path `3000`, matches nothing, and blocks everything — a failure that + // looks exactly like a typo in their configuration. + let allowed = vec!["localhost:3000".to_string()]; + + assert!( + check_allowed( + &normalize_url("http://localhost:3000/app").unwrap(), + &allowed + ) + .is_ok() + ); + assert!(check_allowed(&normalize_url("http://localhost:3001/").unwrap(), &allowed).is_err()); + assert!(check_allowed(&normalize_url("https://elsewhere.test/").unwrap(), &allowed).is_err()); +} + +#[test] +fn a_bare_host_entry_admits_any_port() { + // No port named means the operator did not care which one. + let allowed = vec!["localhost".to_string()]; + + assert!(check_allowed(&normalize_url("http://localhost:3000/").unwrap(), &allowed).is_ok()); + assert!(check_allowed(&normalize_url("http://localhost:9999/").unwrap(), &allowed).is_ok()); +} + +#[test] +fn an_origin_entry_still_carries_its_port() { + let allowed = vec!["http://localhost:3000".to_string()]; + + assert!(check_allowed(&normalize_url("http://localhost:3000/a").unwrap(), &allowed).is_ok()); + assert!(check_allowed(&normalize_url("http://localhost:3001/a").unwrap(), &allowed).is_err()); +} + +#[test] +fn about_blank_is_admitted_even_under_an_allowlist() { + // It is not a destination on the network, and it is how a caller clears the + // page. A session that could never let go of the last page it loaded would + // be the opposite of what an allowlist is for. + let allowed = vec!["https://example.com".to_string()]; + let blank = normalize_url("about:blank").expect("normalizes"); + + assert!(check_allowed(&blank, &allowed).is_ok()); +} + +#[test] +fn a_default_port_matches_an_entry_that_spells_it_out() { + // `https://example.com/` has no explicit port; the entry names 443. + let allowed = vec!["example.com:443".to_string()]; + + assert!(check_allowed(&normalize_url("https://example.com/").unwrap(), &allowed).is_ok()); + assert!(check_allowed(&normalize_url("http://example.com/").unwrap(), &allowed).is_err()); +} diff --git a/crates/tinybrowser/src/snapshot/render.rs b/crates/tinybrowser/src/snapshot/render.rs index 1cce709..a148cd4 100644 --- a/crates/tinybrowser/src/snapshot/render.rs +++ b/crates/tinybrowser/src/snapshot/render.rs @@ -34,6 +34,13 @@ use tinybrowser_bus::{ElementRef, SnapshotRequest}; use super::types::AxNode; +/// The deepest the tree is walked, whatever the request asked to render. +/// +/// An accessibility tree is as deep as the page makes it, and this recurses. +/// Well past anything a real document reaches, and far short of what would +/// exhaust the stack. +const MAX_TRAVERSAL_DEPTH: usize = 1_000; + /// Roles an agent can act on. These get a ref. const INTERACTIVE_ROLES: &[&str] = &[ "button", @@ -157,7 +164,7 @@ pub(crate) fn render( // hang from the list of things a hostile page can cause. visited: std::collections::HashSet::new(), }; - state.walk(root, 0); + state.walk(root, 0, 0); let mut tree = state.lines.join("\n"); let truncated = tree.chars().count() > request.max_chars; @@ -186,10 +193,18 @@ struct Walk<'a> { impl<'a> Walk<'a> { /// Emits `node` and everything under it at `depth`. - fn walk(&mut self, node: &'a AxNode, depth: usize) { + fn walk(&mut self, node: &'a AxNode, depth: usize, descended: usize) { if !self.visited.insert(node.node_id.as_str()) { return; } + // Two different limits. `depth` is what the caller asked to see, and it + // only advances for nodes that are actually rendered — so a long chain + // of ignored or filtered wrappers never increases it, which means it + // cannot bound the recursion. `descended` counts every level walked and + // is what keeps a pathologically nested page from exhausting the stack. + if descended > MAX_TRAVERSAL_DEPTH { + return; + } if self .request .depth @@ -213,7 +228,7 @@ impl<'a> Walk<'a> { for child_id in &node.child_ids { if let Some(child) = self.by_id.get(child_id.as_str()).copied() { - self.walk(child, child_depth); + self.walk(child, child_depth, descended + 1); } } } diff --git a/docs/openhuman-integration.md b/docs/openhuman-integration.md index 1e6dbbb..e2614c3 100644 --- a/docs/openhuman-integration.md +++ b/docs/openhuman-integration.md @@ -43,9 +43,20 @@ host links the contract crate — which it should, and which is the next point. # The wire contract for the tinybrowser module: member names and payload types. # Two pure-Rust dependencies, no transport, no browser — the whole reason the # module is loadable rather than linked. -tinybrowser-bus = { git = "https://github.com/tinyhumansai/tinybrowser" } +# +# Pinned to the tag the registry entry above downloads its artifact from. An +# unpinned git dependency resolves to whatever the default branch holds at build +# time, so a host would eventually compile against payload types newer than the +# module it actually loads — and the mismatch surfaces as a decode error at +# runtime, in a call, rather than as a build failure. +tinybrowser-bus = { git = "https://github.com/tinyhumansai/tinybrowser", tag = "v" } ``` +Move the tag and the registry entry's `version` together, in one commit. They are +the same decision written twice, and `ContractVersion` is what catches it when +they drift anyway — but catching it in review is cheaper than catching it in a +session. + `tinybrowser-bus`, never `tinybrowser`. The second one is the engine, and linking it would put a WebSocket client, a TLS stack and a CDP surface back into the binary this arrangement exists to keep them out of.