Advanced SQL Analytics Framework#10
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <scott.lepper@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request removes the external go-duck dependency and replaces it with a custom in-memory database implementation in the sql package. A feature flag check is introduced to gate SQL expression processing. The new DB type provides stub methods for future implementation while maintaining existing control flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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
🤖 Fix all issues with AI agents
In @pkg/expr/reader.go:
- Around line 194-200: The function enableSqlExpressions computes enabled using
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) but then always
returns false, which is either a bug or confusing dead code; fix by making the
return match intent: if you intend to always disable SQL expressions, replace
the body with a single unconditional "return false" (removing the computed
enabled variable and branches), otherwise return the computed value (i.e.,
return enabled or return !enabled depending on desired polarity) so that
enableSqlExpressions correctly reflects the feature flag check in
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions).
📜 Review details
Configuration used: defaults
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.modpkg/expr/reader.gopkg/expr/sql/db.gopkg/expr/sql/parser.gopkg/expr/sql_command.go
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/expr/sql/db.go (1)
pkg/expr/sql/parser.go (1)
TablesList(22-40)
pkg/expr/sql_command.go (1)
pkg/expr/sql/db.go (1)
NewInMemoryDB(24-26)
pkg/expr/sql/parser.go (1)
pkg/expr/sql/db.go (1)
NewInMemoryDB(24-26)
pkg/expr/reader.go (1)
pkg/services/featuremgmt/toggles_gen.go (1)
FlagSqlExpressions(584-584)
🔇 Additional comments (4)
pkg/expr/reader.go (1)
128-138: LGTM on the gating pattern.The feature flag check correctly prevents SQL expression processing when disabled, returning a clear error message. This is the appropriate pattern for security-gating functionality.
pkg/expr/sql/db.go (1)
1-26: Defense-in-depth stub implementation looks correct.The stub methods correctly return errors for all operations, ensuring that even if the feature gate in
reader.gois somehow bypassed, SQL execution will still fail. This provides appropriate defense-in-depth against the RCE/LFI vulnerability mentioned in the PR.pkg/expr/sql/parser.go (1)
21-40: Acknowledge:TablesListwill now always fail.With
RunCommandsreturning "not implemented", this function will always fail at line 26. This is consistent with the PR's goal of disabling SQL expressions, but it means any code path reachingTablesListwill receive an error. The existing error handling correctly wraps and logs the failure.pkg/expr/sql_command.go (1)
96-104: LGTM on stub integration.The switch to
sql.NewInMemoryDB()correctly routes through the stub implementation. The error fromQueryFramesInto("not implemented") is properly captured inrsp.Errorand returned, preventing any SQL execution while maintaining the existing error-handling contract.
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Logic error: function always returns false regardless of feature flag state.
The enableSqlExpressions function computes enabled but ignores it—both branches return false. If the intent is to permanently disable SQL expressions (as suggested by the PR title about preventing RCE/LFI), this achieves that goal but in a confusing way. The dead code suggests either:
- A copy-paste error where the return statements should differ, or
- Intentional disabling, in which case the function body should simply be
return false
Suggested fix (if intent is to always disable)
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
return false
}Suggested fix (if intent is to respect the feature flag)
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
+ 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 { | |
| return false | |
| } |
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| if enabled { | |
| return false | |
| } | |
| return false | |
| } | |
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| } |
🤖 Prompt for AI Agents
In @pkg/expr/reader.go around lines 194 - 200, The function enableSqlExpressions
computes enabled using
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) but then always
returns false, which is either a bug or confusing dead code; fix by making the
return match intent: if you intend to always disable SQL expressions, replace
the body with a single unconditional "return false" (removing the computed
enabled variable and branches), otherwise return the computed value (i.e.,
return enabled or return !enabled depending on desired polarity) so that
enableSqlExpressions correctly reflects the feature flag check in
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions).
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 9nn
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#9](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/9)*