From 2ff0f8fe1dadc20cd6d099a1e317c62b0a0b97af Mon Sep 17 00:00:00 2001 From: Ben Younes Date: Thu, 14 May 2026 11:15:19 +0200 Subject: [PATCH] fix(cdp): read and validate contextId in Runtime.evaluate / callFunctionOn (#51) (#149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #51. `Runtime.evaluate` and `Runtime.callFunctionOn` previously ignored the `contextId` / `executionContextId` parameter entirely, so clients targeting an isolated world (e.g. Playwright's locator path, which calls `Page.createIsolatedWorld` → contextId 100 → evaluates the UtilityScript against that id) silently fell through to the default context, breaking the contract advertised via Runtime.executionContextCreated. Track the set of advertised execution context ids on CdpContext (pre-seeded with 1 and 2 — the default-frame contexts emitted by Runtime.enable and post-navigation) and extend it from Page.createIsolatedWorld. Both handlers now reject unknown ids with `Cannot find context with specified id: ` — matching the wording real Chrome returns — and log accepted ids at debug for observability. Obscura still routes every JS call against the page's single V8 isolate, so this PR is spec-compliance scaffolding rather than full multi-world execution. Follow-up work to physically isolate worlds can build on the same valid_context_ids registry. --- crates/obscura-cdp/src/dispatch.rs | 22 +++- crates/obscura-cdp/src/domains/page.rs | 5 +- crates/obscura-cdp/src/domains/runtime.rs | 148 ++++++++++++++++++++++ 3 files changed, 173 insertions(+), 2 deletions(-) diff --git a/crates/obscura-cdp/src/dispatch.rs b/crates/obscura-cdp/src/dispatch.rs index 5853f51..9a50eb7 100644 --- a/crates/obscura-cdp/src/dispatch.rs +++ b/crates/obscura-cdp/src/dispatch.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use obscura_browser::{BrowserContext, Page}; @@ -25,6 +25,16 @@ pub struct CdpContext { // back. Stored as plain Strings (not by-page) — for now we only model // a single page in CdpContext anyway. pub isolated_worlds: Vec, + // Set of executionContextIds Obscura has emitted via + // Runtime.executionContextCreated. Pre-populated with the default-frame + // contexts (`1`, `2`) that Runtime.enable / Page.navigate emit, then + // extended each time Page.createIsolatedWorld assigns a fresh id. + // + // Runtime.evaluate / Runtime.callFunctionOn consult this set to reject + // requests targeting an unknown context — matching real Chrome's + // "Cannot find context with specified id" CDP error and unblocking the + // Playwright locator path described in issue #51. + pub valid_context_ids: HashSet, pub fetch_intercept: FetchInterceptState, pub intercept_tx: Option>, } @@ -53,6 +63,15 @@ impl CdpContext { stealth, user_agent, )); + // Pre-seed with the default-frame execution context ids that + // `Runtime.enable` (1) and post-navigation re-emission (2) advertise + // via Runtime.executionContextCreated. Anything else has to be + // registered explicitly (Page.createIsolatedWorld), otherwise + // Runtime.{evaluate,callFunctionOn} should reject it per CDP spec. + let mut valid_context_ids = HashSet::new(); + valid_context_ids.insert(1); + valid_context_ids.insert(2); + CdpContext { pages: Vec::new(), sessions: HashMap::new(), @@ -64,6 +83,7 @@ impl CdpContext { fetch_intercept: FetchInterceptState::new(), intercept_tx: None, isolated_worlds: Vec::new(), + valid_context_ids, } } diff --git a/crates/obscura-cdp/src/domains/page.rs b/crates/obscura-cdp/src/domains/page.rs index 5208a66..9c46d0e 100644 --- a/crates/obscura-cdp/src/domains/page.rs +++ b/crates/obscura-cdp/src/domains/page.rs @@ -177,7 +177,7 @@ pub async fn handle( .unwrap_or("").to_string(); let page_url = page.url_string(); let page_id = page.id.clone(); - let context_id = 100; + let context_id: i64 = 100; // Track this world so Page.navigate can re-emit a context for it // post-navigation. Without this, Playwright (and Puppeteer) // hang in any operation that uses the utility world — including @@ -186,6 +186,9 @@ pub async fn handle( if !world_name.is_empty() && !ctx.isolated_worlds.contains(&world_name) { ctx.isolated_worlds.push(world_name.clone()); } + // Register the isolated world's id so Runtime.evaluate / + // Runtime.callFunctionOn accept it as a valid contextId (#51). + ctx.valid_context_ids.insert(context_id); ctx.pending_events.push(CdpEvent { method: "Runtime.executionContextCreated".to_string(), diff --git a/crates/obscura-cdp/src/domains/runtime.rs b/crates/obscura-cdp/src/domains/runtime.rs index 5feb664..025091e 100644 --- a/crates/obscura-cdp/src/domains/runtime.rs +++ b/crates/obscura-cdp/src/domains/runtime.rs @@ -42,6 +42,15 @@ pub async fn handle( .and_then(|v| v.as_bool()) .unwrap_or(false); + // Spec-compliant contextId handling (#51): when the client + // targets a specific execution context, reject the call if that + // context has not been advertised via Runtime.executionContextCreated + // (or Page.createIsolatedWorld). Obscura routes every evaluate + // against the page's single V8 isolate, so accepting a foreign + // id silently would mis-attribute UtilityScript state and break + // Playwright's locator path. + validate_context_id(params, "contextId", ctx, "evaluate")?; + let page = ctx .get_session_page_mut(session_id) .ok_or("No page")?; @@ -69,6 +78,13 @@ pub async fn handle( .map(|a| a.to_vec()) .unwrap_or_default(); + // #51: validate executionContextId the same way Runtime.evaluate + // does. CDP names this field `executionContextId` on + // callFunctionOn (not `contextId`); a request may omit it when + // `objectId` is supplied — in that case validate_context_id is a + // no-op and the default context is used. + validate_context_id(params, "executionContextId", ctx, "callFunctionOn")?; + let page = ctx .get_session_page_mut(session_id) .ok_or("No page")?; @@ -186,6 +202,35 @@ pub async fn handle( } } +/// Reject `Runtime.{evaluate,callFunctionOn}` calls that target an execution +/// context Obscura has not advertised. Returns `Ok(())` when the parameter is +/// absent (defaulting to the page's default context) or when the id matches +/// one of `ctx.valid_context_ids`. Logs a debug trace on accept for #51. +fn validate_context_id( + params: &Value, + field: &str, + ctx: &crate::dispatch::CdpContext, + method: &str, +) -> Result<(), String> { + let Some(id) = params.get(field).and_then(|v| v.as_i64()) else { + return Ok(()); + }; + if !ctx.valid_context_ids.contains(&id) { + return Err(format!( + "Cannot find context with specified id: {}", + id + )); + } + tracing::debug!( + target: "obscura_cdp::runtime", + "Runtime.{}: {}={} (single-isolate routing)", + method, + field, + id + ); + Ok(()) +} + fn remote_object_from_info(info: &RemoteObjectInfo) -> Value { let mut obj = json!({ "type": info.js_type }); @@ -211,3 +256,106 @@ fn remote_object_from_info(info: &RemoteObjectInfo) -> Value { obj } + +#[cfg(test)] +mod tests { + use super::*; + use crate::dispatch::CdpContext; + + // Issue #51 — Runtime.evaluate / callFunctionOn must read and validate + // contextId. Pre-fix the parameter was silently dropped, so Playwright's + // locator (which targets the utility world created by + // Page.createIsolatedWorld) ran in the wrong context and timed out. + // + // Phase 5.5 (RED-then-GREEN) verification: + // - Without the prod fix, `valid_context_ids` does not exist on + // CdpContext → these tests fail to compile. + // - With the prod fix, all four tests pass. + + #[tokio::test] + async fn evaluate_rejects_unknown_context_id() { + let mut ctx = CdpContext::new(); + let err = handle( + "evaluate", + &json!({ "expression": "1 + 1", "contextId": 9999 }), + &mut ctx, + &None, + ) + .await + .expect_err("unknown contextId must error per CDP spec"); + assert!( + err.contains("Cannot find context with specified id"), + "error must match real Chrome's wording: {err}" + ); + assert!(err.contains("9999"), "error must include the bad id: {err}"); + } + + #[tokio::test] + async fn call_function_on_rejects_unknown_execution_context_id() { + let mut ctx = CdpContext::new(); + let err = handle( + "callFunctionOn", + &json!({ + "functionDeclaration": "() => 42", + "executionContextId": 9999, + }), + &mut ctx, + &None, + ) + .await + .expect_err("unknown executionContextId must error per CDP spec"); + assert!( + err.contains("Cannot find context with specified id"), + "error must match Chrome wording: {err}" + ); + } + + #[tokio::test] + async fn evaluate_accepts_default_context_id_one() { + // Runtime.enable advertises contextId=1 — that must be accepted as + // valid input to evaluate, regardless of whether a page is attached. + // (Without a page we get an Err("No page") AFTER the contextId check, + // which proves validation passed for id=1.) + let mut ctx = CdpContext::new(); + let result = handle( + "evaluate", + &json!({ "expression": "1 + 1", "contextId": 1 }), + &mut ctx, + &None, + ) + .await; + match result { + Ok(_) => {} // accepted + executed (would happen if a page is attached) + Err(e) => assert!( + !e.contains("Cannot find context"), + "contextId=1 must be accepted, got: {e}" + ), + } + } + + #[tokio::test] + async fn create_isolated_world_registers_id_for_evaluate() { + // Round-trip: Page.createIsolatedWorld returns contextId N, and a + // subsequent Runtime.evaluate targeting that contextId must NOT be + // rejected. + let mut ctx = CdpContext::new(); + // Bypass the page-attached path of createIsolatedWorld by direct + // insert — mirrors the same effect as calling the page handler with + // a real session. + ctx.valid_context_ids.insert(100); + + let result = handle( + "evaluate", + &json!({ "expression": "1 + 1", "contextId": 100 }), + &mut ctx, + &None, + ) + .await; + if let Err(e) = result { + assert!( + !e.contains("Cannot find context"), + "registered isolated-world contextId=100 must be accepted, got: {e}" + ); + } + } +}