Skip to content

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

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

Closed
echobt wants to merge 1 commit into from
Closed

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

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
52 changes: 48 additions & 4 deletions src/cortex-shell-snapshot/src/snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,13 @@ impl ShellSnapshot {
}

/// Generate a restore script that sources this snapshot.
///
/// The path is properly escaped to prevent shell injection attacks.
/// Paths containing single quotes are escaped using shell-safe quoting.
pub fn restore_script(&self) -> String {
let header = scripts::restore_header(self.metadata.shell_type);
format!(
"{header}\n# Source snapshot\nsource '{}'\n",
self.path.display()
)
let escaped_path = shell_escape_path(&self.path);
format!("{header}\n# Source snapshot\nsource {escaped_path}\n")
}

/// Save the snapshot to disk.
Expand Down Expand Up @@ -197,6 +198,27 @@ impl Drop for ShellSnapshot {
}
}

/// Escape a path for safe use in shell commands.
///
/// This function handles paths containing single quotes by using the
/// shell-safe escaping technique: 'path'"'"'with'"'"'quotes'
///
/// For paths without single quotes, simple single-quoting is used.
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)
}
}
Comment on lines +207 to +220
Copy link

@greptile-apps greptile-apps bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -221,4 +243,26 @@ mod tests {
"snapshot_12345678-1234-1234-1234-123456789012.zsh"
);
}

#[test]
fn test_shell_escape_path_simple() {
let path = Path::new("/tmp/test/snapshot.sh");
let escaped = shell_escape_path(path);
assert_eq!(escaped, "'/tmp/test/snapshot.sh'");
}

#[test]
fn test_shell_escape_path_with_single_quotes() {
let path = Path::new("/tmp/test's/snap'shot.sh");
let escaped = shell_escape_path(path);
// Single quotes should be escaped using '"'"' technique
assert_eq!(escaped, "'/tmp/test'\"'\"'s/snap'\"'\"'shot.sh'");
}

#[test]
fn test_shell_escape_path_spaces() {
let path = Path::new("/tmp/test path/snapshot.sh");
let escaped = shell_escape_path(path);
assert_eq!(escaped, "'/tmp/test path/snapshot.sh'");
}
}