From c40973424d3eaa5519e83233a25f54fa66806d3c Mon Sep 17 00:00:00 2001
From: Markus Opolka <markus.opolka@netways.de>
Date: Mon, 12 Aug 2024 11:42:48 +0200
Subject: [PATCH] Validate filenames before trying to store them

 - Avoids path traversal issues with e.g. `../../viewname`
---
 library/Toplevelview/Model/View.php           | 16 ++++++++
 library/Toplevelview/ViewConfig.php           | 10 +++++
 .../library/Toplevelview/Model/ViewTest.php   | 39 +++++++++++++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 test/php/library/Toplevelview/Model/ViewTest.php

diff --git a/library/Toplevelview/Model/View.php b/library/Toplevelview/Model/View.php
index 6ca16b1..ea30bcd 100644
--- a/library/Toplevelview/Model/View.php
+++ b/library/Toplevelview/Model/View.php
@@ -233,6 +233,22 @@ public function setText($text)
         return $this;
     }
 
+    /**
+     * validateName validates the name of the view.
+     * This can be used to ensure the YAML files have proper/expected names
+     * @return bool
+     */
+    public function validateName(): bool
+    {
+        if (empty($this->name)) {
+            return false;
+        }
+        if (preg_match('/[!@#\$%^&*\/\\\()]/', $this->name)) {
+            return false;
+        }
+        return true;
+    }
+
     /**
      * getName returns the name of this View
      * @return ?string
diff --git a/library/Toplevelview/ViewConfig.php b/library/Toplevelview/ViewConfig.php
index 6ccb4ae..a5d79ec 100644
--- a/library/Toplevelview/ViewConfig.php
+++ b/library/Toplevelview/ViewConfig.php
@@ -246,6 +246,11 @@ public function loadAll($format = self::FORMAT_YAML): array
      */
     public function storeToSession($view): void
     {
+        // Assert the name is valid, to avoid tricky filenames such as '../../view'
+        if (!$view->validateName()) {
+            throw new NotWritableError('Invalid filename: %s', $view->getName());
+        }
+
         // Assert the user has rights to edit this view
         $restrictions = $this->getRestrictions('toplevelview/filter/edit');
         $this->assertAccessToView($restrictions, $view->getName());
@@ -270,6 +275,11 @@ public function clearSession($view): void
      */
     public function storeToFile($view): void
     {
+        // Assert the name is valid, to avoid tricky filenames such as '../../view'
+        if (!$view->validateName()) {
+            throw new NotWritableError('Invalid filename: %s', $view->getName());
+        }
+
         // Assert the user has rights to edit this file
         $restrictions = $this->getRestrictions('toplevelview/filter/edit');
         $this->assertAccessToView($restrictions, $view->getName());
diff --git a/test/php/library/Toplevelview/Model/ViewTest.php b/test/php/library/Toplevelview/Model/ViewTest.php
new file mode 100644
index 0000000..2cae54f
--- /dev/null
+++ b/test/php/library/Toplevelview/Model/ViewTest.php
@@ -0,0 +1,39 @@
+<?php
+
+namespace Tests\Icinga\Module\Toplevelview;
+
+use Icinga\Module\Toplevelview\Model\View;
+
+use PHPUnit\Framework\TestCase;
+
+final class ViewTest extends TestCase
+{
+    public function testViewValidateName()
+    {
+        $v = new View('myview', 'yml');
+        $this->assertSame('myview', $v->getName());
+
+        $tests = [
+            'example' => true,
+            'Example' => true,
+            'ex_ample' => true,
+            'ex_am-ple' => true,
+            '1ex_am-ple2' => true,
+            'Ex_a2m-ple123' => true,
+            '1ex_am-ple2' => true,
+            'example.yaml' => true,
+            'e#x(_)am_123-ple' => false,
+            'ex/ample' => false,
+            'ex\ample' => false,
+            '' => false,
+            'Филе' => true,
+            '😺' => true,
+            '../../example' => false
+        ];
+
+        foreach ($tests as $name => $value) {
+            $v->setName($name);
+            $this->assertSame($v->validateName(), $value);
+        }
+    }
+}