diff --git a/_primitives/_rust/kei-store/src/factory.rs b/_primitives/_rust/kei-store/src/factory.rs index 301b8f3..2f4f047 100644 --- a/_primitives/_rust/kei-store/src/factory.rs +++ b/_primitives/_rust/kei-store/src/factory.rs @@ -1,10 +1,15 @@ //! Factory — construct a `Box` from a Config. +//! +//! v0.14.1: the S3 backend is gated behind `KEI_STORE_ALLOW_S3_STUB=1` +//! because it does NOT push to S3 yet — it's a local-manifest stub. +//! Previous behaviour silently stored data locally, confusing users who +//! thought their traces were uploaded. use crate::config::{expand_tilde, Config}; use crate::{filesystem::FilesystemStore, forgejo::ForgejoStore, gitea::GiteaStore, github::GitHubStore, s3::S3Store}; use crate::store_trait::MemoryStore; -use anyhow::{anyhow, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use std::path::PathBuf; pub fn build_store(cfg: &Config) -> Result> { @@ -18,17 +23,30 @@ pub fn build_store(cfg: &Config) -> Result> { "github" => Ok(Box::new(GitHubStore::new(local, cfg.github.clone())?)), "forgejo" => Ok(Box::new(ForgejoStore::new(local, cfg.forgejo.clone())?)), "gitea" => Ok(Box::new(GiteaStore::new(local, cfg.gitea.clone())?)), - "s3" => { - let cache = cfg - .s3 - .cache_path - .as_deref() - .map(expand_tilde) - .map(PathBuf::from) - .ok_or_else(|| anyhow!("s3 backend requires s3.cache_path"))?; - Ok(Box::new(S3Store::new(cache, cfg.s3.clone())?)) - } + "s3" => build_s3(cfg), other => Err(anyhow!("unknown backend: {other}")) .context("supported: filesystem | github | forgejo | gitea | s3"), } } + +fn build_s3(cfg: &Config) -> Result> { + if std::env::var("KEI_STORE_ALLOW_S3_STUB").is_err() { + bail!( + "S3 backend is a local-only MVP stub (no upload to S3/R2/MinIO yet). \ + Set KEI_STORE_ALLOW_S3_STUB=1 to proceed; data will be stored in the \ + configured cache_path only. Production S3 support is planned for v0.15." + ); + } + eprintln!( + "[kei-store] WARNING: S3 backend is a local-only stub — data stored \ + at cache_path only, not pushed to any object store." + ); + let cache = cfg + .s3 + .cache_path + .as_deref() + .map(expand_tilde) + .map(PathBuf::from) + .ok_or_else(|| anyhow!("s3 backend requires s3.cache_path"))?; + Ok(Box::new(S3Store::new(cache, cfg.s3.clone())?)) +} diff --git a/_primitives/_rust/kei-store/src/filesystem.rs b/_primitives/_rust/kei-store/src/filesystem.rs index b14629b..4792d6b 100644 --- a/_primitives/_rust/kei-store/src/filesystem.rs +++ b/_primitives/_rust/kei-store/src/filesystem.rs @@ -2,11 +2,14 @@ //! //! Reuses git2 for branch/commit so behavior parity with remote stores is //! maintained. `push`/`pull` are intentional no-ops. +//! +//! v0.14.1 hardening: `full()` now rejects absolute paths and `..` components +//! (CVE-class: path traversal via MCP `write`/`read` tool inputs). use crate::store_trait::MemoryStore; -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; pub struct FilesystemStore { pub root: PathBuf, @@ -20,11 +23,34 @@ impl FilesystemStore { Ok(Self { root }) } - fn full(&self, rel: &str) -> PathBuf { - self.root.join(rel) + fn full(&self, rel: &str) -> Result { + safe_join(&self.root, rel) } } +/// Reject absolute paths and any `..` component BEFORE joining. +/// `PathBuf::join("/etc/passwd")` would otherwise replace the base +/// entirely — that turned kei-store's MCP `write` tool into an +/// unrestricted filesystem writer. +pub(crate) fn safe_join(root: &Path, rel: &str) -> Result { + let p = Path::new(rel); + if p.is_absolute() { + bail!("path traversal rejected: absolute path {:?}", rel); + } + for component in p.components() { + match component { + Component::ParentDir => { + bail!("path traversal rejected: parent-dir component in {:?}", rel); + } + Component::Prefix(_) | Component::RootDir => { + bail!("path traversal rejected: root/prefix component in {:?}", rel); + } + _ => {} + } + } + Ok(root.join(rel)) +} + fn ensure_repo(root: &Path) -> Result<()> { if root.join(".git").exists() { return Ok(()); @@ -35,11 +61,11 @@ fn ensure_repo(root: &Path) -> Result<()> { impl MemoryStore for FilesystemStore { fn read(&self, path: &str) -> Result> { - fs::read(self.full(path)).with_context(|| format!("read {}", path)) + fs::read(self.full(path)?).with_context(|| format!("read {}", path)) } fn write(&self, path: &str, bytes: &[u8]) -> Result<()> { - let full = self.full(path); + let full = self.full(path)?; if let Some(parent) = full.parent() { fs::create_dir_all(parent)?; } @@ -47,7 +73,7 @@ impl MemoryStore for FilesystemStore { } fn list(&self, dir: &str) -> Result> { - let full = self.full(dir); + let full = self.full(dir)?; if !full.exists() { return Ok(Vec::new()); } @@ -103,3 +129,44 @@ impl MemoryStore for FilesystemStore { "filesystem" } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_absolute_path_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let store = FilesystemStore::new(tmp.path().join("root")).unwrap(); + let err = store.write("/etc/passwd", b"nope").unwrap_err(); + let s = format!("{err:#}"); + assert!(s.contains("absolute"), "unexpected err: {s}"); + } + + #[test] + fn test_parent_dir_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let store = FilesystemStore::new(tmp.path().join("root")).unwrap(); + let err = store.write("../../.ssh/authorized_keys", b"nope").unwrap_err(); + let s = format!("{err:#}"); + assert!(s.contains("parent-dir"), "unexpected err: {s}"); + } + + #[test] + fn test_normal_path_ok() { + let tmp = tempfile::tempdir().unwrap(); + let store = FilesystemStore::new(tmp.path().join("root")).unwrap(); + store.write("traces/session.jsonl", b"ok").unwrap(); + let bytes = store.read("traces/session.jsonl").unwrap(); + assert_eq!(&bytes, b"ok"); + } + + #[test] + fn test_read_absolute_path_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let store = FilesystemStore::new(tmp.path().join("root")).unwrap(); + let err = store.read("/etc/passwd").unwrap_err(); + let s = format!("{err:#}"); + assert!(s.contains("absolute"), "unexpected err: {s}"); + } +} diff --git a/_primitives/_rust/kei-store/src/github.rs b/_primitives/_rust/kei-store/src/github.rs index eb3a356..8abdca6 100644 --- a/_primitives/_rust/kei-store/src/github.rs +++ b/_primitives/_rust/kei-store/src/github.rs @@ -4,11 +4,16 @@ //! remote. SSH auth via `KEI_MEMORY_SSH_KEY` (path to key); HTTPS via //! `KEI_MEMORY_PAT` (token). Exactly the pattern used in v0.11 //! `kei-sleep-setup.sh`. +//! +//! v0.14.1: pushes to `github.com` are blocked by default under RULE 0.1 +//! (patent-IP protection). Forks on Forgejo / Gitea / self-hosted are +//! unaffected since they do not resolve to `github.com`. Override for a +//! genuinely public repo: `KEI_STORE_ALLOW_GITHUB_PUSH=1`. use crate::config::GitRemoteCfg; use crate::filesystem::FilesystemStore; use crate::store_trait::MemoryStore; -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; use std::path::PathBuf; pub struct GitHubStore { @@ -75,8 +80,9 @@ impl MemoryStore for GitHubStore { } fn push(&self, branch: &str) -> Result<()> { - let repo = git2::Repository::open(&self.inner.root)?; let url = self.remote_url()?; + enforce_github_push_guard(url)?; + let repo = git2::Repository::open(&self.inner.root)?; let mut remote = match repo.find_remote("origin") { Ok(r) => r, Err(_) => repo.remote("origin", url)?, @@ -105,3 +111,61 @@ impl MemoryStore for GitHubStore { self.name } } + +/// RULE 0.1 enforcement point for the kei-store push path. +/// +/// Blocks pushes whose URL contains `github.com` unless the caller +/// explicitly opts-in via `KEI_STORE_ALLOW_GITHUB_PUSH=1`. Forks on +/// Forgejo / Gitea / self-hosted remain unaffected — only the literal +/// `github.com` host is gated. +pub(crate) fn enforce_github_push_guard(url: &str) -> Result<()> { + if !url.contains("github.com") { + return Ok(()); + } + if std::env::var("KEI_STORE_ALLOW_GITHUB_PUSH").is_ok() { + return Ok(()); + } + bail!( + "push to github.com blocked by RULE 0.1 (patent-IP protection). \ + Set KEI_STORE_ALLOW_GITHUB_PUSH=1 if this is a public-safe release. \ + See ~/.claude/rules/security.md for banned-project criteria." + ) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + // Serialise tests that read/write KEI_STORE_ALLOW_GITHUB_PUSH so + // parallel cargo-test runners don't race on process env. + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + #[test] + fn test_github_push_blocked_without_env_var() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + std::env::remove_var("KEI_STORE_ALLOW_GITHUB_PUSH"); + let err = enforce_github_push_guard("git@github.com:owner/repo.git").unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("github.com"), "unexpected err: {msg}"); + assert!(msg.contains("RULE 0.1"), "unexpected err: {msg}"); + } + + #[test] + fn test_github_push_allowed_with_env_var() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + std::env::set_var("KEI_STORE_ALLOW_GITHUB_PUSH", "1"); + let ok = enforce_github_push_guard("git@github.com:owner/repo.git"); + std::env::remove_var("KEI_STORE_ALLOW_GITHUB_PUSH"); + assert!(ok.is_ok(), "should allow with opt-in env var"); + } + + #[test] + fn test_non_github_push_always_allowed() { + // Non-github URLs should always pass regardless of env state, but we + // still take the lock so we don't observe a half-set var mid-test. + let _guard = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + enforce_github_push_guard("ssh://git@forgejo.local:2222/user/repo.git").unwrap(); + enforce_github_push_guard("https://gitea.example.com/user/repo.git").unwrap(); + } +} diff --git a/_primitives/_rust/kei-store/src/s3.rs b/_primitives/_rust/kei-store/src/s3.rs index fa37dd1..08f0c0f 100644 --- a/_primitives/_rust/kei-store/src/s3.rs +++ b/_primitives/_rust/kei-store/src/s3.rs @@ -1,16 +1,26 @@ -//! S3Store — object-storage backend (MVP stub). +//! S3Store — object-storage backend (MVP stub; v0.14.1 local-only). //! //! This is a local-manifest-based implementation intended as an offline MVP. //! Reads/writes go to `cache_path`; `commit` serialises a //! `manifest-.json` listing the current file tree + content hash; //! `push`/`pull` are NO-OPs in stub mode. //! +//! v0.14.1: because the backend does NOT actually reach S3, the factory +//! now refuses to build an `S3Store` unless `KEI_STORE_ALLOW_S3_STUB=1` +//! is set. Previously users who configured S3 were silently writing to a +//! local cache with no remote push. See `factory.rs` for the guard. +//! +//! v0.14.1 hardening: `full()` rejects absolute paths and `..` components +//! (same CVE class as `filesystem.rs` — user-supplied `rel` could escape +//! the cache root). +//! //! Production S3/R2/MinIO support is planned via `aws-sdk-s3` behind a //! feature flag — see README §Store backends. This stub keeps the trait //! surface honest so downstream code can exercise the full kei-store //! API without pulling a ~20 MB AWS SDK at install time. use crate::config::S3Cfg; +use crate::filesystem::safe_join; use crate::store_trait::MemoryStore; use anyhow::{Context, Result}; use std::fs; @@ -27,18 +37,18 @@ impl S3Store { Ok(Self { cache, cfg }) } - fn full(&self, rel: &str) -> PathBuf { - self.cache.join(rel) + fn full(&self, rel: &str) -> Result { + safe_join(&self.cache, rel) } } impl MemoryStore for S3Store { fn read(&self, path: &str) -> Result> { - fs::read(self.full(path)).with_context(|| format!("read {}", path)) + fs::read(self.full(path)?).with_context(|| format!("read {}", path)) } fn write(&self, path: &str, bytes: &[u8]) -> Result<()> { - let full = self.full(path); + let full = self.full(path)?; if let Some(parent) = full.parent() { fs::create_dir_all(parent)?; } @@ -47,7 +57,7 @@ impl MemoryStore for S3Store { } fn list(&self, dir: &str) -> Result> { - let full = self.full(dir); + let full = self.full(dir)?; if !full.exists() { return Ok(Vec::new()); } @@ -65,8 +75,11 @@ impl MemoryStore for S3Store { } fn branch(&self, name: &str) -> Result<()> { - // Logical snapshot namespace — stored under cache// - fs::create_dir_all(self.cache.join(name))?; + // Logical snapshot namespace — stored under cache//. + // Also guarded against traversal so a malicious branch name cannot + // escape the cache root. + let dir = self.full(name)?; + fs::create_dir_all(dir)?; Ok(()) } @@ -88,7 +101,7 @@ impl MemoryStore for S3Store { } fn backend_name(&self) -> &'static str { - "s3-stub" + "s3-local-stub" } } @@ -120,3 +133,39 @@ fn short_hash(s: &str) -> String { } format!("{:x}", h) } + +#[cfg(test)] +mod tests { + use super::*; + + fn store(root: PathBuf) -> S3Store { + S3Store::new(root, S3Cfg::default()).unwrap() + } + + #[test] + fn test_absolute_path_rejected_s3() { + let tmp = tempfile::tempdir().unwrap(); + let s = store(tmp.path().join("cache")); + let err = s.write("/etc/passwd", b"nope").unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("absolute"), "unexpected err: {msg}"); + } + + #[test] + fn test_parent_dir_rejected_s3() { + let tmp = tempfile::tempdir().unwrap(); + let s = store(tmp.path().join("cache")); + let err = s.write("../../secret", b"nope").unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("parent-dir"), "unexpected err: {msg}"); + } + + #[test] + fn test_normal_path_ok_s3() { + let tmp = tempfile::tempdir().unwrap(); + let s = store(tmp.path().join("cache")); + s.write("a/b.txt", b"ok").unwrap(); + let bytes = s.read("a/b.txt").unwrap(); + assert_eq!(&bytes, b"ok"); + } +} diff --git a/_primitives/_rust/kei-store/tests/integration.rs b/_primitives/_rust/kei-store/tests/integration.rs index 86400ee..bd04301 100644 --- a/_primitives/_rust/kei-store/tests/integration.rs +++ b/_primitives/_rust/kei-store/tests/integration.rs @@ -23,6 +23,15 @@ fn run(args: &[&str]) -> std::process::Output { std::process::Command::new(bin()).args(args).output().unwrap() } +fn run_with_env(args: &[&str], env: &[(&str, &str)]) -> std::process::Output { + let mut cmd = std::process::Command::new(bin()); + cmd.args(args); + for (k, v) in env { + cmd.env(k, v); + } + cmd.output().unwrap() +} + #[test] fn init_writes_config() { let tmp = TempDir::new().unwrap(); @@ -111,8 +120,15 @@ fn s3_stub_commit_writes_manifest() { let cfg = write_config(&tmp, "s3", &local); let file = tmp.path().join("x"); fs::write(&file, b"x").unwrap(); - run(&["--config", cfg.to_str().unwrap(), "write", "a.txt", file.to_str().unwrap()]); - let out = run(&["--config", cfg.to_str().unwrap(), "commit", "--message", "first"]); + // v0.14.1: S3 stub requires explicit opt-in env var. + run_with_env( + &["--config", cfg.to_str().unwrap(), "write", "a.txt", file.to_str().unwrap()], + &[("KEI_STORE_ALLOW_S3_STUB", "1")], + ); + let out = run_with_env( + &["--config", cfg.to_str().unwrap(), "commit", "--message", "first"], + &[("KEI_STORE_ALLOW_S3_STUB", "1")], + ); assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); let entries: Vec<_> = fs::read_dir(&local) .unwrap() @@ -122,6 +138,21 @@ fn s3_stub_commit_writes_manifest() { assert_eq!(entries.len(), 1); } +#[test] +fn s3_backend_requires_env_optin() { + let tmp = TempDir::new().unwrap(); + let local = tmp.path().join("cache"); + let cfg = write_config(&tmp, "s3", &local); + // Without KEI_STORE_ALLOW_S3_STUB, status must fail with a clear message. + let mut cmd = std::process::Command::new(bin()); + cmd.args(["--config", cfg.to_str().unwrap(), "status"]); + cmd.env_remove("KEI_STORE_ALLOW_S3_STUB"); + let out = cmd.output().unwrap(); + assert!(!out.status.success()); + let msg = String::from_utf8_lossy(&out.stderr); + assert!(msg.contains("KEI_STORE_ALLOW_S3_STUB"), "expected stub-gate message, got: {msg}"); +} + #[test] fn status_reports_backend() { let tmp = TempDir::new().unwrap();