[CORE][StoreRemoteMedia] Fixed bug where sometimes images were written outside the site root

actions/attachment.php

@@ -208,7 +208,8 @@ class AttachmentAction extends ManagedAction
      */
     static function sendFile(string $filepath, $filesize) {
         if (is_string(common_config('site', 'x-static-delivery'))) {
-            $relative_path = end(explode(INSTALLDIR, $filepath));
+            $tmp = explode(INSTALLDIR, $filepath);
+            $relative_path = end($tmp);
             common_debug("Using Static Delivery with header: '" .
                          common_config('site', 'x-static-delivery') . ": {$relative_path}'");
             header(common_config('site', 'x-static-delivery') . ": {$relative_path}");

classes/File.php

@@ -617,7 +617,7 @@ class File extends Managed_DataObject
             }
         } else {
             try {
-                return File_thumbnail::byFile($this)->getPath();
+                return File_thumbnail::byFile($this, true)->getPath();
             } catch (NoResultException $e) {
                 // File not stored locally
                 throw new FileNotStoredLocallyException($this);
@@ -637,10 +637,10 @@ class File extends Managed_DataObject
     {
         if (!empty($thumbnail)) {
             $filepath = $thumbnail->getPath();
-        } elseif (empty($this->filename)) {
+        } elseif (!empty($this->filename)) {
-            $filepath = File_thumbnail::byFile($this)->getPath();
-        } else {
             return $this->mimetype;
+        } else {
+            $filepath = File_thumbnail::byFile($this, true)->getPath();
         }
 
         $info = @getimagesize($filepath);

classes/File_thumbnail.php

@@ -244,11 +244,9 @@ class File_thumbnail extends Managed_DataObject
      */
     public function getHtmlAttrs(array $orig=array(), $overwrite=true)
     {
-        $attrs = [
+        $attrs = [ 'height' => $this->getHeight(),
-                'height' => $this->getHeight(),
+                   'width'  => $this->getWidth(),
-                'width'  => $this->getWidth(),
+                   'src'    => $this->getUrl() ];
-                'src'    => $this->getUrl(),
-            ];
         return $overwrite ? array_merge($orig, $attrs) : array_merge($attrs, $orig);
     }
 

lib/httpclient.php

@@ -402,4 +402,19 @@ class HTTPClient extends HTTP_Request2
         } while ($maxRedirs);
         return new GNUsocial_HTTPResponse($response, $this->getUrl(), $redirs);
     }
+
+    public static function get_filename(string $url, array $headers = null) : string {
+        if ($headers === null) {
+            $head = (new HTTPClient())->head($url);
+            $headers = $head->getHeader();
+            $headers = array_change_key_case($headers, CASE_LOWER);
+        }
+        if (array_key_exists('content-disposition', $headers) &&
+            preg_match('/^.+; filename="(.+?)"$/', $headers['content-disposition'], $matches) === 1) {
+            return $matches[1];
+        } else {
+            common_log(LOG_INFO, "Couldn't determine filename for url: {$url}");
+            return _('Untitled attachment');
+        }
+    }
 }

lib/imagefile.php

@@ -125,12 +125,10 @@ class ImageFile extends MediaFile
             }
 
             // And we'll only consider it an image if it has such a media type
-            switch ($media) {
+            if($media !== 'image') {
-            case 'image':
-                $imgPath = $file->getPath();
-                break;
-            default:
                 throw new UnsupportedMediaException(_m('Unsupported media format.'), $file->getPath());
+            } else if (!empty($file->filename)) {
+                $imgPath = $file->getPath();
             }
         }
 
@@ -376,7 +374,11 @@ class ImageFile extends MediaFile
 
         $type = $this->preferredType();
         $ext = image_type_to_extension($type, true);
-        $outpath = preg_replace("/\.[^\.]+$/", $ext, $outpath);
+        // Decoding returns null if the file is in the old format
+        $filename = MediaFile::decodeFilename(basename($outpath));
+        // Encoding null makes the file use 'untitled', and also replaces the extension
+        $outfilename = MediaFile::encodeFilename($filename, $this->filehash, $ext);
+        $outpath = dirname($outpath) . DIRECTORY_SEPARATOR . $outfilename;
 
         switch ($type) {
          case IMAGETYPE_GIF:
@@ -573,9 +575,8 @@ class ImageFile extends MediaFile
             return $thumb;
         }
 
-        $filename = basename($this->filepath);
         $extension = File::guessMimeExtension($this->mimetype);
-        $outname = "thumb-{$this->fileRecord->getID()}-{$width}x{$height}-{$filename}";
+        $outname = "thumb-{$this->fileRecord->getID()}-{$width}x{$height}-{$this->filename}";
         $outpath = File_thumbnail::path($outname);
 
         // The boundary box for our resizing
