Skip to content

Commit

Permalink
Validate filenames before trying to store them
Browse files Browse the repository at this point in the history
 - Avoids path traversal issues with e.g. `../../viewname`
  • Loading branch information
martialblog committed Aug 12, 2024
1 parent 93cc9b8 commit 3f7d772
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 0 deletions.
16 changes: 16 additions & 0 deletions library/Toplevelview/Model/View.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions library/Toplevelview/ViewConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,18 @@ public function loadAll($format = self::FORMAT_YAML): array
/**
* storeToSession stores a View's text to the user's session
*
* @throws NotWritableError if the data cannot be stored
* @throws SecurityException if the user has no access to edit
*
* @param $view
*/
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());
Expand All @@ -266,10 +274,18 @@ public function clearSession($view): void
/**
* storeToFile stores a View to its configuration file
*
* @throws NotWritableError if the data cannot be stored
* @throws SecurityException if the user has no access to edit
*
* @param $view
*/
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());
Expand Down
39 changes: 39 additions & 0 deletions test/php/library/Toplevelview/Model/ViewTest.php
Original file line number Diff line number Diff line change
@@ -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);
}
}
}

0 comments on commit 3f7d772

Please sign in to comment.