diff --git a/superset/commands/logs/prune.py b/superset/commands/logs/prune.py index 0ca6eaa4f13..10d8e309868 100644 --- a/superset/commands/logs/prune.py +++ b/superset/commands/logs/prune.py @@ -64,8 +64,11 @@ class LogPruneCommand(BaseCommand): start_time = time.time() # Select all IDs that need to be deleted + # Log.dttm is stored as a naive UTC datetime (no tzinfo), so compute + # the cutoff with utcnow() to avoid a naive/aware mismatch that raises + # on PostgreSQL ("operator does not exist: timestamp without time zone"). select_stmt = sa.select(Log.id).where( - Log.dttm < datetime.now() - timedelta(days=self.retention_period_days) + Log.dttm < datetime.utcnow() - timedelta(days=self.retention_period_days) ) # Optionally limited by max_rows_per_run diff --git a/superset/utils/core.py b/superset/utils/core.py index f361034e10b..1eded8dc4a4 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -829,6 +829,9 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many smtp_mail_to = recipients_string_to_list(to) msg = MIMEMultipart(mime_subtype) + # Strip CR/LF from the subject so the value cannot inject additional + # email headers via header folding/splitting. + subject = subject.replace("\r", "").replace("\n", " ").strip() msg["Subject"] = subject msg["From"] = smtp_mail_from msg["To"] = ", ".join(smtp_mail_to) diff --git a/superset/utils/csv.py b/superset/utils/csv.py index bf5045728dd..01b6354ced5 100644 --- a/superset/utils/csv.py +++ b/superset/utils/csv.py @@ -32,11 +32,14 @@ negative_number_re = re.compile(r"^-[0-9.]+$") # This regex will match if the string starts with: # -# 1. one of -, @, +, |, =, % +# 1. one of -, @, +, |, =, %, a tab, or a carriage return # 2. two double quotes immediately followed by one of -, @, +, |, =, % # 3. one or more spaces immediately followed by one of -, @, +, |, =, % # -problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%]') +# A leading tab or carriage return is treated as dangerous on its own because +# some spreadsheet software trims that leading whitespace and then evaluates +# the remaining cell content as a formula. +problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%\t\r]') def escape_value(value: str) -> str: diff --git a/tests/unit_tests/commands/logs/__init__.py b/tests/unit_tests/commands/logs/__init__.py new file mode 100644 index 00000000000..13a83393a91 --- /dev/null +++ b/tests/unit_tests/commands/logs/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/tests/unit_tests/commands/logs/prune_test.py b/tests/unit_tests/commands/logs/prune_test.py new file mode 100644 index 00000000000..73ab8e79f61 --- /dev/null +++ b/tests/unit_tests/commands/logs/prune_test.py @@ -0,0 +1,54 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from unittest.mock import MagicMock, patch + + +def test_prune_cutoff_is_naive_utc() -> None: + """ + The retention cutoff is a naive UTC datetime matching how Log.dttm is + stored (datetime.utcnow, no tzinfo). Using a timezone-aware cutoff would + raise "operator does not exist: timestamp without time zone" on PostgreSQL. + """ + from superset.commands.logs.prune import LogPruneCommand + + captured: dict[str, datetime] = {} + + def fake_execute(stmt): # noqa: ANN001 + # Capture the bound cutoff value from the WHERE clause. + for param in stmt.compile().params.values(): + if isinstance(param, datetime): + captured["cutoff"] = param + result = MagicMock() + result.scalars.return_value.all.return_value = [] + return result + + session = MagicMock() + session.execute.side_effect = fake_execute + + with patch("superset.commands.logs.prune.db") as mock_db: + mock_db.session = session + LogPruneCommand(retention_period_days=30).run() + + cutoff = captured["cutoff"] + # The cutoff must be timezone-naive to match Log.dttm column type. + assert cutoff.tzinfo is None + + expected = datetime.utcnow() - timedelta(days=30) + # Allow a small delta for execution time between computing the two values. + assert abs((cutoff - expected).total_seconds()) < 60 diff --git a/tests/unit_tests/utils/csv_tests.py b/tests/unit_tests/utils/csv_tests.py index 747e8b32870..661a9ef9a91 100644 --- a/tests/unit_tests/utils/csv_tests.py +++ b/tests/unit_tests/utils/csv_tests.py @@ -63,6 +63,17 @@ def test_escape_value(): result = csv.escape_value(" =10+2") assert result == "' =10+2" + # A leading tab or carriage return followed by a dangerous char was already + # handled by \s{1,} in the pre-existing regex. The cases below test the + # new behavior: tab/CR alone (not followed by a dangerous char) are now + # also treated as dangerous prefixes because some spreadsheet software trims + # leading whitespace and then evaluates the remaining content as a formula. + result = csv.escape_value("\t10") + assert result == "'\t10" + + result = csv.escape_value("\rfoo") + assert result == "'\rfoo" + def fake_get_chart_csv_data_none(chart_url, auth_cookies=None): return None @@ -182,6 +193,35 @@ def test_df_to_escaped_csv(): assert df_to_escaped_csv(df, encoding="utf8", index=False) == '0\n1\n""\n' +def test_df_to_escaped_csv_preserves_numeric_columns(): + """ + A string cell beginning with a dangerous prefix is escaped while genuinely + numeric columns are left untouched. + """ + df = pd.DataFrame( + data={ + "formula": ["=cmd()", "safe"], + "amount": [10, -20], + } + ) + + escaped_csv_str = df_to_escaped_csv( + df, + encoding="utf8", + index=False, + ) + + rows = [row.split(",") for row in escaped_csv_str.strip().split("\n")] + + # Header + 2 data rows. + assert rows[0] == ["formula", "amount"] + # Dangerous string cell is escaped with a leading single quote. + assert rows[1] == ["'=cmd()", "10"] + # Safe string is untouched; numeric values (including negatives) are not + # escaped or quoted. + assert rows[2] == ["safe", "-20"] + + def test_get_chart_dataframe_returns_none_when_no_content( monkeypatch: pytest.MonkeyPatch, ): diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py index 5e663dd095a..bfccda8c4ff 100644 --- a/tests/unit_tests/utils/test_core.py +++ b/tests/unit_tests/utils/test_core.py @@ -1730,3 +1730,44 @@ def test_markdown_with_markup_wrap() -> None: assert isinstance(result, Markup) assert "bold" in str(result) + + +def test_send_email_smtp_strips_crlf_from_subject() -> None: + """ + CR/LF characters are stripped from the email subject so the value cannot + be used to inject additional email headers (header folding/splitting). + """ + from email.mime.multipart import MIMEMultipart + + from superset.utils.core import send_email_smtp + + captured: dict[str, MIMEMultipart] = {} + + def capture_mutator(msg: MIMEMultipart, **kwargs: Any) -> MIMEMultipart: + captured["msg"] = msg + return msg + + config = { + "SMTP_MAIL_FROM": "from@example.com", + "EMAIL_HEADER_MUTATOR": capture_mutator, + "SMTP_HOST": "localhost", + "SMTP_PORT": 25, + "SMTP_USER": "", + "SMTP_PASSWORD": "", + "SMTP_STARTTLS": False, + "SMTP_SSL": False, + "SMTP_SSL_SERVER_AUTH": False, + } + + send_email_smtp( + to="to@example.com", + subject="Hello\r\nBcc: attacker@example.com", + html_content="body", + config=config, + dryrun=True, + ) + + subject = captured["msg"]["Subject"] + assert "\r" not in subject + assert "\n" not in subject + assert subject == "Hello Bcc: attacker@example.com"