fix(rls): enforce row-level security on ad-hoc SQL chart creation - #43090
fix(rls): enforce row-level security on ad-hoc SQL chart creation #43090eugeneo17 wants to merge 1 commit into
Conversation
Charts created from SQL Lab ad-hoc SQL bypassed Row-Level Security that SQL Lab itself enforced, silently rendering unfiltered data (issue apache#33346: 3 filtered rows in SQL Lab, 125 unfiltered in the chart). In embedded / multi-tenant deployments where RLS is the tenant boundary this is a cross-tenant disclosure. Root cause is path divergence: charts built from SQL Lab use a Query datasource whose is_rls_supported is False and whose get_sqla_row_level_filters() was a stub returning [], and the RLS cache key skipped Query datasources. This routes every ad-hoc-SQL datasource render through a single, fail-closed, server-side enforcement gate (superset/security/ rls_enforcement.py) that rewrites the ad-hoc SQL over the referenced RLS-governed tables, fixes the cache-key identity, surfaces a non-disclosive denial to the user, and records queryable enforcement evidence. Adds a CLI, a DB migration for the evidence table, frontend RLS badge / error surfacing, and adversarial tests. Addresses apache#33346
dosubot
Bot
added
authentication:row-level-security
|
Bito Automatic Review Skipped - Large PR |
github-actions
Bot
added
risk:db-migration
👷 Deploy Preview for superset-docs-preview processing.
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| from superset import db | ||
| from superset.connectors.sqla.models import SqlaTable | ||
| from superset.models.core import Database | ||
|
|
||
| _setup_metadata(session) | ||
| database = Database(database_name="d", sqlalchemy_uri="sqlite://") | ||
| physical = SqlaTable( | ||
| table_name="governed_orders", | ||
| schema="main", | ||
| database=database, | ||
| ) | ||
| virtual = SqlaTable( | ||
| table_name="my_saved_chart", | ||
| schema="main", | ||
| database=database, | ||
| sql="SELECT * FROM governed_orders", | ||
| ) | ||
| db.session.add_all([database, physical, virtual]) | ||
| db.session.flush() |
There was a problem hiding this comment.
Suggestion: The helper accepts an in-memory session but persists through the global db.session instead. Consequently, the objects are created in a different database/session than the one whose metadata was initialized, so these tests can fail with missing tables or exercise the application database rather than the supplied isolated session. Use the passed session consistently for both add_all and flush. [api mismatch]
Severity Level: Major ⚠️
- ❌ Virtual-dataset RLS regression tests can fail during fixture setup.
- ⚠️ Tests may exercise application metadata instead of isolated SQLite state.
- ⚠️ Five fallback-RLS test cases depend on this helper.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/security/test_fallback_rls_virtual_dataset.py
**Line:** 76:94
**Comment:**
*Api Mismatch: The helper accepts an in-memory `session` but persists through the global `db.session` instead. Consequently, the objects are created in a different database/session than the one whose metadata was initialized, so these tests can fail with missing tables or exercise the application database rather than the supplied isolated session. Use the passed `session` consistently for both `add_all` and `flush`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def _sqlite_session() -> Session: | ||
| """Real SQLite session backed by the evidence model's own metadata.""" | ||
| engine = create_engine("sqlite://", future=True) | ||
| RlsEnforcementEvidence.__table__.create(bind=engine) | ||
| factory = sessionmaker(bind=engine, future=True) | ||
| return factory() |
There was a problem hiding this comment.
Suggestion: The SQLite fixture creates the evidence table with the model's BigInteger primary key and relies on autoincrement. SQLite only auto-generates identifiers for an INTEGER PRIMARY KEY; this column is emitted as BIGINT, so inserts omit a required id, causing record_evidence to fail and swallow the error, leaving no rows for the evidence and hash-chain assertions. Use a SQLite-compatible integer primary key for this test table or explicitly provide identifiers. [type error]
Severity Level: Major ⚠️
- ❌ Evidence persistence tests fail to observe inserted rows.
- ❌ Hash-chain tests cannot create predecessor-linked records.
- ⚠️ Sink failures are swallowed, masking the SQLite fixture defect.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/security/test_rls_evidence_sink.py
**Line:** 50:55
**Comment:**
*Type Error: The SQLite fixture creates the evidence table with the model's `BigInteger` primary key and relies on autoincrement. SQLite only auto-generates identifiers for an `INTEGER PRIMARY KEY`; this column is emitted as `BIGINT`, so inserts omit a required `id`, causing `record_evidence` to fail and swallow the error, leaving no rows for the evidence and hash-chain assertions. Use a SQLite-compatible integer primary key for this test table or explicitly provide identifiers.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| engine = create_engine("sqlite://", future=True) | ||
| _create_table(engine) | ||
| factory = sessionmaker(bind=engine, future=True) | ||
| session: Session = factory() | ||
|
|
||
| ts = datetime(2026, 8, 7, 12, 0, 0, tzinfo=timezone.utc).replace(tzinfo=None) | ||
| row = RlsEnforcementEvidence( | ||
| ts=ts, | ||
| path="embedded_guest", | ||
| identity_handle="opaque-handle", | ||
| datasource_kind="query", | ||
| datasource_id=42, | ||
| outcome="applied", | ||
| applied_filter_count=2, | ||
| ) | ||
| session.add(row) | ||
| session.commit() |
There was a problem hiding this comment.
Suggestion: The test creates the model on SQLite even though its auto-incrementing primary key is declared as BigInteger; SQLite only auto-generates row IDs for an exact INTEGER PRIMARY KEY, so the subsequent insert can fail with a NOT NULL constraint error instead of round-tripping. Use a SQLite-compatible integer variant for this test or run the model insert against a database that supports the declared type. [type error]
Severity Level: Major ⚠️
- ❌ RLS evidence model tests fail during SQLite insertion.
- ⚠️ CI cannot validate evidence row persistence.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_rls_evidence.py
**Line:** 101:117
**Comment:**
*Type Error: The test creates the model on SQLite even though its auto-incrementing primary key is declared as `BigInteger`; SQLite only auto-generates row IDs for an exact `INTEGER PRIMARY KEY`, so the subsequent insert can fail with a NOT NULL constraint error instead of round-tripping. Use a SQLite-compatible integer variant for this test or run the model insert against a database that supports the declared type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| mocker.patch( | ||
| "flask_appbuilder.security.decorators.verify_jwt_in_request", | ||
| return_value=True, | ||
| ) | ||
| mocker.patch.object(security_manager, "is_item_public", return_value=False) | ||
| mocker.patch.object(security_manager, "has_access", return_value=False) | ||
|
|
||
| response = client.get(EVIDENCE_URL) | ||
|
|
||
| assert response.status_code in (401, 403) |
There was a problem hiding this comment.
Suggestion: This negative test never establishes an authenticated user: patching verify_jwt_in_request to return True does not populate the request's authentication context, and the plain test client remains anonymous. The endpoint can therefore return 401 before evaluating has_access, allowing the test to pass even if the dedicated permission check is broken. Authenticate a user with valid access to the application but without the evidence permission, then assert the permission denial. [security]
Severity Level: Major ⚠️
- ⚠️ Authorization test can pass without checking permissions.
- ❌ Dedicated evidence-permission regressions may go undetected.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/security/test_rls_evidence_api.py
**Line:** 159:168
**Comment:**
*Security: This negative test never establishes an authenticated user: patching `verify_jwt_in_request` to return `True` does not populate the request's authentication context, and the plain test client remains anonymous. The endpoint can therefore return 401 before evaluating `has_access`, allowing the test to pass even if the dedicated permission check is broken. Authenticate a user with valid access to the application but without the evidence permission, then assert the permission denial.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| predicates = collect_rls_predicates_for_sql( | ||
| sql, | ||
| database, | ||
| source.catalog, | ||
| schema, | ||
| ) | ||
| return bool(predicates) |
There was a problem hiding this comment.
Suggestion: The discovery command silently omits artifacts whose SQL cannot be parsed or whose database dialect is unsupported. collect_rls_predicates_for_sql() converts those failures into an empty list, while the enforcement gate denies such queries fail-closed, so the report falsely claims they are safe and is incomplete for migration readiness. Treat resolution failures as at-risk instead of relying only on bool(predicates). [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ `rls find-at-risk` produces incomplete migration-readiness results.
- ⚠️ Unparseable saved queries are falsely presented as safe.
- ⚠️ Operators cannot identify artifacts that will fail closed after rollout.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/rls.py
**Line:** 83:89
**Comment:**
*Incomplete Implementation: The discovery command silently omits artifacts whose SQL cannot be parsed or whose database dialect is unsupported. `collect_rls_predicates_for_sql()` converts those failures into an empty list, while the enforcement gate denies such queries fail-closed, so the report falsely claims they are safe and is incomplete for migration readiness. Treat resolution failures as at-risk instead of relying only on `bool(predicates)`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| from superset import security_manager | ||
|
|
||
| identity = security_manager.get_current_guest_user_if_guest() | ||
| decision = enforce(self, sql, identity, "get_query_str_extended") |
There was a problem hiding this comment.
Suggestion: The new enforcement call double-applies RLS for Query datasources: get_sqla_query() invokes get_from_clause(), which already calls apply_rls() on the virtual SQL, and this gate rewrites the resulting SQL again. This can duplicate predicates, break aliases or dialect-specific SQL, and report an inflated filter count. Apply enforcement at only one of these stages or skip already-rewritten Query SQL. [logic error]
Severity Level: Major ⚠️
- ❌ Governed ad-hoc SQL charts can fail during query construction.
- ⚠️ Rewritten SQL may contain duplicate tenant predicates.
- ⚠️ Dialect-specific virtual dataset queries may become invalid.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1543:1543
**Comment:**
*Logic Error: The new enforcement call double-applies RLS for `Query` datasources: `get_sqla_query()` invokes `get_from_clause()`, which already calls `apply_rls()` on the virtual SQL, and this gate rewrites the resulting SQL again. This can duplicate predicates, break aliases or dialect-specific SQL, and report an inflated filter count. Apply enforcement at only one of these stages or skip already-rewritten `Query` SQL.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| elif isinstance(datasource, SqlaTable): | ||
| # The SqlaTable path already has its row filters injected upstream; the | ||
| # gate verifies passage without double-applying, so the SQL is re-emitted | ||
| # unchanged. | ||
| decision = EnforcementDecision( | ||
| outcome=EnforcementOutcome.NOOP, sql=compiled_sql | ||
| ) |
There was a problem hiding this comment.
Suggestion: The SqlaTable branch always emits NOOP, even when upstream virtual-dataset or physical-dataset processing has already injected RLS. Because record_evidence intentionally persists only APPLIED and DENIED outcomes, every normal dataset render with RLS applied upstream is omitted from the evidence table. Return an applied decision, or otherwise record the upstream enforcement state at this chokepoint. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Normal dataset RLS renders lack enforcement evidence.
- ⚠️ Auditors cannot query complete RLS application history.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/rls_enforcement.py
**Line:** 144:150
**Comment:**
*Incomplete Implementation: The `SqlaTable` branch always emits `NOOP`, even when upstream virtual-dataset or physical-dataset processing has already injected RLS. Because `record_evidence` intentionally persists only `APPLIED` and `DENIED` outcomes, every normal dataset render with RLS applied upstream is omitted from the evidence table. Return an applied decision, or otherwise record the upstream enforcement state at this chokepoint.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # dynamic_sql — unrendered templating leaves the table set indeterminate. | ||
| if any(marker in compiled_sql for marker in _TEMPLATE_MARKERS): | ||
| return "dynamic_sql" |
There was a problem hiding this comment.
Suggestion: The raw substring check treats braces inside legitimate SQL literals or comments as unrendered Jinja. For example, a query selecting or filtering the literal {{ is classified as dynamic_sql and denied even though its table set is fully deterministic. Detect template syntax outside SQL literals/comments, or rely on the parser/template-rendering state instead of scanning the raw SQL text. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Valid SQL charts fail when literals contain Jinja delimiters.
- ⚠️ Unrelated comments can trigger security-policy errors.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/rls_enforcement.py
**Line:** 200:202
**Comment:**
*Incorrect Condition Logic: The raw substring check treats braces inside legitimate SQL literals or comments as unrendered Jinja. For example, a query selecting or filtering the literal `{{` is classified as `dynamic_sql` and denied even though its table set is fully deterministic. Detect template syntax outside SQL literals/comments, or rely on the parser/template-rendering state instead of scanning the raw SQL text.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # cannot parse/transform this engine's SQL, so injection can't be verified. | ||
| engine = database.db_engine_spec.engine | ||
| if engine not in SQLGLOT_DIALECTS: | ||
| return "unsupported_dialect" |
There was a problem hiding this comment.
Suggestion: The classifier denies every ad-hoc query whose engine is absent from SQLGLOT_DIALECTS before checking whether any RLS rule applies. This changes the documented no-RLS behavior: an otherwise ungoverned query on a supported SQL Lab database is rejected as unsupported_dialect instead of executing unchanged. Determine whether the query is governed first, or preserve the no-rule path for dialects that cannot require rewriting. [api mismatch]
Severity Level: Major ⚠️
- ❌ Ad-hoc charts fail on unmapped SQL Lab databases.
- ⚠️ Ungoverned queries lose documented no-op behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/rls_enforcement.py
**Line:** 205:208
**Comment:**
*Api Mismatch: The classifier denies every ad-hoc query whose engine is absent from `SQLGLOT_DIALECTS` before checking whether any RLS rule applies. This changes the documented no-RLS behavior: an otherwise ungoverned query on a supported SQL Lab database is rejected as `unsupported_dialect` instead of executing unchanged. Determine whether the query is governed first, or preserve the no-rule path for dialects that cannot require rewriting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if _hash_chain_enabled(): | ||
| prev_hash = _latest_chain_hash(session) | ||
| row.integrity_prev_hash = prev_hash | ||
| row.integrity_hash = compute_evidence_hash(row, prev_hash) | ||
|
|
||
| session.add(row) | ||
| session.commit() |
There was a problem hiding this comment.
Suggestion: When hash chaining is enabled, concurrent requests can both read the same latest hash before either commits its row. This creates multiple rows with the same predecessor, so the append-only chain forks and verify_evidence_chain rejects the resulting sequence. Serialize predecessor selection and insertion, or use a database lock/transactional sequencing mechanism. [race condition]
Severity Level: Major ⚠️
- ⚠️ Enabled evidence chains become unverifiable during concurrent renders.
- ⚠️ RLS audit integrity cannot distinguish races from tampering.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/rls_enforcement.py
**Line:** 526:532
**Comment:**
*Race Condition: When hash chaining is enabled, concurrent requests can both read the same latest hash before either commits its row. This creates multiple rows with the same predecessor, so the append-only chain forks and `verify_evidence_chain` rejects the resulting sequence. Serialize predecessor selection and insertion, or use a database lock/transactional sequencing mechanism.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix1 participant
SUMMARY
Addresses #33346.
Row-Level Security is enforced on the SQL Lab query path but bypassed when a chart is created from that same ad-hoc SQL, silently rendering unfiltered data (the issue reproduces with 3 filtered rows in SQL Lab vs. 125 unfiltered in the resulting chart). In embedded / multi-tenant deployments, where RLS is the tenant boundary, this is a cross-tenant disclosure, and it is silent — neither the restricted analyst nor an admin can detect the over-exposure from the UI.
The root cause is path divergence: charts built from SQL Lab use a Query datasource whose is_rls_supported is False and whose get_sqla_row_level_filters() was a stub returning [] (superset/models/helpers.py), and the RLS cache key was skipped for Query datasources (superset/security/manager.py). Superset already has two separate RLS mechanisms (SQL-rewriting in SQL Lab; WHERE-injection for charts) and the chart-from-SQL path fell between them.
This PR converges enforcement to a single, fail-closed, server-side gate (superset/security/rls_enforcement.py) that every ad-hoc-SQL datasource render is routed through. It rewrites the ad-hoc SQL over the referenced RLS-governed tables at query-build time (superset/utils/rls.py), so enforcement is path-independent; fails closed with a fixed, non-disclosive message when RLS cannot be safely resolved (the sensitive denial reason stays server-side); fixes the RLS cache-key identity so filtered and unfiltered results can no longer collide; records queryable enforcement evidence (new model + REST API + DB migration + CLI); and surfaces enforcement in the UI (an "RLS active" badge and a dedicated RLS error message). Behaviour is unchanged when no RLS applies.
WORTH NOTING
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A for the enforcement change (backend security fix). The UI adds an "RLS active" badge on charts backed by ad-hoc SQL over governed tables.
TESTING INSTRUCTIONS
pytest tests/unit_tests/security/test_rls_enforcement.py
tests/unit_tests/security/test_fail_closed_decision.py
tests/unit_tests/security/test_rls_cache_key_identity.py
tests/unit_tests/models/test_helpers_rls_gate.py
pytest tests/integration_tests/security/rls_cache_isolation_tests.py
tests/integration_tests/security/row_level_security_tests.py
New tests cover the guest/embedded variant, cache isolation, the fail-closed decision, non-disclosive denial, and a zero-impact baseline. Before merge: reproduce #33346 end-to-end (RLS rule → restricted user → SQL Lab filtered → Create chart → chart shows the same filtered rows, not the full set).