Backport HTML safety fix for 2.x (#1962)

Author: camertron

docs/CHANGELOG.md

@@ -10,6 +10,10 @@ nav_order: 5
 
 ## main
 
+* Ensure HTML output safety.
+
+    *Cameron Dutro*
+
 ## 2.82.0
 
 * Revert "Avoid loading ActionView::Base during initialization (#1528)"

lib/view_component/base.rb

@@ -130,7 +130,12 @@ def render_in(view_context, &block)
       before_render
 
       if render?
-        render_template_for(@__vc_variant).to_s + output_postamble
+        # Avoid allocating new string when output_postamble is blank
+        if output_postamble.blank?
+          safe_render_template_for(@__vc_variant).to_s
+        else
+          safe_render_template_for(@__vc_variant).to_s + safe_output_postamble
+        end
       else
         ""
       end
@@ -157,7 +162,7 @@ def render_parent
     #
     # @return [String]
     def output_postamble
-      ""
+      @@default_output_postamble ||= "".html_safe
     end
 
     # Called before rendering the component. Override to perform operations that
@@ -309,6 +314,38 @@ def content_evaluated?
       @__vc_content_evaluated
     end
 
+    def maybe_escape_html(text)
+      return text if request && !request.format.html?
+      return text if text.blank?
+
+      if text.html_safe?
+        text
+      else
+        yield
+        html_escape(text)
+      end
+    end
+
+    def safe_render_template_for(variant)
+      if compiler.renders_template_for_variant?(variant)
+        render_template_for(variant)
+      else
+        maybe_escape_html(render_template_for(variant)) do
+          Kernel.warn("WARNING: The #{self.class} component rendered HTML-unsafe output. The output will be automatically escaped, but you may want to investigate.")
+        end
+      end
+    end
+
+    def safe_output_postamble
+      maybe_escape_html(output_postamble) do
+        Kernel.warn("WARNING: The #{self.class} component was provided an HTML-unsafe postamble. The postamble will be automatically escaped, but you may want to investigate.")
+      end
+    end
+
+    def compiler
+      @compiler ||= self.class.compiler
+    end
+
     # Set the controller used for testing components:
     #
     # ```ruby

lib/view_component/compiler.rb

@@ -16,6 +16,7 @@ class Compiler
     def initialize(component_class)
       @component_class = component_class
       @redefinition_lock = Mutex.new
+      @variants_rendering_templates = Set.new
     end
 
     def compiled?
@@ -61,6 +62,7 @@ def compile(raise_errors: false, force: false)
         # Remove existing compiled template methods,
         # as Ruby warns when redefining a method.
         method_name = call_method_name(template[:variant])
+        @variants_rendering_templates << template[:variant]
 
         redefinition_lock.synchronize do
           component_class.silence_redefinition_of_method(method_name)
@@ -81,6 +83,10 @@ def #{method_name}
       CompileCache.register(component_class)
     end
 
+    def renders_template_for_variant?(variant)
+      @variants_rendering_templates.include?(variant)
+    end
+
     private
 
     attr_reader :component_class, :redefinition_lock

test/sandbox/app/components/after_render_component.rb

@@ -2,10 +2,10 @@
 
 class AfterRenderComponent < ViewComponent::Base
   def call
-    "Hello, "
+    "Hello, ".html_safe
   end
 
   def output_postamble
-    "World!"
+    "World!".html_safe
   end
 end

test/sandbox/app/components/custom_test_controller_component.rb

@@ -2,6 +2,6 @@
 
 class CustomTestControllerComponent < ViewComponent::Base
   def call
-    helpers.foo
+    html_escape(helpers.foo)
   end
 end

test/sandbox/app/components/deprecated_slots_setter_component.rb

@@ -8,6 +8,6 @@ class DeprecatedSlotsSetterComponent < ViewComponent::Base
 
   def call
     header
-    items
+    html_escape(items)
   end
 end

test/sandbox/app/components/inherited_from_uncompilable_component.rb

@@ -2,6 +2,6 @@
 
 class InheritedFromUncompilableComponent < UncompilableComponent
   def call
-    "<div>hello world</div>"
+    "<div>hello world</div>".html_safe
   end
 end

test/sandbox/app/components/inline_render_component.rb

@@ -6,6 +6,6 @@ def initialize(items:)
   end
 
   def call
-    @items.map { |c| render(c) }.join
+    @items.map { |c| render(c) }.join.html_safe
   end
 end

test/sandbox/app/components/message_component.rb

@@ -6,6 +6,6 @@ def initialize(message:)
   end
 
   def call
-    @message
+    html_escape(@message)
   end
 end

test/sandbox/app/components/unsafe_component.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class UnsafeComponent < ViewComponent::Base
+  def call
+    user_input = "<script>alert('hello!')</script>"
+
+    "<div>hello #{user_input}</div>"
+  end
+end

test/sandbox/app/components/unsafe_postamble_component.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class UnsafePostambleComponent < ViewComponent::Base
+  def call
+    "<div>some content</div>".html_safe
+  end
+
+  def output_postamble
+    "<script>alert('hello!')</script>"
+  end
+end

test/sandbox/app/components/variant_ivar_component.rb

@@ -6,6 +6,6 @@ def initialize(variant:)
   end
 
   def call
-    @variant.to_s
+    html_escape(@variant.to_s)
   end
 end

test/sandbox/app/controllers/integration_examples_controller.rb

@@ -55,4 +55,12 @@ def inherited_sidecar
   def inherited_from_uncompilable_component
     render(InheritedFromUncompilableComponent.new)
   end
+
+  def unsafe_component
+    render(UnsafeComponent.new)
+  end
+
+  def unsafe_postamble_component
+    render(UnsafePostambleComponent.new)
+  end
 end

test/sandbox/config/routes.rb

@@ -31,6 +31,8 @@
   get :cached_partial, to: "integration_examples#cached_partial"
   get :inherited_sidecar, to: "integration_examples#inherited_sidecar"
   get :inherited_from_uncompilable_component, to: "integration_examples#inherited_from_uncompilable_component"
+  get :unsafe_component, to: "integration_examples#unsafe_component"
+  get :unsafe_postamble_component, to: "integration_examples#unsafe_postamble_component"
 
   constraints(lambda { |request| request.env["warden"].authenticate! }) do
     get :constraints_with_env, to: "integration_examples#index"

test/sandbox/test/collection_test.rb

@@ -12,7 +12,7 @@ def initialize(**attributes)
       end
 
       def call
-        "<div data-name='#{product.name}'><h1>#{product.name}</h1></div>"
+        "<div data-name='#{product.name}'><h1>#{product.name}</h1></div>".html_safe
       end
     end
 

test/sandbox/test/integration_test.rb

@@ -165,7 +165,7 @@ def test_rendering_component_with_a_partial
     get "/partial"
     assert_response :success
 
-    assert_select("div", "hello,partial world!", count: 2)
+    assert_select("div", text: "hello,partial world!", count: 4)
   end
 
   def test_rendering_component_without_variant
@@ -685,4 +685,22 @@ def test_config_options_shared_between_base_and_engine
       config_entrypoints.rotate!
     end
   end
+
+  def test_unsafe_component
+    warnings = capture_warnings { get "/unsafe_component" }
+    assert_select("script", false)
+    assert(
+      warnings.any? { |warning| warning.include?("component rendered HTML-unsafe output") },
+      "Rendering UnsafeComponent did not emit an HTML safety warning"
+    )
+  end
+
+  def test_unsafe_postamble_component
+    warnings = capture_warnings { get "/unsafe_postamble_component" }
+    assert_select("script", false)
+    assert(
+      warnings.any? { |warning| warning.include?("component was provided an HTML-unsafe postamble") },
+      "Rendering UnsafePostambleComponent did not emit an HTML safety warning"
+    )
+  end
 end

test/test_helper.rb

@@ -167,3 +167,11 @@ def with_compiler_mode(mode)
 ensure
   ViewComponent::Compiler.mode = previous_mode
 end
+
+def capture_warnings(&block)
+  [].tap do |warnings|
+    Kernel.stub(:warn, ->(msg) { warnings << msg }) do
+      block.call
+    end
+  end
+end