Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 161 additions & 0 deletions code-rs/browser/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,16 @@ fn should_stop_handler<E: std::fmt::Display>(
false
}
Err(err) => {
let message = err.to_string();
let message_lower = message.to_ascii_lowercase();
// Chromiumoxide surfaces "oneshot canceled" when a request future is dropped
// (e.g., due to our own timeouts). These are expected and should not
// be treated as connection failures.
if message_lower.contains("oneshot canceled") || message_lower.contains("oneshot cancelled") {
*consecutive_errors = 0;
debug!("{label} event handler error ignored: {message}");
return false;
}
*consecutive_errors = consecutive_errors.saturating_add(1);
let count = *consecutive_errors;
if count <= HANDLER_ERROR_LIMIT {
Expand All @@ -161,6 +171,7 @@ async fn discover_ws_via_host_port(host: &str, port: u16) -> Result<String> {

let client_start = tokio::time::Instant::now();
let client = Client::builder()
.no_proxy()
.timeout(Duration::from_secs(5)) // Allow Chrome time to bring up /json/version on fresh launch
Comment on lines 172 to 175

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve proxies for non-local CDP discovery

Calling Client::builder().no_proxy() here disables all proxy use for discover_ws_via_host_port, not just localhost. If a user sets connect_host to a remote Chrome endpoint that is only reachable via HTTP(S)_PROXY (common in corporate networks), discovery will now bypass the proxy and fail to reach /json/version. The change is intended for localhost, but it regresses remote/proxied CDP setups by forcing direct connections.

Useful? React with 👍 / 👎.

.build()
.map_err(|e| BrowserError::CdpError(format!("Failed to build HTTP client: {}", e)))?;
Expand Down Expand Up @@ -248,6 +259,7 @@ async fn scan_for_chrome_debug_port() -> Option<u16> {
let test_future = async move {
let url = format!("http://127.0.0.1:{}/json/version", port);
let client = Client::builder()
.no_proxy()
.timeout(Duration::from_millis(200)) // Shorter timeout for parallel tests
.build()
.ok()?;
Expand Down Expand Up @@ -2502,7 +2514,28 @@ pub struct BrowserStatus {

#[cfg(test)]
mod tests {
use super::discover_ws_via_host_port;
use super::should_restart_handler;
use super::should_stop_handler;
use std::io::Read;
use std::io::Write;
use std::net::TcpListener;
use std::process::Command;
use std::sync::Arc;
use std::sync::atomic::AtomicBool;
use std::sync::atomic::Ordering;
use std::thread;
use std::time::Duration;
use std::time::Instant;

#[derive(Debug)]
struct TestError(&'static str);

impl std::fmt::Display for TestError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(self.0)
}
}

#[test]
fn handler_restarts_after_repeated_errors() {
Expand All @@ -2511,4 +2544,132 @@ mod tests {
assert!(!should_restart_handler(2));
assert!(should_restart_handler(3));
}

#[test]
fn handler_ignores_oneshot_cancellations() {
let mut consecutive_errors = 0u32;
for _ in 0..10 {
let should_stop = should_stop_handler(
"[test]",
Err(TestError("oneshot canceled")),
&mut consecutive_errors,
);
assert!(!should_stop);
assert_eq!(consecutive_errors, 0);
}
}

const TEST_PROXY_WS_URL: &str = "ws://proxy.invalid/devtools/browser/proxy";
const TEST_TARGET_WS_URL: &str = "ws://target.invalid/devtools/browser/target";

fn spawn_json_version_server(
ws_url: &str,
) -> (u16, Arc<AtomicBool>, thread::JoinHandle<()>) {
let listener = TcpListener::bind("127.0.0.1:0").expect("bind server");
listener
.set_nonblocking(true)
.expect("set non-blocking");
let port = listener.local_addr().expect("server addr").port();
let stop = Arc::new(AtomicBool::new(false));
let stop_thread = Arc::clone(&stop);
let ws_url = ws_url.to_string();

let handle = thread::spawn(move || {
let deadline = Instant::now() + Duration::from_secs(10);
while !stop_thread.load(Ordering::Relaxed) && Instant::now() < deadline {
match listener.accept() {
Ok((mut stream, _)) => {
let mut buffer = [0u8; 1024];
let _ = stream.read(&mut buffer);

let body = format!(r#"{{"webSocketDebuggerUrl":"{ws_url}"}}"#);
let response = format!(
"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\n\r\n{}",
body.len(),
body
);
let _ = stream.write_all(response.as_bytes());
}
Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => {
thread::sleep(Duration::from_millis(10));
}
Err(_) => break,
}
}
});

(port, stop, handle)
}

#[test]
fn cdp_discovery_ignores_proxy_env_vars() {
let (proxy_port, proxy_stop, proxy_handle) =
spawn_json_version_server(TEST_PROXY_WS_URL);
let (target_port, target_stop, target_handle) =
spawn_json_version_server(TEST_TARGET_WS_URL);

let exe = std::env::current_exe().expect("current exe");
let proxy_url = format!("http://127.0.0.1:{proxy_port}");

let output = Command::new(exe)
.arg("--exact")
.arg("manager::tests::cdp_discovery_ignores_proxy_env_vars_child")
.arg("--ignored")
.arg("--nocapture")
.env("CODE_BROWSER_TEST_TARGET_PORT", target_port.to_string())
.env("HTTP_PROXY", &proxy_url)
.env("HTTPS_PROXY", &proxy_url)
.env("ALL_PROXY", &proxy_url)
.env("http_proxy", &proxy_url)
.env("https_proxy", &proxy_url)
.env("all_proxy", &proxy_url)
.env("NO_PROXY", "")
.env("no_proxy", "")
.output()
.expect("spawn child test");

proxy_stop.store(true, Ordering::Relaxed);
target_stop.store(true, Ordering::Relaxed);
let _ = proxy_handle.join();
let _ = target_handle.join();

if !output.status.success() {
panic!(
"child test failed: status={:?}\nstdout:\n{}\nstderr:\n{}",
output.status,
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr),
);
}
}

#[ignore]
#[tokio::test]
async fn cdp_discovery_ignores_proxy_env_vars_child() {
let target_port: u16 = std::env::var("CODE_BROWSER_TEST_TARGET_PORT")
.expect("CODE_BROWSER_TEST_TARGET_PORT")
.parse()
.expect("valid port");

let url = format!("http://127.0.0.1:{target_port}/json/version");
let default_client = reqwest::Client::builder()
.timeout(Duration::from_secs(2))
.build()
.expect("build default client");
let resp = default_client
.get(&url)
.send()
.await
.expect("default request");

let proxy_version: super::JsonVersion =
resp.json().await.expect("parse proxy json");
assert_eq!(proxy_version.web_socket_debugger_url, TEST_PROXY_WS_URL);

let discovered = discover_ws_via_host_port("127.0.0.1", target_port)
.await
.expect("discover ws url");
assert_eq!(discovered, TEST_TARGET_WS_URL);
}

}
19 changes: 19 additions & 0 deletions code-rs/tui/src/history_cell/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ pub(crate) fn diff_record_from_string(title: String, diff: &str) -> DiffRecord {
(DiffLineKind::Addition, rest.to_string())
} else if let Some(rest) = line.strip_prefix('-') {
(DiffLineKind::Removal, rest.to_string())
} else if let Some(rest) = line.strip_prefix(' ') {
(DiffLineKind::Context, rest.to_string())
} else {
(DiffLineKind::Context, line)
};
Expand Down Expand Up @@ -159,4 +161,21 @@ mod tests {
assert!(!lines[0].content.contains('\u{001B}'));
assert!(!lines[1].content.contains('\u{001B}'));
}

#[test]
fn diff_record_strips_context_prefix_space() {
let diff = concat!(
"@@ -1,3 +1,3 @@\n",
" unchanged\n",
"-old\n",
"+new\n",
);
let record = diff_record_from_string(String::new(), diff);
assert_eq!(record.hunks.len(), 1);
let lines = &record.hunks[0].lines;
assert_eq!(lines.len(), 3);
assert_eq!(lines[0].content, "unchanged");
assert_eq!(lines[1].content, "old");
assert_eq!(lines[2].content, "new");
}
}
Loading