From d80e8fa62cad6661a0882753a8babf5512f9bb12 Mon Sep 17 00:00:00 2001
From: Seamus Lee <seamuslee001@gmail.com>
Date: Fri, 16 Dec 2022 11:55:57 +1100
Subject: [PATCH] security/core#120 Use Rich's patch (ArtfulRobot) with minor
 variation to try to resolve the issue and add unit tests

Fix Regex and include test cases as per Rich's comments
---
 CRM/Core/Page/Inline/Help.php            | 13 ++++---
 tests/phpunit/CRM/Core/Page/HelpTest.php | 46 ++++++++++++++++++++++++
 tests/phpunit/CiviTest/test.hlp          |  1 +
 3 files changed, 56 insertions(+), 4 deletions(-)
 create mode 100644 tests/phpunit/CRM/Core/Page/HelpTest.php
 create mode 100644 tests/phpunit/CiviTest/test.hlp

diff --git a/CRM/Core/Page/Inline/Help.php b/CRM/Core/Page/Inline/Help.php
index 0e0e01ae0b..6f299be382 100644
--- a/CRM/Core/Page/Inline/Help.php
+++ b/CRM/Core/Page/Inline/Help.php
@@ -21,9 +21,10 @@ class CRM_Core_Page_Inline_Help {
 
   public function run() {
     $args = $_REQUEST;
-    if (!empty($args['file']) && strpos($args['file'], '..') === FALSE) {
-      $file = $args['file'] . '.hlp';
-      $additionalTPLFile = $args['file'] . '.extra.hlp';
+    $file = (string) ($args['file'] ?? '');
+    if (preg_match('@^[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)*$@', $file)) {
+      $additionalTPLFile = "$file.extra.hlp";
+      $file .= '.hlp';
       $smarty = CRM_Core_Smarty::singleton();
       $smarty->assign('id', $args['id']);
       CRM_Utils_Array::remove($args, 'file', 'class_name', 'type', 'q', 'id');
@@ -41,7 +42,11 @@ class CRM_Core_Page_Inline_Help {
           $output = '';
         }
       }
-      exit($output . $extraoutput);
+      echo trim($output . $extraoutput);
+      CRM_Utils_System::civiExit();
+    }
+    else {
+      throw new CRM_Core_Exception('File name is not valid');
     }
   }
 
diff --git a/tests/phpunit/CRM/Core/Page/HelpTest.php b/tests/phpunit/CRM/Core/Page/HelpTest.php
new file mode 100644
index 0000000000..32f719c477
--- /dev/null
+++ b/tests/phpunit/CRM/Core/Page/HelpTest.php
@@ -0,0 +1,46 @@
+<?php
+
+/**
+ * Class CRM_Core_Page_HelpTest
+ * @group headless
+ */
+class CRM_Core_Page_HelpTest extends CiviUnitTestCase {
+
+  /**
+   * various test cases
+   */
+  public function fileTestCases(): array {
+    $cases = [];
+    $cases['relative paths are allowed'] = ['CRM/Admin/Form/Setting/Url', TRUE];
+    $cases['absolute path are not allowed'] = [\Civi::paths()->getPath('[civicrm.root]/tests/phpunit/CiviTest/test'), FALSE];
+    $cases['valid but uses disallowed ..'] = ['CRM/Admin/Form/../Form/Setting/Url', FALSE];
+    $cases[] = ['.dot/not/allowed', FALSE];
+    $cases[] = ['dot/not/.allowed/in/path/either', FALSE];
+    $cases[] = ['C:\win\paths\bad', FALSE];
+    $cases[] = ['', FALSE];
+    $cases[] = [NULL, FALSE];
+    return $cases;
+  }
+
+  /**
+   * @dataProvider fileTestCases
+   */
+  public function testHelpFileLoad($testCase, $expectedSuccess): void {
+    $_REQUEST = [];
+    $_REQUEST['class_name'] = 'CRM_Core_Page_Inline_Help';
+    $_REQUEST['file'] = $testCase;
+    $_REQUEST['id'] = 'url_vars';
+    $page = new CRM_Core_Page_Inline_Help();
+    try {
+      $page->run();
+    }
+    catch (CRM_Core_Exception $e) {
+      $this->assertEquals('File name is not valid', $e->getMessage());
+      $this->assertFalse($expectedSuccess);
+    }
+    catch (CRM_Core_Exception_PrematureExitException $e) {
+      $this->assertTrue($expectedSuccess);
+    }
+  }
+
+}
diff --git a/tests/phpunit/CiviTest/test.hlp b/tests/phpunit/CiviTest/test.hlp
new file mode 100644
index 0000000000..5a1afc2151
--- /dev/null
+++ b/tests/phpunit/CiviTest/test.hlp
@@ -0,0 +1 @@
+{php}echo date("Y-m-d") . ' boo!'{/php}
-- 
2.25.1