Loofah-integration #11218
Loofah-integration #11218
Conversation
ghost
assigned 
| end | ||
|
|
||
| def protocol_separator | ||
| ActiveSupport::Deprecation.warn('protocol_separator has been deprecated and has no effect.') |
There was a problem hiding this comment.
We can call self.class.protocol_separator here and in all the deprecated instance methods to avoid duplication.
There was a problem hiding this comment.
Ahh, of course!
Den 01/07/2013 kl. 18.25 skrev Rafael Mendonça França notifications@github.com:
In actionview/lib/action_view/helpers/sanitize_helper/sanitizers.rb:
- def sanitize(html, options = {})
return nil unless htmlvalidate_options(options)
loofah_fragment = Loofah.fragment(html)loofah_fragment.scrub!(:strip)loofah_fragment.xpath("./form").each { |form| form.remove }loofah_fragment.to_s- end
- def sanitize_css(style_string)
Loofah::HTML5::Scrub.scrub_css style_string- end
- def protocol_separator
ActiveSupport::Deprecation.warn('protocol_separator has been deprecated and has no effect.')—
Reply to this email directly or view it on GitHub.
|
cc @carlosantoniodasilva @josevalim @jeremy @NZKoz @tenderlove mind to review this one? I think we are done |
| @@ -65,5 +65,9 @@ | |||
| * Fix removing trailing slash for mounted apps #3215 | |||
|
|
|||
| *Piotr Sarnacki* | |||
|
|
|||
| * Loofah replaces html-scanner in dom assertions | |||
There was a problem hiding this comment.
New entries must be on the top.
|
@guilleiguaran I'll second that. |
|
Let's move selector.rb and tag.rb also to actionview 😄 |
|
Fine by me. |
| end | ||
| var | ||
| end | ||
| end |
There was a problem hiding this comment.
Are these no newlines generally allowed?
There was a problem hiding this comment.
Generally, there is a newline at the end of files. This is the only one missing a newline. Mostly a nitpick...
There was a problem hiding this comment.
Ah, I see. I wasn't aware of that. I'll look through my files and add newlines where needed.
|
The guides say that Should I remove the file assertions/tag.rb, @rafaelfranca? |
| @@ -2,6 +2,9 @@ source 'https://rubygems.org' | |||
|
|
|||
| gemspec | |||
|
|
|||
| # temporary gem while working on loofah integration | |||
| gem 'loofah', '~> 1.2.1', github: 'kaspth/loofah' | |||
There was a problem hiding this comment.
Why it's pointing to your fork of loofah? Did you made any changes? Are they already merged to official master branch?
There was a problem hiding this comment.
I think there are changes essential for this to work if I am not wrong.
There was a problem hiding this comment.
Yep. It would be good to merge those changes to official loofah repository before merging this PR.
There was a problem hiding this comment.
There are a lot of changes that are yet to be merged, which have not received response yet from @flavorjones .
After he does that , it should be fine to change this.
There was a problem hiding this comment.
Yes, these PR are pending, but the idea is remove this git dependency.
There was a problem hiding this comment.
@strzalek Yes, I have made lots of changes. You can check out the PRs in the description if you're interested 😉
@vipulnsward is right, we're still waiting for Mike to pull in the changes.
Once Loofah includes them, this line will be removed.
(that's the reason for the 'temporary' comment, I see now that that wasn't clear why the gem was temporary.)
There was a problem hiding this comment.
@kaspth you can now probably change this. Thanks to @flavorjones !
There was a problem hiding this comment.
Yes, I can. Much love to @flavorjones! Thank you kindly, Mike.
|
@kaspth yes, lets remove it |
|
@rafaelfranca Yippee! |
|
@strzalek is there anything I can do to make it easier for you to move dom.rb and selector.rb in action_dispatch/testing/assertions/ into actionview while I'm changing the files, anyway? |
|
I'm actually not touching those files at all so feel free to do whatever you want with them. They unlikely be conflicted with my changes. No worries. |
|
Then it's all good then. |
|
Please take in account this: index 3b52b20..6ff7b68 100644
--- a/actionview/test/template/sanitize_helper_test.rb
+++ b/actionview/test/template/sanitize_helper_test.rb
@@ -38,6 +38,9 @@ class SanitizeHelperTest < ActionView::TestCase
assert_equal("<<<bad html", strip_tags("<<<bad html"))
assert_equal("<<", strip_tags("<<<bad html>"))
+ assert_equal "This is <-- not\n a comment here.",
+ strip_tags("This is <-- not\n a comment here.")
+
assert_equal("Weirdos", strip_tags("Wei<<a>a onclick='alert(document.cookie);'</a>/>rdos"))from #11629 |
|
@pftg Sorry for the late response. The test case has been added. |
|
🆒 thanks |
|
Monster PR is monster. Where do we stand here? Are we waiting on approval of this PR to cut a release of loofah in order to update the dependency on the PR? |
|
There are more things to do @kaspth we need to update the TODO list |
|
Some small number of tests, regarding the new assert_select, still aren't passing. @rafaelfranca I'll update it now. |
…as it is included in ActionController::TestCase.
…on WhiteListSanitizer.
…coding ASCII-8BIT test errors.
…t_select can be called without specifying a root.
Loofah-integration Conflicts: actionpack/CHANGELOG.md actionview/CHANGELOG.md
|
I merged this branch on the rails/rails loofah branch. I'll test this branch with some applications I have and will test the deprecated gems too. If you have something to change on this branch please open PRs against this new branch. @kaspth awesome work! ❤️ |
|
epic! |
|
Hell yeah! Kasper
|
|
🤘 |
|
❤️ 💚 💛 💜 ❤️ |
|
Awesome! Great work @kaspth ! |
Today Rails uses the HTML-scanner gem to do its sanitization. We will switch the gem that is used, while keeping the old API for backwards compatibility. Instead of the scanner gem, we will use Loofah. Loofah is built on top of Nokogiri, meaning we rid the implementation's reliance on regular expressions and we get speed. On large documents and fragments Loofah is around 60 to 100% faster than the current implementation.
Notes
The sanitizers used in
ActionView::SanitizeHelperhave been extracted to the rails-html-sanitizer gem.https://github.com/rafaelfranca/rails-html-sanitizer
The
DomAssertionsandSelectorAssertionshave been extracted to the rails-dom-testing gem.https://github.com/kaspth/rails-dom-testing
The substitution values syntax in
assert_selecthas changed.The attribute to match should be enclosed in quotes to avoid issues with Nokogiri's css selector syntax parsing. It is not necessary to do so with the question mark.
Todos
Pending Test Fixes
Output Error: Unknown Encoding ASCII-8BITthree times in date_helper_test and once in form_helper_test. Related: [Bug?] unknown encoding ASCII-8BIT sparklemotion/nokogiri#553Sanitizers
FullSanitizer,LinkSanitizerandWhiteListSanitizerinsanitizers.rbprotocol_separatorandbad_tagsforWhiteListSanitizersanitizeaccept custom:tagsand:attributesoptionssanitizeaccept aLoofah::Scrubbervia:scrubberoptionPermitScrubberTargetScrubberSanitizers Testing
sanitizers_test.rbPermitScrubberTargetScrubberPermitScrubber's peculiarities fromsanitizers_test.rbDom and Selector Assertions
assert_dom_equalwith LoofahRelated issues
flavorjones/loofah#44
flavorjones/loofah#45
flavorjones/loofah#46
flavorjones/loofah#47
flavorjones/loofah#51
flavorjones/loofah#52
flavorjones/loofah#54
//@rafaelfranca