fitzee commented on code in PR #44090: URL: https://github.com/apache/superset/pull/44090#discussion_r4003224607
########## tests/unit_tests/views/test_error_bodies.py: ########## @@ -0,0 +1,84 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""sc-120052: error-response bodies must carry their message. + +``json_error_response`` used to set ``payload["error"]`` only for exact +``str`` arguments; a flask-babel ``lazy_gettext`` proxy (LazyString) failed +that isinstance check and the body silently degraded to ``{}`` while the +status still said "denied". Five live call sites in ``superset/views`` +shipped empty 403/404 bodies that way. Pins: the helper now coerces +string-like proxies, and a binding-aware source scan keeps lazy ``_()`` +out of error bodies at the call sites (the eager alias stays preferred — +the coercion is the safety net, not the convention). +""" + +import pathlib +import re +from typing import cast + +from flask import Response +from flask_babel import lazy_gettext + +from superset.utils import json + + +def test_json_error_response_carries_lazy_string_message(app_context) -> None: + """A LazyString body must not degrade to {} (the sc-120052 bug).""" + from superset.views.error_handling import json_error_response + + resp = cast( + Response, + json_error_response(lazy_gettext("permalink state not found"), status=404), + ) + + body = json.loads(resp.get_data(as_text=True)) + assert body.get("error") == "permalink state not found" + assert resp.status_code == 404 + + +def test_json_error_response_still_carries_plain_str(app_context) -> None: + from superset.views.error_handling import json_error_response + + resp = cast(Response, json_error_response("nope", status=403)) + + assert json.loads(resp.get_data(as_text=True)).get("error") == "nope" + + +def test_no_lazy_gettext_reaches_json_error_response() -> None: + """Binding-aware source tripwire for the call-site convention. + + In any module that binds ``_`` to ``lazy_gettext``, an error body built + as ``json_error_response(_(...))`` is the sc-120052 bug shape. The + helper now coerces (see the tests above), so this scan guards the + convention rather than correctness — if it fires, switch the site to + the eager ``__()`` alias. + """ + import superset + + pattern = re.compile(r"json_error_response\(\s*_\(", re.S) Review Comment: **The source-scan tripwire gives false confidence for the bug class it claims to pin, and does full-tree I/O to do it.** The `lazy_gettext as _` gate (line 79) does cut most false positives, but the `json_error_response\(\s*_\(` pattern only catches the exact *inline* `json_error_response(_(...))` shape. It silently misses every other reintroduction of the same bug: - indirect: `msg = _("..."); json_error_response(msg)` - assignment binding: `_ = lazy_gettext` (doesn't even pass the `import ... as _` gate, so the module is skipped entirely) - a different alias: `from flask_babel import lazy_gettext as _l` - the direct form: `json_error_response(lazy_gettext("x"))` So a real recurrence passes CI green. There's also a residual false-positive surface: the gate matches the literal substring `lazy_gettext as _` even inside a comment/docstring, so an eager-`_` module that merely mentions it and calls `json_error_response(_(...))` would fail the whole unit suite. And mechanically it `rglob('*.py')` + `read_text`s the entire `superset` package on every run (lines 75-77). A convention like "error bodies use eager gettext" is better enforced as a ruff/flake8 rule (AST-based, no false substring matches, no tree walk) than a runtime test — and the ruff rule would actually catch the indirect shapes. The two behavioral unit tests above (lazy + plain str) are the valuable part and pin the real fix. ########## superset/views/core.py: ########## @@ -201,7 +201,7 @@ def explore( # noqa: C901 initial_form_data["url_params"] = dict(url_params) else: return json_error_response( - _("Error: permalink state not found"), status=404 + __("Error: permalink state not found"), status=404 Review Comment: **Altitude: once the helper coerces LazyString, these six `_`→`__` call-site conversions no longer change behavior.** After the `error_handling.py` branch lands, passing lazy `_("...")` to `json_error_response` already yields a correct body (same `sanitize_error_message(str(...))` result as the eager path). So these 6 edits — plus the source-scan test that exists only to keep call sites on `__` — are belt-and-suspenders enforcing a convention, not a correctness fix. That's a legitimate defense-in-depth choice (the coercion is deliberately narrow, so not depending on it is defensible), but it's worth being explicit that the helper fix alone closes the bug; the churn + brittle CI test are the cost of the convention. If the convention is worth keeping, a lint rule would enforce it more cheaply and reliably than the tree-scanning test. -- 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]
