eschutho opened a new pull request, #43896:
URL: https://github.com/apache/superset/pull/43896
### SUMMARY
Follow-up addressing the review of #43834. That PR guarded
`is_sanitization_required()` so a raising user loader could no longer turn
error responses into bare 500s. On review it was pointed out that the guard
**fails open** — it weakens the very feature it protects — and that the same
"an error handler must never itself raise" invariant still has gaps elsewhere.
This PR addresses those three follow-ups.
**1. The fail-open guard could disclose to real embedded guests**
`is_sanitization_required()` returned `False` ("do not sanitize") whenever
`security_manager.is_guest_user()` raised, on the assumption that an
unresolvable principal is "by definition not a guest." That assumption is false
— the check can raise for a *resolvable* guest:
- `get_guest_user_from_request` calls `get_guest_user_from_token(...)`
**outside** its `try/except`, and `get_guest_user_from_token` calls
`find_role(...)` — a metadata-DB round trip. A valid, unrevoked guest token on
a request whose DB session is already broken (a `PendingRollbackError`,
precisely because the error being handled is itself a `SQLAlchemyError`) makes
`find_role` raise.
- `is_guest_user` consults `is_feature_enabled("EMBEDDED_SUPERSET")` first,
which routes through a deployment-supplied hook that can dereference `g.user`
and raise.
In either case a genuine embedded guest received the raw engine error (e.g.
`no such table: ...`) instead of a redacted message — a disclosure that did not
exist before #43834 (it was a 500 then).
The fix makes the fallback **incapable of raising** and **fails closed**
when a token is actually present: on failure, inside a request context it falls
back to whether the request carries a guest token (reading the token
header/form field cannot raise). Anyone presenting a token is redacted; a
genuinely anonymous request keeps its error, so ordinary failures are not
over-sanitized. This is a deliberate availability-over-confidentiality
trade-off, and the docstrings (source + the two tests from #43834) are reworded
to say so rather than claim a definitional truth.
**Celery nuance:** `sanitize_error_dicts` also runs in a Celery worker
(`tasks/async_queries.py`) inside `override_user(...)`, where there is **no
request context** and `request.headers` would itself raise. The fallback is
guarded with `flask.has_request_context()`; with no request there is no
handler-of-a-handler concern and a guest principal may be active via
`override_user`, so it **fails closed and redacts** rather than leaking into
the job payload delivered to the embedded viewer.
**2. Resolve once / thread the boolean (N+1, log-spam, half-redaction)**
`is_sanitization_required()` was re-invoked per error, so a 10-error
response called a raising loader 11× and emitted 11 WARNING tracebacks — for
one response, on every 404 including scanners/health probes.
`charts/data/api.py` also half-redacted: the raw outer `is_guest_user()` could
succeed while the inner guarded `sanitize_error_message` failed, popping
`stacktrace` but keeping the raw `error`. The sanitize helpers now take an
optional `required: bool | None = None` (None = compute) so
`sanitize_superset_errors` / `sanitize_error_dicts` resolve the decision once
and thread it down; `charts/data/api.py` computes the guarded decision once and
reuses it for both the `stacktrace` pop and the message sanitization. Behavior
is identical when sanitization is not required.
**3. Last-resort handler could still 500**
`show_unexpected_exception` called `send_file` for `500.html` without the
`try/except FileNotFoundError` its three siblings have. A missing `500.html` (a
webpack artifact, absent in API-only/unbuilt deployments) yielded a bare
Werkzeug 500 with no SIP-40 body. It now has the same JSON fallback. (A
handler-level decorator enforcing "the error handler must never itself raise"
generally would be a broader follow-up; this PR fixes the concrete gap
minimally, matching the existing sibling handlers.)
Credit to the reviewer's analysis of the fail-open direction, the
resolve-once opportunity, and the last-resort handler gap.
### TESTING INSTRUCTIONS
New unit tests (each verified to fail without its corresponding change and
pass with it):
- `tests/unit_tests/utils/test_error_sanitization.py`
- a request carrying a guest token whose resolution **raises** is still
redacted (header and form-field variants), and asserts the patched
`is_guest_user` was actually called — locks the fail-closed direction;
- no-request-context (Celery-style) resolution failure fails closed and
redacts;
- a genuinely anonymous request whose resolution raises is left untouched.
- `tests/unit_tests/charts/test_chart_data_api.py` — the half-redaction case
(outer succeeds, inner raises) now redacts both `stacktrace` and `error`
consistently.
- `tests/unit_tests/views/test_error_handling.py` —
`show_unexpected_exception` returns a SIP-40 JSON body when `500.html` is
absent; the class docstring is reworded around the trade-off.
Run:
```bash
pytest tests/unit_tests/utils/test_error_sanitization.py \
tests/unit_tests/views/test_error_handling.py \
tests/unit_tests/charts/test_chart_data_api.py \
tests/unit_tests/tasks/test_async_queries.py
```
All pass; `pre-commit` (ruff, ruff-format, mypy, pylint) is green on the
changed 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
--
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]
