Sanitize texts instead of using html_safe #3747

javierm · 2019-10-05T16:47:19Z

Objectives

Make it easier to understand when a text is considered safe
Add rubocop and erb-lint rules so we don't use html_safe in the future

houndci-bot

Some files could not be reviewed due to errors:

warning: parser/current is loading parser/ruby26, which recognizes

warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 13 linters...

erb statement not allowed here; did you mean '<%=' ?
In file: app/views/layouts/application.html.erb:25

houndci-bot · 2019-10-05T16:47:42Z

lib/tasks/proposals.rake

@@ -12,7 +12,7 @@ namespace :proposals do
      model.find_each do |resource|
        if resource.external_url.present?
          Globalize.with_locale(I18n.default_locale) do
-            new_description = "#{resource.description} <p>#{text_with_links(resource.external_url)}</p>"
+            new_description = "#{resource.description} <p>#{sanitize_and_auto_link(resource.external_url)}</p>"


Metrics/LineLength: Line is too long. [111/110] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)

app/views/layouts/_footer.html.erb

houndci-bot · 2019-10-08T11:22:34Z

lib/tasks/proposals.rake

@@ -12,7 +12,7 @@ namespace :proposals do
      model.find_each do |resource|
        if resource.external_url.present?
          Globalize.with_locale(I18n.default_locale) do
-            new_description = "#{resource.description} <p>#{text_with_links(resource.external_url)}</p>"
+            new_description = "#{resource.description} <p>#{sanitize_and_auto_link(resource.external_url)}</p>"


Metrics/LineLength: Line is too long. [111/110] (https://rubystyle.guide#80-character-limits)

The name `html_safe` is very confusing, and many developers (including me a few years ago) think what that method does is convert the HTML contents to safe content. It's actually quite the opposite: it marks the string as safe, so the HTML inside it isn't stripped out by Rails. In some cases we were marking strings as safe because we wanted to add some HTML. However, it meant the whole string was considered safe, and not just the contents which were under our control. In particular, some translations added by admins to the database or through crowding were marked as safe, when it wasn't necessarily the case. Although AFAIK crowdin checks for potential cross-site scripting attacks, it's a good practice to sanitize parts of a string potentially out of our control before marking the string as HTML safe.

This was causing erb-lint to issue a warning.

Using `html_safe` on the whole text meant the translations were also considered HTML safe, but they are not supposed to have HTML.

We need to update other gems as well if we update this one. Dependabot updated it automatically when updating `foundation_rails_helper`, but it doesn't seem to be necessary.

It's possible to create a newsletter or a proposed action with <script> tags by filling in the body using a textarea instead of a CKEditor. While we trust our administrators not to do so, it's better to completely eliminate that possibility.

There's a case where we would face a Cross-Site Scripting attack. An attacker could use the browser's developer tools to add (on their browser) a `<code>` tag with a `<script>` tag inside in the text of the draft version. After doing so, commenting on that text would result in the attacker's JavaScript being executed.

If we don't sanitize them, valuators might attempt Cross-Site Scripting attacks.

The name `safe_html_with_links` was confusing and could make you think it takes care of making the HTML safe. So I've renamed it in a way that makes it a bit more intuitive that it expects its input to be already sanitized. I've changed `text_with_links` as well so now the two method names complement each other.

We were confused about what `.html_safe` did, and were automatically marking as safe content which was not.

We were using `Rinku.auto_link` the same way twice. And it makes sense that the method `sanitize_and_auto_link` first sanitizes the text and then calls `auto_link_already_sanitized_text`.

This way we can remove all those `html_safe` calls and we avoid potential XSS attacks in label texts.

There's a slight chance an attribute like an author's name might contain an attempt to perform XSS attacks. So, instead of marking the whole text as HTML safe, we can sanitize it. Also note I'm removing the `_html` suffix in the i18n key, since it's got the same effect as using `html_safe`.

Sometimes we're interpolating a link inside a translation, and marking the whole translations as HTML safe. However, some translations added by admins to the database or through crowdin are not entirely under our control. Although AFAIK crowdin checks for potential cross-site scripting attacks, it's a good practice to sanitize parts of a string potentially out of our control before marking the string as HTML safe.

The difference is `html_safe` allows every HTML tag, including the `<script>` tag, while `sanitize` only allows tags which are considered safe. In this case, we want to allow a `<span>` tag in a translation, and links inside flash messages.

We were using the markdown renderer with the `filter_html` option set to false, so we weren't removing hypothetical `<script>` tags.

They do the exact same thing; however `html_safe` might confuse developers into thinking it will make the HTML safe. Using `raw` makes it clear that we're inserting the text without escaping it.

This way we make sure we won't add `html_safe` or `raw` calls in the future. I'm excluding `text_with_links_helpers` for this check, because in this situation the use of `html_safe` is justified: we check the original input is safe, and we're only adding link tags to raw URLs.

Using `<%==` is the same as using `raw`. I'm not sure if we meant `sanitize` in this case, or it's just a typo. I'm assuming the latter since we don't use anything similar in any other places.

Using `<%==` is the same as using `raw`, and here we only want to mark as safe a `<br>` tag.

We were using `<%==`, which is the same as using `raw`. Note ERB Lint doesn't warn us of this usage. Brakeman does warn us, though.

app/views/layouts/application.html.erb

houndci-bot · 2019-10-08T17:12:19Z

app/views/layouts/application.html.erb

@@ -11,35 +11,35 @@
                     type: "image/png" %>
    <%= content_for :social_media_meta_tags %>

-    <%= setting["html.per_page_code_head"].try(:html_safe) %>
+    <%= raw setting["html.per_page_code_head"] %>
