Merge pull request #3107 from marc1706/ticket/13280

[ticket/13280] Properly format the current page and add sanitizer to tests

* marc1706/ticket/13280:
  [ticket/13280] Remove unneeded str_replace in build_url()
  [ticket/13280] Only run sanitizer for server superglobal and modify tests
  [ticket/13280] Seperate server sanitizer call and add comment
  [ticket/13280] Add additional sanitizer for ampersands in server superglobal
  [ticket/13280] Correctly format user page for build_url()
  [ticket/13280] Properly format the current page and add sanitizer to tests

phpBB/phpbb/session.php

@@ -43,7 +43,7 @@ class session
 
 		// First of all, get the request uri...
 		$script_name = $symfony_request->getScriptName();
-		$args = explode('&', $symfony_request->getQueryString());
+		$args = explode('&', $symfony_request->getQueryString());
 
 		// If we are unable to get the script name we use REQUEST_URI as a failover and note it within the page array for easier support...
 		if (!$script_name)
@@ -61,8 +61,8 @@ class session
 
 		// Since some browser do not encode correctly we need to do this with some "special" characters...
 		// " -> %22, ' => %27, < -> %3C, > -> %3E
-		$find = array('"', "'", '<', '>');
+		$find = array('"', "'", '<', '>', '&quot;', '&lt;', '&gt;');
-		$replace = array('%22', '%27', '%3C', '%3E');
+		$replace = array('%22', '%27', '%3C', '%3E', '%22', '%3C', '%3E');
 
 		foreach ($args as $key => $argument)
 		{

phpBB/phpbb/symfony_request.php

@@ -30,6 +30,12 @@ class symfony_request extends Request
 			$type_cast_helper->set_var($value, $value, gettype($value), true);
 		};
 
+		// This function is meant for additional handling of server variables
+		$server_sanitizer = function(&$value, $key) use ($sanitizer) {
+			$sanitizer($value, $key);
+			$value = str_replace('&', '&', $value);
+		};
+
 		$get_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::GET);
 		$post_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::POST);
 		$server_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::SERVER);
@@ -38,10 +44,12 @@ class symfony_request extends Request
 
 		array_walk_recursive($get_parameters, $sanitizer);
 		array_walk_recursive($post_parameters, $sanitizer);
-		array_walk_recursive($server_parameters, $sanitizer);
 		array_walk_recursive($files_parameters, $sanitizer);
 		array_walk_recursive($cookie_parameters, $sanitizer);
 
+		// Run special sanitizer for server superglobal
+		array_walk_recursive($server_parameters, $server_sanitizer);
+
 		parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters);
 	}
 }

tests/functions/build_url_test.php

@@ -69,6 +69,11 @@ class phpbb_build_url_test extends phpbb_test_case
 				array('f', 'style', 't'),
 				'http://test.phpbb.com/viewtopic.php?',
 			),
+			array(
+				'posting.php?f=2&mode=delete&p=20%22%3Cscript%3Ealert%281%29%3B%3C%2Fscript%3E',
+				false,
+				'phpBB/posting.php?f=2&mode=delete&p=20%22%3Cscript%3Ealert%281%29%3B%3C%2Fscript%3E',
+			)
 		);
 	}
 
@@ -80,6 +85,7 @@ class phpbb_build_url_test extends phpbb_test_case
 		global $user, $phpbb_root_path;
 
 		$user->page['page'] = $page;
+
 		$output = build_url($strip_vars);
 
 		$this->assertEquals($expected, $output);

tests/security/extract_current_page_test.php

@@ -37,16 +37,16 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base
 		));
 		$symfony_request->expects($this->any())
 			->method('getScriptName')
-			->will($this->returnValue($url));
+			->will($this->returnValue($this->sanitizer($url)));
 		$symfony_request->expects($this->any())
 			->method('getQueryString')
-			->will($this->returnValue($query_string));
+			->will($this->returnValue($this->sanitizer($query_string)));
 		$symfony_request->expects($this->any())
 			->method('getBasePath')
 			->will($this->returnValue($server['REQUEST_URI']));
-		$symfony_request->expects($this->any())
+		$symfony_request->expects($this->sanitizer($this->any()))
 			->method('getPathInfo')
-			->will($this->returnValue('/'));
+			->will($this->returnValue($this->sanitizer('/')));
 		$result = \phpbb\session::extract_current_page('./');
 
 		$label = 'Running extract_current_page on ' . $query_string . ' with PHP_SELF filled.';
@@ -65,20 +65,32 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base
 		));
 		$symfony_request->expects($this->any())
 			->method('getScriptName')
-			->will($this->returnValue($url));
+			->will($this->returnValue($this->sanitizer($url)));
 		$symfony_request->expects($this->any())
 			->method('getQueryString')
-			->will($this->returnValue($query_string));
+			->will($this->returnValue($this->sanitizer($query_string)));
 		$symfony_request->expects($this->any())
 			->method('getBasePath')
-			->will($this->returnValue($server['REQUEST_URI']));
+			->will($this->returnValue($this->sanitizer($server['REQUEST_URI'])));
 		$symfony_request->expects($this->any())
 			->method('getPathInfo')
-			->will($this->returnValue('/'));
+			->will($this->returnValue($this->sanitizer('/')));
 
 		$result = \phpbb\session::extract_current_page('./');
 
 		$label = 'Running extract_current_page on ' . $query_string . ' with REQUEST_URI filled.';
 		$this->assertEquals($expected, $result['query_string'], $label);
 	}
+
+	protected function sanitizer($value)
+	{
+		// Fix for objects passed in phpunit
+		if (is_object($value))
+		{
+			return $value;
+		}
+		$type_cast_helper = new \phpbb\request\type_cast_helper();
+		$type_cast_helper->set_var($value, $value, gettype($value), true);
+		return str_replace('&', '&', $value);
+	}
 }