Skip to content

fix(daemon): make the credential scan provably terminating #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acamarata merged 1 commit into from
Sep 16, 2026
Merged

fix(daemon): make the credential scan provably terminating #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 28 additions & 175 deletions apps/daemon/src/security/guard.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,33 +69,39 @@ pub fn strip_null_bytes(s: &str) -> String {
s.replace('\0', "")
}

/// A character of the base64 alphabet, as the credential scanner sees it.
fn is_base64_char(c: char) -> bool {
c.is_ascii_alphanumeric() || c == '+' || c == '/'
}

/// Runs of at least this many base64-alphabet characters are treated as
/// credential material rather than ordinary text.
const MIN_REDACTED_RUN: usize = 40;

/// Sanitize tool input before storing in the audit log.
///
/// Replaces any 40+ character base64-like strings (API keys, tokens) with
/// `[REDACTED]` to prevent credential leakage into audit storage.
/// Replaces any run of `MIN_REDACTED_RUN`+ base64-alphabet characters (API
/// keys, tokens) with `[REDACTED]` to prevent credential leakage into audit
/// storage. Everything else is copied through unchanged.
///
/// The scan groups the input into maximal runs of same-class characters rather
/// than walking a cursor by hand. That is deliberate: the previous version
/// advanced an index with `i += run` / `i += 1` inside a `while i < len` loop,
/// which left three separate ways for the loop to stop making progress — and
/// the mutation gate found all three, hanging the job for 2 hours instead of
/// reporting a result. Iterating over groups makes progress structural, so no
/// arithmetic slip can turn this into an infinite loop.
pub fn sanitize_tool_input(input: &str) -> String {
// Match long base64 or hex-like strings: [A-Za-z0-9+/]{40,}
// Use a simple state-machine approach to avoid the regex crate dependency.
let mut result = String::with_capacity(input.len());
let chars: Vec<char> = input.chars().collect();
let mut i = 0;
let mut result = String::with_capacity(input.len());

while i < chars.len() {
// Count how many base64-alphabet chars in a row start here
let mut run = 0;
let mut j = i;
while j < chars.len()
&& (chars[j].is_ascii_alphanumeric() || chars[j] == '+' || chars[j] == '/')
{
run += 1;
j += 1;
}
if run >= 40 {
for group in chars.chunk_by(|a, b| is_base64_char(*a) == is_base64_char(*b)) {
// `chunk_by` never yields an empty group, so `group[0]` is safe and
// decides the class of the whole run.
if group.len() >= MIN_REDACTED_RUN && is_base64_char(group[0]) {
result.push_str("[REDACTED]");
i += run;
} else {
result.push(chars[i]);
i += 1;
result.extend(group.iter());
}
}
result
Expand Down Expand Up @@ -219,159 +225,6 @@ pub fn validate_session_id(id: &str) -> Result<()> {
Ok(())
}

// Tests live in guard/tests.rs.
#[cfg(test)]
mod tests {
use super::*;
use std::path::Path;

#[test]
fn test_safe_path_normal() {
let base = Path::new("/home/user/repo");
let result = safe_path(base, Path::new("src/main.rs")).unwrap();
assert_eq!(result, PathBuf::from("/home/user/repo/src/main.rs"));
}

#[test]
fn test_safe_path_traversal_blocked() {
let base = Path::new("/home/user/repo");
let result = safe_path(base, Path::new("../../etc/passwd"));
assert!(result.is_err(), "path traversal should be blocked");
}

#[test]
fn test_safe_path_absolute_blocked() {
let base = Path::new("/home/user/repo");
let result = safe_path(base, Path::new("/etc/passwd"));
assert!(result.is_err(), "absolute paths should be blocked");
}

#[test]
fn test_normalize_path() {
let p = Path::new("/a/b/../c/./d");
assert_eq!(normalize_path(p), PathBuf::from("/a/c/d"));
}

#[test]
fn test_validate_session_id_valid() {
assert!(validate_session_id("550e8400-e29b-41d4-a716-446655440000").is_ok());
}

#[test]
fn test_validate_session_id_invalid() {
assert!(validate_session_id("not-a-uuid").is_err());
assert!(validate_session_id("550e8400-e29b-41d4-a716-44665544000X").is_err());
}

// ── Tool call gating (DC.T40) ─────────────────────────────────────────────

fn default_sec() -> crate::config::SecurityConfig {
crate::config::SecurityConfig::default()
}

#[test]
fn test_empty_allowlist_permits_all() {
let cfg = default_sec();
assert!(check_tool_call("Bash", "echo hello", &cfg).is_ok());
assert!(check_tool_call("Read", "", &cfg).is_ok());
assert!(check_tool_call("WebFetch", "", &cfg).is_ok());
}

#[test]
fn test_allowlist_blocks_unlisted_tool() {
let cfg = crate::config::SecurityConfig {
allowed_tools: vec!["Read".into(), "Grep".into()],
..Default::default()
};
assert!(check_tool_call("Read", "", &cfg).is_ok());
assert!(check_tool_call("Bash", "echo", &cfg).is_err());
}

#[test]
fn test_denylist_blocks_listed_tool() {
let cfg = crate::config::SecurityConfig {
denied_tools: vec!["WebFetch".into()],
..Default::default()
};
assert!(check_tool_call("WebFetch", "", &cfg).is_err());
assert!(check_tool_call("Read", "", &cfg).is_ok());
}

#[test]
fn test_tool_name_comparison_case_insensitive() {
let cfg = crate::config::SecurityConfig {
denied_tools: vec!["bash".into()],
..Default::default()
};
assert!(check_tool_call("Bash", "echo", &cfg).is_err());
assert!(check_tool_call("BASH", "echo", &cfg).is_err());
}

#[test]
fn test_denied_path_blocks_bash_call() {
let cfg = crate::config::SecurityConfig {
denied_paths: vec!["/etc".into()],
..Default::default()
};
assert!(check_tool_call("Bash", "cat /etc/passwd", &cfg).is_err());
assert!(check_tool_call("Bash", "echo hello", &cfg).is_ok());
}

#[test]
fn test_denied_path_only_applies_to_bash() {
let cfg = crate::config::SecurityConfig {
denied_paths: vec!["/etc".into()],
..Default::default()
};
// Read tool with /etc path should still be allowed (path check is Bash-only)
assert!(check_tool_call("Read", "/etc/passwd", &cfg).is_ok());
}

// ── Input sanitization (DC.T43) ───────────────────────────────────────────

#[test]
fn test_sanitize_short_string_unchanged() {
let s = "echo hello world";
assert_eq!(sanitize_tool_input(s), s);
}

#[test]
fn test_sanitize_long_base64_redacted() {
let key = "A".repeat(44); // 44 base64 chars → REDACTED
let input = format!("curl -H 'Authorization: Bearer {}'", key);
let result = sanitize_tool_input(&input);
assert!(
result.contains("[REDACTED]"),
"long token should be redacted: {result}"
);
assert!(!result.contains(&key), "original key should not appear");
}

#[test]
fn test_sanitize_normal_code_unchanged() {
let code = "let x = 42; println!(\"{x}\");";
assert_eq!(sanitize_tool_input(code), code);
}

// ── Repo path safety (DC.T41) ─────────────────────────────────────────────

#[test]
fn test_repo_path_does_not_overlap_data_dir() {
let data_dir = Path::new("/tmp/clawd_test_data");
let repo_path = Path::new("/home/user/my_project");
// Should not bail for non-overlapping paths
// (note: canonicalize will fail for non-existent paths, so normalize_path is used)
let result = check_repo_path_safety(repo_path, data_dir);
assert!(
result.is_ok(),
"non-overlapping paths should be ok: {result:?}"
);
}

#[test]
fn test_session_create_rejects_data_dir_as_repo() {
let data_dir = Path::new("/tmp/clawd_data_test_12345");
let repo_path = Path::new("/tmp/clawd_data_test_12345");
let result = check_repo_path_safety(repo_path, data_dir);
assert!(result.is_err(), "repo_path == data_dir should be rejected");
}
}
mod tests;
Loading