rusackas commented on code in PR #40662:
URL: https://github.com/apache/superset/pull/40662#discussion_r3384765462
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -69,6 +77,58 @@ def validate(self) -> None:
)
security_manager.raise_for_access(database=self._database)
+ def _apply_sql_security(self, sql: str) -> str:
+ """Run the disallowed-function/table, DML and RLS controls against the
+ SQL to be estimated, mirroring ``sql_lab.execute_sql_statements``.
+
+ Returns the SQL with RLS predicates injected (when ``RLS_IN_SQLLAB`` is
+ enabled), so the cost estimate reflects the same constrained query the
+ user would actually be allowed to run.
+ """
+ db_engine_spec = self._database.db_engine_spec
+ parsed_script = SQLScript(sql, engine=db_engine_spec.engine)
+
+ disallowed_functions = app.config["DISALLOWED_SQL_FUNCTIONS"].get(
+ db_engine_spec.engine,
+ set(),
+ )
+ if disallowed_functions and parsed_script.check_functions_present(
+ disallowed_functions
+ ):
+ raise SupersetDisallowedSQLFunctionException(disallowed_functions)
+
+ disallowed_tables = app.config["DISALLOWED_SQL_TABLES"].get(
+ db_engine_spec.engine,
+ set(),
+ )
+ if disallowed_tables and
parsed_script.check_tables_present(disallowed_tables):
+ found_tables = set()
+ for statement in parsed_script.statements:
+ present = {table.table.lower() for table in statement.tables}
+ for table in disallowed_tables:
+ if table.lower() in present:
+ found_tables.add(table)
+ raise SupersetDisallowedSQLTableException(found_tables or
disallowed_tables)
+
+ if parsed_script.has_mutation() and not self._database.allow_dml:
+ raise SupersetDMLNotAllowedException()
+
+ if is_feature_enabled("RLS_IN_SQLLAB"):
+ # Resolve the default catalog/schema the same way the execution
path
+ # does (``SQLExecutor._prepare_scripts`` /
+ # ``sql_lab.execute_sql_statements``) before injecting RLS. Without
+ # this, unqualified table references can't be matched against
+ # datasets registered under the database's default schema, so the
+ # estimate would skip predicates the real query enforces — breaking
+ # the security parity this command exists to provide.
+ catalog = self._catalog or self._database.get_default_catalog()
+ schema = self._schema or
self._database.get_default_schema(catalog) or ""
Review Comment:
Good catch, and you're right that this was a remaining gap. Fixed in
38ea393: default-schema resolution for RLS now goes through
`get_default_schema_for_query` (mirroring `sql_lab.execute_sql_statements`)
instead of the static `get_default_schema`. That routes through the same
per-query engine gate as execution, so the Postgres `search_path` rejection
(and any other engine-specific per-query check) now fires on the estimate path
too, and RLS is resolved against the effective per-query schema. Added a unit
test pinning that the engine schema gate propagates and that RLS injection is
skipped once a query is rejected.
--
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]
