eschutho opened a new pull request, #43834:
URL: https://github.com/apache/superset/pull/43834
### SUMMARY
Fixes a regression introduced by #42796, which rewired
`json_error_response()`
to run every error response through the guest-user error sanitization added
for
embedded viewers.
Both `sanitize_superset_errors()` and `sanitize_error_message()` begin by
calling `is_sanitization_required()`, which resolves the request principal
via
`security_manager.is_guest_user()`. Reading `g.user` lazily triggers the
deployment's Flask-Login user loader. Some loaders (for example, a JWT
request
loader that raises when a request carries no valid credential, rather than
falling back to an anonymous user) *raise* instead of returning `None`.
Because `is_sanitization_required()` now runs **inside** Flask's HTTP error
handler (`show_http_exception` → `json_error_response` →
`sanitize_superset_errors`
→ `is_sanitization_required`), there is no handler-of-a-handler: when the
principal lookup raises, Flask discards the intended status (a 504 gateway
timeout, a 404, etc.) and returns a bare **500**. So any error response on a
request whose principal cannot be resolved was silently rewritten to a 500.
The fix guards the single principal-resolution call in
`is_sanitization_required()` with a broad `except` and returns `False` on
failure: a request whose principal cannot be resolved is by definition not an
embedded guest viewer, so there is nothing to redact. The failure is logged
at
warning with `exc_info=True`. A broad `except Exception` is deliberate —
different deployments' user loaders raise different exception types, and the
invariant being protected is simply that *the error handler must never itself
raise*.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only behavior change, covered by unit tests).
### TESTING INSTRUCTIONS
New regression tests were added and verified to fail without the source
change
and pass with it:
- `tests/unit_tests/utils/test_error_sanitization.py` — patches
`SupersetSecurityManager.is_guest_user` to raise and asserts
`is_sanitization_required()` returns `False` (does not propagate) and that
`sanitize_error_message(...)` returns the message unchanged.
- `tests/unit_tests/views/test_error_handling.py` — exercises the real
error-handler path: a `werkzeug.exceptions.GatewayTimeout` (504) raised on
a
request whose loader raises keeps its 504 status instead of becoming a 500,
and a direct `json_error_response(..., status=504)` call keeps its status.
The existing positive tests confirming that a genuine guest user still
triggers
sanitization remain green.
Run:
```bash
pytest tests/unit_tests/utils/test_error_sanitization.py \
tests/unit_tests/views/test_error_handling.py
```
### 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
Fixes a regression from #42796.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
