Ensure query parameters are URL encoded in Arguments

- Simplify Arguments->determine
- Remove stripZRLs and stripQueryParam Arguments methods
- Updated tests

src/App/Arguments.php

@@ -47,7 +47,7 @@ class Arguments
 	 */
 	private $argc;
 
-	public function __construct(string $queryString = '', string $command = '', array $argv = [Module::DEFAULT], int $argc = 1)
+	public function __construct(string $queryString = '', string $command = '', array $argv = [], int $argc = 0)
 	{
 		$this->queryString = $queryString;
 		$this->command     = $command;
@@ -56,7 +56,7 @@ class Arguments
 	}
 
 	/**
-	 * @return string The whole query string of this call
+	 * @return string The whole query string of this call with url-encoded query parameters
 	 */
 	public function getQueryString()
 	{
@@ -121,50 +121,27 @@ class Arguments
 	 */
 	public function determine(array $server, array $get)
 	{
-		$queryString = '';
+		// removing leading / - maybe a nginx problem
+		$server['QUERY_STRING'] = ltrim($server['QUERY_STRING'] ?? '', '/');
 
-		if (!empty($server['QUERY_STRING']) && strpos($server['QUERY_STRING'], 'pagename=') === 0) {
+		$queryParameters = [];
-			$queryString = urldecode(substr($server['QUERY_STRING'], 9));
+		parse_str($server['QUERY_STRING'], $queryParameters);
-		} elseif (!empty($server['QUERY_STRING']) && strpos($server['QUERY_STRING'], 'q=') === 0) {
-			$queryString = urldecode(substr($server['QUERY_STRING'], 2));
-		}
-
-		// eventually strip ZRL
-		$queryString = $this->stripZRLs($queryString);
-
-		// eventually strip OWT
-		$queryString = $this->stripQueryParam($queryString, 'owt');
-
-		// removing trailing / - maybe a nginx problem
-		$queryString = ltrim($queryString, '/');
 
 		if (!empty($get['pagename'])) {
 			$command = trim($get['pagename'], '/\\');
+		} elseif (!empty($queryParameters['pagename'])) {
+			$command = trim($queryParameters['pagename'], '/\\');
 		} elseif (!empty($get['q'])) {
+			// Legacy page name parameter, now conflicts with the search query parameter
 			$command = trim($get['q'], '/\\');
 		} else {
-			$command = Module::DEFAULT;
+			$command = '';
 		}
 
-
+		// Remove generated and one-time use parameters
-		// fix query_string
+		unset($queryParameters['pagename']);
-		if (!empty($command)) {
+		unset($queryParameters['zrl']);
-			$queryString = str_replace(
+		unset($queryParameters['owt']);
-				$command . '&',
-				$command . '?',
-				$queryString
-			);
-		}
-
-		// unix style "homedir"
-		if (substr($command, 0, 1) === '~') {
-			$command = 'profile/' . substr($command, 1);
-		}
-
-		// Diaspora style profile url
-		if (substr($command, 0, 2) === 'u/') {
-			$command = 'profile/' . substr($command, 2);
-		}
 
 		/*
 		 * Break the URL path into C style argc/argv style arguments for our
@@ -173,41 +150,17 @@ class Arguments
 		 *   [0] => 'module'
 		 *   [1] => 'arg1'
 		 *   [2] => 'arg2'
-		 *
-		 *
-		 * There will always be one argument. If provided a naked domain
-		 * URL, $this->argv[0] is set to "home".
 		 */
+		if ($command) {
+			$argv = explode('/', $command);
+		} else {
+			$argv = [];
+		}
 
-		$argv = explode('/', $command);
 		$argc = count($argv);
 
+		$queryString = $command . ($queryParameters ? '?' . http_build_query($queryParameters) : '');
 
 		return new Arguments($queryString, $command, $argv, $argc);
 	}
-
+}
-	/**
-	 * Strip zrl parameter from a string.
-	 *
-	 * @param string $queryString The input string.
-	 *
-	 * @return string The zrl.
-	 */
-	public function stripZRLs(string $queryString)
-	{
-		return preg_replace('/[?&]zrl=(.*?)(&|$)/ism', '$2', $queryString);
-	}
-
-	/**
-	 * Strip query parameter from a string.
-	 *
-	 * @param string $queryString The input string.
-	 * @param string $param
-	 *
-	 * @return string The query parameter.
-	 */
-	public function stripQueryParam(string $queryString, string $param)
-	{
-		return preg_replace('/[?&]' . $param . '=(.*?)(&|$)/ism', '$2', $queryString);
-	}
-}

