Skip to content

Commit ccac182

Browse files
bryaneganclaude
andcommitted
Harden state_key and package mount error handling
Sharp edge fixes found during review of the three prior changes: - state_key: reject keys containing '..', '/', or '\' (path traversal) - state_key: create_dir_all now returns error instead of .ok() (was silently failing when project dir was unwritable) - needs_packages: create_dir_all for container mount point now returns actionable error instead of .ok() - sandbox-init Phase 2b: check bm.host.is_dir() not bm.container (container path may not exist yet on host) New proptest: state_key_rejects_traversal (arbitrary strings containing .. / \ are always rejected). Updated state_key_produces_valid_sandbox to use a real writable path. 14 proptests + 19 e2e tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d118d8e commit ccac182

3 files changed

Lines changed: 39 additions & 4 deletions

File tree

crates/gleisner-container/src/task.rs

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -319,16 +319,33 @@ impl TaskSandbox {
319319
// Ensure the container mount point exists on the host so
320320
// sandbox-init doesn't skip it as "nonexistent path".
321321
if pkg.host_path != pkg.container_path {
322-
std::fs::create_dir_all(&pkg.container_path).ok();
322+
if let Err(e) = std::fs::create_dir_all(&pkg.container_path) {
323+
return Err(ContainerError::Config(format!(
324+
"cannot create package mount point {}: {e} \
325+
(the container_path must be writable on the host)",
326+
pkg.container_path.display()
327+
)));
328+
}
323329
}
324330
sb.mount_readonly(&pkg.host_path, &pkg.container_path);
325331
}
326332

327333
// ── State persistence ─────────────────────────────────
328334

329335
if let Some(ref key) = self.state_key {
336+
// Validate: reject path traversal in state keys
337+
if key.contains("..") || key.contains('/') || key.contains('\\') {
338+
return Err(ContainerError::Config(format!(
339+
"state_key '{key}' contains path separator or traversal"
340+
)));
341+
}
330342
let state_dir = self.project_dir.join(format!(".gleisner/state/{key}"));
331-
std::fs::create_dir_all(&state_dir).ok();
343+
std::fs::create_dir_all(&state_dir).map_err(|e| {
344+
ContainerError::Config(format!(
345+
"cannot create state dir {}: {e}",
346+
state_dir.display()
347+
))
348+
})?;
332349
sb.bind_rw(&state_dir);
333350
}
334351

crates/gleisner-container/tests/proptests.rs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,10 +250,28 @@ proptest! {
250250
fn state_key_produces_valid_sandbox(
251251
key in "[a-z][a-z0-9_-]{0,15}",
252252
) {
253-
let task = TaskSandbox::new("/workspace")
253+
// Use a real writable path (not /workspace which doesn't exist)
254+
let dir = std::path::PathBuf::from("/datar/workspace/claude_code_experiments/gleisner/target/proptest-state");
255+
std::fs::create_dir_all(&dir).ok();
256+
let task = TaskSandbox::new(&dir)
254257
.state_key(&key);
255258
let sb = task.build();
256259
prop_assert!(sb.is_ok(), "state_key '{key}' should produce valid sandbox");
260+
// Clean up state dirs
261+
std::fs::remove_dir_all(dir.join(".gleisner")).ok();
262+
}
263+
264+
// ── Property: state_key rejects traversal ──────────────────────
265+
266+
#[test]
267+
fn state_key_rejects_traversal(
268+
key in "\\PC{1,30}",
269+
) {
270+
let task = TaskSandbox::new("/workspace").state_key(&key);
271+
let result = task.build();
272+
if key.contains("..") || key.contains('/') || key.contains('\\') {
273+
prop_assert!(result.is_err(), "state_key '{key}' with traversal should be rejected");
274+
}
257275
}
258276

259277
// ── Property: minimal_toml parse never panics ─────────────────

crates/gleisner-sandbox-init/src/runtime.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ fn setup_filesystem(spec: &SandboxSpec) -> Result<(), String> {
388388
continue;
389389
}
390390
let target = new_root.join(bm.container.strip_prefix("/").unwrap_or(&bm.container));
391-
if bm.container.is_dir() || bm.host.is_dir() {
391+
if bm.host.is_dir() {
392392
fs::create_dir_all(&target).map_err(|e| format!("mkdir {}: {e}", target.display()))?;
393393
} else if let Some(parent) = target.parent() {
394394
fs::create_dir_all(parent)

0 commit comments

Comments
 (0)