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

"JSON::GenericObject.json_creatable = true" not working #159

Closed
phoet opened this issue Feb 13, 2013 · 6 comments
Closed

"JSON::GenericObject.json_creatable = true" not working #159

phoet opened this issue Feb 13, 2013 · 6 comments

Comments

@phoet
Copy link

phoet commented Feb 13, 2013

when updating our application a lot of tests failed, because object creation is now disabled by default.

setting JSON::GenericObject.json_creatable = true does not change the behavior. is there any chance, that the global config is not working as expected? see the comment below.

d0a62f3#commitcomment-2613600

@flori
Copy link
Member

flori commented Feb 13, 2013

I am not sure what you mean. The new default for :create_additions is false, JSON::GenericObject.json_creatable defaults to false as well.

The JSON::GenericObject should only be created when

  • :create_additions is true AND
  • JSON::GenericObject.json_creatable? returns true OR
  • JSON.parse json_text, object_class: JSON::GenericObject is used (JSON::GenericObject.json_creatable doesn't matter in this case).

The main points of the fix are, that

  • JSON.parse uses a safe default (in order to parse untrustworthy user input),
  • JSON.load uses a unsafer default (in order to parse data that was serialised by a trusted source),
  • That JSON::GenericObject can be used by either
    • passing it as a dedicated object class or
    • enabling the json_creatable flag, just as you would have to if you implement your own json creatable classes.

This probably should be spelled out in the documentation more clearly, especially that the dump/load interface of Marshal, YAML and JSON should not be used for parsing untrusted user input.

@phoet
Copy link
Author

phoet commented Feb 13, 2013

but in that commit the default is :create_additions => true or am i mistaken? d0a62f3#L9R305

could you provide an example-configuration of what to do to get the old behavior back, that is compatible to version 1.7.6?

@flori
Copy link
Member

flori commented Feb 13, 2013

Like I said you have to change both settings to get the old behaviour back with JSON.parse. Have a look

>> JSON::GenericObject.json_creatable?
 # => false is default:
>> JSON.parse '{"json_class": "JSON::GenericObject"}'
 # => {"json_class"=>"JSON::GenericObject"}
>> JSON::GenericObject.json_creatable = true
 # => true is not enough:
>> JSON.parse '{"json_class": "JSON::GenericObject"}'
 # => {"json_class"=>"JSON::GenericObject"}
>> JSON.parse '{"json_class": "JSON::GenericObject"}', create_additions: true
 # => #<JSON::GenericObject> this is the old behaviour

With JSON.load you only have to enable JSON::GenericObject.json_creatable, see the load default:

>> JSON.load_default_options[:create_additions]
 # => true

@flori flori closed this as completed Feb 14, 2013
@phoet
Copy link
Author

phoet commented Feb 14, 2013

so it is not possible to get the old behavior back when using JSON.parse except for changing all calls to parse with the options create_additions: true.

so a lib that relies on this (like couch_potato) needs to change all the calls to JSON.load instead of JSON.parse.

is that correct?

@flori
Copy link
Member

flori commented Feb 14, 2013

Yes. It's just too unsafe to create additions by default, not only because of SQL injection in rails but also because of possible memory overflow problems due to ruby's symbols which aren't GC'ed. The current consensus on ruby core seems to be that the load/dump interface is to be used on trusted input as in the case of database deserialisation/serialisation. It's really unfortunate, that probably a lot of people use load to parse untrusted input already, but at least parse should be reasonably safe for people who don't bother to consider the security implications.

@phoet
Copy link
Author

phoet commented Feb 14, 2013

as a workaround you can provide a monkey patch from: https://gist.github.com/mnin/4952990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants