fix(embedded): don't let guest-user check turn error responses into 500s - #43834
Conversation
The guest-user error redaction added in #42796 runs is_sanitization_required() inside Flask's HTTP error handler (show_http_exception -> json_error_response -> sanitize_superset_errors -> is_sanitization_required). That call resolves the request principal via security_manager.is_guest_user(), which reads g.user and lazily triggers the deployment's Flask-Login user loader. Some loaders (e.g. a JWT request loader) raise rather than fall back to an anonymous user when a request carries no valid credential. Because this now happens inside the error handler, Flask has no handler-of-a-handler: it discards the intended status (a 504, a 404, etc.) and returns a bare 500. So any error response on a request whose principal can't be resolved was rewritten to 500. Guard the single principal-resolution call with a broad except and return 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 broad except is deliberate -- different deployments' loaders raise different exception types, and the invariant being protected is that the error handler must never itself raise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>
Code Review Agent Run #5602f1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43834 +/- ##
===========================================
+ Coverage 66.47% 79.41% +12.94%
===========================================
Files 2895 2895
Lines 167943 167949 +6
Branches 38896 38896
===========================================
+ Hits 111645 133385 +21740
+ Misses 54024 32064 -21960
- Partials 2274 2500 +226
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Approving — the diagnosis is right and this fixes a real production problem. One design question I do think is worth settling before it ships, plus some smaller follow-ups.
The one substantive thing: this guard fails open
Returning False on failure means "do not sanitize", so on the exact path #42796 exists to protect, the failure mode is now disclosure rather than a 500.
The docstring justifies it as a tautology:
A request whose principal cannot be resolved is by definition not an embedded guest viewer
That premise doesn't hold, and there's a concrete counterexample upstream. get_guest_user_from_request catches parse/claim/revocation failures, but its final line is outside the try (security/manager.py:5246):
except Exception: # pylint: disable=broad-except
logger.warning("Invalid guest token", exc_info=True)
return None
return self.get_guest_user_from_token(cast(GuestToken, token))and get_guest_user_from_token (:5345) calls self.find_role(...) — a metadata-DB round trip. So: an embedded viewer with a valid, unexpired, unrevoked token, on an endpoint where g.user hasn't been resolved yet, with the metadata DB degraded — or, quite plausibly here, a session already in PendingRollbackError because the error being handled is a SQLAlchemyError. find_role raises. Before this PR: bare 500, nothing disclosed. After: False, and the untouched message (no such table: sales_pii_2024, permission denied for schema finance_pii) goes to the guest.
Related: the try also covers more than the docstring claims. is_guest_user (:5486) runs if not is_feature_enabled("EMBEDDED_SUPERSET") first, which routes through the deployment-supplied IS_FEATURE_ENABLED_FUNC/GET_FEATURE_FLAGS_FUNC — and config.py's own documented example for that hook dereferences g.user. A raising flag hook now silently disables sanitization for real guests.
I don't think the answer is to flip to True, which would over-sanitize legitimate anonymous errors on precisely the deployments this targets. A syntactic fallback that can't itself raise gets both properties:
except Exception:
logger.warning(..., exc_info=True)
# Couldn't build the principal — fall back to whether the request even
# carries a guest token. Reading headers cannot raise.
return bool(
request.headers.get(get_conf()["GUEST_TOKEN_HEADER_NAME"])
or request.form.get("guest_token")
)Statuses are still preserved (the actual bug), and a request presenting a guest token still gets redacted. Either way, I'd reword the docstring to describe this as a deliberate availability-over-confidentiality trade-off rather than a definitional truth — that phrasing is repeated in both new test docstrings, so it'll propagate.
Smaller things
Three callers inherit the fail-open outside any HTTP error handler, where the "handler has no handler" justification doesn't apply: tasks/async_queries.py:139 and charts/data/api.py:698,700. The async one matters most — sanitize_error_dicts runs in a Celery worker under override_user with a guest principal, so on failure the unredacted engine error is written into the job payload delivered to the embedded viewer. There's no handler-of-a-handler problem in a Celery task; the previous loud failure was arguably correct there.
charts/data/api.py:587 vs :601 can now half-redact. The outer if security_manager.is_guest_user(): is unguarded; the inner sanitize_error_message() is guarded. Outer succeeds (True), inner raises (False) → stacktrace popped but the raw error kept. Threading the already-computed flag down instead of re-resolving removes the possibility.
N+1 principal resolutions, now N+1 logged tracebacks. sanitize_superset_errors checks once at :158, then each sanitize_superset_error re-checks at :133. On the deployment this targets (loader always raises), a 10-error chart-data response invokes the raising loader 11 times and emits 11 WARNING tracebacks with exc_info=True — for one response, on every 404 including scanners and health probes. Resolving once and passing the boolean down fixes this and the half-redaction above together.
Tests pin status only, not the disclosure consequence. Neither new test asserts the patched mock was actually called, and nothing covers the security-relevant case: a request that carries a guest token whose resolution raises. That's the case that would lock in whichever fallback direction you choose.
Follow-up, not this PR
The stated invariant — "the error handler must never itself raise" — is still violated two functions away. views/error_handling.py:295-297, show_unexpected_exception (the last-resort handler) calls send_file without the try/except FileNotFoundError that all three sibling handlers have (:199-202, :236-239, :264-267). 500.html is a webpack artifact absent from an API-only or unbuilt deployment, so any unhandled exception on a browser request yields a bare Werkzeug 500 with no SIP-40 body — the same hole, same file. Worth a follow-up; if the invariant is worth stating, it probably wants enforcing at the handler level (a decorator in set_app_error_handlers, where the intended status is still in scope). Note this PR's test can't catch it: it raises GatewayTimeout, and show_http_exception only attempts send_file for ex.code in {404, 500}.
Housekeeping
The description ends with a 🤖 Generated with Claude Code line — worth stripping, especially on a public apache/superset PR.
3 participants
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()andsanitize_error_message()begin bycalling
is_sanitization_required(), which resolves the request principal viasecurity_manager.is_guest_user(). Readingg.userlazily triggers thedeployment'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 errorhandler (
show_http_exception→json_error_response→sanitize_superset_errors→
is_sanitization_required), there is no handler-of-a-handler: when theprincipal 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 broadexceptand returnsFalseonfailure: 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 broadexcept Exceptionis 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— patchesSupersetSecurityManager.is_guest_userto raise and assertsis_sanitization_required()returnsFalse(does not propagate) and thatsanitize_error_message(...)returns the message unchanged.tests/unit_tests/views/test_error_handling.py— exercises the realerror-handler path: a
werkzeug.exceptions.GatewayTimeout(504) raised on arequest 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:
pytest tests/unit_tests/utils/test_error_sanitization.py \ tests/unit_tests/views/test_error_handling.pyADDITIONAL INFORMATION
Fixes a regression from #42796.
🤖 Generated with Claude Code