eschutho opened a new pull request, #43795: URL: https://github.com/apache/superset/pull/43795
### SUMMARY `SecurityManager.query_context_modified()` (superset/security/manager.py) parsed a chart's stored `query_context` with `json.loads()` and **no exception handling**. Here `json` is `superset.utils.json`, which re-exports `simplejson.JSONDecodeError` (a `ValueError` subclass). The query-context-only chart update path — `ChartUpdateCommand._validate_query_context_datasource()`, used by the report-worker save path — intentionally allows a `query_context` string that fails to parse as JSON to be persisted (it catches `(TypeError, ValueError)` around its own `json.loads` and returns early, deferring validation downstream). Nothing downstream actually handled that malformed value. When a guest user later loads an embedded chart whose stored `query_context` is malformed, `raise_for_access()` calls `query_context_modified()`, which crashed with a **raw `simplejson.JSONDecodeError` (HTTP 500)** instead of the intended 403 (`SupersetSecurityException`, "Guest user cannot modify chart payload"). ### PROBLEM - A malformed stored `query_context` (which the update path is allowed to persist) turns a guest embedded-chart load into an unhandled 500 error. - Every other mismatch check in `query_context_modified()` treats a suspicious payload as tampered (logs a warning and returns `True`, which lets `raise_for_access` deny with a 403). The unparseable-JSON case fell through this net. ### FIX Wrap the `json.loads` call in `try/except (json.JSONDecodeError, TypeError)` — the exact exception tuple already used by the sibling `_validate_child_in_parent_multilayer()` in the same class, which parses a different stored-chart field (`parent_slice.params`) with the same risk shape. On failure, log a warning (matching the function's existing rejection-branch style) and `return True`, so the pre-existing `raise_for_access` call site raises the correct `SupersetSecurityException` 403 automatically. Surgical, single call site. No new exception class, no API-layer change, and `commands/chart/update.py` is deliberately left untouched — its deferral of validation is intentional per its own docstring; this fix only closes the downstream gap. The only behavior change is on the already-broken crash path: a malformed stored `query_context` now correctly denies with 403 instead of crashing with 500 — the same outcome the surrounding code already produces for every other kind of guest-payload mismatch. This continues the raw-exception-leak cleanup series (cf. #42401, #43772). ### TESTING INSTRUCTIONS Added `test_query_context_modified_malformed_stored_query_context` in `tests/unit_tests/security/manager_test.py`, alongside the existing `test_query_context_modified*` tests. It sets `slice_.query_context` to a non-JSON string with a matching `slice_id` so execution reaches `json.loads`, and asserts `query_context_modified()` returns `True`. Verification: - On pre-fix code the new test fails with an unhandled `simplejson.errors.JSONDecodeError`. - With the fix it passes. - Full file green: `pytest tests/unit_tests/security/manager_test.py -q` → 122 passed. - `ruff check` and `ruff format --check` clean on both files. ### 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 <sub>Tracking: [sc-119641](https://app.shortcut.com/preset/story/119641)</sub> -- 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]
