Merge pull request #45 from phpbb/ticket/security/234

[ticket/security/234] Add URL validation for input fields & main site URL

phpBB/includes/acp/acp_board.php

@@ -30,10 +30,13 @@ class acp_board
 
 	function main($id, $mode)
 	{
-		global $user, $template, $request;
+		global $user, $template, $request, $language;
 		global $config, $phpbb_root_path, $phpEx;
 		global $cache, $phpbb_container, $phpbb_dispatcher, $phpbb_log;
 
+		/** @var \phpbb\language\language $language Language object */
+		$language = $phpbb_container->get('language');
+
 		$user->add_lang('acp/board');
 
 		$submit = (isset($_POST['submit']) || isset($_POST['allow_quick_reply_enable'])) ? true : false;
@@ -56,7 +59,7 @@ class acp_board
 						'legend1'				=> 'ACP_BOARD_SETTINGS',
 						'sitename'				=> array('lang' => 'SITE_NAME',				'validate' => 'string',	'type' => 'text:40:255', 'explain' => false),
 						'site_desc'				=> array('lang' => 'SITE_DESC',				'validate' => 'string',	'type' => 'text:40:255', 'explain' => false),
-						'site_home_url'			=> array('lang' => 'SITE_HOME_URL',			'validate' => 'string',	'type' => 'url:40:255', 'explain' => true),
+						'site_home_url'			=> array('lang' => 'SITE_HOME_URL',			'validate' => 'url',	'type' => 'url:40:255', 'explain' => true),
 						'site_home_text'		=> array('lang' => 'SITE_HOME_TEXT',		'validate' => 'string',	'type' => 'text:40:255', 'explain' => true),
 						'board_index_text'		=> array('lang' => 'BOARD_INDEX_TEXT',		'validate' => 'string',	'type' => 'text:40:255', 'explain' => true),
 						'board_disable'			=> array('lang' => 'DISABLE_BOARD',			'validate' => 'bool',	'type' => 'custom', 'method' => 'board_disable', 'explain' => true),

phpBB/includes/functions_acp.php

@@ -419,7 +419,7 @@ function build_cfg_template($tpl_type, $key, &$new_ary, $config_key, $vars)
 */
 function validate_config_vars($config_vars, &$cfg_array, &$error)
 {
-	global $phpbb_root_path, $user, $phpbb_dispatcher, $phpbb_filesystem;
+	global $phpbb_root_path, $user, $phpbb_dispatcher, $phpbb_filesystem, $language;
 
 	$type	= 0;
 	$min	= 1;
@@ -442,6 +442,16 @@ function validate_config_vars($config_vars, &$cfg_array, &$error)
 		// Validate a bit. ;) (0 = type, 1 = min, 2= max)
 		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']));
+				}
+
+			// no break here
+
 			case 'string':
 				$length = utf8_strlen($cfg_array[$config_name]);
 

phpBB/language/en/acp/common.php

@@ -325,6 +325,7 @@ $lang = array_merge($lang, array(
 	'TOTAL_SIZE'			=> 'Total size',
 
 	'UCP'					=> 'User Control Panel',
+	'URL_INVALID'			=> 'The provided URL for the setting “%1$s” is invalid.',
 	'USERNAMES_EXPLAIN'		=> 'Place each username on a separate line.',
 	'USER_CONTROL_PANEL'	=> 'User Control Panel',
 

tests/functions_acp/validate_config_vars_test.php

@@ -19,10 +19,11 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case
 	{
 		parent::setUp();
 
-		global $user;
+		global $language, $user;
 
 		$user = new phpbb_mock_user();
 		$user->lang = new phpbb_mock_lang();
+		$language = $user->lang;
 	}
 
 	/**
@@ -44,6 +45,7 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case
 					'test_int_32'			=> array('lang' => 'TEST_INT',			'validate' => 'int:32'),
 					'test_int_32_64'		=> array('lang' => 'TEST_INT',			'validate' => 'int:32:64'),
 					'test_lang'				=> array('lang' => 'TEST_LANG',			'validate' => 'lang'),
+					'test_url'				=> array('lang' => 'TEST_URL',			'validate' => 'url'),
 					/*
 					'test_sp'				=> array('lang' => 'TEST_SP',			'validate' => 'script_path'),
 					'test_rpath'			=> array('lang' => 'TEST_RPATH',		'validate' => 'rpath'),
@@ -64,6 +66,7 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case
 					'test_int_32'		=> 32,
 					'test_int_32_64'	=> 48,
 					'test_lang'			=> 'en',
+					'test_url'			=> 'http://foobar.com',
 				),
 			),
 		);
@@ -148,6 +151,11 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case
 				array('test_lang'		=> 'this_is_no_language'),
 				array('WRONG_DATA_LANG'),
 			),
+			array(
+				array('test_url'		=> array('lang' => 'TEST_URL',			'validate' => 'url')),
+				array('test_url'		=> 'javascript://foobar.com'),
+				array('URL_INVALID TEST_URL'),
+			),
 		);
 	}