Fix remember-me being silently broken by unrelated logins #6204

Closed

rsertelon wants to merge 1 commit from rsertelon:fix-mfa-remember-me into development

pull from: rsertelon:fix-mfa-remember-me

merge into: bookstack:development

bookstack:development

bookstack:l10n_development

bookstack:release

bookstack:v26-05-branch

bookstack:progress_indicator

bookstack:snyk-action

bookstack:ci_and_static

bookstack:mfa_key

bookstack:phpstan_june26

bookstack:llm_only

bookstack:vectors

bookstack:captcha_example

Conversation 2 Commits 1 Files changed 2 +210 -1

rsertelon commented 2026-08-31 22:44:48 +02:00

First-time contributor

Copy link

Hey there.

I've been using Bookstack for several months now (as a family knowledge base) and stumbled upon the Remember Me not remembering me issue #2985

Being a developer myself, but not in PHP, and having access to Claude AI, I asked it the source of the bug. It found what seems to be the issue.

I'm not sure which policy this project has with AI usage, in any case, I've reviewed the code, and tried to make it as readable and close to guidelines as possible.

Hopefully this helps, tell me if I shouldn't have done that, or if the quality is too low, my goal isn't to take your valuable review time with a bad pull request :)

Thanks,

Hey there. I've been using Bookstack for several months now (as a family knowledge base) and stumbled upon the Remember Me not remembering me issue #2985 Being a developer myself, but not in PHP, and having access to Claude AI, I asked it the source of the bug. It found what seems to be the issue. I'm not sure which policy this project has with AI usage, in any case, I've reviewed the code, and tried to make it as readable and close to guidelines as possible. Hopefully this helps, tell me if I shouldn't have done that, or if the quality is too low, my goal isn't to take your valuable review time with a bad pull request :) Thanks,

rsertelon added 1 commit 2026-08-31 22:44:49 +02:00

Fix remember-me being silently broken by unrelated logins ... cc76b69add

LoginService::attempt() holds a login open pending MFA/email
confirmation via auth()->attempt() + auth()->logout(). Laravel's
logout() unconditionally cycles the user's remember_token if one is
set, silently invalidating a remember-me cookie active elsewhere
(e.g. another device) whenever the user logged in anywhere without
requesting 'remember', MFA or not.

Blank the in-memory token before calling logout() so its "has an
existing token" check is false and it never touches the database,
then restore it in-memory so the real $remember value is honored
below. The probing attempt() call itself also drops 'remember' to
avoid an unnecessary token cycle on the current device.

Covered by tests/Auth/RememberMeTest.php.

Fixes #2985

Signed-off-by: Romain Sertelon <romain@sertelon.fr>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

rsertelon force-pushed fix-mfa-remember-me from cc76b69add to fe91a960cf 2026-09-10 09:31:08 +02:00 Compare

danb commented 2026-09-13 19:19:59 +02:00

Owner

Copy link

Thanks for offering this @rsertelon.
Our guidelines when it comes to AI use can be found at https://www.bookstackapp.com/about/community-rules/#use-of-llmsai

Regardless though, from a quick look at the code changes, I don't think this would address the issue.
Fundamentally, We need to perform a more substantial revamp of the user session handling so that the remember-me is session based instead of user based, and so that we can better manage sessions overall.
For the kinds of changes needed, I'm not keen on having them provided via LLM-based PRs, or from folks without some continued experience in our codebase.

Therefore I'm going to close this off.

Thanks for offering this @rsertelon. Our guidelines when it comes to AI use can be found at https://www.bookstackapp.com/about/community-rules/#use-of-llmsai Regardless though, from a quick look at the code changes, I don't think this would address the issue. Fundamentally, We need to perform a more substantial revamp of the user session handling so that the remember-me is session based instead of user based, and so that we can better manage sessions overall. For the kinds of changes needed, I'm not keen on having them provided via LLM-based PRs, or from folks without some continued experience in our codebase. Therefore I'm going to close this off.

danb closed this pull request 2026-09-13 19:19:59 +02:00

rsertelon commented 2026-09-13 20:17:48 +02:00

Author

First-time contributor

Copy link

Hey @danb Thanks for the AI guidelines, I'm sorry I didn't find them earlier. Thanks for your answer :)

Hey @danb Thanks for the AI guidelines, I'm sorry I didn't find them earlier. Thanks for your answer :)

rsertelon deleted branch fix-mfa-remember-me 2026-09-13 20:17:57 +02:00

Some checks failed

Hide checks

analyse-php / build (pull_request) Has been cancelled

Details

lint-php / build (pull_request) Has been cancelled

Details

test-migrations / build (8.2) (pull_request) Has been cancelled

Details

test-migrations / build (8.3) (pull_request) Has been cancelled

Details

test-migrations / build (8.4) (pull_request) Has been cancelled

Details

test-migrations / build (8.5) (pull_request) Has been cancelled

Details

test-php / build (8.2) (pull_request) Has been cancelled

Details

test-php / build (8.3) (pull_request) Has been cancelled

Details

test-php / build (8.4) (pull_request) Has been cancelled

Details

test-php / build (8.5) (pull_request) Has been cancelled

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Focus: A11y

Accessibility

  

Focus: Admin/Meta

  

Focus: Authentication

  

Focus: Back-End

  

Focus: Database

  

Focus: Design & UX

  

Focus: Editor - Markdown

  

Focus: Editor - WYSIWYG

  

Focus: Export System

  

Focus: Front-End

  

Focus: Translations

  

Focus: View Customization

  

Is: Docs Update

  

Is: Enhancement

  

Is: Priority

  

Is: Security

  

Is: Upstream

  

Status

Blocked

  

Status

Open to discussion

  

Status

Out of scope

  

Status

Pending Validation

  

Type

API Request

  

Type

Bug Report

  

Type

Feature Request

  

Type

Happy feedback

  

Type

Maintenance

  

Type

Question

  

Type

Support

No labels

Focus: A11y

Focus: Admin/Meta

Focus: Authentication

Focus: Back-End

Focus: Database

Focus: Design & UX

Focus: Editor - Markdown

Focus: Editor - WYSIWYG

Focus: Export System

Focus: Front-End

Focus: Translations

Focus: View Customization

Is: Docs Update

Is: Enhancement

Is: Priority

Is: Security

Is: Upstream

Status

Blocked

Status

Open to discussion

Status

Out of scope

Status

Pending Validation

Type

API Request

Type

Bug Report

Type

Feature Request

Type

Happy feedback

Type

Maintenance

Type

Question

Type

Support

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

bookstack/bookstack!6204

No description provided.