diff --git a/Cargo.lock b/Cargo.lock index c8ac1ac..326626c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -923,6 +923,7 @@ dependencies = [ "hex", "insta", "keyring", + "libc", "proptest", "regex", "reqwest 0.13.4", diff --git a/crates/clx-core/Cargo.toml b/crates/clx-core/Cargo.toml index e21eba1..c8437a0 100644 --- a/crates/clx-core/Cargo.toml +++ b/crates/clx-core/Cargo.toml @@ -67,6 +67,15 @@ fastembed = "5" security-framework = "2.11" core-foundation = "0.9" +# `bounded_read` opens untrusted-path files with O_NONBLOCK on Unix so the +# open() call itself cannot hang on a FIFO with no writer attached (a plain +# blocking open on such a FIFO blocks indefinitely -- verified -- which would +# reintroduce the exact hang the module exists to prevent). Already resolved +# in the workspace lockfile transitively (0.2.180); pinning it directly here +# adds no new supply-chain surface. +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [lints] workspace = true diff --git a/crates/clx-core/src/bounded_read.rs b/crates/clx-core/src/bounded_read.rs new file mode 100644 index 0000000..5eb9ce2 --- /dev/null +++ b/crates/clx-core/src/bounded_read.rs @@ -0,0 +1,301 @@ +//! Bounded, TOCTOU-hardened file reads for paths CLX did not choose itself. +//! +//! Three call sites in this codebase read a file whose path is either +//! externally supplied or otherwise not fully trusted (a credential-store +//! file that could have been replaced out from under us, a project +//! instructions file living in a repo we don't control, a config file a +//! user points the CLI at). Reading such a path with plain +//! `std::fs::read`/`read_to_string` has two failure modes: +//! +//! * A FIFO, character device (`/dev/zero`), block device, or socket at the +//! path blocks or streams forever. A naive `metadata.len()` size check +//! does not catch this: all of these report length 0, so the check passes +//! and the subsequent read never terminates (unbounded memory growth -> +//! OOM/SIGKILL, or the process simply hangs on a FIFO with no writer). +//! * A directory or a legitimate-looking but enormous regular file wastes +//! CPU and memory if read in full before any size check runs. +//! +//! [`read_bounded`] and [`read_bounded_to_string`] close both holes. They +//! generalize the pattern already established in +//! `clx-hook/src/transcript.rs` (`safe_transcript_path` / +//! `MAX_TRANSCRIPT_BYTES`), with one hardening improvement: this helper +//! calls `File::open` FIRST and `fstat`s the open handle (`file.metadata()`) +//! rather than `std::fs::metadata(path)` on the path, which closes the +//! TOCTOU window between checking the path and opening it. Opening the +//! handle and `fstat`-ing it also already resolves symlinks to the real +//! target, so no separate canonicalization step is needed here (unlike +//! `transcript.rs`, which canonicalizes for its own path-echoing reasons). +//! +//! ## Why the open itself is non-blocking on Unix +//! +//! Opening-before-stat has a sharp edge that the naive fix would reintroduce +//! the exact bug it's meant to close: a plain, blocking `File::open` on a +//! FIFO with **no writer connected** does not return at all until a writer +//! shows up (confirmed empirically: `open()` alone hangs, no `read()` +//! required). That means the open call itself — before we ever get to the +//! file-type check — would hang forever on exactly the FIFO case this +//! module exists to reject. To avoid that, the Unix open path sets +//! `O_NONBLOCK`. Per POSIX this makes a read-only FIFO open return +//! immediately regardless of whether a writer is attached; it has no effect +//! on regular files (open and read never block on those), so the flag is a +//! no-op for the common case and only changes behavior for the exact +//! pathological case it targets. The file-type check below still runs +//! before any `read`, so a FIFO is rejected without the process ever +//! blocking on it. +//! +//! This module never logs. Callers decide whether a rejection is a +//! best-effort skip (`warn!` + continue) or a hard error, and whether the +//! path needs `redact_secrets` before it appears in a log line or error +//! message. + +use std::fs::{File, FileType}; +use std::io::{ErrorKind, Read}; +use std::path::Path; + +/// Open `path` read-only such that the open call itself cannot block. +/// +/// On Unix this sets `O_NONBLOCK`, which is what prevents a blocking hang +/// when `path` is a FIFO with no writer attached (see module docs). The +/// flag is inert for regular files. Non-Unix targets have no equivalent +/// named-pipe-at-a-path hazard reachable via `std::fs::File::open`, so a +/// plain open is used there. +fn open_nonblocking(path: &Path) -> std::io::Result { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK) + .open(path) + } + #[cfg(not(unix))] + { + File::open(path) + } +} + +/// Error returned by [`read_bounded`] / [`read_bounded_to_string`]. +/// +/// Variants are distinct — rather than a single IO error bucket — so +/// callers can tell "there is no file here" (often a legitimate, silent +/// case) apart from "there IS something here but we refuse to read it" +/// (never silent: treating a refusal as equivalent to "absent" can be +/// actively harmful, e.g. the credentials-store call site, which must not +/// let a directory or FIFO dropped at `credentials.age` be mistaken for +/// "no credentials yet" and subsequently overwritten). +#[derive(Debug, thiserror::Error)] +pub enum Error { + /// No file exists at the given path. Mapped from `ErrorKind::NotFound` + /// on `File::open`. + #[error("file not found")] + NotFound, + + /// The path exists but is not a regular file: FIFO, character or block + /// device, directory, or socket. All of these report + /// `metadata.len() == 0`, which is exactly why this check must run + /// before, and independently of, the size check below — a size-only + /// guard would let every one of them through. + #[error("not a regular file: {file_type:?}")] + NotRegularFile { + /// The actual file type found at the path. + file_type: FileType, + }, + + /// The path is a regular file, but its size (read via `fstat` on the + /// already-open handle) exceeds the caller-supplied cap. + #[error("file is {len} bytes, exceeding the {cap} byte cap")] + TooLarge { + /// Size reported by `fstat`. + len: u64, + /// Cap the caller passed in. + cap: u64, + }, + + /// Any other IO failure: opening (other than not-found), reading, or + /// (for [`read_bounded_to_string`]) the file's bytes not being valid + /// UTF-8 (reported as `ErrorKind::InvalidData`). + #[error(transparent)] + Io(#[from] std::io::Error), +} + +/// Read up to `cap` bytes from the regular file at `path`. +/// +/// Rejects (see [`Error`]): +/// * a missing path (`Error::NotFound`); +/// * anything that is not a regular file (`Error::NotRegularFile`) — this +/// is what catches a FIFO, character/block device, directory, or socket, +/// all of which would otherwise pass a naive `len() == 0` size check; +/// * a regular file whose `fstat`-reported size exceeds `cap` +/// (`Error::TooLarge`). +/// +/// Even after the size gate passes, the actual read is still bounded by +/// `Read::take(cap)`: a regular file can grow between the `fstat` and the +/// read (a second, narrower TOCTOU window than the path-vs-handle one that +/// opening-then-`fstat`-ing already closed), so the reader itself must +/// never trust the earlier length forever. +pub fn read_bounded(path: &Path, cap: u64) -> Result, Error> { + let file = open_nonblocking(path).map_err(|e| { + if e.kind() == ErrorKind::NotFound { + Error::NotFound + } else { + Error::Io(e) + } + })?; + + // fstat the OPEN handle, not `std::fs::metadata(path)`: stat-then-open + // leaves a window where the path can be swapped between the check and + // the open (TOCTOU). fstat on an already-open fd has no such window. + let metadata = file.metadata()?; + + if !metadata.file_type().is_file() { + return Err(Error::NotRegularFile { + file_type: metadata.file_type(), + }); + } + + let len = metadata.len(); + if len > cap { + return Err(Error::TooLarge { len, cap }); + } + + // Pre-allocate by the bounded, already-validated length -- never by a + // raw untrusted value -- then still read through `take(cap)` in case + // the file grows between the fstat above and this read. + let prealloc = usize::try_from(len.min(cap)).unwrap_or(usize::MAX); + let mut buf = Vec::with_capacity(prealloc); + file.take(cap).read_to_end(&mut buf)?; + Ok(buf) +} + +/// Like [`read_bounded`], but decodes the result as UTF-8. +/// +/// Invalid UTF-8 is reported as `Error::Io` with `ErrorKind::InvalidData`, +/// matching how `std::fs::read_to_string` folds the same failure into +/// `io::Error`. +pub fn read_bounded_to_string(path: &Path, cap: u64) -> Result { + let bytes = read_bounded(path, cap)?; + String::from_utf8(bytes).map_err(|e| Error::Io(std::io::Error::new(ErrorKind::InvalidData, e))) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::io::Write; + + fn write_file(dir: &Path, name: &str, bytes: &[u8]) -> std::path::PathBuf { + let path = dir.join(name); + let mut f = File::create(&path).expect("create test file"); + f.write_all(bytes).expect("write test file"); + path + } + + #[test] + fn happy_path_reads_back_byte_identical() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = write_file(tmp.path(), "small.txt", b"hello bounded world"); + + let bytes = read_bounded(&path, 1024).expect("read_bounded should succeed"); + assert_eq!(bytes, b"hello bounded world"); + + let text = read_bounded_to_string(&path, 1024).expect("read_bounded_to_string"); + assert_eq!(text, "hello bounded world"); + } + + #[test] + fn one_byte_over_cap_is_rejected() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = write_file(tmp.path(), "over.bin", &[0u8; 11]); + + let err = + read_bounded(&path, 10).expect_err("11 bytes over a 10 byte cap must be rejected"); + match err { + Error::TooLarge { len, cap } => { + assert_eq!(len, 11); + assert_eq!(cap, 10); + } + other => panic!("expected TooLarge, got {other:?}"), + } + } + + #[test] + fn exactly_at_cap_is_accepted() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = write_file(tmp.path(), "exact.bin", &[7u8; 10]); + + let bytes = read_bounded(&path, 10).expect("exactly-at-cap file must be accepted"); + assert_eq!(bytes.len(), 10); + } + + #[test] + fn missing_path_is_not_found() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("does-not-exist.txt"); + + let err = read_bounded(&path, 1024).expect_err("missing file must error"); + assert!(matches!(err, Error::NotFound)); + } + + #[test] + fn directory_is_not_a_regular_file() { + let tmp = tempfile::tempdir().expect("tempdir"); + let dir_path = tmp.path().join("a-directory"); + fs::create_dir(&dir_path).expect("mkdir"); + + let err = read_bounded(&dir_path, 1024).expect_err("directory must be rejected"); + match err { + Error::NotRegularFile { file_type } => assert!(file_type.is_dir()), + other => panic!("expected NotRegularFile, got {other:?}"), + } + } + + #[test] + fn invalid_utf8_is_reported_as_io_invalid_data() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = write_file(tmp.path(), "invalid.bin", &[0xFF, 0xFE, 0xFD]); + + let err = read_bounded_to_string(&path, 1024).expect_err("invalid UTF-8 must error"); + match err { + Error::Io(io_err) => assert_eq!(io_err.kind(), ErrorKind::InvalidData), + other => panic!("expected Io(InvalidData), got {other:?}"), + } + } + + // FIFOs are unix-only. No `nix`/`libc` dev-dependency exists in this + // crate (checked before adding this test), so the FIFO is created by + // shelling out to the `mkfifo` binary rather than pulling in a new + // dependency just for one test. If `mkfifo` is unavailable in the test + // environment, the test skips instead of failing. + #[cfg(unix)] + #[test] + fn fifo_is_not_a_regular_file() { + let tmp = tempfile::tempdir().expect("tempdir"); + let fifo_path = tmp.path().join("a-fifo"); + + let status = std::process::Command::new("mkfifo") + .arg(&fifo_path) + .status(); + let Ok(status) = status else { + eprintln!("skipping fifo_is_not_a_regular_file: `mkfifo` not available"); + return; + }; + if !status.success() { + eprintln!("skipping fifo_is_not_a_regular_file: `mkfifo` failed"); + return; + } + + // No writer is ever connected to this FIFO. A plain blocking + // `File::open` would hang here indefinitely (verified: the open + // call itself blocks on a writerless FIFO, not just the read) -- + // that is precisely the hang `open_nonblocking`'s `O_NONBLOCK` is + // there to prevent. This call must return promptly with + // `NotRegularFile` rather than hang the test. + let err = read_bounded(&fifo_path, 1024).expect_err("FIFO must be rejected"); + match err { + Error::NotRegularFile { file_type } => { + assert!(!file_type.is_file()); + } + other => panic!("expected NotRegularFile, got {other:?}"), + } + } +} diff --git a/crates/clx-core/src/credentials/backend.rs b/crates/clx-core/src/credentials/backend.rs index eb7a5ad..2854cd3 100644 --- a/crates/clx-core/src/credentials/backend.rs +++ b/crates/clx-core/src/credentials/backend.rs @@ -29,8 +29,17 @@ use fs4::FileExt; use fs4::TryLockError; use secrecy::ExposeSecret; +use crate::bounded_read::{self, Error as BoundedReadError}; + use super::{CredentialError, Result}; +/// Hard ceiling on `credentials.age`'s size. An age-encrypted map of a +/// handful of provider API keys is kilobytes at most; 16 MiB is absurd +/// headroom that only ever rejects something that is obviously wrong (a +/// directory, device, or unrelated huge file dropped at the path), never a +/// legitimate credential store. +const MAX_CREDENTIALS_FILE_BYTES: u64 = 16 * 1024 * 1024; + /// Storage/retrieval of already-scoped credential keys. /// /// Implementations MUST NOT prompt the user under any default code path. @@ -271,12 +280,43 @@ impl AgeFileBackend { /// (then `set`/`migrate` repopulates from scratch). /// * File present, non-zero garbage -> already errors at the age decoder; /// that behavior is preserved. + /// * File present but NOT a regular file (directory, FIFO, device, ...), + /// or larger than [`MAX_CREDENTIALS_FILE_BYTES`] -> a HARD error, never + /// treated as an empty store. Silently falling back to "empty" here + /// would let a later `set`/`delete` re-encrypt an empty map and + /// overwrite whatever is actually at that path, permanently destroying + /// real credentials that might exist behind a symlink/mount swap. fn load_map(&self, identity: &age::x25519::Identity) -> Result> { - let encrypted = match fs::read(&self.cred_file) { - Ok(b) => b, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(BTreeMap::new()), - Err(e) => return Err(Self::map_err("read credentials.age", e)), - }; + let encrypted = + match bounded_read::read_bounded(&self.cred_file, MAX_CREDENTIALS_FILE_BYTES) { + Ok(b) => b, + Err(BoundedReadError::NotFound) => return Ok(BTreeMap::new()), + Err(BoundedReadError::NotRegularFile { file_type }) => { + return Err(Self::map_err( + "read credentials.age", + format!( + "{} is not a regular file ({file_type:?}); refusing to read it \ + (this is a hard error, not an empty store, so a later write can \ + never overwrite whatever is actually there)", + self.cred_file.display() + ), + )); + } + Err(BoundedReadError::TooLarge { len, cap }) => { + return Err(Self::map_err( + "read credentials.age", + format!( + "{} is {len} bytes, exceeding the {cap} byte cap; refusing to read \ + it (hard error, not an empty store, for the same reason as a \ + non-regular file above)", + self.cred_file.display() + ), + )); + } + Err(BoundedReadError::Io(e)) => { + return Err(Self::map_err("read credentials.age", e)); + } + }; if encrypted.is_empty() { return Err(self.zero_byte_corruption_error()); } diff --git a/crates/clx-core/src/lib.rs b/crates/clx-core/src/lib.rs index 782eeb4..ba8c312 100644 --- a/crates/clx-core/src/lib.rs +++ b/crates/clx-core/src/lib.rs @@ -7,6 +7,7 @@ //! - Shared types and error definitions //! - Configuration management +pub mod bounded_read; pub mod config; pub mod credentials; pub mod embeddings; diff --git a/crates/clx-core/tests/credentials_backend_behavior.rs b/crates/clx-core/tests/credentials_backend_behavior.rs index 8d80f15..3f17d29 100644 --- a/crates/clx-core/tests/credentials_backend_behavior.rs +++ b/crates/clx-core/tests/credentials_backend_behavior.rs @@ -254,6 +254,9 @@ fn persistent_zero_byte_read_still_errors_after_retries() { fn absent_file_is_legitimate_empty_store_and_writable() { // Fresh install (no credentials.age): empty store, zero prompts, and a // subsequent set works (distinct from the zero-byte corruption case). + // This is also the highest-value regression pin for the new bounded-read + // guard's `NotFound` mapping: it must still yield an empty map, not an + // error. let tmp = tempfile::tempdir().unwrap(); let b = file_backend(tmp.path()); assert_eq!(b.list_keys().unwrap(), Vec::::new()); @@ -262,6 +265,67 @@ fn absent_file_is_legitimate_empty_store_and_writable() { assert_eq!(b.get("clx:global:fresh").unwrap().as_deref(), Some("ok")); } +// ========================================================================= +// 5b. Bounded-read guards on credentials.age: a directory or an oversized +// file at the path is a HARD error, never mistaken for an empty store +// (the bounded_read module's NotRegularFile / TooLarge -> load_map). +// ========================================================================= + +#[test] +fn directory_at_credentials_path_is_a_hard_error_not_an_empty_store() { + // If a directory ever ends up at `credentials.age` (misconfiguration, a + // botched migration, a malicious symlink swap), `get` must refuse to + // read it rather than silently treating it as "no credentials yet" -- + // the latter would let a subsequent `set` overwrite it and destroy + // whatever was actually there. + let tmp = tempfile::tempdir().unwrap(); + let b = file_backend(tmp.path()); + // Trigger key-file + dir creation, then replace the data file with a + // directory. + b.set("clx:global:seed", "x").unwrap(); + let cred = tmp.path().join("credentials.age"); + std::fs::remove_file(&cred).unwrap(); + std::fs::create_dir(&cred).unwrap(); + + let err = b + .get("clx:global:seed") + .expect_err("a directory at credentials.age must be a hard error"); + let msg = format!("{err}"); + assert!( + msg.contains("not a regular file"), + "expected a not-a-regular-file error, got: {msg}" + ); + + // Must NOT have been treated as empty: a `set` must not proceed to + // overwrite the directory in a way that fabricates an empty store. + let set_err = b + .set("clx:global:new", "y") + .expect_err("set must not overwrite a directory as if it were an empty store"); + assert!(format!("{set_err}").contains("not a regular file")); +} + +#[test] +fn oversized_credentials_file_is_a_hard_error_not_an_empty_store() { + // A credentials.age far larger than any legitimate age-encrypted map + // must be rejected outright, never treated as an empty store. + let tmp = tempfile::tempdir().unwrap(); + let b = file_backend(tmp.path()); + b.set("clx:global:seed", "x").unwrap(); + let cred = tmp.path().join("credentials.age"); + // 16 MiB cap + 1 byte. + let oversized = vec![0u8; 16 * 1024 * 1024 + 1]; + std::fs::write(&cred, &oversized).unwrap(); + + let err = b + .get("clx:global:seed") + .expect_err("an oversized credentials.age must be a hard error"); + let msg = format!("{err}"); + assert!( + msg.contains("exceeding"), + "expected a too-large error, got: {msg}" + ); +} + #[test] fn nonzero_garbage_blob_errors_with_context() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/clx-hook/src/context.rs b/crates/clx-hook/src/context.rs index fea8bcb..cd11f93 100644 --- a/crates/clx-hook/src/context.rs +++ b/crates/clx-hook/src/context.rs @@ -2,9 +2,18 @@ use crate::embedding::truncate_to_char_boundary; use crate::host::Host; +use clx_core::bounded_read::{self, Error as BoundedReadError}; +use clx_core::redaction::redact_secrets; use clx_core::storage::Storage; use clx_core::types::SessionId; -use tracing::debug; +use tracing::{debug, warn}; + +/// Hard ceiling on a project/global instructions file (`CLAUDE.md`, +/// `AGENTS.md`, ...). These are hand-written prose docs; 4 MiB is generous +/// headroom while still bounding a FIFO/device/oversized-file footgun at a +/// path this process does not control (project repos are third-party +/// content from CLX's point of view). +const MAX_RULES_FILE_BYTES: u64 = 4 * 1024 * 1024; /// Load previous session summary for context pub(crate) fn load_previous_session_summary( @@ -40,32 +49,71 @@ pub(crate) fn load_project_rules(cwd: &str, host: &dyn Host) -> Option { let label = host.instructions_file_label(); // 1. Check project-specific instructions file (e.g. cwd/CLAUDE.md). + // + // Best-effort: any failure to read skips this source exactly as the + // former `if let Ok(content) = ...` did, EXCEPT `NotRegularFile` / + // `TooLarge`, which now warn (redacted path) instead of failing + // silently -- a project repo is not fully trusted content, so a FIFO, + // device, or oversized file dropped at `CLAUDE.md`/`AGENTS.md` should be + // visible somewhere, not swallowed. `.exists()` is redundant with + // `NotFound` and dropped. let project_instructions = Path::new(cwd).join(label); - if project_instructions.exists() - && let Ok(content) = std::fs::read_to_string(&project_instructions) - { - let rules = extract_critical_rules(&content); - if !rules.is_empty() { - all_rules.push(format!("## Project Rules ({cwd})\n{rules}")); + match bounded_read::read_bounded_to_string(&project_instructions, MAX_RULES_FILE_BYTES) { + Ok(content) => { + let rules = extract_critical_rules(&content); + if !rules.is_empty() { + all_rules.push(format!("## Project Rules ({cwd})\n{rules}")); + } + } + Err(BoundedReadError::NotFound) => {} + Err(BoundedReadError::NotRegularFile { file_type }) => { + warn!( + "project rules file '{}' is not a regular file ({file_type:?}); skipping", + redact_secrets(&project_instructions.display().to_string()) + ); } + Err(BoundedReadError::TooLarge { len, cap }) => { + warn!( + "project rules file '{}' is {len} bytes (> {cap} byte cap); skipping", + redact_secrets(&project_instructions.display().to_string()) + ); + } + Err(BoundedReadError::Io(_)) => {} } // 2. Check the host's global instructions file (e.g. ~/.claude/CLAUDE.md). // Cursor has no global file (`global_instructions_path` -> None). if let Some(home) = dirs::home_dir() && let Some(global_path) = host.global_instructions_path(&home) - && global_path.exists() - && let Ok(content) = std::fs::read_to_string(&global_path) { - let rules = extract_critical_rules(&content); - if !rules.is_empty() { - // Render the global path with a `~` prefix when it lives under - // $HOME, preserving the historical "~/.claude/CLAUDE.md" header. - let display = global_path.strip_prefix(&home).map_or_else( - |_| global_path.display().to_string(), - |rel| format!("~/{}", rel.display()), - ); - all_rules.push(format!("## Global Rules ({display})\n{rules}")); + match bounded_read::read_bounded_to_string(&global_path, MAX_RULES_FILE_BYTES) { + Ok(content) => { + let rules = extract_critical_rules(&content); + if !rules.is_empty() { + // Render the global path with a `~` prefix when it lives + // under $HOME, preserving the historical + // "~/.claude/CLAUDE.md" header. + let display = global_path.strip_prefix(&home).map_or_else( + |_| global_path.display().to_string(), + |rel| format!("~/{}", rel.display()), + ); + all_rules.push(format!("## Global Rules ({display})\n{rules}")); + } + } + Err(BoundedReadError::NotFound) => {} + Err(BoundedReadError::NotRegularFile { file_type }) => { + warn!( + "global rules file '{}' is not a regular file ({file_type:?}); skipping", + redact_secrets(&global_path.display().to_string()) + ); + } + Err(BoundedReadError::TooLarge { len, cap }) => { + warn!( + "global rules file '{}' is {len} bytes (> {cap} byte cap); skipping", + redact_secrets(&global_path.display().to_string()) + ); + } + Err(BoundedReadError::Io(_)) => {} } } diff --git a/crates/clx-hook/src/tests.rs b/crates/clx-hook/src/tests.rs index 29a1371..aed4e84 100644 --- a/crates/clx-hook/src/tests.rs +++ b/crates/clx-hook/src/tests.rs @@ -668,6 +668,51 @@ fn test_load_project_rules_returns_content_when_claude_md_exists() { ); } +#[test] +fn test_load_project_rules_skips_directory_at_claude_md_path() { + // A directory (not a file) sitting at cwd/CLAUDE.md must be rejected by + // the bounded-read guard (`NotRegularFile`) and silently skipped as a + // project-rules source -- never panics, never treats directory entries + // as rule text. + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("CLAUDE.md")).unwrap(); + let cwd = dir.path().to_str().unwrap(); + + let result = load_project_rules(cwd, &crate::host::ClaudeHost); + + // Whatever the overall outcome (None, or Some sourced only from an + // unrelated global CLAUDE.md on this machine -- same ambiguity the test + // above already accepts), the PROJECT section specifically must be + // absent: a directory must never be read as project rules. + if let Some(rules) = result { + assert!( + !rules.contains(&format!("Project Rules ({cwd})")), + "a directory at CLAUDE.md must not be read as project rules, got: {rules}" + ); + } +} + +#[test] +fn test_load_project_rules_skips_oversized_claude_md() { + // A CLAUDE.md larger than the 4 MiB cap must be rejected (`TooLarge`) + // and skipped as a source, never partially read into the rules text. + let dir = tempfile::tempdir().unwrap(); + let claude_md = dir.path().join("CLAUDE.md"); + let mut content = String::from("# Rules [STRICT]\nOversized should be skipped.\n"); + content.push_str(&"x".repeat(4 * 1024 * 1024 + 1)); + std::fs::write(&claude_md, content).unwrap(); + let cwd = dir.path().to_str().unwrap(); + + let result = load_project_rules(cwd, &crate::host::ClaudeHost); + + if let Some(rules) = result { + assert!( + !rules.contains("Oversized should be skipped"), + "an oversized CLAUDE.md must be capped/skipped, not read in full, got: {rules}" + ); + } +} + #[test] fn test_extract_critical_rules_extracts_only_critical_lines() { // Arrange diff --git a/crates/clx/src/commands/trust.rs b/crates/clx/src/commands/trust.rs index a3e152f..62e80ec 100644 --- a/crates/clx/src/commands/trust.rs +++ b/crates/clx/src/commands/trust.rs @@ -21,12 +21,19 @@ use chrono::{DateTime, Duration, Utc}; use clap::Subcommand; use colored::Colorize; +use clx_core::bounded_read::{self, Error as BoundedReadError}; use clx_core::config::Config; use clx_core::config::trust::{TrustList, compute_file_hash, trusted_configs_path}; use clx_core::types::TrustToken; use crate::Cli; +/// Hard ceiling on a project `.clx/config.yaml` being trusted. These are +/// small hand-written YAML files; 4 MiB is generous headroom while still +/// bounding a FIFO/device/oversized-file footgun at a path the CLI user +/// supplied on the command line. +const MAX_CONFIG_FILE_BYTES: u64 = 4 * 1024 * 1024; + /// Trust mode subcommands. #[derive(Debug, Clone, Subcommand)] pub enum TrustAction { @@ -353,8 +360,31 @@ pub fn cmd_config_trust(cli: &Cli, action: ConfigTrustAction) -> Result<()> { fn handle_config_trust_add(cli: &Cli, path: PathBuf, yes: bool) -> Result<()> { let canonical = std::fs::canonicalize(&path) .with_context(|| format!("config file not found: {}", path.display()))?; - let content = std::fs::read_to_string(&canonical) - .with_context(|| format!("failed to read {}", canonical.display()))?; + let content = match bounded_read::read_bounded_to_string(&canonical, MAX_CONFIG_FILE_BYTES) { + Ok(content) => content, + // Extremely unlikely after a canonicalize that just succeeded (a + // TOCTOU race would have to remove the file in between), but handled + // for completeness rather than assumed away. + Err(BoundedReadError::NotFound) => { + bail!("config file not found: {}", canonical.display()); + } + Err(BoundedReadError::NotRegularFile { file_type }) => { + bail!( + "{} is not a regular file ({file_type:?}); refusing to trust it", + canonical.display() + ); + } + Err(BoundedReadError::TooLarge { len, cap }) => { + bail!( + "{} is {len} bytes, exceeding the {cap} byte cap for a project config; \ + refusing to read it", + canonical.display() + ); + } + Err(BoundedReadError::Io(e)) => { + return Err(e).with_context(|| format!("failed to read {}", canonical.display())); + } + }; let hash = compute_file_hash(&content); let mut tl = TrustList::load()?; diff --git a/crates/clx/tests/cli_trust_deep_e2e.rs b/crates/clx/tests/cli_trust_deep_e2e.rs index 09e17b2..6d800ce 100644 --- a/crates/clx/tests/cli_trust_deep_e2e.rs +++ b/crates/clx/tests/cli_trust_deep_e2e.rs @@ -534,3 +534,50 @@ fn config_trust_remove_ambiguous_prefix_is_clean_error() { .failure() .stderr(predicate::str::contains("ambiguous")); } + +// =========================================================================== +// config-trust add: bounded-read guards (not-a-regular-file, oversized) -- +// `handle_config_trust_add` must hard-error rather than trust a directory +// or an oversized file, and the error must name the actual reason. +// =========================================================================== + +#[test] +fn config_trust_add_rejects_directory_at_config_path() { + // `canonicalize` succeeds on a directory just as readily as a file, so + // the bounded-read guard (not the canonicalize step) must be what + // catches this: a directory dropped at the config path must never be + // "trusted" as if it were readable YAML. + let t = tmp(); + let proj = home_path(&t, "work/.clx"); + std::fs::create_dir_all(&proj).unwrap(); + let dir_as_config = proj.join("config.yaml"); + std::fs::create_dir(&dir_as_config).unwrap(); + + clx(&t) + .args(["config-trust", "add"]) + .arg(&dir_as_config) + .arg("-y") + .assert() + .failure() + .stderr(predicate::str::contains("not a regular file")); +} + +#[test] +fn config_trust_add_rejects_oversized_config() { + // A config file over the 4 MiB cap must be a hard, actionable error -- + // never truncated-and-hashed, never silently accepted. + let t = tmp(); + let proj = home_path(&t, "work/.clx"); + std::fs::create_dir_all(&proj).unwrap(); + let cfg = proj.join("config.yaml"); + let oversized = vec![b'a'; 4 * 1024 * 1024 + 1]; + std::fs::write(&cfg, &oversized).unwrap(); + + clx(&t) + .args(["config-trust", "add"]) + .arg(&cfg) + .arg("-y") + .assert() + .failure() + .stderr(predicate::str::contains("exceeding")); +}