From 4877c7461bc4927eddf99142bcc8cf1d4f65f3a4 Mon Sep 17 00:00:00 2001 From: Christopher Gurnee Date: Thu, 23 Apr 2026 18:26:50 -0400 Subject: [PATCH 1/3] Zipdownload: fix escaping in Mbox downloads --- plugins/zipdownload/composer.json | 2 +- plugins/zipdownload/zipdownload.php | 68 +++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/plugins/zipdownload/composer.json b/plugins/zipdownload/composer.json index 7639225275..45650344c8 100644 --- a/plugins/zipdownload/composer.json +++ b/plugins/zipdownload/composer.json @@ -3,7 +3,7 @@ "type": "roundcube-plugin", "description": "Adds an option to download all attachments to a message in one zip file, when a message has multiple attachments. Also allows the download of a selection of messages in one zip file. Supports mbox and maildir format.", "license": "GPL-3.0-or-later", - "version": "3.6", + "version": "3.7", "authors": [ { "name": "Thomas Bruederli", diff --git a/plugins/zipdownload/zipdownload.php b/plugins/zipdownload/zipdownload.php index d3aa92baed..7ece616388 100644 --- a/plugins/zipdownload/zipdownload.php +++ b/plugins/zipdownload/zipdownload.php @@ -393,23 +393,73 @@ private function _filename_from_subject($str) } } +/** + * Unfortunately a body line which could be mistaken for an Mbox header, + * and which therefore must be escaped with '>', can be split between + * buckets (and between calls to filter()), so if the bucket being filtered + * ends in a way that *might* be a line needing escaping, postpone placing + * it in the $out brigade until we've seen the next bucket and can decide + * whether or not to escape. Don't bother checking if such a line might be + * split among more than 2 buckets -- given even a little buffering, this + * should never happen in practice. + */ class zipdownload_mbox_filter extends \php_user_filter { + private $prev_bucket; + private $prev_match; + private $first = true; + #[\Override] - #[\ReturnTypeWillChange] - public function filter($in, $out, &$consumed, $closing) + public function filter($in, $out, &$consumed, $closing): int { + $out_empty = true; + $replaced = 0; + $m = []; + while ($bucket = stream_bucket_make_writeable($in)) { - // messages are read line by line - if (preg_match('/^>*From /', $bucket->data)) { - $bucket->data = '>' . $bucket->data; - $bucket->datalen++; + // Escape lines which definitely need it + $anchor = $this->first ? '^' : "\n"; // Only the file's first line may match w/o \n + $this->first = false; + $bucket->data = preg_replace("/({$anchor}>*)From /m", '\1>From ', $bucket->data, -1, $replaced); + $consumed += $bucket->datalen; + $bucket->datalen += $replaced; + + // If the previous bucket was postponed... + if ($this->prev_bucket) { + // Escape its last line if necessary + $joined = $this->prev_match . substr($bucket->data, 0, strspn($bucket->data, '>From ')); + if (preg_match('/^>*From /', $joined)) { + if (strlen($this->prev_match)) { // prev_match == end of prev_bucket w/o \n, e.g. 'Fro' or '>>F' or '' + $this->prev_bucket->data = substr_replace( + $this->prev_bucket->data, '>' . $this->prev_match, -strlen($this->prev_match)); + } else { + $this->prev_bucket->data .= '>'; + } + $this->prev_bucket->datalen++; + } + + // And in either case send it out + stream_bucket_append($out, $this->prev_bucket); + $this->prev_bucket = null; + $out_empty = false; } - $consumed += (int) $bucket->datalen; - stream_bucket_append($out, $bucket); + // Decide if the current bucket should be postponed or sent immediately + if (str_contains("\n>From", substr($bucket->data, -1)) + && preg_match('/\G\n>*(?:F(?:r(?:om?)?)?)?\z/', $bucket->data, $m, 0, strrpos($bucket->data, "\n")) + ) { + $this->prev_bucket = $bucket; + $this->prev_match = substr($m[0], 1); // What was matched without the initial \n (could be '') + } else { + stream_bucket_append($out, $bucket); + $out_empty = false; + } } - return \PSFS_PASS_ON; + if ($closing && $this->prev_bucket) { + stream_bucket_append($out, $this->prev_bucket); + $this->prev_bucket = null; + } + return !$out_empty || $closing ? \PSFS_PASS_ON : \PSFS_FEED_ME; } } From 9807870677abe71498da194b3b6c33678e301127 Mon Sep 17 00:00:00 2001 From: Christopher Gurnee Date: Fri, 8 May 2026 08:30:54 -0400 Subject: [PATCH 2/3] Zipdownload: add mbox_filter unit tests --- plugins/zipdownload/tests/MboxFilterTest.php | 144 +++++++++++++++++++ plugins/zipdownload/zipdownload.php | 2 +- 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 plugins/zipdownload/tests/MboxFilterTest.php diff --git a/plugins/zipdownload/tests/MboxFilterTest.php b/plugins/zipdownload/tests/MboxFilterTest.php new file mode 100644 index 0000000000..c60c051141 --- /dev/null +++ b/plugins/zipdownload/tests/MboxFilterTest.php @@ -0,0 +1,144 @@ +fp = fopen('php://memory', 'w+'); + $this->filter = stream_filter_append($this->fp, 'test_mbox_filter', \STREAM_FILTER_WRITE); + } + + /** + * Basic test with no special case + */ + public function test_escape() + { + $this->assertIsResource($this->filter); + $this->assertSame(15, fwrite($this->fp, "test\nFrom \ntest")); + $this->assertTrue(stream_filter_remove($this->filter)); + $this->assertTrue(rewind($this->fp)); + $this->assertSame("test\n>From \ntest", fread($this->fp, 100)); + } + + /** + * The very first line may be escaped + */ + public function test_escape_first_line() + { + fwrite($this->fp, '>From test'); + $this->assertFalse(test_mbox_filter::was_maybe_split()); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame('>>From test', fread($this->fp, 100)); + } + + /** + * The beginning of a bucket which isn't the beginning of a new line + * must not be escaped + */ + public function test_noescape_bucket_beginning() + { + fwrite($this->fp, 'test'); + fwrite($this->fp, 'From '); + $this->assertFalse(test_mbox_filter::was_maybe_split()); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame('testFrom ', fread($this->fp, 100)); + } + + /** + * A split From line with a minimal portion in the first bucket + */ + public function test_escape_split_min() + { + fwrite($this->fp, "test\n"); + $this->assertTrue(test_mbox_filter::was_maybe_split()); + fwrite($this->fp, 'From '); + $this->assertFalse(test_mbox_filter::was_maybe_split()); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame("test\n>From ", fread($this->fp, 100)); + } + + /** + * A split From line with a maximal portion in the first bucket + */ + public function test_escape_split_max() + { + fwrite($this->fp, "test\n>From"); + $this->assertTrue(test_mbox_filter::was_maybe_split()); + fwrite($this->fp, ' '); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame("test\n>>From ", fread($this->fp, 100)); + } + + /** + * A From line that is maybe-split in the first bucket, but not + * actually a split From line after seeing the second bucket + */ + public function test_noescape_split() + { + fwrite($this->fp, "test\n>From"); + $this->assertTrue(test_mbox_filter::was_maybe_split()); + fwrite($this->fp, "\ntest"); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame("test\n>From\ntest", fread($this->fp, 100)); + } + + /** + * A From line that is maybe-split with no second bucket + */ + public function test_noescape_split_last() + { + fwrite($this->fp, "test\n>From"); + $this->assertTrue(test_mbox_filter::was_maybe_split()); + stream_filter_remove($this->filter); + rewind($this->fp); + $this->assertSame("test\n>From", fread($this->fp, 100)); + } +} + +/** + * The assumption is that separate writes result in separate calls to + * zipdownload_mbox_filter::filter() which tries to detect a From line + * that might be split between calls. This checks the internal state of + * the most recently created (by PHP) zipdownload_mbox_filter to ensure + * that assumption is correct, otherwise the tests above may be invalid. + */ +class test_mbox_filter extends \zipdownload_mbox_filter +{ + private static $most_recently_created; + + #[\Override] + public function onCreate(): bool + { + self::$most_recently_created = $this; + return parent::onCreate(); + } + + /** + * If prev_bucket is a bucket (a stdClass or StreamBucket object, depending + * on PHP version), zipdownload_mbox_filter's most recently seen bucket has + * what looks like a split From line at its end. + */ + public static function was_maybe_split() + { + return is_object(self::$most_recently_created->prev_bucket); + } +} diff --git a/plugins/zipdownload/zipdownload.php b/plugins/zipdownload/zipdownload.php index 7ece616388..983a9c39db 100644 --- a/plugins/zipdownload/zipdownload.php +++ b/plugins/zipdownload/zipdownload.php @@ -405,7 +405,7 @@ private function _filename_from_subject($str) */ class zipdownload_mbox_filter extends \php_user_filter { - private $prev_bucket; + protected $prev_bucket; private $prev_match; private $first = true; From 2183f9ae071efcf2279c8167cd2ce399745599b2 Mon Sep 17 00:00:00 2001 From: Christopher Gurnee Date: Tue, 12 May 2026 14:53:40 -0400 Subject: [PATCH 3/3] Zipdownload: check zip contents in E2E tests 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). --- plugins/zipdownload/tests/Browser/MailTest.php | 18 ++++++++++++++---- tests/Browser/data/mail/list_00.eml | 1 + 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/plugins/zipdownload/tests/Browser/MailTest.php b/plugins/zipdownload/tests/Browser/MailTest.php index dd4696958a..7464f228a3 100644 --- a/plugins/zipdownload/tests/Browser/MailTest.php +++ b/plugins/zipdownload/tests/Browser/MailTest.php @@ -65,7 +65,10 @@ public function testMailUI() $files = $this->getFilesFromZip($browser, $filename); $browser->removeDownloadedFile($filename); - $this->assertSame(['INBOX.mbox'], $files); + $this->assertSame(['INBOX.mbox'], array_keys($files)); + $this->assertStringStartsWith('From test-from', array_first($files)); + $this->assertStringContainsString("\nFrom thomas", array_first($files)); + $this->assertStringContainsString("\n>From line which needs to be escaped", array_first($files)); }); // Test More > Download > Maildir format (two messages selected) @@ -82,6 +85,11 @@ public function testMailUI() $files = $this->getFilesFromZip($browser, $filename); $browser->removeDownloadedFile($filename); $this->assertCount(2, $files); + $this->assertStringContainsString(' Test ', array_key_first($files)); + $this->assertStringEndsWith('.eml', array_key_first($files)); + $this->assertStringEndsWith(' Lines.eml', array_key_last($files)); + $this->assertStringContainsString('Attached image:', array_first($files)); + $this->assertStringContainsString("\nFrom line which needs to be escaped in mbox format.", array_last($files)); }); // Test attachments download @@ -98,12 +106,14 @@ public function testMailUI() $files = $this->getFilesFromZip($browser, $filename); $browser->removeDownloadedFile($filename); $expected = ['lines.txt', 'lines_lf.txt']; - $this->assertSame($expected, $files); + $this->assertSame($expected, array_keys($files)); + $this->assertStringContainsString("foo\r\nbar\r\ngna", array_first($files)); + $this->assertStringContainsString("foo\nbar\ngna", array_last($files)); }); } /** - * Helper to extract files list from downloaded zip file + * Helper to extract filenames and contents from downloaded zip file */ private function getFilesFromZip($browser, $filename) { @@ -138,7 +148,7 @@ private function getFilesFromZip($browser, $filename) if ($zip->open($filename)) { for ($i = 0; $i < $zip->numFiles; $i++) { - $files[] = $zip->getNameIndex($i); + $files[$zip->getNameIndex($i)] = $zip->getFromIndex($i); } } diff --git a/tests/Browser/data/mail/list_00.eml b/tests/Browser/data/mail/list_00.eml index 3c0d10cc74..fc799dc322 100644 --- a/tests/Browser/data/mail/list_00.eml +++ b/tests/Browser/data/mail/list_00.eml @@ -21,6 +21,7 @@ Content-Type: text/plain; charset=UTF-8; format=flowed Plain text message body. +From line which needs to be escaped in mbox format. -- Developer of Free Software