Merge pull request #4412 from nilsteampassnet/code_review

Code review fixes
nilsteampassnet · Oct 30, 2024 · 4ae7911 · 4ae7911
2 parents a4a36fd + fc44b3f
commit 4ae7911
Show file tree

Hide file tree

Showing 38 changed files with 445 additions and 296 deletions.
diff --git a/.snyk b/.snyk
@@ -0,0 +1,9 @@
+ignore:
+  - "vendor/**"
+  - "plugins/**"
+  - "includes/libraries/cryptojs/**"
+  - "includes/libraries/csrfp/**"
+  - "includes/libraries/ezimuel/**"
+  - "includes/libraries/plupload/**"
+  - "includes/libraries/yubico/**"
+  - "install/**"
diff --git a/api/Controller/Api/BaseController.php b/api/Controller/Api/BaseController.php
@@ -80,8 +80,7 @@ public function sanitizeUrl(array $array)
 
         return dataSanitizer(
             $array,
-            $filters,
-            __DIR__.'/../../..'
+            $filters
         );
     }
 

diff --git a/api/Controller/Api/ItemController.php b/api/Controller/Api/ItemController.php
@@ -185,29 +185,38 @@ public function createAction(array $userData)
                 // get parameters
                 $arrQueryStringParams = $this->getQueryStringParams();
 
