From 1b681222a44bed72c2cbca53fcd1583d89ff3dcc Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Wed, 9 Sep 2020 00:24:44 -0400 Subject: [PATCH] Ensure query parameters are URL encoded in Arguments - Simplify Arguments->determine - Remove stripZRLs and stripQueryParam Arguments methods - Updated tests --- src/App/Arguments.php | 89 +++++++-------------------- tests/legacy/ApiTest.php | 2 +- tests/src/App/ArgumentsTest.php | 106 ++++++++++++++++---------------- 3 files changed, 74 insertions(+), 123 deletions(-) diff --git a/src/App/Arguments.php b/src/App/Arguments.php index e2f60b1956..de3fecf9ed 100644 --- a/src/App/Arguments.php +++ b/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) { - $queryString = urldecode(substr($server['QUERY_STRING'], 9)); - } 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, '/'); + $queryParameters = []; + parse_str($server['QUERY_STRING'], $queryParameters); 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 = ''; } - - // fix query_string - if (!empty($command)) { - $queryString = str_replace( - $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); - } + // Remove generated and one-time use parameters + unset($queryParameters['pagename']); + unset($queryParameters['zrl']); + unset($queryParameters['owt']); /* * 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); - } -} \ No newline at end of file +} diff --git a/tests/legacy/ApiTest.php b/tests/legacy/ApiTest.php index f6f23263e5..1ff66efb6a 100644 --- a/tests/legacy/ApiTest.php +++ b/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 = [ diff --git a/tests/src/App/ArgumentsTest.php b/tests/src/App/ArgumentsTest.php index a7ef451d98..9a1b5bdad8 100644 --- a/tests/src/App/ArgumentsTest.php +++ b/tests/src/App/ArgumentsTest.php @@ -45,8 +45,8 @@ class ArgumentsTest extends TestCase $this->assertArguments([ 'queryString' => '', 'command' => '', - 'argv' => ['home'], - 'argc' => 1, + 'argv' => [], + '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', - 'argv' => ['profile', 'test', 'it'], - 'argc' => 3, + 'command' => '~test/it', + 'argv' => ['~test', 'it'], + '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', - 'argv' => ['profile', 'test', 'it'], + 'command' => 'u/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' => '', + 'queryString' => 'profile/test/it?wrong=profile%2Ftest%2Fit&arg1=value1&arg2=value2%2F', '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/', - 'command' => App\Module::DEFAULT, - 'argv' => [App\Module::DEFAULT], - 'argc' => 1, + 'queryString' => 'notvalid/it?arg1=value1&arg2=value2%2F', + 'command' => 'notvalid/it', + 'argv' => ['notvalid', 'it'], + '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()); }