bito-code-review[bot] commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3383824762
##########
superset/databases/api.py:
##########
@@ -568,7 +573,11 @@ def put(self, pk: int) -> Response:
exc_info=True,
)
return self.response_422(message=str(ex))
- except (SSHTunnelingNotEnabledError, SSHTunnelDatabasePortError) as ex:
+ except (
+ SSHTunnelingNotEnabledError,
+ SSHTunnelDatabasePortError,
+ SSHTunnelHostKeyVerificationError,
+ ) as ex:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code in exception handler</b></div>
<div id="fix">
The `SSHTunnelHostKeyVerificationError` in the `put()` handler is
unreachable dead code. `UpdateDatabaseCommand` (line 553) does not invoke
`TestConnectionDatabaseCommand` and performs no SSH host key verification, so
this exception cannot propagate from it. The import on line 59 is still needed
for the other three handlers.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/databases/api.py
+++ b/superset/databases/api.py
@@ -573,10 +573,8 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
)
return self.response_422(message=str(ex))
except (
SSHTunnelingNotEnabledError,
SSHTunnelDatabasePortError,
- SSHTunnelHostKeyVerificationError,
) as ex:
return self.response_400(message=str(ex))
```
</div>
</details>
</div>
<small><i>Code Review Run #5ed75e</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
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,131 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
+ # The server presents the same key we expect: verification passes.
+ server_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(_authorized_key(server_key))
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = server_key
+
+ manager._verify_host_key(tunnel) # should not raise
+
+ # The TCP connect is bounded by an explicit timeout, and the resulting
+ # socket is handed to Transport.
+ mock_create_connection.assert_called_once_with(
+ ("ssh.example.com", 22), timeout=321.0
+ )
+
mock_transport_cls.assert_called_once_with(mock_create_connection.return_value)
+ transport.start_client.assert_called_once()
+ transport.close.assert_called_once()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_mismatch_raises(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
+ # The server presents a different key than expected: verification fails.
+ expected_key = paramiko.RSAKey.generate(2048)
+ presented_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(_authorized_key(expected_key))
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = presented_key
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+ mock_create_connection.assert_called_once()
+ transport.close.assert_called_once()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+def test_verify_host_key_connect_failure_raises(
+ mock_create_connection: Mock,
+) -> None:
+ # A bounded TCP connect failure surfaces as a host-key verification error.
+ manager = _make_manager(strict=False)
+ server_key = paramiko.RSAKey.generate(2048)
+ tunnel = _ssh_tunnel(_authorized_key(server_key))
+
+ mock_create_connection.side_effect = OSError("connection refused")
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_unset_non_strict_skips(mock_transport_cls: Mock) ->
None:
+ # Back-compat: no expected key + strict checking off => no verification at
all.
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(None)
+
+ manager._verify_host_key(tunnel) # should not raise
+
+ mock_transport_cls.assert_not_called()
+
+
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_unset_strict_raises(mock_transport_cls: Mock) -> None:
+ # Fail-closed: no expected key + strict checking on => reject.
+ manager = _make_manager(strict=True)
+ tunnel = _ssh_tunnel(None)
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+ mock_transport_cls.assert_not_called()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match_ignores_comment_and_whitespace(
+ mock_transport_cls: Mock,
+ mock_create_connection: Mock,
+) -> None:
+ # The stored key may carry a trailing comment and extra whitespace.
+ server_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ stored = f" {_authorized_key(server_key)} user@host "
+ tunnel = _ssh_tunnel(stored)
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = server_key
+
+ manager._verify_host_key(tunnel) # should not raise
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertions for whitespace
parsing</b></div>
<div id="fix">
Test `test_verify_host_key_match_ignores_comment_and_whitespace` only
verifies no exception is raised but lacks assertions confirming the TCP socket
and Transport are invoked with correct timeout. Compare with
`test_verify_host_key_match` at line 86-91 which verifies all interaction
steps. Without these assertions, a regression that silently skips verification
(e.g., broken whitespace stripping) would produce a false pass.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- tests/unit_tests/extensions/ssh_test.py
+++ tests/unit_tests/extensions/ssh_test.py
@@ -166,4 +166,11 @@
transport = mock_transport_cls.return_value
transport.get_remote_server_key.return_value = server_key
manager._verify_host_key(tunnel) # should not raise
+
+ mock_create_connection.assert_called_once_with(
+ ("ssh.example.com", 22), timeout=321.0
+ )
+
mock_transport_cls.assert_called_once_with(mock_create_connection.return_value)
+ transport.start_client.assert_called_once()
+ transport.close.assert_called_once()
```
</div>
</details>
</div>
<small><i>Code Review Run #5ed75e</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]
