Skip to content

Commit 7673aa6

Browse files
dwalleckclaude
andcommitted
fix: Address PR #21 review feedback - improve safety
This commit addresses the two high-priority issues identified in PR review: ## 1. Fixed .unwrap() on Path Conversion (status.rs:163) **Issue:** Using `.unwrap()` on `path.to_str()` panics on non-UTF-8 paths. **Fix:** Replaced with proper error handling: ```rust let settings_path_str = settings_path .to_str() .ok_or_else(|| { CatalystError::InvalidPath(format!( "Settings path contains non-UTF-8 characters: {:?}", settings_path )) })?; ``` Now gracefully handles non-UTF-8 paths with a descriptive error message. ## 2. Added Binary Name Validation (status.rs:534-544) **Issue:** Potential template injection in `fix_hook_wrapper` where binary_name is extracted from wrapper_name and used in template substitution without validation. **Fix:** Added strict validation that only allows safe characters: ```rust // Validate binary name to prevent potential injection // Only allow alphanumeric characters, hyphens, and underscores if !binary_name .chars() .all(|c| c.is_alphanumeric() || c == '-' || c == '_') { return Err(CatalystError::InvalidConfig(format!( "Invalid binary name '{}': must contain only alphanumeric, hyphens, underscores", binary_name ))); } ``` This prevents injection of shell metacharacters like `;`, `$`, `|`, `..`, etc. ## Test Coverage Added comprehensive test `test_fix_hook_wrapper_validates_binary_name`: - ✅ Valid names accepted: `skill-activation-prompt`, `file-change-tracker` - ❌ Invalid names rejected: `test;rm-rf`, `test$command`, `test/../etc/passwd` All tests passing: **36 in catalyst-cli + 28 in catalyst-core = 64 total** ## Security Impact These fixes eliminate two potential security issues: 1. **Path handling**: No more panics on non-UTF-8 paths 2. **Injection prevention**: Malicious binary names cannot inject commands The changes maintain backward compatibility while improving robustness. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 59bbc9d commit 7673aa6

1 file changed

Lines changed: 53 additions & 2 deletions

File tree

catalyst-cli/src/status.rs

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,14 @@ fn validate_hooks(target_dir: &Path, platform: Platform) -> Result<Vec<HookStatu
160160
}
161161

162162
// Parse settings.json
163-
let settings = match ClaudeSettings::read(settings_path.to_str().unwrap()) {
163+
let settings_path_str = settings_path.to_str().ok_or_else(|| {
164+
CatalystError::InvalidPath(format!(
165+
"Settings path contains non-UTF-8 characters: {:?}",
166+
settings_path
167+
))
168+
})?;
169+
170+
let settings = match ClaudeSettings::read(settings_path_str) {
164171
Ok(s) => s,
165172
Err(_) => {
166173
// Invalid settings.json - report with issue
@@ -522,6 +529,18 @@ fn fix_hook_wrapper(target_dir: &Path, wrapper_name: &str, platform: Platform) -
522529
.trim_end_matches(".sh")
523530
.trim_end_matches(".ps1");
524531

532+
// Validate binary name to prevent potential injection
533+
// Only allow alphanumeric characters, hyphens, and underscores
534+
if !binary_name
535+
.chars()
536+
.all(|c| c.is_alphanumeric() || c == '-' || c == '_')
537+
{
538+
return Err(CatalystError::InvalidConfig(format!(
539+
"Invalid binary name '{}': must contain only alphanumeric characters, hyphens, and underscores",
540+
binary_name
541+
)));
542+
}
543+
525544
// Use the init module's wrapper generation
526545
// For now, we'll just recreate the wrapper using the same logic
527546
let hooks_dir = target_dir.join(HOOKS_DIR);
@@ -535,7 +554,7 @@ fn fix_hook_wrapper(target_dir: &Path, wrapper_name: &str, platform: Platform) -
535554
Platform::Windows => include_str!("../resources/wrapper-template.ps1"),
536555
};
537556

538-
// Replace template variable
557+
// Replace template variable (safe after validation above)
539558
let content = template.replace("{{BINARY_NAME}}", binary_name);
540559

541560
// Write wrapper file
@@ -631,4 +650,36 @@ mod tests {
631650
let content = fs::read_to_string(&version_path).unwrap();
632651
assert_eq!(content.trim(), env!("CARGO_PKG_VERSION"));
633652
}
653+
654+
#[test]
655+
fn test_fix_hook_wrapper_validates_binary_name() {
656+
let temp_dir = TempDir::new().unwrap();
657+
let hooks_dir = temp_dir.path().join(".claude/hooks");
658+
fs::create_dir_all(&hooks_dir).unwrap();
659+
660+
// Valid binary names should work
661+
let result = fix_hook_wrapper(
662+
temp_dir.path(),
663+
"skill-activation-prompt.sh",
664+
Platform::Linux,
665+
);
666+
assert!(result.is_ok());
667+
668+
let result = fix_hook_wrapper(temp_dir.path(), "file-change-tracker.sh", Platform::Linux);
669+
assert!(result.is_ok());
670+
671+
// Invalid binary names should be rejected
672+
let result = fix_hook_wrapper(temp_dir.path(), "test;rm-rf.sh", Platform::Linux);
673+
assert!(result.is_err());
674+
assert!(result
675+
.unwrap_err()
676+
.to_string()
677+
.contains("Invalid binary name"));
678+
679+
let result = fix_hook_wrapper(temp_dir.path(), "test$command.sh", Platform::Linux);
680+
assert!(result.is_err());
681+
682+
let result = fix_hook_wrapper(temp_dir.path(), "test/../etc/passwd.sh", Platform::Linux);
683+
assert!(result.is_err());
684+
}
634685
}

0 commit comments

Comments
 (0)