[FILE][TemporaryFile] Fix various issues now that we also have Symfony's file abstractions

components/Avatar/Controller/Avatar.php

@@ -38,7 +38,6 @@ use Symfony\Component\Form\Extension\Core\Type\FileType;
 use Symfony\Component\Form\Extension\Core\Type\HiddenType;
 use Symfony\Component\Form\Extension\Core\Type\SubmitType;
 use Symfony\Component\Form\FormError;
-use Symfony\Component\HttpFoundation\File\File as SymfonyFile;
 use Symfony\Component\HttpFoundation\Request;
 
 class Avatar extends Controller
@@ -84,32 +83,29 @@ class Avatar extends Controller
                     $form->addError(new FormError(_m('No avatar set, so cannot delete')));
                 }
             } else {
-                $sfile = null;
                 if (isset($data['hidden'])) {
                     // Cropped client side
                     $matches = [];
                     if (!empty(preg_match('/data:([^;]*)(;(base64))?,(.*)/', $data['hidden'], $matches))) {
-                        list(, $mimetype_user, , $encoding_user, $data_user) = $matches;
+                        list(, , , $encoding_user, $data_user) = $matches;
-                        if ($encoding_user == 'base64') {
+                        if ($encoding_user === 'base64') {
                             $data_user = base64_decode($data_user);
-                            $tempfile  = new TemporaryFile(['prefix' => 'avatar']);
+                            $tempfile  = new TemporaryFile(['prefix' => 'gs-avatar']);
-                            $path      = $tempfile->getRealPath();
+                            $tempfile->write($data_user);
-                            file_put_contents($path, $data_user);
-                            $sfile = new SymfonyFile($path);
                         } else {
                             Log::info('Avatar upload got an invalid encoding, something\'s fishy and/or wrong');
                         }
                     }
                 } elseif (isset($data['avatar'])) {
                     // Cropping failed (e.g. disabled js), have file as uploaded
-                    $sfile = $data['avatar'];
+                    $file = $data['avatar'];
                 } else {
                     throw new ClientException('Invalid form');
                 }
-                $attachment     = GSFile::validateAndStoreFileAsAttachment($sfile, Common::config('avatar', 'dir'), $title = null, $is_local = true, $use_unique = $gsactor_id);
+                $attachment = GSFile::validateAndStoreFileAsAttachment($file, dest_dir: Common::config('avatar', 'dir'), is_local: true, actor_id: $gsactor_id);
+                // Must get old id before inserting another one
                 $old_attachment = null;
                 $avatar         = DB::find('avatar', ['gsactor_id' => $gsactor_id]);
-                // Must get old id before inserting another one
                 if ($avatar != null) {
                     $old_attachment = $avatar->delete();
                 }

docs/developer/src/SUMMARY.md

@@ -2,6 +2,7 @@
 
 - [Architecture: Modules](./architecture.md)
     - [Programming Style](./paradigms.md)
+    - [Exceptions](./exceptions.md)
     - [Events](./events.md)
     - [Database](./database.md)
     - [Cache](./cache.md)
@@ -33,4 +34,5 @@
     - [Queues](./core/queues.md)
     - [Files](./core/files.md)
     - [Security](./core/security.md)
-    - [HTTP Client](./core/http.md)
+    - [HTTP Client](./core/http.md)
+    - [Exception handling](./core/exception_handler.md)

docs/developer/src/architecture.md

@@ -12,10 +12,11 @@ which is elaborated in [Database](./database.md);
 - [Routes and Controllers](./routes_and_controllers.md);
 - [Templates](./templates.md);
 - [Internationalization (i18n)](https://en.wikipedia.org/wiki/Internationalization_and_localization), elaborated in [Internationalization](internationalization.md);
+- [Exceptions](./exceptions.md);
 - [Log](./log.md);
 - [Queues](./queues.md);
 - [Files](./files.md);
-- [Security](./security.md);
+- [Sessions and Security](./sessions_and_security.md);
 - [HTTP Client](./http.md).
 
 Everything else uses most of this.
@@ -48,6 +49,13 @@ Currently, GNU social has the following components:
 - Avatar
 - Posting
 
+#### Design principles
+
+- Components are independent so do not interfere with each other;
+- Component implementations are hidden;
+- Communication is through well-defined events and interfaces (for models);
+- One component can be replaced by another if its events are maintained.
+
 ### Plugins (Unix Tools Design Philosophy)
 
 GNU social is true to the Unix-philosophy of small programs to do a small job.

docs/developer/src/core.md

@@ -14,5 +14,5 @@ The `core` tries to be minimal. The essence of it being various wrappers around
 - [Utils](./core/util.md);
 - [Queues](./core/queues.md);
 - [Files](./core/files.md);
-- [Security](./core/security.md);
+- [Sessions and Security](./core/security.md);
 - [HTTP Client](./core/http.md).

docs/developer/src/plugins/configuration.md

@@ -0,0 +1,6 @@
+# Adding configuration to a plugin
+
+## The trade-off between re-usability and usability
+
+The more general the interface, the greater the re-usability, but it is then more complex and hence less usable.
+

plugins/ImageEncoder/ImageEncoder.php

@@ -21,6 +21,8 @@ namespace Plugin\ImageEncoder;
 
 use App\Core\Event;
 use App\Core\GSFile;
+use App\Util\Exception\TemporaryFileException;
+use SplFileInfo;
 use function App\Core\I18n\_m;
 use App\Core\Log;
 use App\Core\Modules\Plugin;
@@ -56,18 +58,18 @@ class ImageEncoder extends Plugin
     /**
      * Encodes the image to self::preferredType() format ensuring it's valid.
      *
-     * @param \SplFileInfo $file
+     * @param SplFileInfo $file
-     * @param null|string  $mimetype in/out
+     * @param null|string   $mimetype in/out
-     * @param null|string  $title    in/out
+     * @param null|string   $title    in/out
-     * @param null|int     $width    out
+     * @param null|int      $width    out
-     * @param null|int     $height   out
+     * @param null|int      $height   out
      *
      * @throws Vips\Exception
-     * @throws \App\Util\Exception\TemporaryFileException
+     * @throws TemporaryFileException
      *
      * @return bool
      */
-    public function onAttachmentValidation(\SplFileInfo &$file, ?string &$mimetype, ?string &$title, ?int &$width, ?int &$height): bool
+    public function onAttachmentValidation(SplFileInfo &$file, ?string &$mimetype, ?string &$title, ?int &$width, ?int &$height): bool
     {
         $original_mimetype = $mimetype;
         if (GSFile::mimetypeMajor($original_mimetype) != 'image') {
@@ -77,14 +79,12 @@ class ImageEncoder extends Plugin
 
         $type      = self::preferredType();
         $extension = image_type_to_extension($type, include_dot: true);
-        $temp      = new TemporaryFile(['prefix' => 'image', 'suffix' => $extension]); // This handles deleting the file if some error occurs
+        // If title seems to be a filename with an extension
-        $mimetype  = image_type_to_mime_type($type);
+        if (preg_match('/\.[a-z0-9]/i', $title) === 1) {
-        if ($mimetype != $original_mimetype) {
+            $title = substr($title, 0, strrpos($title, '.')) . $extension;
-            // If title seems to be a filename with an extension
-            if (preg_match('/\.[a-z0-9]/i', $title) === 1) {
-                $title = substr($title, 0, strrpos($title, '.')) . $extension;
-            }
         }
+        // TemporaryFile handles deleting the file if some error occurs
+        $temp = new TemporaryFile(['prefix' => 'image', 'suffix' => $extension]);
 
         $image  = Vips\Image::newFromFile($file->getRealPath(), ['access' => 'sequential']);
         $width  = Common::clamp($image->width, 0, Common::config('attachments', 'max_width'));
@@ -93,13 +93,9 @@ class ImageEncoder extends Plugin
         $image->writeToFile($temp->getRealPath());
 
         $filesize = $temp->getSize();
-        $filepath = $file->getRealPath();
-        @unlink($filepath);
 
         Event::handle('EnforceQuota', [$filesize]);
 
-        $temp->commit($filepath);
-
         return Event::stop;
     }
 
@@ -155,6 +151,6 @@ class ImageEncoder extends Plugin
         } finally {
             ini_set('memory_limit', $old_limit); // Restore the old memory limit
         }
-        return Event::next;
+        return Event::stop;
     }
 }

src/Core/GSFile.php

@@ -22,49 +22,73 @@
 namespace App\Core;
 
 use App\Core\DB\DB;
-use function App\Core\I18n\_m;
 use App\Entity\Attachment;
 use App\Util\Common;
 use App\Util\Exception\ClientException;
+use App\Util\Exception\DuplicateFoundException;
 use App\Util\Exception\NoSuchFileException;
 use App\Util\Exception\NotFoundException;
 use App\Util\Exception\ServerException;
+use InvalidArgumentException;
+use SplFileInfo;
 use Symfony\Component\HttpFoundation\BinaryFileResponse;
-use Symfony\Component\HttpFoundation\File\File as SymfonyFile;
 use Symfony\Component\HttpFoundation\HeaderUtils;
 use Symfony\Component\HttpFoundation\Response;
+use function App\Core\I18n\_m;
 
+/**
+ * GNU social's File Abstraction
+ *
+ * @category  Files
+ * @package   GNUsocial
+ *
+ * @author    Hugo Sales <hugo@hsal.es>
+ * @author    Diogo Peralta Cordeiro <mail@diogo.site>
+ * @copyright 2020-2021 Free Software Foundation, Inc http://www.fsf.org
+ * @license   https://www.gnu.org/licenses/agpl.html GNU AGPL v3 or later
+ */
 class GSFile
 {
     /**
      * Perform file validation (checks and normalization) and store the given file
+     *
+     * @param SplFileInfo $file
+     * @param string $dest_dir
+     * @param null|string $title
+     * @param bool $is_local
+     * @param null|int $actor_id
+     *
+     * @return Attachment
+     * @throws DuplicateFoundException
      */
-    public static function validateAndStoreFileAsAttachment(SymfonyFile $sfile,
+    public static function validateAndStoreFileAsAttachment(SplFileInfo $file,
                                                             string $dest_dir,
                                                             ?string $title = null,
                                                             bool $is_local = true,
                                                             int $actor_id = null): Attachment
     {
-        Event::handle('HashFile', [$sfile->getPathname(), &$hash]);
+        $hash = null;
+        Event::handle('HashFile', [$file->getPathname(), &$hash]);
         try {
             return DB::findOneBy('attachment', ['file_hash' => $hash]);
         } catch (NotFoundException) {
             // The following properly gets the mimetype with `file` or other
             // available methods, so should be safe
-            $mimetype = $sfile->getMimeType();
+            $mimetype = $file->getMimeType();
-            Event::handle('AttachmentValidation', [&$sfile, &$mimetype, &$title, &$width, &$height]);
+            $title = $width = $height = null;
+            Event::handle('AttachmentValidation', [&$file, &$mimetype, &$title, &$width, &$height]);
             $attachment = Attachment::create([
-                'file_hash'  => $hash,
+                'file_hash' => $hash,
                 'gsactor_id' => $actor_id,
-                'mimetype'   => $mimetype,
+                'mimetype' => $mimetype,
-                'title'      => $title ?: _m('Untitled attachment'),
+                'title' => $title ?: _m('Untitled attachment'),
-                'filename'   => $hash,
+                'filename' => $hash,
-                'is_local'   => $is_local,
+                'is_local' => $is_local,
-                'size'       => $sfile->getSize(),
+                'size' => $file->getSize(),
-                'width'      => $width,
+                'width' => $width,
-                'height'     => $height,
+                'height' => $height,
             ]);
-            $sfile->move($dest_dir, $hash);
+            $file->move($dest_dir, $hash);
             DB::persist($attachment);
             Event::handle('AttachmentStoreNew', [&$attachment]);
             return $attachment;
@@ -74,32 +98,32 @@ class GSFile
     /**
      * Create an attachment for the given URL, fetching the mimetype
      *
-     * @throws \InvalidArgumentException
+     * @throws InvalidArgumentException
      */
     public static function validateAndStoreURLAsAttachment(string $url): Attachment
     {
         if (Common::isValidHttpUrl($url)) {
             $head = HTTPClient::head($url);
             // This must come before getInfo given that Symfony HTTPClient is lazy (thus forcing curl exec)
-            $headers  = $head->getHeaders();
+            $headers = $head->getHeaders();
-            $url      = $head->getInfo('url'); // The last effective url (after getHeaders so it follows redirects)
+            $url = $head->getInfo('url'); // The last effective url (after getHeaders so it follows redirects)
             $url_hash = hash(Attachment::URLHASH_ALGO, $url);
             try {
                 return DB::findOneBy('attachment', ['remote_url_hash' => $url_hash]);
             } catch (NotFoundException) {
-                $headers    = array_change_key_case($headers, CASE_LOWER);
+                $headers = array_change_key_case($headers, CASE_LOWER);
                 $attachment = Attachment::create([
-                    'remote_url'      => $url,
+                    'remote_url' => $url,
                     'remote_url_hash' => $url_hash,
-                    'mimetype'        => $headers['content-type'][0],
+                    'mimetype' => $headers['content-type'][0],
-                    'is_local'        => false,
+                    'is_local' => false,
                 ]);
                 DB::persist($attachment);
                 Event::handle('AttachmentStoreNew', [&$attachment]);
                 return $attachment;
             }
         } else {
-            throw new \InvalidArgumentException();
+            throw new InvalidArgumentException();
         }
     }
 
@@ -116,9 +140,9 @@ class GSFile
                 Response::HTTP_OK,
                 [
                     'Content-Description' => 'File Transfer',
-                    'Content-Type'        => $mimetype,
+                    'Content-Type' => $mimetype,
                     'Content-Disposition' => HeaderUtils::makeDisposition($disposition, $output_filename ?: _m('Untitled attachment'), _m('Untitled attachment')),
-                    'Cache-Control'       => 'public',
+                    'Cache-Control' => 'public',
                 ],
                 $public = true,
                 $disposition = null,
@@ -181,7 +205,7 @@ class GSFile
      */
     public static function getAttachmentFileInfo(int $id): array
     {
-        $res             = self::getFileInfo($id);
+        $res = self::getFileInfo($id);
         $res['filepath'] = Common::config('attachments', 'dir') . $res['file_hash'];
         return $res;
     }

src/Entity/AttachmentThumbnail.php

@@ -44,6 +44,7 @@ use DateTimeInterface;
  * @author    Mikael Nordfeldth <mmn@hethane.se>
  * @copyright 2009-2014 Free Software Foundation, Inc http://www.fsf.org
  * @author    Hugo Sales <hugo@hsal.es>
+ * @author    Diogo Peralta Cordeiro <mail@diogo.site>
  * @copyright 2020-2021 Free Software Foundation, Inc http://www.fsf.org
  * @license   https://www.gnu.org/licenses/agpl.html GNU AGPL v3 or later
  */
@@ -131,6 +132,17 @@ class AttachmentThumbnail extends Entity
         }
     }
 
+    /**
+     * @param Attachment $attachment
+     * @param int        $width
+     * @param int        $height
+     * @param bool       $crop
+     *
+     * @throws ServerException
+     * @throws \App\Util\Exception\TemporaryFileException
+     *
+     * @return mixed
+     */
     public static function getOrCreate(Attachment $attachment, int $width, int $height, bool $crop)
     {
         try {
@@ -141,16 +153,15 @@ class AttachmentThumbnail extends Entity
                 });
         } catch (NotFoundException $e) {
             $ext        = image_type_to_extension(IMAGETYPE_WEBP, include_dot: true);
-            $temp       = new TemporaryFile(['prefix' => 'thumbnail', 'suffix' => $ext]);
+            $temp       = new TemporaryFile(['prefix' => 'gs-thumbnail', 'suffix' => $ext]);
             $thumbnail  = self::create(['attachment_id' => $attachment->getId()]);
             $event_map  = ['image' => 'ResizeImagePath', 'video' => 'ResizeVideoPath'];
             $major_mime = GSFile::mimetypeMajor($attachment->getMimetype());
-            if (in_array($major_mime, array_keys($event_map))) {
+            if (in_array($major_mime, array_keys($event_map)) && !Event::handle($event_map[$major_mime], [$attachment->getPath(), $temp->getRealPath(), &$width, &$height, $crop, &$mimetype])) {
-                Event::handle($event_map[$major_mime], [$attachment->getPath(), $temp->getRealPath(), &$width, &$height, $crop, &$mimetype]);
                 $thumbnail->setWidth($width);
                 $thumbnail->setHeight($height);
                 $filename = "{$width}x{$height}{$ext}-" . $attachment->getFileHash();
-                $temp->commit(Common::config('thumbnail', 'dir') . $filename);
+                $temp->move(Common::config('thumbnail', 'dir'), $filename);
                 $thumbnail->setFilename($filename);
                 DB::persist($thumbnail);
                 DB::flush();

src/Util/TemporaryFile.php

@@ -20,6 +20,7 @@
 namespace App\Util;
 
 use App\Util\Exception\TemporaryFileException;
+use Symfony\Component\Mime\MimeTypes;
 
 /**
  * Class oriented at providing automatic temporary file handling.
@@ -27,7 +28,9 @@ use App\Util\Exception\TemporaryFileException;
  * @package   GNUsocial
  *
  * @author    Alexei Sorokin <sor.alexei@meowr.ru>
- * @copyright 2020 Free Software Foundation, Inc http://www.fsf.org
+ * @author    Hugo Sales <hugo@hsal.es>
+ * @author    Diogo Peralta Cordeiro <mail@diogo.site>
+ * @copyright 2020-2021 Free Software Foundation, Inc http://www.fsf.org
  * @license   https://www.gnu.org/licenses/agpl.html GNU AGPL v3 or later
  */
 class TemporaryFile extends \SplFileInfo
@@ -36,19 +39,25 @@ class TemporaryFile extends \SplFileInfo
 
     /**
      * @param array $options - ['prefix' => ?string, 'suffix' => ?string, 'mode' => ?string, 'directory' => ?string]
+     *                       Description of options:
+     *                       > prefix: The file name will begin with that prefix, default is 'gs-php'
+     *                       > suffix: The file name will begin with that prefix, default is ''
+     *                       > mode: The file name will begin with that prefix, default is 'w+b'
+     *                       > directory: The file name will begin with that prefix, default is the system's temporary
+     *
+     * @throws TemporaryFileException
      */
     public function __construct(array $options = [])
     {
         $attempts = 16;
+        $filename = uniqid(($options['directory'] ?? (sys_get_temp_dir() . '/')) . ($options['prefix'] ?? 'gs-php')) . ($options['suffix'] ?? '');
         for ($count = 0; $count < $attempts; ++$count) {
-            $filename = uniqid(($options['directory'] ?? (sys_get_temp_dir() . '/')) . ($options['prefix'] ?? 'gs-php')) . ($options['suffix'] ?? '');
-
             $this->resource = @fopen($filename, $options['mode'] ?? 'w+b');
             if ($this->resource !== false) {
                 break;
             }
         }
-        if ($count == $attempts) {
+        if ($count == $attempts && $this->resource !== false) {
             // @codeCoverageIgnoreStart
             $this->cleanup();
             throw new TemporaryFileException('Could not open file: ' . $filename);
@@ -64,21 +73,28 @@ class TemporaryFile extends \SplFileInfo
         $this->cleanup();
     }
 
-    public function write($data): int
+    /**
+     * Binary-safe file write
+     *
+     * @see https://php.net/manual/en/function.fwrite.php
+     *
+     * @param string $data The string that is to be written.
+     *
+     * @return null|false|int the number of bytes written, false on error, null on null resource/stream
+     */
+    public function write(string $data): int | false | null
     {
         if (!is_null($this->resource)) {
             return fwrite($this->resource, $data);
         } else {
-            // @codeCoverageIgnoreStart
             return null;
-            // @codeCoverageIgnoreEnd
         }
     }
 
     /**
      * Closes the file descriptor if opened.
      *
-     * @return bool Whether successful
+     * @return bool true on success or false on failure.
      */
     protected function close(): bool
     {
@@ -120,15 +136,27 @@ class TemporaryFile extends \SplFileInfo
      * Release the hold on the temporary file and move it to the desired
      * location, setting file permissions in the process.
      *
-     * @param string File destination
+     * @param string $directory Path where the file should be stored
-     * @param int    New file permissions (in octal mode)
+     * @param string $filename  The filename
+     * @param int    $dirmode   New directory permissions (in octal mode)
+     * @param int    $filemode  New file permissions (in octal mode)
      *
      * @throws TemporaryFileException
      *
      * @return void
      */
-    public function commit(string $destpath, int $umode = 0644): void
+    public function move(string $directory, string $filename, int $dirmode = 0655, int $filemode = 0644): void
     {
+        if (!is_dir($directory)) {
+            if (false === @mkdir($directory, $dirmode, true) && !is_dir($directory)) {
+                throw new TemporaryFileException(sprintf('Unable to create the "%s" directory.', $directory));
+            }
+        } elseif (!is_writable($directory)) {
+            throw new TemporaryFileException(sprintf('Unable to write in the "%s" directory.', $directory));
+        }
+
+        $destpath = rtrim($directory, '/\\') . DIRECTORY_SEPARATOR . $this->getName($filename);
+
         $temppath = $this->getRealPath();
 
         // Might be attempted, and won't end well
@@ -138,21 +166,59 @@ class TemporaryFile extends \SplFileInfo
 
         // Memorise if the file was there and see if there is access
         $exists = file_exists($destpath);
-        if (!@touch($destpath)) {
+
-            throw new TemporaryFileException(
-                'Insufficient permissions for destination: "' . $destpath . '"'
-            );
-        } elseif (!$exists) {
-            // If the file wasn't there, clean it up in case of a later failure
-            unlink($destpath);
-        }
         if (!$this->close()) {
             // @codeCoverageIgnoreStart
             throw new TemporaryFileException('Could not close the resource');
             // @codeCoverageIgnoreEnd
         }
 
-        rename($temppath, $destpath);
+        set_error_handler(function ($type, $msg) use (&$error) { $error = $msg; });
-        chmod($destpath, $umode);
+        $renamed = rename($this->getPathname(), $destpath);
+        restore_error_handler();
+        chmod($destpath, $filemode);
+        if (!$renamed) {
+            if (!$exists) {
+                // If the file wasn't there, clean it up in case of a later failure
+                unlink($destpath);
+            }
+            throw new TemporaryFileException(sprintf('Could not move the file "%s" to "%s" (%s).', $this->getPathname(), $destpath, strip_tags($error)));
+        }
+    }
+
+    /**
+     * This function is a copy of Symfony\Component\HttpFoundation\File\File->getMimeType()
+     * Returns the mime type of the file.
+     *
+     * The mime type is guessed using a MimeTypeGuesserInterface instance,
+     * which uses finfo_file() then the "file" system binary,
+     * depending on which of those are available.
+     *
+     * @return null|string The guessed mime type (e.g. "application/pdf")
+     *
+     * @see MimeTypes
+     */
+    public function getMimeType()
+    {
+        if (!class_exists(MimeTypes::class)) {
+            throw new \LogicException('You cannot guess the mime type as the Mime component is not installed. Try running "composer require symfony/mime".');
+        }
+
+        return MimeTypes::getDefault()->guessMimeType($this->getPathname());
+    }
+
+    /**
+     * This function is a copy of Symfony\Component\HttpFoundation\File\File->getName()
+     * Returns locale independent base name of the given path.
+     *
+     * @return string
+     */
+    protected function getName(string $name)
+    {
+        $originalName = str_replace('\\', '/', $name);
+        $pos          = strrpos($originalName, '/');
+        $originalName = false === $pos ? $originalName : substr($originalName, $pos + 1);
+
+        return $originalName;
     }
 }

tests/Util/TemporaryFileTest.php

@@ -33,7 +33,7 @@ class TemporaryFileTest extends WebTestCase
         $temp = new TemporaryFile();
         static::assertNotNull($temp->getResource());
         $filename = uniqid(sys_get_temp_dir() . '/');
-        $temp->commit($filename);
+        $temp->move($filename);
         static::assertTrue(file_exists($filename));
         @unlink($filename);
     }
@@ -42,7 +42,7 @@ class TemporaryFileTest extends WebTestCase
     {
         $temp     = new TemporaryFile();
         $filename = $temp->getRealPath();
-        static::assertThrows(TemporaryFileException::class, fn () => $temp->commit($filename));
+        static::assertThrows(TemporaryFileException::class, fn () => $temp->move($filename));
-        static::assertThrows(TemporaryFileException::class, fn () => $temp->commit('/root/cannot_write_here'));
+        static::assertThrows(TemporaryFileException::class, fn () => $temp->move('/root/cannot_write_here'));
     }
 }