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

fix css scrubbing tests #113

Merged
merged 3 commits into from
Apr 22, 2021
Merged

fix css scrubbing tests #113

merged 3 commits into from
Apr 22, 2021

Conversation

flavorjones
Copy link
Member

Closes #111.

Notably, this test:

def test_should_sanitize_div_background_image_unicode_encoded
  raw = %(background-image:\0075\0072\006C\0028'\006a\0061\0076\0061\0073\0063\0072\0069\0070\0074\003a\0061\006c\0065\0072\0074\0028.1027\0058.1053\0053\0027\0029'\0029)
  assert_equal '', sanitize_css(raw)
end

is incorrectly encoded and has been since 2007; and should never have passed with earlier versions of Loofah. The property value background-image:52C8'a161332904a1c5248.10278.1053379'9 is garbage, but it's not potentially harmful garbage that needs to be scrubbed.

This test was originally taken from OWASP's XSS cheat sheet, or a related source, and exists in that doc as:

<DIV STYLE="background-image:\0075\0072\006C\0028'\006a\0061\0076\0061\0073\0063\0072\0069\0070\0074\003a\0061\006c\0065\0072\0074\0028.1027\0058.1053\0053\0027\0029'\0029">

but you'll notice that \00XX isn't how Ruby encodes unicode strings. Ruby uses \uXXXX, so this test is an incorrect translation/encoding of the original intent.

In addition, that original source contains an additional error, which is that .1027 and .1053 should be \u0027 and \u0053. Both errors are corrected in this PR.

A short history of how encoding and typographical errors have propagated through this test in html5lib, loofah, rails, and rails-html-sanitizer is at flavorjones/loofah#205 for anyone who's interested.

which changed in Loofah v2.9.0

Related to #111
See flavorjones/loofah#205 for a short history
of this test string.

Related to #111
@flavorjones flavorjones mentioned this pull request Apr 8, 2021
See flavorjones/loofah#205 for a short history
of this test string.

Related to #111
Copy link

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw your comment on #112, I'm not too worried about which change is merged, but I had a question 😄

test/sanitizer_test.rb Show resolved Hide resolved
@flavorjones flavorjones merged commit 3b7551c into rails:master Apr 22, 2021
@flavorjones flavorjones deleted the flavorjones-fix-css-scrubbing-tests branch April 22, 2021 03:40
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 this pull request may close these issues.

test failures against current loofah/nokogiri
2 participants