fix(keys): Unsafe dict access in get_native_filters_params() crashes execution (#38272)

superset/commands/report/execute.py

@@ -99,6 +99,7 @@ class BaseReportState:
         self._scheduled_dttm = scheduled_dttm
         self._start_dttm = datetime.utcnow()
         self._execution_id = execution_id
+        self._filter_warnings: list[str] = []
 
     def update_report_schedule_and_log(
         self,
@@ -265,7 +266,11 @@ class BaseReportState:
         if (
             dashboard_state := self._report_schedule.extra.get("dashboard")
         ) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"):
-            native_filter_params = self._report_schedule.get_native_filters_params()
+            native_filter_params, filter_warnings = (
+                self._report_schedule.get_native_filters_params()
+            )
+            if filter_warnings:
+                self._filter_warnings.extend(filter_warnings)
             if anchor := dashboard_state.get("anchor"):
                 try:
                     anchor_list = json.loads(anchor)
@@ -835,7 +840,13 @@ class ReportNotTriggeredErrorState(BaseReportState):
                     )
                     return
             self.send()
-            self.update_report_schedule_and_log(ReportState.SUCCESS)
+            # Include filter warnings in the log if any were collected
+            warning_message = (
+                ";".join(self._filter_warnings) if self._filter_warnings else None
+            )
+            self.update_report_schedule_and_log(
+                ReportState.SUCCESS, error_message=warning_message
+            )
         except (SupersetErrorsException, Exception) as first_ex:
             error_message = str(first_ex)
             if isinstance(first_ex, SupersetErrorsException):
@@ -980,7 +991,13 @@ class ReportSuccessState(BaseReportState):
 
         try:
             self.send()
-            self.update_report_schedule_and_log(ReportState.SUCCESS)
+            # Include filter warnings in the log if any were collected
+            warning_message = (
+                ";".join(self._filter_warnings) if self._filter_warnings else None
+            )
+            self.update_report_schedule_and_log(
+                ReportState.SUCCESS, error_message=warning_message
+            )
         except Exception as ex:  # pylint: disable=broad-except
             try:
                 self.update_report_schedule_and_log(

superset/reports/models.py

@@ -16,6 +16,7 @@
 # under the License.
 """A collection of ORM sqlalchemy models for Superset"""
 
+import logging
 from typing import Any, Optional
 
 import prison
@@ -47,6 +48,8 @@ from superset.reports.types import ReportScheduleExtra
 from superset.utils.backports import StrEnum
 from superset.utils.core import MediumText
 
+logger = logging.getLogger(__name__)
+
 metadata = Model.metadata  # pylint: disable=no-member
 
 
@@ -187,24 +190,46 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model):
     def crontab_humanized(self) -> str:
         return get_description(self.crontab)
 
-    def get_native_filters_params(self) -> str:
+    def get_native_filters_params(self) -> tuple[str, list[str]]:
+        """
+        Generate native filter params for dashboard URL.
+
+        Returns:
+            A tuple of (rison_encoded_params, list_of_warning_messages).
+            Warnings are returned so they can be surfaced to users in the
+            execution log.
+        """
         params: dict[str, Any] = {}
+        warnings: list[str] = []
         dashboard = self.extra.get("dashboard")
         if dashboard and dashboard.get("nativeFilters"):
-            for filter in dashboard.get("nativeFilters") or []:  # type: ignore
+            native_filters = dashboard.get("nativeFilters") or []
+            for native_filter in native_filters:  # type: ignore
+                native_filter_id = native_filter.get("nativeFilterId")
+                filter_type = native_filter.get("filterType")
+
+                if native_filter_id is None or filter_type is None:
+                    warning_msg = (
+                        f"Skipping malformed native filter missing required "
+                        f"fields: {native_filter}"
+                    )
+                    warnings.append(warning_msg)
+                    logger.warning(warning_msg)
+                    continue
+
                 params = {
                     **params,
                     **self._generate_native_filter(
-                        filter["nativeFilterId"],
-                        filter["filterType"],
-                        filter["columnName"],
-                        filter["filterValues"],
+                        native_filter_id,
+                        filter_type,
+                        native_filter.get("columnName") or "",
+                        native_filter.get("filterValues") or [],
                     ),
                 }
         # hack(hughhh): workaround for escaping prison not handling quotes right
         rison = prison.dumps(params)
         rison = rison.replace("'", "%27")
-        return rison
+        return rison, warnings
 
     def _generate_native_filter(
         self,

tests/unit_tests/commands/report/execute_test.py

@@ -290,6 +290,10 @@ def test_get_dashboard_urls_with_multiple_tabs(
             "urlParams": None,
         }
     }
+    mock_report_schedule.get_native_filters_params.return_value = (  # type: ignore
+        "()",
+        [],
+    )
 
     class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
@@ -333,6 +337,10 @@ def test_get_dashboard_urls_with_exporting_dashboard_only(
             "urlParams": None,
         }
     }
