diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index dcd85231bd..b212fb7a5b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3967,6 +3967,7 @@ dependencies = [ "codex-utils-redacted-string", "codex-utils-string", "encoding_rs", + "gix-url", "globset", "http 1.4.0", "icu_decimal", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 45bbc3642c..f992ac7814 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -339,6 +339,7 @@ flate2 = "1.1.8" futures = { version = "0.3", default-features = false } gethostname = "1.1.0" gix = { version = "0.81.0", default-features = false, features = ["sha1"] } +gix-url = "0.35.2" glob = "0.3" globset = "0.4" hmac = "0.12.1" diff --git a/codex-rs/analytics/src/accepted_lines.rs b/codex-rs/analytics/src/accepted_lines.rs index f858df8199..8bb9185963 100644 --- a/codex-rs/analytics/src/accepted_lines.rs +++ b/codex-rs/analytics/src/accepted_lines.rs @@ -109,8 +109,8 @@ pub async fn accepted_line_repo_hash_for_cwd(cwd: &Path) -> Option { .get("origin") .or_else(|| remotes.values().next()) .map(|remote_url| { - let canonical_remote_url = - canonicalize_git_remote_url(remote_url).unwrap_or_else(|| remote_url.to_string()); + let canonical_remote_url = canonicalize_git_remote_url(remote_url.as_str()) + .unwrap_or_else(|| remote_url.to_string()); fingerprint_hash("repo", &canonical_remote_url) }) } diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index c86f7f5939..1ab0d9843a 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -148,6 +148,7 @@ use codex_app_server_protocol::ThreadItem; use codex_app_server_protocol::TurnSteerResponse; use codex_app_server_protocol::UserInput; use codex_app_server_protocol::WebSearchAction; +use codex_git_utils::SanitizedGitUrl; use codex_git_utils::collect_git_info; use codex_git_utils::get_git_repo_root; use codex_login::default_client::originator; @@ -1228,7 +1229,7 @@ impl AnalyticsReducer { None }; let skill_id = skill_id_for_local_skill( - repo_url.as_deref(), + repo_url.as_ref().map(SanitizedGitUrl::as_str), repo_root.as_deref(), path.as_path(), invocation.skill_name.as_str(), @@ -1270,7 +1271,7 @@ impl AnalyticsReducer { invoke_type: Some(invocation.invocation_type), model_slug: Some(tracking.model_slug.clone()), product_client_id: Some(tracking.product_client_id.clone()), - repo_url, + repo_url: repo_url.map(String::from), skill_scope, plugin_id: invocation.plugin_id, remote_plugin_id: invocation.remote_plugin_id, diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index 49cd2677d9..c2b2a8fa55 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -17,6 +17,7 @@ use codex_app_server_protocol::ThreadSectionMoveParams; use codex_app_server_protocol::ThreadSectionMoveResponse; use codex_extension_api::ExtensionDataInit; use codex_extension_api::ThreadIdleCause; +use codex_protocol::SanitizedGitUrl; use codex_protocol::config_types::MultiAgentMode; use codex_protocol::error::CodexErrorDetails; use codex_protocol::mcp::ClientMcpExtensions; @@ -1901,16 +1902,25 @@ impl ThreadRequestProcessor { return Err(invalid_request("gitInfo must include at least one field")); } + let origin_url = + Self::normalize_thread_metadata_git_field(origin_url, "gitInfo.originUrl")?; + let origin_url = match origin_url { + Some(Some(origin_url)) => { + Some(Some(SanitizedGitUrl::try_from(origin_url).map_err( + |_| invalid_request("gitInfo.originUrl must be a valid Git remote"), + )?)) + } + Some(None) => Some(None), + None => None, + }; + Ok(StoreGitInfoPatch { sha: Self::normalize_thread_metadata_git_field(sha, "gitInfo.sha")?, branch: Self::normalize_thread_metadata_git_field( branch, "gitInfo.branch", )?, - origin_url: Self::normalize_thread_metadata_git_field( - origin_url, - "gitInfo.originUrl", - )?, + origin_url, }) }, ) @@ -5840,7 +5850,7 @@ pub(crate) fn thread_from_stored_thread( let git_info = thread.git_info.map(|info| ApiGitInfo { sha: info.commit_hash.map(|sha| sha.0), branch: info.branch, - origin_url: info.repository_url, + origin_url: info.repository_url.map(String::from), }); let cwd = AbsolutePathBuf::relative_to_current_dir(path_utils::normalize_for_native_workdir( thread.cwd, @@ -5916,7 +5926,7 @@ fn summary_from_stored_thread( let git_info = thread.git_info.map(|git| ConversationGitInfo { sha: git.commit_hash.map(|sha| sha.0), branch: git.branch, - origin_url: git.repository_url, + origin_url: git.repository_url.map(String::from), }); ConversationSummary { conversation_id: thread.thread_id, @@ -6016,7 +6026,7 @@ fn summary_from_thread_metadata(metadata: &ThreadMetadata) -> ConversationSummar metadata.agent_role.clone(), metadata.git_sha.clone(), metadata.git_branch.clone(), - metadata.git_origin_url.clone(), + metadata.git_origin_url.clone().map(String::from), ) } diff --git a/codex-rs/app-server/src/request_processors/thread_summary.rs b/codex-rs/app-server/src/request_processors/thread_summary.rs index 422867f20a..5dacd34643 100644 --- a/codex-rs/app-server/src/request_processors/thread_summary.rs +++ b/codex-rs/app-server/src/request_processors/thread_summary.rs @@ -131,7 +131,7 @@ fn map_git_info(git_info: &CoreGitInfo) -> ConversationGitInfo { ConversationGitInfo { sha: git_info.commit_hash.as_ref().map(|sha| sha.0.clone()), branch: git_info.branch.clone(), - origin_url: git_info.repository_url.clone(), + origin_url: git_info.repository_url.clone().map(String::from), } } diff --git a/codex-rs/app-server/tests/suite/v2/thread_list.rs b/codex-rs/app-server/tests/suite/v2/thread_list.rs index 32af51abdb..202bbbe82c 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_list.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_list.rs @@ -37,6 +37,7 @@ use codex_app_server_protocol::UserInput; use codex_core::ARCHIVED_SESSIONS_SUBDIR; use codex_features::Feature; use codex_git_utils::GitSha; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::protocol::GitInfo as CoreGitInfo; use codex_protocol::protocol::MultiAgentVersion; @@ -1886,7 +1887,10 @@ async fn thread_list_includes_git_info() -> Result<()> { let git_info = CoreGitInfo { commit_hash: Some(GitSha::new("abc123")), branch: Some("main".to_string()), - repository_url: Some("https://example.com/repo.git".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("https://example.com/repo.git") + .expect("repository URL should be valid"), + ), }; let conversation_id = create_fake_rollout( codex_home.path(), @@ -1926,6 +1930,65 @@ async fn thread_list_includes_git_info() -> Result<()> { Ok(()) } +/// Legacy rollout credentials must be sanitized before thread/list returns Git metadata. +#[tokio::test] +async fn thread_list_sanitizes_git_info_from_existing_rollouts() -> Result<()> { + let codex_home = TempDir::new()?; + create_minimal_config(codex_home.path())?; + + let git_info = CoreGitInfo { + commit_hash: Some(GitSha::new("abc123")), + branch: Some("main".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("https://example.com/repo.git") + .expect("repository URL should be valid"), + ), + }; + let conversation_id = create_fake_rollout( + codex_home.path(), + "2025-02-01T09-00-00", + "2025-02-01T09:00:00Z", + "Git info preview", + Some("mock_provider"), + Some(git_info), + )?; + let path = rollout_path(codex_home.path(), "2025-02-01T09-00-00", &conversation_id); + let rollout = fs::read_to_string(&path)?; + fs::write( + path, + rollout.replace( + "https://example.com/repo.git", + "https://alice:synthetic-rollout-secret@example.com/repo.git", + ), + )?; + + let mut mcp = init_mcp(codex_home.path()).await?; + let ThreadListResponse { data, .. } = list_threads( + &mut mcp, + /*cursor*/ None, + Some(10), + Some(vec!["mock_provider".to_string()]), + /*source_kinds*/ None, + /*archived*/ None, + ) + .await?; + let thread = data + .iter() + .find(|thread| thread.id == conversation_id) + .expect("expected thread for created rollout"); + + assert_eq!( + thread.git_info, + Some(ApiGitInfo { + sha: Some("abc123".to_string()), + branch: Some("main".to_string()), + origin_url: Some("https://example.com/repo.git".to_string()), + }) + ); + + Ok(()) +} + #[tokio::test] async fn thread_list_default_sorts_by_created_at() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs b/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs index d82ed2c4f8..f4fce236f1 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs @@ -9,6 +9,7 @@ use codex_app_server_protocol::GitInfo; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ThreadHistoryMode; use codex_app_server_protocol::ThreadListParams; use codex_app_server_protocol::ThreadListResponse; use codex_app_server_protocol::ThreadMetadataGitInfoUpdateParams; @@ -30,6 +31,7 @@ use codex_app_server_protocol::ThreadStatus; use codex_core::ARCHIVED_SESSIONS_SUBDIR; use codex_features::Feature; use codex_git_utils::GitSha; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::protocol::GitInfo as RolloutGitInfo; use codex_rollout::state_db::reconcile_rollout; @@ -651,6 +653,90 @@ async fn thread_metadata_update_patches_git_branch_and_returns_updated_thread() Ok(()) } +/// Client-provided Git credentials must not reach API responses, SQLite, or rollout files. +#[tokio::test] +async fn thread_metadata_update_sanitizes_git_origin_before_persisting() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + mock_responses_config(&server.uri()).write(codex_home.path())?; + let state_db = init_state_db(codex_home.path()).await?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build() + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request_with_auto_env(ThreadStartParams { + model: Some("mock-model".to_string()), + history_mode: Some(ThreadHistoryMode::Legacy), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + let update_id = mcp + .send_thread_metadata_update_request(ThreadMetadataUpdateParams { + thread_id: thread.id.clone(), + project_id: None, + git_info: Some(ThreadMetadataGitInfoUpdateParams { + sha: None, + branch: None, + origin_url: Some(Some( + "https://alice:synthetic-git-secret@example.invalid/org/repo.git".to_string(), + )), + }), + }) + .await?; + let update_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(update_id)), + ) + .await??; + let ThreadMetadataUpdateResponse { thread: updated } = + to_response::(update_resp)?; + let expected_git_info = Some(GitInfo { + sha: None, + branch: None, + origin_url: Some("https://example.invalid/org/repo.git".to_string()), + }); + assert_eq!(updated.git_info, expected_git_info); + + let persisted = state_db + .get_thread(ThreadId::from_string(&thread.id)?) + .await? + .expect("updated thread must be persisted"); + assert_eq!( + persisted.git_origin_url.as_deref(), + Some("https://example.invalid/org/repo.git") + ); + let rollout = fs::read_to_string(persisted.rollout_path)?; + assert!(!rollout.contains("synthetic-git-secret")); + assert!(rollout.contains("https://example.invalid/org/repo.git")); + + let read_id = mcp + .send_thread_read_request(ThreadReadParams { + thread_id: thread.id, + include_turns: false, + }) + .await?; + let read_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(read_id)), + ) + .await??; + let ThreadReadResponse { thread: read, .. } = to_response::(read_resp)?; + assert_eq!(read.git_info, expected_git_info); + + Ok(()) +} + #[tokio::test] async fn thread_metadata_update_rejects_empty_git_info_patch() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; @@ -1004,7 +1090,10 @@ async fn thread_metadata_update_can_clear_stored_git_fields() -> Result<()> { Some(RolloutGitInfo { commit_hash: Some(GitSha::new("abc123")), branch: Some("feature/sidebar-pr".to_string()), - repository_url: Some("git@example.com:openai/codex.git".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("repository URL should be valid"), + ), }), )?; let _state_db = init_state_db(codex_home.path()).await?; diff --git a/codex-rs/cloud-tasks/src/env_detect.rs b/codex-rs/cloud-tasks/src/env_detect.rs index 36907cab59..31fbc99e2f 100644 --- a/codex-rs/cloud-tasks/src/env_detect.rs +++ b/codex-rs/cloud-tasks/src/env_detect.rs @@ -1,3 +1,4 @@ +use codex_git_utils::SanitizedGitUrl; use codex_http_client::RouteAwareClientPool; use http::StatusCode; use http::header::CONTENT_TYPE; @@ -44,13 +45,13 @@ async fn autodetect_environment_id_with_origins( base_url: &str, headers: &HeaderMap, desired_label: Option, - origins: &[String], + origins: &[SanitizedGitUrl], ) -> anyhow::Result { // 1) Try repo-specific environments based on local git origins (GitHub only, like VSCode) crate::append_error_log(format!("env: git origins: {origins:?}")); let mut by_repo_envs: Vec = Vec::new(); for origin in origins { - if let Some((owner, repo)) = parse_owner_repo(origin) { + if let Some((owner, repo)) = parse_owner_repo(origin.as_str()) { let url = if base_url.contains("/backend-api") { format!( "{}/wham/environments/by-repo/{}/{}/{}", @@ -213,7 +214,7 @@ impl EnvironmentHttp for RouteAwareClientPool { } } -fn get_git_origins() -> Vec { +fn get_git_origins() -> Vec { // Prefer: git config --get-regexp remote\..*\.url let out = std::process::Command::new("git") .args(["-c", codex_git_utils::SAFE_BARE_REPOSITORY_CONFIG]) @@ -225,8 +226,10 @@ fn get_git_origins() -> Vec { let s = String::from_utf8_lossy(&ok.stdout); let mut urls = Vec::new(); for line in s.lines() { - if let Some((_, url)) = line.split_once(' ') { - urls.push(url.trim().to_string()); + if let Some((_, url)) = line.split_once(' ') + && let Ok(url) = SanitizedGitUrl::try_from(url.trim()) + { + urls.push(url); } } if !urls.is_empty() { @@ -245,8 +248,10 @@ fn get_git_origins() -> Vec { let mut urls = Vec::new(); for line in s.lines() { let parts: Vec<&str> = line.split_whitespace().collect(); - if parts.len() >= 2 { - urls.push(parts[1].to_string()); + if parts.len() >= 2 + && let Ok(url) = SanitizedGitUrl::try_from(parts[1]) + { + urls.push(url); } } if !urls.is_empty() { @@ -256,7 +261,7 @@ fn get_git_origins() -> Vec { Vec::new() } -fn uniq(mut v: Vec) -> Vec { +fn uniq(mut v: Vec) -> Vec { v.sort(); v.dedup(); v @@ -285,6 +290,7 @@ fn parse_owner_repo(url: &str) -> Option<(String, String)> { "http://github.com/", "git://github.com/", "github.com/", + "github.com:", ] { if let Some(rest) = s.strip_prefix(prefix) { let rest = rest.trim_start_matches('/').trim_end_matches(".git"); @@ -312,13 +318,13 @@ async fn list_environments_with_origins( http: &impl EnvironmentHttp, base_url: &str, headers: &HeaderMap, - origins: &[String], + origins: &[SanitizedGitUrl], ) -> anyhow::Result> { let mut map: HashMap = HashMap::new(); // 1) By-repo lookup for each parsed GitHub origin for origin in origins { - if let Some((owner, repo)) = parse_owner_repo(origin) { + if let Some((owner, repo)) = parse_owner_repo(origin.as_str()) { let url = if base_url.contains("/backend-api") { format!( "{}/wham/environments/by-repo/{}/{}/{}", diff --git a/codex-rs/cloud-tasks/src/env_detect_tests.rs b/codex-rs/cloud-tasks/src/env_detect_tests.rs index 6032806f07..3299ff87f3 100644 --- a/codex-rs/cloud-tasks/src/env_detect_tests.rs +++ b/codex-rs/cloud-tasks/src/env_detect_tests.rs @@ -133,7 +133,7 @@ async fn autodetect_requests_exact_repository_endpoint_and_decodes_selection() { BASE_URL, &headers, Some("Repository".to_string()), - &["git@github.com:openai/codex.git".to_string()], + &[SanitizedGitUrl::try_from("git@github.com:openai/codex.git").expect("valid Git remote")], ) .await .expect("repository environment should be selected"); @@ -154,6 +154,68 @@ async fn autodetect_requests_exact_repository_endpoint_and_decodes_selection() { ); } +/// Repository discovery must accept credential-bearing origins without exposing credentials. +#[tokio::test] +async fn autodetect_sanitizes_credential_bearing_git_origins() { + let http = FakeHttp::new(HashMap::from([( + BY_REPO_URL.to_string(), + json_response(r#"[{"id":"env-repo","label":"Repository"}]"#), + )])); + + let selection = autodetect_environment_id_with_origins( + &http, + BASE_URL, + &HeaderMap::new(), + /*desired_label*/ None, + &[SanitizedGitUrl::try_from( + "https://alice:synthetic-git-secret@github.com/openai/codex.git", + ) + .expect("valid Git remote")], + ) + .await + .expect("repository environment should be selected"); + + assert_eq!( + selection, + AutodetectSelection { + id: "env-repo".to_string(), + label: Some("Repository".to_string()), + } + ); + assert_eq!(http.requested_urls(), vec![BY_REPO_URL.to_string()]); +} + +/// Removing an SCP remote username must not prevent repository-specific discovery. +#[tokio::test] +async fn autodetect_recognizes_sanitized_scp_git_origins() { + let http = FakeHttp::new(HashMap::from([( + BY_REPO_URL.to_string(), + json_response(r#"[{"id":"env-repo","label":"Repository"}]"#), + )])); + + let selection = autodetect_environment_id_with_origins( + &http, + BASE_URL, + &HeaderMap::new(), + /*desired_label*/ None, + &[ + SanitizedGitUrl::try_from("org-123@github.com:openai/codex.git") + .expect("valid Git remote"), + ], + ) + .await + .expect("repository environment should be selected"); + + assert_eq!( + selection, + AutodetectSelection { + id: "env-repo".to_string(), + label: Some("Repository".to_string()), + } + ); + assert_eq!(http.requested_urls(), vec![BY_REPO_URL.to_string()]); +} + #[tokio::test] async fn autodetect_falls_back_to_exact_global_endpoint_and_decodes_selection() { let http = FakeHttp::new(HashMap::from([ @@ -169,7 +231,7 @@ async fn autodetect_falls_back_to_exact_global_endpoint_and_decodes_selection() BASE_URL, &HeaderMap::new(), /*desired_label*/ None, - &["git@github.com:openai/codex.git".to_string()], + &[SanitizedGitUrl::try_from("git@github.com:openai/codex.git").expect("valid Git remote")], ) .await .expect("global environment should be selected"); @@ -206,7 +268,10 @@ async fn list_requests_exact_repository_and_global_endpoints_and_merges_results( &http, BASE_URL, &HeaderMap::new(), - &["https://github.com/openai/codex.git".to_string()], + &[ + SanitizedGitUrl::try_from("https://github.com/openai/codex.git") + .expect("valid Git remote"), + ], ) .await .expect("environment list should decode"); diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index 05bc54a9ad..8cadb3b7ba 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -16,6 +16,7 @@ use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_git_utils::GitInfo; use codex_git_utils::GitSha; +use codex_git_utils::SanitizedGitUrl; use codex_git_utils::collect_git_info; use codex_git_utils::get_git_repo_root; use codex_git_utils::get_has_changes_in_repo; @@ -406,7 +407,10 @@ async fn test_collect_git_info_with_remote() { .to_string(); // Should have repository URL - assert_eq!(git_info.repository_url, Some(expected_remote)); + assert_eq!( + git_info.repository_url, + Some(SanitizedGitUrl::try_from(expected_remote.as_str()).unwrap()) + ); } #[tokio::test] @@ -966,7 +970,9 @@ fn test_git_info_serialization() { let git_info = GitInfo { commit_hash: Some(GitSha::new("abc123def456")), branch: Some("main".to_string()), - repository_url: Some("https://github.com/example/repo.git".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("https://github.com/example/repo.git").unwrap(), + ), }; let json = serde_json::to_string(&git_info).expect("Should serialize GitInfo"); diff --git a/codex-rs/core/src/responses_metadata.rs b/codex-rs/core/src/responses_metadata.rs index a520072975..61d67d3dfd 100644 --- a/codex-rs/core/src/responses_metadata.rs +++ b/codex-rs/core/src/responses_metadata.rs @@ -6,6 +6,7 @@ use codex_analytics::CompactionPhase; use codex_analytics::CompactionReason; use codex_analytics::CompactionStrategy; use codex_analytics::CompactionTrigger; +use codex_git_utils::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; @@ -163,7 +164,7 @@ impl CodexResponsesRequestKind { #[derive(Clone, Debug, Serialize, Default)] pub(crate) struct TurnMetadataWorkspace { #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) associated_remote_urls: Option>, + pub(crate) associated_remote_urls: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) latest_git_commit_hash: Option, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/codex-rs/core/src/turn_metadata.rs b/codex-rs/core/src/turn_metadata.rs index 05b50f1894..388417ca29 100644 --- a/codex-rs/core/src/turn_metadata.rs +++ b/codex-rs/core/src/turn_metadata.rs @@ -25,6 +25,7 @@ use crate::responses_metadata::subagent_header_value; use crate::responses_metadata::subagent_metadata_kind; use crate::sandbox_tags::permission_profile_policy_tag; use crate::sandbox_tags::permission_profile_sandbox_tag; +use codex_git_utils::SanitizedGitUrl; use codex_git_utils::get_git_remote_urls_assume_git_repo; use codex_git_utils::get_git_repo_root; use codex_git_utils::get_has_changes_in_repo; @@ -51,7 +52,7 @@ pub(crate) struct McpTurnMetadataContext<'a> { #[derive(Clone, Debug, Default)] struct WorkspaceGitMetadata { - associated_remote_urls: Option>, + associated_remote_urls: Option>, latest_git_commit_hash: Option, has_changes: Option, } diff --git a/codex-rs/core/tests/responses_headers.rs b/codex-rs/core/tests/responses_headers.rs index ff15c541c3..9beca451bd 100644 --- a/codex-rs/core/tests/responses_headers.rs +++ b/codex-rs/core/tests/responses_headers.rs @@ -686,3 +686,96 @@ async fn responses_stream_includes_turn_metadata_header_for_git_workspace_e2e() Some(false) ); } + +/// Credential-bearing Git remotes must be redacted from both metadata transports. +#[tokio::test] +async fn responses_stream_redacts_git_remote_credentials_from_turn_metadata() { + core_test_support::skip_if_no_network!(); + + let server = responses::start_mock_server().await; + let test = test_codex() + .with_model("test-gpt-5.1-codex") + .build(&server) + .await + .expect("build test codex"); + let cwd = test.cwd_path(); + let remote_token = "test-git-secret-token"; + let credential_bearing_origin = + "https://alice:test-git-secret-token@example.invalid/openai/codex.git"; + + for args in [ + ["init"].as_slice(), + ["remote", "add", "origin", credential_bearing_origin].as_slice(), + ] { + let output = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + let first_response = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call( + "wait-for-git", + "test_sync_tool", + r#"{"wait_for_git_enrichment":true}"#, + ), + responses::ev_completed("resp-1"), + ]); + let follow_up_response = responses::sse(vec![ + responses::ev_response_created("resp-2"), + responses::ev_assistant_message("msg-1", "done"), + responses::ev_completed("resp-2"), + ]); + let request_log = responses::mount_response_sequence( + &server, + vec![ + responses::sse_response(first_response), + responses::sse_response(follow_up_response), + ], + ) + .await; + + test.submit_turn("inspect the workspace") + .await + .expect("submit turn prompt"); + + let requests = request_log.requests(); + assert_eq!(requests.len(), 2, "expected a follow-up model request"); + let header_metadata = requests[1] + .header("x-codex-turn-metadata") + .expect("request header should include turn metadata"); + let request_body = requests[1].body_json(); + let body_metadata = request_body["client_metadata"]["x-codex-turn-metadata"] + .as_str() + .expect("request body should include turn metadata"); + + for (location, metadata) in [ + ("request header", header_metadata.as_str()), + ("request body", body_metadata), + ] { + assert!( + !metadata.contains(remote_token), + "{location} should not expose git remote credentials" + ); + let metadata: serde_json::Value = + serde_json::from_str(metadata).expect("turn metadata should be valid JSON"); + let remotes = metadata + .get("workspaces") + .and_then(serde_json::Value::as_object) + .and_then(|workspaces| workspaces.values().next()) + .and_then(|workspace| workspace.get("associated_remote_urls")) + .expect("turn metadata should include git remote URLs"); + assert_eq!( + remotes, + &serde_json::json!({ "origin": "https://example.invalid/openai/codex.git" }), + "{location} should contain the sanitized remote" + ); + } +} diff --git a/codex-rs/core/tests/suite/cli_stream.rs b/codex-rs/core/tests/suite/cli_stream.rs index 81f635214f..715da9dc5b 100644 --- a/codex-rs/core/tests/suite/cli_stream.rs +++ b/codex-rs/core/tests/suite/cli_stream.rs @@ -1,3 +1,4 @@ +use codex_git_utils::SanitizedGitUrl; use codex_git_utils::collect_git_info; use codex_login::CODEX_ACCESS_TOKEN_ENV_VAR; use codex_login::CODEX_API_KEY_ENV_VAR; @@ -769,6 +770,7 @@ async fn integration_git_info_unit_test() { .unwrap() .trim() .to_string(); + let expected_remote_url = SanitizedGitUrl::try_from(expected_remote_url.as_str()).unwrap(); assert_eq!( repo_url, &expected_remote_url, "Repository URL should match git remote get-url output" @@ -785,7 +787,13 @@ async fn integration_git_info_unit_test() { assert_eq!(git_info.commit_hash, deserialized.commit_hash); assert_eq!(git_info.branch, deserialized.branch); - assert_eq!(git_info.repository_url, deserialized.repository_url); + assert_eq!( + git_info + .repository_url + .as_ref() + .map(SanitizedGitUrl::as_str), + deserialized.repository_url.as_deref() + ); println!("✅ Git info serialization test passed!"); } diff --git a/codex-rs/core/tests/suite/git_enrichment.rs b/codex-rs/core/tests/suite/git_enrichment.rs index 206f775fa7..0bad1eb28b 100644 --- a/codex-rs/core/tests/suite/git_enrichment.rs +++ b/codex-rs/core/tests/suite/git_enrichment.rs @@ -148,6 +148,66 @@ async fn startup_prewarm_skips_git_enrichment_and_user_turn_observes_fresh_state Ok(()) } +/// Repository credentials must never cross the outbound model-request boundary. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_turn_git_enrichment_redacts_remote_credentials() -> Result<()> { + skip_if_no_network!(Ok(())); + + let (repo, head) = create_git_repo()?; + run_git( + repo.path(), + &[ + "remote", + "set-url", + "origin", + "https://git-user:git-secret-token@example.invalid/cxa5426/repo.git", + ], + )?; + let server = start_websocket_server(vec![vec![ + vec![ev_response_created("warm-1"), ev_completed("warm-1")], + vec![ + ev_response_created("resp-1"), + ev_function_call( + "wait-for-git", + "test_sync_tool", + r#"{"wait_for_git_enrichment":true}"#, + ), + ev_completed("resp-1"), + ], + vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "done"), + ev_completed("resp-2"), + ], + ]]) + .await; + let cwd = repo.path().to_path_buf(); + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_config(move |config| { + config.cwd = cwd.abs(); + }); + let test = builder.build_with_websocket_server(&server).await?; + + test.submit_turn("inspect the workspace").await?; + let turn = server + .single_connection() + .get(2) + .context("turn follow-up request")? + .body_json(); + let serialized_turn = serde_json::to_string(&turn)?; + assert!(!serialized_turn.contains("git-user")); + assert!(!serialized_turn.contains("git-secret-token")); + assert_eq!( + turn_metadata(&turn)?["workspaces"], + expected_workspace(repo.path(), &head, /*has_changes*/ false) + ); + + test.codex.shutdown_and_wait().await?; + server.shutdown().await; + Ok(()) +} + #[cfg(not(target_os = "windows"))] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_prewarm_and_review_skip_redundant_git_enrichment() -> Result<()> { diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index be5c6680f3..092acd0dab 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -12,6 +12,7 @@ use tokio::time::Duration as TokioDuration; use ts_rs::TS; use crate::GitSha; +use crate::SanitizedGitUrl; use crate::git_process::run_git_command_with_timeout_output; /// Return `true` if the project folder specified by the `Config` is inside a @@ -49,7 +50,7 @@ pub struct GitInfo { pub branch: Option, /// Repository URL (if available from remote) #[serde(skip_serializing_if = "Option::is_none")] - pub repository_url: Option, + pub repository_url: Option, } #[derive(Serialize, Deserialize, Clone, Debug)] @@ -110,14 +111,14 @@ pub async fn collect_git_info(cwd: &Path) -> Option { && output.status.success() && let Ok(url) = String::from_utf8(output.stdout) { - git_info.repository_url = Some(url.trim().to_string()); + git_info.repository_url = SanitizedGitUrl::try_from(url.trim()).ok(); } Some(git_info) } /// Collect fetch remotes in a multi-root-friendly format: {"origin": "https://..."}. -pub async fn get_git_remote_urls(cwd: &Path) -> Option> { +pub async fn get_git_remote_urls(cwd: &Path) -> Option> { let is_git_repo = run_git_command_with_timeout(&["rev-parse", "--git-dir"], cwd) .await? .status @@ -130,7 +131,9 @@ pub async fn get_git_remote_urls(cwd: &Path) -> Option> } /// Collect fetch remotes without checking whether `cwd` is in a git repo. -pub async fn get_git_remote_urls_assume_git_repo(cwd: &Path) -> Option> { +pub async fn get_git_remote_urls_assume_git_repo( + cwd: &Path, +) -> Option> { let output = run_git_command_with_timeout(&["remote", "-v"], cwd).await?; if !output.status.success() { return None; @@ -258,7 +261,7 @@ fn trim_git_suffix(value: &str) -> &str { value.strip_suffix(".git").unwrap_or(value) } -fn parse_git_remote_urls(stdout: &str) -> Option> { +fn parse_git_remote_urls(stdout: &str) -> Option> { let mut remotes = BTreeMap::new(); for line in stdout.lines() { let Some(fetch_line) = line.strip_suffix(" (fetch)") else { @@ -273,8 +276,10 @@ fn parse_git_remote_urls(stdout: &str) -> Option> { }; let url = url_part.trim_start(); - if !url.is_empty() { - remotes.insert(name.to_string(), url.to_string()); + if !url.is_empty() + && let Ok(url) = SanitizedGitUrl::try_from(url) + { + remotes.insert(name.to_string(), url); } } @@ -921,6 +926,103 @@ mod tests { } } + /// Fetch remotes must be sanitized before they enter workspace metadata. + #[test] + fn parse_git_remote_urls_sanitizes_fetch_credentials() { + let remotes = parse_git_remote_urls( + "origin\thttps://alice:secret@github.com/org/repo.git (fetch)\n\ + origin\thttps://alice:secret@github.com/org/repo.git (push)\n\ + upstream\tgit@github.com:org/upstream.git (fetch)\n", + ); + + assert_eq!( + remotes, + Some(BTreeMap::from([ + ( + "origin".to_string(), + SanitizedGitUrl::try_from("https://github.com/org/repo.git") + .expect("parse expected remote URL"), + ), + ( + "upstream".to_string(), + SanitizedGitUrl::try_from("git@github.com:org/upstream.git") + .expect("parse expected remote URL"), + ), + ])) + ); + } + + /// Malformed remote entries are omitted instead of leaking their raw contents. + #[test] + fn parse_git_remote_urls_skips_malformed_entries() { + let remotes = parse_git_remote_urls( + "bad\thttps://alice:secret@[invalid (fetch)\n\ + origin\thttps://github.com/org/repo.git (fetch)\n", + ); + + assert_eq!( + remotes, + Some(BTreeMap::from([( + "origin".to_string(), + SanitizedGitUrl::try_from("https://github.com/org/repo.git") + .expect("parse expected remote URL"), + )])) + ); + } + + /// Both metadata collection paths must sanitize credentials introduced by `insteadOf`. + #[tokio::test] + async fn git_metadata_collection_sanitizes_rewritten_remote_credentials() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo = temp_dir.path(); + + let status = std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(repo) + .status() + .expect("initialize test repository"); + assert!(status.success(), "initialize test repository"); + + let status = std::process::Command::new("git") + .args([ + "config", + "url.https://alice:secret-token@example.invalid/.insteadOf", + "https://short.invalid/", + ]) + .current_dir(repo) + .status() + .expect("configure credential-bearing remote rewrite"); + assert!( + status.success(), + "configure credential-bearing remote rewrite" + ); + + let status = std::process::Command::new("git") + .args([ + "remote", + "add", + "origin", + "https://short.invalid/org/repo.git", + ]) + .current_dir(repo) + .status() + .expect("configure rewritten remote"); + assert!(status.success(), "configure rewritten remote"); + + let git_info = collect_git_info(repo).await.expect("collect git info"); + let remotes = get_git_remote_urls_assume_git_repo(repo).await; + let expected = SanitizedGitUrl::try_from("https://example.invalid/org/repo.git") + .expect("parse expected remote URL"); + + assert_eq!( + (git_info.repository_url, remotes), + ( + Some(expected.clone()), + Some(BTreeMap::from([("origin".to_string(), expected)])), + ) + ); + } + #[tokio::test] async fn local_git_branches_excludes_detached_head_entry() { let temp_dir = tempfile::tempdir().expect("create temp dir"); diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 74f811d41f..ec02057ce9 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -27,6 +27,7 @@ pub use baseline::diff_since_latest_init; pub use baseline::ensure_git_baseline_repository; pub use baseline::reset_git_repository; pub use branch::merge_base_with_head; +pub use codex_protocol::SanitizedGitUrl; pub use codex_protocol::protocol::GitSha; pub use errors::GitToolingError; pub use fsmonitor::FsmonitorOverride; diff --git a/codex-rs/protocol/Cargo.toml b/codex-rs/protocol/Cargo.toml index f7f4b779d5..4e2bf12aa5 100644 --- a/codex-rs/protocol/Cargo.toml +++ b/codex-rs/protocol/Cargo.toml @@ -26,6 +26,7 @@ codex-utils-path-uri = { workspace = true } codex-utils-redacted-string = { workspace = true } codex-utils-string = { workspace = true } encoding_rs = { workspace = true } +gix-url = { workspace = true } globset = { workspace = true } http = { workspace = true } icu_decimal = { workspace = true } diff --git a/codex-rs/protocol/src/lib.rs b/codex-rs/protocol/src/lib.rs index f52dbe0cdc..efdfda6eba 100644 --- a/codex-rs/protocol/src/lib.rs +++ b/codex-rs/protocol/src/lib.rs @@ -2,6 +2,7 @@ pub mod account; mod agent_path; pub mod auth; mod response_item_id; +mod sanitized_git_url; mod session_id; mod thread_id; mod tool_name; @@ -9,6 +10,7 @@ pub use agent_path::AgentPath; pub use permission_profile_intersection::PermissionIntersectionError; pub use permission_profile_intersection::intersect_effective_permission_profiles; pub use response_item_id::ResponseItemId; +pub use sanitized_git_url::SanitizedGitUrl; pub use session_id::SessionId; pub use thread_id::RolloutId; pub use thread_id::ThreadId; diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index f76267a233..55b319f781 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -16,6 +16,7 @@ use strum_macros::EnumIter; use crate::AgentPath; use crate::ResponseItemId; +use crate::SanitizedGitUrl; use crate::SessionId; use crate::ThreadId; use crate::approvals::ElicitationRequestEvent; @@ -3212,8 +3213,13 @@ pub struct GitInfo { #[serde(skip_serializing_if = "Option::is_none")] pub branch: Option, /// Repository URL (if available from remote) - #[serde(skip_serializing_if = "Option::is_none")] - pub repository_url: Option, + #[serde( + default, + deserialize_with = "crate::sanitized_git_url::deserialize_optional_sanitized_git_url", + skip_serializing_if = "Option::is_none" + )] + #[schemars(with = "Option")] + pub repository_url: Option, } #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] diff --git a/codex-rs/protocol/src/sanitized_git_url.rs b/codex-rs/protocol/src/sanitized_git_url.rs new file mode 100644 index 0000000000..daf6345d8b --- /dev/null +++ b/codex-rs/protocol/src/sanitized_git_url.rs @@ -0,0 +1,159 @@ +use std::fmt; +use std::ops::Deref; + +use gix_url::Scheme; +use gix_url::Url; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; +use ts_rs::TS; + +/// A git remote URL with authentication credentials removed. +#[derive( + Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize, JsonSchema, TS, +)] +#[serde(try_from = "String", into = "String")] +#[schemars(with = "String")] +#[ts(type = "string")] +pub struct SanitizedGitUrl(String); + +impl SanitizedGitUrl { + pub fn as_str(&self) -> &str { + self.0.as_str() + } +} + +impl TryFrom<&str> for SanitizedGitUrl { + type Error = &'static str; + + fn try_from(value: &str) -> Result { + // Remote helpers wrap another remote, so peel off their prefixes + // without recursion before passing the address to the Git URL parser. + let mut address = value; + while let Some((transport, nested_address)) = address.split_once("::") + && !transport.is_empty() + && transport.chars().all(|character| { + character.is_ascii_alphanumeric() || matches!(character, '+' | '-' | '.') + }) + { + // Helpers can also receive executable command lines containing + // arbitrary secrets; scan the whole payload once and reject those. + if address.len() == value.len() && nested_address.chars().any(char::is_whitespace) { + return Err("invalid git remote URL"); + } + address = nested_address; + } + let helper_prefix = &value[..value.len() - address.len()]; + let original = value; + let value = address; + + // Parse first to recognize both URL and SCP-style remotes and reject + // malformed inputs before reconstructing the original text. + let url = match Url::try_from(value) { + Ok(url) => url, + Err(_) => { + // gix mistakes an IPv6 address's first colon for the SCP path + // separator when a username comes before the bracketed host. + let (user, host_and_path) = + value.split_once('@').ok_or("invalid git remote URL")?; + let (host, path) = host_and_path + .strip_prefix('[') + .and_then(|host_and_path| host_and_path.split_once("]:")) + .ok_or("invalid git remote URL")?; + if user.is_empty() || path.is_empty() { + return Err("invalid git remote URL"); + } + // SCP paths are opaque and can contain characters that URL parsing rejects. + let ssh_url = format!("ssh://{user}@[{host}]/"); + Url::try_from(ssh_url.as_str()).map_err(|_| "invalid git remote URL")? + } + }; + + // SSH's conventional `git` user is a transport identity, not a secret; + // every other username and all passwords must be removed. + let preserve_ssh_git_user = url.scheme == Scheme::Ssh && url.user() == Some("git"); + + // Inspect the authority directly because gix treats the complete + // `file://` authority as its host and does not expose its userinfo. + // Rebuilding only this part also preserves escaped repository paths. + if let Some((scheme, authority_and_path)) = value.split_once("://") { + let authority_end = authority_and_path + .find('/') + .unwrap_or(authority_and_path.len()); + let (authority, path) = authority_and_path.split_at(authority_end); + let Some((_, host)) = authority.rsplit_once('@') else { + return Ok(Self(original.to_owned())); + }; + if preserve_ssh_git_user && url.password().is_none() { + return Ok(Self(original.to_owned())); + } + let user = if preserve_ssh_git_user { "git@" } else { "" }; + return Ok(Self(format!( + "{helper_prefix}{scheme}://{user}{host}{path}" + ))); + } + + // Parsed serialization can alter Git paths, so preserve the exact + // original representation whenever no credential needs removal. + if url.password().is_none() && (url.user().is_none() || preserve_ssh_git_user) { + return Ok(Self(original.to_owned())); + } + + // SCP-style remotes have no URL authority; remove only `user@` and + // leave the original `host:path` untouched. + let authority_end = value.find(':').unwrap_or(value.len()); + let user_end = value[..authority_end] + .rfind('@') + .ok_or("invalid git remote URL")?; + Ok(Self(format!("{helper_prefix}{}", &value[user_end + 1..]))) + } +} + +impl TryFrom for SanitizedGitUrl { + type Error = &'static str; + + fn try_from(value: String) -> Result { + Self::try_from(value.as_str()) + } +} + +impl From for String { + fn from(value: SanitizedGitUrl) -> Self { + value.0 + } +} + +impl AsRef for SanitizedGitUrl { + fn as_ref(&self) -> &str { + self.as_str() + } +} + +impl Deref for SanitizedGitUrl { + type Target = str; + + fn deref(&self) -> &Self::Target { + self.as_str() + } +} + +impl fmt::Display for SanitizedGitUrl { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +pub(crate) fn deserialize_optional_sanitized_git_url<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + // Malformed legacy origins must not prevent the rest of a rollout from loading. + Ok(Option::::deserialize(deserializer)? + .and_then(|value| SanitizedGitUrl::try_from(value).ok())) +} + +#[cfg(test)] +#[path = "sanitized_git_url_tests.rs"] +mod tests; diff --git a/codex-rs/protocol/src/sanitized_git_url_tests.rs b/codex-rs/protocol/src/sanitized_git_url_tests.rs new file mode 100644 index 0000000000..dc02f22d8a --- /dev/null +++ b/codex-rs/protocol/src/sanitized_git_url_tests.rs @@ -0,0 +1,281 @@ +use super::SanitizedGitUrl; +use crate::protocol::GitInfo; +use pretty_assertions::assert_eq; +use serde::Deserialize; +use serde::de::value::Error; +use serde::de::value::StrDeserializer; +use serde_json::json; + +/// HTTPS usernames and passwords must never survive construction. +#[test] +fn removes_https_username_and_password() { + let url = SanitizedGitUrl::try_from("https://alice:secret@github.com/org/repo.git") + .expect("parse remote URL"); + + assert_eq!(url.as_str(), "https://github.com/org/repo.git"); +} + +/// Tokens placed in the username field are credentials too. +#[test] +fn removes_https_username_without_password() { + let url = SanitizedGitUrl::try_from("https://secret-token@github.com/org/repo.git") + .expect("parse remote URL"); + + assert_eq!(url.as_str(), "https://github.com/org/repo.git"); +} + +/// Percent-encoded credentials must not bypass git-aware URL parsing. +#[test] +fn removes_percent_encoded_credentials() { + let url = SanitizedGitUrl::try_from( + "https://alice%40example.com:secret%3Atoken@github.com/org/repo.git", + ) + .expect("parse remote URL"); + + assert_eq!(url.as_str(), "https://github.com/org/repo.git"); +} + +/// File URLs must lose credentials even though gix stores their userinfo as host text. +#[test] +fn removes_file_url_credentials() { + for (remote, expected) in [ + ( + "file://alice:secret@localhost/repo.git", + "file://localhost/repo.git", + ), + ( + "file://secret-token@server/share/a%2Fb.git", + "file://server/share/a%2Fb.git", + ), + ( + "file://git:secret@server/share/repo.git", + "file://server/share/repo.git", + ), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse file remote URL"); + + assert_eq!(url.as_str(), expected); + } +} + +/// Git remote-helper transports preserve their prefix and sanitize nested URLs. +#[test] +fn sanitizes_remote_helper_urls() { + for (remote, expected) in [ + ( + "hg::https://example.invalid/org/repo.git", + "hg::https://example.invalid/org/repo.git", + ), + ( + "hg::https://alice:secret@example.invalid/org/repo.git", + "hg::https://example.invalid/org/repo.git", + ), + ( + "remote-hg::git@github.com:org/repo.git", + "remote-hg::git@github.com:org/repo.git", + ), + ( + "remote-hg::file://alice:secret@server/share/repo.git", + "remote-hg::file://server/share/repo.git", + ), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse remote helper URL"); + + assert_eq!(url.as_str(), expected); + } +} + +/// Nested helper prefixes must not overflow the stack or repeatedly rebuild the remote. +#[test] +fn sanitizes_deeply_nested_remote_helpers_without_recursion() { + let prefix = "hg::".repeat(4_096); + let remote = format!("{prefix}https://alice:secret@example.invalid/org/repo.git"); + let expected = format!("{prefix}https://example.invalid/org/repo.git"); + + let url = SanitizedGitUrl::try_from(remote.as_str()).expect("parse nested remote helper URL"); + + assert_eq!(url.as_str(), expected); +} + +/// Opaque helper command lines can hide credentials that Git URL parsing cannot identify. +#[test] +fn rejects_remote_helper_command_payloads() { + for remote in [ + "ext::sshpass -p secret-token ssh alice@example.com git-upload-pack /repo", + "ext::ssh alice:secret-token@example.com git-upload-pack /repo", + ] { + let error = SanitizedGitUrl::try_from(remote).expect_err("reject helper command payload"); + + assert_eq!(error, "invalid git remote URL"); + } +} + +/// Removing credentials must not decode reserved characters in repository paths. +#[test] +fn preserves_percent_encoded_path_delimiters() { + for (remote, expected) in [ + ( + "https://example.invalid/org/a%2Fb%3Fc%23d.git", + "https://example.invalid/org/a%2Fb%3Fc%23d.git", + ), + ( + "https://alice:secret@example.invalid/org/a%2Fb%3Fc%23d.git", + "https://example.invalid/org/a%2Fb%3Fc%23d.git", + ), + ( + "hg::https://alice:secret@example.invalid/org/a%2Fb.git", + "hg::https://example.invalid/org/a%2Fb.git", + ), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse percent-encoded remote URL"); + + assert_eq!(url.as_str(), expected); + } +} + +/// The conventional non-secret SSH username is preserved in both Git URL forms. +#[test] +fn preserves_git_ssh_username() { + for remote in [ + "git@github.com:org/repo.git", + "ssh://git@github.com/org/repo.git", + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse remote URL"); + + assert_eq!(url.as_str(), remote); + } +} + +/// SSH passwords are always removed even for the preserved `git` username. +#[test] +fn removes_ssh_password_for_git_username() { + let url = SanitizedGitUrl::try_from("ssh://git:secret@github.com/org/repo.git") + .expect("parse remote URL"); + + assert_eq!(url.as_str(), "ssh://git@github.com/org/repo.git"); +} + +/// Only the exact SSH username `git` is exempt from credential removal. +#[test] +fn removes_other_ssh_usernames() { + for (remote, expected) in [ + ("alice@github.com:org/repo.git", "github.com:org/repo.git"), + ( + "ssh://alice@github.com/org/repo.git", + "ssh://github.com/org/repo.git", + ), + ( + "https://git@github.com/org/repo.git", + "https://github.com/org/repo.git", + ), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse remote URL"); + + assert_eq!(url.as_str(), expected); + } +} + +/// gix-url misidentifies an IPv6 colon as the SCP path separator when a user precedes the host. +#[test] +fn sanitizes_scp_style_ipv6_remotes_with_usernames() { + for (remote, expected) in [ + ( + "git@[2001:db8::1]:org/repo.git", + "git@[2001:db8::1]:org/repo.git", + ), + ( + "alice@[2001:db8::1]:org/repo.git", + "[2001:db8::1]:org/repo.git", + ), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse SCP-style IPv6 remote URL"); + + assert_eq!(url.as_str(), expected); + } +} + +/// IPv6 fallback validation must not interpret opaque SCP paths as URL paths. +#[test] +fn preserves_opaque_scp_style_ipv6_paths() { + for (remote, expected) in [ + ("alice@[::1]:dir name/repo", "[::1]:dir name/repo"), + ("alice@[::1]:repo%FF.git", "[::1]:repo%FF.git"), + ("git@[::1]:dir name/repo", "git@[::1]:dir name/repo"), + ] { + let url = SanitizedGitUrl::try_from(remote).expect("parse opaque SCP-style IPv6 path"); + + assert_eq!(url.as_str(), expected); + } +} + +/// Invalid remotes fail closed without including their embedded secret in errors. +#[test] +fn rejects_malformed_urls_without_exposing_input() { + let error = SanitizedGitUrl::try_from("https://alice:secret-token@[invalid") + .expect_err("reject malformed remote URL"); + + assert_eq!(error, "invalid git remote URL"); + assert!(!error.contains("secret-token")); +} + +/// Deserializing persisted metadata must enforce the constructor invariant. +#[test] +fn deserialization_sanitizes_credentials() { + let deserializer = + StrDeserializer::::new("https://alice:secret-token@github.com/org/repo.git"); + let url = SanitizedGitUrl::deserialize(deserializer).expect("deserialize remote URL"); + + assert_eq!(url.as_str(), "https://github.com/org/repo.git"); +} + +/// Ordinary HTTPS remotes retain their existing wire representation. +#[test] +fn preserves_https_remote_without_credentials() { + let remote = "https://github.com/org/repo.git"; + let url = SanitizedGitUrl::try_from(remote.to_string()).expect("parse remote URL"); + + assert_eq!(String::from(url), remote); +} + +/// Legacy rollout origins must be sanitized as soon as their protocol metadata is parsed. +#[test] +fn git_info_deserialization_sanitizes_legacy_remote_credentials() { + let info: GitInfo = serde_json::from_value(json!({ + "repository_url": "https://alice:secret-token@github.com/org/repo.git", + })) + .expect("deserialize legacy Git metadata"); + + assert_eq!( + info.repository_url.as_deref(), + Some("https://github.com/org/repo.git") + ); +} + +/// An invalid historical remote must not make its entire rollout impossible to resume. +#[test] +fn git_info_deserialization_discards_malformed_legacy_remote() { + let info: GitInfo = serde_json::from_value(json!({ + "repository_url": "https://alice:secret-token@[invalid", + })) + .expect("deserialize Git metadata despite malformed legacy remote"); + + assert_eq!(info.repository_url, None); +} + +/// Older rollout metadata may omit repository URLs entirely. +#[test] +fn git_info_deserialization_accepts_missing_repository_url() { + let info: GitInfo = serde_json::from_value(json!({})) + .expect("deserialize legacy Git metadata without a repository URL"); + + assert_eq!(info.repository_url, None); +} + +/// Explicitly absent repository URLs retain their existing protocol representation. +#[test] +fn git_info_deserialization_accepts_null_repository_url() { + let info: GitInfo = serde_json::from_value(json!({ "repository_url": null })) + .expect("deserialize legacy Git metadata with a null repository URL"); + + assert_eq!(info.repository_url, None); +} diff --git a/codex-rs/rollout/src/list.rs b/codex-rs/rollout/src/list.rs index f75622fced..3c775e3124 100644 --- a/codex-rs/rollout/src/list.rs +++ b/codex-rs/rollout/src/list.rs @@ -25,6 +25,7 @@ use crate::protocol::EventMsg; use crate::state_db; use codex_file_search as file_search; use codex_protocol::RolloutId; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::items::TurnItem; use codex_protocol::protocol::SessionMetaLine; @@ -68,7 +69,7 @@ pub struct ThreadItem { /// Git commit SHA from session metadata. pub git_sha: Option, /// Git origin URL from session metadata. - pub git_origin_url: Option, + pub git_origin_url: Option, /// Session source from session metadata. pub source: Option, /// Persisted thread history contract selected when this thread was created. @@ -108,7 +109,7 @@ struct HeadTailSummary { cwd: Option, git_branch: Option, git_sha: Option, - git_origin_url: Option, + git_origin_url: Option, source: Option, history_mode: ThreadHistoryMode, parent_thread_id: Option, diff --git a/codex-rs/rollout/src/metadata_tests.rs b/codex-rs/rollout/src/metadata_tests.rs index 28a14eb2d6..afea81849f 100644 --- a/codex-rs/rollout/src/metadata_tests.rs +++ b/codex-rs/rollout/src/metadata_tests.rs @@ -8,6 +8,7 @@ use chrono::DateTime; use chrono::NaiveDateTime; use chrono::Timelike; use chrono::Utc; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::protocol::GitInfo; use codex_protocol::protocol::SessionMeta; @@ -315,7 +316,10 @@ async fn backfill_sessions_preserves_existing_git_branch_and_fills_missing_git_f Some(GitInfo { commit_hash: Some(codex_git_utils::GitSha::new("rollout-sha")), branch: Some("rollout-branch".to_string()), - repository_url: Some("git@example.com:openai/codex.git".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("valid git remote URL"), + ), }), ); diff --git a/codex-rs/rollout/src/recorder_tests.rs b/codex-rs/rollout/src/recorder_tests.rs index 22cbcf5dee..42cf907028 100644 --- a/codex-rs/rollout/src/recorder_tests.rs +++ b/codex-rs/rollout/src/recorder_tests.rs @@ -6,6 +6,7 @@ use crate::RolloutItem; use crate::RolloutLine; use crate::config::RolloutConfig; use chrono::TimeZone; +use codex_protocol::SanitizedGitUrl; use codex_protocol::SessionId; use codex_protocol::ThreadId; use codex_protocol::models::ResponseItem; @@ -1471,7 +1472,9 @@ async fn list_threads_metadata_filter_overlays_state_db_list_metadata() -> std:: builder.cwd = home.path().to_path_buf(); builder.git_branch = Some("sqlite-branch".to_string()); builder.git_sha = Some("sqlite-sha".to_string()); - builder.git_origin_url = Some("https://example.com/repo.git".to_string()); + builder.git_origin_url = Some( + SanitizedGitUrl::try_from("https://example.com/repo.git").expect("valid git remote URL"), + ); let mut metadata = builder.build(config.model_provider_id.as_str()); metadata.first_user_message = Some("Hello from user".to_string()); metadata.preview = metadata.first_user_message.clone(); @@ -1521,7 +1524,10 @@ fn fill_missing_thread_item_metadata_preserves_identity_and_prefers_state_git_fi cwd: None, git_branch: Some("filesystem-branch".to_string()), git_sha: Some("filesystem-sha".to_string()), - git_origin_url: Some("https://example.com/filesystem.git".to_string()), + git_origin_url: Some( + SanitizedGitUrl::try_from("https://example.com/filesystem.git") + .expect("valid git remote URL"), + ), source: None, history_mode: Default::default(), parent_thread_id: None, @@ -1547,7 +1553,10 @@ fn fill_missing_thread_item_metadata_preserves_identity_and_prefers_state_git_fi cwd: Some(PathBuf::from("/tmp/state-cwd")), git_branch: Some("state-branch".to_string()), git_sha: Some("state-sha".to_string()), - git_origin_url: Some("https://example.com/state.git".to_string()), + git_origin_url: Some( + SanitizedGitUrl::try_from("https://example.com/state.git") + .expect("valid git remote URL"), + ), source: Some(SessionSource::Exec), history_mode: Default::default(), parent_thread_id: None, diff --git a/codex-rs/state/src/model/thread_metadata.rs b/codex-rs/state/src/model/thread_metadata.rs index c032ff118f..1e2da1c843 100644 --- a/codex-rs/state/src/model/thread_metadata.rs +++ b/codex-rs/state/src/model/thread_metadata.rs @@ -1,6 +1,7 @@ use anyhow::Result; use chrono::DateTime; use chrono::Utc; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::AskForApproval; @@ -183,7 +184,7 @@ pub struct ThreadMetadata { /// The git branch name, if known. pub git_branch: Option, /// The git origin URL, if known. - pub git_origin_url: Option, + pub git_origin_url: Option, } /// Builder data required to construct [`ThreadMetadata`] without parsing filenames. @@ -228,7 +229,7 @@ pub struct ThreadMetadataBuilder { /// The git branch name, if known. pub git_branch: Option, /// The git origin URL, if known. - pub git_origin_url: Option, + pub git_origin_url: Option, } impl ThreadMetadataBuilder { @@ -626,7 +627,8 @@ impl TryFrom for ThreadMetadata { project_id, git_sha, git_branch, - git_origin_url, + git_origin_url: git_origin_url + .and_then(|origin_url| SanitizedGitUrl::try_from(origin_url).ok()), }) } } @@ -693,6 +695,7 @@ mod tests { use super::ThreadRow; use chrono::DateTime; use chrono::Utc; + use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::ThreadHistoryMode; @@ -810,7 +813,10 @@ mod tests { reconciled.history_mode = ThreadHistoryMode::Paginated; reconciled.git_sha = Some("rollout-sha".to_string()); reconciled.git_branch = Some("rollout-branch".to_string()); - reconciled.git_origin_url = Some("rollout-origin".to_string()); + reconciled.git_origin_url = Some( + SanitizedGitUrl::try_from("https://example.com/rollout-origin") + .expect("valid git remote URL"), + ); let existing = expected_thread_metadata(/*reasoning_effort*/ None); let expected = reconciled.clone(); diff --git a/codex-rs/state/src/runtime/threads.rs b/codex-rs/state/src/runtime/threads.rs index 237cc3b08b..86200c59ca 100644 --- a/codex-rs/state/src/runtime/threads.rs +++ b/codex-rs/state/src/runtime/threads.rs @@ -1,5 +1,6 @@ use super::*; use crate::SortDirection; +use codex_protocol::SanitizedGitUrl; use codex_protocol::protocol::SessionSource; use std::sync::atomic::AtomicI64; use std::sync::atomic::Ordering; @@ -847,7 +848,7 @@ impl StateRuntime { thread_id: ThreadId, git_sha: Option>, git_branch: Option>, - git_origin_url: Option>, + git_origin_url: Option>, ) -> anyhow::Result { let result = sqlx::query( r#" @@ -864,7 +865,7 @@ WHERE id = ? .bind(git_branch.is_some()) .bind(git_branch.flatten()) .bind(git_origin_url.is_some()) - .bind(git_origin_url.flatten()) + .bind(git_origin_url.flatten().map(SanitizedGitUrl::as_str)) .bind(thread_id.to_string()) .execute(self.pool.as_ref()) .await?; @@ -2661,7 +2662,10 @@ mod tests { git: Some(GitInfo { commit_hash: Some(codex_git_utils::GitSha::new("rollout-sha")), branch: Some("rollout-branch".to_string()), - repository_url: Some("git@example.com:openai/codex.git".to_string()), + repository_url: Some( + SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("valid git remote URL"), + ), }), })]; @@ -2700,7 +2704,10 @@ mod tests { let mut metadata = test_thread_metadata(&codex_home, thread_id, codex_home.clone()); metadata.git_sha = Some("sqlite-sha".to_string()); metadata.git_branch = Some("sqlite-branch".to_string()); - metadata.git_origin_url = Some("git@example.com:openai/codex.git".to_string()); + metadata.git_origin_url = Some( + SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("valid git remote URL"), + ); runtime .upsert_thread(&metadata) @@ -2710,7 +2717,10 @@ mod tests { let mut rollout_metadata = metadata.clone(); rollout_metadata.git_sha = Some("rollout-sha".to_string()); rollout_metadata.git_branch = Some("rollout-branch".to_string()); - rollout_metadata.git_origin_url = Some("https://example.com/repo.git".to_string()); + rollout_metadata.git_origin_url = Some( + SanitizedGitUrl::try_from("https://example.com/repo.git") + .expect("valid git remote URL"), + ); runtime .upsert_thread(&rollout_metadata) @@ -2849,7 +2859,10 @@ mod tests { thread_id, Some(Some("abc123")), Some(Some("feature/branch")), - Some(Some("git@example.com:openai/codex.git")), + Some(Some( + &SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("valid git remote URL"), + )), ) .await .expect("git info update should succeed"); @@ -2940,7 +2953,10 @@ mod tests { let mut metadata = test_thread_metadata(&codex_home, thread_id, codex_home.clone()); metadata.git_sha = Some("abc123".to_string()); metadata.git_branch = Some("feature/branch".to_string()); - metadata.git_origin_url = Some("git@example.com:openai/codex.git".to_string()); + metadata.git_origin_url = Some( + SanitizedGitUrl::try_from("git@example.com:openai/codex.git") + .expect("valid git remote URL"), + ); runtime .upsert_thread(&metadata) diff --git a/codex-rs/thread-store/src/local/helpers.rs b/codex-rs/thread-store/src/local/helpers.rs index 1323252e82..a4f638945d 100644 --- a/codex-rs/thread-store/src/local/helpers.rs +++ b/codex-rs/thread-store/src/local/helpers.rs @@ -10,6 +10,7 @@ use std::time::SystemTime; use chrono::DateTime; use chrono::Utc; use codex_git_utils::GitSha; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; @@ -309,7 +310,7 @@ fn parse_legacy_sandbox_policy(value: &str) -> serde_json::Result pub(super) fn git_info_from_parts( sha: Option, branch: Option, - origin_url: Option, + origin_url: Option, ) -> Option { if sha.is_none() && branch.is_none() && origin_url.is_none() { return None; diff --git a/codex-rs/thread-store/src/local/update_thread_metadata.rs b/codex-rs/thread-store/src/local/update_thread_metadata.rs index b4812cb2c7..3ec6f4d680 100644 --- a/codex-rs/thread-store/src/local/update_thread_metadata.rs +++ b/codex-rs/thread-store/src/local/update_thread_metadata.rs @@ -2,6 +2,7 @@ use std::path::Path; use std::path::PathBuf; use chrono::Utc; +use codex_protocol::SanitizedGitUrl; use codex_protocol::ThreadId; use codex_protocol::protocol::GitInfo; use codex_protocol::protocol::SessionSource; @@ -708,7 +709,7 @@ async fn apply_thread_git_info_patch( git_info .origin_url .as_ref() - .map(|origin_url| origin_url.as_deref()), + .map(|origin_url| origin_url.as_ref()), ) .await .map_err(|err| ThreadStoreError::Internal { @@ -728,7 +729,7 @@ async fn apply_thread_git_info( thread_id: ThreadId, sha: &Option, branch: &Option, - origin_url: &Option, + origin_url: &Option, ) -> ThreadStoreResult<()> { let Some(state_db) = store.state_db().await else { return Err(ThreadStoreError::Internal { @@ -740,7 +741,7 @@ async fn apply_thread_git_info( thread_id, Some(sha.as_deref()), Some(branch.as_deref()), - Some(origin_url.as_deref()), + Some(origin_url.as_ref()), ) .await .map_err(|err| ThreadStoreError::Internal { @@ -758,7 +759,7 @@ async fn apply_thread_git_info( fn resolve_git_info_patch( existing: Option, git_info: GitInfoPatch, -) -> (Option, Option, Option) { +) -> (Option, Option, Option) { let (existing_sha, existing_branch, existing_origin_url) = match existing { Some(info) => ( info.commit_hash.map(|sha| sha.0), @@ -778,7 +779,7 @@ async fn apply_thread_git_info_to_rollout( thread_id: ThreadId, sha: &Option, branch: &Option, - origin_url: &Option, + origin_url: &Option, memory_mode: Option<&str>, ) -> ThreadStoreResult<()> { let mut session_meta = @@ -1435,7 +1436,10 @@ mod tests { git_info: Some(GitInfoPatch { sha: Some(Some("abc123".to_string())), branch: Some(Some("main".to_string())), - origin_url: Some(Some("https://github.com/openai/codex".to_string())), + origin_url: Some(Some( + SanitizedGitUrl::try_from("https://github.com/openai/codex") + .expect("valid git remote URL"), + )), }), ..Default::default() }, @@ -1536,7 +1540,10 @@ mod tests { git_info: Some(GitInfoPatch { sha: Some(Some("abc123".to_string())), branch: Some(Some("main".to_string())), - origin_url: Some(Some("https://github.com/openai/codex".to_string())), + origin_url: Some(Some( + SanitizedGitUrl::try_from("https://github.com/openai/codex") + .expect("valid git remote URL"), + )), }), ..Default::default() }, @@ -1596,7 +1603,10 @@ mod tests { git_info: Some(GitInfoPatch { sha: Some(Some("abc123".to_string())), branch: Some(Some("main".to_string())), - origin_url: Some(Some("https://github.com/openai/codex".to_string())), + origin_url: Some(Some( + SanitizedGitUrl::try_from("https://github.com/openai/codex") + .expect("valid git remote URL"), + )), }), ..Default::default() }, diff --git a/codex-rs/thread-store/src/types.rs b/codex-rs/thread-store/src/types.rs index 496a076478..5cedf0be09 100644 --- a/codex-rs/thread-store/src/types.rs +++ b/codex-rs/thread-store/src/types.rs @@ -5,6 +5,7 @@ use chrono::DateTime; use chrono::Utc; use codex_app_server_protocol::CodexErrorInfo; use codex_app_server_protocol::ThreadTimelineEntry; +use codex_protocol::SanitizedGitUrl; use codex_protocol::SessionId; use codex_protocol::ThreadId; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -663,7 +664,7 @@ pub struct GitInfoPatch { skip_serializing_if = "Option::is_none", with = "optional_option" )] - pub origin_url: ClearableField, + pub origin_url: ClearableField, } impl GitInfoPatch {