diff --git a/go.mod b/go.mod index a327f6f8..f8562939 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( golang.org/x/term v0.41.0 golang.org/x/text v0.35.0 gopkg.in/yaml.v3 v3.0.1 - mvdan.cc/sh/v3 v3.13.1-0.20260321230027-916eaf543095 + mvdan.cc/sh/v3 v3.13.1-0.20260326224822-2315483a6fdb ) require ( diff --git a/go.sum b/go.sum index d130ec1a..ee34e5c4 100644 --- a/go.sum +++ b/go.sum @@ -573,8 +573,8 @@ honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg= -mvdan.cc/sh/v3 v3.13.1-0.20260321230027-916eaf543095 h1:i5YnBI5NNESfNLCSYaZDJ3QaEwYywhhI8eoe+4NYTdw= -mvdan.cc/sh/v3 v3.13.1-0.20260321230027-916eaf543095/go.mod h1:KV1GByGPc/Ho0X1E6Uz9euhsIQEj4hwyKnodLlFLoDM= +mvdan.cc/sh/v3 v3.13.1-0.20260326224822-2315483a6fdb h1:QUrsjLC1hh5rPS8QXCOeNhHT54kXobzwlCDVChL/RoI= +mvdan.cc/sh/v3 v3.13.1-0.20260326224822-2315483a6fdb/go.mod h1:lXJ8SexMvEVcHCoDvAGLZgFJ9Wsm2sulmoNEXGhYZD0= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= diff --git a/internal/rules/ast_fallback_test.go b/internal/rules/ast_fallback_test.go index 0064541a..f78f0519 100644 --- a/internal/rules/ast_fallback_test.go +++ b/internal/rules/ast_fallback_test.go @@ -27,7 +27,7 @@ func TestASTFallbackEscapes(t *testing.T) { { name: "trailing backslash with U+FFFD", cmd: "cat /etc/\ufffd\\", - wantPaths: []string{"/etc/\ufffd"}, + wantPaths: []string{"/etc/\ufffd\\"}, // interpreter preserves escaped backslash }, { name: "escaped backslash in path with background", @@ -356,9 +356,12 @@ func TestCategoryBRedirectExtraction(t *testing.T) { } // ============================================================================= -// TestCategoryABuiltinPanics: verifies that newly discovered interpreter panics -// (shopt -p/-q, nameref array append) are caught by nodeHasUnsafe and paths -// are still extracted through the AST fallback path. +// TestCategoryABuiltinPanics: verifies that interpreter panics still present +// upstream (shopt -p/-q) are caught by nodeHasUnsafe and paths are still +// extracted through the AST fallback path. +// +// Nameref (declare -n) panics were fixed in mvdan.cc/sh v3.13.1-0.20260326 +// and no longer need nodeHasUnsafe guards — they're caught by defer/recover. // ============================================================================= func TestCategoryABuiltinPanics(t *testing.T) { @@ -383,22 +386,10 @@ func TestCategoryABuiltinPanics(t *testing.T) { t.Error("nodeHasUnsafe should NOT flag shopt -s") } }) - t.Run("nodeHasUnsafe detects declare -n", func(t *testing.T) { + t.Run("nodeHasUnsafe allows declare -n (fixed upstream)", func(t *testing.T) { file, _ := parser.Parse(strings.NewReader("declare -n ref=arr"), "") - if !nodeHasUnsafe(file) { - t.Error("nodeHasUnsafe should flag declare -n") - } - }) - t.Run("nodeHasUnsafe detects declare -rn", func(t *testing.T) { - file, _ := parser.Parse(strings.NewReader("declare -rn ref=arr"), "") - if !nodeHasUnsafe(file) { - t.Error("nodeHasUnsafe should flag declare -rn") - } - }) - t.Run("nodeHasUnsafe detects local -n", func(t *testing.T) { - file, _ := parser.Parse(strings.NewReader("local -n ref=arr"), "") - if !nodeHasUnsafe(file) { - t.Error("nodeHasUnsafe should flag local -n") + if nodeHasUnsafe(file) { + t.Error("nodeHasUnsafe should NOT flag declare -n (fixed upstream)") } }) t.Run("nodeHasUnsafe allows declare -a", func(t *testing.T) { @@ -408,7 +399,7 @@ func TestCategoryABuiltinPanics(t *testing.T) { } }) - // Bypass hardening: combined flags, command prefix, quoted flags + // Bypass hardening: combined flags, command prefix t.Run("nodeHasUnsafe detects shopt -sp (combined)", func(t *testing.T) { file, _ := parser.Parse(strings.NewReader("shopt -sp extglob"), "") if !nodeHasUnsafe(file) { @@ -427,12 +418,6 @@ func TestCategoryABuiltinPanics(t *testing.T) { t.Error("nodeHasUnsafe should flag builtin shopt -q") } }) - t.Run("nodeHasUnsafe detects declare quoted -n", func(t *testing.T) { - file, _ := parser.Parse(strings.NewReader(`declare "-n" ref=arr`), "") - if !nodeHasUnsafe(file) { - t.Error("nodeHasUnsafe should flag declare with quoted -n") - } - }) t.Run("nodeHasUnsafe allows command shopt -s", func(t *testing.T) { file, _ := parser.Parse(strings.NewReader("command shopt -s extglob"), "") if nodeHasUnsafe(file) { @@ -440,6 +425,39 @@ func TestCategoryABuiltinPanics(t *testing.T) { } }) + // test -N / [ -N / [[ -N: panics with "unhandled unary test op: -N" + t.Run("nodeHasUnsafe detects test -N", func(t *testing.T) { + file, _ := parser.Parse(strings.NewReader("test -N /tmp"), "") + if !nodeHasUnsafe(file) { + t.Error("nodeHasUnsafe should flag test -N") + } + }) + t.Run("nodeHasUnsafe detects [ -N ]", func(t *testing.T) { + file, _ := parser.Parse(strings.NewReader("[ -N /tmp ]"), "") + if !nodeHasUnsafe(file) { + t.Error("nodeHasUnsafe should flag [ -N ]") + } + }) + t.Run("nodeHasUnsafe detects [[ -N ]]", func(t *testing.T) { + file, _ := parser.Parse(strings.NewReader("[[ -N /tmp ]]"), "") + if !nodeHasUnsafe(file) { + t.Error("nodeHasUnsafe should flag [[ -N ]]") + } + }) + t.Run("nodeHasUnsafe allows [[ -f ]]", func(t *testing.T) { + file, _ := parser.Parse(strings.NewReader("[[ -f /tmp ]]"), "") + if nodeHasUnsafe(file) { + t.Error("nodeHasUnsafe should NOT flag [[ -f ]]") + } + }) + t.Run("test -N with path extraction", func(t *testing.T) { + args, _ := json.Marshal(map[string]string{"command": "test -N /etc/passwd && cat /etc/shadow"}) + info := ext.Extract("Bash", json.RawMessage(args)) + if !slices.Contains(info.Paths, "/etc/shadow") { + t.Errorf("path /etc/shadow not found in %v", info.Paths) + } + }) + // E2e: paths still extracted through AST fallback t.Run("shopt -p with path extraction", func(t *testing.T) { args, _ := json.Marshal(map[string]string{"command": "shopt -p; cat /etc/shadow"}) @@ -456,16 +474,17 @@ func TestCategoryABuiltinPanics(t *testing.T) { } }) - // Verify interpreter does NOT panic (guarded by nodeHasUnsafe) + // shopt -p: guarded by nodeHasUnsafe (still panics upstream) t.Run("shopt -p does not reach interpreter", func(t *testing.T) { args, _ := json.Marshal(map[string]string{"command": "shopt -p"}) info := ext.Extract("Bash", json.RawMessage(args)) _ = info // must not panic }) - t.Run("nameref array append does not reach interpreter", func(t *testing.T) { + // nameref: fixed upstream, reaches interpreter safely now + t.Run("nameref array append handled by interpreter", func(t *testing.T) { args, _ := json.Marshal(map[string]string{"command": "declare -n ref=arr; ref+=(x)"}) info := ext.Extract("Bash", json.RawMessage(args)) - _ = info // must not panic + _ = info // must not panic (fixed in mvdan.cc/sh v3.13.1-0.20260326) }) } diff --git a/internal/rules/extractor_shell.go b/internal/rules/extractor_shell.go index c651bd08..bb61ec68 100644 --- a/internal/rules/extractor_shell.go +++ b/internal/rules/extractor_shell.go @@ -1861,15 +1861,22 @@ func wordHasExpansion(w *syntax.Word) bool { // Two categories of guards: // // Category A — CRASH / HANG (must skip interpreter entirely): -// - Background (cmd &): spawns goroutines where pattern.go panics are -// unrecoverable via defer/recover +// +// - Background (cmd &): spawns goroutines where panics are unrecoverable +// via defer/recover +// // - CoprocClause: same goroutine issue +// // - ProcSubst >(cmd)/<(cmd): hangs in dry-run (FIFO deadlock) -// - Lit with U+FFFD, control chars, or non-ASCII globs: crashes -// regexp.MustCompile in pattern/pattern.go (unfixed upstream) +// // - shopt -p / shopt -q: panics "unhandled shopt flag" (builtin.go:795) -// - declare/local/typeset -n (nameref): panics on subsequent array append -// ref+=(x) with "unhandled conversion of kind" (vars.go:423) +// +// Previously guarded but fixed upstream (v3.13.1-0.20260326): +// +// - Non-ASCII globs, U+FFFD, control chars in Lit (pattern.go panic) +// +// - declare -n (nameref) + array append (vars.go:423 panic) +// These are now caught by the general defer/recover in runShellFileInterp. // // Category B — UNSUPPORTED REDIRECT (no crash, but command not executed): // - fd >= 3 redirects, fd dup (DplIn/DplOut), RdrClob, RdrInOut, etc. @@ -1899,19 +1906,14 @@ func nodeHasUnsafe(root syntax.Node) bool { case *syntax.ProcSubst: found = true return false - case *syntax.Lit: - if litHasUnsafeChars(n.Value) { - found = true - return false - } case *syntax.CallExpr: if callHasUnsafeBuiltin(n) { found = true return false } - case *syntax.DeclClause: - if declHasUnsafeNameref(n) { + case *syntax.TestClause: + if testClauseHasUnsafeOp(n) { found = true return false } @@ -1929,34 +1931,10 @@ func nodeHasUnsafe(root syntax.Node) bool { return found } -// litHasUnsafeChars returns true if the literal value contains characters that -// crash the interpreter's glob expansion (pattern/pattern.go bugs, unfixed). -func litHasUnsafeChars(s string) bool { - // U+FFFD crashes regexp.MustCompile during glob expansion. - if strings.ContainsRune(s, '\uFFFD') { - return true - } - // Control characters (except \t, \n, \r) crash glob expansion in pipes. - for _, r := range s { - if r < 0x20 && r != '\t' && r != '\n' && r != '\r' { - return true - } - } - // Non-ASCII bytes in glob patterns crash regexp.MustCompile: the - // glob-to-regex converter splits multi-byte UTF-8 into invalid sequences. - if strings.ContainsAny(s, "*?[") { - for _, b := range []byte(s) { - if b > 0x7F { - return true - } - } - } - return false -} - // callHasUnsafeBuiltin returns true if a CallExpr invokes a builtin that // panics in the interpreter. Currently detects: // - shopt -p / shopt -q: panics with "unhandled shopt flag" (builtin.go:795) +// - test -N / [ -N: panics with "unhandled unary test op: -N" (test.go:215) // // Checks all arg positions for the command name to handle "command shopt -p" // and "builtin shopt -p". Also detects combined flags like "-sp". @@ -1967,59 +1945,57 @@ func callHasUnsafeBuiltin(call *syntax.CallExpr) bool { if len(call.Args) < 2 { return false } - // Find "shopt" in any position to handle "command shopt" / "builtin shopt". - shoptIdx := -1 - for i, arg := range call.Args { - v := wordToLiteral(arg) - if v == "shopt" { - shoptIdx = i - break - } - // Stop scanning after the first non-prefix command (command/builtin). + + // Resolve command name past "command" / "builtin" prefixes. + cmdIdx := 0 + for cmdIdx < len(call.Args) { + v := wordToLiteral(call.Args[cmdIdx]) if v != "command" && v != "builtin" { break } + cmdIdx++ } - if shoptIdx < 0 { + if cmdIdx >= len(call.Args) { return false } - for _, arg := range call.Args[shoptIdx+1:] { - v := wordToLiteral(arg) - // Check for -p, -q, or combined flags containing p/q (e.g., -sp, -qo). - if strings.HasPrefix(v, "-") && (strings.ContainsAny(v, "pq")) { - return true + cmd := wordToLiteral(call.Args[cmdIdx]) + args := call.Args[cmdIdx+1:] + + switch cmd { + case "shopt": + for _, arg := range args { + v := wordToLiteral(arg) + if strings.HasPrefix(v, "-") && strings.ContainsAny(v, "pq") { + return true + } + } + case "test", "[": + for _, arg := range args { + if wordToLiteral(arg) == "-N" { + return true + } } } return false } -// declHasUnsafeNameref returns true if a DeclClause uses -n (nameref), which -// can panic on subsequent array append (ref+=(x)) due to an unhandled Kind in -// assignVal (vars.go:423). Conservative: flags the declare -n itself since the -// panic requires a separate statement (ref+=(x)) that we can't link statically. -// -// Handles bare (-n), combined (-rn), and quoted ("-n") flags. -// Bypasses via variable indirection (declare $F) or eval are caught by the -// defer/recover in runShellFileInterp. -func declHasUnsafeNameref(decl *syntax.DeclClause) bool { - if decl.Variant == nil { - return false - } - v := decl.Variant.Value - if v != "declare" && v != "typeset" && v != "local" { - return false - } - for _, assign := range decl.Args { - if assign.Name != nil { - continue // this is a name=value, not a flag +// testClauseHasUnsafeOp returns true if a [[ ]] test clause uses a unary +// operator that panics in the interpreter. +// - -N: panics with "unhandled unary test op: -N" (test.go:215) +func testClauseHasUnsafeOp(tc *syntax.TestClause) bool { + unsafe := false + syntax.Walk(tc, func(node syntax.Node) bool { + if unsafe { + return false } - // Use wordToLiteral to handle both bare and quoted flags. - w := wordToLiteral(assign.Value) - if strings.HasPrefix(w, "-") && strings.ContainsRune(w, 'n') { - return true + ue, ok := node.(*syntax.UnaryTest) + if ok && ue.Op == syntax.TsModif { + unsafe = true + return false } - } - return false + return true + }) + return unsafe } // interpSupportsRedirect returns true if the mvdan.cc/sh interpreter can