You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2024/06/26 13:51:20 UTC
(superset) branch 4.0 updated (d12a5d0446 -> ff2907fa46)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a change to branch 4.0
in repository https://gitbox.apache.org/repos/asf/superset.git
discard d12a5d0446 fix: adds the ability to disallow SQL functions per engine (#28639)
new ff2907fa46 fix: adds the ability to disallow SQL functions per engine (#28639)
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (d12a5d0446)
\
N -- N -- N refs/heads/4.0 (ff2907fa46)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
superset/config.py | 1 +
superset/db_engine_specs/trino.py | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
(superset) 01/01: fix: adds the ability to disallow SQL functions per engine (#28639)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 4.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ff2907fa46b2e10d07d0633974fba2ab485583e6
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Wed May 29 10:51:28 2024 +0100
fix: adds the ability to disallow SQL functions per engine (#28639)
---
.../Header/HeaderActionsDropdown/index.jsx | 2 +-
superset/config.py | 9 +++++
superset/db_engine_specs/base.py | 6 ++++
superset/db_engine_specs/trino.py | 16 ++++++---
superset/exceptions.py | 15 ++++++++
superset/sql_parse.py | 42 ++++++++++++++++++++++
tests/unit_tests/db_engine_specs/test_trino.py | 24 +++++++------
tests/unit_tests/sql_parse_tests.py | 26 ++++++++++++++
8 files changed, 125 insertions(+), 15 deletions(-)
diff --git a/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx b/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx
index 6cdd6e4778..1d201f53e2 100644
--- a/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx
+++ b/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React from 'react';
+import React, { PureComponent } from 'react';
import PropTypes from 'prop-types';
import { isEmpty } from 'lodash';
import { connect } from 'react-redux';
diff --git a/superset/config.py b/superset/config.py
index 197e4bac42..859915c3b5 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1203,6 +1203,15 @@ DASHBOARD_TEMPLATE_ID = None
# as such `create_engine(url, **params)`
DB_CONNECTION_MUTATOR = None
+# A set of disallowed SQL functions per engine. This is used to restrict the use of
+# unsafe SQL functions in SQL Lab and Charts. The keys of the dictionary are the engine
+# names, and the values are sets of disallowed functions.
+DISALLOWED_SQL_FUNCTIONS: dict[str, set[str]] = {
+ "postgresql": {"version", "query_to_xml", "inet_server_addr", "inet_client_addr"},
+ "clickhouse": {"url"},
+ "mysql": {"version"},
+}
+
# A function that intercepts the SQL to be executed and can alter it.
# The use case is can be around adding some sort of comment header
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index 5b850913ed..62704268db 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -59,6 +59,7 @@ from superset import security_manager, sql_parse
from superset.constants import TimeGrain as TimeGrainConstants
from superset.databases.utils import make_url_safe
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import DisallowedSQLFunction
from superset.sql_parse import ParsedQuery, Table
from superset.superset_typing import ResultSetColumnType, SQLAColumnType
from superset.utils import core as utils
@@ -1584,6 +1585,11 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
"""
if not cls.allows_sql_comments:
query = sql_parse.strip_comments_from_sql(query, engine=cls.engine)
+ disallowed_functions = current_app.config["DISALLOWED_SQL_FUNCTIONS"].get(
+ cls.engine, set()
+ )
+ if sql_parse.check_sql_functions_exist(query, disallowed_functions, cls.engine):
+ raise DisallowedSQLFunction(disallowed_functions)
if cls.arraysize:
cursor.arraysize = cls.arraysize
diff --git a/superset/db_engine_specs/trino.py b/superset/db_engine_specs/trino.py
index 6d95f9589e..1e902e7f30 100644
--- a/superset/db_engine_specs/trino.py
+++ b/superset/db_engine_specs/trino.py
@@ -23,7 +23,7 @@ import time
from typing import Any, TYPE_CHECKING
import simplejson as json
-from flask import current_app
+from flask import current_app, Flask
from sqlalchemy.engine.reflection import Inspector
from sqlalchemy.engine.url import URL
from sqlalchemy.exc import NoSuchTableError
@@ -206,11 +206,14 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
execute_result: dict[str, Any] = {}
execute_event = threading.Event()
- def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ def _execute(
+ results: dict[str, Any], event: threading.Event, app: Flask
+ ) -> None:
logger.debug("Query %d: Running query: %s", query_id, sql)
try:
- cls.execute(cursor, sql)
+ with app.app_context():
+ cls.execute(cursor, sql)
except Exception as ex: # pylint: disable=broad-except
results["error"] = ex
finally:
@@ -218,7 +221,12 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
execute_thread = threading.Thread(
target=_execute,
- args=(execute_result, execute_event),
+ args=(
+ execute_result,
+ execute_event,
+ # pylint: disable=protected-access
+ current_app._get_current_object(),
+ ),
)
execute_thread.start()
diff --git a/superset/exceptions.py b/superset/exceptions.py
index 3642a9279e..a7e811a088 100644
--- a/superset/exceptions.py
+++ b/superset/exceptions.py
@@ -295,3 +295,18 @@ class SupersetMarshmallowValidationError(SupersetErrorException):
extra={"messages": exc.messages, "payload": payload},
)
super().__init__(error)
+
+
+class DisallowedSQLFunction(SupersetErrorException):
+ """
+ Disallowed function found on SQL statement
+ """
+
+ def __init__(self, functions: set[str]):
+ super().__init__(
+ SupersetError(
+ message=f"SQL statement contains disallowed function(s): {functions}",
+ error_type=SupersetErrorType.SYNTAX_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
diff --git a/superset/sql_parse.py b/superset/sql_parse.py
index 657fde1982..6aa435240d 100644
--- a/superset/sql_parse.py
+++ b/superset/sql_parse.py
@@ -36,6 +36,7 @@ from sqlglot.optimizer.scope import Scope, ScopeType, traverse_scope
from sqlparse import keywords
from sqlparse.lexer import Lexer
from sqlparse.sql import (
+ Function,
Identifier,
IdentifierList,
Parenthesis,
@@ -219,6 +220,19 @@ def get_cte_remainder_query(sql: str) -> tuple[str | None, str]:
return cte, remainder
+def check_sql_functions_exist(
+ sql: str, function_list: set[str], engine: str | None = None
+) -> bool:
+ """
+ Check if the SQL statement contains any of the specified functions.
+
+ :param sql: The SQL statement
+ :param function_list: The list of functions to search for
+ :param engine: The engine to use for parsing the SQL statement
+ """
+ return ParsedQuery(sql, engine=engine).check_functions_exist(function_list)
+
+
def strip_comments_from_sql(statement: str, engine: str | None = None) -> str:
"""
Strips comments from a SQL statement, does a simple test first
@@ -288,6 +302,34 @@ class ParsedQuery:
self._tables = self._extract_tables_from_sql()
return self._tables
+ def _check_functions_exist_in_token(
+ self, token: Token, functions: set[str]
+ ) -> bool:
+ if (
+ isinstance(token, Function)
+ and token.get_name() is not None
+ and token.get_name().lower() in functions
+ ):
+ return True
+ if hasattr(token, "tokens"):
+ for inner_token in token.tokens:
+ if self._check_functions_exist_in_token(inner_token, functions):
+ return True
+ return False
+
+ def check_functions_exist(self, functions: set[str]) -> bool:
+ """
+ Check if the SQL statement contains any of the specified functions.
+
+ :param functions: A set of functions to search for
+ :return: True if the statement contains any of the specified functions
+ """
+ for statement in self._parsed:
+ for token in statement.tokens:
+ if self._check_functions_exist_in_token(token, functions):
+ return True
+ return False
+
def _extract_tables_from_sql(self) -> set[Table]:
"""
Extract all table references in a query.
diff --git a/tests/unit_tests/db_engine_specs/test_trino.py b/tests/unit_tests/db_engine_specs/test_trino.py
index 22c5f10649..7f175e8fa5 100644
--- a/tests/unit_tests/db_engine_specs/test_trino.py
+++ b/tests/unit_tests/db_engine_specs/test_trino.py
@@ -396,7 +396,7 @@ def test_handle_cursor_early_cancel(
assert cancel_query_mock.call_args is None
-def test_execute_with_cursor_in_parallel(mocker: MockerFixture):
+def test_execute_with_cursor_in_parallel(app, mocker: MockerFixture):
"""Test that `execute_with_cursor` fetches query ID from the cursor"""
from superset.db_engine_specs.trino import TrinoEngineSpec
@@ -411,16 +411,20 @@ def test_execute_with_cursor_in_parallel(mocker: MockerFixture):
mock_cursor.query_id = query_id
mock_cursor.execute.side_effect = _mock_execute
+ with patch.dict(
+ "superset.config.DISALLOWED_SQL_FUNCTIONS",
+ {},
+ clear=True,
+ ):
+ TrinoEngineSpec.execute_with_cursor(
+ cursor=mock_cursor,
+ sql="SELECT 1 FROM foo",
+ query=mock_query,
+ )
- TrinoEngineSpec.execute_with_cursor(
- cursor=mock_cursor,
- sql="SELECT 1 FROM foo",
- query=mock_query,
- )
-
- mock_query.set_extra_json_key.assert_called_once_with(
- key=QUERY_CANCEL_KEY, value=query_id
- )
+ mock_query.set_extra_json_key.assert_called_once_with(
+ key=QUERY_CANCEL_KEY, value=query_id
+ )
def test_get_columns(mocker: MockerFixture):
diff --git a/tests/unit_tests/sql_parse_tests.py b/tests/unit_tests/sql_parse_tests.py
index 5150c37987..8abb569495 100644
--- a/tests/unit_tests/sql_parse_tests.py
+++ b/tests/unit_tests/sql_parse_tests.py
@@ -32,6 +32,7 @@ from superset.exceptions import (
)
from superset.sql_parse import (
add_table_name,
+ check_sql_functions_exist,
extract_table_references,
extract_tables_from_jinja_sql,
get_rls_for_table,
@@ -1292,6 +1293,31 @@ def test_strip_comments_from_sql() -> None:
)
+def test_check_sql_functions_exist() -> None:
+ """
+ Test that comments are stripped out correctly.
+ """
+ assert not (
+ check_sql_functions_exist("select a, b from version", {"version"}, "postgresql")
+ )
+
+ assert check_sql_functions_exist("select version()", {"version"}, "postgresql")
+
+ assert check_sql_functions_exist(
+ "select version from version()", {"version"}, "postgresql"
+ )
+
+ assert check_sql_functions_exist(
+ "select 1, a.version from (select version from version()) as a",
+ {"version"},
+ "postgresql",
+ )
+
+ assert check_sql_functions_exist(
+ "select 1, a.version from (select version()) as a", {"version"}, "postgresql"
+ )
+
+
def test_sanitize_clause_valid():
# regular clauses
assert sanitize_clause("col = 1") == "col = 1"