rusackas commented on code in PR #44036:
URL: https://github.com/apache/superset/pull/44036#discussion_r3963770514
##########
superset/db_engine_specs/base.py:
##########
@@ -901,6 +901,38 @@ def get_oauth2_config(cls) -> OAuth2ClientConfig | None:
return config
+ @staticmethod
+ def _validate_oauth2_endpoint_host(uri: str) -> None:
+ """
+ Validate an OAuth2 authorization/token endpoint URI before it's used.
+
+ ``config["authorization_request_uri"]``/``config["token_request_uri"]``
+ can come from a database's own ``encrypted_extra.oauth2_client_info``
+ (editable by anyone with ``can_write`` on Database, not just the
+ deployment operator). The authorization URI is handed to the user's
+ browser as a redirect target; the token URI is POSTed to directly by
+ this server, carrying the connection's ``client_secret`` in the
+ request body. Neither is otherwise validated, so an attacker with
+ write access to one database's config could point either at an
+ internal host, exfiltrating the client secret (token URI) or using
+ Superset as an open redirect into the internal network (authorization
+ URI) -- and since the connection is typically shared, this is
+ exercised by every user who goes through that database's OAuth2 flow,
+ not just the one who configured it.
+
+ Operators with a legitimately internal IdP can opt out via
+ ``DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS``.
+ """
+ if app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]:
+ return
Review Comment:
Good catch, fixed. The scheme check now runs before the
`DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS` early-return instead of after it, so
that flag only widens which hosts are acceptable, not which schemes are.
##########
superset/db_engine_specs/base.py:
##########
@@ -901,6 +901,38 @@ def get_oauth2_config(cls) -> OAuth2ClientConfig | None:
return config
+ @staticmethod
+ def _validate_oauth2_endpoint_host(uri: str) -> None:
+ """
+ Validate an OAuth2 authorization/token endpoint URI before it's used.
+
+ ``config["authorization_request_uri"]``/``config["token_request_uri"]``
+ can come from a database's own ``encrypted_extra.oauth2_client_info``
+ (editable by anyone with ``can_write`` on Database, not just the
+ deployment operator). The authorization URI is handed to the user's
+ browser as a redirect target; the token URI is POSTed to directly by
+ this server, carrying the connection's ``client_secret`` in the
+ request body. Neither is otherwise validated, so an attacker with
+ write access to one database's config could point either at an
+ internal host, exfiltrating the client secret (token URI) or using
+ Superset as an open redirect into the internal network (authorization
+ URI) -- and since the connection is typically shared, this is
+ exercised by every user who goes through that database's OAuth2 flow,
+ not just the one who configured it.
+
+ Operators with a legitimately internal IdP can opt out via
+ ``DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS``.
+ """
+ if app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]:
+ return
+
+ parsed = urlparse(uri)
Review Comment:
Good catch, wrapped the `urlparse` call in a try/except now. The
token-exchange path already had a broad except further up the stack, but
`get_oauth2_authorization_uri`/`start_oauth2_dance` didn't.
##########
superset/db_engine_specs/base.py:
##########
@@ -947,6 +980,7 @@ def get_oauth2_token(
"""
timeout = app.config["DATABASE_OAUTH2_TIMEOUT"].total_seconds()
uri = config["token_request_uri"]
+ cls._validate_oauth2_endpoint_host(uri)
Review Comment:
Agreed, this one's real, thanks. Added a peer-validating requester (same
pattern already used for webhook dispatch in
`superset/reports/notifications/webhook.py`, factored out into
`superset.utils.network.get_ssrf_safe_requester`) and turned off redirect
following on both token calls.
##########
superset/commands/semantic_layer/update.py:
##########
@@ -71,6 +88,21 @@ def _unmask_configuration(
except (TypeError, ValueError):
existing_configuration = {}
+ masked_keys = {
+ key
+ for key, value in new_configuration.items()
+ if value == PASSWORD_MASK and key in existing_configuration
+ }
+ if masked_keys and any(
+ key not in masked_keys and existing_configuration.get(key) != value
+ for key, value in new_configuration.items()
+ ):
Review Comment:
Good catch, fixed. Switched to a sentinel default (`.get(key, _MISSING)`) so
an absent key can't be confused with one explicitly stored as `None`.
--
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]
