Merge pull request #456 from h4ckf0r0day/fix/cdp-context-isolation-449

Isolate CDP browser contexts

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");
+    }
 }

crates/obscura-cdp/src/dispatch.rs

@@ -14,7 +14,9 @@ pub struct CdpContext {
     pub sessions: HashMap<String, String>, // session_id -> page_id
     pub pending_events: Vec<CdpEvent>,
     pub default_context: Arc<BrowserContext>,
+    pub browser_contexts: HashMap<String, Arc<BrowserContext>>,
     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
+    /// Build a CDP context around an already-constructed default browser
-    /// [`BrowserContext`]. The thread-per-connection server (issue #430 /
+    /// context. The server passes a fresh isolated context per WebSocket; tests
-    /// "Option 2") gives each connection its own `CdpContext` on its own OS
+    /// and embedders may construct their own.
-    /// 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<BrowserContext>` to every connection's context does that.
     pub fn new_with_shared_context(default_context: Arc<BrowserContext>) -> 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<String, String> {
+        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<BrowserContext>> {
+        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<Vec<String>, 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<String> = 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> {

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()
         );

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<String>,
+) -> Result<std::sync::Arc<obscura_net::CookieJar>, 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<String>,
+    session_id: &Option<String>,
 ) -> Result<Value, String> {
     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<Value> = 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(),

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();
+            let id = ctx.create_browser_context();
-            Ok(json!({ "browserContextId": ctx.default_context.id }))
+            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();

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
+    // This context is a configuration and persistence template. Each WebSocket
-    // reuses it (one cookie jar, one HTTP client) while running on its own OS
+    // gets an isolated copy with its own cookie jar and HTTP client (#449),
-    // thread, so each page's V8 isolate is confined to a single thread and the
+    // while the thread-per-connection layout from #430 still confines that
-    // #430 cross-page abort cannot happen (V8's TryGetCurrent check is
+    // connection's V8 isolates to one OS thread.
-    // per-thread). This is the thread-per-runtime fix for #430.
     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<obscura_browser::BrowserContext>,
+    context_template: Arc<obscura_browser::BrowserContext>,
+    persistence_context: Arc<obscura_browser::BrowserContext>,
+    persistence_lock: Arc<std::sync::Mutex<()>>,
     shutdown_notify: Arc<Notify>,
     live_connections: Arc<AtomicUsize>,
 ) {
@@ -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<BrowserContext>` (one cookie jar, one HTTP client). The shared
+/// own isolated `BrowserContext` (cookie jar and HTTP client). Cookie deltas are
-/// context's cookies are persisted once by the accept side on shutdown.
+/// merged into the persistence template when the connection thread exits.
 async fn cdp_processor(
     mut rx: mpsc::UnboundedReceiver<ServerMessage>,
     default_context: Arc<obscura_browser::BrowserContext>,
@@ -692,9 +767,8 @@ async fn cdp_processor(
 
     }
 
-    // Cookies live in the shared BrowserContext and are persisted once by the
+    // The connection thread merges this context's cookie delta into the
-    // accept side when the whole server shuts down, so a single connection
+    // persistence template after the processor stops.
-    // closing (or its processor being aborted) does not need to save here.
     let _ = &ctx;
 }
 
@@ -1172,15 +1246,6 @@ fn fast_path_response(text: &str) -> Option<String> {
         "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, &current);
+
+        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