refactor: get rid of direct access of $_FILES

thorsten · Oct 5, 2024 · 7dc64e7 · 7dc64e7
1 parent b7f70f7
commit 7dc64e7
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 61 deletions.
diff --git a/phpmyfaq/admin/backup.import.php b/phpmyfaq/admin/backup.import.php
@@ -26,6 +26,7 @@
 use phpMyFAQ\Template\TwigWrapper;
 use phpMyFAQ\Translation;
 use phpMyFAQ\User\CurrentUser;
+use Symfony\Component\HttpFoundation\Request;
 
 if (!defined('IS_VALID_PHPMYFAQ')) {
     http_response_code(400);
@@ -48,31 +49,34 @@
         'adminHeaderRestore' => Translation::get('ad_csv_rest')
     ];
 
-    if (isset($_FILES['userfile']) && 0 === $_FILES['userfile']['error']) {
+    $request = Request::createFromGlobals();
+    $file = $request->files->get('userfile');
+
+    if ($file && $file->isValid()) {
         $ok = 1;
         $fileInfo = new finfo(FILEINFO_MIME_ENCODING);
 
         $dbHelper = new DatabaseHelper($faqConfig);
         $backup = new Backup($faqConfig, $dbHelper);
 
-        if ('utf-8' !== $fileInfo->file($_FILES['userfile']['tmp_name'])) {
+        if ('utf-8' !== $fileInfo->file($file->getPathname())) {
             $templateVars = [
                 ...$templateVars,
                 'errorMessageWrongEncoding' => 'This file is not UTF-8 encoded.'
             ];
             $ok = 0;
         }
 
-        $handle = fopen($_FILES['userfile']['tmp_name'], 'r');
+        $handle = fopen($file->getPathname(), 'r');
         $backupData = fgets($handle, 65536);
         $versionFound = Strings::substr($backupData, 0, 9);
         $versionExpected = '-- pmf' . substr($faqConfig->getVersion(), 0, 3);
         $queries = [];
 
-        $fileName = $_FILES['userfile']['name'];
+        $fileName = $file->getClientOriginalName();
 
         try {
-            $verification = $backup->verifyBackup(file_get_contents($_FILES['userfile']['tmp_name']), $fileName);
+            $verification = $backup->verifyBackup(file_get_contents($file->getPathname()), $fileName);
             if ($verification) {
                 $ok = 1;
             } else {
@@ -184,7 +188,7 @@
             ];
         }
     } else {
-        $errorMessage = match ($_FILES['userfile']['error']) {
+        $errorMessage = match ($file->getError()) {
             1 => 'The uploaded file exceeds the upload_max_filesize directive in php.ini.',
             2 => 'The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the ' . 'HTML form.',
             3 => 'The uploaded file was only partially uploaded.',

diff --git a/phpmyfaq/admin/category.main.php b/phpmyfaq/admin/category.main.php
@@ -31,6 +31,7 @@
 use phpMyFAQ\Template\TwigWrapper;
 use phpMyFAQ\Translation;
 use phpMyFAQ\User\CurrentUser;
+use Symfony\Component\HttpFoundation\Request;
 
 if (!defined('IS_VALID_PHPMYFAQ')) {
     http_response_code(400);
@@ -45,7 +46,8 @@
 //
 // Image upload
 //
-$uploadedFile = (isset($_FILES['image']['size']) && $_FILES['image']['size'] > 0) ? $_FILES['image'] : [];
+$request = Request::createFromGlobals();
+$uploadedFile = $request->files->get('image');
 $categoryImage = new Image($faqConfig);
 $categoryImage->setUploadedFile($uploadedFile);
 

diff --git a/phpmyfaq/src/phpMyFAQ/Attachment/AttachmentFactory.php b/phpmyfaq/src/phpMyFAQ/Attachment/AttachmentFactory.php
@@ -116,25 +116,4 @@ public static function init(string $defaultKey, bool $encryptionEnabled): void
             self::$encryptionEnabled = $encryptionEnabled;
         }
     }
-
-    /**
-     * Re-arranges the $_FILES array for multiple file uploads.
-     *
-     * @param array $filePost
-     * @return array
-     */
-    public static function rearrangeUploadedFiles(array $filePost): array
-    {
-        $filesArray = [];
-        $filesCount = is_countable($filePost['name']) ? count($filePost['name']) : 0;
-        $filesKeys = array_keys($filePost);
-
-        for ($i = 0; $i < $filesCount; ++$i) {
-            foreach ($filesKeys as $fileKey) {
-                $filesArray[$i][$fileKey] = $filePost[$fileKey][$i];
-            }
-        }
-
-        return $filesArray;
-    }
 }
diff --git a/phpmyfaq/src/phpMyFAQ/Attachment/Filesystem/AbstractFile.php b/phpmyfaq/src/phpMyFAQ/Attachment/Filesystem/AbstractFile.php
@@ -20,6 +20,7 @@
 use phpMyFAQ\Attachment\Filesystem\File\FileException;
 use RecursiveDirectoryIterator;
 use RecursiveIteratorIterator;
+use Symfony\Component\HttpFoundation\Request;
 
 /**
  * Class File.
@@ -97,23 +98,21 @@ abstract public function putChunk($chunk);
     /**
      * Deletes the file.
      *
-     * @see inc/PMF_Attachment/Filesystem/PMF_Attachment_Filesystem_Entry#delete()
-     *
      * @throws FileException
      */
     public function delete(): bool
     {
-        $deleted = true;
-
         if ($this->handle) {
             fclose($this->handle);
         }
 
-        if (isset($_FILES['userfile']) && $this->path !== $_FILES['userfile']['tmp_name'] && file_exists($this->path)) {
+        $request = Request::createFromGlobals();
+        $uploadedFile = $request->files->get('userfile');
+        if ($uploadedFile && $this->path !== $uploadedFile->getPathname() && file_exists($this->path)) {
             return $this->deleteDir(dirname($this->path));
         }
 
-        return $deleted;
+        return true;
     }
 
     /**

diff --git a/phpmyfaq/src/phpMyFAQ/Category/Image.php b/phpmyfaq/src/phpMyFAQ/Category/Image.php
@@ -47,8 +47,6 @@ public function __construct(private readonly Configuration $configuration)
 
     /**
      * Sets the uploaded file array from $_FILES.
-     *
-     *
      */
     public function setUploadedFile(array $uploadedFile): Image
     {

diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Administration/AttachmentController.php b/phpmyfaq/src/phpMyFAQ/Controller/Administration/AttachmentController.php
@@ -49,12 +49,10 @@ public function delete(Request $request): JsonResponse
 
             $attachment = AttachmentFactory::create($deleteData->attId);
             if ($attachment->delete()) {
-                $result = ['success' => Translation::get('msgAttachmentsDeleted')];
-                return $this->json($result, Response::HTTP_OK);
-            } else {
-                $result = ['error' => Translation::get('ad_att_delfail')];
-                return $this->json($result, Response::HTTP_BAD_REQUEST);
+                return $this->json(['success' => Translation::get('msgAttachmentsDeleted')], Response::HTTP_OK);
             }
+
+            return $this->json(['error' => Translation::get('ad_att_delfail')], Response::HTTP_BAD_REQUEST);
         } catch (AttachmentException $attachmentException) {
             $result = ['error' => $attachmentException->getMessage()];
             return $this->json($result, Response::HTTP_INTERNAL_SERVER_ERROR);
@@ -99,24 +97,24 @@ public function upload(Request $request): JsonResponse
     {
         $this->userHasPermission(PermissionType::ATTACHMENT_ADD);
 
-        if (!isset($_FILES['filesToUpload'])) {
-            return $this->json([], Response::HTTP_BAD_REQUEST);
+        $files = $request->files->get('filesToUpload');
+
+        if (!$files) {
+            return $this->json(['error' => 'No files to upload.'], Response::HTTP_BAD_REQUEST);
         }
 
-        $files = AttachmentFactory::rearrangeUploadedFiles($_FILES['filesToUpload']);
         $uploadedFiles = [];
-
         foreach ($files as $file) {
             if (
-                is_uploaded_file($file['tmp_name']) &&
-                $file['size'] <= Configuration::getConfigurationInstance()->get('records.maxAttachmentSize') &&
-                $file['type'] !== "text/html"
+                $file->isValid() &&
+                $file->getSize() <= $this->configuration->get('records.maxAttachmentSize') &&
+                $file->getMimeType() !== 'text/html'
             ) {
                 $attachment = AttachmentFactory::create();
                 $attachment->setRecordId($request->request->get('record_id'));
                 $attachment->setRecordLang($request->request->get('record_lang'));
                 try {
-                    if (!$attachment->save($file['tmp_name'], $file['name'])) {
+                    if (!$attachment->save($file->getPathname(), $file->getClientOriginalName())) {
                         throw new AttachmentException();
                     }
                 } catch (AttachmentException) {
@@ -130,7 +128,7 @@ public function upload(Request $request): JsonResponse
                     'faqLanguage' => $request->request->get('record_lang')
                 ];
             } else {
-                return $this->json('The image is too large.', Response::HTTP_BAD_REQUEST);
+                return $this->json(['error' => 'The image is too large.'], Response::HTTP_BAD_REQUEST);
             }
         }
 

diff --git a/phpmyfaq/src/phpMyFAQ/Controller/Administration/ImageController.php b/phpmyfaq/src/phpMyFAQ/Controller/Administration/ImageController.php
@@ -38,43 +38,41 @@ public function upload(Request $request): JsonResponse
     {
         $this->userHasPermission(PermissionType::FAQ_EDIT);
 
-        $configuration = Configuration::getConfigurationInstance();
-        $uploadDir =  PMF_CONTENT_DIR . '/user/images/';
+        $uploadDir = PMF_CONTENT_DIR . '/user/images/';
         $validFileExtensions = ['gif', 'jpg', 'jpeg', 'png'];
         $timestamp = time();
 
         if (!Token::getInstance()->verifyToken('edit-faq', $request->query->get('csrf'))) {
             return $this->json(['error' => Translation::get('err_NotAuth')], Response::HTTP_UNAUTHORIZED);
         }
 
-        reset($_FILES);
-        $temp = current($_FILES);
+        $file = $request->files->get('file');
         $headers = [];
-        if (is_uploaded_file($temp['tmp_name'])) {
+        if ($file && $file->isValid()) {
             if (
                 $request->server->get('HTTP_ORIGIN') !== null &&
-                $request->server->get('HTTP_ORIGIN') . '/' === $configuration->getDefaultUrl()
+                $request->server->get('HTTP_ORIGIN') . '/' === $this->configuration->getDefaultUrl()
             ) {
                 $headers = ['Access-Control-Allow-Origin', $request->server->get('HTTP_ORIGIN')];
             }
 
             // Sanitize input
-            if (preg_match("/([^\w\s\d\-_~,;:\[\]\(\).])|([\.]{2,})/", (string) $temp['name'])) {
+            if (preg_match("/([^\w\s\d\-_~,;:\[\]\(\).])|([\.]{2,})/", $file->getClientOriginalName())) {
                 return $this->json([], Response::HTTP_BAD_REQUEST, $headers);
             }
 
             // Verify extension
-            if (!in_array(strtolower(pathinfo((string) $temp['name'], PATHINFO_EXTENSION)), $validFileExtensions)) {
+            if (!in_array(strtolower($file->getClientOriginalExtension()), $validFileExtensions)) {
                 return $this->json([], Response::HTTP_BAD_REQUEST, $headers);
             }
 
             // Accept upload if there was no origin, or if it is an accepted origin
-            $fileName = $timestamp . '_' . $temp['name'];
-            move_uploaded_file($temp['tmp_name'], $uploadDir . $fileName);
+            $fileName = $timestamp . '_' . $file->getClientOriginalName();
+            $file->move($uploadDir, $fileName);
 
             // Respond to the successful upload with JSON with the full URL of the uploaded image.
             return $this->json(
-                ['location' => $configuration->getDefaultUrl() . 'content/user/images/' . $fileName],
+                ['location' => $this->configuration->getDefaultUrl() . 'content/user/images/' . $fileName],
                 Response::HTTP_OK,
                 $headers
             );