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

Cookies serializer improvements #13945

Merged
merged 18 commits into from Feb 13, 2014

Conversation

Projects
None yet
10 participants
@chancancode
Member

chancancode commented Feb 4, 2014

  • Since the option actually changes how encrypted cookies are serialized, we should name it properly and include the signed cookie jar as well.
  • Better tests. Right now the test coverage is pretty sparse. Need to better test the existing cookie features and make sure they work with both of the built-in serializers.
  • Fix flash. (#13910, #13944)
  • Find out what else is broken by swapping the default serializers, then fix those.
  • Add HybridSerializer (need a better name): subclass JsonSerializer to transparently migrate existing marshalled cookies into JSON cookies (we can detect the marshal signiture "\x04\x08" and branch accordingly).
  • Update docs, guides, changelogs to reflect this. Also mention the restrictions on what can be reliably stored in the serialized jars.
  • Upgrade guides on migrating existing cookies.
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode
Member

chancancode commented Feb 4, 2014

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Feb 4, 2014

Contributor

Hey guys just a reminder, I don't think it's only flash messages that are broken (not accessible via symbols). But everything on the session object we call on controllers for example. Turning that into hwia should fix it I guess? Another place I thought we could call it is here

@serializer.load(decrypted_data)

Contributor

huoxito commented Feb 4, 2014

Hey guys just a reminder, I don't think it's only flash messages that are broken (not accessible via symbols). But everything on the session object we call on controllers for example. Turning that into hwia should fix it I guess? Another place I thought we could call it is here

@serializer.load(decrypted_data)

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 4, 2014

Member

@huoxito I think the session object is already behaving like a HWIA by calling to_s on its [] and []= method, but the overall caveat of this change is that you should not expect to be able to put arbitrary objects into the signed/encrypted cookie jars and expect them to come out exactly the same.

We should obviously fix the parts that Rails uses internally (flash, etc), but if you do session[:today] = Date.today, it will come out as a String unless you use the Marshal serializer.

We'll make sure this is documented by the end of this.

Member

chancancode commented Feb 4, 2014

@huoxito I think the session object is already behaving like a HWIA by calling to_s on its [] and []= method, but the overall caveat of this change is that you should not expect to be able to put arbitrary objects into the signed/encrypted cookie jars and expect them to come out exactly the same.

We should obviously fix the parts that Rails uses internally (flash, etc), but if you do session[:today] = Date.today, it will come out as a String unless you use the Marshal serializer.

We'll make sure this is documented by the end of this.

@lukesarnacki

This comment has been minimized.

Show comment
Hide comment
@lukesarnacki

lukesarnacki Feb 4, 2014

Contributor

Thanks @chancancode, this is awesome insight.

When it comes to naming, maybe use special name for current JSON serializer, something like :safe_json (or other name other name connected to being safe / simple etc.).

I will try to find out other things that could be affected by serializer changes.

Contributor

lukesarnacki commented Feb 4, 2014

Thanks @chancancode, this is awesome insight.

When it comes to naming, maybe use special name for current JSON serializer, something like :safe_json (or other name other name connected to being safe / simple etc.).

I will try to find out other things that could be affected by serializer changes.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Feb 4, 2014

Member

I like the idea of a hybrid serializer, 4.2 could default to that then 4.NEXT could switch to the pure json serializer. The hybrid serializer could take extra care to raise informative exceptions when people put in non-roundtrippable data structures.

Member

NZKoz commented Feb 4, 2014

I like the idea of a hybrid serializer, 4.2 could default to that then 4.NEXT could switch to the pure json serializer. The hybrid serializer could take extra care to raise informative exceptions when people put in non-roundtrippable data structures.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 4, 2014

Member

I thought about that, but that would still be a (subtle) breaking change, wouldn't it? (Because people might depends on non-primitives type in their apps.)

I like the idea of more aggressively pushing this, but I am not sure if we can make it the default for existing apps. Once you switched to the hybrid serializer, it's kind of a one way street. 

Sent from Mailbox for iPhone

On Tue, Feb 4, 2014 at 12:29 PM, Michael Koziarski
notifications@github.com wrote:

I like the idea of a hybrid serializer, 4.2 could default to that then 4.NEXT could switch to the pure json serializer. The hybrid serializer could take extra care to raise informative exceptions when people put in non-roundtrippable data structures.

Reply to this email directly or view it on GitHub:
#13945 (comment)

Member

chancancode commented Feb 4, 2014

I thought about that, but that would still be a (subtle) breaking change, wouldn't it? (Because people might depends on non-primitives type in their apps.)

I like the idea of more aggressively pushing this, but I am not sure if we can make it the default for existing apps. Once you switched to the hybrid serializer, it's kind of a one way street. 

Sent from Mailbox for iPhone

On Tue, Feb 4, 2014 at 12:29 PM, Michael Koziarski
notifications@github.com wrote:

I like the idea of a hybrid serializer, 4.2 could default to that then 4.NEXT could switch to the pure json serializer. The hybrid serializer could take extra care to raise informative exceptions when people put in non-roundtrippable data structures.

Reply to this email directly or view it on GitHub:
#13945 (comment)

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 11, 2014

Member

Alright, I think this is pretty much done 😄 The "Find out what else is broken by swapping the default serializers, then fix those." TODO is still open, because a lot of the tests are testing against the marshal serializer with hardcoded Marshal dumps =/

We should improve that going forward (because it means that a lot of tests aren't covering the JSON serializer – which became the new default now), but maybe we can probably do it post 4.1?

Member

chancancode commented Feb 11, 2014

Alright, I think this is pretty much done 😄 The "Find out what else is broken by swapping the default serializers, then fix those." TODO is still open, because a lot of the tests are testing against the marshal serializer with hardcoded Marshal dumps =/

We should improve that going forward (because it means that a lot of tests aren't covering the JSON serializer – which became the new default now), but maybe we can probably do it post 4.1?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 11, 2014

Member

@guilleiguaran do you know if hash values in signed cookies are supported? e.g.

cookies.signed[:some_key] = { value: { "some": "hash" } }

Currently, this doesn't migrate correctly, because when you assign a Hash to self[name] it gets treated differently, so verify_and_upgrade_legacy_signed_message would actually nullify any hash-based values 😓

The session "hash" escapes this narrowly because it's not actually a Hash...

This change would fix that, if it's supported.

@guilleiguaran do you know if hash values in signed cookies are supported? e.g.

cookies.signed[:some_key] = { value: { "some": "hash" } }

Currently, this doesn't migrate correctly, because when you assign a Hash to self[name] it gets treated differently, so verify_and_upgrade_legacy_signed_message would actually nullify any hash-based values 😓

The session "hash" escapes this narrowly because it's not actually a Hash...

This change would fix that, if it's supported.

# Passing the NullSerializer downstream to the Message{Encryptor,Verifier}
# allows us to handle the (de)serialization step within the cookie jar,
# which gives us the opportunity to detect and migrate legacy cookies.

This comment has been minimized.

@NZKoz

NZKoz Feb 11, 2014

Member

Why not have an explicit hybrid serializer and keep the \x04 stuff in there? have deserialize support both and serialize write json?

@NZKoz

NZKoz Feb 11, 2014

Member

Why not have an explicit hybrid serializer and keep the \x04 stuff in there? have deserialize support both and serialize write json?

This comment has been minimized.

@chancancode

chancancode Feb 11, 2014

Member

It was changed in ead947a to enable automatica write-back.

Previously, I was passing a HybridSerializer downstream to the Message{Encryptor,Verifier}, however, if we want to actually migrate the cookie value on read (i.e. if I access cookies.signed[:some_key] it will re-write it as json), then it has to be done on the cookie jar level, because it needs access to []= on the cookie jar.

That is mainly to mirror the behaviour on the legacy cookie jar migration thing we did from 3.2 -> 4.0, and to more aggressively migrate legacy cookies.

If we don't care for that, it could be much simpler.

@chancancode

chancancode Feb 11, 2014

Member

It was changed in ead947a to enable automatica write-back.

Previously, I was passing a HybridSerializer downstream to the Message{Encryptor,Verifier}, however, if we want to actually migrate the cookie value on read (i.e. if I access cookies.signed[:some_key] it will re-write it as json), then it has to be done on the cookie jar level, because it needs access to []= on the cookie jar.

That is mainly to mirror the behaviour on the legacy cookie jar migration thing we did from 3.2 -> 4.0, and to more aggressively migrate legacy cookies.

If we don't care for that, it could be much simpler.

This comment has been minimized.

@chancancode

chancancode Feb 11, 2014

Member

For reference, this is the before code:

class HybridSerializer < JsonSerializer
MARSHAL_SIGNATURE = "\x04\x08".freeze
def self.load(value)
if value.start_with?(MARSHAL_SIGNATURE)
Marshal.load(value)
else
super
end
end
end

@chancancode

chancancode Feb 11, 2014

Member

For reference, this is the before code:

class HybridSerializer < JsonSerializer
MARSHAL_SIGNATURE = "\x04\x08".freeze
def self.load(value)
if value.start_with?(MARSHAL_SIGNATURE)
Marshal.load(value)
else
super
end
end
end

This comment has been minimized.

@NZKoz

NZKoz Feb 11, 2014

Member

I see, if you can't piggyback on @set_cookies in the cookie jar then I guess this is the best way to handle it, does seem a bit janky is all

@NZKoz

NZKoz Feb 11, 2014

Member

I see, if you can't piggyback on @set_cookies in the cookie jar then I guess this is the best way to handle it, does seem a bit janky is all

This comment has been minimized.

@chancancode

chancancode Feb 11, 2014

Member

Yeah, it's definitely a bit of a hack. I tried to come up with better ways to implement this, but I couldn't =/ The main problem is that if the deserialization is happening downstream, the cookie jar has no insight on whether a migration is necessary or not. If there are any suggestions over this I'd love to hear them 😄

@chancancode

chancancode Feb 11, 2014

Member

Yeah, it's definitely a bit of a hack. I tried to come up with better ways to implement this, but I couldn't =/ The main problem is that if the deserialization is happening downstream, the cookie jar has no insight on whether a migration is necessary or not. If there are any suggestions over this I'd love to hear them 😄

guilleiguaran added a commit that referenced this pull request Feb 13, 2014

@guilleiguaran guilleiguaran merged commit de5ef15 into master Feb 13, 2014

1 check passed

default The Travis CI build passed
Details
@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Feb 25, 2014

Member

This is in 4-1-stable branch already and 4.1.0.rc1 was created from 4-1-stable branch 😁

what problem are you seeing?

Member

guilleiguaran commented Feb 25, 2014

This is in 4-1-stable branch already and 4.1.0.rc1 was created from 4-1-stable branch 😁

what problem are you seeing?

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Feb 25, 2014

Contributor

@guilleiguaran Nevermind. I removed my earlier comment. I was storing a HWIA in the session. Apparently I can no longer do that with the default JSON serializer.

Contributor

jcoyne commented Feb 25, 2014

@guilleiguaran Nevermind. I removed my earlier comment. I was storing a HWIA in the session. Apparently I can no longer do that with the default JSON serializer.

jcoyne added a commit to projectblacklight/blacklight that referenced this pull request Feb 25, 2014

Don't use complex objects (e.g. HashWithIndifferentAccess) in session
In rails 4.1 the default session serializer has been switched to a JSON
serializer, which doesn't support serializing class.

See rails/rails#13945 (comment)
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 4, 2014

Member

Just upgraded my app to the RC and noticed that Symbol keys changed to String keys. Is this expected behavior? It should be documented in the upgrading guide at least. This will break apps for sure.

Member

senny commented on a668bef Mar 4, 2014

Just upgraded my app to the RC and noticed that Symbol keys changed to String keys. Is this expected behavior? It should be documented in the upgrading guide at least. This will break apps for sure.

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal

kuldeepaggarwal Mar 5, 2014

Contributor

@senny This won't break any app, as if we try to access notice like flash[:notice], then

def [](k)
  @flash[k.to_s]
end

will save you... 😄

Contributor

kuldeepaggarwal replied Mar 5, 2014

@senny This won't break any app, as if we try to access notice like flash[:notice], then

def [](k)
  @flash[k.to_s]
end

will save you... 😄

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Mar 5, 2014

Member

@kuldeepaggarwal it breaks apps that were iterating over the keys and checking for specific keys, for example:

<% flash.keys.each do |key, value| %>
    <span class="<%= (key == :notice) ? 'green' : 'red' %>"><%= value %></span>
<% end %>  
Member

guilleiguaran replied Mar 5, 2014

@kuldeepaggarwal it breaks apps that were iterating over the keys and checking for specific keys, for example:

<% flash.keys.each do |key, value| %>
    <span class="<%= (key == :notice) ? 'green' : 'red' %>"><%= value %></span>
<% end %>  

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 5, 2014

Member

@kuldeepaggarwal flash access works but the keys need to be stored in some sort. Before they were symbols and now they are Strings. If you have applications that work with the internal flash representation (like the example of @guilleiguaran) you get different behavior than on Rails 4. I know there are many tutorials and even gems out there that suggest looping through the flash. So this is a compatibility issue.

It's easy to fix though so we need to make sure the user knows what the cause is an how to proceed.

Member

senny replied Mar 5, 2014

@kuldeepaggarwal flash access works but the keys need to be stored in some sort. Before they were symbols and now they are Strings. If you have applications that work with the internal flash representation (like the example of @guilleiguaran) you get different behavior than on Rails 4. I know there are many tutorials and even gems out there that suggest looping through the flash. So this is a compatibility issue.

It's easy to fix though so we need to make sure the user knows what the cause is an how to proceed.

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal

kuldeepaggarwal Mar 5, 2014

Contributor

Sorry I missed that...

Contributor

kuldeepaggarwal replied Mar 5, 2014

Sorry I missed that...

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Mar 9, 2014

Member
Member

chancancode replied Mar 9, 2014

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Mar 9, 2014

Member
Member

chancancode replied Mar 9, 2014

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 9, 2014

Member

i updated the upgrading guide. will take care of changelog/ release guide sync next week.

Member

senny replied Mar 9, 2014

i updated the upgrading guide. will take care of changelog/ release guide sync next week.

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal
Contributor

kuldeepaggarwal replied Mar 9, 2014

@senny 👍

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Mar 9, 2014

Member
Member

chancancode replied Mar 9, 2014

jcoyne added a commit to projectblacklight/blacklight that referenced this pull request Mar 6, 2014

Don't use complex objects (e.g. HashWithIndifferentAccess) in session
In rails 4.1 the default session serializer has been switched to a JSON
serializer, which doesn't support serializing class.

See rails/rails#13945 (comment)

jcoyne added a commit to projectblacklight/blacklight that referenced this pull request Mar 10, 2014

Don't use complex objects (e.g. HashWithIndifferentAccess) in session
In rails 4.1 the default session serializer has been switched to a JSON
serializer, which doesn't support serializing class.

See rails/rails#13945 (comment)

@lukesarnacki lukesarnacki referenced this pull request Mar 12, 2014

Merged

Custom encoder class #16

@chancancode chancancode deleted the json_cookie_serializer_improvements branch May 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment