From 47b14c9bdecf59def4eab628f431357eab14f0db Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 May 2015 17:32:06 -0700 Subject: [PATCH] Convert inline profile image transforms to new transformations Summary: Ref T7707. Fixes T7879. Fixes T4406. When creating profile images: - Use the new transforms; - mark them as "profile" images so they're forced to the most-open policies. Test Plan: - Set restrictive default file policies. - Changed profile picture, project pictures, etc. Verified they were visible to logged-out users. - Registered via OAuth. - Updated a Conpherence thread image. - Browsed around looking for profile images, fixed sizing on everything I could find. Reviewers: chad, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7879, T7707, T4406 Differential Revision: https://secure.phabricator.com/D12821 --- .../PhabricatorAuthRegisterController.php | 14 +- .../files/PhabricatorImageTransformer.php | 167 +----------------- .../PhabricatorFileComposeController.php | 7 +- .../PhabricatorFileInfoController.php | 6 + .../files/storage/PhabricatorFile.php | 18 ++ .../PhabricatorFileImageTransform.php | 6 +- .../PhabricatorFileThumbnailTransform.php | 10 ++ .../transform/PhabricatorFileTransform.php | 12 ++ ...bricatorPeopleProfilePictureController.php | 9 +- ...habricatorProjectEditPictureController.php | 9 +- webroot/rsrc/css/phui/phui-header-view.css | 1 + webroot/rsrc/css/phui/phui-timeline-view.css | 1 + 12 files changed, 67 insertions(+), 193 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php index 6c40375227..d27a644480 100644 --- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php +++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php @@ -604,17 +604,9 @@ final class PhabricatorAuthRegisterController return null; } - try { - $xformer = new PhabricatorImageTransformer(); - return $xformer->executeProfileTransform( - $file, - $width = 50, - $min_height = 50, - $max_height = 50); - } catch (Exception $ex) { - phlog($ex); - return null; - } + $xform = PhabricatorFileTransform::getTransformByKey( + PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); + return $xform->executeTransform($file); } protected function renderError($message) { diff --git a/src/applications/files/PhabricatorImageTransformer.php b/src/applications/files/PhabricatorImageTransformer.php index 34cdf67023..829f5d9f2d 100644 --- a/src/applications/files/PhabricatorImageTransformer.php +++ b/src/applications/files/PhabricatorImageTransformer.php @@ -20,22 +20,6 @@ final class PhabricatorImageTransformer { )); } - public function executeProfileTransform( - PhabricatorFile $file, - $x, - $min_y, - $max_y) { - - $image = $this->crudelyCropTo($file, $x, $min_y, $max_y); - - return PhabricatorFile::newFromFileData( - $image, - array( - 'name' => 'profile-'.$file->getName(), - 'canCDN' => true, - )); - } - public function executeConpherenceTransform( PhabricatorFile $file, $top, @@ -54,38 +38,11 @@ final class PhabricatorImageTransformer { $image, array( 'name' => 'conpherence-'.$file->getName(), + 'profile' => true, 'canCDN' => true, )); } - private function crudelyCropTo(PhabricatorFile $file, $x, $min_y, $max_y) { - $data = $file->loadFileData(); - $img = imagecreatefromstring($data); - $sx = imagesx($img); - $sy = imagesy($img); - - $scaled_y = ($x / $sx) * $sy; - if ($scaled_y > $max_y) { - // This image is very tall and thin. - $scaled_y = $max_y; - } else if ($scaled_y < $min_y) { - // This image is very short and wide. - $scaled_y = $min_y; - } - - $cropped = $this->applyScaleWithImagemagick($file, $x, $scaled_y); - if ($cropped != null) { - return $cropped; - } - - $img = $this->applyScaleTo( - $file, - $x, - $scaled_y); - - return self::saveImageDataInAnyFormat($img, $file->getMimeType()); - } - private function crasslyCropTo(PhabricatorFile $file, $top, $left, $w, $h) { $data = $file->loadFileData(); $src = imagecreatefromstring($data); @@ -116,34 +73,6 @@ final class PhabricatorImageTransformer { return $dst; } - private function applyScaleTo(PhabricatorFile $file, $dx, $dy) { - $data = $file->loadFileData(); - $src = imagecreatefromstring($data); - - $x = imagesx($src); - $y = imagesy($src); - - $scale = min(($dx / $x), ($dy / $y), 1); - - $sdx = $scale * $x; - $sdy = $scale * $y; - - $dst = $this->getBlankDestinationFile($dx, $dy); - imagesavealpha($dst, true); - imagefill($dst, 0, 0, imagecolorallocatealpha($dst, 255, 255, 255, 127)); - - imagecopyresampled( - $dst, - $src, - ($dx - $sdx) / 2, ($dy - $sdy) / 2, - 0, 0, - $sdx, $sdy, - $x, $y); - - return $dst; - - } - public static function getScaleForCrop( PhabricatorFile $file, $des_width, @@ -302,49 +231,6 @@ final class PhabricatorImageTransformer { ); } - private function applyScaleWithImagemagick(PhabricatorFile $file, $dx, $dy) { - $img_type = $file->getMimeType(); - $imagemagick = PhabricatorEnv::getEnvConfig('files.enable-imagemagick'); - - if ($img_type != 'image/gif' || $imagemagick == false) { - return null; - } - - $data = $file->loadFileData(); - $src = imagecreatefromstring($data); - - $x = imagesx($src); - $y = imagesy($src); - - if (self::isEnormousGIF($x, $y)) { - return null; - } - - $scale = min(($dx / $x), ($dy / $y), 1); - - $sdx = $scale * $x; - $sdy = $scale * $y; - - $input = new TempFile(); - Filesystem::writeFile($input, $data); - - $resized = new TempFile(); - - $future = new ExecFuture( - 'convert %s -coalesce -resize %sX%s%s %s', - $input, - $sdx, - $sdy, - '!', - $resized); - - // Don't spend more than 10 seconds resizing; just fail if it takes longer - // than that. - $future->setTimeout(10)->resolvex(); - - return Filesystem::readFile($resized); - } - private function applyMemeWithImagemagick( $input, $above, @@ -382,57 +268,6 @@ final class PhabricatorImageTransformer { return Filesystem::readFile($output); } -/* -( Detecting Enormous Files )------------------------------------------- */ - - - /** - * Determine if an image is enormous (too large to transform). - * - * Attackers can perform a denial of service attack by uploading highly - * compressible images with enormous dimensions but a very small filesize. - * Transforming them (e.g., into thumbnails) may consume huge quantities of - * memory and CPU relative to the resources required to transmit the file. - * - * In general, we respond to these images by declining to transform them, and - * using a default thumbnail instead. - * - * @param int Width of the image, in pixels. - * @param int Height of the image, in pixels. - * @return bool True if this image is enormous (too large to transform). - * @task enormous - */ - public static function isEnormousImage($x, $y) { - // This is just a sanity check, but if we don't have valid dimensions we - // shouldn't be trying to transform the file. - if (($x <= 0) || ($y <= 0)) { - return true; - } - - return ($x * $y) > (4096 * 4096); - } - - - /** - * Determine if a GIF is enormous (too large to transform). - * - * For discussion, see @{method:isEnormousImage}. We need to be more - * careful about GIFs, because they can also have a large number of frames - * despite having a very small filesize. We're more conservative about - * calling GIFs enormous than about calling images in general enormous. - * - * @param int Width of the GIF, in pixels. - * @param int Height of the GIF, in pixels. - * @return bool True if this image is enormous (too large to transform). - * @task enormous - */ - public static function isEnormousGIF($x, $y) { - if (self::isEnormousImage($x, $y)) { - return true; - } - - return ($x * $y) > (800 * 800); - } - /* -( Saving Image Data )-------------------------------------------------- */ diff --git a/src/applications/files/controller/PhabricatorFileComposeController.php b/src/applications/files/controller/PhabricatorFileComposeController.php index 6903cff5d3..bb95cb67eb 100644 --- a/src/applications/files/controller/PhabricatorFileComposeController.php +++ b/src/applications/files/controller/PhabricatorFileComposeController.php @@ -58,7 +58,7 @@ final class PhabricatorFileComposeController } $root = dirname(phutil_get_library_root('phabricator')); - $icon_file = $root.'/resources/sprite/projects_1x/'.$icon.'.png'; + $icon_file = $root.'/resources/sprite/projects_2x/'.$icon.'.png'; $icon_data = Filesystem::readFile($icon_file); @@ -68,6 +68,7 @@ final class PhabricatorFileComposeController $data, array( 'name' => 'project.png', + 'profile' => true, 'canCDN' => true, )); @@ -325,10 +326,10 @@ final class PhabricatorFileComposeController $color_string = idx($map, $color, '#ff00ff'); $color_const = hexdec(trim($color_string, '#')); - $canvas = imagecreatetruecolor(50, 50); + $canvas = imagecreatetruecolor(100, 100); imagefill($canvas, 0, 0, $color_const); - imagecopy($canvas, $icon_img, 0, 0, 0, 0, 50, 50); + imagecopy($canvas, $icon_img, 0, 0, 0, 0, 100, 100); return PhabricatorImageTransformer::saveImageDataInAnyFormat( $canvas, diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index fdc5a7773c..0e3d041eac 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -247,6 +247,12 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $finfo->addProperty(pht('Builtin'), $builtin_string); + $is_profile = $file->getIsProfileImage() + ? pht('Yes') + : pht('No'); + + $finfo->addProperty(pht('Profile'), $is_profile); + $storage_properties = new PHUIPropertyListView(); $box->addPropertyList($storage_properties, pht('Storage')); diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 3de1bdc9f6..34a2bb5df5 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -34,6 +34,7 @@ final class PhabricatorFile extends PhabricatorFileDAO const METADATA_CAN_CDN = 'canCDN'; const METADATA_BUILTIN = 'builtin'; const METADATA_PARTIAL = 'partial'; + const METADATA_PROFILE = 'profile'; protected $name; protected $mimeType; @@ -1112,6 +1113,15 @@ final class PhabricatorFile extends PhabricatorFileDAO return $this; } + public function getIsProfileImage() { + return idx($this->metadata, self::METADATA_PROFILE); + } + + public function setIsProfileImage($value) { + $this->metadata[self::METADATA_PROFILE] = $value; + return $this; + } + protected function generateOneTimeToken() { $key = Filesystem::readRandomCharacters(16); @@ -1213,6 +1223,11 @@ final class PhabricatorFile extends PhabricatorFileDAO $this->setBuiltinName($builtin); } + $profile = idx($params, 'profile'); + if ($profile) { + $this->setIsProfileImage(true); + } + $mime_type = idx($params, 'mime-type'); if ($mime_type) { $this->setMimeType($mime_type); @@ -1280,6 +1295,9 @@ final class PhabricatorFile extends PhabricatorFileDAO if ($this->isBuiltin()) { return PhabricatorPolicies::getMostOpenPolicy(); } + if ($this->getIsProfileImage()) { + return PhabricatorPolicies::getMostOpenPolicy(); + } return $this->getViewPolicy(); case PhabricatorPolicyCapability::CAN_EDIT: return PhabricatorPolicies::POLICY_NOONE; diff --git a/src/applications/files/transform/PhabricatorFileImageTransform.php b/src/applications/files/transform/PhabricatorFileImageTransform.php index b6e6844653..6c40e6bbfa 100644 --- a/src/applications/files/transform/PhabricatorFileImageTransform.php +++ b/src/applications/files/transform/PhabricatorFileImageTransform.php @@ -38,6 +38,10 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform { $this->imageY = null; } + protected function getFileProperties() { + return array(); + } + protected function applyCropAndScale( $dst_w, $dst_h, $src_x, $src_y, @@ -144,7 +148,7 @@ abstract class PhabricatorFileImageTransform extends PhabricatorFileTransform { array( 'name' => $name, 'canCDN' => true, - )); + ) + $this->getFileProperties()); } diff --git a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php index 328742f2e8..c7b579e836 100644 --- a/src/applications/files/transform/PhabricatorFileThumbnailTransform.php +++ b/src/applications/files/transform/PhabricatorFileThumbnailTransform.php @@ -43,6 +43,16 @@ final class PhabricatorFileThumbnailTransform return $this->key; } + protected function getFileProperties() { + $properties = array(); + switch ($this->key) { + case self::TRANSFORM_PROFILE: + $properties['profile'] = true; + break; + } + return $properties; + } + public function generateTransforms() { return array( id(new PhabricatorFileThumbnailTransform()) diff --git a/src/applications/files/transform/PhabricatorFileTransform.php b/src/applications/files/transform/PhabricatorFileTransform.php index 633a80887a..caaf46920d 100644 --- a/src/applications/files/transform/PhabricatorFileTransform.php +++ b/src/applications/files/transform/PhabricatorFileTransform.php @@ -15,6 +15,18 @@ abstract class PhabricatorFileTransform extends Phobject { return array($this); } + public function executeTransform(PhabricatorFile $file) { + if ($this->canApplyTransform($file)) { + try { + return $this->applyTransform($file); + } catch (Exception $ex) { + // Ignore. + } + } + + return $this->getDefaultTransform($file); + } + public static function getAllTransforms() { static $map; diff --git a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php index 231181614d..0f59e23286 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php @@ -70,12 +70,9 @@ final class PhabricatorPeopleProfilePictureController 'This server only supports these image formats: %s.', implode(', ', $supported_formats)); } else { - $xformer = new PhabricatorImageTransformer(); - $xformed = $xformer->executeProfileTransform( - $file, - $width = 50, - $min_height = 50, - $max_height = 50); + $xform = PhabricatorFileTransform::getTransformByKey( + PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); + $xformed = $xform->executeTransform($file); } } diff --git a/src/applications/project/controller/PhabricatorProjectEditPictureController.php b/src/applications/project/controller/PhabricatorProjectEditPictureController.php index 58e345930e..ca99159718 100644 --- a/src/applications/project/controller/PhabricatorProjectEditPictureController.php +++ b/src/applications/project/controller/PhabricatorProjectEditPictureController.php @@ -68,12 +68,9 @@ final class PhabricatorProjectEditPictureController 'This server only supports these image formats: %s.', implode(', ', $supported_formats)); } else { - $xformer = new PhabricatorImageTransformer(); - $xformed = $xformer->executeProfileTransform( - $file, - $width = 50, - $min_height = 50, - $max_height = 50); + $xform = PhabricatorFileTransform::getTransformByKey( + PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); + $xformed = $xform->executeTransform($file); } } diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index ef707191e5..5b48f722c6 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -90,6 +90,7 @@ body.device-phone .phui-header-view { .phui-header-image { display: inline-block; background-repeat: no-repeat; + background-size: 50px; border: 2px solid white; width: 50px; height: 50px; diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css index b82c9385a7..6e956037aa 100644 --- a/webroot/rsrc/css/phui/phui-timeline-view.css +++ b/webroot/rsrc/css/phui/phui-timeline-view.css @@ -92,6 +92,7 @@ .phui-timeline-image { background-repeat: no-repeat; + background-size: 50px; position: absolute; border-radius: 3px; }