Update Rails/OutputSafety to disallow wrapping safe_join

[klesse413]

At Betterment, we built custom cops to prevent usage of raw and html_safe without review, but we'd love to get on the mainline version now that there is a built-in Rails/OutputSafety cop. However, we can't switch unless this cop can provide the guarantees that we need. Hopefully our learnings are valuable to the community.

Proposed Change

I would like to propose changing the behavior of the Rails/OutputSafety cop. The change is to no longer allow usage of raw and html_safe when they are wrapped in safe_join. Instead, raw and html_safe should only be used when the cop is explicitly disabled. This change essentially reverts the Rails/OutputSafety cop back to its original functionality.

Background

When it was first introduced, the Rails/OutputSafety cop only allowed the usage of raw or html_safe if the cop was explicitly disabled.

For reference, here is a link to that original PR: #3135 In that PR, @josh explained:

In instances .html_safe were is necessary, we use an inline # rubocop:disable Rails/OutputSafety comment after security review.

A few months after the introduction of this cop, an issue was opened here: #3676 which was addressed by this PR: #3682. The change is to allow usage of raw or html_safe when the usages are wrapped in safe_join.

Why

In summary, safe_join is not a declaration about the safety of the buffers it joins, and it shouldn't be used by rubocop to impute that.

I'm proposing this change because usage of safe_join to combine buffers that have already been declared "safe" by using raw or html_safe to return a SafeBuffer does not add any additional "safety" to the buffer. Calling raw or html_safe essentially blesses a buffer as safe by returning a SafeBuffer containing the content, like so:

[1] pry(main)> include ActionView::Helpers::OutputSafetyHelper
=> Object
[2] pry(main)> result = "<p>hi</p>".html_safe
=> "<p>hi</p>"
[3] pry(main)> result.class
=> ActiveSupport::SafeBuffer
[4] pry(main)> result = raw("<p>hi</p>")
=> "<p>hi</p>"
[5] pry(main)> result.class
=> ActiveSupport::SafeBuffer

Rather than bless content as already "safe", safe_join actually escapes the content, like so:

[6] pry(main)> safe_join(["<p>hi</p>", "<p>bye</p>"])
=> "&lt;p&gt;hi&lt;/p&gt;&lt;p&gt;bye&lt;/p&gt;"

However, when passed content that has already been blessed as safe (because it's a SafeBuffer), safe_join does not escape the content, like so:

[7] pry(main)> safe_join([raw("<p>hi</p>"), "<p>bye</p>".html_safe])
=> "<p>hi</p><p>bye</p>"

Therefore, if the Rails/OutputSafety cop tells us that we should not use raw or html_safe without explicitly disabling it, we should still hold to the standard of needing to explicitly disable it when usages are wrapped in safe_join, since safe_join does not provide any additional safety.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Used the same coding conventions as the rest of the project.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• All tests are passing.
• The new code doesn't generate RuboCop offenses.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

[josh]

Yikes, good catch @klesse413!

[bbatsov]

Before merging this I'd like @monfresh and @b-t-g to also chime in, so we're all on the same page in the end.

[b-t-g]

I'm similarly convinced by @klesse413's great write-up that this pull request is a good idea. However, the error message hasn't been updated in this PR (which explicitly states it allows safe_join), which would be confusing. Either the line about safe_join should be removed or, even better, detect safe_join and output a different error message declaring that safe_join is, in fact, not safe.

[bbatsov]

detect safe_join and output a different error message declaring that safe_join is, in fact, not safe.

That would be better IMO.

[josh]

declaring that safe_join is, in fact, not safe.

I don't really understand what you mean by "safe_join not being safe". There is nothing about safe_join itself that can lead to an escaping issue. Any incorrect usage of html_safe can lead to a potential security issue.

[b-t-g]

@josh My apologies, sloppy wording on my part (this is what I get for making a comment before finishing my morning coffee :) ). I was referring to the last sentence in @klesse413's PR, namely the line "since safe_join does not provide any additional safety"

[josh]

I was referring to the last sentence in @klesse413's PR, namely the line "since safe_join does not provide any additional safety"

Ah, gotcha.

Maybe we should just change the copy to not include any "quick fix" suggestions as its a complicated issue. We can just leave it as "Tagging a string as html safe may be a security risk" (or similar).

Then improve the styleguide reference for this cop as we can include much more context and examples there about how best to resolve issues where other safer helpers could be used in place of html_safe.

An example with safe_join could be

# bad
( person.login + " " + content_tag(:span, person.email) ).html_safe

# good
safe_join([person.login, " ", content_tag(:span, person.email)])

[klesse413]

I agree with that idea @josh - I'll update this PR 👍

[klesse413]

updated!

[bbatsov]

You'll also have to update the cop documentation and examples. The rationale for the cop can certainly be expanded a bit.

[klesse413]

Ah sorry about that - updated the documentation with a bit of an explanation and added the new example

[bbatsov]

Please remove the spaces inside the outer parens.

[bbatsov]

This needs a changelog entry and it's good to go. :-)

[klesse413]

Awesome, thanks! Should be good now 👍

[bbatsov]

👍

[monfresh]

@bbatsov thanks for pinging me on this PR. I found @klesse413's argument convincing as well, and I give a 👍 to this PR.