Inconsistent hash access with hashes stored in the FlashHash between flash.now and flash.type #14409

Closed
larslevie opened this Issue Mar 17, 2014 · 11 comments

Comments

Projects
None yet
6 participants
@larslevie

I noticed when moving from Rails 3.2.17 to Rails 4.1 RC1 that hashes stored in the FlashHash get inconsistent access, symbol vs. string, depending on whether or not you use the now method. Here's an example:

This uses string access: flash.alert = { title: 'Foobar', content: 'Lorem ipsum." }
so flash.alert['title'] == 'Foobar', but flash.alert[:title] == nil

This uses symbol access:
flash.now.alert = { title: 'Foobar', content: 'Lorem ipsum." }
so flash.alert[:title] == 'Foobar', but flash.alert['title'] == nil

Previously (3.2.17) both examples above used symbol access.

@rafaelfranca rafaelfranca added this to the 4.1.0 milestone Mar 17, 2014

@chancancode

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Mar 17, 2014

Member

Hey thanks for reporting this. I believe the intention of the change is that both of these would have indifferent access, so it shouldn't matter in either case whether you use symbol or string. Which means it's probably a bug we missed. I'll have to double check this though.

Member

chancancode commented Mar 17, 2014

Hey thanks for reporting this. I believe the intention of the change is that both of these would have indifferent access, so it shouldn't matter in either case whether you use symbol or string. Which means it's probably a bug we missed. I'll have to double check this though.

@larslevie

This comment has been minimized.

Show comment Hide comment
@larslevie

larslevie Mar 17, 2014

Figured that was the intended direction.

Figured that was the intended direction.

@guilleiguaran guilleiguaran self-assigned this Mar 17, 2014

@rushabhnagda11

This comment has been minimized.

Show comment Hide comment
@rushabhnagda11

rushabhnagda11 Mar 18, 2014

Hi @larslevie i tried out your code on Rails 4.1 RC1 but was unable to reproduce your issue. In my case in both examples, symbol access is used

require "action_dispatch"
flash = ActionDispatch::Flash::FlashHash.new

flash.alert = { title: 'Foobar', content: 'Lorem ipsum.' }
puts "String access: #{flash.alert['title']} Symbol access: #{flash.alert[:title]}"

flash.now.alert = { title: 'Foobar', content: 'Lorem ipsum.'}
puts "String access: #{flash.alert['title']} Symbol access: #{flash.alert[:title]}"

Hi @larslevie i tried out your code on Rails 4.1 RC1 but was unable to reproduce your issue. In my case in both examples, symbol access is used

require "action_dispatch"
flash = ActionDispatch::Flash::FlashHash.new

flash.alert = { title: 'Foobar', content: 'Lorem ipsum.' }
puts "String access: #{flash.alert['title']} Symbol access: #{flash.alert[:title]}"

flash.now.alert = { title: 'Foobar', content: 'Lorem ipsum.'}
puts "String access: #{flash.alert['title']} Symbol access: #{flash.alert[:title]}"
@chancancode

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Mar 18, 2014

Member

Sorry I intended to give an update on this. I think we figured out @larslevie's problem. The problem occurs when you are using the JSON cookie serializer (which is the new default for 4.1). Recall that the JSON serializer cannot serialize arbitrary values.

When you do flash.now.alert = { ...some symbol keyed hash... } and access within the same request, the content is just retrieved from memory and won't go through the serializer. Since this hash is just an ordinary hash (read: not HashWithIndifferentAccess) with symbol keys, accessing it with strings won't work.

When you use flash.alert = { ...some symbol keyed hash... } and access it on the next request, it already went through a full round-trip with serializer. The hash will be (de)serialized with string keys (because JSON) and so you will only be able to access it through string keys.

We have two options –

  1. advise users that they need to be aware of what can and cannot be serialized with the JSON cookie jar (e.g. if you put a Date in there it'll come out as a string), and categorize this as one of those "unsupported" cases (you can easily fix this by calling with_indifferent_access on it or symbolize_keys)
  2. special case Hash can call with_indifferent_access for people

I checked the docs, stack overflow, rails tutorial, railscasts, and github code search, it seems like this usage (assigning a hash to flash.*) seems very rare, so I'm more inclined towards the first option. I think this is rather like how cookes.encrypted[:some_key] = { ...some symbol keyed hash... } won't work using the JSON cookie jar.

Member

chancancode commented Mar 18, 2014

Sorry I intended to give an update on this. I think we figured out @larslevie's problem. The problem occurs when you are using the JSON cookie serializer (which is the new default for 4.1). Recall that the JSON serializer cannot serialize arbitrary values.

When you do flash.now.alert = { ...some symbol keyed hash... } and access within the same request, the content is just retrieved from memory and won't go through the serializer. Since this hash is just an ordinary hash (read: not HashWithIndifferentAccess) with symbol keys, accessing it with strings won't work.

When you use flash.alert = { ...some symbol keyed hash... } and access it on the next request, it already went through a full round-trip with serializer. The hash will be (de)serialized with string keys (because JSON) and so you will only be able to access it through string keys.

We have two options –

  1. advise users that they need to be aware of what can and cannot be serialized with the JSON cookie jar (e.g. if you put a Date in there it'll come out as a string), and categorize this as one of those "unsupported" cases (you can easily fix this by calling with_indifferent_access on it or symbolize_keys)
  2. special case Hash can call with_indifferent_access for people

I checked the docs, stack overflow, rails tutorial, railscasts, and github code search, it seems like this usage (assigning a hash to flash.*) seems very rare, so I'm more inclined towards the first option. I think this is rather like how cookes.encrypted[:some_key] = { ...some symbol keyed hash... } won't work using the JSON cookie jar.

@guilleiguaran

This comment has been minimized.

Show comment Hide comment
@guilleiguaran

guilleiguaran Mar 19, 2014

Owner

@chancancode I choose option 1. Lets update the docs and release notes with a note about this.

Owner

guilleiguaran commented Mar 19, 2014

@chancancode I choose option 1. Lets update the docs and release notes with a note about this.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 19, 2014

Owner

@guilleiguaran better to close only when it is solved.

I prefer option 1 too.

Owner

rafaelfranca commented Mar 19, 2014

@guilleiguaran better to close only when it is solved.

I prefer option 1 too.

@rafaelfranca rafaelfranca reopened this Mar 19, 2014

@tilsammans

This comment has been minimized.

Show comment Hide comment
@tilsammans

tilsammans Mar 19, 2014

Contributor

This is going to break every Date stored in cookies. Is this worth choosing JSON over Marshal?

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

Contributor

tilsammans commented Mar 19, 2014

This is going to break every Date stored in cookies. Is this worth choosing JSON over Marshal?

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 19, 2014

Owner

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

This is exactly how it works now.

Is this worth choosing JSON over Marshal?

Yes. For security perspective if you use Marshal a well crafted attack can execute arbitrary code in your application.

Owner

rafaelfranca commented Mar 19, 2014

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

This is exactly how it works now.

Is this worth choosing JSON over Marshal?

Yes. For security perspective if you use Marshal a well crafted attack can execute arbitrary code in your application.

@tilsammans

This comment has been minimized.

Show comment Hide comment
@tilsammans

tilsammans Mar 19, 2014

Contributor

Thanks for the clarification. I got confused because the submitter seemed to have done an upgrade and encountered the bug.

Completely agree that JSON is better suited as a format for this serialization.

Contributor

tilsammans commented Mar 19, 2014

Thanks for the clarification. I got confused because the submitter seemed to have done an upgrade and encountered the bug.

Completely agree that JSON is better suited as a format for this serialization.

@chancancode

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Mar 19, 2014

Member

@tilsammans

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

This is in fact how it works. New apps generated with 4.1 comes with a configuration that sets the (encrypted/signed) cookie jar serializer to JSON, and existing apps without the configuration defaults to marshal.

This is going to break every Date stored in cookies.

To be clear – storing a Date inside a cookie, as in cookies[:expiration] = Date.tomorrow never worked the way you expected. Vanilla HTTP cookies has always been string based, and if you need to put anything else in there you'd have to handle the conversion yourself.

Only the encrypted and signed cookie jars (which the session store, and in turns flash is based upon) automatically (de)serialization data. But obviously there are a lot of things that can't be serialized (e.g. a file) and you need to be aware of that. This change does expand that list, but IMO if you are putting anything other than strings and numbers in your cookies/session you are probably doing it wrong.

Is this worth choosing JSON over Marshal?

The advantage is actually a security reason – Marshal loading values coming from outside of the app is quite dangerous, so we prefer to move away from Marshal.

Member

chancancode commented Mar 19, 2014

@tilsammans

I'd prefer it if only new 4.1 apps got the JSON serializer, and existing apps retained the Marshal one.

This is in fact how it works. New apps generated with 4.1 comes with a configuration that sets the (encrypted/signed) cookie jar serializer to JSON, and existing apps without the configuration defaults to marshal.

This is going to break every Date stored in cookies.

To be clear – storing a Date inside a cookie, as in cookies[:expiration] = Date.tomorrow never worked the way you expected. Vanilla HTTP cookies has always been string based, and if you need to put anything else in there you'd have to handle the conversion yourself.

Only the encrypted and signed cookie jars (which the session store, and in turns flash is based upon) automatically (de)serialization data. But obviously there are a lot of things that can't be serialized (e.g. a file) and you need to be aware of that. This change does expand that list, but IMO if you are putting anything other than strings and numbers in your cookies/session you are probably doing it wrong.

Is this worth choosing JSON over Marshal?

The advantage is actually a security reason – Marshal loading values coming from outside of the app is quite dangerous, so we prefer to move away from Marshal.

@chancancode

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Mar 19, 2014

Member

I got confused because the submitter seemed to have done an upgrade and encountered the bug.

Actually, that's a good point. @larslevie are you upgrading an existing app? If so this could be something else.

Member

chancancode commented Mar 19, 2014

I got confused because the submitter seemed to have done an upgrade and encountered the bug.

Actually, that's a good point. @larslevie are you upgrading an existing app? If so this could be something else.

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