Merge pull request #14283 from jeabakker/fixes

fix(icons): improved handling of invalid cropping coordinates

engine/classes/Elgg/EntityIconService.php

@@ -4,6 +4,7 @@
 
 use Elgg\Database\EntityTable;
 use Elgg\Exceptions\InvalidParameterException;
+use Elgg\Exceptions\LogicException;
 use Elgg\Filesystem\MimeTypeService;
 use Elgg\Http\Request as HttpRequest;
 use Elgg\Traits\Loggable;
@@ -249,6 +250,22 @@ public function saveIcon(\ElggEntity $entity, \ElggFile $file, $type = 'icon', a
 				$this->deleteIcon($entity, $type);
 				return false;
 			}
+
+			// validate cropping coords to prevent out-of-bounds issues
+			try {
+				$sizes = $this->getSizes($entity->getType(), $entity->getSubtype(), $type);
+				$coords = array_merge($sizes['master'], $coords);
+
+				$icon = $this->getIcon($entity, 'master', $type, false);
+
+				$this->images->normalizeResizeParameters($icon->getFilenameOnFilestore(), $coords);
+			} catch (LogicException $e) {
+				// cropping coords are wrong, reset to 0
+				$x1 = 0;
+				$x2 = 0;
+				$y1 = 0;
+				$y2 = 0;
+			}
 		}
 
 		if ($type == 'icon') {
@@ -700,7 +717,7 @@ protected function detectCroppingCoordinates(string $input_name) {
 		];
 
 		$auto_coords = array_filter($auto_coords, function($value) {
-			return !elgg_is_empty($value) && is_numeric($value);
+			return !elgg_is_empty($value) && is_numeric($value) && (int) $value >= 0;
 		});
 
 		if (count($auto_coords) !== 4) {
@@ -712,6 +729,11 @@ protected function detectCroppingCoordinates(string $input_name) {
 			$value = (int) $value;
 		});
 
+		// make sure coords make sense x2 > x1 && y2 > y1
+		if ($auto_coords['x2'] <= $auto_coords['x1'] || $auto_coords['y2'] <= $auto_coords['y1']) {
+			return false;
+		}
+
 		return $auto_coords;
 	}
 

engine/classes/Elgg/ImageService.php

