Repository: aio-libs/aiosmtpd
Pull request #30 (2016-12-27 15:05:36)

Refactor DATA method
Here is two fixes with DATA method.
1. Respect original line endings. That`s not obvious, as RFC says that line endings must be only CR LF but on practice there can be anything, and better to keep it "as is" to not confuse on email debug. Anyway it is good idea to have ability to get real original data of email. Also, email.parse_message works fine with any line endings, so there is no real need to fix them.
2. Size limit had a side effect. If limit is reached, all other lines of DATA will fallback to root handler and will be handled as syntax errors. Better to read them, but ignore.

Also I removed using of instance properties for local variables. Seems that it is migrated from state-machine implementation.

kozzztik: Hi! Sorry for long response. Fixed/updated. But seems that Travis taked not that commit during last build. Cant you rerun it? I can do it only with reopen request or fake commit and thats all worse.
ericvsmith: >> @@ -407,14 +407,17 @@ def test_deliver(self):
>                  'anne@example.com', ['bart@example.com'], self.message)
>              client.quit()
>              mock().connect.assert_called_once_with('localhost', 9025)
> +            # SMTP always fixes eols, so it must be always CRLF as delimeter
> +            msg = NEWLINE.join([
> +                'From: Anne Person <anne@example.com>',
> +                'To: Bart Person <bart@example.com>',

I think it's telling that the comment says CRLF, the code says NEWLINE, 
and Barry proposes changing it to CRNL! I'm just not sure what it tells me.

I'm as old as Barry, and in all my years I've only ever seen this as 
CRLF, although I understand CRNL is more correct.

(And I don't really care, but 'delimiter' is misspelled.)

Eric.


warsaw: On Jan 26, 2017, at 04:13 PM, ericvsmith wrote:

>I think it's telling that the comment says CRLF, the code says NEWLINE, 
>and Barry proposes changing it to CRNL! I'm just not sure what it tells me.
>
>I'm as old as Barry, and in all my years I've only ever seen this as 
>CRLF, although I understand CRNL is more correct.

It tells you I'm an idiot (or just enough older than Eric that my memory is
going).  Yes, CRLF please! :)

>(And I don't really care, but 'delimiter' is misspelled.)

Ooh, good catch, let's fix that.


kozzztik: Guys, you make me a little bit crazy ) Not sure that understood right, but check it out. Also added one more test for checking that line delimeters saved as-is (as smtplib don`t gives ability to check it).

Patch:
diff --git a/aiosmtpd/handlers.py b/aiosmtpd/handlers.py
index acb123a..2c405e7 100644
--- a/aiosmtpd/handlers.py
+++ b/aiosmtpd/handlers.py
@@ -13,11 +13,13 @@ import mailbox
 import smtplib
 
 from email import message_from_bytes, message_from_string
+from email.feedparser import NLCRE
 from public import public
 
 
+EMPTYSTRING = ''
 COMMASPACE = ', '
-NEWLINE = '\n'
+CRLF = '\r\n'
 log = logging.getLogger('mail.debug')
 
 
@@ -89,15 +91,17 @@ class Proxy:
         self._port = remote_port
 
     def process_message(self, peer, mailfrom, rcpttos, data, **kws):
-        lines = data.split('\n')
+        lines = data.splitlines(keepends=True)
         # Look for the last header
         i = 0
+        ending = CRLF
         for line in lines:                          # pragma: nobranch
-            if not line:
+            if NLCRE.match(line):
+                ending = line
                 break
             i += 1
-        lines.insert(i, 'X-Peer: %s' % peer[0])
-        data = NEWLINE.join(lines)
+        lines.insert(i, 'X-Peer: %s%s' % (peer[0], ending))
+        data = EMPTYSTRING.join(lines)
         refused = self._deliver(mailfrom, rcpttos, data)
         # TBD: what to do with refused addresses?
         log.info('we got some refusals: %s', refused)
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index f722570..9a24047 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -23,6 +23,7 @@ log = logging.getLogger('mail.log')
 
 NEWLINE = '\n'
 DATA_SIZE_DEFAULT = 33554432
+EMPTYBYTES = b''
 
 
 @public
@@ -62,7 +63,7 @@ class SMTP(asyncio.StreamReaderProtocol):
             self._dotsep = '.'
             self._newline = NEWLINE
         else:
-            self._emptystring = b''
+            self._emptystring = EMPTYBYTES
             self._linesep = b'\r\n'
             self._dotsep = ord(b'.')
             self._newline = b'\n'
@@ -144,8 +145,6 @@ class SMTP(asyncio.StreamReaderProtocol):
     def _set_rset_state(self):
         """Reset all state variables except the greeting."""
         self._set_post_data_state()
-        self.received_data = ''
-        self.received_lines = []
 
     @asyncio.coroutine
     def push(self, msg):
@@ -505,29 +504,35 @@ class SMTP(asyncio.StreamReaderProtocol):
             return
         yield from self.push('354 End data with <CR><LF>.<CR><LF>')
         data = []
-        self.num_bytes = 0
+        num_bytes = 0
+        size_exceeded = False
         while not self.connection_closed:
             line = yield from self._reader.readline()
             if line == b'.\r\n':
+                if data:
+                    data[-1] = data[-1].rstrip(b'\r\n')
                 break
-            self.num_bytes += len(line)
-            if self.data_size_limit and self.num_bytes > self.data_size_limit:
+            num_bytes += len(line)
+            if (not size_exceeded and
+                    self.data_size_limit and
+                    num_bytes > self.data_size_limit):
+                size_exceeded = True
                 yield from self.push('552 Error: Too much mail data')
-            # XXX this rstrip may not exactly preserve the old behavior
-            line = line.rstrip(b'\r\n')
-            if self._decode_data:
-                data.append(line.decode('utf-8'))
-            else:
+            if not size_exceeded:
                 data.append(line)
+        if size_exceeded:
+            self._set_post_data_state()
+            return
         # Remove extraneous carriage returns and de-transparency
         # according to RFC 5321, Section 4.5.2.
         for i in range(len(data)):
             text = data[i]
-            if text and text[0] == self._dotsep:
+            if text and text[:1] == b'.':
                 data[i] = text[1:]
-        self.received_data = self._newline.join(data)
-        args = (self.peer, self.mailfrom, self.rcpttos,
-                self.received_data)
+        received_data = EMPTYBYTES.join(data)
+        if self._decode_data:
+            received_data = received_data.decode('utf-8')
+        args = (self.peer, self.mailfrom, self.rcpttos, received_data)
         kwargs = {}
         if not self._decode_data:
             kwargs = {


Test patch:
diff --git a/aiosmtpd/tests/test_handlers.py b/aiosmtpd/tests/test_handlers.py
index ede90c9..ac517e0 100644
--- a/aiosmtpd/tests/test_handlers.py
+++ b/aiosmtpd/tests/test_handlers.py
@@ -15,6 +15,8 @@ from smtplib import SMTP, SMTPRecipientsRefused
 from tempfile import TemporaryDirectory
 from unittest.mock import call, patch
 
+CRLF = '\r\n'
+
 
 class UTF8Controller(Controller):
     def factory(self):
@@ -407,14 +409,16 @@ Testing
                 'anne@example.com', ['bart@example.com'], self.message)
             client.quit()
             mock().connect.assert_called_once_with('localhost', 9025)
+            # SMTP always fixes eols, so it must be always CRLF as delimiter
+            msg = CRLF.join([
+                'From: Anne Person <anne@example.com>',
+                'To: Bart Person <bart@example.com>',
+                'Subject: A test',
+                'X-Peer: ::1',
+                '',
+                'Testing'])
             mock().sendmail.assert_called_once_with(
-                'anne@example.com', ['bart@example.com'], """\
-From: Anne Person <anne@example.com>
-To: Bart Person <bart@example.com>
-Subject: A test
-X-Peer: ::1
-
-Testing""")
+                'anne@example.com', ['bart@example.com'], msg)
             mock().quit.assert_called_once_with()
 
     def test_recipients_refused(self):
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index 753cc4e..635176c 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -1,12 +1,17 @@
 """Test the SMTP protocol."""
 
 import socket
+import asyncio
 import unittest
 
 from aiosmtpd.controller import Controller
 from aiosmtpd.handlers import Sink
 from aiosmtpd.smtp import SMTP as Server, __ident__ as GREETING
-from smtplib import SMTP, SMTPDataError
+from smtplib import SMTP, SMTPDataError, SMTPResponseException
+from unittest.mock import Mock
+
+CRLF = '\r\n'
+BCRLF = b'\r\n'
 
 
 class UTF8Controller(Controller):
@@ -19,6 +24,16 @@ class NoDecodeController(Controller):
         return Server(self.handler, decode_data=False)
 
 
+class ReceivingHandler:
+    box = None
+
+    def __init__(self):
+        self.box = []
+
+    def process_message(self, *args, **kws):
+        self.box.append(args)
+
+
 class SizedController(Controller):
     def __init__(self, handler, size, loop=None, hostname='::0', port=8025):
         self.size = size
@@ -50,6 +65,42 @@ class ErroringHandler:
         return '499 Could not accept the message'
 
 
+class TestProtocol(unittest.TestCase):
+    def setUp(self):
+        self.transport = Mock()
+        self._old_loop = asyncio.get_event_loop()
+        self.loop = asyncio.new_event_loop()
+        asyncio.set_event_loop(self.loop)
+
+    def tearDown(self):
+        self.loop.close()
+        asyncio.set_event_loop(self._old_loop)
+
+    def _get_protocol(self, *args, **kwargs):
+        protocol = Server(*args, loop=self.loop, **kwargs)
+        protocol.connection_made(self.transport)
+        return protocol
+
+    def test_honors_mail_delimeters(self):
+        handler = ReceivingHandler()
+        data = b'test\r\nmail\rdelimeters\nsaved'
+        protocol = self._get_protocol(handler)
+        protocol.data_received(BCRLF.join([
+            b'HELO example.org',
+            b'MAIL FROM: <anne@example.com>',
+            b'RCPT TO: <anne@example.com>',
+            b'DATA',
+            data + b'\r\n.',
+            b'QUIT\r\n'
+        ]))
+        try:
+            self.loop.run_until_complete(protocol._handler_coroutine)
+        except asyncio.CancelledError:
+            pass
+        assert len(handler.box) == 1
+        assert handler.box[0][3] == data
+
+
 class TestSMTP(unittest.TestCase):
     def setUp(self):
         controller = UTF8Controller(Sink)
@@ -590,6 +641,32 @@ Testing
                 self.assertEqual(cm.exception.response,
                                  b'Could not accept the message')
 
+    def test_too_long_message_body(self):
+        controller = SizedController(Sink(), size=100)
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.helo('example.com')
+            mail = '\r\n'.join(['z' * 20] * 10)
+            with self.assertRaises(SMTPResponseException) as cm:
+                client.sendmail('anne@example.com', ['bart@example.com'], mail)
+            self.assertEqual(cm.exception.smtp_code, 552)
+            self.assertEqual(cm.exception.smtp_error,
+                             b'Error: Too much mail data')
+
+    def test_dots_escaped(self):
+        handler = ReceivingHandler()
+        controller = UTF8Controller(handler)
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.helo('example.com')
+            mail = CRLF.join(['Test', '.', 'mail'])
+            client.sendmail('anne@example.com', ['bart@example.com'], mail)
+            self.assertEqual(len(handler.box), 1)
+            mail = handler.box[0]
+            self.assertEqual(mail[3], 'Test\r\n.\r\nmail')
+
 
 class TestCustomizations(unittest.TestCase):
     def test_custom_hostname(self):