From da82048d779e2e8c4253837be2d6b98596878c4f Mon Sep 17 00:00:00 2001 From: Miguel Dantas Date: Fri, 28 Jun 2019 00:18:27 +0100 Subject: [PATCH] [MEDIA] Fix trying to display file wich is not available locally --- actions/attachment.php | 51 ++++++++++++------- actions/attachment_download.php | 2 +- actions/attachment_thumbnail.php | 19 ++++--- actions/attachment_view.php | 2 +- classes/File.php | 10 ++-- lib/mediafile.php | 4 ++ .../StoreRemoteMediaPlugin.php | 4 ++ 7 files changed, 59 insertions(+), 33 deletions(-) diff --git a/actions/attachment.php b/actions/attachment.php index 04b261506b..0f4153cbcf 100644 --- a/actions/attachment.php +++ b/actions/attachment.php @@ -59,15 +59,20 @@ class AttachmentAction extends ManagedAction { parent::prepare($args); - if (!empty($id = $this->trimmed('attachment'))) { - $this->attachment = File::getByID($id); - } elseif (!empty($filehash = $this->trimmed('filehash'))) { - $this->attachment = File::getByHash($filehash); + try { + if (!empty($id = $this->trimmed('attachment'))) { + $this->attachment = File::getByID($id); + } 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; } - - return filemtime($this->attachment->getPath()); + try { + 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()); - $etag = $cache->get($key); - if($etag === false) { - $etag = crc32(file_get_contents($this->attachment->getPath())); - $cache->set($key,$etag); + try { + $key = Cache::key('attachments:etag:' . $this->attachment->getPath()); + $etag = $cache->get($key); + if($etag === false) { + $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($size)) { - $size = filesize($filepath); + if (empty($filesize)) { + $filesize = 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})."); } } } diff --git a/actions/attachment_download.php b/actions/attachment_download.php index b4b01389d8..6a42c29350 100644 --- a/actions/attachment_download.php +++ b/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); } } diff --git a/actions/attachment_thumbnail.php b/actions/attachment_thumbnail.php index f7cc17f347..3e5fbf222f 100644 --- a/actions/attachment_thumbnail.php +++ b/actions/attachment_thumbnail.php @@ -64,12 +64,17 @@ class Attachment_thumbnailAction extends AttachmentAction $file = $e->file; } - if (!empty($thumb)) { - $filepath = $thumb->getPath(); - $filesize = 0; - } else { - $filepath = $file->getPath(); - $filesize = $file->size; + try { + if (!empty($thumb)) { + $filepath = $thumb->getPath(); + $size = 0; + } elseif ($file->isLocal()) { + $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); } } diff --git a/actions/attachment_view.php b/actions/attachment_view.php index 71e1e0fd6c..d75a45cb76 100644 --- a/actions/attachment_view.php +++ b/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); } } diff --git a/classes/File.php b/classes/File.php index 8ccae352e8..2d27115678 100644 --- a/classes/File.php +++ b/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 - $file = File::getByID($args['attachment']); - return $file; - } catch (EmptyPkeyValueException $e) { - // ...but $args['attachment'] can also be 0... + if (!empty($args['attachment'])) { + return File::getByID($args['attachment']); + } elseif ($args['filehash']) { + return File::getByHash($args['filehash']); + } } catch (NoResultException $e) { // apparently this link goes to us, but is _not_ an existing attachment (File) ID? } diff --git a/lib/mediafile.php b/lib/mediafile.php index bbd7e31b2e..aebe40cb9e 100644 --- a/lib/mediafile.php +++ b/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 diff --git a/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php b/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php index 89635db8b8..e222f4ea60 100644 --- a/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php +++ b/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;