HTML5 empty attributes are being scrubbed

[dometto]

a6922ce seems to have had the effect of scrubbing valueless attributes even if they are in SafeList. For example, even after adding controls to the SafeList with:

Loofah::HTML5::SafeList::ALLOWED_ATTRIBUTES.add('controls')

The controls attribute will be removed from video tags like the following:

<video ... controls>...</video>
<video ... controls="">...</video>

But not from the following:

<video ... controls="true">...</video>

However, just having controls without a value seems to be valid HTML5.

Is this intended behaviour or a bug?

[dometto]

Ah, it seems that the commit I referenced didn't introduce this behavior, but just provided an exception for data- attributes. I suppose my question is whether it makes sense to remove empty-value attributes at all?

[flavorjones]

Hi! Thanks for opening this issue. Let me try to summarize what you're seeing?

#! /usr/bin/env ruby

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "loofah"
end

Loofah.fragment('<video controls></video>').scrub!(:strip).to_s
# => "<video></video>"

Loofah.fragment('<video controls=""></video>').scrub!(:strip).to_s
# => "<video></video>"

Loofah.fragment('<video controls="true"></video>').scrub!(:strip).to_s
# => "<video></video>"

Loofah::HTML5::SafeList::ALLOWED_ATTRIBUTES.add('controls')

Loofah.fragment('<video controls></video>').scrub!(:strip).to_s
# => "<video></video>"

Loofah.fragment('<video controls=""></video>').scrub!(:strip).to_s
# => "<video></video>"

Loofah.fragment('<video controls="true"></video>').scrub!(:strip).to_s
# => "<video controls=\"true\"></video>"

but we'd expect all three of the cases to retain the controls attribute once it's added to the allowed attributes?

I'll investigate!

[flavorjones]

This behavior dates back to #51 which is a requirement and contribution that came from the Rails team.

I'd like to explore removing this behavior, but I'm hesitant to do so without fully understanding the reasons this behavior was introduced in the first place. Additionally there are two larger changes we're trying make short-term:

• [draft] default to html5 parsing #239 to have Loofah default to using Nokogiri's HTML5 parser (currently it uses the HTML4 parser) which will affect the behavior of some HTML5 attributes
• experiment: use loofah attribute scrubber to explore functional drift rails/rails-html-sanitizer#136 which will end in cleanly decoupling the Rails-specific sanitization behavior from Loofah's core functionality

Once both of those have landed, hopefully in the next month or two, then I'll feel more confident about making a change like this. So I'm going to park this for now and come back to it.

I hope that all makes sense! Feel free to ask questions, I may have missed something.

[dometto]

Thanks for the quick and clear reaction @flavorjones! The summary you give is indeed accurate, and the approach seems very reasonable. For now we'll just add the ="true" to the attribute in gollum as a quick fix, and I'll monitor this issue to see when it can be removed.

[flavorjones]

Just updating this with a note: Loofah now supports HTML5 parsing, along with Rails 7.1.

I still want to finish the work in #136 to decouple rails-html-sanitizer from Loofah before tackling this, did want to let you know that it's still on my to-do list.