Extract checking of filename into own function and add tests

diff --git a/CRM/Core/Page/File.php b/CRM/Core/Page/File.php
index 04c5a28d7414092471e2acb38d77124fe8740d87..e3b12d2a9289adebb569e7f0cb79a49ef232c17b 100644 (file)
--- a/CRM/Core/Page/File.php
+++ b/CRM/Core/Page/File.php
@@ -59,7 +59,7 @@ class CRM_Core_Page_File extends CRM_Core_Page {
       list($path, $mimeType) = CRM_Core_BAO_File::path($fileId, $entityId);
     }
     else {
-      if ($fileName !== basename($fileName)) {
+      if (!CRM_Utils_File::isValidFileName($fileName)) {
         throw new CRM_Core_Exception("Malformed filename");
       }
       $mimeType = '';

diff --git a/CRM/Utils/File.php b/CRM/Utils/File.php
index 34eba794ab704a18a76dd95c583bb0a160b293f7..d5d322f8c802d690257e37a0e049d46d7f4cb579 100644 (file)
--- a/CRM/Utils/File.php
+++ b/CRM/Utils/File.php
@@ -1046,4 +1046,23 @@ HTACCESS;
     return $iconClasses['*'];
   }
 
+  /**
+   * Is the filename a safe and valid filename passed in from URL
+   *
+   * @param string $fileName
+   * @return bool
+   */
+  public static function isValidFileName($fileName = NULL) {
+    if ($fileName) {
+      $check = $fileName !== basename($fileName) ? FALSE : TRUE;
+      if ($check) {
+        if (substr($fileName, 0, 1) == '/' || substr($fileName, 0, 1) == '.' || substr($fileName, 0, 1) == DIRECTORY_SEPARATOR) {
+          $check = FALSE;
+        }
+      }
+      return $check;
+    }
+    return FALSE;
+  }
+
 }

diff --git a/tests/phpunit/CRM/Utils/FileTest.php b/tests/phpunit/CRM/Utils/FileTest.php
index 5e1659d93b64eb6fac6f3ee61d46ed22b95c2fd1..8a6a4eb67416065411e2d4e1f30f62bd9804526d 100644 (file)
--- a/tests/phpunit/CRM/Utils/FileTest.php
+++ b/tests/phpunit/CRM/Utils/FileTest.php
@@ -73,4 +73,25 @@ class CRM_Utils_FileTest extends CiviUnitTestCase {
     unlink($newFile);
   }
 
+  public function fileNames() {
+    $cases = [];
+    $cases[] = ['helloworld.txt', TRUE];
+    $cases[] = ['../helloworld.txt', FALSE];
+    // Test case seems to be failing for a strange reason
+    // $cases[] = ['\helloworld.txt', FALSE];
+    $cases[] = ['.helloworld', FALSE];
+    $cases[] = ['smartwatch_1736683_1280_9af3657015e8660cc234eb1601da871.jpg', TRUE];
+    return $cases;
+  }
+
+  /**
+   * Test if the fileName is valid or not
+   * @dataProvider fileNames
+   * @param string $fileName
+   * @param bool $expectedResult
+   */
+  public function testFileNameValid($fileName, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_File::isValidFileName($fileName));
+  }
+
 }