This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch fix/oauth2-guest-token-semantic-layer-boundaries in repository https://gitbox.apache.org/repos/asf/superset.git
commit 4ec3612e2fe8de352a526c9bbe3a79963aff8770 Author: Evan Rusackas <[email protected]> AuthorDate: Tue Sep 8 17:32:40 2026 -0700 fix: tighten object-level and destination checks across four unrelated endpoints Four independent fixes, bundled together for review efficiency (each touches a different subsystem, no shared root cause): - superset/db_engine_specs/base.py, gsheets.py, snowflake.py, config.py: a database's own encrypted_extra.oauth2_client_info sets authorization_request_uri/token_request_uri with no host validation. The token endpoint is POSTed to directly by the server, carrying the connection's client_secret in the body. Validate both endpoint hosts (is_safe_host, matching the existing pattern in db_engine_specs/impala.py's cancel_query and reports/notifications/webhook.py) before use, with an opt-out config flag for legitimately internal identity providers. - superset/security/manager.py: validate_guest_token_resources only checked that a dashboard exists and is embedded, never that the principal minting the token has access to it. A narrower can_grant_guest_token grant (a realistic non-Admin "embedding backend service" role) could mint a valid guest token scoped to any embedded dashboard in the instance. - superset/commands/semantic_layer/update.py: masked configuration fields were restored from the stored value unconditionally, without checking whether other submitted fields also changed. An editor could reveal a masked secret while changing an unrelated field in the same request, persisting the secret against changed configuration. - superset/views/utils.py: get_dashboard_extra_filters resolved a dashboard by id with only a chart-membership check, no access check. Chart reuse across dashboards means chart access doesn't imply dashboard access. Regression tests added for all four; each verified to fail without its corresponding fix and pass with it. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- superset/commands/semantic_layer/update.py | 32 ++++++ superset/config.py | 9 ++ superset/db_engine_specs/base.py | 39 ++++++- superset/db_engine_specs/gsheets.py | 1 + superset/db_engine_specs/snowflake.py | 1 + superset/security/manager.py | 17 ++- superset/views/utils.py | 12 ++ .../commands/semantic_layer/update_test.py | 43 +++++++ tests/unit_tests/db_engine_specs/test_base.py | 126 ++++++++++++++++++++- .../unit_tests/db_engine_specs/test_databricks.py | 4 + .../db_engine_specs/test_databricks_multi_cloud.py | 4 + tests/unit_tests/db_engine_specs/test_snowflake.py | 1 + tests/unit_tests/db_engine_specs/test_trino.py | 1 + tests/unit_tests/security/manager_test.py | 78 ++++++++++++- tests/unit_tests/views/test_utils.py | 69 ++++++++++- 15 files changed, 430 insertions(+), 7 deletions(-) diff --git a/superset/commands/semantic_layer/update.py b/superset/commands/semantic_layer/update.py index f01072d41ce..5a85ef175c5 100644 --- a/superset/commands/semantic_layer/update.py +++ b/superset/commands/semantic_layer/update.py @@ -63,6 +63,23 @@ def _unmask_configuration( write-only, since a client only ever sends the sentinel back for a value it previously received masked (including a value masked by the fail-closed fallback). + + A masked field is only ever restored, though, when every OTHER + submitted key is unchanged from what's stored -- 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 field changing at all is treated as unsafe + to combine with a secret reveal. """ try: existing_configuration = ( @@ -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() + ): + 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." + ) + return { key: ( existing_configuration[key] diff --git a/superset/config.py b/superset/config.py index eb20815eb56..b786f8c7023 100644 --- a/superset/config.py +++ b/superset/config.py @@ -2732,6 +2732,15 @@ DATABASE_OAUTH2_JWT_ALGORITHM = "HS256" # 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 diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index a287b856281..864319cd949 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -35,7 +35,7 @@ from typing import ( TypedDict, Union, ) -from urllib.parse import urlencode, urljoin +from urllib.parse import urlencode, urljoin, urlparse from uuid import UUID, uuid4 import pandas as pd @@ -95,7 +95,7 @@ from superset.utils import core as utils, json 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 is_hostname_valid, is_port_open, is_safe_host from superset.utils.oauth2 import ( encode_oauth2_state, generate_code_challenge, @@ -901,6 +901,38 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods 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) + if parsed.scheme not in ("http", "https"): + raise OAuth2Error("Invalid OAuth2 endpoint URI") + 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 +948,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods (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 +980,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods """ 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"], @@ -978,6 +1012,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods """ 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"], diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index 7fd1ac1156b..3d2e317d8c4 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -197,6 +197,7 @@ class GSheetsEngineSpec(ShillelaghEngineSpec): 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", diff --git a/superset/db_engine_specs/snowflake.py b/superset/db_engine_specs/snowflake.py index 63ae8886228..c1bcd86dca3 100644 --- a/superset/db_engine_specs/snowflake.py +++ b/superset/db_engine_specs/snowflake.py @@ -373,6 +373,7 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): 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 diff --git a/superset/security/manager.py b/superset/security/manager.py index ac8910b043f..303d3cbaf55 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -5332,10 +5332,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods audience = audience() return audience - @staticmethod - def validate_guest_token_resources(resources: GuestTokenResources) -> None: + def validate_guest_token_resources(self, resources: GuestTokenResources) -> None: # pylint: disable=import-outside-toplevel from superset.commands.dashboard.embedded.exceptions import ( + EmbeddedDashboardAccessDeniedError, EmbeddedDashboardNotFoundError, ) from superset.daos.dashboard import EmbeddedDashboardDAO @@ -5349,11 +5349,24 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods embedded = EmbeddedDashboardDAO.find_by_id(str(resource["id"])) if not embedded: raise EmbeddedDashboardNotFoundError() + dashboard = embedded.dashboard elif not dashboard.embedded: # A raw dashboard id must still reference an embedded dashboard; # otherwise a guest token could be scoped to a non-embedded one. raise EmbeddedDashboardNotFoundError() + # The caller minting the token must themselves be entitled to + # the dashboard being scoped. `grant_guest_token` is a + # coarse, instance-wide permission -- without this check, an + # operator who narrows it to a non-Admin role (a realistic + # "embedding backend service" grant) would let that + # principal mint a fully valid guest token for *any* + # embedded dashboard, not just ones they have access to. + try: + self.raise_for_access(dashboard=dashboard) + except SupersetSecurityException as ex: + raise EmbeddedDashboardAccessDeniedError() from ex + def create_guest_access_token( self, user: GuestTokenUser, diff --git a/superset/views/utils.py b/superset/views/utils.py index af982778fc7..63bb7037586 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -44,6 +44,7 @@ from superset.common.db_query_status import QueryStatus from superset.exceptions import ( SerializationError, SupersetException, + SupersetSecurityException, ) from superset.extensions import security_manager from superset.legacy import update_time_range @@ -504,6 +505,17 @@ def get_dashboard_extra_filters( ): return [] + # Does the caller actually have access to this dashboard? A chart can + # legitimately be reused across multiple dashboards, so passing chart + # membership above isn't an entitlement check -- without this, a + # principal who owns/can access some chart also embedded on a + # dashboard they have no access to could pull that dashboard's default + # filter configuration into their own request. + try: + security_manager.raise_for_access(dashboard=dashboard) + except SupersetSecurityException: + return [] + with contextlib.suppress(json.JSONDecodeError): json_metadata = json.loads(dashboard.json_metadata) native_filters = [ diff --git a/tests/unit_tests/commands/semantic_layer/update_test.py b/tests/unit_tests/commands/semantic_layer/update_test.py index 9ae58afccf3..800b67e6274 100644 --- a/tests/unit_tests/commands/semantic_layer/update_test.py +++ b/tests/unit_tests/commands/semantic_layer/update_test.py @@ -528,6 +528,49 @@ def test_unmask_configuration_missing_existing_key() -> None: assert result == {"account": "test", "password": PASSWORD_MASK} +def test_unmask_configuration_rejects_secret_reveal_with_changed_field() -> None: + """ + A masked field must not be revealed in the same update that also + changes some other configuration field. An editor is entitled to edit + this connection, but not to see its real secret (that's the entire + reason the read path masks it) -- revealing it while also changing a + potentially destination-relevant field would poison the stored + configuration with the real secret attached to attacker-controlled + config, silently leaking it on the next legitimate use of this layer. + """ + with pytest.raises(SemanticLayerInvalidError): + _unmask_configuration( + '{"account": "prod-account", "password": "hunter2"}', + {"account": "attacker-account", "password": PASSWORD_MASK}, + ) + + +def test_unmask_configuration_allows_fresh_secret_with_changed_field() -> None: + """ + A deliberate configuration change is still possible when a genuinely + fresh (non-masked) secret is supplied alongside it. + """ + result = _unmask_configuration( + '{"account": "prod-account", "password": "hunter2"}', + {"account": "new-account", "password": "fresh-secret"}, + ) + + assert result == {"account": "new-account", "password": "fresh-secret"} + + +def test_unmask_configuration_allows_unrelated_field_addition_with_no_mask() -> None: + """ + Adding/changing fields with no masked value present at all is + unaffected -- there's no secret being reused, so nothing to protect. + """ + result = _unmask_configuration( + '{"account": "prod-account"}', + {"account": "new-account", "extra_option": "value"}, + ) + + assert result == {"account": "new-account", "extra_option": "value"} + + def test_update_semantic_layer_preserves_masked_secret_end_to_end( mocker: MockerFixture, ) -> None: diff --git a/tests/unit_tests/db_engine_specs/test_base.py b/tests/unit_tests/db_engine_specs/test_base.py index e85ebe9be20..fca638d7d03 100644 --- a/tests/unit_tests/db_engine_specs/test_base.py +++ b/tests/unit_tests/db_engine_specs/test_base.py @@ -39,7 +39,7 @@ from superset.db_engine_specs.base import ( convert_inspector_columns, ) from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import OAuth2RedirectError +from superset.exceptions import OAuth2Error, OAuth2RedirectError from superset.sql.parse import Table from superset.superset_typing import ( OAuth2ClientConfig, @@ -953,6 +953,18 @@ def test_extract_errors_no_match_falls_back(mocker: MockerFixture) -> None: assert result == [expected] [email protected](autouse=True) +def _mock_safe_oauth2_host(mocker: MockerFixture) -> None: + """ + OAuth2 endpoint URIs are now validated via ``is_safe_host`` (real DNS + resolution) before use. The test fixtures below use non-resolving + example hostnames, so mock it the same way test_impala.py mocks it for + its own SSRF check; SSRF-rejection behavior itself is covered by + dedicated tests further down that override this per-test. + """ + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=True) + + def test_get_oauth2_authorization_uri_standard_params(mocker: MockerFixture) -> None: """ Test that BaseEngineSpec.get_oauth2_authorization_uri uses standard OAuth 2.0 @@ -1315,6 +1327,117 @@ def test_get_oauth2_fresh_token_raises_on_server_error(mocker: MockerFixture) -> BaseEngineSpec.get_oauth2_fresh_token(config, "refresh-token") +def _oauth2_config_targeting(uri: str) -> OAuth2ClientConfig: + return { + "id": "client-id", + "secret": "client-secret", + "scope": "read write", + "redirect_uri": "http://localhost:8088/api/v1/database/oauth2/";, + "authorization_request_uri": uri, + "token_request_uri": uri, + "request_content_type": "json", + } + + +def test_get_oauth2_token_rejects_unsafe_host(mocker: MockerFixture) -> None: + """ + ``token_request_uri`` can come from a database's own + ``encrypted_extra.oauth2_client_info`` (editable by anyone with + ``can_write`` on Database), and is POSTed to directly by this server + carrying the connection's client_secret. An internal/private target + must be refused rather than silently reaching it. + """ + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=False) + mock_requests = mocker.patch("superset.db_engine_specs.base.requests") + + config = _oauth2_config_targeting("http://169.254.169.254/latest/meta-data/";) + + with pytest.raises(OAuth2Error): + BaseEngineSpec.get_oauth2_token(config, "code") + + mock_requests.post.assert_not_called() + + +def test_get_oauth2_fresh_token_rejects_unsafe_host(mocker: MockerFixture) -> None: + """ + Same protection as ``get_oauth2_token``, for the refresh-token exchange. + """ + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=False) + mock_requests = mocker.patch("superset.db_engine_specs.base.requests") + + config = _oauth2_config_targeting("http://10.0.0.5/token";) + + with pytest.raises(OAuth2Error): + BaseEngineSpec.get_oauth2_fresh_token(config, "refresh-token") + + mock_requests.post.assert_not_called() + + +def test_get_oauth2_authorization_uri_rejects_unsafe_host( + mocker: MockerFixture, +) -> None: + """ + ``authorization_request_uri`` is handed to the user's browser as a + redirect target; an internal host would turn Superset into an open + redirect into the internal network. + """ + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=False) + + config = _oauth2_config_targeting("http://192.168.1.1/authorize";) + state: OAuth2State = { + "database_id": 1, + "user_id": 1, + "default_redirect_uri": "http://localhost:8088/api/v1/oauth2/";, + "tab_id": "1234", + } + + with pytest.raises(OAuth2Error): + BaseEngineSpec.get_oauth2_authorization_uri(config, state) + + +def test_oauth2_endpoint_rejects_non_http_scheme(mocker: MockerFixture) -> None: + """ + A non-http(s) scheme is refused outright, before any host resolution. + """ + is_safe_host = mocker.patch("superset.db_engine_specs.base.is_safe_host") + mock_requests = mocker.patch("superset.db_engine_specs.base.requests") + + config = _oauth2_config_targeting("file:///etc/passwd") + + with pytest.raises(OAuth2Error): + BaseEngineSpec.get_oauth2_token(config, "code") + + is_safe_host.assert_not_called() + mock_requests.post.assert_not_called() + + +def test_oauth2_endpoint_allows_internal_host_when_configured( + mocker: MockerFixture, +) -> None: + """ + Operators with a legitimately internal IdP can opt out via + DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS -- the host is not even checked + once that's set. + """ + mocker.patch.dict( + "superset.db_engine_specs.base.app.config", + {"DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS": True}, + ) + is_safe_host = mocker.patch("superset.db_engine_specs.base.is_safe_host") + mock_post = mocker.patch("superset.db_engine_specs.base.requests.post") + mock_post.return_value.json.return_value = { + "access_token": "access-token", + "expires_in": 3600, + } + + config = _oauth2_config_targeting("http://10.0.0.5/token";) + + BaseEngineSpec.get_oauth2_token(config, "code") + + is_safe_host.assert_not_called() + mock_post.assert_called_once() + + def test_start_oauth2_dance_uses_config_redirect_uri(mocker: MockerFixture) -> None: """ Test that start_oauth2_dance uses DATABASE_OAUTH2_REDIRECT_URI config if set. @@ -1327,6 +1450,7 @@ def test_start_oauth2_dance_uses_config_redirect_uri(mocker: MockerFixture) -> N "DATABASE_OAUTH2_REDIRECT_URI": custom_redirect_uri, "SECRET_KEY": "test-secret-key", "DATABASE_OAUTH2_JWT_ALGORITHM": "HS256", + "DATABASE_OAUTH2_ALLOW_INTERNAL_HOSTS": True, }, ) mocker.patch("superset.daos.key_value.KeyValueDAO") diff --git a/tests/unit_tests/db_engine_specs/test_databricks.py b/tests/unit_tests/db_engine_specs/test_databricks.py index a963a764d57..fa134303e5f 100644 --- a/tests/unit_tests/db_engine_specs/test_databricks.py +++ b/tests/unit_tests/db_engine_specs/test_databricks.py @@ -989,6 +989,10 @@ def test_get_oauth2_authorization_uri_derives_from_workspace_host( database = mocker.MagicMock() database.url_object.host = host mocker.patch("superset.db.session.get", return_value=database) + # is_safe_host does live DNS resolution; whether these fixture hosts + # happen to resolve depends on real-world DNS state outside test + # control, so pin it rather than relying on that. + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=True) url = spec.get_oauth2_authorization_uri( _unresolved_oauth2_config(), _oauth2_state() diff --git a/tests/unit_tests/db_engine_specs/test_databricks_multi_cloud.py b/tests/unit_tests/db_engine_specs/test_databricks_multi_cloud.py index 65f8218e3a1..948d97655b7 100644 --- a/tests/unit_tests/db_engine_specs/test_databricks_multi_cloud.py +++ b/tests/unit_tests/db_engine_specs/test_databricks_multi_cloud.py @@ -91,6 +91,10 @@ def test_get_oauth2_authorization_uri_uses_workspace_host( "superset.db.session.get", return_value=_mock_database(mocker, host), ) + # is_safe_host does live DNS resolution; whether these fixture hosts + # happen to resolve depends on real-world DNS state outside test + # control, so pin it rather than relying on that. + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=True) state: OAuth2State = { "database_id": 1, diff --git a/tests/unit_tests/db_engine_specs/test_snowflake.py b/tests/unit_tests/db_engine_specs/test_snowflake.py index c7f14e9e337..e176113753c 100644 --- a/tests/unit_tests/db_engine_specs/test_snowflake.py +++ b/tests/unit_tests/db_engine_specs/test_snowflake.py @@ -542,6 +542,7 @@ def test_get_oauth2_token( """ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=True) requests: mock.MagicMock = mocker.patch("superset.db_engine_specs.base.requests") requests.post().json.return_value = { "access_token": "access-token", diff --git a/tests/unit_tests/db_engine_specs/test_trino.py b/tests/unit_tests/db_engine_specs/test_trino.py index 0d744d16ae1..5e22f9f626c 100644 --- a/tests/unit_tests/db_engine_specs/test_trino.py +++ b/tests/unit_tests/db_engine_specs/test_trino.py @@ -1109,6 +1109,7 @@ def test_get_oauth2_token( """ from superset.db_engine_specs.trino import TrinoEngineSpec + mocker.patch("superset.db_engine_specs.base.is_safe_host", return_value=True) requests = mocker.patch("superset.db_engine_specs.base.requests") requests.post().json.return_value = { "access_token": "access-token", diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index ebf73db4927..08b3ab7c880 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -3985,18 +3985,94 @@ def test_validate_guest_token_resources_rejects_non_embedded_int_id( def test_validate_guest_token_resources_accepts_embedded_int_id( app_context: None, mocker: MockerFixture ) -> None: - """A raw int id for an embedded dashboard is accepted.""" + """A raw int id for an embedded dashboard is accepted when the caller + minting the token is entitled to it.""" from superset.security.guest_token import GuestTokenResourceType sm = SupersetSecurityManager(appbuilder) embedded_dash = MagicMock() embedded_dash.embedded = [MagicMock()] # embedded mocker.patch("superset.models.dashboard.Dashboard.get", return_value=embedded_dash) + raise_for_access = mocker.patch.object(sm, "raise_for_access") sm.validate_guest_token_resources( [{"type": GuestTokenResourceType.DASHBOARD, "id": 5}] ) + raise_for_access.assert_called_once_with(dashboard=embedded_dash) + + +def test_validate_guest_token_resources_rejects_unauthorized_dashboard( + app_context: None, mocker: MockerFixture +) -> None: + """ + Minting a guest token for a dashboard the calling principal is not + themselves entitled to must be refused. Without this, a non-Admin role + granted only the coarse `can_grant_guest_token` permission (a realistic + "embedding backend service" grant, since SECURITY.md treats an + operator-narrowed permission as shifting the boundary, not redefining + the model) could mint a valid guest token scoped to *any* embedded + dashboard in the instance, not just ones it can see. + """ + from superset.commands.dashboard.embedded.exceptions import ( + EmbeddedDashboardAccessDeniedError, + ) + from superset.exceptions import SupersetSecurityException + from superset.security.guest_token import GuestTokenResourceType + + sm = SupersetSecurityManager(appbuilder) + embedded_dash = MagicMock() + embedded_dash.embedded = [MagicMock()] # embedded + mocker.patch("superset.models.dashboard.Dashboard.get", return_value=embedded_dash) + mocker.patch.object( + sm, + "raise_for_access", + side_effect=SupersetSecurityException(mocker.MagicMock()), + ) + + with pytest.raises(EmbeddedDashboardAccessDeniedError): + sm.validate_guest_token_resources( + [{"type": GuestTokenResourceType.DASHBOARD, "id": 5}] + ) + + +def test_validate_guest_token_resources_checks_access_via_embedded_dao_fallback( + app_context: None, mocker: MockerFixture +) -> None: + """ + The same access check applies on the EmbeddedDashboardDAO lookup path + (a resource id that isn't a plain dashboard id -- e.g. the embedded + config's own uuid), not just the `Dashboard.get` path. + """ + from superset.exceptions import SupersetSecurityException + from superset.security.guest_token import GuestTokenResourceType + + sm = SupersetSecurityManager(appbuilder) + mocker.patch("superset.models.dashboard.Dashboard.get", return_value=None) + target_dashboard = MagicMock() + embedded = MagicMock() + embedded.dashboard = target_dashboard + mocker.patch( + "superset.daos.dashboard.EmbeddedDashboardDAO.find_by_id", + return_value=embedded, + ) + raise_for_access = mocker.patch.object( + sm, + "raise_for_access", + side_effect=SupersetSecurityException(mocker.MagicMock()), + ) + + from superset.commands.dashboard.embedded.exceptions import ( + EmbeddedDashboardAccessDeniedError, + ) + + with pytest.raises(EmbeddedDashboardAccessDeniedError): + sm.validate_guest_token_resources( + [{"type": GuestTokenResourceType.DASHBOARD, "id": "some-uuid"}] + ) + + raise_for_access.assert_called_once_with(dashboard=target_dashboard) + def test_is_editor_query_owner(mocker: MockerFixture, app_context: None) -> None: """ diff --git a/tests/unit_tests/views/test_utils.py b/tests/unit_tests/views/test_utils.py index 805b760c66a..b1f5d65ebc8 100644 --- a/tests/unit_tests/views/test_utils.py +++ b/tests/unit_tests/views/test_utils.py @@ -110,7 +110,10 @@ def test_get_dashboard_extra_filters_includes_native_filter_defaults( db.session.add_all([chart, dashboard]) db.session.flush() - with patch("superset.charts.data.dashboard_filter_context._check_dashboard_access"): + with ( + patch("superset.charts.data.dashboard_filter_context._check_dashboard_access"), + patch("superset.views.utils.security_manager.raise_for_access"), + ): extra_filters = get_dashboard_extra_filters(chart.id, dashboard.id) assert extra_filters == [{"col": "region", "op": "IN", "val": ["APAC"]}] @@ -123,6 +126,7 @@ def test_get_dashboard_extra_filters_includes_native_filter_defaults( with ( patch("superset.charts.data.dashboard_filter_context._check_dashboard_access"), + patch("superset.views.utils.security_manager.raise_for_access"), patch( "superset.views.utils.build_extra_filters", return_value=[legacy_filter], @@ -134,3 +138,66 @@ def test_get_dashboard_extra_filters_includes_native_filter_defaults( legacy_filter, {"col": "region", "op": "IN", "val": ["APAC"]}, ] + + +def test_get_dashboard_extra_filters_denies_unauthorized_dashboard( + session: Session, +) -> None: + """ + A chart can legitimately be reused across multiple dashboards, so + chart-membership on a dashboard is not an entitlement check for that + dashboard. A caller who can access the chart but not the dashboard it's + also placed on must not have that dashboard's filter configuration + pulled into their request. + """ + Dashboard.metadata.create_all(session.get_bind()) + + dataset = SqlaTable( + table_name="unauthorized_dash_table", + database=Database( + database_name="unauthorized_dash_db", sqlalchemy_uri="sqlite://" + ), + ) + db.session.add(dataset) + db.session.flush() + + chart = Slice( + slice_name="shared_chart", + datasource_id=dataset.id, + datasource_type="table", + ) + dashboard = Dashboard( + dashboard_title="dashboard_caller_cant_access", + slices=[chart], + published=True, + json_metadata=json.dumps( + { + "default_filters": json.dumps( + {"legacy-filter": {"country": ["Brazil"]}} + ), + "filter_scopes": {}, + } + ), + position_json="{}", + ) + db.session.add_all([chart, dashboard]) + db.session.flush() + + from unittest.mock import MagicMock + + from superset.exceptions import SupersetSecurityException + + with ( + patch("superset.charts.data.dashboard_filter_context._check_dashboard_access"), + patch( + "superset.views.utils.security_manager.raise_for_access", + side_effect=SupersetSecurityException(MagicMock()), + ), + patch( + "superset.views.utils.build_extra_filters", + return_value=[{"col": "country", "op": "in", "val": ["Brazil"]}], + ), + ): + extra_filters = get_dashboard_extra_filters(chart.id, dashboard.id) + + assert extra_filters == []
