Repository: mozilla/bleach

Issue:
A cleaning pipeline
One thing that occured to me while working on the `BleachSanitizerMixin.sanitize_token` is that it's code is getting pretty long and kind of ugly, the more functionality one needs do add.

One possible way to counter this problem might be to implement a "pipeline". I've seen this concept first in [django-social-auth](https://github.com/omab/django-social-auth) which I'm also using in a project. To be more specific [this part of the README](https://github.com/omab/django-social-auth#id14) explains it pretty good. Personally I really love the concept. The code of the [default social auth pipeline is here](https://github.com/omab/django-social-auth/tree/master/social_auth/backends/pipeline)

So what I am proposing is something very much in the vain of social auth's pipeline:
1. Instead of having to tie into the (big) if condition, we iterate over an iterable with functions::
   
   ``` python
   PIPELINE = ('SkipAllowedElements', 'StripScripts',...)
   ```
2. While iterating, each function get's called and returns either a cleaned token which gets passed on to the next function in the iterable _or_ `None` for skip. Note that maybe it might be more expressive to use Exceptions (like `SkipToken`) here.

This may also clean up a bit of the code I introduce with the `strip_script_content` fork (like doing `self.previous_token = token` right before every `return`)

So, basically this is me requesting for a comment on refactoring things a bit :D – Maybe the same kind of _pipeline-logic_ could be used for #56


Discussion:

Patch:
diff --git a/bleach/__init__.py b/bleach/__init__.py
index 12788eb..04d69b0 100644
--- a/bleach/__init__.py
+++ b/bleach/__init__.py
@@ -115,9 +115,8 @@ class Cleaner(object):
 
     def __init__(self, tags=ALLOWED_TAGS, attributes=ALLOWED_ATTRIBUTES,
                  styles=ALLOWED_STYLES, protocols=ALLOWED_PROTOCOLS, strip=False,
-                 strip_comments=True):
-        """
-        :arg tags: whitelist of allowed tags; defaults to
+                 strip_comments=True, filters=None):
+        """:arg tags: whitelist of allowed tags; defaults to
             ``bleach.ALLOWED_TAGS``
 
         :arg attributes: whitelist of allowed attributes; defaults to
@@ -133,6 +132,16 @@ class Cleaner(object):
 
         :arg strip_comments: whether or not to strip HTML comments
 
+        :arg filters: list of html5lib Filter classes to pass streamed content through
+
+            See http://html5lib.readthedocs.io/en/latest/movingparts.html#filters
+
+            .. Warning::
+
+               Using filters changes the output of
+               :py:method:`bleach.Cleaner.clean`. Make sure the way the filters
+               change the output are secure.
+
         """
         self.tags = tags
         self.attributes = attributes
@@ -140,6 +149,7 @@ class Cleaner(object):
         self.protocols = protocols
         self.strip = strip
         self.strip_comments = strip_comments
+        self.filters = filters or []
 
         self.parser = html5lib.HTMLParser(namespaceHTMLElements=False)
         self.walker = html5lib.getTreeWalker('etree')
@@ -183,12 +193,16 @@ class Cleaner(object):
             allowed_svg_properties=[],
         )
 
+        # Apply any filters after the BleachSanitizerFilter
+        for filter_class in self.filters:
+            filtered = filter_class(source=filtered)
+
         return self.serializer.render(filtered)
 
 
 def clean(text, tags=ALLOWED_TAGS, attributes=ALLOWED_ATTRIBUTES,
           styles=ALLOWED_STYLES, protocols=ALLOWED_PROTOCOLS, strip=False,
-          strip_comments=True):
+          strip_comments=True, filters=None):
     """Clean an HTML fragment of malicious content and return it
 
     This function is a security-focused function whose sole purpose is to
@@ -228,6 +242,16 @@ def clean(text, tags=ALLOWED_TAGS, attributes=ALLOWED_ATTRIBUTES,
 
     :arg strip_comments: whether or not to strip HTML comments
 
+    :arg filters: list of html5lib Filter classes to pass streamed content through
+
+        See http://html5lib.readthedocs.io/en/latest/movingparts.html#filters
+
+        .. Warning::
+
+           Using filters changes the output of
+           `bleach.Cleaner.clean`. Make sure the way the filters
+           change the output are secure.
+
     :returns: cleaned text as unicode
 
     """
@@ -238,6 +262,7 @@ def clean(text, tags=ALLOWED_TAGS, attributes=ALLOWED_ATTRIBUTES,
         protocols=protocols,
         strip=strip,
         strip_comments=strip_comments,
+        filters=filters,
     )
     return cleaner.clean(text)
 
diff --git a/bleach/sanitizer.py b/bleach/sanitizer.py
index b7162ee..62bbf64 100644
--- a/bleach/sanitizer.py
+++ b/bleach/sanitizer.py
@@ -16,7 +16,6 @@ def _attr_key(attr):
 
     """
     key = (attr[0][0] or ''), attr[0][1]
-    print(key)
     return key
 
 
diff --git a/docs/clean.rst b/docs/clean.rst
index 63f0427..8e310f5 100644
--- a/docs/clean.rst
+++ b/docs/clean.rst
@@ -5,7 +5,7 @@
 ``bleach.clean()``
 ==================
 
-:py:func:`bleach.clean`` is Bleach's HTML sanitization method.
+:py:func:`bleach.clean` is Bleach's HTML sanitization method.
 
 Given a fragment of HTML, Bleach will parse it according to the HTML5 parsing
 algorithm and sanitize any disallowed tags or attributes. This algorithm also
@@ -48,13 +48,41 @@ The default value is a relatively conservative list found in
 ``bleach.ALLOWED_TAGS``.
 
 
-Attribute Whitelist
-===================
+Allowed Attributes
+==================
+
+The ``attributes`` kwarg lets you specify which attributes are allowed.
+
+The default value is also a conservative dict found in
+``bleach.ALLOWED_ATTRIBUTES``.
+
+
+As a list
+---------
+
+The ``attributes`` value can be a list, in which case the attributes are allowed
+for any tag.
+
+For example:
+
+.. doctest::
+
+   >>> import bleach
+
+   >>> bleach.clean(
+   ...     u'<p class="foo" style="color: red; font-weight: bold;">blah blah blah</p>',
+   ...     tags=['p'],
+   ...     attributes=['style'],
+   ...     styles=['color'],
+   ... )
+   u'<p style="color: red;">blah blah blah</p>'
+
+
+As a dict
+---------
 
-The ``attributes`` kwarg is a whitelist of attributes. It can be a list, in
-which case the attributes are allowed for any tag, or a dictionary, in which
-case the keys are tag names (or a wildcard: ``*`` for all tags) and the values
-are lists of allowed attributes.
+The ``attributes`` value can be a dict, in which case the keys are tag names (or
+a wildcard: ``*`` for all tags) and the values are lists of allowed attributes.
 
 For example:
 
@@ -80,23 +108,19 @@ In this case, ``class`` is allowed on any allowed element (from the ``tags``
 argument), ``<a>`` tags are allowed to have ``href`` and ``rel`` attributes,
 and so on.
 
-The default value is also a conservative dict found in
-``bleach.ALLOWED_ATTRIBUTES``.
-
 
-Callable Filters
-----------------
+Using functions
+---------------
 
-You can also use a callable (instead of a list) in the ``attributes`` kwarg. If
-the callable returns ``True``, the attribute is allowed. Otherwise, it is
-stripped. For example:
+You can also use callables. If the callable returns ``True``, the attribute is
+allowed. Otherwise, it is stripped. For example:
 
 .. doctest::
 
     >>> from urlparse import urlparse
     >>> import bleach
 
-    >>> def filter_src(name, value):
+    >>> def allow_src(name, value):
     ...     if name in ('alt', 'height', 'width'):
     ...         return True
     ...     if name == 'src':
@@ -108,7 +132,7 @@ stripped. For example:
     ...    u'<img src="http://example.com" alt="an example">',
     ...    tags=['img'],
     ...    attributes={
-    ...        'img': filter_src
+    ...        'img': allow_src
     ...    }
     ... )
     u'<img alt="an example">'
@@ -229,3 +253,47 @@ By default, Bleach will strip out HTML comments. To disable this behavior, set
 
    >>> bleach.clean(html, strip_comments=False)
    u'my<!-- commented --> html'
+
+
+html5lib Filters
+================
+
+Bleach sanitizing is implemented as an html5lib Filter. The consequence of this
+is that we can pass the streamed content through additional specified filters
+after the :py:class:`bleach.sanitizer.BleachSanitizingFilter` filter has run.
+
+This lets you add data, drop data and change data as it is being serialized back
+to a unicode.
+
+Documentation on html5lib Filters is here:
+http://html5lib.readthedocs.io/en/latest/movingparts.html#filters
+
+Trivial Filter example:
+
+.. doctest::
+
+   >>> import bleach
+   >>> from html5lib.filters.base import Filter
+
+   >>> class MooFilter(Filter):
+   ...     def __iter__(self):
+   ...         for token in Filter.__iter__(self):
+   ...             if token['type'] in ['StartTag', 'EmptyTag'] and token['data']:
+   ...                 for attr, value in token['data'].items():
+   ...                     token['data'][attr] = 'moo'
+   ...             yield token
+   ...
+   >>> ATTRS = {
+   ...     'img': ['rel', 'src']
+   ... }
+   ...
+   >>> TAGS = ['img']
+   >>> dirty = 'this is cute! <img src="http://example.com/puppy.jpg" rel="nofollow">'
+   >>> bleach.clean(dirty, tags=TAGS, attributes=ATTRS, filters=[MooFilter])
+   u'this is cute! <img rel="moo" src="moo">'
+
+
+.. Warning::
+
+   Filters change the output of cleaning. Make sure that whatever changes the
+   filter is applying maintain the safety guarantees of the output.


Test patch:
diff --git a/tests/test_basics.py b/tests/test_basics.py
index 919c867..620a42d 100644
--- a/tests/test_basics.py
+++ b/tests/test_basics.py
@@ -1,4 +1,5 @@
 import html5lib
+from html5lib.filters.base import Filter
 import pytest
 import six
 
@@ -147,14 +148,14 @@ class TestClean:
         )
 
     def test_allowed_styles(self):
-        ATTR = ['style']
+        ATTRS = ['style']
         STYLE = ['color']
         blank = '<b style=""></b>'
         s = '<b style="color: blue;"></b>'
-        assert bleach.clean('<b style="top:0"></b>', attributes=ATTR) == blank
-        assert bleach.clean(s, attributes=ATTR, styles=STYLE) == s
+        assert bleach.clean('<b style="top:0"></b>', attributes=ATTRS) == blank
+        assert bleach.clean(s, attributes=ATTRS, styles=STYLE) == s
         assert (
-            bleach.clean('<b style="top: 0; color: blue;"></b>', attributes=ATTR, styles=STYLE) ==
+            bleach.clean('<b style="top: 0; color: blue;"></b>', attributes=ATTRS, styles=STYLE) ==
             s
         )
 
@@ -165,7 +166,7 @@ class TestClean:
         assert bleach.clean(dirty, attributes=['class']) == clean
 
     def test_wildcard_attributes(self):
-        ATTR = {
+        ATTRS = {
             '*': ['id'],
             'img': ['src'],
         }
@@ -173,7 +174,7 @@ class TestClean:
         dirty = ('both <em id="foo" style="color: black">can</em> have '
                  '<img id="bar" src="foo"/>')
         assert (
-            bleach.clean(dirty, tags=TAG, attributes=ATTR) ==
+            bleach.clean(dirty, tags=TAG, attributes=ATTRS) ==
             'both <em id="foo">can</em> have <img id="bar" src="foo">'
         )
 
@@ -273,6 +274,27 @@ class TestClean:
         cleaned_href = '<a>invalid href</a>'
         assert bleach.clean(invalid_href, protocols=['my_protocol']) == cleaned_href
 
+    def test_filters(self):
+        # Create a Filter that changes all the attr values to "moo"
+        class MooFilter(Filter):
+            def __iter__(self):
+                for token in Filter.__iter__(self):
+                    if token['type'] in ['StartTag', 'EmptyTag'] and token['data']:
+                        for attr, value in token['data'].items():
+                            token['data'][attr] = 'moo'
+
+                    yield token
+
+        ATTRS = {
+            'img': ['rel', 'src']
+        }
+        TAGS = ['img']
+        dirty = 'this is cute! <img src="http://example.com/puppy.jpg" rel="nofollow">'
+        assert (
+            bleach.clean(dirty, tags=TAGS, attributes=ATTRS, filters=[MooFilter]) ==
+            'this is cute! <img rel="moo" src="moo">'
+        )
+
 
 class TestCleaner:
     def test_basics(self):