mirror of
https://github.com/openai/codex.git
synced 2026-11-14 21:40:39 +00:00
Harden Git remote collection and limit remote URL metadata (#61488)
## Why Parsing `git remote -v` can mistake partial clone filter text for remote entries. Large remote URLs can also push turn metadata beyond the Responses API's 8,096-character limit. ## What changed - Read fetch URLs from NUL-delimited Git configuration, sanitize credentials, and let Git resolve `insteadOf` rewrites and empty URL resets without contacting remotes. - Resolve ambiguous URLs with bounded concurrency and a shared deadline, and expose an observer that publishes verified URLs as they become available. - When turn metadata exceeds the limit, fit complete remote URLs into the remaining budget while preserving other metadata. Prioritize `origin`, then shorter entries; omit URLs rather than truncate repository identities. Apply this to both client metadata and compatibility headers. ## Testing Add regression coverage for partial clone filter injection, URL rewrites and resets, malformed and non-UTF-8 configuration, `GIT_CONFIG` overrides, progressive updates, and long remote URLs. Add metadata tests for the exact size boundary, oversized remote omission, ASCII serialization, and preservation of other fields. GitOrigin-RevId: 522ffe291db6113ca9856fc98309609c8abe4ac0
This commit is contained in:
@@ -47,6 +47,9 @@ pub(crate) const ANALYTICS_ENABLED_KEY: &str = "analytics_enabled";
|
||||
pub(crate) const HISTORY_INITIALIZATION_KEY: &str = "history_initialization";
|
||||
pub(crate) const MCP_ATTRIBUTION_CLIENT_METADATA_KEY: &str = "mcp_attribution";
|
||||
pub(crate) const MAX_MCP_ATTRIBUTION_BYTES: usize = 16 * 1024;
|
||||
// ResponsesAPI rejects a whole client_metadata value above this character limit.
|
||||
// Turn metadata is ASCII JSON, so its byte and character lengths are identical.
|
||||
const MAX_TURN_METADATA_BYTES: usize = 8096;
|
||||
|
||||
pub(crate) const FORKED_FROM_THREAD_ID_KEY: &str = "forked_from_thread_id";
|
||||
pub(crate) const FORKED_FROM_ORDINAL_EXCLUSIVE_KEY: &str = "forked_from_ordinal_exclusive";
|
||||
@@ -328,7 +331,7 @@ impl CodexResponsesMetadata {
|
||||
}
|
||||
|
||||
pub(crate) fn turn_metadata_json(&self) -> Option<String> {
|
||||
to_ascii_json_string(&self.turn_metadata_payload()).ok()
|
||||
bounded_turn_metadata_json(self.turn_metadata_payload()).ok()
|
||||
}
|
||||
|
||||
pub(crate) fn turn_metadata_value(&self) -> Option<Value> {
|
||||
@@ -393,7 +396,7 @@ impl CodexResponsesMetadata {
|
||||
// Direct x-codex-turn-metadata is compatibility output. Keep the unbounded tool inventory
|
||||
// in client_metadata only so HTTP and WebSocket compatibility headers remain bounded.
|
||||
if self.has_turn_metadata()
|
||||
&& let Ok(turn_metadata_json) = to_ascii_json_string(&CodexTurnMetadataPayload {
|
||||
&& let Ok(turn_metadata_json) = bounded_turn_metadata_json(CodexTurnMetadataPayload {
|
||||
tool_namespaces_info: None,
|
||||
..self.turn_metadata_payload()
|
||||
})
|
||||
@@ -555,7 +558,78 @@ fn non_empty_workspaces(
|
||||
(!workspaces.is_empty()).then_some(workspaces)
|
||||
}
|
||||
|
||||
#[derive(Serialize)]
|
||||
fn bounded_turn_metadata_json(payload: CodexTurnMetadataPayload<'_>) -> serde_json::Result<String> {
|
||||
let full = to_ascii_json_string(&payload)?;
|
||||
if full.len() <= MAX_TURN_METADATA_BYTES {
|
||||
return Ok(full);
|
||||
}
|
||||
let Some(original_workspaces) = payload.workspaces else {
|
||||
return Ok(full);
|
||||
};
|
||||
|
||||
// Reserve all non-remote metadata before fitting complete URLs. A remote must
|
||||
// never be shortened into a different repository identity.
|
||||
let mut workspaces = original_workspaces.clone();
|
||||
let mut candidates = Vec::new();
|
||||
for path in original_workspaces.keys() {
|
||||
let Some(workspace) = workspaces.get_mut(path) else {
|
||||
continue;
|
||||
};
|
||||
let Some(remotes) = workspace.associated_remote_urls.take() else {
|
||||
continue;
|
||||
};
|
||||
if remotes.is_empty() {
|
||||
workspace.associated_remote_urls = Some(remotes);
|
||||
continue;
|
||||
}
|
||||
let without_remotes_len = to_ascii_json_string(workspace)?.len();
|
||||
workspace.associated_remote_urls = Some(BTreeMap::new());
|
||||
let first_map_overhead = to_ascii_json_string(workspace)?.len() - without_remotes_len;
|
||||
workspace.associated_remote_urls = None;
|
||||
for (name, url) in remotes {
|
||||
// [name,url] and name:url differ by the two array delimiters.
|
||||
let entry_len = to_ascii_json_string(&(&name, &url))?.len() - 2;
|
||||
candidates.push((path.as_str(), name, url, entry_len, first_map_overhead));
|
||||
}
|
||||
}
|
||||
let mut projected_len = to_ascii_json_string(&CodexTurnMetadataPayload {
|
||||
workspaces: Some(&workspaces),
|
||||
..payload.clone()
|
||||
})?
|
||||
.len();
|
||||
if projected_len > MAX_TURN_METADATA_BYTES || candidates.is_empty() {
|
||||
return Ok(full);
|
||||
}
|
||||
candidates.sort_unstable_by(|a, b| {
|
||||
(a.1 != "origin")
|
||||
.cmp(&(b.1 != "origin"))
|
||||
.then_with(|| a.3.cmp(&b.3))
|
||||
.then_with(|| a.0.cmp(b.0))
|
||||
.then_with(|| a.1.cmp(&b.1))
|
||||
});
|
||||
for (path, name, url, entry_len, first_map_overhead) in candidates {
|
||||
let Some(workspace) = workspaces.get_mut(path) else {
|
||||
continue;
|
||||
};
|
||||
let overhead = match &workspace.associated_remote_urls {
|
||||
Some(_) => 1,
|
||||
None => first_map_overhead,
|
||||
};
|
||||
if entry_len + overhead <= MAX_TURN_METADATA_BYTES - projected_len {
|
||||
projected_len += entry_len + overhead;
|
||||
workspace
|
||||
.associated_remote_urls
|
||||
.get_or_insert_default()
|
||||
.insert(name, url);
|
||||
}
|
||||
}
|
||||
to_ascii_json_string(&CodexTurnMetadataPayload {
|
||||
workspaces: Some(&workspaces),
|
||||
..payload
|
||||
})
|
||||
}
|
||||
|
||||
#[derive(Clone, Serialize)]
|
||||
struct CodexTurnMetadataPayload<'a> {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
guardian_recorded_items: Option<bool>,
|
||||
@@ -620,3 +694,7 @@ struct CodexTurnMetadataPayload<'a> {
|
||||
#[serde(flatten)]
|
||||
extra: &'a BTreeMap<String, String>,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "responses_metadata_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
#![cfg(test)]
|
||||
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
|
||||
fn metadata_with_remote(url: &str) -> CodexResponsesMetadata {
|
||||
let mut metadata = CodexResponsesMetadata::new(
|
||||
"install".into(),
|
||||
"session".into(),
|
||||
"thread".into(),
|
||||
"window".into(),
|
||||
);
|
||||
metadata.request_kind = Some(CodexResponsesRequestKind::Turn);
|
||||
metadata.turn_id = Some("turn".into());
|
||||
metadata.workspaces.insert(
|
||||
"/workspace/東京".into(),
|
||||
TurnMetadataWorkspace {
|
||||
associated_remote_urls: Some(BTreeMap::from([(
|
||||
"origin".into(),
|
||||
SanitizedGitUrl::try_from(url).unwrap(),
|
||||
)])),
|
||||
latest_git_commit_hash: Some("abc123".into()),
|
||||
has_changes: Some(false),
|
||||
},
|
||||
);
|
||||
metadata
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn turn_metadata_keeps_the_exact_boundary_and_omits_an_overlong_remote() {
|
||||
let prefix = "https://example.invalid/";
|
||||
let initial_len = metadata_with_remote(prefix)
|
||||
.turn_metadata_json()
|
||||
.unwrap()
|
||||
.len();
|
||||
let padding = "x".repeat(MAX_TURN_METADATA_BYTES - initial_len);
|
||||
let boundary_url = format!("{prefix}{padding}");
|
||||
let boundary = metadata_with_remote(&boundary_url);
|
||||
let expected = to_ascii_json_string(&boundary.turn_metadata_payload()).unwrap();
|
||||
assert_eq!(expected.len(), MAX_TURN_METADATA_BYTES);
|
||||
assert_eq!(boundary.turn_metadata_json().unwrap(), expected);
|
||||
|
||||
let over = metadata_with_remote(&format!("{boundary_url}x"));
|
||||
let body = over.client_metadata(/*include_internal*/ false);
|
||||
let canonical = &body[X_CODEX_TURN_METADATA_HEADER];
|
||||
assert_eq!(
|
||||
canonical,
|
||||
over.compatibility_headers()[X_CODEX_TURN_METADATA_HEADER]
|
||||
.to_str()
|
||||
.unwrap()
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::from_str::<Value>(canonical).unwrap()["workspaces"]["/workspace/東京"],
|
||||
json!({"latest_git_commit_hash":"abc123","has_changes":false})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oversized_remote_preserves_other_metadata_and_short_remote_urls() {
|
||||
let mut metadata =
|
||||
metadata_with_remote(&format!("https://example.invalid/{}é", "o".repeat(5000)));
|
||||
let workspace = metadata.workspaces.get_mut("/workspace/東京").unwrap();
|
||||
let remotes = workspace.associated_remote_urls.as_mut().unwrap();
|
||||
remotes.insert(
|
||||
"large".into(),
|
||||
SanitizedGitUrl::try_from(format!("https://example.invalid/{}", "x".repeat(5000)).as_str())
|
||||
.unwrap(),
|
||||
);
|
||||
remotes.insert(
|
||||
"upstream".into(),
|
||||
SanitizedGitUrl::try_from("git@example.invalid:upstream.git").unwrap(),
|
||||
);
|
||||
metadata.extra.insert("custom".into(), "retained".into());
|
||||
metadata.tool_namespaces_info = Some(BTreeMap::from([(
|
||||
"shell".into(),
|
||||
TurnToolNamespaceInfo {
|
||||
name: "shell".into(),
|
||||
functions: BTreeMap::new(),
|
||||
},
|
||||
)]));
|
||||
let body = metadata.client_metadata(/*include_internal*/ true);
|
||||
let canonical = &body[X_CODEX_TURN_METADATA_HEADER];
|
||||
assert!(canonical.len() <= MAX_TURN_METADATA_BYTES);
|
||||
assert!(canonical.is_ascii());
|
||||
let actual: Value = serde_json::from_str(canonical).unwrap();
|
||||
let mut expected = metadata.turn_metadata_value().unwrap();
|
||||
expected["workspaces"]["/workspace/東京"]["associated_remote_urls"]
|
||||
.as_object_mut()
|
||||
.unwrap()
|
||||
.remove("large");
|
||||
assert_eq!(actual, expected);
|
||||
let header = metadata.compatibility_headers();
|
||||
let header: Value =
|
||||
serde_json::from_str(header[X_CODEX_TURN_METADATA_HEADER].to_str().unwrap()).unwrap();
|
||||
assert_eq!(header["workspaces"], expected["workspaces"]);
|
||||
assert!(header.get("tool_namespaces_info").is_none());
|
||||
}
|
||||
@@ -26,7 +26,7 @@ serde = { workspace = true, features = ["derive"] }
|
||||
similar = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
thiserror = { workspace = true }
|
||||
tokio = { workspace = true, features = ["macros", "process", "rt", "time"] }
|
||||
tokio = { workspace = true, features = ["macros", "process", "rt", "sync", "time"] }
|
||||
ts-rs = { workspace = true, features = [
|
||||
"uuid-impl",
|
||||
"serde-json-impl",
|
||||
|
||||
+18
-101
@@ -1,4 +1,9 @@
|
||||
use std::collections::BTreeMap;
|
||||
mod remotes;
|
||||
|
||||
pub use remotes::get_git_remote_urls;
|
||||
pub use remotes::get_git_remote_urls_assume_git_repo;
|
||||
pub use remotes::observe_git_remote_urls;
|
||||
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
@@ -122,32 +127,6 @@ pub async fn collect_git_info(cwd: &Path) -> Option<GitInfo> {
|
||||
Some(git_info)
|
||||
}
|
||||
|
||||
/// Collect fetch remotes in a multi-root-friendly format: {"origin": "https://..."}.
|
||||
pub async fn get_git_remote_urls(cwd: &Path) -> Option<BTreeMap<String, SanitizedGitUrl>> {
|
||||
let is_git_repo = run_git_command_with_timeout(&["rev-parse", "--git-dir"], cwd)
|
||||
.await?
|
||||
.status
|
||||
.success();
|
||||
if !is_git_repo {
|
||||
return None;
|
||||
}
|
||||
|
||||
get_git_remote_urls_assume_git_repo(cwd).await
|
||||
}
|
||||
|
||||
/// 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<BTreeMap<String, SanitizedGitUrl>> {
|
||||
let output = run_git_command_with_timeout(&["remote", "-v"], cwd).await?;
|
||||
if !output.status.success() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let stdout = String::from_utf8(output.stdout).ok()?;
|
||||
parse_git_remote_urls(stdout.as_str())
|
||||
}
|
||||
|
||||
/// Return the current HEAD commit hash without checking whether `cwd` is in a git repo.
|
||||
pub async fn get_head_commit_hash(cwd: &Path) -> Option<GitSha> {
|
||||
let output = run_git_command_with_timeout(&["rev-parse", "HEAD"], cwd).await?;
|
||||
@@ -266,35 +245,6 @@ fn trim_git_suffix(value: &str) -> &str {
|
||||
value.strip_suffix(".git").unwrap_or(value)
|
||||
}
|
||||
|
||||
fn parse_git_remote_urls(stdout: &str) -> Option<BTreeMap<String, SanitizedGitUrl>> {
|
||||
let mut remotes = BTreeMap::new();
|
||||
for line in stdout.lines() {
|
||||
let Some(fetch_line) = line.strip_suffix(" (fetch)") else {
|
||||
continue;
|
||||
};
|
||||
|
||||
let Some((name, url_part)) = fetch_line
|
||||
.split_once('\t')
|
||||
.or_else(|| fetch_line.split_once(' '))
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
|
||||
let url = url_part.trim_start();
|
||||
if !url.is_empty()
|
||||
&& let Ok(url) = SanitizedGitUrl::try_from(url)
|
||||
{
|
||||
remotes.insert(name.to_string(), url);
|
||||
}
|
||||
}
|
||||
|
||||
if remotes.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(remotes)
|
||||
}
|
||||
}
|
||||
|
||||
/// A minimal commit summary entry used for pickers (subject + timestamp + sha).
|
||||
#[derive(Clone, Debug, Serialize, Deserialize)]
|
||||
pub struct CommitLogEntry {
|
||||
@@ -417,6 +367,16 @@ pub(crate) async fn run_git_command_with_timeout_from(
|
||||
cwd: &Path,
|
||||
fsmonitor: crate::FsmonitorOverride,
|
||||
) -> Option<std::process::Output> {
|
||||
let mut command = local_git_command(git, args, cwd, fsmonitor);
|
||||
run_git_command_with_timeout_output(&mut command, GIT_COMMAND_TIMEOUT).await
|
||||
}
|
||||
|
||||
pub(crate) fn local_git_command(
|
||||
git: &Path,
|
||||
args: &[&str],
|
||||
cwd: &Path,
|
||||
fsmonitor: crate::FsmonitorOverride,
|
||||
) -> Command {
|
||||
let mut command = Command::new(git);
|
||||
command
|
||||
.env("GIT_OPTIONAL_LOCKS", "0")
|
||||
@@ -427,7 +387,7 @@ pub(crate) async fn run_git_command_with_timeout_from(
|
||||
.args(["-c", fsmonitor.git_config_arg()])
|
||||
.args(args)
|
||||
.current_dir(cwd);
|
||||
run_git_command_with_timeout_output(&mut command, GIT_COMMAND_TIMEOUT).await
|
||||
command
|
||||
}
|
||||
|
||||
async fn get_git_remotes(cwd: &Path) -> Option<Vec<String>> {
|
||||
@@ -847,6 +807,7 @@ pub async fn current_branch_name(cwd: &Path) -> Option<String> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::BTreeMap;
|
||||
use std::process::Stdio;
|
||||
|
||||
#[tokio::test]
|
||||
@@ -938,50 +899,6 @@ 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() {
|
||||
|
||||
@@ -0,0 +1,207 @@
|
||||
//! Collects sanitized fetch URLs from NUL-delimited Git configuration without contacting remotes.
|
||||
//! Git resolves rewritten or reset URLs within the same collection deadline.
|
||||
|
||||
use std::collections::BTreeMap;
|
||||
use std::path::Path;
|
||||
|
||||
use futures::FutureExt;
|
||||
use futures::StreamExt;
|
||||
use futures::stream;
|
||||
|
||||
use super::DISABLED_HOOKS_PATH;
|
||||
use super::GIT_COMMAND_TIMEOUT;
|
||||
use super::local_git_command;
|
||||
use super::run_git_command_with_timeout;
|
||||
use crate::SanitizedGitUrl;
|
||||
use crate::git_process::run_git_command_with_timeout_output;
|
||||
|
||||
const MAX_CONCURRENT_REMOTE_LOOKUPS: usize = 8;
|
||||
|
||||
/// Collect fetch remotes in a multi-root-friendly format: {"origin": "https://..."}.
|
||||
pub async fn get_git_remote_urls(cwd: &Path) -> Option<BTreeMap<String, SanitizedGitUrl>> {
|
||||
let is_git_repo = run_git_command_with_timeout(&["rev-parse", "--git-dir"], cwd)
|
||||
.await?
|
||||
.status
|
||||
.success();
|
||||
if !is_git_repo {
|
||||
return None;
|
||||
}
|
||||
|
||||
get_git_remote_urls_assume_git_repo(cwd).await
|
||||
}
|
||||
|
||||
/// 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<BTreeMap<String, SanitizedGitUrl>> {
|
||||
get_git_remote_urls_with_progress(cwd, |_| {}).await
|
||||
}
|
||||
|
||||
/// Publishes verified fetch URLs as they become available, keeping previously resolved names.
|
||||
pub async fn observe_git_remote_urls(
|
||||
cwd: &Path,
|
||||
updates: &tokio::sync::watch::Sender<BTreeMap<String, SanitizedGitUrl>>,
|
||||
) {
|
||||
get_git_remote_urls_with_progress(cwd, |urls| {
|
||||
updates.send_modify(|known| known.extend(urls.clone()));
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Publishes unambiguous configured URLs, then each successful native resolution.
|
||||
async fn get_git_remote_urls_with_progress(
|
||||
cwd: &Path,
|
||||
mut publish_verified: impl FnMut(&BTreeMap<String, SanitizedGitUrl>),
|
||||
) -> Option<BTreeMap<String, SanitizedGitUrl>> {
|
||||
let deadline = tokio::time::Instant::now() + GIT_COMMAND_TIMEOUT;
|
||||
// `git remote` ignores GIT_CONFIG, but `git config` treats it as an explicit
|
||||
// file. Clear it so the machine-readable query observes the same configuration.
|
||||
let mut command = local_git_command(
|
||||
Path::new("git"),
|
||||
&["config", "--null", "--get-regexp", r"^(remote\.|url\.)"],
|
||||
cwd,
|
||||
crate::FsmonitorOverride::Disabled,
|
||||
);
|
||||
command.env_remove("GIT_CONFIG");
|
||||
let config = run_git_command_with_timeout_output(&mut command, GIT_COMMAND_TIMEOUT).await?;
|
||||
if !config.status.success() {
|
||||
return None;
|
||||
}
|
||||
let mut configured = BTreeMap::<&str, Vec<&[u8]>>::new();
|
||||
let mut rewrite_prefixes = Vec::new();
|
||||
for entry in config.stdout.split(|byte| *byte == b'\0') {
|
||||
if entry.is_empty() {
|
||||
continue;
|
||||
}
|
||||
let separator = entry
|
||||
.iter()
|
||||
.position(|byte| *byte == b'\n')
|
||||
.unwrap_or(entry.len());
|
||||
let (key, value) = entry.split_at(separator);
|
||||
if let Some(name) = key
|
||||
.strip_prefix(b"remote.")
|
||||
.and_then(|key| key.strip_suffix(b".url"))
|
||||
.and_then(|name| std::str::from_utf8(name).ok())
|
||||
{
|
||||
// Git interprets an empty remote subsection as the name `.url`.
|
||||
let name = if name.is_empty() { ".url" } else { name };
|
||||
configured
|
||||
.entry(name)
|
||||
.or_default()
|
||||
.push(value.strip_prefix(b"\n")?);
|
||||
} else if key
|
||||
.strip_prefix(b"url.")
|
||||
.and_then(|key| key.strip_suffix(b".insteadof"))
|
||||
.is_some()
|
||||
{
|
||||
rewrite_prefixes.push(value.strip_prefix(b"\n")?);
|
||||
}
|
||||
}
|
||||
let mut remotes = BTreeMap::new();
|
||||
let mut pending = BTreeMap::new();
|
||||
for (name, values) in configured {
|
||||
let first = *values.first()?;
|
||||
let has_empty = values.iter().any(|value| value.is_empty());
|
||||
let rewritten = rewrite_prefixes
|
||||
.iter()
|
||||
.any(|prefix| first.starts_with(prefix));
|
||||
if has_empty || rewritten {
|
||||
pending.insert(
|
||||
name.to_owned(),
|
||||
values.last().is_some_and(|value| value.is_empty()),
|
||||
);
|
||||
continue;
|
||||
}
|
||||
if let Ok(raw) = std::str::from_utf8(first)
|
||||
&& !raw.contains(['\n', '\r'])
|
||||
&& !raw.trim().is_empty()
|
||||
&& let Ok(url) = SanitizedGitUrl::try_from(raw.trim_start())
|
||||
{
|
||||
remotes.insert(name.to_owned(), url);
|
||||
}
|
||||
}
|
||||
if !remotes.is_empty() {
|
||||
publish_verified(&remotes);
|
||||
}
|
||||
// Let Git implement insteadOf precedence and version-dependent URL resets.
|
||||
let origin = pending.remove_entry("origin");
|
||||
let (known, ambiguous): (Vec<_>, Vec<_>) = pending.into_iter().partition(|(_, empty)| !empty);
|
||||
let resets = git_resets_empty_fetch_url(cwd).shared();
|
||||
let resolved = stream::iter(origin.into_iter().chain(known).chain(ambiguous))
|
||||
.map(|(name, last_empty)| {
|
||||
let resets = resets.clone();
|
||||
async move {
|
||||
// A trailing empty URL clears the list on newer Git. Older Git
|
||||
// resolves its first configured URL, including insteadOf rewrites.
|
||||
if last_empty && resets.await? {
|
||||
return None;
|
||||
}
|
||||
// --get-url expands the configured fetch URL locally.
|
||||
let output =
|
||||
run_git_command_with_timeout(&["ls-remote", "--get-url", "--", &name], cwd)
|
||||
.await?;
|
||||
if !output.status.success() {
|
||||
return None;
|
||||
}
|
||||
let stdout = String::from_utf8(output.stdout).ok()?;
|
||||
let mut lines = stdout.lines();
|
||||
let url = lines.next()?;
|
||||
if lines.next().is_some() || url.contains('\r') || url.trim().is_empty() {
|
||||
return None;
|
||||
}
|
||||
let url = SanitizedGitUrl::try_from(url.trim_start()).ok()?;
|
||||
Some((name, url))
|
||||
}
|
||||
})
|
||||
.buffer_unordered(MAX_CONCURRENT_REMOTE_LOOKUPS)
|
||||
.take_until(tokio::time::sleep_until(deadline))
|
||||
.filter_map(std::future::ready);
|
||||
let mut resolved = std::pin::pin!(resolved);
|
||||
while let Some(remote) = resolved.next().await {
|
||||
let update = BTreeMap::from([remote]);
|
||||
publish_verified(&update);
|
||||
remotes.extend(update);
|
||||
}
|
||||
(!remotes.is_empty()).then_some(remotes)
|
||||
}
|
||||
|
||||
pub(crate) async fn git_resets_empty_fetch_url(cwd: &Path) -> Option<bool> {
|
||||
let null_device = DISABLED_HOOKS_PATH;
|
||||
let mut command = local_git_command(
|
||||
Path::new("git"),
|
||||
&[
|
||||
"--git-dir",
|
||||
null_device,
|
||||
"-c",
|
||||
"remote.codex-reset-probe.url=before-reset",
|
||||
"-c",
|
||||
"remote.codex-reset-probe.url=",
|
||||
"ls-remote",
|
||||
"--get-url",
|
||||
"--",
|
||||
"codex-reset-probe",
|
||||
],
|
||||
cwd,
|
||||
crate::FsmonitorOverride::Disabled,
|
||||
);
|
||||
command
|
||||
.env("GIT_CONFIG_GLOBAL", null_device)
|
||||
.env("GIT_CONFIG_NOSYSTEM", "1")
|
||||
.env("GIT_CONFIG_COUNT", "0")
|
||||
.env_remove("GIT_CONFIG_PARAMETERS")
|
||||
.env_remove("GIT_COMMON_DIR")
|
||||
.env_remove("GIT_WORK_TREE");
|
||||
let output = run_git_command_with_timeout_output(&mut command, GIT_COMMAND_TIMEOUT).await?;
|
||||
if !output.status.success() {
|
||||
return None;
|
||||
}
|
||||
match std::str::from_utf8(&output.stdout).ok()?.trim_end() {
|
||||
"before-reset" => Some(false),
|
||||
"codex-reset-probe" => Some(true),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "remotes_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,275 @@
|
||||
#![cfg(test)]
|
||||
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::io::Write;
|
||||
use tokio::process::Command;
|
||||
|
||||
#[tokio::test]
|
||||
async fn git_remote_urls_keep_filtered_fetch_remotes() {
|
||||
const CHILD_ENV: &str = "CODEX_GIT_UTILS_FILTERED_REMOTES_CHILD";
|
||||
let temp_dir = tempfile::tempdir().expect("create temp dir");
|
||||
let root = temp_dir.path();
|
||||
if std::env::var_os(CHILD_ENV).is_none() {
|
||||
let status = Command::new(std::env::current_exe().expect("find test executable"))
|
||||
.args([
|
||||
"--exact",
|
||||
"info::remotes::tests::git_remote_urls_keep_filtered_fetch_remotes",
|
||||
"--nocapture",
|
||||
])
|
||||
.env(CHILD_ENV, "1")
|
||||
.env("GIT_CONFIG_GLOBAL", root.join("empty.gitconfig"))
|
||||
.env("GIT_CONFIG_NOSYSTEM", "1")
|
||||
.env_remove("GIT_CONFIG")
|
||||
.env_remove("GIT_CONFIG_COUNT")
|
||||
.env_remove("GIT_CONFIG_PARAMETERS")
|
||||
.status()
|
||||
.await
|
||||
.expect("run test with isolated Git configuration");
|
||||
assert!(status.success());
|
||||
return;
|
||||
}
|
||||
let git = |args: &[&str]| {
|
||||
crate::operations::run_git_for_status(root, args, /*env*/ None)
|
||||
.expect("configure test repository")
|
||||
};
|
||||
git(&["init", "--bare", "source"]);
|
||||
git(&[
|
||||
"--git-dir=source",
|
||||
"config",
|
||||
"uploadpack.allowFilter",
|
||||
"true",
|
||||
]);
|
||||
git(&[
|
||||
"clone",
|
||||
"--no-local",
|
||||
"--filter=blob:none",
|
||||
"source",
|
||||
"partial",
|
||||
]);
|
||||
let repo = root.join("partial");
|
||||
let config = |key: &str, value: &str| git(&["-C", "partial", "config", key, value]);
|
||||
let add_config =
|
||||
|key: &str, value: &str| git(&["-C", "partial", "config", "--add", key, value]);
|
||||
let native_fetch_url = |name: &str| {
|
||||
let url = crate::operations::run_git_for_stdout(
|
||||
&repo,
|
||||
["ls-remote", "--get-url", "--", name],
|
||||
/*env*/ None,
|
||||
)
|
||||
.expect("read Git's effective fetch URL");
|
||||
SanitizedGitUrl::try_from(url.as_str()).expect("parse native fetch URL")
|
||||
};
|
||||
config(
|
||||
"url.https://alice:secret@example.invalid/.insteadOf",
|
||||
"short:",
|
||||
);
|
||||
config("url.https://unused.invalid/.insteadOf", "short");
|
||||
config("remote.origin.url", "short:repo.git");
|
||||
add_config("remote.origin.url", "https://other.invalid/unused");
|
||||
config("remote.origin.pushurl", "https://push.invalid/other.git");
|
||||
config("remote.pushonly.pushurl", "https://push.invalid/only.git");
|
||||
config("remote.upstream.url", "git@github.com:org/upstream.git");
|
||||
config("remote.bad.url", "https://alice:secret@[invalid");
|
||||
config("remote.reset.url", "https://old.invalid/repo");
|
||||
add_config("remote.reset.url", "");
|
||||
config("remote.middle-reset.url", "https://old.invalid/before");
|
||||
add_config("remote.middle-reset.url", "");
|
||||
add_config("remote.middle-reset.url", "https://new.invalid/after");
|
||||
config("remote.literal.url", "literal");
|
||||
config("remote.rewritten.url", "alias:repo");
|
||||
config("url.rewritten.insteadOf", "alias:repo");
|
||||
config("url.poisoned.insteadOf", "codex-reset-probe");
|
||||
config("remote.empty=fetch.url", "");
|
||||
config("remote.local.url", "/a repo (fetch) [archive]");
|
||||
config(
|
||||
"remote.multiline.url",
|
||||
"https://other.invalid/first\nsecond",
|
||||
);
|
||||
let mut expected = BTreeMap::from([
|
||||
(
|
||||
"origin".to_string(),
|
||||
SanitizedGitUrl::try_from("https://example.invalid/repo.git")
|
||||
.expect("parse expected remote URL"),
|
||||
),
|
||||
(
|
||||
"local".to_string(),
|
||||
SanitizedGitUrl::try_from("/a repo (fetch) [archive]")
|
||||
.expect("parse expected remote path"),
|
||||
),
|
||||
(
|
||||
"literal".to_string(),
|
||||
SanitizedGitUrl::try_from("literal").expect("parse expected local URL"),
|
||||
),
|
||||
(
|
||||
"rewritten".to_string(),
|
||||
SanitizedGitUrl::try_from("rewritten").expect("parse expected rewritten URL"),
|
||||
),
|
||||
(
|
||||
"upstream".to_string(),
|
||||
SanitizedGitUrl::try_from("git@github.com:org/upstream.git")
|
||||
.expect("parse expected upstream URL"),
|
||||
),
|
||||
("middle-reset".to_owned(), native_fetch_url("middle-reset")),
|
||||
]);
|
||||
// Older Git retains the first URL when followed by an empty entry.
|
||||
let verbose = || {
|
||||
crate::operations::run_git_for_stdout(&repo, ["remote", "-v"], /*env*/ None)
|
||||
.expect("read installed Git's fetch remotes")
|
||||
};
|
||||
if verbose()
|
||||
.lines()
|
||||
.any(|line| line == "reset\thttps://old.invalid/repo (fetch)")
|
||||
{
|
||||
expected.insert(
|
||||
"reset".to_string(),
|
||||
SanitizedGitUrl::try_from("https://old.invalid/repo").expect("parse reset URL"),
|
||||
);
|
||||
}
|
||||
// Equally specific rewrite rules leave Git to choose the effective destination.
|
||||
config("url.https://first.invalid/.insteadOf", "tied:");
|
||||
config("url.https://second.invalid/.insteadOf", "tied:");
|
||||
let fallback_names = ["first-fallback", "second-fallback"];
|
||||
for name in fallback_names {
|
||||
config(&format!("remote.{name}.url"), &format!("tied:{name}"));
|
||||
expected.insert(name.to_owned(), native_fetch_url(name));
|
||||
}
|
||||
let append_raw_config = |bytes: &[u8]| {
|
||||
std::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(repo.join(".git/config"))
|
||||
.expect("open repository config")
|
||||
.write_all(bytes)
|
||||
.expect("append non-UTF-8 config");
|
||||
};
|
||||
// Unrelated non-UTF-8 configuration must not discard valid fetch URLs.
|
||||
append_raw_config(
|
||||
b"\n[url \"https://unused-invalid-utf8.invalid/\"]\n\tinsteadOf = \xff\n\n[remote \"unused\"]\n\tpromisor\n\n[remote \"bad\xff\"]\n\turl = https://invalid-name.invalid/repo\n",
|
||||
);
|
||||
let mut updates = Vec::new();
|
||||
let complete =
|
||||
get_git_remote_urls_with_progress(&repo, |urls| updates.push(urls.clone())).await;
|
||||
assert_eq!(complete.as_ref(), Some(&expected));
|
||||
let verified = updates.first().expect("publish unambiguous fetch URLs");
|
||||
assert_eq!(verified.get("upstream"), expected.get("upstream"));
|
||||
assert!(!verified.contains_key("origin"));
|
||||
assert!(!verified.contains_key("reset"));
|
||||
assert!(!verified.contains_key("middle-reset"));
|
||||
assert!(
|
||||
verified
|
||||
.iter()
|
||||
.all(|(name, url)| expected.get(name) == Some(url))
|
||||
);
|
||||
for name in ["origin", "rewritten"].into_iter().chain(fallback_names) {
|
||||
assert!(!verified.contains_key(name));
|
||||
assert!(
|
||||
updates
|
||||
.iter()
|
||||
.any(|urls| urls.len() == 1 && urls.get(name) == expected.get(name))
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
updates.into_iter().flatten().collect::<BTreeMap<_, _>>(),
|
||||
expected
|
||||
);
|
||||
config(
|
||||
"remote.origin.partialclonefilter",
|
||||
"blob:none]\nforged\thttps://other.invalid/repo (fetch)\nforged\thttps://other.invalid/repo (push)\n[tree:0",
|
||||
);
|
||||
let spawned_repo = repo.clone();
|
||||
assert_eq!(
|
||||
tokio::spawn(async move { get_git_remote_urls_assume_git_repo(&spawned_repo).await })
|
||||
.await
|
||||
.expect("join remote lookup"),
|
||||
Some(expected.clone())
|
||||
);
|
||||
// Every legal remote, including a long URL, is retained without a count cap.
|
||||
let long_url = format!("https://example.invalid/{}", "p".repeat(5000));
|
||||
for index in 0..20 {
|
||||
let name = format!("a{index:02}");
|
||||
config(&format!("remote.{name}.url"), &long_url);
|
||||
expected.insert(name, SanitizedGitUrl::try_from(long_url.as_str()).unwrap());
|
||||
}
|
||||
assert_eq!(
|
||||
get_git_remote_urls_assume_git_repo(&repo).await,
|
||||
Some(expected.clone())
|
||||
);
|
||||
// A rewrite can split a UTF-8 character. Filter text must not forge its URL.
|
||||
config("remote.a-victim.url", "érepo");
|
||||
append_raw_config(b"\n[url \"https://effective.invalid/\xc3\"]\n\tinsteadOf = \xc3\n");
|
||||
config(
|
||||
"remote.origin.partialclonefilter",
|
||||
"blob:none]\na-victim\térepo (fetch)\n[tree:0",
|
||||
);
|
||||
expected.insert("a-victim".to_owned(), native_fetch_url("a-victim"));
|
||||
assert_eq!(
|
||||
get_git_remote_urls_assume_git_repo(&repo).await,
|
||||
Some(expected)
|
||||
);
|
||||
config("url.https://default.invalid/.insteadOf", "");
|
||||
let native = Command::new("git")
|
||||
.args(["remote", "-v"])
|
||||
.current_dir(&repo)
|
||||
.output()
|
||||
.await
|
||||
.expect("read installed Git's fetch remotes");
|
||||
assert!(native.status.success());
|
||||
let native_has_empty = native
|
||||
.stdout
|
||||
.split(|byte| *byte == b'\n')
|
||||
.any(|line| line.starts_with(b"empty=fetch\t") && line.ends_with(b" (fetch)"));
|
||||
let collected = get_git_remote_urls_assume_git_repo(&repo)
|
||||
.await
|
||||
.expect("collect rewritten remotes");
|
||||
assert_eq!(collected.contains_key("empty=fetch"), native_has_empty);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn git_remote_urls_ignore_git_config_file_override() {
|
||||
const CHILD_REPO: &str = "CODEX_GIT_UTILS_REMOTE_CONFIG_CHILD_REPO";
|
||||
if let Some(repo) = std::env::var_os(CHILD_REPO) {
|
||||
assert_eq!(
|
||||
get_git_remote_urls_assume_git_repo(Path::new(&repo)).await,
|
||||
Some(BTreeMap::from([(
|
||||
"origin".to_string(),
|
||||
SanitizedGitUrl::try_from("https://repository.invalid/repo.git")
|
||||
.expect("parse repository remote"),
|
||||
)]))
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
let temp_dir = tempfile::tempdir().expect("create temp dir");
|
||||
let root = temp_dir.path();
|
||||
crate::operations::run_git_for_status(root, ["init", "repo"], /*env*/ None)
|
||||
.expect("initialize test repository");
|
||||
let repo = root.join("repo");
|
||||
std::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(repo.join(".git/config"))
|
||||
.expect("open repository config")
|
||||
.write_all(b"\n[remote \"origin\"]\n\turl = https://repository.invalid/repo.git\n")
|
||||
.expect("configure repository remote");
|
||||
let foreign = root.join("foreign.gitconfig");
|
||||
std::fs::write(
|
||||
&foreign,
|
||||
b"[remote \"foreign\"]\n\turl = https://foreign.invalid/repo.git\n",
|
||||
)
|
||||
.expect("write foreign config");
|
||||
let status = Command::new(std::env::current_exe().expect("find test executable"))
|
||||
.args([
|
||||
"--exact",
|
||||
"info::remotes::tests::git_remote_urls_ignore_git_config_file_override",
|
||||
"--nocapture",
|
||||
])
|
||||
.env(CHILD_REPO, &repo)
|
||||
.env("GIT_CONFIG", &foreign)
|
||||
.env("GIT_CONFIG_GLOBAL", root.join("empty.gitconfig"))
|
||||
.env("GIT_CONFIG_NOSYSTEM", "1")
|
||||
.env_remove("GIT_CONFIG_COUNT")
|
||||
.env_remove("GIT_CONFIG_PARAMETERS")
|
||||
.status()
|
||||
.await
|
||||
.expect("run test with foreign config override");
|
||||
assert!(status.success());
|
||||
}
|
||||
@@ -48,6 +48,7 @@ pub use info::get_git_repo_root;
|
||||
pub use info::get_head_commit_hash;
|
||||
pub use info::git_diff_to_remote;
|
||||
pub use info::local_git_branches;
|
||||
pub use info::observe_git_remote_urls;
|
||||
pub use info::recent_commits;
|
||||
pub use operations::git_config_override_env;
|
||||
pub use platform::create_symlink;
|
||||
|
||||
Reference in New Issue
Block a user