codeant-ai-for-open-source[bot] commented on PR #36722: URL: https://github.com/apache/superset/pull/36722#issuecomment-3668338210
## 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/36722/files#diff-60f1a27918bbfd5fd3e1ee91d7e37281577729f72b52eb6a6949371426aa2a54R176-R186'><strong>Possible SQL injection</strong></a><br>The PR adds validation for `cancel_query_id` (numeric), but the cancel SQL is still built using Python string interpolation (f-string). Relying solely on validation increases risk if the validation is ever changed or bypassed. Prefer using a parameterized query to remove interpolation entirely.<br> - [ ] <a href='https://github.com/apache/superset/pull/36722/files#diff-021730eaa7a0388769fa4bec2a421b292a5aa09002f8f1eb1cbbf2a79b980341R281-R288'><strong>SQL injection risk</strong></a><br>The code still uses string interpolation to build the SQL executed by the cursor: `cursor.execute(f"SELECT SYSTEM$CANCEL_ALL_QUERIES({cancel_query_id})")`. Although the new validation reduces risk, relying on a regex and an f-string leaves a surface where future changes to the pattern or a missed validation could reintroduce injection. Prefer parameterized execution where the DB-API/driver supports it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36722/files#diff-d2f5299660a67fb698ed13825f1bf74b253d559d48a40206320008675003776eR301-R304'><strong>SQL string interpolation</strong></a><br>The code still constructs the CALL statement with f-string interpolation of `cancel_query_id`. Even with validation this is fragile. Prefer using parameterized execution or the driver's native cancellation API to avoid accidental injection if validation or patterns change.<br> - [ ] <a href='https://github.com/apache/superset/pull/36722/files#diff-8cec2642bdfebf02492bfb132c78133c229f409661a4e56c8ea96868a1afd63fR476-R486'><strong>Residual SQL injection risk</strong></a><br>The function still constructs the cancel SQL using string interpolation. Although the new validation reduces risk, using interpolated SQL literals can be fragile if validation changes or is bypassed. Prefer parameterized queries or explicit type coercion to eliminate string interpolation entirely.<br> - [ ] <a href='https://github.com/apache/superset/pull/36722/files#diff-41b8cbe85e196c2930177f3364dcc7e76c6323d50ec2c24daedc3d48b1d21909R544-R551'><strong>Interpolated SQL still risky</strong></a><br>The code validates `cancel_query_id` with a regex but still injects the (string) value into SQL via an f-string. If validation is ever bypassed or the pattern is changed, this interpolation could be exploited. Stronger sanitization (e.g., explicit integer casting of each whitespace-separated token) or constructing the command from integers should be used to guarantee safety.<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]
