Conversation
…it and ORDER BY (#289, #290) Two SQL injection vulnerabilities fixed: 1. SET memory_limit in duckdb.go and subprocess.go interpolated the configured value without escaping single quotes. Applied escapeSQLString() in duckdb.go and inline single-quote doubling in subprocess.go (different package, avoids cross-package dependency). 2. buildOrderByClause in job.go wrapped sort key names in double quotes but did not escape internal double quotes, allowing identifier breakout. Added strings.ReplaceAll(key, "\"", "\"\"") before quoting, matching the pattern already used in dedup.go for tag columns. Config validation regex (memoryLimitRe) provides defense-in-depth at load time, but the SQL escaping is now a failsafe at the point of use.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses two SQL injection vulnerabilities: one in the DuckDB memory limit configuration and another in the compaction ORDER BY clause. The changes implement proper SQL escaping for memory limit values and sort key identifiers. I have no further feedback, as the reviewer's suggestion to centralize the escaping logic is a valid improvement for maintainability and consistency across the codebase.
…review Moved inline single-quote escaping in subprocess.go to a dedicated escapeSQLString() function in job.go (alongside the existing escapeSQLPath helper), matching the pattern in database/duckdb.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
1 participant
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.
Summary
SET memory_limitin bothduckdb.goandsubprocess.gointerpolated the configured value without escaping single quotes — appliedescapeSQLString()/ inline single-quote doublingbuildOrderByClauseinjob.gowrapped sort keys in double quotes but didn't escape internal"characters — addedstrings.ReplaceAll(key, "\"", "\"\"")before quoting, matching the pattern already used indedup.gofor tag columnsTest plan
go build ./internal/...passesgo test ./internal/compaction/...passesgo test ./internal/database/...passesCloses #289
Closes #290