eschutho opened a new pull request, #44204:
URL: https://github.com/apache/superset/pull/44204
### SUMMARY
`BaseReportScheduleCommand._validate_report_extra` and
`_validate_native_filters`
(in `superset/commands/report/base.py`) call `json.loads` on a dashboard's
`position_json` and `json_metadata` without guarding against malformed JSON.
Both methods are reached from `CreateReportScheduleCommand.validate()` and
`UpdateReportScheduleCommand.validate()`, invoked from `POST /api/v1/report/`
and `PUT /api/v1/report/{id}`. When the referenced dashboard (via
`extra.dashboard`) has a `position_json` or `json_metadata` that is not valid
JSON, `json.loads` raises `simplejson.JSONDecodeError`. The report API's
`except` clauses only catch `ReportScheduleNotFoundError`,
`ReportScheduleInvalidError`, `ReportScheduleCreateFailedError`,
`ReportScheduleForbiddenError`/`ReportScheduleUpdateFailedError` and
marshmallow's `ValidationError` — `JSONDecodeError` is none of those, so it
escapes as a raw, opaque HTTP 500 instead of a clean validation error.
Dashboards should not normally reach this state given write-time validation,
but legacy/imported dashboards predating stricter validation can (see
issue/PR
history around #43574 / #43575).
### PROBLEM
Two unguarded `json.loads` sites:
- `_validate_report_extra`: `position_data =
json.loads(dashboard.position_json or "{}")`
- `_validate_native_filters`: `json_metadata =
json.loads(dashboard.json_metadata or "{}")`
The same method already demonstrates the correct pattern for the `anchor`
field (`try/except json.JSONDecodeError`), so this fix simply mirrors it.
### FIX
Wrap both `json.loads` calls in `try/except json.JSONDecodeError` and append
a
marshmallow `ValidationError` to the `exceptions` list (surfaced as a
`ReportScheduleInvalidError` → 422) instead of letting the raw exception
propagate:
- `position_json`: append `ValidationError("extra.dashboard.position_json is
not valid JSON", "extra")` and `return` early (mirroring the existing
`not isinstance(dashboard_state, dict)` early-return branch, since
`position_data` is required by everything below).
- `json_metadata`: append `ValidationError("extra.nativeFilters could not be
validated: dashboard metadata is not valid JSON", "extra")` and `break` out
of the native-filter loop (since `valid_filter_ids` can no longer be
computed).
This touches two sites in the same file for the identical bug class,
mirroring
how #42401 covered four sites in one file.
This is part of the same recurring raw-exception-leak cleanup pipeline as
#42366 and #42401 (raw system/library exception escaping instead of a proper
Superset/marshmallow validation error).
### TESTING INSTRUCTIONS
Added two regression tests in
`tests/unit_tests/commands/report/create_test.py`
(`test_validate_report_extra_malformed_position_json`,
`test_validate_report_extra_malformed_json_metadata`), following the existing
house style in that file. Each drives validation with a dashboard whose
`position_json` / `json_metadata` is non-JSON and asserts a `ValidationError`
(field `extra`) is collected rather than a raw `JSONDecodeError` escaping.
Verified fail-before / pass-after: on pre-fix code both tests fail with an
escaping `simplejson.errors.JSONDecodeError`; after the fix both pass. Full
`tests/unit_tests/commands/report/` suite (381 tests) passes. `ruff check`,
`ruff format --check`, and `mypy` are green on the changed files.
```
pytest tests/unit_tests/commands/report/create_test.py -k malformed
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
