fix: preserve parser compatibility for unmatched ')' handling

scumfrog · scumfrog · commit b074fa047f43 · 2026-04-13T18:11:53.000+02:00
diff --git a/shellwords.go b/shellwords.go
@@ -204,13 +204,12 @@ loop:
 		case ')':
 			if !singleQuoted && !doubleQuoted && !backQuote {
 				if p.ParseBacktick {
-					// Hardened fix:
+					// Security fix:
 					// A bare ')' must never open dollarQuote state.
-					// Only close an already-open $(...) region.
+					// Preserve prior behavior by rejecting unmatched ')'
+					// when command substitution parsing is enabled.
 					if !dollarQuote {
-						buf += string(r)
-						got = argSingle
-						continue
+						return nil, errors.New("invalid command line string")
 					}
 
 					out, err := shellRun(backtick, p.Dir)
@@ -230,17 +229,19 @@ loop:
 					continue
 				}
 
-				// Default mode:
-				// Do not toggle dollarQuote on a bare ')'.
-				// Treat it as a literal character.
-				if !dollarQuote {
+				// Backtick parsing disabled:
+				// preserve literal text for $(...) constructs instead of
+				// silently dropping the closing ')'.
+				if dollarQuote {
 					buf += string(r)
+					backtick = ""
+					dollarQuote = false
 					got = argSingle
 					continue
 				}
 
-				backtick = ""
-				dollarQuote = false
+				buf += string(r)
+				got = argSingle
 				continue
 			}
 
@@ -350,4 +351,4 @@ func Parse(line string) ([]string, error) {
 
 func ParseWithEnvs(line string) (envs []string, args []string, err error) {
 	return NewParser().ParseWithEnvs(line)
-}
+}
diff --git a/shellwords_security_test.go b/shellwords_security_test.go
@@ -5,41 +5,36 @@ import (
 	"testing"
 )
 
-func TestUnmatchedClosingParen_NoPanic_ParseBacktickEnabled(t *testing.T) {
+// ParseBacktick=true → an unmatched ‘)’ should be treated as an ERROR (not a literal)
+func TestUnmatchedClosingParen_ReturnsError_ParseBacktickEnabled(t *testing.T) {
 	p := NewParser()
 	p.ParseBacktick = true
 
-	defer func() {
-		if r := recover(); r != nil {
-			t.Fatalf("unexpected panic: %v", r)
-		}
-	}()
-
-	out, err := p.Parse("))")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	if len(out) != 1 || out[0] != "))" {
-		t.Fatalf("unexpected output: %#v", out)
+	_, err := p.Parse("))")
+	if err == nil {
+		t.Fatal("expected error for unmatched ')', got nil")
 	}
 }
 
-func TestBareClosingParen_DoesNotTriggerCommandExecution(t *testing.T) {
+// ParseBacktick=true → Do not execute; must fail
+func TestBareClosingParen_NoExecution_ReturnsError(t *testing.T) {
 	p := NewParser()
 	p.ParseBacktick = true
 
-	out, err := p.Parse("prefix)echo PWNED)")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+	_, err := p.Parse("prefix)echo PWNED)")
+	if err == nil {
+		t.Fatal("expected error for unmatched ')', got nil")
 	}
+}
 
-	got := strings.Join(out, " ")
-	if strings.Contains(got, "PWNED") && got != "prefix)echo PWNED)" {
-		t.Fatalf("unexpected command execution or substitution occurred: %q", got)
+// Default behavior → ‘)’ is a literal; it does not break parsing
+func TestBareClosingParen_DefaultParsingLiteral(t *testing.T) {
+	out, err := Parse(")a b)c d")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
 	}
 
-	want := []string{"prefix)echo", "PWNED)"}
+	want := []string{")a", "b)c", "d"}
 	if len(out) != len(want) {
 		t.Fatalf("unexpected token count: got %d, want %d, out=%#v", len(out), len(want), out)
 	}
@@ -50,23 +45,26 @@ func TestBareClosingParen_DoesNotTriggerCommandExecution(t *testing.T) {
 	}
 }
 
-func TestBareClosingParen_DefaultParsingLiteral(t *testing.T) {
-	out, err := Parse(")a b)c d")
+// ParseBacktick=false → $(...) remains a FULL literal
+func TestDollarParenLiteralWhenParseBacktickDisabled(t *testing.T) {
+	p := NewParser()
+	p.ParseBacktick = false
+
+	out, err := p.Parse(`$(echo "foo")`)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
-	want := []string{")a", "b)c", "d"}
+	want := []string{`$(echo "foo")`}
 	if len(out) != len(want) {
 		t.Fatalf("unexpected token count: got %d, want %d, out=%#v", len(out), len(want), out)
 	}
-	for i := range want {
-		if out[i] != want[i] {
-			t.Fatalf("unexpected token at %d: got %q, want %q, out=%#v", i, out[i], want[i], out)
-		}
+	if out[0] != want[0] {
+		t.Fatalf("unexpected output: got %q, want %q", out[0], want[0])
 	}
 }
 
+// If true → $(...) continues to work with ParseBacktick=true
 func TestValidDollarCommandSubstitutionStillWorks(t *testing.T) {
 	p := NewParser()
 	p.ParseBacktick = true
@@ -82,6 +80,7 @@ func TestValidDollarCommandSubstitutionStillWorks(t *testing.T) {
 	}
 }
 
+// Error → Unclosed $(
 func TestUnclosedDollarCommandSubstitutionReturnsError(t *testing.T) {
 	p := NewParser()
 	p.ParseBacktick = true
@@ -90,4 +89,4 @@ func TestUnclosedDollarCommandSubstitutionReturnsError(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected error for unclosed command substitution, got nil")
 	}
-}
+}
