sha174n commented on code in PR #44003:
URL: https://github.com/apache/superset/pull/44003#discussion_r3959662653
##########
superset/commands/database/update.py:
##########
@@ -180,3 +188,50 @@ def validate(self) -> None:
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 and submitted_password in (
+ None,
+ PASSWORD_MASK,
+ ):
+ raise
DatabaseInvalidError(exceptions=[DatabaseUpdateUnsafeRebindError()])
Review Comment:
The rebind check here covers the URI password and (separately) the SSH
tunnel, but not `encrypted_extra`. Since a masked `masked_encrypted_extra` is
reattached to the submitted destination via `unmask_encrypted_extra` in
`run()`, gating only on the URI password (a) rejects legitimate destination
changes for engines that keep auth in `encrypted_extra` and carry no URI
password (e.g. BigQuery, GSheets), and (b) lets a reused `encrypted_extra`
carry over when a fresh URI password is supplied. Suggest gating
`encrypted_extra` on its own freshness too, the way the SSH-tunnel path already
does:
```suggestion
if connection_identity_changed:
try:
stored_uri_password =
make_url_safe(model.sqlalchemy_uri).password
except DatabaseInvalidError:
stored_uri_password = None
uri_password_reused = stored_uri_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.
encrypted_extra_reused = bool(model.encrypted_extra) 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()]
)
```
One caveat: `bool(model.encrypted_extra)` slightly over-triggers when
`encrypted_extra` is present but holds no secret (e.g. `"{}"`); a "has stored
secret" check would be tighter. The same pattern applies anywhere else a stored
`encrypted_extra` is reattached on a changed destination.
--
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]
