Session IDs are not being preserved across requests

VERIFIED FIXED in 4.0.2

Status

addons.mozilla.org Graveyard
Administration
--
major
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: stephend, Assigned: wenzel)

Tracking

unspecified
4.0.2

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Summary: Form errors trying to delete user account with JavaScript disabled

Steps to Reproduce:

1. Disable JavaScript
2. Load https://preview.addons.mozilla.org/en-US/firefox/users/delete
3. Type a valid password, check the "Confirm" checkbox
4. Click "Delete my user account now"

Expected Results:

User account is deleted

Actual Results:

"There are errors in this form. Please correct them and resubmit."
(Assignee)

Comment 1

9 years ago
I can confirm this, however this is curious, to say the very least, as the feature does not use any JavaScript at all (!).

I am currently digging for what's going wrong here. It seems, the session hash generated in the view differs from the one it is being checked against later. If that was the case, we should have form errors everywhere on the page, not only for user deletion?

Comment 2

9 years ago
Also see bug 455516. Same error through different means.
(Assignee)

Comment 3

9 years ago
AMO's session IDs are not being preserved across requests. As far as I can tell, this is not related to the user deletion code.

It seems to occur particularly often with JS disabled, but if bug 455516 is related, it happens with JS on as well.

If you debug() the output of session_id() and reload the page, you will notice that it changes on almost every request (for me, on almost every reload with JS off, on about every tenth reload with JS on). Consequently, the CSRF code throws an error when a POST form is submitted.

CCing both Wil and Ryan, who have patched Cake's session code a while ago, and may remember something that could help us figure out the cause of this?
Assignee: fwenzel → nobody
No longer blocks: 432614
Summary: Form errors trying to delete user account with JavaScript disabled → Session IDs are not being preserved across requests
(Assignee)

Comment 4

9 years ago
Another observation: This phenomenon does not seem to lead to a logout of the user (otherwise we'd have noticed it decades ago) -- that means, the cookie sent by the browser is analyzed, considered *valid*, and after that, the session ID is regenerated and the new cookie is sent back to the user.

In my DB's session table, I notice that the same row is being replaced by a new ID when this occurs. That looks like, instead of spawning *new* sessions over and over, the *same* session persists but constantly receives a newly generated ID.
(Assignee)

Comment 5

9 years ago
Laura pointed out that this behavior is intentional, and it is our CSRF check that's broken, because it compares the secret with its supposed value after the session ID is regenerated, not before.

If that code is put into cake's session code (cake/lib/session) before the session ID regeneration, the problem will be fixed. Taking bug, marking for 4.0.2.
Assignee: nobody → fwenzel
Target Milestone: --- → 4.0.2
(Assignee)

Comment 6

9 years ago
Created attachment 342301 [details] [diff] [review]
preserve ID before renewal for CSRF check

This patch preserves a (legitimately) regenerated session ID and checks against that instead of the new one in the CSRF checking code.
Attachment #342301 - Flags: review?(laura)

Updated

9 years ago
Attachment #342301 - Flags: review?(laura) → review+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 440823
(Assignee)

Updated

9 years ago
Duplicate of this bug: 455516
(Assignee)

Comment 9

9 years ago
Fixed in r18942. Thanks for pointing me into the right direction, Laura!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

9 years ago
I:

* registered a throwaway AMO account
* logged in
* changed my password
* browsed around
* deleted my account

...all with JavaScript disabled, on preview.AMO

Verified FIXED; if I see something else, I'll file a new, individually scoped bug, but this looks fixed.

Thanks, Fred!
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.