From dd77cffb1aab2e8503758f5490251959c3d2e618 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 14 Nov 2019 12:09:50 -0800 Subject: [PATCH 1/6] Escape spaces in target directory path. The script was not working when run in folder where a parent folder name contained a space. --- src/ScriptHandler.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ScriptHandler.php b/src/ScriptHandler.php index 1b4d6cf..3115678 100644 --- a/src/ScriptHandler.php +++ b/src/ScriptHandler.php @@ -44,6 +44,9 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul $command = 'cp -r'; } + // Escape spaces in path. + $targetDirname = preg_replace('/(? Date: Thu, 14 Nov 2019 12:10:58 -0800 Subject: [PATCH 2/6] Update ScriptHandler.php --- src/ScriptHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ScriptHandler.php b/src/ScriptHandler.php index 3115678..43ca287 100644 --- a/src/ScriptHandler.php +++ b/src/ScriptHandler.php @@ -45,7 +45,7 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul } // Escape spaces in path. - $targetDirname = preg_replace('/(? Date: Fri, 15 Nov 2019 12:39:56 -0800 Subject: [PATCH 3/6] phpcs --- src/ScriptHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ScriptHandler.php b/src/ScriptHandler.php index 43ca287..0f2aa4a 100644 --- a/src/ScriptHandler.php +++ b/src/ScriptHandler.php @@ -46,7 +46,7 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul // Escape spaces in path. $targetDirname = preg_replace('/(? Date: Fri, 15 Nov 2019 12:41:03 -0800 Subject: [PATCH 4/6] Unit test for escaping spaces in paths --- tests/ScriptHandlerTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/ScriptHandlerTest.php b/tests/ScriptHandlerTest.php index 1225c1f..94bbe24 100644 --- a/tests/ScriptHandlerTest.php +++ b/tests/ScriptHandlerTest.php @@ -54,4 +54,22 @@ function test_symlinks_creation() ]); ScriptHandler::createSymlinks($this->event, $this->filesystem); } + + /** + * As the `ln` command is executed, it first `cd`s into the destination directory. If there is a space + * anywhere in the path, the `cd` command fails to run properly and the operation fails. + * + * @see https://www.phpliveregex.com/p/uaf#tab-preg-replace + */ + function test_escape_spaces_in_target_dir() + { + $sampledir = '/users/BrianHenryIE/Sites/foo bar/'; + + $expected = '/users/BrianHenryIE/Sites/foo\ bar/'; + + $actual = preg_replace('/(?assertSame($expected, $actual); + } + } From 2316821b18953fc5d8e43fca8df8c71dd8e14a10 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Fri, 15 Nov 2019 12:41:29 -0800 Subject: [PATCH 5/6] Bugfix: data loss when target contains trailing slash I pretty comprehensively tested this with a mix of leading a trailing slashes and found that when the symlink already exists, if the target in the composer json dictionary has a trailing slash, it gets deleted (MacOS anyway). I can't think of any instance (file or folder) where the destination would need a trailing slash, so I'm removing it. --- src/ScriptHandler.php | 3 +++ tests/ScriptHandlerTest.php | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/ScriptHandler.php b/src/ScriptHandler.php index 0f2aa4a..c0b43ab 100644 --- a/src/ScriptHandler.php +++ b/src/ScriptHandler.php @@ -30,6 +30,9 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul $filesystem->remove($targetAbsolutePath); } + // Remove trailing slash that can cause the target to be deleted by ln. + $targetRelativePath = rtrim($targetRelativePath, '/'); + $event->getIO()->write(sprintf( 'Creating symlink for "%s" into "%s"', $sourceRelativePath, diff --git a/tests/ScriptHandlerTest.php b/tests/ScriptHandlerTest.php index 94bbe24..386c9aa 100644 --- a/tests/ScriptHandlerTest.php +++ b/tests/ScriptHandlerTest.php @@ -72,4 +72,27 @@ function test_escape_spaces_in_target_dir() $this->assertSame($expected, $actual); } + /** + * When the destination of the symlink contained a trailing slash, the source would be deleted. + * + * e.g. the config "trunk": "wp-content/plugins/bh-wp-technique-gym/" would delete trunk. + */ + function test_input_sanitization() + { + $this->io + ->expects($this->exactly(1)) + ->method('write') + ->withConsecutive( + ['Creating symlink for "foo" into "bar"'], + ['Creating symlink for "foo2" into "bar"'] + ); + + $this->package->setExtra([ + 'symlinks' => [ + 'foo' => 'bar/', + 'foo2' => 'bar', + ], + ]); + ScriptHandler::createSymlinks($this->event, $this->filesystem); + } } From b9c18a35c3856136b3d4f5b1f66557a2fd64a6f1 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Fri, 15 Nov 2019 13:38:11 -0800 Subject: [PATCH 6/6] Move the bugfix earlier in the function. The test wasn't wrong, it just wasn't very good. --- src/ScriptHandler.php | 6 +++--- tests/ScriptHandlerTest.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ScriptHandler.php b/src/ScriptHandler.php index c0b43ab..5553f02 100644 --- a/src/ScriptHandler.php +++ b/src/ScriptHandler.php @@ -20,6 +20,9 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul $filesystem = $filesystem ?: new Filesystem; foreach ($symlinks as $sourceRelativePath => $targetRelativePath) { + // Remove trailing slash that can cause the target to be deleted by ln. + $targetRelativePath = rtrim($targetRelativePath, '/'); + $sourceAbsolutePath = sprintf('%s/%s', $rootPath, $sourceRelativePath); $targetAbsolutePath = sprintf('%s/%s', $rootPath, $targetRelativePath); if (!file_exists($sourceAbsolutePath)) { @@ -30,9 +33,6 @@ public static function createSymlinks(Event $event, Filesystem $filesystem = nul $filesystem->remove($targetAbsolutePath); } - // Remove trailing slash that can cause the target to be deleted by ln. - $targetRelativePath = rtrim($targetRelativePath, '/'); - $event->getIO()->write(sprintf( 'Creating symlink for "%s" into "%s"', $sourceRelativePath, diff --git a/tests/ScriptHandlerTest.php b/tests/ScriptHandlerTest.php index 386c9aa..511d5da 100644 --- a/tests/ScriptHandlerTest.php +++ b/tests/ScriptHandlerTest.php @@ -73,7 +73,7 @@ function test_escape_spaces_in_target_dir() } /** - * When the destination of the symlink contained a trailing slash, the source would be deleted. + * When the location of the new symlink contained a trailing slash, the original data would be deleted. * * e.g. the config "trunk": "wp-content/plugins/bh-wp-technique-gym/" would delete trunk. */