@@ -601,16 +602,8 @@ class ImageFile extends MediaFile
         ));
 
         // Perform resize and store into file
-        $this->resizeTo($outpath, $box);
+        $outpath = $this->resizeTo($outpath, $box);
-
+        $outname = basename($outpath);
-        try {
-            // Avoid deleting the original
-            if (!in_array($this->getPath(), [File::path($this->filename), File_thumbnail::path($this->filename)])) {
-                $this->unlink();
-            }
-        } catch (FileNotFoundException $e) {
-            // $this->getPath() says the file doesn't exist anyway, so no point in trying to delete it!
-        }
 
         return File_thumbnail::saveThumbnail(
             $this->fileRecord->getID(),

lib/mediafile.php

@@ -241,7 +241,7 @@ class MediaFile
      * having an extension in the file, removing trust in extensions, while keeping the original name
      * @throws ClientException
      */
-    public static function encodeFilename(string $original_name, string $filehash, string $ext = null) : string
+    public static function encodeFilename($original_name, string $filehash, $ext = null) : string
     {
         if (empty($original_name)) {
             $original_name = _('Untitled attachment');
@@ -274,7 +274,8 @@ class MediaFile
      */
     public static function decodeFilename(string $encoded_filename)
     {
-        $ret = preg_match('/^([^-]+?)-[^-]+$/', $encoded_filename, $matches);
+        // The x is because it is using in thumbnails
+        $ret = preg_match('/^([^-x]+?)-[^-]+$/', $encoded_filename, $matches);
         if ($ret === false) {
             return false;
         } elseif ($ret === 0) {
@@ -650,7 +651,7 @@ class MediaFile
         } elseif ($filename === null) {
             // The old file name format was "{hash}.{ext}" so we didn't have a name
             // This extracts the extension
-            $ret = preg_match('/^.+?\.(.+)$/', $file->filename, $matches);
+            $ret = preg_match('/^.+?\.+?(.+)$/', $file->filename, $matches);
             if ($ret !== 1) {
                 common_log(LOG_ERR, $log_error_msg);
                 return _('Untitled attachment');

plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php

@@ -75,8 +75,15 @@ class StoreRemoteMediaPlugin extends Plugin
                                  'unnecessarily downloading too large files. URL: %s',
                                  $file->getID(), $remoteUrl));
 
+            $url = $remoteUrl;
             $head = $http->head($remoteUrl);
             $remoteUrl = $head->getEffectiveUrl();   // to avoid going through redirects again
+
+            if (empty($remoteUrl)) {
+                common_log(LOG_ERR, "URL after redirects is somehow empty, for URL {$url}");
+                return true;
+            }
+
             if (!$this->checkBlackList($remoteUrl)) {
                 common_log(LOG_WARN, sprintf('%s: Non-blacklisted URL %s redirected to blacklisted URL %s',
                                              __CLASS__, $file->getUrl(), $remoteUrl));
@@ -84,6 +91,7 @@ class StoreRemoteMediaPlugin extends Plugin
             }
 
             $headers = $head->getHeader();
+            $headers = array_change_key_case($headers, CASE_LOWER);
             $filesize = isset($headers['content-length']) ?: $file->getSize();
 
             if (empty($filesize)) {
@@ -133,18 +141,16 @@ class StoreRemoteMediaPlugin extends Plugin
 
             //FIXME: Add some code so we don't have to store duplicate File rows for same hash files.
         } catch (NoResultException $e) {
-            if (preg_match('/^.+; filename="(.+?)"$/', $headers['content-disposition'], $matches) === 1) {
+            $original_name = HTTPClient::get_filename($remoteUrl, $headers);
-                $filename = MediaFile::encodeFilename($matches[1], $filehash);
+            $filename = MediaFile::encodeFilename($original_name, $filehash);
-            } else {
-                common_log(LOG_ERR, "Couldn't determine filename for url: {$remoteUrl}");
-                // throw new ServerError(_("Couldn't determine filename for url: {$remoteUrl}"));
-            }
             $fullpath = File::path($filename);
 
-            common_debug("StoreRemoteMedia retrieved file with id={$file->id} and will store in {$filename}");
+            common_debug("StoreRemoteMedia retrieved url {$remoteUrl} for file with id={$file->id} " .
+                         "and will store in {$fullpath}");
 
             // Write the file to disk if it doesn't exist yet. Throw Exception on failure.
-            if (!file_exists($fullpath) && file_put_contents($fullpath, $imgData) === false) {
+            if ((!file_exists($fullpath) || substr($fullpath, 0, strlen(INSTALLDIR)) != INSTALLDIR) &&
+                file_put_contents($fullpath, $imgData) === false) {
                 throw new ServerException(_('Could not write downloaded file to disk.'));
             }