Ensure HTML output safety (#1950)

camertron · web-flow · commit 0d26944a8d27 · 2024-01-04T11:25:54.000-08:00
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -89,7 +89,7 @@ jobs:
       uses: actions/upload-artifact@v3.1.3
       if: always()
       with:
-        name: simplecov-resultset-rails${{matrix.rails_version}}-ruby${{matrix.ruby_version}}
+        name: simplecov-resultset-rails${{matrix.rails_version}}-ruby${{matrix.ruby_version}}-${{matrix.mode}}
         path: coverage
   primer_view_components_compatibility:
     name: Test compatibility with Primer ViewComponents (main)
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -1,3 +1,4 @@
+ruby_version: 2.5
 require:
   - standard
   - "rubocop-md"
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
@@ -38,6 +38,10 @@ nav_order: 5
 
     *Mitchell Henke*
 
+* Ensure HTML output safety.
+
+    *Cameron Dutro*
+
 ## 3.8.0
 
 * Use correct value for the `config.action_dispatch.show_exceptions` config option for edge Rails.
diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb
@@ -106,9 +106,9 @@ def render_in(view_context, &block)
       if render?
         # Avoid allocating new string when output_postamble is blank
         if output_postamble.blank?
-          render_template_for(@__vc_variant).to_s
+          safe_render_template_for(@__vc_variant).to_s
         else
-          render_template_for(@__vc_variant).to_s + output_postamble
+          safe_render_template_for(@__vc_variant).to_s + safe_output_postamble
         end
       else
         ""
@@ -160,7 +160,7 @@ def render_parent_to_string
     #
     # @return [String]
     def output_postamble
-      ""
+      @@default_output_postamble ||= "".html_safe
     end
 
     # Called before rendering the component. Override to perform operations that
@@ -307,6 +307,38 @@ def content_evaluated?
       defined?(@__vc_content_evaluated) && @__vc_content_evaluated
     end
 
+    def maybe_escape_html(text)
+      return text if request && !request.format.html?
+      return text if text.nil? || text.empty?
+
+      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
diff --git a/lib/view_component/compiler.rb b/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?
@@ -68,6 +69,7 @@ def render_template_for(variant = nil)
       else
         templates.each do |template|
           method_name = call_method_name(template[:variant])
+          @variants_rendering_templates << template[:variant]
 
           redefinition_lock.synchronize do
             component_class.silence_redefinition_of_method(method_name)
@@ -89,6 +91,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
diff --git a/lib/view_component/test_helpers.rb b/lib/view_component/test_helpers.rb
@@ -178,13 +178,14 @@ def with_controller_class(klass)
     # @param full_path [String] The path to set for the current request.
     # @param host [String] The host to set for the current request.
     # @param method [String] The request method to set for the current request.
-    def with_request_url(full_path, host: nil, method: nil)
+    def with_request_url(full_path, host: nil, method: nil, format: :html)
       old_request_host = vc_test_request.host
       old_request_method = vc_test_request.request_method
       old_request_path_info = vc_test_request.path_info
       old_request_path_parameters = vc_test_request.path_parameters
       old_request_query_parameters = vc_test_request.query_parameters
       old_request_query_string = vc_test_request.query_string
+      old_request_format = vc_test_request.format.symbol
       old_controller = defined?(@vc_test_controller) && @vc_test_controller
 
       path, query = full_path.split("?", 2)
@@ -197,6 +198,7 @@ def with_request_url(full_path, host: nil, method: nil)
       vc_test_request.set_header("action_dispatch.request.query_parameters",
         Rack::Utils.parse_nested_query(query).with_indifferent_access)
       vc_test_request.set_header(Rack::QUERY_STRING, query)
+      vc_test_request.format = format
       yield
     ensure
       vc_test_request.host = old_request_host
@@ -205,6 +207,7 @@ def with_request_url(full_path, host: nil, method: nil)
       vc_test_request.path_parameters = old_request_path_parameters
       vc_test_request.set_header("action_dispatch.request.query_parameters", old_request_query_parameters)
       vc_test_request.set_header(Rack::QUERY_STRING, old_request_query_string)
+      vc_test_request.format = old_request_format
       @vc_test_controller = old_controller
     end
 
diff --git a/test/sandbox/app/components/after_render_component.rb b/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
diff --git a/test/sandbox/app/components/content_predicate_component.rb b/test/sandbox/app/components/content_predicate_component.rb
@@ -5,7 +5,7 @@ def call
     if content?
       content
     else
-      "Default"
+      "Default".html_safe
     end
   end
 end
diff --git a/test/sandbox/app/components/custom_test_controller_component.rb b/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
diff --git a/test/sandbox/app/components/inherited_from_uncompilable_component.rb b/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
diff --git a/test/sandbox/app/components/inline_render_component.rb b/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
diff --git a/test/sandbox/app/components/message_component.rb b/test/sandbox/app/components/message_component.rb
@@ -6,6 +6,6 @@ def initialize(message:)
   end
 
   def call
-    @message
+    html_escape(@message)
   end
 end
diff --git a/test/sandbox/app/components/render_check_component.rb b/test/sandbox/app/components/render_check_component.rb
@@ -6,6 +6,6 @@ def render?
   end
 
   def call
-    "Rendered"
+    "Rendered".html_safe
   end
 end
diff --git a/test/sandbox/app/components/unsafe_component.rb b/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
diff --git a/test/sandbox/app/components/unsafe_postamble_component.rb b/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
diff --git a/test/sandbox/app/components/variant_ivar_component.rb b/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
diff --git a/test/sandbox/app/controllers/integration_examples_controller.rb b/test/sandbox/app/controllers/integration_examples_controller.rb
@@ -59,4 +59,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
diff --git a/test/sandbox/config/routes.rb b/test/sandbox/config/routes.rb
@@ -29,6 +29,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"
   post :create, to: "integration_examples#create"
 
   constraints(lambda { |request| request.env["warden"].authenticate! }) do
diff --git a/test/sandbox/test/collection_test.rb b/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
 
diff --git a/test/sandbox/test/integration_test.rb b/test/sandbox/test/integration_test.rb
@@ -723,4 +723,22 @@ def test_path_traversal_raises_error
       get "/_system_test_entrypoint?file=#{path}"
     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
diff --git a/test/sandbox/test/rendering_test.rb b/test/sandbox/test/rendering_test.rb
@@ -151,7 +151,9 @@ def test_renders_haml_template
   end
 
   def test_render_jbuilder_template
-    render_inline(JbuilderComponent.new(message: "bar")) { "foo" }
+    with_request_url("/", format: :json) do
+      render_inline(JbuilderComponent.new(message: "bar")) { "foo" }
+    end
 
     assert_text("foo")
     assert_text("bar")
@@ -1084,7 +1086,7 @@ def test_content_predicate_false
   end
 
   def test_content_predicate_true
-    render_inline(ContentPredicateComponent.new.with_content("foo"))
+    render_inline(ContentPredicateComponent.new.with_content("foo".html_safe))
 
     assert_text("foo")
   end
diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -179,3 +179,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

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+ruby_version: 2.5`
`1`	`2`	`require:`
`2`	`3`	`- standard`
`3`	`4`	`- "rubocop-md"`