keep browser login alive on non-callback requests

src/providers/codex/auth/browser_login.rs

@@ -10,6 +10,7 @@ use super::pkce::{
 };
 
 const BROWSER_LOGIN_TIMEOUT: Duration = Duration::from_secs(300);
+const CALLBACK_READ_TIMEOUT: Duration = Duration::from_secs(5);
 
 // ---------------------------------------------------------------------------
 // Config
@@ -116,7 +117,25 @@ pub fn run_browser_login_with_config(
 
         match listener.accept() {
             Ok((mut stream, _)) => {
-                return handle_callback(&mut stream, &config.issuer, &redirect_uri, &pkce, &state);
+                let remaining = deadline.saturating_duration_since(std::time::Instant::now());
+                if remaining.is_zero() {
+                    anyhow::bail!("OAuth timeout");
+                }
+                let read_timeout = remaining.min(CALLBACK_READ_TIMEOUT);
+                match handle_callback_with_timeout(
+                    &mut stream,
+                    &config.issuer,
+                    &redirect_uri,
+                    &pkce,
+                    &state,
+                    read_timeout,
+                ) {
+                    // Ignore unrelated or malformed requests and keep waiting for
+                    // the actual OAuth callback.
+                    Ok(None) => continue,
+                    Ok(Some(tokens)) => return Ok(tokens),
+                    Err(e) => return Err(e),
+                }
             }
             Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
                 std::thread::sleep(Duration::from_millis(100));
@@ -128,27 +147,46 @@ pub fn run_browser_login_with_config(
     }
 }
 
+#[cfg(test)]
 fn handle_callback(
     stream: &mut TcpStream,
     issuer: &str,
     redirect_uri: &str,
     pkce: &PkceCodes,
     state: &str,
-) -> Result<TokenResponse, anyhow::Error> {
+) -> Result<Option<TokenResponse>, anyhow::Error> {
+    handle_callback_with_timeout(
+        stream,
+        issuer,
+        redirect_uri,
+        pkce,
+        state,
+        CALLBACK_READ_TIMEOUT,
+    )
+}
+
+fn handle_callback_with_timeout(
+    stream: &mut TcpStream,
+    issuer: &str,
+    redirect_uri: &str,
+    pkce: &PkceCodes,
+    state: &str,
+    read_timeout: Duration,
+) -> Result<Option<TokenResponse>, anyhow::Error> {
     let _ = stream.set_nonblocking(false);
-    let _ = stream.set_read_timeout(Some(Duration::from_secs(5)));
+    let _ = stream.set_read_timeout(Some(read_timeout));
 
     let (path, query) = match extract_request_path_and_query(stream) {
         Some(pair) => pair,
         None => {
             write_response(stream, 400, "text/plain", "Bad request");
-            anyhow::bail!("Bad request");
+            return Ok(None);
         }
     };
 
     if path != "/auth/callback" {
         write_response(stream, 404, "text/plain", "Not found");
-        anyhow::bail!("Not found");
+        return Ok(None);
     }
 
     let params = parse_query(&query);
@@ -180,7 +218,7 @@ fn handle_callback(
                 "text/html",
                 "<html><body><h1>Authorization Successful</h1><p>You can close this window.</p></body></html>",
             );
-            Ok(tokens)
+            Ok(Some(tokens))
         }
         Err(e) => {
             write_response(stream, 500, "text/plain", &e.to_string());
@@ -197,6 +235,7 @@ fn handle_callback(
 mod tests {
     use super::*;
     use crate::providers::codex::auth::test_http;
+    use std::io::{Read, Write};
     use std::sync::Arc;
 
     /// A minimal TCP-based mock auth server for browser login tests.
@@ -226,15 +265,15 @@ mod tests {
     }
 
     #[test]
-    fn browser_login_rejects_wrong_path() {
+    fn handle_callback_ignores_wrong_path() {
         let listener = TcpListener::bind("127.0.0.1:0").unwrap();
         let port = listener.local_addr().unwrap().port();
         listener.set_nonblocking(true).unwrap();
 
         let handle = std::thread::spawn(move || {
             loop {
                 if let Ok((mut stream, _)) = listener.accept() {
-                    let _ = handle_callback(
+                    let result = handle_callback(
                         &mut stream,
                         "http://fake-issuer",
                         "http://localhost:1455/auth/callback",
@@ -244,6 +283,10 @@ mod tests {
                         },
                         "test_state",
                     );
+                    assert!(
+                        matches!(result, Ok(None)),
+                        "non-callback request must return Ok(None)"
+                    );
                     break;
                 }
                 std::thread::sleep(Duration::from_millis(10));
@@ -260,6 +303,36 @@ mod tests {
         );
     }
 
+    #[test]
+    fn callback_read_timeout_bounds_silent_connections() {
+        let listener = TcpListener::bind("127.0.0.1:0").unwrap();
+        let addr = listener.local_addr().unwrap();
+        let handle = std::thread::spawn(move || {
+            let (mut stream, _) = listener.accept().unwrap();
+            handle_callback_with_timeout(
+                &mut stream,
+                "http://fake-issuer",
+                "http://localhost:1455/auth/callback",
+                &PkceCodes {
+                    verifier: "v".into(),
+                    challenge: "c".into(),
+                },
+                "test_state",
+                Duration::from_millis(100),
+            )
+        });
+
+        let started = std::time::Instant::now();
+        let _silent_client = TcpStream::connect(addr).unwrap();
+        let result = handle.join().unwrap();
+        assert!(matches!(result, Ok(None)));
+        assert!(
+            started.elapsed() < Duration::from_secs(1),
+            "silent connection exceeded its read timeout: {:?}",
+            started.elapsed()
+        );
+    }
+
     #[test]
     fn callback_rejects_wrong_state() {
         let listener = TcpListener::bind("127.0.0.1:0").unwrap();
@@ -386,9 +459,11 @@ mod tests {
                         &pkce,
                         "expected_state",
                     );
-                    let stored = result
-                        .map(|tokens| (tokens.access_token, tokens.refresh_token))
-                        .map_err(|err| err.to_string());
+                    let stored = match result {
+                        Ok(Some(tokens)) => Ok((tokens.access_token, tokens.refresh_token)),
+                        Ok(None) => Err("callback ignored non-callback request".to_string()),
+                        Err(err) => Err(err.to_string()),
+                    };
                     *callback_result_thread.lock().unwrap() = Some(stored);
                     break;
                 }
@@ -467,6 +542,62 @@ mod tests {
         drop(fail_server);
     }
 
+    #[test]
+    fn browser_login_ignores_non_callback_requests_until_oauth_error() {
+        // Malformed and unrelated requests must not terminate an in-flight login.
+        let listener = TcpListener::bind("127.0.0.1:0").unwrap();
+        let port = listener.local_addr().unwrap().port();
+        drop(listener);
+
+        let config = BrowserLoginConfig {
+            issuer: "http://fake-issuer".into(),
+            port,
+            timeout: Duration::from_secs(2),
+        };
+
+        let handle = std::thread::spawn(move || {
+            run_browser_login_with_config(&config).map_err(|err| err.to_string())
+        });
+
+        let addr: std::net::SocketAddr = format!("127.0.0.1:{port}").parse().unwrap();
+        // Wait for the login thread to bind. A bare connect is itself a
+        // non-callback probe and must not kill the login either.
+        let deadline = std::time::Instant::now() + Duration::from_secs(5);
+        loop {
+            if std::net::TcpStream::connect(addr).is_ok() {
+                break;
+            }
+            assert!(
+                std::time::Instant::now() < deadline,
+                "login thread did not bind in time"
+            );
+            std::thread::sleep(Duration::from_millis(10));
+        }
+
+        let mut malformed_stream = TcpStream::connect(addr).unwrap();
+        malformed_stream.write_all(b"GET\r\n").unwrap();
+        let mut malformed_response = String::new();
+        malformed_stream
+            .read_to_string(&mut malformed_response)
+            .unwrap();
+        assert!(
+            malformed_response.contains("400"),
+            "unexpected response: {malformed_response}"
+        );
+
+        let response = test_http::send_get(addr, "/");
+        assert!(response.contains("404"), "unexpected response: {response}");
+
+        let response = test_http::send_get(addr, "/auth/callback?error=access_denied");
+        assert!(response.contains("400"), "unexpected response: {response}");
+
+        let result = handle.join().unwrap();
+        assert!(
+            matches!(result, Err(ref err) if err.contains("access_denied")),
+            "OAuth errors must still terminate browser login: {result:?}"
+        );
+    }
+
     #[test]
     fn browser_login_times_out() {
         // Get a free port nobody is sending callbacks to