* Mail - Filter / and ? from filenames when saving to VFS or disk

mail/inc/class.mail_ui.inc.php

@@ -2761,11 +2761,13 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 		{
 			$headers = Horde_Mime_Headers::parseHeaders($message);
 			$subject = str_replace('$$','__',Mail::decode_header($headers['SUBJECT']));
+			$subject = $this->clean_subject_for_filename($subject);
 			Api\Header\Content::safe($message, $subject.".eml", $mime='message/rfc822', $size=0, true, true);
 			echo $message;
 		}
 		else
 		{
+			$subject = $this->clean_subject_for_filename($subject);
 			Api\Header\Content::safe($message, $subject.".eml", $mime='text/html', $size=0, true, false);
 			print '<pre>'. htmlspecialchars($message, ENT_NOQUOTES|ENT_SUBSTITUTE, 'utf-8') .'</pre>';
 		}
@@ -2844,11 +2846,11 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 			if (Vfs::is_dir($path))
 			{
 				$headers = $this->mail_bo->getMessageHeader($uid,$partID,true,false,$mailbox);
-				$file = $dir . '/'.preg_replace('$[\f\n\t\v\\:*#?<>\|/]$',"_",$headers['SUBJECT']).'.eml';
+				$file = $dir . '/'.$this->clean_subject_for_filename($headers['SUBJECT']).'.eml';
 			}
 			else
 			{
-				$file = $path;
+				$file = $dir . '/' . $this->clean_subject_for_filename(str_replace($dir.'/', '', $path));
 			}
 
 			// Check if file already exists, then try to assign a none existance filename
@@ -2915,7 +2917,8 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 		else
 		{
 			$dir = Vfs::dirname($path);
-			$filename = Vfs::basename($path);
+			// Need to deal with any ? here, or basename will truncate
+			$filename = $this->clean_subject_for_filename(str_replace('?','_',Vfs::basename($path)));
 		}
 
 		if (!Vfs::is_writable($dir))
@@ -2996,7 +2999,7 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 				$attachment = $this->mail_bo->getAttachment($params['uid'],$params['part'],$params['is_winmail'],false);
 			}
 
-			$file = $dir. '/' . ($filename ? $filename : preg_replace('$[\f\n\t\v\\:*#?<>\|/]$',"_",$attachment['filename']));
+			$file = $dir. '/' . ($filename ? $filename : $this->clean_subject_for_filename($attachment['filename']));
 
 			$counter = 1;
 			$tmp_file = $file;
@@ -3031,6 +3034,25 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 		return $res;
 	}
 
