Repository: aio-libs/aiosmtpd
Pull request #80 (2017-05-03 00:47:37)

Handle UnicodeDecodeErrors
#4 with explicit handling of not-unexpected `UnicodeDecodeError`.


Patch:
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index 51c4e69..916a7d4 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -44,6 +44,7 @@ class Envelope:
     def __init__(self):
         self.mail_from = None
         self.mail_options = []
+        self.smtp_utf8 = False
         self.content = None
         self.rcpt_tos = []
         self.rcpt_options = []
@@ -80,12 +81,6 @@ class SMTP(asyncio.StreamReaderProtocol):
         self.event_handler = handler
         self.data_size_limit = data_size_limit
         self.enable_SMTPUTF8 = enable_SMTPUTF8
-        if enable_SMTPUTF8:
-            if decode_data:
-                raise ValueError(
-                    "decode_data and enable_SMTPUTF8 cannot be set to "
-                    "True at the same time")
-            decode_data = False
         self._decode_data = decode_data
         self.command_size_limits.clear()
         if hostname:
@@ -177,7 +172,6 @@ class SMTP(asyncio.StreamReaderProtocol):
     def _set_post_data_state(self):
         """Reset state variables to their post-DATA state."""
         self.envelope = self._create_envelope()
-        self.require_SMTPUTF8 = False
 
     def _set_rset_state(self):
         """Reset all state variables except the greeting."""
@@ -186,7 +180,7 @@ class SMTP(asyncio.StreamReaderProtocol):
     @asyncio.coroutine
     def push(self, status):
         response = bytes(
-            status + '\r\n', 'utf-8' if self.require_SMTPUTF8 else 'ascii')
+            status + '\r\n', 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
         self._writer.write(response)
         log.debug(response)
         yield from self._writer.drain()
@@ -213,18 +207,43 @@ class SMTP(asyncio.StreamReaderProtocol):
                 return
             try:
                 # XXX this rstrip may not completely preserve old behavior.
-                line = line.decode('utf-8').rstrip('\r\n')
+                line = line.rstrip(b'\r\n')
                 log.info('%r Data: %s', self.session.peer, line)
                 if not line:
                     yield from self.push('500 Error: bad syntax')
                     continue
-                i = line.find(' ')
+                i = line.find(b' ')
+                # Decode to string only the command name part, which must be
+                # ASCII as per RFC.  If there is an argument, it is decoded to
+                # UTF-8/surrogateescape so that non-UTF-8 data can be
+                # re-encoded back to the original bytes when the SMTP command
+                # is handled.
                 if i < 0:
-                    command = line.upper()
+                    command = line.upper().decode(encoding='ascii')
                     arg = None
                 else:
-                    command = line[:i].upper()
+                    command = line[:i].upper().decode(encoding='ascii')
                     arg = line[i+1:].strip()
+                    # Remote SMTP servers can send us UTF-8 content despite
+                    # whether they've declared to do so or not.  Some old
+                    # servers can send 8-bit data.  Use surrogateescape so
+                    # that the fidelity of the decoding is preserved, and the
+                    # original bytes can be retrieved.
+                    if self.enable_SMTPUTF8:
+                        arg = str(
+                            arg, encoding='utf-8', errors='surrogateescape')
+                    else:
+                        try:
+                            arg = str(arg, encoding='ascii', errors='strict')
+                        except UnicodeDecodeError:
+                            # This happens if enable_SMTPUTF8 is false, meaning
+                            # that the server explicitly does not want to
+                            # accept non-ASCII, but the client ignores that and
+                            # sends non-ASCII anyway.
+                            yield from self.push(
+                                '500 Error: strict ASCII mode')
+                            # Should we yield from self.handle_exception()?
+                            continue
                 max_sz = (self.command_size_limits[command]
                           if self.session.extended_smtp
                           else self.command_size_limit)
@@ -468,13 +487,14 @@ class SMTP(asyncio.StreamReaderProtocol):
                 yield from self.push(
                     '501 Error: BODY can only be one of 7BIT, 8BITMIME')
                 return
-        if self.enable_SMTPUTF8:
-            smtputf8 = params.pop('SMTPUTF8', False)
-            if smtputf8 is True:
-                self.require_SMTPUTF8 = True
-            elif smtputf8 is not False:
-                yield from self.push('501 Error: SMTPUTF8 takes no arguments')
-                return
+        smtputf8 = params.pop('SMTPUTF8', False)
+        if not isinstance(smtputf8, bool):
+            yield from self.push('501 Error: SMTPUTF8 takes no arguments')
+            return
+        if smtputf8 and not self.enable_SMTPUTF8:
+            yield from self.push('501 Error: SMTPUTF8 disabled')
+            return
+        self.envelope.smtp_utf8 = smtputf8
         size = params.pop('SIZE', None)
         if size:
             if isinstance(size, bool) or not size.isdigit():
@@ -600,7 +620,18 @@ class SMTP(asyncio.StreamReaderProtocol):
                 data[i] = text[1:]
         content = original_content = EMPTYBYTES.join(data)
         if self._decode_data:
-            content = original_content.decode('utf-8')
+            if self.enable_SMTPUTF8:
+                content = original_content.decode(
+                    'utf-8', errors='surrogateescape')
+            else:
+                try:
+                    content = original_content.decode('ascii', errors='strict')
+                except UnicodeDecodeError:
+                    # This happens if enable_smtputf8 is false, meaning that
+                    # the server explicitly does not want to accept non-ascii,
+                    # but the client ignores that and sends non-ascii anyway.
+                    yield from self.push('500 Error: strict ASCII mode')
+                    return
         self.envelope.content = content
         self.envelope.original_content = original_content
         # Call the new API first if it's implemented.


Test patch:
diff --git a/aiosmtpd/tests/test_server.py b/aiosmtpd/tests/test_server.py
index 2f3741d..c24467e 100644
--- a/aiosmtpd/tests/test_server.py
+++ b/aiosmtpd/tests/test_server.py
@@ -11,12 +11,6 @@ from smtplib import SMTP
 
 
 class TestServer(unittest.TestCase):
-    def test_constructor_contraints(self):
-        # These two arguments cannot both be set.
-        self.assertRaises(ValueError, Server, Sink(),
-                          enable_SMTPUTF8=True,
-                          decode_data=True)
-
     def test_smtp_utf8(self):
         controller = Controller(Sink())
         controller.start()
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index d37d0d8..d073511 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -19,7 +19,7 @@ BCRLF = b'\r\n'
 
 class DecodingController(Controller):
     def factory(self):
-        return Server(self.handler, decode_data=True)
+        return Server(self.handler, decode_data=True, enable_SMTPUTF8=True)
 
 
 class NoDecodeController(Controller):
@@ -35,7 +35,7 @@ class ReceivingHandler:
 
     @asyncio.coroutine
     def handle_DATA(self, server, session, envelope):
-        self.box.append(envelope.content)
+        self.box.append(envelope)
         return '250 OK'
 
 
@@ -48,6 +48,11 @@ class SizedController(Controller):
         return Server(self.handler, data_size_limit=self.size)
 
 
+class StrictASCIIController(Controller):
+    def factory(self):
+        return Server(self.handler, enable_SMTPUTF8=False, decode_data=True)
+
+
 class CustomHostnameController(Controller):
     def factory(self):
         return Server(self.handler, hostname='custom.localhost')
@@ -121,7 +126,7 @@ class TestProtocol(unittest.TestCase):
         except asyncio.CancelledError:
             pass
         self.assertEqual(len(handler.box), 1)
-        self.assertEqual(handler.box[0], data)
+        self.assertEqual(handler.box[0].content, data)
 
     def test_empty_email(self):
         handler = ReceivingHandler()
@@ -140,7 +145,7 @@ class TestProtocol(unittest.TestCase):
             pass
         self.assertEqual(self.responses[5], b'250 OK\r\n')
         self.assertEqual(len(handler.box), 1)
-        self.assertEqual(handler.box[0], b'')
+        self.assertEqual(handler.box[0].content, b'')
 
 
 class TestSMTP(unittest.TestCase):
@@ -179,7 +184,8 @@ class TestSMTP(unittest.TestCase):
             lines = response.splitlines()
             self.assertEqual(lines[0], bytes(socket.getfqdn(), 'utf-8'))
             self.assertEqual(lines[1], b'SIZE 33554432')
-            self.assertEqual(lines[2], b'HELP')
+            self.assertEqual(lines[2], b'SMTPUTF8')
+            self.assertEqual(lines[3], b'HELP')
 
     def test_ehlo_duplicate(self):
         with SMTP(*self.address) as client:
@@ -611,15 +617,31 @@ class TestSMTPWithController(unittest.TestCase):
                 b'Error: message size exceeds fixed maximum message size')
 
     def test_mail_with_compatible_smtputf8(self):
-        controller = Controller(Sink())
+        handler = ReceivingHandler()
+        controller = Controller(handler)
         controller.start()
         self.addCleanup(controller.stop)
+        recipient = 'bart\xCB@example.com'
+        sender = 'anne\xCB@example.com'
         with SMTP(controller.hostname, controller.port) as client:
             client.ehlo('example.com')
-            code, response = client.docmd(
-                'MAIL FROM: <anne@example.com> SMTPUTF8')
+            client.send(bytes(
+                'MAIL FROM: <' + sender + '> SMTPUTF8\r\n',
+                encoding='utf-8'))
+            code, response = client.getreply()
             self.assertEqual(code, 250)
             self.assertEqual(response, b'OK')
+            client.send(bytes(
+                'RCPT TO: <' + recipient + '>\r\n',
+                encoding='utf-8'))
+            code, response = client.getreply()
+            self.assertEqual(code, 250)
+            self.assertEqual(response, b'OK')
+            code, response = client.data('')
+            self.assertEqual(code, 250)
+            self.assertEqual(response, b'OK')
+        self.assertEqual(handler.box[0].rcpt_tos[0], recipient)
+        self.assertEqual(handler.box[0].mail_from, sender)
 
     def test_mail_with_unrequited_smtputf8(self):
         controller = Controller(Sink())
@@ -706,7 +728,7 @@ Testing
             mail = CRLF.join(['Test', '.', 'mail'])
             client.sendmail('anne@example.com', ['bart@example.com'], mail)
             self.assertEqual(len(handler.box), 1)
-            self.assertEqual(handler.box[0], 'Test\r\n.\r\nmail')
+            self.assertEqual(handler.box[0].content, 'Test\r\n.\r\nmail')
 
     def test_unexpected_errors(self):
         handler = ErroringHandler()
@@ -743,6 +765,32 @@ Testing
         # handle_exception() method.
         self.assertIsNone(handler.error)
 
+    def test_bad_encodings(self):
+        handler = ReceivingHandler()
+        controller = DecodingController(handler)
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.helo('example.com')
+            mail_from = b'anne\xFF@example.com'
+            mail_to = b'bart\xFF@example.com'
+            client.ehlo('test')
+            client.send(b'MAIL FROM:' + mail_from + b'\r\n')
+            code, response = client.getreply()
+            self.assertEqual(code, 250)
+            client.send(b'RCPT TO:' + mail_to + b'\r\n')
+            code, response = client.getreply()
+            self.assertEqual(code, 250)
+            client.data('Test mail')
+            self.assertEqual(len(handler.box), 1)
+            envelope = handler.box[0]
+            mail_from2 = envelope.mail_from.encode(
+                'utf-8', errors='surrogateescape')
+            self.assertEqual(mail_from2, mail_from)
+            mail_to2 = envelope.rcpt_tos[0].encode(
+                'utf-8', errors='surrogateescape')
+            self.assertEqual(mail_to2, mail_to)
+
 
 class TestCustomizations(unittest.TestCase):
     def test_custom_hostname(self):
@@ -844,3 +892,49 @@ class TestClientCrash(unittest.TestCase):
             # Don't include the CRLF.
             client.send('FOO')
             client.close()
+
+
+class TestStrictASCII(unittest.TestCase):
+    def setUp(self):
+        controller = StrictASCIIController(Sink())
+        controller.start()
+        self.addCleanup(controller.stop)
+        self.address = (controller.hostname, controller.port)
+
+    def test_ehlo(self):
+        with SMTP(*self.address) as client:
+            code, response = client.ehlo('example.com')
+            self.assertEqual(code, 250)
+            lines = response.splitlines()
+            self.assertNotIn(b'SMTPUTF8', lines)
+
+    def test_bad_encoded_param(self):
+        with SMTP(*self.address) as client:
+            client.ehlo('example.com')
+            client.send(b'MAIL FROM: <anne\xFF@example.com>\r\n')
+            code, response = client.getreply()
+            self.assertEqual(code, 500)
+            self.assertIn(b'Error: strict ASCII mode', response)
+
+    def test_mail_param(self):
+        with SMTP(*self.address) as client:
+            client.ehlo('example.com')
+            code, response = client.docmd(
+                'MAIL FROM: <anne@example.com> SMTPUTF8')
+            self.assertEqual(code, 501)
+            self.assertEqual(response, b'Error: SMTPUTF8 disabled')
+
+    def test_data(self):
+        with SMTP(*self.address) as client:
+            code, response = client.ehlo('example.com')
+            self.assertEqual(code, 250)
+            with self.assertRaises(SMTPDataError) as cm:
+                client.sendmail('anne@example.com', ['bart@example.com'], b"""\
+From: anne@example.com
+To: bart@example.com
+Subject: A test
+
+Testing\xFF
+""")
+            self.assertEqual(cm.exception.smtp_code, 500)
+            self.assertIn(b'Error: strict ASCII mode', cm.exception.smtp_error)