From 0f3c2df13b693682716d0940e57f48d2191259c3 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 5 Oct 2026 15:58:12 +1300 Subject: [PATCH 1/2] refactor(config): write .env secrets through abnegate-config claudear kept its own .env upserter (env_writer.rs). abnegate-config's EnvironmentFile does the same job and is the shared implementation, so the local copy goes, together with its public re-exports `claudear::env_writer`, `claudear_config::env_writer` and `claudear_config::update_env_file`. claudear-config keeps one entry point, environment::update, which both writers (the GitHub App manifest callback and `webhook --setup`) call, with constants for the 13 keys claudear writes. It renders the abnegate-config error chain into Error::Config, so failures still name their cause ("Failed to write '.env': Permission denied (os error 13)"). The write is now atomic: a temporary file in the same directory is renamed over the target. That has consequences an operator can hit: - The parent DIRECTORY must be writable by the user running claudear. A layout such as EnvironmentFile=/etc/claudear/env in a root-owned 0755 directory, with the file chowned to a non-root service user, now makes `claudear webhook --setup --env-file /etc/claudear/env` fail with Permission denied, where the old in-place write succeeded. (The systemd unit in the docs has no User= and runs as root, so it keeps working.) - The file gets a new inode, so its owner and group become the writing user's and any hard link to it no longer sees updates. A symlink at the path is replaced rather than written through. - A .env bind-mounted into a container as a single file cannot be renamed over and fails with EBUSY. The shipped docker-compose.yml does not mount it that way. - A missing parent directory is created 0700; the file is always 0600. Previously it was written in place and chmodded afterwards, best effort, so a crash could leave it truncated or briefly world readable. Because `webhook --setup` writes the secrets only after creating every webhook remotely, a path that can no longer be written would have failed after Linear, Sentry, GitLab, GitHub and Telegram hooks existed, dropping their secrets, and Linear setup refuses to re-run against an existing webhook URL. configure() therefore calls environment::probe first: it fails the way update would (unreadable file, or a directory that will not accept a new file, checking the nearest existing ancestor when the directory is still to be created) without leaving anything behind, before any remote call. Quoting changes: values the shell would interpret (spaces, $, #, ", \) are single quoted instead of double quoted. Only values holding ' or a line break are double quoted, with \n, \r, \\, \", \$ and \` escapes; none of the values claudear writes (generated secrets, ids, URLs) do. Those escapes are not universal: dotenvy 0.15 rejects \r and \` inside double quotes and systemd's EnvironmentFile= keeps \n literal. Bare and single-quoted values read the same everywhere. New keys are appended sorted after a blank line without the "# Auto-configured webhook secrets" header. Existing files keep working: comments, blank lines, the old header and old double-quoted values are left as they are and still read back. Keys must be shell variable names; all 13 are. Tests in claudear-config pin what claudear relies on: every value round-trips under every written key, existing lines survive, the quoting forms, a file from the old writer still reads, rewrites are idempotent, the file is 0600 and a created directory 0700, a read-only directory yields Error::Config naming the permission error, and the probe fails the same way while leaving no trace. A configurator test points env_path into a 0500 directory and asserts configure() fails with that error before its stand-in Jira server sees any connection. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 211 +++++++++- Cargo.toml | 1 + crates/claudear-config/Cargo.toml | 7 +- crates/claudear-config/src/env_writer.rs | 369 ------------------ crates/claudear-config/src/environment.rs | 94 +++++ crates/claudear-config/src/lib.rs | 3 +- crates/claudear-config/tests/environment.rs | 279 +++++++++++++ .../src/github_app/routes.rs | 69 ++-- .../src/webhook/configurator.rs | 102 ++++- src/lib.rs | 1 - 10 files changed, 691 insertions(+), 445 deletions(-) delete mode 100644 crates/claudear-config/src/env_writer.rs create mode 100644 crates/claudear-config/src/environment.rs create mode 100644 crates/claudear-config/tests/environment.rs diff --git a/Cargo.lock b/Cargo.lock index 636a5e2c..e25f9024 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,20 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "abnegate-config" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bd648a5fa3968334c2eaaae6c0b85b56e5b9feb7fbb124a534ce2a82d4834905" +dependencies = [ + "abnegate-secret", + "dirs 7.0.0", + "serde", + "tempfile", + "thiserror 2.0.18", + "toml", +] + [[package]] name = "abnegate-index" version = "0.1.0" @@ -13,6 +27,24 @@ dependencies = [ "thiserror 2.0.18", ] +[[package]] +name = "abnegate-secret" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7aa24383e65469ff8390024afe21418dc61eb448c1abf94cc878ff9b6f9df4f7" +dependencies = [ + "aes-gcm 0.11.1", + "base64 0.23.1", + "dirs 7.0.0", + "getrandom 0.4.1", + "hex", + "serde", + "subtle", + "thiserror 2.0.18", + "tracing", + "zeroize", +] + [[package]] name = "addr2line" version = "0.25.1" @@ -38,6 +70,16 @@ dependencies = [ "generic-array", ] +[[package]] +name = "aead" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1973cfbc1a2daf9cf550e74e1f088c28e7f7d8c1e1418fb6c9dc5184b7e84c99" +dependencies = [ + "crypto-common 0.2.2", + "inout 0.2.2", +] + [[package]] name = "aes" version = "0.8.4" @@ -45,24 +87,51 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" dependencies = [ "cfg-if", - "cipher", + "cipher 0.4.4", "cpufeatures 0.2.17", ] +[[package]] +name = "aes" +version = "0.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "35f0f96ce78e38c3dc6d8948aa8163d06385be74000f3c7a95bf1eef35d3ea32" +dependencies = [ + "cipher 0.5.2", + "cpubits", + "cpufeatures 0.3.0", + "zeroize", +] + [[package]] name = "aes-gcm" version = "0.10.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "831010a0f742e1209b3bcea8fab6a8e149051ba6099432c8cb2cc117dec3ead1" dependencies = [ - "aead", - "aes", - "cipher", - "ctr", - "ghash", + "aead 0.5.2", + "aes 0.8.4", + "cipher 0.4.4", + "ctr 0.9.2", + "ghash 0.5.1", "subtle", ] +[[package]] +name = "aes-gcm" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f2b8006a0c83f52b62ba44a97b58bf76fe2f70a329e588f67f89691d93d498f" +dependencies = [ + "aead 0.6.1", + "aes 0.9.3", + "cipher 0.5.2", + "ctr 0.10.1", + "ctutils", + "ghash 0.6.0", + "zeroize", +] + [[package]] name = "ahash" version = "0.8.12" @@ -466,6 +535,12 @@ version = "0.22.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" +[[package]] +name = "base64" +version = "0.23.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac07cdecf99051d9a5238b80f35af32cdeba5b336e55d957b318b50137e18da5" + [[package]] name = "base64ct" version = "1.8.3" @@ -564,7 +639,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e412e2cd0f2b2d93e02543ceae7917b3c70331573df19ee046bcbc35e45e87d7" dependencies = [ "byteorder", - "cipher", + "cipher 0.4.4", ] [[package]] @@ -691,7 +766,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773f3b9af64447d2ce9850330c473515014aa235e6a783b02db81ff39e4a3dad" dependencies = [ "crypto-common 0.1.7", - "inout", + "inout 0.1.4", +] + +[[package]] +name = "cipher" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e8cf2a2c93cd704877c0858356ed03480ff301ee950b43f1cbe4573b088bfa6c" +dependencies = [ + "block-buffer 0.12.1", + "crypto-common 0.2.2", + "inout 0.2.2", ] [[package]] @@ -749,7 +835,7 @@ checksum = "3a822ea5bc7590f9d40f1ba12c0dc3c2760f3482c6984db1573ad11031420831" name = "claudear" version = "1.0.0" dependencies = [ - "aes-gcm", + "aes-gcm 0.10.3", "anyhow", "async-trait", "axum", @@ -764,7 +850,7 @@ dependencies = [ "claudear-engine", "claudear-integrations", "claudear-storage", - "dirs", + "dirs 6.0.0", "encoding_rs", "fastembed", "futures", @@ -846,7 +932,7 @@ dependencies = [ "claudear-config", "claudear-core", "claudear-storage", - "dirs", + "dirs 6.0.0", "fastembed", "futures", "ort", @@ -886,9 +972,10 @@ dependencies = [ name = "claudear-config" version = "1.0.0" dependencies = [ + "abnegate-config", "chrono", "claudear-core", - "dirs", + "dirs 6.0.0", "regex-lite", "serde", "serde_json", @@ -901,12 +988,12 @@ dependencies = [ name = "claudear-core" version = "1.0.0" dependencies = [ - "aes-gcm", + "aes-gcm 0.10.3", "anyhow", "async-trait", "base64 0.22.1", "chrono", - "dirs", + "dirs 6.0.0", "futures", "hex", "hmac", @@ -1002,7 +1089,7 @@ dependencies = [ "claudear-config", "claudear-core", "claudear-storage", - "dirs", + "dirs 6.0.0", "encoding_rs", "futures", "futures-util", @@ -1068,6 +1155,12 @@ dependencies = [ "cc", ] +[[package]] +name = "cmov" +version = "0.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c9ea0ac24bc397ab3c98583a3c9ba74fa56b09a4449bbe172b9b1ddb016027a" + [[package]] name = "colorchoice" version = "1.0.4" @@ -1183,6 +1276,12 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpubits" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "15b85f9c39137c3a891689859392b1bd49812121d0d61c9caf00d46ed5ce06ae" + [[package]] name = "cpufeatures" version = "0.2.17" @@ -1273,7 +1372,9 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ce6e4c961d6cd6c9a86db418387425e8bdeaf05b3c8bc1411e6dca4c252f1453" dependencies = [ + "getrandom 0.4.1", "hybrid-array", + "rand_core 0.10.0", ] [[package]] @@ -1282,7 +1383,25 @@ version = "0.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0369ee1ad671834580515889b80f2ea915f23b8be8d0daa4bbaf2ac5c7590835" dependencies = [ - "cipher", + "cipher 0.4.4", +] + +[[package]] +name = "ctr" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baaca1c4b237092596f64d571e9db6ce4109c4ef9742e27590f1709594461f21" +dependencies = [ + "cipher 0.5.2", +] + +[[package]] +name = "ctutils" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d5515a3834141de9eafb9717ad39eea8247b5674e6066c404e8c4b365d2a29e" +dependencies = [ + "cmov", ] [[package]] @@ -1469,6 +1588,15 @@ dependencies = [ "dirs-sys", ] +[[package]] +name = "dirs" +version = "7.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8d57d423b3c82e89b9a24ca3091fee61f456a26edbd28d26c65906f4bc1dcd8f" +dependencies = [ + "dirs-sys", +] + [[package]] name = "dirs-sys" version = "0.5.0" @@ -1995,7 +2123,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0d8a4362ccb29cb0b265253fb0a2728f592895ee6854fd9bc13f2ffda266ff1" dependencies = [ "opaque-debug", - "polyval", + "polyval 0.6.2", +] + +[[package]] +name = "ghash" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2eecf2d5dc9b66b732b97707a0210906b1d30523eb773193ab777c0c84b3e8d5" +dependencies = [ + "polyval 0.7.3", + "zeroize", ] [[package]] @@ -2163,7 +2301,7 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "629d8f3bbeda9d148036d6b0de0a3ab947abd08ce90626327fc3547a49d59d97" dependencies = [ - "dirs", + "dirs 6.0.0", "http", "indicatif 0.17.11", "libc", @@ -2547,6 +2685,15 @@ dependencies = [ "generic-array", ] +[[package]] +name = "inout" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4250ce6452e92010fdf7268ccc5d14faa80bb12fc741938534c58f16804e03c7" +dependencies = [ + "hybrid-array", +] + [[package]] name = "ipnet" version = "2.11.0" @@ -3663,7 +3810,19 @@ dependencies = [ "cfg-if", "cpufeatures 0.2.17", "opaque-debug", - "universal-hash", + "universal-hash 0.5.1", +] + +[[package]] +name = "polyval" +version = "0.7.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0fa31d631f2b2cb2a544d0aa321ce847a94764d701ca2becc411138b93d49cd" +dependencies = [ + "cpubits", + "cpufeatures 0.3.0", + "universal-hash 0.6.1", + "zeroize", ] [[package]] @@ -5633,6 +5792,16 @@ dependencies = [ "subtle", ] +[[package]] +name = "universal-hash" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f4987bdc12753382e0bec4a65c50738ffaabc998b9cdd1f952fb5f39b0048a96" +dependencies = [ + "crypto-common 0.2.2", + "ctutils", +] + [[package]] name = "untrusted" version = "0.9.0" @@ -6541,9 +6710,9 @@ dependencies = [ [[package]] name = "zeroize" -version = "1.8.2" +version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" +checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" [[package]] name = "zerotrie" diff --git a/Cargo.toml b/Cargo.toml index 16a089ee..74a769c4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -151,6 +151,7 @@ mockall = "0.14" http-body-util = "0.1" # Shared infrastructure +abnegate-config = "0.1.3" abnegate-index = "0.1.0" # Internal crates diff --git a/crates/claudear-config/Cargo.toml b/crates/claudear-config/Cargo.toml index 54571d87..76cb09cd 100644 --- a/crates/claudear-config/Cargo.toml +++ b/crates/claudear-config/Cargo.toml @@ -9,6 +9,10 @@ workspace = true [dependencies] claudear-core = { workspace = true } +# .env files +abnegate-config = { workspace = true } +tempfile = { workspace = true } + # Serialization serde = { workspace = true } serde_json = { workspace = true } @@ -27,6 +31,3 @@ dirs = { workspace = true } # Regex regex-lite = { workspace = true } - -[dev-dependencies] -tempfile = { workspace = true } diff --git a/crates/claudear-config/src/env_writer.rs b/crates/claudear-config/src/env_writer.rs deleted file mode 100644 index cbf125fb..00000000 --- a/crates/claudear-config/src/env_writer.rs +++ /dev/null @@ -1,369 +0,0 @@ -//! Utility for writing to .env files. - -use claudear_core::error::{Error, Result}; -use std::collections::HashMap; -use std::fs; -use std::path::Path; - -/// Updates or appends key-value pairs in a .env file. -/// -/// This function: -/// - Creates the file if it doesn't exist -/// - Updates existing keys with new values -/// - Appends new keys at the end -/// - Preserves comments and formatting -pub fn update_env_file(path: &Path, updates: &HashMap) -> Result<()> { - let content = if path.exists() { - fs::read_to_string(path) - .map_err(|e| Error::config(format!("Failed to read .env file at {:?}: {}", path, e)))? - } else { - String::new() - }; - - let updated_content = update_env_content(&content, updates); - - fs::write(path, updated_content) - .map_err(|e| Error::config(format!("Failed to write .env file at {:?}: {}", path, e)))?; - - // Set restrictive permissions on the .env file (owner read/write only) - // since it may contain secrets like webhook secrets and API tokens - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - fs::set_permissions(path, perms).ok(); - } - - Ok(()) -} - -/// Updates .env content string with new key-value pairs. -/// -/// Returns the updated content. -fn update_env_content(content: &str, updates: &HashMap) -> String { - let mut result_lines: Vec = Vec::new(); - let mut updated_keys: std::collections::HashSet = std::collections::HashSet::new(); - - for line in content.lines() { - let trimmed = line.trim(); - - // Preserve empty lines and comments - if trimmed.is_empty() || trimmed.starts_with('#') { - result_lines.push(line.to_string()); - continue; - } - - // Parse key=value (handle various formats) - if let Some((key, _)) = parse_env_line(trimmed) { - if let Some(new_value) = updates.get(&key) { - // Update existing key - result_lines.push(format!("{}={}", key, quote_value(new_value))); - updated_keys.insert(key); - } else { - // Keep original line - result_lines.push(line.to_string()); - } - } else { - // Keep unrecognized lines - result_lines.push(line.to_string()); - } - } - - // Append new keys that weren't in the original file - let mut new_keys: Vec<_> = updates - .iter() - .filter(|(k, _)| !updated_keys.contains(*k)) - .collect(); - new_keys.sort_by_key(|(a, _)| *a); // Sort for deterministic output - - if !new_keys.is_empty() { - // Add a blank line before new keys if the file isn't empty and doesn't end with blank line - if !result_lines.is_empty() { - let last_line = result_lines.last().unwrap(); - if !last_line.trim().is_empty() { - result_lines.push(String::new()); - } - } - - // Add comment for auto-generated section - result_lines.push("# Auto-configured webhook secrets".to_string()); - - for (key, value) in new_keys { - result_lines.push(format!("{}={}", key, quote_value(value))); - } - } - - // Ensure file ends with newline - let mut result = result_lines.join("\n"); - if !result.is_empty() && !result.ends_with('\n') { - result.push('\n'); - } - - result -} - -/// Parse a line into key and value. -fn parse_env_line(line: &str) -> Option<(String, String)> { - let line = line.trim(); - - // Skip comments and empty lines - if line.is_empty() || line.starts_with('#') { - return None; - } - - // Find the first '=' sign - let eq_pos = line.find('=')?; - let key = line[..eq_pos].trim().to_string(); - let value = line[eq_pos + 1..].trim(); - - // Remove surrounding quotes if present - let value = if (value.starts_with('"') && value.ends_with('"')) - || (value.starts_with('\'') && value.ends_with('\'')) - { - value[1..value.len() - 1].to_string() - } else { - value.to_string() - }; - - Some((key, value)) -} - -/// Quote a value if it contains special characters. -fn quote_value(value: &str) -> String { - // Quote if contains spaces, quotes, or special shell characters - if value.contains(' ') - || value.contains('"') - || value.contains('\'') - || value.contains('#') - || value.contains('$') - || value.contains('\n') - || value.contains('\\') - { - // Use double quotes and escape internal quotes - let escaped = value.replace('\\', "\\\\").replace('"', "\\\""); - format!("\"{}\"", escaped) - } else { - value.to_string() - } -} - -#[cfg(test)] -mod tests { - use super::*; - use std::collections::HashMap; - use tempfile::TempDir; - - #[test] - fn test_parse_env_line_simple() { - let (key, value) = parse_env_line("KEY=value").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, "value"); - } - - #[test] - fn test_parse_env_line_with_spaces() { - let (key, value) = parse_env_line(" KEY = value ").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, "value"); - } - - #[test] - fn test_parse_env_line_double_quoted() { - let (key, value) = parse_env_line("KEY=\"quoted value\"").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, "quoted value"); - } - - #[test] - fn test_parse_env_line_single_quoted() { - let (key, value) = parse_env_line("KEY='quoted value'").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, "quoted value"); - } - - #[test] - fn test_parse_env_line_empty_value() { - let (key, value) = parse_env_line("KEY=").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, ""); - } - - #[test] - fn test_parse_env_line_comment() { - assert!(parse_env_line("# comment").is_none()); - } - - #[test] - fn test_parse_env_line_empty() { - assert!(parse_env_line("").is_none()); - assert!(parse_env_line(" ").is_none()); - } - - #[test] - fn test_parse_env_line_value_with_equals() { - let (key, value) = parse_env_line("KEY=value=with=equals").unwrap(); - assert_eq!(key, "KEY"); - assert_eq!(value, "value=with=equals"); - } - - #[test] - fn test_quote_value_simple() { - assert_eq!(quote_value("simple"), "simple"); - } - - #[test] - fn test_quote_value_with_spaces() { - assert_eq!(quote_value("with spaces"), "\"with spaces\""); - } - - #[test] - fn test_quote_value_with_quotes() { - assert_eq!(quote_value("with\"quote"), "\"with\\\"quote\""); - } - - #[test] - fn test_quote_value_with_hash() { - assert_eq!(quote_value("with#hash"), "\"with#hash\""); - } - - #[test] - fn test_update_env_content_new_key() { - let content = "EXISTING=value\n"; - let mut updates = HashMap::new(); - updates.insert("NEW_KEY".to_string(), "new_value".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("EXISTING=value")); - assert!(result.contains("NEW_KEY=new_value")); - assert!(result.contains("# Auto-configured webhook secrets")); - } - - #[test] - fn test_update_env_content_update_existing() { - let content = "KEY=old_value\n"; - let mut updates = HashMap::new(); - updates.insert("KEY".to_string(), "new_value".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("KEY=new_value")); - assert!(!result.contains("old_value")); - } - - #[test] - fn test_update_env_content_preserve_comments() { - let content = "# This is a comment\nKEY=value\n"; - let mut updates = HashMap::new(); - updates.insert("KEY".to_string(), "new_value".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("# This is a comment")); - assert!(result.contains("KEY=new_value")); - } - - #[test] - fn test_update_env_content_preserve_empty_lines() { - let content = "KEY1=value1\n\nKEY2=value2\n"; - let mut updates = HashMap::new(); - updates.insert("KEY1".to_string(), "new1".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("KEY1=new1")); - assert!(result.contains("\n\n")); // Empty line preserved - } - - #[test] - fn test_update_env_content_empty_file() { - let content = ""; - let mut updates = HashMap::new(); - updates.insert("NEW_KEY".to_string(), "value".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("NEW_KEY=value")); - } - - #[test] - fn test_update_env_content_multiple_updates() { - let content = "KEY1=old1\nKEY2=old2\n"; - let mut updates = HashMap::new(); - updates.insert("KEY1".to_string(), "new1".to_string()); - updates.insert("KEY3".to_string(), "new3".to_string()); - - let result = update_env_content(content, &updates); - assert!(result.contains("KEY1=new1")); - assert!(result.contains("KEY2=old2")); - assert!(result.contains("KEY3=new3")); - } - - #[test] - fn test_update_env_file_creates_new() { - let temp_dir = TempDir::new().unwrap(); - let env_path = temp_dir.path().join(".env"); - - let mut updates = HashMap::new(); - updates.insert("KEY".to_string(), "value".to_string()); - - update_env_file(&env_path, &updates).unwrap(); - - let content = fs::read_to_string(&env_path).unwrap(); - assert!(content.contains("KEY=value")); - } - - #[test] - fn test_update_env_file_updates_existing() { - let temp_dir = TempDir::new().unwrap(); - let env_path = temp_dir.path().join(".env"); - - // Create initial file - fs::write(&env_path, "EXISTING=old\n").unwrap(); - - let mut updates = HashMap::new(); - updates.insert("EXISTING".to_string(), "new".to_string()); - updates.insert("NEW_KEY".to_string(), "value".to_string()); - - update_env_file(&env_path, &updates).unwrap(); - - let content = fs::read_to_string(&env_path).unwrap(); - assert!(content.contains("EXISTING=new")); - assert!(content.contains("NEW_KEY=value")); - } - - #[test] - fn test_update_env_content_ends_with_newline() { - let content = "KEY=value"; - let updates = HashMap::new(); - - let result = update_env_content(content, &updates); - assert!(result.ends_with('\n')); - } - - #[test] - fn test_parse_env_line_no_equals() { - assert!(parse_env_line("NOEQUALS").is_none()); - } - - #[test] - fn test_quote_value_with_backslash() { - let result = quote_value("path\\to\\file"); - assert_eq!(result, "\"path\\\\to\\\\file\""); - } - - #[test] - fn test_quote_value_with_dollar() { - assert_eq!(quote_value("value$var"), "\"value$var\""); - } - - #[test] - fn test_update_env_content_preserves_order() { - let content = "FIRST=1\nSECOND=2\nTHIRD=3\n"; - let mut updates = HashMap::new(); - updates.insert("SECOND".to_string(), "updated".to_string()); - - let result = update_env_content(content, &updates); - let first_pos = result.find("FIRST").unwrap(); - let second_pos = result.find("SECOND").unwrap(); - let third_pos = result.find("THIRD").unwrap(); - - assert!(first_pos < second_pos); - assert!(second_pos < third_pos); - } -} diff --git a/crates/claudear-config/src/environment.rs b/crates/claudear-config/src/environment.rs new file mode 100644 index 00000000..c57e5755 --- /dev/null +++ b/crates/claudear-config/src/environment.rs @@ -0,0 +1,94 @@ +//! The `.env` file that webhook setup and the GitHub App callback write secrets to. + +use abnegate_config::EnvironmentFile; +use claudear_core::error::{Error, Result}; +use std::collections::BTreeMap; +use std::error::Error as StandardError; +use std::fs; +use std::io; +use std::iter; +use std::path::Path; +use tempfile::NamedTempFile; + +pub const DISCORD_WEBHOOK_URL: &str = "DISCORD_WEBHOOK_URL"; +pub const GITHUB_APP_BASE_URL: &str = "GITHUB_APP_BASE_URL"; +pub const GITHUB_APP_CLIENT_ID: &str = "GITHUB_APP_CLIENT_ID"; +pub const GITHUB_APP_CLIENT_SECRET: &str = "GITHUB_APP_CLIENT_SECRET"; +pub const GITHUB_APP_ID: &str = "GITHUB_APP_ID"; +pub const GITHUB_APP_PRIVATE_KEY_PATH: &str = "GITHUB_APP_PRIVATE_KEY_PATH"; +pub const GITHUB_APP_WEBHOOK_SECRET: &str = "GITHUB_APP_WEBHOOK_SECRET"; +pub const GITHUB_WEBHOOK_SECRET: &str = "GITHUB_WEBHOOK_SECRET"; +pub const GITLAB_WEBHOOK_SECRET: &str = "GITLAB_WEBHOOK_SECRET"; +pub const LINEAR_WEBHOOK_SECRET: &str = "LINEAR_WEBHOOK_SECRET"; +pub const SENTRY_CLIENT_SECRET: &str = "SENTRY_CLIENT_SECRET"; +pub const TELEGRAM_WEBHOOK_SECRET: &str = "TELEGRAM_WEBHOOK_SECRET"; +pub const WHATSAPP_WEBHOOK_VERIFY_TOKEN: &str = "WHATSAPP_WEBHOOK_VERIFY_TOKEN"; + +/// Every key claudear writes to the `.env` file. +pub const KEYS: &[&str] = &[ + DISCORD_WEBHOOK_URL, + GITHUB_APP_BASE_URL, + GITHUB_APP_CLIENT_ID, + GITHUB_APP_CLIENT_SECRET, + GITHUB_APP_ID, + GITHUB_APP_PRIVATE_KEY_PATH, + GITHUB_APP_WEBHOOK_SECRET, + GITHUB_WEBHOOK_SECRET, + GITLAB_WEBHOOK_SECRET, + LINEAR_WEBHOOK_SECRET, + SENTRY_CLIENT_SECRET, + TELEGRAM_WEBHOOK_SECRET, + WHATSAPP_WEBHOOK_VERIFY_TOKEN, +]; + +/// Sets every key in `values` in the `.env` file at `path`, keeping its other +/// lines, and replaces the file atomically as readable only by its owner. +/// +/// The replacement is renamed into place, so the parent directory must be +/// writable. Failures become [`Error::Config`] carrying the full cause chain. +pub fn update(path: &Path, values: &BTreeMap) -> Result<()> { + EnvironmentFile::new(path) + .update(values) + .map_err(|error| Error::config(describe(&error))) +} + +/// Fails the way [`update`] would, without changing anything: the file, if it +/// exists, must be readable, and the directory its replacement is renamed from +/// must accept a new file. When that directory does not exist yet, the nearest +/// existing ancestor is checked instead, since [`update`] creates it there. +pub fn probe(path: &Path) -> Result<()> { + match fs::read_to_string(path) { + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + return Err(Error::config(format!( + "Failed to read '{}': {error}", + path.display() + ))); + } + } + + NamedTempFile::new_in(nearest_existing_directory(path)) + .map(drop) + .map_err(|error| Error::config(format!("Failed to write '{}': {error}", path.display()))) +} + +fn nearest_existing_directory(path: &Path) -> &Path { + let current = Path::new("."); + let parent = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or(current); + + parent + .ancestors() + .find(|ancestor| !ancestor.as_os_str().is_empty() && ancestor.exists()) + .unwrap_or(current) +} + +fn describe(error: &(dyn StandardError + 'static)) -> String { + iter::successors(Some(error), |&error| error.source()) + .map(ToString::to_string) + .collect::>() + .join(": ") +} diff --git a/crates/claudear-config/src/lib.rs b/crates/claudear-config/src/lib.rs index e0ac95f9..f62de003 100644 --- a/crates/claudear-config/src/lib.rs +++ b/crates/claudear-config/src/lib.rs @@ -1,9 +1,8 @@ //! Configuration loading, validation, and user registry for claudear. pub mod config; -pub mod env_writer; +pub mod environment; pub mod users; pub use config::*; -pub use env_writer::update_env_file; pub use users::{ResolvedUser, UserRegistry}; diff --git a/crates/claudear-config/tests/environment.rs b/crates/claudear-config/tests/environment.rs new file mode 100644 index 00000000..907e7090 --- /dev/null +++ b/crates/claudear-config/tests/environment.rs @@ -0,0 +1,279 @@ +use std::collections::BTreeMap; +use std::fs; +use std::path::Path; + +use abnegate_config::EnvironmentFile; +use claudear_config::environment; +use claudear_core::error::Error; +use tempfile::TempDir; + +const TRICKY_VALUES: [&str; 7] = [ + "4f9c2a7e1b3d5f60", + "https://discord.com/api/webhooks/123/abc-DEF_ghi", + "with spaces", + "pa$$word", + "it's \"quoted\" \\ `id`", + "line\nbreak\rreturn", + "", +]; + +fn values(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect() +} + +fn read(path: &Path) -> BTreeMap { + EnvironmentFile::new(path).read().unwrap() +} + +#[test] +fn test_every_value_reads_back_unchanged_under_every_key_claudear_writes() { + for value in TRICKY_VALUES { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + let written: BTreeMap = environment::KEYS + .iter() + .map(|key| (key.to_string(), value.to_string())) + .collect(); + assert_eq!(written.len(), environment::KEYS.len()); + + environment::update(&path, &written).unwrap(); + + assert_eq!(read(&path), written, "value {value:?}"); + } +} + +#[test] +fn test_existing_lines_survive_and_new_keys_are_appended_sorted_without_a_header() { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + fs::write( + &path, + "# Claudear\nCLAUDEAR_LINEAR_API_KEY=lin_api_123\n\nGITHUB_WEBHOOK_SECRET=old\nCLAUDEAR_LOG=info\n", + ) + .unwrap(); + + environment::update( + &path, + &values(&[ + (environment::LINEAR_WEBHOOK_SECRET, "linear"), + (environment::GITHUB_WEBHOOK_SECRET, "new"), + (environment::GITLAB_WEBHOOK_SECRET, "gitlab"), + ]), + ) + .unwrap(); + + assert_eq!( + fs::read_to_string(&path).unwrap(), + "# Claudear\nCLAUDEAR_LINEAR_API_KEY=lin_api_123\n\nGITHUB_WEBHOOK_SECRET=new\nCLAUDEAR_LOG=info\n\nGITLAB_WEBHOOK_SECRET=gitlab\nLINEAR_WEBHOOK_SECRET=linear\n" + ); +} + +#[test] +fn test_values_are_quoted_so_a_shell_reads_them_literally() { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + + environment::update( + &path, + &values(&[ + ( + environment::DISCORD_WEBHOOK_URL, + "https://discord.com/api/webhooks/1/a-b_c", + ), + (environment::GITHUB_APP_CLIENT_SECRET, "with spaces"), + (environment::GITHUB_APP_ID, "123456"), + (environment::GITHUB_WEBHOOK_SECRET, "pa$$word#1"), + (environment::LINEAR_WEBHOOK_SECRET, "it's"), + (environment::SENTRY_CLIENT_SECRET, "line\nINJECTED=1"), + ]), + ) + .unwrap(); + + assert_eq!( + fs::read_to_string(&path).unwrap(), + concat!( + "DISCORD_WEBHOOK_URL=https://discord.com/api/webhooks/1/a-b_c\n", + "GITHUB_APP_CLIENT_SECRET='with spaces'\n", + "GITHUB_APP_ID=123456\n", + "GITHUB_WEBHOOK_SECRET='pa$$word#1'\n", + "LINEAR_WEBHOOK_SECRET=\"it's\"\n", + "SENTRY_CLIENT_SECRET=\"line\\nINJECTED=1\"\n", + ) + ); +} + +#[test] +fn test_a_file_written_by_the_previous_writer_still_reads_and_keeps_its_lines() { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + let previous = "EXISTING=keep\n\n# Auto-configured webhook secrets\nLINEAR_WEBHOOK_SECRET=\"with spaces\"\nSENTRY_CLIENT_SECRET=\"say \\\"hi\\\" \\\\ $HOME\"\n"; + fs::write(&path, previous).unwrap(); + + environment::update( + &path, + &values(&[(environment::TELEGRAM_WEBHOOK_SECRET, "telegram")]), + ) + .unwrap(); + + assert_eq!( + fs::read_to_string(&path).unwrap(), + format!("{previous}\nTELEGRAM_WEBHOOK_SECRET=telegram\n") + ); + assert_eq!( + read(&path), + values(&[ + ("EXISTING", "keep"), + (environment::LINEAR_WEBHOOK_SECRET, "with spaces"), + (environment::SENTRY_CLIENT_SECRET, "say \"hi\" \\ $HOME"), + (environment::TELEGRAM_WEBHOOK_SECRET, "telegram"), + ]) + ); +} + +#[test] +fn test_writing_the_same_values_again_changes_nothing() { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + let secrets = values(&[ + (environment::GITHUB_WEBHOOK_SECRET, "with spaces"), + (environment::LINEAR_WEBHOOK_SECRET, "abc123"), + ]); + environment::update(&path, &secrets).unwrap(); + let first = fs::read_to_string(&path).unwrap(); + + environment::update(&path, &secrets).unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), first); +} + +#[test] +fn test_a_missing_file_and_its_directory_are_created() { + let directory = TempDir::new().unwrap(); + let nested = directory.path().join("nested"); + let path = nested.join(".env"); + + environment::update(&path, &values(&[(environment::GITHUB_APP_ID, "1")])).unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), "GITHUB_APP_ID=1\n"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + let mode = fs::metadata(&nested).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "directory mode was {mode:o}"); + } +} + +#[cfg(unix)] +#[test] +fn test_the_file_is_readable_only_by_its_owner() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + fs::write(&path, "EXISTING=value\n").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + + environment::update( + &path, + &values(&[(environment::GITHUB_WEBHOOK_SECRET, "secret")]), + ) + .unwrap(); + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "mode was {mode:o}"); +} + +#[cfg(unix)] +#[test] +fn test_a_read_only_directory_fails_with_a_config_error_naming_the_cause() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + let locked = directory.path().join("locked"); + fs::create_dir(&locked).unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o500)).unwrap(); + + let probe = locked.join("probe"); + if fs::write(&probe, "").is_ok() { + fs::remove_file(&probe).unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o700)).unwrap(); + eprintln!("skipped: this user can write to a 0500 directory"); + return; + } + + let result = environment::update( + &locked.join(".env"), + &values(&[(environment::GITHUB_WEBHOOK_SECRET, "secret")]), + ); + + fs::set_permissions(&locked, fs::Permissions::from_mode(0o700)).unwrap(); + match result { + Err(Error::Config(message)) => { + assert!(message.contains("Failed to write"), "{message}"); + assert!(message.contains("Permission denied"), "{message}"); + } + other => panic!("expected a config error, got {other:?}"), + } +} + +#[test] +fn test_probe_leaves_no_trace_when_the_file_and_its_directory_are_missing() { + let directory = TempDir::new().unwrap(); + let nested = directory.path().join("nested"); + let path = nested.join(".env"); + + environment::probe(&path).unwrap(); + + assert!(!nested.exists()); + assert_eq!(fs::read_dir(directory.path()).unwrap().count(), 0); +} + +#[test] +fn test_probe_leaves_an_existing_file_untouched() { + let directory = TempDir::new().unwrap(); + let path = directory.path().join(".env"); + fs::write(&path, "EXISTING=value\n").unwrap(); + + environment::probe(&path).unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), "EXISTING=value\n"); + assert_eq!(fs::read_dir(directory.path()).unwrap().count(), 1); +} + +#[cfg(unix)] +#[test] +fn test_probe_fails_like_update_when_the_directory_is_read_only() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + let locked = directory.path().join("locked"); + fs::create_dir(&locked).unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o500)).unwrap(); + + let probe = locked.join("probe"); + if fs::write(&probe, "").is_ok() { + fs::remove_file(&probe).unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o700)).unwrap(); + eprintln!("skipped: this user can write to a 0500 directory"); + return; + } + + let probed = environment::probe(&locked.join(".env")); + let nested = environment::probe(&locked.join("missing").join(".env")); + + fs::set_permissions(&locked, fs::Permissions::from_mode(0o700)).unwrap(); + for result in [probed, nested] { + match result { + Err(Error::Config(message)) => { + assert!(message.contains("Failed to write"), "{message}"); + assert!(message.contains("Permission denied"), "{message}"); + } + other => panic!("expected a config error, got {other:?}"), + } + } +} diff --git a/crates/claudear-integrations/src/github_app/routes.rs b/crates/claudear-integrations/src/github_app/routes.rs index aa3fc090..b372db11 100644 --- a/crates/claudear-integrations/src/github_app/routes.rs +++ b/crates/claudear-integrations/src/github_app/routes.rs @@ -11,9 +11,10 @@ use crate::github_app::manifest::AppManifest; use axum::extract::Query; use axum::response::{Html, Redirect}; -use claudear_config::env_writer::update_env_file; +use claudear_config::environment; use claudear_core::error::{Error, Result}; use serde::Deserialize; +use std::collections::BTreeMap; use std::collections::HashMap; use std::fs; use std::path::Path; @@ -276,46 +277,50 @@ async fn exchange_code_for_credentials(code: &str) -> Result Result<()> { - // Save private key to PEM file +fn save_credentials(credentials: &ManifestConversionResponse, base_url: &str) -> Result<()> { let pem_path = "github-app-key.pem"; - fs::write(pem_path, &creds.pem).map_err(|e| { + fs::write(pem_path, &credentials.pem).map_err(|error| { Error::config(format!( "Failed to write private key to {}: {}", - pem_path, e + pem_path, error )) })?; - // Set restrictive permissions on the PEM file (Unix only) #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - fs::set_permissions(pem_path, perms).ok(); - } - - // Update .env file - let env_path = Path::new(".env"); - let mut updates = HashMap::new(); - updates.insert("GITHUB_APP_ID".to_string(), creds.id.to_string()); - updates.insert( - "GITHUB_APP_PRIVATE_KEY_PATH".to_string(), - pem_path.to_string(), - ); - updates.insert( - "GITHUB_APP_WEBHOOK_SECRET".to_string(), - creds.webhook_secret.clone(), - ); - updates.insert("GITHUB_APP_CLIENT_ID".to_string(), creds.client_id.clone()); - updates.insert( - "GITHUB_APP_CLIENT_SECRET".to_string(), - creds.client_secret.clone(), - ); - updates.insert("GITHUB_APP_BASE_URL".to_string(), base_url.to_string()); - - update_env_file(env_path, &updates)?; - - Ok(()) + let permissions = std::fs::Permissions::from_mode(0o600); + fs::set_permissions(pem_path, permissions).ok(); + } + + let updates = BTreeMap::from([ + ( + environment::GITHUB_APP_ID.to_string(), + credentials.id.to_string(), + ), + ( + environment::GITHUB_APP_PRIVATE_KEY_PATH.to_string(), + pem_path.to_string(), + ), + ( + environment::GITHUB_APP_WEBHOOK_SECRET.to_string(), + credentials.webhook_secret.clone(), + ), + ( + environment::GITHUB_APP_CLIENT_ID.to_string(), + credentials.client_id.clone(), + ), + ( + environment::GITHUB_APP_CLIENT_SECRET.to_string(), + credentials.client_secret.clone(), + ), + ( + environment::GITHUB_APP_BASE_URL.to_string(), + base_url.to_string(), + ), + ]); + + environment::update(Path::new(".env"), &updates) } /// Escape HTML special characters to prevent XSS. diff --git a/crates/claudear-integrations/src/webhook/configurator.rs b/crates/claudear-integrations/src/webhook/configurator.rs index 443bfaf8..d41147a6 100644 --- a/crates/claudear-integrations/src/webhook/configurator.rs +++ b/crates/claudear-integrations/src/webhook/configurator.rs @@ -7,10 +7,10 @@ use claudear_config::config::{ Config, DiscordNotifierConfig, GitHubConfig, GitLabConfig, JiraConfig, LinearConfig, SentryConfig, SlackSourceConfig, TelegramConfig, WhatsAppConfig, }; -use claudear_config::env_writer::update_env_file; +use claudear_config::environment; use claudear_core::error::{Error, Result}; use serde::Deserialize; -use std::collections::HashMap; +use std::collections::BTreeMap; use uuid::Uuid; /// Result of webhook auto-configuration. @@ -98,7 +98,10 @@ impl WebhookConfigurator { /// (Linear, Sentry, GitHub PAT/App, GitLab, Jira, Telegram, Slack, WhatsApp, /// and Discord notifier webhook URLs) /// 2. Emit notes for enabled services that currently require manual setup - /// 2. Write the returned secrets to the .env file + /// 3. Write the returned secrets to the .env file + /// + /// The .env file is checked first, so a path the secrets could not be + /// written to fails before any webhook is created remotely. /// /// # Arguments /// * `base_url` - The public URL where webhooks will be received @@ -106,8 +109,10 @@ impl WebhookConfigurator { tracing::info!("Starting webhook auto-configuration..."); tracing::info!("Base URL: {}", base_url); + environment::probe(&self.env_path)?; + let mut result = WebhookSetupResult::default(); - let mut env_updates: HashMap = HashMap::new(); + let mut updates: BTreeMap = BTreeMap::new(); let mut configured_any = false; let mut attempted_auto_config = false; let mut failure_warnings: Vec = Vec::new(); @@ -122,7 +127,7 @@ impl WebhookConfigurator { result.linear_configured = true; result.linear_webhook_id = Some(webhook_id); result.linear_secret = Some(secret.clone()); - env_updates.insert("LINEAR_WEBHOOK_SECRET".to_string(), secret); + updates.insert(environment::LINEAR_WEBHOOK_SECRET.to_string(), secret); configured_any = true; tracing::info!("Linear webhook configured successfully"); } @@ -145,7 +150,7 @@ impl WebhookConfigurator { result.sentry_project_count = count; if let Some(s) = secret { result.sentry_secret = Some(s.clone()); - env_updates.insert("SENTRY_CLIENT_SECRET".to_string(), s); + updates.insert(environment::SENTRY_CLIENT_SECRET.to_string(), s); } configured_any = true; tracing::info!("Sentry webhooks configured for {} project(s)", count); @@ -216,7 +221,8 @@ impl WebhookConfigurator { Ok(generated_secret) => { configured_any = true; if let Some(secret) = generated_secret { - env_updates.insert("TELEGRAM_WEBHOOK_SECRET".to_string(), secret); + updates + .insert(environment::TELEGRAM_WEBHOOK_SECRET.to_string(), secret); } notes.push("Telegram webhook configured (Bot API setWebhook)".to_string()); } @@ -255,7 +261,7 @@ impl WebhookConfigurator { match self.configure_discord_notifier(discord_notifier).await { Ok(webhook_url) => { configured_any = true; - env_updates.insert("DISCORD_WEBHOOK_URL".to_string(), webhook_url); + updates.insert(environment::DISCORD_WEBHOOK_URL.to_string(), webhook_url); notes.push( "Discord notifier webhook URL auto-created for configured channel" .to_string(), @@ -378,7 +384,10 @@ impl WebhookConfigurator { Ok(generated_verify_token) => { configured_any = true; if let Some(token) = generated_verify_token { - env_updates.insert("WHATSAPP_WEBHOOK_VERIFY_TOKEN".to_string(), token); + updates.insert( + environment::WHATSAPP_WEBHOOK_VERIFY_TOKEN.to_string(), + token, + ); } notes.push( "WhatsApp webhook subscription configured (WABA subscribed_apps)" @@ -418,7 +427,8 @@ impl WebhookConfigurator { Ok((group_count, generated_secret)) => { configured_any = true; if let Some(secret) = generated_secret { - env_updates.insert("GITLAB_WEBHOOK_SECRET".to_string(), secret); + updates + .insert(environment::GITLAB_WEBHOOK_SECRET.to_string(), secret); } let note = format!( "GitLab issue webhooks configured for {} group(s)", @@ -455,7 +465,7 @@ impl WebhookConfigurator { Ok((repo_count, generated_secret)) => { configured_any = true; if let Some(secret) = generated_secret { - env_updates.insert("GITHUB_WEBHOOK_SECRET".to_string(), secret); + updates.insert(environment::GITHUB_WEBHOOK_SECRET.to_string(), secret); } let note = format!( "GitHub review webhooks configured for {} repo(s)", @@ -477,11 +487,13 @@ impl WebhookConfigurator { Ok((secret, persist_app_secret, persist_github_secret)) => { configured_any = true; if persist_app_secret { - env_updates - .insert("GITHUB_APP_WEBHOOK_SECRET".to_string(), secret.clone()); + updates.insert( + environment::GITHUB_APP_WEBHOOK_SECRET.to_string(), + secret.clone(), + ); } if persist_github_secret { - env_updates.insert("GITHUB_WEBHOOK_SECRET".to_string(), secret); + updates.insert(environment::GITHUB_WEBHOOK_SECRET.to_string(), secret); } notes .push("GitHub App webhook configured via /app/hook/config".to_string()); @@ -499,10 +511,9 @@ impl WebhookConfigurator { } } - // Write secrets to .env file - if !env_updates.is_empty() { + if !updates.is_empty() { tracing::info!("Writing secrets to {}", self.env_path.display()); - update_env_file(&self.env_path, &env_updates)?; + environment::update(&self.env_path, &updates)?; } result.warnings.extend(failure_warnings.clone()); @@ -2806,6 +2817,63 @@ mod tests { assert!(err_msg.contains("Jira")); } + #[cfg(unix)] + #[tokio::test] + async fn test_configure_fails_before_any_remote_call_when_the_env_directory_is_read_only() { + use std::os::unix::fs::PermissionsExt; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + let directory = tempfile::TempDir::new().unwrap(); + let locked = directory.path().join("locked"); + std::fs::create_dir(&locked).unwrap(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o500)).unwrap(); + let probe = locked.join("probe"); + if std::fs::write(&probe, "").is_ok() { + std::fs::remove_file(&probe).unwrap(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o700)).unwrap(); + eprintln!("skipped: this user can write to a 0500 directory"); + return; + } + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let connections = Arc::new(AtomicUsize::new(0)); + let accepted = Arc::clone(&connections); + let server = tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + accepted.fetch_add(1, Ordering::SeqCst); + drop(stream); + } + }); + + let mut config = test_config(); + config.issues.jira = Some(JiraConfig { + enabled: true, + base_url: format!("http://{address}"), + api_token: "jira-token".into(), + ..Default::default() + }); + let configurator = WebhookConfigurator::new(config, locked.join(".env")); + + let result = configurator.configure("https://example.com").await; + + server.abort(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o700)).unwrap(); + assert_eq!( + connections.load(Ordering::SeqCst), + 0, + "a webhook API was called before the .env path was checked" + ); + match result { + Err(Error::Config(message)) => { + assert!(message.contains("Failed to write"), "{message}"); + assert!(message.contains("Permission denied"), "{message}"); + } + other => panic!("expected a config error, got {other:?}"), + } + } + #[tokio::test] async fn test_configure_linear_disabled_sentry_disabled_returns_error() { // Both sources present but disabled => error about no sources enabled diff --git a/src/lib.rs b/src/lib.rs index b069c055..9e05d2ab 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,7 +34,6 @@ pub use claudear_core::types; // Re-exported from claudear-config pub use claudear_config::config; -pub use claudear_config::env_writer; pub use claudear_config::users; // Re-exported from claudear-storage From e2983267095ed7aaf252605d22ddf156ba31c740 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 9 Oct 2026 15:29:27 +1300 Subject: [PATCH 2/2] fix(config): refuse a .env that a rename cannot replace before creating webhooks The probe only created a temporary file next to the destination. That succeeds for a single file bind-mounted into a container and for another user's file in a sticky directory, yet the rename that replaces it then fails with EBUSY, EXDEV or EPERM, after webhook setup has already created the remote webhooks whose secrets it cannot save. It also passed a directory that cannot be opened for the post-rename sync, which the writer reports as a failure. The probe now rehearses the writer: it renames a sibling temporary file over a scratch file in the same directory and syncs the directory, and it refuses a destination that is a mount point (its device differs from its directory's, or /proc/self/mountinfo lists it) or that belongs to another user in a sticky directory neither of them owns. Co-Authored-By: Claude Opus 5.5 --- crates/claudear-config/src/environment.rs | 83 ++++++-- .../src/environment/mount_table.rs | 118 +++++++++++ .../src/environment/refusal.rs | 25 +++ .../src/environment/replacement.rs | 200 ++++++++++++++++++ crates/claudear-config/tests/environment.rs | 61 ++++++ 5 files changed, 474 insertions(+), 13 deletions(-) create mode 100644 crates/claudear-config/src/environment/mount_table.rs create mode 100644 crates/claudear-config/src/environment/refusal.rs create mode 100644 crates/claudear-config/src/environment/replacement.rs diff --git a/crates/claudear-config/src/environment.rs b/crates/claudear-config/src/environment.rs index c57e5755..93ead7ee 100644 --- a/crates/claudear-config/src/environment.rs +++ b/crates/claudear-config/src/environment.rs @@ -1,15 +1,26 @@ //! The `.env` file that webhook setup and the GitHub App callback write secrets to. +#[cfg(unix)] +mod mount_table; +#[cfg(unix)] +mod refusal; +#[cfg(unix)] +mod replacement; + use abnegate_config::EnvironmentFile; use claudear_core::error::{Error, Result}; use std::collections::BTreeMap; use std::error::Error as StandardError; use std::fs; +use std::fs::File; use std::io; use std::iter; use std::path::Path; use tempfile::NamedTempFile; +#[cfg(unix)] +use replacement::Replacement; + pub const DISCORD_WEBHOOK_URL: &str = "DISCORD_WEBHOOK_URL"; pub const GITHUB_APP_BASE_URL: &str = "GITHUB_APP_BASE_URL"; pub const GITHUB_APP_CLIENT_ID: &str = "GITHUB_APP_CLIENT_ID"; @@ -52,10 +63,15 @@ pub fn update(path: &Path, values: &BTreeMap) -> Result<()> { .map_err(|error| Error::config(describe(&error))) } -/// Fails the way [`update`] would, without changing anything: the file, if it -/// exists, must be readable, and the directory its replacement is renamed from -/// must accept a new file. When that directory does not exist yet, the nearest -/// existing ancestor is checked instead, since [`update`] creates it there. +/// Fails the way [`update`] would, without changing anything. +/// +/// The file, if it exists, must be readable. Its directory must let a sibling +/// temporary file be renamed over another file and then be synced, which is +/// how [`update`] replaces it. A file a rename cannot replace is refused: a +/// mount point, such as a single file bind-mounted into a container, or +/// another user's file in a sticky directory. When the directory does not +/// exist yet, the nearest existing ancestor must accept a new entry instead, +/// since [`update`] creates the directory there. pub fn probe(path: &Path) -> Result<()> { match fs::read_to_string(path) { Ok(_) => {} @@ -68,22 +84,63 @@ pub fn probe(path: &Path) -> Result<()> { } } - NamedTempFile::new_in(nearest_existing_directory(path)) - .map(drop) + let directory = directory_of(path); + let rehearsal = if directory.is_dir() { + rehearse_replacement(path, directory) + } else { + NamedTempFile::new_in(nearest_existing_ancestor(directory)).map(drop) + }; + + rehearsal .map_err(|error| Error::config(format!("Failed to write '{}': {error}", path.display()))) } -fn nearest_existing_directory(path: &Path) -> &Path { - let current = Path::new("."); - let parent = path - .parent() +fn rehearse_replacement(destination: &Path, directory: &Path) -> io::Result<()> { + let temporary = NamedTempFile::new_in(directory)?; + let scratch = NamedTempFile::new_in(directory)?.into_temp_path(); + + refuse_unreplaceable(destination, directory, temporary.as_file())?; + temporary.persist(&scratch)?; + + sync(directory) +} + +#[cfg(unix)] +fn refuse_unreplaceable(destination: &Path, directory: &Path, created: &File) -> io::Result<()> { + match Replacement::inspect(destination, directory, created)? + .and_then(|replacement| replacement.refusal()) + { + Some(refusal) => Err(io::Error::other(refusal.to_string())), + None => Ok(()), + } +} + +#[cfg(not(unix))] +fn refuse_unreplaceable(_destination: &Path, _directory: &Path, _created: &File) -> io::Result<()> { + Ok(()) +} + +#[cfg(unix)] +fn sync(directory: &Path) -> io::Result<()> { + File::open(directory)?.sync_all() +} + +#[cfg(not(unix))] +fn sync(_directory: &Path) -> io::Result<()> { + Ok(()) +} + +fn directory_of(path: &Path) -> &Path { + path.parent() .filter(|parent| !parent.as_os_str().is_empty()) - .unwrap_or(current); + .unwrap_or(Path::new(".")) +} - parent +fn nearest_existing_ancestor(directory: &Path) -> &Path { + directory .ancestors() .find(|ancestor| !ancestor.as_os_str().is_empty() && ancestor.exists()) - .unwrap_or(current) + .unwrap_or(Path::new(".")) } fn describe(error: &(dyn StandardError + 'static)) -> String { diff --git a/crates/claudear-config/src/environment/mount_table.rs b/crates/claudear-config/src/environment/mount_table.rs new file mode 100644 index 00000000..bfba9834 --- /dev/null +++ b/crates/claudear-config/src/environment/mount_table.rs @@ -0,0 +1,118 @@ +use std::ffi::OsString; +use std::fs; +use std::os::unix::ffi::OsStringExt; +use std::path::Path; +use std::path::PathBuf; + +const MOUNT_INFORMATION: &str = "/proc/self/mountinfo"; +const MOUNT_POINT_FIELD: usize = 4; +const FIELD_SEPARATOR: char = ' '; +const ESCAPE: u8 = b'\\'; +const OCTAL_DIGITS: usize = 3; +const OCTAL_RADIX: u32 = 8; + +/// The mount points this process sees, as Linux lists them in +/// `/proc/self/mountinfo`. Empty where that file does not exist. +#[derive(Debug, Default, PartialEq, Eq)] +pub(super) struct MountTable { + mount_points: Vec, +} + +impl MountTable { + pub(super) fn read() -> Self { + fs::read_to_string(MOUNT_INFORMATION) + .map(|table| Self::parse(&table)) + .unwrap_or_default() + } + + pub(super) fn contains(&self, path: &Path) -> bool { + self.mount_points.iter().any(|point| point == path) + } + + fn parse(table: &str) -> Self { + Self { + mount_points: table + .lines() + .filter_map(|line| line.split(FIELD_SEPARATOR).nth(MOUNT_POINT_FIELD)) + .map(Self::unescape) + .collect(), + } + } + + fn unescape(field: &str) -> PathBuf { + let bytes = field.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut index = 0; + + while index < bytes.len() { + match Self::escaped_byte(&bytes[index..]) { + Some(byte) => { + decoded.push(byte); + index += 1 + OCTAL_DIGITS; + } + None => { + decoded.push(bytes[index]); + index += 1; + } + } + } + + PathBuf::from(OsString::from_vec(decoded)) + } + + fn escaped_byte(bytes: &[u8]) -> Option { + let (&first, rest) = bytes.split_first()?; + let digits = rest.get(..OCTAL_DIGITS)?; + + if first != ESCAPE || !digits.iter().all(|digit| (b'0'..=b'7').contains(digit)) { + return None; + } + + let value = digits.iter().fold(0, |value, digit| { + value * OCTAL_RADIX + u32::from(digit - b'0') + }); + u8::try_from(value).ok() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const TABLE: &str = concat!( + "22 1 259:2 / / rw,relatime shared:1 - ext4 /dev/root rw\n", + "36 22 0:52 / /app rw,relatime - overlay overlay rw\n", + "37 36 259:2 /srv/claudear/.env /app/.env rw,relatime - ext4 /dev/root rw\n", + "38 36 259:2 /srv/my\\040secrets /app/my\\040secrets\\134.env rw - ext4 /dev/root rw\n", + ); + + #[test] + fn test_a_single_file_bind_mount_is_a_mount_point() { + let table = MountTable::parse(TABLE); + + assert!(table.contains(Path::new("/app/.env"))); + assert!(table.contains(Path::new("/app"))); + assert!(!table.contains(Path::new("/app/other.env"))); + assert!(!table.contains(Path::new("/srv/claudear/.env"))); + } + + #[test] + fn test_escaped_spaces_and_backslashes_in_a_mount_point_are_decoded() { + let table = MountTable::parse(TABLE); + + assert!(table.contains(Path::new("/app/my secrets\\.env"))); + assert!(!table.contains(Path::new("/app/my\\040secrets\\134.env"))); + } + + #[test] + fn test_a_backslash_without_three_octal_digits_is_kept() { + assert_eq!(MountTable::unescape("/a\\9b"), PathBuf::from("/a\\9b")); + assert_eq!(MountTable::unescape("/a\\04"), PathBuf::from("/a\\04")); + assert_eq!(MountTable::unescape("/a\\777"), PathBuf::from("/a\\777")); + } + + #[test] + fn test_an_empty_table_contains_nothing() { + assert!(!MountTable::parse("").contains(Path::new("/"))); + } +} diff --git a/crates/claudear-config/src/environment/refusal.rs b/crates/claudear-config/src/environment/refusal.rs new file mode 100644 index 00000000..ae3613d4 --- /dev/null +++ b/crates/claudear-config/src/environment/refusal.rs @@ -0,0 +1,25 @@ +use std::fmt; + +/// Why a rename cannot replace a destination that is already there. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Refusal { + /// The destination is a mount point, such as a single file bind-mounted + /// into a container. Renaming over it fails with EBUSY or EXDEV. + MountPoint, + /// Another user owns the destination in a sticky directory this user does + /// not own either. Renaming over it fails with EPERM. + ForeignFileInStickyDirectory, +} + +impl fmt::Display for Refusal { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(match self { + Self::MountPoint => { + "it is a mount point, such as a single file bind-mounted into a container, so a rename cannot replace it; mount its directory instead" + } + Self::ForeignFileInStickyDirectory => { + "another user owns it in a sticky directory, so a rename cannot replace it; run as its owner or move it to a directory this user owns" + } + }) + } +} diff --git a/crates/claudear-config/src/environment/replacement.rs b/crates/claudear-config/src/environment/replacement.rs new file mode 100644 index 00000000..9f4620f4 --- /dev/null +++ b/crates/claudear-config/src/environment/replacement.rs @@ -0,0 +1,200 @@ +use std::fs; +use std::fs::File; +use std::io; +use std::os::unix::fs::MetadataExt; +use std::path::Path; + +use super::mount_table::MountTable; +use super::refusal::Refusal; + +const STICKY: u32 = 0o1000; +const SUPERUSER: u32 = 0; + +/// What the kernel weighs when a rename replaces a destination that is +/// already there. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) struct Replacement { + destination_device: u64, + directory_device: u64, + mounted: bool, + sticky: bool, + destination_owner: u32, + directory_owner: u32, + user: u32, +} + +impl Replacement { + /// The replacement of `destination` in `directory`, or `None` when nothing + /// is there to replace. `created` is a file this process just created in + /// `directory`: its owner is the user the kernel checks the rename as. + pub(super) fn inspect( + destination: &Path, + directory: &Path, + created: &File, + ) -> io::Result> { + let target = match fs::symlink_metadata(destination) { + Ok(target) => target, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(error), + }; + let container = fs::metadata(directory)?; + let mounted = match destination.file_name() { + Some(name) => MountTable::read().contains(&fs::canonicalize(directory)?.join(name)), + None => false, + }; + + Ok(Some(Self { + destination_device: target.dev(), + directory_device: container.dev(), + mounted, + sticky: container.mode() & STICKY != 0, + destination_owner: target.uid(), + directory_owner: container.uid(), + user: created.metadata()?.uid(), + })) + } + + pub(super) fn refusal(&self) -> Option { + if self.mounted || self.destination_device != self.directory_device { + return Some(Refusal::MountPoint); + } + + let privileged = [SUPERUSER, self.destination_owner, self.directory_owner]; + if self.sticky && !privileged.contains(&self.user) { + return Some(Refusal::ForeignFileInStickyDirectory); + } + + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::NamedTempFile; + use tempfile::TempDir; + + const DEVICE: u64 = 66; + const OTHER_DEVICE: u64 = 67; + const USER: u32 = 501; + const OTHER_USER: u32 = 502; + + fn ordinary() -> Replacement { + Replacement { + destination_device: DEVICE, + directory_device: DEVICE, + mounted: false, + sticky: false, + destination_owner: USER, + directory_owner: USER, + user: USER, + } + } + + fn foreign_in_sticky_directory() -> Replacement { + Replacement { + sticky: true, + destination_owner: OTHER_USER, + directory_owner: OTHER_USER, + ..ordinary() + } + } + + #[test] + fn test_an_own_file_on_its_directory_device_is_replaceable() { + assert_eq!(ordinary().refusal(), None); + } + + #[test] + fn test_a_file_on_another_device_than_its_directory_is_a_mount_point() { + let bind_mounted = Replacement { + destination_device: OTHER_DEVICE, + ..ordinary() + }; + + assert_eq!(bind_mounted.refusal(), Some(Refusal::MountPoint)); + } + + #[test] + fn test_a_mount_point_on_the_same_device_as_its_directory_is_refused() { + let bind_mounted = Replacement { + mounted: true, + ..ordinary() + }; + + assert_eq!(bind_mounted.refusal(), Some(Refusal::MountPoint)); + } + + #[test] + fn test_another_users_file_in_another_users_sticky_directory_is_refused() { + assert_eq!( + foreign_in_sticky_directory().refusal(), + Some(Refusal::ForeignFileInStickyDirectory) + ); + } + + #[test] + fn test_a_sticky_directory_lets_the_file_owner_the_directory_owner_and_root_replace() { + let file_owner = Replacement { + destination_owner: USER, + ..foreign_in_sticky_directory() + }; + let directory_owner = Replacement { + directory_owner: USER, + ..foreign_in_sticky_directory() + }; + let root = Replacement { + user: SUPERUSER, + ..foreign_in_sticky_directory() + }; + + for replacement in [file_owner, directory_owner, root] { + assert_eq!(replacement.refusal(), None, "{replacement:?}"); + } + } + + #[test] + fn test_another_users_file_without_the_sticky_bit_is_replaceable() { + let foreign = Replacement { + sticky: false, + ..foreign_in_sticky_directory() + }; + + assert_eq!(foreign.refusal(), None); + } + + #[test] + fn test_nothing_is_replaced_when_the_destination_is_missing() { + let directory = TempDir::new().unwrap(); + let created = NamedTempFile::new_in(directory.path()).unwrap(); + + let replacement = Replacement::inspect( + &directory.path().join(".env"), + directory.path(), + created.as_file(), + ) + .unwrap(); + + assert_eq!(replacement, None); + } + + #[test] + fn test_an_own_file_in_an_own_sticky_directory_is_inspected_as_replaceable() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + fs::set_permissions(directory.path(), fs::Permissions::from_mode(0o1700)).unwrap(); + let destination = directory.path().join(".env"); + fs::write(&destination, "EXISTING=value\n").unwrap(); + let created = NamedTempFile::new_in(directory.path()).unwrap(); + + let replacement = Replacement::inspect(&destination, directory.path(), created.as_file()) + .unwrap() + .unwrap(); + + assert!(replacement.sticky, "{replacement:?}"); + assert!(!replacement.mounted, "{replacement:?}"); + assert_eq!(replacement.destination_owner, replacement.user); + assert_eq!(replacement.refusal(), None); + } +} diff --git a/crates/claudear-config/tests/environment.rs b/crates/claudear-config/tests/environment.rs index 907e7090..8ff7fdd5 100644 --- a/crates/claudear-config/tests/environment.rs +++ b/crates/claudear-config/tests/environment.rs @@ -277,3 +277,64 @@ fn test_probe_fails_like_update_when_the_directory_is_read_only() { } } } + +#[cfg(unix)] +#[test] +fn test_probe_fails_like_update_when_the_directory_cannot_be_opened_to_sync_the_rename() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + let unlistable = directory.path().join("unlistable"); + fs::create_dir(&unlistable).unwrap(); + let path = unlistable.join(".env"); + fs::write(&path, "EXISTING=value\n").unwrap(); + fs::set_permissions(&unlistable, fs::Permissions::from_mode(0o300)).unwrap(); + + if fs::read_dir(&unlistable).is_ok() { + fs::set_permissions(&unlistable, fs::Permissions::from_mode(0o700)).unwrap(); + eprintln!("skipped: this user can list a 0300 directory"); + return; + } + + let probed = environment::probe(&path); + let updated = environment::update( + &path, + &values(&[(environment::GITHUB_WEBHOOK_SECRET, "secret")]), + ); + + fs::set_permissions(&unlistable, fs::Permissions::from_mode(0o700)).unwrap(); + assert!( + updated.is_err(), + "update succeeded, so the probe has nothing to catch" + ); + match probed { + Err(Error::Config(message)) => { + assert!(message.contains("Failed to write"), "{message}"); + assert!(message.contains("Permission denied"), "{message}"); + } + other => panic!("expected a config error, got {other:?}"), + } +} + +#[cfg(unix)] +#[test] +fn test_probe_accepts_a_file_this_user_owns_in_a_sticky_directory() { + use std::os::unix::fs::PermissionsExt; + + let directory = TempDir::new().unwrap(); + let sticky = directory.path().join("sticky"); + fs::create_dir(&sticky).unwrap(); + fs::set_permissions(&sticky, fs::Permissions::from_mode(0o1700)).unwrap(); + let path = sticky.join(".env"); + fs::write(&path, "EXISTING=value\n").unwrap(); + + environment::probe(&path).unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), "EXISTING=value\n"); + assert_eq!(fs::read_dir(&sticky).unwrap().count(), 1); + environment::update( + &path, + &values(&[(environment::GITHUB_WEBHOOK_SECRET, "secret")]), + ) + .unwrap(); +}