From 5eb4a7d71190a04508b250ca5980168c558ef976 Mon Sep 17 00:00:00 2001 From: Miguel Dantas Date: Tue, 11 Jun 2019 02:42:33 +0100 Subject: [PATCH] [MEDIA] File downloader now in PHP, added proper name in the UI and changed the format for new attachment file names The file downloader was changed from a simple redirect to the file to one implemented in PHP, which should make it safer, by making it possible disallow direct access to the file, to prevent executing of atttachments The filename has a new format: bin2hex("{$original_name}")."-{$filehash}" This format should be respected. Notice the dash, which is important to distinguish it from the previous format, which was "{$hash}.{$ext}" This change was made to both make the experience more user friendly, by providing a readable name for files, as opposed to it's hash. This name is taken from the upload filename, but, clearly, as this wasn't done before, it's impossible to have a proper name for older files, so those are displayed as "untitled.{$ext}". This new name is displayed in the UI, instead of the previous name. --- README.md | 2 +- actions/attachment.php | 3 +- actions/attachment_download.php | 30 ++++++++++- classes/File.php | 2 +- lib/framework.php | 4 +- lib/mediafile.php | 89 ++++++++++++++++++++++++++------- 6 files changed, 106 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index b6d1dd89d1..0746163e87 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# GNU social 1.20.x +# GNU social 1.21.x (c) 2010-2019 Free Software Foundation, Inc This is the README file for GNU social, the free diff --git a/actions/attachment.php b/actions/attachment.php index 3f2ae5c1ce..fb85a50d07 100644 --- a/actions/attachment.php +++ b/actions/attachment.php @@ -41,9 +41,8 @@ if (!defined('GNUSOCIAL')) { exit(1); } class AttachmentAction extends ManagedAction { /** - * Attachment object to show + * Attachment File object to show */ - var $attachment = null; /** diff --git a/actions/attachment_download.php b/actions/attachment_download.php index 6792c45993..e9a1667a5a 100644 --- a/actions/attachment_download.php +++ b/actions/attachment_download.php @@ -15,6 +15,34 @@ class Attachment_downloadAction extends AttachmentAction { public function showPage() { - common_redirect($this->attachment->getUrl(), 302); + // Checks file exists or throws FileNotStoredLocallyException + $filepath = $this->attachment->getPath(); + + $filename = MediaFile::getDisplayName($this->attachment); + + // Disable errors, to not mess with the file contents (suppress errors in case access to this + // function is blocked, like in some shared hosts). Automatically reset at the end of the + // script execution, and we don't want to have any more errors until then, so don't reset it + @ini_set('display_errors', 0); + + header("Content-Description: File Transfer"); + header("Content-Type: {$this->attachment->mimetype}"); + header("Content-Disposition: attachment; filename=\"{$filename}\""); + header('Expires: 0'); + header('Content-Transfer-Encoding: binary'); // FIXME? Can this be different? + $filesize = $this->attachment->size; + // 'if available', it says, so ensure we have it + if (empty($filesize)) { + $filesize = filesize($this->attachment->filename); + } + header("Content-Length: {$filesize}"); + // header('Cache-Control: private, no-transform, no-store, must-revalidate'); + + $ret = @readfile($filepath); + + if ($ret === false || $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."); + } } } diff --git a/classes/File.php b/classes/File.php index 61e611a124..1af73d28be 100644 --- a/classes/File.php +++ b/classes/File.php @@ -746,7 +746,7 @@ class File extends Managed_DataObject public function getTitle() { - $title = $this->title ?: $this->filename; + $title = $this->title ?: MediaFile::getDisplayName($this); return $title ?: null; } diff --git a/lib/framework.php b/lib/framework.php index 0b34091836..355894e993 100644 --- a/lib/framework.php +++ b/lib/framework.php @@ -32,8 +32,8 @@ defined('GNUSOCIAL') || die(); define('GNUSOCIAL_ENGINE', 'GNU social'); define('GNUSOCIAL_ENGINE_URL', 'https://www.gnu.org/software/social/'); -define('GNUSOCIAL_BASE_VERSION', '1.20.9'); -define('GNUSOCIAL_LIFECYCLE', 'release'); // 'dev', 'alpha[0-9]+', 'beta[0-9]+', 'rc[0-9]+', 'release' +define('GNUSOCIAL_BASE_VERSION', '1.21.0'); +define('GNUSOCIAL_LIFECYCLE', 'dev'); // 'dev', 'alpha[0-9]+', 'beta[0-9]+', 'rc[0-9]+', 'release' define('GNUSOCIAL_VERSION', GNUSOCIAL_BASE_VERSION . '-' . GNUSOCIAL_LIFECYCLE); diff --git a/lib/mediafile.php b/lib/mediafile.php index dd8f82720c..3445f4b8e5 100644 --- a/lib/mediafile.php +++ b/lib/mediafile.php @@ -28,10 +28,7 @@ * @link https://www.gnu.org/software/social/ */ -if (!defined('GNUSOCIAL')) { - exit(1); -} - +defined('GNUSOCIAL') || die(); /** * Class responsible for abstracting media files @@ -307,6 +304,11 @@ class MediaFile * * Tries to set the mimetype correctly, using the most secure method available and rejects the file otherwise. * In case the upload is an image, this function returns an new ImageFile (which extends MediaFile) + * The filename has a new format: + * bin2hex("{$original_name}.{$ext}")."-{$filehash}" + * This format should be respected. Notice the dash, which is important to distinguish it from the previous + * format ("{$hash}.{$ext}") + * * @param string $param * @param Profile|null $scoped * @return ImageFile|MediaFile @@ -378,11 +380,28 @@ class MediaFile $mimetype = self::getUploadedMimeType($_FILES[$param]['tmp_name'], $_FILES[$param]['name']); $media = common_get_mime_media($mimetype); - $basename = basename($_FILES[$param]['name']); - $filename = $filehash . '.' . File::guessMimeExtension($mimetype, $basename); - $filepath = File::path($filename); - $result = move_uploaded_file($_FILES[$param]['tmp_name'], $filepath); + $basename = preg_replace("/\..+$/i", '', basename($_FILES[$param]['name'])); + $ext = File::guessMimeExtension($mimetype, $basename); + if ($media === 'image') { + // Use -1 for the id to avoid adding this temporary file to the DB + $img = new ImageFile(-1, $_FILES[$param]['tmp_name']); + // Validate the image by reencoding it. Additionally normalizes old formats to PNG, + // keeping JPEG and GIF untouched + $outpath = $img->resizeTo($img->filepath); + $ext = image_type_to_extension($img->preferredType(), false); + } + + // New file name format + $original_filename = bin2hex("{$basename}.{$ext}"); + $filename = "{$original_filename}-{$filehash}"; + $filepath = File::path($filename); + + if ($media === 'image') { + $result = rename($outpath, $filepath); + } else { + $result = move_uploaded_file($_FILES[$param]['tmp_name'], $filepath); + } if (!$result) { // TRANS: Client exception thrown when a file upload operation fails because the file could // TRANS: not be moved from the temporary folder to the permanent file location. @@ -391,15 +410,6 @@ class MediaFile } if ($media === 'image') { - // Use -1 for the id to avoid adding this temporary file to the DB - $img = new ImageFile(-1, $filepath); - // Validate the image by reencoding it. Additionally normalizes old formats to PNG, - // keeping JPEG and GIF untouched - $outpath = $img->resizeTo($img->filepath); - $ext = image_type_to_extension($img->preferredType()); - $filename = $filehash . $ext; - $filepath = File::path($filename); - $result = rename($outpath, $filepath); return new ImageFile(null, $filepath); } } @@ -612,4 +622,49 @@ class MediaFile } throw new ClientException($hint); } + + /** + * Title for a file, to display in the interface (if there's no better title) and + * for download filenames + * @param $file File object + * @returns string + */ + public static function getDisplayName(File $file) : string { + // 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 + // of code (could be a file with utf8 chars in the name) + $user_error_mesg = "Invalid file name ({$file->filename})."; + $log_error_msg = "Invalid file name for File with id={$file->id} " . + "({$file->filename}). Some plugin probably did something wrong."; + + if ($ret === false) { + common_log(LOG_ERR, $log_error_msg); + throw new ServerException($user_error_msg); + } elseif ($ret === 1) { + $filename = hex2bin($matches[1]); + } else { + // The old file name format was "{hash}.{ext}" + // This estracts the extension + $ret = preg_match('/^[^\.]+\.(.+)$/', $file->filename, $matches); + if ($ret !== 1) { + common_log(LOG_ERR, $log_error_msg); + throw new ServerException($user_error_msg); + } + $ext = $matches[1]; + // Previously, there was a blacklisted extension array, which could have an alternative + // extension, such as phps, to replace php. We want to turn it back + $blacklist = common_config('attachments', 'extblacklist'); + if (is_array($blacklist)) { + foreach ($blacklist as $upload_ext => $safe_ext) { + if ($ext === $safe_ext) { + $ext = $upload_ext; + break; + } + } + } + $filename = "untitled.{$ext}"; + } + return $filename; + } }