Session changes lost in concurrent requests

trevorgehman commented 4 years ago

Laravel Version: 6.9.0
PHP Version: 7.3.13
Database Driver & Version: Redis

Description:

Since Laravel persists the entire session data array at the end every request, in the case of concurrent requests, any changed session data is very likely to be lost.

https://github.com/laravel/framework/blob/50e43a5728cc65a0697187bdc8e8e39520707ec4/src/Illuminate/Session/Middleware/StartSession.php#L62-L65

Related issues:

Proposed fixes:

Don't save the session in the terminate() method. https://github.com/laravel/ideas/issues/1161. This was changed in 5.8 with this PR: https://github.com/laravel/framework/pull/26410. But that change doesn't fix this issue.
Only save data to session that is actually changed. https://github.com/laravel/framework/issues/18187
Use session locking. https://github.com/rairlie/laravel-locking-session
Check whether session data has changed before saving it. https://github.com/laravel/framework/pull/29410

Steps To Reproduce:

In the below example of concurrent requests, the change in Request A will be lost, overridden when StartSession::saveSession($request) is called at the end of Request B's lifecycle:

    |--------  Request A (changes session variable) --------|
|------------ Request B (no changes to session data) ------------|

Recommendation:

The only complete solution to this problem is to use session locking. A package like this one will work: https://github.com/rairlie/laravel-locking-session. However, that basically makes all your AJAX requests synchronous, which isn't a great solution for modern SPA's.

@taylorotwell has mentioned some form of locking as a solution to the problem in https://github.com/laravel/framework/issues/7549#issuecomment-233631351.

However, I do think we can make other improvements. The fact is, most AJAX requests in a modern SPA aren't modifying session data. Nevertheless, they persist the session data at the end of their lifecycle. This means that a single long-running request makes the session data unable to be changed reliably.

After looking at all these related issues and proposed fixes, I think something like this PR https://github.com/laravel/framework/pull/29410 is the most tenable. It basically only persists the session data if it is dirty. It probably could be altered to be enabled/disable through a config option. It doesn't solve the problem if concurrent requests all want to alter the session data, but it does solve the 90% use case where most requests are just reading session data. The one issue I foresee with this approach is session timeout.

Krisell commented 4 years ago

The dirty-solution should "work" for the cookie-based session driver as well (just skip settings cookie-headers and the browser will keep the old data), however proposed fixes 1 or 2 would not, right?

themsaid commented 4 years ago

I remember looking into this over a year ago and the use case shared wasn't actually a valid use case for session usage, it seems like people who hit this issue are using the session as a key/value store which isn't actually the best tool for the job.

Can you please share a use case where you need to update the session that way?

trevorgehman commented 4 years ago

Our use-case is very simple. A user has access to multiple accounts. We store the ID of the account they are using in the session. They can switch between accounts, in which case we update the account ID in the session.

We see this issue come up when someone switches accounts when some AJAX requests are still pending (in our case, chart data that takes a few seconds to load).

I believe this is an appropriate way to use sessions.

themsaid commented 4 years ago

@trevorgehman I'd store the current account ID in storage, we follow that pattern in multiple laravel projects, don't think using a session for that is needed.

trevorgehman commented 4 years ago

@trevorgehman I'd store the current account ID in storage, we follow that pattern in multiple laravel projects, don't think using a session for that is needed.

I’ll likely do something similar as a workaround to this issue, but I don’t think it’s inappropriate to use sessions this way. I assume you’re tying the storage key to the user ID and session ID?

Regardless, I’m sure there is a solution for my use-case. I’m just trying to summarize this issue because I ran into it and it’s been discussed in the past with no clear conclusion. The underlying issue is simply: one cannot set session data reliably if there are concurrent requests.

ishendyweb commented 4 years ago

Assuming concurrent requests to mean concurrent AJAX requests isn't the right way to think about the problem.

trevorgehman commented 4 years ago

Assuming concurrent requests to mean concurrent AJAX requests isn't the right way to think about the problem.

Can you elaborate? Two AJAX requests happening at the same time. If one of them modifies the session, depending on which request ends first, that change can be lost.