+	/**
+	 * Make the provided filename safe to store in the VFS
+	 *
+	 * Some characters found in subjects that cause problems if we try to put
+	 * them as filenames (Windows) so we remove any characters that might result
+	 * in additional directories, or issues on Windows.
+	 *
+	 * Under Windows the characters < > ? " : | \ / * are not allowed.
+	 * % causes problems with VFS UI
+	 *
+	 * @param string $filename
+	 * @return Cleaned filename, with problematic characters replaced with '_'.
+	 */
+	protected function clean_subject_for_filename($filename)
+	{
+		static $filter_pattern = '$[\f\n\t\v\\:*#?<>%"\|/\\\?]$';
+		return preg_replace($filter_pattern, "_", $filename);
+	}
+
 	/**
 	 * Zip all attachments and send to user
 	 * @param string $message_id = null
@@ -3070,7 +3092,7 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 
 		// Add subject to path, so it gets used as the file name, replacing ':'
 		// as it seems to cause an error
-		$path = $temp_path . '/' . ($header['SUBJECT'] ? Vfs::encodePathComponent(str_replace(':','-', $header['SUBJECT'])) : lang('mail')) .'/';
+		$path = $temp_path . '/' . ($header['SUBJECT'] ? Vfs::encodePathComponent($this->clean_subject_for_filename(str_replace(':','-', $header['SUBJECT']))) : lang('mail')) .'/';
 		if(!Vfs::mkdir($path, 0700, true))
 		{
 			echo "Unable to open temp directory $path";
@@ -3109,7 +3131,7 @@ $filter['before']= date("d-M-Y", $cutoffdate2);
 					pathinfo($file['filename'], PATHINFO_EXTENSION);
 			}
 			// Strip special characters to make sure the files are visible for all OS (windows has issues)
-			$target_name = iconv($file['charset'] ? $file['charset'] : $GLOBALS['egw_info']['server']['system_charset'], 'ASCII//IGNORE', $file['filename']);
+			$target_name = $this->clean_subject_for_filename(iconv($file['charset'] ? $file['charset'] : $GLOBALS['egw_info']['server']['system_charset'], 'ASCII//IGNORE', $file['filename']));
 
 			if (!($fp = Vfs::fopen($path.$target_name,'wb')) ||
 				!(!fseek($attachment['attachment'], 0, SEEK_SET) && stream_copy_to_stream($attachment['attachment'], $fp)))

mail/js/app.js

@@ -2999,7 +2999,8 @@ app.classes.mail = AppJS.extend(
 			var _id = _elems[i].id;
 			var dataElem = egw.dataGetUIDdata(_id);
 			var subject = dataElem? dataElem.data.subject: _elems[i].subject;
-			var filename = subject.replace(/[\f\n\t\v]/g,"_")|| 'unknown';
+			// Replace these now, they really cause problems later
+			var filename = subject.replace(/[\f\n\t\v\/\\\?]/g,"_")|| 'unknown';
 			ids.push(_id);
 			names.push(filename+'.eml');
 		}

mail/tests/SaveToVfsTest.php

@@ -0,0 +1,116 @@
+<?php
+
+/**
+ * Test the save to VFS functions
+ *
+ * @link http://www.egroupware.org
+ * @author Nathan Gray
+ * @package mail
+ * @copyright (c) 2018 by Nathan Gray
+ * @license http://opensource.org/licenses/gpl-license.php GPL - GNU General Public License
+ */
+
+
+namespace EGroupware\Mail;
+
+require_once realpath(__DIR__.'/../../api/tests/AppTest.php');	// Application test base
+
+use Egroupware\Api;
+
+class SaveToVfsTest extends \EGroupware\Api\AppTest
+{
+
+	// Mail object under test
+	protected $ui = null;
+
+	/**
+	 * Create a custom status we can use to test
+	 */
+	public static function setUpBeforeClass()
+	{
+		parent::setUpBeforeClass();
+
+	}
+	public static function tearDownAfterClass()
+	{
+
+		// Have to remove custom status first, before the DB is gone
+		parent::tearDownAfterClass();
+	}
+
+	public function setUp()
+	{
+		$this->ui = new VfsTestMailUi(false);
+	}
+
+	public function tearDown()
+	{
+		$this->ui = null;
+	}
+
+	/**
+	 * Test that we make nice filenames for the VFS
+	 *
+	 * Under Windows the characters < > ? " : | \ / * are not allowed.
+	 * % causes problems with VFS UI
+	 *
+	 * @param String $filename
+	 * @dataProvider filenameProvider
+	 */
+	public function testVfsFilename($filename, $replacements)
+	{
+		$cleaned = $this->ui->clean_subject_for_filename($filename);
+
+		$this->assertNotContains('<', $cleaned);
+		$this->assertNotContains('>', $cleaned);
+		$this->assertNotContains('"', $cleaned);
+		$this->assertNotContains('#', $cleaned);
+		$this->assertNotContains(':', $cleaned);
+		$this->assertNotContains('|', $cleaned);
+		$this->assertNotContains('\\', $cleaned);
+		$this->assertNotContains('*', $cleaned);
+		$this->assertNotContains('/', $cleaned);
+		$this->assertNotContains('?', $cleaned);
+
+		// Length should stay the same
+		$this->assertEquals(strlen($filename), strlen($cleaned));
+
+		if(!$replacements)
+		{
+			$this->assertEquals($filename, $cleaned);
+		}
+		else
+		{
+			$this->assertNotEquals($filename, $cleaned);
+		}
+	}
+
+	public function filenameProvider()
+	{
+		return array(
+			array('Normal! All allowed (!@$^&) {\'} []', false),
+			array('Contains a >', true),
+			array('Contains a <', true),
+			array('Contains a "', true),
+			array('Contains a #', true),
+			array('Contains a :', true),
+			array('Contains a |', true),
+			array('Contains a \ ', true),
+			array('Contains a *', true),
+			array('Contains a /', true),
+			array('Contains a ?', true),
+			array('Contains a %', true),
+			array('This one contains them all < > " : | \ * / ? % are not allowed', true)
+		);
+	}
+}
+
+
+class VfsTestMailUi extends \mail_ui
+{
+	// Expose for testing
+	public function clean_subject_for_filename($filename)
+	{
+		return parent::clean_subject_for_filename($filename);
+	}
+}