codeant-ai-for-open-source[bot] commented on PR #36783: URL: https://github.com/apache/superset/pull/36783#issuecomment-3679452517
## 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/36783/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1179-R1181'><strong>Exposure surface</strong></a><br>The property returns the full connection string via `conn.render_as_string(hide_password=False)`. Ensure callers of this property don't log or leak the returned URL; changes here preserve encoded password and may make accidental leaks harder to notice in logs.<br> - [ ] <a href='https://github.com/apache/superset/pull/36783/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1110-R1138'><strong>Possible double-encoding</strong></a><br>The logic in `sqlalchemy_uri_decrypted` percent-encodes `raw_password` with `quote(raw_password, safe="")`. If the raw password is already percent-encoded (e.g. coming from a custom password store or external input), this may produce double-encoded values and result in incorrect credentials when SQLAlchemy parses the URL. Consider normalizing by unquoting before quoting or otherwise ensuring idempotent encoding.<br> - [ ] <a href='https://github.com/apache/superset/pull/36783/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1169-R1176'><strong>Double-encoding risk</strong></a><br>The code always calls urllib.parse.quote() on `raw_password` before setting it on the URL. If `raw_password` is already percent-encoded (for example stored that way by some custom stores or during import), quoting again will double-encode the value and break authentication.<br> - [ ] <a href='https://github.com/apache/superset/pull/36783/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1161-R1167'><strong>Non-str password types</strong></a><br>`raw_password` may be bytes or another non-str type (coming from custom password stores or encrypted fields). Passing a bytes value to `quote()` will raise or behave unexpectedly. Coerce/validate the type before quoting.<br> - [ ] <a href='https://github.com/apache/superset/pull/36783/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR1010-R1050'><strong>Empty vs None handling</strong></a><br>The implementation treats empty string and None differently: empty string is quoted into "" and set as password, while None clears the password. Ensure this maps to expected behavior when constructing and parsing URLs (some URL parsers may interpret empty password differently). Confirm the tests cover both cases and that downstream consumers accept both.<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]
