mikebridge opened a new pull request, #44090:
URL: https://github.com/apache/superset/pull/44090
### SUMMARY
`json_error_response` sets `payload["error"]` only for exact `str`
arguments. A flask-babel `lazy_gettext` proxy (LazyString) fails that
`isinstance` check, 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 in `superset/views/core.py` ship 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:
- **Call sites** switch to the module's eager `__()` alias (`core.py`
imports `gettext as __, lazy_gettext as _`, and the neighboring sites already
use `__` — these were the stragglers).
- **Root cause**: `json_error_response` now coerces non-`str`,
non-`SupersetError` values with `str(...)`, so any future lazy call produces a
correct body instead of `{}`. 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 `_` to
`lazy_gettext` can have the bug; several files import `_` as eager `gettext`
and are fine) confirmed `views/core.py` holds 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 into `payload["error"]` (fails pre-fix), the plain-`str` path is
unchanged, and a binding-aware source scan asserts no
`json_error_response(_(...))` remains in any module where `_` is `lazy_gettext`
(fails pre-fix on the five sites). Reverting the production change flips the
mechanism test and the tripwire.
### 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
--
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]
