bito-code-review[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4114162106
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+def _require_mysql_verified_tls(
+ driver: str, query: dict[str, Any], args: dict[str, Any]
+) -> None:
+ """Use required verification on drivers without an encryption-only mode."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ pymysql = import_module("pymysql")
+
+ # Older releases silently fall back even with explicit SSL options.
+ if pymysql.VERSION[:2] < (1, 2):
+ raise ValueError("The MySQL SSL toggle requires PyMySQL >= 1.2")
+ # SQLAlchemy folds URL ssl_ca/cert/key into an ssl dictionary,
+ # but PyMySQL ignores that dictionary when ssl_verify_cert is set.
+ # Keep these as native connect_args so the CA is not discarded.
+ for key in ("ssl_ca", "ssl_cert", "ssl_key"):
+ if key in query:
+ args.setdefault(key, query.pop(key))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: asbool Crashes On Bare Param</b></div>
<div id="fix">
`asbool` raises `ValueError` on empty strings (verified in SQLAlchemy 2.0.52
`sqlalchemy/util/langhelpers.py`), and SQLAlchemy 2.0 URL parsing yields `""`
for a valueless query param like `mysql://host/db?ssl_check_hostname`. The
guard on line 106 passes, then `asbool(query.pop("ssl_check_hostname"))` on
line 108 raises, so `update_params`/`get_sqla_engine` fails for a valid RFC
3986 URI. Handle the empty form explicitly before calling `asbool`.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Silent TLS Override Conflict</b></div>
<div id="fix">
`args.setdefault("ssl_verify_identity", ...)` silently keeps a pre-existing
`ssl_verify_identity=False` connect_arg, so the toggle reports enforced TLS
while certificate/identity verification is actually disabled. Sibling guards in
`_require_mysql_verified_tls` fail closed on such conflicts (`ssl_disabled`
line 90, `ssl_verify_cert` line 92, `ssl` line 112). Detect and reject the
conflicting key instead of silently ignoring it.
</div>
</div>
<small><i>Code Review Run #6870e8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
