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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
913f4b9
Fixed #35378 -- Added maximum header length for long addresses.
lufafajoshua Apr 26, 2024
e401be6
fixed failing github linters/black tests
lufafajoshua Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions django/core/mail/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ def as_string(self, unixfrom=False, linesep="\n"):
lines that begin with 'From '. See bug #13433 for details.
"""
fp = StringIO()
g = generator.Generator(fp, mangle_from_=False)
g = generator.Generator(fp, mangle_from_=False, maxheaderlen=0)
g.flatten(self, unixfrom=unixfrom, linesep=linesep)
return fp.getvalue()

Expand All @@ -144,7 +144,7 @@ def as_bytes(self, unixfrom=False, linesep="\n"):
lines that begin with 'From '. See bug #13433 for details.
"""
fp = BytesIO()
g = generator.BytesGenerator(fp, mangle_from_=False)
g = generator.BytesGenerator(fp, mangle_from_=False, maxheaderlen=0)
g.flatten(self, unixfrom=unixfrom, linesep=linesep)
return fp.getvalue()

Expand Down
15 changes: 15 additions & 0 deletions tests/mail/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,21 @@ def test_dont_mangle_from_in_body(self):
)
self.assertNotIn(b">From the future", email.message().as_bytes())

def test_long_address_folding(self):
# Ticket #35378
# should encode long addresses with special characters using
# 7bit Content-Transfer-Encoding
Comment on lines +898 to +900
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.

msg = EmailMessage(
"Subject",
"Long address with special characters",
"from@example.com",
['"Người nhận a very very long, name" <to@example.com>'],
)
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)
Comment on lines +907 to +910
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 🤔


def test_dont_base64_encode(self):
# Ticket #3472
# Shouldn't use Base64 encoding at all
Expand Down