Skip to content

gh-128949: Preserve payload line endings in BytesParser.parse - #157726

Open
sankalpsthakur wants to merge 4 commits into
python:mainfrom
sankalpsthakur:fix/gh-128949-email-newlines
Open

sankalpsthakur wants to merge 4 commits into
python:mainfrom
sankalpsthakur:fix/gh-128949-email-newlines

Conversation

@sankalpsthakur

@sankalpsthakur sankalpsthakur commented Sep 18, 2026 •

Copy link
Copy Markdown

Fixes #128949. Thanks to tnakamot and medmunds for the reports.

Preserve CRLF and bare CR bytes when parsing binary files with BytesParser and BytesHeaderParser, so unencoded attachments agree with parsebytes(). Header recognition and ownership of the caller's stream are unchanged.

The mailbox follow-up in 9634201 retains mailbox.Message's existing universal-newline behaviour for binary streams. 004a504 corrects the NEWS link to the documented BytesHeaderParser class without changing runtime code.

Regression coverage includes both policies, payload/transfer-encoding variants, multipart attachments, an 8192-byte read boundary, header-only parsing and mailbox construction from streams/bytes.

All reported upstream checks pass on 004a504, including Docs, Doctest, EPUB, Windows, macOS, Linux and the required-check aggregate. Maintainer review remains. The NEWS correction also removed exactly the failing reference in local nitpicky Sphinx output; the repository's new-NEWS warning gate changed from exit 255 to 0, with no new warnings.

AI tools assisted with implementation, validation and this description. The author reviewed and approved the latest correction.

Copy link
Copy Markdown
Author

Found a mailbox compatibility regression while investigating the Windows failures on 5a0df99: with CRLF storage, Maildir/MH now return CRLF payloads while get_bytes() still normalizes to LF. test_add_8bit_body expects LF.

A candidate follow-up keeps universal-newline conversion in mailbox.Message's binary-stream constructor, without reverting the email parser fix. On Linux/Python 3.13.5, the new constructor tests have 12 failing subcases with the PR behavior and none with that follow-up; the separate compatibility probe passes 18/18. This uses the inspected constructor and simulated CRLF storage, not a CPython 3.16 or native Windows run. The Windows job logs returned 404, so I have not confirmed this accounts for every CI failure. The follow-up is not committed yet.

sankalpsthakur commented Sep 18, 2026 •

Copy link
Copy Markdown
Author

The mailbox fix is in 9634201, and all seven Windows jobs now pass; the remaining failure is the NEWS reference to BytesHeaderParser.parse.
A two-line correction links to the documented class instead; patch-application checks pass, but it is not pushed or Sphinx-tested.

Link the documented class rather than its inherited parse method. The author reviewed and approved this one-line correction. AI-assisted: Codex.
@sankalpsthakur

Copy link
Copy Markdown
Author

The NEWS reference correction is now in 004a504. All reported checks pass on that head, including Docs and the required-check aggregate. Runtime and mailbox behaviour are unchanged by this follow-up.

@python-cla-bot

python-cla-bot Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@sankalpsthakur
sankalpsthakur force-pushed the fix/gh-128949-email-newlines branch from db3d2c1 to d19b94e Compare October 7, 2026 02:26
@bitdancer

Copy link
Copy Markdown
Member

The email fix looks good, but I have my doubts about the mailbox change. It seems to me that it may be better to view this as a problem with the tests when run on windows. I remember needing to fix some email tests to handle being run on windows when binary support was re-introduced in the python3 email package.

Which tests are failing without the mailbox.py adjustment?

Related to the email fix: it seems like in a strict interpretation of the RFCs, if a CTE 7bit (the default) binary part contains bare LF, they should be converted to CRLF on binary part retrieval. The RFCs basically make bare LF technically illegal. In practical terms I don't think we want to do that; better to preserve what we are given, which this fix should do.

@sankalpsthakur

Copy link
Copy Markdown
Author

thanks. without the mailbox.py hunk, 17 tests in test_mailbox fail on all seven windows jobs, all in TestMaildir and TestMH: test_add, test_add_8bit_body, test_get, test_get_message, test_getitem, test_pop, test_set_item and test_update in both, plus TestMaildir.test_set_MM (log at 5a0df99). bodies and folded headers come back with \r\n where \n is expected; mbox, mmdf and babyl pass.

mailbox writes messages with os.linesep, and its other read paths convert that back explicitly (get_bytes()/get_string() for every format, get_message() for mbox, mmdf and babyl). maildir and mh get_message() instead pass the open binary file to the message class, so they relied on BytesParser.parse() translating newlines. without the hunk, on windows their get_message() would return \r\n while get_bytes() returns \n. the hunk keeps mailbox.Message(binary_file) behaving as before. if you'd still rather treat it as a test issue, i can drop the hunk and make those tests linesep-aware instead.

agreed on bare lf: preserving what we're given seems right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

email.parser.BytesParser.parse() cannot handle binary data that include \x0d \x0a correctly.

2 participants