Fix severe session data loss during user possession/unpossession#19
Fix severe session data loss during user possession/unpossession#19insign wants to merge 2 commits into
Conversation
This patch removes `Session::invalidate()` and `Session::flush()` from `PossessionManager::logoutAndDestroySession()`. Previously, possessing or unpossessing a user destroyed the entire session, which wiped out active states in other guards, shopping carts, or flash messages. This removes that destructive behavior while correctly maintaining authentication state using Laravel's standard guard logic. A new test `SessionDataTest` is added to guarantee that standard session variables survive the impersonation cycle. Co-authored-by: insign <1113045+insign@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
💡 Codex Review
Possession/src/PossessionManager.php
Line 94 in 3e18330
When possession/unpossession is triggered from a web request, this now logs out one principal and logs in another without replacing the CSRF token because the previous Session::regenerateToken() call was removed. That means forms or scripts that obtained a valid _token before the identity switch can still submit state-changing requests after the switch as the impersonated/restored user; keeping just Session::regenerateToken() would preserve custom session data while invalidating pre-switch tokens.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Security fix required before merge. The data-loss bug is real, but removing |
Thank you for the security feedback. I have updated |
Update `logoutAndDestroySession` to use `Session::regenerate()` and `Session::regenerateToken()` instead of flushing and invalidating. This fixes the data-loss bug (e.g. wiping other guard logins and user session data) while properly maintaining security state rotation against session fixation/CSRF during impersonation transitions. Updated test suite to verify CSRF and session IDs successfully rotate while session data is maintained. Co-authored-by: insign <1113045+insign@users.noreply.github.com>
This PR removes destructive calls to
Session::invalidate()andSession::flush()when possessing or unpossessing a user. This is highly useful because the previous behavior caused severe data loss, completely wiping out developers' custom session data (e.g., shopping carts, multi-step forms) and destroying active logins across other guards.The
PossessionManager::logoutAndDestroySession()now properly relies solely on Laravel's standardAuth::guard()->logout()which securely handles authentication state termination without wiping the entire application session.A new test
SessionDataTestguarantees that custom session keys survive the impersonation lifecycle as expected. No Markdown updates were necessary because this modifies internal behavior fixing a bug, leaving the documented API intact. No unrelated files or cosmetic changes are included.PR created automatically by Jules for task 3667247477557378525 started by @insign