13

Today I noticed something disturbing while inspecting session files in storage/framework/sessions folder created by Laravel 5.

Here is what happened:

  1. I logged in as user A
  2. I navigated to a page which stores variable X in the Session
  3. I logged out, but did not close the browser.
  4. Session file in storage/framework/sessions was still there, and browser cookie was alive.
  5. I logged in as user B.
  6. The old session file in storage/framework/sessions got deleted and a new session file was there.
  7. I looked into the new session file - surprise! variable X has survived log-out and is still there, accessible for user B!

It leads to security concerns because now user B has access to the data of user A.

While debugging through Laravel source code, I found out that Session Store is never being cleared up during logout/login process. Only login credentials are being removed in Illuminate\Auth\Guard::clearUserDataFromStorage() method, but all the session Store attributes are still there, and then later when $kernel->terminate($request, $response); is called, which in turn leads to Illuminate\Session\Middleware\StartSession::terminate() calling Store::save(), which blindly saves $this->attributes to the new session, ignoring the fact that it now belongs to another user.

From one hand, it seems logical - Laravel has no assumptions about my data and whether I want it to expire together with authentication or not. But it would be great to have it documented somewhere with a solution to attach some sensitive data to authentication object and expire together with it.

This means that I as a programmer am responsible for completely clearing away all the sensitive data from current session when a new (or the same) user is logging in.

Clearing on logging out would not be reliable, because user might never click Logout link but wait for the session to "expire", which for Laravel still does not clear up the session.

One more thing to keep in mind: I should not clear the session too early - there is AntiForgery token which must be present, or else login form will always fail.

I have found a forum topic which also tries to solve somewhat similar problem:

http://laravel.io/forum/04-27-2014-how-to-expire-session-data

I got confused by this:

I had another go at it today and realised what the problem was: Session::flush() does not delete session data that the app creates, such as the shopping cart details

If this is true, then the only way to get completely rid of session would be to use PHP native session_unset() and session_destroy() but I wouldn't want to go that way - I would prefer to find a cleaner, Laravel-ish solution, if possible.

How do I tell Laravel that I want my old session data to be removed together with user authentication data when authentication expires or user logs out?

2 Answers 2

5

In the laravel docs it says you can:

Removing An Item From The Session

Session::forget('key');

Removing All Items From The Session

Session::flush();

You could navigate to the AuthenticatesAndRegistersUsers.php trait and rewrite

   /**
     * Log the user out of the application.
     *
     * @return \Illuminate\Http\Response
     */
    public function getLogout()
    {
        $this->auth->logout();

        return redirect(property_exists($this, 'redirectAfterLogout') ? $this->redirectAfterLogout : '/');
    }

to

   /**
     * Log the user out of the application.
     *
     * @return \Illuminate\Http\Response
     */
    public function getLogout()
    {
        Session::flush();

        $this->auth->logout();

        return redirect(property_exists($this, 'redirectAfterLogout') ? $this->redirectAfterLogout : '/');
    }

I have no idea if this actually work, but give it a try :)

Update

According to this answer here on Stack Overflow, you can set the session to expire on browser close, or after XXX minutes. Used together with the above solution, it should fix the problem?

In config/session.php

   /*
    |--------------------------------------------------------------------------
    | Session Lifetime
    |--------------------------------------------------------------------------
    |
    | Here you may specify the number of minutes that you wish the session
    | to be allowed to remain idle before it expires. If you want them
    | to immediately expire on the browser closing, set that option.
    |
    */

    'lifetime' => 120,

    'expire_on_close' => false
6
  • 1
    Yes, this would work when user logs out on purpose. But in case if he just keeps browser open till session expires, I'll have to manually call those forget and flush functions. It seems, I'm not the only one who have discovered this potential security issue: github.com/laravel/framework/issues/8661 Jun 15, 2015 at 12:31
  • Updated the answer. Does that fix the issue for you?
    – MartinJH
    Jun 15, 2015 at 13:04
  • Yes, if the user closes the browser, then it always works fine and there is no issues at all. The problem is only if user just leaves and then much later another user comes and logs in - then old session variables are present. I guess, I'll have to live with those flush() calls during both logout and login, while there is no better built-in solution in Laravel to specify that I don't want some session variables to stay alive after session has changed. Jun 15, 2015 at 13:26
  • Yeah, okay. I hope you'll find a better solution :) Or that laravel creates it
    – MartinJH
    Jun 15, 2015 at 13:51
  • But if you worry about "much later", then why not set the expire time to something like 1-5 minutes? My bank logs me out after 10 minutes or so of inactive usage. But 2 minutes before auto logout they ask me if I want to renew/extend my session.
    – MartinJH
    Jun 15, 2015 at 13:59
0

I believe this is the correct answer to this question/problem:

When making multiple requests in one test, the state of your laravel application is not reset between the requests. The Auth manager is a singleton in the laravel container, and it keeps a local cache of the resolved auth guards. The resolved auth guards keep a local cache of the authed user.

So, your first request to your api/logout endpoint resolves the auth manager, which resolves the api guard, which stores a references to the authed user whose token you will be revoking.

Now, when you make your second request to /api/user, the already resolved auth manager is pulled from the container, the already resolved api guard is pulled from it's local cache, and the same already resolved user is pulled from the guard's local cache. This is why the second request passes authentication instead of failing it.

When testing auth related stuff with multiple requests in the same test, you need to reset the resolved instances between tests. Also, you can't just unset the resolved auth manager instance, because when it is resolved again, it won't have the extended passport driver defined.

So, the easiest way I've found is to use reflection to unset the protected guards property on the resolved auth manager. You also need to call the logout method on the resolved session guards.

