From bb20f3966f1a2151ec60dbdbc869fc3a8fbca65d Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 11 Jun 2020 21:46:05 +0700 Subject: [PATCH 1/5] [ticket/16526] Correctly handle ACP CSV settings PHPBB3-16526 --- phpBB/includes/acp/acp_board.php | 15 ++++++++++++++- phpBB/includes/functions_acp.php | 12 ++++++++++++ phpBB/language/en/acp/common.php | 2 ++ phpBB/phpbb/textformatter/s9e/factory.php | 4 +++- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/acp/acp_board.php b/phpBB/includes/acp/acp_board.php index 70d0c8cf58..8e571de379 100644 --- a/phpBB/includes/acp/acp_board.php +++ b/phpBB/includes/acp/acp_board.php @@ -193,7 +193,7 @@ class acp_board 'allow_post_flash' => array('lang' => 'ALLOW_POST_FLASH', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'allow_smilies' => array('lang' => 'ALLOW_SMILIES', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => false), 'allow_post_links' => array('lang' => 'ALLOW_POST_LINKS', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), - 'allowed_schemes_links' => array('lang' => 'ALLOWED_SCHEMES_LINKS', 'validate' => 'string', 'type' => 'text:0:255', 'explain' => true), + 'allowed_schemes_links' => array('lang' => 'ALLOWED_SCHEMES_LINKS', 'validate' => 'csv', 'type' => 'text:0:255', 'explain' => true), 'allow_nocensors' => array('lang' => 'ALLOW_NO_CENSORS', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'allow_bookmarks' => array('lang' => 'ALLOW_BOOKMARKS', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'enable_post_confirm' => array('lang' => 'VISUAL_CONFIRM_POST', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), @@ -497,6 +497,19 @@ class acp_board $cfg_array = (isset($_REQUEST['config'])) ? $request->variable('config', array('' => ''), true) : $this->new_config; $error = array(); + // Prevalidate allowed URL schemes + if ($mode == 'post') + { + $schemes = array_filter(explode(',', $cfg_array['allowed_schemes_links'])); + foreach ($schemes as $scheme) + { + if (!preg_match('#^[a-z][a-z0-9+\\-.]*$#Di', $scheme)) + { + $error[] = $language->lang('URL_SCHEME_INVALID', $language->lang('ALLOWED_SCHEMES_LINKS'), $scheme); + } + } + } + // We validate the complete config if wished validate_config_vars($display_vars['vars'], $cfg_array, $error); diff --git a/phpBB/includes/functions_acp.php b/phpBB/includes/functions_acp.php index a013af2c89..7c6eebb5a8 100644 --- a/phpBB/includes/functions_acp.php +++ b/phpBB/includes/functions_acp.php @@ -453,8 +453,20 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) $error[] = $language->lang('URL_INVALID', $language->lang($config_definition['lang'])); } + goto validate_string; + + case 'csv': + // Validate comma separated values + $unfiltered_array = explode(',', $cfg_array[$config_name]); + $filtered_array = array_filter($unfiltered_array); + if (!empty($filtered_array) && count($unfiltered_array) !== count($filtered_array)) + { + $error[] = $language->lang('CSV_INVALID', $language->lang($config_definition['lang'])); + } + // no break here + validate_string: case 'string': $length = utf8_strlen($cfg_array[$config_name]); diff --git a/phpBB/language/en/acp/common.php b/phpBB/language/en/acp/common.php index 3705678112..29d0ec5947 100644 --- a/phpBB/language/en/acp/common.php +++ b/phpBB/language/en/acp/common.php @@ -235,6 +235,7 @@ $lang = array_merge($lang, array( 'CRON_NO_SUCH_TASK' => 'Could not find cron task “%s”.', 'CRON_NO_TASK' => 'No cron tasks need to be run right now.', 'CRON_NO_TASKS' => 'No cron tasks could be found.', + 'CSV_INVALID' => 'The provided comma-separated setting “%1$s” is invalid. The values should be delimited by comma only, it should not contain any leading or trailing delimiters.', 'CURRENT_VERSION' => 'Current version', 'DEACTIVATE' => 'Deactivate', @@ -316,6 +317,7 @@ $lang = array_merge($lang, array( 'UCP' => 'User Control Panel', 'URL_INVALID' => 'The provided URL for the setting “%1$s” is invalid.', + 'URL_SCHEME_INVALID' => 'The provided scheme “%2$s” in comma-separated setting “%1$s” is invalid. Scheme should start with a latin character followed by alphanumeric characters, hyphens or dots.', 'USERNAMES_EXPLAIN' => 'Place each username on a separate line.', 'USER_CONTROL_PANEL' => 'User Control Panel', diff --git a/phpBB/phpbb/textformatter/s9e/factory.php b/phpBB/phpbb/textformatter/s9e/factory.php index 7d12abad90..eadd1ba2de 100644 --- a/phpBB/phpbb/textformatter/s9e/factory.php +++ b/phpBB/phpbb/textformatter/s9e/factory.php @@ -218,7 +218,9 @@ class factory implements \phpbb\textformatter\cache_interface { $configurator->urlConfig->disallowScheme($scheme); } - foreach (array_filter(explode(',', $this->config['allowed_schemes_links'])) as $scheme) + + $schemes = array_filter(explode(',', $this->config['allowed_schemes_links'])); + foreach ($schemes as $scheme) { $configurator->urlConfig->allowScheme(trim($scheme)); } From 8c79e9a61eb847071c9f831cff7d8ee83ae29b32 Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 12 Jun 2020 00:59:12 +0700 Subject: [PATCH 2/5] [ticket/16526] Add test PHPBB3-16526 --- tests/functional/acp_post_settings_test.php | 61 +++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 tests/functional/acp_post_settings_test.php diff --git a/tests/functional/acp_post_settings_test.php b/tests/functional/acp_post_settings_test.php new file mode 100644 index 0000000000..3c279e8129 --- /dev/null +++ b/tests/functional/acp_post_settings_test.php @@ -0,0 +1,61 @@ + +* @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_post_settings_test extends phpbb_functional_test_case +{ + public function test_allowed_schemes_links() + { + $this->add_lang(['acp/common', 'acp/board']); + $this->login(); + $this->admin_login(); + + $crawler = self::request('GET', 'adm/index.php?i=acp_board&mode=post&sid=' . $this->sid); + $this->assertContainsLang('ACP_POST_SETTINGS_EXPLAIN', $this->get_content()); + + // Test trailing comma - invalid + $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ + 'config[allowed_schemes_links]' => 'http,https,ftp,' + ]); + $crawler = self::submit($form); + $this->assertContainsLang('WARNING', $crawler->filter('div[class="errorbox"] > h3')->text()); + $this->assertContains($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); + + // Test trailing comma and invalid scheme - invalid + $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ + 'config[allowed_schemes_links]' => 'http,https,2ftp,' + ]); + $crawler = self::submit($form); + $this->assertContainsLang('WARNING', $crawler->filter('div[class="errorbox"] > h3')->text()); + $this->assertContains($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); + $this->assertContains($this->lang('URL_SCHEME_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS'), '2ftp'), $crawler->filter('div[class="errorbox"] > p')->text()); + + // Test empty setting - valid + $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ + 'config[allowed_schemes_links]' => '' + ]); + $crawler = self::submit($form); + $this->assertContainsLang('CONFIG_UPDATED', $crawler->filter('div[class="successbox"] > p')->text()); + + // Restore default setting - 'http,https,ftp' + $crawler = self::request('GET', 'adm/index.php?i=acp_board&mode=post&sid=' . $this->sid); + $this->assertContainsLang('ACP_POST_SETTINGS_EXPLAIN', $this->get_content()); + $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ + 'config[allowed_schemes_links]' => 'http,https,ftp' + ]); + $crawler = self::submit($form); + $this->assertContainsLang('CONFIG_UPDATED', $crawler->filter('div[class="successbox"] > p')->text()); + } +} From 59900357a72081eae3bc843128b0f7f8a807dfd8 Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 12 Jun 2020 10:31:24 +0700 Subject: [PATCH 3/5] [ticket/16526] Revert unneeded s9e/factory change PHPBB3-16526 --- phpBB/phpbb/textformatter/s9e/factory.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/phpBB/phpbb/textformatter/s9e/factory.php b/phpBB/phpbb/textformatter/s9e/factory.php index eadd1ba2de..7d12abad90 100644 --- a/phpBB/phpbb/textformatter/s9e/factory.php +++ b/phpBB/phpbb/textformatter/s9e/factory.php @@ -218,9 +218,7 @@ class factory implements \phpbb\textformatter\cache_interface { $configurator->urlConfig->disallowScheme($scheme); } - - $schemes = array_filter(explode(',', $this->config['allowed_schemes_links'])); - foreach ($schemes as $scheme) + foreach (array_filter(explode(',', $this->config['allowed_schemes_links'])) as $scheme) { $configurator->urlConfig->allowScheme(trim($scheme)); } From 6cd54639b5e3e36fa13c09e5c9910c836230ccb6 Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 14 Aug 2020 04:30:20 +0700 Subject: [PATCH 4/5] [ticket/16526] Adjust test to use better assertions PHPBB3-16526 --- tests/functional/acp_post_settings_test.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/functional/acp_post_settings_test.php b/tests/functional/acp_post_settings_test.php index 3c279e8129..6d03d4c853 100644 --- a/tests/functional/acp_post_settings_test.php +++ b/tests/functional/acp_post_settings_test.php @@ -31,7 +31,7 @@ class phpbb_functional_acp_post_settings_test extends phpbb_functional_test_case ]); $crawler = self::submit($form); $this->assertContainsLang('WARNING', $crawler->filter('div[class="errorbox"] > h3')->text()); - $this->assertContains($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); + $this->assertStringContainsString($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); // Test trailing comma and invalid scheme - invalid $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ @@ -39,8 +39,8 @@ class phpbb_functional_acp_post_settings_test extends phpbb_functional_test_case ]); $crawler = self::submit($form); $this->assertContainsLang('WARNING', $crawler->filter('div[class="errorbox"] > h3')->text()); - $this->assertContains($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); - $this->assertContains($this->lang('URL_SCHEME_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS'), '2ftp'), $crawler->filter('div[class="errorbox"] > p')->text()); + $this->assertStringContainsString($this->lang('CSV_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS')), $crawler->filter('div[class="errorbox"] > p')->text()); + $this->assertStringContainsString($this->lang('URL_SCHEME_INVALID', $this->lang('ALLOWED_SCHEMES_LINKS'), '2ftp'), $crawler->filter('div[class="errorbox"] > p')->text()); // Test empty setting - valid $form = $crawler->selectButton($this->lang('SUBMIT'))->form([ From 13aa1ff76051edef3a288cae7861a304135f300c Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 30 Aug 2020 17:56:12 +0700 Subject: [PATCH 5/5] [ticket/16526] Get rid of goto operator PHPBB3-16526 --- phpBB/includes/functions_acp.php | 34 +++++++++++++++++--------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/phpBB/includes/functions_acp.php b/phpBB/includes/functions_acp.php index 7c6eebb5a8..c8ea536334 100644 --- a/phpBB/includes/functions_acp.php +++ b/phpBB/includes/functions_acp.php @@ -446,27 +446,29 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) switch ($validator[$type]) { case 'url': - $cfg_array[$config_name] = trim($cfg_array[$config_name]); - - if (!empty($cfg_array[$config_name]) && !preg_match('#^' . get_preg_expression('url') . '$#iu', $cfg_array[$config_name])) - { - $error[] = $language->lang('URL_INVALID', $language->lang($config_definition['lang'])); - } - - goto validate_string; - case 'csv': - // Validate comma separated values - $unfiltered_array = explode(',', $cfg_array[$config_name]); - $filtered_array = array_filter($unfiltered_array); - if (!empty($filtered_array) && count($unfiltered_array) !== count($filtered_array)) + if ($validator[$type] == 'url') { - $error[] = $language->lang('CSV_INVALID', $language->lang($config_definition['lang'])); - } + $cfg_array[$config_name] = trim($cfg_array[$config_name]); + if (!empty($cfg_array[$config_name]) && !preg_match('#^' . get_preg_expression('url') . '$#iu', $cfg_array[$config_name])) + { + $error[] = $language->lang('URL_INVALID', $language->lang($config_definition['lang'])); + } + } + else if ($validator[$type] == 'csv') + { + // Validate comma separated values + $unfiltered_array = explode(',', $cfg_array[$config_name]); + $filtered_array = array_filter($unfiltered_array); + if (!empty($filtered_array) && count($unfiltered_array) !== count($filtered_array)) + { + $error[] = $language->lang('CSV_INVALID', $language->lang($config_definition['lang'])); + } + + } // no break here - validate_string: case 'string': $length = utf8_strlen($cfg_array[$config_name]);