diff --git a/crates/obscura-browser/src/context.rs b/crates/obscura-browser/src/context.rs index 9be8f86..bbf710b 100644 --- a/crates/obscura-browser/src/context.rs +++ b/crates/obscura-browser/src/context.rs @@ -153,6 +153,46 @@ impl BrowserContext { Self::with_options(id, proxy_url, false) } + /// Create a context with the same browser configuration but independent + /// mutable network state. Persistent copies start with the template's + /// current cookies; incognito copies start empty and never write to the + /// template's storage directory. + pub fn isolated_copy(&self, id: String, persistent: bool) -> Self { + let cookie_jar = Arc::new(CookieJar::new()); + if persistent { + cookie_jar.set_cookies_from_cdp(self.cookie_jar.get_all_cookies()); + } + + let mut client = ObscuraHttpClient::with_full_options( + cookie_jar.clone(), + self.proxy_url.as_deref(), + self.allow_private_network, + ); + if self.stealth { + client.block_trackers = true; + } + if let Ok(mut guard) = client.user_agent.try_write() { + *guard = self.user_agent.clone(); + } + + BrowserContext { + id, + cookie_jar, + http_client: Arc::new(client), + user_agent: self.user_agent.clone(), + platform: self.platform.clone(), + ua_platform: self.ua_platform.clone(), + ua_platform_version: self.ua_platform_version.clone(), + proxy_url: self.proxy_url.clone(), + robots_cache: Arc::new(RobotsCache::new()), + obey_robots: self.obey_robots, + stealth: self.stealth, + allow_file_access: self.allow_file_access, + storage_dir: persistent.then(|| self.storage_dir.clone()).flatten(), + allow_private_network: self.allow_private_network, + } + } + /// Persist cookies to disk if storage_dir is configured. /// Called during graceful shutdown. pub fn save_cookies(&self) { @@ -204,4 +244,26 @@ mod tests { let ctx = BrowserContext::with_options("test".to_string(), None, false); assert!(ctx.user_agent.contains("Chrome")); } + + #[tokio::test(flavor = "current_thread")] + async fn isolated_copy_does_not_share_mutable_network_state() { + let source = BrowserContext::with_full_options( + "source".to_string(), + None, + false, + Some("Template-UA/1.0".to_string()), + ); + source.cookie_jar.set_cookie("sid=source", &url::Url::parse("https://example.com").unwrap()); + + let persistent = source.isolated_copy("persistent".to_string(), true); + let incognito = source.isolated_copy("incognito".to_string(), false); + + assert_eq!(persistent.cookie_jar.get_all_cookies().len(), 1); + assert!(incognito.cookie_jar.get_all_cookies().is_empty()); + persistent.cookie_jar.clear(); + persistent.http_client.set_user_agent("Changed-UA/2.0").await; + + assert_eq!(source.cookie_jar.get_all_cookies().len(), 1); + assert_eq!(source.http_client.user_agent.read().await.as_str(), "Template-UA/1.0"); + } } diff --git a/crates/obscura-cdp/src/dispatch.rs b/crates/obscura-cdp/src/dispatch.rs index a766c9f..90fd2ad 100644 --- a/crates/obscura-cdp/src/dispatch.rs +++ b/crates/obscura-cdp/src/dispatch.rs @@ -14,7 +14,9 @@ pub struct CdpContext { pub sessions: HashMap, // session_id -> page_id pub pending_events: Vec, pub default_context: Arc, + pub browser_contexts: HashMap>, page_counter: u32, + browser_context_counter: u32, pub preload_scripts: Vec<(String, String)>, // (identifier, source) pub preload_counter: u32, // World names registered via Page.createIsolatedWorld. After every @@ -100,12 +102,9 @@ impl CdpContext { Self::_new_inner(proxy, stealth, user_agent, None, allow_file_access, false) } - /// Build a context that reuses an already-constructed, shared - /// [`BrowserContext`]. The thread-per-connection server (issue #430 / - /// "Option 2") gives each connection its own `CdpContext` on its own OS - /// thread so that each page's V8 isolate is confined to one thread, but all - /// connections must still share one cookie jar and one HTTP client. Passing - /// the same `Arc` to every connection's context does that. + /// Build a CDP context around an already-constructed default browser + /// context. The server passes a fresh isolated context per WebSocket; tests + /// and embedders may construct their own. pub fn new_with_shared_context(default_context: Arc) -> Self { // Pre-seed with the default-frame execution context ids that // `Runtime.enable` (1) and post-navigation re-emission (2) advertise via @@ -120,7 +119,9 @@ impl CdpContext { sessions: HashMap::new(), pending_events: Vec::new(), default_context, + browser_contexts: HashMap::new(), page_counter: 0, + browser_context_counter: 0, preload_scripts: Vec::new(), preload_counter: 0, fetch_intercept: FetchInterceptState::new(), @@ -163,12 +164,60 @@ impl CdpContext { } pub fn create_page(&mut self) -> String { + self.create_page_in_context(None) + .expect("default browser context must exist") + } + + pub fn create_page_in_context(&mut self, context_id: Option<&str>) -> Result { + let context = match context_id { + Some(id) => self + .browser_context(id) + .cloned() + .ok_or_else(|| format!("Browser context not found: {}", id))?, + None => self.default_context.clone(), + }; self.page_counter += 1; let page_id = format!("page-{}", self.page_counter); - let mut page = Page::new(page_id.clone(), self.default_context.clone()); + let mut page = Page::new(page_id.clone(), context); page.navigate_blank(); self.pages.push(page); - page_id + Ok(page_id) + } + + pub fn browser_context(&self, id: &str) -> Option<&Arc> { + if id == self.default_context.id { + Some(&self.default_context) + } else { + self.browser_contexts.get(id) + } + } + + pub fn create_browser_context(&mut self) -> String { + self.browser_context_counter += 1; + let id = format!("context-{}", self.browser_context_counter); + let context = Arc::new(self.default_context.isolated_copy(id.clone(), false)); + self.browser_contexts.insert(id.clone(), context); + id + } + + pub fn dispose_browser_context(&mut self, id: &str) -> Result, String> { + if id == self.default_context.id { + return Err("The default browser context cannot be disposed".to_string()); + } + if self.browser_contexts.remove(id).is_none() { + return Err(format!("Browser context not found: {}", id)); + } + + let page_ids: Vec = self + .pages + .iter() + .filter(|page| page.context.id == id) + .map(|page| page.id.clone()) + .collect(); + for page_id in &page_ids { + self.remove_page(page_id); + } + Ok(page_ids) } pub fn get_page(&self, id: &str) -> Option<&Page> { diff --git a/crates/obscura-cdp/src/domains/page.rs b/crates/obscura-cdp/src/domains/page.rs index ed048d5..218e341 100644 --- a/crates/obscura-cdp/src/domains/page.rs +++ b/crates/obscura-cdp/src/domains/page.rs @@ -236,7 +236,11 @@ async fn do_navigate( // any file the obscura process can read. Opt in via // `obscura serve --allow-file-access` when local-HTML // testing is the intended workflow. - if url_is_file_scheme(url) && !ctx.default_context.allow_file_access { + let allow_file_access = ctx + .get_session_page(session_id) + .map(|page| page.context.allow_file_access) + .unwrap_or(ctx.default_context.allow_file_access); + if url_is_file_scheme(url) && !allow_file_access { return Err( "Page.navigate to file:// is disabled. Restart with `obscura serve --allow-file-access` to enable.".to_string() ); diff --git a/crates/obscura-cdp/src/domains/storage.rs b/crates/obscura-cdp/src/domains/storage.rs index 7f32d2e..91a0984 100644 --- a/crates/obscura-cdp/src/domains/storage.rs +++ b/crates/obscura-cdp/src/domains/storage.rs @@ -4,28 +4,45 @@ use crate::cookie_params::{parse_cdp_cookie, parse_delete_cookies_params}; use crate::dispatch::CdpContext; use crate::domains::network::cookie_info_to_cdp_json; +fn cookie_jar_for( + ctx: &CdpContext, + params: &Value, + session_id: &Option, +) -> Result, String> { + match params.get("browserContextId").and_then(|value| value.as_str()) { + Some(id) => ctx + .browser_context(id) + .map(|context| context.cookie_jar.clone()) + .ok_or_else(|| format!("Browser context not found: {}", id)), + None => Ok(ctx + .get_session_page(session_id) + .map(|page| page.context.cookie_jar.clone()) + .unwrap_or_else(|| ctx.default_context.cookie_jar.clone())), + } +} + pub async fn handle( method: &str, params: &Value, ctx: &mut CdpContext, - _session_id: &Option, + session_id: &Option, ) -> Result { match method { "getCookies" => { - let cookies = ctx.default_context.cookie_jar.get_all_cookies(); + let cookies = cookie_jar_for(ctx, params, session_id)?.get_all_cookies(); let cdp_cookies: Vec = cookies.iter().map(cookie_info_to_cdp_json).collect(); Ok(json!({ "cookies": cdp_cookies })) } "setCookies" => { if let Some(cookies) = params.get("cookies").and_then(|v| v.as_array()) { let parsed: Vec<_> = cookies.iter().filter_map(parse_cdp_cookie).collect(); - ctx.default_context.cookie_jar.set_cookies_from_cdp(parsed); + cookie_jar_for(ctx, params, session_id)?.set_cookies_from_cdp(parsed); } Ok(json!({})) } "deleteCookies" => { if let Some(filter) = parse_delete_cookies_params(params) { - ctx.default_context.cookie_jar.delete_cookies_filtered( + cookie_jar_for(ctx, params, session_id)?.delete_cookies_filtered( &filter.name, &filter.domain, filter.path.as_deref(), diff --git a/crates/obscura-cdp/src/domains/target.rs b/crates/obscura-cdp/src/domains/target.rs index ac3d37f..1c93b62 100644 --- a/crates/obscura-cdp/src/domains/target.rs +++ b/crates/obscura-cdp/src/domains/target.rs @@ -59,18 +59,25 @@ pub async fn handle(method: &str, params: &Value, ctx: &mut CdpContext) -> Resul } "createTarget" => { let url = params.get("url").and_then(|v| v.as_str()).unwrap_or("about:blank"); + let context_id = params.get("browserContextId").and_then(|v| v.as_str()); + let context = match context_id { + Some(id) => ctx + .browser_context(id) + .ok_or_else(|| format!("Browser context not found: {}", id))?, + None => &ctx.default_context, + }; // Same gate as Page.navigate (GHSA-q55h-vfv9-qcr5). Without this, // a CDP client can call Target.createTarget {url:"file:///etc/passwd"} // and then Runtime.evaluate the body off the created target, // bypassing the page-domain check entirely. - if url_is_file_scheme(url) && !ctx.default_context.allow_file_access { + if url_is_file_scheme(url) && !context.allow_file_access { return Err( "Target.createTarget to file:// is disabled. Restart with `obscura serve --allow-file-access` to enable.".to_string() ); } - let page_id = ctx.create_page(); + let page_id = ctx.create_page_in_context(context_id)?; let session_id = format!("{}-session", page_id); if let Some(page) = ctx.get_page_mut(&page_id) { @@ -200,14 +207,41 @@ pub async fn handle(method: &str, params: &Value, ctx: &mut CdpContext) -> Resul "detachFromTarget" => Ok(json!({})), "activateTarget" => Ok(json!({})), "getBrowserContexts" => { - Ok(json!({ "browserContextIds": [ctx.default_context.id] })) + let mut ids: Vec<&String> = ctx.browser_contexts.keys().collect(); + ids.sort(); + Ok(json!({ "browserContextIds": ids })) } "createBrowserContext" => { - ctx.default_context.cookie_jar.clear(); - Ok(json!({ "browserContextId": ctx.default_context.id })) + let id = ctx.create_browser_context(); + Ok(json!({ "browserContextId": id })) } "disposeBrowserContext" => { - ctx.default_context.cookie_jar.clear(); + let context_id = params + .get("browserContextId") + .and_then(|v| v.as_str()) + .ok_or("browserContextId required")?; + let sessions: Vec<(String, String)> = ctx + .sessions + .iter() + .filter_map(|(session_id, page_id)| { + ctx.get_page(page_id) + .filter(|page| page.context.id == context_id) + .map(|_| (session_id.clone(), page_id.clone())) + }) + .collect(); + let page_ids = ctx.dispose_browser_context(context_id)?; + for (session_id, page_id) in sessions { + ctx.pending_events.push(CdpEvent::new( + "Target.detachedFromTarget", + json!({ "sessionId": session_id, "targetId": page_id }), + )); + } + for page_id in page_ids { + ctx.pending_events.push(CdpEvent::new( + "Target.targetDestroyed", + json!({ "targetId": page_id }), + )); + } Ok(json!({})) } "getTargetInfo" => { @@ -252,6 +286,48 @@ pub async fn handle(method: &str, params: &Value, ctx: &mut CdpContext) -> Resul mod tests { use super::*; + #[tokio::test] + async fn browser_contexts_are_real_and_do_not_clear_default_cookies() { + let mut ctx = CdpContext::new(); + ctx.default_context.cookie_jar.set_cookie( + "sid=default", + &url::Url::parse("https://example.com").unwrap(), + ); + + let created = handle("createBrowserContext", &json!({}), &mut ctx) + .await + .expect("context creation should succeed"); + let context_id = created["browserContextId"].as_str().unwrap(); + assert_ne!(context_id, "default"); + assert!(ctx.browser_context(context_id).unwrap().cookie_jar.get_all_cookies().is_empty()); + assert_eq!(ctx.default_context.cookie_jar.get_all_cookies().len(), 1); + + let listed = handle("getBrowserContexts", &json!({}), &mut ctx) + .await + .expect("context listing should succeed"); + assert_eq!(listed["browserContextIds"], json!([context_id])); + } + + #[tokio::test] + async fn disposing_context_removes_only_its_pages() { + let mut ctx = CdpContext::new(); + let context_id = ctx.create_browser_context(); + let isolated_page = ctx.create_page_in_context(Some(&context_id)).unwrap(); + let default_page = ctx.create_page(); + + handle( + "disposeBrowserContext", + &json!({"browserContextId": context_id}), + &mut ctx, + ) + .await + .expect("context disposal should succeed"); + + assert!(ctx.get_page(&isolated_page).is_none()); + assert!(ctx.get_page(&default_page).is_some()); + assert!(ctx.browser_contexts.is_empty()); + } + #[tokio::test] async fn attach_to_browser_target_returns_session_id() { let mut ctx = CdpContext::new(); diff --git a/crates/obscura-cdp/src/server.rs b/crates/obscura-cdp/src/server.rs index 48102dc..473810d 100644 --- a/crates/obscura-cdp/src/server.rs +++ b/crates/obscura-cdp/src/server.rs @@ -226,11 +226,10 @@ pub async fn start_with_serve_options_and_limit( } })?; - // Build the shared browser context once. Every connection's processor - // reuses it (one cookie jar, one HTTP client) while running on its own OS - // thread, so each page's V8 isolate is confined to a single thread and the - // #430 cross-page abort cannot happen (V8's TryGetCurrent check is - // per-thread). This is the thread-per-runtime fix for #430. + // This context is a configuration and persistence template. Each WebSocket + // gets an isolated copy with its own cookie jar and HTTP client (#449), + // while the thread-per-connection layout from #430 still confines that + // connection's V8 isolates to one OS thread. let mut bctx = obscura_browser::BrowserContext::with_storage_and_network( "default".to_string(), proxy, @@ -241,6 +240,12 @@ pub async fn start_with_serve_options_and_limit( ); bctx.allow_file_access = allow_file_access; let shared_ctx = Arc::new(bctx); + // Persistence is deliberately separate from the connection template. + // Cookie deltas are merged here, but new connections always clone the + // immutable startup snapshot and can never inherit another live client's + // session state. + let persistence_ctx = Arc::new(shared_ctx.isolated_copy("persistence".to_string(), true)); + let persistence_lock = Arc::new(std::sync::Mutex::new(())); // One graceful-shutdown watcher for the whole server. It flips the accept // flag (stopping the accept thread) and wakes every connection processor via @@ -342,6 +347,8 @@ pub async fn start_with_serve_options_and_limit( run_connection( stream, shared_ctx.clone(), + persistence_ctx.clone(), + persistence_lock.clone(), shutdown_notify.clone(), live_connections.clone(), ); @@ -372,7 +379,7 @@ pub async fn start_with_serve_options_and_limit( } tokio::time::sleep(tokio::time::Duration::from_millis(20)).await; } - shared_ctx.save_cookies(); + persistence_ctx.save_cookies(); Ok(()) } @@ -427,7 +434,9 @@ fn cap_malloc_arenas() { /// plumbing is needed. fn run_connection( std_stream: std::net::TcpStream, - default_context: Arc, + context_template: Arc, + persistence_context: Arc, + persistence_lock: Arc>, shutdown_notify: Arc, live_connections: Arc, ) { @@ -447,6 +456,11 @@ fn run_connection( .name("obscura-cdp-conn".into()) .spawn(move || { let _slot = SlotGuard(slot); + let default_context = Arc::new( + context_template.isolated_copy("default".to_string(), true), + ); + let initial_cookies = default_context.cookie_jar.get_all_cookies(); + let persisted_context = default_context.clone(); let rt = match tokio::runtime::Builder::new_current_thread() .enable_all() .build() @@ -478,7 +492,21 @@ fn run_connection( // Connection closed (or shutting down): stop this connection's // processor so the thread can exit. processor.abort(); + let _ = processor.await; }); + + // Apply only this connection's cookie changes to the persistence + // template. Unchanged cookies cannot overwrite another connection's + // updates, while explicit deletes and replacements still persist. + if persistence_context.storage_dir.is_some() { + let _guard = persistence_lock.lock().unwrap_or_else(|e| e.into_inner()); + merge_cookie_delta( + &persistence_context.cookie_jar, + &initial_cookies, + &persisted_context.cookie_jar.get_all_cookies(), + ); + persistence_context.save_cookies(); + } }); // The closure never ran, so its `SlotGuard` never existed: release the @@ -489,6 +517,53 @@ fn run_connection( } } +fn cookie_key(cookie: &obscura_net::CookieInfo) -> (String, String, String) { + ( + cookie.domain.clone(), + cookie.name.clone(), + cookie.path.clone(), + ) +} + +fn cookie_values_match( + left: &obscura_net::CookieInfo, + right: &obscura_net::CookieInfo, +) -> bool { + left.value == right.value + && left.secure == right.secure + && left.http_only == right.http_only + && left.same_site == right.same_site + && left.expires == right.expires +} + +fn merge_cookie_delta( + destination: &obscura_net::CookieJar, + initial: &[obscura_net::CookieInfo], + current: &[obscura_net::CookieInfo], +) { + let initial: HashMap<_, _> = initial.iter().map(|cookie| (cookie_key(cookie), cookie)).collect(); + let current: HashMap<_, _> = current.iter().map(|cookie| (cookie_key(cookie), cookie)).collect(); + + for (key, cookie) in &initial { + if !current.contains_key(key) { + destination.delete_cookies_filtered( + &cookie.name, + &cookie.domain, + Some(&cookie.path), + ); + } + } + + let changed: Vec<_> = current + .iter() + .filter_map(|(key, cookie)| match initial.get(key) { + Some(previous) if cookie_values_match(previous, cookie) => None, + _ => Some((*cookie).clone()), + }) + .collect(); + destination.set_cookies_from_cdp(changed); +} + /// Turn away a connection that arrived while the server was at its limit. /// /// Best-effort: the socket is going away either way, so a failed write just @@ -609,8 +684,8 @@ fn handle_http_json_blocking( /// isolate is confined to a single thread. This removes the #430 abort by /// construction: V8's `heap->isolate() == Isolate::TryGetCurrent()` invariant is /// per-thread, so two connections' isolates can never collide. All processors -/// share one `Arc` (one cookie jar, one HTTP client). The shared -/// context's cookies are persisted once by the accept side on shutdown. +/// own isolated `BrowserContext` (cookie jar and HTTP client). Cookie deltas are +/// merged into the persistence template when the connection thread exits. async fn cdp_processor( mut rx: mpsc::UnboundedReceiver, default_context: Arc, @@ -692,9 +767,8 @@ async fn cdp_processor( } - // Cookies live in the shared BrowserContext and are persisted once by the - // accept side when the whole server shuts down, so a single connection - // closing (or its processor being aborted) does not need to save here. + // The connection thread merges this context's cookie delta into the + // persistence template after the processor stops. let _ = &ctx; } @@ -1172,15 +1246,6 @@ fn fast_path_response(text: &str) -> Option { "Browser.setDownloadBehavior" | "Browser.getWindowBounds" => { Some(json!({})) } - // Critical: Puppeteer calls this as the *first* CDP command on connect - // (`BrowserConnector._connectToCdpBrowser`). If another client or a long - // `Page.navigate` / interception holds the single `cdp_processor` task, - // queued Target commands starve and Puppeteer hits protocolTimeout on - // `Target.getBrowserContexts`. Fast-path bypasses the queue — same payload - // as `domains::target::handle` when default context id is `"default"`. - "Target.getBrowserContexts" => { - Some(json!({ "browserContextIds": ["default"] })) - } _ => None, }; @@ -1281,9 +1346,39 @@ async fn handle_connection_ws( #[cfg(test)] mod tests { - use super::{is_navigate_method, parse_cdp_headers}; + use super::{is_navigate_method, merge_cookie_delta, parse_cdp_headers}; + use obscura_net::{CookieInfo, CookieJar}; use serde_json::json; + fn cookie(name: &str, value: &str) -> CookieInfo { + CookieInfo { + name: name.to_string(), + value: value.to_string(), + domain: "example.com".to_string(), + path: "/".to_string(), + secure: false, + http_only: false, + same_site: "Lax".to_string(), + expires: None, + } + } + + #[test] + fn cookie_delta_merges_changes_without_reverting_other_connections() { + let destination = CookieJar::new(); + destination.set_cookies_from_cdp(vec![cookie("sid", "newer"), cookie("other", "kept")]); + let initial = vec![cookie("sid", "old"), cookie("removed", "old")]; + let current = vec![cookie("sid", "old"), cookie("added", "value")]; + + merge_cookie_delta(&destination, &initial, ¤t); + + let cookies = destination.get_all_cookies(); + assert!(cookies.iter().any(|c| c.name == "sid" && c.value == "newer")); + assert!(cookies.iter().any(|c| c.name == "other")); + assert!(cookies.iter().any(|c| c.name == "added")); + assert!(!cookies.iter().any(|c| c.name == "removed")); + } + // Issue #363: only an exact Page.navigate may take the spawn-and-defer // navigation path. A substring match also caught Page.navigateToHistoryEntry // (goBack / goForward), which has no `url` param, so it was misrouted into