diff --git a/UPDATING.md b/UPDATING.md index 4fa80ddb816..5a41479ac9e 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,16 @@ assists people when migrating to a new version. ## Next +### SQL parser input length cap (SQL_MAX_PARSE_LENGTH) + +The SQL parser now rejects scripts whose UTF-8 byte length exceeds the new +`SQL_MAX_PARSE_LENGTH` config option (default `1_000_000` bytes) before they are +handed to sqlglot, which bounds parser memory and CPU usage. A single query +larger than the cap (for example a very large `IN (...)` list or a big +virtual-dataset SQL) raises a parse error in SQL Lab and dashboard-generated +queries. Deployments that legitimately run queries above this size should raise +the value, and `SQL_MAX_PARSE_LENGTH = None` disables the check entirely. + ### Guest-token RLS rules reject unknown fields The `rls` rules passed to `POST /api/v1/security/guest_token/` are now validated strictly: a rule may only contain `dataset` and `clause`. Previously unknown fields were silently dropped, so a mistyped or legacy scope key (most commonly `datasource` instead of `dataset`) produced a rule with no `dataset`, which is treated as a *global* rule applied to every dataset the embedded resource can reach. Such a request now returns HTTP 400 identifying the offending field instead of issuing a token with an unintended global rule. Integrators that were sending extra fields in RLS rules must remove them; valid dataset-scoped (`{"dataset": 41, "clause": "..."}`) and global (`{"clause": "..."}`) rules are unaffected. @@ -81,6 +91,7 @@ Deployments that intentionally point webhooks at internal targets (chatops bridg ### Impala cancel_query blocks private/internal hosts by default The Impala engine spec's `cancel_query` issues an HTTP request from the Superset backend to the host configured on the Impala database connection. That host is now validated before the request: if it resolves to a private/internal IP range, the cancel call is refused and a warning is logged. Operators whose Impala cluster runs on an internal network can opt out by setting `IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS = True` in `superset_config.py`. This mirrors the dataset-import and webhook opt-out flags. + ### Map chart renderer and OpenStreetMap migration behavior The MapLibre migration for deck.gl charts preserves saved non-Mapbox styles on diff --git a/superset/config.py b/superset/config.py index 0fad125166a..76f9485e2ef 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1501,6 +1501,13 @@ SQLLAB_SCHEDULE_WARNING_MESSAGE = None # Max payload size (MB) for SQL Lab to prevent browser hangs with large results. SQLLAB_PAYLOAD_MAX_MB = None +# Maximum UTF-8 byte length of a SQL script accepted by the SQL parser. +# Scripts longer than this are rejected before being handed to sqlglot, which +# bounds parser memory and CPU usage. The bound is in bytes (not Unicode +# code points) so multi-byte payloads cannot exceed the intended memory cap. +# Set to None to disable the check. +SQL_MAX_PARSE_LENGTH: int | None = 1_000_000 + # Force refresh while auto-refresh in dashboard DASHBOARD_AUTO_REFRESH_MODE: Literal["fetch", "force"] = "force" # Dashboard auto refresh intervals diff --git a/superset/sql/parse.py b/superset/sql/parse.py index 93fccc44078..8d1330403ea 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -27,6 +27,7 @@ from dataclasses import dataclass from typing import Any, Generic, Optional, TYPE_CHECKING, TypeVar import sqlglot +from flask import current_app, has_app_context from jinja2 import nodes, Template from sqlglot import exp from sqlglot.dialects.dialect import ( @@ -56,6 +57,44 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) +def _check_script_length(script: str, engine: str | None) -> None: + """ + Reject scripts whose UTF-8 byte length exceeds the configured maximum + before they reach sqlglot. Sits at every code path in this module that + hands a string to ``sqlglot.parse`` or ``sqlglot.parse_one`` so the + bound cannot be bypassed by a direct caller. + + The check is in bytes, not Unicode code points, because the + threat model is parser memory and CPU on the encoded payload that + sqlglot ingests. + """ + # Imported lazily to avoid a circular import (``superset.config`` pulls in + # ``superset.jinja_context``, which imports this module). + from superset import config + + # The live app config wins when a Flask app context is active (honoring any + # operator override); otherwise (Alembic migrations, scripts, isolated unit + # tests) fall back to the documented default declared in ``superset.config`` + # so the bound stays sourced from configuration rather than duplicated here. + max_length = ( + current_app.config.get("SQL_MAX_PARSE_LENGTH", config.SQL_MAX_PARSE_LENGTH) + if has_app_context() + else config.SQL_MAX_PARSE_LENGTH + ) + + if max_length is None: + return + if (byte_length := len(script.encode("utf-8"))) > max_length: + raise SupersetParseError( + script, + engine, + message=( + f"SQL script length ({byte_length} bytes) exceeds the " + f"configured maximum of {max_length} bytes." + ), + ) + + # mapping between DB engine specs and sqlglot dialects SQLGLOT_DIALECTS = { "base": Dialects.DIALECT, @@ -717,6 +756,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): supports backticks natively. This handles cases like "Other" database type where users may have MySQL-compatible syntax with backtick-quoted table names. """ + _check_script_length(script, engine) dialect = SQLGLOT_DIALECTS.get(engine) try: statements = sqlglot.parse(script, dialect=dialect) @@ -1150,6 +1190,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): :param predicate: The predicate to parse. :return: The parsed predicate. """ + _check_script_length(predicate, self.engine) return sqlglot.parse_one(predicate, dialect=self._dialect) def apply_rls( @@ -1698,9 +1739,11 @@ def extract_tables_from_statement( if not literal: return set() + pseudo_sql = f"SELECT {literal.this}" try: - pseudo_query = sqlglot.parse_one(f"SELECT {literal.this}", dialect=dialect) - except ParseError: + _check_script_length(pseudo_sql, None) + pseudo_query = sqlglot.parse_one(pseudo_sql, dialect=dialect) + except (ParseError, SupersetParseError): return set() sources = pseudo_query.find_all(exp.Table) else: @@ -1889,6 +1932,7 @@ def transpile_to_dialect( source_dialect = SQLGLOT_DIALECTS.get(source_engine) if source_engine else Dialect try: + _check_script_length(sql, source_engine) parsed = sqlglot.parse_one(sql, dialect=source_dialect) return Dialect.get_or_raise(target_dialect).generate( parsed, diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index e6a4e45aa1d..55a530266be 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -20,12 +20,14 @@ import logging import pytest +import sqlglot from pytest_mock import MockerFixture from sqlglot import Dialects, exp, parse_one from superset.exceptions import QueryClauseValidationException, SupersetParseError from superset.jinja_context import JinjaTemplateProcessor from superset.sql.parse import ( + _check_script_length, CTASMethod, extract_tables_from_statement, JinjaSQLResult, @@ -43,6 +45,7 @@ from superset.sql.parse import ( SQLStatement, Table, tokenize_kql, + transpile_to_dialect, ) from tests.integration_tests.conftest import with_feature_flags @@ -3698,6 +3701,113 @@ def test_backtick_invalid_sql_still_fails() -> None: SQLScript(sql, "base") +# --------------------------------------------------------------------------- +# SQL_MAX_PARSE_LENGTH gate +# --------------------------------------------------------------------------- + + +@pytest.fixture +def _small_parse_cap(mocker: MockerFixture) -> None: + """ + Pin the parse-length cap to 100 bytes and force the no-app-context + fallback path so tests are decoupled from the suite's Flask config. + """ + mocker.patch("superset.config.SQL_MAX_PARSE_LENGTH", 100) + mocker.patch("superset.sql.parse.has_app_context", return_value=False) + + +@pytest.mark.usefixtures("_small_parse_cap") +def test_check_script_length_accepts_at_boundary() -> None: + """A script exactly at the configured cap is accepted.""" + _check_script_length("a" * 100, "postgresql") + + +@pytest.mark.usefixtures("_small_parse_cap") +def test_check_script_length_rejects_one_over() -> None: + """One byte above the cap is rejected before sqlglot runs.""" + with pytest.raises(SupersetParseError) as excinfo: + _check_script_length("a" * 101, "postgresql") + assert "exceeds the configured maximum" in str(excinfo.value) + + +def test_check_script_length_counts_utf8_bytes(mocker: MockerFixture) -> None: + """ + The cap is in UTF-8 bytes, not code points. A multi-byte char string + whose char-count is under the cap but byte-count is over must reject. + """ + mocker.patch("superset.config.SQL_MAX_PARSE_LENGTH", 30) + mocker.patch("superset.sql.parse.has_app_context", return_value=False) + # 20 emoji = 20 code points (under the 30-byte cap) but 80 UTF-8 bytes (over) + payload = "\U0001f600" * 20 + with pytest.raises(SupersetParseError): + _check_script_length(payload, "postgresql") + + +def test_check_script_length_disabled_when_config_none( + mocker: MockerFixture, +) -> None: + """Setting SQL_MAX_PARSE_LENGTH=None disables the check entirely.""" + fake_app = mocker.MagicMock() + fake_app.config = {"SQL_MAX_PARSE_LENGTH": None} + mocker.patch("superset.sql.parse.has_app_context", return_value=True) + mocker.patch("superset.sql.parse.current_app", fake_app) + _check_script_length("a" * 10_000_000, "postgresql") + + +def test_check_script_length_uses_app_config_when_present( + mocker: MockerFixture, +) -> None: + """When an app context is active, the runtime config value wins.""" + fake_app = mocker.MagicMock() + fake_app.config = {"SQL_MAX_PARSE_LENGTH": 50} + mocker.patch("superset.sql.parse.has_app_context", return_value=True) + mocker.patch("superset.sql.parse.current_app", fake_app) + with pytest.raises(SupersetParseError): + _check_script_length("a" * 51, "postgresql") + + +@pytest.mark.usefixtures("_small_parse_cap") +def test_sqlscript_gate_short_circuits_before_sqlglot( + mocker: MockerFixture, +) -> None: + """ + SQLScript construction must reject an over-cap script before any call + to sqlglot.parse, including the MySQL-backtick fallback path. Captures + the original behaviour the PR is closing: the previous code parsed + twice on backtick failures, so the cap MUST short-circuit both. + """ + spy = mocker.spy(sqlglot, "parse") + over_cap_with_backtick = "SELECT * FROM `t` -- " + "x" * 200 + with pytest.raises(SupersetParseError): + SQLScript(over_cap_with_backtick, "base") + assert spy.call_count == 0, "length gate failed to short-circuit sqlglot.parse" + + +@pytest.mark.usefixtures("_small_parse_cap") +def test_parse_predicate_length_check() -> None: + """SQLStatement.parse_predicate also goes through the length gate.""" + stmt = SQLStatement("SELECT 1", "postgresql") + with pytest.raises(SupersetParseError): + stmt.parse_predicate("x" * 101) + + +@pytest.mark.usefixtures("_small_parse_cap") +def test_transpile_to_dialect_length_check() -> None: + """ + The standalone ``transpile_to_dialect`` entry point also gates input. + + The cap-exceeded error surfaces as ``QueryClauseValidationException`` to + preserve the function's existing error contract (callers such as + ``transpile_virtual_dataset_sql`` only catch that type and fall back to + the original SQL). The underlying ``SupersetParseError`` is attached as + ``__cause__`` so over-cap input is still distinguishable from a generic + parse failure. + """ + with pytest.raises(QueryClauseValidationException) as excinfo: + transpile_to_dialect("x" * 101, target_engine="mysql") + assert isinstance(excinfo.value.__cause__, SupersetParseError) + + def test_backtick_fallback_logs_warning(caplog: pytest.LogCaptureFixture) -> None: """ Test that the MySQL dialect fallback emits a warning log.