From 19421fcdef62e50ea335967cc7e4487e7548db87 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 15:02:41 +0100 Subject: [ticket/13568] Validate imagick path as readable absolute path PHPBB3-13568 --- phpBB/adm/index.php | 36 ++++++++++++++++++++++++++++++++++ phpBB/includes/acp/acp_attachments.php | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/phpBB/adm/index.php b/phpBB/adm/index.php index 85908476a1..885c8f0a1c 100644 --- a/phpBB/adm/index.php +++ b/phpBB/adm/index.php @@ -562,6 +562,42 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) } break; + + // Absolute file path + case 'wapath': + case 'apath': + if (!$cfg_array[$config_name]) + { + break; + } + + $cfg_array[$config_name] = trim($cfg_array[$config_name]); + + // Make sure no NUL byte is present... + if (strpos($cfg_array[$config_name], "\0") !== false || strpos($cfg_array[$config_name], '%00') !== false) + { + $cfg_array[$config_name] = ''; + break; + } + + if (!file_exists($cfg_array[$config_name])) + { + $error[] = sprintf($user->lang['DIRECTORY_DOES_NOT_EXIST'], $cfg_array[$config_name]); + } + else if (!is_dir($cfg_array[$config_name])) + { + $error[] = sprintf($user->lang['DIRECTORY_NOT_DIR'], $cfg_array[$config_name]); + } + + // Check if the path is writable + if ($config_definition['validate'] === 'wapath') + { + if (file_exists($cfg_array[$config_name]) && !phpbb_is_writable($cfg_array[$config_name])) + { + $error[] = sprintf($user->lang['DIRECTORY_NOT_WRITABLE'], $cfg_array[$config_name]); + } + } + break; } } diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 147783feae..325c6b63cb 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -127,7 +127,7 @@ class acp_attachments 'img_create_thumbnail' => array('lang' => 'CREATE_THUMBNAIL', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'img_max_thumb_width' => array('lang' => 'MAX_THUMB_WIDTH', 'validate' => 'int', 'type' => 'text:7:15', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_min_thumb_filesize' => array('lang' => 'MIN_THUMB_FILESIZE', 'validate' => 'int', 'type' => 'text:7:15', 'explain' => true, 'append' => ' ' . $user->lang['BYTES']), - 'img_imagick' => array('lang' => 'IMAGICK_PATH', 'validate' => 'path', 'type' => 'text:20:200', 'explain' => true, 'append' => '  [ ' . $user->lang['SEARCH_IMAGICK'] . ' ]'), + 'img_imagick' => array('lang' => 'IMAGICK_PATH', 'validate' => 'apath', 'type' => 'text:20:200', 'explain' => true, 'append' => '  [ ' . $user->lang['SEARCH_IMAGICK'] . ' ]'), 'img_max' => array('lang' => 'MAX_IMAGE_SIZE', 'validate' => 'int', 'type' => 'dimension:3:4', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_link' => array('lang' => 'IMAGE_LINK_SIZE', 'validate' => 'int', 'type' => 'dimension:3:4', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), ) -- cgit v1.2.1 From 99574cf5e536a4ed81d7c7f291f3fc9e9f2466a2 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 16:11:00 +0100 Subject: [ticket/13568] Add functional test for imagick path setting PHPBB3-13568 --- tests/functional/acp_attachments_test.php | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 tests/functional/acp_attachments_test.php diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php new file mode 100644 index 0000000000..6b1d1dc8df --- /dev/null +++ b/tests/functional/acp_attachments_test.php @@ -0,0 +1,44 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +/** + * @group functional + */ +class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case +{ + public function data_imagick_path() + { + return array( + array('/usr/bin', 'Configuration updated successfully'), + array('/usr/bin/', 'Configuration updated successfully'), + array('/usr/nope', 'The entered path “/usr/nope” does not exist.'), + array('mkdir /usr/test', 'The entered path “mkdir /usr/test” does not exist.'), + ); + } + + /** + * @dataProvider data_imagick_path + */ + public function test_imagick_path($imagick_path, $expected) + { + $this->login(); + $this->admin_login(); + + $crawler = self::request('GET', 'adm/index.php?i=attachments&mode=attach&sid=' . $this->sid); + + $form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path)); + + $crawler = self::submit($form); + $this->assertContains($expected, $crawler->text()); + } +} -- cgit v1.2.1 From a93df0e5112962503abc88469c1cc77cef862745 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 18:30:05 +0100 Subject: [ticket/13568] Use more descriptive validation names and merge with path block PHPBB3-13568 --- phpBB/adm/index.php | 47 ++++++---------------------------- phpBB/includes/acp/acp_attachments.php | 2 +- 2 files changed, 9 insertions(+), 40 deletions(-) diff --git a/phpBB/adm/index.php b/phpBB/adm/index.php index 885c8f0a1c..52fe315cc0 100644 --- a/phpBB/adm/index.php +++ b/phpBB/adm/index.php @@ -524,6 +524,9 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) $cfg_array[$config_name] = trim($destination); + // Absolute file path + case 'absolute_path': + case 'absolute_path_writable': // Path being relative (still prefixed by phpbb_root_path), but with the ability to escape the root dir... case 'path': case 'wpath': @@ -542,12 +545,14 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) break; } - if (!file_exists($phpbb_root_path . $cfg_array[$config_name])) + $path = ($config_definition['validate'] === 'wpath' || $config_definition['validate'] === 'path') ? $phpbb_root_path . $cfg_array[$config_name] : $cfg_array[$config_name]; + + if (!file_exists($path)) { $error[] = sprintf($user->lang['DIRECTORY_DOES_NOT_EXIST'], $cfg_array[$config_name]); } - if (file_exists($phpbb_root_path . $cfg_array[$config_name]) && !is_dir($phpbb_root_path . $cfg_array[$config_name])) + if (file_exists($path) && !is_dir($path)) { $error[] = sprintf($user->lang['DIRECTORY_NOT_DIR'], $cfg_array[$config_name]); } @@ -555,49 +560,13 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) // Check if the path is writable if ($config_definition['validate'] == 'wpath' || $config_definition['validate'] == 'rwpath') { - if (file_exists($phpbb_root_path . $cfg_array[$config_name]) && !phpbb_is_writable($phpbb_root_path . $cfg_array[$config_name])) + if (file_exists($path) && !phpbb_is_writable($path)) { $error[] = sprintf($user->lang['DIRECTORY_NOT_WRITABLE'], $cfg_array[$config_name]); } } break; - - // Absolute file path - case 'wapath': - case 'apath': - if (!$cfg_array[$config_name]) - { - break; - } - - $cfg_array[$config_name] = trim($cfg_array[$config_name]); - - // Make sure no NUL byte is present... - if (strpos($cfg_array[$config_name], "\0") !== false || strpos($cfg_array[$config_name], '%00') !== false) - { - $cfg_array[$config_name] = ''; - break; - } - - if (!file_exists($cfg_array[$config_name])) - { - $error[] = sprintf($user->lang['DIRECTORY_DOES_NOT_EXIST'], $cfg_array[$config_name]); - } - else if (!is_dir($cfg_array[$config_name])) - { - $error[] = sprintf($user->lang['DIRECTORY_NOT_DIR'], $cfg_array[$config_name]); - } - - // Check if the path is writable - if ($config_definition['validate'] === 'wapath') - { - if (file_exists($cfg_array[$config_name]) && !phpbb_is_writable($cfg_array[$config_name])) - { - $error[] = sprintf($user->lang['DIRECTORY_NOT_WRITABLE'], $cfg_array[$config_name]); - } - } - break; } } diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 325c6b63cb..bffe6f7db3 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -127,7 +127,7 @@ class acp_attachments 'img_create_thumbnail' => array('lang' => 'CREATE_THUMBNAIL', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'img_max_thumb_width' => array('lang' => 'MAX_THUMB_WIDTH', 'validate' => 'int', 'type' => 'text:7:15', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_min_thumb_filesize' => array('lang' => 'MIN_THUMB_FILESIZE', 'validate' => 'int', 'type' => 'text:7:15', 'explain' => true, 'append' => ' ' . $user->lang['BYTES']), - 'img_imagick' => array('lang' => 'IMAGICK_PATH', 'validate' => 'apath', 'type' => 'text:20:200', 'explain' => true, 'append' => '  [ ' . $user->lang['SEARCH_IMAGICK'] . ' ]'), + 'img_imagick' => array('lang' => 'IMAGICK_PATH', 'validate' => 'absolute_path', 'type' => 'text:20:200', 'explain' => true, 'append' => '  [ ' . $user->lang['SEARCH_IMAGICK'] . ' ]'), 'img_max' => array('lang' => 'MAX_IMAGE_SIZE', 'validate' => 'int', 'type' => 'dimension:3:4', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_link' => array('lang' => 'IMAGE_LINK_SIZE', 'validate' => 'int', 'type' => 'dimension:3:4', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), ) -- cgit v1.2.1 From b53fd867be7db3bf8025a94893c4211b99fbd21e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 18:33:53 +0100 Subject: [ticket/13568] Add more test cases for imagick path PHPBB3-13568 --- tests/functional/acp_attachments_test.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php index 6b1d1dc8df..9f6b61ccd7 100644 --- a/tests/functional/acp_attachments_test.php +++ b/tests/functional/acp_attachments_test.php @@ -21,8 +21,10 @@ class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case return array( array('/usr/bin', 'Configuration updated successfully'), array('/usr/bin/', 'Configuration updated successfully'), + array('C:\Windows\system32', 'The entered path “C:\Windows\system32” does not exist.'), array('/usr/nope', 'The entered path “/usr/nope” does not exist.'), array('mkdir /usr/test', 'The entered path “mkdir /usr/test” does not exist.'), + array('/usr/bin/which', 'The entered path “/usr/bin/which” is not a directory.'), ); } -- cgit v1.2.1 From d50cec998ccf3782a3e1d6dd5031c76e32e60790 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 18:44:48 +0100 Subject: [ticket/13568] Correctly check rpath and rwpath validation options PHPBB3-13568 --- phpBB/adm/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/adm/index.php b/phpBB/adm/index.php index 52fe315cc0..9858d61e7f 100644 --- a/phpBB/adm/index.php +++ b/phpBB/adm/index.php @@ -545,7 +545,7 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) break; } - $path = ($config_definition['validate'] === 'wpath' || $config_definition['validate'] === 'path') ? $phpbb_root_path . $cfg_array[$config_name] : $cfg_array[$config_name]; + $path = in_array($config_definition['validate'], array('wpath', 'path', 'rpath', 'rwpath')) ? $phpbb_root_path . $cfg_array[$config_name] : $cfg_array[$config_name]; if (!file_exists($path)) { -- cgit v1.2.1 From ffe0e46e82f712fb11dacf5be7d482c84ab35263 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Feb 2015 20:39:58 +0100 Subject: [ticket/13568] Add imagick tests for windows PHPBB3-13568 --- tests/functional/acp_attachments_test.php | 48 +++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php index 9f6b61ccd7..c1290bf1ba 100644 --- a/tests/functional/acp_attachments_test.php +++ b/tests/functional/acp_attachments_test.php @@ -16,23 +16,55 @@ */ class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case { - public function data_imagick_path() + public function data_imagick_path_linux() { return array( array('/usr/bin', 'Configuration updated successfully'), - array('/usr/bin/', 'Configuration updated successfully'), - array('C:\Windows\system32', 'The entered path “C:\Windows\system32” does not exist.'), - array('/usr/nope', 'The entered path “/usr/nope” does not exist.'), - array('mkdir /usr/test', 'The entered path “mkdir /usr/test” does not exist.'), + array('/usr/foobar', 'The entered path “/usr/foobar” does not exist.'), array('/usr/bin/which', 'The entered path “/usr/bin/which” is not a directory.'), ); } /** - * @dataProvider data_imagick_path + * @dataProvider data_imagick_path_linux */ - public function test_imagick_path($imagick_path, $expected) + public function test_imagick_path_linux($imagick_path, $expected) { + if (strtolower(substr(PHP_OS, 0, 5)) !== 'linux') + { + $this->markTestSkipped('Unable to test linux specific paths on other OS.'); + } + + $this->login(); + $this->admin_login(); + + $crawler = self::request('GET', 'adm/index.php?i=attachments&mode=attach&sid=' . $this->sid); + + $form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path)); + + $crawler = self::submit($form); + $this->assertContains($expected, $crawler->filter('#main')->text()); + } + + public function data_imagick_path_windows() + { + return array( + array('C:\Windows', 'Configuration updated successfully'), + array('C:\Windows\foobar1', 'The entered path “C:\Windows\foobar1” does not exist.'), + array('C:\Windows\explorer.exe', 'The entered path “C:\Windows\explorer.exe” is not a directory.'), + ); + } + + /** + * @dataProvider data_imagick_path_linux + */ + public function test_imagick_path_windows($imagick_path, $expected) + { + if (strtolower(substr(PHP_OS, 0, 3)) !== 'win') + { + $this->markTestSkipped('Unable to test windows specific paths on other OS.'); + } + $this->login(); $this->admin_login(); @@ -41,6 +73,6 @@ class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case $form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path)); $crawler = self::submit($form); - $this->assertContains($expected, $crawler->text()); + $this->assertContains($expected, $crawler->filter('#main')->text()); } } -- cgit v1.2.1 From a3be5310207193d7ca40591530bfd4183f83c9a5 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 3 Feb 2015 16:34:41 +0100 Subject: [ticket/13568] Use correct data provider on windows PHPBB3-13568 --- tests/functional/acp_attachments_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php index c1290bf1ba..8e810a508a 100644 --- a/tests/functional/acp_attachments_test.php +++ b/tests/functional/acp_attachments_test.php @@ -56,7 +56,7 @@ class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case } /** - * @dataProvider data_imagick_path_linux + * @dataProvider data_imagick_path_windows */ public function test_imagick_path_windows($imagick_path, $expected) { -- cgit v1.2.1 From 7c5d872344a59f0fe2323d34885e543a09111ca7 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 6 Feb 2015 10:15:36 +0100 Subject: [ticket/13568] Also check if absolute_path_writable is writable PHPBB3-13568 --- phpBB/adm/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/adm/index.php b/phpBB/adm/index.php index 9858d61e7f..49c4be09dc 100644 --- a/phpBB/adm/index.php +++ b/phpBB/adm/index.php @@ -558,7 +558,7 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) } // Check if the path is writable - if ($config_definition['validate'] == 'wpath' || $config_definition['validate'] == 'rwpath') + if ($config_definition['validate'] == 'wpath' || $config_definition['validate'] == 'rwpath' || $config_definition['validate'] === 'absolute_path_writable') { if (file_exists($path) && !phpbb_is_writable($path)) { -- cgit v1.2.1