diff --git a/.github/workflows/superset-python-integrationtest.yml b/.github/workflows/superset-python-integrationtest.yml index 5b69fbc46d4..dee0165ab05 100644 --- a/.github/workflows/superset-python-integrationtest.yml +++ b/.github/workflows/superset-python-integrationtest.yml @@ -49,9 +49,6 @@ jobs: image: mysql:8.0 # Authenticated pulls use our higher Docker Hub rate limit. Empty on # fork PRs (secrets unavailable) -> runner falls back to anonymous. - credentials: - username: ${{ secrets.DOCKERHUB_USER }} - password: ${{ secrets.DOCKERHUB_TOKEN }} env: MYSQL_ROOT_PASSWORD: root ports: @@ -63,9 +60,6 @@ jobs: --health-retries=5 redis: image: redis:7-alpine - credentials: - username: ${{ secrets.DOCKERHUB_USER }} - password: ${{ secrets.DOCKERHUB_TOKEN }} options: --entrypoint redis-server ports: - 16379:6379 @@ -140,9 +134,6 @@ jobs: services: postgres: image: postgres:17-alpine - credentials: - username: ${{ secrets.DOCKERHUB_USER }} - password: ${{ secrets.DOCKERHUB_TOKEN }} env: POSTGRES_USER: superset POSTGRES_PASSWORD: superset @@ -152,9 +143,6 @@ jobs: - 15432:5432 redis: image: redis:7-alpine - credentials: - username: ${{ secrets.DOCKERHUB_USER }} - password: ${{ secrets.DOCKERHUB_TOKEN }} ports: - 16379:6379 steps: @@ -204,9 +192,6 @@ jobs: services: redis: image: redis:7-alpine - credentials: - username: ${{ secrets.DOCKERHUB_USER }} - password: ${{ secrets.DOCKERHUB_TOKEN }} ports: - 16379:6379 steps: diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 26bc15ecca8..c82e2713552 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -1449,6 +1449,36 @@ class ExploreMixin: # pylint: disable=too-many-public-methods if is_alias_used_in_orderby(col): col.name = f"{col.name}__" + def _raise_for_disallowed_sql(self, sql: str) -> None: + """ + Mirror the DISALLOWED_SQL_* gate that sql_lab.execute_sql_statement + enforces so both query surfaces honour the same denylist. + """ + engine = self.db_engine_spec.engine + disallowed_functions = app.config["DISALLOWED_SQL_FUNCTIONS"].get(engine, set()) + disallowed_tables = app.config["DISALLOWED_SQL_TABLES"].get(engine, set()) + if not (disallowed_functions or disallowed_tables): + return + + parsed_script = SQLScript(sql, engine=engine) + if disallowed_functions and parsed_script.check_functions_present( + disallowed_functions + ): + raise SupersetDisallowedSQLFunctionException(disallowed_functions) + if disallowed_tables and parsed_script.check_tables_present(disallowed_tables): + # Report only the tables actually found in the query, mirroring the + # canonical execution-time gate so the user-facing error doesn't + # echo the operator's full denylist. + present_tables = { + table.table.lower() + for statement in parsed_script.statements + for table in statement.tables + } + found_tables = { + table for table in disallowed_tables if table.lower() in present_tables + } + raise SupersetDisallowedSQLTableException(found_tables or disallowed_tables) + def query(self, query_obj: QueryObjectDict) -> QueryResult: """ Executes the query and returns a dataframe. @@ -1459,6 +1489,9 @@ class ExploreMixin: # pylint: disable=too-many-public-methods qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) sql = query_str_ext.sql + + self._raise_for_disallowed_sql(sql) + status = QueryStatus.SUCCESS errors = None error_message = None diff --git a/tests/unit_tests/connectors/sqla/models_test.py b/tests/unit_tests/connectors/sqla/models_test.py index eb621431f79..0876588a55a 100644 --- a/tests/unit_tests/connectors/sqla/models_test.py +++ b/tests/unit_tests/connectors/sqla/models_test.py @@ -31,6 +31,8 @@ from superset.daos.dataset import DatasetDAO from superset.daos.exceptions import DatasourceNotFound from superset.exceptions import ( OAuth2RedirectError, + SupersetDisallowedSQLFunctionException, + SupersetDisallowedSQLTableException, SupersetSecurityException, ) from superset.models.core import Database @@ -81,6 +83,116 @@ def test_query_bubbles_errors(mocker: MockerFixture) -> None: sqla_table.query(query_obj) +def _query_obj() -> QueryObjectDict: + return { + "granularity": None, + "from_dttm": None, + "to_dttm": None, + "groupby": ["id"], + "metrics": [], + "is_timeseries": False, + "filter": [], + } + + +def _build_sqla_table_for_query( + mocker: MockerFixture, sql: str, engine: str = "postgresql" +) -> SqlaTable: + db_engine_spec = mocker.MagicMock() + db_engine_spec.engine = engine + database = mocker.MagicMock() + database.db_engine_spec = db_engine_spec + sqla_table = SqlaTable( + table_name="my_sqla_table", + columns=[], + metrics=[], + database=database, + ) + mocker.patch.object( + SqlaTable, + "db_engine_spec", + new=property(lambda self: db_engine_spec), + ) + mocker.patch.object( + sqla_table, + "get_query_str_extended", + return_value=mocker.MagicMock(sql=sql, labels_expected=[]), + ) + return sqla_table + + +def test_query_blocks_disallowed_function_on_chart_data_path( + mocker: MockerFixture, +) -> None: + mocker.patch.dict( + "flask.current_app.config", + { + "DISALLOWED_SQL_FUNCTIONS": {"postgresql": {"version"}}, + "DISALLOWED_SQL_TABLES": {}, + }, + clear=False, + ) + sqla_table = _build_sqla_table_for_query(mocker, "SELECT version()") + with pytest.raises(SupersetDisallowedSQLFunctionException): + sqla_table.query(_query_obj()) + sqla_table.database.get_df.assert_not_called() # type: ignore[attr-defined] + + +def test_query_blocks_disallowed_table_on_chart_data_path( + mocker: MockerFixture, +) -> None: + mocker.patch.dict( + "flask.current_app.config", + { + "DISALLOWED_SQL_FUNCTIONS": {}, + "DISALLOWED_SQL_TABLES": {"postgresql": {"pg_authid"}}, + }, + clear=False, + ) + sqla_table = _build_sqla_table_for_query(mocker, "SELECT rolname FROM pg_authid") + with pytest.raises(SupersetDisallowedSQLTableException): + sqla_table.query(_query_obj()) + sqla_table.database.get_df.assert_not_called() # type: ignore[attr-defined] + + +def test_query_disallowed_table_error_reports_only_matched_tables( + mocker: MockerFixture, +) -> None: + mocker.patch.dict( + "flask.current_app.config", + { + "DISALLOWED_SQL_FUNCTIONS": {}, + "DISALLOWED_SQL_TABLES": { + "postgresql": {"pg_authid", "pg_shadow", "pg_stat_activity"} + }, + }, + clear=False, + ) + sqla_table = _build_sqla_table_for_query(mocker, "SELECT rolname FROM pg_authid") + with pytest.raises(SupersetDisallowedSQLTableException) as excinfo: + sqla_table.query(_query_obj()) + message = str(excinfo.value) + assert "pg_authid" in message + assert "pg_shadow" not in message + assert "pg_stat_activity" not in message + + +def test_query_allows_benign_sql_on_chart_data_path(mocker: MockerFixture) -> None: + mocker.patch.dict( + "flask.current_app.config", + { + "DISALLOWED_SQL_FUNCTIONS": {"postgresql": {"version"}}, + "DISALLOWED_SQL_TABLES": {"postgresql": {"pg_authid"}}, + }, + clear=False, + ) + sqla_table = _build_sqla_table_for_query(mocker, "SELECT id FROM my_sqla_table") + sqla_table.database.get_df.return_value = pd.DataFrame() # type: ignore[attr-defined] + result = sqla_table.query(_query_obj()) + sqla_table.database.get_df.assert_called_once() # type: ignore[attr-defined] + assert result is not None + + def test_permissions_without_catalog() -> None: """ Test permissions when the table has no catalog.