feat: add PostgreSQL parser and reporting capabilities#2
Merged
Conversation
- Implemented a new PostgreSQL parser in `pgparser` that provides structural analysis of SQL queries, including support for SELECT, INSERT, UPDATE, and DELETE statements. - Added unit tests for the PostgreSQL parser to ensure accurate parsing and structural fact extraction. - Introduced a `ConsoleReporter` for outputting analysis results to the terminal with color-coded severity levels. - Created a `JSONReporter` for outputting analysis results in JSON format. - Defined a `Reporter` interface for consistent reporting of analysis results across different formats. - Updated the main `sqlguard` package to support the new PostgreSQL parser and reporting features.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
WalkthroughAdds the complete sqlguard initial release: analyzer (rules, fallback/grammar parsers, redaction, registry), middleware (Guard, cache, dedup, N+1, driver wrappers), CLI (scan/explain), config, reporters, integrations, tests, docs, and CI/tooling. ChangesInitial release
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant Analyzer
participant Guard
participant DB
participant Reporter
CLI->>Config: resolve config/profile
Config->>Analyzer: build analyzer with profile
CLI->>Analyzer: static scan / prepare query
Analyzer->>Guard: runtime Observe / Check
Guard->>DB: run EXPLAIN (rolled-back) / execute via wrapped driver
DB->>Guard: plan/rows
Guard->>Analyzer: fingerprint / normalize
Guard->>Reporter: report findings
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 19: The checkout steps use actions/checkout@v6 without disabling
credential persistence; update each checkout step that uses actions/checkout@v6
(all occurrences) to include persist-credentials: false in the step definition
so the GITHUB_TOKEN is not left in the repo git config (add the
persist-credentials: false key under the uses: actions/checkout@v6 entries).
- Around line 3-7: Add a concurrency policy to the GitHub Actions workflow to
cancel stale runs: in the CI workflow definition (the top-level "on:" block
handling push and pull_request) add a "concurrency" key with a unique group name
(e.g., using github.ref and github.workflow) and set "cancel-in-progress: true"
so that overlapping runs triggered by rapid pushes are cancelled; update the
workflow metadata where "on:" is defined to include this new concurrency
configuration.
In @.github/workflows/codeql.yml:
- Around line 16-18: Add an inline comment above the permissions block
documenting why security-events: write is required (CodeQL uploads SARIF/results
and GitHub requires write access to the security-events permission), mention
that this suppresses the "excessive-permissions" false positive and that
contents: read is only used for workflow context; update the comment near the
permissions keys (security-events: write and contents: read) so reviewers and
scanners see the intent.
- Around line 25-26: The Checkout step using actions/checkout@v6 currently
leaves GITHUB_TOKEN persisted in git config; update the Checkout step (the job
step named "Checkout" that uses actions/checkout@v6) to add persist-credentials:
false so the action does not write the runner's GITHUB_TOKEN into git config and
reduce credential-leakage risk.
In @.gitignore:
- Around line 21-22: .gitignore contains an entry "sqlguard-website" that
doesn't exist elsewhere; either remove that entry or retain it but add an inline
comment explaining what generates or uses that path (e.g., build output, CI
artifact, local dev tool) so future readers know its purpose; update the
"sqlguard-website" line to either be deleted or changed to a commented note
referencing "sqlguard-website" to clarify its origin and intent.
In @.golangci.yml:
- Around line 30-33: The .golangci.yml contains an invalid top-level
"formatters" section with entries "gofmt" and "goimports"; remove that section
and instead enable these tools under the existing "linters" configuration (or
omit them and rely on your Makefile's make fmt). Specifically, delete the
"formatters:" block and add "gofmt" and/or "goimports" to the "linters.enable"
list (or ensure they are enforced via make fmt), so references to
gofmt/goimports are recognized by golangci-lint.
In `@analyzer/redact.go`:
- Line 126: Update the misleading comment above skipSingleQuoted: replace the
incorrect escape reference `s[i] == '\"` with the correct single-quote form so
it reads that the function returns the index past a single-quoted string literal
starting at s[i] == '\'' and honoring '' doubled-quote escapes; ensure the
comment text matches the function name skipSingleQuoted and the actual character
checked in the code.
In `@cmd/sqlguard/db.go`:
- Around line 11-18: The switch mapping dialect to driverName is redundant
(mapping "postgres"→"postgres" and "mysql"→"mysql"); replace the switch with a
simple validation that dialect is one of the supported values and then set
driverName = dialect (i.e., check if dialect != "postgres" && dialect != "mysql"
then return the fmt.Errorf("unsupported dialect: %s", dialect), otherwise assign
driverName = dialect) so the logic in the function using the dialect and
driverName is simplified and clearer.
In `@cmd/sqlguard/scan.go`:
- Around line 224-226: The call to filepath.Abs in shouldSkipDir currently
ignores its error; change it to capture the returned error (e.g., absPath, err
:= filepath.Abs(path)) and handle it by returning that error from shouldSkipDir
so callers can respond appropriately; then continue using absPath for the
comparison with absDir. Ensure the function signature and any upstream callers
already expect/handle the error return from shouldSkipDir.
- Around line 117-119: scanDir currently ignores the error from filepath.Abs
which can break downstream path normalization in keepFile; update scanDir to
check the error returned by filepath.Abs(dir) and return an appropriate error
(or wrap it) instead of proceeding with an empty/invalid absDir, so callers of
scanDir know the operation failed; reference the scanDir function and its use of
absDir (used by keepFile) and ensure the error is propagated from scanDir to the
caller rather than ignored.
In `@explain/explain.go`:
- Around line 178-213: The walkPgPlan function (and likewise analyzeMySQL) is
populating analyzer.Result.Query with the raw query string; call the canonical
redactor before constructing Results and use the redacted value instead.
Specifically, inside PlanAnalyzer.walkPgPlan and analyzeMySQL, compute redacted
:= analyzer.Redact(query) and replace uses of query in each analyzer.Result{
Query: ... } with redacted (leave rawQuery unchanged if needed); ensure every
Result creation (RuleName "seq-scan", "high-cost", and the three analyzeMySQL
results) uses analyzer.Redact to avoid leaking literals.
In `@integrations/pgxguard/go.mod`:
- Line 14: The go.mod entry for golang.org/x/crypto currently pins v0.37.0 which
OSV flags for multiple ssh-related vulnerabilities; update that module to a
patched release (replace the golang.org/x/crypto v0.37.0 line) by running
something like `go get golang.org/x/crypto@latest` (or a specific patched tag),
then run `go mod tidy` and your test suite; confirm the new version appears in
integrations/pgxguard/go.mod and re-scan to ensure the vulnerabilities are
resolved while addressing the pgx fixes.
- Line 7: The go.mod currently pins the dependency "github.com/jackc/pgx/v5
v5.7.6" which has known critical memory-safety and other advisories; update the
pgx module version to at least v5.9.2 by changing the version entry for
github.com/jackc/pgx/v5 to v5.9.2, then run module tooling (go get/go mod tidy)
to refresh go.sum and verify builds and tests; ensure any pgx-related imports or
code paths (uses of pgx types/functions) compile cleanly after the upgrade and
adjust callsites if any API changes surface.
In `@middleware/dedup_test.go`:
- Around line 132-147: TestGuard_DedupConcurrent uses sync.WaitGroup incorrectly
and the loop syntax is invalid; replace the "for range 100" with a standard for
i := 0; i < 100; i++ loop, call wg.Add(1) for each iteration before spawning a
goroutine, and inside the goroutine defer wg.Done() and call g.Check("DELETE
FROM accounts"); this fixes compilation and ensures the countingReporter
receives exactly one concurrent check under NewGuard/WithReporter.
In `@middleware/driver_test.go`:
- Line 16: The tests import the cgo-dependent driver "_
\"github.com/mattn/go-sqlite3\"" but lack a cgo build constraint, so add a Go
build tag to guard these tests: insert the line "//go:build cgo" (and the legacy
"+build cgo" if you want older toolchains) at the top of
middleware/driver_test.go (and the other test files that import "_
\"github.com/mattn/go-sqlite3\"") so the tests are only compiled/run when cgo is
enabled; alternatively, if you prefer runtime skipping, add a check in the test
entry (e.g., Test* functions) to detect CGO availability via build tags or
environment and call t.Skip when CGO is disabled.
In `@parsers/mysqlparser/mysqlparser.go`:
- Around line 120-136: The hasRealFrom function currently does a case-sensitive
comparison tn.Name.String() == "dual" which fails for quoted/backticked
identifiers like `DUAL`; update hasRealFrom to perform a case-insensitive
comparison (e.g., use strings.EqualFold(tn.Name.String(), "dual")) when checking
for "dual", and ensure the strings package is imported; also add unit tests
covering quoted/backticked DUAL variants (e.g., `DUAL`, `Dual`) to verify
HasFrom=false for those cases.
In `@parsers/pgparser/go.mod`:
- Around line 7-33: The go.mod for module parsers/pgparser pulls in
auxten/postgresql-parser v1.0.1 which transitively requires vulnerable versions
of google.golang.org/grpc, golang.org/x/text, github.com/sirupsen/logrus, and
google.golang.org/protobuf; fix by adding explicit require/replace directives in
parsers/pgparser/go.mod to pin those transitive modules to patched versions
(e.g., grpc >= v1.79.3 or at least the minimum patched release, x/text >=
v0.3.8, logrus >= v1.9.3, protobuf >= v1.33.0/1.29.1 as appropriate), or if
upstream auxten/postgresql-parser cannot be updated, fork/replace
auxten/postgresql-parser with a patched fork and point go.mod to that fork;
ensure the changes reference the existing module name auxten/postgresql-parser
v1.0.1 in the replace/require entries so the dependency graph is overridden.
In `@reporter/console.go`:
- Around line 20-23: The exported Out field on ConsoleReporter creates a race
because callers can reassign it while Report is running; make Out unexported
(rename to out) and provide either a constructor that accepts an io.Writer or a
synchronized setter (e.g., SetOut) that locks mu, then update all references
from c.Out to c.out and ensure Report continues to lock mu around any access to
the writer; alternatively, if you prefer to keep it exported, document
immutability and enforce write access via a SetOut method that acquires mu—apply
this change for ConsoleReporter, its Out usages, and any tests/consumers.
In `@reporter/json.go`:
- Around line 53-55: The JSON encoder error from enc.Encode(out) is being
ignored; update the Encode call in reporter.json's code that creates enc :=
json.NewEncoder(j.Out) (and uses enc.SetIndent) to check the returned error and,
since Reporter doesn't return errors, write a clear error message including the
encoding error to os.Stderr (e.g., via fmt.Fprintf(os.Stderr, ...)) so failures
writing to j.Out (closed writer, disk full, etc.) are visible; keep behavior
otherwise unchanged.
- Around line 13-16: The exported Out field on JSONReporter bypasses the mutex
(mu) and can be reassigned concurrently causing a data race; change the struct
to make Out unexported (rename to out) or otherwise ensure it cannot be modified
after construction, update all references to JSONReporter.Out to use the new
unexported field (e.g., JSONReporter.out) or an accessor, and ensure Report
still locks mu before writing; mirror the same fix you applied to
ConsoleReporter to eliminate the race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5cd04aa-a31d-4177-8349-3e0fe8620421
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.sumintegrations/bunguard/go.sumis excluded by!**/*.sumintegrations/entguard/go.sumis excluded by!**/*.sumintegrations/gormguard/go.sumis excluded by!**/*.sumintegrations/pgxguard/go.sumis excluded by!**/*.sumintegrations/sqlxguard/go.sumis excluded by!**/*.sumintegrations/xormguard/go.sumis excluded by!**/*.sumparsers/mysqlparser/go.sumis excluded by!**/*.sumparsers/pgparser/go.sumis excluded by!**/*.sum
📒 Files selected for processing (86)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/codeql.yml.gitignore.golangci.yml.sqlguard.example.ymlCHANGELOG.mdCONTRIBUTING.mdMakefileREADME.mdSECURITY.mdanalyzer/analyzer.goanalyzer/analyzer_test.goanalyzer/fallback.goanalyzer/fallback_test.goanalyzer/parser.goanalyzer/profile_test.goanalyzer/redact.goanalyzer/redact_policy_test.goanalyzer/redact_test.goanalyzer/registry.goanalyzer/result.goanalyzer/rules.goanalyzer/severity.goanalyzer/statement.goanalyzer/suppress.gocmd/sqlguard/db.gocmd/sqlguard/explain.gocmd/sqlguard/main.gocmd/sqlguard/resolve_test.gocmd/sqlguard/root.gocmd/sqlguard/scan.gocmd/sqlguard/scan_test.gocodecov.ymlconfig/config.goconfig/config_test.goconfig/middleware.goconfig/middleware_test.goexplain/explain.goexplain/explain_test.gogo.modintegrations/bunguard/bunguard.gointegrations/bunguard/bunguard_test.gointegrations/bunguard/go.modintegrations/entguard/entguard.gointegrations/entguard/entguard_test.gointegrations/entguard/go.modintegrations/gormguard/go.modintegrations/gormguard/gormguard.gointegrations/gormguard/gormguard_test.gointegrations/pgxguard/go.modintegrations/pgxguard/pgxguard.gointegrations/pgxguard/pgxguard_test.gointegrations/sqlxguard/go.modintegrations/sqlxguard/sqlxguard.gointegrations/sqlxguard/sqlxguard_test.gointegrations/xormguard/go.modintegrations/xormguard/xormguard.gointegrations/xormguard/xormguard_test.gomiddleware/cache.gomiddleware/cache_test.gomiddleware/dedup.gomiddleware/dedup_test.gomiddleware/driver.gomiddleware/driver_fallback_test.gomiddleware/driver_test.gomiddleware/guard.gomiddleware/n_plus_one.gomiddleware/n_plus_one_test.gomiddleware/options.goparsers/mysqlparser/go.modparsers/mysqlparser/mysqlparser.goparsers/mysqlparser/mysqlparser_test.goparsers/pgparser/go.modparsers/pgparser/pgparser.goparsers/pgparser/pgparser_test.goreporter/console.goreporter/console_test.goreporter/json.goreporter/json_test.goreporter/reporter.gosqlguard.go
Comment on lines
+7
to
+33
| require ( | ||
| github.com/auxten/postgresql-parser v1.0.1 | ||
| github.com/certifi/gocertifi v0.0.0-20200922220541-2c3bb06c6054 // indirect | ||
| github.com/cockroachdb/apd v1.1.1-0.20181017181144-bced77f817b4 // indirect | ||
| github.com/cockroachdb/errors v1.8.2 // indirect | ||
| github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f // indirect | ||
| github.com/cockroachdb/redact v1.0.8 // indirect | ||
| github.com/cockroachdb/sentry-go v0.6.1-cockroachdb.2 // indirect | ||
| github.com/dustin/go-humanize v1.0.0 // indirect | ||
| github.com/getsentry/raven-go v0.2.0 // indirect | ||
| github.com/gogo/protobuf v1.3.2 // indirect | ||
| github.com/golang/protobuf v1.4.3 // indirect | ||
| github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect | ||
| github.com/konsorten/go-windows-terminal-sequences v1.0.3 // indirect | ||
| github.com/kr/pretty v0.2.0 // indirect | ||
| github.com/kr/text v0.2.0 // indirect | ||
| github.com/lib/pq v1.9.0 // indirect | ||
| github.com/pkg/errors v0.9.1 // indirect | ||
| github.com/sirupsen/logrus v1.6.0 // indirect | ||
| github.com/spf13/pflag v1.0.10 // indirect | ||
| golang.org/x/sync v0.20.0 // indirect | ||
| golang.org/x/sys v0.0.0-20201214210602-f9fddec55a1e // indirect | ||
| golang.org/x/text v0.3.4 // indirect | ||
| google.golang.org/genproto v0.0.0-20200911024640-645f7a48b24f // indirect | ||
| google.golang.org/grpc v1.33.1 // indirect | ||
| google.golang.org/protobuf v1.25.0 // indirect | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if auxten/postgresql-parser has updates that pull in patched dependencies
gh api repos/auxten/postgresql-parser/releases --jq '.[0] | {tag_name, published_at}'
# Check for known security advisories in the current dependency tree
cd parsers/pgparser
go list -m all | grep -E 'grpc|x/text|logrus|protobuf' | while read mod ver; do
echo "=== $mod $ver ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "'$mod'") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' | jq -r '.data.securityVulnerabilities.nodes[] | select(.vulnerableVersionRange) | "[\(.advisory.severity)] \(.advisory.summary) | Patch: \(.firstPatchedVersion.identifier // "none")"'
doneRepository: KARTIKrocks/sqlguard
Length of output: 1598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S "google\.golang\.org/grpc|grpc-go|logrus|Entry\.Writer|protojson|Unmarshal\(|golang\.org/x/text|x/text/language|rapid reset|gogo/protobuf|cockroachdb|sentry-go" parsers/pgparser || true
rg -n --hidden --no-ignore -S "google\.golang\.org/grpc|logrus|protojson|golang\.org/x/text" parsers/pgparser -g'*.go' || trueRepository: KARTIKrocks/sqlguard
Length of output: 4461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd parsers/pgparser
go mod graph | rg -n 'google\.golang\.org/grpc@|golang\.org/x/text@|github\.com/sirupsen/logrus@|google\.golang\.org/protobuf@' || trueRepository: KARTIKrocks/sqlguard
Length of output: 12170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd parsers/pgparser
# Show whether these modules appear in the build dependency list of ./...
go list -deps ./... | rg -n "google\.golang\.org/grpc|golang\.org/x/text|github\.com/sirupsen/logrus|google\.golang\.org/protobuf" || trueRepository: KARTIKrocks/sqlguard
Length of output: 2521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd parsers/pgparser
go list -f '{{join .Imports "\n"}}' ./... 2>/dev/null | rg -n "google\.golang\.org/grpc|golang\.org/x/text|github\.com/sirupsen/logrus|google\.golang\.org/protobuf" || trueRepository: KARTIKrocks/sqlguard
Length of output: 46
CRITICAL: parsers/pgparser pulls in vulnerable transitive deps via auxten/postgresql-parser v1.0.1.
Building/enabling parsers/pgparser includes these known-vulnerable packages (even if not directly referenced in pgparser source code):
google.golang.org/grpc v1.33.1(CRITICAL): authorization bypass via:pathhandling — patch1.79.3; HTTP/2 Rapid Reset DoS — patches1.58.3/1.57.1/1.56.3golang.org/x/text v0.3.4(HIGH): out-of-bounds read — patch0.3.7; craftedAccept-LanguageDoS — patch0.3.8github.com/sirupsen/logrus v1.6.0(HIGH): DoS viaEntry.Writer()— patch1.9.3google.golang.org/protobuf v1.25.0(HIGH): panic leading to DoS — patch1.29.1; MODERATE:protojson.Unmarshalinfinite loop — patch1.33.0
Upstream auxten/postgresql-parser releases show latest at v1.0.1, so remediation likely requires adding replace/explicit require overrides (or forking) in parsers/pgparser/go.mod to force patched grpc/x/text/logrus/protobuf.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@parsers/pgparser/go.mod` around lines 7 - 33, The go.mod for module
parsers/pgparser pulls in auxten/postgresql-parser v1.0.1 which transitively
requires vulnerable versions of google.golang.org/grpc, golang.org/x/text,
github.com/sirupsen/logrus, and google.golang.org/protobuf; fix by adding
explicit require/replace directives in parsers/pgparser/go.mod to pin those
transitive modules to patched versions (e.g., grpc >= v1.79.3 or at least the
minimum patched release, x/text >= v0.3.8, logrus >= v1.9.3, protobuf >=
v1.33.0/1.29.1 as appropriate), or if upstream auxten/postgresql-parser cannot
be updated, fork/replace auxten/postgresql-parser with a patched fork and point
go.mod to that fork; ensure the changes reference the existing module name
auxten/postgresql-parser v1.0.1 in the replace/require entries so the dependency
graph is overridden.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
analyzer/redact.go (1)
125-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the regressed single-quote comment text.
Line 126 still has the wrong escape marker and a smart quote, so the doc no longer matches
skipSingleQuoted, and it is the current lint hit on this file. Restore the literal tos[i] == '\''.🛠️ Suggested fix
-// literal starting at s[i] == '\”, honoring '' doubled-quote escapes. +// literal starting at s[i] == '\'', honoring '' doubled-quote escapes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@analyzer/redact.go` around lines 125 - 126, The doc comment above the skipSingleQuoted function was regressed with a smart quote and incorrect escape; update the comment text to restore the literal to s[i] == '\'' (i.e., an escaped single-quote) so the comment accurately describes the condition checked in skipSingleQuoted and removes the wrong escape marker and smart quote characters.Source: Linters/SAST tools
reporter/json.go (1)
14-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the output writer immutable or the mutex doesn't actually make this concurrent-safe.
Reportserializes writes, butOutis still exported, so another goroutine can swap it out—or nil it—whilejson.NewEncoder(j.Out)is running. That leaves shared reporter instances racy even after this encode-error handling change.🛠️ Suggested fix
type JSONReporter struct { - Out io.Writer + out io.Writer mu sync.Mutex } // NewJSONReporter creates a JSONReporter that writes to stderr. func NewJSONReporter() *JSONReporter { - return &JSONReporter{Out: os.Stderr} + return &JSONReporter{out: os.Stderr} } @@ - enc := json.NewEncoder(j.Out) + enc := json.NewEncoder(j.out)Also applies to: 36-38, 54-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reporter/json.go` around lines 14 - 21, Make the writer immutable by renaming the exported field Out to an unexported field (e.g., out io.Writer) and only setting it in NewJSONReporter so callers cannot swap or nil it; update JSONReporter usages (including Report and any other methods referencing Out) to use the unexported field name, and ensure NewJSONReporter initializes out to os.Stderr. Alternatively, if you must allow swapping, guard all accesses and mutations of Out with the existing mu (lock in setter and lock around json.NewEncoder(j.out) in Report) and nil-check before use; reference the JSONReporter type, its Out field, mu mutex, NewJSONReporter constructor, and Report method when making the change.cmd/sqlguard/scan.go (1)
117-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the
filepath.Abserror handling inshouldSkipDir.
scanDirnow propagatesfilepath.Abs(dir)failures, but the AST fallback still doesabsPath, _ := filepath.Abs(path)at Line 228. If cwd resolution fails there, directory pruning becomes inconsistent and can skip or retain the wrong subtree.🛠️ Suggested fix
func shouldSkipDir(path, absDir string) error { - absPath, _ := filepath.Abs(path) + absPath, err := filepath.Abs(path) + if err != nil { + return err + } if absPath != absDir { base := filepath.Base(path) if strings.HasPrefix(base, ".") || base == "vendor" || base == "node_modules" {Also applies to: 227-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/sqlguard/scan.go` around lines 117 - 121, The filepath.Abs failure in the AST fallback must be handled the same way as in scanDir: replace the silent ignore (absPath, _ := filepath.Abs(path)) used in shouldSkipDir/AST fallback with proper error handling that propagates or returns the error so directory pruning is consistent; locate the absPath assignment in shouldSkipDir/AST fallback and change it to call filepath.Abs(path), check the error and return it (or bubble it up through shouldSkipDir/scanDir) rather than discarding the error, ensuring callers like scanDir consistently handle and propagate cwd resolution failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@analyzer/redact.go`:
- Around line 125-126: The doc comment above the skipSingleQuoted function was
regressed with a smart quote and incorrect escape; update the comment text to
restore the literal to s[i] == '\'' (i.e., an escaped single-quote) so the
comment accurately describes the condition checked in skipSingleQuoted and
removes the wrong escape marker and smart quote characters.
In `@cmd/sqlguard/scan.go`:
- Around line 117-121: The filepath.Abs failure in the AST fallback must be
handled the same way as in scanDir: replace the silent ignore (absPath, _ :=
filepath.Abs(path)) used in shouldSkipDir/AST fallback with proper error
handling that propagates or returns the error so directory pruning is
consistent; locate the absPath assignment in shouldSkipDir/AST fallback and
change it to call filepath.Abs(path), check the error and return it (or bubble
it up through shouldSkipDir/scanDir) rather than discarding the error, ensuring
callers like scanDir consistently handle and propagate cwd resolution failures.
In `@reporter/json.go`:
- Around line 14-21: Make the writer immutable by renaming the exported field
Out to an unexported field (e.g., out io.Writer) and only setting it in
NewJSONReporter so callers cannot swap or nil it; update JSONReporter usages
(including Report and any other methods referencing Out) to use the unexported
field name, and ensure NewJSONReporter initializes out to os.Stderr.
Alternatively, if you must allow swapping, guard all accesses and mutations of
Out with the existing mu (lock in setter and lock around json.NewEncoder(j.out)
in Report) and nil-check before use; reference the JSONReporter type, its Out
field, mu mutex, NewJSONReporter constructor, and Report method when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e60e9bc0-c742-4a35-875a-fbe7b10855a2
📒 Files selected for processing (3)
analyzer/redact.gocmd/sqlguard/scan.goreporter/json.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pgparserthat provides structural analysis of SQL queries, including support for SELECT, INSERT, UPDATE, and DELETE statements.ConsoleReporterfor outputting analysis results to the terminal with color-coded severity levels.JSONReporterfor outputting analysis results in JSON format.Reporterinterface for consistent reporting of analysis results across different formats.sqlguardpackage to support the new PostgreSQL parser and reporting features.Summary by CodeRabbit
New Features
Documentation
Chores
Tests