hughhhh commented on code in PR #45353:
URL: https://github.com/apache/superset/pull/45353#discussion_r4457949064
##########
superset/utils/log.py:
##########
@@ -125,6 +125,76 @@ def get_object_ids_from_view_args(
return {}
+# Request fields whose value is a secret, wherever they appear in the body: a
+# database password, an SSH tunnel credential, an OAuth2 client secret or
token,
+# an embedded fallback credential, a service-account key, or the import maps
and
+# Secure Extra blobs that carry them. The event log stores the request payload,
+# so without this the Action Log keeps every secret typed into the database
+# dialog, a connection test or an import in clear.
+SENSITIVE_PAYLOAD_KEYS = frozenset(
+ {
+ "access_token",
+ "client_secret",
+ "credentials_info",
+ "encrypted_extra",
+ "encrypted_extra_secrets",
+ "masked_encrypted_extra",
+ "password",
+ "passwords",
+ "private_key",
+ "private_key_password",
+ "privatekey_body",
+ "privatekey_pass",
+ "refresh_token",
+ "secret",
+ "service_account_info",
+ "ssh_tunnel_passwords",
+ "ssh_tunnel_private_key_passwords",
+ "ssh_tunnel_private_keys",
+ "token",
+ }
+)
+
+
+def _redact_uri(uri: Any) -> Any:
+ """Mask the password of a SQLAlchemy URI, keeping the rest readable."""
+ # Prevent circular import.
+ from superset.databases.utils import make_url_safe
+
+ if not isinstance(uri, str) or not uri:
+ return uri
+ try:
+ url = make_url_safe(uri)
+ except Exception: # pylint: disable=broad-except
+ return PASSWORD_MASK # unparseable: it may still hold a credential
+ if url.password is None:
+ return uri
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Fixed in 8363b4d4ba: the claim was correct. `_redact_uri` returned any URI
without a userinfo password unchanged, so `duckdb:///md:db?motherduck_token=…`
was logged in clear.
`_redact_uri` in `superset/utils/log.py` now masks every query value and
keeps the keys. It doesn't match on secret names, because engines choose their
own and a list would leak the next one. The cost is that harmless values like
`role` or `charset` are masked in the Action Log too.
The same change covers three related leaks:
- `parameters.query` is masked too, since the form builds the URI's query
string from it.
- A URI with an unencoded `@` in its password is masked whole. SQLAlchemy
parses `user:a@b@host` with host `b@host`, which left part of the password in
the output.
- A non-string `sqlalchemy_uri` is masked whole. A URI that fails to parse
was already masked, and the exception isn't logged because it could quote the
URI.
Tests in `tests/unit_tests/utils/log_tests.py`, which failed before the
change and pass now:
`test_redact_request_payload_masks_uri_secrets_outside_the_password`,
`test_redact_request_payload_masks_query_values_in_parameters`.
`test_collect_request_payload_masks_a_credential_in_a_uri_query` covers the
same leak through the request path and was added with the fix.
--
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]
