eschutho commented on code in PR #43896:
URL: https://github.com/apache/superset/pull/43896#discussion_r4223029928
##########
superset/views/error_handling.py:
##########
@@ -318,7 +318,15 @@ def show_unexpected_exception(ex: Exception) ->
FlaskResponse:
if "text/html" in request.accept_mimetypes and not app.config["DEBUG"]:
path = files("superset") / "static/assets/500.html"
- return send_file(path, max_age=0), 500
+ # Try to serve HTML file; fall back to JSON if not built. This is
the
+ # last-resort handler, so a missing ``500.html`` (a webpack
artifact
+ # absent in API-only/unbuilt deployments) must not raise its own
+ # ``FileNotFoundError`` and collapse the response to a bare 500
with
+ # no SIP-40 body.
+ try:
+ return send_file(path, max_age=0), 500
+ except FileNotFoundError:
+ pass
Review Comment:
Valid, thanks. Fixed in 13c09837e8: the last-resort
`show_unexpected_exception` handler now catches `OSError` around `send_file`
rather than only `FileNotFoundError`. Before the change I confirmed that
`PermissionError`, `NotADirectoryError` and `IsADirectoryError` all escaped it.
The test is now parametrized over all four. I left the sibling handlers alone:
if their `send_file` raises, Flask routes the error to this handler, so they
still get a SIP-40 JSON 500 rather than a bare one (I checked this for the
`CommandException` and `SupersetException` handlers).
##########
superset/utils/error_sanitization.py:
##########
@@ -100,38 +116,75 @@ def is_sanitization_required() -> bool:
# Never let identifying the principal break the error handler itself.
logger.warning(
"Could not resolve the request principal while deciding whether to
"
- "sanitize an error response; treating it as a non-guest request.",
+ "sanitize an error response; falling back to the presence of a
guest "
+ "token.",
exc_info=True,
)
- return False
+ if not has_request_context():
+ # No request to inspect (e.g. a Celery worker running under
+ # ``override_user``). Fail closed: a guest principal may be active
+ # and the redacted payload is delivered to the embedded viewer.
+ return True
+ # ``.get`` on the config keeps the header read from raising even if the
+ # key is somehow absent.
+ header_name = current_app.config.get("GUEST_TOKEN_HEADER_NAME")
+ if header_name and request.headers.get(header_name):
+ return True
+ try:
+ # ``request.form`` parses the body lazily on first access. A body
+ # whose first parse (in the request loader) already failed part-way
+ # is left partially consumed and re-parses as an empty form; no
guest
+ # was authenticated on such a request, so it is treated as
token-free.
+ return bool(request.form.get("guest_token"))
Review Comment:
I'm leaving this one as is on purpose. The two reads look alike but have
different contracts:
- `request_loader` (and the copy in `get_guest_user_from_request`) is the
authentication path. It may raise. For example, an oversized body raises a 413
there, which is the right response.
- This fallback runs inside the error handler after resolving the principal
has already failed, so it must never raise, and it fails closed (redacts) when
the body can't be read. That's why it uses `current_app.config.get(...)` rather
than `get_conf()[...]`, which can raise `KeyError`, and why the form read has
its own broad `except`.
A shared helper would have to pick one of those two behaviours. That would
either make this fallback able to raise again, which is the bug this PR closes,
or make authentication swallow body errors, which changes auth behaviour and is
out of scope here. The things that could drift are the
`GUEST_TOKEN_HEADER_NAME` config key and the `guest_token` form field. Both are
part of the embedded SDK's request format, and the fallback tests build their
requests from the same config key and field name.
--
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]
