You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/06/16 22:07:41 UTC
(superset) branch master updated: fix(sql): broaden mutating-statement detection in SQL Lab parser (#40421)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 6a1091d5761 fix(sql): broaden mutating-statement detection in SQL Lab parser (#40421)
6a1091d5761 is described below
commit 6a1091d5761a19fe873f29c193b7aa4b8190600f
Author: Shaitan <10...@users.noreply.github.com>
AuthorDate: Tue Jun 16 23:07:34 2026 +0100
fix(sql): broaden mutating-statement detection in SQL Lab parser (#40421)
Co-authored-by: Claude Opus 4.7 <no...@anthropic.com>
Co-authored-by: sha174n <pe...@preset.io>
---
superset/models/helpers.py | 2 +-
superset/sql/parse.py | 141 ++++++++++++-
.../pandas_postprocessing/test_resample.py | 2 +-
tests/unit_tests/sql/parse_tests.py | 226 +++++++++++++++++++++
4 files changed, 362 insertions(+), 9 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 3ebc3f6dd72..2a308544333 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -1947,7 +1947,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
if offset_metrics_df.empty:
offset_metrics_df = pd.DataFrame(
{
- col: [np.nan]
+ col: [np.NaN]
for col in join_keys + list(metrics_mapping.values())
}
)
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index 6a94a414d9d..ec46b5850dc 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -592,6 +592,84 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
This class is used for all engines with dialects that can be parsed using sqlglot.
"""
+ # Function names that mutate server-side state but appear in the AST as
+ # plain function calls inside a non-mutating wrapper. Used by
+ # ``is_mutating()`` to classify e.g. PostgreSQL large-object writers.
+ # Names are uppercased for comparison.
+ _MUTATING_FUNCTION_NAMES: frozenset[str] = frozenset(
+ {
+ "LO_FROM_BYTEA",
+ "LO_EXPORT",
+ "LO_IMPORT",
+ "LO_PUT",
+ "LO_CREATE",
+ "LOWRITE",
+ "LO_UNLINK",
+ # PostgreSQL sequence mutators. `SELECT setval('seq', N)` and
+ # `SELECT nextval('seq')` look like reads but change sequence state
+ # for every subsequent caller. (`currval` only reads the session's
+ # last value, so it is intentionally not listed.)
+ "SETVAL",
+ "NEXTVAL",
+ }
+ )
+
+ # PostgreSQL constructs that sqlglot represents as an opaque ``exp.Command``
+ # (no structured AST). Each can mutate server state or wrap a DML body that
+ # would otherwise be detected by node-type matching. Used by
+ # ``is_mutating()``.
+ _POSTGRES_MUTATING_COMMAND_NAMES: frozenset[str] = frozenset(
+ {
+ "DO", # PL/pgSQL anonymous block
+ "PREPARE", # PREPARE u AS UPDATE ... ; EXECUTE u
+ "EXECUTE", # body is the prepared DML
+ "CALL", # procedure body may mutate
+ "COPY", # server-side file ingest into a table
+ "GRANT",
+ "REVOKE",
+ # Only the command-fallback forms (e.g. SET ROLE / SET SESSION
+ # AUTHORIZATION, which change the effective user) reach here as an
+ # exp.Command. Structured `SET search_path = ...` /
+ # `SET statement_timeout = ...` parse as exp.Set and are NOT matched
+ # by this command-name path.
+ "SET",
+ "RESET", # RESET ROLE / RESET ALL reverts SET; same class as SET
+ "REFRESH", # REFRESH MATERIALIZED VIEW
+ "REINDEX",
+ "VACUUM",
+ # DDL head-tokens that sqlglot falls back to exp.Command for
+ # whenever the body uses syntax it does not model
+ # (CREATE EXTENSION/FUNCTION...LANGUAGE C/PUBLICATION/etc.,
+ # ALTER ROLE/SYSTEM/..., DROP EXTENSION/RULE/...). Well-formed
+ # CREATE TABLE/ALTER TABLE/DROP TABLE are already caught by the
+ # node-type tuple; these entries close the fallback path.
+ "CREATE",
+ "ALTER",
+ "DROP",
+ "LOAD", # LOAD '/path/lib.so' dlopens a shared library on the PG host
+ # NOTE: `SHOW` is intentionally NOT included. It is a read (mutates
+ # nothing), so classifying it as mutating would be wrong for every
+ # is_mutating()/has_mutation() consumer (the commit decision, the
+ # "only SELECT allowed" validators, limit handling), not just the
+ # read-only gate. Gating information-disclosure reads such as
+ # `SHOW server_version` belongs in a denylist (DISALLOWED_SQL_FUNCTIONS
+ # already blocks version()/pg_read_file), not in the mutation check.
+ }
+ )
+
+ # Dialects where `SELECT ... INTO target` is CTAS (creates a table, and so
+ # mutates schema). Elsewhere the same syntax assigns into a variable and is
+ # a read: Oracle PL/SQL `SELECT ... INTO v` and MySQL `SELECT ... INTO @v`
+ # parse into an identical `exp.Select` with an `into` arg, so the dialect is
+ # the only signal that distinguishes the mutating form from the read form.
+ _SELECT_INTO_CTAS_DIALECTS: frozenset[Dialects] = frozenset(
+ {
+ Dialects.POSTGRES,
+ Dialects.REDSHIFT,
+ Dialects.TSQL,
+ }
+ )
+
def __init__(
self,
statement: str | None = None,
@@ -725,25 +803,67 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
exp.Drop,
exp.TruncateTable,
exp.Alter,
+ # sqlglot has structured nodes for these DML/DCL forms in
+ # PostgreSQL and other dialects; without them an opaque exp.Command
+ # check would still miss the structured-parse path.
+ exp.Copy, # COPY <table> FROM/TO (server-side file ingest)
+ exp.Grant,
+ exp.Revoke,
+ # COMMENT ON TABLE/COLUMN/etc. writes to system catalog pg_description.
+ exp.Comment,
)
- for node_type in mutating_nodes:
- if self._parsed.find(node_type):
- return True
+ if self._parsed.find(*mutating_nodes):
+ return True
+
+ # `SELECT ... INTO new_table FROM ...` parses as `exp.Select` with an
+ # `into` arg (Postgres-style CTAS variant). It creates a new table and
+ # therefore mutates schema. Only treat it as mutating for dialects where
+ # the syntax is CTAS; elsewhere it assigns into a variable (a read).
+ if (
+ self._dialect in self._SELECT_INTO_CTAS_DIALECTS
+ and isinstance(self._parsed, exp.Select)
+ and self._parsed.args.get("into")
+ ):
+ return True
+
+ # Function calls that mutate server-side state without an enclosing
+ # mutating AST node. Notable example: PostgreSQL large-object writers
+ # (`lo_export` writes to the server filesystem, `lo_from_bytea`/
+ # `lo_create`/`lo_put`/`lo_import`/`lowrite` mutate the pg_largeobject
+ # catalog). These appear as plain function calls inside an `exp.Select`
+ # and would otherwise pass the read-only gate. Every name in
+ # _MUTATING_FUNCTION_NAMES is PostgreSQL-specific, so the walk is gated
+ # on the dialect: other engines may expose read-only functions/UDFs with
+ # the same names, and flagging those would wrongly block read-only
+ # queries. Each parses as an `exp.Anonymous`, whose `.name` is the bare
+ # function identifier. The walk is restricted to `exp.Anonymous` rather
+ # than the broader `exp.Func`, because for built-in function nodes (e.g.
+ # `exp.Upper`) `.name` returns the first argument's text, not the
+ # function name, so `SELECT upper('lo_export')` would otherwise be
+ # misclassified as mutating.
+ if self._dialect == Dialects.POSTGRES and any(
+ function.name.upper() in self._MUTATING_FUNCTION_NAMES
+ for function in self._parsed.find_all(exp.Anonymous)
+ ):
+ return True
# depending on the dialect (Oracle, MS SQL) the `ALTER` is parsed as a
# command, not an expression - check at root level
if isinstance(self._parsed, exp.Command) and self._parsed.name == "ALTER":
return True # pragma: no cover
+ # PostgreSQL constructs that sqlglot represents as an opaque
+ # `exp.Command` rather than a structured AST. Each of these can mutate
+ # state or wrap a DML body that would otherwise be detected. The
+ # `.name` attribute on `exp.Command` preserves the source-case of the
+ # head keyword (so `create extension ...` would yield `'create'`),
+ # which means the set lookup must be case-insensitive.
if (
self._dialect == Dialects.POSTGRES
and isinstance(self._parsed, exp.Command)
- and self._parsed.name == "DO"
+ and self._parsed.name.upper() in self._POSTGRES_MUTATING_COMMAND_NAMES
):
- # anonymous blocks can be written in many different languages (the default
- # is PL/pgSQL), so parsing them it out of scope of this class; we just
- # assume the anonymous block is mutating
return True
# Postgres runs DMLs prefixed by `EXPLAIN ANALYZE`, see
@@ -864,6 +984,13 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
else:
present.add(function.name.upper())
+ # MySQL `@@<name>` syntax (also Oracle/SQL-Server `@@name`) parses as
+ # `exp.SessionParameter`, which is *not* a subclass of `exp.Func`, so
+ # the walk above misses it. Include those names so denylist entries
+ # like `version` or `hostname` match `SELECT @@version`.
+ for param in self._parsed.find_all(exp.SessionParameter):
+ present.add(param.name.upper())
+
return any(function.upper() in present for function in functions)
def check_tables_present(self, tables: set[str]) -> bool:
diff --git a/tests/unit_tests/pandas_postprocessing/test_resample.py b/tests/unit_tests/pandas_postprocessing/test_resample.py
index 7c742de8f36..207863ab879 100644
--- a/tests/unit_tests/pandas_postprocessing/test_resample.py
+++ b/tests/unit_tests/pandas_postprocessing/test_resample.py
@@ -253,7 +253,7 @@ def test_resample_linear():
]
),
data={
- "label": ["a", np.nan, np.nan, np.nan, "e", np.nan, np.nan, "j"],
+ "label": ["a", np.NaN, np.NaN, np.NaN, "e", np.NaN, np.NaN, "j"],
"y": [1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0],
},
)
diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py
index 7160ea0c3d7..6d58550f028 100644
--- a/tests/unit_tests/sql/parse_tests.py
+++ b/tests/unit_tests/sql/parse_tests.py
@@ -1143,7 +1143,14 @@ def test_split_kql(kql: str, expected: list[str]) -> None:
("postgresql", "DROP TABLE foo", True),
("postgresql", "EXPLAIN ANALYZE SELECT * FROM foo", False),
("postgresql", "EXPLAIN ANALYZE DELETE FROM foo", True),
+ # SHOW reads server configuration; it mutates nothing, so it is NOT
+ # classified as mutating (that would be wrong for the commit/limit/
+ # "only SELECT" consumers of has_mutation()). Gating disclosure reads
+ # belongs in DISALLOWED_SQL_FUNCTIONS, not the mutation check.
("postgresql", "SHOW search_path", False),
+ # SET search_path parses as exp.Set (a structured node), not
+ # exp.Command, so the SET-in-mutating-commands rule does NOT
+ # catch it. Pure GUC reads/writes stay non-mutating.
("postgresql", "SET search_path TO public", False),
(
"postgres",
@@ -1388,6 +1395,9 @@ def test_custom_dialect(app: None) -> None:
("SELECT 1", False),
("with source as ( select 1 as one ) select * from source", False),
("ALTER TABLE foo ADD COLUMN bar INT", True),
+ # COMMENT ON parses as a typed exp.Comment node across dialects; it
+ # writes to the catalog (pg_description on Postgres) so it is gated.
+ ("COMMENT ON TABLE t IS 'note'", True),
],
)
def test_is_mutating(sql: str, engine: str, expected: bool) -> None:
@@ -1434,6 +1444,222 @@ def test_is_mutating_anonymous_block(sql: str, expected: bool) -> None:
assert SQLStatement(sql, "postgresql").is_mutating() == expected
+@pytest.mark.parametrize(
+ "sql, expected",
+ [
+ # PostgreSQL large-object writers: each mutates server state. The bare
+ # SELECT wrapper is irrelevant because the function call itself is the
+ # side effect.
+ ("SELECT lo_from_bytea(0, decode('deadbeef', 'hex'))", True),
+ ("SELECT lo_export(12345, '/tmp/payload.bin')", True),
+ ("SELECT lo_import('/etc/passwd')", True),
+ ("SELECT lo_put(12345, 0, decode('00', 'hex'))", True),
+ ("SELECT lo_create(0)", True),
+ ("SELECT lowrite(12345, decode('00', 'hex'))", True),
+ # lo_unlink deletes a large object outright.
+ ("SELECT lo_unlink(12345)", True),
+ # PostgreSQL sequence mutators. setval()/nextval() look like reads but
+ # advance sequence state for every subsequent caller.
+ ("SELECT setval('public.my_seq', 1000)", True),
+ ("SELECT SETVAL('public.my_seq', 1)", True),
+ ("SELECT nextval('public.my_seq')", True),
+ # currval() only reads the session's last value, so it is not mutating.
+ ("SELECT currval('public.my_seq')", False),
+ # Read-side large-object functions are intentionally NOT classified
+ # as mutating here. They are still blocked via the function denylist
+ # (see DISALLOWED_SQL_FUNCTIONS) but they do not write state.
+ ("SELECT lo_get(12345)", False),
+ ("SELECT loread(12345, 1024)", False),
+ # Case-insensitive matching: the AST stores the raw casing for
+ # anonymous functions, the check uppercases both sides.
+ ("SELECT LO_EXPORT(12345, '/tmp/x')", True),
+ # `SELECT INTO new_table FROM existing` creates a new relation; treat
+ # as mutating even though sqlglot parses it as exp.Select.
+ ("SELECT * INTO new_table FROM existing_table", True),
+ ("SELECT col INTO TEMP new_table FROM existing_table", True),
+ # A built-in function whose first string argument happens to match a
+ # mutating name must NOT be flagged. sqlglot parses these into dedicated
+ # nodes (e.g. exp.Upper) whose `.name` is the argument text, not the
+ # function name, so the walk is restricted to exp.Anonymous to avoid a
+ # false positive on this read-only query.
+ ("SELECT upper('lo_export')", False),
+ ("SELECT length('setval')", False),
+ # Plain SELECT must remain non-mutating.
+ ("SELECT 1", False),
+ ("SELECT * FROM users WHERE id = 1", False),
+ ],
+)
+def test_is_mutating_postgres_function_and_select_into(
+ sql: str, expected: bool
+) -> None:
+ """
+ `is_mutating` must catch mutating function calls (PostgreSQL large-object
+ writers) and `SELECT ... INTO new_table` even though the wrapping AST
+ node is a plain `exp.Select`.
+ """
+ assert SQLStatement(sql, "postgresql").is_mutating() == expected
+
+
+@pytest.mark.parametrize(
+ "engine, sql",
+ [
+ # `SELECT ... INTO new_table` is CTAS only in Postgres/Redshift/T-SQL.
+ # In Oracle PL/SQL and MySQL the same syntax assigns into a variable
+ # and is a read, so it must NOT be classified as mutating.
+ ("oracle", "SELECT col INTO v FROM existing_table"),
+ ("mysql", "SELECT col INTO @v FROM existing_table"),
+ ],
+)
+def test_is_mutating_select_into_variable_is_read(engine: str, sql: str) -> None:
+ """
+ `SELECT ... INTO target` is only CTAS (mutating) for dialects where the
+ syntax creates a table. On Oracle/MySQL it assigns into a variable and is
+ a read, so `is_mutating` must return False there.
+ """
+ assert SQLStatement(sql, engine).is_mutating() is False
+
+
+@pytest.mark.parametrize(
+ "engine, sql",
+ [
+ # `SELECT ... INTO new_table` is CTAS on Redshift and T-SQL just as it
+ # is on Postgres, so each dialect in _SELECT_INTO_CTAS_DIALECTS must
+ # classify the statement as mutating.
+ ("redshift", "SELECT * INTO new_table FROM existing_table"),
+ ("redshift", "SELECT col INTO new_table FROM existing_table"),
+ ("mssql", "SELECT * INTO new_table FROM existing_table"),
+ ("mssql", "SELECT col INTO new_table FROM existing_table"),
+ ],
+)
+def test_is_mutating_select_into_ctas_dialects(engine: str, sql: str) -> None:
+ """
+ `SELECT ... INTO new_table` creates a table on the CTAS dialects beyond
+ Postgres (Redshift, T-SQL), so `is_mutating` must return True there.
+ """
+ assert SQLStatement(sql, engine).is_mutating() is True
+
+
+@pytest.mark.parametrize(
+ "engine, sql",
+ [
+ # The mutating-function names are PostgreSQL built-ins. On other engines
+ # a same-named read-only function or UDF must NOT be flagged as
+ # mutating, otherwise read-only queries get wrongly blocked.
+ ("mysql", "SELECT setval(my_col)"),
+ ("mysql", "SELECT lo_export(id, path) FROM t"),
+ ("base", "SELECT setval(my_col)"),
+ ("trino", "SELECT lowrite(x)"),
+ ],
+)
+def test_is_mutating_function_names_scoped_to_postgres(engine: str, sql: str) -> None:
+ """
+ `_MUTATING_FUNCTION_NAMES` is PostgreSQL-specific, so the function-name walk
+ only runs for the Postgres dialect; same-named functions on other engines
+ must stay non-mutating.
+ """
+ assert SQLStatement(sql, engine).is_mutating() is False
+
+
+@pytest.mark.parametrize(
+ "sql, expected",
+ [
+ # PostgreSQL constructs that sqlglot parses as opaque exp.Command.
+ # Each can wrap a DML body or change effective server state.
+ ("PREPARE u AS UPDATE t SET x = 1", True),
+ ("PREPARE i AS INSERT INTO t VALUES (1)", True),
+ ("EXECUTE my_plan", True),
+ ("CALL my_writing_procedure()", True),
+ ("COPY t FROM '/tmp/data.csv'", True),
+ ("GRANT SELECT ON t TO public", True),
+ ("REVOKE SELECT ON t FROM public", True),
+ ("SET ROLE other_role", True),
+ ("REFRESH MATERIALIZED VIEW mv", True),
+ ("REINDEX TABLE t", True),
+ ("VACUUM t", True),
+ # SHOW commands are reads (they mutate nothing), so they are NOT
+ # classified as mutating. Gating information-disclosure reads such as
+ # SHOW server_version belongs in DISALLOWED_SQL_FUNCTIONS (which already
+ # blocks pg_read_file, version(), etc.), not in the mutation check.
+ ("SHOW search_path", False),
+ ("SHOW all", False),
+ ("SHOW server_version", False),
+ # RESET reverts a prior SET (e.g. RESET ROLE backs out SET ROLE).
+ ("RESET ROLE", True),
+ # DDL head-tokens that sqlglot falls back to exp.Command for when the
+ # body uses syntax it does not model. One representative per
+ # head-token branch (CREATE/ALTER/DROP); they all hit the same
+ # set-lookup so additional CREATE PUBLICATION/SUBSCRIPTION/etc.
+ # cases would not add coverage.
+ (
+ "CREATE FUNCTION x() RETURNS int AS '/tmp/x.so', 'i' LANGUAGE C",
+ True,
+ ),
+ ("CREATE EXTENSION pg_trgm", True), # non-FUNCTION DDL via Command
+ ("ALTER SYSTEM SET wal_level = 'logical'", True),
+ ("DROP EXTENSION pg_trgm", True),
+ # LOAD dlopens a shared library on the PG host. Same RCE primitive
+ # as `CREATE FUNCTION ... LANGUAGE C` if the library path is
+ # attacker-controlled (e.g. via a prior COPY-to-program foothold).
+ ("LOAD '/tmp/x.so'", True),
+ # Case-insensitive: sqlglot preserves source case on Command.name,
+ # so the set lookup must normalise. Regression for the original
+ # bug where a lowercase head-token bypassed the gate.
+ ("create extension pg_trgm", True),
+ ("load '/tmp/x.so'", True),
+ # Pre-existing positive controls
+ ("DO $$ BEGIN UPDATE t SET x = 1; END $$", True),
+ ("EXPLAIN ANALYZE UPDATE t SET x = 1", True),
+ ],
+)
+def test_is_mutating_postgres_command_constructs(sql: str, expected: bool) -> None:
+ """
+ Several PostgreSQL constructs are represented by sqlglot as opaque
+ `exp.Command` nodes (no structured AST). `is_mutating` recognises them
+ by command name so they cannot slip past the read-only gate.
+ """
+ assert SQLStatement(sql, "postgresql").is_mutating() == expected
+
+
+@pytest.mark.parametrize(
+ "sql, engine, functions, expected",
+ [
+ # MySQL `@@<name>` syntax parses as exp.SessionParameter, which is
+ # not a subclass of exp.Func. The walker must include it so the
+ # denylist entry for `version` still catches `SELECT @@version`.
+ ("SELECT @@version", "mysql", {"version"}, True),
+ ("SELECT @@global.version", "mysql", {"version"}, True),
+ ("SELECT @@hostname", "mysql", {"hostname"}, True),
+ ("SELECT @@datadir", "mysql", {"datadir"}, True),
+ # Negative control: a session parameter not in the denylist must
+ # not match.
+ ("SELECT @@autocommit", "mysql", {"version", "hostname"}, False),
+ # A plain SELECT does not introduce session-parameter names.
+ ("SELECT 1", "mysql", {"version"}, False),
+ # The pre-existing exp.Func walk still works for normal calls.
+ ("SELECT version()", "mysql", {"version"}, True),
+ # PostgreSQL large-object functions are exp.Anonymous calls. The
+ # walk includes them; the denylist entry catches them.
+ ("SELECT lo_export(12345, '/tmp/x')", "postgresql", {"lo_export"}, True),
+ (
+ "SELECT lo_from_bytea(0, decode('00','hex'))",
+ "postgresql",
+ {"lo_from_bytea"},
+ True,
+ ),
+ ("SELECT loread(12345, 1024)", "postgresql", {"loread"}, True),
+ ],
+)
+def test_check_functions_present_session_parameter(
+ sql: str, engine: str, functions: set[str], expected: bool
+) -> None:
+ """
+ `check_functions_present` must visit `exp.SessionParameter` so that
+ denylist entries for names like `version` or `hostname` also match
+ `SELECT @@version` / `SELECT @@hostname` in MySQL.
+ """
+ assert SQLScript(sql, engine).check_functions_present(functions) == expected
+
+
@pytest.mark.parametrize(
"sql, expected",
[