Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

configuration for active_support and JSON Encoding #6271

Merged
merged 1 commit into from

5 participants

Egor Homakov Santiago Pastorino José Valim Michael Koziarski Xavier Noria
Egor Homakov

Currently, documented "escape_html_entities_in_json" option is not working. As well as use_standard_json_time_format and encode_big_decimal_as_string parameters for JSON Encoder.

Developer should add them to application.rb (because it is an env-independent options). At the moment additions will not impact on JSON encoder settings - the patch fixes it.

Not sure about adding it to the generator of application.rb.
escape_html_entities_in_json is a very important option though, what about only this? / @wycats @josevalim

Bonus question: Why escape_html_entities_in_json is false? It was true a while ago and everything was OK.. thanks

Santiago Pastorino
Owner

@homakov it's probably better to loop through the options instead of looping through the hardcoded list.
Can you fix that?

Egor Homakov

First time I did it without hardcoded array but experienced problems. sorry I cannot agree for a few reasons.

  • if anybody adds any other config.active_support.OPTION it will produce an error. It does, by the way, already - config.active_support.bare = false will raise an error since ActiveSupport::JSON::Encoding.bare doesn;t exist. We will need to blacklist those options that should not be assigned - much worse than current 3 hardcoded options(whitelisted).
  • really, config.active_support is not supposed to be only about JSON encoding. it's just options related to AS.
Santiago Pastorino spastorino merged commit bae190c into from
Xavier Noria fxn merged commit 5613fb0 into from
Santiago Pastorino
Owner

@homakov I was talking about this 45f6dcd
Thanks for your contributions and reviews :)

Egor Homakov

not bad :) It was another way that I considered. Just not a fan of "verbose" respond_to attitude. Thanks!
let me open the last pull req on the topic - still wonder can we make it true by default...

José Valim
Owner

@spastorino I think your solution to the problem won't work. Since they are not tests with this pull request, it should be reviewed and hopefully some tests will be added.

Santiago Pastorino
Owner

@josevalim not sure if your comment is after we chatted or you still think that the fix won't work. But I've tested it on an application. Anyway agree, we need tests I was waiting for a final definition to see if we are setting this as default or not

Michael Koziarski
Owner

@homakov is right here, from a security perspective there's no good reason not to default escape_html_entities_in_json to true, the values aren't escaped, just encoded differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 11, 2012
  1. Egor Homakov
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 1 deletion.
  1. +11 −1 railties/lib/rails/application/bootstrap.rb
12 railties/lib/rails/application/bootstrap.rb
View
@@ -10,7 +10,17 @@ module Bootstrap
initializer :load_environment_hook, :group => :all do end
initializer :load_active_support, :group => :all do
- require "active_support/all" unless config.active_support.bare
+ unless config.active_support.bare
+ require "active_support/all"
+
+ # Assign config options of JSON encoding
+ [:escape_html_entities_in_json, :use_standard_json_time_format, :encode_big_decimal_as_string].each do |option|
+ value = config.active_support.send(option)
+ if !value.nil?
+ ActiveSupport::JSON::Encoding.send("#{option}=", value)
+ end
+ end
+ end
end
# Preload all frameworks specified by the Configuration#frameworks.
Something went wrong with that request. Please try again.