codeant-ai-for-open-source[bot] commented on PR #36529: URL: https://github.com/apache/superset/pull/36529#issuecomment-3647796013
## 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/36529/files#diff-039906680d45ada0b3a137d5ac10196041b60d29b1a4378d1ab74948e08ee57eR338-R351'><strong>Sensitive information exposure</strong></a><br>The query logging function passes database.sqlalchemy_uri to the configured QUERY_LOGGER callback. That URI may contain credentials or other sensitive connection details; logging or transmitting it without redaction can leak secrets. Ensure the URI is sanitized/redacted before being logged or make it optional.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1279-R1309'><strong>Permission checks</strong></a><br>The new execute_async method delegates directly to SQLExecutor without validating database-level flags (e.g., `allow_run_async`) or checking DML permissions. Confirm that SQLExecutor enforces these checks; if not, callers may run async queries or DML against databases that should be restricted.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-039906680d45ada0b3a137d5ac10196041b60d29b1a4378d1ab74948e08ee57eR111-R117'><strong>Payload serialization</strong></a><br>_serialize_payload returns a bytes object for msgpack but a str for JSON. Downstream code (zlib_compress, results backend storage, and size checks) assumes bytes and uses sys.getsizeof() for size; this can cause incorrect size checks or encoding errors. Also using getsizeof on container objects may not reflect true byte length. Validate and normalize payload to bytes before compressing and use len() for byte-size checks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-a940d28f7bb57d9cc26584bd90023fea1be50531974c6b71ba455266b96fbe49R578-R602'><strong>Disallowed function detection</strong></a><br>_check_disallowed_functions detects disallowed SQL functions by checking whether the function name is a substring of the statement string. This is prone to false positives (e.g. matching identifiers, comments, or partial words). Detection should be done with word boundaries or via AST-aware checks to avoid blocking valid SQL.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-a940d28f7bb57d9cc26584bd90023fea1be50531974c6b71ba455266b96fbe49R89-R171'><strong>Multi-statement result handling</strong></a><br>execute_sql_with_cursor fetches intermediate results with `cursor.fetchall()`. Some DB-API drivers expose additional result sets via `cursor.nextset()` and may not support fetchall for intermediate statements. This can cause failures or missed resultset advancement on some backends; consider using `nextset()` when available and handling drivers that don't implement it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-5cd37c5eb2726beb8fd5f49f36a9516170e5a8402e6d5236fa40bf8fc5be40faR84-R111'><strong>Brittle argument inspection</strong></a><br>Some tests (e.g., verifying that `get_raw_connection` receives `catalog` and `schema`) inspect `call_args[1]` directly. If the call uses positional args rather than kwargs this will raise or silently fail. The assertions should handle both positional and keyword invocation to avoid flakiness.<br> - [ ] <a href='https://github.com/apache/superset/pull/36529/files#diff-5cd37c5eb2726beb8fd5f49f36a9516170e5a8402e6d5236fa40bf8fc5be40faR359-R408'><strong>Duplicate Test</strong></a><br>There are two tests that appear to be identical and duplicate coverage of RLS enforcement. This increases maintenance burden and may mask subtle differences in future changes. Consider removing or consolidating the duplicate test to avoid redundant assertions and confusion.<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]
