[MEDIA] Fix trying to display file wich is not available locally

actions/attachment.php

@@ -59,15 +59,20 @@ class AttachmentAction extends ManagedAction
     {
         parent::prepare($args);
 
-        if (!empty($id = $this->trimmed('attachment'))) {
+        try {
-            $this->attachment = File::getByID($id);
+            if (!empty($id = $this->trimmed('attachment'))) {
-        } elseif (!empty($filehash = $this->trimmed('filehash'))) {
+                $this->attachment = File::getByID($id);
-            $this->attachment = File::getByHash($filehash);
+            } elseif (!empty($filehash = $this->trimmed('filehash'))) {
+                $this->attachment = File::getByHash($filehash);
+            }
+        } catch (Exception $e) {
+            // Not found
         }
-
         if (!$this->attachment instanceof File) {
             // TRANS: Client error displayed trying to get a non-existing attachment.
             $this->clientError(_('No such attachment.'), 404);
+        } elseif (empty($this->attachment->filename)) {
+            $this->clientError(_('Requested local URL for a file that is not stored locally.'), 404);
         }
         return true;
     }
@@ -145,8 +150,11 @@ class AttachmentAction extends ManagedAction
         if (common_config('site', 'use_x_sendfile')) {
             return null;
         }
-
+        try {
-        return filemtime($this->attachment->getPath());
+            return filemtime($this->attachment->getPath());
+        } catch (InvalidFilenameException $e) {
+            return null;
+        }
     }
 
     /**
@@ -165,11 +173,15 @@ class AttachmentAction extends ManagedAction
 
         $cache = Cache::instance();
         if($cache) {
-            $key = Cache::key('attachments:etag:' . $this->attachment->getPath());
+            try {
-            $etag = $cache->get($key);
+                $key = Cache::key('attachments:etag:' . $this->attachment->getPath());
-            if($etag === false) {
+                $etag = $cache->get($key);
-                $etag = crc32(file_get_contents($this->attachment->getPath()));
+                if($etag === false) {
-                $cache->set($key,$etag);
+                    $etag = crc32(file_get_contents($this->attachment->getPath()));
+                    $cache->set($key,$etag);
+                }
+            } catch (InvalidFilenameException $e) {
+                return null;
             }
             return $etag;
         }
@@ -181,22 +193,23 @@ class AttachmentAction extends ManagedAction
     /**
      * Include $filepath in the response, for viewing and downloading
      */
-    static function sendFile(string $filepath, int $size) {
+    static function sendFile(string $filepath, int $filesize) {
         if (common_config('site', 'use_x_sendfile')) {
             header('X-Sendfile: ' . $filepath);
         } else {
-            // ensure we have a file size
+            if (empty($filesize)) {
-            if (empty($size)) {
+                $filesize = filesize($filepath);
-                $size = filesize($filepath);
             }
-            header("Content-Length: {$size}");
+            header("Content-Length: {$filesize}");
             // header('Cache-Control: private, no-transform, no-store, must-revalidate');
 
             $ret = @readfile($filepath);
 
-            if ($ret === false || $ret !== $filesize) {
+            if (ret === false) {
+                common_log(LOG_ERR, "Couldn't read file at {$filepath}.");
+            } elseif ($ret !== $filesize) {
                 common_log(LOG_ERR, "The lengths of the file as recorded on the DB (or on disk) for the file " .
-                           "{$filepath}, with id={$this->attachment->id} differ from what was sent to the user.");
+                           "{$filepath} differ from what was sent to the user ({$filesize} vs {$ret}).");
             }
         }
     }

actions/attachment_download.php

@@ -31,6 +31,6 @@ class Attachment_downloadAction extends AttachmentAction
         header('Expires: 0');
         header('Content-Transfer-Encoding: binary'); // FIXME? Can this be different?
 
-        parent::sendFile($filepath, $filesize);
+        AttachmentAction::sendFile($filepath, $filesize);
     }
 }

actions/attachment_thumbnail.php

@@ -64,12 +64,17 @@ class Attachment_thumbnailAction extends AttachmentAction
             $file = $e->file;
         }
 
-        if (!empty($thumb)) {
+        try {
-            $filepath = $thumb->getPath();
+            if (!empty($thumb)) {
-            $filesize = 0;
+                $filepath = $thumb->getPath();
-        } else {
+                $size = 0;
-            $filepath = $file->getPath();
+            } elseif ($file->isLocal()) {
-            $filesize = $file->size;
+                $filepath = $file->getPath();
+                $size = $file->size;
+            }
+        } catch (InvalidFilenameException $e) {
+            // We don't have a file to display
+            return;
         }
 
         $filename = MediaFile::getDisplayName($file);
@@ -85,6 +90,6 @@ class Attachment_thumbnailAction extends AttachmentAction
         header('Expires: 0');
         header('Content-Transfer-Encoding: binary');
 
-        parent::sendFile($filepath, $filesize);
+        AttachmentAction::sendFile($filepath, $size);
     }
 }

actions/attachment_view.php

@@ -34,6 +34,6 @@ class Attachment_viewAction extends AttachmentAction
         header('Expires: 0');
         header('Content-Transfer-Encoding: binary');
 
-        parrent::sendFile($filepath, $filesize);
+        AttachmentAction::sendFile($filepath, $filesize);
     }
 }

classes/File.php

@@ -127,11 +127,11 @@ class File extends Managed_DataObject
                 $args = $r->map(mb_substr($u['path'], 1));
                 if ($args['action'] === 'attachment') {
                     try {
-                        // $args['attachment'] should always be set if action===attachment, given our routing rules
+                        if (!empty($args['attachment'])) {
-                        $file = File::getByID($args['attachment']);
+                            return File::getByID($args['attachment']);
-                        return $file;
+                        } elseif ($args['filehash']) {
-                    } catch (EmptyPkeyValueException $e) {
+                            return File::getByHash($args['filehash']);
-                        // ...but $args['attachment'] can also be 0...
+                        }
                     } catch (NoResultException $e) {
                         // apparently this link goes to us, but is _not_ an existing attachment (File) ID?
                     }

lib/mediafile.php

@@ -577,6 +577,10 @@ class MediaFile
      * @returns string
      */
     public static function getDisplayName(File $file) : string {
+        if (empty($file->filename)) {
+            return _('Untitled attachment');
+        }
+
         // New file name format is "{bin2hex(original_name.ext)}-{$hash}"
         $ret = preg_match('/^([^\.-]+)-.+$/', $file->filename, $matches);
         // If there was an error in the match, something's wrong with some piece

plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php

@@ -83,6 +83,10 @@ class StoreRemoteMediaPlugin extends Plugin
 
         $remoteUrl = $file->getUrl();
 
+        if (empty($remoteUrl)) {
+            return true;
+        }
+
         if (!$this->checkWhiteList($remoteUrl) ||
             !$this->checkBlackList($remoteUrl)) {
 		    return true;