Guard check filter and row_filter against unsafe SQL (SELECT) by SaptarshiAcharyya99 · Pull Request #1303 · databrickslabs/dqx

SaptarshiAcharyya99 · 2026-07-04T07:28:38Z

Changes

A check's filter / row_filter accepted arbitrary SQL. These are meant to be simple row
predicates, but were compiled directly via F.expr(...) with no validation — so a filter
like id IN (SELECT ... FROM other_table) would run a subquery against another table with
the permissions of the process executing the checks. This is a risk in automated or
multi-tenant pipelines where check definitions come from a less-trusted source.

As discussed on the issue:

is_sql_query_safe gains an optional forbid_select flag (default False), so the
sql_query check — which requires SELECT — is unaffected.
New shared helper safe_filter_expr validates a filter (raising UnsafeSqlQueryError
on an unsafe one) and compiles it; used for both filter and row_filter.
All filter compile sites route through the helper. A tree-wide sweep found 12 sites:
manager.py (check.filter), check_funcs.py (9 row_filter sites), and one each in
anomaly/scoring_utils.py and geo/check_funcs.py — so no site can slip through, and a
future site added via the helper is protected automatically.
sql_expression is intentionally not keyword-blocked (subqueries can be legitimate);
it gets a docstring security note instead, per the issue.

Word-boundary matching means identifiers like select_flag / selected are not false-flagged.

Linked issues

Resolves #1089

Tests

Unit tests cover is_sql_query_safe(forbid_select=True) (including the no-false-positive
case) and safe_filter_expr raising on SELECT. End-to-end tests confirm a row-level check
and a dataset check each reject an unsafe filter through the real DQRuleManager, plus a
safe-filter-passes test.

Documentation and Demos

added/updated demos
added/updated docs
added/updated agent skills

Security note added to the filter field in quality_checks_definition.mdx (bullet + table)
and to the sql_expression docstring.

…hecks

CLAassistant · 2026-07-04T07:28:44Z

All committers have signed the CLA.

mwojtyczka · 2026-07-05T03:42:50Z

Suggestion: I'd keep SELECT allowed in filters rather than blocking it, and scope this PR to the destructive keywords.

Why keep SELECT:

Filters are authored by trusted operators — the same people who define and run the DQX checks. The only real risk from a subquery is a confused-deputy read (a filter reading data its author shouldn't, via the engine's identity). When the filter author and the engine operator are the same trusted principal — which is the case for check definitions — that risk doesn't materialize.
A SELECT subquery in a filter is genuinely more powerful and works today. It lets you express referential / dynamic filters inline (scope a check to rows that exist in a reference table, an allowlist sourced from a table, etc.). Blocking it removes real, working capability.

Why destructive keywords should still be blocked (and this stays a simple change):

A filter compiles to a boolean expression (F.expr(...) / df.filter("<str>")), so DELETE/DROP/INSERT/… can't parse as statements and can't execute anyway. Keeping is_sql_query_safe's existing block (delete, insert, update, drop, truncate, alter, create, replace, grant, revoke, merge, use, refresh, analyze, optimize, zorder) is the right, low-cost guarantee. So the change is just: block the destructive set, allow SELECT.

Tests should cover the destructive keywords:

Parametrize is_sql_query_safe over the full forbidden set, asserting each is rejected.
Add a filter-path test that a drop/delete filter is rejected (via safe_filter_expr and via apply_checks_by_metadata for a representative check).
Add a test asserting a SELECT/subquery filter is allowed and runs end-to-end.

Example of a SELECT filter that should keep working — only check rows whose customer_id is in a reference/allowlist table:

- criticality: error
  filter: "customer_id IN (SELECT customer_id FROM main.ref.active_customers)"
  check:
    function: is_not_null
    arguments:
      column: email

equivalently in code:

DQRowRule(
    check_func=check_funcs.is_not_null,
    column="email",
    criticality="error",
    filter="customer_id IN (SELECT customer_id FROM main.ref.active_customers)",
)

Could you also add an example like this — a filter that references another table — to the PR (and ideally the docs), so the intended "allow SELECT" behavior is demonstrated end-to-end?

mwojtyczka

Thanks for the PR! Let's keep SELECT allowed in filters rather than blocking it, and scope this PR to the destructive keywords only. Explanation in the previous comment.

mwojtyczka

Inline notes to accompany the summary comment above: keep SELECT in filters (trusted-operator authored, and subqueries are a working, powerful feature), while keeping the destructive-keyword block and testing it.

mwojtyczka · 2026-07-05T03:47:57Z

