Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #35378 -- Added maximum header length for long addresses. #18110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lufafajoshua
Copy link
Contributor

Trac ticket number

ticket-35378

Branch description

Incorrect folding of long address headers with special characters when using 7bit Content-Transfer-Encoding in EmailMessage.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Added maxheaderlen parameter to email.generator in Django's
EmailMessage.message class to limit length of long addresses
with special characters while folding using 7bit
 Content-Transfer-Encoding.
Copy link
Contributor

@DevilsAutumn DevilsAutumn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lufafajoshua Thanks for this patch 👍, left small comments.

Comment on lines +898 to +900
# Ticket #35378
# should encode long addresses with special characters using
# 7bit Content-Transfer-Encoding
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer add comments in the tests until its really important. So maybe these can be removed.

Comment on lines +907 to +910
s = msg.message().as_bytes()
self.assertIn(b"Content-Transfer-Encoding: 7bit", s)
s = msg.message().as_string()
self.assertIn("Content-Transfer-Encoding: 7bit", s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted your changes from message.py and was expecting this test to fail but surprisingly it passed.
The main issues mentioned in the ticket was the incorrect folding of long address headers when using 7bit Content-Transfer-Encoding , So if you'll check for the Content-Transfer-Encoding: 7bit in message string it will always be there and the test will never fail.

In your solution, after you set maxheaderlen=0, it will treat all the long addresses as one string and there wont be any new line (\n) in between the address. So, in my opinion it will be better if you check the encoded long address for a new line symbol in somewhere between the address. If its not there then it means that its no longer folding the long addresses and treating it as one single string. Does that make sense to you?

Suggested change
s = msg.message().as_bytes()
self.assertIn(b"Content-Transfer-Encoding: 7bit", s)
s = msg.message().as_string()
self.assertIn("Content-Transfer-Encoding: 7bit", s)
s = msg.message().as_bytes()
self.assertIn(b"=?utf-8?b?TmfGsOG7nWkgbmjhuq1uIGEgdmVyeSB2ZXJ5IGxvbmcsIG5hbWU=?= <to@example.com>\n", s)
s = msg.message().as_string()
self.assertIn("=?utf-8?b?TmfGsOG7nWkgbmjhuq1uIGEgdmVyeSB2ZXJ5IGxvbmcsIG5hbWU=?= <to@example.com>\n", s)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a test update is required 👍
Something like this might work

    def test_long_address_folding(self):
        msg = EmailMessage(
            "Subject",
            "Long address with special characters",
            "from@example.com",
            ['"Người nhận a very very long, name" <to@example.com>'],
        ).message()
        msg.policy = policy.default.clone(cte_type="7bit")
        self.assertIn(
            "=?utf-8?b?TmfGsOG7nWkgbmjhuq1uIGEgdmVyeSB2ZXJ5IGxvbmcs?= name <to@example.com>",
            msg.as_bytes().decode("ascii")
        )

This would use the example from the discussion.

This also fails and I am not sure adding maxheaderlen=0 is the right solution. It goes against previous design decisions (see ticket-31784) and RFC 2822 stating that

Each line of characters MUST be no more than 998 characters, and SHOULD be no more than 78 characters, excluding the CRLF.

Allowing longer line length was discussed and discouraged in this comment.

Allowing longer lines would solve the problem (no folding needed), but has similar compatibility concerns to using 8bit (or worse).

In short, I think more investigation is needed 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants