Fix regression in XSS filtering of HTML characters

[amartinfraguas]

A previous fix of mine for protections for XSS in ActionView::Helpers and ERB::Util introduced a regression by not filtering HTML characters properly. This is a complete fix for that regression, related to #45027 , which was incomplete.

We would need to support XHTML, HTML4 and HTML5. But since XHTML and HTML4 have had a note for future deprecation in the documentation for more than 5 years ( https://github.com/rails/rails/blame/main/actionview/lib/action_view/helpers/tag_helper.rb#L263 ), these changes simplify the filtering by removing support for XHTML and HTML4.

Summary

The regression was caused by filtering according to the XML standard. Instead of that, there are now two different sets of characters for the filtering: one for the names of HTML tags and one for the names of attributes in HTML tags.

We could deprecate the legacy syntax fully, but that may add too many changes to this pull request. Still, I have updated many internal uses of the legacy syntax, especially in the tests.

Other Information

This is a change in the API, so the minor version of Rails should be increased. Since browsers may not support the HTML standard completely, it may be a good idea to have these changes merged in main for a while before releasing them. That way, there will be some time for possible regressions to be noticed and fixed.

These changes probably won't have a big impact on security. However, they seem like an improvement by following the HTML standard closely and reducing the complexity of supporting old standards. The part that looks more risky are the changes in the form tag and the fieldset tag, since there is manual string concatenation, please review those changes carefully.

[eileencodes]

Thanks for the PR @amartinfraguas. It looks like ActionText has related failures, can you take a look?

This is a change in the API, so the minor version of Rails should be increased

Rails doesn't follow semver, but I'm not sure which version this would be released in until I better understand the impact to existing applications.

Since browsers may not support the HTML standard completely, it may be a good idea to have these changes merged in main for a while before releasing them.

Is this change a feature or a bugfix? It sounds like this isn't a change that fixes the security fix we merged from you a few weeks ago. If it's a feature it will only go to main and be released in 7.1. If it's a bugfix for the security fix you sent and followup changes I made, we might release it in all of the last bugfix versions, however 5.2 is now EOL as of today. I would consider doing one more release of 5.2 if the current state was severely broken but I would rather apps upgrade.

It sounds like this PR is trying to get closer to the standard and doesn't necessarily need to be backported to the prior released versions. Can you help me better understand the impact to existing Rails applications that may be on 5.2, 6.0, 6.1, and 7.0? Thanks!

[amartinfraguas]

Thank you for the quick response, @eileencodes :-) I have fixed the test failure in ActionText, there is now a small change in actiontext/app/helpers/action_text/tag_helper.rb

Is this change a feature or a bugfix?

The change is a bugfix. Without it, we are filtering the wrong characters and that breaks the applications that use them. The security issue was probably fixed with the initial release as it included commonly used characters in XSS attacks, though I am not sure if it was a complete fix. In that regard, the changes in this pull request are more complete.

It sounds like this PR is trying to get closer to the standard and doesn't necessarily need to be backported to the prior released versions. Can you help me better understand the impact to existing Rails applications that may be on 5.2, 6.0, 6.1, and 7.0?

Backporting this PR would mean disabling HTML4 and XHTML support. The deprecation note in the documentation was very old and maybe few applications still use those standards, though maybe it would be a bit surprising for Rails users to apply this change to older versions. Another option would be to keep supporting those standards with separate new filtering functionality, which would increase the complexity (especially considering there are several versions of XHTML) and may take a long time to develop.

The summary is that I think it is a choice between security and stability:

• Rails versions that had the previous fix applied gained some level of security with it. Backporting this PR may help security further but disable support for old standards that was going to be deprecated.

With all of this in mind, I am more inclined to backport this (maybe not to 5.2) after having it in the main branch for a while so that possible issues come up and are fixed. But your judgement is probably better than mine, what do you think? My background is that I have worked as a Rails application developer for 10 years followed by 2 years in security.

[amartinfraguas]

My last push only changes my regexp in actiontext/app/helpers/action_text/tag_helper.rb, as I have just noticed a mistake that was still passing the DOM-equal assertion in the tests.

[amartinfraguas]

Hi, @eileencodes . I had not noticed that a couple of tests were failing in the railties and I have fixed them. However, now there are some failed tests in many different Rails components in buildkite that pass successfully in my environment. Could you check what may be happening? Maybe they are random failures? Thank you 🙂

[amartinfraguas]

Hi, @eileencodes . I have rebased on main and I have made a small change and now the tests pass, so my changes seem to be OK. The previous failures may be related to #45370 (see https://buildkite.com/rails/rails/builds/87312 ).