Repository navigation
fix(embedded): fail closed when the guest-user check can't resolve a principal - #43896
Conversation
Code Review Agent Run #48fb6eActionable Suggestions - 0Additional Suggestions - 2
Review 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43896 +/- ##
==========================================
- Coverage 82.72% 82.71% -0.01%
==========================================
Files 3011 3011
Lines 188350 188337 -13
Branches 43725 43715 -10
==========================================
- Hits 155819 155791 -28
- Misses 29761 29775 +14
- Partials 2770 2771 +1
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:
|
github-actions
Bot
added
the
requires:rebase
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Really solid tracing of the fail-open gap, and the resolve-once threading is a nice fix for the half-redaction bug too.
One thing before this merges though: bito's right about request.form.get("guest_token") in the fallback. I checked werkzeug's formparser.py/sansio/multipart.py, and an oversized or malformed multipart body raises RequestEntityTooLarge on first access to .form, lazily. That's inside the except block whose entire point is "must never raise," so a request that trips both the original failure and has a bad body escapes right back into the crash class this PR exists to close. Needs its own try/except around the form read (fail closed on any exception there, same as the no-request-context branch).
Smaller one: in test_send_chart_response_does_not_half_redact_guest_query_error, the side_effect=[True, RuntimeError(...)] second value never fires since is_sanitization_required() only runs once now and threads the result down, so the raise path the docstring describes isn't actually exercised. Doesn't block, but worth a single raising side_effect to match what the test claims to cover.
|
Thanks @rusackas — both addressed in 197954e (after merging master to clear the conflict):
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| except FileNotFoundError: | ||
| pass |
There was a problem hiding this comment.
Suggestion: send_file can raise other filesystem errors, such as PermissionError or NotADirectoryError, which still escape this last-resort error handler.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Possible bug
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/error_handling.py
**Line:** 328:329
**Comment:**
*Possible Bug: `send_file` can raise other filesystem errors, such as `PermissionError` or `NotADirectoryError`, which still escape this last-resort error handler.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Valid, thanks. Fixed in 13c0983: 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).
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 13c0983.
The fallback now catches OSError from send_file and continues to the SIP-40 JSON error response, covering filesystem errors such as PermissionError and NotADirectoryError.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6f36e71.
The HTML error-page fallback now catches OSError from send_file and falls through to the SIP-40 JSON error response.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
The flagged issue is correct: the current error handling in Would you like me to fetch all other comments on this PR to validate them and implement a comprehensive fix for all identified issues? |
|
Follow-up in 6eba8b3: I've broadened the form-read guard to Tests: the unreadable-body test now uses a body with no token, so it can only pass by failing closed. It's parametrized over a real werkzeug |
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #0aa653Actionable 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 |
6eba8b3 to
17d2b14
Compare
|
@rusackas I've rebased onto the latest master to pick up the upstream mypy fix in
Could you take another look when you have a moment? |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #635ee4
Actionable Suggestions - 1
-
superset/utils/error_sanitization.py - 1
- Duplicated guest-token detection · Line 130-138
Additional Suggestions - 2
-
tests/unit_tests/views/test_error_handling.py - 1
-
Missing local var type hints · Line 426-444New test introduces unannotated local variables (`test_app`, `client`, `response`, `payload`). BITO.md adaptive rule 13153 requires explicit type annotations on all local variables in test files. Add them (e.g. `test_app: Flask`, `client: FlaskClient`, `response: Response`, `payload: dict[str, Any]`).
-
-
superset/views/error_handling.py - 1
-
Duplicated fallback logic · Line 326-329This try/except `send_file`/`FileNotFoundError` fallback now appears verbatim in four handlers (`show_superset_exception`, `show_http_exception`, `show_command_exception`, `show_unexpected_exception`). If the fallback semantics change (e.g. also catching `IsADirectoryError` or logging), all four must be updated in lockstep. Consider a shared helper to keep the last-resort behavior in one place.
-
Review Details
-
Files reviewed - 6 · Commit Range:
581ca2b..17d2b14- superset/charts/data/api.py
- superset/utils/error_sanitization.py
- superset/views/error_handling.py
- tests/unit_tests/charts/test_chart_data_api.py
- tests/unit_tests/utils/test_error_sanitization.py
- tests/unit_tests/views/test_error_handling.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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")) |
There was a problem hiding this comment.
The new fallback re-implements the guest-token detection already in security/manager.py (request.headers.get(get_conf()["GUEST_TOKEN_HEADER_NAME"]) or request.form.get("guest_token")). It also diverges: current_app.config.get(...) here vs get_conf()["..."] there. A future change to the header/field name or config access in one place silently desyncs the other. Consider a shared helper.
Code Review Run #635ee4
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
I'm leaving this one as is on purpose. The two reads look alike but have different contracts:
request_loader(and the copy inget_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 thanget_conf()[...], which can raiseKeyError, and why the form read has its own broadexcept.
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.
|
I've gone through the automated review comments from the last round:
@rusackas, your review predates the fixes for your feedback and this round. Could you take another look? |
CodeAnt PR Risk: Low Risk
Assessed commit: |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…principal Follow-up to #43834. That change guarded is_sanitization_required() so a raising user loader could no longer turn error responses into bare 500s, but it failed *open*: when the guest-user check raised it returned False ("do not sanitize"), which can disclose raw engine errors to a genuine embedded guest. The check can raise for a resolvable guest, not only an unresolvable anonymous one: get_guest_user_from_token() calls find_role() (a metadata-DB round trip) outside the request loader's try/except, and is_guest_user() consults the EMBEDDED_SUPERSET feature hook first. On a request whose DB session is already broken -- exactly the state a SQLAlchemyError handler runs in -- a valid guest token then yields the raw driver error instead of a redacted message. Make the fallback incapable of raising and fail closed for anyone presenting a token: - is_sanitization_required(): on failure, inside a request context fall back to whether the request carries a guest token (reading the header or form field cannot raise); redact if it does, leave genuinely anonymous errors untouched. Outside a request context (Celery worker under override_user) fail closed and redact, since the payload is delivered to the embedded viewer. Reworded the docstring as an availability-over- confidentiality trade-off rather than a definitional truth. - Resolve the decision once and thread it down: add an optional `required` param to the sanitize_* helpers so a list is looked up a single time instead of once per error (fixes the N+1 loader calls and WARNING log-spam on multi-error responses). - charts/data/api.py: replace the raw outer is_guest_user() with the guarded, once-resolved decision and reuse it for both the stacktrace pop and the message sanitization, so the block can no longer half-redact (drop the stacktrace while leaking the raw error). - views/error_handling.py: give the last-resort show_unexpected_exception the same try/except FileNotFoundError fallback its siblings have, so a missing 500.html no longer collapses the response to a bare 500 with no SIP-40 body. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>
request.form parses the body lazily on first access, so the guest-token fallback in is_sanitization_required() could itself raise -- werkzeug's RequestEntityTooLarge/ClientDisconnected for an oversized or truncated body, or an OSError from the WSGI input stream or the multipart temp-file spool -- escaping the error handler it exists to protect. Guard the form read and fail closed when the body can't be inspected. Also make test_send_chart_response_does_not_half_redact_guest_query_error exercise the raising path it describes: the decision is resolved once, so use a single raising side_effect and assert one lookup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>
… read The (HTTPException, OSError) catch around the fallback's request.form read was too narrow: when the WSGI server terminates the input stream (e.g. a chunked body under gunicorn), werkzeug reads the server's stream directly, and gunicorn raises ParseException subclasses -- plain Exception, not OSError -- for a malformed chunked trailer. That escaped the error handler as a bare 500. Catch any exception and fail closed. Tests: the unreadable-body case uses a token-free body so it only passes by failing closed, and is parametrized with a server stream that raises a plain Exception subclass to pin the broad catch. Correct the comments on the partially-consumed-body case and on the chart-data sanitize block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>
The last-resort show_unexpected_exception only caught FileNotFoundError around send_file, so an unreadable 500.html (PermissionError, NotADirectoryError, IsADirectoryError) still escaped the handler and collapsed the response to a bare 500. Catch OSError. A sibling handler whose send_file raises also lands here, so it degrades to a SIP-40 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>
13c0983 to
6f36e71
Compare
|
@rusackas Rebased onto master again (no code changes) to pick up the new |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @eschutho, the form read now fails closed on any exception and the test only has one raising side_effect, so both of my points are covered. Leaving the shared helper alone makes sense given the auth path is allowed to raise and this one isn't. LGTM!
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()returnedFalse("do not sanitize") wheneversecurity_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_requestcallsget_guest_user_from_token(...)outside itstry/except, andget_guest_user_from_tokencallsfind_role(...)— a metadata-DB round trip. A valid, unrevoked guest token on a request whose DB session is already broken (aPendingRollbackError, precisely because the error being handled is itself aSQLAlchemyError) makesfind_roleraise.is_guest_userconsultsis_feature_enabled("EMBEDDED_SUPERSET")first, which routes through a deployment-supplied hook that can dereferenceg.userand raise.In either case a genuine embedded guest received the raw engine error instead of a redacted message.
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_dictsalso runs in a Celery worker (tasks/async_queries.py) insideoverride_user(...), where there is no request context andrequest.headerswould itself raise. The fallback is guarded withflask.has_request_context(); with no request there is no handler-of-a-handler concern and a guest principal may be active viaoverride_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.pyalso half-redacted: the raw outeris_guest_user()could succeed while the inner guardedsanitize_error_messagefailed, poppingstacktracebut keeping the rawerror. The sanitize helpers now take an optionalrequired: bool | None = None(None = compute) sosanitize_superset_errors/sanitize_error_dictsresolve the decision once and thread it down;charts/data/api.pycomputes the guarded decision once and reuses it for both thestacktracepop and the message sanitization. Behavior is identical when sanitization is not required.3. Last-resort handler could still 500
show_unexpected_exceptioncalledsend_filefor500.htmlwithout thetry/except FileNotFoundErrorits three siblings have. A missing500.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.pyis_guest_userwas actually called — locks the fail-closed direction;tests/unit_tests/charts/test_chart_data_api.py— the half-redaction case (outer succeeds, inner raises) now redacts bothstacktraceanderrorconsistently.tests/unit_tests/views/test_error_handling.py—show_unexpected_exceptionreturns a SIP-40 JSON body when500.htmlis absent; the class docstring is reworded around the trade-off.Run:
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.pyAll pass;
pre-commit(ruff, ruff-format, mypy, pylint) is green on the changed files.ADDITIONAL INFORMATION