Skip to content

fix bug in re-serializing multipart mime with preamble#540

Open
raphting wants to merge 2 commits into
KumoCorp:mainfrom
raphting:raphael/fixpreamble
Open

fix bug in re-serializing multipart mime with preamble#540
raphting wants to merge 2 commits into
KumoCorp:mainfrom
raphting:raphael/fixpreamble

Conversation

@raphting

Copy link
Copy Markdown
Contributor

check_fix_conformance rewrites the message buffer, but introduces byte changes in the body part.
This breaks DKIM.

Example (after parse + to_message_bytes):

  ...1.0\r\n\r\nThis is a preamble line.\r\n--BOUND\r\n
  ...1.0\r\n\r\n\nThis is a preamble line.\r--BOUND\r\n   ← extra \n, lost \n

Explanation

In recursive_parse, raw_body starts one byte early (body_offset - 1) so the \n--{boundary} needle can match a boundary at the very start of the body. The preamble was then captured as raw_body.slice(0..first_boundary_pos), which:

  1. includes raw_body[0], the \n of the header/body separator that write_message already emits itself -> a duplicated \n after the blank line and
  2. ends on the \r of the \r\n preceding the first boundary (the \n is consumed by the needle and never re-emitted) ->
    \r\n--boundary is rewritten as \r--boundary.

@wez

wez commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

I haven't really immersed myself in this to review fully, this is just a quick drive-by comment: since it sounds like the slice/range is off-by-one, should the struct capture different indices? I'm wondering if this is a sharp edge that should be fixed at source.

@raphting

Copy link
Copy Markdown
Contributor Author

You are right, it would be cleaner to work with proper indices in the struct.

There's also a subtlety I noticed from RFC 2046:

NOTE: The CRLF preceding the boundary delimiter line is conceptually
attached to the boundary so that it is possible to have a part that
does not end with a CRLF (line break). Body parts that must be
considered to end with line breaks, therefore, must have two CRLFs
preceding the boundary delimiter line, the first of which is part of
the preceding body part, and the second of which is part of the
encapsulation boundary.

Even though the code works, it deviates from the RFC's note. In this PR, I would also align this, at the cost of changing the write_message function too.

I'll try to update the code so that we fix this issue where it originates.

@raphting raphting force-pushed the raphael/fixpreamble branch from 3b08076 to 2bc98b3 Compare July 1, 2026 20:21
@raphting

raphting commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Hey @wez ,

thanks for your suggestion to fix the issue at the source! I chose to extract a function for multipart-mime parsing, which reduces the counter-arithmetic that led to an off-by-one error.

In contrast to what I wrote earlier, I won't align the behavior to RFC 2046. The call chain involves other parts than just multipart-mime, and I want to keep this PR small.

Let me know what you think about this approach!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants