From 039f9b37b9e90e7a36b1290dbab33606ea7549ab Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 27 Nov 2013 01:49:25 -0800 Subject: Added failing test for json_escape striping quotation marks Expanded test coverage for html_escape and json_escape --- actionview/test/template/erb_util_test.rb | 45 +++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/actionview/test/template/erb_util_test.rb b/actionview/test/template/erb_util_test.rb index 9a7c617eb3..94552a6d9b 100644 --- a/actionview/test/template/erb_util_test.rb +++ b/actionview/test/template/erb_util_test.rb @@ -1,4 +1,5 @@ require 'abstract_unit' +require 'active_support/json' class ErbUtilTest < ActiveSupport::TestCase include ERB::Util @@ -15,6 +16,50 @@ class ErbUtilTest < ActiveSupport::TestCase end end + HTML_ESCAPE_TEST_CASES = [ + ['
', '<br>'], + ['a & b', 'a & b'], + ['"quoted" string', '"quoted" string'], + ["'quoted' string", ''quoted' string'], + [ + '', + '<script type="application/javascript">alert("You are 'pwned'!")</script>' + ] + ] + + JSON_ESCAPE_TEST_CASES = [ + ['1', '1'], + ['null', 'null'], + ['"&"', '"\u0026"'], + ['""', '"\u003C/script\u003E"'], + ['[""]', '["\u003C/script\u003E"]'], + ['{"name":""}', '{"name":"\u003C/script\u003E"}'] + ] + + def test_html_escape + HTML_ESCAPE_TEST_CASES.each do |(raw, expected)| + assert_equal expected, html_escape(raw) + end + end + + def test_json_escape + JSON_ESCAPE_TEST_CASES.each do |(raw, expected)| + assert_equal expected, json_escape(raw) + end + end + + def test_json_escape_does_not_alter_json_string_meaning + JSON_ESCAPE_TEST_CASES.each do |(raw, _)| + assert_equal ActiveSupport::JSON.decode(raw), ActiveSupport::JSON.decode(json_escape(raw)) + end + end + + def test_json_escape_is_idempotent + JSON_ESCAPE_TEST_CASES.each do |(raw, _)| + assert_equal json_escape(raw), json_escape(json_escape(raw)) + end + end + def test_json_escape_returns_unsafe_strings_when_passed_unsafe_strings value = json_escape("asdf") assert !value.html_safe? -- cgit v1.2.3 From 2f1c5789c1882413df0346fec1f29eed24f37698 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 27 Nov 2013 02:36:10 -0800 Subject: Fixed a long-standing bug in `json_escape` that strips quotation marks --- actionview/CHANGELOG.md | 4 ++ .../core_ext/string/output_safety.rb | 61 +++++++++++++++++----- 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index d53b321f97..65d045d1f7 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fixed a long-standing bug in `json_escape` that causes quotation marks to be stripped. + + *Godfrey Chan* + * `ActionView::MissingTemplate` includes underscore when raised for a partial. Fixes #13002. diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index c21650bb83..d54265f1b1 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -6,7 +6,7 @@ class ERB HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' } HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+));)/ - JSON_ESCAPE_REGEXP = /[&"><]/ + JSON_ESCAPE_REGEXP = /[&><]/ # A utility method for escaping HTML tag characters. # This method is also aliased as h. @@ -48,17 +48,54 @@ class ERB module_function :html_escape_once - # A utility method for escaping HTML entities in JSON strings - # using \uXXXX JavaScript escape sequences for string literals: - # - # json_escape('is a > 0 & a < 10?') - # # => is a \u003E 0 \u0026 a \u003C 10? - # - # Note that after this operation is performed the output is not - # valid JSON. In particular double quotes are removed: - # - # json_escape('{"name":"john","created_at":"2010-04-28T01:39:31Z","id":1}') - # # => {name:john,created_at:2010-04-28T01:39:31Z,id:1} + # A utility method for escaping HTML entities in JSON strings. Specifically, the + # &, > and < characters are replaced with their equivilant unicode escaped form - + # \u0026, \u003e, and \u003c. These sequences has identical meaning as the original + # characters inside the context of a JSON string, so assuming the input is a valid + # and well-formed JSON value, the output will have equivilant meaning when parsed: + # + # json = JSON.generate({ name: ""}) + # # => "{\"name\":\"\"}" + # + # json_escape(json) + # # => "{\"name\":\"\\u003C/script\\u003E\\u003Cscript\\u003Ealert('PWNED!!!')\\u003C/script\\u003E\"}" + # + # JSON.parse(json) == JSON.parse(json_escape(json)) + # # => true + # + # The intended use case for this method is to escape JSON strings before including + # them inside a script tag to avoid XSS vulnerability: + # + # + # + # WARNING: this helper only works with valid JSON. Using this on non-JSON values + # will open up serious XSS vulnerabilities. For example, if you replace the + # +current_user.to_json+ in the example above with user input instead, the browser + # will happily eval() that string as JavaScript. + # + # The escaping performed in this method is identical to those performed in the + # ActiveSupport JSON encoder when +ActiveSupport.escape_html_entities_in_json+ is + # set to true. Because this transformation is idempotent, this helper can be + # applied even if +ActiveSupport.escape_html_entities_in_json+ is already true. + # + # Therefore, when you are unsure if +ActiveSupport.escape_html_entities_in_json+ + # is enabled, or if you are unsure where your JSON string originated from, it + # is recommended that you always apply this helper (other libraries, such as the + # JSON gem, does not provide this kind of protection by default; also some gems + # might override +#to_json+ to bypass ActiveSupport's encoder). + # + # The output of this helper method is marked as HTML safe so that you can directly + # include it inside a +"', '"\u003C/script\u003E"'], - ['[""]', '["\u003C/script\u003E"]'], - ['{"name":""}', '{"name":"\u003C/script\u003E"}'] + ['""', '"\u003c/script\u003e"'], + ['[""]', '["\u003c/script\u003e"]'], + ['{"name":""}', '{"name":"\u003c/script\u003e"}'] ] def test_html_escape diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index d54265f1b1..0e07e5952f 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -4,7 +4,7 @@ require 'active_support/core_ext/kernel/singleton_class' class ERB module Util HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } - JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' } + JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c' } HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+));)/ JSON_ESCAPE_REGEXP = /[&><]/ -- cgit v1.2.3 From 2c564cdbdbe62c319e65abb3631b288f11878987 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 4 Dec 2013 09:43:42 -0800 Subject: Added \u2028 \u2029 to json_escape --- actionview/CHANGELOG.md | 3 +++ actionview/test/template/erb_util_test.rb | 3 ++- .../lib/active_support/core_ext/string/output_safety.rb | 12 +++++++----- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 65d045d1f7..9e58c193b1 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,4 +1,7 @@ * Fixed a long-standing bug in `json_escape` that causes quotation marks to be stripped. + This method also escapes the \u2028 and \u2029 unicode newline characters which are + treated as \n in JavaScript. This matches the behaviour of the AS::JSON encoder. (The + original change in the encoder was introduced in #10534.) *Godfrey Chan* diff --git a/actionview/test/template/erb_util_test.rb b/actionview/test/template/erb_util_test.rb index 62067ad097..9bacbba908 100644 --- a/actionview/test/template/erb_util_test.rb +++ b/actionview/test/template/erb_util_test.rb @@ -33,7 +33,8 @@ class ErbUtilTest < ActiveSupport::TestCase ['"&"', '"\u0026"'], ['""', '"\u003c/script\u003e"'], ['[""]', '["\u003c/script\u003e"]'], - ['{"name":""}', '{"name":"\u003c/script\u003e"}'] + ['{"name":""}', '{"name":"\u003c/script\u003e"}'], + [%({"name":"d\u2028h\u2029h"}), '{"name":"d\u2028h\u2029h"}'] ] def test_html_escape diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index 0e07e5952f..1d23998b88 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -4,9 +4,9 @@ require 'active_support/core_ext/kernel/singleton_class' class ERB module Util HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } - JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c' } + JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c', "\u2028" => '\u2028', "\u2029" => '\u2029' } HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+));)/ - JSON_ESCAPE_REGEXP = /[&><]/ + JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u # A utility method for escaping HTML tag characters. # This method is also aliased as h. @@ -50,9 +50,11 @@ class ERB # A utility method for escaping HTML entities in JSON strings. Specifically, the # &, > and < characters are replaced with their equivilant unicode escaped form - - # \u0026, \u003e, and \u003c. These sequences has identical meaning as the original - # characters inside the context of a JSON string, so assuming the input is a valid - # and well-formed JSON value, the output will have equivilant meaning when parsed: + # \u0026, \u003e, and \u003c. The Unicode sequences \u2028 and \u2029 are also + # escaped as then are treated as newline characters in some JavaScript engines. + # These sequences has identical meaning as the original characters inside the + # context of a JSON string, so assuming the input is a valid and well-formed + # JSON value, the output will have equivilant meaning when parsed: # # json = JSON.generate({ name: ""}) # # => "{\"name\":\"\"}" -- cgit v1.2.3 From 0696547814057eaed3c13e70a6dc6b2b7bb3e1f9 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 4 Dec 2013 09:46:38 -0800 Subject: Also move html_esacpe regex to a constant (see 9d25af60) --- activesupport/lib/active_support/core_ext/string/output_safety.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index 1d23998b88..23f95341f8 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -5,6 +5,7 @@ class ERB module Util HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003e', '<' => '\u003c', "\u2028" => '\u2028', "\u2029" => '\u2029' } + HTML_ESCAPE_REGEXP = /[&"'><]/ HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+));)/ JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u @@ -21,7 +22,7 @@ class ERB if s.html_safe? s else - s.gsub(/[&"'><]/, HTML_ESCAPE).html_safe + s.gsub(HTML_ESCAPE_REGEXP, HTML_ESCAPE).html_safe end end -- cgit v1.2.3