Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,31 @@ test('non-text chart shows screenshot width and message content', () => {
expect(screen.getByText('Screenshot width')).toBeInTheDocument();
});

test('screenshot width input preserves a typed zero instead of dropping it', () => {
const lineChartProps = {
...defaultProps,
dashboardId: undefined,
chart: { id: 1, sliceFormData: { viz_type: VizType.Line } },
chartName: 'My Line Chart',
creationMethod: 'charts' as const,
};
render(<ReportModal {...lineChartProps} />, { useRedux: true });

const widthInput = screen.getByPlaceholderText(
'Input custom width in pixels',
);

// The old `|| null` / `|| ''` logic silently coerced a typed 0 to null, so the
// invalid width was swallowed instead of being submitted and surfaced by the
// server's min-width validation. The field must preserve the literal value.
userEvent.type(widthInput, '0');
expect(widthInput).toHaveDisplayValue('0');

// Clearing the field still yields an empty value (parsed NaN → null).
userEvent.clear(widthInput);
expect(widthInput).toHaveDisplayValue('');
});

test('dashboard report hides message content section', () => {
const dashboardProps = {
...defaultProps,
Expand Down
5 changes: 3 additions & 2 deletions superset-frontend/src/features/reports/ReportModal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -296,11 +296,12 @@ function ReportModal({
<Input
type="number"
name="custom_width"
value={currentReport?.custom_width || ''}
value={currentReport?.custom_width ?? ''}
placeholder={t('Input custom width in pixels')}
onChange={(event: ChangeEvent<HTMLInputElement>) => {
const parsedWidth = parseInt(event.target.value, 10);
setCurrentReport({
custom_width: parseInt(event.target.value, 10) || null,
custom_width: Number.isNaN(parsedWidth) ? null : parsedWidth,
});
}}
/>
Expand Down
18 changes: 14 additions & 4 deletions superset/reports/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
from superset.views.base import BaseFilter


def _escape_like(value: str) -> str:
"""
Escape LIKE/ILIKE wildcard characters so user-supplied search text is matched
literally instead of being interpreted as wildcards (e.g. ``%`` and ``_``).
"""
return value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated


class ReportScheduleFilter(BaseFilter): # pylint: disable=too-few-public-methods
def apply(self, query: Query, value: Any) -> Query:
if security_manager.can_access_all_datasources():
Expand All @@ -47,11 +55,13 @@ class ReportScheduleAllTextFilter(BaseFilter): # pylint: disable=too-few-public
def apply(self, query: Query, value: Any) -> Query:
if not value:
return query
ilike_value = f"%{value}%"
# ``value`` may arrive as a non-string (e.g. an int in the API ``filters``
# array); coerce it so escaping never raises on ``.replace``.
ilike_value = f"%{_escape_like(str(value))}%"
return query.filter(
or_(
ReportSchedule.name.ilike(ilike_value),
ReportSchedule.description.ilike(ilike_value),
ReportSchedule.sql.ilike(ilike_value),
ReportSchedule.name.ilike(ilike_value, escape="\\"),
ReportSchedule.description.ilike(ilike_value, escape="\\"),
ReportSchedule.sql.ilike(ilike_value, escape="\\"),
)
)
37 changes: 37 additions & 0 deletions tests/unit_tests/commands/report/update_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,43 @@ def test_ownership_check_raises_forbidden(mocker: MockerFixture) -> None:
cmd.validate()


# --- Dashboard extra (activeTabs) validation on update ---


def test_update_rejects_invalid_active_tab_ids(mocker: MockerFixture) -> None:
"""On PUT, activeTabs must be validated against the model's dashboard layout.

The dashboard is not in the payload, so validation must fall back to the
existing model's dashboard; tab ids absent from position_json are rejected.
"""
model = _make_model(mocker, model_type=ReportScheduleType.REPORT, database_id=None)
model.dashboard.position_json = '{"TAB-valid": {}}'
_setup_mocks(mocker, model)

cmd = UpdateReportScheduleCommand(
model_id=1,
data={"extra": {"dashboard": {"activeTabs": ["TAB-missing"]}}},
)
with pytest.raises(ReportScheduleInvalidError) as exc_info:
cmd.validate()
messages = _get_validation_messages(exc_info)
assert "extra" in messages
assert "invalid tab ids" in messages["extra"].lower()


def test_update_accepts_valid_active_tab_ids(mocker: MockerFixture) -> None:
"""A tab id present in the model dashboard's position_json passes validation."""
model = _make_model(mocker, model_type=ReportScheduleType.REPORT, database_id=None)
model.dashboard.position_json = '{"TAB-valid": {}}'
_setup_mocks(mocker, model)

cmd = UpdateReportScheduleCommand(
model_id=1,
data={"extra": {"dashboard": {"activeTabs": ["TAB-valid"]}}},
)
cmd.validate() # should not raise


# --- Database not found for alert ---


Expand Down
39 changes: 39 additions & 0 deletions tests/unit_tests/reports/dao_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,42 @@ def test_find_last_error_notification_returns_log_when_only_errors(

result = ReportScheduleDAO.find_last_error_notification(schedule)
assert result is error_log


@patch("superset.daos.report.ReportSchedule")
@patch("superset.daos.report.db")
def test_find_by_extra_metadata_escapes_like_wildcards(
mock_db: MagicMock, mock_report_schedule: MagicMock
) -> None:
"""LIKE wildcards in the slug must be escaped so they match literally."""
from superset.daos.report import ReportScheduleDAO

expected: list[MagicMock] = [MagicMock()]
mock_db.session.query.return_value.filter.return_value.all.return_value = expected

result = ReportScheduleDAO.find_by_extra_metadata("100%_off")

assert result is expected
# autoescape=True is what neutralises the LIKE wildcards in the slug
mock_report_schedule.extra_json.contains.assert_called_once_with(
"100%_off", autoescape=True
)
Comment thread
sadpandajoe marked this conversation as resolved.
Outdated


@patch("superset.daos.report.ReportSchedule")
@patch("superset.daos.report.db")
def test_find_by_native_filter_id_escapes_like_wildcards(
mock_db: MagicMock, mock_report_schedule: MagicMock
) -> None:
"""LIKE wildcards in the filter id must be escaped so they match literally."""
from superset.daos.report import ReportScheduleDAO

expected: list[MagicMock] = [MagicMock()]
mock_db.session.query.return_value.filter.return_value.all.return_value = expected

result = ReportScheduleDAO.find_by_native_filter_id("NATIVE_FILTER-%_x")

assert result is expected
mock_report_schedule.extra_json.contains.assert_called_once_with(
"NATIVE_FILTER-%_x", autoescape=True
)
44 changes: 44 additions & 0 deletions tests/unit_tests/reports/filters_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,47 @@ def test_report_schedule_all_text_filter_applies_ilike() -> None:
f = ReportScheduleAllTextFilter("name", MagicMock())
f.apply(query, "test")
query.filter.assert_called_once()


@patch("superset.reports.filters.or_")
@patch("superset.reports.filters.ReportSchedule")
def test_report_schedule_all_text_filter_escapes_wildcards(
mock_report_schedule: MagicMock, mock_or: MagicMock
) -> None:
"""User-supplied wildcards must be escaped so they match literally."""
from superset.reports.filters import ReportScheduleAllTextFilter

query = MagicMock()
f = ReportScheduleAllTextFilter("name", MagicMock())
# raw input contains every LIKE special character plus a backslash
f.apply(query, "50%_off\\promo")

# %, _ and \ are all escaped, and the literal is wrapped for a "contains" match
expected = "%50\\%\\_off\\\\promo%"
Comment thread
sadpandajoe marked this conversation as resolved.
for column in (
mock_report_schedule.name,
mock_report_schedule.description,
mock_report_schedule.sql,
):
column.ilike.assert_called_once_with(expected, escape="\\")


@patch("superset.reports.filters.or_")
@patch("superset.reports.filters.ReportSchedule")
def test_report_schedule_all_text_filter_coerces_non_string(
mock_report_schedule: MagicMock, mock_or: MagicMock
) -> None:
"""A non-string value (e.g. an int) must not raise when escaping."""
from superset.reports.filters import ReportScheduleAllTextFilter

query = MagicMock()
f = ReportScheduleAllTextFilter("name", MagicMock())
f.apply(query, 50)

expected = "%50%"
for column in (
mock_report_schedule.name,
mock_report_schedule.description,
mock_report_schedule.sql,
):
column.ilike.assert_called_once_with(expected, escape="\\")
Loading