-
Notifications
You must be signed in to change notification settings - Fork 128
Guard check filter and row_filter against unsafe SQL #1303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
54ddb42
490b9c5
e4e09b7
064c747
067a2c4
5db6eb2
ad370a1
71b52db
901e440
3941ec1
4b5c69c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |||||
| from databricks.labs.dqx.utils import ( | ||||||
| get_column_name_or_alias, | ||||||
| is_sql_query_safe, | ||||||
| safe_filter_expr, | ||||||
| normalize_col_str, | ||||||
| get_columns_as_strings, | ||||||
| to_lowercase, | ||||||
|
|
@@ -430,6 +431,11 @@ def sql_expression( | |||||
|
|
||||||
| Args: | ||||||
| expression: SQL expression. Fail if expression evaluates to False, pass if it evaluates to True. | ||||||
| Security note: this parameter accepts arbitrary SQL and is evaluated as-is, so it may | ||||||
| include subqueries and run with the permissions of the process executing the checks. | ||||||
| Only use check definitions from trusted sources, especially in automated or multi-tenant | ||||||
| pipelines. Unlike *filter* and *row_filter*, SELECT is not blocked here because subqueries | ||||||
| can be legitimate in an SQL expression. | ||||||
| msg: optional message of the *Column* type, automatically generated if None | ||||||
| name: optional name of the resulting column, automatically generated if None | ||||||
| negate: if the condition should be negated (true) or not. For example, "col is not null" will mark null | ||||||
|
|
@@ -1258,7 +1264,7 @@ def apply(df: DataFrame) -> DataFrame: | |||||
| f"Column '{col_expr_str}' must be of numeric type to perform outlier detection using MAD method, " | ||||||
| f"but got type '{column_type.simpleString()}' instead." | ||||||
| ) | ||||||
| filter_condition = F.expr(row_filter) if row_filter else F.lit(True) | ||||||
| filter_condition = safe_filter_expr(row_filter) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: |
||||||
| median, mad = _calculate_median_absolute_deviation(df, col_expr_str, row_filter) | ||||||
|
mwojtyczka marked this conversation as resolved.
Outdated
|
||||||
| if median is not None and mad is not None: | ||||||
| median = float(median) | ||||||
|
|
@@ -1349,7 +1355,7 @@ def apply(df: DataFrame) -> DataFrame: | |||||
|
|
||||||
| filter_condition = F.lit(True) | ||||||
| if row_filter: | ||||||
| filter_condition = filter_condition & F.expr(row_filter) | ||||||
| filter_condition = filter_condition & safe_filter_expr(row_filter) | ||||||
|
|
||||||
| if nulls_distinct: | ||||||
| # All columns must be non-null | ||||||
|
|
@@ -1478,7 +1484,7 @@ def apply(df: DataFrame, spark: SparkSession, ref_dfs: dict[str, DataFrame]) -> | |||||
| ref_alias = f"__ref_{col_str_norm}_{unique_str}" | ||||||
| ref_df_distinct = ref_df.select(ref_col_expr.alias(ref_alias)).distinct() | ||||||
|
|
||||||
| filter_expr = F.expr(row_filter) if row_filter else F.lit(True) | ||||||
| filter_expr = safe_filter_expr(row_filter) | ||||||
|
|
||||||
| # col_expr.isNotNull() only filters rows in the single-column non-null-safe path; | ||||||
| # when col_expr is a struct (composite keys or null_safe=True), the struct is never NULL | ||||||
|
|
@@ -1595,7 +1601,7 @@ def _replace_template(sql: str, replacements: dict[str, str]) -> str: | |||||
| def apply(df: DataFrame, spark: SparkSession, ref_dfs: dict[str, DataFrame]) -> DataFrame: | ||||||
| filtered_df = df | ||||||
| if row_filter: | ||||||
| filtered_df = df.filter(F.expr(row_filter)) | ||||||
| filtered_df = df.filter(safe_filter_expr(row_filter)) | ||||||
|
|
||||||
| # since the check could be applied multiple times, the views created here must be unique | ||||||
| filtered_df.createOrReplaceTempView(unique_input_view) | ||||||
|
|
@@ -1968,7 +1974,7 @@ def apply(df: DataFrame) -> DataFrame: | |||||
| f"but got type '{time_col_type.simpleString()}' instead." | ||||||
| ) | ||||||
|
|
||||||
| filter_col = F.expr(row_filter) if row_filter else F.lit(True) | ||||||
| filter_col = filter_col = safe_filter_expr(row_filter) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| filtered_expr = F.when(filter_col, aggr_col_expr) if row_filter else aggr_col_expr | ||||||
| aggr_expr = _build_aggregate_expression(aggr_type, filtered_expr, aggr_params) | ||||||
|
|
||||||
|
|
@@ -2202,7 +2208,7 @@ def apply(df: DataFrame, spark: SparkSession, ref_dfs: dict[str, DataFrame]) -> | |||||
| skipped_columns = [col for col in df.columns if col not in compare_columns and col not in pk_column_names] | ||||||
|
|
||||||
| # apply filter before aliasing to avoid ambiguity | ||||||
| df = df.withColumn(filter_col, F.expr(row_filter) if row_filter else F.lit(True)) | ||||||
| df = df.withColumn(filter_col, safe_filter_expr(row_filter)) | ||||||
|
|
||||||
| df = df.alias("df") | ||||||
| ref_df = ref_df.alias("ref_df") | ||||||
|
|
@@ -2309,7 +2315,7 @@ def apply(df: DataFrame) -> DataFrame: | |||||
| # Build filter condition | ||||||
| filter_condition = F.lit(True) | ||||||
| if row_filter: | ||||||
| filter_condition = filter_condition & F.expr(row_filter) | ||||||
| filter_condition = filter_condition & safe_filter_expr(row_filter) | ||||||
|
|
||||||
| # Limit checking to be within the lookback window if needed | ||||||
| if lookback_windows is not None: | ||||||
|
|
@@ -3372,7 +3378,7 @@ def apply(df: DataFrame) -> DataFrame: | |||||
| Returns: | ||||||
| The DataFrame with additional condition and metric columns for aggregation validation. | ||||||
| """ | ||||||
| filter_col = F.expr(row_filter) if row_filter else F.lit(True) | ||||||
| filter_col = safe_filter_expr(row_filter) | ||||||
| filtered_expr = F.when(filter_col, aggr_col_expr) if row_filter else aggr_col_expr | ||||||
|
|
||||||
| # Build aggregation expression | ||||||
|
|
@@ -3890,7 +3896,7 @@ def _apply_dataset_level_sql_check( | |||||
| # - If row_filter is provided, only rows matching the filter get the condition value | ||||||
| # - Rows not matching the filter get None (consistent with row-level checks) | ||||||
| if row_filter: | ||||||
| filter_expr = F.expr(row_filter) | ||||||
| filter_expr = safe_filter_expr(row_filter) | ||||||
| result_df = df.withColumn( | ||||||
| unique_condition_column, F.when(filter_expr, F.lit(condition_value)).otherwise(F.lit(None)) | ||||||
| ) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
| import pyspark.sql.functions as F | ||
| from databricks.sdk import WorkspaceClient | ||
| from databricks.labs.blueprint.limiter import rate_limited | ||
| from databricks.labs.dqx.errors import InvalidParameterError | ||
| from databricks.labs.dqx.errors import InvalidParameterError, UnsafeSqlQueryError | ||
| from databricks.labs.dqx.table_manager import SparkTableDataProvider | ||
| from databricks.sdk.errors import NotFound | ||
|
|
||
|
|
@@ -214,7 +214,7 @@ def normalize_col_str(col_str: str) -> str: | |
| return re.sub(COLUMN_NORMALIZE_EXPRESSION, "_", col_str[:max_chars].lower()).rstrip("_") | ||
|
|
||
|
|
||
| def is_sql_query_safe(query: str) -> bool: | ||
| def is_sql_query_safe(query: str, forbid_select: bool = False) -> bool: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not needed anymore since we are not blocking select
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor (efficiency/consistency): this rebuilds |
||
| # Normalize the query by removing extra whitespace and converting to lowercase | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pre-existing issue: The blocklist isn't robust — no comment/literal handling. Normalization only collapses whitespace and lowercases; it doesn't strip SQL comments, so |
||
| normalized_query = re.sub(r"\s+", " ", query).strip().lower() | ||
|
|
||
|
|
@@ -237,9 +237,32 @@ def is_sql_query_safe(query: str) -> bool: | |
| "optimize", | ||
| "zorder", | ||
| ] | ||
| if forbid_select: | ||
| forbidden_statements = forbidden_statements + ["select"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed anymore |
||
| return not any(re.search(rf"\b{kw}\b", normalized_query) for kw in forbidden_statements) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pre-existing issue: False positives: the blocklist matches keywords inside string literals / values.
All of these compiled fine via |
||
|
|
||
|
|
||
| def safe_filter_expr(filter_expr: str | None) -> Column: | ||
| """Build a Spark column from a filter expression, rejecting unsafe SQL. | ||
|
|
||
| Validates the filter with *is_sql_query_safe* using *forbid_select=True* before | ||
| compiling it, since a filter must be a simple predicate and never a full query | ||
| (e.g. a subquery). Used for both check filters and *row_filter* parameters. | ||
|
|
||
| Args: | ||
| filter_expr: The filter predicate as a string, or None. | ||
|
|
||
| Returns: | ||
| The compiled filter column, or a literal true column when no filter is given. | ||
|
|
||
| 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): | ||
|
mwojtyczka marked this conversation as resolved.
Outdated
|
||
| raise UnsafeSqlQueryError(f"Unsafe filter expression: '{filter_expr}'") | ||
| return F.expr(filter_expr) if filter_expr else F.lit(True) | ||
|
|
||
|
|
||
| def safe_json_load(value: str): | ||
| """ | ||
| Safely load a JSON string, returning the original value if it fails to parse. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.