codeant-ai-for-open-source[bot] commented on code in PR #44910:
URL: https://github.com/apache/superset/pull/44910#discussion_r4170130467
##########
tests/testcontainers/db_engine_specs/test_mysql.py:
##########
@@ -115,3 +147,94 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
assert spec is not None
assert spec.generic_type == GenericDataType.NUMERIC
assert isinstance(spec.sqla_type, Integer)
+
+
[email protected](scope="module")
+def no_tls_container() -> Iterator[MySqlContainer]:
+ """A server with TLS fully disabled, to test the fail-closed guarantee.
+
+ `--ssl=0` is MySQL's deprecated-but-still-supported spelling for turning
+ off TLS support entirely (`have_ssl` reports `DISABLED`), verified
+ directly against this image before writing this fixture.
+ """
+ with MySqlContainer("mysql:8.0", command="--ssl=0") as container:
+ yield container
+
+
+def _require_tls_connect_args(uri_query: str = "ssl=1") -> tuple[URL,
dict[str, Any]]:
+ """Run a `ssl=1` request through the real `adjust_engine_params` path.
+
+ Goes through `MySQLEngineSpec.adjust_engine_params` rather than calling
+ `require_mysql_tls` directly, so the test exercises the exact call path
+ production code takes (including driver-name resolution from a bare
+ `mysql://` URL), not a shortcut around it.
+ """
+ uri = make_url(f"mysql://root:test@placeholder:3306/test?{uri_query}")
+ return MySQLEngineSpec.adjust_engine_params(uri, {})
+
+
+def test_require_mysql_tls_fails_closed_without_server_tls(
+ no_tls_container: MySqlContainer,
+) -> None:
+ """
+ The core guarantee apache/superset#44723 exists to provide: a server
+ offering no TLS at all must make the connection fail, never succeed
+ silently in cleartext. mysqlclient linked against MariaDB Connector/C
+ maps `ssl_mode=REQUIRED` to *opportunistic* TLS -- it degrades to
+ cleartext instead of refusing when the server can't negotiate TLS --
+ which is exactly the silent-downgrade this fix closes by requiring
+ `VERIFY_CA` instead for that client library. Only a real client library
+ actually attempting real TLS negotiation against a real non-TLS server
+ can show this; a mocked cursor never negotiates anything.
+ """
+ host = _tcp_host(no_tls_container)
+ port = no_tls_container.get_exposed_port(no_tls_container.port)
+ uri, connect_args = _require_tls_connect_args()
+ uri = uri.set(host=host, port=int(port), database=no_tls_container.dbname)
+ engine = create_engine(uri, connect_args=connect_args)
+ with pytest.raises(OperationalError):
+ with engine.connect():
+ pass
+
+
[email protected](scope="module")
+def tls_ca_path(mysql_container: MySqlContainer) -> Iterator[str]:
+ """Extract the shared `engine` container's auto-generated CA to a file.
+
+ `ssl_ca` resolves (per `SHOW VARIABLES LIKE 'ssl_ca'`, verified directly
+ against this image) to `ca.pem` under the data directory. There's no
+ testcontainers API for pulling a single file back out of a running
+ container, so this shells out via the same `exec()` the rest of this
+ suite already depends on. Depending on `mysql_container` (not spinning
+ up a second one) reuses the exact server `engine`'s tests already talk
+ to, so the extracted CA is guaranteed to match.
+ """
+ result = mysql_container.exec(["cat", "/var/lib/mysql/ca.pem"])
+ assert result.exit_code == 0, "could not read the auto-generated CA cert"
+ with tempfile.NamedTemporaryFile(mode="wb", suffix=".pem", delete=False)
as ca_file:
+ ca_file.write(result.output)
+ yield ca_file.name
+
+
+def test_require_mysql_tls_connects_with_verified_tls(
+ engine: Engine, tls_ca_path: str
+) -> None:
+ """
+ The other side of the same guarantee as the fail-closed test above: a
+ fix that merely refused every connection would also pass that test
+ while breaking every legitimate encrypted connection. The default
+ `mysql:8.0` image auto-generates a CA/server cert pair with TLS
+ available out of the box (verified directly: `have_ssl` reports `YES`),
+ so a request that actually trusts that CA must still succeed, and must
+ be genuinely encrypted rather than merely accepted -- asserted here via
+ `Ssl_cipher`, which MySQL reports empty for a plaintext session.
+ """
+ uri, connect_args =
_require_tls_connect_args(f"ssl=1&ssl_ca={tls_ca_path}")
+ uri = uri.set(
+ host=engine.url.host, port=engine.url.port,
database=engine.url.database
+ )
+ tls_engine = create_engine(uri, connect_args=connect_args)
+ with tls_engine.connect() as conn:
+ cipher = conn.execute(text("SHOW STATUS LIKE 'Ssl_cipher'")).fetchone()
+ assert cipher is not None
+ assert cipher[1], "connection succeeded but is not actually using TLS"
Review Comment:
**Suggestion:** The assertion checks encryption only; a connection using
unverified REQUIRED TLS would pass, so the test can miss loss of certificate
verification.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Possible bug`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a3d910f20384d23befdb9edadfe65a3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7a3d910f20384d23befdb9edadfe65a3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/testcontainers/db_engine_specs/test_mysql.py
**Line:** 240:240
**Comment:**
*Possible Bug: The assertion checks encryption only; a connection using
unverified REQUIRED TLS would pass, so the test can miss loss of certificate
verification.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44910&comment_hash=5fc01f59c94ad1f5cd10548fd2380fff273322a489b345b833dbf47f546fcfbc&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44910&comment_hash=5fc01f59c94ad1f5cd10548fd2380fff273322a489b345b833dbf47f546fcfbc&reaction=dislike'>๐</a>
--
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]
