Escape / and > also so apps cannot be broken by browsers trying to 'correct' the HTML

[JackMc]

No description provided.

[ljharb]

Can you elaborate more on how this doesn't work as-is?

[JackMc]

Oh whoops, I didn't know I was opening this on the main repo. :) Sorry for the small description.

In Firefox, any of the props containing </script will cause the application to break, as Firefox tries to complete the tag and creates a </script>.

[JackMc]

Additionally, for the failing test, it seems to also fail on master so not sure how to fix it.

[clayton-shopify]

Since it's JSON that's being generated, I think it would make more sense to use JSON escaping rather than HTML escaping.

Rails' to_json takes this approach to allow JSON to be used safely inside <script> tags:

https://github.com/rails/rails/blob/b2eb1d1c55a59fee1e6c4cba7030d8ceb524267c/activesupport/lib/active_support/json/encoding.rb#L39-L65

We could use that approach here like so:

diff --git a/lib/hypernova/blank_renderer.rb b/lib/hypernova/blank_renderer.rb
index 59ae67d..00bd3c7 100644
--- a/lib/hypernova/blank_renderer.rb+++ b/lib/hypernova/blank_renderer.rb@@ -16,12 +16,20 @@ class Hypernova::BlankRenderer
attr_reader :job
+ ESCAPED_CHARS = {+ "\u2028" => '\u2028',+ "\u2029" => '\u2029',+ ">" => '\u003e',+ "<" => '\u003c',+ "&" => '\u0026',+ }+
def data
job[:data]
end
def encode
- JSON.generate(data).gsub(/&/, '&amp;').gsub(/>/, '&gt;')+ JSON.generate(data).gsub(/[\u2028\u2029><&]/u, ESCAPED_CHARS)
end
def key
diff --git a/spec/blank_renderer_spec.rb b/spec/blank_renderer_spec.rb
index c5b6255..9cfe9c9 100644
--- a/spec/blank_renderer_spec.rb+++ b/spec/blank_renderer_spec.rb@@ -32,15 +32,15 @@ describe Hypernova::BlankRenderer do
it "encodes data correctly" do
str = described_class.new({
data: {
- foo: '</script>',+ foo: "\u2028\u2029</script>",
bar: '&gt;',
baz: '&amp;',
}
}).send(:encode)
- expect(str).to match(/<\/script&gt;/)- expect(str).to match(/&amp;gt;/)- expect(str).to match(/&amp;amp;/)+ expect(str).to match(/\\u2028\\u2029\\u003c\/script\\u003e/)+ expect(str).to match(/\\u0026gt;/)+ expect(str).to match(/\\u0026amp;/)
end
end

[richter-alex]

Hey @ljharb or @goatslacker - would either of you have any bandwidth available to offer thoughts on the above proposal? Also, running tests locally seems to be passing now, so maybe we can restart the build.