Merge pull request #106 from mailgun/sergey/html5lib

use html5lib to parse html

setup.py

@@ -29,7 +29,7 @@ class InstallCommand(install):
 
 
 setup(name='talon',
-      version='1.2.14',
+      version='1.3.0',
       description=("Mailgun library "
                    "to extract message quotations and signatures."),
       long_description=open("README.rst").read(),
@@ -53,6 +53,7 @@ setup(name='talon',
           'cchardet>=0.3.5',
           'cssselect',
           'six>=1.10.0',
+          'html5lib'
           ],
       tests_require=[
           "mock",

talon/html_quotations.py

@@ -6,6 +6,7 @@ messages (without quoted messages) from html
 from __future__ import absolute_import
 import regex as re
 
+from talon.utils import cssselect 
 
 CHECKPOINT_PREFIX = '#!%!'
 CHECKPOINT_SUFFIX = '!%!#'
@@ -78,7 +79,7 @@ def delete_quotation_tags(html_note, counter, quotation_checkpoints):
 
 def cut_gmail_quote(html_message):
     ''' Cuts the outermost block element with class gmail_quote. '''
-    gmail_quote = html_message.cssselect('div.gmail_quote')
+    gmail_quote = cssselect('div.gmail_quote', html_message)
     if gmail_quote and (gmail_quote[0].text is None or not RE_FWD.match(gmail_quote[0].text)):
         gmail_quote[0].getparent().remove(gmail_quote[0])
         return True
@@ -135,7 +136,7 @@ def cut_microsoft_quote(html_message):
 def cut_by_id(html_message):
     found = False
     for quote_id in QUOTE_IDS:
-        quote = html_message.cssselect('#{}'.format(quote_id))
+        quote = cssselect('#{}'.format(quote_id), html_message)
         if quote:
             found = True
             quote[0].getparent().remove(quote[0])

talon/quotations.py

@@ -12,7 +12,8 @@ from copy import deepcopy
 
 from lxml import html, etree
 
-from talon.utils import get_delimiter, html_to_text
+from talon.utils import (get_delimiter, html_tree_to_text,
+                         html_document_fromstring)
 from talon import html_quotations
 from six.moves import range
 import six
@@ -391,11 +392,12 @@ def _extract_from_html(msg_body):
     if msg_body.strip() == b'':
         return msg_body
 
-    msg_body = msg_body.replace(b'\r\n', b'').replace(b'\n', b'')
-    html_tree = html.document_fromstring(
-        msg_body,
-        parser=html.HTMLParser(encoding="utf-8")
-    )
+    msg_body = msg_body.replace(b'\r\n', b'\n')
+    html_tree = html_document_fromstring(msg_body)
+
+    if html_tree is None:
+        return msg_body
+
     cut_quotations = (html_quotations.cut_gmail_quote(html_tree) or
                       html_quotations.cut_zimbra_quote(html_tree) or
                       html_quotations.cut_blockquote(html_tree) or
@@ -407,8 +409,7 @@ def _extract_from_html(msg_body):
 
     number_of_checkpoints = html_quotations.add_checkpoint(html_tree, 0)
     quotation_checkpoints = [False] * number_of_checkpoints
-    msg_with_checkpoints = html.tostring(html_tree)
-    plain_text = html_to_text(msg_with_checkpoints)
+    plain_text = html_tree_to_text(html_tree)
     plain_text = preprocess(plain_text, '\n', content_type='text/html')
     lines = plain_text.splitlines()
 
@@ -431,25 +432,31 @@ def _extract_from_html(msg_body):
     return_flags = []
     process_marked_lines(lines, markers, return_flags)
     lines_were_deleted, first_deleted, last_deleted = return_flags
+
+    if not lines_were_deleted and not cut_quotations:
+        return msg_body
+
     if lines_were_deleted:
         #collect checkpoints from deleted lines
         for i in range(first_deleted, last_deleted):
             for checkpoint in line_checkpoints[i]:
                 quotation_checkpoints[checkpoint] = True
-    else:
-        if cut_quotations:
-            return html.tostring(html_tree_copy)
-        else:
-            return msg_body
 
         # Remove tags with quotation checkpoints
         html_quotations.delete_quotation_tags(
             html_tree_copy, 0, quotation_checkpoints
         )
 
+    if _readable_text_empty(html_tree_copy):
+        return msg_body
+
     return html.tostring(html_tree_copy)
 
 
+def _readable_text_empty(html_tree):
+    return not bool(html_tree_to_text(html_tree).strip())
+
+
 def is_splitter(line):
     '''
     Returns Matcher object if provided string is a splitter and
@@ -463,7 +470,7 @@ def is_splitter(line):
 
 def text_content(context):
     '''XPath Extension function to return a node text content.'''
-    return context.context_node.text_content().strip()
+    return context.context_node.xpath("string()").strip()
 
 
 def tail(context):

talon/utils.py

@@ -7,9 +7,11 @@ import chardet
 import cchardet
 import regex as re
 
-from lxml import html
+from lxml.html import html5parser
 from lxml.cssselect import CSSSelector
 
+import html5lib
+
 from talon.constants import RE_DELIMITER
 import six
 
@@ -113,29 +115,18 @@ def get_delimiter(msg_body):
     return delimiter
 
 
-def html_to_text(string):
-    """
-    Dead-simple HTML-to-text converter:
-        >>> html_to_text("one<br>two<br>three")
-        >>> "one\ntwo\nthree"
-
-    NOTES:
-        1. the string is expected to contain UTF-8 encoded HTML!
-        2. returns utf-8 encoded str (not unicode)
-    """
-    if isinstance(string, six.text_type):
-        string = string.encode('utf8')
-
-    s = _prepend_utf8_declaration(string)
-    s = s.replace(b"\n", b"")
-
-    tree = html.fromstring(s)
-
+def html_tree_to_text(tree):
     for style in CSSSelector('style')(tree):
         style.getparent().remove(style)
 
     for c in tree.xpath('//comment()'):
-        c.getparent().remove(c)
+        parent = c.getparent()
+
+        # comment with no parent does not impact produced text
+        if parent is None:
+            continue
+
+        parent.remove(c)
 
     text   = ""
     for el in tree.iter():
@@ -159,6 +150,52 @@ def html_to_text(string):
     return _encode_utf8(retval)
 
 
+def html_to_text(string):
+    """
+    Dead-simple HTML-to-text converter:
+        >>> html_to_text("one<br>two<br>three")
+        >>> "one\ntwo\nthree"
+
+    NOTES:
+        1. the string is expected to contain UTF-8 encoded HTML!
+        2. returns utf-8 encoded str (not unicode)
+        3. if html can't be parsed returns None
+    """
+    if isinstance(string, six.text_type):
+        string = string.encode('utf8')
+
+    s = _prepend_utf8_declaration(string)
+    s = s.replace(b"\n", b"")
+    tree = html_fromstring(s)
+
+    if tree is None:
+        return None
+
+    return html_tree_to_text(tree)
+
+
+def html_fromstring(s):
+    """Parse html tree from string. Return None if the string can't be parsed.
+    """
+    try:
+        return html5parser.fromstring(s, parser=_HTML5LIB_PARSER)
+    except Exception:
+        pass
+
+
+def html_document_fromstring(s):
+    """Parse html tree from string. Return None if the string can't be parsed.
+    """
+    try:
+        return html5parser.document_fromstring(s, parser=_HTML5LIB_PARSER)
+    except Exception:
+        pass
+
+
+def cssselect(expr, tree):
+    return CSSSelector(expr)(tree)
+
+
 def _contains_charset_spec(s):
     """Return True if the first 4KB contain charset spec
     """
@@ -190,5 +227,15 @@ _UTF8_DECLARATION = (b'<meta http-equiv="Content-Type" content="text/html;'
 _BLOCKTAGS  = ['div', 'p', 'ul', 'li', 'h1', 'h2', 'h3']
 _HARDBREAKS = ['br', 'hr', 'tr']
 
-
 _RE_EXCESSIVE_NEWLINES = re.compile("\n{2,10}")
+
+# html5lib is a pure-python library that conforms to the WHATWG HTML spec
+# and is not vulnarable to certain attacks common for XML libraries
+_HTML5LIB_PARSER = html5lib.HTMLParser(
+    # build lxml tree
+    html5lib.treebuilders.getTreeBuilder("lxml"),
+    # remove namespace value from inside lxml.html.html5paser element tag
+    # otherwise it yields something like "{http://www.w3.org/1999/xhtml}div"
+    # instead of "div", throwing the algo off
+    namespaceHTMLElements=False
+)

tests/html_quotations_test.py

@@ -27,7 +27,7 @@ def test_quotation_splitter_inside_blockquote():
 
 </blockquote>"""
 
-    eq_("<html><body><p>Reply</p></body></html>",
+    eq_("<html><head></head><body>Reply</body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -44,7 +44,7 @@ def test_quotation_splitter_outside_blockquote():
   </div>
 </blockquote>
 """
-    eq_("<html><body><p>Reply</p></body></html>",
+    eq_("<html><head></head><body>Reply</body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -62,7 +62,7 @@ def test_regular_blockquote():
   </div>
 </blockquote>
 """
-    eq_("<html><body><p>Reply</p><blockquote>Regular</blockquote></body></html>",
+    eq_("<html><head></head><body>Reply<blockquote>Regular</blockquote></body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -85,6 +85,7 @@ Reply
 
     reply = """
 <html>
+<head></head>
 <body>
 Reply
 
@@ -128,7 +129,7 @@ def test_gmail_quote():
     </div>
   </div>
 </div>"""
-    eq_("<html><body><p>Reply</p></body></html>",
+    eq_("<html><head></head><body>Reply</body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -139,7 +140,7 @@ def test_gmail_quote_compact():
                '<div>Test</div>' \
                '</div>' \
                '</div>'
-    eq_("<html><body><p>Reply</p></body></html>",
+    eq_("<html><head></head><body>Reply</body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -166,7 +167,7 @@ def test_unicode_in_reply():
   Quote
 </blockquote>""".encode("utf-8")
 
-    eq_("<html><body><p>Reply&#160;&#160;Text<br></p><div><br></div>"
+    eq_("<html><head></head><body>Reply&#160;&#160;Text<br><div><br></div>"
         "</body></html>",
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
@@ -192,6 +193,7 @@ def test_blockquote_disclaimer():
 
     stripped_html = """
 <html>
+  <head></head>
   <body>
   <div>
     <div>
@@ -223,7 +225,7 @@ def test_date_block():
   </div>
 </div>
 """
-    eq_('<html><body><div>message<br></div></body></html>',
+    eq_('<html><head></head><body><div>message<br></div></body></html>',
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -240,7 +242,7 @@ Subject: You Have New Mail From Mary!<br><br>
 text
 </div></div>
 """
-    eq_('<html><body><div>message<br></div></body></html>',
+    eq_('<html><head></head><body><div>message<br></div></body></html>',
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -258,7 +260,7 @@ def test_reply_shares_div_with_from_block():
 
   </div>
 </body>'''
-    eq_('<html><body><div>Blah<br><br></div></body></html>',
+    eq_('<html><head></head><body><div>Blah<br><br></div></body></html>',
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -269,13 +271,13 @@ def test_reply_quotations_share_block():
 
 
 def test_OLK_SRC_BODY_SECTION_stripped():
-    eq_('<html><body><div>Reply</div></body></html>',
+    eq_('<html><head></head><body><div>Reply</div></body></html>',
         RE_WHITESPACE.sub(
             '', quotations.extract_from_html(OLK_SRC_BODY_SECTION)))
 
 
 def test_reply_separated_by_hr():
-    eq_('<html><body><div>Hi<div>there</div></div></body></html>',
+    eq_('<html><head></head><body><div>Hi<div>there</div></div></body></html>',
         RE_WHITESPACE.sub(
             '', quotations.extract_from_html(REPLY_SEPARATED_BY_HR)))
 
@@ -296,7 +298,7 @@ Reply
   </div>
 </div>
 '''
-    eq_('<html><body><p>Reply</p><div><hr></div></body></html>',
+    eq_('<html><head></head><body>Reply<div><hr></div></body></html>',
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
 
 
@@ -356,7 +358,8 @@ def test_CRLF():
     assert_false(symbol in extracted)
     eq_('<html></html>', RE_WHITESPACE.sub('', extracted))
 
-    msg_body = """Reply
+    msg_body = """My
+reply
 <blockquote>
 
   <div>
@@ -371,8 +374,8 @@ def test_CRLF():
     msg_body = msg_body.replace('\n', '\r\n')
     extracted = quotations.extract_from_html(msg_body)
     assert_false(symbol in extracted)    
-    eq_("<html><body><p>Reply</p></body></html>",
-        RE_WHITESPACE.sub('', extracted))
+    # Keep new lines otherwise "My reply" becomes one word - "Myreply" 
+    eq_("<html><head></head><body>My\nreply\n</body></html>", extracted)
 
 
 def test_gmail_forwarded_msg():
@@ -392,3 +395,27 @@ def test_too_large_html():
                '</div>'
     eq_(RE_WHITESPACE.sub('', msg_body),
         RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
+
+
+def test_readable_html_empty():
+    msg_body = """
+<blockquote>
+  Reply
+  <div>
+    On 11-Apr-2011, at 6:54 PM, Bob &lt;bob@example.com&gt; wrote:
+  </div>
+
+  <div>
+    Test
+  </div>
+
+</blockquote>"""
+
+    eq_(RE_WHITESPACE.sub('', msg_body),
+        RE_WHITESPACE.sub('', quotations.extract_from_html(msg_body)))
+
+
+@patch.object(quotations, 'html_document_fromstring', Mock(return_value=None))
+def test_bad_html():
+    bad_html = "<html></html>"
+    eq_(bad_html, quotations.extract_from_html(bad_html))

tests/utils_test.py

@@ -6,6 +6,7 @@ from . import *
 from talon import utils as u
 import cchardet
 import six
+from lxml import html
 
 
 def test_get_delimiter():
@@ -107,3 +108,26 @@ font: 13px 'Lucida Grande', Arial, sans-serif;
 <p>TEXT 2 <!-- COMMENT 2 --></p>
 </div>"""
     eq_(b'TEXT 1 \nTEXT 2', u.html_to_text(html))
+
+
+def test_comment_no_parent():
+    s = "<!-- COMMENT 1 --> no comment"
+    d = u.html_document_fromstring(s)
+    eq_("no comment", u.html_tree_to_text(d))
+
+
+@patch.object(u.html5parser, 'fromstring', Mock(side_effect=Exception()))
+def test_html_fromstring_exception():
+    eq_(None, u.html_fromstring("<html></html>"))
+
+
+@patch.object(u.html5parser, 'document_fromstring')
+def test_html_document_fromstring_exception(document_fromstring):
+    document_fromstring.side_effect = Exception()
+    eq_(None, u.html_document_fromstring("<html></html>"))
+
+
+@patch.object(u, 'html_fromstring', Mock(return_value=None))
+def test_bad_html_to_text():
+    bad_html = "one<br>two<br>three"
+    eq_(None, u.html_to_text(bad_html))