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/05/18 03:20:29 UTC
[PR] test(security): regression test for session cookie after logout (#24713) [superset]
rusackas opened a new pull request, #40201:
URL: https://github.com/apache/superset/pull/40201
### SUMMARY
This is a **test-only PR** opened as a TDD-style validation of issue #24713.
#24713 (filed 2023-07) reports that after calling `/logout/`, the same session cookie can be replayed in another browser to bypass authentication. The cookie should be invalidated server-side at logout.
This PR adds one regression test on the session/logout flow:
1. **`test_session_cookie_invalidated_after_logout`** — logs in as admin, captures the `session` cookie, calls `/logout/`, then replays the captured cookie via a fresh client against a protected endpoint (`api/v1/dashboard/`). Asserts the replay does **not** return 200.
### How to interpret CI
- **CI green** → server-side session invalidation is in place; merging closes #24713 and locks in the regression guard.
- **CI red** → the issue is still live; logout is purely client-side. Fix likely involves bumping the Flask session serializer salt / rotating the session token on logout, or hooking `user_logged_out` in `superset/security/manager.py`.
### TESTING INSTRUCTIONS
\`\`\`bash
pytest tests/integration_tests/security/api_tests.py::TestLogoutSessionInvalidation -v
\`\`\`
### ADDITIONAL INFORMATION
- [x] Has associated issue: closes #24713
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 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(security): regression test for session cookie after logout (#24713) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40201:
URL: https://github.com/apache/superset/pull/40201#issuecomment-4473989400
<!-- Bito Reply -->
The issue is valid and should be addressed. The `set_cookie` call in the test is using the outdated signature with `server_name`, which is no longer supported in the current Flask/Werkzeug version. This causes a `TypeError` and prevents the test from validating the session invalidation logic. To fix this, update the call to use the newer `key, value` signature, optionally including the `domain` keyword argument if needed.
Here's how to fix it:
1. **Locate the call:** In `tests/integration_tests/security/api_tests.py` at line 432, find the line: `replay_client.set_cookie('localhost', name, value)`.
2. **Update the call:** Change it to `replay_client.set_cookie(name, value)` or `replay_client.set_cookie(name, value, domain='localhost')` if the domain is required.
3. **Verify the fix:** Run the test again to ensure it no longer raises a `TypeError` and correctly validates the session invalidation behavior.
After applying this fix, I recommend checking the rest of the PR comments to see if there are other similar issues that need addressing.
**tests/integration_tests/security/api_tests.py**
```
replay_client.set_cookie(name, value)
```
--
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): regression test for session cookie after logout (#24713) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40201:
URL: https://github.com/apache/superset/pull/40201
--
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): regression test for session cookie after logout (#24713) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40201:
URL: https://github.com/apache/superset/pull/40201#issuecomment-4475349743
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): regression test for session cookie after logout (#24713) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40201:
URL: https://github.com/apache/superset/pull/40201#issuecomment-4474083828
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40201?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 55.89%. Comparing base ([`427e7e5`](https://app.codecov.io/gh/apache/superset/commit/427e7e53cd7fe04e0b02633b0e71b2a581bd1dc0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1719d24`](https://app.codecov.io/gh/apache/superset/commit/1719d24a17a6405da981209ec7a87e61bcf7f67c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40201 +/- ##
==========================================
- Coverage 64.16% 55.89% -8.27%
==========================================
Files 2591 2591
Lines 138163 138163
Branches 32048 32048
==========================================
- Hits 88648 77228 -11420
- Misses 47986 60272 +12286
+ Partials 1529 663 -866
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40201/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/40201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.46% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40201/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/40201/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/40201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.15% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/40201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `42.41% <ø> (-18.27%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/40201?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(security): regression test for session cookie after logout (#24713) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40201:
URL: https://github.com/apache/superset/pull/40201#issuecomment-4473933146
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): regression test for session cookie after logout (#24713) [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 #40201:
URL: https://github.com/apache/superset/pull/40201#discussion_r3256154768
##########
tests/integration_tests/security/api_tests.py:
##########
@@ -402,3 +403,36 @@ def test_get_roles_with_specific_test_data(self):
assert sorted(role2_api["user_ids"]) == role2_expected["user_ids"]
assert sorted(role2_api["permission_ids"]) == role2_expected["permission_ids"]
assert role2_api["group_ids"] == role2_expected["group_ids"]
+
+
+class TestLogoutSessionInvalidation(SupersetTestCase):
+ """Regression for #24713: a session cookie captured pre-logout must not grant
+ access after the user logs out. The original report describes copying the
+ session cookie out, calling /logout/, and successfully reusing the cookie in
+ a second browser to bypass authentication."""
+
+ def test_session_cookie_invalidated_after_logout(self):
+ self.login(ADMIN_USERNAME)
+
+ resp_authed = self.client.get("api/v1/dashboard/", follow_redirects=False)
+ assert resp_authed.status_code == 200, (
+ f"Login did not yield an authenticated session "
+ f"(got {resp_authed.status_code})"
+ )
+
+ captured = {
+ c.name: c.value for c in self.client.cookie_jar if c.name == "session"
+ }
+ assert captured, "expected a session cookie after login"
+
+ self.client.get("/logout/", follow_redirects=True)
+
+ replay_client = app.test_client()
+ for name, value in captured.items():
+ replay_client.set_cookie("localhost", name, value)
Review Comment:
**Suggestion:** `set_cookie` is called with the old positional signature (`server_name, key, value`), but this test suite already uses the newer Flask/Werkzeug client signature (`key, value`, optional `domain=` kwarg). In current environments this raises a `TypeError` and the regression test fails before validating logout behavior. Update this call to the supported argument contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Regression test errors before asserting session invalidation.
- ⚠️ Logout/session regression for #24713 remains unguarded.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the regression test
`TestLogoutSessionInvalidation.test_session_cookie_invalidated_after_logout` defined in
`tests/integration_tests/security/api_tests.py:408-437`, which logs in, captures the
session cookie, and then attempts to replay it via a fresh test client.
2. During the replay step, the test constructs `replay_client = app.test_client()` at
`tests/integration_tests/security/api_tests.py:430` and then calls
`replay_client.set_cookie("localhost", name, value)` at
`tests/integration_tests/security/api_tests.py:432`.
3. Elsewhere in the same test suite, the Flask test client is used with the newer
`set_cookie` signature:
`self.client.set_cookie(app.config["GLOBAL_ASYNC_QUERIES_JWT_COOKIE_NAME"], "")` at
`tests/integration_tests/async_events/api_tests.py:136` and
`test_client.set_cookie(app.config["GLOBAL_ASYNC_QUERIES_JWT_COOKIE_NAME"], "foo")` at
`tests/integration_tests/charts/data/api_tests.py:897`, both passing only `key, value`
without a `server_name` positional argument.
4. Under the current Flask/Werkzeug version (confirmed by these existing tests using `key,
value` without failure), `FlaskClient.set_cookie` no longer accepts a leading
`server_name` positional parameter; when `test_session_cookie_invalidated_after_logout`
executes, the three-positional-argument call at `security/api_tests.py:432` raises a
`TypeError` before the request to `api/v1/dashboard/` is made, so the test errors out
instead of validating whether the session cookie is invalidated after logout.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Fintegration_tests%2Fsecurity%2Fapi_tests.py%0A%2A%2ALine%3A%2A%2A%20432%3A432%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60set_cookie%60%20is%20called%20with%20the%20old%20positional%20signature%20%28%60server_name%2C%20key%2C%20value%60%29%2C%20but%20this%20test%20suite%20already%20uses%20the%20newer%20Flask%2FWerkzeug%20client%20signature%20%28%60key%2C%20value%60%2C%20optional%20%60domain%3D%60%20kwarg%29.%20In%20current%20environments%20this%20raises%20a%20%60TypeError%60%20and%20the%20regression%20test%20fails%20before%20validating%20logout%20behavior.%20Update%20this%20call%20to%20the%20supported%20argument%20contract.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20
make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Fintegration_tests%2Fsecurity%2Fapi_tests.py%0A%2A%2ALine%3A%2A%2A%20432%3A432%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60set_cookie%60%20is%20called%20with%20the%20old%20positional%20signature%20%28%60server_name%2C%20key%2C%20value%60%29%2C%20but%20this%20test%20suite%20already%20uses%20the%20newer%20Flask%2FWerkzeug%20client%20signature%20%28%60key%2C%20value%60%2C%20optional%20%60domain%3D%60%20kwarg%29.%20In%20current%20environments%20this%20r
aises%20a%20%60TypeError%60%20and%20the%20regression%20test%20fails%20before%20validating%20logout%20behavior.%20Update%20this%20call%20to%20the%20supported%20argument%20contract.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/integration_tests/security/api_tests.py
**Line:** 432:432
**Comment:**
*Api Mismatch: `set_cookie` is called with the old positional signature (`server_name, key, value`), but this test suite already uses the newer Flask/Werkzeug client signature (`key, value`, optional `domain=` kwarg). In current environments this raises a `TypeError` and the regression test fails before validating logout behavior. Update this call to the supported argument contract.
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%2F40201&comment_hash=e1b970870c5ade3ed540f759eb6b3a17552b89e721ee72ecc9949f5d507ee6c2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40201&comment_hash=e1b970870c5ade3ed540f759eb6b3a17552b89e721ee72ecc9949f5d507ee6c2&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): regression test for session cookie after logout (#24713) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40201:
URL: https://github.com/apache/superset/pull/40201#discussion_r3256317825
##########
tests/integration_tests/security/api_tests.py:
##########
@@ -402,3 +403,36 @@ def test_get_roles_with_specific_test_data(self):
assert sorted(role2_api["user_ids"]) == role2_expected["user_ids"]
assert sorted(role2_api["permission_ids"]) == role2_expected["permission_ids"]
assert role2_api["group_ids"] == role2_expected["group_ids"]
+
+
+class TestLogoutSessionInvalidation(SupersetTestCase):
+ """Regression for #24713: a session cookie captured pre-logout must not grant
+ access after the user logs out. The original report describes copying the
+ session cookie out, calling /logout/, and successfully reusing the cookie in
+ a second browser to bypass authentication."""
+
+ def test_session_cookie_invalidated_after_logout(self):
+ self.login(ADMIN_USERNAME)
+
+ resp_authed = self.client.get("api/v1/dashboard/", follow_redirects=False)
+ assert resp_authed.status_code == 200, (
+ f"Login did not yield an authenticated session "
+ f"(got {resp_authed.status_code})"
+ )
+
+ captured = {
+ c.name: c.value for c in self.client.cookie_jar if c.name == "session"
+ }
+ assert captured, "expected a session cookie after login"
+
+ self.client.get("/logout/", follow_redirects=True)
+
+ replay_client = app.test_client()
+ for name, value in captured.items():
+ replay_client.set_cookie("localhost", name, value)
Review Comment:
You're right — fixed in 94d2aff5. Switched from the old `client.cookie_jar` / `set_cookie(server_name, key, value)` API to the current Werkzeug 2.3+ contract (`client._cookies` for read, `set_cookie(key, value, domain=...)` for write). CI confirmed an `AttributeError: 'FlaskClient' object has no attribute 'cookie_jar'` before this — should now exercise the actual logout/replay path.
--
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