Repository navigation
fix(session): Prevent stale requests from replacing a regenerated session - #65928
Conversation
|
/backport to stable35 |
|
/backport to stable34 |
|
/backport to stable33 |
82b49e0 to
cf9752b
Compare
cf9752b to
d94241c
Compare
d94241c to
783eb52
Compare
…sion Regenerating the session id - on login, on unlocking a public share, on a remember-me login - leaves the requests that are already on their way with the previous session cookie. PHP starts a session for that id again and sends it back in a Set-Cookie header, so the id the browser had just stored is overwritten and everything the regenerating request did is lost. For a logged-in user who unlocks a password-protected public share this shows up as a 401 on PROPFIND /public.php/dav/files/<token>/. Mark the old session as superseded instead of deleting it, and move the requests that still arrive on it to an empty session. Within a grace period of 60 seconds they get no session cookie at all, so they can no longer replace the regenerated id the browser holds. reopen() does the same and only sends a cookie when the id it was given is gone for good. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
783eb52 to
25c1ba3
Compare
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
Sounds like a good approach for the tricky problem with the old session cookies overwriting the new ones 👍
Nit: I see some time() calls in prod and test code, which can make the tests fragile. There is a time factory to mock it deterministically.
|
The backport to # Switch to the target branch and update it
git checkout stable33
git pull origin stable33
# Create the new backport branch
git checkout -b backport/65928/stable33
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 25c1ba3a
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/65928/stable33Error: Failed to check for changes with origin/stable33: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable34
git pull origin stable34
# Create the new backport branch
git checkout -b backport/65928/stable34
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 25c1ba3a
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/65928/stable34Error: Failed to check for changes with origin/stable34: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
Time factory fix will come in a new PR |
|
I tried to apply the patch in production earlier today to see if it fixed #64659 and it didn't seem to have an effect on that |
That's why it's only listed as "related to". The symptoms are the same for multiple session bugs. The ticket you mention is only one of them. I try to fix them one-by-one (see #65959 and another one that is still in the making). |
Summary
When the session id is being regenerated (for example when you unlock a public share while a request from another tab is already on its way), the session can flap back to the old one due to stale requests. When PHP receives a stale request with the previous session id, it starts a new session with the old session id and sets a new cookie with the old session id. The browser is pushed back onto the old session and loses everything the regenerating request did, which leads to intermittent 401 errors - for a password-protected public share, the file list fails with a 401 on PROPFIND
/public.php/dav/files/<token>/.To fix the issue, I no longer delete the old session: it keeps a marker that its id was replaced. A request that still arrives on that id gets an empty session and, for a grace period of 60 seconds, no session cookie at all, so it can no longer replace the id the browser just stored. After the grace period it gets a fresh session with a cookie as usual.
Internal::reopen()now starts the session without cookies for the same reason - until now it re-emitted the cookie on every late session write.Checklist
3. to review, feature component)stable32)AI (if applicable)