+    mock_report_schedule.get_native_filters_params.return_value = (  # type: ignore
+        "()",
+        [],
+    )
     mock_run.return_value = "url1"
 
     class_instance: BaseReportState = BaseReportState(

tests/unit_tests/reports/model_test.py

@@ -14,7 +14,6 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-import pytest
 
 from superset.reports.models import ReportSchedule
 
@@ -37,9 +36,14 @@ def test_get_native_filters_params():
         }
     }
 
-    assert report_schedule.get_native_filters_params() == (
-        "(filter_id:(extraFormData:(filters:!((col:column_name,op:IN,val:!(value1,value2)))),filterState:(label:column_name,validateStatus:!f,value:!(value1,value2)),id:filter_id,ownState:()))"
+    result, warnings = report_schedule.get_native_filters_params()
+    expected = (
+        "(filter_id:(extraFormData:(filters:!((col:column_name,op:IN,"
+        "val:!(value1,value2)))),filterState:(label:column_name,"
+        "validateStatus:!f,value:!(value1,value2)),id:filter_id,ownState:()))"
     )
+    assert result == expected
+    assert warnings == []
 
 
 def test_get_native_filters_params_multiple_filters():
@@ -66,9 +70,17 @@ def test_get_native_filters_params_multiple_filters():
         }
     }
 
-    assert report_schedule.get_native_filters_params() == (
-        "(filter_id_1:(extraFormData:(filters:!((col:column_name_1,op:IN,val:!(value1,value2)))),filterState:(label:column_name_1,validateStatus:!f,value:!(value1,value2)),id:filter_id_1,ownState:()),filter_id_2:(extraFormData:(filters:!((col:column_name_2,op:IN,val:!(value3,value4)))),filterState:(label:column_name_2,validateStatus:!f,value:!(value3,value4)),id:filter_id_2,ownState:()))"
+    result, warnings = report_schedule.get_native_filters_params()
+    expected = (
+        "(filter_id_1:(extraFormData:(filters:!((col:column_name_1,op:IN,"
+        "val:!(value1,value2)))),filterState:(label:column_name_1,"
+        "validateStatus:!f,value:!(value1,value2)),id:filter_id_1,ownState:()),"
+        "filter_id_2:(extraFormData:(filters:!((col:column_name_2,op:IN,"
+        "val:!(value3,value4)))),filterState:(label:column_name_2,"
+        "validateStatus:!f,value:!(value3,value4)),id:filter_id_2,ownState:()))"
     )
+    assert result == expected
+    assert warnings == []
 
 
 def test_report_generate_native_filter_no_values():
@@ -99,9 +111,10 @@ def test_report_generate_native_filter_no_values():
     }
 
 
-def test_get_native_filters_params_invalid_structure():
+def test_get_native_filters_params_missing_filter_values():
     """
-    Test the ``get_native_filters_params`` method with invalid structure.
+    Test the ``get_native_filters_params`` method with missing filterValues.
+    Should handle gracefully by using empty list as default.
     """
     report_schedule = ReportSchedule()
     report_schedule.extra = {
@@ -111,29 +124,85 @@ def test_get_native_filters_params_invalid_structure():
                     "nativeFilterId": "filter_id",
                     "columnName": "column_name",
                     "filterType": "filter_select",
-                    # Missing "filterValues" key
+                    # Missing "filterValues" key - should default to []
                 }
             ]
         }
     }
 
