diff --git a/app/src/ai/agent_sdk/driver/git_credentials.rs b/app/src/ai/agent_sdk/driver/git_credentials.rs index f23a81fdd98..a2c235c7105 100644 --- a/app/src/ai/agent_sdk/driver/git_credentials.rs +++ b/app/src/ai/agent_sdk/driver/git_credentials.rs @@ -147,6 +147,49 @@ fn write_secret_file(path: &std::path::Path, content: &str) -> Result<()> { Ok(()) } +/// Atomically replace `path` with `content` through a uniquely named, owner-only (0600 on Unix) +/// temporary file in the same directory, so concurrent writers never share a temporary path. +fn replace_secret_file(path: &Path, content: &str) -> Result<()> { + use std::io::Write as _; + + let dir = path + .parent() + .with_context(|| format!("{} has no parent directory", path.display()))?; + let file_name = path + .file_name() + .with_context(|| format!("{} has no file name", path.display()))? + .to_string_lossy(); + let mut temp_file = tempfile::Builder::new() + .prefix(&format!(".{}.tmp-", file_name.trim_start_matches('.'))) + .tempfile_in(dir) + .with_context(|| format!("Failed to create a temporary file in {}", dir.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + temp_file + .as_file() + .set_permissions(std::fs::Permissions::from_mode(0o600)) + .with_context(|| { + format!( + "Failed to set permissions on a temporary file in {}", + dir.display() + ) + })?; + } + temp_file + .write_all(content.as_bytes()) + .with_context(|| format!("Failed to write a temporary file in {}", dir.display()))?; + temp_file + .as_file() + .sync_all() + .with_context(|| format!("Failed to sync a temporary file in {}", dir.display()))?; + temp_file + .persist(path) + .map_err(|error| error.error) + .with_context(|| format!("Failed to replace {}", path.display()))?; + Ok(()) +} + fn azure_devops_auth_dir(home: &Path) -> PathBuf { home.join(".warp").join(AZURE_DEVOPS_AUTH_DIR) } @@ -173,43 +216,7 @@ fn prepare_azure_devops_auth_dir(home: &Path) -> Result { } fn write_azure_cli_token(auth_dir: &Path, token: &str) -> Result<()> { - use std::io::Write as _; - - let token_path = auth_dir.join(AZURE_DEVOPS_TOKEN_FILENAME); - let mut temp_file = tempfile::Builder::new() - .prefix(&format!(".{AZURE_DEVOPS_TOKEN_FILENAME}.tmp-")) - .tempfile_in(auth_dir) - .with_context(|| { - format!( - "Failed to create a temporary token in {}", - auth_dir.display() - ) - })?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - temp_file - .as_file() - .set_permissions(std::fs::Permissions::from_mode(0o600)) - .with_context(|| { - format!( - "Failed to set permissions on temporary token in {}", - auth_dir.display() - ) - })?; - } - temp_file - .write_all(token.as_bytes()) - .with_context(|| format!("Failed to write temporary token in {}", auth_dir.display()))?; - temp_file - .as_file() - .sync_all() - .with_context(|| format!("Failed to sync temporary token in {}", auth_dir.display()))?; - temp_file - .persist(&token_path) - .map_err(|error| error.error) - .with_context(|| format!("Failed to replace {}", token_path.display()))?; - Ok(()) + replace_secret_file(&auth_dir.join(AZURE_DEVOPS_TOKEN_FILENAME), token) } fn shell_single_quote(value: &Path) -> String { @@ -398,26 +405,15 @@ fn merge_git_credentials_file_content(existing: &str, credentials: &[GitCredenti /// - `https://x-access-token:{token}@{host}` for service-account tokens /// /// The write is done atomically: a temporary file is written then renamed. -fn write_git_credentials_file(credentials: &[GitCredential]) -> Result<()> { +fn write_git_credentials_file(credentials: &[GitCredential], home: &Path) -> Result<()> { if credentials.is_empty() { return Ok(()); } - let home = home_dir()?; let path = home.join(".git-credentials"); - let tmp_path = home.join(".git-credentials.tmp"); let existing = std::fs::read_to_string(&path).unwrap_or_default(); let content = merge_git_credentials_file_content(&existing, credentials); - write_secret_file(&tmp_path, &content)?; - std::fs::rename(&tmp_path, &path).with_context(|| { - format!( - "Failed to rename {} to {}", - tmp_path.display(), - path.display() - ) - })?; - - Ok(()) + replace_secret_file(&path, &content) } /// The directory `gh` reads its config from, in `gh`'s own precedence order. On Windows `gh` @@ -470,7 +466,6 @@ fn write_gh_hosts_yml(credentials: &[GitCredential], gh_config_dir: &Path) -> Re std::fs::create_dir_all(gh_config_dir) .with_context(|| format!("Failed to create {}", gh_config_dir.display()))?; let path = gh_config_dir.join(GH_HOSTS_FILENAME); - let tmp_path = gh_config_dir.join(format!("{GH_HOSTS_FILENAME}.tmp")); let mut yaml = String::new(); for cred in github_credentials { @@ -482,16 +477,7 @@ fn write_gh_hosts_yml(credentials: &[GitCredential], gh_config_dir: &Path) -> Re } } - write_secret_file(&tmp_path, &yaml)?; - std::fs::rename(&tmp_path, &path).with_context(|| { - format!( - "Failed to rename {} to {}", - tmp_path.display(), - path.display() - ) - })?; - - Ok(()) + replace_secret_file(&path, &yaml) } /// Write `~/.config/glab-cli/config.yml` so the `glab` CLI is authenticated. @@ -518,7 +504,6 @@ fn write_glab_config(credentials: &[GitCredential], home: &std::path::Path) -> R std::fs::create_dir_all(&glab_config_dir) .with_context(|| format!("Failed to create {}", glab_config_dir.display()))?; let path = glab_config_dir.join(GLAB_CONFIG_FILENAME); - let tmp_path = glab_config_dir.join(format!("{GLAB_CONFIG_FILENAME}.tmp")); let mut yaml = String::new(); yaml.push_str("hosts:\n"); @@ -529,16 +514,7 @@ fn write_glab_config(credentials: &[GitCredential], home: &std::path::Path) -> R yaml.push_str(" api_protocol: https\n"); } - write_secret_file(&tmp_path, &yaml)?; - std::fs::rename(&tmp_path, &path).with_context(|| { - format!( - "Failed to rename {} to {}", - tmp_path.display(), - path.display() - ) - })?; - - Ok(()) + replace_secret_file(&path, &yaml) } /// Formats non-sensitive metadata for verifying local credential injection. @@ -586,7 +562,7 @@ pub(crate) fn write_git_credentials_with_failures( } let home = home_dir()?; let outcomes = [ - write_git_credentials_file(credentials), + write_git_credentials_file(credentials, &home), write_gh_hosts_yml(credentials, &gh_config_dir(&home)), write_glab_config(credentials, &home), write_azure_cli_auth(credentials, &home), diff --git a/app/src/ai/agent_sdk/driver/git_credentials_tests.rs b/app/src/ai/agent_sdk/driver/git_credentials_tests.rs index 557bf9ca96c..5abebfcd6c9 100644 --- a/app/src/ai/agent_sdk/driver/git_credentials_tests.rs +++ b/app/src/ai/agent_sdk/driver/git_credentials_tests.rs @@ -419,15 +419,9 @@ fn write_gh_hosts_yml_uses_gh_cli_filename() -> Result<()> { &gh_config_dir, )?; - let hosts_path = gh_config_dir.join(GH_HOSTS_FILENAME); - assert!(hosts_path.exists()); - assert!( - !gh_config_dir - .join(format!("{GH_HOSTS_FILENAME}.tmp")) - .exists() - ); + assert_eq!(sorted_file_names(&gh_config_dir)?, [GH_HOSTS_FILENAME]); - let hosts = std::fs::read_to_string(hosts_path)?; + let hosts = std::fs::read_to_string(gh_config_dir.join(GH_HOSTS_FILENAME))?; assert!(hosts.contains("github.com:")); assert!(hosts.contains(" oauth_token: token")); assert!(hosts.contains(" git_protocol: https")); @@ -579,6 +573,75 @@ fn gitlab_credential() -> GitCredential { } } +fn sorted_file_names(dir: &Path) -> Result> { + let mut names = std::fs::read_dir(dir)? + .map(|entry| Ok(entry?.file_name().to_string_lossy().into_owned())) + .collect::>>()?; + names.sort(); + Ok(names) +} + +/// Direct-backend sandboxes on one host share `HOME` and `GH_CONFIG_DIR`, so concurrently starting +/// tasks write the same credential stores at the same time. +#[test] +fn concurrent_credential_store_writes_share_directories_safely() -> Result<()> { + const WRITERS: usize = 8; + const WRITES_PER_WRITER: usize = 25; + let home = tempfile::tempdir()?; + let home = home.path(); + let gh_config_dir = home.join(".config").join("gh"); + let barrier = std::sync::Barrier::new(WRITERS); + + std::thread::scope(|scope| { + let writers = (0..WRITERS) + .map(|writer| { + let barrier = &barrier; + let gh_config_dir = &gh_config_dir; + scope.spawn(move || -> Result<()> { + let credentials = [ + GitCredential { + token: format!("github-token-{writer}"), + ..github_credential() + }, + GitCredential { + token: format!("gitlab-token-{writer}"), + ..gitlab_credential() + }, + ]; + barrier.wait(); + for _ in 0..WRITES_PER_WRITER { + write_git_credentials_file(&credentials, home)?; + write_gh_hosts_yml(&credentials, gh_config_dir)?; + write_glab_config(&credentials, home)?; + } + Ok(()) + }) + }) + .collect::>(); + writers + .into_iter() + .try_for_each(|writer| writer.join().expect("credential writer panicked")) + })?; + + assert_eq!(sorted_file_names(home)?, [".config", ".git-credentials"]); + assert_eq!(sorted_file_names(&gh_config_dir)?, [GH_HOSTS_FILENAME]); + assert_eq!( + sorted_file_names(&home.join(".config").join("glab-cli"))?, + [GLAB_CONFIG_FILENAME] + ); + assert_eq!( + std::fs::read_to_string(home.join(".git-credentials"))? + .lines() + .count(), + 2 + ); + assert!( + std::fs::read_to_string(gh_config_dir.join(GH_HOSTS_FILENAME))? + .contains("oauth_token: github-token-") + ); + Ok(()) +} + #[test] fn merged_credentials_include_each_provider_host() { let content = merge_git_credentials_file_content( @@ -976,15 +1039,9 @@ fn write_glab_config_uses_glab_cli_filename() -> Result<()> { temp_dir.path(), )?; - let config_path = glab_config_dir.join(GLAB_CONFIG_FILENAME); - assert!(config_path.exists()); - assert!( - !glab_config_dir - .join(format!("{GLAB_CONFIG_FILENAME}.tmp")) - .exists() - ); + assert_eq!(sorted_file_names(&glab_config_dir)?, [GLAB_CONFIG_FILENAME]); - let config = std::fs::read_to_string(config_path)?; + let config = std::fs::read_to_string(glab_config_dir.join(GLAB_CONFIG_FILENAME))?; assert!(config.contains("hosts:")); assert!(config.contains(" gitlab.com:")); assert!(config.contains(" token: gitlab-token"));