eschutho opened a new pull request, #43687:
URL: https://github.com/apache/superset/pull/43687
### SUMMARY
`CreateChartCommand.__init__` parses the client-supplied `params` field with
`json.loads` (backed by `simplejson`) without guarding it. The `params`
marshmallow field on `POST /api/v1/chart` is a plain `fields.String()` with no
JSON-validity check, so a client that posts a malformed JSON string in `params`
triggers a raw `simplejson.errors.JSONDecodeError` (a `ValueError` subclass)
straight out of `__init__` — before `run()`'s `@transaction` decorator or
`validate()` ever execute.
The `create()` handler in `superset/charts/api.py` wraps
`CreateChartCommand(item).run()` in `try/except DashboardsForbiddenError /
ChartInvalidError / ChartCreateFailedError`, none of which catch
`JSONDecodeError`, so it falls through to Flask-AppBuilder's `@safe` decorator
as an opaque **500**. Malformed client input should be a **422**, not a server
error.
**Problem**
```python
def __init__(self, data: dict[str, Any]):
self._properties = data.copy()
if params_str := self._properties.get("params"):
params = json.loads(params_str) # raw JSONDecodeError leaks
here
if isinstance(params, dict) and "viz_type" in params:
self._properties.setdefault("viz_type", params["viz_type"])
```
**Fix**
Wrap the parse and re-raise as a `ChartInvalidError` composed from a new
small `ChartParamsInvalidJSONValidationError` (field `params`), mirroring the
established `*ValidationError` idiom already used throughout
`commands/chart/exceptions.py` (e.g.
`ChartQueryContextDatasourceMismatchValidationError`):
```python
if params_str := self._properties.get("params"):
try:
params = json.loads(params_str)
except json.JSONDecodeError as ex:
raise ChartInvalidError(
exceptions=[ChartParamsInvalidJSONValidationError()]
) from ex
if isinstance(params, dict) and "viz_type" in params:
self._properties.setdefault("viz_type", params["viz_type"])
```
Because the command is constructed inside the same `try:` block as `.run()`,
`charts/api.py`'s existing `except ChartInvalidError as ex: return
self.response_422(...)` already covers it — **no change to `charts/api.py` is
required**.
This follows the same sibling patterns already established in this codebase
for the exact same field / bug class:
- `commands/chart/export.py` already does `try: payload["params"] =
json.loads(payload["params"]) except json.JSONDecodeError: logger.info(...)`.
-
`commands/chart/update.py::UpdateChartCommand._validate_query_context_datasource`
already catches `(TypeError, ValueError)` around an equivalent `json.loads`
and treats it as unverifiable input.
Part of the ongoing raw-exception-cleanup series; the most recent PR in the
same series is #43651 ("catch sqlglot ParseError when parsing RLS predicates").
Referenced for context only — this change stands on its own and has no
dependency on it.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend error-handling change.
### TESTING INSTRUCTIONS
New unit test file `tests/unit_tests/commands/chart/create_test.py` covers
both paths:
- `test_init_with_invalid_json_params_raises_chart_invalid_error` — a
malformed `params` string raises `ChartInvalidError` (not a raw
`JSONDecodeError`), with a `ChartParamsInvalidJSONValidationError` present in
the composed exceptions.
- `test_init_with_valid_json_params_populates_viz_type` — a valid `params`
JSON string still falls back to its `viz_type` (happy path unchanged).
Run:
```bash
python3 -m pytest tests/unit_tests/commands/chart/create_test.py -q
```
Verified fail-before / pass-after: against pre-fix source the invalid-JSON
test fails with the raw `simplejson.errors.JSONDecodeError` leaking out of
`__init__`; with the fix applied both tests pass (`2 passed`). `ruff check`,
`ruff format`, and `mypy` (via pre-commit) are clean on the changed files.
To reproduce the original 500 manually: `POST /api/v1/chart` with a body
containing `"params": "{not valid json"` (plus a valid `datasource_id` /
`datasource_type`). Before this change the response is a 500; after, it is a
422 with a `params` validation message.
### 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]
