Add output_preamble (to match existing output_postamble)

[kdonovan]

What are you trying to accomplish?

Add output_preamble to match existing output_postamble, as suggested in this issue.

What approach did you choose and why?

To keep this PR as simple as possible I've just mirrored the existing output_postamble code.

Anything you want to highlight for special attention from reviewers?

If this gets merged, we should also update this performance PR to address the preamble case (I didn't include it here as that PR hasn't yet been approved and wasn't sure if the team wanted to take that approach).

[kdonovan]

OK, pushed an array-joining approach that fixes the FrozenError... I'm now re-reading @camertron's original message, though, and it turns out just (output_preamble + render_template_for(@__vc_variant).to_s + output_postamble).html_safe also passes tests locally.

I'm wondering if I'm missing some context where .html_safe isn't a valid solution, though..

[camertron]

@kdonovan hmm yeah I'm not sure where the FrozenError is coming from either, but I imagine there are scenarios in our tests where a template returns a single literal string (Rails ERB freezes literal strings by default).

When I was first looking into this, I didn't try (a + b + c).html_safe, but if that works then let's go with that. It's one less object allocation than the array-based approach.

[camertron]

Weird, I thought we fixed that flake in the PVC tests 🤔

[BlakeWilliams]

Is this safe to call? Do we need to check if render_template_for(@__vc_variant).to_s is html_safe first?

[kathleenteamshares]

I think it is okay as it... @camertron do you have a strong opinion here?

[BlakeWilliams]

I'm a little unsure, I wrote a component like:

# frozen_string_literal: true

class UnsafeComponent < ViewComponent::Base
  def call
    user_input = "<script>alert('hello!')</script>"

    "<div>hello #{user_input}</div>"
  end
end

Which is marked as html_safe # => false:

    107:         x = render_template_for(@__vc_variant).to_s + output_postamble
 => 108:         require 'pry'; binding.pry if $debug
    109:         x
    110:       else
    111:         ""
    112:       end
    113:     ensure

[1] pry(#<UnsafeComponent>)> x
=> "<div>hello <script>alert('hello!')</script></div>"
[2] pry(#<UnsafeComponent>)> x.html_safe?
=> false

Using that same component with this change:

    108:           output_preamble +
    109:           render_template_for(@__vc_variant).to_s +
    110:           output_postamble
    111:         ).html_safe
    112:
 => 113:         require 'pry'; binding.pry if $debug
    114:         x
    115:       else
    116:         ""
    117:       end
    118:     ensure

[1] pry(#<UnsafeComponent>)> x
=> "<div>hello <script>alert('hello!')</script></div>"
[2] pry(#<UnsafeComponent>)> x.html_safe?
=> true

I started writing a test for it but ran into some unexpected behavior, but we should be extra sure about the behavior of this when using html_safe either way.

[BlakeWilliams]

Here's a diff you can apply for a failing test (passes on main):

diff --git a/test/sandbox/app/components/unsafe_component.rb b/test/sandbox/app/components/unsafe_component.rb
new file mode 100644
index 00000000..df09ff1d
--- /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 = "<script>alert('hello!')</script>"
+
+    "<div>hello #{user_input}</div>"
+  end
+end
diff --git a/test/sandbox/app/components/unsafe_wrapper_component.html.erb b/test/sandbox/app/components/unsafe_wrapper_component.html.erb
new file mode 100644
index 00000000..3c25ab31
--- /dev/null
+++ b/test/sandbox/app/components/unsafe_wrapper_component.html.erb
@@ -0,0 +1 @@
+<%= render UnsafeComponent.new %>
diff --git a/test/sandbox/app/components/unsafe_wrapper_component.rb b/test/sandbox/app/components/unsafe_wrapper_component.rb
new file mode 100644
index 00000000..2dd93a17
--- /dev/null
+++ b/test/sandbox/app/components/unsafe_wrapper_component.rb
@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class UnsafeWrapperComponent < ViewComponent::Base
+end
diff --git a/test/sandbox/test/rendering_test.rb b/test/sandbox/test/rendering_test.rb
index 796be4e5..abe78067 100644
--- a/test/sandbox/test/rendering_test.rb
+++ b/test/sandbox/test/rendering_test.rb
@@ -1077,6 +1077,12 @@ class RenderingTest < ViewComponent::TestCase
     end
   end
 
+  def test_inline_safe
+    render_inline(UnsafeWrapperComponent.new)
+
+    refute_selector("script", visible: :hidden)
+  end
+
   def test_content_predicate_false
     render_inline(ContentPredicateComponent.new)

I tried this without the wrapper component and it always failed, which is concerning and likely needs to be addressed, but that's not relevant to this PR.

[camertron]

With @BlakeWilliams help I was able to determine that any component that defines a #call method can render unescaped content and return it to the browser 😱 In other words #render_template_for can return an HTML-unsafe string, as can #output_postamble and #output_preamble. I'm working on a PR using @BlakeWilliams' diff above that automatically escapes the return value from #render_template_for. I think we can use a similar approach to safely concatenate the return value of #output_postamble. I'll comment again on this PR once the HTML safety fixes have been landed, and we can talk about applying the same techniques to #output_preamble.

It's worth noting that, in most cases, unsafe strings returned from #call methods aren't dangerous because they are automatically escaped when rendered inside another component or view. Things get dicey though when the component is rendered directly by a controller, eg:

class MyController < ApplicationController
  def index
    render(UnsafeComponent.new)
  end
end

[camertron]

Ok, sorry for the churn here @kdonovan and @kathleenteamshares 😬 I just landed the change for ensuring HTML safety and released it as part of v3.9.0. Please rebase or merge main into this branch and take a look at ViewComponent::Base#safe_output_postamble. You should be able to copy/paste and tweak it for the output preamble 👍

[camertron]

Closing in favor of #1960