458763 - Session IDs are not being preserved across requests

Status: VERIFIED FIXED

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

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

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."

Comment 1

[Fred Wenzel [:wenzel]]

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

[Dave Garrett]

Also see bug 455516. Same error through different means.

Comment 3

[Fred Wenzel [:wenzel]]

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?

Comment 4

[Fred Wenzel [:wenzel]]

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.

Comment 5

[Fred Wenzel [:wenzel]]

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.

Comment 6

[Fred Wenzel [:wenzel]]

Attachment: 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.

Comment 9

[Fred Wenzel [:wenzel]]

Fixed in r18942. Thanks for pointing me into the right direction, Laura!

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

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!