Repository navigation
fix(views): error-response bodies must carry their message - #44090
Conversation
Five json_error_response call sites in views/core.py (both permalink-404s
and the create-chart / alter-dashboard / create-dashboard denials) passed
flask-babel's lazy_gettext proxy, which fails the helper's
isinstance(error_details, str) check — so those 403/404 responses have
been shipping an empty {} body: status says denied, payload says nothing.
(A sixth site, the overwrite-chart denial, carries the identical fix on
the in-review apache#44025 branch; fixed here too so the tripwire below can
assert zero.)
Two-layer fix: the sites switch to the module's eager gettext alias, and
json_error_response itself now coerces string-like proxies so any future
lazy call degrades to a correct body instead of {}. Tests pin the
mechanism (a LazyString body survives), the str path, and a binding-aware
source tripwire that keeps lazy _() out of error bodies in any module
binding _ to lazy_gettext. Reverting the production change flips the
mechanism test and the tripwire.
sc-120052
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44090 +/- ##
==========================================
+ Coverage 80.08% 80.14% +0.05%
==========================================
Files 2922 2923 +1
Lines 171962 172510 +548
Branches 39907 40014 +107
==========================================
+ Hits 137717 138250 +533
+ Misses 31664 31662 -2
- Partials 2581 2598 +17
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:
|
Committer-round folds: the coercion arm stringified ANY out-of-contract object — sanitize_error_message only masks for embedded guests, so a future mis-caller would leak str(obj) to normal users where the old behavior leaked nothing. Narrowed to isinstance(LazyString) (the case it exists for), with the type hint widened to match; any other out-of-contract value keeps degrading to an empty body. The source-scan tripwire is now package-anchored (superset.__file__) instead of CWD-relative, so it cannot pass vacuously from another working directory. sc-120052 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #02b85bActionable Suggestions - 0Additional Suggestions - 1
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 |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Adversarial review of the error-body LazyString fix. The production fix is correct: LazyString isn't a str subclass, the new isinstance branch coerces it so the body carries the message instead of degrading to {}, and the deliberate narrowness (only LazyString, not arbitrary objects whose str() could leak internals) is a sound security-conscious choice — I'm not flagging that. Two quality/altitude notes below, both about the guardrails around the fix rather than the fix itself. (Automated review; posted by a human reviewer.)
…ror_response Review round (fitzee): the regex scan matched only the inline json_error_response(_(...)) shape and skipped any module not binding lazy_gettext to exactly `_`. Replaced with an AST classifier that resolves the module's actual lazy Babel bindings (aliases, bare calls, module-attribute calls) and flags any json_error_response call whose argument subtree contains a lazy call — nesting and keyword args included, docstring text inert; 8 classifier corner cases pinned. Variable indirection is documented as out of scope for a static scan: the runtime LazyString coercion (behaviorally pinned above) is the guard for every shape — the scan enforces the call-site convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Code Review Agent Run #fafba8Actionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed 14b900e8e977; no substantive findings. CI is green. Review was based on source and available test evidence; full local integration/browser validation was not performed.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit e797f3d)
SUMMARY
json_error_responsesetspayload["error"]only for exactstrarguments. A flask-babellazy_gettextproxy (LazyString) fails thatisinstancecheck, so the body silently degrades to{}while the status still says "denied" — the client gets a 403/404 with no message. Five live call sites insuperset/views/core.pyship that today: both "permalink state not found" 404s and the create-chart / alter-dashboard / create-dashboard permission denials. (The overwrite-chart denial has the identical fix on the in-review #44025 branch; it is included here as well so the tripwire below can assert zero — identical hunks reconcile cleanly whichever lands first.)Two-layer fix:
__()alias (core.pyimportsgettext as __, lazy_gettext as _, and the neighboring sites already use__— these were the stragglers).json_error_responsenow coercesLazyStringvalues withstr(...), so any future lazy call produces a correct body instead of{}— deliberately narrowed to exactly that case, so any other out-of-contract object keeps degrading to an empty body rather than leakingstr(obj)to the client (per committer review). The eager alias remains the convention; the coercion is the safety net.Found via sc-120011/#44025, where a message-asserting regression test caught the same empty-body shape on a new denial — and turned out to be catching this pre-existing class. A binding-aware repo sweep (only modules binding
_tolazy_gettextcan have the bug; several files import_as eagergettextand are fine) confirmedviews/core.pyholds all the live sites.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — API error bodies only. Before:
{}. After:{"error": "You don't have the rights to create a dashboard"}etc.TESTING INSTRUCTIONS
pytest tests/unit_tests/views/test_error_bodies.py— 3 tests: a LazyString body survives intopayload["error"](fails pre-fix), the plain-strpath is unchanged, and a binding-aware source scan asserts nojson_error_response(_(...))remains in any module where_islazy_gettext(fails pre-fix on the five sites). Reverting the production change flips the mechanism test and the tripwire.ADDITIONAL INFORMATION
🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW