[#48258] - [6.2] fix(cookie auth): fix concurrency #46587
- Pending
- Medium
- Build: 6.2-dev
- # 48258
- Diff
- NicolasDerumigny:dev/nderumig/cookie-fix
- Failure continuous-integration/drone/pr Build is failing Details
User tests: Successful: Unsuccessful:
Pull Request resolves #46587.
- I read the Generative AI policy and my contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later.
Summary of Changes
Modify the behaviour of the cookie authenticator. Instead of rotating the token and deleting the previous one in the database, keep the old one in the database and delete it only upon successful login with the new token. This circumvents race conditions in which multiple requests with the same cookies arrive simultaneously and cases in which the connection is cut on the user side before the cookie update happens/
Testing Instructions
- Login with the
remember meoption checked. - In the browser, delete all session cookies (simulate the end of the session)
- In the js console, execute (20 requests in parallel to the server, each triggering potentially a cookie update):
Promise.all([fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}, {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}, {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}, {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}, {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'}),fetch("/component/ajax/?format=json", {method: 'POST'})])- wait ~5-10 second to ensure execution of al requests
- refresh the page
Actual result BEFORE applying this Pull Request
The user gets disconnected from all of his devices
Expected result AFTER applying this Pull Request
The user remains connected on all of his devices.
Link to documentations
Please select:
-
Documentation link for guide.joomla.org:
-
No documentation changes for guide.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Plugins |
| Labels |
Added:
PR-6.2-dev
|
||
| Title |
|
||||||
tecpromotion
- | Title |
|
||||||
Thanks for the feedback! I am seeing bugs in my testing environment, so converting this PR to draft until this is sorted.
Edit: found and updated in the last commit.
| Category | Front End Plugins | ⇒ | SQL Administration com_admin Postgresql Installation Front End Plugins |
I am not convinced that this is a correct fix if i understood it correctly
The existing schema deliberately makes series unique. The PR removes that uniqueness in both MySQL and PostgreSQL and then changes authentication from "there must be exactly one matching record" to "there may be several records; accept whichever token verifies".
Indeed. But as far as I know, there is no way in PHP to reliably set up a critical section where only one request is allowed to execute. Furthermore, nothing guarantees that the response (i.e., cookie update) is actually received by the client, which currently cause a spurious logout on all sessions.
Indeed. But as far as I know, there is no way in PHP to reliably set up a critical section where only one request is allowed to execute. Furthermore, nothing guarantees that the response (i.e., cookie update) is actually received by the client, which currently cause a spurious logout on all sessions.
Also, it accepts whichever token verifies, but then still deletes all previous tokens on successful login, so rotation still occurs. In a case where cookie update was performed flawlessly, then only 2 tokens per session (old one / new one) are kept in memory.
Indeed. But as far as I know, there is no way in PHP to reliably set up a critical section where only one request is allowed to execute. Furthermore, nothing guarantees that the response (i.e., cookie update) is actually received by the client, which currently cause a spurious logout on all sessions.
Also, it accepts whichever token verifies, but then still deletes all previous tokens on successful login, so rotation still occurs. In a case where cookie update was performed flawlessly, then only 2 tokens per session (last one / current one) are kept in memory.
![avatar github-actions[bot]](/images/avatars/github-actions%5Bbot%5D.png)
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Thanks for the feedback! I am seeing bugs in my testing environment, so converting this PR to draft until this is sorted.