From 61a991990fa4fd0c511ccaaab3875371796be956 Mon Sep 17 00:00:00 2001 From: willwang-openai Date: Thu, 22 Oct 2026 20:49:06 +0000 Subject: [PATCH] Reject backslashes in HTTP(S) marketplace URL userinfo (#56263) ## Why The URL parser treats backslashes as HTTP(S) path separators, while Git/libcurl does not. This difference can make a marketplace URL appear to match an allowed host pattern while Git connects to a different host. ## What changed Reject HTTP(S) URLs containing literal backslashes in userinfo when checking marketplace host patterns. Continue allowing encoded backslashes in userinfo and backslashes in paths or query strings. ## Testing Expand regression coverage to verify that denied URLs never invoke the cloner, create the marketplace install directory, or write configuration. Extend host-pattern tests to cover accepted HTTP(S) URL variants. GitOrigin-RevId: cfdd29d6e83fb1718b505abaae85d37257317953 --- codex-rs/core-plugins/src/marketplace_add.rs | 65 ++++++++++++------- .../core-plugins/src/marketplace_policy.rs | 16 +++++ .../src/marketplace_policy_tests.rs | 6 ++ 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/codex-rs/core-plugins/src/marketplace_add.rs b/codex-rs/core-plugins/src/marketplace_add.rs index 8a0e46cca4..e834ede250 100644 --- a/codex-rs/core-plugins/src/marketplace_add.rs +++ b/codex-rs/core-plugins/src/marketplace_add.rs @@ -270,7 +270,6 @@ mod tests { #[test] fn denied_git_marketplace_does_not_clone_or_create_install_root() { - let codex_home = TempDir::new().expect("create Codex home"); let requirements = requirements( r#" [marketplaces] @@ -279,34 +278,50 @@ restrict_to_allowed_sources = true [marketplaces.allowed_sources.company] source = "git" url = "https://github.com/example/allowed.git" + +[marketplaces.allowed_sources.internal] +source = "host_pattern" +host_pattern = '^allowed\.test$' "#, ); - let cloner_called = Cell::new(false); + for source in [ + "https://github.com/example/blocked.git", + "https://127.0.0.1:18767/r.git", + "https://allowed.test@127.0.0.1:18767/r.git", + r"http://allowed.test\@127.0.0.1:18767/r.git", + r"https://allowed.test\@127.0.0.1:18767/r.git", + r"http://allowed.test\anything@127.0.0.1:18767/r.git", + r"https://allowed.test\name:password@127.0.0.1:18767/r.git", + r"https:///allowed.test\anything@127.0.0.1:18767/r.git", + ] { + let codex_home = TempDir::new().expect("create Codex home"); + let cloner_called = Cell::new(false); - let err = add_marketplace_sync_with_cloner( - codex_home.path(), - &requirements, - MarketplaceAddRequest { - source: "https://github.com/example/blocked.git".to_string(), - ref_name: None, - sparse_paths: Vec::new(), - }, - |_url, _ref_name, _sparse_paths, _destination| { - cloner_called.set(true); - Ok(()) - }, - ) - .expect_err("blocked marketplace should fail"); + let err = add_marketplace_sync_with_cloner( + codex_home.path(), + &requirements, + MarketplaceAddRequest { + source: source.to_string(), + ref_name: None, + sparse_paths: Vec::new(), + }, + |_url, _ref_name, _sparse_paths, _destination| { + cloner_called.set(true); + Ok(()) + }, + ) + .expect_err("blocked marketplace should fail"); - assert!(err.to_string().contains("is not allowed by requirements")); - assert!(!cloner_called.get()); - assert!(!marketplace_install_root(codex_home.path()).exists()); - assert!( - !codex_home - .path() - .join(codex_config::CONFIG_TOML_FILE) - .exists() - ); + assert!(err.to_string().contains("is not allowed by requirements")); + assert!(!cloner_called.get(), "cloned denied source: {source}"); + assert!(!marketplace_install_root(codex_home.path()).exists()); + assert!( + !codex_home + .path() + .join(codex_config::CONFIG_TOML_FILE) + .exists() + ); + } } #[test] diff --git a/codex-rs/core-plugins/src/marketplace_policy.rs b/codex-rs/core-plugins/src/marketplace_policy.rs index 64b0c38e89..b2580aa2ce 100644 --- a/codex-rs/core-plugins/src/marketplace_policy.rs +++ b/codex-rs/core-plugins/src/marketplace_policy.rs @@ -583,6 +583,22 @@ fn primary_runtime_cache_dir() -> Option { } fn git_hostname(url: &str) -> Option { + // Url treats backslashes as HTTP(S) path separators, but Git/libcurl does not. + // A backslash anywhere in userinfo can therefore hide the host after the final @. + if let Some(authority_and_path) = url + .strip_prefix("http://") + .or_else(|| url.strip_prefix("https://")) + { + let authority = authority_and_path + .trim_start_matches('/') + .split(['/', '?', '#']) + .next()?; + if let Some((userinfo, _)) = authority.rsplit_once('@') + && userinfo.contains('\\') + { + return None; + } + } if let Ok(url) = Url::parse(url) { return url.host_str().map(str::to_ascii_lowercase); } diff --git a/codex-rs/core-plugins/src/marketplace_policy_tests.rs b/codex-rs/core-plugins/src/marketplace_policy_tests.rs index f838b064fb..703341b29a 100644 --- a/codex-rs/core-plugins/src/marketplace_policy_tests.rs +++ b/codex-rs/core-plugins/src/marketplace_policy_tests.rs @@ -164,7 +164,13 @@ ref = "ignored" ); for source in [ + "http://git.example.com/team/plugins.git", "https://git.example.com/team/plugins.git", + "https://user@git.example.com:8443/team/plugins.git", + "https://user%5Cname@git.example.com/team/plugins.git", + r"http://git.example.com/team\plugins.git", + r"https://git.example.com/team\@plugins.git", + r"https://git.example.com?repository=team\@plugins.git", "ssh://git@git.example.com/team/plugins.git", "git@git.example.com:team/plugins.git", ] {