Add regexpdynamicpattern linter: flag non-constant regexp compile patterns - #50674
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer that detects dynamic patterns passed to regexp.Compile and regexp.MustCompile.
Changes:
- Implements and registers
regexpdynamicpattern. - Adds analysistest fixtures and suppression coverage.
- Updates analyzer documentation and synchronization tests.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go |
Implements pattern analysis and diagnostics. |
pkg/linters/regexpdynamicpattern/regexpdynamicpattern_test.go |
Runs analyzer fixtures. |
pkg/linters/regexpdynamicpattern/testdata/src/regexpdynamicpattern/regexpdynamicpattern.go |
Covers constant, dynamic, and suppressed patterns. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds documentation synchronization coverage. |
pkg/linters/README.md |
Documents the new analyzer. |
pkg/linters/doc.go |
Updates package documentation and analyzer count. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go:69
- This diagnostic makes two incorrect claims for calls the analyzer flags:
regexp.Compilereturns malformed-pattern errors rather than panicking, and Go'sregexpimplementation uses linear-time matching rather than catastrophic backtracking. Report the policy violation without those claims, or emit separateMustCompile-specific wording.
Message: "regexp pattern is not a compile-time constant; dynamic patterns can panic at runtime or enable ReDoS if influenced by untrusted input",
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| // 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. |
| // 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 { |
| panicinlibrarycode.Analyzer, | ||
| rawloginlib.Analyzer, | ||
| regexpcompileinfunction.Analyzer, | ||
| regexpdynamicpattern.Analyzer, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Build tag compliance: Both files carry Coverage quality note: Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-structured. The type-checker-based package resolution properly handles aliased imports. The hasConstantStringPattern function correctly handles string literals, const identifiers, and constant-only concatenation. nolint suppression on both same-line and previous-line is tested. Wiring into registry, docs, and spec tests is complete.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20 AIC · ⌖ 9.21 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: no blocking issues
Solid, focused addition — the new linter correctly detects non-constant regexp patterns using type-checker-based package resolution and constant-value inspection.
Review notes
isRegexpCompileCallcorrectly resolves theregexppackage viatypes.PkgName, avoiding false positives from local shadowing.hasConstantStringPatterncorrectly usestypes.TypeAndValue.Valueto detect compile-time constants (literals, const idents, const-only concatenation).- Nolint suppression (same-line and previous-line) is tested and works as expected.
- Minor gap: doc comments claim handling of aliased imports and shadowed identifiers, but no test fixtures cover those cases (see inline comment) — non-blocking, worth adding for regression safety.
- Wiring (registry, doc.go, README, spec_test) is consistent and complete.
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, regexpdynamicpattern.Analyzer, "regexpdynamicpattern") | ||
| } |
There was a problem hiding this comment.
Test coverage doesn't exercise the two false-positive scenarios the package doc explicitly claims to handle (aliased regexp import, shadowed local identifier named regexp).
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on test-coverage gaps before merging.
📋 Key Themes & Highlights
Issues Found
- Missing POSIX variants:
regexp.CompilePOSIX/regexp.MustCompilePOSIXcarry identical risks but are not checked (line 80) - Aliased import not tested: the PR claims type-checker resolution handles aliased imports, but no testdata fixture validates this path (testdata line 9)
- Minor: zero-arg guard (line 103) is good, but has no test to document the invariant
Positive Highlights
- ✅ Excellent use of
types.PkgName.Imported().Path()to avoid false positives from shadowed identifiers - ✅
hasConstantStringPatterncorrectly usestv.Valuefrom the type checker — handles consts and const-only expressions cleanly - ✅ Both same-line and previous-line
//nolintsuppression are tested - ✅ Documentation fully wired across README, doc.go, registry, and spec_test
- ✅ Clear, accurate PR description linking to the security motivation
| // 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) |
There was a problem hiding this comment.
[/tdd] regexp.CompilePOSIX and regexp.MustCompilePOSIX are not checked here, but they carry the same dynamic-pattern risk as their non-POSIX counterparts.
💡 Suggested fix
Extend the name check:
if sel.Sel.Name != "MustCompile" && sel.Sel.Name != "Compile" &&
sel.Sel.Name != "MustCompilePOSIX" && sel.Sel.Name != "CompilePOSIX" {
return false
}Add testdata fixtures for the POSIX variants to keep the test specification complete.
@copilot please address this.
| } | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
[/tdd] The len(call.Args) == 0 guard is a good defensive check, but there is no test fixture verifying a zero-argument call to regexp.Compile() is silently skipped rather than crashing. Zero-arg calls are technically invalid Go code, but the analyzer should not panic on malformed ASTs under analysis.
💡 Suggested test case
Since analysistest runs against valid Go, add a comment in the testdata noting this guard and consider a brief unit-test that exercises hasConstantStringPattern directly with a synthetic ast.CallExpr{Args: nil} to document the invariant.
@copilot please address this.
| ) | ||
|
|
||
| // not flagged: literal pattern at package level. | ||
| var packageLevelRegexp = regexp.MustCompile(`^[a-z]+$`) |
There was a problem hiding this comment.
[/tdd] The testdata does not cover an aliased import of regexp (e.g. import re "regexp"). The analyzer claims to handle aliased imports via type-checker resolution, but this case is not proven by a test fixture.
💡 Suggested fixture additions
import re "regexp"
// not flagged: constant pattern via aliased import
var aliasedLiteral = re.MustCompile(`^[a-z]+$`)
// flagged: dynamic pattern via aliased import
func validateAliased(pattern string) { (nolint/redacted):...
re.MustCompile(pattern) // want `regexp pattern is not a compile-time constant...`
}This would give confidence that isRegexpCompileCall really resolves pkgName.Imported().Path() correctly regardless of local alias.
@copilot please address this.
Records the architectural decision to introduce a static analysis pass that flags non-constant regexp.Compile/MustCompile patterns, including the rationale, alternatives considered, and trade-offs.
|
@copilot please follow up on the latest review feedback on this PR:
|
regexp.Compile/regexp.MustCompilecalls built from dynamic input (string concatenation,fmt.Sprintf, function parameters) can panic at runtime on malformed patterns or enable ReDoS if the dynamic portion is influenced by untrusted input. The existingregexpcompileinfunctionlinter only checks where compilation happens (in-function vs. package-level), not whether the pattern itself is constant.New linter
pkg/linters/regexpdynamicpattern/regexpdynamicpattern.go— flagsregexp.Compile/MustCompilecalls whose pattern argument is not a compile-time constant string (literal, const identifier, or constant-only expression). Resolves theregexppackage identity via the type checker to handle aliased imports and avoid false positives from shadowed identifiers. Applies at both package level and inside functions, and respects//nolint:regexpdynamicpatternsuppressions.Wiring and docs
pkg/linters/registry.go(All())pkg/linters/doc.goandpkg/linters/README.md(bullet list, analyzer table, import example, subpackages list), with active-analyzer count updatedpkg/linters/spec_test.go'sdocumentedAnalyzers()list to keep doc-sync tests alignedTests
regexpdynamicpattern_test.gousinganalysistest, with fixtures undertestdata/src/regexpdynamicpattern/covering literal/const patterns (not flagged),fmt.Sprintf-built and concatenation-with-variable patterns (flagged), pattern passed through from a parameter (flagged), constant-only concatenation (not flagged), and//nolint:regexpdynamicpatternsuppression on both the same line and the previous line.