TypeError: unicode argument expected, got 'str'

[Ryan J Ollos]

Found in the logs,

[pid 24004 139801362245376] 2017-06-19 08:03:12,877 Trac[formatter] ERROR: Processor html failed for <Resource u'wiki:SandBox@165'>:
Traceback (most recent call last):
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/trac/wiki/formatter.py", line 1195, in _exec_processor
    return processor.process(text)
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/trac/wiki/formatter.py", line 375, in process
    text = self.processor(text)
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/trac/wiki/formatter.py", line 228, in _html_processor
    return self._sanitizer.sanitize(text)
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/trac/util/html.py", line 645, in sanitize
    transform.feed(html)
  File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
    self.goahead(0)
  File "/usr/lib/python2.7/HTMLParser.py", line 222, in goahead
    self.handle_data("&")
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/trac/util/html.py", line 1012, in handle_data
    self.out.write(data)
TypeError: unicode argument expected, got 'str'

I've been seeing the error in the logs for a week or two, but until we upgraded to the latest Trac trunk with improved logging, couldn't see that the exception was being raised from SandBox@165.

[Ryan J Ollos]

Issue also seen in comment:description:ticket:2558.

[Jun Omae]

TracHTMLSanitizer with html special characters leads this issue:

>>> from trac.util.html import TracHTMLSanitizer
>>> sanitizer = TracHTMLSanitizer()
>>> sanitizer.sanitize(u'<div> &amp; </div>')
Markup(u'<div> &amp; </div>')
>>> sanitizer.sanitize(u'<div> & </div>')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/html.py", line 645, in sanitize
    transform.feed(html)
  File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
    self.goahead(0)
  File "/usr/lib/python2.7/HTMLParser.py", line 222, in goahead
    self.handle_data("&")
  File "trac/util/html.py", line 1012, in handle_data
    self.out.write(data)
TypeError: unicode argument expected, got 'str'
>>> sanitizer.sanitize(u'<div> < </div>')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/html.py", line 645, in sanitize
    transform.feed(html)
  File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
    self.goahead(0)
  File "/usr/lib/python2.7/HTMLParser.py", line 171, in goahead
    self.handle_data("<")
  File "trac/util/html.py", line 1012, in handle_data
    self.out.write(data)
TypeError: unicode argument expected, got 'str'
>>> sanitizer.sanitize(u'<div> > </div>')
Markup(u'<div> > </div>')

I think we should add test cases for sanitizer with malformed html.

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Seems to be an issue only on trunk.

HTMLParser ​passes a string type:

                    self.handle_data("&")

It seems that the issue is fundamentally:

>>> import io
>>> s = io.StringIO()
>>> s.write('&')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    s.write('&')
TypeError: unicode argument expected, got 'str'
>>> s.write(u'&')
1L

… and probably remedied by using StringIO.StringIO rather than io.StringIO. However, using io module is good for forward-compatibility, looking towards Python3.

Does the following patch make any sense?:

trac/util/html.py

