This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 33c6df77c99 fix(database): don't reattach stored connection details
when the effective destination changes (#44003)
33c6df77c99 is described below
commit 33c6df77c99fa83991271b9fe1f95d691ba3fa9f
Author: Evan Rusackas <[email protected]>
AuthorDate: Tue Sep 8 11:02:42 2026 -0700
fix(database): don't reattach stored connection details when the effective
destination changes (#44003)
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Shaitan <[email protected]>
---
superset/commands/database/exceptions.py | 29 ++
superset/commands/database/importers/v1/utils.py | 60 ++---
superset/commands/database/test_connection.py | 41 ++-
superset/commands/database/update.py | 87 ++++++
superset/commands/database/utils.py | 96 +++++++
superset/commands/database/validate.py | 47 +++-
superset/daos/database.py | 17 +-
.../commands/databases/test_connection_test.py | 60 +++++
tests/unit_tests/commands/databases/update_test.py | 297 +++++++++++++++++++++
.../unit_tests/commands/databases/validate_test.py | 205 ++++++++++++++
.../databases/commands/importers/v1/import_test.py | 43 +++
11 files changed, 936 insertions(+), 46 deletions(-)
diff --git a/superset/commands/database/exceptions.py
b/superset/commands/database/exceptions.py
index b8877270fd5..3002696a42e 100644
--- a/superset/commands/database/exceptions.py
+++ b/superset/commands/database/exceptions.py
@@ -44,6 +44,26 @@ class DatabaseExistsValidationError(ValidationError):
)
+class DatabaseUpdateUnsafeRebindError(ValidationError):
+ """
+ Marshmallow validation error for an update that would change a
+ database's effective connection destination while leaving the stored
+ password/encrypted_extra/SSH tunnel credential masked.
+ """
+
+ def __init__(self, field_name: str = "sqlalchemy_uri") -> None:
+ super().__init__(
+ _(
+ "This update would change the connection's effective "
+ "destination (host/port, engine parameters, or SSH tunnel "
+ "endpoint) while reusing the stored credential. Provide "
+ "the real password (or SSH tunnel credential) to confirm "
+ "a connection move."
+ ),
+ field_name=field_name,
+ )
+
+
class DatabaseRequiredFieldValidationError(ValidationError):
def __init__(self, field_name: str) -> None:
super().__init__(
@@ -174,6 +194,15 @@ class DatabaseSecurityUnsafeError(CommandInvalidError):
message = _("Stopped an unsafe database connection")
+class DatabaseTestConnectionUnsafeRebindError(CommandInvalidError):
+ message = _(
+ "Testing this connection would change its effective destination "
+ "(engine parameters or SSH tunnel endpoint) while reusing the stored "
+ "password. Provide the real password to test a connection whose "
+ "destination has changed."
+ )
+
+
class DatabaseTestConnectionDriverError(CommandInvalidError):
message = _("Could not load database driver")
diff --git a/superset/commands/database/importers/v1/utils.py
b/superset/commands/database/importers/v1/utils.py
index 973aaad8b80..5357500c197 100644
--- a/superset/commands/database/importers/v1/utils.py
+++ b/superset/commands/database/importers/v1/utils.py
@@ -22,7 +22,12 @@ from flask import current_app as app
from superset import db, security_manager
from superset.commands.database.exceptions import DatabaseInvalidError
-from superset.commands.database.utils import add_permissions
+from superset.commands.database.utils import (
+ add_permissions,
+ engine_params_changed,
+ ssh_tunnel_rebind_unsafe,
+ uri_identity_changed,
+)
from superset.commands.exceptions import ImportFailedError
from superset.constants import PASSWORD_MASK
from superset.databases.ssh_tunnel.models import SSHTunnel
@@ -41,14 +46,19 @@ 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 DatabaseInvalidError:
- # An unparseable URI cannot be compared: treat it as a change so
- # stored secrets never survive onto it.
+ if uri_identity_changed(existing.sqlalchemy_uri,
config.get("sqlalchemy_uri")):
return True
- return stored != incoming
+
+ # The URI's host/port aren't the whole story: `extra.engine_params`
+ # (e.g. `connect_args.host`/`port`) is merged into the actual DBAPI
+ # connect kwargs and can override them. An import that opens a live
+ # connection (`add_permissions` -> `get_all_catalog_names`) with a
+ # rehydrated stored password must not do so against a destination this
+ # field silently redirected.
+ submitted_extra = config.get("extra")
+ if isinstance(submitted_extra, dict):
+ submitted_extra = json.dumps(submitted_extra)
+ return engine_params_changed(existing.extra, submitted_extra)
def _refuse_stored_secret_reuse(existing: Database, config: dict[str, Any]) ->
None:
@@ -77,33 +87,13 @@ def _refuse_stored_secret_reuse(existing: Database, config:
dict[str, Any]) -> N
"connection to confirm the change."
)
- if ssh_tunnel := config.get("ssh_tunnel"):
- existing_tunnel = existing.ssh_tunnel
- if existing_tunnel and (
- ssh_tunnel.get("server_address") != existing_tunnel.server_address
- or ssh_tunnel.get("server_port") != existing_tunnel.server_port
- ):
- has_fresh_credential = any(
- ssh_tunnel.get(field) not in (None, PASSWORD_MASK)
- for field in ("password", "private_key")
- )
- # A passphrase-protected private key's stored passphrase is a
- # secret in its own right: if the existing tunnel had one, a
- # repoint that supplies a fresh private_key but leaves
- # private_key_password masked/absent would keep the old
- # passphrase attached to the new key rather than requiring the
- # importer to confirm it too.
- stale_private_key_password = (
- existing_tunnel.private_key_password is not None
- and ssh_tunnel.get("private_key_password") in (None,
PASSWORD_MASK)
- )
- if not has_fresh_credential or stale_private_key_password:
- raise ImportFailedError(
- f"Import would change the SSH tunnel endpoint of database "
- f"'{existing.database_name}' without providing new tunnel "
- "credentials. Re-enter the SSH tunnel credentials to "
- "confirm the change."
- )
+ if ssh_tunnel_rebind_unsafe(existing.ssh_tunnel, config.get("ssh_tunnel")):
+ raise ImportFailedError(
+ f"Import would change the SSH tunnel endpoint of database "
+ f"'{existing.database_name}' without providing new tunnel "
+ "credentials. Re-enter the SSH tunnel credentials to "
+ "confirm the change."
+ )
def import_database( # noqa: C901
diff --git a/superset/commands/database/test_connection.py
b/superset/commands/database/test_connection.py
index 1247a2dee5a..a128afc272d 100644
--- a/superset/commands/database/test_connection.py
+++ b/superset/commands/database/test_connection.py
@@ -26,13 +26,18 @@ from superset.commands.database.exceptions import (
DatabaseSecurityUnsafeError,
DatabaseTestConnectionDriverError,
DatabaseTestConnectionUnexpectedError,
+ DatabaseTestConnectionUnsafeRebindError,
)
from superset.commands.database.ssh_tunnel.exceptions import (
SSHTunnelDatabasePortError,
SSHTunnelHostKeyVerificationError,
SSHTunnelingNotEnabledError,
)
-from superset.commands.database.utils import ping
+from superset.commands.database.utils import (
+ engine_params_changed,
+ ping,
+ ssh_tunnel_endpoint_changed,
+)
from superset.daos.database import DatabaseDAO
from superset.databases.utils import make_url_safe
from superset.errors import ErrorLevel, SupersetErrorType
@@ -65,6 +70,8 @@ class TestConnectionDatabaseCommand(BaseCommand):
_model: Optional[Database] = None
_context: dict[str, Any]
_uri: str
+ _identity_changed: bool
+ _ssh_tunnel_endpoint_changed: bool
def __init__(self, data: dict[str, Any]):
self._properties = data.copy()
@@ -73,8 +80,27 @@ class TestConnectionDatabaseCommand(BaseCommand):
self._model = DatabaseDAO.get_database_by_name(database_name)
uri = self._properties.get("sqlalchemy_uri", "")
- if self._model and uri == self._model.safe_sqlalchemy_uri():
- uri = self._model.sqlalchemy_uri_decrypted
+ self._identity_changed = False
+ self._ssh_tunnel_endpoint_changed = False
+ if (model := self._model) is not None:
+ # A stored password (and, below, encrypted_extra / SSH tunnel
+ # credentials) must never be rehydrated onto a connection whose
+ # final effective destination the requester can change. The
+ # visible `sqlalchemy_uri` is only one part of that destination:
+ # `extra.engine_params` (merged into the DBAPI connect kwargs,
+ # e.g. `connect_args.host`/`port`) and the SSH tunnel endpoint
+ # can both override it after this decision is made.
+ self._ssh_tunnel_endpoint_changed = ssh_tunnel_endpoint_changed(
+ model.ssh_tunnel, self._properties.get("ssh_tunnel")
+ )
+ self._identity_changed = (
+ engine_params_changed(model.extra,
self._properties.get("extra", "{}"))
+ or self._ssh_tunnel_endpoint_changed
+ )
+ if uri == model.safe_sqlalchemy_uri():
+ if self._identity_changed:
+ raise DatabaseTestConnectionUnsafeRebindError()
+ uri = model.sqlalchemy_uri_decrypted
url = make_url_safe(uri)
@@ -102,7 +128,7 @@ class TestConnectionDatabaseCommand(BaseCommand):
"masked_encrypted_extra",
"{}",
)
- if self._model:
+ if self._model and not self._identity_changed:
serialized_encrypted_extra = (
self._model.db_engine_spec.unmask_encrypted_extra(
self._model.encrypted_extra,
@@ -112,7 +138,12 @@ class TestConnectionDatabaseCommand(BaseCommand):
# collect SSH tunnel info
ssh_tunnel_properties = self._properties.get("ssh_tunnel")
- if ssh_tunnel_properties and self._model and self._model.ssh_tunnel:
+ if (
+ ssh_tunnel_properties
+ and self._model
+ and self._model.ssh_tunnel
+ and not self._ssh_tunnel_endpoint_changed
+ ):
# unmask password while allowing for updated values
ssh_tunnel_properties = unmask_password_info(
ssh_tunnel_properties,
diff --git a/superset/commands/database/update.py
b/superset/commands/database/update.py
index 124bf11c87b..f0d079b9121 100644
--- a/superset/commands/database/update.py
+++ b/superset/commands/database/update.py
@@ -30,10 +30,18 @@ from superset.commands.database.exceptions import (
DatabaseInvalidError,
DatabaseNotFoundError,
DatabaseUpdateFailedError,
+ DatabaseUpdateUnsafeRebindError,
MissingOAuth2TokenError,
)
from superset.commands.database.sync_permissions import SyncPermissionsCommand
+from superset.commands.database.utils import (
+ engine_params_changed,
+ ssh_tunnel_rebind_unsafe,
+ uri_identity_changed,
+)
+from superset.constants import PASSWORD_MASK
from superset.daos.database import DatabaseDAO
+from superset.databases.utils import make_url_safe
from superset.exceptions import OAuth2RedirectError
from superset.models.core import Database
from superset.utils import json
@@ -180,3 +188,82 @@ class UpdateDatabaseCommand(BaseCommand):
database_name,
):
raise
DatabaseInvalidError(exceptions=[DatabaseExistsValidationError()])
+
+ if self._model:
+ self._check_no_unsafe_secret_rebind()
+
+ def _check_no_unsafe_secret_rebind(self) -> None:
+ """
+ Refuse an update that changes the connection's effective destination
+ (URI host/port, `extra.engine_params`, or the SSH tunnel endpoint)
+ while leaving the corresponding stored secret masked.
+
+ Without this, an editor could silently redirect the real stored
+ password/encrypted_extra/SSH tunnel credential to a different
+ destination -- and since an update persists, every subsequent use of
+ the database (by any user) would send the real secret there, not
+ just the editor's own request.
+ """
+ model = self._model
+ assert model is not None
+
+ connection_identity_changed = False
+ submitted_password: str | None = None
+
+ if "sqlalchemy_uri" in self._properties:
+ submitted_uri = self._properties["sqlalchemy_uri"] or ""
+ connection_identity_changed = uri_identity_changed(
+ model.sqlalchemy_uri, submitted_uri
+ )
+ try:
+ submitted_password = make_url_safe(submitted_uri).password
+ except DatabaseInvalidError:
+ submitted_password = None
+
+ if "extra" in self._properties and engine_params_changed(
+ model.extra, self._properties["extra"]
+ ):
+ connection_identity_changed = True
+
+ if connection_identity_changed:
+ # The URI password is only one of the secrets that can silently
+ # carry over onto a changed destination. `encrypted_extra` (e.g.
+ # a service-account key or OAuth2 client secret) is reattached
+ # unconditionally in `run()` via `unmask_encrypted_extra` unless
+ # we catch it here -- gating on the URI password alone would
+ # both miss that reuse when a fresh URI password is supplied,
+ # and wrongly block engines that keep credentials entirely in
+ # `encrypted_extra` and carry no URI password at all (BigQuery,
+ # GSheets), since those never have a "fresh" URI password to
+ # give.
+ uri_password_reused = model.password is not None and
submitted_password in (
+ None,
+ PASSWORD_MASK,
+ )
+ # encrypted_extra is a blob with per-field masks, so "reused"
+ # means unmasking the submission against the stored value
+ # changes nothing -- including not submitting it at all, which
+ # leaves the old (real) value attached unchanged.
+ encrypted_extra_reused = model.encrypted_extra not in (
+ None,
+ "",
+ "{}",
+ ) and (
+ "masked_encrypted_extra" not in self._properties
+ or model.db_engine_spec.unmask_encrypted_extra(
+ model.encrypted_extra,
+ self._properties["masked_encrypted_extra"],
+ )
+ == model.encrypted_extra
+ )
+ if uri_password_reused or encrypted_extra_reused:
+ raise DatabaseInvalidError(
+ exceptions=[DatabaseUpdateUnsafeRebindError()]
+ )
+
+ if "ssh_tunnel" in self._properties and ssh_tunnel_rebind_unsafe(
+ model.ssh_tunnel, self._properties["ssh_tunnel"]
+ ):
+ raise DatabaseInvalidError(
+
exceptions=[DatabaseUpdateUnsafeRebindError(field_name="ssh_tunnel")]
+ )
diff --git a/superset/commands/database/utils.py
b/superset/commands/database/utils.py
index 0c25f8173c2..0d40516419c 100644
--- a/superset/commands/database/utils.py
+++ b/superset/commands/database/utils.py
@@ -19,6 +19,7 @@ from __future__ import annotations
import logging
import sqlite3
from contextlib import closing
+from typing import Any
from flask import current_app as app
from flask_appbuilder.security.sqla.models import (
@@ -30,14 +31,109 @@ from sqlalchemy.engine import Engine
from sqlalchemy.orm import Session
from superset import security_manager
+from superset.commands.database.exceptions import DatabaseInvalidError
+from superset.constants import PASSWORD_MASK
+from superset.databases.ssh_tunnel.models import SSHTunnel
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.models.core import Database
from superset.security.manager import SupersetSecurityManager
+from superset.utils import json
from superset.utils.core import timeout
logger = logging.getLogger(__name__)
+def uri_identity_changed(existing_uri: str | None, submitted_uri: str | None)
-> bool:
+ """
+ Whether two SQLAlchemy URIs differ once their password is stripped --
+ i.e. whether the effective connection destination (driver, host, port,
+ database, username, query params) changed.
+ """
+ try:
+ stored = make_url_safe(existing_uri or "")._replace(password=None)
+ incoming = make_url_safe(submitted_uri or "")._replace(password=None)
+ except DatabaseInvalidError:
+ # An unparseable URI cannot be compared: treat it as a change so a
+ # stored secret never survives onto it.
+ return True
+ return stored != incoming
+
+
+def engine_params_changed(
+ existing_extra: str | None, submitted_extra: str | None
+) -> bool:
+ """
+ Whether ``submitted_extra`` carries different ``engine_params`` than
+ ``existing_extra``.
+
+ ``engine_params`` (in particular ``engine_params.connect_args``) is
+ merged into the actual DBAPI connect kwargs, so it can override the
+ host/port/etc. carried in the SQLAlchemy URI itself. Any caller that
+ conditionally reattaches a stored secret (password, encrypted_extra, SSH
+ tunnel credentials) based on the URI being unchanged must also check
+ this, or the destination can be silently redirected while the real
+ secret rides along.
+ """
+
+ def _engine_params(serialized_extra: str | None) -> dict[str, Any]:
+ try:
+ return json.loads(serialized_extra or "{}").get("engine_params",
{})
+ except (json.JSONDecodeError, AttributeError):
+ # Unparseable/non-dict `extra` cannot be compared: treat it as a
+ # change so a stored secret never rides along with input that
+ # can't be verified to leave the connection identity untouched.
+ return {"__unparseable__": True}
+
+ return _engine_params(submitted_extra) != _engine_params(existing_extra)
+
+
+def ssh_tunnel_endpoint_changed(
+ existing_tunnel: SSHTunnel | None, submitted_tunnel: dict[str, Any] | None
+) -> bool:
+ """
+ Whether a submitted SSH tunnel config points at a different endpoint
+ than the stored tunnel it would otherwise inherit credentials from.
+ """
+ if not submitted_tunnel or not existing_tunnel:
+ return False
+ return bool(
+ submitted_tunnel.get("server_address") !=
existing_tunnel.server_address
+ or submitted_tunnel.get("server_port") != existing_tunnel.server_port
+ )
+
+
+def ssh_tunnel_rebind_unsafe(
+ existing_tunnel: SSHTunnel | None, submitted_tunnel: dict[str, Any] | None
+) -> bool:
+ """
+ Whether a submitted SSH tunnel config repoints the tunnel at a
+ different endpoint without supplying credentials fresh enough to
+ justify it -- i.e. whether carrying the stored tunnel secrets over
+ onto this submission would be unsafe.
+ """
+ if not ssh_tunnel_endpoint_changed(existing_tunnel, submitted_tunnel):
+ return False
+
+ assert submitted_tunnel is not None
+ assert existing_tunnel is not None
+
+ has_fresh_credential = any(
+ submitted_tunnel.get(field) not in (None, PASSWORD_MASK)
+ for field in ("password", "private_key")
+ )
+ # A passphrase-protected private key's stored passphrase is a secret in
+ # its own right: if the existing tunnel had one, a repoint that
+ # supplies a fresh private_key but leaves private_key_password
+ # masked/absent would keep the old passphrase attached to the new key
+ # rather than requiring the caller to confirm it too.
+ stale_private_key_password = (
+ existing_tunnel.private_key_password is not None
+ and submitted_tunnel.get("private_key_password") in (None,
PASSWORD_MASK)
+ )
+ return not has_fresh_credential or stale_private_key_password
+
+
def ping(engine: Engine) -> bool:
try:
time_delta = app.config["TEST_DATABASE_CONNECTION_TIMEOUT"]
diff --git a/superset/commands/database/validate.py
b/superset/commands/database/validate.py
index df7b4c0f363..6edf192d842 100644
--- a/superset/commands/database/validate.py
+++ b/superset/commands/database/validate.py
@@ -27,6 +27,10 @@ from superset.commands.database.exceptions import (
InvalidEngineError,
InvalidParametersError,
)
+from superset.commands.database.utils import (
+ engine_params_changed,
+ ssh_tunnel_endpoint_changed,
+)
from superset.daos.database import DatabaseDAO
from superset.databases.utils import make_url_safe
from superset.db_engine_specs import get_engine_spec
@@ -90,11 +94,28 @@ class ValidateDatabaseParametersCommand(BaseCommand):
event_logger.log_with_context(action="validation_error",
engine=engine)
raise InvalidParametersError(errors)
+ # A stored password/encrypted_extra/SSH tunnel credential must never
+ # be rehydrated onto a connection whose final effective destination
+ # the caller can change. `parameters` only covers what feeds into
+ # `sqlalchemy_uri` here -- `extra.engine_params` (merged into the
+ # actual DBAPI connect kwargs, e.g. `connect_args.host`/`port`) and
+ # the SSH tunnel endpoint can both override it independently.
+ identity_changed = False
+ ssh_tunnel_changed = False
+ if (model := self._model) is not None:
+ ssh_tunnel_changed = ssh_tunnel_endpoint_changed(
+ model.ssh_tunnel, self._properties.get("ssh_tunnel")
+ )
+ identity_changed = (
+ engine_params_changed(model.extra,
self._properties.get("extra", "{}"))
+ or ssh_tunnel_changed
+ )
+
serialized_encrypted_extra = self._properties.get(
"masked_encrypted_extra",
"{}",
)
- if self._model:
+ if self._model and not identity_changed:
serialized_encrypted_extra = engine_spec.unmask_encrypted_extra(
self._model.encrypted_extra,
serialized_encrypted_extra,
@@ -110,13 +131,35 @@ class ValidateDatabaseParametersCommand(BaseCommand):
encrypted_extra,
)
if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri():
+ if identity_changed:
+ raise InvalidParametersError(
+ [
+ SupersetError(
+ message=__(
+ "Testing this connection would change its "
+ "effective destination (engine parameters "
+ "or SSH tunnel endpoint) while reusing the "
+ "stored password. Provide the real "
+ "password to test a connection whose "
+ "destination has changed."
+ ),
+
error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ]
+ )
sqlalchemy_uri = self._model.sqlalchemy_uri_decrypted
# Forward the SSH tunnel into the connection test so that
# tunnel-only databases are reached through the tunnel rather
# than directly, mirroring the existing test_connection flow.
ssh_tunnel_properties = self._properties.get("ssh_tunnel")
- if ssh_tunnel_properties and self._model and self._model.ssh_tunnel:
+ if (
+ ssh_tunnel_properties
+ and self._model
+ and self._model.ssh_tunnel
+ and not ssh_tunnel_changed
+ ):
ssh_tunnel_properties = unmask_password_info(
ssh_tunnel_properties,
self._model.ssh_tunnel,
diff --git a/superset/daos/database.py b/superset/daos/database.py
index 6d84a695138..4f0f2bc3cd4 100644
--- a/superset/daos/database.py
+++ b/superset/daos/database.py
@@ -179,11 +179,20 @@ class DatabaseDAO(BaseDAO[Database]):
@staticmethod
def get_database_by_name(database_name: str) -> Database | None:
- return (
- db.session.query(Database)
- .filter(Database.database_name == database_name)
- .one_or_none()
+ """
+ Look up a database by name, scoped to the requesting user's
object-level
+ visibility (the same ``DatabaseFilter`` boundary ``find_by_id``/
+ ``get_connection`` already apply). An unfiltered lookup would let any
+ principal with class-level ``can_write`` reference an arbitrary
+ existing database by name -- including one they have no catalog,
+ schema, datasource, or database access to -- and ride along with
+ whatever secret-rehydration behavior callers apply to the result.
+ """
+ query = db.session.query(Database).filter(
+ Database.database_name == database_name
)
+ query = DatabaseDAO._apply_base_filter(query)
+ return query.one_or_none()
@staticmethod
def build_db_for_connection_test(
diff --git a/tests/unit_tests/commands/databases/test_connection_test.py
b/tests/unit_tests/commands/databases/test_connection_test.py
index 59ca89df276..3f3197d90d8 100644
--- a/tests/unit_tests/commands/databases/test_connection_test.py
+++ b/tests/unit_tests/commands/databases/test_connection_test.py
@@ -19,10 +19,70 @@ import pytest
from pytest_mock import MockerFixture
from superset.commands.database.test_connection import
TestConnectionDatabaseCommand
+from superset.constants import PASSWORD_MASK
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import OAuth2RedirectError
+def test_ssh_tunnel_unmasked_when_only_engine_params_changed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An unrelated `extra.engine_params` change must not block reattaching
+ the stored SSH tunnel password when the tunnel's own endpoint is
+ unchanged -- gating the tunnel unmask on the combined identity flag
+ (rather than the tunnel's own endpoint check) would spuriously break a
+ perfectly legitimate connection test.
+ """
+ mocker.patch(
+ "superset.commands.database.test_connection.is_feature_enabled",
+ return_value=True,
+ )
+
+ tunnel = mocker.MagicMock()
+ tunnel.server_address = "ssh.example.com"
+ tunnel.server_port = 22
+ tunnel.password = "real-tunnel-secret" # noqa: S105
+
+ existing = mocker.MagicMock()
+ existing.safe_sqlalchemy_uri.return_value =
"postgresql://u:XXXXXXXXXX@host1:5432/d"
+ existing.extra = "{}"
+ existing.ssh_tunnel = tunnel
+
+ database = mocker.MagicMock()
+ with database.get_sqla_engine() as engine:
+ engine.dialect.do_ping.return_value = True
+
+ DatabaseDAO = mocker.patch( # noqa: N806
+ "superset.commands.database.test_connection.DatabaseDAO"
+ )
+ DatabaseDAO.get_database_by_name.return_value = existing
+ DatabaseDAO.build_db_for_connection_test.return_value = database
+
+ properties = {
+ "database_name": "victim",
+ # a fresh (non-masked) URI password, as if the user supplied the
+ # real one -- this is the legitimate path that gets past the main
+ # URI-identity check
+ "sqlalchemy_uri": "postgresql://u:realpass@host1:5432/d",
+ # unrelated to the tunnel, but still an identity-affecting change
+ "extra": '{"engine_params": {"connect_args": {"connect_timeout":
30}}}',
+ "ssh_tunnel": {
+ "server_address": "ssh.example.com",
+ "server_port": 22,
+ "username": "tunnel_user",
+ "password": PASSWORD_MASK,
+ },
+ }
+ command = TestConnectionDatabaseCommand(properties)
+ command.run()
+
+ forwarded_tunnel =
DatabaseDAO.build_db_for_connection_test.call_args.kwargs[
+ "ssh_tunnel"
+ ]
+ assert forwarded_tunnel["password"] == "real-tunnel-secret" # noqa: S105
+
+
def test_command(mocker: MockerFixture) -> None:
"""
Test the happy path of the command.
diff --git a/tests/unit_tests/commands/databases/update_test.py
b/tests/unit_tests/commands/databases/update_test.py
index 07663df2450..ced1ab2271c 100644
--- a/tests/unit_tests/commands/databases/update_test.py
+++ b/tests/unit_tests/commands/databases/update_test.py
@@ -17,10 +17,13 @@
from unittest.mock import MagicMock
+import pytest
from pytest_mock import MockerFixture
from superset import db
+from superset.commands.database.exceptions import DatabaseInvalidError
from superset.commands.database.update import UpdateDatabaseCommand
+from superset.constants import PASSWORD_MASK
from superset.extensions import security_manager
from superset.utils import json
from tests.conftest import with_config
@@ -667,3 +670,297 @@ def test_update_broken_connection(mocker: MockerFixture)
-> None:
UpdateDatabaseCommand(1, {}).run()
update_catalog_attribute.assert_called_once_with(1, "main")
+
+
+def test_update_host_change_requires_new_credentials(mocker: MockerFixture) ->
None:
+ """
+ An update that repoints an existing database at a different host while
+ the URI's password stays masked must not silently reuse the stored
+ password: the update persists, so every subsequent use of the database
+ (by any user) would send the real credential to the new host.
+ """
+ existing = mocker.MagicMock()
+ existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ existing.extra = "{}"
+ existing.ssh_tunnel = None
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = existing
+
+ with pytest.raises(DatabaseInvalidError):
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "sqlalchemy_uri": (
+
"postgresql://user:[email protected]:5432/prod"
+ )
+ },
+ ).run()
+
+ database_dao.update.assert_not_called()
+
+
+def test_update_engine_params_change_requires_new_credentials(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An update that changes `extra.engine_params` (e.g.
+ `connect_args.host`/`port`, merged into the actual DBAPI connect kwargs
+ ahead of anything in `sqlalchemy_uri`) while the URI's password stays
+ masked must not silently reuse the stored password.
+ """
+ existing = mocker.MagicMock()
+ existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ existing.extra = "{}"
+ existing.ssh_tunnel = None
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = existing
+
+ with pytest.raises(DatabaseInvalidError):
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "extra": json.dumps(
+ {
+ "engine_params": {
+ "connect_args": {
+ "host": "attacker.example.com",
+ "port": 15432,
+ }
+ }
+ }
+ )
+ },
+ ).run()
+
+ database_dao.update.assert_not_called()
+
+
+def test_update_ssh_tunnel_host_change_requires_new_credentials(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An update that repoints an existing database's SSH tunnel at a
+ different server must not silently reuse the stored tunnel password.
+ """
+ existing = mocker.MagicMock()
+ existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ existing.extra = "{}"
+ tunnel = mocker.MagicMock()
+ tunnel.server_address = "10.0.0.1"
+ tunnel.server_port = 22
+ existing.ssh_tunnel = tunnel
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = existing
+
+ with pytest.raises(DatabaseInvalidError):
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "ssh_tunnel": {
+ "server_address": "attacker.example.com",
+ "server_port": 22,
+ "username": "tunnel_user",
+ "password": PASSWORD_MASK,
+ }
+ },
+ ).run()
+
+ database_dao.update.assert_not_called()
+
+
+def test_update_ssh_tunnel_private_key_password_not_carried_over(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An update that repoints the SSH tunnel and supplies a fresh
+ private_key but omits private_key_password must not silently keep the
+ old, real passphrase attached to the new key.
+ """
+ tunnel = mocker.MagicMock()
+ tunnel.server_address = "10.0.0.1"
+ tunnel.server_port = 22
+ tunnel.private_key_password = "original-passphrase" # noqa: S105
+
+ existing = mocker.MagicMock()
+ existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ existing.extra = "{}"
+ existing.ssh_tunnel = tunnel
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = existing
+
+ with pytest.raises(DatabaseInvalidError):
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "ssh_tunnel": {
+ "server_address": "attacker.example.com",
+ "server_port": 22,
+ "username": "tunnel_user",
+ "private_key": "-----BEGIN PRIVATE
KEY-----\nNew\n-----END-----",
+ # private_key_password omitted entirely
+ }
+ },
+ ).run()
+
+ database_dao.update.assert_not_called()
+
+
+def
test_update_encrypted_extra_reused_when_uri_password_fresh_requires_new_credentials(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A fresh URI password alone isn't enough: if `encrypted_extra` carries a
+ real secret and the submission leaves it masked, the destination change
+ must still be refused, or that secret silently rides along to the new
+ destination too.
+ """
+
+ def _unmask(old: str, new: str) -> str:
+ old_config = json.loads(old)
+ new_config = json.loads(new)
+ for key, value in new_config.items():
+ if value == PASSWORD_MASK and key in old_config:
+ new_config[key] = old_config[key]
+ return json.dumps(new_config)
+
+ old_database = mocker.MagicMock()
+ old_database.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ old_database.password = "oldpass" # noqa: S105
+ old_database.extra = "{}"
+ old_database.encrypted_extra = json.dumps({"client_secret": "real-secret"})
+ old_database.ssh_tunnel = None
+ old_database.db_engine_spec.unmask_encrypted_extra.side_effect = _unmask
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = old_database
+
+ with pytest.raises(DatabaseInvalidError):
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "sqlalchemy_uri": (
+ "postgresql://user:[email protected]:5432/prod"
+ ),
+ "masked_encrypted_extra": json.dumps({"client_secret":
PASSWORD_MASK}),
+ },
+ ).run()
+
+ database_dao.update.assert_not_called()
+
+
+def
test_update_destination_change_with_fresh_encrypted_extra_and_no_uri_password(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Engines that store credentials entirely in `encrypted_extra` and carry
+ no URI password at all (BigQuery, GSheets) must still be able to move
+ destinations when a genuinely fresh credential is supplied -- gating
+ solely on URI-password freshness would block them unconditionally,
+ since they never have one to give.
+ """
+
+ def _unmask(old: str, new: str) -> str:
+ old_config = json.loads(old)
+ new_config = json.loads(new)
+ for key, value in new_config.items():
+ if value == PASSWORD_MASK and key in old_config:
+ new_config[key] = old_config[key]
+ return json.dumps(new_config)
+
+ old_database = mocker.MagicMock(allow_multi_catalog=False)
+ old_database.sqlalchemy_uri = "bigquery://old-project"
+ old_database.password = None
+ old_database.extra = "{}"
+ old_database.encrypted_extra = json.dumps({"credentials_info":
"old-creds"})
+ old_database.ssh_tunnel = None
+ old_database.db_engine_spec.unmask_encrypted_extra.side_effect = _unmask
+ old_database.get_default_catalog.return_value = "old-project"
+ old_database.id = 1
+
+ new_database = mocker.MagicMock(allow_multi_catalog=False)
+ new_database.get_default_catalog.return_value = "new-project"
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = old_database
+ database_dao.update.return_value = new_database
+
+ mocker.patch("superset.commands.database.update.SyncPermissionsCommand")
+ mocker.patch.object(UpdateDatabaseCommand, "_update_catalog_attribute")
+
+ UpdateDatabaseCommand(
+ 1,
+ {
+ "sqlalchemy_uri": "bigquery://old-project",
+ "extra": json.dumps(
+ {"engine_params": {"connect_args": {"host": "new-host"}}}
+ ),
+ "masked_encrypted_extra": json.dumps(
+ {"credentials_info": "brand-new-creds"}
+ ),
+ },
+ ).run()
+
+ database_dao.update.assert_called_once()
+
+
+def test_update_host_change_with_new_credentials(mocker: MockerFixture) ->
None:
+ """
+ A deliberate connection move is still possible when the update supplies
+ a fresh password for the new destination.
+ """
+ old_database = mocker.MagicMock(allow_multi_catalog=False)
+ old_database.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ old_database.password = "oldpass" # noqa: S105
+ old_database.extra = "{}"
+ old_database.encrypted_extra = "{}"
+ old_database.ssh_tunnel = None
+ old_database.get_default_catalog.return_value = "prod"
+ old_database.id = 1
+
+ new_database = mocker.MagicMock(allow_multi_catalog=False)
+ new_database.get_default_catalog.return_value = "prod"
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = old_database
+ database_dao.update.return_value = new_database
+
+ mocker.patch("superset.commands.database.update.SyncPermissionsCommand")
+ mocker.patch.object(UpdateDatabaseCommand, "_update_catalog_attribute")
+
+ UpdateDatabaseCommand(
+ 1,
+ {"sqlalchemy_uri": "postgresql://user:newpass@host2:5432/prod"},
+ ).run()
+
+ database_dao.update.assert_called_once()
+
+
+def test_update_unrelated_fields_still_work(mocker: MockerFixture) -> None:
+ """
+ An update that doesn't touch `sqlalchemy_uri`, `extra`, or `ssh_tunnel`
+ at all must not be blocked by the new destination-change check.
+ """
+ old_database = mocker.MagicMock(allow_multi_catalog=False)
+ old_database.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1"
+ old_database.extra = "{}"
+ old_database.ssh_tunnel = None
+ old_database.get_default_catalog.return_value = "prod"
+ old_database.id = 1
+
+ new_database = mocker.MagicMock(allow_multi_catalog=False)
+ new_database.get_default_catalog.return_value = "prod"
+
+ database_dao =
mocker.patch("superset.commands.database.update.DatabaseDAO")
+ database_dao.find_by_id.return_value = old_database
+ database_dao.update.return_value = new_database
+
+ mocker.patch("superset.commands.database.update.SyncPermissionsCommand")
+ mocker.patch.object(UpdateDatabaseCommand, "_update_catalog_attribute")
+
+ UpdateDatabaseCommand(1, {"expose_in_sqllab": False}).run()
+
+ database_dao.update.assert_called_once()
diff --git a/tests/unit_tests/commands/databases/validate_test.py
b/tests/unit_tests/commands/databases/validate_test.py
index 1d507bd944a..85d4cf9268f 100644
--- a/tests/unit_tests/commands/databases/validate_test.py
+++ b/tests/unit_tests/commands/databases/validate_test.py
@@ -24,7 +24,9 @@ from superset.commands.database.exceptions import (
InvalidParametersError,
)
from superset.commands.database.validate import
ValidateDatabaseParametersCommand
+from superset.constants import PASSWORD_MASK
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.utils import json
def test_command(mocker: MockerFixture) -> None:
@@ -508,6 +510,88 @@ def test_ssh_tunnel_forwarded_to_connection_test(
)
+def test_ssh_tunnel_unmasked_when_only_engine_params_changed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An unrelated `extra.engine_params` change must not block reattaching
+ the stored SSH tunnel password when the tunnel's own endpoint is
+ unchanged -- gating the tunnel unmask on the combined identity flag
+ (rather than the tunnel's own endpoint check) would spuriously fail a
+ perfectly legitimate connection test.
+ """
+ mocker.patch(
+ "superset.commands.database.validate.is_feature_enabled",
+ return_value=True,
+ )
+
+ tunnel = mocker.MagicMock()
+ tunnel.server_address = "ssh.example.com"
+ tunnel.server_port = 22
+ tunnel.password = "real-tunnel-secret" # noqa: S105
+
+ existing = mocker.MagicMock()
+ existing.safe_sqlalchemy_uri.return_value =
"postgresql://u:XXXXXXXXXX@host1/d"
+ existing.sqlalchemy_uri_decrypted = "postgresql://u:realpass@host1/d"
+ existing.encrypted_extra = "{}"
+ existing.extra = "{}"
+ existing.ssh_tunnel = tunnel
+
+ database = mocker.MagicMock()
+ with database.get_sqla_engine() as engine:
+ engine.dialect.do_ping.return_value = True
+
+ DatabaseDAO = mocker.patch( # noqa: N806
+ "superset.commands.database.validate.DatabaseDAO"
+ )
+ DatabaseDAO.find_by_id.return_value = existing
+ DatabaseDAO.validate_update_uniqueness.return_value = True
+ DatabaseDAO.build_db_for_connection_test.return_value = database
+
+ mocker.patch(
+ "superset.commands.database.validate.get_engine_spec",
+ return_value=mocker.MagicMock(
+ validate_parameters=mocker.MagicMock(return_value=[]),
+ # a fresh (non-masked) password, as if the user supplied the
+ # real one for the main connection -- this is the legitimate
+ # path that gets past the main URI-identity check below
+ build_sqlalchemy_uri=mocker.MagicMock(
+ return_value="postgresql://u:realpass@host1/d"
+ ),
+ unmask_encrypted_extra=mocker.MagicMock(return_value="{}"),
+ ),
+ )
+
+ properties = {
+ "id": 1,
+ "engine": "postgresql",
+ "parameters": {
+ "host": "host1",
+ "port": 5432,
+ "username": "u",
+ "password": "realpass",
+ "database": "d",
+ },
+ # unrelated to the tunnel, but still an identity-affecting change
+ "extra": json.dumps(
+ {"engine_params": {"connect_args": {"connect_timeout": 30}}}
+ ),
+ "ssh_tunnel": {
+ "server_address": "ssh.example.com",
+ "server_port": 22,
+ "username": "tunnel_user",
+ "password": PASSWORD_MASK,
+ },
+ }
+ command = ValidateDatabaseParametersCommand(properties)
+ command.run()
+
+ forwarded_tunnel =
DatabaseDAO.build_db_for_connection_test.call_args.kwargs[
+ "ssh_tunnel"
+ ]
+ assert forwarded_tunnel["password"] == "real-tunnel-secret" # noqa: S105
+
+
def test_get_ssh_tunnel_errors_skipped_when_parameters_ssh_false(
mocker: MockerFixture,
) -> None:
@@ -648,6 +732,127 @@ def
test_validate_ssh_tunnel_feature_disabled_via_parameters_ssh(
)
+def test_validate_engine_params_change_requires_new_credentials(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Validating an existing database's parameters (unchanged host/port/etc,
+ so `sqlalchemy_uri` matches the stored masked URI) must not silently
+ reuse the stored password when `extra.engine_params` would redirect the
+ actual DBAPI connect kwargs elsewhere.
+ """
+ existing = mocker.MagicMock()
+ existing.safe_sqlalchemy_uri.return_value =
"postgresql://u:XXXXXXXXXX@host1/d"
+ existing.sqlalchemy_uri_decrypted = "postgresql://u:realpass@host1/d"
+ existing.encrypted_extra = "{}"
+ existing.extra = "{}"
+ existing.ssh_tunnel = None
+
+ DatabaseDAO = mocker.patch( # noqa: N806
+ "superset.commands.database.validate.DatabaseDAO"
+ )
+ DatabaseDAO.find_by_id.return_value = existing
+ DatabaseDAO.validate_update_uniqueness.return_value = True
+
+ mocker.patch(
+ "superset.commands.database.validate.get_engine_spec",
+ return_value=mocker.MagicMock(
+ validate_parameters=mocker.MagicMock(return_value=[]),
+ build_sqlalchemy_uri=mocker.MagicMock(
+ return_value="postgresql://u:XXXXXXXXXX@host1/d"
+ ),
+ unmask_encrypted_extra=mocker.MagicMock(return_value="{}"),
+ ),
+ )
+
+ properties = {
+ "id": 1,
+ "engine": "postgresql",
+ "parameters": {
+ "host": "host1",
+ "port": 5432,
+ "username": "u",
+ "database": "d",
+ },
+ "extra": json.dumps(
+ {
+ "engine_params": {
+ "connect_args": {"host": "attacker.example.com", "port":
15432}
+ }
+ }
+ ),
+ }
+ command = ValidateDatabaseParametersCommand(properties)
+ with pytest.raises(InvalidParametersError):
+ command.run()
+
+ DatabaseDAO.build_db_for_connection_test.assert_not_called()
+
+
+def test_validate_ssh_tunnel_host_change_requires_new_credentials(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Validating with an unchanged host/parameters set must not silently
+ reuse the stored SSH tunnel password when the tunnel endpoint itself
+ is being redirected.
+ """
+ mocker.patch(
+ "superset.commands.database.validate.is_feature_enabled",
+ return_value=True,
+ )
+
+ tunnel = mocker.MagicMock()
+ tunnel.server_address = "10.0.0.1"
+ tunnel.server_port = 22
+
+ existing = mocker.MagicMock()
+ existing.safe_sqlalchemy_uri.return_value =
"postgresql://u:XXXXXXXXXX@host1/d"
+ existing.sqlalchemy_uri_decrypted = "postgresql://u:realpass@host1/d"
+ existing.encrypted_extra = "{}"
+ existing.extra = "{}"
+ existing.ssh_tunnel = tunnel
+
+ DatabaseDAO = mocker.patch( # noqa: N806
+ "superset.commands.database.validate.DatabaseDAO"
+ )
+ DatabaseDAO.find_by_id.return_value = existing
+ DatabaseDAO.validate_update_uniqueness.return_value = True
+
+ mocker.patch(
+ "superset.commands.database.validate.get_engine_spec",
+ return_value=mocker.MagicMock(
+ validate_parameters=mocker.MagicMock(return_value=[]),
+ build_sqlalchemy_uri=mocker.MagicMock(
+ return_value="postgresql://u:XXXXXXXXXX@host1/d"
+ ),
+ unmask_encrypted_extra=mocker.MagicMock(return_value="{}"),
+ ),
+ )
+
+ properties = {
+ "id": 1,
+ "engine": "postgresql",
+ "parameters": {
+ "host": "host1",
+ "port": 5432,
+ "username": "u",
+ "database": "d",
+ },
+ "ssh_tunnel": {
+ "server_address": "attacker.example.com",
+ "server_port": 22,
+ "username": "tunnel_user",
+ "password": PASSWORD_MASK,
+ },
+ }
+ command = ValidateDatabaseParametersCommand(properties)
+ with pytest.raises(InvalidParametersError):
+ command.run()
+
+ DatabaseDAO.build_db_for_connection_test.assert_not_called()
+
+
def test_ssh_tunnel_missing_message_is_interpolated(
mocker: MockerFixture,
) -> None:
diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py
b/tests/unit_tests/databases/commands/importers/v1/import_test.py
index 2ccf0cc1fb2..27c2e4c6f69 100644
--- a/tests/unit_tests/databases/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py
@@ -593,6 +593,49 @@ def test_import_database_host_change_with_new_credentials(
assert database.password == "newpass" # noqa: S105
+def test_import_database_engine_params_change_requires_new_credentials(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ An overwrite that changes `extra.engine_params` (e.g.
+ `connect_args.host`/`port`, which the DBAPI merges into the actual
+ connection target ahead of anything carried in `sqlalchemy_uri`) must
+ not silently reuse the stored password: the submitted URI can still
+ match the stored host, while the DBAPI actually connects elsewhere.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import database_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
mocker.patch("superset.commands.database.importers.v1.utils.add_permissions")
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config)
+ database = import_database(config)
+ assert database.password == "pass" # noqa: S105
+
+ hostile = copy.deepcopy(database_config)
+ hostile["sqlalchemy_uri"] = f"postgresql://user:{PASSWORD_MASK}@host1"
+ hostile["extra"] = {
+ "engine_params": {
+ "connect_args": {"host": "attacker.example.com", "port": 15432}
+ }
+ }
+ with pytest.raises(ImportFailedError):
+ import_database(hostile, overwrite=True)
+
+ # the same engine_params (a normal re-import of an exported bundle) still
works
+ unchanged = copy.deepcopy(database_config)
+ unchanged["sqlalchemy_uri"] = f"postgresql://user:{PASSWORD_MASK}@host1"
+ unchanged["password"] = "pass" # noqa: S105
+ database = import_database(unchanged, overwrite=True)
+ assert database.password == "pass" # noqa: S105
+
+
def test_import_database_unparseable_uri_treated_as_change(
mocker: MockerFixture, session: Session
) -> None:
