search

flavorjones / loofah

Ruby library for HTML/XML transformation and sanitization
MIT License
934 stars 137 forks source link

Whitespace Added around "/" in CSS #271

Closed davidjstein closed 1 year ago

davidjstein commented 1 year ago

I am running loofah 2.21.3 (main branch) in a rails app and get the following at the rails console: [1] pry(main)> Loofah::HTML5::Scrub.scrub_css("font:13px/1.5 Arial, sans-serif") => "font:13px / 1.5 Arial , sans-serif;"

The added space around the "/" in scrubbed documents seems to cause issues with the browser in at least one version of Outlook desktop. From what I can gather, the space before the slash is not valid CSS syntax. It seems that other browsers are more forgiving, but we have customers on Outlook who are complaining.

As an experiment, I tried changing line 114 in lib/loofah/html5/scrub.rb to the following: propstring = format("%s:%s", name, value.join(" ")).gsub(/ \/ /, '/')

When I did that, I got the following results at the rails console: [1] pry(main)> Loofah::HTML5::Scrub.scrub_css("font:13px/1.5 Arial, sans-serif") => "font:13px/1.5 Arial , sans-serif;"

Would you consider this a bug, and if so, would you like me to submit a pull request?

flavorjones commented 1 year ago

@davidjstein Thanks for opening this issue! Let me take a look at what's going on.

flavorjones commented 1 year ago

OK, this was definitely a bug and a misunderstanding on my part about how some CSS properties are written.

cf https://developer.mozilla.org/en-US/docs/Web/CSS/font which says

line-height must immediately follow font-size, preceded by "/", like this: "16px/3"

I've drafted https://github.com/flavorjones/loofah/pull/273 which I think is what we should have been doing all along.

flavorjones commented 1 year ago

Fixed in Release 2.21.4 / 2023-10-10 · flavorjones/loofah