sadpandajoe commented on code in PR #43474:
URL: https://github.com/apache/superset/pull/43474#discussion_r3856142155
##########
superset/semantic_layers/api.py:
##########
@@ -81,10 +81,51 @@
logger = logging.getLogger(__name__)
+def _mask_configuration(layer: SemanticLayer, config: dict[str, Any]) ->
dict[str, Any]:
+ """
+ Redact configuration values the connector's schema marks as write-only.
+
+ A connector publishes its configuration shape via
``get_configuration_schema``;
+ a property with ``"writeOnly": true`` (the standard JSON Schema way of
+ marking a field that's set but never echoed back, e.g. a password or API
+ key) is replaced with ``PASSWORD_MASK`` here rather than returned in the
+ clear.
+ """
+ cls = registry.get(layer.type)
+ if not cls:
Review Comment:
If a persisted layer's extension is removed or fails to register, this
returns its decrypted configuration unchanged, so the detail and list endpoints
can expose its credentials. Should the missing-connector path fail closed like
the schema-load error path?
##########
superset/semantic_layers/api.py:
##########
@@ -81,10 +81,51 @@
logger = logging.getLogger(__name__)
+def _mask_configuration(layer: SemanticLayer, config: dict[str, Any]) ->
dict[str, Any]:
+ """
+ Redact configuration values the connector's schema marks as write-only.
+
+ A connector publishes its configuration shape via
``get_configuration_schema``;
+ a property with ``"writeOnly": true`` (the standard JSON Schema way of
+ marking a field that's set but never echoed back, e.g. a password or API
+ key) is replaced with ``PASSWORD_MASK`` here rather than returned in the
+ clear.
+ """
+ cls = registry.get(layer.type)
+ if not cls:
+ return config
+
+ try:
+ schema = cls.get_configuration_schema()
+ except Exception: # pylint: disable=broad-except
+ # We can't tell which fields are secret, so fail closed: mask every
+ # truthy value rather than risk echoing a credential back in the clear.
+ logger.warning(
+ "Failed to load configuration schema for semantic layer type %s; "
+ "masking all configuration values.",
+ layer.type,
+ )
+ return {key: PASSWORD_MASK if value else value for key, value in
config.items()}
+
+ secret_keys = {
+ key
+ for key, prop in schema.get("properties", {}).items()
+ if isinstance(prop, dict) and prop.get("writeOnly")
+ }
+ if not secret_keys:
+ return config
+
+ return {
+ key: PASSWORD_MASK if key in secret_keys and value else value
Review Comment:
Masking this value in GET means the edit modal sends the mask back in its
full configuration on a name-only save, overwriting the stored credential and
breaking later connections. Can the update path preserve masked fields and
cover that edit round trip?
--
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]
