Recover a multipart part's boundary past a run of malformed header lines - #14
Merged
Merged
Conversation
A single bad line before otherwise-valid headers already recovers (see #11), but a part whose Content-Type is followed by more than one bad line was fully discarded -- headers, body, and any nested boundary it declared. Real-world credential-phishing content has been found hidden exactly this way: paragraphs of quoted-reply text with no blank line before the real sub-parts, some lines colon-shaped enough to look like headers, none of it terminating the header block before the real boundary appears further down. readPartHeader replaces the two-call retry with one continuous pass that checkpoints at the first bad line. A second bad line rolls the header back to that checkpoint and hands everything since then to the caller as leftover raw bytes instead of discarding them. newPart folds that leftover into the part's Body: if the checkpointed header still declares a multipart Content-Type, NextPart's existing preamble-skipping finds the real boundary past the garbage on its own, the same as ordinary RFC 2046 preamble text -- no new boundary-matching code needed. If no boundary survives, the raw bytes still become the part's body rather than being thrown away. ReadHeader now shares its line-parsing step with readPartHeader instead of duplicating it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
readContinuedLineSlice never returns line content and an error together: every path is either (content, nil) or (nil, err). So once badLines > 0, treating any err at the blank-line check as "found the end of the header" was wrong -- that err can only be a real read failure, never a normal terminator, and it was being discarded in favor of the earlier bad-line error, silently misreporting a broken stream as a successfully recovered part. Also stop copying every header line into raw unconditionally: only lines from the first bad line onward can ever end up in leftover, so accumulate lazily instead of paying a copy on every well-formed part. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves malformed multipart-header recovery while preserving nested multipart boundaries and raw body content.
Changes:
- Adds checkpoint-based part-header recovery.
- Reuses shared header-line parsing helpers.
- Expands malformed-header and I/O-error tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
textproto/multipart.go |
Implements recovery and leftover-body handling. |
textproto/header.go |
Extracts shared header parsing helpers. |
textproto/multipart_test.go |
Tests nested-boundary and error recovery. |
Suppressed comments (1)
textproto/multipart.go:198
- Do not skip empty-key fields before recording checkpointed bytes.
parseHeaderFieldLineaccepts a colon-only line withkey == ""; after the first malformed line, this branch silently removes that line from the recovered body, contradicting the guarantee that everything from the checkpoint is preserved.
if key == "" {
continue
}
if badLines > 0 {
raw = append(raw, kv...)
}
fs = append(fs, newHeaderField(key, value, kv))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
readLineSlice strips the line ending, so the initial-continuation-line recovery path was appending it to raw with nothing to separate it from the next line's bytes -- gluing them together. If that next line were a nested boundary, it would no longer start a line of its own and the recovery this PR exists for would fail on it. Restore the CRLF that was stripped. Also stop skipping the raw append for a colon-only (empty key) line: it was being dropped from leftover entirely on that path, silently losing a byte range from the reconstructed body. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thunderkatz
marked this pull request as ready for review
September 1, 2026 20:30
ender336
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
https://app.notion.com/p/sublimesecurity/Parse-EML-sub-parts-with-malformed-boundaries-3ce04655fc9d817dbf61d15ac6190622
(HUMAN): header.go is just a refactoring to extract shared logic. multipart.go is the real logic. If a part with malformed headers defines a sub-boundary, mail clients still extract it, so we need to too. We don't want to allow an arbitrary number of malformed lines, but we can still keep the headers we did parse out and use that to find other valid parts.
Validated against the sample that with this change we do parse out the HTML body.
(CLAUDE): Follow-up to #11. A single bad line before otherwise-valid headers already recovers, but a part whose
Content-Typeis followed by more than one bad line was still fully discarded — headers, body, and any nested boundary it declared. Real-world credential-phishing content has been found hidden exactly this way: paragraphs of quoted-reply text with no blank line before the real sub-parts, some lines colon-shaped enough to look like headers, none of it terminating the header block before the real boundary appears further down.readPartHeaderreplaces the two-call retry with one continuous pass that checkpoints at the first bad line. A second bad line rolls the header back to that checkpoint and hands everything since then to the caller as leftover raw bytes instead of discarding them.newPartfolds that leftover into the part'sBody: if the checkpointed header still declares a multipartContent-Type,NextPart's existing preamble-skipping finds the real boundary past the garbage on its own — the same as ordinary RFC 2046 preamble text, so no new boundary-matching code is needed. If no boundary survives, the raw bytes still become the part's body rather than being thrown away.ReadHeadernow shares its line-parsing step (readHeaderInitialLine,parseHeaderFieldLine) withreadPartHeaderinstead of duplicating it.🤖 Generated with Claude Code