codeant-ai-for-open-source[bot] commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3565610173
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,53 @@ def test_csrf_exempt_blueprints_with_api_key(app: Any,
app_context: None) -> Non
assert "ApiKeyApi" in {blueprint.name for blueprint in
csrf._exempt_blueprints}
+def test_security_api_trailing_slash_matches_route_ownership(client: Any) ->
None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints respond
to
+ a misspelled (wrong trailing-slash) URL differently, and that difference is
+ the *intended* behavior — a Werkzeug routing artifact of who owns each
route,
+ not a bug.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions because they come from different
+ owners:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash)
+ Flask-AppBuilder's own route. Superset does not own or register it, so
+ it inherits FAB's no-trailing-slash convention. Werkzeug hard-404s a
+ request that adds a stray trailing slash to a no-slash route (there is
+ no canonical slashed URL to redirect to).
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash)
+ Superset's own routes, whose trailing-slash URLs are the documented
+ canonical URLs (the Embedded SDK depends on them). Werkzeug
308-redirects
+ a request that omits the trailing slash to the canonical slashed URL.
+
+ Unifying the two would either break the documented ``csrf_token`` /
+ ``guest_token`` URLs the Embedded SDK relies on, or require patching FAB /
+ an app-wide routing change. So the divergence is working-as-designed. This
+ test pins that intended per-route contract so the behavior stays documented
+ and any accidental future change is caught.
+ """
+ # FAB-owned no-trailing-slash route: adding a stray slash hard-404s because
+ # there is no canonical slashed URL to redirect to.
+ response = client.open(
+ "/api/v1/security/login/", method="POST", follow_redirects=False
+ )
+ assert response.status_code == 404
+
+ # Superset-owned canonical trailing-slash routes: omitting the trailing
+ # slash 308-redirects to the documented canonical URL.
+ response = client.open(
+ "/api/v1/security/csrf_token", method="GET", follow_redirects=False
+ )
+ assert response.status_code == 308
+
+ response = client.open(
+ "/api/v1/security/guest_token", method="POST", follow_redirects=False
+ )
+ assert response.status_code == 308
Review Comment:
**Suggestion:** The redirect assertions validate only status code `308` and
never verify the `Location` header, so a regression that redirects to the wrong
destination would still pass. Assert that each response redirects to its
canonical trailing-slash URL (`/api/v1/security/csrf_token/` and
`/api/v1/security/guest_token/`) to pin the contract described in the test
docstring. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- Misrouted csrf_token redirect may pass regression suite.
- guest_token canonical URL changes without breaking this test.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/unit_tests/security/api_test.py and locate
test_security_api_trailing_slash_matches_route_ownership at lines 69-113,
which describes
csrf_token and guest_token as canonical trailing-slash endpoints.
2. Inspect the csrf_token check at lines 105-108:
client.open("/api/v1/security/csrf_token", method="GET",
follow_redirects=False) is called
and the test asserts only response.status_code == 308.
3. Inspect the guest_token check at lines 110-113:
client.open("/api/v1/security/guest_token", method="POST",
follow_redirects=False) is
called and again the test asserts only response.status_code == 308, with no
validation of
response.headers["Location"].
4. Because the test currently verifies only that a 308 redirect is returned,
a future
regression that redirects /api/v1/security/csrf_token or
/api/v1/security/guest_token to
an incorrect destination while still using status code 308 would leave this
regression
test passing, even though the canonical trailing-slash URLs documented in
the test
docstring (lines 79-88) are no longer honored.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d1dd09bfafd64073949dd90b90eb3987&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d1dd09bfafd64073949dd90b90eb3987&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/api_test.py
**Line:** 105:113
**Comment:**
*Incomplete Implementation: The redirect assertions validate only
status code `308` and never verify the `Location` header, so a regression that
redirects to the wrong destination would still pass. Assert that each response
redirects to its canonical trailing-slash URL (`/api/v1/security/csrf_token/`
and `/api/v1/security/guest_token/`) to pin the contract described in the test
docstring.
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=421d37164d1a4c88d99ece638e5c5347ce71ba28568436f7f7412e0b96c13b19&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=421d37164d1a4c88d99ece638e5c5347ce71ba28568436f7f7412e0b96c13b19&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,53 @@ def test_csrf_exempt_blueprints_with_api_key(app: Any,
app_context: None) -> Non
assert "ApiKeyApi" in {blueprint.name for blueprint in
csrf._exempt_blueprints}
+def test_security_api_trailing_slash_matches_route_ownership(client: Any) ->
None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints respond
to
+ a misspelled (wrong trailing-slash) URL differently, and that difference is
+ the *intended* behavior — a Werkzeug routing artifact of who owns each
route,
+ not a bug.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions because they come from different
+ owners:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash)
+ Flask-AppBuilder's own route. Superset does not own or register it, so
+ it inherits FAB's no-trailing-slash convention. Werkzeug hard-404s a
+ request that adds a stray trailing slash to a no-slash route (there is
+ no canonical slashed URL to redirect to).
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash)
+ Superset's own routes, whose trailing-slash URLs are the documented
+ canonical URLs (the Embedded SDK depends on them). Werkzeug
308-redirects
+ a request that omits the trailing slash to the canonical slashed URL.
+
+ Unifying the two would either break the documented ``csrf_token`` /
+ ``guest_token`` URLs the Embedded SDK relies on, or require patching FAB /
+ an app-wide routing change. So the divergence is working-as-designed. This
+ test pins that intended per-route contract so the behavior stays documented
+ and any accidental future change is caught.
+ """
+ # FAB-owned no-trailing-slash route: adding a stray slash hard-404s because
+ # there is no canonical slashed URL to redirect to.
+ response = client.open(
+ "/api/v1/security/login/", method="POST", follow_redirects=False
+ )
+ assert response.status_code == 404
Review Comment:
**Suggestion:** The `login` check only asserts that the malformed `/login/`
URL returns 404, which will also be true if the `login` route is accidentally
removed entirely. Add a control assertion against the canonical
`/api/v1/security/login` path (expecting a non-404 route-level response) so
this test actually verifies trailing-slash behavior of an existing route
instead of a generic not-found result. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- Broken /api/v1/security/login may go undetected here.
- Security API trailing-slash contract not fully regression-tested.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/unit_tests/security/api_test.py and locate
test_security_api_trailing_slash_matches_route_ownership at lines 69-113,
which documents
differing trailing-slash behavior for /api/v1/security/* endpoints.
2. Inspect the login portion of the test at lines 96-101, where
client.open("/api/v1/security/login/", method="POST",
follow_redirects=False) is called
and only assert response.status_code == 404 is performed.
3. Confirm by code inspection that there is no request in this test to the
canonical login
path "/api/v1/security/login" without a trailing slash; the test never
asserts a non-404
response for the correct URL.
4. From this structure, it follows that if a future change removes or
mis-registers the
/api/v1/security/login route so both "/api/v1/security/login" and
"/api/v1/security/login/" return 404, this regression test will still pass,
because it
only validates that the malformed URL returns 404 and does not verify the
existence or
behavior of the canonical login endpoint.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83b5fc5816244b3ca6c2227a09227a01&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83b5fc5816244b3ca6c2227a09227a01&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/api_test.py
**Line:** 98:101
**Comment:**
*Incomplete Implementation: The `login` check only asserts that the
malformed `/login/` URL returns 404, which will also be true if the `login`
route is accidentally removed entirely. Add a control assertion against the
canonical `/api/v1/security/login` path (expecting a non-404 route-level
response) so this test actually verifies trailing-slash behavior of an existing
route instead of a generic not-found result.
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=1a6bad4e8cf5325b52e91df42dfc05a8a1d57f915468e82ffa911d24500bfb98&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=1a6bad4e8cf5325b52e91df42dfc05a8a1d57f915468e82ffa911d24500bfb98&reaction=dislike'>👎</a>
--
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]
