From 2bdb5404c7e909f6aa7d10a951f516d3e7265e96 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 May 2015 10:50:02 -0700 Subject: [PATCH] Implement new profile transform with amazing "error handling" feature Summary: Ref T7707. Ref T4406. Ref T2479. This implements the profile-style (fixed width and height) transforms in a modern way. - Added a "regnerate" feature to the support UI to make testing easier and surface errors. - Laboriously check errors from everything. - Fix the profile thumbnailing so it crops properly instead of leaving margins. - Also defuses the "gigantic white PNG" attack. This doesn't handle the imagemagick case (for animated GIFs) yet. Test Plan: - Uploaded a variety of wide/narrow/small/large files and converted them into sensible profile pictures. - Tried to thumbnail some text files. - Set the pixel-size and file-size limits artificially small and hit them. - Used "regenerate" a bunch while testing the rest of this stuff. - Verified that non-regenerate flows still produce a default/placeholder image. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4406, T2479, T7707 Differential Revision: https://secure.phabricator.com/D12811 --- .../PhabricatorFileTransformController.php | 34 ++- ...PhabricatorFileTransformListController.php | 11 +- .../PhabricatorFileImageTransform.php | 284 ++++++++++++++++++ .../PhabricatorFileThumbnailTransform.php | 48 ++- 4 files changed, 354 insertions(+), 23 deletions(-) diff --git a/src/applications/files/controller/PhabricatorFileTransformController.php b/src/applications/files/controller/PhabricatorFileTransformController.php index a7fcbc3c0a..dd7c355061 100644 --- a/src/applications/files/controller/PhabricatorFileTransformController.php +++ b/src/applications/files/controller/PhabricatorFileTransformController.php @@ -13,6 +13,8 @@ final class PhabricatorFileTransformController // NOTE: This is a public/CDN endpoint, and permission to see files is // controlled by knowing the secret key, not by authentication. + $is_regenerate = $request->getBool('regenerate'); + $source_phid = $request->getURIData('phid'); $file = id(new PhabricatorFileQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) @@ -35,7 +37,11 @@ final class PhabricatorFileTransformController $transform); if ($xform) { - return $this->buildTransformedFileResponse($xform); + if ($is_regenerate) { + $this->destroyTransform($xform); + } else { + return $this->buildTransformedFileResponse($xform); + } } $type = $file->getMimeType(); @@ -57,10 +63,12 @@ final class PhabricatorFileTransformController try { $xformed_file = $xforms[$transform]->applyTransform($file); } catch (Exception $ex) { - // TODO: Provide a diagnostic mode to surface these to the viewer. - // In normal transform mode, we ignore failures and generate a - // default transform instead. + // default transform below. If we're explicitly regenerating the + // thumbnail, rethrow the exception. + if ($is_regenerate) { + throw $ex; + } } } @@ -165,4 +173,22 @@ final class PhabricatorFileTransformController return $xformer->executeThumbTransform($file, $x, $y); } + private function destroyTransform(PhabricatorTransformedFile $xform) { + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($xform->getTransformedPHID())) + ->executeOne(); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + if (!$file) { + $xform->delete(); + } else { + $engine = new PhabricatorDestructionEngine(); + $engine->destroyObject($file); + } + + unset($unguarded); + } + } diff --git a/src/applications/files/controller/PhabricatorFileTransformListController.php b/src/applications/files/controller/PhabricatorFileTransformListController.php index ace9abff4c..6ef1818962 100644 --- a/src/applications/files/controller/PhabricatorFileTransformListController.php +++ b/src/applications/files/controller/PhabricatorFileTransformListController.php @@ -58,12 +58,13 @@ final class PhabricatorFileTransformListController if ($xform->canApplyTransform($file)) { $can_apply = pht('Yes'); + $view_href = $file->getURIForTransform($xform); - if ($dst_phid) { - $view_text = pht('View Transform'); - } else { - $view_text = pht('Generate Transform'); - } + $view_href = new PhutilURI($view_href); + $view_href->setQueryParam('regenerate', 'true'); + + $view_text = pht('Regenerate'); + $view_link = phutil_tag( 'a', array( diff --git a/src/applications/files/transform/PhabricatorFileImageTransform.php b/src/applications/files/transform/PhabricatorFileImageTransform.php index 498685f356..ae5cbad305 100644 --- a/src/applications/files/transform/PhabricatorFileImageTransform.php +++ b/src/applications/files/transform/PhabricatorFileImageTransform.php @@ -2,6 +2,12 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform { + private $file; + private $data; + private $image; + private $imageX; + private $imageY; + public function canApplyTransform(PhabricatorFile $file) { if (!$file->isViewableImage()) { return false; @@ -14,4 +20,282 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform { return true; } + protected function willTransformFile(PhabricatorFile $file) { + $this->file = $file; + $this->data = null; + $this->image = null; + $this->imageX = null; + $this->imageY = null; + } + + protected function applyCropAndScale( + $dst_w, $dst_h, + $src_x, $src_y, + $src_w, $src_h) { + + // Figure out the effective destination width, height, and offsets. We + // never want to scale images up, so if we're copying a very small source + // image we're just going to center it in the destination image. + $cpy_w = min($dst_w, $src_w); + $cpy_h = min($dst_h, $src_h); + $off_x = ($dst_w - $cpy_w) / 2; + $off_y = ($dst_h - $cpy_h) / 2; + + // TODO: Support imagemagick for animated GIFs. + + $src = $this->getImage(); + $dst = $this->newEmptyImage($dst_w, $dst_h); + + $trap = new PhutilErrorTrap(); + $ok = @imagecopyresampled( + $dst, + $src, + $off_x, $off_y, + $src_x, $src_y, + $cpy_w, $cpy_h, + $src_w, $src_h); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + + if ($ok === false) { + throw new Exception( + pht( + 'Failed to imagecopyresampled() image: %s', + $errors)); + } + + $data = PhabricatorImageTransformer::saveImageDataInAnyFormat( + $dst, + $this->file->getMimeType()); + + return $this->newFileFromData($data); + } + + + /** + * Create a new @{class:PhabricatorFile} from raw data. + * + * @param string Raw file data. + */ + protected function newFileFromData($data) { + $name = $this->getTransformKey().'-'.$this->file->getName(); + + return PhabricatorFile::newFromFileData( + $data, + array( + 'name' => $name, + 'canCDN' => true, + )); + } + + + /** + * Create a new image filled with transparent pixels. + * + * @param int Desired image width. + * @param int Desired image height. + * @return resource New image resource. + */ + protected function newEmptyImage($w, $h) { + $w = (int)$w; + $h = (int)$h; + + if (($w <= 0) || ($h <= 0)) { + throw new Exception( + pht('Can not create an image with nonpositive dimensions.')); + } + + $trap = new PhutilErrorTrap(); + $img = @imagecreatetruecolor($w, $h); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + if ($img === false) { + throw new Exception( + pht( + 'Unable to imagecreatetruecolor() a new empty image: %s', + $errors)); + } + + $trap = new PhutilErrorTrap(); + $ok = @imagesavealpha($img, true); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + if ($ok === false) { + throw new Exception( + pht( + 'Unable to imagesavealpha() a new empty image: %s', + $errors)); + } + + $trap = new PhutilErrorTrap(); + $color = @imagecolorallocatealpha($img, 255, 255, 255, 127); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + if ($color === false) { + throw new Exception( + pht( + 'Unable to imagecolorallocatealpha() a new empty image: %s', + $errors)); + } + + $trap = new PhutilErrorTrap(); + $ok = @imagefill($img, 0, 0, $color); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + if ($ok === false) { + throw new Exception( + pht( + 'Unable to imagefill() a new empty image: %s', + $errors)); + } + + return $img; + } + + + /** + * Get the pixel dimensions of the image being transformed. + * + * @return list Width and height of the image. + */ + protected function getImageDimensions() { + if ($this->imageX === null) { + $image = $this->getImage(); + + $trap = new PhutilErrorTrap(); + $x = @imagesx($image); + $y = @imagesy($image); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + + if (($x === false) || ($y === false) || ($x <= 0) || ($y <= 0)) { + throw new Exception( + pht( + 'Unable to determine image dimensions with '. + 'imagesx()/imagesy(): %s', + $errors)); + } + + $this->imageX = $x; + $this->imageY = $y; + } + + return array($this->imageX, $this->imageY); + } + + + /** + * Get the raw file data for the image being transformed. + * + * @return string Raw file data. + */ + protected function getData() { + if ($this->data !== null) { + return $this->data; + } + + $file = $this->file; + + $max_size = (1024 * 1024 * 4); + $img_size = $file->getByteSize(); + if ($img_size > $max_size) { + throw new Exception( + pht( + 'This image is too large to transform. The transform limit is %s '. + 'bytes, but the image size is %s bytes.', + new PhutilNumber($max_size), + new PhutilNumber($img_size))); + } + + $data = $file->loadFileData(); + $this->data = $data; + return $this->data; + } + + + /** + * Get the GD image resource for the image being transformed. + * + * @return resource GD image resource. + */ + protected function getImage() { + if ($this->image !== null) { + return $this->image; + } + + if (!function_exists('imagecreatefromstring')) { + throw new Exception( + pht( + 'Unable to transform image: the imagecreatefromstring() function '. + 'is not available. Install or enable the "gd" extension for PHP.')); + } + + $data = $this->getData(); + $data = (string)$data; + + // First, we're going to write the file to disk and use getimagesize() + // to determine its dimensions without actually loading the pixel data + // into memory. For very large images, we'll bail out. + + // In particular, this defuses a resource exhaustion attack where the + // attacker uploads a 40,000 x 40,000 pixel PNGs of solid white. These + // kinds of files compress extremely well, but require a huge amount + // of memory and CPU to process. + + $tmp = new TempFile(); + Filesystem::writeFile($tmp, $data); + $tmp_path = (string)$tmp; + + $trap = new PhutilErrorTrap(); + $info = @getimagesize($tmp_path); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + + unset($tmp); + + if ($info === false) { + throw new Exception( + pht( + 'Unable to get image information with getimagesize(): %s', + $errors)); + } + + list($width, $height) = $info; + if (($width <= 0) || ($height <= 0)) { + throw new Exception( + pht( + 'Unable to determine image width and height with getimagesize().')); + } + + $max_pixels = (4096 * 4096); + $img_pixels = ($width * $height); + + if ($img_pixels > $max_pixels) { + throw new Exception( + pht( + 'This image (with dimensions %spx x %spx) is too large to '. + 'transform. The image has %s pixels, but transforms are limited '. + 'to images with %s or fewer pixels.', + new PhutilNumber($width), + new PhutilNumber($height), + new PhutilNumber($img_pixels), + new PhutilNumber($max_pixels))); + } + + $trap = new PhutilErrorTrap(); + $image = @imagecreatefromstring($data); + $errors = $trap->getErrorsAsString(); + $trap->destroy(); + + if ($image === false) { + throw new Exception( + pht( + 'Unable to load image data with imagecreatefromstring(): %s', + $errors)); + } + + $this->image = $image; + return $this->image; + } + } diff --git a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php index 75c16e2ddf..2921869c2c 100644 --- a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php +++ b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php @@ -59,16 +59,41 @@ final class PhabricatorFileThumbnailTransform } public function applyTransform(PhabricatorFile $file) { - $x = $this->dstX; - $y = $this->dstY; - $xformer = new PhabricatorImageTransformer(); - - if ($y === null) { - return $xformer->executePreviewTransform($file, $x); - } else { - return $xformer->executeThumbTransform($file, $x, $y); + if ($this->dstY === null) { + return $xformer->executePreviewTransform($file, $this->dstX); } + + $this->willTransformFile($file); + + list($src_x, $src_y) = $this->getImageDimensions(); + $dst_x = $this->dstX; + $dst_y = $this->dstY; + + // Figure out how much we'd have to scale the image down along each + // dimension to get the entire thing to fit. + $scale_x = min(($dst_x / $src_x), 1); + $scale_y = min(($dst_y / $src_y), 1); + + if ($scale_x > $scale_y) { + // This image is relatively tall and narrow. We're going to crop off the + // top and bottom. + $copy_x = $src_x; + $copy_y = min($src_y, $dst_y / $scale_x); + } else { + // This image is relatively short and wide. We're going to crop off the + // left and right. + $copy_x = min($src_x, $dst_x / $scale_y); + $copy_y = $src_y; + } + + return $this->applyCropAndScale( + $dst_x, + $dst_y, + ($src_x - $copy_x) / 2, + ($src_y - $copy_y) / 2, + $copy_x, + $copy_y); } public function getDefaultTransform(PhabricatorFile $file) { @@ -76,15 +101,10 @@ final class PhabricatorFileThumbnailTransform $y = (int)$this->dstY; $name = 'image-'.$x.'x'.nonempty($y, $x).'.png'; - $params = array( - 'name' => $name, - 'canCDN' => true, - ); - $root = dirname(phutil_get_library_root('phabricator')); $data = Filesystem::readFile($root.'/resources/builtin/'.$name); - return PhabricatorFile::newFromFileData($data, $params); + return $this->newFileFromData($data); } }