If AJAX requests are synchronous, there isn't an issue. This is why session locking solves the problem: it forces your requests to be synchronous.

ishendyweb commented 4 years ago

@trevorgehman There are other cases as well beside AJAX that would clarify the problem more. Concurrency happens with or without AJAX.

trevorgehman commented 4 years ago

@trevorgehman There are other cases as well beside AJAX that would clarify the problem more. Concurrency happens with or without AJAX.

Correct. Concurrent requests using the same session will exhibit this problem. The most common scenario is AJAX requests. For example, https://github.com/laravel/framework/issues/7549 and https://github.com/laravel/framework/issues/28147.

liepumartins commented 4 years ago

I remember looking into this over a year ago and the use case shared wasn't actually a valid use case for session usage, it seems like people who hit this issue are using the session as a key/value store which isn't actually the best tool for the job.

Can you please share a use case where you need to update the session that way?

It appears I may have this problem. @themsaid Care to explain? I have numerous session based variables like current language, preferred order of items, boolean flag if MOTD has been read, active account context like @trevorgehman mentioned and more. And what did You mean by 'Storage' for storing?

taylorotwell commented 4 years ago

To do team switching in Vapor we store the current user team on in a database column on the user table. Then it is persistent even after the session has expired and you do not have to worry about any concurrent requests stepping on your session data.

ishendywebdeveloper commented 4 years ago

@taylorotwell @themsaid I really think Laravel should explain this clearly somewhere to developers since I was shocked to realize that we really shouldn't use sessions for storing data like this temporarily before persisting it by choice to the DB. I read @taylorotwell comment and @themsaid before and I think I understand what you mean, but right away I switched to a Facebook group where a guy is asking about a way to implement a shopping cart in Laravel and everyone seems to agree on using sessions to store the cart basically while the user is logged in, and they referred to him a Stackoverflow question where it talks about PHP native session_start() function which doesn't have any problem with concurrent requests, as a way to say why sessions are better for temporary storage and stuff. I am no expert, may be a beginner even, but I think developers are used to dealing with sessions more freely and not ready to be limited to not being able to store in it values like explained here in multiple cases. Also, there could be security threats if we are going to use sessions in Laravel the wrong way while expecting concurrent requests!

ishendywebdeveloper commented 4 years ago

Also, flashing session data doesn't really fit with concurrent requests, does it? It works, but like it says in the documentation: $request->session()->flash('status', 'Task was successful!');

Sometimes you may wish to store items in the session only for the next request. You may do so using the flash method...

And in concurrent requests, you don't really know which request is the next request, is it the request you are expecting which will show the output to the user, or is it another request that would race that request and take the data, then it is not what you are expecting at all?

trevorgehman commented 4 years ago

To do team switching in Vapor we store the current user team on in a database column on the user table. Then it is persistent even after the session has expired and you do not have to worry about any concurrent requests stepping on your session data.

@taylorotwell That is certainly a good solution, however, there is a reason in our case we don't do this:

If you have admin that can login as users (for example using either Auth::loginUsingId($id) or a package like laravel-impersonate), then you will be unable to switch accounts (or teams in your case) without interrupting those users' sessions and changing their "in-use" account/team in the DB.

Regarding persisting their account/team selection, we use a cookie for this.

IMO there are valid reasons for using sessions this way once you need certain functionality.

liepumartins commented 4 years ago

To do team switching in Vapor we store the current user team on in a database column on the user table. Then it is persistent even after the session has expired and you do not have to worry about any concurrent requests stepping on your session data.

Thanks, this is good solution for authenticated users. For guests, however, we still have to store references(cart id at least) in session and it might get destroyed and thus the user cart would be lost.

adam1010 commented 4 years ago

I noticed that Session Locking is a proposed solution and was very surprised that Laravel doesn't do this by default (or even allow you to enable it, at least in the case of the Redis driver). Unfortunately the referenced library for locking isn't ideal because it writes lock files to local disk. Ideally we'd use Redis::lock() (when available) and then fallback to file based locking (just like native PHP file-based session handler does).

