Closed Bug 458763 Opened 11 years ago Closed 11 years ago

Session IDs are not being preserved across requests

Categories

(addons.mozilla.org Graveyard :: Administration, defect, major)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: stephend, Assigned: wenzel)

References

()

Details

Attachments

(1 file)

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."
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?
Also see bug 455516. Same error through different means.
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
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.
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
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)
Attachment #342301 - Flags: review?(laura) → review+
Duplicate of this bug: 440823
Duplicate of this bug: 455516
Fixed in r18942. Thanks for pointing me into the right direction, Laura!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.