+    Raises:
+        UnsafeSqlQueryError: If the filter contains a forbidden statement such as SELECT.
+    """
+    if filter_expr and not is_sql_query_safe(filter_expr, forbid_select=True):


I'd not forbid SELECT for filters here. A subquery in a filter is a working, powerful feature (inline referential/dynamic filters), and check filters are authored by trusted operators — the same principals who run the engine — so the confused-deputy read risk doesn't apply. Suggest dropping forbid_select=True and keeping only the destructive-keyword block. If you want a safety valve for deployments that load untrusted filters, expose forbid_select as an opt-in (default off) rather than always-on.

mwojtyczka · 2026-07-05T03:47:57Z

            )
-        filter_condition = F.expr(row_filter) if row_filter else F.lit(True)
+        filter_condition = safe_filter_expr(row_filter)
        median, mad = _calculate_median_absolute_deviation(df, col_expr_str, row_filter)


This passes the raw row_filter string into _calculate_median_absolute_deviation, which applies it via df.filter(<str>) — bypassing safe_filter_expr entirely, so even the destructive-keyword guard doesn't cover this path (has_no_outliers). Route it through safe_filter_expr (or pass the already-built filter_condition) so validation is consistent across all checks.

Agree with passing filter_condition

mwojtyczka · 2026-07-05T03:47:57Z

@@ -258,6 +259,32 @@ def test_mixed_content():
    assert not is_sql_query_safe("WITH cte AS (UPDATE users SET x=1) SELECT * FROM cte")


+def test_select_blocked_when_forbid_select():


Two coverage asks: (1) parametrize is_sql_query_safe over the full destructive set (delete, insert, update, drop, truncate, alter, create, replace, grant, revoke, merge, use, refresh, analyze, optimize, zorder), asserting each is rejected — not just select. (2) Since the recommendation is to keep SELECT allowed, add a test that a SELECT/subquery filter is allowed and runs end-to-end (e.g. customer_id IN (SELECT customer_id FROM ...)), rather than asserting it's blocked.

SaptarshiAcharyya99 · 2026-07-05T07:39:57Z

Thanks for the detailed review — this makes sense and I agree with the direction. Happy to keep
SELECT allowed in filters and scope the guard to the destructive keywords.

Plan:

Allow SELECT in filters. Drop forbid_select=True in safe_filter_expr so filters use
the existing destructive-keyword block and subqueries work. I'll keep forbid_select on
is_sql_query_safe as an off-by-default opt-in, per your suggestion, for deployments that load
untrusted filters.
Fix the has_no_outliers bypass — good catch. _calculate_median_absolute_deviation
applies the raw row_filter string directly. I'll pass the already-built, validated
filter_condition into it instead, so that path is guarded like the rest.
Tests. is_sql_query_safe already parametrizes the full destructive set in
test_query_with_forbidden_commands; I'll add filter-path coverage on top — safe_filter_expr
and apply_checks_by_metadata reject a drop/delete filter — plus tests that a
SELECT/subquery filter is allowed (through safe_filter_expr and end-to-end), and flip
the existing SELECT assertions from blocked to allowed.
Docs. Update the filter note in quality_checks_definition.mdx and the sql_expression
docstring to reflect: destructive keywords blocked, SELECT/subqueries allowed. I'll add your
customer_id IN (SELECT customer_id FROM …) example so the intended behavior is demonstrated.

Pushing the updates shortly.

ghanse

Looking good. I agree with the previous feedback. Left a few additional comments on the documentation.

ghanse · 2026-07-07T17:46:33Z

   - `for_each_column`: (optional) list of column names or expressions for which the same check should be applied. When saving with `DQEngine.save_checks` to Delta or Lakebase tables, DQX stores such checks in compact format (one row per check, `for_each_column` preserved).
 - (optional) `name`: name of the check: autogenerated if not provided.
- (optional) `filter`: spark expression to filter the rows for which the check is applied (e.g. `"business_unit = 'Finance'"`). The check function will run only on the rows matching the filter condition. The condition can reference any column of the validated dataset, not only the one where you apply the check function. When using dataset-level checks, the filter condition is pushed down as `row_filter` to the check function and applied before aggregation, ensuring that the check operates only on the relevant subset of rows rather than on the aggregated results.
+- (optional) `filter`: spark expression to filter the rows for which the check is applied (e.g. `"business_unit = 'Finance'"`). The check function will run only on the rows matching the filter condition. The condition can reference any column of the validated dataset, not only the one where you apply the check function. When using dataset-level checks, the filter condition is pushed down as `row_filter` to the check function and applied before aggregation, ensuring that the check operates only on the relevant subset of rows rather than on the aggregated results. For security, `filter` and `row_filter` are restricted to simple predicates: they cannot contain a `SELECT` statement, and an unsafe filter raises an error. If you need arbitrary SQL (including subqueries), use the `sql_expression` check instead, and only accept check definitions from trusted sources in automated or multi-tenant pipelines.


Instead of saying "For security, filter and row_filter are restricted to simple predicates: they cannot contain a SELECT statement, and an unsafe filter raises an error.", I think we should just tell the user what is not allowed with a few examples:

"For security, filter and row_filter cannot contain unsafe DML keywords like (e.g. UPDATE, DELETE). An unsafe filter raises an error during execution.

ghanse · 2026-07-07T17:47:12Z

 | └─ `for_each_column`   | `array<string>`       | (Optional) List of columns for which the same check applies. Stored in compact format (one row per check). Manual tables can use compact or expanded (individual rules instead of for_each_column) format. |
 | └─ `arguments`         | `map<string, string>` | Keyword arguments for the check function. All parameters without a default must be present (or supplied via `for_each_column` for `column` / `columns`).  |
-| `filter`               | `string`              | (Optional) Spark SQL expression to filter rows to which the check is applied, e.g. `"business_unit = 'Finance'"`. The check function will run only on the rows matching the filter condition. The condition can reference any column of the validated dataset, not only the one where you apply the check function. When using dataset-level checks, the filter condition is pushed down as `row_filter` to the check function and applied before aggregation, ensuring that the check operates only on the relevant subset of rows rather than on the aggregated results. |
+| `filter`               | `string`              | (Optional) Spark SQL expression to filter rows to which the check is applied, e.g. `"business_unit = 'Finance'"`. The check function will run only on the rows matching the filter condition. The condition can reference any column of the validated dataset, not only the one where you apply the check function. When using dataset-level checks, the filter condition is pushed down as `row_filter` to the check function and applied before aggregation, ensuring that the check operates only on the relevant subset of rows rather than on the aggregated results. For security, `filter` and `row_filter` are restricted to simple predicates: they cannot contain a `SELECT` statement, and an unsafe filter raises an error. If you need arbitrary SQL (including subqueries), use the `sql_expression` check instead, and only accept check definitions from trusted sources in automated or multi-tenant pipelines.|


Same comment applies here.

ghanse · 2026-07-07T17:47:31Z

            )
-        filter_condition = F.expr(row_filter) if row_filter else F.lit(True)
+        filter_condition = safe_filter_expr(row_filter)
        median, mad = _calculate_median_absolute_deviation(df, col_expr_str, row_filter)


Agree with passing filter_condition

ghanse · 2026-07-07T17:47:46Z

+    Raises:
+        UnsafeSqlQueryError: If the filter contains a forbidden statement such as SELECT.
+    """
+    if filter_expr and not is_sql_query_safe(filter_expr, forbid_select=True):


