Fix, secure and test the impact icon URL computation

fixes #18483
glpi-project · Dec 11, 2024 · 0f751e3 · 0f751e3
1 parent efd5b72
commit 0f751e3
Show file tree

Hide file tree

Showing 7 changed files with 241 additions and 128 deletions.
diff --git a/.phpstan-baseline.php b/.phpstan-baseline.php
@@ -3949,12 +3949,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/src/Impact.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^PHPDoc tag @var with type class\\-string is not subtype of native type TKey of int\\|string\\.$#',
-	'identifier' => 'varTag.nativeType',
-	'count' => 2,
-	'path' => __DIR__ . '/src/Impact.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Right side of \\|\\| is always true\\.$#',
 	'identifier' => 'booleanOr.rightAlwaysTrue',

diff --git a/ajax/impact.php b/ajax/impact.php
@@ -69,19 +69,12 @@
                 if (empty($itemtype)) {
                     throw new BadRequestHttpException("Missing itemtype");
                 }
-                $icon = $CFG_GLPI["impact_asset_types"][$itemtype];
-                // Execute search
 
+                // Execute search
                 $assets = Impact::searchAsset($itemtype, json_decode($used), $filter, $page);
                 foreach ($assets['items'] as $index => $item) {
-                    $plugin_icon = Plugin::doHookFunction(Hooks::SET_ITEM_IMPACT_ICON, [
-                        'itemtype' => $itemtype,
-                        'items_id' => $item['id']
-                    ]);
-                    if ($plugin_icon && is_string($plugin_icon)) {
-                        $icon = ltrim($plugin_icon, '/');
-                    }
-                    $item['image'] = $CFG_GLPI['root_doc'] . '/' . $icon;
+                    $item['image'] = Impact::getImpactIcon($itemtype, $item['id']);
+
                     $assets['items'][$index] = $item;
                 }
                 header('Content-Type: application/json');

diff --git a/phpunit/functional/Glpi/Toolbox/URLTest.php b/phpunit/functional/Glpi/Toolbox/URLTest.php
@@ -44,82 +44,110 @@ class URLTest extends \GLPITestCase
     public static function urlProvider(): iterable
     {
         yield [
-            'url'      => null,
-            'expected' => '',
+            'url'       => null,
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => '',
-            'expected' => '',
+            'url'       => '',
+            'sanitized' => '',
+            'relative'  => false,
         ];
 
         // Javascript URL
         yield [
-            'url'      => 'javascript:alert(1);',
-            'expected' => '',
+            'url'       => 'javascript:alert(1);',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => "java\nscript:alert(1);",
-            'expected' => '',
+            'url'       => "java\nscript:alert(1);",
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => "j a v\t\ta\n  s c \t ript  :alert(1);",
-            'expected' => '',
+            'url'       => "j a v\t\ta\n  s c \t ript  :alert(1);",
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => 'jAvAscrIPt:alert(1);',
-            'expected' => '',
+            'url'       => 'jAvAscrIPt:alert(1);',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => 'javascript:alert(1);" title="XSS!"',
-            'expected' => '',
+            'url'       => 'javascript:alert(1);" title="XSS!"',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => 'javascript:alert(1)',
-            'expected' => '',
+            'url'       => 'javascript:alert(1)',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => 'javascript://%0aalert();',
-            'expected' => '',
+            'url'       => 'javascript://%0aalert();',
+            'sanitized' => '',
+            'relative'  => false,
         ];
 
         // Invalid URL
         yield [
-            'url'      => 'ht tp://www.domain.tld/test',
-            'expected' => '',
+            'url'       => 'ht tp://www.domain.tld/test',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => 'http:/www.domain.tld/test',
-            'expected' => '',
+            'url'       => 'http:/www.domain.tld/test',
+            'sanitized' => '',
+            'relative'  => false,
         ];
         yield [
-            'url'      => '15//test',
-            'expected' => '',
+            'url'       => '15//test',
+            'sanitized' => '',
+            'relative'  => false,
         ];
 
         // Sane URL
         yield [
-            'url'      => 'http://www.domain.tld/test',
-            'expected' => 'http://www.domain.tld/test',
+            'url'       => 'http://www.domain.tld/test',
+            'sanitized' => 'http://www.domain.tld/test',
+            'relative'  => false,
         ];
         yield [
-            'url'      => '//hostname/path/to/file',
-            'expected' => '//hostname/path/to/file',
+            'url'       => '//hostname/path/to/file',
+            'sanitized' => '//hostname/path/to/file',
+            'relative'  => false,
         ];
         yield [
-            'url'      => '/test?abc=12',
-            'expected' => '/test?abc=12',
+            'url'       => '/test?abc=12',
+            'sanitized' => '/test?abc=12',
+            'relative'  => true,
         ];
         yield [
-            'url'      => '/',
-            'expected' => '/',
+            'url'       => '/Path/To/Resource/15',
+            'sanitized' => '/Path/To/Resource/15',
+            'relative'  => true,
         ];
+        yield [
+            'url'       => '/',
+            'sanitized' => '/',
+            'relative'  => true,
+        ];
+    }
+
+    #[DataProvider('urlProvider')]
+    public function testSanitizeURL(?string $url, string $sanitized, bool $relative): void
+    {
+        $instance = new \Glpi\Toolbox\URL();
+        $this->assertEquals($sanitized, $instance->sanitizeURL($url));
     }
 
     #[DataProvider('urlProvider')]
