diff --git a/superset/commands/database/validate_sql.py b/superset/commands/database/validate_sql.py index d7ab40f882c..17de0062b20 100644 --- a/superset/commands/database/validate_sql.py +++ b/superset/commands/database/validate_sql.py @@ -21,6 +21,7 @@ from typing import Any, Optional from flask import current_app as app from flask_babel import gettext as __ +from superset import security_manager from superset.commands.base import BaseCommand from superset.commands.database.exceptions import ( DatabaseNotFoundError, @@ -69,6 +70,17 @@ class ValidateSQLCommand(BaseCommand): schema = self._properties.get("schema") template_params = self._properties.get("template_params") or {} + # Check access before rendering the Jinja template (mirrors the SQL + # Lab execute path). + security_manager.raise_for_access( + database=self._model, + sql=sql, + catalog=catalog, + schema=schema, + template_params=template_params, + force_dataset_match=True, + ) + try: # Render Jinja templates to handle template syntax before # validation. Note: The ENABLE_TEMPLATE_PROCESSING feature flag is diff --git a/superset/commands/sql_lab/estimate.py b/superset/commands/sql_lab/estimate.py index fd68e201c63..c73996f18e3 100644 --- a/superset/commands/sql_lab/estimate.py +++ b/superset/commands/sql_lab/estimate.py @@ -23,8 +23,9 @@ from flask import current_app as app from flask_babel import gettext as __ from jinja2.exceptions import TemplateError -from superset import db, is_feature_enabled, security_manager +from superset import is_feature_enabled, security_manager from superset.commands.base import BaseCommand +from superset.daos.database import DatabaseDAO from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( SupersetDisallowedSQLFunctionException, @@ -66,8 +67,10 @@ class QueryEstimationCommand(BaseCommand): self._catalog = params.get("catalog") def validate(self) -> None: - self._database = db.session.query(Database).get(self._database_id) - if not self._database: + # Load the database through the DAO so ``DatabaseFilter`` scopes + # visibility the same way it does on the SQL Lab execution path. + database = DatabaseDAO.find_by_id(self._database_id) + if not database: raise SupersetErrorException( SupersetError( message=__("The database could not be found"), @@ -76,7 +79,17 @@ class QueryEstimationCommand(BaseCommand): ), status=404, ) - security_manager.raise_for_access(database=self._database) + self._database = database + # Pass the SQL so table-level authorization runs, mirroring the SQL + # Lab execution path. Runs before Jinja templating in ``run()``. + security_manager.raise_for_access( + database=self._database, + sql=self._sql, + catalog=self._catalog, + schema=self._schema or None, + template_params=self._template_params, + force_dataset_match=True, + ) def _apply_sql_security(self, sql: str) -> str: """Run the disallowed-function/table, DML and RLS controls against the @@ -150,6 +163,7 @@ class QueryEstimationCommand(BaseCommand): sql = self._sql if self._template_params: + # Access is already checked in validate() before any rendering. template_processor = get_template_processor(self._database) try: sql = template_processor.process_template(sql, **self._template_params) diff --git a/superset/db_engine_specs/postgres.py b/superset/db_engine_specs/postgres.py index 870718268b6..53cc97e597b 100644 --- a/superset/db_engine_specs/postgres.py +++ b/superset/db_engine_specs/postgres.py @@ -641,12 +641,11 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec): """ Return the default schema for a given query. - This method simply uses the parent method after checking that there are no - malicious path setting in the query. + This method simply uses the parent method after checking that the query + cannot rebind the schema used to resolve unqualified table names. """ script = process_jinja_sql(query.sql, database, template_params).script - settings = script.get_settings() - if "search_path" in settings: + if script.changes_default_schema(): raise SupersetSecurityException( SupersetError( error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 04fc082f540..b902f184d14 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -915,6 +915,18 @@ class BaseTemplateProcessor: """ return self._context.copy() + def get_template_context(self, **kwargs: Any) -> dict[str, Any]: + """ + Build the validated context used to render a template. + + Split out from ``process_template`` so that validation paths which + render a pre-parsed template (``superset.sql.parse.process_jinja_sql``) + use exactly the same context as execution, keeping the validated SQL + identical to the executed SQL. + """ + kwargs.update(self._context) + return validate_template_context(self.engine, kwargs) + def process_template(self, sql: str, **kwargs: Any) -> str: """Processes a sql template @@ -984,8 +996,7 @@ class BaseTemplateProcessor: raise SupersetTemplateException(message) from ex - kwargs.update(self._context) - context = validate_template_context(self.engine, kwargs) + context = self.get_template_context(**kwargs) try: return template.render(context) @@ -1133,27 +1144,21 @@ class HiveTemplateProcessor(PrestoTemplateProcessor): class SparkTemplateProcessor(HiveTemplateProcessor): engine = "spark" - def process_template(self, sql: str, **kwargs: Any) -> str: - template = self.env.from_string(sql) - kwargs.update(self._context) - + def get_template_context(self, **kwargs: Any) -> dict[str, Any]: + context = super().get_template_context(**kwargs) # Backwards compatibility if migrating from Hive. - context = validate_template_context(self.engine, kwargs) context["hive"] = context["spark"] - return template.render(context) + return context class TrinoTemplateProcessor(PrestoTemplateProcessor): engine = "trino" - def process_template(self, sql: str, **kwargs: Any) -> str: - template = self.env.from_string(sql) - kwargs.update(self._context) - + def get_template_context(self, **kwargs: Any) -> dict[str, Any]: + context = super().get_template_context(**kwargs) # Backwards compatibility if migrating from Presto. - context = validate_template_context(self.engine, kwargs) context["presto"] = context["trino"] - return template.render(context) + return context DEFAULT_PROCESSORS = { diff --git a/superset/security/manager.py b/superset/security/manager.py index 233ee32ca38..67bdcf6f041 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3971,6 +3971,21 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods if query in self.session: self.session.expunge(query) + # When only ``database`` is provided, enforce database-level access + # here so the call is not a no-op. + if database and not (table or query): + if not self.can_access_database(database): + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR, + message=_( + "You need access to the following database: %(name)s", + name=database.database_name, + ), + level=ErrorLevel.WARNING, + ) + ) + if database and table or query: if query: # Type narrow: only SQL Lab Query objects have .database attribute @@ -4053,6 +4068,24 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods level=ErrorLevel.ERROR, ) ) + # Statements that rebind how unqualified table names resolve + # (``USE``, ``SET SCHEMA``, or a ``search_path`` change) make + # the qualification below diverge from what the engine uses at + # execution time, so reject them regardless of engine. + if force_dataset_match and parse_result.script.changes_default_schema(): + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, + message=_( + "SQL Lab cannot authorise a script that " + "changes the schema used to resolve " + "unqualified table names (e.g. USE or " + "search_path changes). Qualify tables " + "explicitly instead." + ), + level=ErrorLevel.ERROR, + ) + ) tables = { table_.qualify( catalog=query.catalog or default_catalog, diff --git a/superset/sql/parse.py b/superset/sql/parse.py index 37783e4a73a..c194c4e63ef 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -616,6 +616,18 @@ class BaseSQLStatement(Generic[InternalRepresentation]): """ return False + def changes_default_schema(self) -> bool: + """ + Check if the statement changes the schema used to resolve unqualified + table names. + + Defaults to ``False``; engines whose statements can rebind unqualified + schema resolution override this. + + :return: True if the statement rebinds default schema resolution + """ + return False + def get_disallowed_tables( self, tables: set[str], @@ -751,26 +763,24 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): } ) - # PostgreSQL constructs that sqlglot represents as an opaque ``exp.Command`` - # (no structured AST). Each can mutate server state or wrap a DML body that - # would otherwise be detected by node-type matching. Used by - # ``is_mutating()``. - _POSTGRES_MUTATING_COMMAND_NAMES: frozenset[str] = frozenset( + # Constructs that sqlglot represents as an opaque ``exp.Command`` (no + # structured AST). Each can mutate server state or wrap a DML body that + # would otherwise be detected by node-type matching. The head keywords + # are not engine-specific (MySQL ``CALL`` / ``LOAD DATA INFILE`` and + # MSSQL ``EXEC`` reach the same ``exp.Command`` fallback as their + # PostgreSQL counterparts), so ``is_mutating()`` applies this list for + # every dialect: an opaque command with one of these heads is treated as + # mutating. + _MUTATING_COMMAND_NAMES: frozenset[str] = frozenset( { "DO", # PL/pgSQL anonymous block "PREPARE", # PREPARE u AS UPDATE ... ; EXECUTE u "EXECUTE", # body is the prepared DML + "EXEC", # MSSQL spelling of EXECUTE; the procedure body may mutate "CALL", # procedure body may mutate "COPY", # server-side file ingest into a table "GRANT", "REVOKE", - # Only the command-fallback forms (e.g. SET ROLE / SET SESSION - # AUTHORIZATION, which change the effective user) reach here as an - # exp.Command. Structured `SET search_path = ...` / - # `SET statement_timeout = ...` parse as exp.Set and are NOT matched - # by this command-name path. - "SET", - "RESET", # RESET ROLE / RESET ALL reverts SET; same class as SET "REFRESH", # REFRESH MATERIALIZED VIEW "REINDEX", "VACUUM", @@ -783,7 +793,9 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): "CREATE", "ALTER", "DROP", - "LOAD", # LOAD '/path/lib.so' dlopens a shared library on the PG host + # MySQL LOAD DATA INFILE ingests server files into a table; + # PostgreSQL LOAD '/path/lib.so' dlopens a shared library. + "LOAD", # NOTE: `SHOW` is intentionally NOT included. It is a read (mutates # nothing), so classifying it as mutating would be wrong for every # is_mutating()/has_mutation() consumer (the commit decision, the @@ -794,6 +806,20 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): } ) + # PostgreSQL-only command-fallback heads. Only the command-fallback + # forms (e.g. SET ROLE / SET SESSION AUTHORIZATION, which change the + # effective user) reach here as an exp.Command; structured + # `SET search_path = ...` / `SET statement_timeout = ...` parse as + # exp.Set and are NOT matched by this path. On other dialects the `SET` + # fallback covers session variables (e.g. Hive `SET hivevar:x=1`), + # which do not mutate data, so these heads stay dialect-gated. + _POSTGRES_MUTATING_COMMAND_NAMES: frozenset[str] = frozenset( + { + "SET", + "RESET", # RESET ROLE / RESET ALL reverts SET; same class as SET + } + ) + # Dialects where `SELECT ... INTO target` is CTAS (creates a table, and so # mutates schema). Elsewhere the same syntax assigns into a variable and is # a read: Oracle PL/SQL `SELECT ... INTO v` and MySQL `SELECT ... INTO @v` @@ -926,7 +952,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): """ return isinstance(self._parsed, exp.Select) - def is_mutating(self) -> bool: + def is_mutating(self) -> bool: # noqa: C901 """ Check if the statement mutates data (DDL/DML). @@ -949,6 +975,14 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): exp.Revoke, # COMMENT ON TABLE/COLUMN/etc. writes to system catalog pg_description. exp.Comment, + # A bare COMMIT persists earlier writes on the same connection, so + # treat it as mutating. + exp.Commit, + # EXEC/EXECUTE invokes a stored procedure whose body is opaque; + # some dialects (e.g. MSSQL) parse it as this structured node + # rather than an opaque exp.Command, so treat it as mutating here + # too. + exp.Execute, ) if self._parsed.find(*mutating_nodes): @@ -986,37 +1020,76 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): ): return True - # depending on the dialect (Oracle, MS SQL) the `ALTER` is parsed as a - # command, not an expression - check at root level - if isinstance(self._parsed, exp.Command) and self._parsed.name == "ALTER": - return True # pragma: no cover + # Statements that sqlglot cannot model parse as an opaque + # `exp.Command`. The `.name` attribute on `exp.Command` preserves + # the source-case of the head keyword (so `create extension ...` + # would yield `'create'`), which means the lookups must be + # case-insensitive. This also covers the dialects (Oracle, MS SQL) + # where `ALTER` itself is parsed as a command, not an expression. + if isinstance(self._parsed, exp.Command): + command_name = self._parsed.name.upper() - # PostgreSQL constructs that sqlglot represents as an opaque - # `exp.Command` rather than a structured AST. Each of these can mutate - # state or wrap a DML body that would otherwise be detected. The - # `.name` attribute on `exp.Command` preserves the source-case of the - # head keyword (so `create extension ...` would yield `'create'`), - # which means the set lookup must be case-insensitive. - if ( - self._dialect == Dialects.POSTGRES - and isinstance(self._parsed, exp.Command) - and self._parsed.name.upper() in self._POSTGRES_MUTATING_COMMAND_NAMES - ): - return True + if command_name in self._MUTATING_COMMAND_NAMES: + return True - # Postgres runs DMLs prefixed by `EXPLAIN ANALYZE`, see - # https://www.postgresql.org/docs/current/sql-explain.html - if ( - self._dialect == Dialects.POSTGRES - and isinstance(self._parsed, exp.Command) - and self._parsed.name == "EXPLAIN" - and self._parsed.expression.name.upper().startswith("ANALYZE ") - ): - analyzed_sql = self._parsed.expression.name[len("ANALYZE ") :] - return SQLStatement( - statement=analyzed_sql, - engine=self.engine, - ).is_mutating() + if ( + self._dialect == Dialects.POSTGRES + and command_name in self._POSTGRES_MUTATING_COMMAND_NAMES + ): + return True + + # `EXPLAIN ANALYZE ` executes the statement for real + # (PostgreSQL and MySQL both run the body), see + # https://www.postgresql.org/docs/current/sql-explain.html + # The flag may be spelled `ANALYSE`, be separated by any + # whitespace, or appear in a parenthesized option list such as + # `EXPLAIN (ANALYZE, BUFFERS) ...`, so the raw tail is + # normalized before the inner statement is classified. Anything + # that carries the flag but cannot be classified is treated as + # mutating. + if command_name == "EXPLAIN": + tail = ( + self._parsed.expression.name.strip() + if self._parsed.expression + else "" + ) + + # sqlglot preserves the raw tail text, comments included; + # strip leading comments so an option list hidden behind + # `/* ... */` or `-- ...` is still recognized. + while True: + if tail.startswith("/*") and "*/" in tail: + tail = tail.split("*/", 1)[1].lstrip() + elif tail.startswith("--"): + parts = tail.split("\n", 1) + tail = parts[1].lstrip() if len(parts) > 1 else "" + else: + break + + has_analyze = False + if tail.startswith("("): + options, _, tail = tail[1:].partition(")") + has_analyze = bool( + re.search(r"\b(ANALYZE|ANALYSE)\b", options, re.IGNORECASE) + ) + else: + while match := re.match( + r"(ANALYZE|ANALYSE|VERBOSE)\s+", tail, re.IGNORECASE + ): + if match.group(1).upper() != "VERBOSE": + has_analyze = True + tail = tail[match.end() :] + + if has_analyze: + if not (inner_sql := tail.strip()): + return True + try: + return SQLStatement( + statement=inner_sql, + engine=self.engine, + ).is_mutating() + except SupersetParseError: + return True return False @@ -1188,6 +1261,58 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): return bool(tokens) and tokens[0].strip('"').lower() == "search_path" return False + def changes_default_schema(self) -> bool: + """ + Return True if the statement rebinds default schema resolution. + + Covers ``USE`` statements (MySQL-, Doris- and Snowflake-family + engines) and ``SET [CURRENT] SCHEMA`` / ``SET CATALOG`` variants, in + addition to anything that changes the Postgres ``search_path``. + Unqualified table names in later statements on the same cursor then + resolve against a different schema. + """ + for use in self._parsed.find_all(exp.Use): + kind = use.args.get("kind") + # `USE WAREHOUSE ...` selects compute, not a namespace, and does + # not affect how table names resolve. + if kind and kind.name.upper() == "WAREHOUSE": + continue + return True + # `SET SCHEMA 'x'` / `SET CATALOG 'x'` rebind resolution through a + # structured setting rather than a search path. + rebinding_settings = { + "schema", + "current_schema", + "current schema", + "catalog", + } + if any( + key.strip('"').lower() in rebinding_settings for key in self.get_settings() + ): + return True + # A `set_config()` with a non-literal setting name may set + # `search_path` at runtime, so treat it as a schema change; literal + # names are handled by `changes_search_path`. + for func in self._parsed.find_all(exp.Anonymous): + if func.name.lower() == "set_config" and not ( + func.expressions and isinstance(func.expressions[0], exp.Literal) + ): + return True + # `SET SCHEMA` / `SET CATALOG` forms that fall back to an opaque + # exp.Command: match the leading setting name, mirroring + # `changes_search_path`. + parsed = self._parsed + if isinstance(parsed, exp.Command) and parsed.name.upper() == "SET": + tokens = str(parsed.expression).replace("=", " ").split() + while tokens and tokens[0].upper() in {"SESSION", "LOCAL", "CURRENT"}: + tokens.pop(0) + if tokens and tokens[0].strip('"').strip("'").lower() in { + "schema", + "catalog", + }: + return True + return self.changes_search_path() + def get_disallowed_tables( self, tables: set[str], @@ -1819,12 +1944,16 @@ class SQLScript: def has_unparseable_statement(self) -> bool: """ True if any statement in the script cannot be fully modeled as an - AST whose table references Superset can enumerate. This covers two - cases that must both fail closed under strict scoping: + AST whose table references Superset can enumerate. This covers the + following cases, which must all fail closed under strict scoping: * SQLGlot ``exp.Command`` nodes: statements sqlglot recognises but cannot fully parse (e.g. dynamic SQL inside a stored-procedure call); ``extract_tables_from_statement`` cannot see the tables. + * ``exp.Show`` statements with no extractable target (e.g. + ``SHOW TABLES FROM some_schema``): the statement reads database + metadata, but there is no table reference for the per-table check + to enforce against. * Non-sqlglot engines (e.g. Kusto KQL): the statement class does not produce a sqlglot AST at all and its ``_extract_tables_from_statement`` returns an empty set, so the @@ -1835,6 +1964,11 @@ class SQLScript: return True if isinstance(statement._parsed, exp.Command): # noqa: SLF001 return True + if ( + isinstance(statement._parsed, exp.Show) # noqa: SLF001 + and not statement.tables + ): + return True return False def get_settings(self) -> dict[str, str | bool]: @@ -1868,6 +2002,16 @@ class SQLScript: """ return any(statement.is_destructive() for statement in self.statements) + def changes_default_schema(self) -> bool: + """ + Check if any statement rebinds default schema resolution. + + :return: True if any statement changes the schema (``USE``, + ``SET SCHEMA``) or the Postgres ``search_path`` used to resolve + unqualified table names + """ + return any(statement.changes_default_schema() for statement in self.statements) + def optimize(self) -> SQLScript: """ Return optimized script. @@ -1986,6 +2130,31 @@ def extract_tables_from_statement( except (ParseError, SupersetParseError): return set() sources = pseudo_query.find_all(exp.Table) + elif isinstance(statement, exp.Show): + # Structured metadata statements (`SHOW CREATE TABLE foo.bar`, + # `SHOW COLUMNS FROM foo`, ...) reference their target via dedicated + # args rather than query sources, so build the table references + # explicitly. Statements with no extractable target (e.g. + # `SHOW TABLES FROM some_schema`) yield an empty set and are treated + # as unparseable for authorization purposes (see + # `SQLScript.has_unparseable_statement`). + show_tables = { + Table( + source.name, + source.db if source.db != "" else None, + source.catalog if source.catalog != "" else None, + ) + for source in statement.find_all(exp.Table) + } + if target := statement.args.get("target"): + db = statement.args.get("db") + show_tables.add( + Table( + target.name if isinstance(target, exp.Expression) else str(target), + db.name if isinstance(db, exp.Expression) else db, + ) + ) + return show_tables else: sources = [ source @@ -2071,6 +2240,17 @@ def remove_quotes(val: T) -> T: return val +# Jinja macros that execute statements against the analytical database when +# rendered; their table references are extracted before rendering, and the +# macros are stubbed out during a validation-time render. +PARTITION_MACRO_NAMES = ( + "first_latest_partition", + "latest_partition", + "latest_partitions", + "latest_sub_partition", +) + + def process_jinja_sql( sql: str, database: Database, template_params: Optional[dict[str, Any]] = None ) -> JinjaSQLResult: @@ -2091,10 +2271,13 @@ def process_jinja_sql( :returns: JinjaSQLResult containing the processed script and table references :raises SupersetSecurityException: If SQLGlot is unable to parse the SQL statement :raises jinja2.exceptions.TemplateError: If the Jinjafied SQL could not be rendered + :raises SupersetParseError: If a partition macro references a table that + cannot be determined statically """ from superset.jinja_context import ( # pylint: disable=import-outside-toplevel get_template_processor, + NoOpTemplateProcessor, ) processor = get_template_processor(database) @@ -2102,37 +2285,74 @@ def process_jinja_sql( tables = set() + def raise_for_unresolvable_macro() -> Any: + raise SupersetParseError( + sql, + database.db_engine_spec.engine, + message=( + "Unable to determine the table referenced by a partition " + "macro; use a single constant table reference" + ), + ) + for node in ast.find_all(nodes.Call): - if isinstance(node.node, nodes.Getattr) and node.node.attr in ( - "latest_partition", - "latest_sub_partition", + if ( + isinstance(node.node, nodes.Getattr) + and node.node.attr in PARTITION_MACRO_NAMES ): - # Try to extract the table referenced in the macro. + # Extract the table referenced in the macro. The reference must + # be statically evaluable; otherwise raise rather than render. try: + if len(node.args) != 1: + raise nodes.Impossible() tables.add( Table( *[ remove_quotes(part.strip()) for part in node.args[0].as_const().split(".")[::-1] - if len(node.args) == 1 ] ) ) except nodes.Impossible: - pass + raise_for_unresolvable_macro() # Replace the potentially problematic Jinja macro with some benign SQL. node.__class__ = nodes.TemplateData node.fields = nodes.TemplateData.fields node.data = "NULL" - # re-render template back into a string - code = processor.env.compile(ast) - template = Template.from_code(processor.env, code, globals=processor.env.globals) - rendered_sql = template.render(processor.get_context(), **(template_params or {})) + # Render the neutralized template once, using the same context + # ``process_template`` builds at execution time, so the validated SQL + # matches the executed SQL. A no-op processor runs the raw SQL at + # execution time, so validate that raw SQL directly. + if isinstance(processor, NoOpTemplateProcessor): + rendered_sql = processor.process_template(sql) + else: + code = processor.env.compile(ast) + template = Template.from_code( + processor.env, + code, + globals=processor.env.globals, + ) + # Replace live partition macros with stubs so a call that survives + # neutralization (e.g. via a dynamic attribute lookup) does not + # execute during this render. + context = processor.get_template_context(**(template_params or {})) + if (engine := getattr(processor, "engine", None)) and isinstance( + context.get(engine), dict + ): + context[engine] = { + key: ( + (lambda *args, **kwargs: raise_for_unresolvable_macro()) + if key in PARTITION_MACRO_NAMES + else value + ) + for key, value in context[engine].items() + } + rendered_sql = template.render(context) parsed_script = SQLScript( - processor.process_template(rendered_sql), + rendered_sql, engine=database.db_engine_spec.engine, ) for parsed_statement in parsed_script.statements: diff --git a/superset/sqllab/api.py b/superset/sqllab/api.py index 62e5a683cb3..e8f4362a711 100644 --- a/superset/sqllab/api.py +++ b/superset/sqllab/api.py @@ -258,6 +258,14 @@ class SqlLabRestApi(BaseSupersetApi): else template_params ) if template_params: + # Check access before rendering the Jinja + # template (mirrors the SQL Lab execute path). + security_manager.raise_for_access( + database=database, + sql=sql, + template_params=template_params, + force_dataset_match=True, + ) template_processor = get_template_processor( database=database ) diff --git a/superset/sqllab/sqllab_execution_context.py b/superset/sqllab/sqllab_execution_context.py index 0e579ede9b6..6e9bc7d0019 100644 --- a/superset/sqllab/sqllab_execution_context.py +++ b/superset/sqllab/sqllab_execution_context.py @@ -22,9 +22,12 @@ from dataclasses import dataclass from typing import Any, cast, TYPE_CHECKING from flask import g +from flask_babel import gettext as __ from sqlalchemy.orm.exc import DetachedInstanceError from superset import is_feature_enabled +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetErrorException from superset.models.sql_lab import Query from superset.sql.parse import CTASMethod from superset.utils import core as utils, json @@ -128,9 +131,45 @@ class SqlJsonExecutionContext: # pylint: disable=too-many-instance-attributes if self.catalog is None: self.catalog = database.get_default_catalog() if self.select_as_cta: + self._validate_ctas_is_allowed(database) schema_name = self._get_ctas_target_schema_name(database) self.create_table_as_select.target_schema_name = schema_name # type: ignore + def _validate_ctas_is_allowed(self, database: Database) -> None: + """ + Enforce the per-database CTAS/CVAS grants server-side. + + The database's ``allow_ctas``/``allow_cvas`` flags are checked at + submission, mirroring the ``allow_dml`` gate on the execution path. + """ + ctas = cast(CreateTableAsSelect, self.create_table_as_select) + if ctas.ctas_method == CTASMethod.TABLE and not database.allow_ctas: + raise SupersetErrorException( + SupersetError( + message=__( + "This database does not allow creating tables from " + "queries (CTAS). Please contact your administrator " + "for more assistance." + ), + error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ), + status=403, + ) + if ctas.ctas_method == CTASMethod.VIEW and not database.allow_cvas: + raise SupersetErrorException( + SupersetError( + message=__( + "This database does not allow creating views from " + "queries (CVAS). Please contact your administrator " + "for more assistance." + ), + error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ), + status=403, + ) + def _get_ctas_target_schema_name(self, database: Database) -> str | None: if database.force_ctas_schema: return database.force_ctas_schema diff --git a/tests/integration_tests/celery_tests.py b/tests/integration_tests/celery_tests.py index 565b4f0ead2..87198aec172 100644 --- a/tests/integration_tests/celery_tests.py +++ b/tests/integration_tests/celery_tests.py @@ -70,10 +70,26 @@ def get_query_by_id(id: int): @pytest.fixture(autouse=True, scope="module") def setup_sqllab(): + # These tests exercise CTAS/CVAS, which the example database must be + # granted to allow. Enable the grants for the duration of the module and + # restore the originals afterwards. + with app.app_context(): + example_db = get_example_database() + original_allow_ctas = example_db.allow_ctas + original_allow_cvas = example_db.allow_cvas + example_db.allow_ctas = True + example_db.allow_cvas = True + db.session.commit() + yield + # clean up after all tests are done # use a new app context with app.app_context(): + example_db = get_example_database() + example_db.allow_ctas = original_allow_ctas + example_db.allow_cvas = original_allow_cvas + db.session.commit() db.session.query(Query).delete() db.session.commit() for tbl in TMP_TABLES: diff --git a/tests/integration_tests/sql_lab/api_tests.py b/tests/integration_tests/sql_lab/api_tests.py index 4527112000c..b93db730c7f 100644 --- a/tests/integration_tests/sql_lab/api_tests.py +++ b/tests/integration_tests/sql_lab/api_tests.py @@ -262,8 +262,11 @@ class TestSqlLabApi(SupersetTestCase): return_value=formatter_response ) - with mock.patch("superset.commands.sql_lab.estimate.db") as mock_superset_db: - mock_superset_db.session.query().get.return_value = db_mock + with ( + mock.patch("superset.commands.sql_lab.estimate.DatabaseDAO") as mock_dao, + mock.patch("superset.security_manager.raise_for_access"), + ): + mock_dao.find_by_id.return_value = db_mock data = {"database_id": 1, "sql": "SELECT 1"} rv = self.client.post( diff --git a/tests/integration_tests/sql_lab/commands_tests.py b/tests/integration_tests/sql_lab/commands_tests.py index f82500c2374..a4163302130 100644 --- a/tests/integration_tests/sql_lab/commands_tests.py +++ b/tests/integration_tests/sql_lab/commands_tests.py @@ -49,8 +49,8 @@ class TestQueryEstimationCommand(SupersetTestCase): data: EstimateQueryCostSchema = schema.dump(params) command = estimate.QueryEstimationCommand(data) - with mock.patch("superset.commands.sql_lab.estimate.db") as mock_superset_db: - mock_superset_db.session.query().get.return_value = None + with mock.patch("superset.commands.sql_lab.estimate.DatabaseDAO") as mock_dao: + mock_dao.find_by_id.return_value = None with pytest.raises(SupersetErrorException) as ex_info: command.validate() assert ( @@ -81,8 +81,11 @@ class TestQueryEstimationCommand(SupersetTestCase): db_mock.db_engine_spec.query_cost_formatter = mock.Mock(return_value=None) is_feature_enabled.return_value = False - with mock.patch("superset.commands.sql_lab.estimate.db") as mock_superset_db: - mock_superset_db.session.query().get.return_value = db_mock + with ( + mock.patch("superset.commands.sql_lab.estimate.DatabaseDAO") as mock_dao, + mock.patch("superset.security_manager.raise_for_access"), + ): + mock_dao.find_by_id.return_value = db_mock with pytest.raises(SupersetErrorException) as ex_info: command.run() assert ( @@ -107,8 +110,11 @@ class TestQueryEstimationCommand(SupersetTestCase): db_mock.db_engine_spec.estimate_query_cost = mock.Mock(return_value=100) db_mock.db_engine_spec.query_cost_formatter = mock.Mock(return_value=payload) - with mock.patch("superset.commands.sql_lab.estimate.db") as mock_superset_db: - mock_superset_db.session.query().get.return_value = db_mock + with ( + mock.patch("superset.commands.sql_lab.estimate.DatabaseDAO") as mock_dao, + mock.patch("superset.security_manager.raise_for_access"), + ): + mock_dao.find_by_id.return_value = db_mock result = command.run() assert result == payload diff --git a/tests/unit_tests/commands/databases/validate_sql_test.py b/tests/unit_tests/commands/databases/validate_sql_test.py index 46df10cecfd..247dfc64654 100644 --- a/tests/unit_tests/commands/databases/validate_sql_test.py +++ b/tests/unit_tests/commands/databases/validate_sql_test.py @@ -44,6 +44,11 @@ def mock_database(mocker: MockerFixture) -> MagicMock: "superset.commands.database.validate_sql.DatabaseDAO" ) DatabaseDAO.find_by_id.return_value = database + # Access validation runs before template processing; it has its own + # coverage, so keep it a no-op here. + mocker.patch( + "superset.commands.database.validate_sql.security_manager.raise_for_access" + ) return database diff --git a/tests/unit_tests/commands/sql_lab/test_estimate.py b/tests/unit_tests/commands/sql_lab/test_estimate.py index c59f2f7bc68..a34123af850 100644 --- a/tests/unit_tests/commands/sql_lab/test_estimate.py +++ b/tests/unit_tests/commands/sql_lab/test_estimate.py @@ -57,13 +57,13 @@ def _security_exception() -> SupersetSecurityException: @patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) -@patch("superset.commands.sql_lab.estimate.db") +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") def test_validate_raises_when_database_not_found( - mock_db: MagicMock, + mock_dao: MagicMock, mock_security_manager: MagicMock, ) -> None: """404 is raised before the access check when the database does not exist.""" - mock_db.session.query.return_value.get.return_value = None + mock_dao.find_by_id.return_value = None command = QueryEstimationCommand(_make_params()) with pytest.raises(SupersetErrorException) as exc_info: @@ -79,23 +79,21 @@ def test_validate_raises_when_database_not_found( @patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) -@patch("superset.commands.sql_lab.estimate.db") +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") def test_validate_raises_when_database_access_denied( - mock_db: MagicMock, + mock_dao: MagicMock, mock_security_manager: MagicMock, ) -> None: """SupersetSecurityException propagates when raise_for_access denies access.""" mock_database = MagicMock() - mock_db.session.query.return_value.get.return_value = mock_database + mock_dao.find_by_id.return_value = mock_database mock_security_manager.raise_for_access.side_effect = _security_exception() command = QueryEstimationCommand(_make_params()) with pytest.raises(SupersetSecurityException): command.validate() - mock_security_manager.raise_for_access.assert_called_once_with( - database=mock_database - ) + mock_security_manager.raise_for_access.assert_called_once() # --------------------------------------------------------------------------- @@ -104,22 +102,21 @@ def test_validate_raises_when_database_access_denied( @patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) -@patch("superset.commands.sql_lab.estimate.db") +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") def test_validate_succeeds_for_authorised_user( - mock_db: MagicMock, + mock_dao: MagicMock, mock_security_manager: MagicMock, ) -> None: """validate() completes without error when access is granted.""" mock_database = MagicMock() - mock_db.session.query.return_value.get.return_value = mock_database + mock_dao.find_by_id.return_value = mock_database mock_security_manager.raise_for_access.return_value = None command = QueryEstimationCommand(_make_params()) command.validate() # must not raise - mock_security_manager.raise_for_access.assert_called_once_with( - database=mock_database - ) + call_kwargs = mock_security_manager.raise_for_access.call_args.kwargs + assert call_kwargs["database"] is mock_database # --------------------------------------------------------------------------- @@ -128,15 +125,15 @@ def test_validate_succeeds_for_authorised_user( @patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) -@patch("superset.commands.sql_lab.estimate.db") +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") def test_raise_for_access_called_with_correct_database( - mock_db: MagicMock, + mock_dao: MagicMock, mock_security_manager: MagicMock, ) -> None: """The database object fetched from the session is passed to raise_for_access.""" mock_database = MagicMock() mock_database.id = 42 - mock_db.session.query.return_value.get.return_value = mock_database + mock_dao.find_by_id.return_value = mock_database mock_security_manager.raise_for_access.return_value = None command = QueryEstimationCommand(_make_params(database_id=42)) @@ -146,6 +143,39 @@ def test_raise_for_access_called_with_correct_database( assert call_kwargs["database"] is mock_database +# --------------------------------------------------------------------------- +# Regression: the SQL to be estimated must be authorized, not just the handle +# --------------------------------------------------------------------------- + + +@patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") +def test_validate_authorizes_the_sql_to_be_estimated( + mock_dao: MagicMock, + mock_security_manager: MagicMock, +) -> None: + """ + ``raise_for_access`` must receive the SQL so table-level authorization + runs; a bare ``database=`` argument matches no branch and checks nothing. + """ + mock_database = MagicMock() + mock_dao.find_by_id.return_value = mock_database + + command = QueryEstimationCommand( + _make_params(sql="SELECT * FROM secret_table", schema="main") + ) + command.validate() + + mock_security_manager.raise_for_access.assert_called_once_with( + database=mock_database, + sql="SELECT * FROM secret_table", + catalog=None, + schema="main", + template_params={}, + force_dataset_match=True, + ) + + # --------------------------------------------------------------------------- # SQL security controls applied on the estimate path (parity with executor) # --------------------------------------------------------------------------- @@ -386,9 +416,9 @@ def test_apply_sql_security_propagates_engine_schema_gate( @patch("superset.commands.sql_lab.estimate.get_template_processor") @patch("superset.commands.sql_lab.estimate.security_manager", new_callable=MagicMock) -@patch("superset.commands.sql_lab.estimate.db") +@patch("superset.commands.sql_lab.estimate.DatabaseDAO") def test_run_wraps_raw_jinja_undefined_error( - mock_db: MagicMock, + mock_dao: MagicMock, mock_security_manager: MagicMock, mock_get_template_processor: MagicMock, ) -> None: @@ -401,7 +431,7 @@ def test_run_wraps_raw_jinja_undefined_error( from jinja2.exceptions import UndefinedError mock_database = MagicMock() - mock_db.session.query.return_value.get.return_value = mock_database + mock_dao.find_by_id.return_value = mock_database mock_security_manager.raise_for_access.return_value = None mock_get_template_processor.return_value.process_template.side_effect = ( UndefinedError("'foo' is undefined") diff --git a/tests/unit_tests/db_engine_specs/test_postgres.py b/tests/unit_tests/db_engine_specs/test_postgres.py index c1840d9fd71..56e2815549e 100644 --- a/tests/unit_tests/db_engine_specs/test_postgres.py +++ b/tests/unit_tests/db_engine_specs/test_postgres.py @@ -182,6 +182,27 @@ SELECT * FROM some_table; ) +def test_get_default_schema_for_query_set_config(mocker: MockerFixture) -> None: + """ + A ``set_config('search_path', ...)`` call rebinds unqualified-name + resolution on the shared cursor just like ``SET search_path``, so it + must be rejected too. + """ + database = mocker.MagicMock() + query = mocker.MagicMock() + query.schema = "foo" + query.sql = ( + "SELECT set_config('search_path', 'tenant_b', false); SELECT * FROM orders" + ) + + with pytest.raises(SupersetSecurityException) as excinfo: + spec.get_default_schema_for_query(database, query) + assert ( + str(excinfo.value) + == "Users are not allowed to set a search path for security reasons." + ) + + def test_adjust_engine_params() -> None: """ Test `adjust_engine_params`. diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index d7d940c9099..0e6b13f86ba 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -408,10 +408,29 @@ def test_extract_tables_illdefined() -> None: def test_extract_tables_show_tables_from() -> None: """ Test `SHOW TABLES FROM`. + + No individual table target is extractable, so the statement must be + flagged as unparseable for authorization purposes instead of passing + strict scoping with an empty table set. """ assert ( extract_tables_from_sql("SHOW TABLES FROM s1 like '%order%'", "mysql") == set() ) + assert SQLScript( + "SHOW TABLES FROM s1 like '%order%'", "mysql" + ).has_unparseable_statement + + +def test_extract_tables_show_create_table() -> None: + """ + Test `SHOW CREATE TABLE`. + + The target table must enter table-level authorization. + """ + assert extract_tables_from_sql("SHOW CREATE TABLE s1.t1", "mysql") == { + Table("t1", "s1") + } + assert not SQLScript("SHOW CREATE TABLE s1.t1", "mysql").has_unparseable_statement def test_format_show_tables() -> None: @@ -1589,6 +1608,44 @@ def test_is_mutating(sql: str, engine: str, expected: bool) -> None: assert SQLStatement(sql, engine).is_mutating() == expected +@pytest.mark.parametrize( + "sql, engine", + [ + # Opaque `exp.Command` fallbacks must fail closed on every dialect, + # not only PostgreSQL. + ("CALL evil_proc()", "mysql"), + ("LOAD '/tmp/x.so'", "postgres"), + ("EXEC dbo.evil_proc", "mssql"), + # The EXPLAIN ANALYZE unwrap must handle the parenthesized + # option-list, whitespace, alternate-spelling, and leading-comment + # forms: PostgreSQL executes the inner DML for all of them. + ("EXPLAIN (ANALYZE) UPDATE t SET x = 1", "postgresql"), + ("EXPLAIN (ANALYZE, BUFFERS) DELETE FROM t", "postgresql"), + ("EXPLAIN ANALYZE\nUPDATE t SET x = 1", "postgresql"), + ("EXPLAIN ANALYSE UPDATE t SET x = 1", "postgresql"), + ("EXPLAIN /* c */ (ANALYZE) UPDATE t SET x = 1", "postgresql"), + # A bare COMMIT persists every prior write on the connection even + # when the execution layer skips its own commit call. + ("COMMIT", "postgresql"), + ("COMMIT", "mysql"), + # Further EXPLAIN ANALYZE edge forms: a leading line comment before + # the option, a VERBOSE qualifier, an empty option list, and an + # inner statement that cannot be parsed all fail closed as mutating. + ("EXPLAIN --c\nANALYZE UPDATE t SET x = 1", "postgresql"), + ("EXPLAIN ANALYZE VERBOSE UPDATE t SET x = 1", "postgresql"), + ("EXPLAIN (ANALYZE)", "postgresql"), + ("EXPLAIN ANALYZE )))", "postgresql"), + ], +) +def test_is_mutating_fails_closed_on_gate_blind_spots(sql: str, engine: str) -> None: + """ + `is_mutating` must fail closed on statements that slip past node-type + matching: non-PostgreSQL command fallbacks, normalized `EXPLAIN ANALYZE` + variants, and structured `COMMIT`. + """ + assert SQLStatement(sql, engine).is_mutating() + + @pytest.mark.parametrize( "sql, expected", [ @@ -3481,6 +3538,7 @@ def test_sqlstatement_format_preserves_multi_arg_distinct(engine: str) -> None: assert "CASE WHEN" not in formatted +@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True) @pytest.mark.parametrize( "engine", [ @@ -3509,12 +3567,12 @@ def test_sqlstatement_format_preserves_multi_arg_distinct(engine: str) -> None: {Table(table="bar", schema="foo")}, ), ( - "latest_partition('foo.%s'|format(str('bar')))", - set(), + "latest_partitions('foo.bar')", + {Table(table="bar", schema="foo")}, ), ( - "latest_partition('foo.{}'.format('bar'))", - set(), + "first_latest_partition('foo.bar')", + {Table(table="bar", schema="foo")}, ), ], ) @@ -3533,6 +3591,42 @@ def test_extract_tables_from_jinja_sql( ) +@pytest.mark.parametrize( + "engine", + [ + "hive", + "presto", + "trino", + ], +) +@pytest.mark.parametrize( + "macro", + [ + "latest_partition('foo.%s'|format(str('bar')))", + "latest_partition('foo.{}'.format('bar'))", + "latest_partitions('foo.{}'.format('bar'))", + # A partition macro with the wrong number of arguments cannot be + # resolved to a single table, so it must also fail closed. + "latest_partition('foo.bar', 'extra')", + ], +) +def test_extract_tables_from_jinja_sql_fails_closed( + mocker: MockerFixture, + engine: str, + macro: str, +) -> None: + """ + A partition macro whose table reference cannot be evaluated statically + must fail closed, as the macro would otherwise execute against a table + that never entered the authorization check. + """ + with pytest.raises(SupersetParseError): + process_jinja_sql( + sql=f"'{{{{ {engine}.{macro} }}}}'", + database=mocker.MagicMock(backend=engine), + ) + + @with_feature_flags(ENABLE_TEMPLATE_PROCESSING=False) def test_extract_tables_from_jinja_sql_disabled(mocker: MockerFixture) -> None: """ @@ -3622,6 +3716,31 @@ def test_process_jinja_sql_template_params_parameter(mocker: MockerFixture) -> N assert result.tables == {Table("table_name")} +@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True) +def test_process_jinja_sql_renders_exactly_once(mocker: MockerFixture) -> None: + """ + The authorization path must validate exactly the SQL that executes. + + A template whose first render emits Jinja comment markers inside SQL + comments used to be rendered a second time, which stripped the markers + and everything between them from the validated SQL while the executed + SQL (rendered once) kept the extra statement text. + """ + database = mocker.MagicMock(backend="postgresql") + database.db_engine_spec.engine = "postgresql" + + result = process_jinja_sql( + sql=( + 'SELECT * FROM granted /*{{ "{#" }}*/ ' + 'UNION SELECT * FROM restricted /*{{ "#}" }}*/' + ), + database=database, + ) + + assert Table("restricted") in result.tables + assert Table("granted") in result.tables + + @pytest.mark.parametrize( "sql, engine, expected", [ @@ -4164,6 +4283,60 @@ def test_changes_search_path(sql: str, expected: bool) -> None: assert SQLStatement(sql, "postgresql").changes_search_path() == expected +@pytest.mark.parametrize( + "sql, engine, expected", + [ + # `USE` rebinds the schema for every later statement on the cursor. + ("USE tenant_b; SELECT * FROM orders", "mysql", True), + ("use `tenant_b`", "mysql", True), + ("USE SCHEMA tenant_b", "snowflake", True), + # Warehouse selection changes compute, not name resolution. + ("USE WAREHOUSE compute_wh", "snowflake", False), + # Search-path changes are schema rebinds too. + ("SET search_path = tenant_b", "postgresql", True), + ( + "SELECT set_config('search_path', 'tenant_b', false)", + "postgresql", + True, + ), + # A `set_config()` with a computed setting name fails closed. + ( + "SELECT set_config('search' || '_path', 'tenant_b', false)", + "postgresql", + True, + ), + # `SET SCHEMA` is an alias for a search-path rebind on Postgres and + # a schema rebind on DB2-family engines. + ("SET SCHEMA 'tenant_b'", "postgresql", True), + ("SELECT * FROM orders", "mysql", False), + ("SET statement_timeout = 10", "postgresql", False), + # A structured `SET current_schema = ...` rebinds resolution through + # a setting rather than a search path. + ("SET current_schema = foo", "postgresql", True), + # `SET CATALOG`/`SET SCHEMA` that fall back to an opaque command are + # schema rebinds, including the `CURRENT` spelling; an unrelated `SET` + # command (e.g. `SET ROLE`) is not. + ("SET CATALOG tenant_b", "postgresql", True), + ("SET CURRENT SCHEMA foo", "postgresql", True), + ("SET ROLE admin", "postgresql", False), + # A `set_config()` whose setting name is a column reference rather than + # a literal is treated conservatively as a schema change. + ("SELECT set_config(schema_col, 'tenant_b', false)", "postgresql", True), + # Engines without a sqlglot AST (e.g. Kusto KQL) do not rebind schema + # resolution through these forms. + ("print x = 1", "kustokql", False), + ], +) +def test_changes_default_schema(sql: str, engine: str, expected: bool) -> None: + """ + `changes_default_schema` detects statements that rebind unqualified-name + resolution (`USE`, `SET SCHEMA`, search-path changes) so the SQL Lab + authorization path can reject the script before qualifying tables with + the schema the user selected. + """ + assert SQLScript(sql, engine).changes_default_schema() == expected + + @pytest.mark.parametrize( "sql, denylist, expected", [ diff --git a/tests/unit_tests/sql_lab_execution_context.py b/tests/unit_tests/sql_lab_execution_context.py index 41374ec293b..dddff082f29 100644 --- a/tests/unit_tests/sql_lab_execution_context.py +++ b/tests/unit_tests/sql_lab_execution_context.py @@ -16,8 +16,12 @@ # under the License. # pylint: disable=import-outside-toplevel, invalid-name, unused-argument, too-many-locals +from unittest.mock import MagicMock + import pytest +from superset.errors import SupersetErrorType +from superset.exceptions import SupersetErrorException from superset.sql.parse import CTASMethod from superset.sqllab.sqllab_execution_context import ( CreateTableAsSelect, @@ -101,3 +105,45 @@ def test_create_table_as_select(): assert ctas.ctas_method == CTASMethod.TABLE assert ctas.target_schema_name == "public" assert ctas.target_table_name == "temp_table" + + +def test_set_database_rejects_ctas_when_database_disallows_it(query_params): + """ + ``allow_ctas`` must be enforced server-side at submission: the + ``select_as_cta``/``ctas_method`` payload fields are client-supplied. + """ + query_params["select_as_cta"] = True + query_params["ctas_method"] = "TABLE" + query_params["tmp_table_name"] = "tmp_target" + context = SqlJsonExecutionContext(query_params) + + database = MagicMock() + database.allow_ctas = False + + with pytest.raises(SupersetErrorException) as exc_info: + context.set_database(database) + + assert ( + exc_info.value.error.error_type == SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR + ) + + +def test_set_database_rejects_cvas_when_database_disallows_it(query_params): + """ + ``allow_cvas`` must be enforced server-side at submission, mirroring the + ``allow_ctas``/VIEW branch of ``_validate_ctas_is_allowed``. + """ + query_params["select_as_cta"] = True + query_params["ctas_method"] = "VIEW" + query_params["tmp_table_name"] = "tmp_target" + context = SqlJsonExecutionContext(query_params) + + database = MagicMock() + database.allow_cvas = False + + with pytest.raises(SupersetErrorException) as exc_info: + context.set_database(database) + + assert ( + exc_info.value.error.error_type == SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR + ) diff --git a/tests/unit_tests/sqllab/api_test.py b/tests/unit_tests/sqllab/api_test.py index 384e8cb5ddf..fc739748e61 100644 --- a/tests/unit_tests/sqllab/api_test.py +++ b/tests/unit_tests/sqllab/api_test.py @@ -17,9 +17,14 @@ from __future__ import annotations import re +from typing import Any from unittest.mock import MagicMock, patch from flask import Flask +from pytest_mock import MockerFixture + +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetSecurityException def _disposition_filename(form_filename: str | None) -> str: @@ -62,3 +67,44 @@ def test_streaming_csv_falls_back_when_filename_empty() -> None: assert filename.startswith("sqllab_abc123_") assert filename.endswith(".csv") + + +def test_format_sql_checks_access_before_rendering( + mocker: MockerFixture, + client: Any, + full_api_access: None, +) -> None: + """ + Access must be checked before Jinja rendering, as some Jinja macros + execute statements against the database upon rendering. + """ + database = mocker.MagicMock() + database.db_engine_spec.engine = "presto" + mocker.patch( + "superset.sqllab.api.DatabaseDAO.find_by_id", + return_value=database, + ) + get_template_processor = mocker.patch("superset.sqllab.api.get_template_processor") + raise_for_access = mocker.patch( + "superset.sqllab.api.security_manager.raise_for_access", + side_effect=SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, + message="You need access to the following tables: `s.t`", + level=ErrorLevel.ERROR, + ) + ), + ) + + response = client.post( + "/api/v1/sqllab/format_sql/", + json={ + "sql": "SELECT '{{ presto.latest_partition('s.t') }}'", + "database_id": 1, + "template_params": '{"foo": "bar"}', + }, + ) + + assert response.status_code == 403 + raise_for_access.assert_called_once() + get_template_processor.assert_not_called()