Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ the same version in lockstep.

### Fixed

- **CLI `scan` rejected the `./...` path every doc example uses**, failing with
`scan failed: lstat ./...: no such file or directory`. The scan has always
been recursive, so the pattern suffix is now trimmed and `./pkg/...` selects
exactly what `./pkg` does.
- **CLI `explain` could not connect to any database**: the binary linked no
SQL driver, so every invocation failed with
`sql: unknown driver "postgres" (forgotten import?)`. `cmd/sqlguard` now
Expand Down
47 changes: 43 additions & 4 deletions cmd/sqlguard/scan.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ var formatFlag string
var scanCmd = &cobra.Command{
Use: "scan [path]",
Short: "Scan Go source files for SQL query issues",
Long: "Statically analyzes Go source files to find SQL queries and check them for common issues.",
Args: cobra.MaximumNArgs(1),
RunE: runScan,
Long: "Statically analyzes Go source files to find SQL queries and check them for common issues.\n\n" +
"The scan is always recursive. A path may be written either plainly (./pkg)\n" +
"or with the Go package-pattern suffix (./pkg/...); both select the same files.",
Args: cobra.MaximumNArgs(1),
RunE: runScan,
}

func init() {
Expand All @@ -56,7 +58,7 @@ func runScan(cmd *cobra.Command, args []string) error {

dir := "."
if len(args) > 0 {
dir = args[0]
dir = trimPatternSuffix(args[0])
}

rep, err := newReporter(formatFlag)
Expand Down Expand Up @@ -97,6 +99,43 @@ func runScan(cmd *cobra.Command, args []string) error {
return nil
}

// trimPatternSuffix accepts the `./...` spelling every Go tool takes. The scan
// is already recursive, so `dir/...` selects exactly what `dir` does and the
// suffix only has to be removed before the path reaches the filesystem.
func trimPatternSuffix(path string) string {
return trimPatternSuffixSep(path, filepath.Separator)
}

// trimPatternSuffixSep takes the separator explicitly so both platforms'
// behavior is testable from either one.
//
// The suffix must be separator-anchored: a directory really named `weird...`
// is a legal path, and trimming it unanchored would silently scan `weird`
// instead and report a clean exit for a tree that was never looked at.
//
// `\...` counts only where the OS separator is a backslash. The go command
// rewrites `\` to `/` in relative arguments "as a courtesy to Windows
// developers" (cmd/go/internal/search), so `.\...` is a spelling users do
// type — but on Unix a backslash is an ordinary filename byte, and a directory
// named `weird\...` there must reach the filesystem intact.
func trimPatternSuffixSep(path string, sep rune) string {
if path == "..." {
return "."
}
trimmed, ok := strings.CutSuffix(path, "/...")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Literal directory is skipped If the final path component is an existing directory named ..., this trim scans its parent instead. For example, sqlguard scan ./queries/... cannot target a literal queries/... directory. If the parent has no findings, the command can exit successfully without examining the requested directory. Check whether the literal path exists before treating it as a pattern, or provide a way to address it.

Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/sqlguard/scan.go
Line: 125

Comment:
**Literal directory is skipped** If the final path component is an existing directory named `...`, this trim scans its parent instead. For example, `sqlguard scan ./queries/...` cannot target a literal `queries/...` directory. If the parent has no findings, the command can exit successfully without examining the requested directory. Check whether the literal path exists before treating it as a pattern, or provide a way to address it.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

if !ok && sep == '\\' {
trimmed, ok = strings.CutSuffix(path, `\...`)
}
if !ok {
return path
}
// A bare root ("/...", or "C:\..." on Windows) loses its separator above.
if trimmed == "" || strings.HasSuffix(trimmed, ":") {
return trimmed + string(sep)
}
return trimmed
}

func newReporter(format string) (reporter.Reporter, error) {
switch format {
case "json":
Expand Down
159 changes: 158 additions & 1 deletion cmd/sqlguard/scan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,13 @@ func f(db *sql.DB) {
// Returns the output and the error (errIssuesFound if issues were found).
func captureScanOutput(t *testing.T, dir string) (string, error) {
t.Helper()
return captureScanTarget(t, dir)
}

// captureScanTarget is captureScanOutput for a target that is not a plain
// directory path, such as the `./...` package-pattern spelling.
func captureScanTarget(t *testing.T, target string) (string, error) {
t.Helper()

// Reset format flag to default for each test
formatFlag = "console"
Expand All @@ -344,7 +351,7 @@ func captureScanOutput(t *testing.T, dir string) (string, error) {
r, w, _ := os.Pipe()
os.Stderr = w

err := runScan(&cobra.Command{}, []string{dir})
err := runScan(&cobra.Command{}, []string{target})

w.Close()
os.Stderr = old
Expand Down Expand Up @@ -397,3 +404,153 @@ func TestScanCommand_NoUsageDumpOnIssues(t *testing.T) {
t.Errorf("expected the finding in output, got:\n%s", out)
}
}

func TestTrimPatternSuffix(t *testing.T) {
// sep is the OS separator the call is evaluated under, so Windows
// behavior is covered from a Unix host and vice versa.
cases := []struct {
sep rune
in, want string
}{
// Slash patterns work on every platform.
{'/', "./...", "."},
{'/', "...", "."},
{'/', "/...", "/"},
{'/', "./pkg/...", "./pkg"},
{'/', "pkg/...", "pkg"},
{'/', "/abs/pkg/...", "/abs/pkg"},
{'\\', "./...", "."},
{'\\', "...", "."},
{'\\', "./pkg/...", "./pkg"},

// Plain paths are untouched.
{'/', ".", "."},
{'/', "./pkg", "./pkg"},
{'/', "/abs/pkg", "/abs/pkg"},
{'/', "", ""},

// Separator-anchored: directory names, not patterns.
{'/', "weird...", "weird..."},
{'/', "./weird...", "./weird..."},
{'/', "....", "...."},
{'/', "/abs/weird...", "/abs/weird..."},

// A backslash is an ordinary filename byte on Unix, so `weird\...`
// and `.\...` are directory names there — but patterns on Windows.
{'/', `.\...`, `.\...`},
{'/', `weird\...`, `weird\...`},
{'/', `./a\...`, `./a\...`},
{'\\', `.\...`, "."},
{'\\', `.\pkg\...`, `.\pkg`},
{'\\', `pkg\...`, "pkg"},
{'\\', `weird...`, `weird...`},

// Bare roots keep their separator.
{'\\', `C:\...`, `C:\`},
{'\\', `\...`, `\`},
}
for _, c := range cases {
if got := trimPatternSuffixSep(c.in, c.sep); got != c.want {
t.Errorf("trimPatternSuffixSep(%q, %q) = %q, want %q", c.in, c.sep, got, c.want)
}
}
}

// TestScan_AcceptsPackagePattern pins the `./...` spelling the README and the
// docs site use. It used to reach filepath.Abs verbatim and fail with
// "lstat ./...: no such file or directory", so the documented invocation was
// the one invocation that did not work. The nested file also proves the
// pattern still reaches subdirectories rather than silently scanning one level.
func TestScan_AcceptsPackagePattern(t *testing.T) {
dir := t.TempDir()
createTestFile(t, dir, "top.go", `package example
import "database/sql"
func f(db *sql.DB) {
db.Query("SELECT * FROM users")
}
`)
sub := filepath.Join(dir, "nested")
if err := os.Mkdir(sub, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
createTestFile(t, sub, "deep.go", `package nested
import "database/sql"
func g(db *sql.DB) {
db.Exec("DELETE FROM sessions")
}
`)

out, err := captureScanTarget(t, filepath.Join(dir, "..."))

if !errors.Is(err, errIssuesFound) {
t.Fatalf("expected errIssuesFound for ./... target, got %v\noutput:\n%s", err, out)
}
if !strings.Contains(out, "select-star") {
t.Errorf("pattern target missed the top-level file, got:\n%s", out)
}
if !strings.Contains(out, "delete-without-where") {
t.Errorf("pattern target did not recurse into the subdirectory, got:\n%s", out)
}
}

// TestScan_PatternMatchesPlainPath is the equivalence the fix rests on: the
// scan is already recursive, so `dir/...` must select exactly what `dir` does.
func TestScan_PatternMatchesPlainPath(t *testing.T) {
dir := t.TempDir()
createTestFile(t, dir, "a.go", `package example
import "database/sql"
func f(db *sql.DB) {
db.Query("SELECT * FROM users")
}
`)

plain, errPlain := captureScanTarget(t, dir)
pattern, errPattern := captureScanTarget(t, filepath.Join(dir, "..."))

if !errors.Is(errPlain, errIssuesFound) || !errors.Is(errPattern, errIssuesFound) {
t.Fatalf("both spellings should report issues: plain=%v pattern=%v", errPlain, errPattern)
}
if plain != pattern {
t.Errorf("plain and pattern targets disagree:\nplain:\n%s\npattern:\n%s", plain, pattern)
}
}

// TestScan_DottedDirectoryIsNotAPattern guards the separator anchor. A
// directory named `weird...` is a legal path; trimming the suffix unanchored
// pointed the scan at a sibling `weird` instead, which reported that
// directory's findings under the name the user did not ask for — and would
// have exited clean had the sibling been clean.
func TestScan_DottedDirectoryIsNotAPattern(t *testing.T) {
root := t.TempDir()
dotted := filepath.Join(root, "weird...")
plain := filepath.Join(root, "weird")
for _, d := range []string{dotted, plain} {
if err := os.Mkdir(d, 0o755); err != nil {
t.Fatalf("mkdir %s: %v", d, err)
}
}
createTestFile(t, dotted, "a.go", `package weird
import "database/sql"
func f(db *sql.DB) {
db.Query("SELECT * FROM users")
}
`)
createTestFile(t, plain, "b.go", `package weird
import "database/sql"
func g(db *sql.DB) {
db.Exec("DELETE FROM sessions")
}
`)

out, err := captureScanTarget(t, dotted)

if !errors.Is(err, errIssuesFound) {
t.Fatalf("expected the dotted directory to be scanned, got %v\noutput:\n%s", err, out)
}
if !strings.Contains(out, "select-star") {
t.Errorf("did not scan the directory that was named, got:\n%s", out)
}
if strings.Contains(out, "delete-without-where") {
t.Errorf("scanned the sibling directory instead of the one named, got:\n%s", out)
}
}
8 changes: 8 additions & 0 deletions website/docs/scan.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ sqlguard scan ./internal/repository # one package tree
sqlguard scan --format json ./... # machine-readable
```

The scan is always recursive, so a path may be written plainly (`./internal`)
or with the Go package-pattern suffix (`./internal/...`); both select the same
files. With no path at all it scans the current directory.

_Changed in 0.3._ In 0.2 the pattern spelling was rejected outright —
`sqlguard scan ./...` failed with `lstat ./...: no such file or directory` —
so the form used throughout these docs had to be written as `sqlguard scan .`.

| Flag | Default | Effect |
| --- | --- | --- |
| `--format console\|json` | `console` | Output shape. JSON is an array of `{rule, severity, query, fingerprint, message, suggestion, file, line}`. |
Expand Down
Loading