#19063: fix set_payload handling of non-ASCII string input.

bitdancer · bitdancer · commit 50bfbb990317 · 2013-12-11T16:52:11.000-05:00
This version of the fix raises an error instead of accepting the invalid
input (ie: if a non-ASCII string is used but no charset is specified).
diff --git a/Doc/library/email.message.rst b/Doc/library/email.message.rst
@@ -196,7 +196,13 @@ Here are the methods of the :class:`Message` class:
 
       Set the entire message object's payload to *payload*.  It is the client's
       responsibility to ensure the payload invariants.  Optional *charset* sets
-      the message's default character set; see :meth:`set_charset` for details.
+      the message's character set; see :meth:`set_charset` for details.  If
+      *payload* is a string containing non-ASCII characters, *charset* is
+      required.
+
+      .. versionchanged:: 3.4
+         Previous to 3.4 *charset* was not required when *payload* was a
+         non-ASCII string, but omitting it produced nonsense results.
 
    .. method:: set_charset(charset)
 
diff --git a/Lib/email/charset.py b/Lib/email/charset.py
@@ -378,18 +378,19 @@ def _get_encoder(self, header_bytes):
             return None
 
     def body_encode(self, string):
-        """Body-encode a string by converting it first to bytes.
+        """Body-encode a string, converting it first to bytes if needed.
 
         The type of encoding (base64 or quoted-printable) will be based on
-        self.body_encoding.  If body_encoding is None, we assume the
-        output charset is a 7bit encoding, so re-encoding the decoded
-        string using the ascii codec produces the correct string version
-        of the content.
+        self.body_encoding.  If body_encoding is None, we perform no CTE
+        encoding (the CTE will be either 7bit or 8bit), we just encode the
+        binary representation to ascii using the surrogateescape error handler,
+        which will enable the Generators to produce the correct output.
         """
-        # 7bit/8bit encodings return the string unchanged (module conversions)
+        if not string:
+            return string
+        if isinstance(string, str):
+            string = string.encode(self.output_charset)
         if self.body_encoding is BASE64:
-            if isinstance(string, str):
-                string = string.encode(self.output_charset)
             return email.base64mime.body_encode(string)
         elif self.body_encoding is QP:
             # quopromime.body_encode takes a string, but operates on it as if
@@ -398,15 +399,7 @@ def body_encode(self, string):
             # character set, then, we must turn it into pseudo bytes via the
             # latin1 charset, which will encode any byte as a single code point
             # between 0 and 255, which is what body_encode is expecting.
-            #
-            # Note that this clause doesn't handle the case of a _payload that
-            # is already bytes.  It never did, and the semantics of _payload
-            # being bytes has never been nailed down, so fixing that is a
-            # longer term TODO.
-            if isinstance(string, str):
-                string = string.encode(self.output_charset).decode('latin1')
+            string = string.decode('latin1')
             return email.quoprimime.body_encode(string)
         else:
-            if isinstance(string, str):
-                string = string.encode(self.output_charset).decode('ascii')
-            return string
+            return string.decode('ascii', 'surrogateescape')
diff --git a/Lib/email/message.py b/Lib/email/message.py
@@ -301,9 +301,23 @@ def set_payload(self, payload, charset=None):
         Optional charset sets the message's default character set.  See
         set_charset() for details.
         """
-        if isinstance(payload, bytes):
-            payload = payload.decode('ascii', 'surrogateescape')
-        self._payload = payload
+        if hasattr(payload, 'encode'):
+            if charset is None:
+                try:
+                    payload.encode('ascii', 'surrogateescape')
+                except UnicodeError:
+                    raise TypeError("charset argument must be specified"
+                        " when non-ASCII characters are used in the"
+                        " payload") from None
+                self._payload = payload
+                return
+            if not isinstance(charset, Charset):
+                charset = Charset(charset)
+            payload = payload.encode(charset.output_charset)
+        if hasattr(payload, 'decode'):
+            self._payload = payload.decode('ascii', 'surrogateescape')
+        else:
+            self._payload = payload
         if charset is not None:
             self.set_charset(charset)
 
@@ -342,7 +356,7 @@ def set_charset(self, charset):
             try:
                 cte(self)
             except TypeError:
-                self._payload = charset.body_encode(self._payload)
+                self._payload = charset.body_encode(self.get_payload(decode=True))
                 self.add_header('Content-Transfer-Encoding', cte)
 
     def get_charset(self):
diff --git a/Lib/test/test_email/test_contentmanager.py b/Lib/test/test_email/test_contentmanager.py
@@ -208,12 +208,11 @@ def test_get_text_plain_bad_utf8_quoted_printable_ignore_errors(self):
                          "Basìc tëxt.\n")
 
     def test_get_text_plain_utf8_base64_recoverable_bad_CTE_data(self):
