Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try parsing time in Rememberable before giving up #3917

Closed
wants to merge 1 commit into from

Conversation

boblail
Copy link

@boblail boblail commented Jan 26, 2016

I encountered the exception that 048d05a fixes; but the value of generated_at (now being discarded) was a serialized time: "2016-01-26 17:13:52 UTC".

After following these steps:
1. Sign in with Remember me checked
2. Delete the session key (but not remember_user_token) from the cookies
3. Refresh

Using devise v3.5.5, my application would crash.

Using 048d05a, you'd be signed out.

Using this pull request, you'd be signed in.

@josevalim
Copy link
Contributor

I would rather try to figure out why it is becoming a string in the first place and avoid it. For example, we could try to always make it a string or always an integer to avoid handling all possible scenarios.

@josevalim
Copy link
Contributor

Are you using any special mechanism for cookie serialization?

@lucasmazza
Copy link
Contributor

@josevalim afaik the JSON cookie serialization doesn't deserialize timestamps as Time objects.

@lucasmazza
Copy link
Contributor

Similar fixes we had on timeoutable when rails migrated to JSON over Marshal for serialization: eb9db7b and eb9db7b.

The sane way back then to support for serializers was to deal with integers, and we can do the same here.

@josevalim
Copy link
Contributor

I guess we have no option then. Let's handle this in the function you have just extracted. We will need to backport it though. :S

@lucasmazza
Copy link
Contributor

@boblail can you move your patch to the remember_me? method?

lucasmazza added a commit that referenced this pull request Jan 29, 2016
Time objects aren't properly coerced back when using the JSON cookie serialization,
so we need to do it ourselves.

To avoid any new JSON serialization issues, we now store the `generated_at` as
an String with the timestamp seconds + miliseconds in the cookie but still the
previous JSON encoded format.

Thanks to @boblail at #3917 for the
initial patch.
lucasmazza added a commit that referenced this pull request Jan 29, 2016
Time objects aren't properly coerced back when using the JSON cookie serialization,
so we need to do it ourselves.

To avoid any new JSON serialization issues, we now store the `generated_at` as
an String with the timestamp seconds + miliseconds in the cookie but still the
previous JSON encoded format.

Thanks to @boblail at #3917 for the
initial patch.
@lucasmazza
Copy link
Contributor

Closed by #3927.

@lucasmazza lucasmazza closed this Jan 31, 2016
lucasmazza added a commit that referenced this pull request Jan 31, 2016
Time objects aren't properly coerced back when using the JSON cookie serialization,
so we need to do it ourselves.

To avoid any new JSON serialization issues, we now store the `generated_at` as
an String with the timestamp seconds + miliseconds in the cookie but still the
previous JSON encoded format.

Thanks to @boblail at #3917 for the
initial patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants