Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Ensure device has not been logged out #467

Merged
merged 30 commits into from
Aug 30, 2023
Merged

[3.x] Ensure device has not been logged out #467

merged 30 commits into from
Aug 30, 2023

Conversation

crynobone
Copy link
Member

Continue from #461

crynobone and others added 4 commits August 22, 2023 15:48
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
This adds a middleware to check that the password has in session is the same as the current users password.

This fixes a security issue where an attacker can keep sending requests to an API using the sanctum auth after the password has been changed.

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
crynobone and others added 4 commits August 22, 2023 16:01
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Copy link
Contributor

@patrickomeara patrickomeara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @crynobone

This solution will log out the current sanctum session, when the password is changed via a sanctum guarded request.

See below recording.

Screen.Recording.2023-08-23.at.11.16.43.mov

There are two reasons.

  • The password hash isn't saved after it is changed like it is when using AuthenticateSession
  • Sanctum requests change default guard from web to sanctum after the first check is done.

I have created a new branch on my test repo that uses this pr branch https://github.com/patrickomeara/sanctum-logout-issue/tree/crynobone-solution

Details below.

src/Http/Middleware/EnsureDeviceHasNotBeenLoggedOut.php Outdated Show resolved Hide resolved
src/Http/Middleware/EnsureDeviceHasNotBeenLoggedOut.php Outdated Show resolved Hide resolved
crynobone and others added 10 commits August 23, 2023 09:50
Co-authored-by: Patrick O'Meara <pat@maintainly.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone marked this pull request as ready for review August 23, 2023 04:44
Copy link
Contributor

@patrickomeara patrickomeara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @crynobone

Does this recent solution deal with changing the password in a sanctum request and leaving the sanctum session logged in?

crynobone and others added 3 commits August 23, 2023 13:28
Co-authored-by: Patrick O'Meara <pat@maintainly.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone
Copy link
Member Author

Does this recent solution deal with changing the password in a sanctum request and leaving the sanctum session logged in?

Fixed

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
crynobone and others added 4 commits August 23, 2023 19:36
@taylorotwell taylorotwell merged commit d64ce69 into 3.x Aug 30, 2023
8 checks passed
@taylorotwell taylorotwell deleted the patch-2 branch August 30, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants