Ujjwaljain16 opened a new pull request, #43985:
URL: https://github.com/apache/superset/pull/43985
### SUMMARY
`Query.get_extra_cache_keys()` (superset/models/sql_lab.py) — the method that
supplies extra, RLS-sensitive cache-key material for a SQL Lab ad-hoc query
used as a chart's datasource — unconditionally returned `[]`. `SqlaTable`
already includes RLS predicates from a virtual dataset's underlying tables
in its own `get_extra_cache_keys()` via `collect_rls_predicates_for_sql()`;
`Query` never got the same treatment.
This is adjacent to, but distinct from, #33346. **The original #33346
query-execution vulnerability is already fixed** on current master:
`ExploreMixin.get_from_clause` (shared by `SqlaTable` virtual datasets and
`Query` alike) parses the raw SQL for referenced tables and injects those
tables' RLS predicates at query-construction time, and fails closed rather
than running unfiltered if that can't be safely resolved. That fix was not
tied to #33346 in its commit history. A regression test for that
already-fixed path is added here since none existed for the `Query`
datasource type specifically. This PR fixes an adjacent RLS cache-key
isolation gap affecting the same SQL Lab ad-hoc query flow.
`Query.is_rls_supported` is (correctly) `False`: an ad-hoc SQL Lab query has
no RLS rules directly attached to it as a dataset, so the dataset-level
`security_manager.get_rls_cache_key()` mechanism correctly contributes
nothing. But that's a different axis from "the RLS applicable to the
*tables this query's SQL references*" cache-key material, which `Query` was
simply missing.
**Why this matters**: a chart built from a SQL Lab query can never be
*saved* as a Slice/dashboard — `CreateChartCommand.validate()` explicitly
rejects any `datasource_type != TABLE` for exactly this reason (it can't
render on reload). It *can*, however, be viewed by a second user via a
shared Explore permalink: `check_query_access` grants a non-author the
same catalog/schema/`datasource_access` checks as any other datasource,
not a free pass. If two such viewers have different RLS on the query's
underlying tables, and query-result caching is enabled (the docs recommend
`DATA_CACHE_CONFIG`/Redis for production), the second viewer's request
could hit the first viewer's cache entry.
**Fix**: wire `Query.get_extra_cache_keys()` up to
`collect_rls_predicates_for_sql()` — the same helper `SqlaTable` already
uses for virtual datasets — mirroring that existing pattern exactly. No new
abstractions.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only, no UI change). Before/after values of
`Query.get_extra_cache_keys()` for two viewers with differing RLS on the
same underlying table, verified live on Postgres:
| | gamma (restricted) | admin (unrestricted) |
|---|---|---|
| Before | `[]` | `[]` |
| After | `["(col2 = 'a')"]` | `[]` |
### TESTING INSTRUCTIONS
```
pytest
tests/integration_tests/security/row_level_security_tests.py::test_rls_filter_applies_to_sqllab_adhoc_query
pytest
tests/integration_tests/security/row_level_security_tests.py::test_rls_filter_in_sqllab_adhoc_query_cache_key
```
- `test_rls_filter_applies_to_sqllab_adhoc_query`: regression coverage for
#33346's already-fixed query-construction path (previously untested for
the `Query` datasource type specifically).
- `test_rls_filter_in_sqllab_adhoc_query_cache_key`: regression coverage for
this fix; fails without it (`Query.get_extra_cache_keys()` returns `[]`
for both a restricted and unrestricted viewer), passes with it.
Verified on real Postgres via the project's `pytest-runner` CI-parity
environment, before and after the fix. Full
`row_level_security_tests.py` suite: 51 passed (1 pre-existing, unrelated
failure confirmed identical without this change). `sql_lab_test.py` +
`rls_test.py` unit suites: 27 passed, including a test that documents real
RLS enforcement never goes through this cache-key helper — confirming this
change cannot affect query correctness, only cache isolation. `ruff
format`/`ruff check`: clean. `pylint`: 10.00/10. `mypy`: no new errors
introduced.
### ADDITIONAL INFORMATION
- [ ] Has associated issue: adjacent to #33346
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### SCOPE LIMITATIONS
- Does not change `Query.cache_timeout` (`0`, which Superset's caching
layer treats as "never expire," not "disabled" — only `-1` disables
caching). A stale-but-now-correctly-isolated cache entry can still
outlive its RLS context if a user's own role changes later; that's a
separate, broader caching-freshness question, not a cross-user bypass,
and is out of scope here.
- Does not address a separate, pre-existing case-sensitivity gap in
`get_predicates_for_table`'s table-name matching (confirmed to affect
virtual datasets too, not specific to this code path) — tracked
separately, not touched by this PR.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
