Copilot commented on code in PR #42438:
URL: https://github.com/apache/superset/pull/42438#discussion_r3667845185
##########
tests/unit_tests/views/test_base.py:
##########
@@ -315,3 +318,35 @@ def endpoint(self: BaseSupersetView) -> None:
formatted = template % tuple(args)
assert "5.0.0. Use the following API endpoint instead" in formatted
assert "5.0.0 . Use" not in formatted
+
+
+def test_language_pack_endpoint_is_public(app: "SupersetApp") -> None:
+ """The language pack endpoint must be accessible without authentication.
+
+ Translation data is non-sensitive and the embedded dashboard SPA needs to
+ fetch it with a bare ``fetch()`` (no guest-token header). Previously the
+ endpoint was protected by ``@has_access`` which caused a 302 redirect to
+ ``/login/`` for unauthenticated requests, silently breaking i18n in
+ embedded dashboards (issue #42433).
+
+ When the compiled catalog exists the endpoint returns 200 with JSON.
+ When it is missing the endpoint returns 404 (never a 302 to /login/).
+ """
+ with app.test_client() as client:
+ resp = client.get("/language_pack/en/")
Review Comment:
The test currently allows 404 due to environment-dependent presence of
compiled catalogs, which makes it hard to assert the successful (200 + JSON)
behavior described in the docstring. Consider patching/mocking the underlying
language-pack loader for this test so it deterministically returns a small
catalog, then assert 200 plus content type/JSON shape; keep a separate test for
the 'missing catalog returns 404' path if needed.
##########
tests/unit_tests/views/test_base.py:
##########
@@ -315,3 +318,35 @@ def endpoint(self: BaseSupersetView) -> None:
formatted = template % tuple(args)
assert "5.0.0. Use the following API endpoint instead" in formatted
assert "5.0.0 . Use" not in formatted
+
+
+def test_language_pack_endpoint_is_public(app: "SupersetApp") -> None:
+ """The language pack endpoint must be accessible without authentication.
+
+ Translation data is non-sensitive and the embedded dashboard SPA needs to
+ fetch it with a bare ``fetch()`` (no guest-token header). Previously the
+ endpoint was protected by ``@has_access`` which caused a 302 redirect to
+ ``/login/`` for unauthenticated requests, silently breaking i18n in
+ embedded dashboards (issue #42433).
+
+ When the compiled catalog exists the endpoint returns 200 with JSON.
+ When it is missing the endpoint returns 404 (never a 302 to /login/).
+ """
+ with app.test_client() as client:
+ resp = client.get("/language_pack/en/")
+ # The endpoint must be reachable without authentication.
+ # In the test environment compiled catalogs may not exist, so
+ # a 404 is acceptable; a 302 to /login/ would indicate the old
+ # @has_access guard is still in place.
+ assert resp.status_code in (200, 404)
+ assert resp.status_code != 302
Review Comment:
This check is narrowly keyed to 302, but auth redirects can also surface as
other 3xx codes (e.g., 301/303/307/308 depending on configuration). To make the
regression test more robust, assert that the response is not a redirect (e.g.,
`not resp.is_redirect` / `resp.status_code` not in 300–399) and, if applicable,
assert the `Location` header is not `/login/`.
##########
tests/unit_tests/views/test_base.py:
##########
@@ -315,3 +318,35 @@ def endpoint(self: BaseSupersetView) -> None:
formatted = template % tuple(args)
assert "5.0.0. Use the following API endpoint instead" in formatted
assert "5.0.0 . Use" not in formatted
+
+
+def test_language_pack_endpoint_is_public(app: "SupersetApp") -> None:
+ """The language pack endpoint must be accessible without authentication.
+
+ Translation data is non-sensitive and the embedded dashboard SPA needs to
+ fetch it with a bare ``fetch()`` (no guest-token header). Previously the
+ endpoint was protected by ``@has_access`` which caused a 302 redirect to
+ ``/login/`` for unauthenticated requests, silently breaking i18n in
+ embedded dashboards (issue #42433).
+
+ When the compiled catalog exists the endpoint returns 200 with JSON.
+ When it is missing the endpoint returns 404 (never a 302 to /login/).
+ """
+ with app.test_client() as client:
+ resp = client.get("/language_pack/en/")
+ # The endpoint must be reachable without authentication.
+ # In the test environment compiled catalogs may not exist, so
+ # a 404 is acceptable; a 302 to /login/ would indicate the old
+ # @has_access guard is still in place.
+ assert resp.status_code in (200, 404)
+ assert resp.status_code != 302
+
+
+def test_language_pack_endpoint_rejects_invalid_lang(app: "SupersetApp") ->
None:
+ """Invalid language codes are rejected with 400."""
+ with app.test_client() as client:
+ resp = client.get("/language_pack/../../../etc/passwd/")
+ assert resp.status_code in (400, 404)
+
+ resp = client.get("/language_pack/en-US/injection/")
+ assert resp.status_code in (400, 404)
Review Comment:
These requests include additional slashes in the `<lang>` segment, so Flask
routing will typically fail to match `/language_pack/<lang>/` and return 404
before the view’s language validation runs. As written (and by allowing 404),
the test may pass without ever exercising the rejection logic. Prefer using a
single-path-segment invalid `lang` (e.g., `en-US` if dashes are invalid,
`invalid!!`, overly long strings) and assert a strict 400. If you want to cover
traversal attempts, use an encoded payload that still lands in `<lang>` (or add
a lower-level test that directly validates the language parsing/validation
function).
--
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]
