diff --git a/cmd/shell-allowlist/changed.go b/cmd/shell-allowlist/changed.go index 5969da45..1e68d0c6 100644 --- a/cmd/shell-allowlist/changed.go +++ b/cmd/shell-allowlist/changed.go @@ -8,10 +8,12 @@ import ( "fmt" "os" "os/exec" + "regexp" + "strconv" "strings" ) -// changedFiles selects whole scripts relative to the merge base, including local edits. +// changedFiles selects the scripts that differ from the merge base, including local edits. func changedFiles(base string) ([]string, error) { mergeBase, err := gitOutput("merge-base", base, "HEAD") if err != nil { @@ -73,3 +75,65 @@ func gitOutput(args ...string) (string, error) { } return string(out), nil } + +var hunkHeader = regexp.MustCompile(`^@@ -\S+ \+(\d+)(?:,(\d+))? @@`) + +// addedLines maps each tracked path to the line numbers it adds relative to the merge base. +// An untracked path maps to nil because every line of a new file counts as added. +func addedLines(base string, paths []string) (map[string]map[uint]bool, error) { + mergeBase, err := gitOutput("merge-base", base, "HEAD") + if err != nil { + return nil, err + } + untracked, err := gitOutput("ls-files", "--others", "--exclude-standard", "-z") + if err != nil { + return nil, err + } + isUntracked := map[string]bool{} + for path := range strings.SplitSeq(untracked, "\x00") { + isUntracked[path] = true + } + mergeBase = strings.TrimSpace(mergeBase) + diffArgs := []string{"diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", "--text"} + added := map[string]map[uint]bool{} + for _, path := range paths { + if isUntracked[path] { + added[path] = nil + continue + } + var diff string + if diff, err = gitOutput(append(diffArgs, mergeBase, "--", path)...); err != nil { + return nil, err + } + if added[path], err = parseAdded(diff); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + } + return added, nil +} + +// parseAdded reads the new-side line ranges from the hunk headers of a zero-context diff. +// A header it can't read fails the check rather than shrinking the judged lines. +func parseAdded(diff string) (map[uint]bool, error) { + lines := map[uint]bool{} + for line := range strings.SplitSeq(diff, "\n") { + match := hunkHeader.FindStringSubmatch(line) + if match == nil { + continue + } + start, err := strconv.ParseUint(match[1], 10, 32) + if err != nil { + return nil, fmt.Errorf("reading diff hunk %q: %w", line, err) + } + count := uint64(1) + if match[2] != "" { + if count, err = strconv.ParseUint(match[2], 10, 32); err != nil { + return nil, fmt.Errorf("reading diff hunk %q: %w", line, err) + } + } + for n := range count { + lines[uint(start+n)] = true + } + } + return lines, nil +} diff --git a/cmd/shell-allowlist/changed_test.go b/cmd/shell-allowlist/changed_test.go index 73461a41..e79580f9 100644 --- a/cmd/shell-allowlist/changed_test.go +++ b/cmd/shell-allowlist/changed_test.go @@ -75,6 +75,102 @@ func TestChangedScripts(t *testing.T) { assert.Equal(t, ".claude/skills/local/scripts/run.sh:1: ruby: banned\n", out.String()) } +func TestChangedLinesOnly(t *testing.T) { + root, err := filepath.Abs("../..") + require.NoError(t, err) + dir := t.TempDir() + gitTest(t, "clone", "--quiet", "--shared", "--no-checkout", root, dir) + t.Chdir(dir) + gitTest(t, "read-tree", "HEAD") + write := func(path, body string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + } + write(".devtools/shell-allowlist.toml", "commands = ['printf']\n") + write("lib.sh", "ruby old\nprintf one\nprintf two\n") + write("cont.sh", "print\\\nf one\n") + write("raw.sh", "printf ok\n") + write(".gitattributes", "raw.sh -diff\n") + gitTest(t, "add", "--", ".devtools/shell-allowlist.toml", "lib.sh", "cont.sh", "raw.sh", ".gitattributes") + gitTest(t, "-c", "user.name=t", "-c", "user.email=t@example.test", "-c", "commit.gpgsign=false", + "commit", "--quiet", "--no-verify", "-m", "base") + gitTest(t, "update-ref", "refs/remotes/origin/main", "HEAD") + check := func() (int, string) { + var out bytes.Buffer + status := Run([]string{"--changed", "origin/main"}, &out) + return status, out.String() + } + + // A finding on a line this change didn't add isn't judged. + write("lib.sh", "ruby old\nprintf one\nprintf two\nprintf added\n") + status, out := check() + assert.Equal(t, 0, status) + assert.Empty(t, out) + + // A banned command on an added line fails, and the old finding isn't reported. + write("lib.sh", "ruby old\nprintf one\nperl added\nprintf two\n") + status, out = check() + assert.Equal(t, 1, status) + assert.Equal(t, "lib.sh:3: perl: banned\n", out) + + // Editing the line with the old finding makes it an added line. + write("lib.sh", "ruby edited\nprintf one\nprintf two\n") + status, out = check() + assert.Equal(t, 1, status) + assert.Equal(t, "lib.sh:1: ruby: banned\n", out) + + // A pure deletion adds nothing to judge. + write("lib.sh", "ruby old\nprintf two\n") + status, out = check() + assert.Equal(t, 0, status) + assert.Empty(t, out) + + // Editing only a continuation line changes the command word that starts on a preceding line. + write("cont.sh", "print\\\ng one\n") + status, out = check() + assert.Equal(t, 1, status) + assert.Equal(t, "cont.sh:1: print"+"g: not on the allow list\n", out) + write("cont.sh", "print\\\nf one\n") + + // A -diff attribute doesn't hide an added line from the judge. + write("raw.sh", "printf ok\nruby added\n") + status, out = check() + assert.Equal(t, 1, status) + assert.Equal(t, "raw.sh:2: ruby: banned\n", out) + write("raw.sh", "printf ok\n") + + // Text that resembles binary diff markers is an ordinary added or deleted line. + write("raw.sh", "printf ok\n# GIT binary patch\nprintf 'Binary files changed'\n") + status, out = check() + assert.Equal(t, 0, status) + assert.Empty(t, out) + write("raw.sh", "printf ok\n") + status, out = check() + assert.Equal(t, 0, status) + assert.Empty(t, out) + + // An untracked script counts in full. + write("lib.sh", "ruby old\nprintf one\nprintf two\n") + write("new.sh", "printf ok\nruby new\n") + status, out = check() + assert.Equal(t, 1, status) + assert.Equal(t, "new.sh:2: ruby: banned\n", out) +} + +func TestParseAdded(t *testing.T) { + t.Parallel() + diff := "diff --git a/x.sh b/x.sh\n@@ -1,2 +1 @@\n@@ -5 +4,0 @@\n@@ -9,0 +8,3 @@\n@@ -20 +22 @@\n" + got, err := parseAdded(diff) + require.NoError(t, err) + assert.Equal(t, map[uint]bool{1: true, 8: true, 9: true, 10: true, 22: true}, got) + got, err = parseAdded("") + require.NoError(t, err) + assert.Empty(t, got) + _, err = parseAdded("@@ -1 +99999999999 @@\n") + require.Error(t, err) +} + func TestShellAllowlistGateWiring(t *testing.T) { t.Chdir("../..") data, err := os.ReadFile("mise.toml") diff --git a/cmd/shell-allowlist/run.go b/cmd/shell-allowlist/run.go index bbce7f91..d98f3caa 100644 --- a/cmd/shell-allowlist/run.go +++ b/cmd/shell-allowlist/run.go @@ -12,17 +12,23 @@ import ( "github.com/tbhb-dev/devtools/internal/shell" ) -// Run checks the given paths, or the scripts changed since a base ref when the arguments start with the changed-files flag. +// Run checks the given paths, or the lines added to scripts since a base ref when the arguments start with the changed-files flag. func Run(args []string, out io.Writer) int { if len(args) == 1 && args[0] == "--summary" { return summary(out) } + // A missing entry, or a nil one, means every line of the file counts. + var added map[string]map[uint]bool if len(args) == 2 && args[0] == "--changed" { paths, err := changedFiles(args[1]) if err != nil { fmt.Fprintln(out, err) return 1 } + if added, err = addedLines(args[1], paths); err != nil { + fmt.Fprintln(out, err) + return 1 + } args = paths } allowed, err := readAllowlist() @@ -32,19 +38,20 @@ func Run(args []string, out io.Writer) int { } status := 0 for _, path := range args { - if !checkFile(path, allowed, out) { + if !checkFile(path, allowed, added[path], out) { status = 1 } } return status } -func checkFile(path string, allowed *shell.Allowlist, out io.Writer) bool { - violations, failed := scanFile(path, allowed, out) +func checkFile(path string, allowed *shell.Allowlist, only map[uint]bool, out io.Writer) bool { + violations, failed := scanFile(path, allowed, only, out) return violations == 0 && !failed } -func scanFile(path string, allowed *shell.Allowlist, out io.Writer) (int, bool) { +// scanFile reports findings on the lines in only, or on every line when only is nil. +func scanFile(path string, allowed *shell.Allowlist, only map[uint]bool, out io.Writer) (int, bool) { //nolint:gosec // G703: reading the caller's exact file paths is this command's purpose. data, err := os.ReadFile(path) if err != nil { @@ -57,8 +64,24 @@ func scanFile(path string, allowed *shell.Allowlist, out io.Writer) (int, bool) fmt.Fprintf(out, "%s: invalid shell syntax\n", path) return 0, true } + reported := 0 for _, finding := range findings { + if only != nil && !touches(only, finding) { + continue + } fmt.Fprintln(out, finding.String()) + reported++ + } + return reported, false +} + +// touches reports whether an added line falls anywhere in the finding's command word, so +// an edit to a continuation line of a backslash-continued word still counts. +func touches(added map[uint]bool, finding shell.Finding) bool { + for line := finding.Line; line <= finding.EndLine; line++ { + if added[line] { + return true + } } - return len(findings), false + return false } diff --git a/cmd/shell-allowlist/summary.go b/cmd/shell-allowlist/summary.go index 2749d258..acf3d9ce 100644 --- a/cmd/shell-allowlist/summary.go +++ b/cmd/shell-allowlist/summary.go @@ -32,7 +32,7 @@ func summary(out io.Writer) int { total, errors := 0, 0 escape := strings.NewReplacer("&", "&", "<", "<", ">", ">", "|", "|", "\n", " ", "\r", " ") for _, path := range paths { - count, failed := scanFile(path, allowed, io.Discard) + count, failed := scanFile(path, allowed, nil, io.Discard) analysisErrors := 0 if failed { analysisErrors = 1 diff --git a/docs/shell-allowlist.md b/docs/shell-allowlist.md index a417e0f1..d14b0f1f 100644 --- a/docs/shell-allowlist.md +++ b/docs/shell-allowlist.md @@ -31,3 +31,7 @@ Unsupported escape forms and expanding words produce findings. Name checks can miss tools that a permitted wrapper invokes. Don't treat this check as an execution sandbox. + +With `--changed BASE`, the command judges only the lines a change adds. It reads the added line ranges from a zero-context diff against the merge base and reports a finding only when its line is one of them. A script with no committed history counts in full. + +Editing an existing script no longer fails on commands the script already ran before the edit. A changed line that runs a banned command still fails. diff --git a/internal/shell/shell.go b/internal/shell/shell.go index d1d6d0ce..08bd4289 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -56,8 +56,10 @@ func isBanned(name string) bool { // Finding is one reported command. type Finding struct { - File string - Line uint + File string + Line uint + // EndLine is the last line of the command word, which differs from Line when a backslash continues it. + EndLine uint Command string Kind Kind } @@ -210,7 +212,7 @@ func (c *checker) call(call *syntax.CallExpr, defined map[string]bool) { kind = NotAllowed } if kind != "" { - c.findings = append(c.findings, Finding{c.file, word.Pos().Line(), name, kind}) + c.findings = append(c.findings, Finding{c.file, word.Pos().Line(), word.End().Line(), name, kind}) } if !literal || defined[name] { clear(defined) diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index 79476050..c92aea9b 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -94,8 +94,8 @@ func TestNotAllowed(t *testing.T) { t.Parallel() got := check(t, repoList(t), "git status\nbash -c true\nrsync a b\n") assert.Equal(t, []shell.Finding{ - {File: "script.sh", Line: 2, Command: "bash", Kind: shell.NotAllowed}, - {File: "script.sh", Line: 3, Command: "rsync", Kind: shell.NotAllowed}, + {File: "script.sh", Line: 2, EndLine: 2, Command: "bash", Kind: shell.NotAllowed}, + {File: "script.sh", Line: 3, EndLine: 3, Command: "rsync", Kind: shell.NotAllowed}, }, got) assert.Equal(t, "script.sh:2: bash: not on the allow list", got[0].String()) } @@ -121,7 +121,7 @@ func TestBannedNames(t *testing.T) { require.Len(t, got, 1, line) assert.Equal( t, - shell.Finding{File: "script.sh", Line: 1, Command: name, Kind: shell.Banned}, + shell.Finding{File: "script.sh", Line: 1, EndLine: 1, Command: name, Kind: shell.Banned}, got[0], line, ) @@ -133,7 +133,7 @@ func TestBannedHeredocInput(t *testing.T) { src := "python3 <