Loofah-integration #11218

Closed
wants to merge 192 commits into
from

Projects

None yet
@kaspth
Member
kaspth commented Jul 1, 2013

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::SanitizeHelper have been extracted to the rails-html-sanitizer gem.
https://github.com/rafaelfranca/rails-html-sanitizer

The DomAssertions and SelectorAssertions have been extracted to the rails-dom-testing gem.
https://github.com/kaspth/rails-dom-testing

The substitution values syntax in assert_select has changed.

assert_select "div#?", /\d+/
assert_select "div:match('id', ?)", /\d+/

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

  • Extract failing tests to mark them as pending.
  • Find areas where backwards compatibility is broken
Pending Test Fixes
  • Find out what causes Output Error: Unknown Encoding ASCII-8BIT three times in date_helper_test and once in form_helper_test. Related: sparklemotion/nokogiri#553
  • Research the other failing tests
Sanitizers
  • Implement Action View FullSanitizer, LinkSanitizer and WhiteListSanitizer in sanitizers.rb
  • Deprecate protocol_separator and bad_tags for WhiteListSanitizer
  • Make sanitize accept custom :tags and :attributes options
  • Make sanitize accept a Loofah::Scrubber via :scrubber option
  • Make PermitScrubber
  • Make TargetScrubber
Sanitizers Testing
  • Add new tests in sanitizers_test.rb
  • Complete test coverage for PermitScrubber
  • Complete test coverage for TargetScrubber
  • Test behavior of changing sanitization
  • Move testing of PermitScrubber's peculiarities from sanitizers_test.rb
Dom and Selector Assertions
  • Reimplement assert_dom_equal with Loofah
  • Move Dom and Selector assertions to Action View
  • Fix test_nested_css_select failing on line 245
  • Fix test_feed_item_encoded finding two p elements when it shouldn't
  • Marked as pending the colliding xml namespaces issue discussed here: https://groups.google.com/forum/#!topic/nokogiri-talk/Nv8kX4p_r7I

Related issues

flavorjones/loofah#44
flavorjones/loofah#45
flavorjones/loofah#46
flavorjones/loofah#47
flavorjones/loofah#51
flavorjones/loofah#52
flavorjones/loofah#54

//@rafaelfranca

@rafaelfranca rafaelfranca was assigned Jul 1, 2013
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jul 1, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ def sanitize(html, options = {})
+ return nil unless html
+ validate_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.')
@rafaelfranca
rafaelfranca Jul 1, 2013 Member

We can call self.class.protocol_separator here and in all the deprecated instance methods to avoid duplication.

@kaspth
kaspth Jul 1, 2013 Member

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 html
    
  •  validate_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.')
    
    We can call self.class.protocol_separator here and in all the deprecated instance methods to avoid duplication.


Reply to this email directly or view it on GitHub.

@rafaelfranca
Member

cc @carlosantoniodasilva @josevalim @jeremy @NZKoz @tenderlove mind to review this one? I think we are done

@frodsan frodsan commented on an outdated diff Jul 15, 2013
actionpack/CHANGELOG.md
@@ -65,5 +65,9 @@
* Fix removing trailing slash for mounted apps #3215
*Piotr Sarnacki*
+
+* Loofah replaces html-scanner in dom assertions
@frodsan
frodsan Jul 15, 2013 Contributor

New entries must be on the top.

@guilleiguaran guilleiguaran commented on an outdated diff Jul 15, 2013
actionpack/actionpack.gemspec
@@ -22,6 +22,8 @@ Gem::Specification.new do |s|
s.add_dependency 'activesupport', version
s.add_dependency 'actionview', version
+ s.add_dependency 'loofah', '~> 1.2.1'
+
@guilleiguaran
guilleiguaran Jul 15, 2013 Member

remove that whitespace between the dependencies 😄

@frodsan frodsan commented on an outdated diff Jul 15, 2013
actionpack/lib/action_dispatch/testing/assertions/dom.rb
+ # Determines further comparison via said type
+ # i.e. element node children with equal names has their attributes compared using +attributes_are_equal?+
+ def equal_children?(child, other_child)
+ return false unless child.type == other_child.type
+
+ case child.type
+ when Nokogiri::XML::Node::ELEMENT_NODE
+ child.name == other_child.name && attributes_are_equal?(child, other_child)
+ else
+ child.to_s == other_child.to_s
+ end
+ end
+
+ # +attributes_are_equal?+ sorts elements attributes by name and compares
+ # each attribute by calling +equal_attribute?+
+ # If those are true the attributes are considered equal
@frodsan
frodsan Jul 15, 2013 Contributor

+true+

@guilleiguaran
Member

I think we should move actionpack/lib/action_dispatch/testing/assertions/dom.rb to actionview and make to loofah only a dependency for actionview

/cc @drogus @strzalek

@kaspth
Member
kaspth commented Jul 15, 2013

@guilleiguaran I'll second that.
Though html-scanner is still required in selector.rb and tag.rb in assertions/, which I still haven't converted to using Loofah.

@guilleiguaran
Member

Let's move selector.rb and tag.rb also to actionview 😄

@kaspth
Member
kaspth commented Jul 15, 2013

Fine by me.

@mdespuits mdespuits and 1 other commented on an outdated diff Jul 15, 2013
...ction_view/helpers/sanitize_helper/permit_scrubber.rb
+
+ def text_or_cdata_node?(node)
+ case node.type
+ when Nokogiri::XML::Node::TEXT_NODE, Nokogiri::XML::Node::CDATA_SECTION_NODE
+ return true
+ end
+ false
+ end
+
+ def validate!(var, name)
+ if var && !var.is_a?(Enumerable)
+ raise ArgumentError, "You should pass :#{name} as an Enumerable"
+ end
+ var
+ end
+end
@mdespuits
mdespuits Jul 15, 2013 Contributor

Are these no newlines generally allowed?

@kaspth
kaspth Jul 16, 2013 Member

What do you mean?

@mdespuits
mdespuits Jul 16, 2013 Contributor

Generally, there is a newline at the end of files. This is the only one missing a newline. Mostly a nitpick...

@kaspth
kaspth Jul 17, 2013 Member

Ah, I see. I wasn't aware of that. I'll look through my files and add newlines where needed.

@kaspth
Member
kaspth commented Jul 16, 2013

The guides say that assert_tag is deprecated: http://guides.rubyonrails.org/testing.html#testing-views
So I'm guessing we shouldn't bother moving that over.

