fix(agent): the allowlist covers the whole line, and coder test stops being the way around it (#1531)

Strict mode checked the first word of a command and nothing else. A line is
a sequence of invocations, so that made any allowed command a passphrase
for the rest of it: ls followed by an ampersand pair carried an arbitrary
command straight past the gate. "Only commands from the allowlist can be
executed" was not true of any line with an operator in it. Every segment is
held to the same rule now, decomposed by the shell parser that was already
in the tree, so a quoted operator stays a quoted operator and is not
mistaken for a chain.

Two things kept that from becoming a regression. A line the parser cannot
read falls back to the previous single-command check rather than failing
closed, because a host whose shell is not bash would otherwise lose every
command. And the navigation builtins joined the list: cd, pwd, pushd and
their neighbours carry no capability of their own, and refusing them once
every segment is checked would have broken a directory change followed by a
build while withholding nothing.

Nineteen commands the documentation lists as allowed were not on the list
at all, so strict mode refused npx, base64, openssl, poetry, zig, istioctl
and the rest while the page said otherwise. They are on it now.

Two settings did nothing when written the way they were documented. The
custom allowlist split on commas while the documented example used
semicolons, which registered one command with semicolons in its name; extra
read paths split on the native separator while the documented example used
semicolons, which produced one path that exists nowhere. Both accept either
spelling now, except that a colon is still never a separator on Windows,
where it separates a drive letter from its path.

The coder test subcommand takes an arbitrary command and ran it through
none of the guards its sibling applies: not the dangerous-pattern check,
not the sandbox, and not the upstream gate, which only ever looked at exec.
The same payload the exec path refuses executed through test. It is guarded
now, with the same escape hatches, so a suite that legitimately needs them
is not newly refused, and the upstream gate covers both subcommands rather
than the one that happens to be named after running commands.

Co-authored-by: diillson <diiilllson@gmail.com>

Author: diillson

cli/agent/allowlist_chain_test.go

@@ -0,0 +1,155 @@
+/*
+ * ChatCLI - Command Line Interface for LLM interaction
+ * Copyright (c) 2024 Edilson Freitas
+ * License: Apache-2.0
+ */
+package agent
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+// Prefixing a benign command used to be a passphrase for the rest of the
+// line: only the first word was checked, so anything after && or | rode in.
+func TestAllowlist_ChecksEveryCommandOnTheLine(t *testing.T) {
+	t.Setenv("CHATCLI_AGENT_SECURITY_MODE", "strict")
+	al := NewCommandAllowlist()
+
+	bypasses := []string{
+		"ls && nmap -sS 10.0.0.0/8",
+		"echo hi; nmap localhost",
+		"ls | nmap -",
+		"ls || nmap -",
+		"ls && ls && nmap -",
+		"(cd /tmp && nmap -)",
+		"ls & nmap -",
+	}
+	for _, cmd := range bypasses {
+		if ok, _, _ := al.IsAllowed(cmd); ok {
+			t.Errorf("bypass still open: %q passed the strict allowlist", cmd)
+		}
+	}
+}
+
+// Everything legitimate must keep working — a chain of allowed commands is
+// the normal case, not an attack.
+func TestAllowlist_AllowsChainsOfAllowedCommands(t *testing.T) {
+	t.Setenv("CHATCLI_AGENT_SECURITY_MODE", "strict")
+	al := NewCommandAllowlist()
+
+	fine := []string{
+		"ls -la",
+		"go build ./... && go test ./...",
+		"cat go.mod | grep module",
+		"git status; git diff",
+		"grep -r foo . | sort | uniq -c | head",
+		"FOO=bar go test ./...",
+		"cd /tmp && ls",
+	}
+	for _, cmd := range fine {
+		if ok, _, reason := al.IsAllowed(cmd); !ok {
+			t.Errorf("legitimate command refused: %q (%s)", cmd, reason)
+		}
+	}
+}
+
+// Quoting must not be mistaken for a chain: the shell parser is the point.
+func TestAllowlist_QuotedOperatorsAreNotSegments(t *testing.T) {
+	t.Setenv("CHATCLI_AGENT_SECURITY_MODE", "strict")
+	al := NewCommandAllowlist()
+
+	for _, cmd := range []string{
+		`echo "a && b"`,
+		`grep "foo | bar" file.txt`,
+		`echo 'nmap is a word here'`,
+	} {
+		if ok, _, reason := al.IsAllowed(cmd); !ok {
+			t.Errorf("quoted operator treated as a chain: %q (%s)", cmd, reason)
+		}
+	}
+}
+
+// The sudo prefix keeps being handled by the denylist, not by demanding
+// that "sudo" itself be an allowlisted command.
+func TestAllowlist_SudoPrefixStillResolvesToTheRealCommand(t *testing.T) {
+	t.Setenv("CHATCLI_AGENT_SECURITY_MODE", "strict")
+	al := NewCommandAllowlist()
+
+	if ok, _, reason := al.IsAllowed("sudo ls -la"); !ok {
+		t.Errorf("sudo prefix changed the allowlist verdict: %s", reason)
+	}
+	if ok, _, _ := al.IsAllowed("sudo nmap -"); ok {
+		t.Error("sudo hid a command that is not on the allowlist")
+	}
+}
+
+// Every command the documentation lists as allowed must actually be allowed.
+func TestAllowlist_CoversTheDocumentedCommands(t *testing.T) {
+	t.Setenv("CHATCLI_AGENT_SECURITY_MODE", "strict")
+	al := NewCommandAllowlist()
+
+	documented := []string{
+		"ag", "argocd", "base64", "cal", "clear", "cmp", "csvtool", "flux",
+		"istioctl", "kotlinc", "look", "npx", "openssl", "poetry", "reset",
+		"stty", "tput", "xmllint", "zig",
+	}
+	for _, cmd := range documented {
+		if ok, _, _ := al.IsAllowed(cmd + " --version"); !ok {
+			t.Errorf("%q is documented as allowed and is not", cmd)
+		}
+	}
+}
+
+// The value shape the documentation gave has to work, or the setting looks
+// applied and is not.
+func TestAllowlist_CustomListAcceptsBothSeparators(t *testing.T) {
+	for _, value := range []string{
+		"mycli;internal-tool;company-deploy",
+		"mycli,internal-tool,company-deploy",
+		"mycli; internal-tool ,company-deploy",
+	} {
+		t.Setenv("CHATCLI_AGENT_ALLOWLIST", value)
+		al := NewCommandAllowlist()
+		for _, cmd := range []string{"mycli", "internal-tool", "company-deploy"} {
+			if ok, _, _ := al.IsAllowed(cmd + " run"); !ok {
+				t.Errorf("CHATCLI_AGENT_ALLOWLIST=%q did not register %q", value, cmd)
+			}
+		}
+	}
+}
+
+func TestExtraReadPaths_AcceptsBothSeparators(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("':' is a drive-letter separator on Windows")
+	}
+	for _, value := range []string{
+		"/etc/hosts;/usr/local/share/config",
+		"/etc/hosts:/usr/local/share/config",
+	} {
+		t.Setenv("CHATCLI_AGENT_EXTRA_READ_PATHS", value)
+		s := NewSensitiveReadPaths()
+		if len(s.extraReadPaths) != 2 {
+			t.Errorf("CHATCLI_AGENT_EXTRA_READ_PATHS=%q parsed to %v", value, s.extraReadPaths)
+			continue
+		}
+		if s.extraReadPaths[0] != "/etc/hosts" || s.extraReadPaths[1] != "/usr/local/share/config" {
+			t.Errorf("CHATCLI_AGENT_EXTRA_READ_PATHS=%q parsed to %v", value, s.extraReadPaths)
+		}
+	}
+}
+
+// A path list that genuinely needs a semicolon in a name still resolves via
+// the native separator, so nothing that worked stops working.
+func TestExtraReadPaths_NativeSeparatorStillWorks(t *testing.T) {
+	dir := t.TempDir()
+	a := filepath.Join(dir, "a")
+	b := filepath.Join(dir, "b")
+	t.Setenv("CHATCLI_AGENT_EXTRA_READ_PATHS", a+string(os.PathListSeparator)+b)
+	s := NewSensitiveReadPaths()
+	if len(s.extraReadPaths) != 2 {
+		t.Fatalf("native separator parsed to %v", s.extraReadPaths)
+	}
+}

