diff --git a/docs/adr/50674-add-regexpdynamicpattern-linter.md b/docs/adr/50674-add-regexpdynamicpattern-linter.md new file mode 100644 index 00000000000..5d0f299dd31 --- /dev/null +++ b/docs/adr/50674-add-regexpdynamicpattern-linter.md @@ -0,0 +1,63 @@ +# ADR-50674: Add regexpdynamicpattern Static Analysis Linter + +**Date**: 2026-08-05 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `pkg/linters` package provides custom Go static analysis passes that enforce codebase-wide safety +and style invariants. `regexp.Compile` and `regexp.MustCompile` are safe when called with compile-time +constant patterns, but when the pattern is built from dynamic input (e.g. `fmt.Sprintf`, string +concatenation with variables, or function parameters) two distinct risks arise: a malformed pattern +causes a runtime panic (or a returned error that callers often ignore), and if the dynamic portion is +influenced by untrusted input the pattern can trigger catastrophic backtracking (ReDoS). The existing +`regexpcompileinfunction` linter only enforces *where* compilation occurs (package-level vs. +function-level), not *what* is being compiled, leaving the dynamic-pattern risk unaddressed. + +### Decision + +We will introduce a new `regexpdynamicpattern` analyzer in `pkg/linters/regexpdynamicpattern/` that +reports any `regexp.Compile` or `regexp.MustCompile` call whose first argument is not a compile-time +constant string (literal, `const` identifier, or constant-only expression). Package identity is +resolved via the type checker to handle aliased imports without false positives. The analyzer is +registered in `pkg/linters/registry.go` alongside the existing linters and respects +`//nolint:regexpdynamicpattern` suppressions for intentional dynamic patterns. + +### Alternatives Considered + +#### Alternative 1: Rely solely on the existing `regexpcompileinfunction` linter + +`regexpcompileinfunction` enforces that regexp compilation occurs at package level. A package-level +`var re = regexp.MustCompile(buildPattern())` would still pass that linter while introducing a +dynamic-pattern risk. This alternative was rejected because it does not address the class of risk +identified: the *content* of the pattern, not its *location*, determines the safety hazard. + +#### Alternative 2: Runtime validation wrapper + +Wrap `regexp.Compile`/`MustCompile` with a project-internal helper that validates or sanitizes the +pattern at runtime. This would catch panics but cannot prevent ReDoS (the unsafe pattern still +executes) and introduces runtime overhead on every compilation call. It also requires migrating all +call sites, whereas a static linter operates without any code changes to call sites that already use +constant patterns. + +### Consequences + +#### Positive +- Eliminates an entire class of runtime panics caused by malformed dynamically-constructed regexp patterns. +- Reduces the ReDoS attack surface by flagging call sites where untrusted input could flow into regexp compilation. +- Consistent with the project's existing philosophy of enforcing safety invariants at analysis time rather than at runtime. + +#### Negative +- Intentional dynamic regexp patterns (e.g. test helpers that build patterns from parameters) require `//nolint:regexpdynamicpattern` suppressions, adding annotation noise. +- The linter operates only on packages compiled with full type-checker information; packages analyzed without `TypesInfo` populated will silently skip pattern-constant checks (the analyzer returns `false` for unknown patterns rather than reporting a finding). + +#### Neutral +- The `pkg/linters/doc.go` active-analyzer count increments from 62 to 63; documentation and `spec_test.go` must be updated whenever the analyzer list changes (this is already the project convention). +- The new analyzer composes with the existing `nolint` and `filecheck` infrastructure (generated-file skipping, suppression directives) without requiring any changes to those internal packages. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/linters b/linters index 5725ab9455a..636ef73a18f 100755 Binary files a/linters and b/linters differ diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 836eb89dfdb..60fe1ff1a20 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -43,6 +43,7 @@ This package currently provides custom Go analyzers in the following subpackages - `panic-in-library-code` — reports `panic()` calls in library packages (`pkg/*`) where errors should be returned instead. - `rawloginlib` — reports direct usage of the standard `log` package in library packages, where `pkg/logger` should be used. - `regexpcompileinfunction` — reports `regexp.MustCompile` / `regexp.Compile` calls inside functions that should be package-level. +- `regexpdynamicpattern` — reports `regexp.MustCompile` / `regexp.Compile` calls whose pattern is not a compile-time constant string. - `seenmapbool` — reports `map[string]bool` used as a set (values always `true`) that should use `map[string]struct{}` instead. - `sortslice` — reports `sort.Slice` / `sort.SliceStable` calls that should use `slices.SortFunc` / `slices.SortStableFunc`. - `sprintferrdot` — reports redundant `.Error()` calls on error values passed to `fmt` format functions where the fmt package calls `.Error()` automatically. @@ -112,6 +113,7 @@ This package currently provides custom Go analyzers in the following subpackages | `panic-in-library-code` | Custom `go/analysis` analyzer that flags `panic()` usage in library packages | | `rawloginlib` | Custom `go/analysis` analyzer that flags standard-library `log` package calls in library packages | | `regexpcompileinfunction` | Custom `go/analysis` analyzer that flags regexp compilation inside function bodies | +| `regexpdynamicpattern` | Custom `go/analysis` analyzer that flags regexp.MustCompile/Compile calls with non-constant patterns | | `seenmapbool` | Custom `go/analysis` analyzer that flags `map[string]bool` used as a set that should use `map[string]struct{}` | | `sortslice` | Custom `go/analysis` analyzer that flags `sort.Slice` / `sort.SliceStable` calls that should use `slices.SortFunc` / `slices.SortStableFunc` | | `sprintferrdot` | Custom `go/analysis` analyzer that flags redundant `.Error()` calls on error values passed to `fmt` format functions | @@ -170,6 +172,7 @@ import ( panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib" "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" + "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/sprintfbool" "github.com/github/gh-aw/pkg/linters/sprintfint" @@ -199,6 +202,7 @@ _ = osexitinlibrary.Analyzer _ = panicinlibrarycode.Analyzer _ = rawloginlib.Analyzer _ = regexpcompileinfunction.Analyzer +_ = regexpdynamicpattern.Analyzer _ = sortslice.Analyzer _ = sprintfbool.Analyzer _ = sprintfint.Analyzer @@ -245,6 +249,7 @@ _ = trimleftright.Analyzer - `github.com/github/gh-aw/pkg/linters/panic-in-library-code` — panic-in-library-code analyzer subpackage - `github.com/github/gh-aw/pkg/linters/rawloginlib` — raw-log-in-lib analyzer subpackage - `github.com/github/gh-aw/pkg/linters/regexpcompileinfunction` — regexp-compile-in-function analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/regexpdynamicpattern` — regexp-dynamic-pattern analyzer subpackage - `github.com/github/gh-aw/pkg/linters/seenmapbool` — seen-map-bool analyzer subpackage - `github.com/github/gh-aw/pkg/linters/sortslice` — sort-slice analyzer subpackage - `github.com/github/gh-aw/pkg/linters/sprintferrdot` — sprintf-err-dot analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 3a0f83c8de4..4bfc26d79bb 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 62 active analyzers: +// All 63 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -39,6 +39,7 @@ // - panic-in-library-code — flags panic() calls in library packages // - rawloginlib — flags direct usage of the standard log package in library packages // - regexpcompileinfunction — flags regexp.MustCompile/Compile calls inside functions +// - regexpdynamicpattern — flags regexp.MustCompile/Compile calls whose pattern is not a compile-time constant // - seenmapbool — flags map[string]bool used as a set that should use map[string]struct{} // - sortslice — flags sort.Slice / sort.SliceStable calls that should use slices.SortFunc / slices.SortStableFunc // - sprintferrdot — flags redundant .Error() calls on error values passed to fmt format functions diff --git a/pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go b/pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go new file mode 100644 index 00000000000..438248bb223 --- /dev/null +++ b/pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go @@ -0,0 +1,123 @@ +// Package regexpdynamicpattern implements a Go analysis linter that flags +// calls to regexp.MustCompile() and regexp.Compile() whose pattern argument +// is not a compile-time constant string. Dynamically constructed patterns can +// panic at runtime on malformed input and, when influenced by untrusted +// input, can enable catastrophic-backtracking (ReDoS) denial-of-service +// attacks. +package regexpdynamicpattern + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" + "github.com/github/gh-aw/pkg/logger" +) + +var pkgLog = logger.New("linters:regexpdynamicpattern") + +// Analyzer is the regexp-dynamic-pattern analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "regexpdynamicpattern", + Doc: "reports regexp.MustCompile and regexp.Compile calls whose pattern is not a compile-time constant string", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/regexpdynamicpattern", + Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + pkgLog.Printf("analyzing package %s", pass.Pkg.Path()) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintIndex, err := nolint.Index(pass) + if err != nil { + return nil, err + } + generatedFiles, err := filecheck.Index(pass) + if err != nil { + return nil, err + } + + for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { + call, ok := cur.Node().(*ast.CallExpr) + if !ok || !isRegexpCompileCall(pass, call) { + continue + } + if hasConstantStringPattern(pass, call) { + continue + } + + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + continue + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "regexpdynamicpattern") { + continue + } + pkgLog.Printf("flagging dynamic regexp pattern at %s", pos) + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "regexp pattern is not a compile-time constant; dynamic patterns can panic at runtime or enable ReDoS if influenced by untrusted input", + }) + } + + return nil, nil +} + +// isRegexpCompileCall checks if the call is to regexp.MustCompile or regexp.Compile, +// resolving the package identity via the type checker to handle aliased imports +// and avoid false positives from local identifiers named "regexp". +func isRegexpCompileCall(pass *analysis.Pass, call *ast.CallExpr) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if sel.Sel.Name != "MustCompile" && sel.Sel.Name != "Compile" { + return false + } + ident, ok := sel.X.(*ast.Ident) + if !ok || pass.TypesInfo == nil { + return false + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return false + } + pkgName, ok := obj.(*types.PkgName) + if !ok || pkgName.Imported() == nil { + return false + } + return pkgName.Imported().Path() == "regexp" +} + +// hasConstantStringPattern checks whether the regexp pattern is a compile-time constant string, +// such as a string literal, const identifier, or an expression built entirely from constants +// (e.g. concatenation of string literals/consts). Non-constant expressions such as +// fmt.Sprintf results, concatenation involving variables, or function parameters return false. +func hasConstantStringPattern(pass *analysis.Pass, call *ast.CallExpr) bool { + if len(call.Args) == 0 { + return false + } + + patternArg := call.Args[0] + if lit, ok := patternArg.(*ast.BasicLit); ok && lit.Kind == token.STRING { + return true + } + + tv, ok := pass.TypesInfo.Types[patternArg] + if !ok || tv.Value == nil || tv.Type == nil { + return false + } + + basic, ok := tv.Type.Underlying().(*types.Basic) + return ok && basic.Kind() == types.String +} diff --git a/pkg/linters/regexpdynamicpattern/regexpdynamicpattern_test.go b/pkg/linters/regexpdynamicpattern/regexpdynamicpattern_test.go new file mode 100644 index 00000000000..52318ce451f --- /dev/null +++ b/pkg/linters/regexpdynamicpattern/regexpdynamicpattern_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package regexpdynamicpattern_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, regexpdynamicpattern.Analyzer, "regexpdynamicpattern") +} diff --git a/pkg/linters/regexpdynamicpattern/testdata/src/regexpdynamicpattern/regexpdynamicpattern.go b/pkg/linters/regexpdynamicpattern/testdata/src/regexpdynamicpattern/regexpdynamicpattern.go new file mode 100644 index 00000000000..5924ce22370 --- /dev/null +++ b/pkg/linters/regexpdynamicpattern/testdata/src/regexpdynamicpattern/regexpdynamicpattern.go @@ -0,0 +1,71 @@ +package regexpdynamicpattern + +import ( + "fmt" + "regexp" +) + +// not flagged: literal pattern at package level. +var packageLevelRegexp = regexp.MustCompile(`^[a-z]+$`) + +const constPattern = `^const$` +const constSuffix = `$` + +// not flagged: literal pattern. +func ValidateLiteral(input string) bool { + re := regexp.MustCompile(`^[a-z]+$`) + return re.MatchString(input) +} + +// not flagged: const identifier pattern. +func ValidateConst(input string) bool { + re := regexp.MustCompile(constPattern) + return re.MatchString(input) +} + +// not flagged: concatenation of constant-only expressions. +func ValidateConstConcat(input string) bool { + re := regexp.MustCompile(`^const` + constSuffix) + return re.MatchString(input) +} + +// flagged: pattern built with fmt.Sprintf. +func ValidateSprintf(prefix, input string) (bool, error) { + re, err := regexp.Compile(fmt.Sprintf("^%s$", prefix)) // want `regexp pattern is not a compile-time constant; dynamic patterns can panic at runtime or enable ReDoS if influenced by untrusted input` + if err != nil { + return false, err + } + return re.MatchString(input), nil +} + +// flagged: string concatenation with a variable. +func ValidateConcatVariable(suffix, input string) bool { + re := regexp.MustCompile(`^prefix` + suffix) // want `regexp pattern is not a compile-time constant; dynamic patterns can panic at runtime or enable ReDoS if influenced by untrusted input` + return re.MatchString(input) +} + +// flagged: pattern passed through from a function parameter. +func ValidateDynamic(pattern, input string) (bool, error) { + re, err := regexp.Compile(pattern) // want `regexp pattern is not a compile-time constant; dynamic patterns can panic at runtime or enable ReDoS if influenced by untrusted input` + if err != nil { + return false, err + } + return re.MatchString(input), nil +} + +func suppressedPreviousLine(pattern, input string) (bool, error) { + //nolint:regexpdynamicpattern + re, err := regexp.Compile(pattern) + if err != nil { + return false, err + } + return re.MatchString(input), nil +} + +func suppressedSameLine(pattern, input string) (bool, error) { + re, err := regexp.Compile(pattern) //nolint:regexpdynamicpattern + if err != nil { + return false, err + } + return re.MatchString(input), nil +} diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index af0285ac238..c6f7cdc2a32 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -40,6 +40,7 @@ import ( panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib" "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" + "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" "github.com/github/gh-aw/pkg/linters/seenmapbool" "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/sprintfbool" @@ -111,6 +112,7 @@ var allAnalyzers = []*analysis.Analyzer{ panicinlibrarycode.Analyzer, rawloginlib.Analyzer, regexpcompileinfunction.Analyzer, + regexpdynamicpattern.Analyzer, ssljson.Analyzer, seenmapbool.Analyzer, sortslice.Analyzer, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 394b634d283..84babacbe22 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -48,6 +48,7 @@ import ( panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib" "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" + "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" "github.com/github/gh-aw/pkg/linters/seenmapbool" "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/sprintfbool" @@ -98,7 +99,7 @@ type docAnalyzer struct { // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, // goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, -// regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, +// regexpcompileinfunction, regexpdynamicpattern, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, // tolowerequalfold, trimleftright, uncheckedflushreturn, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { @@ -140,6 +141,7 @@ func documentedAnalyzers() []docAnalyzer { {"panic-in-library-code", panicinlibrarycode.Analyzer}, {"rawloginlib", rawloginlib.Analyzer}, {"regexpcompileinfunction", regexpcompileinfunction.Analyzer}, + {"regexpdynamicpattern", regexpdynamicpattern.Analyzer}, {"seenmapbool", seenmapbool.Analyzer}, {"sortslice", sortslice.Analyzer}, {"sprintferrdot", sprintferrdot.Analyzer},