codeant-ai-for-open-source[bot] commented on PR #36539: URL: https://github.com/apache/superset/pull/36539#issuecomment-3642901284
## 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/36539/files#diff-bfa222bca38457f6b2cf8b875aad06d2a05be78afab468c08559543797dc25e3R456-R466'><strong>Sensitive data exposure</strong></a><br>The PR now returns the full `form_data` and `form_data_key` to callers. `form_data` can contain filters, SQL snippets, ad-hoc queries, or other fields that may expose sensitive information (PII, credentials leaked in parameters, or private logic). Verify that returning raw `form_data` is acceptable for all clients and that it is sanitized/enforced by authorization before being returned.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-c53108521a676f135ad7846c809dddc3eb33240abb7d123d0948cd5f7b2e9765R1158-R1166'><strong>Sensitive Data Exposure</strong></a><br>`form_data` often contains SQL fragments, filter values, tokens, or dynamic parameters that could include sensitive information. Returning the full form_data to external clients without redaction or whitelisting may leak secrets (API keys, credentials) or PII. The pipeline that populates this schema must scrub or whitelist keys before serialization.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-bfa222bca38457f6b2cf8b875aad06d2a05be78afab468c08559543797dc25e3R304-R311'><strong>CommandParameters usage</strong></a><br>The `CommandParameters` instantiation passes `form_data=json.dumps(...)` and sets fields such as `tab_id=None`. Ensure `CommandParameters` actually accepts these keys and that the parameter names match the expected constructor/attributes. Mismatched parameters would raise at runtime (though currently caught) and result in missing `form_data_key`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-5f5783c745d6c46e34d8d32dac96ae489c191fae284171fd3c8755a3c212d869R121-R123'><strong>Serialization / Data exposure risk</strong></a><br>Returning the raw `form_data` may include non-JSON-serializable objects (datetimes, objects, numpy types) or sensitive values inside filters/params. Ensure the returned object is safe and JSON-serializable and that no sensitive fields are unintentionally exposed.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-bfa222bca38457f6b2cf8b875aad06d2a05be78afab468c08559543797dc25e3R288-R316'><strong>Blocking sync call in async handler</strong></a><br>The code calls `MCPCreateFormDataCommand(cmd_params).run()` synchronously inside the async `generate_chart` function. If `run()` performs IO (DB/cache/network), this will block the event loop. Consider running this in a thread or using an async API.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-c53108521a676f135ad7846c809dddc3eb33240abb7d123d0948cd5f7b2e9765R1120-R1166'><strong>Consistency with ChartInfo</strong></a><br>There is already a `form_data` field on `ChartInfo`. Make sure the semantics and serialization of `ChartInfo.form_data` and `GenerateChartResponse.form_data` are consistent (types, redaction, and when each is populated) to avoid confusing clients that may read either field.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-5f5783c745d6c46e34d8d32dac96ae489c191fae284171fd3c8755a3c212d869R109-R112'><strong>Fragile query parsing</strong></a><br>The code extracts `form_data_key` by splitting the URL string ("form_data_key=" + split on "&"), which is brittle: - fails with URL-encoding, anchors, or unusual param ordering, - can break if the param appears multiple times, - doesn't use standard URL parsing utilities. Prefer using urllib.parse.urlparse + parse_qs for robust parsing.<br> - [ ] <a href='https://github.com/apache/superset/pull/36539/files#diff-5f5783c745d6c46e34d8d32dac96ae489c191fae284171fd3c8755a3c212d869R116-R118'><strong>Unprotected length call</strong></a><br>The logging uses len(explore_url) without guaranteeing `explore_url` is a string. If `generate_url` returns None (or a non-string), this will raise a TypeError. Use a safe fallback (e.g. len(explore_url or "")) or validate the return type first.<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]
