mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Sanitize credentials from Git remote metadata (#40713)
## Why Git remote URLs can embed usernames, passwords, or tokens. Codex carries these URLs through turn metadata and persisted thread metadata, so credentials must be removed before a remote enters those paths. ## What changed - Add `SanitizedGitUrl`, which parses Git URL and SCP-style remotes, strips authentication data, and preserves the conventional `git` SSH user. - Use sanitized remotes when collecting Git metadata, enriching model requests, discovering cloud environments, updating thread metadata, and reading legacy rollouts. Reject or omit malformed remotes instead of retaining their raw contents. - Keep API, schema, and TypeScript representations as strings while enforcing sanitization in Rust. ## Testing - Cover URL schemes, SCP and IPv6 forms, remote helpers, encoded paths, malformed values, and legacy deserialization. - Verify credentials do not appear in model requests, API responses, SQLite, or rollout files. GitOrigin-RevId: 6435efc4c45bfbfad4723ce7a0457cb00175f25c
This commit is contained in:
committed by
copyberry
parent
399be2d6b5
commit
1bc02aea50
@@ -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<String>,
|
||||
/// Repository URL (if available from remote)
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub repository_url: Option<String>,
|
||||
pub repository_url: Option<SanitizedGitUrl>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Clone, Debug)]
|
||||
@@ -110,14 +111,14 @@ pub async fn collect_git_info(cwd: &Path) -> Option<GitInfo> {
|
||||
&& 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<BTreeMap<String, String>> {
|
||||
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
|
||||
@@ -130,7 +131,9 @@ pub async fn get_git_remote_urls(cwd: &Path) -> Option<BTreeMap<String, String>>
|
||||
}
|
||||
|
||||
/// 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, String>> {
|
||||
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;
|
||||
@@ -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<BTreeMap<String, String>> {
|
||||
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 {
|
||||
@@ -273,8 +276,10 @@ fn parse_git_remote_urls(stdout: &str) -> Option<BTreeMap<String, String>> {
|
||||
};
|
||||
|
||||
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");
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user