Is there any interest from the Laravel development team to implement Session Locking into the framework (even if disabled by default)? That way it would function just like native PHP and prevent concurrent requests, which for most websites is likely desirable (so that the additional complexity of race conditions can be avoided). Maybe it could even be a Middleware so that developers can easily decide which routes should prevent concurrent requests (regardless of the Session driver in use).

Since Session Locking is it's own issue I've opened a ticket to address that functionality directly.

Session Locking Feature: https://github.com/laravel/framework/issues/31405

taylorotwell commented 4 years ago

@adam1010 if it's a middleware I think you could release it as a package. If you can do that and it really takes off maybe we could evaluate including it in the core. Until then I'm going to close this because I personally don't plan to take any action on this feature at the moment.

CurvesTech commented 4 years ago

https://github.com/rairlie/laravel-locking-session Finally a simple solution, that works and is simple to incorporate in your project.

alexw23 commented 4 years ago

I'm still seeing this issue and the PR #32636 has yet to solve it. As @trevorgehman mentioned session blocking basically turns an asynchronous app into a synchronous one.

The issue we have, a user visits their dashboard /account and then decides to logout. However, there are many GET requests still loading in the background whilst this takes place.

|------------ Request A (GET user info, no changes to session) ------------|
     |------------ Request B (GET account info, no changes to session) -------|
         |------------ Request C (GET other info, no changes to session) ---------|
                  |--------  Request D (POST logout, writes to session) --------|

After hitting the POST route, we redirect them back to /login at which point it turns out the user is actually still logged in (because the last request wrote back the logged in session)

A few solutions I see that might work for this issue:

PR #29410 - check to see if the session has been modified, and only write to the driver it if it has.
Have an exclusion of routes that should only read from session, not write. Throw an error if they do try to write. In our case GET /* should be excluded. Because when should a GET query ever write to a session?
Session ID blacklist (specific to our use case of login/logout issues). This would mean that once a user is logged out, their session ID is added to a blacklist, when another request is tried, it will return a 401 as such.

driesvints commented 4 years ago

@alexw23 we don't have plans to tackle this ourselves but you are however free to attempt new PRs.

driesvints commented 4 years ago

@alex23 also: does the package from @CurvesTech not work for you?

mattford commented 3 years ago

@taylorotwell Is this not a security issue since the default auth guard stores the login information in the session?

I have just had a user session brought back after they logged out due to this bug, steps to repro:

Have tab open with logout button (makes req to route which does Auth::logout())
Open another tab to a page which makes a lot of AJAX requests
Click logout

Expected result: session is logged out (login_web_xxx key is removed from session) Actual result: user still logged in and login_web_xxx is present in session

ibarrajo commented 3 years ago

Just a heads up, I don't know when it was introduced, but SessionManager does a check if it should block.

    /**
     * Determine if requests for the same session should wait for each to finish before executing.
     *
     * @return bool
     */
    public function shouldBlock()
    {
        return $this->config->get('session.block', false);
    }

this means you can force the request to be syncronous by adding this line to your config/session.php

'block' => env('SESSION_BLOCK', false),

and setting SESSION_BLOCK=true in your .env of course

ani37 commented 1 year ago

@ibarrajo what value did you keep in session.block_store

liepumartins commented 1 year ago

Was introduced here 7192de7

session.block_store appears to take same type of values as session.store - e.g. file, memcached, redis, etc.

MrEko commented 1 month ago

I believe this issue isn't completely resolved, correct?

According to the documentation: "Session data loss can occur in a small subset of applications that make concurrent requests to two different application endpoints which both write data to the session."

However, this isn't entirely accurate, as @trevorgehman demonstrated that even read-only requests can overwrite data.

At the very least, the documentation should be updated.

trevorgehman commented 1 month ago

We have moved away entirely from storing data in the session as it's just not reliable. The data can very easily be overwritten if you have an SPA making lots of AJAX requests that take different amounts of time to complete. We previously stored the current account ID in the session, but now we store it in a cookie instead.

laravel / framework