From a8218b1eeb10c866f1ec9e02918e82be3e871e2c Mon Sep 17 00:00:00 2001 From: Aleksander Machniak Date: Wed, 17 Jul 2024 20:34:55 +0200 Subject: [PATCH] Fix bug where some messages could get malformed in an import from a MBOX file (#9510) --- CHANGELOG.md | 1 + program/actions/mail/import.php | 107 ++++++++++++++++++++---------- tests/ActionTestCase.php | 2 + tests/Actions/Mail/ImportTest.php | 91 ++++++++++++++++++++++++- tests/src/import.mbox | 30 +++++++++ 5 files changed, 193 insertions(+), 38 deletions(-) create mode 100644 tests/src/import.mbox diff --git a/CHANGELOG.md b/CHANGELOG.md index c36a79c4fb5..6651ae978fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ - Fix fatal error when parsing some TNEF attachments (#9462) - Fix double scrollbar when composing a mail with many plain text lines (#7760) - Fix decoding mail parts with multiple base64-encoded text blocks (#9290) +- Fix bug where some messages could get malformed in an import from a MBOX file (#9510) ## Release 1.6.7 diff --git a/program/actions/mail/import.php b/program/actions/mail/import.php index fcb0d68ffcb..c773176c782 100644 --- a/program/actions/mail/import.php +++ b/program/actions/mail/import.php @@ -59,38 +59,7 @@ public function run($args = []) } foreach ((array) $filepath as $file) { - // read the first few lines to detect header-like structure - $fp = fopen($file, 'r'); - do { - $line = fgets($fp); - } while ($line !== false && trim($line) == ''); - - if (!preg_match('/^From .+/', $line) && !preg_match('/^[a-z-_]+:\s+.+/i', $line)) { - continue; - } - - $message = $lastline = ''; - fseek($fp, 0); - - while (($line = fgets($fp)) !== false) { - // importing mbox file, split by From - lines - if ($lastline === '' && strncmp($line, 'From ', 5) === 0 && strlen($line) > 5) { - if (!empty($message)) { - $imported += (int) self::save_message($folder, $message); - } - - $message = $line; - $lastline = ''; - continue; - } - - $message .= $line; - $lastline = rtrim($line); - } - - if (!empty($message)) { - $imported += (int) self::save_message($folder, $message); - } + $imported += self::import_file($file, $folder); // remove temp files extracted from zip if (is_array($filepath)) { @@ -116,6 +85,13 @@ public function run($args = []) $rcmail->output->send('iframe'); } + /** + * Extract files from a zip archive + * + * @param string $path ZIP file location + * + * @return array List of extracted files + */ public static function zip_extract($path) { if (!class_exists('ZipArchive', false)) { @@ -148,11 +124,69 @@ public static function zip_extract($path) return $files; } - public static function save_message($folder, &$message) + /** + * Parse input file in mbox or eml format, save email messages + * + * @param string $file Filename + * @param string $folder IMAP folder + * + * @return int Number of imported messages + */ + public static function import_file($file, $folder) + { + $fp = fopen($file, 'r'); + + // read the first few lines to detect header-like structure + do { + $line = fgets($fp); + } while ($line !== false && trim($line) == ''); + + $format = null; + if (strncmp($line, 'From ', 5) === 0) { + $format = 'mbox'; + } elseif (preg_match('/^[a-z-_]+:\s+.+/i', $line)) { + $format = 'eml'; + } else { + return 0; + } + + $imported = 0; + $message = ''; + + fseek($fp, 0); + + while (($line = fgets($fp)) !== false) { + // importing mbox file, split by From - lines + if ($format == 'mbox' && strncmp($line, 'From ', 5) === 0 && strlen($line) > 5) { + if (strlen($message)) { + $imported += (int) self::save_message($folder, $message, $format); + } + + $message = $line; + $lastline = ''; + continue; + } + + $message .= $line; + } + + if (strlen($message)) { + $imported += (int) self::save_message($folder, $message, $format); + } + + fclose($fp); + + return $imported; + } + + /** + * Append the message to an IMAP folder + */ + public static function save_message($folder, &$message, $format = 'mbox') { $date = null; - if (strncmp($message, 'From ', 5) === 0) { + if ($format == 'mbox') { // Extract the mbox from_line $pos = strpos($message, "\n"); $from = substr($message, 0, $pos); @@ -176,10 +210,11 @@ public static function save_message($folder, &$message) // ignore } } + + // unquote ">From " lines in message body + $message = preg_replace('/\n>([>]*)From /', "\n\\1From ", $message); } - // unquote ">From " lines in message body - $message = preg_replace('/\n>([>]*)From /', "\n\\1From ", $message); $message = rtrim($message); $rcmail = rcmail::get_instance(); diff --git a/tests/ActionTestCase.php b/tests/ActionTestCase.php index d30d0577fa7..ee40796b7f3 100644 --- a/tests/ActionTestCase.php +++ b/tests/ActionTestCase.php @@ -34,6 +34,8 @@ public static function tearDownAfterClass(): void $rcmail->shutdown(); \html::$doctype = 'xhtml'; + + $_FILES = []; } #[\Override] diff --git a/tests/Actions/Mail/ImportTest.php b/tests/Actions/Mail/ImportTest.php index 82912eeb373..d7de37f032b 100644 --- a/tests/Actions/Mail/ImportTest.php +++ b/tests/Actions/Mail/ImportTest.php @@ -3,6 +3,7 @@ namespace Roundcube\Tests\Actions\Mail; use Roundcube\Tests\ActionTestCase; +use Roundcube\Tests\OutputJsonMock; /** * Test class to test rcmail_action_mail_import @@ -14,8 +15,94 @@ class ImportTest extends ActionTestCase */ public function test_class() { - $object = new \rcmail_action_mail_import(); + $action = new \rcmail_action_mail_import(); + $output = $this->initOutput(\rcmail_action::MODE_AJAX, 'mail', 'import'); - $this->assertInstanceOf(\rcmail_action::class, $object); + $this->assertInstanceOf(\rcmail_action::class, $action); + $this->assertTrue($action->checks()); + + $_SERVER['REQUEST_METHOD'] = 'POST'; + + // No files uploaded case + $this->runAndAssert($action, OutputJsonMock::E_EXIT); + + $result = $output->getOutput(); + + $this->assertSame(['Content-Type: application/json; charset=UTF-8'], $output->headers); + $this->assertSame('import', $result['action']); + // TODO: Assert error message + // $this->assertTrue(strpos($result['exec'], '') !== false); + + // Upload a EML file + $_POST = [ + '_mbox' => 'Test', + ]; + $_FILES['_file'] = [ + 'name' => ['import.eml'], + 'type' => ['message/rfc822'], + 'tmp_name' => [__DIR__ . '/../../src/filename.eml'], + 'error' => [null], + 'size' => [123], + 'id' => [123], + ]; + + // Set expected storage function calls/results + $storage = self::mockStorage() + ->registerFunction('get_folder', 'Test') + ->registerFunction('save_message', 123); + + $this->runAndAssert($action, OutputJsonMock::E_EXIT); + + $result = $output->getOutput(); + + $this->assertTrue(strpos($result['exec'], 'Successfully imported 1 messages') !== false); + $this->assertTrue(strpos($result['exec'], 'this.command("list")') !== false); + + $args = $storage->methodCalls[1]['args']; + $this->assertSame('Test', $args[0]); + $this->assertTrue(strpos($args[1], 'From: "Thomas B." ') === 0); + + // Upload a MBOX file + $_FILES['_file'] = [ + 'name' => ['import.eml'], + 'type' => ['text/plain'], + 'tmp_name' => [__DIR__ . '/../../src/import.mbox'], + 'error' => [null], + 'size' => [123], + 'id' => [123], + ]; + + // Set expected storage function calls/results + $storage = self::mockStorage() + ->registerFunction('get_folder', 'Test') + ->registerFunction('save_message', 1) + ->registerFunction('save_message', 2) + ->registerFunction('save_message', 3); + + $this->runAndAssert($action, OutputJsonMock::E_EXIT); + + $result = $output->getOutput(); + + $this->assertTrue(strpos($result['exec'], 'Successfully imported 3 messages') !== false); + $this->assertTrue(strpos($result['exec'], 'this.command("list")') !== false); + + $args = $storage->methodCalls[1]['args']; + $this->assertSame('Test', $args[0]); + $this->assertTrue(strpos($args[1], 'From: test@rc.net') === 0); + $this->assertStringContainsString('1234', $args[1]); + + $args = $storage->methodCalls[2]['args']; + $this->assertSame('Test', $args[0]); + $this->assertTrue(strpos($args[1], 'From: test1@rc.net') === 0); + $this->assertTrue(strpos($args[1], "\nFrom me") !== false); + + $args = $storage->methodCalls[3]['args']; + $this->assertSame('Test', $args[0]); + $this->assertTrue(strpos($args[1], 'From: test2@rc.net') === 0); + $this->assertStringContainsString('XXXX', $args[1]); + + // TODO: Test error handling + // TODO: Test ZIP file input + $this->markTestIncomplete(); } } diff --git a/tests/src/import.mbox b/tests/src/import.mbox new file mode 100644 index 00000000000..078554b338a --- /dev/null +++ b/tests/src/import.mbox @@ -0,0 +1,30 @@ +From test@rc.net Sun Jul 16 15:06:25 2023 +From: test@rc.net +To: Tom Tester +Subject: Import 1 +MIME-Version: 1.0 +Content-Type: text/plain; +Date: Fri, 23 May 2014 19:44:50 +0200 +Message-ID: + +1234 +From test1@rc.net Sun Jul 16 15:06:25 2023 +From: test1@rc.net +To: Tom Tester +Subject: Import 2 +MIME-Version: 1.0 +Content-Type: text/plain; +Date: Fri, 23 May 2014 19:44:50 +0200 +Message-ID: + +>From me +From test2@rc.net Sun Jul 16 15:06:25 2023 +From: test2@rc.net +To: Tom Tester +Subject: Import 3 +MIME-Version: 1.0 +Content-Type: text/plain; +Date: Fri, 23 May 2014 19:44:50 +0200 +Message-ID: + +XXXX