-    public function testSanitizeURL(?string $url, string $expected): void
+    public function testIsGLPIRelativeUrl(?string $url, string $sanitized, bool $relative): void
     {
         $instance = new \Glpi\Toolbox\URL();
-        $this->assertEquals($expected, $instance->sanitizeURL($url));
+        $this->assertEquals($relative, $instance->isGLPIRelativeUrl((string) $url));
     }
 
     public static function extractItemtypeFromUrlPathProvider(): iterable

diff --git a/phpunit/functional/ImpactTest.php b/phpunit/functional/ImpactTest.php
@@ -37,11 +37,12 @@
 
 use CommonDBTM;
 use Computer;
+use Glpi\Plugin\Hooks;
 use ImpactCompound;
 use ImpactItem;
 use ImpactRelation;
 use Item_Ticket;
-use PHPUnit\Framework\Attributes\DataProvider;
+use Plugin;
 use Ticket;
 
 class ImpactTest extends \DbTestCase
@@ -533,4 +534,55 @@ public function testFilterGraph()
             );
         }
     }
+
+    public function testGetImpactIconFromConfig(): void
+    {
+        /**
+         * @var array $CFG_GLPI
+         */
+        global $CFG_GLPI;
+
+        foreach (['', '/glpi'] as $root_doc) {
+            $CFG_GLPI['root_doc'] = $root_doc;
+
+            foreach ($CFG_GLPI['impact_asset_types'] as $itemtype => $expected_icon) {
+                $this->assertSame($root_doc . $expected_icon, \Impact::getImpactIcon($itemtype));
+                // By default, targetting a particular ID does not change the result.
+                $this->assertSame($root_doc . $expected_icon, \Impact::getImpactIcon($itemtype, 1));
+            }
+
+            $this->assertSame($root_doc . '/pics/impact/default.png', \Impact::getImpactIcon('NotAnAssetType'));
+            // By default, targetting a particular ID does not change the result.
+            $this->assertSame($root_doc . '/pics/impact/default.png', \Impact::getImpactIcon('NotAnAssetType', 1));
+        }
+    }
+
+    public function testGetImpactIconFromPluginHook(): void
+    {
+        /**
+         * @var array $CFG_GLPI
+         * @var array $PLUGIN_HOOKS
+         */
+        global $CFG_GLPI, $PLUGIN_HOOKS;
+
+        (new Plugin())->init(true); // The `tester` plugin must be considered as loaded/active.
+
+        $PLUGIN_HOOKS[Hooks::SET_ITEM_IMPACT_ICON]['tester'] = function (array $params) {
+            if ($params['itemtype'] === 'PluginTesterMyAsset') {
+                return $params['items_id'] > 0
+                    ? sprintf('/plugins/tester/MyAsset/Picture/%d', $params['items_id'])
+                    : '/plugins/tester/pics/myasset.png';
+            }
+            return null;
+        };
+
+        foreach (['', '/glpi'] as $root_doc) {
+            $CFG_GLPI['root_doc'] = $root_doc;
+
+            $this->assertSame($root_doc . '/plugins/tester/pics/myasset.png', \Impact::getImpactIcon('PluginTesterMyAsset'));
+            $this->assertSame($root_doc . '/plugins/tester/MyAsset/Picture/7', \Impact::getImpactIcon('PluginTesterMyAsset', 7));
+
+            $this->assertSame($root_doc . '/pics/impact/default.png', \Impact::getImpactIcon('PluginTesterAnotherAsset'));
+        }
+    }
 }
diff --git a/src/Glpi/Toolbox/URL.php b/src/Glpi/Toolbox/URL.php
@@ -47,7 +47,7 @@ final class URL
      *
      * @return string
      */
-    final public static function sanitizeURL(?string $url): string
+    public static function sanitizeURL(?string $url): string
     {
         if ($url === null) {
             return '';
@@ -75,6 +75,42 @@ final public static function sanitizeURL(?string $url): string
         return $url;
     }
 
+    /**
+     * Checks whether an URL can be considered as a valid GLPI relative URL.
+     *
+     * @param string $url
+     *
+     * @return bool
+     */
+    public static function isGLPIRelativeUrl(string $url): bool
+    {
+        if ($url === '') {
+            return false;
+        }
+
+        if (self::sanitizeURL($url) !== $url) {
+            return false;
+        }
+
+        $parsed_url = parse_url($url);
+
+        if (
+            // URL is not parsable, it is invalid.
+            $parsed_url === false
+            // A relative URL should not contain a `scheme` or a `host` token
+            || array_key_exists('scheme', $parsed_url)
+            || array_key_exists('host', $parsed_url)
+            // A relative URL should contain a `path` token.
+            || !array_key_exists('path', $parsed_url)
+            // GLPI URLs are not supposed to contain special chars.
+            || preg_match('#[^a-z0-9_/\.-]#i', $parsed_url['path']) === 1
+        ) {
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Extract (lowercase) itemtype from a given URL path.
      *