Skip to content

Zipdownload: fix escaping in Mbox downloads#10151

Open
gurnec wants to merge 3 commits into
roundcube:masterfrom
gurnec:zipdownload-mbox
Open

Zipdownload: fix escaping in Mbox downloads#10151
gurnec wants to merge 3 commits into
roundcube:masterfrom
gurnec:zipdownload-mbox

Conversation

@gurnec

@gurnec gurnec commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Closes #10150.

This unfortunately adds some extra complication... a simpler alternative might be slurp each entire email into a variable and just preg_replace() it all at once in _download_messages(). That would add additional memory overhead though.

@gurnec gurnec force-pushed the zipdownload-mbox branch 2 times, most recently from 7e9bc04 to 9ecb474 Compare May 2, 2026 11:04
@gurnec gurnec force-pushed the zipdownload-mbox branch 3 times, most recently from 0cad7a6 to 9bf4cc5 Compare May 13, 2026 22:44
'INBOX.mbox' => ['From test-from', "\nFrom thomas", "\n>From line which needs to be escaped"],
]);
} finally {
$browser->removeDownloadedFile($filename);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer doing assertions after removing the file. The test files are small so we can keep them in memory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like:

  1. extract filenames / file contents,
  2. remove zip file (no try/finally block required),
  3. do all assertions

probably all in checkFilesInZip()?

Also, just for future reference, would you prefer that I add new commits to this branch, or force-push existing commit(s) to keep the history a bit cleaner?

@alecpl alecpl Jun 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

1. extract filenames / file contents,
2. remove zip file (no try/finally block required),
3. do all assertions

probably all in checkFilesInZip()?

Yes, not necessarily in a single method.

Also, just for future reference, would you prefer that I add new commits to this branch, or force-push existing commit(s) to keep the history a bit cleaner?

Doesn't matter, I usually use the "Squash and merge" button.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've simplified the E2E tests w/o changing what is tested, should be better now.

gurnec added 3 commits June 19, 2026 11:06
This checks an Mbox download has a properly escaped From line,
and also checks at least some content of each zipped file
(which as a side effect verifies the zips' CRCs).
@gurnec gurnec force-pushed the zipdownload-mbox branch from 9bf4cc5 to 2183f9a Compare June 19, 2026 17:17
@MiMoHo

MiMoHo commented Jul 5, 2026

Copy link
Copy Markdown

I did an independent review and test of this PR (AI-assisted, all results verified locally):

Functionality

  • All 8 MboxFilterTest unit tests pass locally on PHP 8.5.8.
  • Fuzz-tested the filter beyond the unit tests: 2,000 randomly generated documents built from adversarial tokens (From , >From, bare Fro/om , CRLF endings, > runs, …), written through the filter in random 16–64 byte chunks, and compared byte-for-byte against a whole-string reference implementation (preg_replace('/(?:^|(?<=\n))(>*From )/', '>\1', $data)) — zero mismatches, including From-lines split across bucket boundaries and the closing flush.
  • Deterministic split cases verified as well: boundary between \r and \n, split inside From, split inside a > chain, split exactly after \n, first-line escaping, and an unterminated trailing >From at EOF — all correct.

The three failing CI checks are unrelated to this PR:

  • Coding Style: the failure flags fully_qualified_strict_types in files this PR doesn't touch (installer/test.php, plugins/new_user_dialog/…, …) — a toolchain drift on master at the time of the run (2026-06-19); CI on master is green again since (e.g. the runs of 2026-07-04/05).
  • Linux / PHP 8.4 (message rendering): transient network error pulling the docker image from ghcr.io (Client.Timeout exceeded while awaiting headers).
  • Windows / PHP 8.1: transient composer download failure (file could not be written … Permission denied).

So a rebase onto current master (or re-running the checks) should turn CI green; functionally this looks ready from my side.

@gurnec gurnec requested a review from alecpl July 13, 2026 20:34
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.

Zipdownload's Mbox format does not escape "From " lines

3 participants