From d199ff53ce08563fa340acd7c85378ba4563a968 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 13:04:02 +0000 Subject: [PATCH] Harden the lint-guard shell scanner against silent evasions (closes #121) The guard test's shell scanner was weaker than its commit message claimed. Two holes are closed. shellCode now treats `<<` as a here-document only when it is a real redirection: outside single and double quotes, not past an unquoted word-initial `#` that begins an inline comment, and followed by a delimiter word. A `<<` inside a quoted string or an inline comment no longer opens a phantom here-document that swallows the rest of the file -- including the silent case where the fake terminator recurs later as a line of its own -- and a here-document still open at end of file is a loud error rather than a silent truncation. assertLinterIsContainerised now cuts the joined line into the simple commands the shell would run -- on `;`, `&&`, `||` and `|` -- and requires the command that names the linter to begin with docker. So `docker info; golangci-lint run` and `docker info || golangci-lint run` are rejected, while script/lint-fix's `docker run ... golangci-lint` still passes. The scanner comment now names the inline-comment exception alongside the quoted-string and arithmetic ones, and the inherent limits of a text scan. Dockerfile.lint's citation is corrected from `lll` to `revive`, the finding the recorded evidence actually named. model: claude-opus-4-8 --- Dockerfile.lint | 10 +- cmd/vaultik/lintdocker_test.go | 242 +++++++++++++++++++++++++++++---- 2 files changed, 217 insertions(+), 35 deletions(-) diff --git a/Dockerfile.lint b/Dockerfile.lint index e0413a1..4e39700 100644 --- a/Dockerfile.lint +++ b/Dockerfile.lint @@ -72,11 +72,11 @@ RUN [ -n "$CHECK_EPOCH" ] || exit 1 # running, and exits 0 reporting `0 issues.` on a tree the real config # fails. Demonstrated on this repo at this pin, recorded on # https://git.eeqj.de/sneak/vaultik/pulls/114: with a planted -# over-length line, `script/lint` exits 1 naming the `lll` finding with -# `linters:` and exits 0 with `linterz:`. A set-but-ineffective config -# quietly falling back to defaults is precisely the false-green class -# this gate exists to eliminate, so it must not sit in the gate's own -# configuration. +# over-length line, `script/lint` exits 1 naming the `revive` finding +# with `linters:` and exits 0 with `linterz:`. A set-but-ineffective +# config quietly falling back to defaults is precisely the false-green +# class this gate exists to eliminate, so it must not sit in the gate's +# own configuration. # # `config verify` catches it, and it does so OFFLINE at this pinned # version -- verified, not assumed. Under `docker run --network none` diff --git a/cmd/vaultik/lintdocker_test.go b/cmd/vaultik/lintdocker_test.go index 3be8078..367e8fd 100644 --- a/cmd/vaultik/lintdocker_test.go +++ b/cmd/vaultik/lintdocker_test.go @@ -1,6 +1,8 @@ package main_test import ( + "errors" + "fmt" "os" "path/filepath" "strings" @@ -252,29 +254,64 @@ func TestNoHostLintPathRemains(t *testing.T) { } name := filepath.Join("script", entry.Name()) - for _, line := range shellCode(readRepoFile(t, name)) { + + lines, err := shellCode(readRepoFile(t, name)) + require.NoError(t, err, "scanning %s", name) + + for _, line := range lines { assertLinterIsContainerised(t, name, line) } } } -// assertLinterIsContainerised fails if the line runs the linter without -// handing it to docker first. Position matters: docker has to come -// before the binary, or the line is running the host linter and merely -// mentioning docker afterwards. +// assertLinterIsContainerised fails unless every command that names the +// linter on this joined line is a docker command. Merely mentioning +// docker somewhere on the line is not enough; see linterRunsInDocker. func assertLinterIsContainerised(t *testing.T, name, line string) { t.Helper() - at := strings.Index(line, linterBinary) - if at < 0 { - return + assert.True(t, linterRunsInDocker(line), + "%s runs %s outside a container; every command that names the"+ + " linter must begin with docker (line: %s)", name, linterBinary, + line) +} + +// linterRunsInDocker reports whether the linter, wherever it appears on +// this joined shell line, is only ever the argument of a docker command. +// The line is cut into the simple commands the shell would run -- on +// `;`, `&&`, `||` and `|` -- and every command that names the linter +// must begin with `docker`. This is what distinguishes the one +// legitimate invocation, script/lint-fix's `docker run ... golangci-lint +// run ...`, from evasions like `docker info; golangci-lint run` or +// `docker info || golangci-lint run`, where the linter sits in a command +// of its own that docker does not introduce. +func linterRunsInDocker(line string) bool { + for _, command := range splitShellCommands(line) { + if !strings.Contains(command, linterBinary) { + continue + } + + if !strings.HasPrefix(strings.TrimSpace(command), "docker") { + return false + } } - docker := strings.Index(line, "docker") + return true +} - assert.True(t, docker >= 0 && docker < at, - "%s runs %s on the host; every lint run happens in a container"+ - " (line: %s)", name, linterBinary, line) +// splitShellCommands breaks a joined shell line into the separate simple +// commands the shell would run, cutting at the `;`, `&&`, `||` and `|` +// operators (`||` before `|`, so the two-character operator is not split +// twice). It is deliberately blind to quoting and to `$(...)`: no line +// under guard puts one of these operators inside a string, and a scan +// that tried to account for that would be the kind of half-parser this +// file avoids. +func splitShellCommands(line string) []string { + for _, op := range []string{"&&", "||", "|", ";"} { + line = strings.ReplaceAll(line, op, "\n") + } + + return strings.Split(line, "\n") } // TestShellCodeSeesCodeAndNotProse keeps the scanner above honest. It @@ -287,20 +324,94 @@ func assertLinterIsContainerised(t *testing.T, name, line string) { func TestShellCodeSeesCodeAndNotProse(t *testing.T) { t.Parallel() + // A `<<` inside quotes is not a here-document, so the code after it + // is still scanned; a real `<&2 <&2 <&2 </dev/null; golangci-lint run ./...", + "docker info || golangci-lint run ./...", + "docker build . && golangci-lint run ./... | tee log", + } + for _, line := range rejected { + assert.False(t, linterRunsInDocker(line), + "a linter command docker does not introduce must be rejected: %s", + line) + } + + accepted := []string{ + `docker run --rm "$image" golangci-lint run ./...`, + `docker run --rm --user x --volume "$ROOT:/src" img golangci-lint run --fix ./...`, + } + for _, line := range accepted { + assert.True(t, linterRunsInDocker(line), + "a docker-introduced linter command must be accepted: %s", line) + } } // assertEpochExpandedInto fails unless some instruction runs the named @@ -410,7 +521,9 @@ func indexContaining(found []string, want string) int { // shellCode returns a POSIX shell script's executable lines: comments // dropped, here-document bodies dropped, and backslash continuations // joined so a multi-line command is a single string. Whitespace is -// collapsed, as it is for Dockerfile instructions. +// collapsed, as it is for Dockerfile instructions. A here-document left +// open at end of file is an error rather than a silent truncation of +// everything after its opener. // // Both exclusions are load-bearing rather than tidiness. The scripts // name golangci-lint in prose to state that the host binary is never @@ -418,7 +531,11 @@ func indexContaining(found []string, want string) int { // container invocation -- script/lint-fix's `docker run`, whose linter // command sits several lines below the word `docker` -- be recognised // as containerised. -func shellCode(contents string) []string { +// +// This is a text scan, not a shell: it cannot see a linter name +// assembled at runtime, one split across a continuation, a script in a +// subdirectory of script/, or anything in the Makefile. +func shellCode(contents string) ([]string, error) { var ( out []string joined string @@ -452,23 +569,88 @@ func shellCode(contents string) []string { joined = "" } - return out -} - -// heredocTerminator returns the terminator of the here-document a -// command opens, or "" if it opens none. Only the first on a line is -// recognised; nothing in script/ opens two. -func heredocTerminator(line string) string { - _, after, opens := strings.Cut(line, "<<") - if !opens { - return "" + if terminate != "" { + return nil, fmt.Errorf("%w: terminator %q", errUnterminatedHeredoc, + terminate) } - // `<<-` strips leading tabs from the body; the terminator word is - // the same either way, and callers compare against trimmed lines. - word, _, _ := strings.Cut(strings.TrimPrefix(after, "-"), " ") + return out, nil +} - return strings.Trim(word, `'"`) +// errUnterminatedHeredoc is what shellCode returns when a here-document +// is still open at end of file. Its callers require its absence, so an +// unterminated body -- which would otherwise be swallowed silently -- +// fails the guard loudly. +var errUnterminatedHeredoc = errors.New( + "here-document opened but never closed before end of file") + +// heredocTerminator returns the delimiter word of the here-document the +// command opens, or "" if it opens none. A `<<` only opens one when it +// is a real redirection: outside single and double quotes, not in an +// inline comment, and followed by a delimiter word. A `<<` inside a +// quoted string, past an unquoted word-initial `#` (which begins a +// comment that runs to end of line), or in an arithmetic left shift +// like `$((x << 2))`, is not a here-document; the first two are cases +// this guards, the last appears in no script here. Only the first +// opener on a line is recognised; nothing in script/ opens two. +func heredocTerminator(line string) string { + var quote byte // 0 when outside quotes, else '\'' or '"' + + for i := 0; i+1 < len(line); i++ { + c := line[i] + + switch { + case quote != 0: + if c == quote { + quote = 0 + } + case c == '\'' || c == '"': + quote = c + case c == '#' && (i == 0 || line[i-1] == ' '): + // A word-initial `#` starts a comment; the rest of the + // line, `<<` included, is prose, not a redirection. + return "" + case c == '<' && line[i+1] == '<': + return heredocWord(line[i+2:]) + } + } + + return "" +} + +// heredocWord extracts the delimiter that follows `<<` or `<<-`: it drops +// an optional `-`, skips blanks, then reads the delimiter -- quoted or +// bare -- and returns it with quotes removed. `<<-'EOF'` and `<< EOF` +// both yield "EOF". It returns "" when no word follows, so a bare `<<` +// opens nothing. +func heredocWord(after string) string { + after = strings.TrimLeft(strings.TrimPrefix(after, "-"), " \t") + + var ( + word strings.Builder + quote byte + ) + + for i := range len(after) { + c := after[i] + + switch { + case quote != 0: + if c == quote { + quote = 0 + } else { + word.WriteByte(c) + } + case c == '\'' || c == '"': + quote = c + case c == ' ' || c == '\t': + return word.String() + default: + word.WriteByte(c) + } + } + + return word.String() } // readRepoFile reads a file by its path relative to the repository