This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new f02f53df234 fix(sql): reject client-side file-transfer statements in
query execution (#44496)
f02f53df234 is described below
commit f02f53df2342f639ce4f93f277e82f1362c0b3e7
Author: Shaitan <[email protected]>
AuthorDate: Thu Oct 1 03:37:23 2026 +0100
fix(sql): reject client-side file-transfer statements in query execution
(#44496)
Co-authored-by: Claude Opus 4.8 <[email protected]>
Co-authored-by: Amin Ghadersohi <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
---
UPDATING.md | 9 +
superset/commands/report/alert.py | 12 +-
superset/commands/sql_lab/estimate.py | 10 +-
superset/exceptions.py | 18 ++
superset/sql/execution/executor.py | 13 +
superset/sql/parse.py | 333 ++++++++++++++++++++-
superset/sql_lab.py | 5 +
superset/translations/messages.pot | 4 +
tests/unit_tests/commands/report/alert_test.py | 37 +++
tests/unit_tests/commands/sql_lab/test_estimate.py | 30 ++
tests/unit_tests/sql/execution/test_executor.py | 51 ++++
tests/unit_tests/sql/parse_tests.py | 207 +++++++++++++
tests/unit_tests/sql_lab_test.py | 39 +++
13 files changed, 756 insertions(+), 12 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 1b60597110f..da66b164f96 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -308,6 +308,15 @@ upgrading. See the two migrations' docstrings
(`superset/migrations/versions/
`..._00-01_3ce9a4572f8a_rename_deprecated_permissions_33272.py`) for the full
per-permission mapping and reasoning.
+- Snowflake stage file-management statements (`PUT`, `GET`, `REMOVE` and its
+ `RM` alias) are rejected in user-submitted SQL (SQL Lab, the cost-estimate
+ path and alert queries), regardless of the database's `allow_dml` setting.
+ `PUT`/`GET` perform file I/O on the host running the
+ query, and `REMOVE`/`RM` delete files within a stage; none of them read or
+ write table data, so `allow_dml` does not govern them. Deployments that ran
+ these through SQL Lab should manage stage files with Snowflake's own clients
+ instead. `LIST`/`LS`, which only enumerate staged files, are unaffected.
+
### MySQL metadata database now actually defaults to READ COMMITTED
Superset has always *intended* to default the metadata-database isolation
diff --git a/superset/commands/report/alert.py
b/superset/commands/report/alert.py
index a8927ad22f7..8bf8f1e2a07 100644
--- a/superset/commands/report/alert.py
+++ b/superset/commands/report/alert.py
@@ -186,12 +186,22 @@ class AlertCommand(BaseCommand):
def _validate_rendered_sql(self, rendered_sql: str) -> None:
"""
Enforce SQL-level constraints on the rendered alert query: a single
- statement, and no mutations unless the database allows DML.
+ statement, no client-side file transfer, and no mutations unless the
+ database allows DML.
"""
database = self._report_schedule.database
script = SQLScript(rendered_sql, engine=database.backend)
if len(script.statements) != 1:
raise AlertQueryError(message=_("Alert query must be a single
statement"))
+ # Rejected regardless of `allow_dml`: these do host file I/O, not DML.
+ if commands := script.get_client_file_transfer_commands():
+ raise AlertQueryError(
+ message=_(
+ "Alert query must not contain the file-transfer "
+ "command(s): %(commands)s",
+ commands=", ".join(commands),
+ )
+ )
if script.has_mutation() and not database.allow_dml:
raise AlertQueryError(message=_("Alert query must be read-only"))
diff --git a/superset/commands/sql_lab/estimate.py
b/superset/commands/sql_lab/estimate.py
index 2a5963169e7..abd3e137fd4 100644
--- a/superset/commands/sql_lab/estimate.py
+++ b/superset/commands/sql_lab/estimate.py
@@ -29,6 +29,7 @@ from superset.daos.database import DatabaseDAO
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import (
OAuth2RedirectError,
+ SupersetDisallowedClientFileTransferException,
SupersetDisallowedSQLFunctionException,
SupersetDisallowedSQLTableException,
SupersetDMLNotAllowedException,
@@ -94,8 +95,9 @@ class QueryEstimationCommand(BaseCommand):
)
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``.
+ """Run the disallowed-function/table, file-transfer, 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
@@ -148,6 +150,10 @@ class QueryEstimationCommand(BaseCommand):
if found_tables:
raise SupersetDisallowedSQLTableException(found_tables)
+ # Rejected regardless of `allow_dml`: these do host file I/O, not DML.
+ if file_transfer_commands :=
parsed_script.get_client_file_transfer_commands():
+ raise
SupersetDisallowedClientFileTransferException(file_transfer_commands)
+
if parsed_script.has_mutation() and not self._database.allow_dml:
raise SupersetDMLNotAllowedException()
diff --git a/superset/exceptions.py b/superset/exceptions.py
index 86d950da3a7..9280d12ca36 100644
--- a/superset/exceptions.py
+++ b/superset/exceptions.py
@@ -492,6 +492,24 @@ class
SupersetDisallowedSQLTableException(SupersetErrorException):
)
+class SupersetDisallowedClientFileTransferException(SupersetErrorException):
+ """
+ Client-side file-transfer command found in SQL statement
+ """
+
+ def __init__(self, commands: list[str]):
+ super().__init__(
+ SupersetError(
+ message=(
+ "SQL statement contains disallowed client-side "
+ f"file-transfer command(s): {', '.join(commands)}"
+ ),
+ error_type=SupersetErrorType.SYNTAX_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
class AcquireDistributedLockFailedException(Exception): # noqa: N818
"""
Exception to signalize failure to acquire lock.
diff --git a/superset/sql/execution/executor.py
b/superset/sql/execution/executor.py
index e357d396216..bc235fda30e 100644
--- a/superset/sql/execution/executor.py
+++ b/superset/sql/execution/executor.py
@@ -604,6 +604,19 @@ class SQLExecutor:
)
)
+ # Rejected regardless of `allow_dml`: these do host file I/O, not DML.
+ if file_transfer_commands :=
script.get_client_file_transfer_commands():
+ raise SupersetSecurityException(
+ SupersetError(
+ message=(
+ "Disallowed client-side file-transfer command(s): "
+ f"{', '.join(file_transfer_commands)}"
+ ),
+ error_type=SupersetErrorType.INVALID_SQL_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
# Check DML permission
if script.has_mutation() and not self.database.allow_dml:
raise SupersetSecurityException(
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index 667b818e94c..5028f8dab05 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -24,7 +24,7 @@ import re
import urllib.parse
from collections.abc import Callable
from dataclasses import dataclass
-from functools import lru_cache
+from functools import cached_property, lru_cache
from typing import Any, Generic, Optional, TYPE_CHECKING, TypeVar
import sqlglot
@@ -37,6 +37,7 @@ from sqlglot.dialects.dialect import (
DialectType,
NormalizationStrategy,
)
+from sqlglot.dialects.mysql import MySQL
from sqlglot.dialects.singlestore import SingleStore
from sqlglot.errors import OptimizeError, ParseError
from sqlglot.generator import Generator
@@ -652,6 +653,16 @@ class BaseSQLStatement(Generic[InternalRepresentation]):
"""
raise NotImplementedError()
+ def get_client_file_transfer_command(self) -> str | None:
+ """
+ Return the client-side file-transfer command head, if this is one.
+
+ Defaults to ``None``; engines that have such commands override this.
+
+ :return: The uppercased command head (e.g. ``"PUT"``), else ``None``.
+ """
+ return None
+
def optimize(self) -> BaseSQLStatement[InternalRepresentation]:
"""
Return optimized statement.
@@ -888,6 +899,97 @@ def _find_last_token_node(node: exp.Expression) ->
exp.Expression:
return node
+# Alternation ordered so a quoted region is consumed whole before either
+# comment form can match inside it, keeping a `--` or `/*` that is merely
+# part of a literal from truncating the text after it. Each of the two
+# regions that scan forward for a closing delimiter -- the dollar-quoted
+# literal and the block comment -- needs a trailing "runs to end"
+# alternative for the unterminated case: without one the lazy `.*?` rescans
+# to end of text from every opener in turn, which is quadratic on input a
+# user controls. The unterminated literal stays inside the `literal` group
+# so it is preserved rather than blanked, since text a gate would match must
+# not disappear.
+_COMMENT_RE_TEMPLATE = r"""
+ (?P<literal>
+ {single_quoted} # single-quoted string
+ | {double_quoted} # double-quoted identifier
+ | \$(?P<tag>\w*)\$.*?\$(?P=tag)\$ # dollar-quoted string
+ | \$\w*\$.* # unterminated: runs to end
+ )
+ | {line_comment}[^\n]* # line comment
+ | /\*.*?\*/ # block comment
+ | /\*.* # unterminated: runs to end
+ """
+
+
+def _quoted_literal_pattern(quote: str, backslash_escapes: bool) -> str:
+ """
+ Build the pattern matching one quoted region, delimiters included.
+
+ The doubled delimiter (``'it''s'``) always escapes. A backslash only
+ escapes on the dialects that say so, and where it does it must be
+ recognised: without it the scan ends the literal at the escaped quote of
+ ``'it\\'s -- x'``, reads the rest as code, and lets the ``--`` blank out
+ text the server actually runs, hiding it from every gate that scans here.
+
+ The alternatives are kept mutually exclusive -- the negated class drops
+ the escape character rather than overlapping with it -- so exactly one
+ branch can start at any position. An overlapping form would let a run of
+ backslashes with no closing quote backtrack exponentially, on text a user
+ controls.
+
+ :param quote: The delimiter character
+ :param backslash_escapes: Whether a backslash escapes on this dialect
+ :return: The pattern for one quoted region
+ """
+ if backslash_escapes:
+ return rf"{quote}(?:[^{quote}\\]|\\.|{quote}{quote})*{quote}"
+ return rf"{quote}(?:[^{quote}]|{quote}{quote})*{quote}"
+
+
+@lru_cache(maxsize=None)
+def _comment_re_for(dialect: Dialects | None) -> re.Pattern[str]:
+ """
+ Return the comment pattern a dialect's raw statement text is lexed with.
+
+ Both rules that vary are read off sqlglot rather than listed here, so a
+ dialect cannot be lexed with the wrong one because a list was not kept in
+ step: the MySQL family is identified by subclassing (``Doris``,
+ ``StarRocks`` and ``SingleStore`` all subclass ``MySQL``, as does this
+ repo's own ``Pinot``), and backslash escaping is read off the tokenizer.
+
+ MySQL-family engines only start a comment on ``--`` when whitespace (or
+ end of line) follows: ``1--2`` is arithmetic there. Stripping it as a
+ comment would delete text the server executes and blind every gate that
+ scans this body, so those dialects get the stricter rule.
+
+ An unknown dialect is lexed as if backslashes escape. That direction is
+ the safe one: treating an escape that is not one only ever folds more
+ text into a literal, which is preserved and still scanned, while missing
+ a real escape blanks executable text.
+
+ Cached because the answer depends only on the dialect, so a script would
+ otherwise re-derive it, and instantiate a sqlglot ``Dialect``, per
+ statement.
+
+ :param dialect: The statement's sqlglot dialect, or ``None`` when unknown
+ :return: The compiled comment pattern
+ """
+ dialect_cls = Dialect.get_or_raise(dialect) if dialect else None
+ backslash_escapes = (
+ dialect_cls is None or "\\" in
dialect_cls.tokenizer_class.STRING_ESCAPES
+ )
+ line_comment = r"--(?=[ \t\r\n]|$)" if isinstance(dialect_cls, MySQL) else
"--"
+ return re.compile(
+ _COMMENT_RE_TEMPLATE.format(
+ single_quoted=_quoted_literal_pattern("'", backslash_escapes),
+ double_quoted=_quoted_literal_pattern('"', backslash_escapes),
+ line_comment=line_comment,
+ ),
+ re.DOTALL | re.VERBOSE,
+ )
+
+
class SQLStatement(BaseSQLStatement[exp.Expression]):
"""
A SQL statement.
@@ -1003,6 +1105,41 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
}
)
+ # Stage file-management heads: ``PUT``/``GET`` move files between the host
+ # running the query and a stage, so they do host file I/O; ``REMOVE`` and
+ # its ``RM`` alias delete files within the stage. None read or write table
+ # data. This is Snowflake syntax but is matched on every dialect, since the
+ # heads are not valid statements elsewhere: a global list cannot reject a
+ # real query, while scoping it to one dialect would let
Snowflake-compatible
+ # engines through.
+ _CLIENT_FILE_TRANSFER_COMMAND_NAMES: frozenset[str] = frozenset(
+ {"PUT", "GET", "REMOVE", "RM"}
+ )
+
+ # The same heads inside a nested body, which cannot be re-parsed and so is
+ # matched on raw text. A stage (``@``) or a ``file://`` URL has to follow
+ # the head, since these words are ordinary identifiers elsewhere and a bare
+ # keyword match would flag a body merely selecting a column named
+ # ``remove``. A run of quotes is skipped rather than exactly one: a body
+ # nested inside a string literal carries its own quotes doubled
+ # (``EXECUTE IMMEDIATE 'PUT ''file://...'''``).
+ _CLIENT_FILE_TRANSFER_NESTED_BODY_RE = re.compile(
+ rf"\b({'|'.join(sorted(_CLIENT_FILE_TRANSFER_COMMAND_NAMES))})"
+ r"""\s+['"]*(?:@|file://)""",
+ re.IGNORECASE,
+ )
+
+ # Opening delimiter of a dollar-quoted region, e.g. `$$` or `$tag$`.
+ _DOLLAR_QUOTE_OPEN_RE = re.compile(r"\$\w*\$")
+
+ # A literal nests once per level of dynamic-SQL indirection (an
+ # `EXECUTE IMMEDIATE` inside an `EXECUTE IMMEDIATE`), so a handful covers
+ # every form that actually executes. The bound is what stops a body of
+ # deeply nested `$tag$` regions, whose nesting a user controls, from
+ # recursing once per level and exhausting the stack; past it the text is
+ # left as found, which keeps it visible to the gates rather than removed.
+ _MAX_LITERAL_NESTING = 32
+
# Command-fallback heads that are only mutating on dialects where the
# structured form (`exp.Set`) is reserved for benign session variables,
# so the opaque-Command fallback is reached exclusively by the dangerous
@@ -1173,20 +1310,157 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
parsed = self._parsed
return parsed.name.upper() if isinstance(parsed, exp.Command) else None
+ @cached_property
def _nested_body_text(self) -> str | None:
"""
Return the raw body of a command that carries a statement as text.
The body of a :attr:`_NESTED_BODY_COMMAND_NAMES` command is invisible
to node-type matching and cannot be re-parsed, so gates that inspect
- the tree fall back to scanning this text.
+ the tree fall back to scanning this text. Comments are stripped here
+ rather than by each caller, so every such gate scans the same text;
+ see :meth:`_strip_comments` for why.
+
+ Cached because three gates ask for the same body in a single request,
+ one of them twice, so the strip would otherwise run about five times
+ over identical text. Caching is safe
+ because the two methods that rebuild ``_parsed`` in place cannot reach
a
+ statement this returns text for: ``set_limit`` returns early unless the
+ node is an ``exp.Query``, and ``remove_unbounded_top_level_order_by``
+ needs an ``order`` argument, while a nested body is only ever carried
by
+ an opaque ``exp.Command``, which is neither.
+
+ :return: The body text with comments removed, or ``None`` when this
+ statement does not carry a nested body
+ """
+ if (head := self._command_head()) not in
self._NESTED_BODY_COMMAND_NAMES:
+ return None
+ body = self._parsed.expression
+ # sqlglot keeps the body as a literal node, whose `str()` re-renders it
+ # as a quoted SQL string: the whole body gets wrapped in quotes and the
+ # quotes inside it are doubled. Reading the value off the node yields
+ # the body as written, so a scan sees the same text the server runs.
+ text = body.name if isinstance(body, exp.Literal) else str(body)
+ # A `DO $$ ... $$` body arrives with its dollar-quote wrapper attached.
+ # That wrapper is statement syntax, not a literal -- the code inside it
+ # runs -- so it is peeled off before the strip, leaving the text either
+ # side of it in place. Any `$tag$` region still nested inside really is
+ # a literal, and is preserved as one.
+ text = self._peel_dollar_quote(text, head)
+ return self._strip_comments(text)
- :return: The raw body text, or ``None`` when this statement does not
- carry a nested body
+ @property
+ def _comment_re(self) -> re.Pattern[str]:
"""
- if self._command_head() not in self._NESTED_BODY_COMMAND_NAMES:
- return None
- return str(self._parsed.expression)
+ Return the comment pattern this statement's dialect is lexed with.
+
+ :return: The compiled comment pattern for this statement's dialect
+ """
+ return _comment_re_for(self._dialect)
+
+ @classmethod
+ def _peel_dollar_quote(cls, text: str, head: str | None) -> str:
+ """
+ Remove the delimiters of the body's dollar-quoted code wrapper, keeping
+ its contents and the text either side of it.
+
+ Only a region that the head itself introduces is a wrapper: a ``DO``
+ block, which takes nothing else, or a region running to the end of the
+ body (``EXECUTE IMMEDIATE $$...$$``). A region that stops short of the
+ end is one argument among several (``CALL p($q$...$q$, ...)``) and so
is
+ an ordinary literal; unwrapping that would expose its contents as code,
+ letting a ``--`` inside it comment out the rest of the body and blind
+ every gate that scans this text.
+
+ Matching the closing delimiter by search rather than by backtracking
+ regex keeps this linear: a pattern that scans forward for a closer
+ rescans to end of text from every opener that has none, which is
+ quadratic on input a user controls.
+
+ :param text: The raw body text to peel
+ :param head: The statement's command head, which decides whether a
+ region that stops short of the end is a wrapper or an argument
+ :return: The text with the wrapper's delimiters removed, or unchanged
+ when the body has no dollar-quoted wrapper
+ """
+ if not (opener := cls._DOLLAR_QUOTE_OPEN_RE.search(text)):
+ return text
+ delimiter = opener.group()
+ closer = text.find(delimiter, opener.end())
+ if closer == -1:
+ return text
+ if head != "DO" and text[closer + len(delimiter) :].strip():
+ return text
+ return (
+ text[: opener.start()]
+ + text[opener.end() : closer]
+ + text[closer + len(delimiter) :]
+ )
+
+ @staticmethod
+ def _split_literal(literal: str) -> tuple[str, str, str]:
+ """
+ Split a matched literal into its delimiters and the text between them.
+
+ :param literal: The literal as matched, delimiters included
+ :return: The opening delimiter, the interior, and the closing
+ delimiter, which is empty when the literal is unterminated
+ """
+ if not literal.startswith("$"):
+ # `'...'` and `"..."` both delimit with a single character.
+ return literal[0], literal[1:-1], literal[-1]
+ delimiter = literal[: literal.index("$", 1) + 1]
+ interior = literal[len(delimiter) :]
+ if not interior.endswith(delimiter):
+ return delimiter, interior, ""
+ return delimiter, interior[: -len(delimiter)], delimiter
+
+ def _strip_comments(self, text: str, depth: int = 0) -> str:
+ """
+ Blank out SQL comments in raw statement text, preserving literals.
+
+ Commented-out code never runs, so no gate should classify on it.
+ Literals are deliberately kept: a nested body runs its dynamic SQL out
+ of a literal (``EXECUTE IMMEDIATE '...'``), so dropping them would
+ blind such a scan to the very form it exists to catch.
+
+ That same reason makes the text *inside* a literal executable, so its
+ comments are stripped too, one literal at a time. Rescanning only the
+ interior is what keeps that safe: a `--` inside a literal can blank out
+ the rest of that literal, but never reaches past the closing delimiter
+ to truncate the statements after it. Without this a comment wedged into
+ a quoted body (``EXECUTE IMMEDIATE 'PUT/**/file:///a @s'``) would split
+ a head from its argument and hide it from every gate that scans here,
+ while the dollar-quoted spelling of the same statement was caught.
+
+ :param text: The raw statement text to scan
+ :param depth: How many levels of literal this text is already inside
+ :return: The text with each comment replaced by a single space, so
+ tokens either side of a removed comment stay separated
+ """
+ # Text carrying neither comment opener has nothing to strip, and the
+ # substitution below would be an identity: no comment branch can match,
+ # and every literal branch returns the literal unchanged. This is the
+ # same test `replace` already applies per literal, hoisted to the whole
+ # scan so the common comment-free body skips the pass entirely.
+ if "--" not in text and "/*" not in text:
+ return text
+
+ def replace(match: re.Match[str]) -> str:
+ if (literal := match.group("literal")) is None:
+ return " "
+ # A literal carrying neither comment opener has nothing to strip,
+ # and descending anyway costs a full scan per literal on text a
+ # user controls: a body of nothing but quotes is one recursion per
+ # `''` pair, which is ~20x the per-character cost of ordinary text.
+ if depth >= self._MAX_LITERAL_NESTING or (
+ "--" not in literal and "/*" not in literal
+ ):
+ return literal
+ opening, interior, closing = self._split_literal(literal)
+ return opening + self._strip_comments(interior, depth + 1) +
closing
+
+ return self._comment_re.sub(replace, text)
def _explain_analyze_body(self) -> str | None:
"""
@@ -1396,6 +1670,28 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
return False
+ def get_client_file_transfer_command(self) -> str | None:
+ """
+ Return the client-side file-transfer command head, if this is one.
+
+ :return: The uppercased command head (e.g. ``"PUT"``), else ``None``.
+ """
+ # Three shapes to cover: sqlglot models only the quoted-path forms
+ # structurally (``PUT 'file://...' @s`` -> ``exp.Put``, whose ``key``
is
+ # the head lowercased); every other form falls back to an opaque
+ # ``exp.Command``; and a nested body executes for real yet is invisible
+ # to both, so it is scanned as raw text, as ``changes_search_path``
+ # does for its own forms.
+ if isinstance(self._parsed, (exp.Put, exp.Get)):
+ return self._parsed.key.upper()
+ if (head := self._command_head()) in
self._CLIENT_FILE_TRANSFER_COMMAND_NAMES:
+ return head
+ if (body := self._nested_body_text) and (
+ match := self._CLIENT_FILE_TRANSFER_NESTED_BODY_RE.search(body)
+ ):
+ return match.group(1).upper()
+ return None
+
def is_destructive(self) -> bool:
"""
Check if the statement is destructive DDL (DROP, TRUNCATE, ALTER).
@@ -1617,7 +1913,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
# because a computed setting name never spells the latter
# contiguously. Whole-word matching keeps an unrelated identifier that
# merely embeds one of them (`reset_config`) from being flagged.
- body = self._nested_body_text()
+ body = self._nested_body_text
return bool(
body and re.search(r"\b(search_path|set_config)\b", body,
re.IGNORECASE)
)
@@ -1662,7 +1958,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
# required: a body that merely creates or references one is not a
# rebind. The optional qualifier mirrors the tokens the non-nested
# path strips, so `SET LOCAL SCHEMA` is matched in either position.
- body = self._nested_body_text()
+ body = self._nested_body_text
if body and re.search(
r"\bset\s+(?:(?:session|local|current)\s+)?(?:schema|catalog)\b",
body,
@@ -2514,6 +2810,25 @@ class SQLScript:
"""
return any(statement.is_mutating() for statement in self.statements)
+ def get_client_file_transfer_commands(self) -> list[str]:
+ """
+ Return the client-side file-transfer command heads in the script.
+
+ Sorted here so that every caller renders the heads in the same order:
+ the set these are deduplicated into iterates arbitrarily, which would
+ otherwise leave each error message to remember to sort for itself.
+
+ :return: The sorted, deduplicated uppercased command heads found
+ (empty when none).
+ """
+ return sorted(
+ {
+ command
+ for statement in self.statements
+ if (command := statement.get_client_file_transfer_command())
is not None
+ }
+ )
+
def has_destructive(self) -> bool:
"""
Check if the script contains destructive DDL (DROP, TRUNCATE, ALTER).
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index dfb3124948f..4fe37ccb983 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -49,6 +49,7 @@ from superset.db_engine_specs import BaseEngineSpec
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import (
OAuth2RedirectError,
+ SupersetDisallowedClientFileTransferException,
SupersetDisallowedSQLFunctionException,
SupersetDisallowedSQLTableException,
SupersetDMLNotAllowedException,
@@ -518,6 +519,10 @@ def execute_sql_statements( # noqa: C901
if found_tables:
raise SupersetDisallowedSQLTableException(found_tables)
+ # Rejected regardless of `allow_dml`: these do host file I/O, not DML.
+ if file_transfer_commands :=
parsed_script.get_client_file_transfer_commands():
+ raise
SupersetDisallowedClientFileTransferException(file_transfer_commands)
+
if parsed_script.has_mutation() and not database.allow_dml:
raise SupersetDMLNotAllowedException()
diff --git a/superset/translations/messages.pot
b/superset/translations/messages.pot
index 2b228252fba..e38a61d782f 100644
--- a/superset/translations/messages.pot
+++ b/superset/translations/messages.pot
@@ -1483,6 +1483,10 @@ msgstr ""
msgid "Alert query must be read-only"
msgstr ""
+#, python-format
+msgid "Alert query must not contain the file-transfer command(s): %(commands)s"
+msgstr ""
+
msgid "Alert query returned a non-number value."
msgstr ""
diff --git a/tests/unit_tests/commands/report/alert_test.py
b/tests/unit_tests/commands/report/alert_test.py
index 13164230006..cea80fff22b 100644
--- a/tests/unit_tests/commands/report/alert_test.py
+++ b/tests/unit_tests/commands/report/alert_test.py
@@ -569,3 +569,40 @@ def test_execute_query_wraps_template_rendering_error(
with pytest.raises(AlertQueryError):
command._execute_query()
+
+
[email protected]("allow_dml", [False, True])
[email protected](
+ "sql, rejected_head",
+ [
+ ("PUT file:///tmp/data.csv @my_stage", "PUT"),
+ ("SELECT value FROM metrics", None),
+ ],
+)
+def test_validate_rendered_sql_client_file_transfer_gate(
+ mocker: MockerFixture,
+ allow_dml: bool,
+ sql: str,
+ rejected_head: str | None,
+) -> None:
+ """
+ A client-side file-transfer command in an alert query is rejected whether
+ or not the database allows DML: it does file I/O on the host running the
+ query rather than reading or writing table data, so `allow_dml` does not
+ govern it. An ordinary read-only query is left alone either way.
+ """
+ report_schedule_mock = mocker.Mock()
+ report_schedule_mock.database.backend = "snowflake"
+ report_schedule_mock.database.allow_dml = allow_dml
+
+ command = AlertCommand(
+ report_schedule=report_schedule_mock,
+ execution_id=uuid4(),
+ )
+
+ if rejected_head is None:
+ command._validate_rendered_sql(sql)
+ return
+
+ with pytest.raises(AlertQueryError, match=rejected_head):
+ command._validate_rendered_sql(sql)
diff --git a/tests/unit_tests/commands/sql_lab/test_estimate.py
b/tests/unit_tests/commands/sql_lab/test_estimate.py
index 2e2c1317096..b34cca55eef 100644
--- a/tests/unit_tests/commands/sql_lab/test_estimate.py
+++ b/tests/unit_tests/commands/sql_lab/test_estimate.py
@@ -216,6 +216,36 @@ def
test_apply_sql_security_allows_dml_when_enabled(mock_app: MagicMock) -> None
assert command._apply_sql_security("INSERT INTO t VALUES (1)")
[email protected]("allow_dml", [False, True])
[email protected](
+ "sql, expected_heads",
+ [
+ ("PUT file:///tmp/data.csv @my_stage", "PUT"),
+ ("GET @my_stage 'file:///tmp/'", "GET"),
+ ("REMOVE @my_stage/b; PUT file:///tmp/a @my_stage", "PUT, REMOVE"),
+ ],
+)
+@patch("superset.commands.sql_lab.estimate.app")
+def test_apply_sql_security_blocks_client_file_transfer(
+ mock_app: MagicMock, sql: str, expected_heads: str, allow_dml: bool
+) -> None:
+ """
+ The estimate path rejects client-side file-transfer statements regardless
+ of `allow_dml`, matching the execution path.
+ """
+ mock_app.config = {"DISALLOWED_SQL_FUNCTIONS": {},
"DISALLOWED_SQL_TABLES": {}}
+ from superset.exceptions import
SupersetDisallowedClientFileTransferException
+
+ command = _make_command_with_db(sql, allow_dml=allow_dml,
engine="snowflake")
+ with pytest.raises(SupersetDisallowedClientFileTransferException) as
excinfo:
+ command._apply_sql_security(sql)
+
+ assert excinfo.value.error.message == (
+ "SQL statement contains disallowed client-side "
+ f"file-transfer command(s): {expected_heads}"
+ )
+
+
@patch("superset.commands.sql_lab.estimate.is_feature_enabled",
return_value=False)
@patch("superset.commands.sql_lab.estimate.app")
def test_apply_sql_security_blocks_disallowed_table(
diff --git a/tests/unit_tests/sql/execution/test_executor.py
b/tests/unit_tests/sql/execution/test_executor.py
index ac171ff2580..7519cf6cd98 100644
--- a/tests/unit_tests/sql/execution/test_executor.py
+++ b/tests/unit_tests/sql/execution/test_executor.py
@@ -244,6 +244,57 @@ def test_execute_delete_without_permission(
assert "DML queries are not allowed" in result.error_message
+# =============================================================================
+# Client-side file-transfer statement tests
+# =============================================================================
+
+
[email protected]("allow_dml", [False, True])
[email protected](
+ "sql, expected_heads",
+ [
+ # Per-statement parsing of every form is covered in parse_tests; here
+ # one Command fallback, one structured node, one multi-statement.
+ ("PUT file:///tmp/data.csv @my_stage", "PUT"),
+ ("GET @my_stage 'file:///tmp/'", "GET"),
+ ("SELECT 1; PUT file:///tmp/data.csv @my_stage", "PUT"),
+ # Several heads render sorted and comma-separated, not as a raw set.
+ ("REMOVE @my_stage/b; PUT file:///tmp/a @my_stage", "PUT, REMOVE"),
+ ],
+)
+def test_check_security_rejects_client_file_transfer(
+ mocker: MockerFixture,
+ app_context: None,
+ sql: str,
+ expected_heads: str,
+ allow_dml: bool,
+) -> None:
+ """
+ Client-side file-transfer statements are rejected regardless of
+ `allow_dml`: they perform host file I/O, not DML.
+ """
+ from superset.exceptions import SupersetSecurityException
+ from superset.sql.execution.executor import SQLExecutor
+
+ mocker.patch.dict(
+ current_app.config,
+ {"DISALLOWED_SQL_FUNCTIONS": {}, "DISALLOWED_SQL_TABLES": {}},
+ )
+ database = Database(
+ id=3,
+ database_name="test_snowflake",
+ sqlalchemy_uri="snowflake://user:pw@account/db/schema",
+ allow_dml=allow_dml,
+ )
+
+ with pytest.raises(SupersetSecurityException) as excinfo:
+ SQLExecutor(database)._check_security(SQLScript(sql, "snowflake"))
+
+ assert excinfo.value.error.message == (
+ f"Disallowed client-side file-transfer command(s): {expected_heads}"
+ )
+
+
# =============================================================================
# Jinja2 Template Rendering Tests
# =============================================================================
diff --git a/tests/unit_tests/sql/parse_tests.py
b/tests/unit_tests/sql/parse_tests.py
index 616cb802905..ac1888dcdb1 100644
--- a/tests/unit_tests/sql/parse_tests.py
+++ b/tests/unit_tests/sql/parse_tests.py
@@ -2060,6 +2060,154 @@ def test_is_mutating_replace_function_is_read(engine:
str) -> None:
).is_mutating()
[email protected](
+ "sql, expected",
+ [
+ ("PUT file:///tmp/data.csv @my_stage", "PUT"),
+ ("GET @my_stage file:///tmp/", "GET"),
+ # A quoted local path parses to ``exp.Put``/``exp.Get`` rather than
+ # the opaque Command fallback, but is the same documented syntax.
+ ("PUT 'file:///tmp/data.csv' @my_stage", "PUT"),
+ ("GET @my_stage 'file:///tmp/'", "GET"),
+ ("REMOVE @my_stage/path", "REMOVE"),
+ # ``RM`` is a documented alias of ``REMOVE``.
+ ("RM @my_stage/path", "RM"),
+ # The head keyword is matched case-insensitively.
+ ("put file:///tmp/data.csv @my_stage", "PUT"),
+ # Ordinary analytics statements are not file-transfer commands.
+ ("SELECT 1", None),
+ ("INSERT INTO t VALUES (1)", None),
+ # ``LIST``/``LS`` only enumerate staged files (a read), so they are
+ # intentionally not treated as file-transfer commands here.
+ ("LIST @my_stage", None),
+ # A nested body runs for real but is kept as unparsed text, so the
+ # head match cannot see it and the raw body is scanned instead.
+ ("EXECUTE IMMEDIATE $$ REMOVE @my_stage/path $$", "REMOVE"),
+ ("EXECUTE IMMEDIATE 'RM @my_stage/path'", "RM"),
+ ("EXECUTE IMMEDIATE $$ PUT file:///tmp/data.csv @my_stage $$", "PUT"),
+ ("EXECUTE IMMEDIATE $$ GET @my_stage file:///tmp/ $$", "GET"),
+ # The body scan requires a stage/``file://`` reference after the head,
+ # so a body that merely names a column after one is not flagged.
+ ("EXECUTE IMMEDIATE $$ SELECT remove FROM t $$", None),
+ ("EXECUTE IMMEDIATE $$ SELECT put, rm FROM t $$", None),
+ # Commented-out code in a body never runs, so it is not a command.
+ ("EXECUTE IMMEDIATE $$ -- GET @my_stage file:///tmp/\nSELECT 1 $$",
None),
+ ("EXECUTE IMMEDIATE $$ /* PUT file:///tmp/a @s */ SELECT 1 $$", None),
+ # A `--` inside a literal opens no comment, so the text after it is
+ # still scanned.
+ ("EXECUTE IMMEDIATE $$ CALL p('a--b'); RM @my_stage/c $$", "RM"),
+ # A body runs dynamic SQL out of a literal, so a head inside one is
+ # matched: the literal is the statement the server executes.
+ ("CALL run('PUT file:///tmp/data.csv @my_stage')", "PUT"),
+ # A nested literal carries its own quotes doubled, so the scan has to
+ # look past a run of them rather than a single one.
+ ("EXECUTE IMMEDIATE 'PUT ''file:///tmp/data.csv'' @my_stage'", "PUT"),
+ ("CALL run('PUT ''file:///tmp/data.csv'' @my_stage')", "PUT"),
+ ("EXECUTE IMMEDIATE 'GET @my_stage ''file:///tmp/'''", "GET"),
+ # A `--` inside a dollar-quoted literal is data, not a comment, so the
+ # command after it is still scanned.
+ ("EXECUTE IMMEDIATE $$ SELECT $q$--$q$; RM @my_stage/c $$", "RM"),
+ # An unterminated block comment runs to the end of the body, so what
+ # follows it never executes.
+ ("EXECUTE IMMEDIATE $$ /* PUT file:///tmp/a @my_stage $$", None),
+ # A dollar-quoted argument that stops short of the end of the body is
+ # an ordinary literal, not the code wrapper, so it stays quoted: the
+ # `--` inside it opens no comment and the argument after it is scanned.
+ ("CALL p($$--$$, 'PUT file:///tmp/a @my_stage')", "PUT"),
+ ("CALL p($q$--$q$, 'GET @my_stage file:///tmp/a')", "GET"),
+ # An opener with no matching closer delimits nothing, so there is no
+ # wrapper to peel: the body is scanned as written and the command
+ # after the stray opener is still caught.
+ ("EXECUTE IMMEDIATE 'SELECT $q$ ; RM @my_stage/c'", "RM"),
+ # The body a literal carries is what the server runs, so a comment
+ # wedged inside it is stripped like any other: it cannot be used to
+ # split a head from its argument and slip the command past the scan.
+ ("EXECUTE IMMEDIATE 'PUT/*x*/file:///tmp/a @my_stage'", "PUT"),
+ ("EXECUTE IMMEDIATE 'PUT -- x\nfile:///tmp/a @my_stage'", "PUT"),
+ ("CALL p('REMOVE/*x*/@my_stage/b')", "REMOVE"),
+ # Stripping inside a literal stops at its closing delimiter, so a `--`
+ # in one still cannot comment out the statements that follow it.
+ ("EXECUTE IMMEDIATE $$ CALL p('--'); RM @my_stage/c $$", "RM"),
+ # A backslash escapes the quote on this dialect, so the literal does
+ # not end at the quote it precedes. Reading that quote as the closing
+ # one would leave the rest of the body looking like code, letting the
+ # `--` blank out the command that follows it.
+ ("CALL p('it\\'s -- x', 'PUT file:///tmp/a @my_stage')", "PUT"),
+ ('CALL p("it\\"s -- x", \'GET @my_stage file:///tmp/a\')', "GET"),
+ # A head that really is commented out inside the body still does not
+ # run, so it is still not reported.
+ ("EXECUTE IMMEDIATE 'SELECT 1 -- PUT file:///tmp/a @my_stage'", None),
+ ("CALL some_procedure()", None),
+ ],
+)
+def test_get_client_file_transfer_command(sql: str, expected: str | None) ->
None:
+ """
+ `get_client_file_transfer_command` returns the command head for Snowflake
+ client-side file-transfer statements and ``None`` for anything else.
+ """
+ assert SQLStatement(sql, "snowflake").get_client_file_transfer_command()
== expected
+
+
[email protected](
+ "sql, expected",
+ [
+ ("PUT file:///tmp/data.csv @my_stage", ["PUT"]),
+ ("SELECT 1; GET @my_stage file:///tmp/", ["GET"]),
+ ("SELECT 1; PUT 'file:///tmp/data.csv' @my_stage", ["PUT"]),
+ # Heads are deduplicated and returned in sorted order, so the error
+ # messages built from them read the same on every run. Written here in
+ # the reverse of the order the statements appear in.
+ ("REMOVE @s/b; PUT file:///a @s", ["PUT", "REMOVE"]),
+ ("PUT file:///a @s; PUT file:///b @s", ["PUT"]),
+ ("SELECT 1; EXECUTE IMMEDIATE $$ REMOVE @s/b $$", ["REMOVE"]),
+ ("SELECT 1", []),
+ ],
+)
+def test_get_client_file_transfer_commands_script(
+ sql: str, expected: list[str]
+) -> None:
+ """
+ `SQLScript.get_client_file_transfer_commands` collects every file-transfer
+ command head across the statements in a multi-statement script, sorted and
+ deduplicated.
+ """
+ assert SQLScript(sql, "snowflake").get_client_file_transfer_commands() ==
expected
+
+
+def test_strip_comments_bounds_literal_nesting() -> None:
+ """
+ `_strip_comments` stops descending into nested literals past a fixed depth,
+ so a body whose nesting a user controls cannot exhaust the stack. Past the
+ bound the text is left as found rather than dropped, so it stays visible to
+ the gates that scan it.
+ """
+ statement = SQLStatement("SELECT 1", "postgresql")
+ depth = SQLStatement._MAX_LITERAL_NESTING + 1
+ # Each `$t<n>$ ... $t<n>$` region is one level of literal nesting.
+ nested = "/* c */"
+ for level in range(depth):
+ nested = f"$t{level}${nested}$t{level}$"
+
+ stripped = statement._strip_comments(nested)
+
+ # The whole structure survives, and the comment below the bound does not.
+ assert stripped.startswith(f"$t{depth - 1}$")
+ assert "/* c */" in stripped
+ # The same comment one level above the bound is stripped.
+ assert "/* c */" not in statement._strip_comments("$t0$/* c */$t0$")
+
+
+def test_split_literal_reports_unterminated_dollar_quote() -> None:
+ """
+ `_split_literal` reports an empty closing delimiter when a dollar-quoted
+ literal never closes, so a caller can tell an unterminated region apart
+ from one that closes normally rather than treating the two the same.
+ """
+ opening, interior, closing = SQLStatement._split_literal("$t$--")
+
+ assert (opening, interior, closing) == ("$t$", "--", "")
+
+
@pytest.mark.parametrize(
"sql, expected",
[
@@ -5800,6 +5948,18 @@ def test_get_disallowed_tables_search_path_change(
("SELECT set_config('statement_timeout', '0', true)", False),
# An unrelated (non-`set_config`) function call is not a change either.
("SELECT my_custom_func(1)", False),
+ # The nested-body text is scanned with comments removed, so a rebind
+ # that is only commented out is not a change, while one carried in a
+ # string literal (the body's dynamic SQL) still is.
+ ("DO $$ BEGIN -- SET search_path TO evil\nPERFORM 1; END $$", False),
+ ("DO $$ BEGIN /* SET search_path TO evil */ PERFORM 1; END $$", False),
+ ("EXECUTE IMMEDIATE 'SET search_path TO evil'", True),
+ # A `--` inside a dollar-quoted literal is data, not a comment, so
+ # removing comments must not swallow the rebind that follows it.
+ (
+ "DO $$ BEGIN PERFORM $q$--$q$; EXECUTE 'SET search_path TO evil';
END $$",
+ True,
+ ),
("SELECT 1", False),
],
)
@@ -5876,6 +6036,34 @@ def test_changes_search_path(sql: str, expected: bool)
-> None:
("EXPLAIN SET SCHEMA 'tenant_b'", "postgresql", False),
("EXPLAIN VERBOSE SELECT * FROM orders", "postgresql", False),
("EXPLAIN (COSTS) SELECT * FROM orders", "postgresql", False),
+ # The nested-body text is scanned with comments removed, so a rebind
+ # that is only commented out is not a change, while one carried in a
+ # string literal (the body's dynamic SQL) still is.
+ ("DO $$ BEGIN -- SET SCHEMA 'evil'\nPERFORM 1; END $$", "postgresql",
False),
+ ("EXECUTE IMMEDIATE 'SET SCHEMA ''evil'''", "postgresql", True),
+ # MySQL-family engines only open a comment on `--` when whitespace
+ # follows, so `1--2` is arithmetic there and the rest of the body still
+ # executes; a spaced `-- ` is a comment on every dialect.
+ ("CALL p(1--2, 'SET SCHEMA evil')", "mysql", True),
+ ("CALL p(1, 'x') -- SET SCHEMA evil", "mysql", False),
+ ("CALL p(1--2, 'SET SCHEMA evil')", "postgresql", False),
+ # SingleStore speaks the MySQL wire protocol and takes the same rule,
+ # so it must not lose the rest of the body to an unspaced `--`.
+ ("CALL p(1--2, 'SET SCHEMA evil')", "singlestoredb", True),
+ ("CALL p(1, 'x') -- SET SCHEMA evil", "singlestoredb", False),
+ # Whether a backslash escapes is read off the dialect too. Where it
+ # does, the literal does not end at the quote the backslash precedes,
+ # so the `--` after it is still inside that literal and cannot comment
+ # out the rebind carried in the next argument.
+ ("CALL p('it\\'s -- x', 'SET SCHEMA evil')", "snowflake", True),
+ ("CALL p('it\\'s -- x', 'SET SCHEMA evil')", "mysql", True),
+ # PostgreSQL has no backslash escape, so the literal really does end
+ # there and the `--` that follows opens a comment.
+ ("CALL p('it\\'s -- x', 'SET SCHEMA evil')", "postgresql", False),
+ # The family is identified by subclassing `MySQL`, so an engine that
+ # joins it without being named anywhere here takes the rule too.
+ ("CALL p(1--2, 'SET SCHEMA evil')", "pinot", True),
+ ("CALL p(1, 'x') -- SET SCHEMA evil", "pinot", False),
# Engines without a sqlglot AST (e.g. Kusto KQL) do not rebind schema
# resolution through these forms.
("print x = 1", "kustokql", False),
@@ -6437,6 +6625,25 @@ def test_backtick_invalid_sql_still_fails() -> None:
SQLScript(sql, "base")
+def
test_base_sql_statement_get_client_file_transfer_command_defaults_to_none() ->
None:
+ """
+ BaseSQLStatement.get_client_file_transfer_command defaults to None, so a
+ dialect without such commands inherits the safe answer rather than having
+ to override it.
+ """
+ assert BaseSQLStatement.get_client_file_transfer_command(object()) is None
# type: ignore[arg-type] # noqa: E501
+
+
+def test_kusto_kql_has_no_client_file_transfer_command() -> None:
+ """
+ Kusto KQL has no client-side file-transfer commands.
+ """
+ assert (
+ KustoKQLStatement(".show tables",
"kustokql").get_client_file_transfer_command()
+ is None
+ )
+
+
def test_base_sql_statement_is_destructive_raises_not_implemented() -> None:
"""
BaseSQLStatement.is_destructive is abstract; both concrete subclasses
diff --git a/tests/unit_tests/sql_lab_test.py b/tests/unit_tests/sql_lab_test.py
index 13e53b19103..f76ff4c1af8 100644
--- a/tests/unit_tests/sql_lab_test.py
+++ b/tests/unit_tests/sql_lab_test.py
@@ -301,6 +301,45 @@ def
test_execute_sql_statement_within_payload_limit(mocker: MockerFixture, app)
)
[email protected]("allow_dml", [False, True])
+def test_execute_sql_statements_rejects_client_file_transfer(
+ mocker: MockerFixture, app: SupersetApp, allow_dml: bool
+) -> None:
+ """
+ `execute_sql_statements` rejects client-side file-transfer statements
+ regardless of `allow_dml`: they perform host file I/O, not DML.
+ """
+ from superset.exceptions import
SupersetDisallowedClientFileTransferException
+
+ query = mocker.MagicMock()
+ query.limit = 1
+ query.status = "RUNNING"
+ query.select_as_cta = False
+ query.database.allow_dml = allow_dml
+ query.database.allow_run_async = False
+ query.database.db_engine_spec.engine = "snowflake"
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
+
+ with pytest.raises(SupersetDisallowedClientFileTransferException) as
excinfo:
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="REMOVE @my_stage/b; PUT file:///tmp/data.csv
@my_stage",
+ return_results=True,
+ store_results=False,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+
+ # Sorted and comma-separated, not raw set interpolation.
+ assert excinfo.value.error.message == (
+ "SQL statement contains disallowed client-side "
+ "file-transfer command(s): PUT, REMOVE"
+ )
+
+
def test_execute_sql_statements_mutates_before_split_by_default(
mocker: MockerFixture, app: SupersetApp
) -> None:
