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

[10.x] Remove session on authenticatable deletion v2 #47141

Conversation

Boorinio
Copy link
Contributor

(same as #47139 but fixed the tests)

Hello!

So, if somehow in your application you delete the authenticatable (lets say from the users table) without clearing the session and the user still has the cookie, because of
if (! is_null($id) && $this->user = $this->provider->retrieveById($id)) { $this->fireAuthenticatedEvent($this->user); }

Every @can directive every Auth check (anything that uses the Auth::user) will cause for a db query spamming the database since the user is never set. On top of that if you do any auth checks in your application with Auth::id() it doesn't return null because of
return $this->user() ? $this->user()->getAuthIdentifier() : $this->session->get($this->getName());

Which is kind of a security issue. Not sure if it is only in my codebase but I thought it would be safer to open this PR to make you aware of the issue!

Thanks for your time

@nunomaduro nunomaduro changed the title Remove session on authenticatable deletion v2 [10.x] Remove session on authenticatable deletion v2 May 19, 2023
@taylorotwell taylorotwell merged commit 8fe2f65 into laravel:10.x May 22, 2023
17 checks passed
Comment on lines +265 to +266
$guard->setCookieJar($cookies = m::mock(CookieJar::class));
$cookies->shouldReceive('unqueue')->once();
Copy link
Contributor

@mfn mfn Jun 5, 2023

Choose a reason for hiding this comment

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

I'm seeing a problem here in my application since this change: if I hit a route which requires auth but the request is not auth'd (just anonymous), my app throws now

RuntimeException: Cookie jar has not been set. in /…/vendor/laravel/framework/src/Illuminate/Auth/SessionGuard.php:859

But how can there be a cookie jar for an unauthenticated request?

I feel like the addition of these two lines should not be here because they alter the test condition. If I remove them, then this test too fails with:

Failed asserting that exception of type "RuntimeException" matches expected exception "Illuminate\Auth\AuthenticationException". Message was: "Cookie jar has not been set." at…

@Boorinio WDYT about the scenario?

@taylorotwell
Copy link
Member

Reverted.

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.

None yet

3 participants