-    with pytest.raises(KeyError, match="'filterValues'"):
-        report_schedule.get_native_filters_params()
+    # Should not raise, should handle gracefully with empty filterValues
+    result, warnings = report_schedule.get_native_filters_params()
+    assert "filter_id" in result
+    assert "column_name" in result
+    assert warnings == []
 
 
-# todo(hugh): how do we want to handle this case?
-# def test_report_generate_native_filter_invalid_filter_id():
-#     """
-#     Test the ``_generate_native_filter`` method with invalid filter id.
-#     """
-#     report_schedule = ReportSchedule()
-#     native_filter_id = None
-#     column_name = "column_name"
-#     values = ["value1", "value2"]
+def test_get_native_filters_params_explicit_none_values():
+    """
+    Test the ``get_native_filters_params`` method with explicit None values.
+    Should handle gracefully by coercing None to empty string/list.
+    """
+    report_schedule = ReportSchedule()
+    report_schedule.extra = {
+        "dashboard": {
+            "nativeFilters": [
+                {
+                    "nativeFilterId": "filter_id",
+                    "columnName": None,  # Explicit None
+                    "filterType": "filter_select",
+                    "filterValues": None,  # Explicit None
+                }
+            ]
+        }
+    }
 
-#     assert report_schedule._generate_native_filter(
-#         native_filter_id, column_name, values
-#     ) == {}
+    # Should not raise TypeError, should handle gracefully
+    result, warnings = report_schedule.get_native_filters_params()
+    assert "filter_id" in result
+    assert warnings == []
+
+
+def test_get_native_filters_params_missing_required_fields():
+    """
+    Test the ``get_native_filters_params`` method with missing required fields.
+    Filters missing nativeFilterId or filterType should be skipped.
+    """
+    report_schedule = ReportSchedule()
+    report_schedule.extra = {
+        "dashboard": {
+            "nativeFilters": [
+                {
+                    # Missing nativeFilterId - should be skipped
+                    "filterType": "filter_select",
+                    "columnName": "column_name",
+                    "filterValues": ["value1"],
+                },
+                {
+                    # Missing filterType - should be skipped
+                    "nativeFilterId": "filter_2",
+                    "columnName": "column_name",
+                    "filterValues": ["value2"],
+                },
+                {
+                    # Valid filter - should be processed
+                    "nativeFilterId": "filter_3",
+                    "filterType": "filter_select",
+                    "columnName": "column_name",
+                    "filterValues": ["value3"],
+                },
+            ]
+        }
+    }
+
+    result, warnings = report_schedule.get_native_filters_params()
+    # Only the valid filter should be in the result
+    assert "filter_3" in result
+    assert "filter_2" not in result
+    assert "value1" not in result
+    assert "value3" in result
+    # Two malformed filters should generate two warnings
+    assert len(warnings) == 2
+    assert all("Skipping malformed native filter" in w for w in warnings)
 
 
 def test_report_generate_native_filter():
@@ -173,7 +242,9 @@ def test_get_native_filters_params_empty():
     report_schedule = ReportSchedule()
     report_schedule.extra = {}
 
-    assert report_schedule.get_native_filters_params() == "()"
+    result, warnings = report_schedule.get_native_filters_params()
+    assert result == "()"
+    assert warnings == []
 
 
 def test_get_native_filters_params_no_native_filters():
@@ -183,7 +254,9 @@ def test_get_native_filters_params_no_native_filters():
     report_schedule = ReportSchedule()
     report_schedule.extra = {"dashboard": {"nativeFilters": []}}
 
-    assert report_schedule.get_native_filters_params() == "()"
+    result, warnings = report_schedule.get_native_filters_params()
+    assert result == "()"
+    assert warnings == []
 
 
 def test_report_generate_native_filter_empty_values():