@@ -98,13 +98,10 @@ public function resize($source, $destination = null, array $params = []) {
 		}
 
 		try {
+			$resize_params = $this->normalizeResizeParameters($source, $params);
+
 			$image = $this->imagine->open($source);
-
-			$width = $image->getSize()->getWidth();
-			$height = $image->getSize()->getHeight();
-
-			$resize_params = $this->normalizeResizeParameters($width, $height, $params);
-
+
 			$max_width = (int) elgg_extract('w', $resize_params);
 			$max_height = (int) elgg_extract('h', $resize_params);
 
@@ -178,19 +175,23 @@ public function fixOrientation($filename) {
 	/**
 	 * Calculate the parameters for resizing an image
 	 *
-	 * @param int   $width  Natural width of the image
-	 * @param int   $height Natural height of the image
-	 * @param array $params Resize parameters
-	 *                      - 'w' maximum width of the resized image
-	 *                      - 'h' maximum height of the resized image
-	 *                      - 'upscale' allow upscaling
-	 *                      - 'square' constrain to a square
-	 *                      - 'x1', 'y1', 'x2', 'y2' cropping coordinates
+	 * @param string $source The source location of the image to validate the parameters for
+	 * @param array  $params Resize parameters
+	 *                       - 'w' maximum width of the resized image
+	 *                       - 'h' maximum height of the resized image
+	 *                       - 'upscale' allow upscaling
+	 *                       - 'square' constrain to a square
+	 *                       - 'x1', 'y1', 'x2', 'y2' cropping coordinates
 	 *
 	 * @return array
 	 * @throws LogicException
 	 */
-	public function normalizeResizeParameters($width, $height, array $params = []) {
+	public function normalizeResizeParameters(string $source, array $params = []) {
+
+		$image = $this->imagine->open($source);
+
+		$width = $image->getSize()->getWidth();
+		$height = $image->getSize()->getHeight();
 
 		$max_width = (int) elgg_extract('w', $params, 100, false);
 		$max_height = (int) elgg_extract('h', $params, 100, false);

engine/tests/phpunit/integration/Elgg/EntityIconServiceIntegrationTest.php

@@ -0,0 +1,69 @@
+<?php
+
+namespace Elgg;
+
+class EntityIconServiceIntegrationTest extends IntegrationTestCase {
+
+	public function up() {
+		$this->createApplication([
+			'isolate' => true,
+		]);
+	}
+
+	/**
+	 * @dataProvider invalidCoordinatesProvider
+	 */
+	public function testInvalidDetectCroppingCoordinates($input_name, $params) {
+		$request = $this->prepareHttpRequest('', 'POST', $params);
+
+		_elgg_services()->set('request', $request);
+
+		$service = _elgg_services()->iconService;
+		$inspector = new \ReflectionClass($service);
+		$method = $inspector->getMethod('detectCroppingCoordinates');
+		$method->setAccessible(true);
+
+		$this->assertFalse($method->invoke($service, $input_name));
+	}
+
+	public function invalidCoordinatesProvider() {
+		return [
+			['foo', []], // no input
+			['foo', ['x1' => '1', 'x2' => '100', 'y1' => '10']], // missing one coordinate (using fallback coords)
+			['foo', ['foo_x1' => '1', 'foo_x2' => '100', 'foo_y1' => '10']], // missing one coordinate
+			['foo', ['x1' => '1', 'x2' => '100', 'y1' => '10', 'y2' => 'string']], // one invalid coordinate (using fallback coords)
+			['foo', ['foo_x1' => '1', 'foo_x2' => '100', 'foo_y1' => '10', 'foo_y2' => 'string']], // one invalid coordinate
+			['foo', ['x1' => '100', 'x2' => '1', 'y1' => '10', 'y2' => '100']], // x2 < x1 (using fallback coords)
+			['foo', ['foo_x1' => '100', 'foo_x2' => '1', 'foo_y1' => '10', 'foo_y2' => '100']], // x2 < x1
+			['foo', ['x1' => '10', 'x2' => '100', 'y1' => '100', 'y2' => '10']], // y2 < y1 (using fallback coords)
+			['foo', ['foo_x1' => '10', 'foo_x2' => '100', 'foo_y1' => '100', 'foo_y2' => '10']], // y2 < y1
+		];
+	}
+
+	/**
+	 * @dataProvider detectCroppingCoordinatesDataProvider
+	 */
+	public function testDetectCroppingCoordinates($input_name, $params, $expected_value) {
+		$request = $this->prepareHttpRequest('', 'POST', $params);
+
+		_elgg_services()->set('request', $request);
+
+		$reflector = new \ReflectionClass(_elgg_services()->iconService);
+		$method = $reflector->getMethod('detectCroppingCoordinates');
+		$method->setAccessible(true);
+
+		$result = $method->invoke(_elgg_services()->iconService, $input_name);
+
+		$this->assertEquals($expected_value, $result);
+	}
+
+	public function detectCroppingCoordinatesDataProvider() {
+		return [
+			['icon', ['x1' => '1', 'x2' => '100', 'y1' => '10', 'y2' => '100'], ['x1' => 1, 'x2' => 100, 'y1' => 10, 'y2' => 100]],
+			['icon', ['x1' => '1.0', 'x2' => '100.2', 'y1' => '10', 'y2' => '100'], ['x1' => 1, 'x2' => 100, 'y1' => 10, 'y2' => 100]], // handle floats
+			['icon', ['icon_x1' => '1', 'icon_x2' => '100', 'icon_y1' => '10', 'icon_y2' => '100'], ['x1' => 1, 'x2' => 100, 'y1' => 10, 'y2' => 100]],
+			['icon', ['x1' => '1', 'x2' => '100', 'y1' => '10', 'y2' => '100'], ['x1' => 1, 'x2' => 100, 'y1' => 10, 'y2' => 100]], // test BC input is used
+			['icon', ['icon_x1' => '1', 'icon_x2' => '100', 'icon_y1' => '10', 'icon_y2' => '100', 'x1' => '10', 'x2' => '1000', 'y1' => '100', 'y2' => '1000'], ['x1' => 1, 'x2' => 100, 'y1' => 10, 'y2' => 100]], // test BC input isn't used
+		];
+	}
+}