@@ -641 +641 @@
         :rtype: Markup
 
         """
-        transform = HTMLSanitization(self, io.StringIO())
+        class DecodingStringIO(io.StringIO):
+            def write(self, content):
+                super(DecodingStringIO, self).write(unicode(content))
+
+        transform = HTMLSanitization(self, DecodingStringIO())
         transform.feed(html)
         transform.close()
         return Markup(transform.out.getvalue())

[Jun Omae]

Subclasses of HTMLTransform have the same issue (FormTokenInjector, HTMLSanitization).

I think it would be good to convert bytes to unicode before call self.out.write() in this case like this:

diff --git a/trac/util/html.py b/trac/util/html.py
index 57c9475b4..da6092114 100644
--- a/trac/util/html.py
+++ b/trac/util/html.py
@@ -911,33 +911,44 @@ class HTMLTransform(HTMLParser):
     def __init__(self, out):
         HTMLParser.__init__(self)
         self.out = out
+        if isinstance(out, io.TextIOBase):
+            self._convert = lambda v: v.decode('utf-8') \
+                                      if isinstance(v, bytes) else v
+        elif isinstance(out, io.IOBase):
+            self._convert = lambda v: v.encode('utf-8') \
+                                      if isinstance(v, unicode) else v
+        else:
+            self._convert = lambda v: v

 ...
+
+    def _write(self, data):
+        self.out.write(self._convert(data))

See [9665d5e51/jomae.git].

I noticed another issue that TracHTMLSanitizer with Genshi sanitizes unescaped & character to & but TracHTMLSanitizer without Genshi doesn't. I think unescaped & character should be sanitized to &. However, I have no idea to resolve it.

class TracHTMLSanitizerTestCase(unittest.TestCase):

    ...

    def test_special_characters(self):
        self.assertEqual(u'<p> & </p>', self.sanitize(u'<p> & </p>'))
        self.assertEqual(u'<p> < </p>', self.sanitize(u'<p> < </p>'))
        self.assertEqual(u'<p> > </p>', self.sanitize(u'<p> > </p>'))


if genshi:
    class TracHTMLSanitizerLegacyGenshiTestCase(TracHTMLSanitizerTestCase):
        def sanitize(self, html):
            sanitizer = TracHTMLSanitizer(safe_schemes=self.safe_schemes,
                                          safe_origins=self.safe_origins)
            return unicode(HTML(html, encoding='utf-8') | sanitizer)

        def test_special_characters(self):
            self.assertEqual(u'<p> &amp; </p>', self.sanitize(u'<p> & </p>'))
            self.assertEqual(u'<p> &lt; </p>', self.sanitize(u'<p> < </p>'))
            self.assertEqual(u'<p> &gt; </p>', self.sanitize(u'<p> > </p>'))

[Ryan J Ollos]

Replying to Jun Omae:

See [9665d5e51/jomae.git].

Those changes look good.

I noticed another issue that TracHTMLSanitizer with Genshi sanitizes unescaped & character to & but TracHTMLSanitizer without Genshi doesn't. I think unescaped & character should be sanitized to &. However, I have no idea to resolve it.

Can we escape the content in handle_data?

trac/util/html.py

@@ -1020 +1020 @@
 
     def handle_data(self, data):
         if not self.waiting_for:
-            self._write(data)
+            self._write(escape(data))
 
     def handle_endtag(self, tag):
         if self.waiting_for:

[Jun Omae]

I noticed another issue on trunk@16580 while trying your suggestion.

title="&" in attribute is sanitized to title="&":

>>> from trac.util.html import TracHTMLSanitizer
>>> sanitizer = TracHTMLSanitizer()
>>> sanitizer.sanitize(u'<p title="&"> &amp;amp; </p>')
Markup(u'<p title="&amp;"> &amp;amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;"> &amp; </p>')
Markup(u'<p title="&amp;"> &amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;amp;"> &amp;amp; </p>')
Markup(u'<p title="&amp;"> &amp;amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;amp;amp;"> &amp;amp; </p>')
Markup(u'<p title="&amp;amp;"> &amp;amp; </p>')

[Ryan J Ollos]

Replying to Jun Omae:

title="&" in attribute is sanitized to title="&":

The call sequence in HTMLParser is feed → goahead → parse_starttag → unescape. Then the unescaped data is passed → HTMLParser.handle_starttag → HTMLSanitization._handle_start: trunk/trac/util/html.py@16580:980-983#L973​.

With & in the title attribute we get:

• after unescape: &
• self.sanitizer.sanitize_attrs maps: [('title', '&')] to [('title', '&')]
• the values are escaped again to create html_attrs: [('title', '&')]

So maybe the problem is in TracHTMLSanitizer.sanitize_attrs​?

I can't say it's the right solution because I don't understand the code well enough, but this patch fixes the comment:9 issue:

trac/util/html.py

@@ -761 +761 @@
         """
         new_attrs = {}
         for attr, value in attrs.iteritems():
-            value = stripentities(value) if value is not None else attr
+            value = stripentities(value, keepxmlentities=True) \
+                    if value is not None else attr
             if attr not in self.safe_attrs:
                 continue
             elif attr in self.uri_attrs:

>>> from trac.util.html import TracHTMLSanitizer
>>> sanitizer = TracHTMLSanitizer()
>>> sanitizer.sanitize(u'<p title="&"> &amp;amp; </p>')
Markup(u'<p title="&amp;"> &amp;amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;"> &amp; </p>')
Markup(u'<p title="&amp;"> &amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;amp;"> &amp;amp; </p>')
Markup(u'<p title="&amp;amp;"> &amp;amp; </p>')
>>> sanitizer.sanitize(u'<p title="&amp;amp;amp;"> &amp;amp; </p>')
Markup(u'<p title="&amp;amp;amp;"> &amp;amp; </p>')

All tests pass with the comment:8 patch and the above patch, but maybe we just don't have enough test coverage to be sure.

trac/util/html.py

