Refactor Url::getArgSeparator to use ampersand when available

- Fixes #16826
- Closes #18540
- Related to #14969

Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>

libraries/classes/Url.php

@@ -11,13 +11,14 @@ use PhpMyAdmin\Crypto\Crypto;
 
 use function base64_decode;
 use function base64_encode;
-use function htmlentities;
 use function htmlspecialchars;
 use function http_build_query;
 use function in_array;
 use function ini_get;
 use function is_array;
+use function is_string;
 use function json_encode;
+use function method_exists;
 use function str_contains;
 use function strlen;
 use function strtr;
@@ -27,6 +28,9 @@ use function strtr;
  */
 class Url
 {
+    /** @var string|null */
+    private static $inputArgSeparator = null;
+
     /**
      * Generates text with hidden inputs.
      *
@@ -230,7 +234,7 @@ class Url
 
         $query = self::buildHttpQuery($params, $encrypt);
 
-        if (($divider !== '?' && $divider !== '&') || strlen($query) > 0) {
+        if (($divider !== '?' && $divider !== self::getArgSeparator()) || strlen($query) > 0) {
             return $divider . $query;
         }
 
@@ -303,47 +307,42 @@ class Url
     }
 
     /**
-     * Returns url separator
+     * Returns url separator character used for separating url parts.
      *
-     * extracted from arg_separator.input as set in php.ini
-     * we do not use arg_separator.output to avoid problems with & and &
+     * Extracted from 'arg_separator.input' as set in php.ini, but prefers '&' and ';'.
      *
-     * @param string $encode whether to encode separator or not,
-     *                       currently 'none' or 'html'
-     *
-     * @return string  character used for separating url parts usually ; or &
+     * @see https://www.php.net/manual/en/ini.core.php#ini.arg-separator.input
+     * @see https://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2
      */
-    public static function getArgSeparator($encode = 'none')
+    public static function getArgSeparator(): string
     {
-        static $separator = null;
-        static $html_separator = null;
-
-        if ($separator === null) {
-            // use separators defined by php, but prefer ';'
-            // as recommended by W3C
-            // (see https://www.w3.org/TR/1999/REC-html401-19991224/appendix
-            // /notes.html#h-B.2.2)
-            $arg_separator = (string) ini_get('arg_separator.input');
-            if (str_contains($arg_separator, ';')) {
-                $separator = ';';
-            } elseif (strlen($arg_separator) > 0) {
-                $separator = $arg_separator[0];
-            } else {
-                $separator = '&';
-            }
-
-            $html_separator = htmlentities($separator);
+        if (is_string(self::$inputArgSeparator)) {
+            return self::$inputArgSeparator;
         }
 
-        switch ($encode) {
-            case 'html':
-                return $html_separator;
-
-            case 'text':
-            case 'none':
-            default:
-                return $separator;
+        $separator = self::getArgSeparatorValueFromIni();
+        if (! is_string($separator) || $separator === '' || str_contains($separator, '&')) {
+            return self::$inputArgSeparator = '&';
         }
+
+        if (str_contains($separator, ';')) {
+            return self::$inputArgSeparator = ';';
+        }
+
+        // uses first character
+        return self::$inputArgSeparator = $separator[0];
+    }
+
+    /** @return string|false */
+    private static function getArgSeparatorValueFromIni()
+    {
+        /** @psalm-suppress ArgumentTypeCoercion */
+        if (method_exists('PhpMyAdmin\Tests\UrlTest', 'getInputArgSeparator')) {
+            // phpcs:ignore SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFullyQualifiedName
+            return \PhpMyAdmin\Tests\UrlTest::getInputArgSeparator();
+        }
+
+        return ini_get('arg_separator.input');
     }
 
     /**
@@ -352,6 +351,6 @@ class Url
      */
     public static function getFromRoute(string $route, array $additionalParameters = []): string
     {
-        return 'index.php?route=' . $route . self::getCommon($additionalParameters, '&');
+        return 'index.php?route=' . $route . self::getCommon($additionalParameters, self::getArgSeparator());
     }
 }

phpstan-baseline.neon

@@ -6451,12 +6451,7 @@ parameters:
 			path: libraries/classes/Plugins/ImportPlugin.php
 
 		-
-			message: "#^Parameter \\#1 \\$string of function strlen expects string, mixed given\\.$#"
-			count: 1
-			path: libraries/classes/Plugins/Schema/Dia/Dia.php
-
-		-
-			message: "#^Parameter mixed of print cannot be converted to string\\.$#"
+			message: "#^Parameter \\#1 \\$string of function strlen expects string, int\\|string given\\.$#"
 			count: 1
 			path: libraries/classes/Plugins/Schema/Dia/Dia.php
 
@@ -6606,12 +6601,7 @@ parameters:
 			path: libraries/classes/Plugins/Schema/Svg/RelationStatsSvg.php
 
 		-
-			message: "#^Parameter \\#1 \\$string of function strlen expects string, mixed given\\.$#"
-			count: 1
-			path: libraries/classes/Plugins/Schema/Svg/Svg.php
-
-		-
-			message: "#^Parameter mixed of print cannot be converted to string\\.$#"
+			message: "#^Parameter \\#1 \\$string of function strlen expects string, int\\|string given\\.$#"
 			count: 1
 			path: libraries/classes/Plugins/Schema/Svg/Svg.php
 

test/classes/UrlTest.php

@@ -5,7 +5,9 @@ declare(strict_types=1);
 namespace PhpMyAdmin\Tests;
 
 use PhpMyAdmin\Url;
+use ReflectionProperty;
 
+use function ini_get;
 use function is_string;
 use function parse_str;
 use function str_repeat;
@@ -16,6 +18,9 @@ use function urldecode;
  */
 class UrlTest extends AbstractTestCase
 {
+    /** @var string|false|null */
+    private static $inputArgSeparator = null;
+
     /**
      * Sets up the fixture, for example, opens a network connection.
      * This method is called before a test is executed.
@@ -241,4 +246,49 @@ class UrlTest extends AbstractTestCase
         $decrypted = Url::decryptQuery($encrypted);
         $this->assertSame($query, $decrypted);
     }
+
+    /**
+     * @param string|false $iniValue
+     *
+     * @dataProvider getArgSeparatorProvider
+     */
+    public function testGetArgSeparator(string $expected, $iniValue, ?string $cacheValue): void
+    {
+        $property = new ReflectionProperty(Url::class, 'inputArgSeparator');
+        $property->setAccessible(true);
+        $property->setValue(null, $cacheValue);
+
+        self::$inputArgSeparator = $iniValue;
+        self::assertSame($expected, Url::getArgSeparator());
+
+        self::$inputArgSeparator = null;
+        $property->setValue(null, null);
+    }
+
+    /** @psalm-return array<string, array{string, string|false, string|null}> */
+    public static function getArgSeparatorProvider(): array
+    {
+        return [
+            'ampersand' => ['&', '&', null],
+            'semicolon' => [';', ';', null],
+            'prefer ampersand' => ['&', '+;&$', null],
+            'prefer semicolon' => [';', '+;$', null],
+            'first char' => ['+', '+$', null],
+            'cache' => ['$', '&', '$'],
+            'empty value' => ['&', '', null],
+            'false' => ['&', false, null],
+        ];
+    }
+
+    /**
+     * Test double for ini_get('arg_separator.input') as it can't be changed using ini_set()
+     *
+     * @see Url::getArgSeparatorValueFromIni
+     *
+     * @return string|false
+     */
+    public static function getInputArgSeparator()
+    {
+        return self::$inputArgSeparator ?? ini_get('arg_separator.input');
+    }
 }