From f3c783f6f52a53d075eb8716db868487de640dbc Mon Sep 17 00:00:00 2001 From: Ben Younes Date: Thu, 14 May 2026 10:25:55 +0200 Subject: [PATCH] fix(net,js): route op_fetch_url + ES module loader through proxy_url (#139) (#151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the remaining two paths from #139: JS-level fetch()/XHR (op_fetch_url) and dynamic ES module imports (ObscuraModuleLoader) both built their reqwest clients with `Client::builder().build()` — no proxy — so every JS-initiated network call silently bypassed BrowserContext.proxy_url. (The third path, StealthHttpClient, was already fixed in main: page.rs:61 uses with_proxy.) Changes: * obscura-net: expose ObscuraHttpClient::proxy_url() so callers outside the crate can route their own reqwest requests through the same upstream proxy. The field was already there — just lacked an accessor. * obscura-js/module_loader: add `proxy_url: Option` and `with_proxy(base_url, proxy_url)`. ObscuraModuleLoader::new keeps the pre-#139 direct-connection behaviour. load() builds a reqwest client per import, threading the proxy and bubbling up "Invalid module proxy" on bad URLs. * obscura-js/ops.rs: replace the process-wide `static SHARED_HTTP_CLIENT: OnceLock` (which could never be reconfigured after first call) with a per-request `build_request_client(proxy_url)` that pulls the configured proxy out of SharedState.http_client.proxy_url() each call. Per-request build cost is ~1ms vs the network round-trip, and we no longer have a global singleton that lies about its config. * obscura-js/runtime: add ObscuraJsRuntime::with_base_url_and_proxy. with_base_url delegates to it with None so existing tests / callers are unaffected. * obscura-browser/page.rs: thread context.proxy_url through to with_base_url_and_proxy when constructing the JS runtime. --- crates/obscura-browser/src/page.rs | 9 +++- crates/obscura-js/src/module_loader.rs | 32 ++++++++++++- crates/obscura-js/src/ops.rs | 40 +++++++++++----- crates/obscura-js/src/runtime.rs | 66 +++++++++++++++++++++++++- crates/obscura-net/src/client.rs | 8 ++++ 5 files changed, 139 insertions(+), 16 deletions(-) diff --git a/crates/obscura-browser/src/page.rs b/crates/obscura-browser/src/page.rs index a7c5082..1cf0a1f 100644 --- a/crates/obscura-browser/src/page.rs +++ b/crates/obscura-browser/src/page.rs @@ -136,7 +136,14 @@ impl Page { return; } - let mut rt = ObscuraJsRuntime::with_base_url(&self.url_string()); + // Thread the BrowserContext's proxy through to the ES-module loader + // and op_fetch_url so dynamic imports and JS fetch() honour the + // configured upstream proxy (#139). When proxy_url is None this is + // equivalent to with_base_url() (direct connection). + let mut rt = ObscuraJsRuntime::with_base_url_and_proxy( + &self.url_string(), + self.context.proxy_url.clone(), + ); rt.set_url(&self.url_string()); rt.set_title(&self.title); diff --git a/crates/obscura-js/src/module_loader.rs b/crates/obscura-js/src/module_loader.rs index d8457f2..f4433de 100644 --- a/crates/obscura-js/src/module_loader.rs +++ b/crates/obscura-js/src/module_loader.rs @@ -10,12 +10,21 @@ use deno_core::RequestedModuleType; pub struct ObscuraModuleLoader { pub base_url: String, + /// Proxy URL threaded through to every dynamic ES-module fetch (#139). + /// `None` keeps the pre-#139 direct-connection behaviour for callers + /// that haven't been updated. + pub proxy_url: Option, } impl ObscuraModuleLoader { pub fn new(base_url: &str) -> Self { + Self::with_proxy(base_url, None) + } + + pub fn with_proxy(base_url: &str, proxy_url: Option) -> Self { ObscuraModuleLoader { base_url: base_url.to_string(), + proxy_url, } } } @@ -52,13 +61,32 @@ impl ModuleLoader for ObscuraModuleLoader { _requested_module_type: RequestedModuleType, ) -> ModuleLoadResponse { let url = module_specifier.to_string(); + // Capture the loader's proxy here so the async closure below owns a + // plain Option rather than borrowing &self across an `await`. + let proxy_url = self.proxy_url.clone(); ModuleLoadResponse::Async(Pin::from(Box::new(async move { - let client = reqwest::Client::builder() + let mut builder = reqwest::Client::builder(); + if let Some(ref proxy) = proxy_url { + match reqwest::Proxy::all(proxy) { + Ok(p) => builder = builder.proxy(p), + Err(e) => { + return Err(io_err(format!( + "Invalid module proxy '{}': {}", + proxy, e + ))); + } + } + } + let client = builder .build() .map_err(|e| io_err(format!("HTTP client error: {}", e)))?; - tracing::debug!("Loading ES module: {}", url); + tracing::debug!( + "Loading ES module: {} (proxy: {})", + url, + proxy_url.as_deref().unwrap_or("direct") + ); let resp = client .get(&url) diff --git a/crates/obscura-js/src/ops.rs b/crates/obscura-js/src/ops.rs index 744edee..329e27e 100644 --- a/crates/obscura-js/src/ops.rs +++ b/crates/obscura-js/src/ops.rs @@ -1,7 +1,7 @@ use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; -use std::sync::{Arc, OnceLock}; +use std::sync::Arc; use deno_core::op2; use deno_core::OpState; @@ -281,14 +281,25 @@ fn op_console_msg(state: &OpState, #[string] level: &str, #[string] msg: &str) { } } -static SHARED_HTTP_CLIENT: OnceLock = OnceLock::new(); - -fn get_shared_client() -> &'static reqwest::Client { - SHARED_HTTP_CLIENT.get_or_init(|| { - reqwest::Client::builder() - .build() - .expect("failed to build shared reqwest::Client") - }) +// op_fetch_url backs JS-level `fetch()` and XHR. Pre-#139 it used a +// process-wide `OnceLock` initialised with no proxy, so +// every JS network call bypassed the configured upstream proxy. We now +// build a client per request, threading whatever `proxy_url` the page's +// ObscuraHttpClient was configured with. +// +// The per-request build cost is negligible (≪1ms) compared with the actual +// network round-trip; the simplification is worth not having to invalidate +// a cache when the proxy is reconfigured between fetches. +fn build_request_client(proxy_url: Option<&str>) -> Result { + let mut builder = reqwest::Client::builder(); + if let Some(proxy) = proxy_url { + let p = reqwest::Proxy::all(proxy) + .map_err(|e| format!("Invalid op_fetch_url proxy '{}': {}", proxy, e))?; + builder = builder.proxy(p); + } + builder + .build() + .map_err(|e| format!("failed to build reqwest::Client: {}", e)) } #[op2(async)] @@ -317,7 +328,7 @@ async fn op_fetch_url( } } - let (cookie_jar, in_flight, intercept_tx) = { + let (cookie_jar, in_flight, intercept_tx, proxy_url) = { let state_borrow = state.borrow(); let gs = state_borrow.borrow::().clone(); let mut gs = gs.borrow_mut(); @@ -334,6 +345,10 @@ async fn op_fetch_url( } let jar = gs.cookie_jar.clone(); let in_flight = gs.http_client.as_ref().map(|c| c.in_flight.clone()); + // #139: thread the configured proxy through to the per-request + // reqwest::Client. Without this, op_fetch_url silently bypasses + // BrowserContext.proxy_url for every JS fetch() / XHR call. + let proxy_url = gs.http_client.as_ref().and_then(|c| c.proxy_url().map(|s| s.to_string())); tracing::debug!("op_fetch_url: intercept_enabled={}, has_tx={}", gs.intercept_enabled, gs.intercept_tx.is_some()); let itx = if gs.intercept_enabled { gs.intercept_counter += 1; @@ -341,7 +356,7 @@ async fn op_fetch_url( } else { None }; - (jar, in_flight, itx) + (jar, in_flight, itx, proxy_url) }; if let Some((tx, request_id)) = intercept_tx { @@ -385,7 +400,8 @@ async fn op_fetch_url( } } - let client = get_shared_client(); + let client = build_request_client(proxy_url.as_deref()) + .map_err(deno_error::JsErrorBox::generic)?; let request_origin = url::Url::parse(&url) .ok() diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index 46c0bbb..e4a6c37 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -33,10 +33,17 @@ impl ObscuraJsRuntime { } pub fn with_base_url(base_url: &str) -> Self { + Self::with_base_url_and_proxy(base_url, None) + } + + /// Construct a runtime whose ES-module loader routes dynamic imports + /// through `proxy_url` (#139). `None` is equivalent to `with_base_url` + /// (direct connection). + pub fn with_base_url_and_proxy(base_url: &str, proxy_url: Option) -> Self { let state = Rc::new(RefCell::new(ObscuraState::new())); let state_clone = state.clone(); - let module_loader = Rc::new(ObscuraModuleLoader::new(base_url)); + let module_loader = Rc::new(ObscuraModuleLoader::with_proxy(base_url, proxy_url)); let mut runtime = JsRuntime::new(RuntimeOptions { extensions: vec![build_extension()], @@ -1589,4 +1596,61 @@ mod tests { assert_eq!(inf, serde_json::Value::Null); } + // Issue #139 — proxy_url must thread through to both the ES-module + // loader (module_loader.rs) and op_fetch_url's reqwest client + // (ops.rs::build_request_client). Pre-fix both built clients with + // `Client::builder().build()` — no proxy — so JS fetch/XHR and + // dynamic imports silently bypassed BrowserContext.proxy_url. + // + // Phase 5.5 RED check: each test references a symbol that does NOT + // exist on main (proxy_url() accessor, with_proxy ctor, + // with_base_url_and_proxy ctor), so the tests fail to compile without + // the prod fix. + #[test] + fn http_client_round_trips_proxy_url() { + use obscura_net::{CookieJar, ObscuraHttpClient}; + let jar = std::sync::Arc::new(CookieJar::new()); + let configured = + ObscuraHttpClient::with_options(jar.clone(), Some("http://proxy.test:8080")); + assert_eq!( + configured.proxy_url(), + Some("http://proxy.test:8080"), + "proxy_url() must expose the value passed to with_options" + ); + + let direct = ObscuraHttpClient::with_options(jar, None); + assert_eq!( + direct.proxy_url(), + None, + "proxy_url() must return None when no proxy was configured" + ); + } + + #[test] + fn module_loader_stores_proxy_for_dynamic_imports() { + use crate::module_loader::ObscuraModuleLoader; + let loader = ObscuraModuleLoader::with_proxy( + "https://example.com/", + Some("http://proxy.test:8080".to_string()), + ); + assert_eq!(loader.proxy_url.as_deref(), Some("http://proxy.test:8080")); + assert_eq!(loader.base_url, "https://example.com/"); + + // Default constructor must keep the historical "no proxy" behaviour. + let direct = ObscuraModuleLoader::new("https://example.com/"); + assert_eq!(direct.proxy_url, None); + } + + #[test] + fn runtime_with_base_url_and_proxy_constructs_successfully() { + // Sanity-check the public ctor that page.rs uses to thread proxy + // through to the module loader. Direct (None) and proxied paths + // must both initialise the JS environment. + let _direct = ObscuraJsRuntime::with_base_url_and_proxy("https://example.com/", None); + let _proxied = ObscuraJsRuntime::with_base_url_and_proxy( + "https://example.com/", + Some("http://proxy.test:8080".to_string()), + ); + } + } diff --git a/crates/obscura-net/src/client.rs b/crates/obscura-net/src/client.rs index ee7580c..d017281 100644 --- a/crates/obscura-net/src/client.rs +++ b/crates/obscura-net/src/client.rs @@ -212,6 +212,14 @@ impl ObscuraHttpClient { }).await } + /// Read-only accessor for the proxy URL the client was configured with + /// (if any). Exposed so callers outside the `obscura-net` crate — notably + /// `op_fetch_url` in `obscura-js` (#139) — can route their own reqwest + /// requests through the same upstream proxy. + pub fn proxy_url(&self) -> Option<&str> { + self.proxy_url.as_deref() + } + pub async fn fetch(&self, url: &Url) -> Result { self.fetch_with_method(Method::GET, url, None).await }