Advanced SQL Analytics Framework #2
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <[email protected]>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR removes the DuckDB (duck) dependency from go.mod and introduces a local in-memory SQL database abstraction. SQL expression support is now gated behind an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Reader as ExpressionQueryReader
participant Gate as enableSqlExpressions()
participant DB as SQL DB
Client->>Reader: Process SQL Query
Reader->>Gate: Check if enabled
Gate-->>Reader: Returns false
Reader-->>Client: Error: sqlExpressions not implemented
Note over Reader,DB: SQL expression processing blocked<br/>until enableSqlExpressions returns true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/expr/sql/db.go (1)
9-26: DB methods are pure stubs; any usage will always fail at runtimeAll three methods (
TablesList,RunCommands,QueryFramesInto) currently just return"not implemented". That’s fine as scaffolding, but note that:
TablesListis already used inNewSQLCommand, so any attempt to construct a SQLCommand will surface this as an “error serializing sql” / invalid‑SQL error instead of a clear “feature not implemented” signal.- Once
enableSqlExpressionsis hooked up to the feature flag, these will become hot paths.Consider either:
- Implementing minimal behavior before wiring the feature flag on, or
- Making the stubs clearly intentional (e.g., dedicated
ErrNotImplementedSQLDBand/or TODOs) so callers distinguish between user SQL errors and unimplemented engine behavior.pkg/expr/sql/parser.go (1)
23-27: RenameduckDBvariable now that duck dependency is removed
duckDB := NewInMemoryDB()still uses the old duck‑specific name even though it now points to the in‑package DB. For clarity and to avoid confusion when reading logs or future diffs, consider renaming:-func TablesList(rawSQL string) ([]string, error) { - duckDB := NewInMemoryDB() +func TablesList(rawSQL string) ([]string, error) { + db := NewInMemoryDB() rawSQL = strings.Replace(rawSQL, "'", "''", -1) cmd := fmt.Sprintf("SELECT json_serialize_sql('%s')", rawSQL) - ret, err := duckDB.RunCommands([]string{cmd}) + ret, err := db.RunCommands([]string{cmd})This better reflects the current implementation and removes stale references to the old duckDB backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(0 hunks)pkg/expr/reader.go(2 hunks)pkg/expr/sql/db.go(1 hunks)pkg/expr/sql/parser.go(1 hunks)pkg/expr/sql_command.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🔇 Additional comments (2)
pkg/expr/sql_command.go (1)
96-101: Wiring to sql.NewInMemoryDB looks correct, but behavior hinges on DB implementationCreating a fresh
db := sql.NewInMemoryDB()and callingdb.QueryFramesInto(...)matches the new DB interface and keeps the Execute logic localized to this file. Given thatQueryFramesIntocurrently returns"not implemented", this path will only behave as expected once the DB is fully implemented and the SQL feature gate is enabled, so keep those changes coordinated.pkg/expr/reader.go (1)
128-133: The original review comment is incorrectBased on git history, the most recent commit to
pkg/expr/reader.gois "ea71201d ServerSideExpressions: Disable SQL Expressions to prevent RCE and LFI vulnerability (#94942)". TheenableSqlExpressionsfunction returningfalseunconditionally is intentional — it's a security fix to prevent remote code execution and local file inclusion vulnerabilities. This is not a temporary rollout guard requiring clarification; it's a deliberate safety mechanism. The code is working as designed.Likely an incorrect or invalid review comment.
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
enableSqlExpressions ignores the feature flag and always returns false
The current implementation:
- Negates
IsEnabledGlobally, stores it inenabled, but then returnsfalsein all paths. - This is equivalent to
return falseand makes the feature flag usage misleading.
If the intent is to gate behavior on featuremgmt.FlagSqlExpressions, either wire it through properly or remove the unused flag reference for now. For example:
-func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
-}
+func enableSqlExpressions(h *ExpressionQueryReader) bool {
+ // TODO: adjust logic if additional rollout checks are needed.
+ return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
+}This keeps the code aligned with the toggle once the underlying SQL implementation is ready.
I need to verify the code state and understand the function's usage context. Let me gather this information.
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the enableSqlExpressions function at the specified lines
cat pkg/expr/reader.go | sed -n '194,200p'
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for all usages of enableSqlExpressions function
rg -n "enableSqlExpressions" --type go -B2 -A2Length of output: 518
🏁 Script executed:
#!/bin/bash
# Look for any tests related to this function
fd -e "test.go" -e "_test.go" | xargs rg -l "enableSqlExpressions" 2>/dev/null || echo "No test files found"Length of output: 18
🏁 Script executed:
#!/bin/bash
# Check the broader context around the function definition
cat pkg/expr/reader.go | sed -n '180,210p'Length of output: 498
enableSqlExpressions always returns false, ignoring the feature flag
The function at lines 194-200 negates the feature flag but then returns false in both code paths. At the call site (line 129), SQL expressions are unconditionally blocked with an error, making the flag check meaningless.
Fix: Replace with:
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
+ // TODO: adjust logic if additional rollout checks are needed.
+ return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| if enabled { | |
| return false | |
| } | |
| return false | |
| } | |
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| // TODO: adjust logic if additional rollout checks are needed. | |
| return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| } |
🤖 Prompt for AI Agents
In pkg/expr/reader.go around lines 194 to 200, enableSqlExpressions currently
always returns false (it negates the feature flag into `enabled` then returns
false in both paths); replace this with a correct check that returns the actual
feature flag state. Change the function to return the result of
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) (remove the
incorrect negation and the unconditional false return) so callers receive the
true/false value from the feature flag.
Test 9
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/grafana-coderabbit#9