bito-code-review[bot] commented on code in PR #43393:
URL: https://github.com/apache/superset/pull/43393#discussion_r3832250353
##########
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:
<!-- Bito Reply -->
The suggestion to catch `ValueError` specifically instead of a bare
`Exception` is appropriate, as `make_url_safe()` is documented to raise
`ValueError` when the URI is invalid. This change improves code robustness by
avoiding the suppression of unrelated exceptions.
**superset/commands/database/importers/v1/utils.py**
```
if _connection_identity_changed(existing, config):
try:
uri_password = make_url_safe(config["sqlalchemy_uri"]).password
except ValueError:
uri_password = None
```
##########
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:
<!-- Bito Reply -->
The suggestion to replace the broad `Exception` catch with a more specific
one is appropriate for improving code robustness. Since `make_url_safe()` is
documented to raise `DatabaseInvalidError` (as noted in your reply), catching
that specific exception—or the relevant `urllib.parse` exceptions—is a better
practice than catching the generic `Exception` class.
**superset/commands/database/importers/v1/utils.py**
```
try:
stored =
make_url_safe(existing.sqlalchemy_uri)._replace(password=None)
incoming =
make_url_safe(config["sqlalchemy_uri"])._replace(password=None)
except DatabaseInvalidError:
# An unparseable URI cannot be compared: treat it as a change so
# stored secrets never survive onto it.
return True
```
##########
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:
<!-- Bito Reply -->
The changes address the security concern by adding a guard that prevents
revealing stored secrets when the connection endpoint is changed. This
implementation correctly handles the repoint-blocking path for both password
and private key authentication, as well as the encrypted_extra guard.
**superset/commands/database/importers/v1/utils.py**
```
# 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)
):
```
--
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]