-                // check parameters
-                $arrCheck = $this->checkNewItemData($arrQueryStringParams, $userData);
-                if ($arrCheck['error'] === true) {
-                    $strErrorDesc = $arrCheck['strErrorDesc'];
-                    $strErrorHeader = $arrCheck['strErrorHeader'];
+                // Check that the parameters are indeed an array before using them
+                if (is_array($arrQueryStringParams)) {
+                    // check parameters
+                    $arrCheck = $this->checkNewItemData($arrQueryStringParams, $userData);
+
+                    if ($arrCheck['error'] === true) {
+                        $strErrorDesc = $arrCheck['strErrorDesc'];
+                        $strErrorHeader = $arrCheck['strErrorHeader'];
+                    } else {
+                        // launch
+                        $itemModel = new ItemModel();
+                        $ret = $itemModel->addItem(
+                            (int) $arrQueryStringParams['folder_id'],
+                            (string) $arrQueryStringParams['label'],
+                            (string) $arrQueryStringParams['password'],
+                            (string) $arrQueryStringParams['description'],
+                            (string) $arrQueryStringParams['login'],
+                            (string) $arrQueryStringParams['email'],
+                            (string) $arrQueryStringParams['url'],
+                            (string) $arrQueryStringParams['tags'],
+                            (string) $arrQueryStringParams['anyone_can_modify'],
+                            (string) $arrQueryStringParams['icon'],
+                            (int) $userData['id'],
+                            (string) $userData['username'],
+                        );
+                        $responseData = json_encode($ret);
+                    }
+
                 } else {
-                    // launch
-                    $itemModel = new ItemModel();
-                    $ret = $itemModel->addItem(
-                        $arrQueryStringParams['folder_id'],
-                        $arrQueryStringParams['label'],
-                        $arrQueryStringParams['password'],
-                        $arrQueryStringParams['description'],
-                        $arrQueryStringParams['login'],
-                        $arrQueryStringParams['email'],
-                        $arrQueryStringParams['url'],
-                        $arrQueryStringParams['tags'],
-                        $arrQueryStringParams['anyone_can_modify'],
-                        $arrQueryStringParams['icon'],
-                        $userData['id'],
-                        $userData['username'],
-                    );
-                    $responseData = json_encode($ret);
+                    // Gérer le cas où les paramètres ne sont pas un tableau
+                    $strErrorDesc = 'Data not consistent';
+                    $strErrorHeader = 'Expected array, received ' . gettype($arrQueryStringParams);
                 }
             }
         } else {

diff --git a/api/inc/jwt_utils.php b/api/inc/jwt_utils.php
@@ -88,7 +88,8 @@ function get_authorization_header()
 	$authorizationHeader = $request->headers->get('Authorization');
 	$headers = null;
 
-	if (null !== $authorizationHeader) {
+	// Check if the authorization header is not empty
+	if (!empty($authorizationHeader)) {
 		$headers = trim($authorizationHeader);
 	} else if (function_exists('apache_request_headers') === true) {
 		$requestHeaders = (array) apache_request_headers();

diff --git a/api/index.php b/api/index.php
@@ -23,7 +23,18 @@
  * @see       https://www.teampass.net
  */
 
-header("Access-Control-Allow-Origin: ".$_SERVER['HTTP_HOST']);
+// Determine the protocol used
+$protocol = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off') ? 'https://' : 'http://';
+
+// Validate and filter the host
+$host = filter_var($_SERVER['HTTP_HOST'], FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME);
+
+// Allocate the correct CORS header
+if ($host !== false) {
+    header("Access-Control-Allow-Origin: $protocol$host");
+} else {
+    header("Access-Control-Allow-Origin: 'null'");
+}
 header("Content-Type: application/json; charset=UTF-8");
 header("Access-Control-Allow-Methods: POST, GET");
 header("Access-Control-Max-Age: 3600");
@@ -33,7 +44,7 @@
 // sanitize url segments
 $base = new BaseController();
 $uri = $base->getUriSegments();
-if (is_array($uri) === false || is_string($uri) === true) {
+if (!is_array($uri)) {
     $uri = [$uri];  // ensure $uril is table
 }
 

diff --git a/includes/config/include.php b/includes/config/include.php
@@ -28,7 +28,7 @@
 
 define('TP_VERSION', '3.1.2');
 define("UPGRADE_MIN_DATE", "1727110744");
-define('TP_VERSION_MINOR', '143');
+define('TP_VERSION_MINOR', '144');
 define('TP_TOOL_NAME', 'Teampass');
 define('TP_ONE_DAY_SECONDS', 86400);
 define('TP_ONE_WEEK_SECONDS', 604800);

diff --git a/includes/core/login.php b/includes/core/login.php
@@ -77,7 +77,7 @@
         exit;
     } else {
         // Gérer les erreurs
-        echo 'Erreur lors de la récupération des informations utilisateur : ' . $userInfo['message'];
+        echo 'Erreur lors de la récupération des informations utilisateur : ' . htmlspecialchars($userInfo['message'], ENT_QUOTES, 'UTF-8');
     };
 }
 
@@ -87,7 +87,6 @@
     // Check if user exists in Teampass
     if (WIP === true) {
         error_log('---- CALLBACK LOGIN ----');
-        //error_log('Info : ' . print_r($session->get('userOauth2Info'), true));
     }
 
     $session->set('user-login', strstr($session->get('userOauth2Info')['userPrincipalName'], '@', true));

diff --git a/index.php b/index.php
@@ -686,9 +686,9 @@
                     <!-- /.sidebar-menu -->
                 <div class="menu-footer">
                     <div class="" id="sidebar-footer">
-                        <i class="fa-solid fa-clock-o mr-2 infotip text-info pointer" title="<?php echo $lang->get('server_time') . ' ' .
+                        <i class="fa-solid fa-clock-o mr-2 infotip text-info pointer" title="<?php echo htmlspecialchars($lang->get('server_time') . ' ' .
                             date($date_format, (int) $server['request_time']) . ' - ' .
-                            date($time_format, (int) $server['request_time']); ?>"></i>
+                            date($time_format, (int) $server['request_time']), ENT_QUOTES, 'UTF-8'); ?>"></i>
                         <i class="fa-solid fa-users mr-2 infotip text-info pointer" title="<?php echo $session_nb_users_online . ' ' . $lang->get('users_online'); ?>"></i>
                         <a href="<?php echo DOCUMENTATION_URL; ?>" target="_blank" class="text-info"><i class="fa-solid fa-book mr-2 infotip" title="<?php echo $lang->get('documentation_canal'); ?>"></i></a>
                         <a href="<?php echo HELP_URL; ?>" target="_blank" class="text-info"><i class="fa-solid fa-life-ring mr-2 infotip" title="<?php echo $lang->get('admin_help'); ?>"></i></a>

diff --git a/pages/admin.php b/pages/admin.php
@@ -224,7 +224,9 @@
     }
 }
 catch (Exception $e) {
-    error_log('TEAMPASS Error - admin page - '.$e->getMessage());
+    if (defined('LOG_TO_SERVER') && LOG_TO_SERVER === true) {
+        error_log('TEAMPASS Error - admin page - '.$e->getMessage());
+    }
     // deepcode ignore ServerLeak: no critical information is provided
     echo 'An error occurred. Please refer to server logs.';
 }
@@ -306,7 +308,7 @@
 
                         // Test internet access
                         $connected = @fsockopen("www.cloudflare.com", 443, $errno, $errstr, 1); // API Duo API (MFA).
