Skip to content

fix(security): apply global guest RLS rules to sub-queries in virtual datasets - #44650

Merged
EnxDev merged 13 commits into
apache:masterfrom
EnxDev:enxdev/fix/virtual-dataset-subquery-guest-rls
Sep 28, 2026
Merged

EnxDev merged 13 commits into
apache:masterfrom
EnxDev:enxdev/fix/virtual-dataset-subquery-guest-rls

Conversation

@EnxDev

@EnxDev EnxDev commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Follow-up to #44645, from @msyavuz's review there. Stacked on #44645: until that merges, this diff also shows its three commits, so only d164260 is new here.

When a guest queries a virtual dataset, global guest RLS rules (no dataset key) are left out of the inner SQL and applied once, on the outer query (#37395, for #37359). The outer filter only narrows the rows the inner SQL returns, though. A table read inside a sub-query of the virtual SQL isn't constrained by it, so in

SELECT a.*, (SELECT count(*) FROM b) AS n FROM a

with a global org_id = 1, n counted every org's rows in b.

The fix separates the two kinds of table read:

  • SQLStatement.apply_rls takes an optional subquery_predicates set. It's used for reads inside a sqlglot SUBQUERY scope (scalar, IN, EXISTS), for any scope nested inside one, and for CTEs such a scope reads from (plus their nested scopes). Every other read keeps using predicates. A LATERAL / CROSS APPLY body is also typed as a SUBQUERY scope, but its rows reach the output like a join's, so it's left out.
  • On the virtual dataset path, utils.apply_rls leaves global guest rules out of predicates as before, but includes them in subquery_predicates when the statement has a sub-query. FROM, join, derived table, CTE and LATERAL reads still don't get the rule twice.
  • SQLStatement.apply_rls returns whether it actually injected a rule, and utils.apply_rls returns that. This way an untouched statement isn't re-rendered through sqlglot, which is the round trip the rls_applied guard exists to avoid.
  • The fail-closed check in get_from_clause, which runs when injecting RLS raised, counts global guest rules for statements with a sub-query, matching what apply_rls would have injected.

Scope decision: joins are left to the outer filter as before. Applying global rules to joined tables would also close the join case, but it would break every virtual dataset that joins a registered lookup table without the rule's column. This PR only covers sub-queries, whose rows never reach the outer filter at all.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A, backend only.

TESTING INSTRUCTIONS

pytest tests/unit_tests/sql/parse_tests.py tests/unit_tests/security/guest_rls_test.py tests/unit_tests/models/test_virtual_dataset_format.py tests/unit_tests/models/test_double_rls_virtual_dataset.py tests/unit_tests/utils/rls_test.py tests/unit_tests/sql_lab_test.py
  • test_rls_subquery_predicates (parse tests) covers which reads get which predicates. Cases: plain read, join, derived table, CTE in FROM, scalar / IN / EXISTS sub-queries, the same table both in FROM and in a sub-query, a derived table inside a sub-query, and a CTE read only from a sub-query. There's also an AS_SUBQUERY variant, a LATERAL / CROSS APPLY case, and a test for the new return value.
  • test_global_guest_rule_applied_to_virtual_dataset_subquery runs the example above through apply_rls the way get_from_clause does. It fails on fix(security): apply global guest RLS rules inside adhoc sub-queries #44645's head and passes here. test_global_guest_rule_left_to_outer_query_without_subquery checks that the join case keeps its single application.
  • test_get_from_clause_fail_closed_counts_global_guest_rls_in_subqueries covers the fail-closed check.

Manual check: with EMBEDDED_SUPERSET on, create a virtual dataset like the one above, embed a chart on it, and request its data with a guest token carrying a clause-only rule. The generated SQL (result_type: "query") has the clause inside the scalar sub-query, and still only once for a, on the outer query.

ADDITIONAL INFORMATION

The UPDATING.md entry added in #44645 is updated to describe the virtual dataset behaviour.

🤖 Generated with Claude Code

EnxDev and others added 4 commits September 25, 2026 10:16
apply_rls skipped global (unscoped) guest-token RLS rules for every
table it rewrote. That is right for a virtual dataset's inner SQL,
because the outer query already applies those rules, but an adhoc
sub-query is not constrained by the outer WHERE clause. With
ALLOW_ADHOC_SUBQUERY enabled, a sub-query in an adhoc metric or column
therefore ran without the guest's clause-only rules.

apply_rls and get_predicates_for_table take an include_global_guest_rls
flag, defaulting to the old behaviour, and validate_adhoc_subquery
turns it on.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Only a virtual dataset's inner SQL needs global guest RLS rules left
out, since its outer query applies them. Make inclusion the default in
apply_rls and get_predicates_for_table and have the virtual dataset
paths (inner SQL rewrite, fail-closed check and cache key) opt out, so
SQL Lab and any new caller stay scoped to the guest token.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… datasets

A virtual dataset's inner SQL left global (unscoped) guest RLS rules to
the outer query, which only filters the rows the inner SQL returns. A
table read inside a sub-query there, such as a scalar COUNT(*) over
another table, was therefore not scoped to the guest token.

SQLStatement.apply_rls takes a separate predicate set for reads inside
a SUBQUERY scope (scalar, IN, EXISTS), scopes nested in one and CTEs
they read. utils.apply_rls passes the global guest rules in that set
when it leaves them to the outer query, so FROM, join, derived table
and LATERAL reads still avoid double application. apply_rls reports
whether a rule was actually injected, so an untouched statement isn't
re-rendered, and the fail-closed check in get_from_clause counts the
sub-query rules it would have applied.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #bc57cf

Actionable Suggestions - 0
Additional Suggestions - 8
  • tests/unit_tests/security/guest_rls_test.py - 6
    • Test scaffolding duplicates new helper · Line 323-341
      This test repeats the scaffolding (sqlite dialect, `superset.utils.rls.db` stub, `get_rls_filters`/`get_guest_rls_filters`/`is_feature_enabled` patches) that the new `_guest_rls_database` helper centralizes for the later tests in this same diff. Building it on the helper removes ~25 duplicated lines and leaves one place to evolve the guest-RLS setup.
    • Unused local mock_pd in new test · Line 321-321
      `mock_pd` is assigned here and never read in this test — the datasource flows to `get_predicates_for_table` only via the `db.session...one_or_none` stub. Sibling tests use the variable in assertions; this one does not, so the local is dead and slightly misleading. Drop the assignment.
    • Unstubbed get_default_schema shifts lookup path · Line 367-374
      `_guest_rls_database` stubs `get_default_catalog` but not `get_default_schema`, so `_find_datasets` (`superset/utils/rls.py:197`) calls a truthy auto-generated MagicMock and resolves the dataset via the null-schema fallback (`all()`), not the schema match the docstring implies. Consumers `_validate_adhoc_subquery_as_guest` and `_apply_virtual_dataset_rls_as_guest` inherit this; set `get_default_schema.return_value` explicitly.
    • Inline import violates rule · Line 405-405
      BITO rule 12745 requires module-level imports unless a documented circular dependency exists. A test module cannot create an import cycle, and other tests (e.g. `tests/unit_tests/dao/dashboard_test.py`) import `superset.models.helpers` at module level. Move this import to the top-level import block.
    • Redundant inline imports · Line 420-421
      BITO rule 12745 requires module-level imports absent a documented circular dependency. `superset.sql.parse` and `superset.utils.rls` are already imported at module level (lines 45-46), so these function-body imports redundantly re-import the same modules. Move `SQLStatement` and `apply_rls` into the top-level import block.
    • Magic number for dataset id · Line 430-430
      `exclude_dataset_id=99` hardcodes the virtual dataset's id, which encodes the tested scenario (VD id distinct from physical dataset id 42, per `apply_rls`'s contract). Sibling tests name this value (`vd_id = 99`). Assign it to a named local so the intent is explicit and the value cannot drift silently.
  • superset/utils/rls.py - 2
    • Cache key misses subquery global RLS · Line 302-302
      `collect_rls_predicates_for_sql` now hardcodes `include_global_guest_rls=False`, but at query time `apply_rls` (rls.py:109-113) still injects global guest rules into tables read inside sub-queries. Today the gap is masked because `get_rls_cache_key` (security/manager.py:5687) already binds all guest RLS clauses into the chart cache key; if this helper is ever consumed standalone, two guests differing only in global rules could share a cache entry. Consider mirroring `statement.has_subquery()` as `models/helpers.py:3805` does.
    • Redundant second predicate collection · Line 109-113
      When `include_global_guest_rls` is True (the default used by `sql_lab.py:526`, `estimate.py:156`, `executor.py:891`) the ternary short-circuits on `not include_global_guest_rls` and never calls `collect_predicates(True)`, but when it is False and `has_subquery()` is False the closure re-runs the full table scan and `get_predicates_for_table` lookups only to discard the result. Compute it once and return None otherwise.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/utils/rls.py - 1
    • CWE-285: Default Broadens RLS Injection · Line 63-63