cli/agent/command_allowlist.go

@@ -41,6 +41,7 @@ func DefaultAllowedCommands() map[string]string {
 		"ln": "file", "chmod": "file", "chown": "file", "basename": "file",
 		"dirname": "file", "realpath": "file", "readlink": "file",
 		"md5sum": "file", "sha256sum": "file", "sha1sum": "file",
+		"cmp": "file",
 
 		// Text processing
 		"grep": "text", "rg": "text", "sed": "text", "awk": "text",
@@ -50,6 +51,8 @@ func DefaultAllowedCommands() map[string]string {
 		"fold": "text", "expand": "text", "unexpand": "text",
 		"comm": "text", "join": "text", "nl": "text", "rev": "text",
 		"strings": "text", "od": "text", "xxd": "text", "hexdump": "text",
+		"ag": "text", "look": "text", "base64": "text", "xmllint": "text",
+		"csvtool": "text", "openssl": "text",
 
 		// Development tools
 		"go": "dev", "git": "dev", "make": "dev", "npm": "dev",
@@ -65,6 +68,7 @@ func DefaultAllowedCommands() map[string]string {
 		"gofmt": "dev", "golint": "dev", "gopls": "dev",
 		"eslint": "dev", "prettier": "dev", "black": "dev",
 		"pytest": "dev", "jest": "dev", "mocha": "dev",
+		"npx": "dev", "poetry": "dev", "zig": "dev", "kotlinc": "dev",
 
 		// Container / Infrastructure
 		"docker": "container", "podman": "container",
@@ -75,6 +79,7 @@ func DefaultAllowedCommands() map[string]string {
 		"kustomize": "container", "oc": "container",
 		"eksctl": "container", "gcloud": "container",
 		"aws": "container", "az": "container",
+		"istioctl": "container", "argocd": "container", "flux": "container",
 
 		// Network (read-only oriented)
 		"curl": "network", "wget": "network",
@@ -92,6 +97,7 @@ func DefaultAllowedCommands() map[string]string {
 		"id": "sysinfo", "groups": "sysinfo", "lsof": "sysinfo",
 		"ulimit": "sysinfo", "locale": "sysinfo", "getconf": "sysinfo",
 		"arch": "sysinfo", "nproc": "sysinfo", "lscpu": "sysinfo",
+		"cal":   "sysinfo",
 		"lsblk": "sysinfo", "mount": "sysinfo", "lsusb": "sysinfo",
 
 		// Editors / Viewers
@@ -107,6 +113,15 @@ func DefaultAllowedCommands() map[string]string {
 		"export": "shell", "set": "shell", "unset": "shell",
 		"alias": "shell", "type": "shell", "command": "shell",
 		"source": "shell", "eval": "shell", "exec": "shell",
+		"clear": "shell", "reset": "shell", "tput": "shell", "stty": "shell",
+
+		// Navigation and no-op builtins. They carry no capability of their
+		// own, and they are how real command lines are written: refusing
+		// them once every segment is checked would break "cd sub && build"
+		// without withholding anything.
+		"cd": "shell", "pwd": "shell", "pushd": "shell", "popd": "shell",
+		"dirs": "shell", "wait": "shell", "read": "shell", "shift": "shell",
+		"jobs": "shell", ":": "shell", "[": "shell",
 		"sh": "shell", "bash": "shell", "zsh": "shell",
 	}
 	return commands
@@ -124,9 +139,13 @@ func NewCommandAllowlist() *CommandAllowlist {
 		mode:            mode,
 	}
 
-	// Add custom commands from CHATCLI_AGENT_ALLOWLIST env var (comma-separated)
+	// Add custom commands from CHATCLI_AGENT_ALLOWLIST. Both separators are
+	// accepted: the sibling denylist variable is semicolon-separated, the
+	// documentation said semicolon here too, and a value that silently
+	// registers one command named "a;b;c" is a configuration that looks
+	// applied and is not.
 	if extra := os.Getenv("CHATCLI_AGENT_ALLOWLIST"); extra != "" {
-		for _, cmd := range strings.Split(extra, ",") {
+		for _, cmd := range strings.FieldsFunc(extra, func(r rune) bool { return r == ',' || r == ';' }) {
 			cmd = strings.TrimSpace(cmd)
 			if cmd != "" {
 				al.allowedCommands[cmd] = "custom"
@@ -137,8 +156,16 @@ func NewCommandAllowlist() *CommandAllowlist {
 	return al
 }
 
-// IsAllowed checks if a command is in the allowlist.
+// IsAllowed checks whether every command on the line is in the allowlist.
 // Returns (allowed, category, reason).
+//
+// Every command, not the first one: a line is a sequence of invocations, and
+// checking only the leading word means any allowed command is a passphrase
+// for the rest of the line. Prefixing with a benign command was enough to
+// carry an arbitrary one past the gate, which is the opposite of what an
+// allowlist is for.
+//
+// The category returned is the leading command's, which is what callers log.
 func (al *CommandAllowlist) IsAllowed(fullCommand string) (bool, string, string) {
 	al.mu.RLock()
 	defer al.mu.RUnlock()
@@ -148,11 +175,34 @@ func (al *CommandAllowlist) IsAllowed(fullCommand string) (bool, string, string)
 		return false, "", "empty command"
 	}
 
-	if category, ok := al.allowedCommands[baseCmd]; ok {
+	category, ok := al.allowedCommands[baseCmd]
+	if !ok {
+		return false, "", "command '" + baseCmd + "' is not in the security allowlist"
+	}
+
+	// Decompose with a real shell parser and hold every segment to the same
+	// rule. On a line the parser could not read, the legacy single-command
+	// check above stands on its own: a line bash cannot parse is one the
+	// executor's shell is unlikely to run either, and failing closed here
+	// would refuse every command on a host whose shell is not bash.
+	segments, parsed := ParseShellSegmentsChecked(fullCommand)
+	if !parsed {
 		return true, category, ""
 	}
 
-	return false, "", "command '" + baseCmd + "' is not in the security allowlist"
+	for _, seg := range segments {
+		segCmd := extractBaseCommand(seg.Full)
+		if segCmd == "" {
+			// A segment that carries no invocation — a bare assignment, say.
+			// Nothing to authorize.
+			continue
+		}
+		if _, ok := al.allowedCommands[segCmd]; !ok {
+			return false, "", "command '" + segCmd + "' is not in the security allowlist"
+		}
+	}
+
+	return true, category, ""
 }
 
 // GetMode returns the current security mode.

cli/agent/read_path_validator.go

@@ -8,6 +8,7 @@ package agent
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 
 	"github.com/diillson/chatcli/pkg/fspath"
@@ -29,10 +30,15 @@ func NewSensitiveReadPaths() *SensitiveReadPaths {
 		allowKubeconfig: strings.EqualFold(os.Getenv("CHATCLI_AGENT_ALLOW_KUBECONFIG"), "true"),
 	}
 
-	// Parse extra allowed read paths (separated by os.PathListSeparator:
-	// ':' on Unix, ';' on Windows — ':' would split drive letters apart)
+	// Parse extra allowed read paths. The native separator always works
+	// (':' on Unix, ';' on Windows), and ';' is additionally accepted on
+	// Unix because every sibling agent variable uses it and the
+	// documentation said so here too — a value that silently becomes one
+	// path named "/a;/b" is a setting that looks applied and is not.
+	// ':' is never a separator on Windows: it would split drive letters
+	// apart.
 	if extra := os.Getenv("CHATCLI_AGENT_EXTRA_READ_PATHS"); extra != "" {
-		for _, p := range filepath.SplitList(extra) {
+		for _, p := range splitReadPaths(extra) {
 			p = strings.TrimSpace(p)
 			if p != "" {
 				s.extraReadPaths = append(s.extraReadPaths, p)
@@ -213,3 +219,14 @@ func (s *SensitiveReadPaths) isSensitivePath(path string) (bool, string) {
 
 	return false, ""
 }
+
+// splitReadPaths splits a path list on the separators that are unambiguous
+// for the host: the native one everywhere, plus ';' on Unix.
+func splitReadPaths(value string) []string {
+	if runtime.GOOS == "windows" {
+		return filepath.SplitList(value)
+	}
+	return strings.FieldsFunc(value, func(r rune) bool {
+		return r == os.PathListSeparator || r == ';'
+	})
+}

cli/agent/shell_parser.go

@@ -46,17 +46,29 @@ type ShellSegment struct {
 // the dangerous-pattern matcher still runs against the full line as a
 // belt-and-suspenders measure.
 func ParseShellSegments(line string) []ShellSegment {
+	segments, _ := ParseShellSegmentsChecked(line)
+	return segments
+}
+
+// ParseShellSegmentsChecked is ParseShellSegments plus whether the line
+// actually parsed.
+//
+// The distinction matters to a caller that decides policy per segment: the
+// fallback returns the whole line as one segment, which is indistinguishable
+// from a genuine single command, and a gate that cannot tell them apart is a
+// gate that can be talked out of decomposing a line at all.
+func ParseShellSegmentsChecked(line string) (segments []ShellSegment, parsed bool) {
 	trimmed := strings.TrimSpace(line)
 	if trimmed == "" {
-		return nil
+		return nil, true
 	}
 	parser := syntax.NewParser(syntax.Variant(syntax.LangBash))
 	file, err := parser.Parse(strings.NewReader(trimmed), "")
 	if err != nil {
 		// Couldn't parse — return whole line as one segment so callers can
 		// still run their regex matchers. Returning nil would silently bypass
 		// the dangerous-pattern check.
-		return []ShellSegment{singleSegment(trimmed, false, 0)}
+		return []ShellSegment{singleSegment(trimmed, false, 0)}, false
 	}
 
 	var out []ShellSegment
@@ -67,7 +79,7 @@ func ParseShellSegments(line string) []ShellSegment {
 	if len(out) == 0 {
 		out = append(out, singleSegment(trimmed, false, 0))
 	}
-	return out
+	return out, true
 }
 
 // walkStmt recursively flattens a statement into its constituent simple

cli/agent_coder_validation.go

@@ -11,17 +11,27 @@ import (
 	"strings"
 )
 
-// isCoderExecDangerous checks if a @coder exec command contains a dangerous
-// shell command. It extracts the actual shell command from the parsed args
-// and validates it against the agent's CommandValidator.IsDangerous().
+// coderShellSubcommands are the @coder subcommands that run an arbitrary
+// shell line given to them.
+//
+// `test` belongs here as much as `exec` does: it takes the same --cmd and
+// runs it through the same shell. Guarding only the subcommand named after
+// running commands left the one named after running tests as an unguarded
+// path to the same place.
+var coderShellSubcommands = map[string]bool{"exec": true, "test": true}
+
+// isCoderExecDangerous checks if a @coder subcommand that runs a shell line
+// carries a dangerous command. It extracts the actual shell command from the
+// parsed args and validates it against the agent's
+// CommandValidator.IsDangerous().
 // This is the critical security guard that prevents destructive commands
-// from executing through @coder exec even when the policy says "allow".
+// from executing through @coder even when the policy says "allow".
 func (a *AgentMode) isCoderExecDangerous(toolArgs []string) (bool, string) {
 	if len(toolArgs) == 0 {
 		return false, ""
 	}
 	sub := strings.ToLower(strings.TrimSpace(toolArgs[0]))
-	if sub != "exec" {
+	if !coderShellSubcommands[sub] {
 		return false, ""
 	}
 	// Extract the --cmd value from parsed args