fix(shell-snapshot): prevent shell injection in restore_script via path escaping

[echobt]

Summary

Fix shell injection vulnerability in shell snapshot restore_script function.

Problem

The restore_script function was vulnerable to shell injection when paths contained single quotes.

Solution

• Add shell_escape_path() helper function that properly escapes paths
• Use POSIX-compliant quoting technique for single quotes

Related Issue

Fixes issue #5406

[greptile-apps]

Greptile Overview

Greptile Summary

This PR fixes a shell injection vulnerability in the restore_script() function by adding a new shell_escape_path() helper that properly escapes file paths before using them in shell commands.

Key changes:

• Added shell_escape_path() function that wraps paths in single quotes and escapes embedded single quotes using the '"'"' technique
• Updated restore_script() to use the escaped path instead of direct interpolation
• Removed vulnerable single-quote wrapping around self.path.display() in the source command
• Added comprehensive unit tests covering simple paths, paths with single quotes, and paths with spaces

Issue found:

• The implementation doesn't handle paths with embedded newlines or carriage returns, which could still cause command injection or unexpected behavior when the source command encounters a newline character

Confidence Score: 3/5

• The PR improves security by preventing single-quote injection, but paths with newlines could still cause issues
• The fix correctly addresses the reported vulnerability for single quotes, includes good test coverage, and uses a standard shell escaping technique. However, it doesn't handle edge cases like newline characters in paths which could lead to command injection or unexpected behavior, preventing a higher confidence score.
• Pay attention to src/cortex-shell-snapshot/src/snapshot.rs - the shell_escape_path function needs additional handling for newline characters

Important Files Changed

Filename	Overview
src/cortex-shell-snapshot/src/snapshot.rs	Added shell_escape_path() helper to prevent shell injection in restore_script() by escaping paths with single quotes

Sequence Diagram

sequenceDiagram
    participant User
    participant ShellSnapshot
    participant shell_escape_path
    participant Shell

    User->>ShellSnapshot: restore_script()
    ShellSnapshot->>ShellSnapshot: Get restore header
    ShellSnapshot->>shell_escape_path: Escape path with single quotes
    alt Path contains single quotes
        shell_escape_path->>shell_escape_path: Replace ' with '"'"'
        shell_escape_path-->>ShellSnapshot: 'path'"'"'with'"'"'quotes'
    else Path is simple
        shell_escape_path-->>ShellSnapshot: '/simple/path'
    end
    ShellSnapshot->>ShellSnapshot: Format script with escaped path
    ShellSnapshot-->>User: source {escaped_path}
    User->>Shell: Execute restore script
    Shell->>Shell: Source snapshot file safely

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

paths with embedded newlines could cause unexpected behavior - source command will terminate at the first newline inside the single-quoted path, potentially leading to command injection

Suggested change

	fn shell_escape_path(path: &Path) -> String {
	let path_str = path.display().to_string();
	if !path_str.contains('\'') {
	// Simple case: no single quotes, just wrap in single quotes
	format!("'{}'", path_str)
	} else {
	// Complex case: escape single quotes using '"'"' technique
	// This closes the single-quoted string, adds a double-quoted single quote,
	// and reopens the single-quoted string
	let escaped = path_str.replace('\'', "'\"'\"'");
	format!("'{}'", escaped)
	}
	}
	fn shell_escape_path(path: &Path) -> String {
	let path_str = path.display().to_string();
	// Check for newlines which could break the command
	if path_str.contains('\n') || path_str.contains('\r') {
	// For paths with newlines, use $'...' syntax with proper escaping
	let escaped = path_str
	.replace('\\', r"\\")
	.replace('\'', r"\'")
	.replace('\n', r"\n")
	.replace('\r', r"\r");
	format!("$'{}'", escaped)
	} else if !path_str.contains('\'') {
	// Simple case: no single quotes, just wrap in single quotes
	format!("'{}'", path_str)
	} else {
	// Complex case: escape single quotes using '"'"' technique
	let escaped = path_str.replace('\'', "'\"'\"'");
	format!("'{}'", escaped)
	}
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-shell-snapshot/src/snapshot.rs
Line: 207:220

Comment:
paths with embedded newlines could cause unexpected behavior - `source` command will terminate at the first newline inside the single-quoted path, potentially leading to command injection

```suggestion
fn shell_escape_path(path: &Path) -> String {
    let path_str = path.display().to_string();

    // Check for newlines which could break the command
    if path_str.contains('\n') || path_str.contains('\r') {
        // For paths with newlines, use $'...' syntax with proper escaping
        let escaped = path_str
            .replace('\\', r"\\")
            .replace('\'', r"\'")
            .replace('\n', r"\n")
            .replace('\r', r"\r");
        format!("$'{}'", escaped)
    } else if !path_str.contains('\'') {
        // Simple case: no single quotes, just wrap in single quotes
        format!("'{}'", path_str)
    } else {
        // Complex case: escape single quotes using '"'"' technique
        let escaped = path_str.replace('\'', "'\"'\"'");
        format!("'{}'", escaped)
    }
}
```

How can I resolve this? If you propose a fix, please make it concise.

[echobt]

Consolidated into #82 - fix: consolidated security and robustness improvements