fix: tighten object-level and destination checks across four unrelated endpoints (#44036)

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: rusackas

superset/commands/database/exceptions.py

@@ -55,10 +55,11 @@ 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."
+                "destination (host/port, engine parameters, SSH tunnel "
+                "endpoint, or OAuth2 endpoint URIs) while reusing the stored "
+                "credential. Provide the real password (or SSH tunnel "
+                "credential / OAuth2 client secret) to confirm a connection "
+                "move."
             ),
             field_name=field_name,
         )

superset/commands/database/update.py

@@ -37,6 +37,7 @@
 from superset.commands.database.sync_permissions import SyncPermissionsCommand
 from superset.commands.database.utils import (
     engine_params_changed,
+    oauth2_endpoint_rebind_unsafe,
     ssh_tunnel_rebind_unsafe,
     uri_identity_changed,
 )
@@ -255,8 +256,9 @@ def validate(self) -> None:
     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.
+        (URI host/port, `extra.engine_params`, the SSH tunnel endpoint, or the
+        OAuth2 endpoint URIs in `encrypted_extra`) 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
@@ -327,3 +329,17 @@ def _check_no_unsafe_secret_rebind(self) -> None:
             raise DatabaseInvalidError(
                 exceptions=[DatabaseUpdateUnsafeRebindError(field_name="ssh_tunnel")]
             )
+
+        # The OAuth2 endpoints live inside encrypted_extra, so a change there is
+        # invisible to the URI/engine-params check above -- yet the stored client
+        # secret is what the next token exchange posts to the new endpoint.
+        if "masked_encrypted_extra" in self._properties and (
+            oauth2_endpoint_rebind_unsafe(
+                model.encrypted_extra, self._properties["masked_encrypted_extra"]
+            )
+        ):
+            raise DatabaseInvalidError(
+                exceptions=[
+                    DatabaseUpdateUnsafeRebindError(field_name="masked_encrypted_extra")
+                ]
+            )

superset/commands/database/utils.py

@@ -135,6 +135,53 @@ def ssh_tunnel_rebind_unsafe(
     return not has_fresh_credential or stale_private_key_password
 
 
+OAUTH2_ENDPOINT_FIELDS = (
+    "authorization_request_uri",
+    "token_request_uri",
+    "redirect_uri",
+)
+
+
+def oauth2_endpoint_rebind_unsafe(
+    existing_encrypted_extra: str | None, submitted_masked_encrypted_extra: str | None
+) -> bool:
+    """
+    Whether a submitted ``masked_encrypted_extra`` repoints the OAuth2 client
+    at different endpoint URIs while reusing the stored client secret (the
+    ``$.oauth2_client_info.secret`` mask is restored verbatim by
+    ``unmask_encrypted_extra`` before the update persists).
+
+    The URI/engine-params destination check does not see these fields, yet
+    the next token exchange posts the real client secret to whatever
+    ``token_request_uri`` now says -- so an endpoint change must come with a
+    freshly supplied secret, exactly like a host change must come with a
+    fresh password.
+    """
+    try:
+        existing = json.loads(existing_encrypted_extra or "{}")
+        submitted = json.loads(submitted_masked_encrypted_extra or "{}")
+    except (TypeError, ValueError):
+        return False  # malformed payloads are rejected by schema validation elsewhere
+    existing_info = (
+        existing.get("oauth2_client_info") if isinstance(existing, dict) else None
+    )
+    submitted_info = (
+        submitted.get("oauth2_client_info") if isinstance(submitted, dict) else None
+    )
+    if not isinstance(existing_info, dict) or not isinstance(submitted_info, dict):
+        return False
+    if not existing_info.get("secret"):
+        return False  # nothing stored to carry over
+    # Only the mask sentinel restores the stored secret; an absent key drops it,
+    # and a different value is a fresh secret the caller is entitled to attach.
+    secret_reused = submitted_info.get("secret") == PASSWORD_MASK
+    endpoint_changed = any(
+        existing_info.get(field) != submitted_info.get(field)
+        for field in OAUTH2_ENDPOINT_FIELDS
+    )
+    return secret_reused and endpoint_changed
+
+
 def ping(engine: Engine) -> bool:
     try:
         time_delta = app.config["TEST_DATABASE_CONNECTION_TIMEOUT"]

superset/commands/semantic_layer/update.py

@@ -35,6 +35,7 @@
 )
 from superset.commands.semantic_layer.utils import validate_configuration
 from superset.commands.utils import current_user_can_modify_object
