From cda13fbed133d644752fd0e12a819003aeb43d3e Mon Sep 17 00:00:00 2001
From: Thando Mini <thando.mini@sanlam.co.za>
Date: Thu, 11 Jun 2026 13:04:33 +0200
Subject: [PATCH] fix(runtime): tighten run_command metachar reject to
 ContainsAny set
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The ad-hoc substring loop in isCommandAllowed missed:
- bare tab (\t) — only "\t&" was rejected; a tab on its own (e.g.
  "go\ttest ./...") passed the metachar check then failed the prefix
  match, but the inconsistency was a footgun;
- NUL byte (\x00) — "go build\x00evil" passed metachar and failed
  prefix, again silent rejection-as-denial rather than rejection-as-
  intent;
- bare > and < — only "$" / "$(" caught command substitution but
  redirection had no entry;
- \ (backslash) — could be used to escape characters.

Replace the substring loop with one strings.ContainsAny over the
canonical forbidden set: `;&|$`<>\n\r\t\x00\\`. Faster (single linear
scan), and closes the gaps. ; & | $ ` already covered chaining +
expansion + command substitution; > < add redirection; \n \r \t \x00
cover control chars; \\ catches escapes.

7 new table-driven cases in TestIsCommandAllowed_RejectsExtendedMetachars
cover bare tab, NUL, > / <, newline, carriage return, backslash, and
bare $.

Surfaced by the 2026-06-11 security audit (SEC-M3).
---
 internal/runtime/gemma.go      | 17 ++++++++++-------
 internal/runtime/tools_test.go | 27 +++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/internal/runtime/gemma.go b/internal/runtime/gemma.go
index ef45485..2174bca 100644
--- a/internal/runtime/gemma.go
+++ b/internal/runtime/gemma.go
@@ -622,7 +622,8 @@ func safePath(relPath, workDir string) (string, error) {
 // It extracts the binary name from the command (first whitespace-delimited token)
 // and validates that the full command starts with an allowlisted prefix followed
 // by either a space, end-of-string, or the exact match. Shell metacharacters
-// (;, |, &, $, `, \n) are rejected outright to prevent command chaining.
+// (;, |, &, $, `, \n, \r, \t, NUL, <, >) are rejected outright to prevent
+// command chaining, redirection, expansion, and substitution.
 func isCommandAllowed(command string, allowlist []string) bool {
 	command = strings.TrimSpace(command)
 	if command == "" {
@@ -630,12 +631,14 @@ func isCommandAllowed(command string, allowlist []string) bool {
 	}
 
 	// H9: reject any shell metacharacter that could chain commands, redirect
-	// I/O, or escape the allowlist. Belt-and-suspenders alongside the prefix
-	// match below.
-	for _, ch := range []string{";", "&&", "||", "|", "$(", "$", "`", ">", "<", "\n", "\r", "\t&"} {
-		if strings.Contains(command, ch) {
-			return false
-		}
+	// I/O, or escape the allowlist. ContainsAny over a canonical set is
+	// faster than per-pattern substring scans AND closes the gaps the prior
+	// list left open — bare tab (was only "\t&"), NUL byte (would otherwise
+	// pass the metachar check and then fail the prefix match, but better
+	// rejected loudly), and `\` (escapes).
+	const forbidden = ";&|$`<>\n\r\t\x00\\"
+	if strings.ContainsAny(command, forbidden) {
+		return false
 	}
 
 	for _, pattern := range allowlist {
diff --git a/internal/runtime/tools_test.go b/internal/runtime/tools_test.go
index 031f334..1470bfe 100644
--- a/internal/runtime/tools_test.go
+++ b/internal/runtime/tools_test.go
@@ -315,6 +315,33 @@ func TestIsCommandAllowed_EmptyCommand(t *testing.T) {
 	}
 }
 
+// TestIsCommandAllowed_RejectsExtendedMetachars covers the post-SEC-M3
+// expansion of the forbidden set. Bare tab, NUL byte, redirection, and
+// backslash were missing from the old ad-hoc substring loop. The new
+// ContainsAny check rejects every dangerous control / metacharacter.
+func TestIsCommandAllowed_RejectsExtendedMetachars(t *testing.T) {
+	cases := []struct {
+		name string
+		cmd  string
+	}{
+		{"bare tab", "echo\thello"},
+		{"NUL byte", "echo\x00rm -rf /"},
+		{"redirect out", "echo hi > /etc/passwd"},
+		{"redirect in", "echo < /etc/passwd"},
+		{"newline", "echo ok\nrm -rf /"},
+		{"carriage return", "echo ok\rrm -rf /"},
+		{"backslash", "echo \\$HOME"},
+		{"bare dollar (env expansion)", "echo $HOME"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if isCommandAllowed(tc.cmd, []string{"echo"}) {
+				t.Errorf("must reject %q", tc.cmd)
+			}
+		})
+	}
+}
+
 func TestIsCommandAllowed_MultiWordPattern(t *testing.T) {
 	allowlist := []string{"go test", "go build"}
 	if !isCommandAllowed("go test ./...", allowlist) {