-                        if ($connected){
+                        if ($connected !== false) {
                             fclose($connected);
                             $internetAccess = '
                             <p>

diff --git a/pages/tasks.php b/pages/tasks.php
@@ -151,7 +151,9 @@
     }
 }
 catch (Exception $e) {
-    error_log('TEAMPASS Error - tasks page - '.$e->getMessage());
+    if (defined('LOG_TO_SERVER') && LOG_TO_SERVER === true) {
+        error_log('TEAMPASS Error - tasks page - '.$e->getMessage());
+    }
     // deepcode ignore ServerLeak: no critical information is provided
     echo "An error occurred.";
 }?>

diff --git a/pages/uploads.js.php b/pages/uploads.js.php
@@ -32,15 +32,15 @@
 use TeampassClasses\PerformChecks\PerformChecks;
 use TeampassClasses\SessionManager\SessionManager;
 use TeampassClasses\ConfigManager\ConfigManager;
-use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Request as RequestLocal;
 use TeampassClasses\Language\Language;
 // Load functions
 require_once __DIR__.'/../sources/main.functions.php';
 
 // init
 loadClasses();
 $session = SessionManager::getSession();
-$request = Request::createFromGlobals();
+$request = RequestLocal::createFromGlobals();
 $lang = new Language(); 
 
 if ($session->get('key') === null) {

diff --git a/pages/uploads.php b/pages/uploads.php
@@ -28,7 +28,7 @@
  */
 
 use TeampassClasses\SessionManager\SessionManager;
-use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Request as RequestLocal;
 use TeampassClasses\Language\Language;
 use TeampassClasses\NestedTree\NestedTree;
 use TeampassClasses\PerformChecks\PerformChecks;
@@ -40,7 +40,7 @@
 // init
 loadClasses('DB');
 $session = SessionManager::getSession();
-$request = Request::createFromGlobals();
+$request = RequestLocal::createFromGlobals();
 $lang = new Language(); 
 
 // Load config if $SETTINGS not defined

diff --git a/scripts/background_tasks___functions.php b/scripts/background_tasks___functions.php
@@ -101,7 +101,9 @@ function clearTasksLog()
  */
 function provideLog(string $message, array $SETTINGS)
 {
-    error_log((string) date($SETTINGS['date_format'] . ' ' . $SETTINGS['time_format'], time()) . ' - '.$message);
+    if (defined('LOG_TO_SERVER') && LOG_TO_SERVER === true) {
+        error_log((string) date($SETTINGS['date_format'] . ' ' . $SETTINGS['time_format'], time()) . ' - '.$message);
+    }
 }
 
 function performVisibleFoldersHtmlUpdate (int $user_id)

diff --git a/scripts/background_tasks___items_handler.php b/scripts/background_tasks___items_handler.php
@@ -344,7 +344,7 @@ function handleTaskStep(
             0,
             -1,
             (int) $ProcessArguments['item_id'],
-            (string) array_key_exists('pwd', $ProcessArguments) === true ? $ProcessArguments['pwd'] : (array_key_exists('object_key', $ProcessArguments) === true ? $ProcessArguments['object_key'] : ''),
+            (string) (array_key_exists('pwd', $ProcessArguments) === true ? $ProcessArguments['pwd'] : (array_key_exists('object_key', $ProcessArguments) === true ? $ProcessArguments['object_key'] : '')),
             false,
             false,
             [],

diff --git a/scripts/background_tasks___items_handler_subtask.php b/scripts/background_tasks___items_handler_subtask.php
@@ -109,7 +109,7 @@
         0,
         -1,
         (int) $ProcessArguments['item_id'],
-        (string) array_key_exists('pwd', $ProcessArguments) === true ? $ProcessArguments['pwd'] : (array_key_exists('object_key', $ProcessArguments) === true ? $ProcessArguments['object_key'] : ''),
+        (string) (array_key_exists('pwd', $ProcessArguments) === true ? $ProcessArguments['pwd'] : (array_key_exists('object_key', $ProcessArguments) === true ? $ProcessArguments['object_key'] : '')),
         false,
         false,
         [],

diff --git a/scripts/background_tasks___userKeysCreation.php b/scripts/background_tasks___userKeysCreation.php
@@ -250,14 +250,12 @@ function getSubTasks($taskId) {
  */
 function updateSubTask($subTaskId, $args) {
     if (empty($subTaskId) === true) {
-        error_log('Subtask ID is empty ... are we lost!?!');
+        if (defined('LOG_TO_SERVER') && LOG_TO_SERVER === true) {
+            error_log('Subtask ID is empty ... are we lost!?!');
+        }
         return;
     }
     // Convertir les paramètres de la tâche en JSON
-    //$taskParamsJson = json_encode($taskParams);
-
-    //error_log('Subtask update : '.$subTaskId." - ".print_r($taskParams,true));
-
     $query = [
         'updated_at' => time(), // Mettre à jour la date de dernière modification
         'is_in_progress' => 1,
@@ -293,7 +291,6 @@ function reloadSubTask($subTaskId) {
 }
 
 function updateTask($taskId) {
-    //error_log('Task update : '.$taskId);
     // Mettre à jour la tâche dans la base de données
     DB::update(prefixTable('background_tasks'), [
         'updated_at' => time(), // Mettre à jour la date de dernière modification

diff --git a/scripts/background_tasks___userKeysCreation_subtaskHdl.php b/scripts/background_tasks___userKeysCreation_subtaskHdl.php
@@ -58,9 +58,20 @@
 // Once all items are processed, the subtask is marked as FINISHED
 
 $arguments = $_SERVER['argv'];
-array_shift($arguments);
 
+// Is there any arguments in array ?
+if (!is_array($arguments)) {
+    // Stop the script
+    echo "Erreur : Les arguments ne sont pas disponibles ou ne sont pas un tableau.\n";
+
+    if (defined('LOG_TO_SERVER') && LOG_TO_SERVER === true) {
+        error_log('Error: Arguments are not available or not an array. (background_tasks___userKeysCreation_subtaskHdl.php)');
+    }
+    exit(1);
+}
+array_shift($arguments);
 
+// Get the arguments
 $inputData = [
     'subTaskId' => $_SERVER['argv'][1],
     'index' => $_SERVER['argv'][2],

diff --git a/scripts/task_maintenance_purge_old_files.php b/scripts/task_maintenance_purge_old_files.php
@@ -73,42 +73,45 @@ function purgeTemporaryFiles(): void
     // Load expected files
     require_once __DIR__. '/../sources/main.functions.php';
 
-    if (isset($SETTINGS) === true) {
-        //read folder
-        if (is_dir($SETTINGS['path_to_files_folder']) === true) {
-            $dir = opendir($SETTINGS['path_to_files_folder']);
-            if ($dir !== false) {
-                //delete file FILES
-                while (false !== ($f = readdir($dir))) {
-                    if ($f !== '.' && $f !== '..' && $f !== '.htaccess') {
-                        if (file_exists($dir . $f) && ((time() - filectime($dir . $f)) > 604800)) {
-                            fileDelete($dir . '/' . $f, $SETTINGS);
-                        }
+    // Verify if $SETTINGS is defined
+    if (isset($SETTINGS) === false) {
+        throw new \RuntimeException('Settings are not defined.');
+    }
+
+    // $SETTINGS is set then read folder
+    if (is_dir($SETTINGS['path_to_files_folder']) === true) {
+        $dir = opendir($SETTINGS['path_to_files_folder']);
+        if ($dir !== false) {
+            //delete file FILES
+            while (false !== ($f = readdir($dir))) {
+                if ($f !== '.' && $f !== '..' && $f !== '.htaccess') {
+                    $filePath = $SETTINGS['path_to_files_folder'] . '/' . $f;
+                    if (file_exists($filePath) && ((time() - filectime($filePath)) > 604800)) {
+                        fileDelete($dir . '/' . $f, $SETTINGS);
                     }
                 }
-
-                //Close dir
-                closedir($dir);
             }
+
+            //Close dir
+            closedir($dir);
         }
+    }
+
+    //read folder  UPLOAD
+    if (is_dir($SETTINGS['path_to_upload_folder']) === true) {
+        $dir = opendir($SETTINGS['path_to_upload_folder']);
 
-        //read folder  UPLOAD
-        if (is_dir($SETTINGS['path_to_upload_folder']) === true) {
-            $dir = opendir($SETTINGS['path_to_upload_folder']);
-
-            if ($dir !== false) {
-                //delete file
-                while (false !== ($f = readdir($dir))) {
-                    if ($f !== '.' && $f !== '..') {
-                        if (strpos($f, '_delete.') > 0) {
-                            fileDelete($SETTINGS['path_to_upload_folder'] . '/' . $f, $SETTINGS);
-                        }
+        if ($dir !== false) {
+            //delete file
+            while (false !== ($f = readdir($dir))) {
+                if ($f !== '.' && $f !== '..') {
+                    if (strpos($f, '_delete.') > 0) {
+                        fileDelete($SETTINGS['path_to_upload_folder'] . '/' . $f, $SETTINGS);
                     }
                 }
-                //Close dir
-                closedir($dir);
             }
+            //Close dir
+            closedir($dir);
         }
     }
-
 }