Should I remove the file assertions/tag.rb, @rafaelfranca?

@lukaszx0 lukaszx0 and 3 others commented on an outdated diff Jul 17, 2013
Gemfile
@@ -2,6 +2,9 @@ source 'https://rubygems.org'
gemspec
+# temporary gem while working on loofah integration
+gem 'loofah', '~> 1.2.1', github: 'kaspth/loofah'
@lukaszx0
lukaszx0 Jul 17, 2013 Member

Why it's pointing to your fork of loofah? Did you made any changes? Are they already merged to official master branch?

@vipulnsward
vipulnsward Jul 17, 2013 Member

I think there are changes essential for this to work if I am not wrong.

@lukaszx0
lukaszx0 Jul 17, 2013 Member

Yep. It would be good to merge those changes to official loofah repository before merging this PR.

@vipulnsward
vipulnsward Jul 17, 2013 Member

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.

@rafaelfranca
rafaelfranca Jul 17, 2013 Member

Yes, these PR are pending, but the idea is remove this git dependency.

@kaspth
kaspth Jul 17, 2013 Member

@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.)

@vipulnsward
vipulnsward Jul 21, 2013 Member

@kaspth you can now probably change this. Thanks to @flavorjones !

@kaspth
kaspth Jul 21, 2013 Member

Yes, I can. Much love to @flavorjones! Thank you kindly, Mike.

@lukaszx0 lukaszx0 and 2 others commented on an outdated diff Jul 17, 2013
actionview/test/template/date_helper_test.rb
@@ -2102,6 +2102,8 @@ def test_time_select_with_html_options
end
def test_time_select_with_html_options_within_fields_for
+ skip "Pending. Output error: 'unknown encoding ASCII-8BIT' makes Loofah return an empty string"
+
@post = Post.new
@lukaszx0
lukaszx0 Jul 17, 2013 Member

Why all of those are still skipped?

@rafaelfranca
rafaelfranca Jul 17, 2013 Member

We need to decide what will be done with these tests. Right now Loofah behaves differently so we are not sure if we will change the behavior breaking backward compatibility or write code to make it behave like before

@kaspth
kaspth Jul 17, 2013 Member

Loofah uses Nokogiri for html/xml-parsing. The tests revealed that there were many differences between Nokogiri's parsing and html-scanner's. That's why we're skipping those for now.

@rafaelfranca
Member

@kaspth yes, lets remove it

@kaspth
Member
kaspth commented Jul 17, 2013

@rafaelfranca Yippee!

@kaspth
Member
kaspth commented Jul 17, 2013

@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?

@lukaszx0
Member

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.

@kaspth
Member
kaspth commented Jul 18, 2013

Then it's all good then.

@pftg
Contributor
pftg commented Jul 27, 2013

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

@kaspth
Member
kaspth commented Jul 31, 2013

@pftg Sorry for the late response. The test case has been added.

@pftg
Contributor
pftg commented Jul 31, 2013

🆒 thanks

@schneems
Member
schneems commented Aug 1, 2013

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?

@rafaelfranca
Member

There are more things to do @kaspth we need to update the TODO list

@kaspth
Member
kaspth commented Aug 1, 2013

Some small number of tests, regarding the new assert_select, still aren't passing.
Yes, we are waiting for a new release of Loofah too.

@rafaelfranca I'll update it now.

@vipulnsward vipulnsward and 2 others commented on an outdated diff Aug 13, 2013
actionpack/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Loofah replaces html-scanner in dom assertions
+
@vipulnsward
vipulnsward Aug 13, 2013 Member

@kaspth maybe you can expand this a little bit?

@kaspth
kaspth Aug 13, 2013 Member

You're right.
Since the Dom and Selector assertions have been moved to Action View, I'll move this to Action View's changelog.

@robin850
robin850 Sep 2, 2013 Member

Then, this entry can be simply removed since you have added a better one to the actionview's changelog.

@kaspth
kaspth Sep 3, 2013 Member

You're right, I just forgot to remove it. Thanks.

@vipulnsward vipulnsward and 1 other commented on an outdated diff Aug 13, 2013
...ck/lib/action_dispatch/testing/assertions/selector.rb
- root = HTML::Document.new(part.body.to_s).root
- assert_select root, ":root", &block
- end
- end
- end
- end
-
- protected
- # +assert_select+ and +css_select+ call this to obtain the content in the HTML page.
- def response_from_page
- html_document.root
- end
- end
- end
-end
+ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::SelectorAssertions has been moved to ActionView. You can find it in action_view/testing/assertions/selector.rb.")
@vipulnsward
vipulnsward Aug 13, 2013 Member

I think it would make sense to provide class name instead of file name in action_view/testing/assertions/selector.rb

@kaspth
kaspth Aug 13, 2013 Member

Really? Only the location has changed. The name is still SelectorAssertions.

