Ensure query parameters are URL encoded in Arguments

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

diff --git a/src/App/Arguments.php b/src/App/Arguments.php
index e2f60b1956529ad224fe49c7ab524992374c1a06..de3fecf9edbe476e92f6d2976304c10f2244f94e 100644 (file)
--- 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 f6f23263e53bb8c76eea78e999a1d202aa3343c0..1ff66efb6a92a0b9450cf47f91570f54dea0b404 100644 (file)
--- 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 a7ef451d985d6d1a42207180b24365b02b9d1f1a..9a1b5bdad83ddffcc950c45f59ccd928b85a25b6 100644 (file)
--- 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);
        }
@@ -62,108 +62,106 @@ class ArgumentsTest extends TestCase
                                        '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',
                                ],
                        ],
-                       'withQ'                => [
+                       'withUnixHomeDir'      => [
                                'assert' => [
-                                       'queryString' => 'profile/test/it?arg1=value1&arg2=value2',
-                                       'command'     => 'profile/test/it',
-                                       'argv'        => ['profile', 'test', 'it'],
-                                       'argc'        => 3,
+                                       'queryString' => '~test/it?arg1=value1&arg2=value2',
+                                       'command'     => '~test/it',
+                                       'argv'        => ['~test', 'it'],
+                                       'argc'        => 2,
                                ],
                                'server' => [
-                                       'QUERY_STRING' => 'q=profile/test/it?arg1=value1&arg2=value2',
+                                       'QUERY_STRING' => 'pagename=~test/it&arg1=value1&arg2=value2',
                                ],
                                'get'    => [
-                                       'q' => 'profile/test/it',
+                                       'pagename' => '~test/it',
                                ],
                        ],
-                       'withWrongDelimiter'   => [
+                       'withDiasporaHomeDir'  => [
                                'assert' => [
-                                       'queryString' => 'profile/test/it?arg1=value1&arg2=value2',
-                                       'command'     => 'profile/test/it',
-                                       'argv'        => ['profile', 'test', 'it'],
+                                       'queryString' => 'u/test/it?arg1=value1&arg2=value2',
+                                       'command'     => 'u/test/it',
+                                       'argv'        => ['u', 'test', 'it'],
                                        'argc'        => 3,
                                ],
                                'server' => [
-                                       'QUERY_STRING' => 'pagename=profile/test/it&arg1=value1&arg2=value2',
+                                       'QUERY_STRING' => 'pagename=u/test/it&arg1=value1&arg2=value2',
                                ],
                                'get'    => [
-                                       'pagename' => 'profile/test/it',
+                                       'pagename' => 'u/test/it',
                                ],
                        ],
-                       'withUnixHomeDir'      => [
+                       'withTrailingSlash'    => [
                                'assert' => [
-                                       'queryString' => '~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=~test/it?arg1=value1&arg2=value2',
+                                       'QUERY_STRING' => 'pagename=profile/test/it&arg1=value1&arg2=value2/',
                                ],
                                'get'    => [
-                                       'pagename' => '~test/it',
+                                       'pagename' => 'profile/test/it',
                                ],
                        ],
-                       'withDiasporaHomeDir'  => [
+                       'withWrongQueryString' => [
                                'assert' => [
-                                       'queryString' => 'u/test/it?arg1=value1&arg2=value2',
+                                       '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' => 'pagename=u/test/it?arg1=value1&arg2=value2',
+                                       'QUERY_STRING' => 'wrong=profile/test/it&arg1=value1&arg2=value2/',
                                ],
                                'get'    => [
-                                       'pagename' => 'u/test/it',
+                                       'pagename' => 'profile/test/it',
                                ],
                        ],
-                       'withTrailingSlash'    => [
+                       'withMissingPageName'  => [
                                'assert' => [
-                                       'queryString' => 'profile/test/it?arg1=value1&arg2=value2/',
-                                       'command'     => 'profile/test/it',
-                                       'argv'        => ['profile', 'test', 'it'],
-                                       'argc'        => 3,
+                                       'queryString' => 'notvalid/it?arg1=value1&arg2=value2%2F',
+                                       'command'     => 'notvalid/it',
+                                       'argv'        => ['notvalid', 'it'],
+                                       'argc'        => 2,
                                ],
                                'server' => [
-                                       'QUERY_STRING' => 'pagename=profile/test/it?arg1=value1&arg2=value2/',
+                                       'QUERY_STRING' => 'pagename=notvalid/it&arg1=value1&arg2=value2/',
                                ],
                                'get'    => [
-                                       'pagename' => 'profile/test/it',
                                ],
                        ],
-                       'withWrongQueryString' => [
+                       'withNothing'  => [
                                'assert' => [
-                                       // empty query string?!
-                                       'queryString' => '',
-                                       'command'     => 'profile/test/it',
-                                       'argv'        => ['profile', 'test', 'it'],
-                                       'argc'        => 3,
+                                       'queryString' => '?arg1=value1&arg2=value2%2F',
+                                       'command'     => '',
+                                       'argv'        => [],
+                                       'argc'        => 0,
                                ],
                                'server' => [
-                                       'QUERY_STRING' => 'wrong=profile/test/it?arg1=value1&arg2=value2/',
+                                       'QUERY_STRING' => 'arg1=value1&arg2=value2/',
                                ],
                                'get'    => [
-                                       'pagename' => 'profile/test/it',
                                ],
                        ],
-                       'withMissingPageName'  => [
+                       'withFileExtension'  => [
                                'assert' => [
-                                       'queryString' => 'notvalid/it?arg1=value1&arg2=value2/',
-                                       'command'     => App\Module::DEFAULT,
-                                       'argv'        => [App\Module::DEFAULT],
-                                       'argc'        => 1,
+                                       'queryString' => 'api/call.json',
+                                       'command'     => 'api/call.json',
+                                       'argv'        => ['api', 'call.json'],
+                                       'argc'        => 2,
                                ],
                                'server' => [
-                                       'QUERY_STRING' => 'pagename=notvalid/it?arg1=value1&arg2=value2/',
+                                       '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());
        }