…back)

SaptarshiAcharyya99 · 2026-07-14T18:16:12Z

Thanks @mwojtyczka , @ghanse — addressed in the latest commit:

Allow SELECT in filters. safe_filter_expr now uses the default (destructive-keyword)
block; SELECT/subqueries pass through. forbid_select stays on is_sql_query_safe as an
off-by-default opt-in.
has_no_outliers bypass fixed — _calculate_median_absolute_deviation now receives the
already-validated filter_condition instead of the raw string.
Tests: flipped the SELECT assertions to "allowed"; added a destructive-filter rejection
test and an allowed-SELECT test using the customer_id IN (SELECT ... FROM ref) example
(end-to-end at the check level; full cross-table execution would be an integration test).
Docs: rewrote the filter note (bullet + table) to state plainly that subqueries are
allowed with the reference-table example, and that destructive statements are rejected — and
removed the now-inaccurate SELECT line from the sql_expression docstring.

SaptarshiAcharyya99 added 6 commits June 30, 2026 20:02

Add forbid_select option to is_sql_query_safe

54ddb42

Add safe_filter_expr helper to validate and compile filter expressions

490b9c5

Route all filter and row_filter compile sites through safe_filter_expr

e4e09b7

Add end-to-end tests for unsafe filter rejection in row and dataset c…

064c747

…hecks

Document filter SELECT restriction and sql_expression security note

067a2c4

Apply formatting from make fmt

5db6eb2

SaptarshiAcharyya99 requested a review from a team as a code owner July 4, 2026 07:28

SaptarshiAcharyya99 requested review from tombonfert and removed request for a team July 4, 2026 07:28

Merge branch 'main' into feature/guard-check-filter

ad370a1

mwojtyczka requested changes Jul 5, 2026

View reviewed changes

mwojtyczka reviewed Jul 5, 2026

View reviewed changes

mwojtyczka added under-review This PR is currently being reviewed by one of DQX maintainers. needs-changes Changes required after review labels Jul 5, 2026

ghanse requested changes Jul 7, 2026

View reviewed changes

Merge branch 'main' into feature/guard-check-filter

71b52db

mwojtyczka mentioned this pull request Jul 12, 2026

Feature/validate upstream tables #1309

Open

8 tasks

Allow SELECT in filters, block only destructive keywords (review feed…

901e440

…back)

		@@ -258,6 +259,32 @@ def test_mixed_content():
		assert not is_sql_query_safe("WITH cte AS (UPDATE users SET x=1) SELECT * FROM cte")


		def test_select_blocked_when_forbid_select():

Uh oh!

Conversation

SaptarshiAcharyya99 commented Jul 4, 2026

Changes

Linked issues

Tests

Documentation and Demos

Uh oh!

CLAassistant commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwojtyczka commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwojtyczka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwojtyczka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaptarshiAcharyya99 commented Jul 5, 2026

Uh oh!

ghanse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaptarshiAcharyya99 commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Jul 4, 2026 •

edited

Loading

mwojtyczka commented Jul 5, 2026 •

edited

Loading

mwojtyczka left a comment •

edited

Loading