Advanced SQL Analytics Framework

[hussam789]

User description

PR #9

---

PR Type

Bug fix

---

Description

• Disable SQL expressions to prevent RCE and LFI vulnerabilities

• Remove external DuckDB dependency and replace with stub implementation

• Add feature flag check to block SQL expression queries

• Clean up unused dependencies from go.mod and go.sum

---

Diagram Walkthrough

flowchart LR
  A["SQL Expression Query"] --> B["Feature Flag Check"]
  B --> C["Disabled: Return Error"]
  D["DuckDB Dependency"] --> E["Remove from go.mod"]
  F["SQL Parser"] --> G["Use Stub DB Implementation"]
  G --> H["Return Not Implemented"]

Loading

File Walkthrough

	Relevant files
Bug fix	reader.go Add SQL expressions feature flag validation                            pkg/expr/reader.go Add feature flag check enableSqlExpressions() before processing SQL queries Return error "sqlExpressions is not implemented" when feature is disabled Prevent SQL expression execution at query read time +12/-0    parser.go Replace DuckDB with internal stub implementation                  pkg/expr/sql/parser.go Remove import of external github.com/scottlepp/go-duck/duck package Replace duck.NewInMemoryDB() with internal NewInMemoryDB() call Maintain existing SQL parsing logic with stub implementation +1/-2      sql_command.go Update SQL command to use stub database                                    pkg/expr/sql_command.go Remove import of external github.com/scottlepp/go-duck/duck package Replace duck.NewInMemoryDB() with sql.NewInMemoryDB() call Update variable name from duckDB to db for clarity +2/-3
Enhancement	db.go Create stub database implementation                                            pkg/expr/sql/db.go Create new stub DB struct with placeholder methods Implement TablesList(), RunCommands(), and QueryFramesInto() returning "not implemented" errors Add NewInMemoryDB() factory function Replace external DuckDB dependency with internal stub +26/-0
Dependencies	go.mod Remove DuckDB and transitive dependencies                                go.mod Remove github.com/scottlepp/go-duck v0.1.0 dependency Remove transitive dependencies: github.com/JohnCGriffin/overflow, github.com/apache/thrift v0.20.0, github.com/klauspost/asmfmt, github.com/minio/asm2plan9s, github.com/minio/c2goasm, github.com/hairyhenderson/go-which, github.com/iancoleman/orderedmap +0/-8      go.sum Remove dependency checksums from go.sum                                    go.sum Remove checksums for github.com/scottlepp/go-duck package Remove checksums for transitive dependencies related to DuckDB Clean up unused dependency entries +0/-15    go.work.sum Update workspace dependency checksums                                        go.work.sum Remove checksums for github.com/Azure/azure-sdk-for-go/sdk/storage/azblob Add checksums for github.com/JohnCGriffin/overflow and github.com/apache/thrift Add checksums for github.com/klauspost/asmfmt, github.com/minio/asm2plan9s, github.com/minio/c2goasm Remove checksums for github.com/google/go-replayers packages +6/-6

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Feature flag logic flaw Description: The SQL expressions feature flag check always returns false due to inverted logic and unconditional false return, which may unintentionally disable intended protections or behavior and should be corrected to reflect the actual flag state. reader.go [129-199] Referred Code enabled := enableSqlExpressions(h) if !enabled { return eq, fmt.Errorf("sqlExpressions is not implemented") } q := &SQLExpression{} err = iter.ReadVal(q) if err == nil { eq.Properties = q eq.Command, err = NewSQLCommand(common.RefID, q.Expression) } case QueryTypeThreshold: q := &ThresholdQuery{} err = iter.ReadVal(q) if err == nil { referenceVar, err = getReferenceVar(q.Expression, common.RefID) } if err == nil { // we only support one condition for now, we might want to turn this in to "OR" expressions later if len(q.Conditions) != 1 { return eq, fmt.Errorf("threshold expression requires exactly one condition") ... (clipped 50 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misleading naming: Function enableSqlExpressions always returns false despite variable enabled suggesting the opposite, making the code misleading and hard to understand. Referred Code func enableSqlExpressions(h *ExpressionQueryReader) bool { enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) if enabled { return false } return false }
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Generic not implemented: Stub DB methods return generic "not implemented" errors without actionable context (operation, inputs) hindering debugging and edge-case handling. Referred Code func (db *DB) TablesList(rawSQL string) ([]string, error) { return nil, errors.New("not implemented") } func (db *DB) RunCommands(commands []string) (string, error) { return "", errors.New("not implemented") } func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { return errors.New("not implemented")
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit log: The new feature-flag gate for SQL expressions returns early without any auditing/logging of the blocked action, which may hinder reconstructing security-relevant events. Referred Code enabled := enableSqlExpressions(h) if !enabled { return eq, fmt.Errorf("sqlExpressions is not implemented") } q := &SQLExpression{}
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Feature gate logic: The SQL expression feature is gated but the enablement logic appears inverted and always disables the path, which may be intentional but cannot be verified from the diff. Referred Code enabled := enableSqlExpressions(h) if !enabled { return eq, fmt.Errorf("sqlExpressions is not implemented") } q := &SQLExpression{}

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Completely remove the disabled feature Instead of disabling the SQL expressions feature with stubs and a hardcoded feature flag, the suggestion recommends its complete removal. This would simplify the codebase, eliminate dead code, and prevent potential future security risks. Examples: pkg/expr/reader.go [128-200] case QueryTypeSQL: enabled := enableSqlExpressions(h) if !enabled { return eq, fmt.Errorf("sqlExpressions is not implemented") } q := &SQLExpression{} err = iter.ReadVal(q) if err == nil { eq.Properties = q eq.Command, err = NewSQLCommand(common.RefID, q.Expression) ... (clipped 63 lines) pkg/expr/sql/db.go [1-26] package sql import ( "errors" "github.com/grafana/grafana-plugin-sdk-go/data" ) type DB struct { } ... (clipped 16 lines) Solution Walkthrough: Before: // pkg/expr/reader.go func (h *ExpressionQueryReader) ReadQuery(...) { // ... case QueryTypeSQL: enabled := enableSqlExpressions(h) // This always returns false if !enabled { return eq, fmt.Errorf("sqlExpressions is not implemented") } // ... dead code follows ... q := &SQLExpression{} // ... } // pkg/expr/sql/db.go // New stub implementation that is now dead code. type DB struct {} func (db *DB) QueryFramesInto(...) error { return errors.New("not implemented") } After: // pkg/expr/reader.go func (h *ExpressionQueryReader) ReadQuery(...) { // ... // The 'case QueryTypeSQL' block is completely removed. // ... default: return eq, fmt.Errorf("unsupported expression type: %q", eq.QueryType) } // pkg/expr/sql/* // The entire pkg/expr/sql directory and related files like // pkg/expr/sql_command.go are removed. // go.mod & go.sum // All related dependencies are removed. Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the PR's method of disabling the SQL feature is overly complex, creating dead code by adding a feature flag that always returns false while also replacing the implementation with stubs. Removing the feature entirely is a much cleaner and more secure approach.	High
Possible issue	Fix broken feature flag logic The enableSqlExpressions function always returns false due to flawed logic. Correct the function to return the actual value of the FlagSqlExpressions feature flag. pkg/expr/reader.go [194-200] func enableSqlExpressions(h *ExpressionQueryReader) bool { - enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) - if enabled { - return false - } - return false + return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a logical flaw in the enableSqlExpressions function, which always returns false, and proposes a fix to correctly use the feature flag, which is a significant improvement to the code's correctness and maintainability.	High
More