diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 81048b707..eae0b007f 100644 --- a/docs/CHANGELOG.md +++ b/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)" diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index c1e73001f..6050b9f20 100644 --- a/lib/view_component/base.rb +++ b/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 diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 9b2ba4b7d..6560e8b0b 100644 --- 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? @@ -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 diff --git a/test/sandbox/app/components/after_render_component.rb b/test/sandbox/app/components/after_render_component.rb index 86f57d632..86117cb0b 100644 --- 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/custom_test_controller_component.rb b/test/sandbox/app/components/custom_test_controller_component.rb index 9df474e36..1e66a559d 100644 --- 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/deprecated_slots_setter_component.rb b/test/sandbox/app/components/deprecated_slots_setter_component.rb index 769b0dec8..eb4ad60e9 100644 --- a/test/sandbox/app/components/deprecated_slots_setter_component.rb +++ b/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 diff --git a/test/sandbox/app/components/inherited_from_uncompilable_component.rb b/test/sandbox/app/components/inherited_from_uncompilable_component.rb index 1cf97d1f6..271d34456 100644 --- 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 - "
hello world
" + "
hello world
".html_safe end end diff --git a/test/sandbox/app/components/inline_render_component.rb b/test/sandbox/app/components/inline_render_component.rb index 4b6934e4e..bf25e364c 100644 --- 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 index 93c0cdb40..28ad85d00 100644 --- 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/unsafe_component.rb b/test/sandbox/app/components/unsafe_component.rb new file mode 100644 index 000000000..df09ff1d9 --- /dev/null +++ b/test/sandbox/app/components/unsafe_component.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class UnsafeComponent < ViewComponent::Base + def call + user_input = "" + + "
hello #{user_input}
" + end +end diff --git a/test/sandbox/app/components/unsafe_postamble_component.rb b/test/sandbox/app/components/unsafe_postamble_component.rb new file mode 100644 index 000000000..522b1489b --- /dev/null +++ b/test/sandbox/app/components/unsafe_postamble_component.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class UnsafePostambleComponent < ViewComponent::Base + def call + "
some content
".html_safe + end + + def output_postamble + "" + end +end diff --git a/test/sandbox/app/components/variant_ivar_component.rb b/test/sandbox/app/components/variant_ivar_component.rb index 386ca917e..4d1cac5f2 100644 --- 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 index f8b2e97bb..568817eec 100644 --- a/test/sandbox/app/controllers/integration_examples_controller.rb +++ b/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 diff --git a/test/sandbox/config/routes.rb b/test/sandbox/config/routes.rb index 06b39bd52..d87b134d6 100644 --- a/test/sandbox/config/routes.rb +++ b/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" diff --git a/test/sandbox/test/collection_test.rb b/test/sandbox/test/collection_test.rb index 42742b2de..bad2b0aa2 100644 --- a/test/sandbox/test/collection_test.rb +++ b/test/sandbox/test/collection_test.rb @@ -12,7 +12,7 @@ def initialize(**attributes) end def call - "

#{product.name}

" + "

#{product.name}

".html_safe end end diff --git a/test/sandbox/test/integration_test.rb b/test/sandbox/test/integration_test.rb index 27a42b423..a3bb5ef88 100644 --- a/test/sandbox/test/integration_test.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index 6ab5346db..813e2e9ce 100644 --- a/test/test_helper.rb +++ b/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