Better handle invalid character encodings in process_email and feedback_email commands. Add tests of this using stdin. Commit ready for merge.

 - Legacy-Id: 19694

Author: jennifer-richards

ietf/ipr/management/commands/process_email.py

@@ -23,12 +23,9 @@ def add_arguments(self, parser):
 
     def handle(self, *args, **options):
         email = options.get('email', None)
-        if not email:
-            msg = sys.stdin.read()
-            self.msg_bytes = msg.encode()
-        else:
-            self.msg_bytes = io.open(email, "rb").read()
-            msg = self.msg_bytes.decode()
+        binary_input = io.open(email, 'rb') if email else sys.stdin.buffer
+        self.msg_bytes = binary_input.read()
+        msg = self.msg_bytes.decode()
 
         try:
             process_response_email(msg)

ietf/ipr/management/tests.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 """Tests of ipr management commands"""
 import mock
+import sys
 
 from django.core.management import call_command
 from django.test.utils import override_settings
@@ -26,7 +27,7 @@ def test_process_email(self, process_mock):
     @mock.patch('ietf.utils.management.base.send_smtp')
     @mock.patch('ietf.ipr.management.commands.process_email.process_response_email')
     def test_send_error_to_admin(self, process_mock, send_smtp_mock):
-        """The process_email command should email the admins on error"""
+        """The process_email command should email the admins on error in process_response_email"""
         # arrange an mock error during processing
         process_mock.side_effect = RuntimeError('mock error')
 
@@ -47,3 +48,32 @@ def test_send_error_to_admin(self, process_mock, send_smtp_mock):
         self.assertIn('mock.py', content, 'File where error occurred should be included in error email')
         self.assertIn('traceback', traceback.lower(), 'Traceback should be attached to error email')
         self.assertEqual(original, 'contents', 'Original message should be attached to error email')
+
+    @mock.patch('ietf.utils.management.base.send_smtp')
+    @mock.patch('ietf.ipr.management.commands.process_email.process_response_email')
+    def test_invalid_character_encodings(self, process_mock, send_smtp_mock):
+        """The process_email command should attach messages with invalid encoding when using a file input"""
+        invalid_characters = b'\xfe\xff'
+        with name_of_file_containing(invalid_characters, mode='wb') as filename:
+            call_command('process_email', email_file=filename)
+
+        self.assertFalse(process_mock.called)  # should not even try to process illegally encoded messages
+        self.assertTrue(send_smtp_mock.called)
+        (msg,) = send_smtp_mock.call_args.args
+        parts = msg.get_payload()
+        self.assertEqual(len(parts), 3, 'Error email should contain message, traceback, and original message')
+
+    @mock.patch.object(sys.stdin.buffer, 'read')
+    @mock.patch('ietf.utils.management.base.send_smtp')
+    @mock.patch('ietf.ipr.management.commands.process_email.process_response_email')
+    def test_invalid_character_encodings_via_stdin(self, process_mock, send_smtp_mock, stdin_read_mock):
+        """The process_email command should attach messages with invalid encoding when using stdin"""
+        invalid_characters = b'\xfe\xff'
+        stdin_read_mock.return_value = invalid_characters
+        call_command('process_email')
+
+        self.assertFalse(process_mock.called)  # should not even try to process illegally encoded messages
+        self.assertTrue(send_smtp_mock.called)
+        (msg,) = send_smtp_mock.call_args.args
+        parts = msg.get_payload()
+        self.assertEqual(len(parts), 3, 'Error email should contain message, traceback, and original message')

ietf/nomcom/management/commands/feedback_email.py

@@ -42,10 +42,8 @@ def handle(self, *args, **options):
         except NomCom.DoesNotExist:
             raise CommandError("NomCom %s does not exist or it isn't active" % year)
 
-        if not email:
-            self.msg = io.open(sys.stdin.fileno(), 'rb').read()
-        else:
-            self.msg = io.open(email, "rb").read()
+        binary_input = io.open(email, 'rb') if email else sys.stdin.buffer
+        self.msg = binary_input.read()
 
         try:
             feedback = create_feedback_email(self.nomcom, self.msg)

ietf/nomcom/management/tests.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 """Tests of nomcom management commands"""
 import mock
+import sys
 
 from collections import namedtuple
 
@@ -83,3 +84,30 @@ def test_feedback_email(self, create_feedback_mock):
             (self.nomcom, b'feedback message'),
             'feedback_email should process the correct email for the correct nomcom'
         )
+
+    @mock.patch('ietf.utils.management.base.send_smtp')
+    def test_invalid_character_encodings(self, send_smtp_mock):
+        """The feedback_email command should send a message when file input has invalid encoding"""
+        # mock an exception in create_feedback_email()
+        invalid_characters = b'\xfe\xff'
+        with name_of_file_containing(invalid_characters, mode='wb') as filename:
+            call_command('feedback_email', nomcom_year=self.year, email_file=filename)
+
+        self.assertTrue(send_smtp_mock.called)
+        (msg,) = send_smtp_mock.call_args.args  # get the message to be sent
+        parts = msg.get_payload()
+        self.assertEqual(len(parts), 2, 'Error email should contain message and original message')
+
+    @mock.patch.object(sys.stdin.buffer, 'read')
+    @mock.patch('ietf.utils.management.base.send_smtp')
+    def test_invalid_character_encodings_via_stdin(self, send_smtp_mock, stdin_read_mock):
+        """The feedback_email command should send a message when stdin input has invalid encoding"""
+        # mock an exception in create_feedback_email()
+        invalid_characters = b'\xfe\xff'
+        stdin_read_mock.return_value = invalid_characters
+        call_command('feedback_email', nomcom_year=self.year)
+
+        self.assertTrue(send_smtp_mock.called)
+        (msg,) = send_smtp_mock.call_args.args  # get the message to be sent
+        parts = msg.get_payload()
+        self.assertEqual(len(parts), 2, 'Error email should contain message and original message')

ietf/utils/test_utils.py

@@ -104,9 +104,9 @@ def reload_db_objects(*objects):
         return t
 
 @contextmanager
-def name_of_file_containing(contents):
+def name_of_file_containing(contents, mode='w'):
     """Get a context with the name of an email file"""
-    f = NamedTemporaryFile('w', delete=False)
+    f = NamedTemporaryFile(mode, delete=False)
     f.write(contents)
     f.close()
     yield f.name  # hand the filename to the context