Use <script type="application/json"> for props and store (#775) for p…

…erformance

Initial implementation started by https://github.com/eliaslopezgt and then https://github.com/squadette
* Refactor console output to use <script> tag with a certain ID
* Use <script type="application/json"> with JSON content instead of attribute
* Pretty-print JSON in development and testing
* getAttribute() is better, because it handles attributes which are not present.
* Return skip display none to react on rails configuration
* Extract data-js-react-on-rails-store attribute to the const
* Return to_json method call to app_props_server_render to ensure handling the both cases
* Delete unnecessary style method and HIDDEN constant from react component module
* Improve jsonEl parsing to handle props as already converted from JSON to a string
* Move handling props already converted to json string to ruby code
* Escape json from props as json string
* Add test to ensure that even json string props are sanitezed
* Delete test json props because we parse all props to a hash now

CHANGELOG.md

@@ -6,6 +6,9 @@ Contributors: please follow the recommendations outlined at [keepachangelog.com]
 ## [Unreleased]
 *Please add entries here for your pull requests.*
 
+## Improved
+- Use <script type="application/json"> for props and store instead of hidden div [#775] (https://github.com/shakacode/react_on_rails/pull/775) by [cheremukhin23](https://github.com/cheremukhin23).
+
 ## [6.8.2] - 2017-03-24
 ## Fixed
 - Change webpack output path to absolute and update webpack to version ^2.3.1. [#771](https://github.com/shakacode/react_on_rails/pull/771) by [cheremukhin23](https://github.com/cheremukhin23).

app/helpers/react_on_rails_helper.rb

@@ -103,13 +103,10 @@ def react_component(component_name, raw_options = {})
 
     # Setup the page_loaded_js, which is the same regardless of prerendering or not!
     # The reason is that React is smart about not doing extra work if the server rendering did its job.
-
-    component_specification_tag =
-      content_tag(:div,
-                  "",
-                  class: "js-react-on-rails-component",
-                  style: options.style,
-                  data: options.data)
+    component_specification_tag = content_tag(:script,
+                                              json_safe_and_pretty(options.data).html_safe,
+                                              type: "application/json",
+                                              class: "js-react-on-rails-component")
 
     # Create the HTML rendering part
     result = server_rendered_react_component_html(options.props, options.name, options.dom_id,
@@ -174,7 +171,7 @@ def redux_store_hydration_data
   end
 
   def sanitized_props_string(props)
-    props.is_a?(String) ? json_escape(props) : props.to_json
+    props.is_a?(String) ? ERB::Util.json_escape(props) : props.to_json
   end
 
   # Helper method to take javascript expression and returns the output from evaluating it.
@@ -225,30 +222,36 @@ def server_render_js(js_expression, options = {})
 
   private
 
+  def json_safe_and_pretty(something)
+    if Rails.env.development?
+      ERB::Util.json_escape(JSON.pretty_generate(something.as_json))
+    else
+      ERB::Util.json_escape(something.to_json)
+    end
+  end
+
   # prepend the rails_context if not yet applied
   def prepend_render_rails_context(render_value)
     return render_value if @rendered_rails_context
 
-    data = {
-      rails_context: rails_context(server_side: false)
-    }
+    data = rails_context(server_side: false)
 
     @rendered_rails_context = true
 
-    rails_context_content = content_tag(:div,
-                                        "",
-                                        id: "js-react-on-rails-context",
-                                        style: ReactOnRails.configuration.skip_display_none ? nil : "display:none",
-                                        data: data)
+    rails_context_content = content_tag(:script,
+                                        json_safe_and_pretty(data).html_safe,
+                                        type: "application/json",
+                                        id: "js-react-on-rails-context")
+
     "#{rails_context_content}\n#{render_value}".html_safe
   end
 
   def render_redux_store_data(redux_store_data)
-    result = content_tag(:div,
-                         "",
-                         class: "js-react-on-rails-store",
-                         style: ReactOnRails.configuration.skip_display_none ? nil : "display:none",
-                         data: redux_store_data)
+    result = content_tag(:script,
+                         json_safe_and_pretty(redux_store_data[:props]).html_safe,
+                         type: "application/json",
+                         "data-js-react-on-rails-store" => redux_store_data[:store_name].html_safe)
+
     prepend_render_rails_context(result)
   end
 

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt

@@ -70,9 +70,6 @@ ReactOnRails.configure do |config|
   # MISCELLANEOUS OPTIONS
   ################################################################################
 
-  # Default is false, enable if your content security policy doesn't include `style-src: 'unsafe-inline'`
-  config.skip_display_none = false
-
   # The server render method - either ExecJS or NodeJS
   <%- if options.node? -%>
   config.server_render_method = "NodeJS"

lib/react_on_rails/configuration.rb

@@ -9,6 +9,7 @@ def self.configure
   def self.setup_config_values
     ensure_webpack_generated_files_exists
     configure_generated_assets_dirs_deprecation
+    configure_skip_display_none_deprecation
     ensure_generated_assets_dir_present
     ensure_server_bundle_js_file_has_no_path
     check_i18n_directory_exists
@@ -61,6 +62,11 @@ def self.ensure_server_bundle_js_file_has_no_path
     @configuration.server_bundle_js_file = File.basename(@configuration.server_bundle_js_file)
   end
 
+  def self.configure_skip_display_none_deprecation
+    return if @configuration.skip_display_none.nil?
+    puts "[DEPRECATION] ReactOnRails: remove skip_display_none from configuration."
+  end
+
   def self.configuration
     @configuration ||= Configuration.new(
       generated_assets_dirs: nil,
@@ -76,7 +82,9 @@ def self.configuration
       development_mode: Rails.env.development?,
       server_renderer_pool_size: 1,
       server_renderer_timeout: 20,
-      skip_display_none: false,
+      skip_display_none: nil,
+
+      # skip_display_none is deprecated
       webpack_generated_files: [],
       rendering_extension: nil,
       server_render_method: "",
@@ -91,8 +99,8 @@ class Configuration
     attr_accessor :server_bundle_js_file, :prerender, :replay_console,
                   :trace, :development_mode,
                   :logging_on_server, :server_renderer_pool_size,
-                  :server_renderer_timeout, :raise_on_prerender_error,
-                  :skip_display_none, :generated_assets_dirs, :generated_assets_dir,
+                  :server_renderer_timeout, :skip_display_none, :raise_on_prerender_error,
+                  :generated_assets_dirs, :generated_assets_dir,
                   :webpack_generated_files, :rendering_extension, :npm_build_test_command,
                   :npm_build_production_command,
                   :i18n_dir,

lib/react_on_rails/react_component/options.rb

@@ -6,15 +6,15 @@ class Options
       include Utils::Required
 
       NO_PROPS = {}.freeze
-      HIDDEN = "display:none".freeze
 
       def initialize(name: required("name"), options: required("options"))
         @name = name
         @options = options
       end
 
       def props
-        options.fetch(:props) { NO_PROPS }
+        props = options.fetch(:props) { NO_PROPS }
+        props.is_a?(String) ? JSON.parse(ERB::Util.json_escape(props)) : props
       end
 
       def name
@@ -54,11 +54,6 @@ def data
         }
       end
 
-      def style
-        return nil if ReactOnRails.configuration.skip_display_none
-        HIDDEN
-      end
-
       private
 
       attr_reader :options

node_package/src/RenderUtils.js

@@ -1,10 +1,10 @@
 export default {
-  wrapInScriptTags(scriptBody) {
+  wrapInScriptTags(scriptId, scriptBody) {
     if (!scriptBody) {
       return '';
     }
 
-    return `\n<script>
+    return `\n<script id="${scriptId}">
 ${scriptBody}
 </script>`;
   },

node_package/src/buildConsoleReplay.js

@@ -28,5 +28,5 @@ export function consoleReplay() {
 }
 
 export default function buildConsoleReplay() {
-  return RenderUtils.wrapInScriptTags(consoleReplay());
+  return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay());
 }

node_package/src/clientStartup.js

@@ -5,8 +5,7 @@ import ReactDOM from 'react-dom';
 import createReactElement from './createReactElement';
 import isRouterResult from './isCreateReactElementResultNonReactComponent';
 
-const REACT_ON_RAILS_COMPONENT_CLASS_NAME = 'js-react-on-rails-component';
-const REACT_ON_RAILS_STORE_CLASS_NAME = 'js-react-on-rails-store';
+const REACT_ON_RAILS_STORE_ATTRIBUTE = 'data-js-react-on-rails-store';
 
 function findContext() {
   if (typeof window.ReactOnRails !== 'undefined') {
@@ -42,21 +41,28 @@ function forEach(fn, className, railsContext) {
   }
 }
 
+function forEachByAttribute(fn, attributeName, railsContext) {
+  const els = document.querySelectorAll(`[${attributeName}]`);
+  for (let i = 0; i < els.length; i += 1) {
+    fn(els[i], railsContext);
+  }
+}
+
 function forEachComponent(fn, railsContext) {
-  forEach(fn, REACT_ON_RAILS_COMPONENT_CLASS_NAME, railsContext);
+  forEach(fn, 'js-react-on-rails-component', railsContext);
 }
 
 function initializeStore(el, railsContext) {
   const context = findContext();
-  const name = el.getAttribute('data-store-name');
-  const props = JSON.parse(el.getAttribute('data-props'));
+  const name = el.getAttribute(REACT_ON_RAILS_STORE_ATTRIBUTE);
+  const props = JSON.parse(el.textContent);
   const storeGenerator = context.ReactOnRails.getStoreGenerator(name);
   const store = storeGenerator(props, railsContext);
   context.ReactOnRails.setStore(name, store);
 }
 
 function forEachStore(railsContext) {
-  forEach(initializeStore, REACT_ON_RAILS_STORE_CLASS_NAME, railsContext);
+  forEachByAttribute(initializeStore, REACT_ON_RAILS_STORE_ATTRIBUTE, railsContext);
 }
 
 function turbolinksVersion5() {
@@ -91,10 +97,11 @@ DELEGATING TO RENDERER ${name} for dom node with id: ${domNodeId} with props, ra
  */
 function render(el, railsContext) {
   const context = findContext();
-  const name = el.getAttribute('data-component-name');
-  const domNodeId = el.getAttribute('data-dom-id');
-  const props = JSON.parse(el.getAttribute('data-props'));
-  const trace = JSON.parse(el.getAttribute('data-trace'));
+  const jsonEl = JSON.parse(el.textContent);
+  const name = jsonEl.component_name;
+  const domNodeId = jsonEl.dom_id;
+  const props = jsonEl.props;
+  const trace = jsonEl.trace;
 
   try {
     const domNode = document.getElementById(domNodeId);
@@ -130,7 +137,7 @@ You should return a React.Component always for the client side entry point.`);
 function parseRailsContext() {
   const el = document.getElementById('js-react-on-rails-context');
   if (el) {
-    return JSON.parse(el.getAttribute('data-rails-context'));
+    return JSON.parse(el.textContent);
   }
 
   return null;
@@ -145,7 +152,8 @@ export function reactOnRailsPageLoaded() {
 }
 
 function unmount(el) {
-  const domNodeId = el.getAttribute('data-dom-id');
+  const elData = JSON.parse(el.textContent);
+  const domNodeId = elData.dom_id;
   const domNode = document.getElementById(domNodeId);
   ReactDOM.unmountComponentAtNode(domNode);
 }

node_package/tests/buildConsoleReplay.test.js

@@ -85,7 +85,7 @@ test('buildConsoleReplay wraps console replay in a script tag', (assert) => {
   const actual = buildConsoleReplay();
 
   const expected = `
-<script>
+<script id="consoleReplayLog">
 console.log.apply(console, ["some message","{\\"a\\":1,\\"b\\":2}"]);
 console.warn.apply(console, ["other message","{\\"c\\":3,\\"d\\":4}"]);
 </script>`;

spec/dummy/app/controllers/pages_controller.rb

@@ -26,23 +26,24 @@ def initialize_shared_store
   end
 
   def data
+    xss_payload = { "<script>window.alert('xss1');</script>" => '<script>window.alert("xss2");</script>' }
     # This is the props used by the React component.
     @app_props_server_render = {
       helloWorldData: {
         name: "Mr. Server Side Rendering"
-      }
+      }.merge(xss_payload)
     }
 
     @app_props_hello = {
       helloWorldData: {
         name: "Mrs. Client Side Rendering"
-      }
+      }.merge(xss_payload)
     }
 
     @app_props_hello_again = {
       helloWorldData: {
         name: "Mrs. Client Side Hello Again"
-      }
+      }.merge(xss_payload)
     }
   end
 end

spec/dummy/config/initializers/react_on_rails.rb

@@ -81,8 +81,6 @@ def self.custom_context(view_context)
   ################################################################################
   # MISCELLANEOUS OPTIONS
   ################################################################################
-  # Default is false, enable if your content security policy doesn't include `style-src: 'unsafe-inline'`
-  config.skip_display_none = false
 
   # This allows you to add additional values to the Rails Context. Implement one static method
   # called `custom_context(view_context)` and return a Hash.