Review Details
  • Files reviewed - 7 · Commit Range: b49a52f..d164260
    • superset/models/helpers.py
    • superset/sql/parse.py
    • superset/utils/rls.py
    • tests/unit_tests/models/test_virtual_dataset_format.py
    • tests/unit_tests/security/guest_rls_test.py
    • tests/unit_tests/sql/parse_tests.py
    • tests/unit_tests/sql_lab_test.py
  • Files skipped - 1
    • UPDATING.md - Reason: Filter setting
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.22%. Comparing base (ffe9a00) to head (47bde1f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #44650   +/-   ##
=======================================
  Coverage   81.22%   81.22%           
=======================================
  Files        2968     2968           
  Lines      179363   179399   +36     
  Branches    41597    41601    +4     
=======================================
+ Hits       145681   145713   +32     
- Misses      30971    30976    +5     
+ Partials     2711     2710    -1     
Flag Coverage Δ
hive 36.73% <6.00%> (-0.02%) ⬇️
mysql 56.06% <60.00%> (-0.01%) ⬇️
postgres 56.06% <60.00%> (-0.01%) ⬇️
presto 38.62% <6.00%> (-0.02%) ⬇️
python 85.49% <100.00%> (+<0.01%) ⬆️
sqlite 55.78% <60.00%> (-0.01%) ⬇️
unit 77.79% <94.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

# Conflicts:
#	UPDATING.md
#	superset/models/helpers.py
#	superset/utils/rls.py
#	tests/unit_tests/models/test_virtual_dataset_format.py
#	tests/unit_tests/security/guest_rls_test.py
- Keep calling the RLS transformer for every read and derive the
  applied flag from get_predicate(), so the transformers' own
  no-predicate paths stay reachable and covered.
- Only look up the second, global-guest predicate set for a guest user;
  other users have no global guest rules, and has_subquery() is also
  true for plain derived tables and UNIONs.
- Cover a top-level UNION and a nested sub-query in the parse tests,
  and a non-guest user doing a single lookup per table.
- Reword the fail-closed comment in get_from_clause.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread superset/utils/rls.py Outdated
@bito-code-review

Copy link
Copy Markdown
Contributor

Yes, the behavior described is expected and is explicitly noted in the updated documentation within the PR. The PR changes how global guest RLS rules are injected: they are now applied to tables read inside sub-queries (such as scalar sub-queries, IN, or EXISTS clauses) within virtual datasets.

As noted in the updated UPDATING.md file, if a sub-query reads a dataset that lacks a column referenced by a global rule, the query will fail with a column-not-found error. This is a deliberate change to ensure that RLS rules are enforced consistently, even within sub-queries, rather than silently ignoring them or failing to apply them correctly.

UPDATING.md

If a sub-query reads a dataset that lacks a column the rule references, the
query now fails with a column-not-found error instead of reading rows the rule

EnxDev and others added 2 commits September 25, 2026 14:50
…e outer filter

A correlated sub-query, such as a lookup keyed by lookup.id = a.lid, is
tied to the virtual dataset's own rows like a join, so global guest RLS
rules are left to the outer query there, the same as for joins. A lookup
table without the rule's column keeps working. Uncorrelated sub-queries,
which no outer filter constrains, still get the rules.

Correlation is detected from columns qualified with an enclosing table's
name or alias. sqlglot's Scope.is_correlated_subquery isn't used because
it treats every unqualified column as external; here an unqualified
column counts as local, erring toward applying the rules.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@EnxDev
EnxDev requested a review from msyavuz September 25, 2026 12:51
@netlify

netlify Bot commented Sep 25, 2026

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 69067ac
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ab66e23594a710008112518
😎 Deploy Preview https://deploy-preview-44650--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Sep 25, 2026

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit cd739f4
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ab66e3a538c260008a64b6f
😎 Deploy Preview https://deploy-preview-44650--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 47bde1f
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6aba0eaee306190008ff4d19
😎 Deploy Preview https://deploy-preview-44650--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@bito-code-review

bito-code-review Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #1454b2

Actionable Suggestions - 0
Review Details
  • Files reviewed - 5 · Commit Range: d164260..38b778a
    • superset/models/helpers.py
    • superset/sql/parse.py
    • superset/utils/rls.py
    • tests/unit_tests/security/guest_rls_test.py
    • tests/unit_tests/sql/parse_tests.py
  • Files skipped - 1
    • UPDATING.md - Reason: Filter setting
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread superset/sql/parse.py Outdated
Comment thread superset/sql/parse.py
Comment thread superset/utils/rls.py Outdated
Address review on the virtual-dataset sub-query RLS change:

- _is_correlated only looks at a sub-query's own columns. Scope.columns
  also carries the outer references of nested sub-queries, so an
  uncorrelated sub-query wrapping a correlated one lost the rules.
- Sub-query reads no longer exclude the virtual dataset's own RLS, since
  the outer query doesn't constrain them either. The second lookup only
  runs for non-guests when the dataset has RLS for the current user, and
  the fail-closed check matches.
- Reword the correlated sub-query docstring: it is skipped to keep
  lookups without the rule's column working, not because the outer
  filter scopes it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread superset/sql/parse.py Outdated
Comment thread superset/sql/parse.py Outdated
Comment thread superset/sql/parse.py Outdated
Comment thread superset/models/helpers.py
…ing case

A sub-query's Scope.sources holds every CTE in lexical scope, not only the
ones it reads. That made a sub-query correlated to an outer CTE by name look
uncorrelated, and pulled the stricter rules into CTEs only joined in the
main FROM. Use selected_sources for both the correlation check and the CTEs
a sub-query reads.

Compare the names ignoring letter-case, so an alias such as `B` shadowing an
outer `b` on a folding engine counts as the sub-query's own table and keeps
the rules, instead of reading as correlated and skipping them.

Also pin exclude_dataset_id in the fail-closed tests, with a real dataset id
so a regression back to self_id fails.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@EnxDev
EnxDev requested a review from msyavuz September 25, 2026 15:28
@bito-code-review

bito-code-review Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #a4efbc

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset/models/helpers.py - 1
    • Redundant has_subquery calls · Line 3805-3805
      `statement.has_subquery()` does a full sqlglot AST traversal (`find`/`find_all` over `self._parsed`), but here it is evaluated once per `(statement, table)` pair: in the new `exclude_dataset_id` ternary and again in `include_global_guest_rls` (line 3808). It is statement-level invariant, so compute it once per statement to avoid repeated traversals in this error path.
Review Details
  • Files reviewed - 6 · Commit Range: 38b778a..59e14f6
    • superset/models/helpers.py
    • superset/sql/parse.py
    • superset/utils/rls.py
    • tests/unit_tests/security/guest_rls_test.py
    • tests/unit_tests/sql/parse_tests.py
    • tests/unit_tests/models/test_virtual_dataset_format.py
  • Files skipped - 1
    • UPDATING.md - Reason: Filter setting
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread superset/sql/parse.py

@alexandrusoare alexandrusoare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment and we are good

@EnxDev
EnxDev merged commit f3e3c3f into apache:master Sep 28, 2026
72 checks passed
@bito-code-review

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

villebro pushed a commit that referenced this pull request Sep 30, 2026
… datasets (#44650)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
(cherry picked from commit f3e3c3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants