coddingtonbear / django-mailbox

Import mail from POP3, IMAP, local email mailboxes or directly from Postfix or Exim4 into your Django application automatically.
MIT License
356 stars 164 forks source link

message.text throws exception with bad characters in attachment filename #289

Closed Res42 closed 4 months ago

Res42 commented 4 months ago

Hi!

This is a reproduction of a real email from our system. Please remove .txt from the end of the file. I included it because otherwise Github didn't allow it to be uploaded.

bad character attachment.eml.txt

The problem is because of this filename: K�relem.pdf

Exception:

'Header' object has no attribute 'startswith'

Stacktrace:

File "/usr/local/lib/python3.12/site-packages/django_mailbox/models.py", line 483, in get_new_mail_all_mailboxes
for message in messages:
File "/usr/local/lib/python3.12/site-packages/django_mailbox/models.py", line 461, in get_new_mail
msg = self.process_incoming_message(message)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/django_mailbox/models.py", line 284, in process_incoming_message
message_received.send(sender=self, message=msg)
File "/usr/local/lib/python3.12/site-packages/django/dispatch/dispatcher.py", line 189, in send
response = receiver(signal=self, sender=sender, **named)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[...]

File "/usr/local/lib/python3.12/site-packages/django_mailbox/models.py", line 661, in text
return utils.get_body_from_message(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/django_mailbox/utils.py", line 106, in get_body_from_message
if part.get('content-disposition', '').startswith('attachment;'):

If I understand it right the problem is here:

https://github.com/coddingtonbear/django-mailbox/blob/a41768c1602fa94824bdd8f88483a7e303065f44/django_mailbox/utils.py#L106

part.get() can return a Header object or a string; and the code only supports the string case.

Workaround / solution:

I monkey patched the get_body_from_message function like this:

# [...]
if convert_header_to_unicode(part.get('content-disposition', '')).startswith('attachment;'):
# [...]

it seems to work with str() too:

# [...]
if str(part.get('content-disposition', '')).startswith('attachment;'):
# [...]

I can submit a PR for this fix but I don't know which is the better of these solutions (or something else entirely).

Pietro395 commented 4 months ago

What version of django_mailbox are you using? If you are not using the latest one can you try 4.10.1?

I imported the message you attached but I can't reproduce the error, it seems ok

Res42 commented 4 months ago

I was using 4.9.0 but I updated it to 4.10.1 and it still throws:

======================================================================
ERROR: test_messages_with_unicode_attachments_should_parse_1 (mailprocessor.tests.tests.UnicodeAttachmentTestCase.test_messages_with_unicode_attachments_should_parse_1)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "[...]\tests\tests.py", line 72, in test_messages_with_unicode_attachments_should_parse_1
    self.assertIsNotNone(message.text)
                         ^^^^^^^^^^^^
  File "[...]\.venv\Lib\site-packages\django_mailbox\models.py", line 678, in text
    return utils.get_body_from_message(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]\.venv\Lib\site-packages\django_mailbox\utils.py", line 106, in get_body_from_message
    if part.get('content-disposition', '').startswith('attachment;'):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Header' object has no attribute 'startswith'

I'll fork the repo and write a failing test for this.

Res42 commented 4 months ago

Commit: https://github.com/Res42/django-mailbox/commit/232f03755568cf57647e3431921562b45ac7459a

Test failed on CI: https://github.com/Res42/django-mailbox/actions/runs/8830921371/job/24245072678

Pietro395 commented 4 months ago

Commit: Res42@232f037

Test failed on CI: https://github.com/Res42/django-mailbox/actions/runs/8830921371/job/24245072678

You're right, I was using 'processincomingmessage' and for some reason this way I can't reproduce it if I call up the text property of the message.

If you want feel free to make a pull request, I think your first solution is fine