@@ -761 +761 @@
         """
         new_attrs = {}
         for attr, value in attrs.iteritems():
-            value = stripentities(value) if value is not None else attr
+            value = stripentities(value, keepxmlentities=True) \
+                    if value is not None else attr
             if attr not in self.safe_attrs:
                 continue
             elif attr in self.uri_attrs:
@@ -936 +937 @@
         self.out.write('<?%s?>' % data)
 
     def handle_data(self, data):
-        self.out.write(data)
+        self.out.write(escape(data))
 
     def handle_endtag(self, tag):
         self.out.write('</' + tag + '>')
@@ -1011 +1012 @@
 
     def handle_data(self, data):
         if not self.waiting_for:
-            self.out.write(data)
+            self.out.write(escape(data))
 
     def handle_endtag(self, tag):
         if self.waiting_for:

[Jun Omae]

Replying to Ryan J Ollos:

Replying to Jun Omae:

title="&" in attribute is sanitized to title="&":

The call sequence in HTMLParser is feed → goahead → parse_starttag → unescape. Then the unescaped data is passed → HTMLParser.handle_starttag → HTMLSanitization._handle_start: trunk/trac/util/html.py@16580:980-983#L973​.

With & in the title attribute we get:

• after unescape: &
• self.sanitizer.sanitize_attrs maps: [('title', '&')] to [('title', '&')]
• the values are escaped again to create html_attrs: [('title', '&')]

So maybe the problem is in TracHTMLSanitizer.sanitize_attrs​?

HTMLParser.unescape decodes entity reference at ​https://github.com/python/cpython/blob/v2.7.14/Lib/HTMLParser.py#L472. I consider no need to call stripentities in TracHTMLSanitizer.sanitize_attrs.

>>> from HTMLParser import HTMLParser
>>> HTMLParser().unescape('&')
u'&'
>>> HTMLParser().unescape('…')
u'\u2026'
>>> 
>>> from trac.util.html import TracHTMLSanitizer
>>> sanitizer = TracHTMLSanitizer()
>>> sanitizer.sanitize(u'<p title="&amp;hellip;">....</p>')
Markup(u'<p title="\u2026">....</p>')

Also, keepxmlentities=True of stripentities has an issue. It is easy to bypass using character reference with code point.

>>> stripentities('& & & …')
u'& & & \u2026'
>>> stripentities('& & & …', keepxmlentities=True)
u'& & & \u2026'

[Jun Omae]

HTMLParser crashes when large integer in character reference is used. I think we should override unescape to fix it in HTMLTransform which is a subclass of HTMLParser.

>>> sanitizer.sanitize(u'<p title="&#xffffffff;">...</p>')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/html.py", line 647, in sanitize
    transform.feed(html)
  File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
    self.goahead(0)
  File "/usr/lib/python2.7/HTMLParser.py", line 161, in goahead
    k = self.parse_starttag(i)
  File "/usr/lib/python2.7/HTMLParser.py", line 308, in parse_starttag
    attrvalue = self.unescape(attrvalue)
  File "/usr/lib/python2.7/HTMLParser.py", line 475, in unescape
    return re.sub(r"&(#?[xX]?(?:[0-9a-fA-F]+|\w{1,8}));", replaceEntities, s)
  File "/home/jun66j5/venv/py27/lib/python2.7/re.py", line 155, in sub
    return _compile(pattern, flags).sub(repl, string, count)
  File "/usr/lib/python2.7/HTMLParser.py", line 459, in replaceEntities
    return unichr(c)
OverflowError: signed integer is greater than maximum
>>> sanitizer.sanitize(u'<p title="&#x1fffff;">...</p>')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/util/html.py", line 647, in sanitize
    transform.feed(html)
  File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
    self.goahead(0)
  File "/usr/lib/python2.7/HTMLParser.py", line 161, in goahead
    k = self.parse_starttag(i)
  File "/usr/lib/python2.7/HTMLParser.py", line 327, in parse_starttag
    self.handle_starttag(tag, attrs)
  File "trac/util/html.py", line 987, in handle_starttag
    self._handle_start(tag, attrs, '')
  File "trac/util/html.py", line 980, in _handle_start
    new_attrs = self.sanitizer.sanitize_attrs(tag, dict(attrs))
  File "trac/util/html.py", line 764, in sanitize_attrs
    value = stripentities(value) if value is not None else attr
  File "trac/util/html.py", line 195, in stripentities
    return _STRIPENTITIES_RE.sub(_replace_entity, text)
  File "trac/util/html.py", line 181, in _replace_entity
    return unichr(ref)
ValueError: unichr() arg not in range(0x110000) (wide Python build)

On Windows (narrow build), <p title="&#x10000;">` can lead the same issue.

[Jun Omae]

Replying to Jun Omae:

HTMLParser crashes when large integer in character reference is used. I think we should override unescape to fix it in HTMLTransform which is a subclass of HTMLParser.

In [3540fdba4/jomae.git], validate character and entity references using handle_charref and handle_entityref of HTMLTransform rather than unescape.

Revised jomae.git@t12847.1.

However, I noticed Genshi has two issues at least and have no idea to work around.

1. genshi.HTML incorrectly returns \u2026 for … in attribute. Works fine for … in element.

   >>> import genshi
   >>> genshi.__version__
   '0.7'
   >>> from genshi import HTML
   >>> for event in HTML('<img title="&amp;hellip;&amp;"/>', encoding='utf-8'): event
   ...
   ('START', (QName('img'), Attrs([(QName('title'), u'\u2026&')])), (None, 1, 0))
   ('END', QName('img'), (None, 1, 0))
   >>> for event in HTML('<p>&amp;hellip;&amp;</p>', encoding='utf-8'): event
   ...
   ('START', (QName('p'), Attrs()), (None, 1, 0))
   ('TEXT', u'&hellip;&', (None, 1, 3))
   ('END', QName('p'), (None, 1, 20))
   

2. Character reference with out of [0, 0x10ffff] leads ValueError (e.g. �).

   >>> HTML('<img title="&#x110000;"/>', encoding='utf-8')
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 442, in HTML
       return Stream(list(HTMLParser(BytesIO(text), encoding=encoding)))
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/core.py", line 273, in _ensure
       event = stream.next()
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 449, in _coalesce
       for kind, data, pos in chain(stream, [(None, None, None)]):
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 338, in _generate
       self.feed(data)
     File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
       self.goahead(0)
     File "/usr/lib/python2.7/HTMLParser.py", line 161, in goahead
       k = self.parse_starttag(i)
     File "/usr/lib/python2.7/HTMLParser.py", line 325, in parse_starttag
       self.handle_startendtag(tag, attrs)
     File "/usr/lib/python2.7/HTMLParser.py", line 407, in handle_startendtag
       self.handle_starttag(tag, attrs)
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 370, in handle_starttag
       fixed_attrib.append((QName(name), stripentities(value)))
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/util.py", line 227, in stripentities
       return _STRIPENTITIES_RE.sub(_replace_entity, text)
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/util.py", line 215, in _replace_entity
       return unichr(ref)
   ValueError: unichr() arg not in range(0x110000) (wide Python build)
   >>> HTML('<p>&#x110000;</p>', encoding='utf-8')
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 442, in HTML
       return Stream(list(HTMLParser(BytesIO(text), encoding=encoding)))
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/core.py", line 273, in _ensure
       event = stream.next()
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 449, in _coalesce
       for kind, data, pos in chain(stream, [(None, None, None)]):
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 338, in _generate
       self.feed(data)
     File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed
       self.goahead(0)
     File "/usr/lib/python2.7/HTMLParser.py", line 191, in goahead
       self.handle_charref(name)
     File "/dev/shm/venv/local/lib/python2.7/site-packages/genshi/input.py", line 391, in handle_charref
       text = unichr(int(name[1:], 16))
   ValueError: unichr() arg not in range(0x110000) (wide Python build)
   

[Jun Omae]

Replying to Jun Omae:

HTMLParser crashes when large integer in character reference is used. I think we should override unescape to fix it in HTMLTransform which is a subclass of HTMLParser.

Updated jomae.git@t12847.1 to fix these cases.

[Ryan J Ollos]

This looks like a huge amount of work. I'm glad you understand all these details because I do not.

It looks like we need a modification of test cases for narrow Python builds: test_failures.txt​.

I have Python installed via pyenv on OSX:

$ python --version
Python 2.7.14
$ python -c "import sysconfig; print(sysconfig.get_config_var('Py_UNICODE_SIZE'))"
2

[Jun Omae]

Thanks for the testing. Updated jomae.git@t12847.1 which is verified on ​travis-ci and ​appveyor.

I'm considering that we could backport fixes for TracHTMLSanitizer using Genshi (described in comment:13) on 1.0-stable and 1.2-stable. Should we backport the fixes?

[Ryan J Ollos]

Replying to Jun Omae:

I'm considering that we could backport fixes for TracHTMLSanitizer using Genshi (described in comment:13) on 1.0-stable and 1.2-stable. Should we backport the fixes?

I'm certainly not against the idea, if you want to prepare the changes. However, I can't say whether it's definitely needed or not.

[Jun Omae]

Replying to Ryan J Ollos:

Replying to Jun Omae:

I'm considering that we could backport fixes for TracHTMLSanitizer using Genshi (described in comment:13) on 1.0-stable and 1.2-stable. Should we backport the fixes?

I'm certainly not against the idea, if you want to prepare the changes. However, I can't say whether it's definitely needed or not.

Okay. I choose not to consider the backport since the issues are not critical and no one notice the issues until now. We'll consider the backport when anyone report the issues of 1.2-stable.

[Ryan J Ollos]

Looks good to commit!

[Jun Omae]

Thanks for the reviewing! Committed in [16613].