From f8ab57359dde6b6d5de1aee613c18fe60b661aeb Mon Sep 17 00:00:00 2001 From: willwang-openai Date: Thu, 10 Sep 2026 19:42:07 +0000 Subject: [PATCH] Add manual callback input to MCP OAuth login (#44629) ## Why Allow MCP authentication to complete when the browser cannot reach the callback page by accepting the full redirect URL copied from its address bar. ## What changed - Add `codex mcp login --no-browser` to print the authorization URL and accept a pasted callback without launching a browser. HTTP callbacks remain supported while waiting for input. - Validate pasted redirect URLs and OAuth responses before exchanging tokens, hide terminal input, bound input size, and avoid echoing callback values in diagnostics. - Preserve manual input across discovered-scope retries and support cancellation during callback input and token exchange. ## Testing Add unit and CLI integration tests for callback validation, bounded input, credential storage, HTTP callback completion with stdin open, scope retries, and cancellation during token exchange. GitOrigin-RevId: 16ea9c3221f72d93e6e3e5acdc6b9465293a0a65 --- codex-rs/cli/Cargo.toml | 4 +- codex-rs/cli/src/main.rs | 1 + codex-rs/cli/src/mcp_cmd.rs | 79 +-- codex-rs/cli/src/mcp_login.rs | 265 +++++++++++ codex-rs/cli/src/mcp_login_tests.rs | 46 ++ codex-rs/cli/tests/mcp_login.rs | 448 ++++++++++++++++++ codex-rs/rmcp-client/src/lib.rs | 1 + .../rmcp-client/src/oauth_callback_input.rs | 178 +++++++ .../src/oauth_callback_input_tests.rs | 70 +++ .../rmcp-client/src/perform_oauth_login.rs | 18 +- 10 files changed, 1038 insertions(+), 72 deletions(-) create mode 100644 codex-rs/cli/src/mcp_login.rs create mode 100644 codex-rs/cli/src/mcp_login_tests.rs create mode 100644 codex-rs/cli/tests/mcp_login.rs create mode 100644 codex-rs/rmcp-client/src/oauth_callback_input.rs create mode 100644 codex-rs/rmcp-client/src/oauth_callback_input_tests.rs diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index 67e54b7dba..a03d851f35 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -73,7 +73,8 @@ codex-thread-store = { workspace = true } codex-tui = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path = { workspace = true } -crossterm = { workspace = true } +crossterm = { workspace = true, features = ["event-stream"] } +futures = { workspace = true } http = { workspace = true } libc = { workspace = true } os_info = { workspace = true } @@ -120,7 +121,6 @@ assert_matches = { workspace = true } codex-utils-cargo-bin = { workspace = true } codex-utils-pty = { workspace = true } flate2 = { workspace = true } -futures = { workspace = true } insta = { workspace = true } predicates = { workspace = true } pretty_assertions = { workspace = true } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index ee42ceb1e5..9e8e014d14 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -67,6 +67,7 @@ mod exec_server_auth; mod exec_server_telemetry; mod marketplace_cmd; mod mcp_cmd; +mod mcp_login; mod migrate_rollouts; mod plugin_cmd; mod queue_cmd; diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index eb2b70c13d..1b7c0815f5 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -24,26 +24,25 @@ use codex_exec_server::RouteAwareHttpClient; use codex_login::AuthManager; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::McpRuntimeContext; -use codex_mcp::ResolvedMcpOAuthScopes; use codex_mcp::apply_http_headers_helper; use codex_mcp::compute_auth_statuses; use codex_mcp::discover_supported_scopes; use codex_mcp::oauth_login_support; use codex_mcp::resolve_oauth_callback; use codex_mcp::resolve_oauth_scopes; -use codex_mcp::should_retry_without_scopes; use codex_protocol::protocol::McpAuthStatus; use codex_rmcp_client::McpOAuthCallbackMode; use codex_rmcp_client::McpOAuthClientRegistration; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::delete_oauth_tokens; -use codex_rmcp_client::perform_oauth_login; use codex_rmcp_client::resolve_mcp_oauth_callback_url; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::format_env_display; use crate::cloud_config; +use crate::mcp_login::McpLoginMode; +use crate::mcp_login::perform_oauth_login_retry_without_scopes; use crate::plugin_cmd::load_cli_auth_manager; /// Subcommands: @@ -197,6 +196,10 @@ pub struct LoginArgs { /// Name of the MCP server to authenticate with oauth. pub name: String, + /// Print the authorization URL and accept the callback URL without opening a browser. + #[arg(long)] + pub no_browser: bool, + /// Comma-separated list of OAuth scopes to request. #[arg(long, value_delimiter = ',', value_name = "SCOPE,SCOPE")] pub scopes: Vec, @@ -256,69 +259,6 @@ impl McpCli { } } -/// Preserve compatibility with servers that still expect the legacy empty-scope -/// OAuth request. If a discovered-scope request is rejected by the provider, -/// retry the login flow once without scopes. -#[allow(clippy::too_many_arguments)] -async fn perform_oauth_login_retry_without_scopes( - name: &str, - url: &str, - store_mode: codex_config::types::OAuthCredentialsStoreMode, - keyring_backend_kind: codex_config::types::AuthKeyringBackendKind, - http_headers: Option>, - env_http_headers: Option>, - resolved_scopes: &ResolvedMcpOAuthScopes, - oauth_client_id: Option<&str>, - client_registration: McpOAuthClientRegistration, - oauth_resource: Option<&str>, - callback_port: Option, - callback_url: Option<&str>, - global_callback_url: Option<&str>, - http_client: Arc, -) -> Result<()> { - match perform_oauth_login( - name, - url, - store_mode, - keyring_backend_kind, - http_headers.clone(), - env_http_headers.clone(), - &resolved_scopes.scopes, - oauth_client_id, - client_registration, - oauth_resource, - callback_port, - callback_url, - global_callback_url, - Arc::clone(&http_client), - ) - .await - { - Ok(()) => Ok(()), - Err(err) if should_retry_without_scopes(resolved_scopes, &err) => { - println!("OAuth provider rejected discovered scopes. Retrying without scopes…"); - perform_oauth_login( - name, - url, - store_mode, - keyring_backend_kind, - http_headers, - env_http_headers, - &[], - oauth_client_id, - client_registration, - oauth_resource, - callback_port, - callback_url, - global_callback_url, - http_client, - ) - .await - } - Err(err) => Err(err), - } -} - async fn validate_profile_v2_migration( config_overrides: &CliConfigOverrides, loader_overrides: LoaderOverrides, @@ -512,6 +452,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re .or(config.mcp_oauth_callback_url.as_deref()), config.mcp_oauth_callback_url.as_deref(), http_client, + McpLoginMode::Browser, ) .await?; println!("Successfully logged in."); @@ -572,6 +513,7 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { let LoginArgs { name, + no_browser, scopes, oauth_client_registration, } = login_args; @@ -633,6 +575,11 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { callback_url.as_deref(), config.mcp_oauth_callback_url.as_deref(), http_client, + if no_browser { + McpLoginMode::PasteCallback + } else { + McpLoginMode::Browser + }, ) .await?; println!("Successfully logged in to MCP server '{name}'."); diff --git a/codex-rs/cli/src/mcp_login.rs b/codex-rs/cli/src/mcp_login.rs new file mode 100644 index 0000000000..912fa630b4 --- /dev/null +++ b/codex-rs/cli/src/mcp_login.rs @@ -0,0 +1,265 @@ +//! CLI MCP OAuth login modes and bounded, hidden callback input. +//! +//! Manual input remains cancellable while the OAuth flow waits for its HTTP +//! callback, and dropping terminal input restores the terminal immediately. +//! Ctrl-C also cancels token exchange and scope retries in manual login mode. + +use std::collections::HashMap; +use std::io; +use std::io::BufRead; +use std::io::IsTerminal; +use std::io::Write; +use std::sync::Arc; + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use codex_config::types::AuthKeyringBackendKind; +use codex_config::types::OAuthCredentialsStoreMode; +use codex_exec_server::HttpClient; +use codex_mcp::ResolvedMcpOAuthScopes; +use codex_mcp::should_retry_without_scopes; +use codex_rmcp_client::McpOAuthClientRegistration; +use codex_rmcp_client::perform_oauth_login; +use codex_rmcp_client::perform_oauth_login_with_callback_input; +use crossterm::event::Event; +use crossterm::event::EventStream; +use crossterm::event::KeyCode; +use crossterm::event::KeyEventKind; +use crossterm::event::KeyModifiers; +use crossterm::terminal; +use futures::StreamExt; +use tokio::sync::oneshot; + +const MAX_CALLBACK_BYTES: usize = 64 * 1024; + +#[derive(Clone, Copy)] +pub(crate) enum McpLoginMode { + Browser, + PasteCallback, +} + +/// Retry provider-rejected discovered scopes once, retaining the selected input mode. +#[allow(clippy::too_many_arguments)] +pub(crate) async fn perform_oauth_login_retry_without_scopes( + name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + http_headers: Option>, + env_http_headers: Option>, + resolved_scopes: &ResolvedMcpOAuthScopes, + oauth_client_id: Option<&str>, + client_registration: McpOAuthClientRegistration, + oauth_resource: Option<&str>, + callback_port: Option, + callback_url: Option<&str>, + global_callback_url: Option<&str>, + http_client: Arc, + mode: McpLoginMode, +) -> Result<()> { + let mut pending_input = None; + // Tokio retains its process-wide signal handler, so keep listening throughout + // token exchange and scope retries after the callback input reader is dropped. + let ctrl_c = tokio::signal::ctrl_c(); + tokio::pin!(ctrl_c); + for (attempt, scopes) in [resolved_scopes.scopes.as_slice(), &[]] + .into_iter() + .enumerate() + { + let login = async { + match mode { + McpLoginMode::Browser => { + perform_oauth_login( + name, + url, + store_mode, + keyring_backend_kind, + http_headers.clone(), + env_http_headers.clone(), + scopes, + oauth_client_id, + client_registration, + oauth_resource, + callback_port, + callback_url, + global_callback_url, + Arc::clone(&http_client), + ) + .await + } + McpLoginMode::PasteCallback => { + let input = &mut pending_input; + perform_oauth_login_with_callback_input( + name, + url, + store_mode, + keyring_backend_kind, + http_headers.clone(), + env_http_headers.clone(), + scopes, + oauth_client_id, + client_registration, + oauth_resource, + callback_port, + callback_url, + global_callback_url, + Arc::clone(&http_client), + move |authorization_url| read_callback(authorization_url, input), + ) + .await + } + } + }; + let result = tokio::select! { + result = login => result, + result = &mut ctrl_c, if matches!(mode, McpLoginMode::PasteCallback) => { + result.context("failed to listen for Ctrl-C")?; + bail!("OAuth login cancelled"); + } + }; + match result { + Err(error) if attempt == 0 && should_retry_without_scopes(resolved_scopes, &error) => { + println!("OAuth provider rejected discovered scopes. Retrying without scopes…"); + } + result => return result, + } + } + unreachable!("the empty-scope attempt always returns") +} + +async fn read_callback( + authorization_url: String, + pending_input: &mut Option>>, +) -> Result { + println!( + "Authorize the MCP server by opening this URL in your browser:\n{authorization_url}\n" + ); + println!( + "After signing in, copy the full URL from your browser's address bar.\n\ + If the callback page cannot load, paste that URL here anyway." + ); + if io::stdin().is_terminal() { + return read_terminal_callback().await; + } + print_callback_prompt()?; + + // A pipe may stay open after the HTTP callback wins. A detached thread avoids + // making runtime shutdown wait for an uncancellable blocking stdin read. + // Retain an unfinished read if an HTTP provider error triggers a scope retry. + // Otherwise its detached reader could consume the next attempt's pasted URL. + if pending_input.is_none() { + let (tx, rx) = oneshot::channel(); + std::thread::Builder::new() + .name("mcp-oauth-callback-input".to_string()) + .spawn(move || { + let result = read_callback_line(io::stdin().lock()); + let _ = tx.send(result); + }) + .context("failed to start callback input reader")?; + *pending_input = Some(rx); + } + let reader = pending_input + .as_mut() + .context("callback input reader was unavailable")?; + let result = reader.await; + *pending_input = None; + result.context("callback input reader stopped")? +} + +fn read_callback_line(reader: impl BufRead) -> Result { + let mut bytes = Vec::new(); + reader + .take((MAX_CALLBACK_BYTES + 3) as u64) + .read_until(b'\n', &mut bytes) + .context("failed to read callback URL")?; + if bytes.ends_with(b"\n") { + bytes.pop(); + } + if bytes.ends_with(b"\r") { + bytes.pop(); + } + if bytes.len() > MAX_CALLBACK_BYTES { + bail!("OAuth callback URL exceeds 64 KiB"); + } + if bytes.is_empty() { + bail!("No OAuth callback URL received before input closed"); + } + String::from_utf8(bytes).map_err(|_| anyhow::anyhow!("OAuth callback URL must be valid UTF-8")) +} + +struct TerminalInputGuard; + +impl Drop for TerminalInputGuard { + fn drop(&mut self) { + let _ = terminal::disable_raw_mode(); + eprintln!(); + } +} + +fn print_callback_prompt() -> Result<()> { + eprint!("Callback URL (input hidden): "); + io::stderr() + .flush() + .context("failed to display callback prompt") +} + +async fn read_terminal_callback() -> Result { + terminal::enable_raw_mode().context("failed to hide callback input")?; + let _guard = TerminalInputGuard; + print_callback_prompt()?; + let mut events = EventStream::new(); + let mut input = String::new(); + loop { + let event = events + .next() + .await + .context("callback input stream closed")? + .context("failed to read callback input")?; + match event { + Event::Key(key) if key.kind != KeyEventKind::Release => match key.code { + KeyCode::Enter => { + if input.is_empty() { + bail!("No OAuth callback URL entered"); + } + return Ok(input); + } + KeyCode::Char('c') if key.modifiers.contains(KeyModifiers::CONTROL) => { + bail!("OAuth login cancelled"); + } + KeyCode::Char('d') if key.modifiers.contains(KeyModifiers::CONTROL) => { + bail!("No OAuth callback URL received before input closed"); + } + KeyCode::Char('u') if key.modifiers.contains(KeyModifiers::CONTROL) => { + input.clear() + } + KeyCode::Backspace => { + input.pop(); + } + KeyCode::Char(character) + if !character.is_control() + && !key.modifiers.contains(KeyModifiers::CONTROL) => + { + if input.len() + character.len_utf8() > MAX_CALLBACK_BYTES { + bail!("OAuth callback URL exceeds 64 KiB"); + } + input.push(character); + } + _ => {} + }, + Event::Paste(paste) => { + if input.len() + paste.len() > MAX_CALLBACK_BYTES { + bail!("OAuth callback URL exceeds 64 KiB"); + } + input.push_str(&paste); + } + _ => {} + } + // Yield even with queued input so callback completion and timeout stay responsive. + tokio::task::yield_now().await; + } +} + +#[cfg(test)] +#[path = "mcp_login_tests.rs"] +mod tests; diff --git a/codex-rs/cli/src/mcp_login_tests.rs b/codex-rs/cli/src/mcp_login_tests.rs new file mode 100644 index 0000000000..4ee245524d --- /dev/null +++ b/codex-rs/cli/src/mcp_login_tests.rs @@ -0,0 +1,46 @@ +//! Checks bounded callback input independently of terminal state. + +use super::MAX_CALLBACK_BYTES; +use super::read_callback_line; +use pretty_assertions::assert_eq; +use std::io::Cursor; + +#[test] +fn callback_line_accepts_crlf_and_eof_after_input() { + for input in [ + "https://example.test/callback?code=abc\r\n", + "https://example.test/callback?code=abc", + ] { + assert_eq!( + read_callback_line(Cursor::new(input)).expect("read callback URL"), + "https://example.test/callback?code=abc" + ); + } +} + +#[test] +fn callback_line_is_bounded_and_does_not_expose_input_in_errors() { + let maximum_input = "s".repeat(MAX_CALLBACK_BYTES); + assert_eq!( + read_callback_line(Cursor::new(format!("{maximum_input}\r\n"))).expect("bounded input"), + maximum_input + ); + let input = "s".repeat(MAX_CALLBACK_BYTES + 1); + assert_eq!( + read_callback_line(Cursor::new(input)) + .unwrap_err() + .to_string(), + "OAuth callback URL exceeds 64 KiB" + ); + assert_eq!( + read_callback_line(Cursor::new("")).unwrap_err().to_string(), + "No OAuth callback URL received before input closed" + ); + assert_eq!( + format!( + "{:?}", + read_callback_line(Cursor::new(b"sensitive\xff")).unwrap_err() + ), + "OAuth callback URL must be valid UTF-8" + ); +} diff --git a/codex-rs/cli/tests/mcp_login.rs b/codex-rs/cli/tests/mcp_login.rs new file mode 100644 index 0000000000..784b761b3f --- /dev/null +++ b/codex-rs/cli/tests/mcp_login.rs @@ -0,0 +1,448 @@ +//! Exercises terminal OAuth pasteback and its shared callback validation through the CLI. + +use std::collections::BTreeMap; +use std::process::Output; +use std::process::Stdio; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use tempfile::TempDir; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::net::TcpStream; +use tokio::process::Child; +use tokio::process::ChildStdout; +use tokio::process::Command; +use tokio::time::timeout; +use url::Url; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; + +const TEST_TIMEOUT: Duration = Duration::from_secs(30); + +struct Fixture { + server: MockServer, + home: TempDir, + issuer: String, +} + +impl Fixture { + async fn new() -> Result { + let server = MockServer::start().await; + let home = TempDir::new()?; + let issuer = format!("{}/mcp", server.uri()); + std::fs::write( + home.path().join("config.toml"), + format!( + "mcp_oauth_credentials_store = \"file\"\n\ + [mcp_servers.manual]\nurl = \"{issuer}\"\nscopes = [\"mcp.read\"]\n\ + [mcp_servers.manual.oauth]\nclient_id = \"registered-client\"\n" + ), + )?; + Mock::given(method("GET")) + .and(path("/.well-known/oauth-authorization-server/mcp")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "issuer": issuer, + "authorization_endpoint": format!("{}/authorize", server.uri()), + "token_endpoint": format!("{}/token", server.uri()), + "response_types_supported": ["code"], + "code_challenge_methods_supported": ["S256"], + "scopes_supported": ["mcp.read"], + "authorization_response_iss_parameter_supported": true, + }))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "access_token": "mock-access-token", + "token_type": "Bearer", + "refresh_token": "mock-refresh-token", + "scope": "mcp.read", + }))) + .mount(&server) + .await; + Ok(Self { + server, + home, + issuer, + }) + } + + async fn start(&self) -> Result { + let mut child = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?) + .kill_on_drop(true) + .current_dir(self.home.path()) + .env("CODEX_HOME", self.home.path()) + .env("NO_PROXY", "127.0.0.1,localhost") + .env("no_proxy", "127.0.0.1,localhost") + .args(["mcp", "login", "manual", "--no-browser"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + let mut stdout = BufReader::new(child.stdout.take().context("missing stdout")?); + let mut transcript = String::new(); + let authorization_url = read_authorization_url(&mut stdout, &mut transcript).await?; + Ok(Login { + child, + stdout, + transcript, + authorization_url, + }) + } +} + +async fn read_authorization_url( + stdout: &mut BufReader, + transcript: &mut String, +) -> Result { + timeout(TEST_TIMEOUT, async { + loop { + let mut line = String::new(); + if stdout.read_line(&mut line).await? == 0 { + bail!("login exited before printing its authorization URL: {transcript}"); + } + transcript.push_str(&line); + if line.starts_with("http://") { + return Ok(Url::parse(line.trim())?); + } + } + }) + .await + .context("authorization URL timeout")? +} + +struct Login { + child: Child, + stdout: BufReader, + transcript: String, + authorization_url: Url, +} + +impl Login { + fn callback(&self, issuer: &str) -> Result { + let params: BTreeMap<_, _> = self.authorization_url.query_pairs().into_owned().collect(); + let mut callback = Url::parse(¶ms["redirect_uri"])?; + callback + .query_pairs_mut() + .append_pair("code", "mock-code") + .append_pair("state", ¶ms["state"]) + .append_pair("iss", issuer); + Ok(callback) + } + + async fn finish(mut self) -> Result { + let (mut output, _) = timeout(TEST_TIMEOUT, async { + tokio::try_join!( + self.child.wait_with_output(), + self.stdout.read_to_string(&mut self.transcript), + ) + }) + .await + .context("login completion timeout")??; + output.stdout = self.transcript.into_bytes(); + Ok(output) + } +} + +#[tokio::test] +async fn no_browser_exchanges_pasted_callback_and_saves_credentials() -> Result<()> { + let fixture = Fixture::new().await?; + let mut login = fixture.start().await?; + let authorization_url = login.authorization_url.to_string(); + let params: BTreeMap<_, _> = login.authorization_url.query_pairs().into_owned().collect(); + let callback = login.callback(&fixture.issuer)?; + login + .child + .stdin + .as_mut() + .context("missing stdin")? + .write_all(format!("{callback}\n").as_bytes()) + .await?; + let output = login.finish().await?; + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + insta::assert_snapshot!(String::from_utf8(output.stdout)?.replace(&authorization_url, "[AUTHORIZATION_URL]"), @r" + Authorize the MCP server by opening this URL in your browser: + [AUTHORIZATION_URL] + + After signing in, copy the full URL from your browser's address bar. + If the callback page cannot load, paste that URL here anyway. + Successfully logged in to MCP server 'manual'. + "); + assert!(String::from_utf8(output.stderr)?.contains("Callback URL (input hidden): ")); + let requests = fixture + .server + .received_requests() + .await + .context("missing requests")?; + let tokens: Vec<_> = requests + .iter() + .filter(|r| r.url.path() == "/token") + .collect(); + assert_eq!(tokens.len(), 1); + let mut exchange: BTreeMap<_, _> = url::form_urlencoded::parse(&tokens[0].body) + .into_owned() + .collect(); + let verifier = exchange + .remove("code_verifier") + .context("missing PKCE verifier")?; + assert!((43..=128).contains(&verifier.len())); + assert_eq!(params["code_challenge_method"], "S256"); + assert!(!params["code_challenge"].is_empty()); + assert_eq!( + exchange, + BTreeMap::from([ + ("client_id".to_string(), "registered-client".to_string()), + ("code".to_string(), "mock-code".to_string()), + ("grant_type".to_string(), "authorization_code".to_string()), + ("redirect_uri".to_string(), params["redirect_uri"].clone()), + ("resource".to_string(), fixture.issuer.clone()), + ]) + ); + let credentials: BTreeMap = serde_json::from_slice(&std::fs::read( + fixture.home.path().join(".credentials.json"), + )?)?; + assert_eq!( + credentials.into_values().collect::>(), + vec![json!({ + "server_name": "manual", "server_url": fixture.issuer, "issuer": fixture.issuer, + "client_id": "registered-client", "access_token": "mock-access-token", + "refresh_token": "mock-refresh-token", "expires_at": null, "scopes": ["mcp.read"], + })] + ); + Ok(()) +} + +#[tokio::test] +async fn no_browser_rejects_invalid_callbacks_without_exchanging_tokens() -> Result<()> { + for (field, value) in [ + ("state", Some("wrong-state")), + ("iss", None), + ("iss", Some("https://secret-issuer-marker.example")), + ("eof", None), + ] { + let fixture = Fixture::new().await?; + let mut login = fixture.start().await?; + let mut callback = login.callback(&fixture.issuer)?; + let mut pairs: BTreeMap<_, _> = callback.query_pairs().into_owned().collect(); + pairs.remove(field); + if let Some(value) = value { + pairs.insert(field.to_string(), value.to_string()); + } + callback.set_query(None); + callback.query_pairs_mut().extend_pairs(pairs); + if field == "eof" { + drop(login.child.stdin.take()); + } else { + login + .child + .stdin + .as_mut() + .context("missing stdin")? + .write_all(format!("{callback}\n").as_bytes()) + .await?; + } + let output = login.finish().await?; + assert!( + !output.status.success(), + "accepted invalid {field}: {value:?}" + ); + let stderr = String::from_utf8(output.stderr)?; + for secret in ["mock-code", "secret-issuer-marker"] { + assert!( + !stderr.contains(secret), + "callback error leaked {secret}: {stderr}" + ); + } + assert!(!fixture.home.path().join(".credentials.json").exists()); + assert!( + fixture + .server + .received_requests() + .await + .context("missing requests")? + .iter() + .all(|request| request.url.path() != "/token") + ); + } + Ok(()) +} + +#[tokio::test] +async fn no_browser_accepts_http_callback_while_terminal_input_remains_open() -> Result<()> { + let fixture = Fixture::new().await?; + let mut login = fixture.start().await?; + let callback = login.callback(&fixture.issuer)?; + let stdin = login.child.stdin.take().context("missing stdin")?; + let mut stream = TcpStream::connect(( + "127.0.0.1", + callback.port().context("missing callback port")?, + )) + .await?; + stream + .write_all( + format!( + "GET {}?{} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n", + callback.path(), + callback.query().context("missing callback query")? + ) + .as_bytes(), + ) + .await?; + let output = login.finish().await?; + drop(stdin); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(fixture.home.path().join(".credentials.json").exists()); + Ok(()) +} + +#[tokio::test] +async fn no_browser_retries_rejected_discovered_scopes_with_pasteback() -> Result<()> { + let fixture = Fixture::new().await?; + let config_path = fixture.home.path().join("config.toml"); + let config = std::fs::read_to_string(&config_path)?; + std::fs::write(config_path, config.replace("scopes = [\"mcp.read\"]\n", ""))?; + let mut login = fixture.start().await?; + assert!( + login + .authorization_url + .query_pairs() + .any(|(key, value)| key == "scope" && value == "mcp.read") + ); + let mut callback = login.callback(&fixture.issuer)?; + let mut params: BTreeMap<_, _> = callback.query_pairs().into_owned().collect(); + params.remove("code"); + params.insert("error".to_string(), "invalid_scope".to_string()); + callback.set_query(None); + callback.query_pairs_mut().extend_pairs(params); + login + .child + .stdin + .as_mut() + .context("missing stdin")? + .write_all(format!("{callback}\n").as_bytes()) + .await?; + login.authorization_url = + read_authorization_url(&mut login.stdout, &mut login.transcript).await?; + assert!( + !login + .authorization_url + .query_pairs() + .any(|(key, _)| key == "scope") + ); + let callback = login.callback(&fixture.issuer)?; + login + .child + .stdin + .as_mut() + .context("missing stdin")? + .write_all(format!("{callback}\n").as_bytes()) + .await?; + let output = login.finish().await?; + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8(output.stdout)?.contains("Retrying without scopes")); + let requests = fixture + .server + .received_requests() + .await + .context("missing requests")?; + assert_eq!( + requests + .iter() + .filter(|request| request.url.path() == "/token") + .count(), + 1 + ); + Ok(()) +} + +#[cfg(unix)] +#[tokio::test] +async fn no_browser_sigint_cancels_token_exchange_after_pasted_or_http_callback() -> Result<()> { + for callback_delivery in ["paste", "http"] { + let fixture = Fixture::new().await?; + let request_started = std::sync::Arc::new(tokio::sync::Notify::new()); + let notify = std::sync::Arc::clone(&request_started); + Mock::given(method("POST")) + .and(path("/token")) + .respond_with(move |_: &wiremock::Request| { + notify.notify_one(); + ResponseTemplate::new(200) + .set_delay(Duration::from_secs(/*secs*/ 30)) + .set_body_json( + json!({"access_token": "mock-access-token", "token_type": "Bearer"}), + ) + }) + .with_priority(/*priority*/ 1) + .expect(1) + .mount(&fixture.server) + .await; + let mut login = fixture.start().await?; + let callback = login.callback(&fixture.issuer)?; + // Keep the input pipe open after an HTTP callback, including while waiting for exit. + let mut stdin = login.child.stdin.take().context("missing stdin")?; + let mut http_callback = None; + if callback_delivery == "http" { + let mut stream = TcpStream::connect(( + "127.0.0.1", + callback.port().context("missing callback port")?, + )) + .await?; + stream + .write_all( + format!( + "GET {}?{} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n", + callback.path(), + callback.query().context("missing callback query")?, + ) + .as_bytes(), + ) + .await?; + http_callback = Some(stream); + } else { + stdin.write_all(format!("{callback}\n").as_bytes()).await?; + } + timeout(TEST_TIMEOUT, request_started.notified()) + .await + .context("token exchange never started")?; + let child_pid = i32::try_from(login.child.id().context("login exited before SIGINT")?)?; + // SAFETY: this positive PID belongs to the live child process owned by this test. + assert_eq!(unsafe { libc::kill(child_pid, libc::SIGINT) }, 0); + let output = timeout(Duration::from_secs(/*secs*/ 5), login.finish()) + .await + .with_context(|| { + format!("SIGINT did not cancel {callback_delivery} callback token exchange") + })??; + drop((stdin, http_callback)); + assert!( + !output.status.success(), + "SIGINT login unexpectedly succeeded" + ); + assert!(String::from_utf8(output.stderr)?.contains("OAuth login cancelled")); + assert!(!fixture.home.path().join(".credentials.json").exists()); + } + Ok(()) +} diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 0fd91d3867..51d94292c8 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -77,6 +77,7 @@ pub use perform_oauth_login::OauthLoginHandle; pub use perform_oauth_login::perform_oauth_login; pub use perform_oauth_login::perform_oauth_login_return_url; pub use perform_oauth_login::perform_oauth_login_silent; +pub use perform_oauth_login::perform_oauth_login_with_callback_input; pub use protocol_mode::McpProtocolMode; pub use rmcp::model::ElicitationAction; pub use rmcp_client::CancellableEventStreamRequest; diff --git a/codex-rs/rmcp-client/src/oauth_callback_input.rs b/codex-rs/rmcp-client/src/oauth_callback_input.rs new file mode 100644 index 0000000000..5358764137 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth_callback_input.rs @@ -0,0 +1,178 @@ +//! Accepts a pasted OAuth redirect without navigating to it. The prepared callback address +//! is checked before the existing OAuth flow validates state/issuer and exchanges the code. + +use super::CallbackResult; +use super::OAuthHttpContext; +use super::OAuthLoginPurpose; +use super::OAuthProviderError; +use super::OauthCallbackResult; +use super::OauthLoginFlow; +use crate::McpOAuthClientRegistration; +use crate::StreamableHttpRedirectMode; +use crate::save_oauth_tokens; +use anyhow::Context; +use anyhow::Result; +use anyhow::anyhow; +use anyhow::bail; +use codex_config::types::AuthKeyringBackendKind; +use codex_config::types::OAuthCredentialsStoreMode; +use codex_exec_server::HttpClient; +use std::collections::HashMap; +use std::future::Future; +use std::sync::Arc; +use tokio::time::timeout; +use url::Url; + +/// Runs MCP OAuth without opening a browser, accepting either the HTTP callback or a +/// full redirect URL returned by `read_callback`. The reader receives the authorization +/// URL and must release terminal state when its future is dropped (callback or timeout). +#[allow(clippy::too_many_arguments)] +pub async fn perform_oauth_login_with_callback_input( + server_name: &str, + server_url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + http_headers: Option>, + env_http_headers: Option>, + scopes: &[String], + oauth_client_id: Option<&str>, + client_registration: McpOAuthClientRegistration, + oauth_resource: Option<&str>, + callback_port: Option, + callback_url: Option<&str>, + global_callback_url: Option<&str>, + http_client: Arc, + read_callback: impl FnOnce(String) -> F, +) -> Result<()> +where + F: Future>, +{ + let mut flow = OauthLoginFlow::new( + server_name, + server_url, + store_mode, + keyring_backend_kind, + OAuthHttpContext { + http_headers, + env_http_headers, + http_client, + redirect_mode: StreamableHttpRedirectMode::Legacy, + }, + scopes, + oauth_client_id, + OAuthLoginPurpose::Mcp, + client_registration, + oauth_resource, + /*launch_browser*/ false, + callback_port, + callback_url, + global_callback_url, + /*timeout_secs*/ None, + ) + .await?; + let authorization_url = flow.authorization_url(); + let callback = timeout(flow.timeout, async { + tokio::select! { + callback = &mut flow.rx => callback.context("OAuth callback was cancelled"), + input = read_callback(authorization_url.clone()) => { + parse_callback_url(&input?, &flow.redirect_uri, &authorization_url) + } + } + }) + .await + .context("timed out waiting for OAuth callback")??; + // RMCP's issuer-mismatch error includes the received value. Reject it here + // without echoing any part of a pasted callback into terminal diagnostics. + if let CallbackResult::Success(callback) = &callback + && callback + .issuer + .as_deref() + .is_some_and(|issuer| Some(issuer) != flow.authorization_server_issuer.as_deref()) + { + bail!("OAuth callback issuer does not match this login"); + } + let stored = flow.complete_callback(callback).await?; + save_oauth_tokens(server_name, &stored, store_mode, keyring_backend_kind).await +} + +fn parse_callback_url( + input: &str, + redirect_uri: &str, + authorization_url: &str, +) -> Result { + if input.len() > 64 * 1024 { + bail!("OAuth callback URL exceeds 64 KiB"); + } + let mut callback = Url::parse(input.trim()).context("Invalid OAuth callback URL")?; + let mut expected = Url::parse(redirect_uri).context("Invalid OAuth redirect URI")?; + if callback.fragment().is_some() + || !callback.username().is_empty() + || callback.password().is_some() + { + bail!("OAuth callback URL must not contain credentials or a fragment"); + } + let mut response_params: Vec<_> = callback.query_pairs().into_owned().collect(); + // Configured redirect query parameters must survive unchanged. OAuth response + // parameters are appended by the authorization server, not part of that address. + let expected_params: Vec<_> = expected.query_pairs().into_owned().collect(); + for expected_param in &expected_params { + let position = response_params + .iter() + .position(|param| param == expected_param) + .ok_or_else(|| { + anyhow!("OAuth callback URL does not match this login's redirect URI") + })?; + response_params.remove(position); + } + if response_params + .iter() + .any(|(name, _)| expected_params.iter().any(|(key, _)| key == name)) + { + bail!("OAuth callback URL changes this login's redirect query parameters"); + } + callback.set_query(/*query*/ None); + expected.set_query(/*query*/ None); + if callback != expected { + bail!("OAuth callback URL does not match this login's redirect URI"); + } + + let mut params = HashMap::new(); + for (name, value) in response_params { + if params.insert(name, value).is_some() { + bail!("OAuth callback URL contains duplicate parameters"); + } + } + let state = params + .remove("state") + .filter(|state| !state.is_empty()) + .ok_or_else(|| anyhow!("OAuth callback URL is missing state"))?; + if params.contains_key("error") { + if params.contains_key("code") { + bail!("OAuth callback URL contains both a code and an error"); + } + let authorization = Url::parse(authorization_url)?; + if !authorization + .query_pairs() + .any(|(key, value)| key == "state" && value == state) + { + bail!("OAuth callback state does not match this login"); + } + // Do not print arbitrary pasted error descriptions or other callback values. + return Ok(CallbackResult::Error(OAuthProviderError::new( + /*error*/ None, /*error_description*/ None, + ))); + } + let code = params + .remove("code") + .filter(|code| !code.is_empty()) + .ok_or_else(|| anyhow!("OAuth callback URL is missing an authorization code"))?; + Ok(CallbackResult::Success(OauthCallbackResult { + code, + state, + issuer: params.remove("iss"), + })) +} + +#[cfg(test)] +#[path = "oauth_callback_input_tests.rs"] +mod tests; diff --git a/codex-rs/rmcp-client/src/oauth_callback_input_tests.rs b/codex-rs/rmcp-client/src/oauth_callback_input_tests.rs new file mode 100644 index 0000000000..21f2b3486e --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth_callback_input_tests.rs @@ -0,0 +1,70 @@ +//! Validates pasted callback addresses, parameter ambiguity, and redacted errors. + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn pasted_callback_preserves_configured_query_and_decodes_response() { + let CallbackResult::Success(callback) = parse_callback_url( + " https://callback.example:9443/registered?tenant=a&code=a%2Bb&state=csrf&iss=https%3A%2F%2Fissuer.example ", + "https://callback.example:9443/registered?tenant=a", + "https://issuer.example/authorize?state=csrf", + ) + .unwrap() else { + panic!("expected successful callback"); + }; + assert_eq!( + callback, + OauthCallbackResult { + code: "a+b".to_string(), + state: "csrf".to_string(), + issuer: Some("https://issuer.example".to_string()), + } + ); +} + +#[test] +fn pasted_callback_rejects_ambiguous_or_unbound_urls_without_echoing_values() { + let redirect = "http://127.0.0.1:1234/callback/server?tenant=a"; + for input in [ + "http://127.0.0.1:1234/other?tenant=a&code=secret&state=csrf", + "http://127.0.0.1:1235/callback/server?tenant=a&code=secret&state=csrf", + "http://127.0.0.1:1234/callback/server?tenant=b&code=secret&state=csrf", + "http://127.0.0.1:1234/callback/server?tenant=a&tenant=b&code=secret&state=csrf", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&%63ode=second&state=csrf", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&state=csrf&state=second", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&state=csrf&iss=a&iss=b", + "http://secret@127.0.0.1:1234/callback/server?tenant=a&code=secret&state=csrf", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&state=csrf#fragment", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&state=", + "http://127.0.0.1:1234/callback/server?tenant=a&code=secret&state=csrf&error=denied", + ] { + let error = + parse_callback_url(input, redirect, "https://issuer.example?state=csrf").unwrap_err(); + assert!(!format!("{error:#}").contains("secret")); + } + assert!(parse_callback_url(&"x".repeat(/*n*/ 65_537), redirect, "").is_err()); +} + +#[test] +fn pasted_provider_error_requires_matching_state_and_redacts_description() { + let redirect = "http://127.0.0.1:1234/callback/server"; + let input = format!("{redirect}?error=access_denied&error_description=secret&state=csrf"); + let CallbackResult::Error(error) = parse_callback_url( + &input, + redirect, + "https://issuer.example/authorize?state=csrf", + ) + .unwrap() else { + panic!("expected provider error"); + }; + assert_eq!(error.to_string(), "OAuth provider returned an error"); + assert!( + parse_callback_url( + &input, + redirect, + "https://issuer.example/authorize?state=other", + ) + .is_err() + ); +} diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index 22577db9e6..a1b094a041 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -45,6 +45,10 @@ use crate::utils::build_default_headers; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::OAuthCredentialsStoreMode; +#[path = "oauth_callback_input.rs"] +mod callback_input; +pub use callback_input::perform_oauth_login_with_callback_input; + #[derive(Clone, Copy)] pub(crate) enum OAuthLoginPurpose { Mcp, @@ -431,6 +435,7 @@ impl OauthLoginHandle { pub(crate) struct OauthLoginFlow { auth_url: String, + redirect_uri: String, oauth_state: OAuthState, authorization_server_issuer: Option, rx: oneshot::Receiver, @@ -704,6 +709,7 @@ impl OauthLoginFlow { Ok(Self { auth_url, + redirect_uri, oauth_state, authorization_server_issuer, rx, @@ -754,11 +760,15 @@ impl OauthLoginFlow { } } + let callback = timeout(self.timeout, &mut self.rx) + .await + .context("timed out waiting for OAuth callback")? + .context("OAuth callback was cancelled")?; + self.complete_callback(callback).await + } + + async fn complete_callback(mut self, callback: CallbackResult) -> Result { let result = async { - let callback = timeout(self.timeout, &mut self.rx) - .await - .context("timed out waiting for OAuth callback")? - .context("OAuth callback was cancelled")?; let OauthCallbackResult { code, state: csrf_state,