From 51c9b3c62df71fd70089ff0cf302bab75fef537b Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Aug 2014 12:21:19 +0200 Subject: [PATCH 1/4] Add a test to break the slugifyPath() with folder and file afterwards --- tests/lib/files/mapper.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/lib/files/mapper.php b/tests/lib/files/mapper.php index 48ae95b7e72..0e3e56871cb 100644 --- a/tests/lib/files/mapper.php +++ b/tests/lib/files/mapper.php @@ -58,7 +58,20 @@ class Mapper extends \PHPUnit_Framework_TestCase { $this->assertEquals('D:/folder.name.with.peri-ods', $this->mapper->slugifyPath('D:/folder.name.with.peri ods')); $this->assertEquals('D:/folder.name.with.peri-ods/te-st-2.t-x-t', $this->mapper->slugifyPath('D:/folder.name.with.peri ods/te st.t x t', 2)); $this->assertEquals('D:/folder.name.with.peri-ods/te-st.t-x-t', $this->mapper->slugifyPath('D:/folder.name.with.peri ods/te st.t x t')); + } - + /** + * If a foldername is empty, after we stripped out some unicode and other characters, + * the resulting name must be reproducable otherwise uploading a file into that folder + * will not write the file into the same folder. + */ + public function slugifyEmptyUnicodeFoldername() { + // Slugify the folder + $slugifiedFolder = $this->mapper->slugifyPath('D:/ありがとう'); + $this->assertEquals('D:/' . md5('ありがとう'), $slugifiedFolder); + + // Slugify a file in the folder + $slugifiedFileInUtf8Folder = $this->mapper->slugifyPath('D:/ありがとう/issue6722.txt'); + $this->assertEquals('D:/' . md5('ありがとう') . '/issue6722.txt', $slugifiedFileInUtf8Folder); } } From 6a7be8a36a037dbce813dae93a50301b73054c60 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Aug 2014 14:44:51 +0200 Subject: [PATCH 2/4] Use a data provider instead of calling the function manually everytime --- tests/lib/files/mapper.php | 68 ++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/tests/lib/files/mapper.php b/tests/lib/files/mapper.php index 0e3e56871cb..d513f3ce4b3 100644 --- a/tests/lib/files/mapper.php +++ b/tests/lib/files/mapper.php @@ -33,45 +33,47 @@ class Mapper extends \PHPUnit_Framework_TestCase { $this->mapper = new \OC\Files\Mapper('D:/'); } - public function testSlugifyPath() { - // with extension - $this->assertEquals('D:/text.txt', $this->mapper->slugifyPath('D:/text.txt')); - $this->assertEquals('D:/text-2.txt', $this->mapper->slugifyPath('D:/text.txt', 2)); - $this->assertEquals('D:/a/b/text.txt', $this->mapper->slugifyPath('D:/a/b/text.txt')); + public function slugifyPathData() { + return array( + // with extension + array('D:/text.txt', 'D:/text.txt'), + array('D:/text-2.txt', 'D:/text.txt', 2), + array('D:/a/b/text.txt', 'D:/a/b/text.txt'), - // without extension - $this->assertEquals('D:/text', $this->mapper->slugifyPath('D:/text')); - $this->assertEquals('D:/text-2', $this->mapper->slugifyPath('D:/text', 2)); - $this->assertEquals('D:/a/b/text', $this->mapper->slugifyPath('D:/a/b/text')); + // without extension + array('D:/text', 'D:/text'), + array('D:/text-2', 'D:/text', 2), + array('D:/a/b/text', 'D:/a/b/text'), - // with double dot - $this->assertEquals('D:/text.text.txt', $this->mapper->slugifyPath('D:/text.text.txt')); - $this->assertEquals('D:/text.text-2.txt', $this->mapper->slugifyPath('D:/text.text.txt', 2)); - $this->assertEquals('D:/a/b/text.text.txt', $this->mapper->slugifyPath('D:/a/b/text.text.txt')); - - // foldername and filename with periods - $this->assertEquals('D:/folder.name.with.periods', $this->mapper->slugifyPath('D:/folder.name.with.periods')); - $this->assertEquals('D:/folder.name.with.periods/test-2.txt', $this->mapper->slugifyPath('D:/folder.name.with.periods/test.txt', 2)); - $this->assertEquals('D:/folder.name.with.periods/test.txt', $this->mapper->slugifyPath('D:/folder.name.with.periods/test.txt')); + // with double dot + array('D:/text.text.txt', 'D:/text.text.txt'), + array('D:/text.text-2.txt', 'D:/text.text.txt', 2), + array('D:/a/b/text.text.txt', 'D:/a/b/text.text.txt'), - // foldername and filename with periods and spaces - $this->assertEquals('D:/folder.name.with.peri-ods', $this->mapper->slugifyPath('D:/folder.name.with.peri ods')); - $this->assertEquals('D:/folder.name.with.peri-ods/te-st-2.t-x-t', $this->mapper->slugifyPath('D:/folder.name.with.peri ods/te st.t x t', 2)); - $this->assertEquals('D:/folder.name.with.peri-ods/te-st.t-x-t', $this->mapper->slugifyPath('D:/folder.name.with.peri ods/te st.t x t')); + // foldername and filename with periods + array('D:/folder.name.with.periods', 'D:/folder.name.with.periods'), + array('D:/folder.name.with.periods/test-2.txt', 'D:/folder.name.with.periods/test.txt', 2), + array('D:/folder.name.with.periods/test.txt', 'D:/folder.name.with.periods/test.txt'), + + // foldername and filename with periods and spaces + array('D:/folder.name.with.peri-ods', 'D:/folder.name.with.peri ods'), + array('D:/folder.name.with.peri-ods/te-st-2.t-x-t', 'D:/folder.name.with.peri ods/te st.t x t', 2), + array('D:/folder.name.with.peri-ods/te-st.t-x-t', 'D:/folder.name.with.peri ods/te st.t x t'), + + /** + * If a foldername is empty, after we stripped out some unicode and other characters, + * the resulting name must be reproducable otherwise uploading a file into that folder + * will not write the file into the same folder. + */ + array('D:/' . md5('ありがとう'), 'D:/ありがとう'), + array('D:/' . md5('ありがとう') . '/issue6722.txt', 'D:/ありがとう/issue6722.txt'), + ); } /** - * If a foldername is empty, after we stripped out some unicode and other characters, - * the resulting name must be reproducable otherwise uploading a file into that folder - * will not write the file into the same folder. + * @dataProvider slugifyPathData */ - public function slugifyEmptyUnicodeFoldername() { - // Slugify the folder - $slugifiedFolder = $this->mapper->slugifyPath('D:/ありがとう'); - $this->assertEquals('D:/' . md5('ありがとう'), $slugifiedFolder); - - // Slugify a file in the folder - $slugifiedFileInUtf8Folder = $this->mapper->slugifyPath('D:/ありがとう/issue6722.txt'); - $this->assertEquals('D:/' . md5('ありがとう') . '/issue6722.txt', $slugifiedFileInUtf8Folder); + public function testSlugifyPath($slug, $path, $index = null) { + $this->assertEquals($slug, $this->mapper->slugifyPath($path, $index)); } } From b861e1d69670c023f48c621d1c6bc07016f5400d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Aug 2014 12:48:26 +0200 Subject: [PATCH 3/4] Fix code layout before fixing the function --- lib/private/files/mapper.php | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/private/files/mapper.php b/lib/private/files/mapper.php index 666719da12d..93f6f9bab30 100644 --- a/lib/private/files/mapper.php +++ b/lib/private/files/mapper.php @@ -177,14 +177,12 @@ class Mapper /** * @param integer $index */ - public function slugifyPath($path, $index=null) { + public function slugifyPath($path, $index = null) { $path = $this->stripRootFolder($path, $this->unchangedPhysicalRoot); $pathElements = explode('/', $path); $sluggedElements = array(); - - $last= end($pathElements); - + foreach ($pathElements as $pathElement) { // remove empty elements if (empty($pathElement)) { @@ -196,19 +194,18 @@ class Mapper // apply index to file name if ($index !== null) { - $last= array_pop($sluggedElements); + $last = array_pop($sluggedElements); // if filename contains periods - add index number before last period - if (preg_match('~\.[^\.]+$~i',$last,$extension)){ - array_push($sluggedElements, substr($last,0,-(strlen($extension[0]))).'-'.$index.$extension[0]); + if (preg_match('~\.[^\.]+$~i', $last, $extension)) { + array_push($sluggedElements, substr($last, 0, -(strlen($extension[0]))) . '-' . $index . $extension[0]); } else { // if filename doesn't contain periods add index ofter the last char - array_push($sluggedElements, $last.'-'.$index); - } - + array_push($sluggedElements, $last . '-' . $index); + } } - $sluggedPath = $this->unchangedPhysicalRoot.implode('/', $sluggedElements); + $sluggedPath = $this->unchangedPhysicalRoot . implode('/', $sluggedElements); return $this->stripLast($sluggedPath); } @@ -218,8 +215,7 @@ class Mapper * @param string $text * @return string */ - private function slugify($text) - { + private function slugify($text) { // replace non letter or digits or dots by - $text = preg_replace('~[^\\pL\d\.]+~u', '-', $text); From 1846aebfff14971d4c99a04cfe1a90a77c7591ba Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Aug 2014 14:29:07 +0200 Subject: [PATCH 4/4] Use md5() of the original name instead of uniqid() for slugifying Previously we used uniqid() here. However this means that the behaviour is not reproducable, so when uploading files into a "empty" folder, the folders name is different. If there would be a md5() hash collition, the deduplicate check will spot this and append an index later, so this should not be a problem. Fix #6722 --- lib/private/files/mapper.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/private/files/mapper.php b/lib/private/files/mapper.php index 93f6f9bab30..94dda807c2b 100644 --- a/lib/private/files/mapper.php +++ b/lib/private/files/mapper.php @@ -216,6 +216,7 @@ class Mapper * @return string */ private function slugify($text) { + $originalText = $text; // replace non letter or digits or dots by - $text = preg_replace('~[^\\pL\d\.]+~u', '-', $text); @@ -237,7 +238,17 @@ class Mapper $text = preg_replace('~\.+$~', '', $text); if (empty($text)) { - return uniqid(); + /** + * Item slug would be empty. Previously we used uniqid() here. + * However this means that the behaviour is not reproducible, so + * when uploading files into a "empty" folder, the folders name is + * different. + * + * If there would be a md5() hash collision, the deduplicate check + * will spot this and append an index later, so this should not be + * a problem. + */ + return md5($originalText); } return $text;