Source: Method Illuminate\Auth\RequestGuard::logout does not exist Laravel Passport

To use that, add this to:

TestCase.php

protected function resetAuth(array $guards = null) : void
{
    $guards = $guards ?: array_keys(config('auth.guards'));

    foreach ($guards as $guard) {
        $guard = $this->app['auth']->guard($guard);

        if ($guard instanceof SessionGuard) {
            $guard->logout();
        }
    }

    $protectedProperty = new \ReflectionProperty($this->app['auth'], 'guards');
    $protectedProperty->setAccessible(true);
    $protectedProperty->setValue($this->app['auth'], []);
}

Then, use it like this:

LoginTest.php

class LoginTest extends TestCase
{
    use DatabaseTransactions, ThrottlesLogins;

    protected $auth_guard = 'web';

    /** @test */
    public function it_can_login()
    {
        $user = $this->user();

        $this->postJson(route('login'), ['email' => $user->email, 'password' => TestCase::AUTH_PASSWORD])
            ->assertStatus(200)
            ->assertJsonStructure([
                'user' => [
                    'id' ,
                    'status',
                    'name',
                    'email',
                    'email_verified_at',
                    'created_at',
                    'updated_at',
                    'photo_url',
                    'roles_list',
                    'roles',
                ],
            ]);

        $this->assertEquals(Auth::check(), true);
        $this->assertEquals(Auth::user()->email, $user->email);
        $this->assertAuthenticated($this->auth_guard);
        $this->assertAuthenticatedAs($user, $this->auth_guard);

        $this->resetAuth();
    }

    /** @test */
    public function it_can_logout()
    {
        $this->actingAs($this->user())
            ->postJson(route('logout'))
            ->assertStatus(204);

        $this->assertGuest($this->auth_guard);

        $this->resetAuth();
    }

    /** @test */
    public function it_should_get_two_cookies_upon_login_without_remember_me()
    {
        $user = $this->user();

        $response = $this->postJson(route('login'), [
            'email' => $user->email,
            'password' => TestCase::AUTH_PASSWORD,
        ]);

        $response->assertCookieNotExpired(Str::slug(config('app.name'), '_').'_session');
        $response->assertCookieNotExpired('XSRF-TOKEN');
        $this->assertEquals(config('session.http_only'), true);

        $this->resetAuth();
    }

    /** @test */
    public function it_should_get_three_cookies_upon_login_with_remember_me()
    {
        $user = $this->user();

        $response = $this->postJson(route('login'), [
            'email' => $user->email,
            'password' => TestCase::AUTH_PASSWORD,
            'remember' => true,
        ]);

        $response->assertCookieNotExpired(Str::slug(config('app.name'), '_').'_session');
        $response->assertCookieNotExpired('XSRF-TOKEN');
        $response->assertCookieNotExpired(Auth::getRecallerName());

        $this->resetAuth();
    }

    /** @test */
    public function it_should_throw_error_422_on_login_attempt_without_email()
    {
        $this->postJson(route('login'), ['email' => '', 'password' => TestCase::AUTH_PASSWORD])
            ->assertStatus(422)
            ->assertJsonStructure(['message', 'errors' => ['email']]);

        $this->assertGuest($this->auth_guard);

        $this->resetAuth();
    }

    /** @test */
    public function it_should_throw_error_422_on_login_attempt_without_password()
    {
        $this->postJson(route('login'), ['email' => $this->adminUser()->email, 'password' => ''])
            ->assertStatus(422)
            ->assertJsonStructure(['message', 'errors' => ['password']]);

        $this->assertGuest($this->auth_guard);

        $this->resetAuth();
    }

    /** @test */
    public function it_should_throw_error_422_on_login_attempt_with_empty_form()
    {
        $this->postJson(route('login'), ['email' => '', 'password' => ''])
            ->assertStatus(422)
            ->assertJsonStructure(['message', 'errors' => ['email', 'password']]);

        $this->assertGuest($this->auth_guard);

        $this->resetAuth();
    }

    /** @test */
    public function it_should_throw_error_401_as_guest_on_protected_routes()
    {
        $this->assertGuest($this->auth_guard);

        $this->getJson(route('me'))
            ->assertStatus(401)
            ->assertJson(['message' => 'Unauthenticated.']);
    }

    /** @test */
    public function it_should_throw_error_429_when_login_attempt_is_throttled()
    {
        $this->resetAuth();

        $throttledUser = factory(User::class, 1)->create()->first();

        foreach (range(0, 9) as $attempt) {
            $this->postJson(route('login'), ['email' => $throttledUser->email, 'password' => "{TestCase::AUTH_PASSWORD}_{$attempt}"]);
        }

        $this->postJson(route('login'), ['email' => $throttledUser->email, 'password' => TestCase::AUTH_PASSWORD . 'k'])
            ->assertStatus(429)
            ->assertJson(['message' => 'Too Many Attempts.']);

        $this->resetAuth();
    }

}

A note about the throttle one. It took me several days to figure out how to ensure that 429 behaviour. Earlier unit tests will increase the number of 'attempts' leading up to the throttling, so you need to resetAuth before the throttle test or the throttle will be triggered at the wrong time and screw the test.

Given the above unit test code, I am using this:

Route::group(['middleware' => ['guest', 'throttle:10,5']], function () { /**/ });

You can observe it working by changing any of those numbers, like 10,5 to 9,5 or 11,5 and watch how it affects the throttle unit test. You can also uncomment the resetAuth method and watch how it also screws the test.

For unit testing anything related to auth, the resetAuth utility method is extremely useful, must-have. Also the knowledge of auth caching in AuthManager is a must-know to make sense of observed behaviour.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.