bito-code-review[bot] commented on code in PR #43393:
URL: https://github.com/apache/superset/pull/43393#discussion_r3831886780
##########
superset/commands/database/importers/v1/utils.py:
##########
@@ -37,6 +38,63 @@
logger = logging.getLogger(__name__)
+def _connection_identity_changed(existing: Database, config: dict[str, Any])
-> bool:
+ """Whether the import points the database at a different endpoint."""
+ try:
+ stored = make_url_safe(existing.sqlalchemy_uri)._replace(password=None)
+ incoming =
make_url_safe(config["sqlalchemy_uri"])._replace(password=None)
+ except Exception: # pylint: disable=broad-except
+ # An unparseable URI cannot be compared: treat it as a change so
+ # stored secrets never survive onto it.
+ return True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Replace bare Exception with specific exception</b></div>
<div id="fix">
Catching generic `Exception` is too broad. Replace with specific exceptions
from `urllib.parse` that can occur when parsing URLs (e.g., `ValueError`).
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
def _connection_identity_changed(existing: Database, config: dict[str, Any])
-> bool:
"""Whether the import points the database at a different endpoint."""
try:
stored =
make_url_safe(existing.sqlalchemy_uri)._replace(password=None)
incoming =
make_url_safe(config["sqlalchemy_uri"])._replace(password=None)
except ValueError:
# An unparseable URI cannot be compared: treat it as a change so
# stored secrets never survive onto it.
return True
````
</div>
</details>
</div>
<small><i>Code Review Run #5c1645</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
##########
superset/commands/database/importers/v1/utils.py:
##########
@@ -81,7 +144,13 @@ def import_database( # noqa: C901
# For existing DBs, reveal masked sensitive values from current
encrypted_extra.
# For new DBs, schema validation already ensured no fields are still
masked.
if masked_encrypted_extra := config.pop("masked_encrypted_extra", None):
- if existing and existing.encrypted_extra:
+ # Never reveal stored encrypted_extra secrets into a config that
+ # repoints the connection at a different endpoint.
+ if (
+ existing
+ and existing.encrypted_extra
+ and not _connection_identity_changed(existing, config)
+ ):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new security guards</b></div>
<div id="fix">
The encrypted_extra endpoint guard (lines 147–153) and the
`_refuse_stored_secret_reuse` call (line 116) have no dedicated test coverage.
Existing test `test_import_database_with_masked_encrypted_extra_existing_db`
only validates the reveal path when the endpoint is unchanged — it does not
cover the repoint-blocking path. Per BITO.md adaptive rule 11730, comprehensive
unit tests are required for all new security logic covering both success paths
and error/validation scenarios.
</div>
</div>
<small><i>Code Review Run #5c1645</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
##########
superset/commands/database/importers/v1/utils.py:
##########
@@ -37,6 +38,63 @@
logger = logging.getLogger(__name__)
+def _connection_identity_changed(existing: Database, config: dict[str, Any])
-> bool:
+ """Whether the import points the database at a different endpoint."""
+ try:
+ stored = make_url_safe(existing.sqlalchemy_uri)._replace(password=None)
+ incoming =
make_url_safe(config["sqlalchemy_uri"])._replace(password=None)
+ except Exception: # pylint: disable=broad-except
+ # An unparseable URI cannot be compared: treat it as a change so
+ # stored secrets never survive onto it.
+ return True
+ return stored != incoming
+
+
+def _refuse_stored_secret_reuse(existing: Database, config: dict[str, Any]) ->
None:
+ """
+ Refuse an overwrite that changes the connection endpoint without fresh
+ credentials.
+
+ Database UUIDs are not secrets -- they appear in every exported bundle --
+ so an import must not be able to repoint an existing connection at a new
+ host while the stored password (or SSH tunnel key) is silently kept: the
+ next connection would hand the real credential to the new endpoint.
+ """
+ if _connection_identity_changed(existing, config):
+ try:
+ uri_password = make_url_safe(config["sqlalchemy_uri"]).password
+ except Exception: # pylint: disable=broad-except
+ uri_password = None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Replace bare Exception with specific exception</b></div>
<div id="fix">
Catching generic `Exception` is too broad. Consider catching `Exception`
from the `urllib.parse` module specifically when parsing the URI password.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if _connection_identity_changed(existing, config):
try:
uri_password = make_url_safe(config["sqlalchemy_uri"]).password
except ValueError:
uri_password = None
````
</div>
</details>
</div>
<small><i>Code Review Run #5c1645</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]
