Log SQL with placeholders, never bound values (closes #207) (#222)

internal/gormlog/scan_guard_test.go

@@ -0,0 +1,260 @@
+package gormlog_test
+
+import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// minNonTestFiles guards the walk below against passing because it
+// found nothing to look at. The tree held 60 non-test .go files when
+// this was written.
+const minNonTestFiles = 40
+
+// isRowProducer reports whether name is a method that returns a
+// database/sql row handle. GORM's Row and Rows return *sql.Row and
+// *sql.Rows, so Scan on the result of one of them is database/sql's
+// Scan and never (*gorm.DB).Scan.
+func isRowProducer(name string) bool {
+	switch name {
+	case "Row", "Rows", "QueryRow", "QueryRowContext":
+		return true
+	default:
+		return false
+	}
+}
+
+// receiverIsRowHandle reports whether x is syntactically a call to a
+// row producer, which is the only receiver form this check accepts for
+// a Scan.
+func receiverIsRowHandle(x ast.Expr) bool {
+	call, ok := x.(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+
+	return isRowProducer(sel.Sel.Name)
+}
+
+// unguardedScans returns the position of every Scan call in file whose
+// receiver is not a row handle. It fails closed: a receiver it cannot
+// resolve syntactically — a local variable, a struct field — is
+// reported rather than assumed safe.
+func unguardedScans(
+	fset *token.FileSet, file *ast.File,
+) []token.Position {
+	var found []token.Position
+
+	ast.Inspect(file, func(n ast.Node) bool {
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return true
+		}
+
+		sel, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok || sel.Sel.Name != "Scan" {
+			return true
+		}
+
+		if !receiverIsRowHandle(sel.X) {
+			found = append(found, fset.Position(sel.Sel.Pos()))
+		}
+
+		return true
+	})
+
+	return found
+}
+
+// moduleRoot walks up from the working directory to the directory
+// holding go.mod.
+func moduleRoot(t *testing.T) string {
+	t.Helper()
+
+	dir, err := os.Getwd()
+	require.NoError(t, err)
+
+	for {
+		_, statErr := os.Stat(filepath.Join(dir, "go.mod"))
+		if statErr == nil {
+			return dir
+		}
+
+		parent := filepath.Dir(dir)
+		require.NotEqual(t, parent, dir, "no go.mod above %s", dir)
+
+		dir = parent
+	}
+}
+
+// skipDir reports whether a directory holds no source this check
+// governs.
+func skipDir(name string) bool {
+	switch name {
+	case ".git", "bin", "node_modules", "testdata":
+		return true
+	default:
+		return false
+	}
+}
+
+// walkNonTestGo parses every non-test .go file under root and returns
+// how many it parsed along with every unguarded Scan it found.
+func walkNonTestGo(t *testing.T, root string) (int, []string) {
+	t.Helper()
+
+	var (
+		parsed int
+		hits   []string
+	)
+
+	fset := token.NewFileSet()
+
+	require.NoError(t, filepath.WalkDir(
+		root,
+		func(path string, d fs.DirEntry, err error) error {
+			if err != nil {
+				return err
+			}
+
+			if d.IsDir() {
+				if skipDir(d.Name()) {
+					return fs.SkipDir
+				}
+
+				return nil
+			}
+
+			if !isNonTestGo(d.Name()) {
+				return nil
+			}
+
+			file, err := parser.ParseFile(fset, path, nil, 0)
+			if err != nil {
+				return err
+			}
+
+			parsed++
+
+			for _, pos := range unguardedScans(fset, file) {
+				hits = append(hits, relPosition(root, pos))
+			}
+
+			return nil
+		},
+	))
+
+	return parsed, hits
+}
+
+// isNonTestGo reports whether a file name is Go source this check
+// governs.
+func isNonTestGo(name string) bool {
+	return strings.HasSuffix(name, ".go") &&
+		!strings.HasSuffix(name, "_test.go")
+}
+
+// relPosition renders pos with its path relative to root, so a failure
+// names the file the way the repository does.
+func relPosition(root string, pos token.Position) string {
+	name := pos.Filename
+
+	rel, err := filepath.Rel(root, name)
+	if err == nil {
+		name = rel
+	}
+
+	return fmt.Sprintf("%s:%d:%d", name, pos.Line, pos.Column)
+}
+
+// TestGormScanIsNeverCalledOutsideTests keeps (*gorm.DB).Scan out of
+// non-test code.
+//
+// It is the one statement path (*Logger).ParamsFilter does not reach:
+// Scan swaps GORM's own trace recorder in for the adapter, and that
+// recorder does not implement gorm.ParamsFilter, so the statement is
+// logged with its values interpolated. The package comment states the
+// limit; this fails when someone adds a call site anyway.
+//
+// The current tree has one caller, internal/database/database_test.go,
+// which this check does not govern: it is test-only and its SELECT 1
+// binds nothing.
+func TestGormScanIsNeverCalledOutsideTests(t *testing.T) {
+	t.Parallel()
+
+	parsed, offenders := walkNonTestGo(t, moduleRoot(t))
+
+	require.GreaterOrEqual(
+		t, parsed, minNonTestFiles,
+		"parsed %d non-test .go files, so this check found "+
+			"nothing to look at", parsed,
+	)
+	require.Empty(
+		t, offenders,
+		"Scan called on a receiver this check cannot show is a "+
+			"database/sql row handle. (*gorm.DB).Scan logs the "+
+			"statement with its bound values interpolated — use "+
+			"Find, Pluck, or Raw(...).Row().Scan instead. A "+
+			"database/sql Scan reached through a variable is "+
+			"reported too; write it as <producer>().Scan rather "+
+			"than widening this check.",
+	)
+}
+
+// scanGuardCase is one planted snippet and whether the check above
+// should report it.
+type scanGuardCase struct {
+	name string
+	body string
+	want int
+}
+
+func scanGuardCases() []scanGuardCase {
+	return []scanGuardCase{
+		{"gorm chain", `db.DB().Raw("SELECT 1").Scan(&v)`, 1},
+		{"gorm receiver", `gdb.Scan(&v)`, 1},
+		{"gorm via variable", "q := gdb.Raw(\"x\")\nq.Scan(&v)", 1},
+		{"gorm model chain", `gdb.Model(&x).Scan(&v)`, 1},
+		{"sql row", `gdb.Raw("SELECT 1").Row().Scan(&v)`, 0},
+		{"sql rows", `gdb.Raw("SELECT 1").Rows().Scan(&v)`, 0},
+		{"unrelated call", `gdb.Find(&v)`, 0},
+	}
+}
+
+// TestScanGuard_ReportsPlantedCalls proves the check fires. Without it
+// a detector that matched nothing would satisfy the walk above no
+// matter what the tree contained.
+func TestScanGuard_ReportsPlantedCalls(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range scanGuardCases() {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			fset := token.NewFileSet()
+			src := fmt.Sprintf(
+				"package p\n\nfunc f() {\n\t%s\n}\n", tc.body,
+			)
+
+			file, err := parser.ParseFile(
+				fset, tc.name+".go", src, 0,
+			)
+			require.NoError(t, err)
+
+			require.Len(t, unguardedScans(fset, file), tc.want)
+		})
+	}
+}