@vipulnsward vipulnsward and 1 other commented on an outdated diff Aug 17, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+require 'active_support/core_ext/class/attribute'
+require 'active_support/deprecation'
+require 'action_view/helpers/sanitize_helper/scrubbers'
+
+module ActionView
+ XPATHS_TO_REMOVE = %w{.//script .//form comment()}
+
+ class Sanitizer
+ # :nodoc:
+ def sanitize(html, options = {})
+ raise NotImplementedError, "subclasses must implement"
+ end
+
+ def remove_xpaths(html, xpaths)
+ if html.respond_to?(:xpath)
+ xpaths.each { |xpath| html.xpath(xpath).remove }
@vipulnsward
vipulnsward Aug 17, 2013 Member

@kaspth Can you rename the inner variable, it seems a bit hard to read.

@kaspth
kaspth Aug 17, 2013 Member

The 'xpath' in the block?

@kaspth
kaspth Aug 17, 2013 Member

Good news, I found a way to get rid of it entirely: html.xpath(*xpaths).remove

@vipulnsward vipulnsward and 1 other commented on an outdated diff Aug 17, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+
+module ActionView
+ XPATHS_TO_REMOVE = %w{.//script .//form comment()}
+
+ class Sanitizer
+ # :nodoc:
+ def sanitize(html, options = {})
+ raise NotImplementedError, "subclasses must implement"
+ end
+
+ def remove_xpaths(html, xpaths)
+ if html.respond_to?(:xpath)
+ xpaths.each { |xpath| html.xpath(xpath).remove }
+ html
+ else
+ remove_xpaths(Loofah.fragment(html), xpaths).to_s
@vipulnsward
vipulnsward Aug 17, 2013 Member

Is the to_s correct here? Both conditions return different result object types.

@kaspth
kaspth Aug 17, 2013 Member

The idea was to return what you called it with: String => String, DocumentFragment => DocumentFragment.

Thoughts, @rafaelfranca?

@vipulnsward vipulnsward and 2 others commented on an outdated diff Aug 17, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ remove_xpaths(loofah_fragment, XPATHS_TO_REMOVE)
+ loofah_fragment.scrub!(:strip)
+ end
+ loofah_fragment.to_s
+ end
+
+ def sanitize_css(style_string)
+ Loofah::HTML5::Scrub.scrub_css style_string
+ end
+
+ def protocol_separator
+ self.class.protocol_separator
+ end
+
+ def protocol_separator=(value)
+ self.class.protocol_separator
@vipulnsward
vipulnsward Aug 17, 2013 Member

This should have an assign here?

@kaspth
kaspth Aug 17, 2013 Member

Deprecated: self.class.protocol_separator is used to remove duplication.

@robin850
robin850 Sep 3, 2013 Member

I'm not sure to understand here. Do you mean that we can't change the value of the protocol separator ? If it's the case, then we should better put a deprecation warning too, no ? Sorry if I'm misunderstanding.

@kaspth
kaspth Sep 3, 2013 Member

It's deprecated. It calls self.class.protocol_separator, which has the deprecation message.
Should I add a comment to make that more clear?
You aren't the first person to be confused about this.

@vipulnsward vipulnsward and 2 others commented on an outdated diff Aug 17, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+
+ def protocol_separator=(value)
+ self.class.protocol_separator
+ end
+
+ def bad_tags
+ self.class.bad_tags
+ end
+
+ class << self
+ def protocol_separator
+ ActiveSupport::Deprecation.warn('protocol_separator has been deprecated and has no effect.')
+ end
+
+ def protocol_separator=(value)
+ protocol_separator
@vipulnsward
vipulnsward Aug 17, 2013 Member

ditto. I guess this is to be deprecated, so could throw a warning, like above?

@kaspth
kaspth Aug 17, 2013 Member

It should throw a warning 😉

@jeremy
jeremy Sep 25, 2013 Member

Use an explicit deprecation warning for protocol_separator= rather than leaning on protocol_separator's warning.

@vipulnsward vipulnsward and 1 other commented on an outdated diff Aug 17, 2013
.../lib/action_view/helpers/sanitize_helper/scrubbers.rb
@@ -0,0 +1,138 @@
+# === PermitScrubber
+#
+# PermitScrubber allows you to permit only your own tags and/or attributes.
+#
+# PermitScrubber can be subclassed to determine:
+# - When a node should be skipped via +skip_node?+
+# - When a node is allowed via +allowed_node?+
+# - When an attribute should be scrubbed via +scrub_attribute?+
+#
+# Subclasses don't need to worry if tags or attributes are set or not.
+# If tags or attributes are not set, Loofahs behavior will be used.
@kaspth
kaspth Aug 17, 2013 Member

Thanks for all your feedback, @vipulnsward!

@vipulnsward vipulnsward and 1 other commented on an outdated diff Aug 17, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ # # assert that the referenced method generates the appropriate HTML string
+ # assert_dom_equal '<a href="http://www.example.com">Apples</a>', link_to("Apples", "http://www.example.com")
+ def assert_dom_equal(expected, actual, message = nil)
+ assert dom_assertion(message, expected, actual)
+ end
+
+ # The negated form of +assert_dom_equal+.
+ #
+ # # assert that the referenced method does not generate the specified HTML string
+ # assert_dom_not_equal '<a href="http://www.example.com">Apples</a>', link_to("Oranges", "http://www.example.com")
+ def assert_dom_not_equal(expected, actual, message = nil)
+ assert_not dom_assertion(message, expected, actual)
+ end
+
+ protected
+ def dom_assertion(message = nil, *html_strings)
@vipulnsward
vipulnsward Aug 17, 2013 Member

Do we need the *html_stings, since we know we are always passing 2 arguments here?

@kaspth
kaspth Aug 17, 2013 Member

No. You're probably right.

@vipulnsward vipulnsward commented on an outdated diff Aug 17, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ case child.type
+ when Nokogiri::XML::Node::ELEMENT_NODE
+ child.name == other_child.name && attributes_are_equal?(child, other_child)
+ else
+ child.to_s == other_child.to_s
+ end
+ end
+
+ # +attributes_are_equal?+ sorts elements attributes by name and compares
+ # each attribute by calling +equal_attribute?+
+ # If those are +true+ the attributes are considered equal
+ def attributes_are_equal?(element, other_element)
+ first_nodes = element.attribute_nodes.sort_by { |a| a.name }
+ other_nodes = other_element.attribute_nodes.sort_by { |a| a.name }
+
+ return false unless first_nodes.size == other_nodes.size
@vipulnsward
vipulnsward Aug 17, 2013 Member

The condition can be taken above the sorts.

@vipulnsward vipulnsward commented on an outdated diff Aug 17, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+require 'active_support/core_ext/class/attribute'
+require 'active_support/deprecation'
+require 'action_view/helpers/sanitize_helper/scrubbers'
+
+module ActionView
+ XPATHS_TO_REMOVE = %w{.//script .//form comment()}
+
+ class Sanitizer
+ # :nodoc:
+ def sanitize(html, options = {})
+ raise NotImplementedError, "subclasses must implement"
+ end
+
+ def remove_xpaths(html, xpaths)
+ if html.respond_to?(:xpath)
+ html.xpath(*xpaths).remove
@robin850 robin850 commented on the diff Sep 2, 2013
actionpack/lib/action_dispatch/testing/assertions/tag.rb
-
- def find_tag(conditions)
- html_document.find(conditions)
- end
-
- def find_all_tag(conditions)
- html_document.find_all(conditions)
- end
-
- def html_document
- xml = @response.content_type =~ /xml$/
- @html_document ||= HTML::Document.new(@response.body, false, xml)
- end
- end
- end
-end
@robin850
robin850 Sep 2, 2013 Member

Don't we need to put a deprecation warning as well here ?

@kaspth
kaspth Sep 3, 2013 Member

All right, done.

@rafaelfranca rafaelfranca and 2 others commented on an outdated diff Sep 12, 2013
actionpack/actionpack.gemspec
@@ -20,7 +20,9 @@ Gem::Specification.new do |s|
s.requirements << 'none'
s.add_dependency 'activesupport', version
-
+ s.add_dependency 'actionview', version
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

We can't depend on action view anymore. I think we will have to remove all Dom and selectors assertion from Action Pack.

@josevalim @drogus @strzalek thoughts?

@kaspth
kaspth Sep 13, 2013 Member

The Dom, Selector and Tag assertions are removed (well, deprecated) in Action Pack.

Action View isn't required here on master. I think, I might have fudged something up.

@rafaelfranca
rafaelfranca Sep 13, 2013 Member

But ActionView::Assertions is being included in Action Pack. We should remove this too

@kaspth
kaspth Sep 13, 2013 Member

We should remove that they're included in Action View? Is that what you're saying?

@rafaelfranca
rafaelfranca Sep 13, 2013 Member

I'm saying we should not include anything from Action View in Action Pack. So https://github.com/rails/rails/pull/11218/files#L2R616 and https://github.com/rails/rails/pull/11218/files#L7R326 have to be removed.

@kaspth
kaspth Sep 13, 2013 Member

Ok.
Removing these results in two kinds of undefined methods errors: assert_select and html_document.
Both methods are part of DomAssertions.

What can I do to remove the dependencies?
/cc @strzalek

@guilleiguaran
guilleiguaran Sep 13, 2013 Member

IMO, assert_select should live inside of AV, no AP

@kaspth
kaspth Sep 13, 2013 Member

@guilleiguaran assert_select has already been moved to Action View.

We're keeping Action View only as a development dependency, so the tests can use the ActionView::Assertions modules.

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ @link_scrubber.tags = %w(a href)
+ end
+
+ def sanitize(html, options = {})
+ Loofah.scrub_fragment(html, @link_scrubber).to_s
+ end
+ end
+
+ class WhiteListSanitizer < Sanitizer
+
+ def initialize
+ @permit_scrubber = PermitScrubber.new
+ end
+
+ def sanitize(html, options = {})
+ return nil unless html
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

return unless html

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
.../lib/action_view/helpers/sanitize_helper/scrubbers.rb
+# PermitScrubber can be subclassed to determine:
+# - When a node should be skipped via +skip_node?+
+# - When a node is allowed via +allowed_node?+
+# - When an attribute should be scrubbed via +scrub_attribute?+
+#
+# Subclasses don't need to worry if tags or attributes are set or not.
+# If tags or attributes are not set, Loofah's behavior will be used.
+# If you override +allowed_node?+ and no tags are set, it will not be called.
+# Instead Loofahs behavior will be used.
+# Likewise for +scrub_attribute?+ and attributes respectively.
+#
+# Text and CDATA nodes are skipped by default.
+# Unallowed elements will be stripped, i.e. element is removed but its subtree kept.
+# Supplied tags and attributes should be Enumerables
+#
+# +tags=+
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

@fxn could you review this documentation? I think it is not following our style but I don't know how to fix it 😊

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ def sanitize(html, options = {})
+ Loofah.scrub_fragment(html, @link_scrubber).to_s
+ end
+ end
+
+ class WhiteListSanitizer < Sanitizer
+
+ def initialize
+ @permit_scrubber = PermitScrubber.new
+ end
+
+ def sanitize(html, options = {})
+ return nil unless html
+
+ loofah_fragment = Loofah.fragment(html)
+ if scrubber = options[:scrubber]
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

Put a blank line before the if

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+
+ def sanitize(html, options = {})
+ return nil unless html
+
+ loofah_fragment = Loofah.fragment(html)
+ if scrubber = options[:scrubber]
+ # No duck typing, Loofah ensures subclass of Loofah::Scrubber
+ loofah_fragment.scrub!(scrubber)
+ elsif options[:tags] || options[:attributes]
+ @permit_scrubber.tags = options[:tags]
+ @permit_scrubber.attributes = options[:attributes]
+ loofah_fragment.scrub!(@permit_scrubber)
+ else
+ remove_xpaths(loofah_fragment, XPATHS_TO_REMOVE)
+ loofah_fragment.scrub!(:strip)
+ end
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

Put a blank line after the end

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ end
+ end
+
+ class LinkSanitizer < Sanitizer
+ def initialize
+ @link_scrubber = TargetScrubber.new
+ @link_scrubber.tags = %w(a href)
+ end
+
+ def sanitize(html, options = {})
+ Loofah.scrub_fragment(html, @link_scrubber).to_s
+ end
+ end
+
+ class WhiteListSanitizer < Sanitizer
+
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

✂️ this line

@rafaelfranca
rafaelfranca Sep 12, 2013 Member

I mean, the blank line

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
...lib/action_view/helpers/sanitize_helper/sanitizers.rb
+ end
+
+ def remove_xpaths(html, xpaths)
+ if html.respond_to?(:xpath)
+ html.xpath(*xpaths).remove
+ html
+ else
+ remove_xpaths(Loofah.fragment(html), xpaths).to_s
+ end
+ end
+ end
+
+ class FullSanitizer < Sanitizer
+ def sanitize(html, options = {})
+ return nil unless html
+ return html if html.empty?
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

What do you think?

return html if html.blank?

I know it will return false if you pass false but this is not valid use

@kaspth
kaspth Sep 13, 2013 Member

Sounds good to me. Should or can we document that case with false somewhere?

@kaspth
kaspth Sep 13, 2013 Member

I've tried to explain it in the commit message.

@rafaelfranca
rafaelfranca Sep 13, 2013 Member

We don't need to document. But you can put in the commit message 👍

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ message ||= "Expected: #{expected}\nActual: #{actual}"
+ assert compare_doms(expected_dom, actual_dom), message
+ end
+
+ # The negated form of +assert_dom_equal+.
+ #
+ # # assert that the referenced method does not generate the specified HTML string
+ # assert_dom_not_equal '<a href="http://www.example.com">Apples</a>', link_to("Oranges", "http://www.example.com")
+ def assert_dom_not_equal(expected, actual, message = nil)
+ expected_dom, actual_dom = Loofah.fragment(expected), Loofah.fragment(actual)
+ message ||= "Expected: #{expected}\nActual: #{actual}"
+ assert_not compare_doms(expected_dom, actual_dom), message
+ end
+
+ protected
+ def compare_doms(expected, actual)
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

Put a blank line above (this is the guideline)

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ #
+ # # assert that the referenced method does not generate the specified HTML string
+ # assert_dom_not_equal '<a href="http://www.example.com">Apples</a>', link_to("Oranges", "http://www.example.com")
+ def assert_dom_not_equal(expected, actual, message = nil)
+ expected_dom, actual_dom = Loofah.fragment(expected), Loofah.fragment(actual)
+ message ||= "Expected: #{expected}\nActual: #{actual}"
+ assert_not compare_doms(expected_dom, actual_dom), message
+ end
+
+ protected
+ def compare_doms(expected, actual)
+ return false unless expected.children.size == actual.children.size
+
+ expected.children.each_with_index do |child, i|
+ return false unless equal_children?(child, actual.children[i])
+ end
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

put a blank line after the end

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ true
+ end
+
+ def equal_children?(child, other_child)
+ return false unless child.type == other_child.type
+
+ if child.element?
+ child.name == other_child.name &&
+ equal_attribute_nodes?(child.attribute_nodes, other_child.attribute_nodes)
+ else
+ child.to_s == other_child.to_s
+ end
+ end
+
+ def equal_attribute_nodes?(nodes, other_nodes)
+ return false unless nodes.size == other_nodes.size
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

blank line

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
actionview/lib/action_view/testing/assertions/dom.rb
+ if child.element?
+ child.name == other_child.name &&
+ equal_attribute_nodes?(child.attribute_nodes, other_child.attribute_nodes)
+ else
+ child.to_s == other_child.to_s
+ end
+ end
+
+ def equal_attribute_nodes?(nodes, other_nodes)
+ return false unless nodes.size == other_nodes.size
+ nodes = nodes.sort_by(&:name)
+ other_nodes = other_nodes.sort_by(&:name)
+
+ nodes.each_with_index do |attr, i|
+ return false unless equal_attribute?(attr, other_nodes[i])
+ end
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

blank line

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
actionview/test/template/assert_select_test.rb
@@ -173,11 +173,11 @@ def test_counts
def test_substitution_values
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
- assert_select "div#?", /\d+/ do |elements|
+ assert_select "div:match('id', ?)", /\d+/ do |elements|
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

We need to document this change in the CHANGELOG since it will be non-backward compatible

@kaspth
kaspth Sep 13, 2013 Member

I'll copy the description under notes from this PR.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
actionpack/actionpack.gemspec
@@ -20,7 +20,9 @@ Gem::Specification.new do |s|
s.requirements << 'none'
s.add_dependency 'activesupport', version
-
+ s.add_dependency 'actionview', version
+
+ s.add_dependency 'loofah', '~> 1.2.1'
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

We can remove this one also.

@kaspth
kaspth Sep 13, 2013 Member

Ok, per your earlier comment I'm removing the actionview dependency, too.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
actionview/CHANGELOG.md
@@ -1,3 +1,21 @@
+* Dom and Selector assertions has been moved to Action View.
+
+ Loofah replaces html-scanner in `DomAssertions`:
+ `assert_dom_equal`
+ `assert_dom_not_equal`
+
+ Also in `SelectorAssertions`:
+ `css_select`
+ `assert_select`
+ `assert_select_encoded`
+ `assert_select_email`
+
+ *Kasper Timm Hansen*, *Rafael Mendonça França*
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

The credit is all your bro 👏 You can remove my name.

@kaspth
kaspth Sep 13, 2013 Member

O mentor, my mentor! 😱

Ok, ✂️

@rafaelfranca rafaelfranca commented on an outdated diff Sep 12, 2013
actionview/CHANGELOG.md
@@ -1,3 +1,21 @@
+* Dom and Selector assertions has been moved to Action View.
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

We need a similar entry on Action Pack to tell they were removed.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
actionview/CHANGELOG.md
@@ -1,3 +1,21 @@
+* Dom and Selector assertions has been moved to Action View.
+
+ Loofah replaces html-scanner in `DomAssertions`:
+ `assert_dom_equal`
+ `assert_dom_not_equal`
+
+ Also in `SelectorAssertions`:
+ `css_select`
+ `assert_select`
+ `assert_select_encoded`
+ `assert_select_email`
+
+ *Kasper Timm Hansen*, *Rafael Mendonça França*
+
+* Loofah replaces html-scanner in sanitize_helper
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

sanitize_helper

@rafaelfranca
rafaelfranca Sep 12, 2013 Member

Lets talk about sanitize receiving a scrubber.

@kaspth
kaspth Sep 13, 2013 Member

Ok, I'll also add information about PermitScrubber and TargetScrubber.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2013
actionview/lib/action_view.rb
@@ -23,6 +23,7 @@
require 'active_support'
require 'active_support/rails'
+require 'loofah'
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

Maybe we should require this only in the entries points. sanitize_helper/sanitizers.rb and testing/assertions.rb. Could you test if this will work?

@kaspth
kaspth Sep 13, 2013 Member

hmm...
It works for assertions.rb.
But running the tests for sanitizers.rb gave me 195 failures.
What's weird is, I don't get any errors a la uninitialized constant Loofah, the tests just fail.

@kaspth
kaspth Sep 13, 2013 Member

Scratch that.
The failures were from an earlier commit (an unless instead of an if. Ugh).
Requiring only in the entry points works.

@rafaelfranca rafaelfranca commented on the diff Sep 12, 2013
actionview/lib/action_view/helpers/sanitize_helper.rb
@@ -27,7 +27,29 @@ module SanitizeHelper
#
# <%= sanitize @article.body %>
#
- # Custom Use (only the mentioned tags and attributes are allowed, nothing else)
+ # Custom Use - Custom Scrubber
+ # (supply a Loofah::Scrubber that does the sanitization)
+ #
+ # scrubber can either wrap a block:
+ # scrubber = Loofah::Scrubber.new do |node|
+ # node.text = "dawn of cats"
+ # end
+ #
+ # or be a subclass of Loofah::Scrubber which responds to scrub:
+ # class KittyApocalypse < Loofah::Scrubber
@rafaelfranca
rafaelfranca Sep 12, 2013 Member

lol @ name

@NZKoz NZKoz and 1 other commented on an outdated diff Sep 18, 2013
...ck/test/controller/request_forgery_protection_test.rb
@@ -293,10 +293,10 @@ class RequestForgeryProtectionControllerUsingResetSessionTest < ActionController
end
test 'should emit a csrf-param meta tag and a csrf-token meta tag' do
- SecureRandom.stubs(:base64).returns(@token + '<=?')
+ SecureRandom.stubs(:base64).returns(@token + 'U+003c=U+0022U+003fU+0022') # '<="?"'
@NZKoz
NZKoz Sep 18, 2013 Member

It's not clear why you're changing the test code here and then the assertions below. Is this change breaking people's apps if they make similar assertions?

@kaspth
kaspth Sep 19, 2013 Member

Nokogiri leaves the < unescaped, so the assert_select looking for &lt; will never work.
We're working on getting the right assertion.
//@rafaelfranca

@rafaelfranca rafaelfranca commented on an outdated diff Sep 24, 2013
actionpack/lib/action_controller/test_case.rb
@@ -612,6 +613,7 @@ def build_response
included do
include ActionController::TemplateAssertions
include ActionDispatch::Assertions
+ include ActionView::Assertions
@rafaelfranca
rafaelfranca Sep 24, 2013 Member

We have to remove this or we will make Action Pack dependent on Action View.

@rafaelfranca rafaelfranca commented on an outdated diff Sep 24, 2013
actionpack/lib/action_dispatch/testing/integration.rb
@@ -322,7 +323,7 @@ def process(method, path, parameters = nil, headers_or_env = nil)
end
module Runner
- include ActionDispatch::Assertions
+ include ActionDispatch::Assertions, ActionView::Assertions
@rafaelfranca
rafaelfranca Sep 24, 2013 Member

We have to remove this or we will make Action Pack dependent on Action View

@rafaelfranca rafaelfranca commented on an outdated diff Sep 24, 2013
actionpack/lib/action_dispatch/testing/assertions/tag.rb
- def find_tag(conditions)
- html_document.find(conditions)
- end
-
- def find_all_tag(conditions)
- html_document.find_all(conditions)
- end
-
- def html_document
- xml = @response.content_type =~ /xml$/
- @html_document ||= HTML::Document.new(@response.body, false, xml)
- end
- end
- end
-end
+ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::TagAssertions has been eprecated. Use the assert_select methods from SelectorAssertions in action_view/testing/assertions/selector.rb.")
@rafaelfranca
rafaelfranca Sep 24, 2013 Member

Actually we removed it. So we can't say it is deprecated. Lets only remove it.

@kaspth kaspth and 2 others commented on an outdated diff Oct 13, 2013
guides/source/testing.md
@@ -634,7 +634,7 @@ assert_select "ol" do
end
```
-The `assert_select` assertion is quite powerful. For more advanced usage, refer to its [documentation](http://api.rubyonrails.org/classes/ActionDispatch/Assertions/SelectorAssertions.html).
+The `assert_select` assertion is quite powerful. For more advanced usage, refer to its [documentation](http://api.rubyonrails.org/classes/ActionView/Assertions/SelectorAssertions.html).
@kaspth
kaspth Oct 13, 2013 Member

@rafaelfranca where do I point this to now that assert_select is not in rails?

@rafaelfranca
rafaelfranca Oct 13, 2013 Member

Good question. cc @fxn

@fxn
fxn Oct 14, 2013 Member

I believe the gem should be documented, and the Rails guide would maybe just mention the gem and link to it for further details.

@kaspth
kaspth Oct 14, 2013 Member

It is documented.

Is a github link fine?
https://github.com/kaspth/rails-dom-testing

Kasper

Den 14/10/2013 kl. 10.45 skrev Xavier Noria notifications@github.com:

In guides/source/testing.md:

@@ -634,7 +634,7 @@ assert_select "ol" do
end


-The `assert_select` assertion is quite powerful. For more advanced usage, refer to its [documentation](http://api.rubyonrails.org/classes/ActionDispatch/Assertions/SelectorAssertions.html).
+The `assert_select` assertion is quite powerful. For more advanced usage, refer to its [documentation](http://api.rubyonrails.org/classes/ActionView/Assertions/SelectorAssertions.html).
I believe the gem should be documented, and the Rails guide would maybe just mention the gem and link to it for further details.


Reply to this email directly or view it on GitHub.

@robin850 robin850 added this to the 4.2.0 milestone Feb 23, 2014
@kaspth kaspth and 1 other commented on an outdated diff May 28, 2014
actionmailer/lib/action_mailer/test_case.rb
@@ -20,6 +20,8 @@ module Behavior
class_attribute :_mailer_class
setup :initialize_test_deliveries
setup :set_expected_mail
+
+ teardown :restore_delivery_method
@kaspth
kaspth May 28, 2014 Member

@rafaelfranca I'm not sure if this is the right move.

The tests on line 112 and 119 in delivery_methods_test.rb we're breaking when all the actionmailer tests we're running (i.e. running just the file the tests passed).

The tests expected the ActionMailer::Base.delivery_method to be :smtp and not :test, which they were on this branch. The only thing different between master and this branch is that assert_select_email_test file. Indeed commenting out the whole class makes the tests pass, while commenting out just the test methods made them break.

I hope I've made the problem clear, it was very confusing to figure out.

@rafaelfranca
rafaelfranca Jun 11, 2014 Member

This change make sense. I think it is an order dependent problem and this is the right way to fix. Maybe we should also ensure assert_select_email_test file doesn't leak any state.

@kaspth
kaspth Jun 11, 2014 Member

But with this change it won't leak anything if I'm not mistaken?
Looks like Zuhao fixed this on master: c4f4123

@rafaelfranca
rafaelfranca Jun 11, 2014 Member

You are right.

kaspth added some commits Jun 12, 2013
@kaspth kaspth Added Loofah as a dependency in actionview.gemspec.
Implemented ActionView: FullSanitizer, LinkSanitizer and WhiteListSanitizer in sanitizers.rb.
Deprecated protocol_separator and bad_tags.
Added new tests in sanitizers_test.rb and reimplemented assert_dom_equal with Loofah.
c94e24f
@kaspth kaspth Removed duplication in the deprecated methods. d4d1392
@kaspth kaspth Added PermitScrubber which allows you to permit elements for sanitiza…
…tion.
2622da1
@kaspth kaspth Reordered form removal with stripping. 3e4ae8e
@kaspth kaspth Removed the contains_bad_protocols? method as well as the tests for i…
…t. Loofah already deals with this.
167e998
@kaspth kaspth Changed expected value from '<b>' to empty string. d3d979e
@kaspth kaspth bad_tags include form since we remove it. Also to prevent a should_al…
…low_form_tag test creation.
91712cc
@kaspth kaspth Added guard clauses to FullSanitizer. 5dfd394
@kaspth kaspth Extracted failing tests in santiizers_test.rb into their own methods …
…and marked them as pending.
2e8c536
@kaspth kaspth Added video poster sanitization testing (from @vipulnsward). 6a05cb6
@kaspth kaspth Renamed the SanitizerTest class to SanitersTest, to remove the confli…
…ct with the old SanitizerTest for html-scanner.
5282518
@kaspth kaspth Added removal of script tags to WhiteListSanitizer. 55b453f
@kaspth kaspth Extracted the xpath removals into some new API that allows users to r…
…emove xpath subtrees.
68f75b9
@kaspth kaspth Added comment removal. Changed definitation of remove_xpaths to not u…
…se a splat operator.
40bbb49
@kaspth kaspth Extracted one highlight test method and marked it as pending. c80da23
@kaspth kaspth Changed the description of some pending tests. Changed the expected o…
…utput of a script test.
4f67398
@kaspth kaspth Added ActionView::Sanitizer and moved remove_xpaths to there. 4fbec83
@kaspth kaspth Added some tests for ActionView::Sanitizer. d631b37
@kaspth kaspth Marked some tests as pending in date_helper_test.rb. 561fbe0
@kaspth kaspth Marked a test in form_helper_test.rb as pending because of unknown en…
…coding ASCII-8BIT output error.
32850b5
@kaspth kaspth Marked tests in sanitize_helper_test.rb as pending. 7e2f7da
@kaspth kaspth Moved requiring of Loofah from sanitizers.rb to action_view.rb. c88d573
@kaspth kaspth Added ability to pass a custom scrubber to sanitize. Includes test co…
…verage.
6241bb8
@kaspth kaspth Marked the private API as not needing code documentation. 8fdf86c
@kaspth kaspth Updated the documentation to reflect the scrubber option. dad96ef
@kaspth kaspth Updated documentation to tell that a custom scrubber takes precedence. 42f0198
@kaspth kaspth Removed whitespace between dependencies. c63b75a
@kaspth kaspth Corrected documentation bug. 8f5547f
@kaspth kaspth Removed tag.rb since it has been deprecated. 2ff60e8
@kaspth kaspth The first attempt at abstracting argument parsing from selection meth…
…ods.
8c0536c
@kaspth kaspth Changed name to selector. And a bunch of other things. a38c759
kaspth added some commits Aug 13, 2013
@kaspth kaspth Trimmed deprecation message for ActionDispatch::Assertions::SelectorA…
…ssertions.
dddf86a
@kaspth kaspth Moved Action Pack changelog message to Action View. Clarified Dom and…
… Selector assertions changes in there.
63e0fa7
@kaspth kaspth Added related Nokogiri issue link to tests that fail with unknown enc…
…oding ASCII-8BIT.
5a14dbf
@kaspth kaspth Reworked root and selector conditional assignment in css_select. bffa646
@kaspth kaspth Changed wording of missing selector argument exception message in css…
…_select.
86c6f5b
@kaspth kaspth Removed duplication in assert_dom_equal and assert_dom_not_equal. 9dac1e8
@kaspth kaspth Fixed: spelling mistake in SanitizeHelperTest. 9a3a59e
@kaspth kaspth Changed: remove_xpaths called with String returns String, while calle…
…d with Loofah fragment returns Loofah fragment. Added tests for this.
97c5e6f
@kaspth kaspth Renamed: remove_xpaths tests no longer prefixed with sanitizer. 1825edc
@kaspth kaspth Changed: return early from compare_doms if the two doms don't have th…
…e same number of children.
75789d5
@kaspth kaspth Changed: removed @selected and @page variables from HTMLSelector sinc…
…e one method used them. Passed the values directly to there instead.
71aaddb
@kaspth kaspth Changed: HTMLSelector comparisons renamed to equality_tests. 20615ec
@kaspth kaspth Changed: put selector extraction into selector_from, which is renamed…
… to extract_selector.
ce4396b
@kaspth kaspth Removed unnecessary lines from HTMLSelector initialize. 9a536bc
@kaspth kaspth Renamed: HTMLSelector css_selector to selector. 65ed2b6
@kaspth kaspth Changed: using duck typing instead of requiring subclasses of Node an…
…d NodeSet.
cabef14
@kaspth kaspth Moved: initial assignment of @selector_is_second_argument is now in i…
…nitialize.
4b55c0a
@kaspth kaspth Changed conditional check in filter. Removed weird comments. e600b3a
@kaspth kaspth Extracted: create Regexp from match_with and use =~ to compare instea…
…d of checking .is_a? Regexp every time through the loop.
5169b00
@kaspth kaspth Fixed: added apostrophe to possessive noun. c1a7864
@kaspth kaspth Simplified the removal of xpaths in remove_xpaths. Added more tests f…
…or remove_xpaths.
6217178
@kaspth kaspth Changed back to =~ or == comparison in HTMLSelector filter. bab54e4
@kaspth kaspth Removed html_strings variable, no splat operator needed. 73c690d
@kaspth kaspth Changed attributes_are_equal? to equal_attribute_nodes? which takes a…
…ttribute_nodes instead of nodes.
d6067e8
@kaspth kaspth Reworked some internal documentation for equal_attribute_nodes?. 905d2bc
@kaspth kaspth Removed case statement in equal_children? used child.element? instead. 97d20b1
@kaspth kaspth Removed unnecessary documentation in DomAssertions. 7f7a1b5
@kaspth kaspth Removed a bunch of duplicated tests in SanitizeHelperTest. 2563c2c
@kaspth kaspth Fixed uninitialized constant ActionView::HTML error entered after rec…
…ent git rebase.
01e6e1d
@kaspth kaspth Removed dom_assertion method since it created bugs. cb865e1
@kaspth kaspth Added deprecation warning to ActionDispatch::Assertions::TagAssertions. 4e97d75
@kaspth kaspth Changed test expectation from '<<' to '' with string to sanitize '<<<…
…bad html>' in sanitizers_test.
229092f
@kaspth kaspth Removed require's for html-scanner. 170f414
@kaspth kaspth Stylistic improvements. Some light documentation for remove_xpaths. 5430487
@kaspth kaspth Now returning html if html is blank? in FullSanitizer and WhiteListSa…
…nitizer. This means it'll return false if called with false, however that is not a valid use case.
0a0d151
@kaspth kaspth Stylistic improvements in ActionView::Assertions::DomAssertions. dd19557
@kaspth kaspth Updated Action View changelog entries with more information about the…
… changes in the API. Removed mention of mentor (at his request).
240ac66
@kaspth kaspth Added deprecation notice to actionpack changelog. 7cd2eb5
@kaspth kaspth Minor rewording in TargetScrubber documentation. 19406da
@kaspth kaspth Now only requiring Loofah in the places where it is needed. 7f9106d
@kaspth kaspth Changed PermitScrubber's direction to bottom up to align better with …
…Loofah's strip scrubber.
ddc24fd
@kaspth kaspth Added some test coverage for PermitScrubber. facc4f3
@kaspth kaspth Moved some tests to scrubbers_test.rb. Added better testing of access…
…or validation.
b4cfb59
@kaspth kaspth Rounded out PermitScrubber tests. Extracted helper methods to a Scrub…
…berTest class.
15382e9
@kaspth kaspth Added tests for TargetScrubber. af05b01
@kaspth kaspth Nokogiri leaves '<' unescaped, so the assert_select looking for '&lt;…
…' will never work. Switched to assert_matching the reponse body.
535a3b6
@kaspth kaspth Added deprecation warning for invalid selectors and skipping assertions. 9ef95a7
@kaspth kaspth Silenced deprecation warnings in the tests. Documentation uses presen…
…t tense. Changed deprecation message to not use you. Also returning from rescue block in catch_invalid_selector to abort reraising the exception.
68e08fe
@kaspth kaspth Changed ActiveSupport::Derprecation.silence to assert_deprecated. 2e81324
@kaspth kaspth Moved ActionView::Assertions dependency from Action Pack's lib to abs…
…tract_unit.rb.
a766a02
@kaspth kaspth Removed tag.rb, since it is actually removed, not just deprecated. [c…
…i skip]
fa916af
@kaspth kaspth Added rails-dom-testing and rails-html-sanitizer to Gemfile. Added te…
…sts for assert_select_email.
94ca27b
@kaspth kaspth Removed ActionView::Assertions. Getting ready to exchange with Rails:…
…:Dom::Testing::Assertions.
c287572
@kaspth kaspth Exchanged requiring of action view assertions with rails dom testing …
…assertions.
5ac99b0
@kaspth kaspth Changed deprecation message in dom and selector assertions in Action …
…Dispatch.
6061472
@kaspth kaspth Required rails-dom-testing in test_case.rb 5dc57db
@kaspth kaspth Removed assert_select test file, since it has been moved to rails-dom…
…-testing.
72ce9a4
@kaspth kaspth Included DomAssertions in url_helper- and atom_feed_helper_test.rb. 93f2cd8
@kaspth kaspth Removed sanitizers- and scrubbers_test.rb. They are in rails-html-san…
…itizer.
82e0705
@kaspth kaspth Fixed deprecated selector in form_collections_helper_test.rb with fro…
…m catch_invalid_selector. Sweet.
648f748
@kaspth kaspth Included rails-dom-testing in rails_info_controller_test.rb 28fd5eb
@kaspth kaspth Support for changes in SelectorAssertions. 83f1563
@kaspth kaspth Updated html-scanner deprecation message. 50347b1
@kaspth kaspth Moved html_document to ActionDispatch::Assertions. Included the Rails…
…::Dom::Testing::Assertions there as well.
9efdffe
@kaspth kaspth Completed integration of rails-html-sanitizer in SanitizeHelper. Depr…
…ecated protocol_separator accessors and bad_tags=.
38620e1
@kaspth kaspth Updated CHANGELOG message about rails-dom-testing. 51c1986
@kaspth kaspth Updated CHANGELOG message to include info about rails-html-sanitizer. 0926fa5
@kaspth kaspth Remove some whitespace in actionpack.gemspec. 5913a09
@kaspth kaspth Remove unneeded comment in test. 65d0443
@kaspth kaspth Made deprecation messages in sanitize_helper more clear. 2a7f13e
@kaspth kaspth Migrated test away from escaped quotes. dd48b0a
@kaspth kaspth Remove include of rails-dom-testing in rails_info_controller_test.rb …
…as it is included in ActionController::TestCase.
c0e1b20
@kaspth kaspth Deprecate configurations and use allowed_tags and allowed_attributes …
…on WhiteListSanitizer.
13da278
@kaspth kaspth Changed configuration documentation to no longer state it replaces a …
…Set.
7587632
@kaspth kaspth Delegate allowed tags and attributes setting to HTML::WhiteListSaniti…
…zer.
5d3a292
@kaspth kaspth Add a layer of indirection making sanitizers pluggable. 427f3f9
@kaspth kaspth Remove deprecation notice. 017ddc6
@kaspth kaspth Remove html-scanner and its tests. 33019a3
@kaspth kaspth Point gems to all the right places. 33c8bfc
@kaspth kaspth Revert some stuff to use the new sanitizers. d4cd7e2
@kaspth kaspth Change sanitizer_vendor to just be a method and reword documentation. e438c09
@kaspth kaspth Don't splat arguments to allowed tags or attributes. bcd71b4
@kaspth kaspth Fix invalid css selectors in form_collections_helper_test.rb. 01ff0f3
@kaspth kaspth Change date helper tests to expect attributes with double quoted stri…
…ngs.
cdf2f28
@kaspth kaspth Make output_buffers used in tests be utf-8 encoded. Fixing unknown en…
…coding ASCII-8BIT test errors.
6cb6290
@kaspth kaspth Add document_root_element to ActionDispatch::IntegrationTest so asser…
…t_select can be called without specifying a root.
5ffc36d
@kaspth kaspth Use 1.9 syntax. 1a8ca9f
@kaspth kaspth Remove response faking. b276108
@kaspth kaspth Inline Assertion reference. 9c9875b
@kaspth kaspth Restore delivery method on teardowns. ff1b7e7
@rafaelfranca
Member

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! ❤️

@schneems
Member

epic!

@kaspth
Member
kaspth commented Jul 10, 2014

Hell yeah!
Thanks for all the love, Rafael ❤️

Kasper

Den 10/07/2014 kl. 21.54 skrev Rafael Mendonça França notifications@github.com:

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!


Reply to this email directly or view it on GitHub.

@chancancode
Member

🤘

@mdespuits
Contributor

❤️ 💚 💛 💜 ❤️

@vipulnsward
Member

Awesome! Great work @kaspth !

@kaspth kaspth deleted the kaspth:loofah-integration branch Sep 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment