Fix regression in XSS filtering of HTML characters

A previous fix for protections for XSS in `ActionView::Helpers` and
`ERB::Util` introduced a regression by not filtering HTML characters
properly. This is a complete fix for that regression, related to rails#45027.

We would need to support XHTML, HTML4 and HTML5. But since XHTML and
HTML4 have had a note for future deprecation in the documentation for
more than 5 years, simplify the filtering by removing support for XHTML
and HTML4.

actiontext/app/helpers/action_text/tag_helper.rb

@@ -33,6 +33,10 @@ def rich_text_area_tag(name, value = nil, options = {})
       options[:data][:blob_url_template] ||= main_app.rails_service_blob_url(":signed_id", ":filename")
 
       editor_tag = content_tag("trix-editor", "", options)
+                     .gsub("<trixeditor ", "<trix-editor ") # Fix non-compliant HTML tag.
+                     .gsub("</trixeditor>", "</trix-editor>")
+                     .html_safe
+
       input_tag = hidden_field_tag(name, value.try(:to_trix_html) || value, id: options[:input], form: form)
 
       input_tag + editor_tag

actionview/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Fix regression for HTML character filtering in protections for XSS in
+    `ActionView::Helpers` and `ERB::Util`. This is a complete fix for that
+    regression, related to #45027 , which was incomplete.
+
+    We would need to support XHTML, HTML4 and HTML5. But since XHTML and HTML4
+    have had a note for future deprecation in the documentation for more than
+    5 years, simplify the filtering by removing support for XHTML and HTML4.
+
+    *Álvaro Martín Fraguas*
+
 *   Guard against `ActionView::Helpers::FormTagHelper#field_name` calls with nil
     `object_name` arguments. For example:
 

actionview/lib/action_view/helpers/form_helper.rb

@@ -773,7 +773,7 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, **options, &block)
           options[:multipart] ||= builder.multipart?
 
           html_options = html_options_for_form_with(url, model, **options)
-          form_tag_with_body(html_options, output)
+          form_tag_html(html_options, output)
         else
           html_options = html_options_for_form_with(url, model, **options)
           form_tag_html(html_options)

actionview/lib/action_view/helpers/form_tag_helper.rb

@@ -75,7 +75,7 @@ module FormTagHelper
       def form_tag(url_for_options = {}, options = {}, &block)
         html_options = html_options_for_form(url_for_options, options)
         if block_given?
-          form_tag_with_body(html_options, capture(&block))
+          form_tag_html(html_options, capture(&block))
         else
           form_tag_html(html_options)
         end
@@ -647,11 +647,12 @@ def image_submit_tag(source, options = {})
       #     <p><%= text_field_tag 'name' %></p>
       #   <% end %>
       #   # => <fieldset class="format"><p><input id="name" name="name" type="text" /></p></fieldset>
-      def field_set_tag(legend = nil, options = nil, &block)
-        output = tag(:fieldset, options, true)
-        output.safe_concat(content_tag("legend", legend)) unless legend.blank?
-        output.concat(capture(&block)) if block_given?
-        output.safe_concat("</fieldset>")
+      def field_set_tag(legend = nil, options = {}, &block)
+        content = "".html_safe
+        content.concat(content_tag("legend", legend)) unless legend.blank?
+        content.concat(capture(&block)) if block_given?
+
+        tag.fieldset(content, **options)
       end
 
       # Creates a text field of type "color".
@@ -931,7 +932,7 @@ def extra_tags_for_form(html_options)
             case method
             when "get"
               html_options["method"] = "get"
-              ""
+              "".html_safe
             when "post", ""
               html_options["method"] = "post"
               token_tag(authenticity_token, form_options: {
@@ -953,20 +954,21 @@ def extra_tags_for_form(html_options)
           end
         end
 
-        def form_tag_html(html_options)
+        def form_tag_html(html_options, content = nil)
           extra_tags = extra_tags_for_form(html_options)
-          tag(:form, html_options, true) + extra_tags
-        end
 
-        def form_tag_with_body(html_options, content)
-          output = form_tag_html(html_options)
-          output << content.to_s if content
-          output.safe_concat("</form>")
+          tag.form(extra_tags + content.to_s,
+                   **html_options)
         end
 
-        # see http://www.w3.org/TR/html4/types.html#type-name
         def sanitize_to_id(name)
-          name.to_s.delete("]").tr("^-a-zA-Z0-9:.", "_")
+          # See https://html.spec.whatwg.org/#the-id-attribute
+          invalid_characters_in_html_id = "\t\n\f\r "
+
+          name
+            .to_s
+            .delete("]")
+            .gsub(/[\[#{invalid_characters_in_html_id}]/, "_")
         end
 
         def set_default_disable_with(value, tag_options)

actionview/lib/action_view/helpers/tag_helper.rb

@@ -9,8 +9,8 @@
 module ActionView
   # = Action View Tag Helpers
   module Helpers # :nodoc:
-    # Provides methods to generate HTML tags programmatically both as a modern
-    # HTML5 compliant builder style and legacy XHTML compliant tags.
+    # Provides methods to generate HTML tags programmatically, following a modern
+    # HTML5 compliant builder style.
     module TagHelper
       include CaptureHelper
       include OutputSafetyHelper
@@ -69,17 +69,17 @@ def tag_string(name, content = nil, escape: true, **options, &block)
           content = @view_context.capture(self, &block) if block_given?
           self_closing = SVG_SELF_CLOSING_ELEMENTS.include?(name)
           if (HTML_VOID_ELEMENTS.include?(name) || self_closing) && content.nil?
-            "<#{name.to_s.dasherize}#{tag_options(options, escape)}#{self_closing ? " />" : ">"}".html_safe
+            "<#{name}#{tag_options(options, escape)}#{self_closing ? " />" : ">"}".html_safe
           else
-            content_tag_string(name.to_s.dasherize, content || "", options, escape)
+            content_tag_string(name.to_s, content || "", options, escape)
           end
         end
 
         def content_tag_string(name, content, options, escape = true)
           tag_options = tag_options(options, escape) if options
 
           if escape
-            name = ERB::Util.xml_name_escape(name)
+            name = ERB::Util.html_tag_name_escape(name)
             content = ERB::Util.unwrapped_html_escape(content)
           end
 
@@ -133,7 +133,7 @@ def boolean_tag_option(key)
         end
 
         def tag_option(key, value, escape)
-          key = ERB::Util.xml_name_escape(key) if escape
+          key = ERB::Util.html_attribute_name_escape(key) if escape
 
           case value
           when Array, Hash
@@ -167,7 +167,7 @@ def method_missing(called, *args, **options, &block)
           end
       end
 
-      # Returns an HTML tag.
+      # Returns an HTML tag builder.
       #
       # === Building HTML tags
       #
@@ -261,56 +261,22 @@ def method_missing(called, *args, **options, &block)
       # === Legacy syntax
       #
       # The following format is for legacy syntax support. It will be deprecated in future versions of Rails.
+      # It generates an HTML5 tag as well.
       #
-      #   tag(name, options = nil, open = false, escape = true)
-      #
-      # It returns an empty HTML tag of type +name+ which by default is XHTML
-      # compliant. Set +open+ to true to create an open tag compatible
-      # with HTML 4.0 and below. Add HTML attributes by passing an attributes
-      # hash to +options+. Set +escape+ to false to disable attribute value
-      # escaping.
-      #
-      # ==== Options
-      #
-      # You can use symbols or strings for the attribute names.
-      #
-      # Use +true+ with boolean attributes that can render with no value, like
-      # +disabled+ and +readonly+.
-      #
-      # HTML5 <tt>data-*</tt> attributes can be set with a single +data+ key
-      # pointing to a hash of sub-attributes.
+      #   tag(name, options = {})
       #
       # ==== Examples
       #
       #   tag("br")
-      #   # => <br />
-      #
-      #   tag("br", nil, true)
       #   # => <br>
       #
       #   tag("input", type: 'text', disabled: true)
-      #   # => <input type="text" disabled="disabled" />
-      #
-      #   tag("input", type: 'text', class: ["strong", "highlight"])
-      #   # => <input class="strong highlight" type="text" />
-      #
-      #   tag("img", src: "open & shut.png")
-      #   # => <img src="open &amp; shut.png" />
-      #
-      #   tag("img", { src: "open &amp; shut.png" }, false, false)
-      #   # => <img src="open &amp; shut.png" />
-      #
-      #   tag("div", data: { name: 'Stephen', city_state: %w(Chicago IL) })
-      #   # => <div data-name="Stephen" data-city-state="[&quot;Chicago&quot;,&quot;IL&quot;]" />
-      #
-      #   tag("div", class: { highlight: current_user.admin? })
-      #   # => <div class="highlight" />
-      def tag(name = nil, options = nil, open = false, escape = true)
+      #   # => <input type="text" disabled="disabled">
+      def tag(name = nil, options = {})
         if name.nil?
           tag_builder
         else
-          name = ERB::Util.xml_name_escape(name) if escape
-          "<#{name}#{tag_builder.tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
+          tag_builder.public_send(name.to_sym, nil, **options)
         end
       end
 

actionview/lib/action_view/helpers/url_helper.rb

@@ -800,7 +800,7 @@ def token_tag(token = nil, form_options: {})
               end
             tag(:input, type: "hidden", name: request_forgery_protection_token.to_s, value: token, autocomplete: "off")
           else
-            ""
+            "".html_safe
           end
         end
 

actionview/test/template/asset_tag_helper_test.rb

@@ -319,9 +319,9 @@ def content_security_policy_nonce
     %(video_tag("error.avi", "size" => "x")) => %(<video src="/videos/error.avi"></video>),
     %(video_tag("http://media.rubyonrails.org/video/rails_blog_2.mov")) => %(<video src="http://media.rubyonrails.org/video/rails_blog_2.mov"></video>),
     %(video_tag("//media.rubyonrails.org/video/rails_blog_2.mov")) => %(<video src="//media.rubyonrails.org/video/rails_blog_2.mov"></video>),
-    %(video_tag("multiple.ogg", "multiple.avi")) => %(<video><source src="/videos/multiple.ogg" /><source src="/videos/multiple.avi" /></video>),
-    %(video_tag(["multiple.ogg", "multiple.avi"])) => %(<video><source src="/videos/multiple.ogg" /><source src="/videos/multiple.avi" /></video>),
-    %(video_tag(["multiple.ogg", "multiple.avi"], :size => "160x120", :controls => true)) => %(<video controls="controls" height="120" width="160"><source src="/videos/multiple.ogg" /><source src="/videos/multiple.avi" /></video>)
+    %(video_tag("multiple.ogg", "multiple.avi")) => %(<video><source src="/videos/multiple.ogg"><source src="/videos/multiple.avi"></video>),
+    %(video_tag(["multiple.ogg", "multiple.avi"])) => %(<video><source src="/videos/multiple.ogg"><source src="/videos/multiple.avi"></video>),
+    %(video_tag(["multiple.ogg", "multiple.avi"], :size => "160x120", :controls => true)) => %(<video controls="controls" height="120" width="160"><source src="/videos/multiple.ogg"><source src="/videos/multiple.avi"></video>)
   }
 
   AudioPathToTag = {
@@ -357,9 +357,9 @@ def content_security_policy_nonce
     %(audio_tag("rss.wav", :autoplay => true, :controls => true)) => %(<audio autoplay="autoplay" controls="controls" src="/audios/rss.wav"></audio>),
     %(audio_tag("http://media.rubyonrails.org/audio/rails_blog_2.mov")) => %(<audio src="http://media.rubyonrails.org/audio/rails_blog_2.mov"></audio>),
     %(audio_tag("//media.rubyonrails.org/audio/rails_blog_2.mov")) => %(<audio src="//media.rubyonrails.org/audio/rails_blog_2.mov"></audio>),
-    %(audio_tag("audio.mp3", "audio.ogg")) => %(<audio><source src="/audios/audio.mp3" /><source src="/audios/audio.ogg" /></audio>),
-    %(audio_tag(["audio.mp3", "audio.ogg"])) => %(<audio><source src="/audios/audio.mp3" /><source src="/audios/audio.ogg" /></audio>),
-    %(audio_tag(["audio.mp3", "audio.ogg"], :preload => 'none', :controls => true)) => %(<audio preload="none" controls="controls"><source src="/audios/audio.mp3" /><source src="/audios/audio.ogg" /></audio>)
+    %(audio_tag("audio.mp3", "audio.ogg")) => %(<audio><source src="/audios/audio.mp3"><source src="/audios/audio.ogg"></audio>),
+    %(audio_tag(["audio.mp3", "audio.ogg"])) => %(<audio><source src="/audios/audio.mp3"><source src="/audios/audio.ogg"></audio>),
+    %(audio_tag(["audio.mp3", "audio.ogg"], :preload => 'none', :controls => true)) => %(<audio preload="none" controls="controls"><source src="/audios/audio.mp3"><source src="/audios/audio.ogg"></audio>)
   }
 
   FontPathToTag = {
@@ -773,11 +773,11 @@ def test_video_audio_tag_does_not_modify_options
 
   def test_image_tag_interpreting_email_cid_correctly
     # An inline image has no need for an alt tag to be automatically generated from the cid:
-    assert_equal '<img src="cid:thi%25%25sis@acontentid" />', image_tag("cid:thi%25%25sis@acontentid")
+    assert_equal '<img src="cid:thi%25%25sis@acontentid">', image_tag("cid:thi%25%25sis@acontentid")
   end
 
   def test_image_tag_interpreting_email_adding_optional_alt_tag
-    assert_equal '<img alt="Image" src="cid:thi%25%25sis@acontentid" />', image_tag("cid:thi%25%25sis@acontentid", alt: "Image")
+    assert_equal '<img alt="Image" src="cid:thi%25%25sis@acontentid">', image_tag("cid:thi%25%25sis@acontentid", alt: "Image")
   end
 
   def test_should_not_modify_source_string

actionview/test/template/csp_helper_test.rb

@@ -14,11 +14,11 @@ def content_security_policy?
   end
 
   def test_csp_meta_tag
-    assert_equal "<meta name=\"csp-nonce\" content=\"iyhD0Yc0W+c=\" />", csp_meta_tag
+    assert_equal "<meta name=\"csp-nonce\" content=\"iyhD0Yc0W+c=\">", csp_meta_tag
   end
 
   def test_csp_meta_tag_with_options
-    assert_equal "<meta property=\"csp-nonce\" name=\"csp-nonce\" content=\"iyhD0Yc0W+c=\" />", csp_meta_tag(property: "csp-nonce")
+    assert_equal "<meta property=\"csp-nonce\" name=\"csp-nonce\" content=\"iyhD0Yc0W+c=\">", csp_meta_tag(property: "csp-nonce")
   end
 end
 

actionview/test/template/form_tag_helper_test.rb

@@ -68,7 +68,7 @@ def url_for(options)
     end
   end
 
-  VALID_HTML_ID = /^[A-Za-z][-_:.A-Za-z0-9]*$/ # see http://www.w3.org/TR/html4/types.html#type-name
+  VALID_HTML_ID = /[^\t\n\f\r ]/ # see https://html.spec.whatwg.org/#the-id-attribute
 
   def test_check_box_tag
     actual = check_box_tag "admin"
@@ -93,6 +93,11 @@ def test_check_box_tag_id_sanitized
     assert_match VALID_HTML_ID, label_elem["id"]
   end
 
+  def test_id_sanitized_for_html5
+    label_elem = root_elem(check_box_tag("project[2][a(dmin]"))
+    assert_equal "project_2_a(dmin", label_elem["id"]
+  end
+
   def test_form_tag
     actual = form_tag
     expected = whole_form
@@ -689,14 +694,14 @@ def test_submit_tag_with_confirmation
 
   def test_submit_tag_doesnt_have_data_disable_with_twice
     assert_equal(
-      %(<input type="submit" name="commit" value="Save" data-confirm="Are you sure?" data-disable-with="Processing..." />),
+      %(<input type="submit" name="commit" value="Save" data-confirm="Are you sure?" data-disable-with="Processing...">),
       submit_tag("Save", "data-disable-with" => "Processing...", "data-confirm" => "Are you sure?")
     )
   end
 
   def test_submit_tag_doesnt_have_data_disable_with_twice_with_hash
     assert_equal(
-      %(<input type="submit" name="commit" value="Save" data-disable-with="Processing..." />),
+      %(<input type="submit" name="commit" value="Save" data-disable-with="Processing...">),
       submit_tag("Save", data: { disable_with: "Processing..." })
     )
   end

actionview/test/template/output_safety_helper_test.rb

@@ -80,7 +80,7 @@ def setup
       safe_join(["<marquee>shady stuff</marquee>", tag("br")])
     end
     url = "https://example.com"
-    expected = %(<a href="#{url}">#{url}</a> and <p>&lt;marquee&gt;shady stuff&lt;/marquee&gt;<br /></p>)
+    expected = %(<a href="#{url}">#{url}</a> and <p>&lt;marquee&gt;shady stuff&lt;/marquee&gt;<br></p>)
     actual = to_sentence([link_to(url, url), ptag])
     assert_predicate actual, :html_safe?
     assert_equal(expected, actual)