fix(kei-store): path-traversal guard (F2 RELEASE BLOCKER) + S3 stub gate (F7) + GitHub RULE 0.1 guard (F8)

F2: filesystem.rs + s3.rs 'fn full' now Result<PathBuf>, rejects absolute + ParentDir components. 7 new unit tests.
F7: factory.rs rejects 'backend=s3' without KEI_STORE_ALLOW_S3_STUB=1; backend_name() = 's3-local-stub'.
F8: github.rs push() blocks github.com unless KEI_STORE_ALLOW_GITHUB_PUSH=1 (RULE 0.1).
This commit is contained in:
Parfii-bot 2026-04-22 13:36:17 +08:00
parent fbd8adf9cf
commit ef95bf2a7c
5 changed files with 260 additions and 31 deletions

View file

@ -1,10 +1,15 @@
//! Factory — construct a `Box<dyn MemoryStore>` 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<Box<dyn MemoryStore>> {
@ -18,17 +23,30 @@ pub fn build_store(cfg: &Config) -> Result<Box<dyn MemoryStore>> {
"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<Box<dyn MemoryStore>> {
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())?))
}

View file

@ -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<PathBuf> {
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<PathBuf> {
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<Vec<u8>> {
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<Vec<String>> {
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}");
}
}

View file

@ -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();
}
}

View file

@ -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-<hash>.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<PathBuf> {
safe_join(&self.cache, rel)
}
}
impl MemoryStore for S3Store {
fn read(&self, path: &str) -> Result<Vec<u8>> {
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<Vec<String>> {
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/<branch>/
fs::create_dir_all(self.cache.join(name))?;
// Logical snapshot namespace — stored under cache/<branch>/.
// 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");
}
}

View file

@ -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();