diff --git a/UPDATING.md b/UPDATING.md index 64596977fa3..03f851a9264 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -171,6 +171,36 @@ will now get a TypeScript error and must remove the prop; keeping a manual override was exactly the footgun this change removes (see #42510). No callers in the Superset frontend codebase itself passed this prop. +### Row-level security now filters table reads a same-named CTE used to hide + +`extract_tables_from_statement()` decided whether a reference was a CTE by matching its +bare name against the enclosing scope's CTE names; it now resolves the name through +`Scope.cte_sources`. Three kinds of real table read whose bare name collided with a CTE's +were mistaken for the CTE and dropped from a statement's tables, so they were neither +RLS-filtered nor access-checked: a schema- or catalog-qualified reference, a non-recursive +CTE's own name inside its body, and a forward reference to a later `WITH` item. + +```sql +WITH orders AS (SELECT 1 AS d) SELECT * FROM (SELECT * FROM public.orders) AS z +WITH orders AS (SELECT * FROM orders) SELECT * FROM orders +WITH q1 AS (SELECT key FROM q2), q2 AS (SELECT 1 AS key) SELECT * FROM q1 +``` + +Each read is now reported, so it is filtered when `RLS_IN_SQLLAB` is enabled, matched +against `DISALLOWED_SQL_TABLES`, and requires dataset access under +`raise_for_access(force_dataset_match=True)`. A query that previously ran, reading those +rows unfiltered, may now be filtered or rejected. There is no opt-out — the previous +behavior was a row-level-security bypass. + +### Table aliases keep their quoting through the row-level security rewrite + +Both RLS transformers took the table alias as a string with its quoting stripped and +emitted it verbatim; they now carry the parsed identifier. Emitted SQL is unchanged for an +unquoted identifier; a quoted one keeps its quoting, and a column-alias list +(`FROM t AS x (c1, c2)`) survives the rewrite instead of being dropped. This repairs +row-level security for any aliased table on Snowflake, and for at least one statement shape +on MSSQL where the rewrite previously raised `AttributeError`. + ### Principal listing APIs now honour related-field filters Two authorization-related listing behaviors changed for API clients. Neither diff --git a/superset/sql/parse.py b/superset/sql/parse.py index 1036772071e..6d79abe1c0e 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -275,6 +275,9 @@ class RLSTransformer: return None + def __call__(self, node: exp.Table) -> exp.Expression: + raise NotImplementedError() + class RLSAsPredicateTransformer(RLSTransformer): """ @@ -298,17 +301,17 @@ class RLSAsPredicateTransformer(RLSTransformer): databases without support for subqueries. """ - def __call__(self, node: exp.Expression) -> exp.Expression: - if not isinstance(node, exp.Table): - return node - + def __call__(self, node: exp.Table) -> exp.Expression: predicate = self.get_predicate(node) if not predicate: return node - # qualify columns with table name + # Qualify with the parsed alias node, not the ``node.alias`` string (which drops + # quoting and could inject SQL); use the table when the alias has no name. + table_alias = node.args.get("alias") + qualifier = (table_alias and table_alias.this) or node.this for column in predicate.find_all(exp.Column): - column.set("table", node.alias or node.this) + column.set("table", qualifier.copy()) if isinstance(node.parent, exp.From): select = node.parent.parent @@ -354,13 +357,12 @@ class RLSAsSubqueryTransformer(RLSTransformer): all databases. """ - def __call__(self, node: exp.Expression) -> exp.Expression: - if not isinstance(node, exp.Table): - return node - + def __call__(self, node: exp.Table) -> exp.Expression: if predicate := self.get_predicate(node): - if node.alias: - alias = node.alias + if existing_alias := node.args.get("alias"): + # Reuse the parsed alias node, not the ``node.alias`` string: that drops + # quoting (SQL in an alias re-emits as SQL) and the column-alias list. + alias = existing_alias else: # Use just the table name (not schema-qualified) so that # column references like ``table.column`` still resolve after @@ -1542,7 +1544,30 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): raise ValueError(f"Invalid RLS method: {method}") transformer = transformers[method](catalog, schema, predicates) - self._parsed = self._parsed.transform(transformer) + + # Rewrite the real table reads -- the same set ``extract_tables_from_statement`` + # authorizes -- so the filtered set equals the authorized set. (A CTE reference + # sharing a rule's table name is not a read here.) + seen: set[int] = set() + reads: list[exp.Table] = [] + for scope in traverse_scope(self._parsed): + for source in scope.sources.values(): + # dedupe by identity: a correlated LATERAL reaches one node twice + if ( + isinstance(source, exp.Table) + and not is_cte(source, scope) + and id(source) not in seen + ): + seen.add(id(source)) + reads.append(source) + + # Wrap the deepest reads first: a parenthesised-join head carries its join in + # its args, so wrapping an ancestor before its descendant would strand the + # descendant read's replacement off the live tree. + for node in sorted(reads, key=lambda read: read.depth, reverse=True): + replacement = transformer(node) + if replacement is not node: + node.replace(replacement) class KQLSplitState(enum.Enum): @@ -2184,41 +2209,22 @@ def extract_tables_from_statement( def is_cte(source: exp.Table, scope: Scope) -> bool: """ - Is the source a CTE? + Does this reference resolve to a CTE rather than to a real table? - CTEs in the parent scope look like tables (and are represented by - exp.Table objects), but should not be considered as such; - otherwise a user with access to table `foo` could access any table - with a query like this: - - WITH foo AS (SELECT * FROM target_table) SELECT * FROM foo - - A CTE name is always a bare identifier: it can never carry a schema or - catalog qualifier. A schema/catalog-qualified reference therefore always - resolves to a physical table, even when its final name component happens to - match a CTE defined in scope. Such a reference must be reported as a real - table so it resolves to the correct object; otherwise - ``WITH orders AS (...) SELECT * FROM public.orders`` would treat the - qualified ``public.orders`` as the CTE and drop the physical table from the - extracted set. - - Note: an unqualified reference is always resolved relative to the caller's - own schema/catalog before any downstream use, so treating a bare name that - matches a CTE as a CTE stays correct and is intentionally left unchanged - here. + A CTE reference is also an ``exp.Table``, so it must be excluded from a statement's + read tables, or a rule on a table could be evaded by wrapping it in a same-named + CTE. Resolve the name through ``Scope.cte_sources`` (not ``Scope.sources``, keyed by + ``alias_or_name``, which would hide a real table sharing a CTE's alias); a qualified + reference (schema or catalog) is always a table. Where sqlglot registers a name + differently than SQL scopes it (letter-case, a ``WITH RECURSIVE`` self/forward + reference), this errs toward reporting a table -- a spurious check, not a leak. """ if source.db or source.catalog: # Qualified references are always physical tables, never CTEs. return False - parent_sources = scope.parent.sources if scope.parent else {} - ctes_in_scope = { - name - for name, parent_scope in parent_sources.items() - if isinstance(parent_scope, Scope) and parent_scope.scope_type == ScopeType.CTE - } - - return source.name in ctes_in_scope + resolved = scope.cte_sources.get(source.name) + return isinstance(resolved, Scope) and resolved.scope_type == ScopeType.CTE T = TypeVar("T", str, None) diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index e8043d750af..391313afec0 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -780,30 +780,123 @@ SELECT c FROM z def test_extract_tables_reusing_aliases() -> None: + """Test that the parser follows aliases. + + A non-recursive ``WITH`` item sees only items declared before it, so a forward + reference resolves to the table of that name -- a real read that must be extracted. """ - Test that the parser follows aliases. - """ + # `q1` first: the `q2` in its body, and `q2`'s `src`, are both tables. assert extract_tables_from_sql( """ with q1 as ( select key from q2 where key = '5'), q2 as ( select key from src where key = '5') select * from (select key from q1) a """ - ) == {Table("src")} + ) == {Table("q2"), Table("src")} - # weird query with circular dependency - assert ( - extract_tables_from_sql( - """ + # `src` first: its `q2` is a table; `q2`'s `src` and the outer `src` are the CTE. + assert extract_tables_from_sql( + """ with src as ( select key from q2 where key = '5'), q2 as ( select key from src where key = '5') select * from (select key from src) a """ + ) == {Table("q2")} + + +def test_extract_tables_cte_name_shared_with_table() -> None: + """Test that a CTE's name does not hide reads of the table it is named after. + + Only a reference resolving to the CTE may be excluded; dropping any other costs it + both its row filter and its access check. + """ + # A qualified reference -- in the CTE body or elsewhere -- is the table. + assert extract_tables_from_sql( + "WITH orders AS (SELECT * FROM public.orders) SELECT * FROM orders" + ) == {Table("orders", "public")} + assert extract_tables_from_sql( + "WITH orders AS (SELECT 1 AS d) " + "SELECT * FROM (SELECT * FROM public.orders) AS z" + ) == {Table("orders", "public")} + + # A non-recursive CTE cannot see itself, so its own name in its body is the table. + assert extract_tables_from_sql( + "WITH orders AS (SELECT * FROM orders) SELECT * FROM orders" + ) == {Table("orders")} + + # A catalog disqualifies like a schema; `cat..orders` is checked only when pivoted. + assert extract_tables_from_sql( + "WITH orders AS (SELECT 1 AS amt, 'a' AS mth) " + "SELECT * FROM cat..orders PIVOT(SUM(amt) FOR mth IN ('a'))", + engine="snowflake", + ) == {Table("orders", None, "cat")} + + +def test_extract_tables_cte_reference_not_table() -> None: + """Test the counterpart: a reference that resolves to a CTE is not a table. + + A recursive item's reference to itself is the shape a bare-name compare gets wrong. + """ + assert ( + extract_tables_from_sql( + "WITH RECURSIVE t AS (" + "SELECT 1 AS n UNION ALL SELECT n + 1 FROM t WHERE n < 5" + ") SELECT * FROM t" ) == set() ) +def test_extract_tables_pivoted_cte_reference_is_not_a_table() -> None: + """Test that pivoting a CTE reference does not make it a table read. + + Pivoting yields a new relation, so sqlglot keeps the reference as an ``exp.Table`` + -- the one shape where a CTE reference reaches ``is_cte()`` unqualified. + """ + assert extract_tables_from_sql( + "WITH c AS (SELECT a, b FROM other_table) " + "SELECT * FROM c PIVOT(SUM(b) FOR a IN ('p'))", + engine="snowflake", + ) == {Table("other_table")} + # Also when the pivot sits inside a derived table. + assert extract_tables_from_sql( + "WITH c AS (SELECT a, b FROM other_table) " + "SELECT * FROM (SELECT * FROM c PIVOT(SUM(b) FOR a IN ('p'))) AS z", + engine="snowflake", + ) == {Table("other_table")} + + +def test_extract_tables_aliased_cte_does_not_hide_table() -> None: + """Test that aliasing a CTE reference does not erase a table of the same name. + + ``Scope.sources`` is keyed by ``alias_or_name`` and would file the table under the + CTE's alias; ``cte_sources`` is keyed by CTE name only. + """ + assert extract_tables_from_sql( + "WITH c AS (SELECT 1 AS n) SELECT s2.* FROM c AS other_table, other_table AS s2" + ) == {Table("other_table")} + assert extract_tables_from_sql( + "WITH c AS (SELECT 1 AS n) " + "SELECT s2.* FROM c AS other_table LEFT JOIN other_table AS s2 ON TRUE" + ) == {Table("other_table")} + + +def test_extract_tables_cte_reference_over_reported() -> None: + """Test the two shapes that over-report a CTE reference as a table. + + A spurious access check, not a missing one. Pinned so a change either way is meant. + """ + # PostgreSQL resolves `foo` to the CTE; this reports the table. + assert extract_tables_from_sql("WITH Foo AS (SELECT 1 AS d) SELECT * FROM foo") == { + Table("foo") + } + # Legal under RECURSIVE: `q2` is the CTE declared below, not a table. + assert extract_tables_from_sql( + "WITH RECURSIVE q1 AS (SELECT key FROM q2), q2 AS (SELECT 1 AS key) " + "SELECT * FROM q1" + ) == {Table("q2")} + + def test_extract_tables_multistatement() -> None: """ Test that the parser works with multiple statements. @@ -2913,6 +3006,112 @@ FROM ( LIMIT 100 """.strip(), ), + ( + 'SELECT * FROM tbl_a AS "x AND 1 = 0 OR 1 = 1"', + {Table("tbl_a", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + * +FROM ( + SELECT + * + FROM tbl_a + WHERE + id = 42 +) AS "x AND 1 = 0 OR 1 = 1" + """.strip(), + ), + ( + "SELECT c1 FROM tbl_a AS x (c1, c2)", + {Table("tbl_a", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + c1 +FROM ( + SELECT + * + FROM tbl_a + WHERE + id = 42 +) AS x(c1, c2) + """.strip(), + ), + # A CTE sharing the rule's table name is not a read of it: only the real read + # inside the CTE body is wrapped; the CTE reference keeps its own projection. + ( + "WITH some_table AS (SELECT id FROM some_table) SELECT * FROM some_table", + {Table("some_table", "schema1", "catalog1"): "id = 42"}, + """ +WITH some_table AS ( + SELECT + id + FROM ( + SELECT + * + FROM some_table + WHERE + id = 42 + ) AS "some_table" +) +SELECT + * +FROM some_table + """.strip(), + ), + # A correlated ``LATERAL`` reaches the outer read through two scopes: wrapped + # once, not twice. The lateral's own read is a distinct node, wrapped in place. + ( + "SELECT * FROM some_table, LATERAL (" + "SELECT * FROM other_table WHERE other_table.x = some_table.x) t", + { + Table("some_table", "schema1", "catalog1"): "id = 42", + Table("other_table", "schema1", "catalog1"): "id = 7", + }, + """ +SELECT + * +FROM ( + SELECT + * + FROM some_table + WHERE + id = 42 +) AS "some_table", LATERAL ( + SELECT + * + FROM ( + SELECT + * + FROM other_table + WHERE + id = 7 + ) AS "other_table" + WHERE + other_table.x = some_table.x +) AS t + """.strip(), + ), + # A read in a DML statement's subquery is filtered in place, not refused: the + # ``UPDATE`` target is not a source, so only the ``SELECT`` read of ``t`` wraps. + ( + "UPDATE dst SET x = 1 WHERE id IN (SELECT id FROM t)", + {Table("t", "schema1", "catalog1"): "id = 42"}, + """ +UPDATE dst SET x = 1 +WHERE + id IN ( + SELECT + id + FROM ( + SELECT + * + FROM t + WHERE + id = 42 + ) AS "t" + ) + """.strip(), + ), ], ) def test_rls_subquery_transformer( @@ -2933,6 +3132,63 @@ def test_rls_subquery_transformer( assert statement.format() == expected +@pytest.mark.parametrize( + "sql, read_counts", + [ + ("SELECT * FROM t", {"t": 1}), + ("SELECT * FROM t JOIN u ON t.id = u.id", {"t": 1, "u": 1}), + ("SELECT * FROM t, u", {"t": 1, "u": 1}), + ("SELECT * FROM t WHERE id IN (SELECT id FROM u)", {"t": 1, "u": 1}), + # A self-join reads the table through two distinct nodes; both are wrapped. + ("SELECT * FROM t AS a JOIN t AS b ON a.id = b.id", {"t": 2}), + # The CTE body's read of ``t`` and the outer read of ``t`` are both wrapped; + # the CTE reference ``c`` is not a read and carries no rule. + ( + "WITH c AS (SELECT id FROM t) SELECT * FROM c JOIN t AS t2 ON c.id = t2.id", + {"t": 2}, + ), + ("SELECT * FROM (SELECT * FROM t) AS x", {"t": 1}), + # Pins the deepest-first ordering. The parenthesised join head ``t`` carries the + # join to ``u`` in its own args, so ``u`` must be wrapped before ``t``; wrapping + # ``t`` first would copy ``u`` into ``t``'s subquery and drop ``u``'s filter. + # Flipping the sort to ``reverse=False`` makes this case fail. + ("SELECT * FROM (t JOIN u ON t.id = u.id)", {"t": 1, "u": 1}), + # A correlated ``LATERAL`` reaches the outer read through two scopes; it is + # wrapped once, and the lateral's own read is wrapped once. + ( + "SELECT * FROM some_table, LATERAL (" + "SELECT * FROM other_table WHERE other_table.x = some_table.x) t", + {"some_table": 1, "other_table": 1}, + ), + ], +) +def test_rls_subquery_filters_every_authorized_read( + sql: str, + read_counts: dict[str, int], +) -> None: + """The set the rewrite filters equals the set authorization enforces. + + Each read gets a table-specific sentinel predicate; its count in the output must + equal that table's real-read node count, catching a dropped read or a double-wrap. + """ + authorized = {t.table for t in extract_tables_from_statement(parse_one(sql), None)} + assert authorized == set(read_counts) + + statement = SQLStatement(sql) + statement.apply_rls( + "catalog1", + "schema1", + { + Table(table, "schema1", "catalog1"): [parse_one(f"rls_{table} = 1")] + for table in read_counts + }, + RLSMethod.AS_SUBQUERY, + ) + output = statement.format() + for table, count in read_counts.items(): + assert output.count(f"rls_{table} = 1") == count + + def test_rls_invalid_method(mocker: MockerFixture) -> None: """ Test that an invalid RLS method raises an error. @@ -3257,6 +3513,58 @@ VALUES (1, 2) """.strip(), ), + ( + 'SELECT * FROM tbl_a AS "x AND 1 = 0 OR 1 = 1"', + {Table("tbl_a", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + * +FROM tbl_a AS "x AND 1 = 0 OR 1 = 1" +WHERE + "x AND 1 = 0 OR 1 = 1".id = 42 + """.strip(), + ), + ( + 'SELECT * FROM tbl_a AS "a.b"', + {Table("tbl_a", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + * +FROM tbl_a AS "a.b" +WHERE + "a.b".id = 42 + """.strip(), + ), + # A column-list alias has no name (``this`` is ``None``); qualify with the table + # so the predicate does not resolve outward into an enclosing scope. + ( + "SELECT * FROM tbl_a AS (c1, c2)", + {Table("tbl_a", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + * +FROM tbl_a AS _t0(c1, c2) +WHERE + tbl_a.id = 42 + """.strip(), + ), + # A table heading a parenthesised join is a read, but its parent is the wrapping + # ``Subquery``, not a ``From``/``Join``, so the predicate method leaves it -- + # fail-closed (the subquery method filters it). Pinned to catch a shape change. + ( + "SELECT * FROM (some_table JOIN other_table " + "ON some_table.id = other_table.id)", + {Table("some_table", "schema1", "catalog1"): "id = 42"}, + """ +SELECT + * +FROM ( + some_table + JOIN other_table + ON some_table.id = other_table.id +) + """.strip(), + ), ], ) def test_rls_predicate_transformer(