Zipdownload: fix escaping in Mbox downloads

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",

plugins/zipdownload/tests/Browser/MailTest.php

@@ -62,10 +62,13 @@ public function testMailUI()
                         ->click('a.download.mbox');
 
                     $filename = 'INBOX.zip';
-                    $files = $this->getFilesFromZip($browser, $filename);
-                    $browser->removeDownloadedFile($filename);
-
-                    $this->assertSame(['INBOX.mbox'], $files);
+                    try {
+                        $this->checkFilesInZip($browser, $filename, [
+                            'INBOX.mbox' => ['From test-from', "\nFrom thomas", "\n>From line which needs to be escaped"],
+                        ]);
+                    } finally {
+                        $browser->removeDownloadedFile($filename);
+                    }
                 });
 
             // Test More > Download > Maildir format (two messages selected)
@@ -79,9 +82,14 @@ public function testMailUI()
                     $browser->click('a.download.maildir');
 
                     $filename = 'INBOX.zip';
-                    $files = $this->getFilesFromZip($browser, $filename);
-                    $browser->removeDownloadedFile($filename);
-                    $this->assertCount(2, $files);
+                    try {
+                        $this->checkFilesInZip($browser, $filename, [
+                            'Test.eml' => ['Attached image:'],
+                            'Lines.eml' => ["\nFrom line which needs to be escaped in mbox format."],
+                        ]);
+                    } finally {
+                        $browser->removeDownloadedFile($filename);
+                    }
                 });
 
             // Test attachments download
@@ -95,17 +103,21 @@ public function testMailUI()
                 });
 
             $filename = 'Lines.zip';
-            $files = $this->getFilesFromZip($browser, $filename);
-            $browser->removeDownloadedFile($filename);
-            $expected = ['lines.txt', 'lines_lf.txt'];
-            $this->assertSame($expected, $files);
+            try {
+                $this->checkFilesInZip($browser, $filename, [
+                    'lines.txt' => ["foo\r\nbar\r\ngna"],
+                    'lines_lf.txt' => ["foo\nbar\ngna"],
+                ]);
+            } finally {
+                $browser->removeDownloadedFile($filename);
+            }
         });
     }
 
     /**
-     * Helper to extract files list from downloaded zip file
+     * Helper to extract and check files from downloaded zip file
      */
-    private function getFilesFromZip($browser, $filename)
+    private function checkFilesInZip($browser, $filename, $contents)
     {
         $filename = $browser->getDownloadedFilePath($filename);
 
@@ -134,16 +146,24 @@ private function getFilesFromZip($browser, $filename)
         } while ($filesize1 !== $filesize2);
 
         $zip = new \ZipArchive();
-        $files = [];
+        $partial_names = [];
+        $m = [];
 
         if ($zip->open($filename)) {
             for ($i = 0; $i < $zip->numFiles; $i++) {
-                $files[] = $zip->getNameIndex($i);
+                $this->assertSame(1, preg_match('/([a-z]\w*).*(\.[^.]+)$/i', $zip->getNameIndex($i), $m));
+                $first_word_and_ext = $m[1] . $m[2];
+                $partial_names[] = $first_word_and_ext;
+                if (array_key_exists($first_word_and_ext, $contents)) {
+                    $unzipped = $zip->getFromIndex($i);
+                    foreach ($contents[$first_word_and_ext] as $str) {
+                        $this->assertStringContainsString($str, $unzipped);
+                    }
+                }
             }
         }
+        $this->assertSame(array_keys($contents), $partial_names);
 
         $zip->close();
-
-        return $files;
     }
 }

plugins/zipdownload/tests/MboxFilterTest.php

@@ -0,0 +1,144 @@
+<?php
+
+namespace Roundcube\Plugins\Tests;
+
+use PHPUnit\Framework\TestCase;
+
+class MboxFilterTest extends TestCase
+{
+    private $fp;
+    private $filter;
+
+    #[\Override]
+    public static function setUpBeforeClass(): void
+    {
+        stream_filter_register('test_mbox_filter', '\Roundcube\Plugins\Tests\test_mbox_filter');
+    }
+
+    #[\Override]
+    protected function setUp(): void
+    {
+        $this->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);
+    }
+}

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
 {
+    protected $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;
     }
 }

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