Port helpers from hanami-helpers, introduce TagHelper

timriley commented 1 year ago

Introduce these helper modules ported from hanami-helpers:

EscapeHelper
HTMLHelper (may be removed, see below)
LinkToHelper
NumberFormattingHelper

Also introduce a new TagHelper, ported from ActionView::Helpers::TagHelper, with the intention for this to replace HTMLHelper (see below for more).

These helpers are included in hanami-view (as opposed to the hanami gem) because they're of general utility to anyone building views, and we should make them available to anyone using hanami-view standalone. Other helpers that depend on a fully integrated Hanami app will go into the hanami gem via another PR.

These helper modules are not loaded by default. They will be distributed as opt-in extras: they can be required explicitly and included in custom part or scope classes as needed. The spec/integration/helpers_spec.rb integration test demonstrates this approach.

I've sequenced the commits in this PR so that they can be reviewed in order. The first commit copies verbatim the files from @jodosha's original helpers modernisation PR (https://github.com/hanami/helpers/pull/199). From there, you can review the subsequent commits to see the meaningful changes, as opposed to reviewing the +2k new lines as one big blob.

Changes to helpers

The commits will show that I've tweaked the helpers in a few small ways to make them work here. The key changes are:

`module_function`

Wherever it is practical, I've adjusted every helper module to have its methods declared as module_function. This makes it possible to us the helpers directly (e.g. Hanami::View::Helpers::EscapeHelper.escape_html("...") without mixing them into your class, which would be preferable in certain situations.

`NumberFormattingHelper`

Remove dry-types dependency and instead use Ruby's Kernel coercion methods instead. These Kernel methods are what dry-types' Coercible::Integer and Coercible::Float types call anyway, so there's no need for the extra layer of code and extra gem dependency.

As a consequence, drop any custom error classes (because we don't need to catch and replace dry-types' errors), and instead just raise ArgumentError or TypeError in a way that is consistent with the underlying Kernel coercion methods.

`EscapeHelper`

Remove the need for a standalone Escape module by using module_function above all the methods in EscapeHelper.

Rename #escape_url into #sanitize_url, which better describes what it does.

Move the Escape.escape_uri method to being an escape_url helper method (wrapping the call to EscapeUtils.escape_uri).

Remove #escape_html_attribute. I’m not sure what value this brings over just calling #escape_html. It just seems confusing and there’s no equivalent in other Ruby web toolkits or frameworks.

`HTMLHelper`

I'm including notes of the changes to HTMLHelper for completeness, but in the section below, I propose we replace it with TagHelper.

Firstly, rename from HtmlHelper to HTMLHelper to properly reflect the acronym.

Inside HTMLBuilder, update our @buffer string handling to have it work properly with the frozen Hanami::View::HTML::SafeString instances returned by String#html_safe. So instead of creating a SafeString up-front, just create an ordinary mutable String and only return the SafeString via #to_s, which is called at the end of the HTML building.

`TagHelper`

This is a new helper module based on ActionView::Helpers::TagHelper.

My intention with this helper is to have it replace HTMLHelper, since TagHelper provides a much more template-friendly usage, wherein template content can be included in blocks. HTMLHelper, on the other hand, cannot be easily used within templates, since it runs any given blocks through instance_eval rather than expecting them to return a string from the template, which we're already establishing as a core expectation for hanami-view and its helpers.

To demonstrate this, here's what you have to do to use HTMLHelper inside a template:

<%= html.div do %>
  <%= html.p do %>
    <%= text("Hello world!") %>
  <% end %>
<% end %>

See how the text helper is needed to append content from within the block? This is not a natural expectation people would make about how to use these helpers from within templates. The inclination would be to do something like this:

<%= html.div do %>
  <%= html.p do %>
    Hello world!
  <% end %>
<% end %>

But this doesn't work at all: "Hello world!" is ignored and will not make it into the rendered template output, which would be just <div><p></p></div>.

However, with the tag helper, usage is more natural. This works as expected:

<%= tag.div do %>
  <%= tag.p do %>
    Hello world!
  <% end %>
<% end %>

Here the template renders <div><p>Hello world!</p></div> as expected.

What I think we've discovered here is an important principle for hanami-view helpers: they should be equally as functional and useful within templates as within plain Ruby environments.

TagHelper meets this principle, but HTMLHelper does not.

In addition, TagHelper comes with a range of other helpful behaviours, such as shortcuts for data: and aria: attributes, and other things like building class attribute strings from arrays as well as hashes of true/false values.

TagHelper is also a simpler, easier to understand helper: there's no instance_eval-style behaviour to worry about, which even tripped me up a number of times while trying to get HTMLHelper to work here. And given it already works with blocks providing content just like HTMLBuilder, the only difference is having to write tag. in front of each tag, which I think is a worthwhile tradeoff: tag is extra typing, yes, but it's both short and descriptive, and it makes it much clearer what is doing the tag generation.

I also ran into troubles with the HTMLBuilder used inside Hanami::Helpers::FormHelper (which I'm preparing in parallel in https://github.com/hanami/hanami/pull/1305).

IIRC, I think the problem (again) came from trying to use the form helper inside templates, e.g.

<%= form_for("/posts") do |f| %>
  <div>
    Title:
    <%= f.text_field "post.title" %>
  </div>
  <div>
    Body:
    <%= f.text_area "post.body" %>
  </div>
<% end %>

In this case the f.text_field call would output itself at the time of that ERB expression, but also at the end of the form_for call, because every method on f would also append extra contents to the buffer from the FormBuilder' own HTMLBuilder instance. So the tags inside the block (the text_field and text_area) would be output twice.

In this case, I sorted it out by avoiding use of the HTMLBuilder entirely, and just shelling out to the top-level html helper instead: https://github.com/hanami/hanami/pull/1305/commits/888d8e85b3e542761406524639ec8ea02afe3578. But either way, I think this illustrates how the HTMLHelper in its current structure is really suited to pure-ruby usage only, not usage within templates. This is potentially a remnant of the situation we had in Hanami 1.x, where we couldn't mix helpers with blocks and template content in the way we can today with hanami-view 2.0.

If you're happy with the switch to TagHelper, I'll make sure to remove HTMLHelper before merging this PR.

Mark strings from template-captured blocks as HTML safe

While preparing these helpers, notably the HTML/Tag helpers that will auto-escape strings returned from a yielded block based on their .html_safe? status, I realised that we needed blocks captured to strings via the templates automatically marked as .html_safe. After all, it's the entire purpose of our templates to generate HTML, so that HTML should be preserved and not escaped.

I've taken care of this in 80725926fa5327fd8b90ae946b14a1181c8ae7d6, via a little specialised buffer class that we use for Tilt's capture_generator across all three of our supported template engines. I'm very happy with how this part turned out!

Compatibility questions

I've a couple of compatibility-related questions at this point:

The API docs on these helpers say thing like @since 0.1.0. However, with this changed we've moved them into a new gem and into a new Ruby namespace. Should this change them to @since 2.0.0?
With the original #escape_url becoming #sanitize_url, and with Escape.escape_uri becoming #escape_url, this introduces a behavioural change for any existing user depending on the behaviour of the original #escape_url. Are we comfortable with this? Would release/upgrade notes be sufficient to notify users of this change? I do believe the new names are more self-descriptive, but I think we should be wary here. One option is to remove the #escape_url helper entirely (and keep #sanitize_url only). This would result in a NoMethodError for users, which would be an easy prompt to upgrade (or we could preserve #escape_url as an alias for #sanitize_url along with a deprecation message).

jodosha commented 1 year ago

@timriley I can't review ATM (traveling today), but escaping HTML attrs should be preserved as per OWASP recommendations: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html

timriley commented 1 year ago

HTML attrs should be preserved as per OWASP recommendations

@jodosha — safe travels, and thanks for the quick bit of feedback!

I definitely understand the need to ensure we escape HTML tag attributes. My question here was really about whether we needed a separate method for it when our EscapeHelper#escape_html will probably already do the job?

From looking at the code in your original PR, #escape_html and #escape_attribute had almost completely parallel implementations, which seemed to introduce a lot of complexity, and left me confused as to why we had to have them both.

To help with the discussion here, let me demonstrate all the various ways of escaping HTML attribute that are present either already or as a result of this PR:

require "bundler/setup"
require "hanami/view"
require "hanami/view/erb/template"
require "hanami/view/helpers/escape_helper"
require "hanami/view/helpers/tag_helper"

Scope = Class.new {
  include Hanami::View::Helpers::EscapeHelper
  include Hanami::View::Helpers::TagHelper
}.new

def render(src)
  Hanami::View::ERB::Template.new { src }.render(Scope)
end

# 1. Escaping of manually constructed attributes courtesy of ERB expression tags (similar for Haml/Slim)

puts render(<<~ERB)
  <div class="<%= "<script>" %>"></div>
ERB
# <div class="&lt;script&gt;"></div>

# 2. Escaping via new `tag.attributes` helper

puts render(<<~ERB)
  <div <%= tag.attributes(class: "<script>") %>></div>
ERB
# <div class="&lt;script&gt;"></div>

# 3. Escaping via attribute arguments supplied to `tag` builder

puts render(<<~ERB)
  <%= tag.div(class: "<script>") %>
ERB
# <div class="&lt;script&gt;"></div>

# 4. Escaping via manual use of `escape_html` (i.e. within other helper methods)

module MyHelper
  include Hanami::View::Helpers::EscapeHelper

  def my_helper
    # This is here as a contrived demonstration only. For real usage, you'd be much better off using
    # `tag.div` here.
    %(<div class="#{escape_html("<script>")}"></div>)
  end
end
puts Class.new { include MyHelper }.new.instance_eval { my_helper }
# <div class="&lt;script&gt;"></div>

Given these, is there a reason we would still need a separate #escape_attribute helper?

timriley commented 1 year ago

@jodosha:

Adding escape_utils as a runtime dependency made Hanami::Utils::Escape obsolete.

Please check if the existing specs in hanami-utils can expand the coverage for this PR, including the HTML attributes escaping.

👉 If the new implementation makes the old HTML attributes escaping specs to pass, then let's remove it.

Consider removing the code from hanami-utils.

I can confirm that all the tests from spec/unit/hanami/utils/escape_spec.rb in Hanami-utils are covered here in spec/unit/helpers/escape_helper/escape_html_spec.rb and spec/unit/helpers/escape_helper/sanitize_url_spec.rb.

I've just pushed up https://github.com/hanami/view/pull/229/commits/84adad9e915827f7c1d295dfb39c2000cd07a047 which (temporarily) adds comments to our specs to highlight the small number of differences.

I feel comfortable with the differences:

Most come down to us relying upon Temple::Utils.escape_html_safe instead of doing the manual character-by-character replacement approach that is present in hanami-utils' Escape.html method. I think our approach here is preferable because that makes the escaping consistent across both templates (which also use Temple's escaping) and helpers. The differences seem minor and no less safe.
At this point we're no longer forcing everything into UTF-8 encoding. Personally, this feels a little heavy-handed to me, and not really necessary for HTML escaping utilities? Releasing 2.0 with some updated helpers might be a good opportunity to jettison that extra complexity?

@jodosha — would you mind taking a look at that commit and my notes above and letting me know what you think?

In the meantime, I'll start a PR to remove Escape from hanami-utils.

timriley commented 1 year ago

In 2.x, hanami-assets must depend on hanami-view. Are we OK with that? I am, just wanted you to know.

Yep, I'm good with this!

If we discover later that this dependency is somehow problematic, we can always adjust things then, but I like the idea of our view-related things living together in our view gem.

timriley commented 1 year ago

@jodosha I've finished with all my polishing here now. This is ready to go pending your final feedback on the couple of open threads above 😄

timriley commented 1 year ago

Just one note regarding EscapeUtils: in https://github.com/hanami/view/pull/229/commits/820e997d8e7c498d456874c8160ba016224d75ce I actually removed it, and the EscapeHelper#escape_url method that used it.

As far as I can tell, EscapeUtils dependency was actually introduced in https://github.com/hanami/helpers/pull/199, so it was never actually shipped in any released Hanami gem. And the one method that used it in that PR (Hanami::Helpers::Escape.uri) was only used in one place by the HtmlHelper implementation that we have since removed from this PR.

Given this, I didn't think we should keep it.

Users who want this behaviour in their apps can include EscapeUtils themselves, or reach for standard library alternatives like ERB::Util.url_encode.

Importantly, by removing the #escape_url method that briefly existed in this PR, it also means that any Hanami 1.x users upgrading will see a NoMethodError for escape_url (whose 1.x implementation was renamed to #sanitize_url in this PR) instead of finding a more permissive method in its place, which would be dangerous.

timriley commented 1 year ago

@jodosha Addressed those last two open issues, thanks for those. Would you mind weighing in on my remaining comments so we can get to the point of merging this? 😄

hanami / view