Address Codacy security findings - #107
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated CI Go version to 1.26, added explicit Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
mason-sharp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/consistency/mtree/merkle.go (1)
379-388: Scopeno semgrepsuppressions to specific rule IDs.Line 379/388/442/451/474/483 use broad suppression comments. Prefer rule-targeted suppressions plus a short reason so future unrelated findings on these lines aren’t accidentally masked.
Example refinement
- rowsH1, err := pool1.Query(m.Ctx, rowHashQuery, args...) // nosemgrep + rowsH1, err := pool1.Query(m.Ctx, rowHashQuery, args...) // nosemgrep: go.lang.security.sql-injection -- identifiers are sanitized with pgx.Identifier.Sanitize and values are parameterizedAlso applies to: 442-452, 474-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/mtree/merkle.go` around lines 379 - 388, The broad "// nosemgrep" suppressions on the pool1.Query and pool2.Query calls (and the other occurrences noted) should be scoped to specific rule IDs and include a short reason; replace each bare "// nosemgrep" comment with a targeted form like "// nosemgrep:<RULE-ID> -- <short reason>" (e.g., reference the call sites pool1.Query, pool2.Query, the m.Ctx usage, and the rowHashQuery/readRowHashes logic) so only the intended Semgrep rule is suppressed and future findings aren’t masked; update all occurrences mentioned (the lines around the pool1.Query/pool2.Query blocks and the similar places referenced) to follow this pattern.internal/consistency/mtree/merkle_test.go (1)
37-160: Add one regression case for identifier-quoting behavior.Given the security goal of this change, add a test that asserts schema/table identifiers are always quoted/sanitized (e.g., mixed-case or embedded quotes) to lock this behavior in.
Example test case addition
tests := []struct { name string schema string table string @@ }{ + { + name: "schema/table identifiers are sanitized", + schema: `Public`, + table: `Order"Items`, + key: []string{"id"}, + cols: []string{"id"}, + whereClause: "TRUE", + colTypes: nil, + wantContains: []string{ + `FROM "Public"."Order""Items"`, + }, + wantOrderBy: `"id"`, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/mtree/merkle_test.go` around lines 37 - 160, Add a regression test case to TestBuildRowHashQuery that verifies schema/table identifiers are always quoted and sanitized: add a test entry (e.g., name "identifier quoting", schema with mixed-case and an embedded quote like `MiXeD"Sch`, table like `weird"Table`, key e.g., ["id"], cols include "id") and assert the returned query contains the properly double-quoted/escaped schema and table (e.g., quotes around identifiers and internal quotes doubled), that ORDER BY uses the quoted key, and that buildRowHashQuery is used to generate the query and orderBy to validate the exact quoted identifiers..github/workflows/test.yml (1)
20-20: Usego-version-file: go.modto keep workflow Go version in sync with the module.Currently pinning
1.25whilego.modrequires1.25.9. This allows patch drift and reduces reproducibility. Either usego-version-file: go.modor explicitly pin to1.25.9.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml at line 20, The workflow currently pins go-version: '1.25' which can drift from the module's exact runtime; update the step to either use go-version-file: go.mod so the runner reads the Go version from your module or explicitly pin go-version to '1.25.9' to match go.mod; change the key from go-version to go-version-file: go.mod (or set go-version: '1.25.9') in the job that defines the Go setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yaml:
- Line 34: Update the GoReleaser cross image tag to match the Go version in
go.mod: replace the goreleaser image tag "v1.25.8" with "v1.25.9" (the tag shown
as goreleaser/goreleaser-cross:v1.25.8) so the workflow's toolchain aligns with
the Go version declared in go.mod (Go 1.25.9); ensure the updated tag appears
wherever goreleaser/goreleaser-cross is referenced in the workflow.
In `@internal/consistency/diff/table_diff.go`:
- Line 338: The nosemgrep suppressions hide a real SQL injection:
buildEffectiveFilter() currently constructs EffectiveFilter by
string-concatenating user-provided TableFilter and that raw string is embedded
into SQL via templates and fmt.Sprintf in the queries executed (see the
pool.QueryRow/Query calls that reference EffectiveFilter and the templates in
db/queries/templates.go). Fix by removing the suppressions and replacing string
embedding with a safe approach: change buildEffectiveFilter() to either (A)
parse/validate the incoming TableFilter against a strict
whitelist/SQL-expression grammar (or build an AST) and produce a parameterized
predicate plus a slice of parameters, or (B) refactor the API to accept
structured filter objects (column/operator/value) and build the WHERE clause
using placeholders. Then update the query templates and the code paths that call
pool.QueryRow / pool.Query (the functions that currently fmt.Sprintf
EffectiveFilter into SQL) to use prepared/parameterized queries and pass the
parameters instead of injecting the raw string. Also add input validation in the
HTTP handler that accepts TableFilter to reject anything not matching the
allowed grammar/whitelist.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Line 20: The workflow currently pins go-version: '1.25' which can drift from
the module's exact runtime; update the step to either use go-version-file:
go.mod so the runner reads the Go version from your module or explicitly pin
go-version to '1.25.9' to match go.mod; change the key from go-version to
go-version-file: go.mod (or set go-version: '1.25.9') in the job that defines
the Go setup.
In `@internal/consistency/mtree/merkle_test.go`:
- Around line 37-160: Add a regression test case to TestBuildRowHashQuery that
verifies schema/table identifiers are always quoted and sanitized: add a test
entry (e.g., name "identifier quoting", schema with mixed-case and an embedded
quote like `MiXeD"Sch`, table like `weird"Table`, key e.g., ["id"], cols include
"id") and assert the returned query contains the properly double-quoted/escaped
schema and table (e.g., quotes around identifiers and internal quotes doubled),
that ORDER BY uses the quoted key, and that buildRowHashQuery is used to
generate the query and orderBy to validate the exact quoted identifiers.
In `@internal/consistency/mtree/merkle.go`:
- Around line 379-388: The broad "// nosemgrep" suppressions on the pool1.Query
and pool2.Query calls (and the other occurrences noted) should be scoped to
specific rule IDs and include a short reason; replace each bare "// nosemgrep"
comment with a targeted form like "// nosemgrep:<RULE-ID> -- <short reason>"
(e.g., reference the call sites pool1.Query, pool2.Query, the m.Ctx usage, and
the rowHashQuery/readRowHashes logic) so only the intended Semgrep rule is
suppressed and future findings aren’t masked; update all occurrences mentioned
(the lines around the pool1.Query/pool2.Query blocks and the similar places
referenced) to follow this pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27132716-a2e4-4d98-a891-f4ede8ee17bf
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.github/workflows/release.yaml.github/workflows/test.ymlDockerfiledb/queries/queries.gogo.modinternal/consistency/diff/table_diff.gointernal/consistency/diff/table_rerun.gointernal/consistency/mtree/merkle.gointernal/consistency/mtree/merkle_test.gointernal/consistency/repair/stale_repair.gointernal/consistency/repair/table_repair.gointernal/infra/cdc/listen.gointernal/infra/db/auth.go
Member
Author
|
Will rebase and modify as a small follow up to #109 |
Sanitize SQL identifiers in merkle tree queries and validate origin filter
- buildRowHashQuery, buildFetchRowsSQLSimple, buildFetchRowsSQLComposite
now take separate schema/table args and quote them with
pgx.Identifier.Sanitize() instead of interpolating a raw
QualifiedTableName string.
- buildEffectiveFilter: replace fragile strings.ReplaceAll("'","''")
escaping of resolvedAgainstOrigin with strconv.Atoi validation. Spock
node IDs are integers; reject non-numeric values instead of escaping.
Add nosemgrep annotations for false-positive SQL injection findings
All flagged query sites use pgx.Identifier.Sanitize() for identifiers
and parameterized placeholders ($N) for values. Annotate each with the
specific reason so opengrep suppressions survive line shifts.
Bump Go version pin GitHub Actions to commit SHAs, update dependencies
Add explicit USER nonroot to Dockerfile for Codacy scanner
mason-sharp
force-pushed
the
fix/SPOC-467/codacy-security
branch
from
April 13, 2026 21:21
419381a to
630a7cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/consistency/diff/table_diff.go`:
- Around line 255-258: The code validates t.resolvedAgainstOrigin with
strconv.Atoi but then interpolates the original string, which can cause
mismatches for equivalent numeric forms; change the logic to parse and normalize
the value (e.g. use strconv.Atoi or strconv.ParseInt to get an integer) and use
the normalized numeric string (via strconv.Itoa or fmt.Sprintf("%d", parsed))
when building the SQL predicate in the parts append call so the predicate uses
the canonical numeric form of t.resolvedAgainstOrigin.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2da19a3f-0f6d-449d-bfc5-0be6f424fb86
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.github/workflows/test.ymlDockerfiledb/queries/queries.gogo.modinternal/consistency/diff/table_diff.gointernal/consistency/diff/table_rerun.gointernal/consistency/mtree/merkle.gointernal/consistency/mtree/merkle_test.gointernal/consistency/repair/stale_repair.gointernal/consistency/repair/table_repair.gointernal/infra/cdc/listen.gointernal/infra/db/auth.go
✅ Files skipped from review due to trivial changes (9)
- .github/workflows/test.yml
- internal/infra/cdc/listen.go
- go.mod
- internal/infra/db/auth.go
- internal/consistency/diff/table_rerun.go
- db/queries/queries.go
- internal/consistency/repair/stale_repair.go
- internal/consistency/repair/table_repair.go
- internal/consistency/mtree/merkle_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
Addresses Coderabbit feedback
rasifr
approved these changes
Apr 14, 2026
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.
pgx.Identifier.Sanitize()insteadof interpolating raw table names
resolvedAgainstOriginwithstrconv.Atoivalidation
// nosemgrepannotations to false-positive SQL injection findings (all useparameterized queries or sanitized identifiers)
CVE-2026-32289, CVE-2026-32288)
USER nonrootto DockerfileNot addressed
docker/docker v28.5.2(CVE-2026-33997) — test-only transitive dependency viatestcontainers-go. v29 is not published as a Go module. Blocked upstream
(testcontainers-go#2256).
Test plan
go test ./... -short)TestBuildRowHashQuerypasses (validates merkle identifier quoting change)