tests/legacy/ApiTest.php

@@ -75,7 +75,7 @@ class ApiTest extends FixtureTest
 		$this->app = DI::app();
 
 		$this->app->argc = 1;
-		$this->app->argv = ['home'];
+		$this->app->argv = [''];
 
 		// User data that the test database is populated with
 		$this->selfUser   = [

tests/src/App/ArgumentsTest.php

@@ -45,8 +45,8 @@ class ArgumentsTest extends TestCase
 		$this->assertArguments([
 			'queryString' => '',
 			'command'     => '',
-			'argv'        => ['home'],
+			'argv'        => [],
-			'argc'        => 1,
+			'argc'        => 0,
 		],
 			$arguments);
 	}
@@ -55,34 +55,6 @@ class ArgumentsTest extends TestCase
 	{
 		return [
 			'withPagename'         => [
-				'assert' => [
-					'queryString' => 'profile/test/it?arg1=value1&arg2=value2',
-					'command'     => 'profile/test/it',
-					'argv'        => ['profile', 'test', 'it'],
-					'argc'        => 3,
-				],
-				'server' => [
-					'QUERY_STRING' => 'pagename=profile/test/it?arg1=value1&arg2=value2',
-				],
-				'get'    => [
-					'pagename' => 'profile/test/it',
-				],
-			],
-			'withQ'                => [
-				'assert' => [
-					'queryString' => 'profile/test/it?arg1=value1&arg2=value2',
-					'command'     => 'profile/test/it',
-					'argv'        => ['profile', 'test', 'it'],
-					'argc'        => 3,
-				],
-				'server' => [
-					'QUERY_STRING' => 'q=profile/test/it?arg1=value1&arg2=value2',
-				],
-				'get'    => [
-					'q' => 'profile/test/it',
-				],
-			],
-			'withWrongDelimiter'   => [
 				'assert' => [
 					'queryString' => 'profile/test/it?arg1=value1&arg2=value2',
 					'command'     => 'profile/test/it',
@@ -99,12 +71,12 @@ class ArgumentsTest extends TestCase
 			'withUnixHomeDir'      => [
 				'assert' => [
 					'queryString' => '~test/it?arg1=value1&arg2=value2',
-					'command'     => 'profile/test/it',
+					'command'     => '~test/it',
-					'argv'        => ['profile', 'test', 'it'],
+					'argv'        => ['~test', 'it'],
-					'argc'        => 3,
+					'argc'        => 2,
 				],
 				'server' => [
-					'QUERY_STRING' => 'pagename=~test/it?arg1=value1&arg2=value2',
+					'QUERY_STRING' => 'pagename=~test/it&arg1=value1&arg2=value2',
 				],
 				'get'    => [
 					'pagename' => '~test/it',
@@ -113,12 +85,12 @@ class ArgumentsTest extends TestCase
 			'withDiasporaHomeDir'  => [
 				'assert' => [
 					'queryString' => 'u/test/it?arg1=value1&arg2=value2',
-					'command'     => 'profile/test/it',
+					'command'     => 'u/test/it',
-					'argv'        => ['profile', 'test', 'it'],
+					'argv'        => ['u', 'test', 'it'],
 					'argc'        => 3,
 				],
 				'server' => [
-					'QUERY_STRING' => 'pagename=u/test/it?arg1=value1&arg2=value2',
+					'QUERY_STRING' => 'pagename=u/test/it&arg1=value1&arg2=value2',
 				],
 				'get'    => [
 					'pagename' => 'u/test/it',
@@ -126,13 +98,13 @@ class ArgumentsTest extends TestCase
 			],
 			'withTrailingSlash'    => [
 				'assert' => [
-					'queryString' => 'profile/test/it?arg1=value1&arg2=value2/',
+					'queryString' => 'profile/test/it?arg1=value1&arg2=value2%2F',
 					'command'     => 'profile/test/it',
 					'argv'        => ['profile', 'test', 'it'],
 					'argc'        => 3,
 				],
 				'server' => [
-					'QUERY_STRING' => 'pagename=profile/test/it?arg1=value1&arg2=value2/',
+					'QUERY_STRING' => 'pagename=profile/test/it&arg1=value1&arg2=value2/',
 				],
 				'get'    => [
 					'pagename' => 'profile/test/it',
@@ -140,14 +112,13 @@ class ArgumentsTest extends TestCase
 			],
 			'withWrongQueryString' => [
 				'assert' => [
-					// empty query string?!
+					'queryString' => 'profile/test/it?wrong=profile%2Ftest%2Fit&arg1=value1&arg2=value2%2F',
-					'queryString' => '',
 					'command'     => 'profile/test/it',
 					'argv'        => ['profile', 'test', 'it'],
 					'argc'        => 3,
 				],
 				'server' => [
-					'QUERY_STRING' => 'wrong=profile/test/it?arg1=value1&arg2=value2/',
+					'QUERY_STRING' => 'wrong=profile/test/it&arg1=value1&arg2=value2/',
 				],
 				'get'    => [
 					'pagename' => 'profile/test/it',
@@ -155,17 +126,44 @@ class ArgumentsTest extends TestCase
 			],
 			'withMissingPageName'  => [
 				'assert' => [
-					'queryString' => 'notvalid/it?arg1=value1&arg2=value2/',
+					'queryString' => 'notvalid/it?arg1=value1&arg2=value2%2F',
-					'command'     => App\Module::DEFAULT,
+					'command'     => 'notvalid/it',
-					'argv'        => [App\Module::DEFAULT],
+					'argv'        => ['notvalid', 'it'],
-					'argc'        => 1,
+					'argc'        => 2,
 				],
 				'server' => [
-					'QUERY_STRING' => 'pagename=notvalid/it?arg1=value1&arg2=value2/',
+					'QUERY_STRING' => 'pagename=notvalid/it&arg1=value1&arg2=value2/',
 				],
 				'get'    => [
 				],
 			],
+			'withNothing'  => [
+				'assert' => [
+					'queryString' => '?arg1=value1&arg2=value2%2F',
+					'command'     => '',
+					'argv'        => [],
+					'argc'        => 0,
+				],
+				'server' => [
+					'QUERY_STRING' => 'arg1=value1&arg2=value2/',
+				],
+				'get'    => [
+				],
+			],
+			'withFileExtension'  => [
+				'assert' => [
+					'queryString' => 'api/call.json',
+					'command'     => 'api/call.json',
+					'argv'        => ['api', 'call.json'],
+					'argc'        => 2,
+				],
+				'server' => [
+					'QUERY_STRING' => 'pagename=api/call.json',
+				],
+				'get'    => [
+					'pagename' => 'api/call.json'
+				],
+			],
 		];
 	}
 
@@ -207,27 +205,27 @@ class ArgumentsTest extends TestCase
 		return [
 			'strippedZRLFirst'  => [
 				'assert' => '?arg1=value1',
-				'input'  => '?zrl=nope&arg1=value1',
+				'input'  => '&zrl=nope&arg1=value1',
 			],
 			'strippedZRLLast'   => [
 				'assert' => '?arg1=value1',
-				'input'  => '?arg1=value1&zrl=nope',
+				'input'  => '&arg1=value1&zrl=nope',
 			],
 			'strippedZTLMiddle' => [
 				'assert' => '?arg1=value1&arg2=value2',
-				'input'  => '?arg1=value1&zrl=nope&arg2=value2',
+				'input'  => '&arg1=value1&zrl=nope&arg2=value2',
 			],
 			'strippedOWTFirst'  => [
 				'assert' => '?arg1=value1',
-				'input'  => '?owt=test&arg1=value1',
+				'input'  => '&owt=test&arg1=value1',
 			],
 			'strippedOWTLast'   => [
 				'assert' => '?arg1=value1',
-				'input'  => '?arg1=value1&owt=test',
+				'input'  => '&arg1=value1&owt=test',
 			],
 			'strippedOWTMiddle' => [
 				'assert' => '?arg1=value1&arg2=value2',
-				'input'  => '?arg1=value1&owt=test&arg2=value2',
+				'input'  => '&arg1=value1&owt=test&arg2=value2',
 			],
 		];
 	}
@@ -242,7 +240,7 @@ class ArgumentsTest extends TestCase
 		$command = 'test/it';
 
 		$arguments = (new App\Arguments())
-			->determine(['QUERY_STRING' => 'q=' . $command . $input,], ['pagename' => $command]);
+			->determine(['QUERY_STRING' => 'pagename=' . $command . $input,], ['pagename' => $command]);
 
 		$this->assertEquals($command . $assert, $arguments->getQueryString());
 	}