-        m = self._str_msg(textwrap.dedent("""\
+        m = self._bytes_msg(textwrap.dedent("""\
             Content-Type: text/plain; charset="utf8"
             Content-Transfer-Encoding: base64
 
-            QmFzw6xjIHTDq3h0Lgo\xFF=
-            """))
+            QmFzw6xjIHTDq3h0Lgo""").encode('ascii') + b'\xFF=\n')
         self.assertEqual(raw_data_manager.get_content(m, errors='ignore'),
                          "Basìc tëxt.\n")
 
diff --git a/Lib/test/test_email/test_email.py b/Lib/test/test_email/test_email.py
@@ -92,6 +92,44 @@ def test_set_payload_with_charset(self):
         msg.set_payload('This is a string payload', charset)
         self.assertEqual(msg.get_charset().input_charset, 'iso-8859-1')
 
+    def test_set_payload_with_8bit_data_and_charset(self):
+        data = b'\xd0\x90\xd0\x91\xd0\x92'
+        charset = Charset('utf-8')
+        msg = Message()
+        msg.set_payload(data, charset)
+        self.assertEqual(msg['content-transfer-encoding'], 'base64')
+        self.assertEqual(msg.get_payload(decode=True), data)
+        self.assertEqual(msg.get_payload(), '0JDQkdCS\n')
+
+    def test_set_payload_with_non_ascii_and_charset_body_encoding_none(self):
+        data = b'\xd0\x90\xd0\x91\xd0\x92'
+        charset = Charset('utf-8')
+        charset.body_encoding = None # Disable base64 encoding
+        msg = Message()
+        msg.set_payload(data.decode('utf-8'), charset)
+        self.assertEqual(msg['content-transfer-encoding'], '8bit')
+        self.assertEqual(msg.get_payload(decode=True), data)
+
+    def test_set_payload_with_8bit_data_and_charset_body_encoding_none(self):
+        data = b'\xd0\x90\xd0\x91\xd0\x92'
+        charset = Charset('utf-8')
+        charset.body_encoding = None # Disable base64 encoding
+        msg = Message()
+        msg.set_payload(data, charset)
+        self.assertEqual(msg['content-transfer-encoding'], '8bit')
+        self.assertEqual(msg.get_payload(decode=True), data)
+
+    def test_set_payload_to_list(self):
+        msg = Message()
+        msg.set_payload([])
+        self.assertEqual(msg.get_payload(), [])
+
+    def test_set_payload_with_non_ascii_and_no_charset_raises(self):
+        data = b'\xd0\x90\xd0\x91\xd0\x92'.decode('utf-8')
+        msg = Message()
+        with self.assertRaises(TypeError):
+            msg.set_payload(data)
+
     def test_get_charsets(self):
         eq = self.assertEqual
 
@@ -558,20 +596,10 @@ def test_broken_base64_payload(self):
         self.assertIsInstance(msg.defects[0],
                               errors.InvalidBase64CharactersDefect)
 
-    def test_broken_unicode_payload(self):
-        # This test improves coverage but is not a compliance test.
-        # The behavior in this situation is currently undefined by the API.
-        x = 'this is a br\xf6ken thing to do'
-        msg = Message()
-        msg['content-type'] = 'text/plain'
-        msg['content-transfer-encoding'] = '8bit'
-        msg.set_payload(x)
-        self.assertEqual(msg.get_payload(decode=True),
-                         bytes(x, 'raw-unicode-escape'))
-
     def test_questionable_bytes_payload(self):
         # This test improves coverage but is not a compliance test,
-        # since it involves poking inside the black box.
+        # since it involves poking inside the black box in a way
+        # that actually breaks the model invariants.
         x = 'this is a quéstionable thing to do'.encode('utf-8')
         msg = Message()
         msg['content-type'] = 'text/plain; charset="utf-8"'
diff --git a/Misc/NEWS b/Misc/NEWS
@@ -26,6 +26,13 @@ Core and Builtins
 Library
 -------
 
+- Issue #19063: if a Charset's body_encoding was set to None, the email
+  package would generate a message claiming the Content-Transfer-Encoding
+  was 7bit, and produce garbage output for the content.  This now works.
+  A couple of other set_payload mishandlings of non-ASCII are also fixed.
+  In addition, calling set_payload with a string argument without
+  specifying a charset now raises an error (this is a new error in 3.4).
+
 - Issue #15475: Add __sizeof__ implementations for itertools objects.
 
 - Issue #19880: Fix a reference leak in unittest.TestCase. Explicitly break
