codeant-ai-for-open-source[bot] commented on PR #35683: URL: https://github.com/apache/superset/pull/35683#issuecomment-3665030600
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-8c934159db24b77a606373070946ced7e10204f92c8e53570722d6ee3eaaa85eR465-R496'><strong>Potential SQL injection</strong></a><br>User-controlled strings (e.g., `ownState.agGridComplexWhere`, `ownState.agGridHavingClause`) are concatenated into `extras.where` / `extras.having` without sanitization or parameterization. These values may be manipulated and can lead to SQL injection or malformed queries on the backend.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-680124ca12d5c696269088683bfa8ca10fee0d2e3f15aff07e3e9ab4cf1033bdR289-R296'><strong>SQL injection</strong></a><br>Filter values and column identifiers are interpolated directly into SQL fragments without escaping or parameterization. Examples: text filters, set values and colId are used inside quotes/backticks directly. Malicious or accidental user input (including column ids supplied by upstream code) could break SQL or be used to inject payloads. Ensure proper escaping or prefer parameterized queries / server-side validation.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-680124ca12d5c696269088683bfa8ca10fee0d2e3f15aff07e3e9ab4cf1033bdR158-R187'><strong>Unvalidated column identifiers</strong></a><br>Column identifiers (`colId`) are directly injected into SQL fragments. If `colId` contains unexpected characters (spaces, quotes, SQL keywords), it can break the generated SQL or enable injection. At minimum validate colId against an allow-list / pattern or quote/escape identifiers properly for the target SQL dialect.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-091ae8a488fee86a5316b8175df3acddecaf8904ba893f55437cd60f37d0650dR170-R184'><strong>Column name validation / SQL injection risk</strong></a><br>Column names are injected directly into SQL clauses via template strings. Although there's a regex validator, it allows many characters (spaces, parentheses, %, *, +, -, /) which may still enable unexpected SQL fragments in some dialects. Column names should be strictly quoted or whitelisted per known dataset columns, or the validator tightened.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-091ae8a488fee86a5316b8175df3acddecaf8904ba893f55437cd60f37d0650dR71-R85'><strong>Incorrect SQL equality operator</strong></a><br>The SQL equality operator is defined as '==' (JS/other languages) instead of the standard SQL '='. This will generate invalid SQL or unexpected behavior when equality filters are used.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-888cc9d72e504a2c52077bddadbadbcbce423d45adffa6b53e0bd8888589a330R196-R213'><strong>Behavior/performance change</strong></a><br>The `resultType` for JSON and Excel exports was changed to `full`. That can request the entire dataset from the backend (potentially large) and change semantics compared to prior `results`. Confirm this is intended and validate performance and UX (timeouts, large downloads). Also ensure server-side supports this mode for the requested export formats.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-595987046ff00d1af428f8b7360bdd1372bef133a42e58ff8ede585ea98cd254R2221-R2235'><strong>None guard for LIKE/NOT_LIKE</strong></a><br>The new NOT_LIKE/NOT_ILIKE handling uses `sqla_col.not_like(eq)` / `sqla_col.not_ilike(eq)` but does not guard against `eq` being `None`. If `eq` can be `None` (e.g., when client passes special NULL_STRING or EMPTY_STRING processed earlier), passing `None` into `.not_like()`/`.not_ilike()` may raise a SQLAlchemy / DB error or produce unexpected SQL. Consider validating `eq` before building the condition and raising a QueryObjectValidationError or handling null semantics explicitly.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-090abddb6d9306c432a5642a19da74b293f5fe31f6b3ac707bf0314d421693c8R269-R273'><strong>SQL dialect compatibility</strong></a><br>Not all SQL dialects support `ILIKE`/`NOT ILIKE` (e.g., MySQL). Confirm the SQL generation logic that consumes `FilterOperator` translates `NOT ILIKE` to a dialect-safe expression (for example, use LOWER(..) NOT LIKE LOWER(..) for dialects without ILIKE) and properly escapes values to avoid logic or injection issues.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-090abddb6d9306c432a5642a19da74b293f5fe31f6b3ac707bf0314d421693c8R269-R273'><strong>Consistency mismatch</strong></a><br>A new operator `NOT ILIKE` was added to `FilterOperator`. Ensure there are corresponding entries/usages across the codebase (e.g., FilterStringOperators, any operator-to-SQL mappings, frontend contract/permalinks, serializers) so the new enum value doesn't cause mismatches or KeyErrors when converting between representations.<br> - [ ] <a href='https://github.com/apache/superset/pull/35683/files#diff-79f6c8dee0430c92fa79feb54af316f6e7d3a951811f38a61123ebce8e5438e0R423-R445'><strong>Date filter coverage</strong></a><br>Date filter tests assert presence of keywords like BETWEEN/IS NOT NULL but don't validate date formatting/quoting or timezone/ISO handling. The SQL produced for dates is sensitive; add stricter checks for date formatting and edge cases (invalid dates, inclusive/exclusive semantics).<br> </td></tr> </table> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
