mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
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
This commit is contained in:
committed by
copyberry
parent
85de07048c
commit
61a991990f
@@ -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]
|
||||
|
||||
@@ -583,6 +583,22 @@ fn primary_runtime_cache_dir() -> Option<PathBuf> {
|
||||
}
|
||||
|
||||
fn git_hostname(url: &str) -> Option<String> {
|
||||
// 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);
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
] {
|
||||
|
||||
Reference in New Issue
Block a user