You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/07/11 18:15:44 UTC
[PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
rusackas opened a new pull request, #41965:
URL: https://github.com/apache/superset/pull/41965
### SUMMARY
Test-only PR pinning the inconsistency reported in #29934: misspelled (wrong trailing-slash) URLs under `/api/v1/security/` are handled inconsistently across sibling endpoints.
Three routes live under the same `/api/v1/security/` prefix but are declared with different slash conventions:
| Endpoint | Declaration | Wrong-slash request | Status on `master` |
|---|---|---|---|
| `login` | `@expose("/login")` (FAB, no trailing slash) | `POST /api/v1/security/login/` | **404** |
| `csrf_token` | `@expose("/csrf_token/")` (trailing slash) | `GET /api/v1/security/csrf_token` | **308** redirect |
| `guest_token` | `@expose("/guest_token/")` (trailing slash) | `POST /api/v1/security/guest_token` | **308** redirect |
Because of the mixed conventions, Werkzeug routing treats a "misspelled" URL three different ways: one endpoint hard-404s, the others silently redirect to the canonical URL. That divergence is exactly the confusion the reporter described.
The added test (`tests/unit_tests/security/api_test.py::test_security_api_trailing_slash_handled_consistently`) asserts the *desired* contract — that a trailing-slash misspelling is handled **uniformly** across the sibling security endpoints — without prescribing which uniform behavior is correct (all-redirect or all-404), only that they agree. No production code is changed.
### How to interpret CI
This is a **test-only** PR that documents current behavior.
- **Red (test fails) on CI** — expected on `master`: the three endpoints return `{404, 308, 308}`, so the consistency assertion fails. This confirms #29934 still reproduces.
- **Green** — the slash conventions have been unified (a real fix landed), and the endpoints now agree.
The failing assertion prints the observed statuses, e.g.:
```
AssertionError: Inconsistent handling of misspelled (wrong trailing-slash) security URLs:
{'/api/v1/security/login/': 404, '/api/v1/security/csrf_token': 308, '/api/v1/security/guest_token': 308}
```
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/security/api_test.py::test_security_api_trailing_slash_handled_consistently -v
```
Runs against a real Superset app (routes registered via `SupersetAppInitializer`) on in-memory sqlite — no external services or DB required. On `master` it fails (red), demonstrating the reported inconsistency.
### ADDITIONAL INFORMATION
- [ ] Has associated issue: Closes #29934
- [x] Test-only change; no production code modified
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3564823903
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for local variables in test files when the project’s type checks are already green and the annotation would add low-value noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
+ statuses = {}
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing type annotations for throwaway local variables in test files when the code is only used within the test body and CI/type-checking already passes.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41965:
URL: https://github.com/apache/superset/pull/41965
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3564822967
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
Review Comment:
Good catch, fixed the comment to describe the two-tuples (method, URL).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4968615129
Thanks @sadpandajoe, fair catch. Just pushed `0d0e9540c7` with both: a control assertion that `/api/v1/security/login` (no slash) is reachable, and `Location` header checks on both 308 redirects.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4970359959
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4948299161
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41965?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 63.97%. Comparing base ([`df209ce`](https://app.codecov.io/gh/apache/superset/commit/df209cedbf851ba7f96e8deff0e94446504e6d1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9443ff5`](https://app.codecov.io/gh/apache/superset/commit/9443ff5f5bf1ffbb38d9d5a24bd53788f263b592?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41965 +/- ##
==========================================
- Coverage 64.77% 63.97% -0.81%
==========================================
Files 2742 2742
Lines 153316 153316
Branches 35143 35143
==========================================
- Hits 99311 98083 -1228
- Misses 52107 53342 +1235
+ Partials 1898 1891 -7
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.07% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.06% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.79% <ø> (-1.64%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.65% <ø> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41965?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4948887996
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4950286930
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4948244980
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3566226551
##########
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:
Good catch, added a control assertion that the canonical `/api/v1/security/login` route (no stray slash) does not 404, so the test actually pins the route existing.
##########
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:
Good catch, added `Location` header assertions for both redirects so a misrouted redirect would actually fail this 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3564823224
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
Review Comment:
This is just a local in a test, and CI's green on typing, so I'd rather not annotate every one of these. Leaving as-is, thanks.
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
+ statuses = {}
Review Comment:
Same here, it's a throwaway local in a test rather than an interface, and typing is passing in CI. Not worth the annotation noise, so I'll leave it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3564775344
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
Review Comment:
**Suggestion:** Add an explicit type annotation for the local collection that stores the endpoint/method tuples. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a newly added Python variable that can be annotated, and it lacks a type hint. That matches the custom rule requiring type hints for relevant variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5d34af4836174cfc993199e38f63c9f0&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=5d34af4836174cfc993199e38f63c9f0&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:** 96:100
**Comment:**
*Custom Rule: Add an explicit type annotation for the local collection that stores the endpoint/method tuples.
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=fa242cb022a8e0ed83d64db73a9b95af0d6b9b2b4bcfb61ea369dad63c5c8632&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=fa242cb022a8e0ed83d64db73a9b95af0d6b9b2b4bcfb61ea369dad63c5c8632&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
+ misspelled = [
+ ("post", "/api/v1/security/login/"),
+ ("get", "/api/v1/security/csrf_token"),
+ ("post", "/api/v1/security/guest_token"),
+ ]
+ statuses = {}
Review Comment:
**Suggestion:** Add a concrete type annotation for the dictionary that maps URLs to status codes. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a newly introduced empty dictionary assigned to a local variable without any annotation, so it violates the rule requiring type hints on relevant variables that can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b35037cae3ac49eda87f9a151356266f&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=b35037cae3ac49eda87f9a151356266f&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:** 101:101
**Comment:**
*Custom Rule: Add a concrete type annotation for the dictionary that maps URLs to status codes.
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=633b91fec593235790910205ea4b8cb2bc324625a98f99fa66217bacd6d8f572&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=633b91fec593235790910205ea4b8cb2bc324625a98f99fa66217bacd6d8f572&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(core): pin trailing-slash inconsistency across security API (#29934) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41965:
URL: https://github.com/apache/superset/pull/41965#discussion_r3564778271
##########
tests/unit_tests/security/api_test.py:
##########
@@ -66,6 +66,50 @@ 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_handled_consistently(client: Any) -> None:
+ """Regression for #29934: sibling ``/api/v1/security/*`` endpoints handle a
+ misspelled (wrong trailing-slash) URL inconsistently.
+
+ Three routes live under the same ``/api/v1/security/`` prefix but are
+ declared with different slash conventions:
+
+ * ``login`` -> ``@expose("/login")`` (no trailing slash, FAB)
+ * ``csrf_token`` -> ``@expose("/csrf_token/")`` (trailing slash, Superset)
+ * ``guest_token`` -> ``@expose("/guest_token/")`` (trailing slash, Superset)
+
+ Because of that mix, a client that gets the slash "wrong" is treated three
+ different ways by Werkzeug routing:
+
+ * ``POST /api/v1/security/login/`` -> 404 (no redirect for the extra slash)
+ * ``GET /api/v1/security/csrf_token`` -> 308 redirect to the canonical URL
+ * ``POST /api/v1/security/guest_token`` -> 308 redirect to the canonical URL
+
+ A user hitting one misspelled security URL gets a clean redirect while
+ another gets a hard 404, which is exactly the inconsistency reported in the
+ issue. This asserts the *desired* contract — that a trailing-slash
+ misspelling is handled uniformly across the sibling endpoints — so it fails
+ (red) on master where the responses diverge, and would pass once the slash
+ conventions are unified. It does not prescribe which uniform behavior is
+ correct (all-redirect or all-404), only that they agree.
+ """
+ # (method, canonical route, misspelled route)
Review Comment:
**Suggestion:** The inline comment is factually incorrect: each tuple only contains two values (method and misspelled URL), not three values including a canonical route. This mismatch can mislead future edits and cause unpacking mistakes when someone relies on the documented tuple shape; update the comment to match the actual data structure. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Misleading comment may confuse future test maintainers.
⚠️ Future edits might incorrectly change tuple structure.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/unit_tests/security/api_test.py and locate function
test_security_api_trailing_slash_handled_consistently at lines 69-110 in the new hunk.
2. Observe the inline comment at line 95 documenting the tuple shape as "(method,
canonical route, misspelled route)".
3. Inspect the misspelled list defined at lines 96-100, where each tuple only contains two
elements: the HTTP method and the misspelled URL.
4. Confirm the loop at lines 101-104 uses "for method, url in misspelled", unpacking
exactly two values; this proves the comment is factually incorrect and could mislead
future edits, even though it does not cause a current runtime failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cd45161b9252441fa65545327086e1db&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=cd45161b9252441fa65545327086e1db&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:** 95:95
**Comment:**
*Comment Mismatch: The inline comment is factually incorrect: each tuple only contains two values (method and misspelled URL), not three values including a canonical route. This mismatch can mislead future edits and cause unpacking mistakes when someone relies on the documented tuple shape; update the comment to match the actual data structure.
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=c5527356897e49541d0d414e0998bbae4951042173c3efecc92d9ffc95fc7244&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41965&comment_hash=c5527356897e49541d0d414e0998bbae4951042173c3efecc92d9ffc95fc7244&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(security): assert intended trailing-slash behavior of security API (#29934) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41965:
URL: https://github.com/apache/superset/pull/41965#issuecomment-4968633059
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 0d0e9540c72f13ec82bb10fbbfb4ed05ac914b6b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a561c68e9cc5d0008358f1e |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41965--superset-docs-preview.netlify.app](https://deploy-preview-41965--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcqxDYAgEADAXb6WCCoE3MAx8B-VBIGAFsa4u3RX3Au5pDNfMAN0sBYb8Wi-iHpf6-3YYMw4sbsEdthIwce9PXI5pGehNpWVSqDSziBK4pzrUepNOPh-) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41965--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org