Merge lp:~mbp/subunit/505078-chunked into lp:~subunit/subunit/trunk

[Martin Pool]

This makes subunit not barf if a \r is missing from the input. You can choose strict or non-strict parsing.

[Martin Pool (mbp)]

Robert said offline: he would like a 'strict' mode flag, defaulting to on, in the chunked parser that controls whether the parser knowingly accepts noncompliant data.

The presence of this flag ought not to imply that it strictly validates the input. There are many things that are incorrect that it will accept at the moment. But it could go towards that in future.

review: Needs Fixing

[Jonathan Lange (jml)]

Thanks Martin.

I don't like raising ValueError, but that's essentially what happens now anyway, so it's fine by me.

I've merged a version of your patch that changes 'cr' to 'CR' in comments (sometimes a man must take a stand) and that updates NEWS.

review: Approve

=== modified file 'python/subunit/chunked.py'
--- python/subunit/chunked.py	2009-10-13 04:52:06 +0000
+++ python/subunit/chunked.py	2011-01-28 01:08:53 +0000
@@ -1,6 +1,7 @@
 #
 #  subunit: extensions to python unittest to get test results from subprocesses.
 #  Copyright (C) 2005  Robert Collins <[e-mail]>
+#  Copyright (C) 2011  Martin Pool <[e-mail]>
 #
 #  Licensed under either the Apache License, Version 2.0 or the BSD 3-clause
 #  license at the users choice. A copy of both licenses are available in the
@@ -19,7 +20,7 @@
 class Decoder(object):
     """Decode chunked content to a byte stream."""
 
-    def __init__(self, output):
+    def __init__(self, output, strict=True):
         """Create a decoder decoding to output.
 
         :param output: A file-like object. Bytes written to the Decoder are
@@ -29,11 +30,18 @@
             when no more data is available, to detect short streams; the
             write method will return none-None when the end of a stream is
             detected.
+
+        :param strict: If True (the default), the decoder will not knowingly
+            accept input that is not conformant to the HTTP specification.
+            (This does not imply that it will catch every nonconformance.)
+            If False, it will accept incorrect input that is still 
+            unambiguous.
         """
         self.output = output
         self.buffered_bytes = []
         self.state = self._read_length
         self.body_length = 0
+        self.strict = strict
 
     def close(self):
         """Close the decoder.
@@ -87,7 +95,12 @@
         if count_chars[-1][-1] != '\n':
             return
         count_str = ''.join(count_chars)
-        self.body_length = int(count_str[:-2], 16)
+        if self.strict:
+            if count_str[-2:] != '\r\n':
+                raise ValueError("chunk header invalid: %r" % count_str)
+            if '\r' in count_str[:-2]:
+                raise ValueError("too many crs in chunk header %r" % count_str)
+        self.body_length = int(count_str.rstrip('\n\r'), 16)
         excess_bytes = len(count_str)
         while excess_bytes:
             if excess_bytes >= len(self.buffered_bytes[0]):

=== modified file 'python/subunit/tests/test_chunked.py'
--- python/subunit/tests/test_chunked.py	2009-10-13 04:52:06 +0000
+++ python/subunit/tests/test_chunked.py	2011-01-28 01:08:53 +0000
@@ -1,6 +1,7 @@
 #
 #  subunit: extensions to python unittest to get test results from subprocesses.
 #  Copyright (C) 2005  Robert Collins <[e-mail]>
+#  Copyright (C) 2011  Martin Pool <[e-mail]>
 #
 #  Licensed under either the Apache License, Version 2.0 or the BSD 3-clause
 #  license at the users choice. A copy of both licenses are available in the
@@ -86,6 +87,29 @@
         self.assertEqual('', self.decoder.write('0\r\n'))
         self.assertEqual('1' * 65536 + '2' * 65536, self.output.getvalue())
 
+    def test_decode_newline_nonstrict(self):
+        """Tolerate chunk markers with no cr character."""
+        # From <http://pad.lv/505078>
+        self.decoder = subunit.chunked.Decoder(self.output, strict=False)
+        self.assertEqual(None, self.decoder.write('a\n'))
+        self.assertEqual(None, self.decoder.write('abcdeabcde'))
+        self.assertEqual('', self.decoder.write('0\n'))
+        self.assertEqual('abcdeabcde', self.output.getvalue())
+
+    def test_decode_strict_newline_only(self):
+        """Reject chunk markers with no cr character in strict mode."""
+        # From <http://pad.lv/505078>
+        self.assertRaises(ValueError,
+            self.decoder.write, 'a\n')
+
+    def test_decode_strict_multiple_crs(self):
+        self.assertRaises(ValueError,
+            self.decoder.write, 'a\r\r\n')
+
+    def test_decode_short_header(self):
+        self.assertRaises(ValueError,
+            self.decoder.write, '\n')
+
 
 class TestEncode(unittest.TestCase):