From f17b1570319391f85ab4de3e2d5cebfd3984390e Mon Sep 17 00:00:00 2001 From: sha174n Date: Mon, 25 May 2026 15:25:39 +0100 Subject: [PATCH 1/6] fix(sql): broaden mutating-statement detection in SQL Lab parser is_mutating() and check_functions_present() in superset/sql/parse.py previously missed several construct shapes that share a meta cause: AST nodes that are not subclasses of the expected types, opaque exp.Command nodes for PostgreSQL constructs that sqlglot cannot fully structure, and function-name patterns that mutate server state without a wrapping DML node. This change addresses: 1. PostgreSQL opaque exp.Command constructs now classified as mutating alongside the existing DO and EXPLAIN ANALYZE handling: PREPARE, EXECUTE, CALL, COPY, GRANT, REVOKE, SET, REFRESH, REINDEX, VACUUM. 2. Structured nodes added to the mutating-node tuple: exp.Copy, exp.Grant, exp.Revoke. Some dialects parse these into structured nodes (e.g. COPY in PostgreSQL becomes exp.Copy, not exp.Command), so node-type matching is needed in addition to command-name matching. 3. PostgreSQL large-object writer functions classified as mutating by name: lo_from_bytea, lo_export, lo_import, lo_put, lo_create, lowrite. These appear as plain function calls inside an exp.Select wrapper and have no enclosing mutating AST node. The full lo_* family (writers and readers) is also added to the PostgreSQL entry in DISALLOWED_SQL_FUNCTIONS for defense in depth. 4. SELECT ... INTO new_table FROM ... parses as exp.Select with an `into` arg (PostgreSQL CTAS variant). It creates a new relation and is now treated as mutating. 5. check_functions_present() now visits exp.SessionParameter so that MySQL's @@ syntax matches denylist entries like `version` or `hostname`. exp.SessionParameter is not a subclass of exp.Func, so the existing exp.Func walk skipped it. Tests ----- - New: test_is_mutating_postgres_function_and_select_into covers the lo_* writers (positive), lo_* readers (negative, blocked separately via denylist), case-insensitivity, and SELECT INTO. - New: test_is_mutating_postgres_command_constructs covers PREPARE/EXECUTE/CALL/COPY/GRANT/REVOKE/SET/REFRESH/REINDEX/VACUUM, with the pre-existing DO and EXPLAIN ANALYZE controls included. - New: test_check_functions_present_session_parameter covers MySQL @@version / @@hostname etc., plus lo_* function denylist matching. - All 562 tests in tests/unit_tests/sql/parse_tests.py pass. - All 117 tests in tests/unit_tests/sql/execution/ + sql_lab_test.py pass; no regressions. Co-Authored-By: Claude Opus 4.7 --- superset/config.py | 13 ++++ superset/sql/parse.py | 77 ++++++++++++++++++- tests/unit_tests/sql/parse_tests.py | 110 ++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+), 4 deletions(-) diff --git a/superset/config.py b/superset/config.py index 2f83c65e4d6f..eac31abb5e77 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1775,6 +1775,19 @@ def engine_context_manager( # pylint: disable=unused-argument "pg_read_file", "pg_ls_dir", "pg_read_binary_file", + # PostgreSQL large-object functions: writers can plant arbitrary + # bytes on the server filesystem (lo_export, lo_from_bytea, lowrite, + # lo_put, lo_create, lo_import) and readers can pull bytes back out + # (lo_get, loread). Defense-in-depth on top of is_mutating()'s + # function-name check. + "lo_from_bytea", + "lo_export", + "lo_import", + "lo_put", + "lo_create", + "lowrite", + "lo_get", + "loread", # XML functions that can execute SQL "database_to_xml", "database_to_xmlschema", diff --git a/superset/sql/parse.py b/superset/sql/parse.py index c4983b6d3ef9..ac89b3411390 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -562,6 +562,41 @@ 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", + } + ) + + # 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", + "SET", # SET ROLE / SET SESSION AUTHORIZATION change effective user + "REFRESH", # REFRESH MATERIALIZED VIEW + "REINDEX", + "VACUUM", + } + ) + def __init__( self, statement: str | None = None, @@ -690,25 +725,52 @@ def is_mutating(self) -> bool: 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 FROM/TO (server-side file ingest) + exp.Grant, + exp.Revoke, ) for node_type in mutating_nodes: if self._parsed.find(node_type): 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. + if 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. + for function in self._parsed.find_all(exp.Func): + name = ( + function.name.upper() + if function.sql_name() == "ANONYMOUS" + else function.sql_name().upper() + ) + if name in self._MUTATING_FUNCTION_NAMES: + 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. if ( self._dialect == Dialects.POSTGRES and isinstance(self._parsed, exp.Command) - and self._parsed.name == "DO" + and self._parsed.name 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 @@ -829,6 +891,13 @@ def check_functions_present(self, functions: set[str]) -> bool: else: present.add(function.name.upper()) + # MySQL `@@` 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/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index b6d0c5580530..2af616395e97 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -1372,6 +1372,116 @@ 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), + # 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), + # 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( + "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), + # 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 `@@` 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", [ From f6c76596e22c541b66a980ca6edf61fb8867ce1f Mon Sep 17 00:00:00 2001 From: sha174n Date: Tue, 26 May 2026 17:00:22 +0100 Subject: [PATCH 2/6] fix(sql): close information_schema, SHOW, and setval gaps Follow-up to the initial broadening of is_mutating() and check_functions_present(). This change closes the remaining bypass surfaces that the SQL Lab read-only gate did not catch: 1. PostgreSQL information_schema views. Adds information_schema.tables, .columns, .schemata, .views, .routines, .role_table_grants, .role_column_grants, .role_routine_grants, .table_privileges, .column_privileges, .usage_privileges, .key_column_usage, .table_constraints, .referential_constraints, .view_table_usage, .applicable_roles, .enabled_roles to DISALLOWED_SQL_TABLES['postgresql']. These entries are schema-qualified, so they require the new schema-aware matching in check_tables_present (see below). Without this change, a user table happening to be named `tables` or `columns` would have been falsely blocked alongside the information_schema views. 2. check_tables_present schema-qualified matching. The previous matcher built `present = {t.table.lower() ...}` from parsed table references, stripping the schema. A denylist entry of `information_schema.tables` therefore could not match without also matching every user table named `tables`. The new matcher distinguishes bare entries (no dot in the denylist string) from schema-qualified entries (with a dot): - Bare entries match by table name regardless of schema (existing behavior preserved for `pg_stat_activity` etc.). - Schema-qualified entries match only when both schema and table match. 3. SHOW commands. Adds SHOW to _POSTGRES_MUTATING_COMMAND_NAMES so it is rejected by the allow_dml=False gate, mirroring the read-side blocks already in DISALLOWED_SQL_FUNCTIONS for pg_read_file, version(), current_setting, etc. SHOW reads server-configuration state (server version, hba_file, ssl, search_path) which has the same information-disclosure profile as those functions. 4. PostgreSQL setval() sequence mutator. Adds SETVAL to _MUTATING_FUNCTION_NAMES. setval() looks like a read but advances sequence state for every subsequent nextval caller. Tests ----- - New test_check_tables_present_schema_qualified covers bare vs schema-qualified matching, including the must-not-match case for user tables sharing a name with an information_schema view. - test_is_mutating_postgres_function_and_select_into extended with setval positive and case-insensitivity cases. - test_is_mutating_postgres_command_constructs extended with SHOW positive cases (search_path, all, server_version). - Pre-existing test_has_mutation expectation for `SHOW search_path` flipped from False to True to reflect the intentional behavior change; `SET search_path TO public` remains non-mutating because it parses as exp.Set, not exp.Command. - All 576 tests in tests/unit_tests/sql/parse_tests.py pass. - All 117 tests in tests/unit_tests/sql/execution/ and sql_lab_test.py pass; no regressions. Co-Authored-By: Claude Opus 4.7 --- superset/config.py | 23 +++++++ superset/sql/parse.py | 41 +++++++++++-- tests/unit_tests/sql/parse_tests.py | 94 ++++++++++++++++++++++++++++- 3 files changed, 153 insertions(+), 5 deletions(-) diff --git a/superset/config.py b/superset/config.py index eac31abb5e77..7ff76085d46a 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1878,6 +1878,29 @@ def engine_context_manager( # pylint: disable=unused-argument "pg_stat_replication", "pg_stat_wal_receiver", "pg_user", + # The SQL-standard `information_schema` views expose table / + # column / privilege / view-definition metadata across the entire + # database role the connection user can see. Entries are + # schema-qualified so `check_tables_present` only matches when the + # query actually references `information_schema.`, not any + # user table that happens to share a name. + "information_schema.tables", + "information_schema.columns", + "information_schema.schemata", + "information_schema.views", + "information_schema.routines", + "information_schema.role_table_grants", + "information_schema.role_column_grants", + "information_schema.role_routine_grants", + "information_schema.table_privileges", + "information_schema.column_privileges", + "information_schema.usage_privileges", + "information_schema.key_column_usage", + "information_schema.table_constraints", + "information_schema.referential_constraints", + "information_schema.view_table_usage", + "information_schema.applicable_roles", + "information_schema.enabled_roles", }, "mysql": { "mysql.user", diff --git a/superset/sql/parse.py b/superset/sql/parse.py index ac89b3411390..3feda11ed410 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -574,6 +574,10 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): "LO_PUT", "LO_CREATE", "LOWRITE", + # PostgreSQL sequence mutators. `SELECT setval('seq', N)` looks + # like a read but changes sequence state for every subsequent + # `nextval` caller. + "SETVAL", } ) @@ -594,6 +598,12 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): "REFRESH", # REFRESH MATERIALIZED VIEW "REINDEX", "VACUUM", + # SHOW reads server configuration (version, hba_file, ssl state, + # search_path, etc.). It does not mutate, but in a read-only + # context (`allow_dml=False`) it is information-disclosure + # equivalent to the read-side entries in DISALLOWED_SQL_FUNCTIONS + # which are also blocked. Treat as gated alongside the writers. + "SHOW", } ) @@ -904,11 +914,34 @@ def check_tables_present(self, tables: set[str]) -> bool: """ Check if any of the given tables are present in the statement. + Denylist entries may be bare (``pg_stat_activity``) or + schema-qualified (``information_schema.tables``). Bare entries + match by table name regardless of schema; qualified entries + require the schema to match too. This lets us block all access + to ``information_schema`` without also blocking any + user-authored table that happens to be named ``tables``. + :param tables: Set of table names to check for (case-insensitive) - :return: True if any of the tables are present - """ - present = {table.table.lower() for table in self.tables} - return any(table.lower() in present for table in tables) + :return: True if any of the given tables is referenced + """ + present_bare: set[str] = set() + present_qualified: set[str] = set() + for t in self.tables: + bare = (t.table or "").lower() + if not bare: + continue + present_bare.add(bare) + if t.schema: + present_qualified.add(f"{t.schema.lower()}.{bare}") + for entry in tables: + needle = entry.lower() + if "." in needle: + if needle in present_qualified: + return True + else: + if needle in present_bare: + return True + return False def get_limit_value(self) -> int | None: """ diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index 2af616395e97..6f32469a995b 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -1105,7 +1105,15 @@ 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), - ("postgresql", "SHOW search_path", False), + # SHOW reads server configuration (version, hba_file, ssl, + # search_path, etc.). It is information-disclosure equivalent to + # the read-side entries already in DISALLOWED_SQL_FUNCTIONS + # (pg_read_file, version, current_setting) and is gated alongside + # the writers so it is rejected when allow_dml=False. + ("postgresql", "SHOW search_path", True), + # 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", @@ -1384,6 +1392,10 @@ def test_is_mutating_anonymous_block(sql: str, expected: bool) -> None: ("SELECT lo_put(12345, 0, decode('00', 'hex'))", True), ("SELECT lo_create(0)", True), ("SELECT lowrite(12345, decode('00', 'hex'))", True), + # PostgreSQL sequence mutator. setval() looks like a read but + # advances sequence state for every subsequent nextval caller. + ("SELECT setval('public.my_seq', 1000)", True), + ("SELECT SETVAL('public.my_seq', 1)", True), # 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. @@ -1428,6 +1440,14 @@ def test_is_mutating_postgres_function_and_select_into( ("REFRESH MATERIALIZED VIEW mv", True), ("REINDEX TABLE t", True), ("VACUUM t", True), + # SHOW commands disclose server configuration (version, hba_file, + # ssl, search_path, etc.). They are read-only but treated as gated + # so they are blocked by the same allow_dml=False gate that blocks + # the writers; this mirrors the existing read-side blocks in + # DISALLOWED_SQL_FUNCTIONS for pg_read_file, version(), etc. + ("SHOW search_path", True), + ("SHOW all", True), + ("SHOW server_version", True), # Pre-existing positive controls ("DO $$ BEGIN UPDATE t SET x = 1; END $$", True), ("EXPLAIN ANALYZE UPDATE t SET x = 1", True), @@ -3243,6 +3263,78 @@ def test_check_tables_present(sql: str, engine: str, expected: bool) -> None: assert SQLScript(sql, engine).check_tables_present(tables) == expected +@pytest.mark.parametrize( + "sql, denylist, expected", + [ + # Schema-qualified denylist entry matches schema-qualified reference. + ( + "SELECT * FROM information_schema.tables", + {"information_schema.tables"}, + True, + ), + # ... and is case-insensitive. + ( + "SELECT * FROM INFORMATION_SCHEMA.TABLES", + {"information_schema.tables"}, + True, + ), + # Schema-qualified denylist entry does NOT match a bare-name table + # of the same name in another schema. A user table named `tables` + # remains queryable. + ( + "SELECT * FROM public.tables", + {"information_schema.tables"}, + False, + ), + ( + "SELECT * FROM tables", + {"information_schema.tables"}, + False, + ), + # Bare-name denylist entry still matches by table name only + # (existing behavior, schema-agnostic). + ( + "SELECT * FROM pg_stat_activity", + {"pg_stat_activity"}, + True, + ), + ( + "SELECT * FROM pg_catalog.pg_stat_activity", + {"pg_stat_activity"}, + True, + ), + # Mixed entries: one schema-qualified, one bare. Match either. + ( + "SELECT * FROM information_schema.columns", + {"information_schema.tables", "information_schema.columns"}, + True, + ), + ( + "SELECT * FROM pg_roles", + {"information_schema.tables", "pg_roles"}, + True, + ), + # Negative control. + ( + "SELECT * FROM my_table", + {"information_schema.tables", "pg_roles"}, + False, + ), + ], +) +def test_check_tables_present_schema_qualified( + sql: str, denylist: set[str], expected: bool +) -> None: + """ + `check_tables_present` must distinguish schema-qualified denylist + entries (e.g. `information_schema.tables`) from bare-name entries + (e.g. `pg_stat_activity`). Schema-qualified entries only match + schema-qualified references in the SQL; bare entries match the + table name regardless of schema. + """ + assert SQLScript(sql, "postgresql").check_tables_present(denylist) == expected + + @pytest.mark.parametrize( "kql, expected", [ From 8146ae11e3e39fa0e1b3e3f57f36b9394aa4c012 Mon Sep 17 00:00:00 2001 From: sha174n Date: Tue, 26 May 2026 17:44:58 +0100 Subject: [PATCH 3/6] test(sql): cover MySQL and MSSQL schema-qualified denylist matching The previous parametrization for test_check_tables_present_schema_qualified exercised only PostgreSQL. The shipped DISALLOWED_SQL_TABLES entries for MySQL and MSSQL are also schema-qualified (mysql.user, performance_schema.threads, performance_schema.processlist, sys.sql_logins, sys.server_principals, sys.configurations) and rely on the same matcher. Without explicit dialect coverage a future refactor of check_tables_present could silently regress one engine while leaving the others working. Adds parametrized cases for: - mysql: mysql.user, performance_schema.threads, performance_schema.processlist (positive); mydb.user (negative, user-authored table sharing the leaf name must not be blocked). - mssql: sys.sql_logins, sys.server_principals, sys.configurations (positive); mydb.sql_logins (negative). All 17 parameter cases pass; no regressions in the broader suite (584 tests in tests/unit_tests/sql/parse_tests.py). Co-Authored-By: Claude Opus 4.7 --- tests/unit_tests/sql/parse_tests.py | 85 ++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index 6f32469a995b..00d1c4e0887d 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -3264,16 +3264,19 @@ def test_check_tables_present(sql: str, engine: str, expected: bool) -> None: @pytest.mark.parametrize( - "sql, denylist, expected", + "engine, sql, denylist, expected", [ - # Schema-qualified denylist entry matches schema-qualified reference. + # Postgres: schema-qualified denylist entry matches schema-qualified + # reference. ( + "postgresql", "SELECT * FROM information_schema.tables", {"information_schema.tables"}, True, ), # ... and is case-insensitive. ( + "postgresql", "SELECT * FROM INFORMATION_SCHEMA.TABLES", {"information_schema.tables"}, True, @@ -3282,11 +3285,13 @@ def test_check_tables_present(sql: str, engine: str, expected: bool) -> None: # of the same name in another schema. A user table named `tables` # remains queryable. ( + "postgresql", "SELECT * FROM public.tables", {"information_schema.tables"}, False, ), ( + "postgresql", "SELECT * FROM tables", {"information_schema.tables"}, False, @@ -3294,45 +3299,109 @@ def test_check_tables_present(sql: str, engine: str, expected: bool) -> None: # Bare-name denylist entry still matches by table name only # (existing behavior, schema-agnostic). ( + "postgresql", "SELECT * FROM pg_stat_activity", {"pg_stat_activity"}, True, ), ( + "postgresql", "SELECT * FROM pg_catalog.pg_stat_activity", {"pg_stat_activity"}, True, ), # Mixed entries: one schema-qualified, one bare. Match either. ( + "postgresql", "SELECT * FROM information_schema.columns", {"information_schema.tables", "information_schema.columns"}, True, ), ( + "postgresql", "SELECT * FROM pg_roles", {"information_schema.tables", "pg_roles"}, True, ), # Negative control. ( + "postgresql", "SELECT * FROM my_table", {"information_schema.tables", "pg_roles"}, False, ), + # MySQL: the shipped DISALLOWED_SQL_TABLES['mysql'] entries are all + # schema-qualified (`mysql.user`, `performance_schema.threads`, + # `performance_schema.processlist`). Without schema-aware matching + # the entries are dead config. These cases pin the fix. + ( + "mysql", + "SELECT user, host, authentication_string FROM mysql.user", + {"mysql.user"}, + True, + ), + ( + "mysql", + "SELECT * FROM performance_schema.threads", + {"performance_schema.threads"}, + True, + ), + ( + "mysql", + "SELECT * FROM performance_schema.processlist", + {"performance_schema.processlist"}, + True, + ), + # MySQL must NOT block a user-authored table that shares the leaf + # name with the system view. + ( + "mysql", + "SELECT * FROM mydb.user", + {"mysql.user"}, + False, + ), + # MSSQL: same shape, `sys.*` entries are schema-qualified. + ( + "mssql", + "SELECT name, password_hash FROM sys.sql_logins", + {"sys.sql_logins"}, + True, + ), + ( + "mssql", + "SELECT name, sid FROM sys.server_principals", + {"sys.server_principals"}, + True, + ), + ( + "mssql", + "SELECT * FROM sys.configurations", + {"sys.configurations"}, + True, + ), + # MSSQL must NOT block a user-authored table sharing the leaf name. + ( + "mssql", + "SELECT * FROM mydb.sql_logins", + {"sys.sql_logins"}, + False, + ), ], ) def test_check_tables_present_schema_qualified( - sql: str, denylist: set[str], expected: bool + engine: str, sql: str, denylist: set[str], expected: bool ) -> None: """ `check_tables_present` must distinguish schema-qualified denylist - entries (e.g. `information_schema.tables`) from bare-name entries - (e.g. `pg_stat_activity`). Schema-qualified entries only match - schema-qualified references in the SQL; bare entries match the - table name regardless of schema. + entries (e.g. `information_schema.tables`, `mysql.user`, + `sys.sql_logins`) from bare-name entries (e.g. `pg_stat_activity`). + Schema-qualified entries only match schema-qualified references in + the SQL; bare entries match the table name regardless of schema. + + Covers Postgres, MySQL, and MSSQL dialects so the shipped + DISALLOWED_SQL_TABLES entries for each remain effective. """ - assert SQLScript(sql, "postgresql").check_tables_present(denylist) == expected + assert SQLScript(sql, engine).check_tables_present(denylist) == expected @pytest.mark.parametrize( From 8e904fc7fc506ce763ed4d8ac9d80bcbae65bb2e Mon Sep 17 00:00:00 2001 From: sha174n Date: Tue, 26 May 2026 18:06:54 +0100 Subject: [PATCH 4/6] fix(deps): bump duckdb 1.4.2 -> 1.5.3 to satisfy check-python-deps Master moved the duckdb pin but never regenerated the pinned requirements file, so every PR's check-python-deps job has been failing on this single-line diff. Regen via the uv-pip-compile path that CI uses (`uv pip compile pyproject.toml requirements/base.in`). --- requirements/development.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/development.txt b/requirements/development.txt index 6b1f3c447af5..2cb8b434ffd6 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -219,7 +219,7 @@ docstring-parser==0.17.0 # via cyclopts docutils==0.22.2 # via rich-rst -duckdb==1.4.2 +duckdb==1.5.3 # via # apache-superset # duckdb-engine From ef69fcda48b96758df8bec092aa5143114d724f3 Mon Sep 17 00:00:00 2001 From: sha174n Date: Tue, 26 May 2026 18:57:56 +0100 Subject: [PATCH 5/6] fix(sql): add coverage for BaseSQLStatement.is_destructive abstract stub Coverage was at 99 (fail-under=100). Both concrete subclasses override is_destructive(), so the base-class `raise NotImplementedError()` line was never executed in tests. Added a direct test that calls the unbound base method on a sentinel object, matching the pattern used elsewhere in the codebase for abstract stubs. --- tests/unit_tests/sql/parse_tests.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index 00d1c4e0887d..2bce68322f00 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -24,6 +24,7 @@ from superset.exceptions import QueryClauseValidationException, SupersetParseError from superset.jinja_context import JinjaTemplateProcessor from superset.sql.parse import ( + BaseSQLStatement, CTASMethod, extract_tables_from_statement, JinjaSQLResult, @@ -3582,3 +3583,14 @@ def test_backtick_invalid_sql_still_fails() -> None: sql = "SELECT * FROM `table` WHERE" with pytest.raises(SupersetParseError): SQLScript(sql, "base") + + +def test_base_sql_statement_is_destructive_raises_not_implemented() -> None: + """ + BaseSQLStatement.is_destructive is abstract; both concrete subclasses + (SQLStatement and KustoKQLStatement) override it, so calling the base + implementation directly must raise. This exercises the abstract stub + so it stays exercised under coverage. + """ + with pytest.raises(NotImplementedError): + BaseSQLStatement.is_destructive(object()) # type: ignore[arg-type] From 4cc0164406bd070a3badcd3a5281363bd04a5b4c Mon Sep 17 00:00:00 2001 From: sha174n Date: Thu, 28 May 2026 13:20:24 +0100 Subject: [PATCH 6/6] fix(sql): drop unreachable empty-table-name guard check_tables_present had `if not bare: continue` after `bare = (t.table or "").lower()`. sqlglot Table.table is always non-empty for any parseable table reference, so the guard never triggered and broke the 100% coverage gate. Use `t.table.lower()` directly. If a future sqlglot version ever returns None, the AttributeError surfaces immediately rather than silently polluting the present-tables set. Co-Authored-By: Claude Opus 4.7 --- superset/sql/parse.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/superset/sql/parse.py b/superset/sql/parse.py index 3feda11ed410..eed439a64e8c 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -927,9 +927,7 @@ def check_tables_present(self, tables: set[str]) -> bool: present_bare: set[str] = set() present_qualified: set[str] = set() for t in self.tables: - bare = (t.table or "").lower() - if not bare: - continue + bare = t.table.lower() present_bare.add(bare) if t.schema: present_qualified.add(f"{t.schema.lower()}.{bare}")