+from superset.constants import PASSWORD_MASK
 from superset.daos.semantic_layer import SemanticLayerDAO, SemanticViewDAO
 from superset.exceptions import SupersetSecurityException
 from superset.semantic_layers.masking import (
@@ -49,6 +50,10 @@
 
 logger = logging.getLogger(__name__)
 
+# Sentinel distinguishing "key absent" from "key present with value None"
+# when reading the stored configuration -- dict.get's own default can't.
+_MISSING = object()
+
 
 def _unmask_configuration(
     existing_raw_configuration: str | None,
@@ -64,13 +69,61 @@ def _unmask_configuration(
     otherwise overwrite the real stored values with the mask string. This
     delegates to :func:`superset.semantic_layers.masking.unmask_configuration`,
     which restores masked values recursively so nested/union secrets survive
-    the round-trip too, not just top-level ones."""
+    the round-trip too, not just top-level ones.
+
+    A masked top-level key is only ever restored, though, when every OTHER
+    submitted top-level key is unchanged from what's stored -- including a
+    stored key being dropped from the payload -- i.e. this is a pure "reveal
+    what I was shown masked" round-trip, not an edit that also changes some
+    other connector field. Without that check, an editor (entitled to edit
+    this connection, but not to see its real secret -- that's the entire
+    reason GET/list mask it) could reveal a masked value while simultaneously
+    changing a destination-relevant field in the same request, poisoning the
+    stored configuration: the very next legitimate call through this layer
+    (``POST /<uuid>/schema/runtime`` always uses the stored, now-poisoned
+    configuration) would send the real secret to wherever that field now
+    points. Semantic layer connector schemas are pluggable and defined
+    outside this repo (see ``superset/core/api/core_api_injection.py``), so
+    unlike the analogous database-connection fix there's no fixed
+    "destination fields" list to narrow this to -- any other top-level field
+    changing at all is treated as unsafe to combine with a secret reveal.
+    """
     try:
         existing_configuration: dict[str, Any] = (
             json.loads(existing_raw_configuration) if existing_raw_configuration else {}
         )
     except (TypeError, ValueError):
         existing_configuration = {}
+
+    masked_keys = {
+        key
+        for key, value in new_configuration.items()
+        if value == PASSWORD_MASK and key in existing_configuration
+    }
+    # `.get(key)` alone can't tell "key absent from storage" apart from "key
+    # present and stored as None" -- both return None -- so a newly
+    # introduced key with an explicit None value would be misread as
+    # unchanged and let a masked secret slip through alongside it. A
+    # sentinel default makes that distinction explicit. Iterating only the
+    # submitted keys would also miss a REMOVED key: the update replaces the
+    # stored dictionary wholesale, so dropping an optional field while
+    # reusing the masked secret changes the effective configuration just as
+    # surely as editing one.
+    removed_keys = set(existing_configuration) - set(new_configuration)
+    if masked_keys and (
+        removed_keys
+        or any(
+            key not in masked_keys
+            and existing_configuration.get(key, _MISSING) != value
+            for key, value in new_configuration.items()
+        )
+    ):
+        raise SemanticLayerInvalidError(
+            "This update changes the configuration while reusing a stored "
+            "secret value (a masked field). Provide the real value for any "
+            "masked field to confirm a configuration change."
+        )
+
     masked_reference: dict[str, Any] = mask_configuration(
         layer_type, existing_configuration
     )

superset/config.py

@@ -2777,6 +2777,15 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument  # noq
 # Timeout when fetching access and refresh tokens.
 DATABASE_OAUTH2_TIMEOUT = timedelta(seconds=30)
 
+# When True, the OAuth2 authorization/token endpoint URIs configured for a
+# database (either via DATABASE_OAUTH2_CLIENTS or, per-connection, via a
+# database's own encrypted_extra.oauth2_client_info) are permitted to target
+# hosts in private/internal IP ranges (RFC-1918, loopback, link-local).
+# Intended for deployments with a legitimately internal identity provider.
+# Leave False (the default) in any deployment where untrusted users can
+# create or edit database connections.
+DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS: bool = False
+
 # Enable/disable CSP warning
 CONTENT_SECURITY_POLICY_WARNING = True
 

superset/db_engine_specs/base.py

@@ -35,11 +35,10 @@
     TypedDict,
     Union,
 )
-from urllib.parse import urlencode, urljoin
+from urllib.parse import urlencode, urljoin, urlparse
 from uuid import UUID, uuid4
 
 import pandas as pd
-import requests
 from apispec import APISpec
 from apispec.ext.marshmallow import MarshmallowPlugin
 from deprecation import deprecated
@@ -95,7 +94,12 @@
 from superset.utils.core import ColumnSpec, GenericDataType, QuerySource
 from superset.utils.hashing import hash_from_str
 from superset.utils.json import redact_sensitive, reveal_sensitive
-from superset.utils.network import is_hostname_valid, is_port_open
+from superset.utils.network import (
+    get_ssrf_safe_requester,
+    is_hostname_valid,
+    is_port_open,
+    is_safe_host,
+)
 from superset.utils.oauth2 import (
     encode_oauth2_state,
     generate_code_challenge,
@@ -901,6 +905,47 @@ 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`` -- but that flag only
+        widens which *hosts* are acceptable, not which URI *schemes* are;
+        a non-http(s) scheme is refused unconditionally.
+        """
+        try:
+            parsed = urlparse(uri)
+        except ValueError as ex:
+            # e.g. an unmatched IPv6 bracket -- urlparse raises rather than
+            # returning an unusable result.
+            raise OAuth2Error("Invalid OAuth2 endpoint URI") from ex
+
+        if parsed.scheme not in ("http", "https"):
+            raise OAuth2Error("Invalid OAuth2 endpoint URI")
+
+        if app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]:
+            return
+
+        if not parsed.hostname or not is_safe_host(parsed.hostname):
+            logger.warning("OAuth2 endpoint refused: target host is not allowed")
+            raise OAuth2Error("Invalid OAuth2 endpoint URI")
+
     @classmethod
     def get_oauth2_authorization_uri(
         cls,
@@ -916,6 +961,7 @@ def get_oauth2_authorization_uri(
         (e.g., Google's prompt=consent).
         """
         uri = config["authorization_request_uri"]
+        cls._validate_oauth2_endpoint_host(uri)
         params: dict[str, str] = {
             "scope": config["scope"],
             "response_type": "code",
@@ -947,6 +993,7 @@ def get_oauth2_token(
         """
         timeout = app.config["DATABASE_OAUTH2_TIMEOUT"].total_seconds()
         uri = config["token_request_uri"]
+        cls._validate_oauth2_endpoint_host(uri)
         req_body: dict[str, str] = {
             "code": code,
             "client_id": config["id"],
@@ -959,10 +1006,21 @@ def get_oauth2_token(
         if code_verifier:
             req_body["code_verifier"] = code_verifier
 
+        # `_validate_oauth2_endpoint_host` only checked the hostname; a
+        # server at that (safe) host could still respond with a 30x
+        # redirecting the actual request to an internal target, or a
+        # low-TTL DNS record could resolve differently by the time this
+        # connects (DNS rebinding). Don't follow redirects, and re-validate
+        # the address actually connected to.
+        requester = get_ssrf_safe_requester(
+            allow_unsafe_hosts=app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]
+        )
         response = (
-            requests.post(uri, data=req_body, timeout=timeout)
+            requester.post(uri, data=req_body, timeout=timeout, allow_redirects=False)
             if config["request_content_type"] == "data"
-            else requests.post(uri, json=req_body, timeout=timeout)
+            else requester.post(
+                uri, json=req_body, timeout=timeout, allow_redirects=False
+            )
         )
         response.raise_for_status()
         return response.json()
@@ -978,16 +1036,26 @@ def get_oauth2_fresh_token(
         """
         timeout = app.config["DATABASE_OAUTH2_TIMEOUT"].total_seconds()
         uri = config["token_request_uri"]
+        cls._validate_oauth2_endpoint_host(uri)
         req_body = {
             "client_id": config["id"],
             "client_secret": config["secret"],
             "refresh_token": refresh_token,
             "grant_type": "refresh_token",
         }
+        # See the matching comment in ``get_oauth2_token``: the hostname
+        # check above doesn't protect against a 30x redirect to an internal
+        # target or DNS rebinding, so route through the peer-validating
+        # requester and refuse to follow redirects.
+        requester = get_ssrf_safe_requester(
+            allow_unsafe_hosts=app.config["DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS"]
+        )
         response = (
-            requests.post(uri, data=req_body, timeout=timeout)
+            requester.post(uri, data=req_body, timeout=timeout, allow_redirects=False)
             if config["request_content_type"] == "data"
-            else requests.post(uri, json=req_body, timeout=timeout)
+            else requester.post(
+                uri, json=req_body, timeout=timeout, allow_redirects=False
+            )
         )
         if response.status_code in (400, 401, 403):
             try:

superset/db_engine_specs/gsheets.py

@@ -197,6 +197,7 @@ def get_oauth2_authorization_uri(
         from superset.utils.oauth2 import encode_oauth2_state, generate_code_challenge
 
         uri = config["authorization_request_uri"]
+        cls._validate_oauth2_endpoint_host(uri)
         params: dict[str, str] = {
             "scope": config["scope"],
             "response_type": "code",

superset/db_engine_specs/snowflake.py

@@ -373,6 +373,7 @@ def get_oauth2_authorization_uri(
         Return URI for initial OAuth2 request.
         """
         uri = config["authorization_request_uri"]
+        cls._validate_oauth2_endpoint_host(uri)
         # When calling the Snowflake OAuth authorization endpoint for a custom client,
         # specify only the query parameters documented in the URL below.
         # Adding unsupported parameters

superset/security/api.py

@@ -37,6 +37,7 @@
 from sqlalchemy.orm import selectinload
 
 from superset.commands.dashboard.embedded.exceptions import (
+    EmbeddedDashboardAccessDeniedError,
     EmbeddedDashboardNotFoundError,
 )
 from superset.commands.exceptions import ForbiddenError
@@ -223,7 +224,9 @@ def guest_token(self) -> Response:
         """
         try:
             body = guest_token_create_schema.load(request.json)
-            self.appbuilder.sm.validate_guest_token_resources(body["resources"])
+            self.appbuilder.sm.validate_guest_token_resources(
+                body["resources"], datasets=body.get("datasets")
+            )
             guest_token_validator_hook = current_app.config.get(
                 "GUEST_TOKEN_VALIDATOR_HOOK"
             )
@@ -265,6 +268,14 @@ def guest_token(self) -> Response:
             return self.response(200, token=token)
         except EmbeddedDashboardNotFoundError as error:
             return self.response_400(message=error.message)
+        except EmbeddedDashboardAccessDeniedError as error:
+            # The minting principal is not entitled to the dashboard being
+            # scoped (see validate_guest_token_resources): an authorization
+            # denial, not a server fault, so answer 403 rather than letting
+            # @safe turn it into a logged 500.
+            # FAB 5.x: response_403() takes no message argument (unlike
+            # response_400), so build the 403 explicitly.
+            return self.response(403, message=error.message)
         except ValidationError as error:
             return self.response_400(message=error.messages)