From 8f31a1a820bcaf8959a095c265db28b4b557bd7e Mon Sep 17 00:00:00 2001 From: Miguel Dantas Date: Sat, 29 Jun 2019 20:10:20 +0100 Subject: [PATCH] [MEDIA][OEMBED] Fixed regression in OEmbed, because it relied on accessing the files directly, which previous commits broke. The File table really should have a bool... --- actions/attachment.php | 43 ++++++++++++++++---------- actions/attachment_download.php | 14 ++++++--- actions/attachment_thumbnail.php | 29 ++++++------------ actions/attachment_view.php | 15 ++++++--- classes/File.php | 52 ++++++++++++++++++++++++++++++++ lib/attachmentlist.php | 2 +- lib/attachmentlistitem.php | 4 ++- lib/imagefile.php | 8 ++--- plugins/Oembed/OembedPlugin.php | 2 +- 9 files changed, 117 insertions(+), 52 deletions(-) diff --git a/actions/attachment.php b/actions/attachment.php index 0f4153cbcf..0851f1ee4e 100644 --- a/actions/attachment.php +++ b/actions/attachment.php @@ -71,7 +71,11 @@ class AttachmentAction extends ManagedAction 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)) { + } + + $filename = $this->attachment->getFileOrThumbnailPath(); + + if (empty($filename)) { $this->clientError(_('Requested local URL for a file that is not stored locally.'), 404); } return true; @@ -100,7 +104,7 @@ class AttachmentAction extends ManagedAction public function showPage() { - if (empty($this->attachment->filename)) { + if (empty($this->attachment->getFileOrThumbnailPath())) { // if it's not a local file, gtfo common_redirect($this->attachment->getUrl(), 303); } @@ -150,9 +154,10 @@ class AttachmentAction extends ManagedAction if (common_config('site', 'use_x_sendfile')) { return null; } - try { - return filemtime($this->attachment->getPath()); - } catch (InvalidFilenameException $e) { + $path = $this->attachment->getFileOrThumbnailPath(); + if (!empty($path)) { + return filemtime($path); + } else { return null; } } @@ -168,26 +173,32 @@ class AttachmentAction extends ManagedAction function etag() { if (common_config('site', 'use_x_sendfile')) { + return null; } + $path = $this->attachment->getFileOrThumbnailPath(); + $cache = Cache::instance(); if($cache) { - 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) { + if (empty($path)) { return null; } + $key = Cache::key('attachments:etag:' . $path); + $etag = $cache->get($key); + if($etag === false) { + $etag = crc32(file_get_contents($path)); + $cache->set($key,$etag); + } return $etag; } - $stat = stat($this->path); - return '"' . $stat['ino'] . '-' . $stat['size'] . '-' . $stat['mtime'] . '"'; + if (!empty($path)) { + $stat = stat($path); + return '"' . $stat['ino'] . '-' . $stat['size'] . '-' . $stat['mtime'] . '"'; + } else { + return null; + } } /** @@ -205,7 +216,7 @@ class AttachmentAction extends ManagedAction $ret = @readfile($filepath); - if (ret === false) { + 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 " . diff --git a/actions/attachment_download.php b/actions/attachment_download.php index 6a42c29350..ebd5e5d956 100644 --- a/actions/attachment_download.php +++ b/actions/attachment_download.php @@ -15,9 +15,15 @@ class Attachment_downloadAction extends AttachmentAction { public function showPage() { - // Checks file exists or throws FileNotStoredLocallyException - $filepath = $this->attachment->getPath(); - $filesize = $this->attachment->size; + // Checks file exists or throws FileNotFoundException + $filepath = $this->attachment->getFileOrThumbnailPath(); + $filesize = $this->attachment->size ?: 0; + $mimetype = $this->attachment->getFileOrThumbnailMimetype(); + + if (empty($filepath)) { + $thiis->clientError(_('No such attachment'), 404); + } + $filename = MediaFile::getDisplayName($this->attachment); // Disable errors, to not mess with the file contents (suppress errors in case access to this @@ -26,7 +32,7 @@ class Attachment_downloadAction extends AttachmentAction @ini_set('display_errors', 0); header("Content-Description: File Transfer"); - header("Content-Type: {$this->attachment->mimetype}"); + header("Content-Type: {$mimetype}"); header("Content-Disposition: attachment; filename=\"{$filename}\""); header('Expires: 0'); header('Content-Transfer-Encoding: binary'); // FIXME? Can this be different? diff --git a/actions/attachment_thumbnail.php b/actions/attachment_thumbnail.php index f0a54883b1..e54e999983 100644 --- a/actions/attachment_thumbnail.php +++ b/actions/attachment_thumbnail.php @@ -55,31 +55,22 @@ class Attachment_thumbnailAction extends AttachmentAction public function showPage() { + // Checks file exists or throws FileNotFoundException + $size = $this->attachment->size ?: 0; + // Returns a File_thumbnail object or throws exception if not available try { - $thumb = $this->attachment->getThumbnail($this->thumb_w, $this->thumb_h, $this->thumb_c); - $file = $thumb->getFile(); + $thumbnail = $this->attachment->getThumbnail($this->thumb_w, $this->thumb_h, $this->thumb_c); + $file = $thumbnail->getFile(); } catch (UseFileAsThumbnailException $e) { // With this exception, the file exists locally $file = $e->file; + } catch(FileNotFoundException $e) { + $this->clientError(_('No such attachment'), 404); } - try { - if (!empty($thumb)) { - $filepath = $thumb->getPath(); - $size = 0; - } elseif ($file->isLocal()) { - $filepath = $file->getPath(); - $size = $file->size; - } - // XXX PHP: Upgrade to PHP 7.1 - // FileNotFoundException | InvalidFilenameException - } catch (Exception $e) { - // We don't have a file to display - $this->clientError(_('No such attachment.'), 404); - return false; - } - + $filepath = $file->getFileOrThumbnailPath(); + $mimetype = $file->getFileOrThumbnailMimetype(); $filename = MediaFile::getDisplayName($file); // Disable errors, to not mess with the file contents (suppress errors in case access to this @@ -88,7 +79,7 @@ class Attachment_thumbnailAction extends AttachmentAction @ini_set('display_errors', 0); header("Content-Description: File Transfer"); - header("Content-Type: {$file->mimetype}"); + header("Content-Type: {$mimetype}"); header("Content-Disposition: inline; filename=\"{$filename}\""); header('Expires: 0'); header('Content-Transfer-Encoding: binary'); diff --git a/actions/attachment_view.php b/actions/attachment_view.php index d75a45cb76..61aba9bd0f 100644 --- a/actions/attachment_view.php +++ b/actions/attachment_view.php @@ -13,9 +13,14 @@ class Attachment_viewAction extends AttachmentAction { public function showPage() { - // Checks file exists or throws FileNotStoredLocallyException - $filepath = $this->attachment->getPath(); - $filesize = $this->attachment->size; + // Checks file exists or throws FileNotFoundException + $filepath = $this->attachment->getFileOrThumbnailPath(); + $filesize = $this->attachment->size ?: 0; + $mimetype = $this->attachment->getFileOrThumbnailMimetype(); + + if (empty($filepath)) { + $thiis->clientError(_('No such attachment'), 404); + } $filename = MediaFile::getDisplayName($this->attachment); @@ -25,8 +30,8 @@ class Attachment_viewAction extends AttachmentAction @ini_set('display_errors', 0); header("Content-Description: File Transfer"); - header("Content-Type: {$this->attachment->mimetype}"); - if (in_array(common_get_mime_media($this->attachment->mimetype), ['image', 'video'])) { + header("Content-Type: {$mimetype}"); + if (in_array(common_get_mime_media($mimetype), ['image', 'video'])) { header("Content-Disposition: inline; filename=\"{$filename}\""); } else { header("Content-Disposition: attachment; filename=\"{$filename}\""); diff --git a/classes/File.php b/classes/File.php index 2d27115678..c6e9b40ac7 100644 --- a/classes/File.php +++ b/classes/File.php @@ -589,6 +589,58 @@ class File extends Managed_DataObject return $filepath; } + /** + * Returns the path to either a file, or it's thumbnail if the file doesn't exist. + * This is useful in case the original file is deleted, or, as is the case for Embed + * thumbnails, we only have a thumbnail and not a file + * @return string Path + * @throws FileNotFoundException + * @throws FileNotStoredLocallyException + * @throws InvalidFilenameException + * @throws ServerException + */ + public function getFileOrThumbnailPath() : string + { + if (!empty($this->filename)) { + $filepath = self::path($this->filename); + if (file_exists($filepath)) { + return $filepath; + } else { + throw new FileNotFoundException($filepath); + } + } else { + try { + return File_thumbnail::byFile($this)->getPath(); + } catch (NoResultException $e) { + // File not stored locally + throw new FileNotStoredLocallyException($this); + } + } + } + + /** + * Return the mime type of the thumbnail if we have it, or, if not, of the File + * @return string + * @throws FileNotFoundException + * @throws NoResultException + * @throws ServerException + * @throws UnsupportedMediaException + */ + public function getFileOrThumbnailMimetype() : string + { + if (empty($this->filename)) { + $filepath = File_thumbnail::byFile($this)->getPath(); + $info = @getimagesize($filepath); + if ($info !== false) { + return $info['mime']; + } else { + throw new UnsupportedMediaException(_("Thumbnail is not an image.")); + } + } else { + return $this->mimetype; + } + } + public function getAttachmentUrl() { return common_local_url('attachment', array('attachment'=>$this->getID())); diff --git a/lib/attachmentlist.php b/lib/attachmentlist.php index 0ce19b0b1e..8c52d7a86e 100644 --- a/lib/attachmentlist.php +++ b/lib/attachmentlist.php @@ -77,7 +77,7 @@ class AttachmentList extends Widget $attachments = $this->notice->attachments(); foreach ($attachments as $key=>$att) { // Remove attachments which are not representable with neither a title nor thumbnail - if ($att->getTitle() === null && !$att->hasThumbnail()) { + if ($att->getTitle() === _('Untitled attachment') && !$att->hasThumbnail()) { unset($attachments[$key]); } } diff --git a/lib/attachmentlistitem.php b/lib/attachmentlistitem.php index 390c450088..af44f7aada 100644 --- a/lib/attachmentlistitem.php +++ b/lib/attachmentlistitem.php @@ -146,7 +146,9 @@ class AttachmentListItem extends Widget } else { try { // getUrl(true) because we don't want to hotlink, could be made configurable - $this->out->element('img', ['class'=>'u-photo', 'src'=>$this->attachment->getUrl(true), 'alt' => $this->attachment->getTitle()]); + $this->out->element('img', ['class'=>'u-photo', + 'src'=>$this->attachment->getUrl(true), + 'alt' => $this->attachment->getTitle()]); } catch (FileNotStoredLocallyException $e) { $url = $e->file->getUrl(false); $this->out->element('a', ['href'=>$url, 'rel'=>'external'], $url); diff --git a/lib/imagefile.php b/lib/imagefile.php index 1bbadd0b24..85747072a5 100644 --- a/lib/imagefile.php +++ b/lib/imagefile.php @@ -535,10 +535,8 @@ class ImageFile extends MediaFile throw new ServerException('No File object attached to this ImageFile object.'); } - // File not stored locally, can't generate a thumbnail - if (empty($this->fileRecord->filename)) { - throw new FileNotStoredLocallyException($this->fileRecord); - } + // Throws FileNotFoundException or FileNotStoredLocallyException + $this->filepath = $this->fileRecord->getFileOrThumbnailPath(); if ($width === null) { $width = common_config('thumbnail', 'width'); @@ -575,7 +573,7 @@ class ImageFile extends MediaFile return $thumb; } - $filename = $this->fileRecord->filehash ?: $this->filename; // Remote files don't have $this->filehash + $filename = basename($this->filepath); $extension = File::guessMimeExtension($this->mimetype); $outname = "thumb-{$this->fileRecord->getID()}-{$width}x{$height}-{$filename}." . $extension; $outpath = File_thumbnail::path($outname); diff --git a/plugins/Oembed/OembedPlugin.php b/plugins/Oembed/OembedPlugin.php index 93fafe361d..707fe1de8f 100644 --- a/plugins/Oembed/OembedPlugin.php +++ b/plugins/Oembed/OembedPlugin.php @@ -411,7 +411,7 @@ class OembedPlugin extends Plugin * states where it isn't oEmbed data, so it doesn't mess up the event handle * for other things hooked into it), or the exception if it fails. */ - public function onCreateFileImageThumbnailSource(File $file, &$imgPath) + public function onCreateFileImageThumbnailSource(File $file, &$imgPath, $media) { // If we are on a private node, we won't do any remote calls (just as a precaution until // we can configure this from config.php for the private nodes)