Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Whitespace Added around "/" in CSS #271

Closed
davidjstein opened this issue Sep 30, 2023 · 3 comments · Fixed by #273
Closed

Whitespace Added around "/" in CSS #271

davidjstein opened this issue Sep 30, 2023 · 3 comments · Fixed by #273

Comments

@davidjstein
Copy link

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
Copy link
Owner

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

flavorjones added a commit that referenced this issue Oct 10, 2023
Keep whitespace if it is present, but do not insert any new whitespace
that isn't there.

Fixes #271
@flavorjones
Copy link
Owner

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 #273 which I think is what we should have been doing all along.

flavorjones added a commit that referenced this issue Oct 10, 2023
Keep whitespace if it is present, but do not insert any new whitespace
that isn't there.

Fixes #271
@flavorjones
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants