From eaccd12562cdde2e2f34b7097f590755b4b3d883 Mon Sep 17 00:00:00 2001 From: Tim Kelty Date: Wed, 3 Jun 2026 19:19:15 -0400 Subject: [PATCH] Skip Cloud asset CDN transforms for action requests --- src/Module.php | 17 +++- src/controllers/AssetsController.php | 62 +++++++++--- src/fs/Fs.php | 117 +++++++++++++++++++++- tests/unit/AssetsControllerTest.php | 142 +++++++++++++++++++++++++++ tests/unit/ImageTransformTest.php | 62 +++++++++++- 5 files changed, 385 insertions(+), 15 deletions(-) diff --git a/src/Module.php b/src/Module.php index 024c59a..8e00e79 100644 --- a/src/Module.php +++ b/src/Module.php @@ -97,7 +97,7 @@ public function bootstrap($app): void Asset::class, Asset::EVENT_BEFORE_GENERATE_TRANSFORM, function(GenerateTransformEvent $event) { - if (!$event->transform || !$event->asset?->fs instanceof AssetsFs) { + if (!$this->shouldUseAssetCdnTransform($event)) { return; } @@ -119,6 +119,21 @@ function(GenerateTransformEvent $event) { } } + protected function shouldUseAssetCdnTransform(GenerateTransformEvent $event): bool + { + if (!$event->transform || !$event->asset?->fs instanceof AssetsFs) { + return false; + } + + if (!(Craft::$app instanceof WebApplication)) { + return true; + } + + // The image editor reads raw source pixels for save/crop math, so keep + // its preview in the same coordinate space as the original asset. + return Craft::$app->getRequest()->getActionSegments() !== ['assets', 'edit-image']; + } + public function getConfig(): Config { if (isset($this->_config)) { diff --git a/src/controllers/AssetsController.php b/src/controllers/AssetsController.php index 16fca1b..4cc640f 100644 --- a/src/controllers/AssetsController.php +++ b/src/controllers/AssetsController.php @@ -14,6 +14,7 @@ use craft\models\Volume; use craft\web\Controller; use DateTime; +use Throwable; use yii\base\Event; use yii\base\Exception; use yii\base\Model; @@ -94,9 +95,6 @@ public function actionCreateAsset(): Response $filename = $this->request->getRequiredBodyParam('filename'); $originalFilename = $this->request->getRequiredBodyParam('originalFilename'); $targetFilename = $this->request->getRequiredBodyParam('targetFilename'); - $size = $this->request->getBodyParam('size'); - $width = $this->request->getBodyParam('width'); - $height = $this->request->getBodyParam('height'); $elementsService = Craft::$app->getElements(); $lastModifiedMs = (int) $this->request->getBodyParam('lastModified'); $dateModified = $lastModifiedMs @@ -159,9 +157,6 @@ public function actionCreateAsset(): Response $asset->uploaderId = Craft::$app->getUser()->getId(); $asset->avoidFilenameConflicts = true; $asset->dateModified = $dateModified; - $asset->size = $size; - $asset->width = $width; - $asset->height = $height; // Setting newFolderId, so that extension validation on newLocation occurs $asset->newFolderId = $folder->id; @@ -172,6 +167,8 @@ public function actionCreateAsset(): Response // Handle special characters that have been encoded from the presigned URL $asset->folderPath = is_string($folder->path) ? Fs::urlEncodePathSegments($folder->path) : $asset->folderPath; + $this->setUploadedAssetMetadata($asset, $filename, $originalFilename); + if (!$selectionCondition) { $asset->newFilename = $targetFilename; } @@ -239,9 +236,6 @@ public function actionReplaceFile(): Response $sourceAssetId = $this->request->getBodyParam('sourceAssetId'); $filename = $this->request->getBodyParam('filename'); $targetFilename = $this->request->getBodyParam('targetFilename'); - $size = $this->request->getBodyParam('size'); - $width = $this->request->getBodyParam('width'); - $height = $this->request->getBodyParam('height'); $lastModifiedMs = (int) $this->request->getBodyParam('lastModified'); $dateModified = $lastModifiedMs ? DateTime::createFromFormat('U', (string) floor($lastModifiedMs / 1000)) @@ -272,9 +266,7 @@ public function actionReplaceFile(): Response // Handle the Element Action if ($assetToReplace !== null && $filename) { - $assetToReplace->width = $width; - $assetToReplace->height = $height; - $assetToReplace->size = $size; + $this->setUploadedAssetMetadata($assetToReplace, $filename, $targetFilename); $assetToReplace->dateModified = $dateModified; if (!$this->replaceAssetFile($assetToReplace, $filename, $targetFilename)) { throw new Exception('Unable to replace asset.'); @@ -396,4 +388,50 @@ private function volumeSubpath(Volume $volume): string { return method_exists($volume, 'getSubpath') ? $volume->getSubpath() : ''; } + + protected function setUploadedAssetMetadata(Asset $asset, string $filename, ?string $displayFilename = null): void + { + try { + $asset->size = $this->uploadedAssetSize($asset, $filename, $displayFilename); + [$width, $height] = $this->uploadedImageDimensions($asset, $filename); + $asset->width = $width; + $asset->height = $height; + } catch (Throwable $e) { + $this->deleteUploadedAsset($asset, $filename); + throw $e; + } + } + + protected function uploadedAssetSize(Asset $asset, string $filename, ?string $displayFilename = null): int + { + $size = $asset->getVolume()->getFileSize($asset->getPath($filename)); + $maxUploadSize = Craft::$app->getConfig()->getGeneral()->maxUploadFileSize; + + if ($maxUploadSize && $size > $maxUploadSize) { + throw new BadRequestHttpException(Craft::t('app', '“{filename}” is too large.', [ + 'filename' => $displayFilename ?: $filename, + ])); + } + + return $size; + } + + protected function uploadedImageDimensions(Asset $asset, string $filename): array + { + $fs = $asset->getVolume()->getFs(); + + // Null dimensions are safer than browser-oriented dimensions for EXIF + // images, but they can still prevent image-editor use until reindexed. + return $fs instanceof Fs + ? $fs->getImageDimensions($asset->getPath($filename)) ?? [null, null] + : [null, null]; + } + + private function deleteUploadedAsset(Asset $asset, string $filename): void + { + try { + $asset->getVolume()->deleteFile($asset->getPath($filename)); + } catch (Throwable) { + } + } } diff --git a/src/fs/Fs.php b/src/fs/Fs.php index 5bdbd44..6374358 100644 --- a/src/fs/Fs.php +++ b/src/fs/Fs.php @@ -10,12 +10,14 @@ use craft\cloud\Module; use craft\cloud\StaticCache; use craft\cloud\StaticCacheTag; +use craft\elements\Asset; use craft\errors\FsException; use craft\flysystem\base\FlysystemFs; use craft\fs\Local; use craft\helpers\App; use craft\helpers\Assets; use craft\helpers\DateTimeHelper; +use craft\helpers\Image; use DateTime; use DateTimeInterface; use Generator; @@ -37,6 +39,11 @@ */ abstract class Fs extends FlysystemFs { + // Most image headers are much smaller, but large EXIF/XMP/ICC blocks can + // push JPEG dimensions farther in. Stay bounded to avoid full downloads. + private const IMAGE_DIMENSION_INITIAL_BYTES = 1024 * 1024; + private const IMAGE_DIMENSION_MAX_BYTES = 8 * 1024 * 1024; + protected static bool $showUrlSetting = false; protected ?string $expires = null; protected ?Local $localFs = null; @@ -94,7 +101,7 @@ public function createUrl(string $path = ''): UriInterface { if ($this->useLocalFs) { return Modifier::wrap($this->getLocalFs()->getRootUrl() ?? '/') - ->appendSegment($this->createPath($path)) + ->appendPath($this->createPath($path)) ->unwrap(); } @@ -519,6 +526,114 @@ public function getFileStream(string $uriPath) return parent::getFileStream($uriPath); } + public function getImageDimensions(string $uriPath): ?array + { + if (Assets::getFileKindByExtension($uriPath) !== Asset::KIND_IMAGE) { + return null; + } + + for ($bytes = self::IMAGE_DIMENSION_INITIAL_BYTES; $bytes <= self::IMAGE_DIMENSION_MAX_BYTES; $bytes *= 2) { + $dimensions = $this->getImageDimensionsFromRange($uriPath, $bytes); + + if ($dimensions !== null) { + return $dimensions; + } + } + + return null; + } + + private function getImageDimensionsFromRange(string $uriPath, int $bytes): ?array + { + $stream = $this->getFileStreamRange($uriPath, 0, $bytes - 1); + + if ($stream === null) { + return null; + } + + try { + $imageSize = Image::imageSizeByStream($stream); + + if ($imageSize === false || !isset($imageSize[0], $imageSize[1])) { + return null; + } + + return [ + (int)$imageSize[0] ?: null, + (int)$imageSize[1] ?: null, + ]; + } catch (Throwable) { + return null; + } finally { + if (is_resource($stream)) { + fclose($stream); + } + } + } + + /** + * @return resource|null + */ + public function getFileStreamRange(string $uriPath, int $start, int $end) + { + if ($start < 0 || $end < $start) { + return null; + } + + $sourceStream = null; + + try { + if (!$this->useLocalFs) { + $bucket = $this->getBucketName(); + + if ($bucket === null) { + return null; + } + + $object = $this->getClient()->getObject([ + 'Bucket' => $bucket, + 'Key' => $this->createBucketPath($uriPath)->toString(), + 'Range' => "bytes=$start-$end", + ]); + + return $this->stringStream((string)$object->get('Body')); + } + + $sourceStream = $this->getFileStream($uriPath); + + if (fseek($sourceStream, $start) === -1) { + return null; + } + + $data = stream_get_contents($sourceStream, $end - $start + 1); + + return is_string($data) ? $this->stringStream($data) : null; + } catch (Throwable) { + return null; + } finally { + if (is_resource($sourceStream)) { + fclose($sourceStream); + } + } + } + + /** + * @return resource|null + */ + private function stringStream(string $contents) + { + $stream = fopen('php://temp', 'r+'); + + if ($stream === false) { + return null; + } + + fwrite($stream, $contents); + rewind($stream); + + return $stream; + } + /** * @inheritDoc */ diff --git a/tests/unit/AssetsControllerTest.php b/tests/unit/AssetsControllerTest.php index d6d85dd..63a0de2 100644 --- a/tests/unit/AssetsControllerTest.php +++ b/tests/unit/AssetsControllerTest.php @@ -5,8 +5,11 @@ use Codeception\Test\Unit; use Craft; use craft\cloud\controllers\AssetsController; +use craft\cloud\fs\Fs; +use craft\elements\Asset; use craft\models\Volume; use ReflectionMethod; +use yii\web\BadRequestHttpException; class AssetsControllerTest extends Unit { @@ -39,6 +42,62 @@ public function testVolumeSubpathReturnsVolumeSubpathOnCraft5(): void $this->assertSame('volume-prefix/', $this->invokeVolumeSubpath($volume)); } + public function testUploadedAssetMetadataUsesVolumeMetadata(): void + { + $controller = new SizeTestAssetsController('cloud-assets', Craft::$app); + $fs = new HeaderTestFs(); + $fs->header = "\xFF\xD8" + . "\xFF\xE1" . pack('n', 4) . 'xx' + . "\xFF\xC0" . pack('n', 17) . "\x08" . pack('n', 3024) . pack('n', 4032) . str_repeat("\0", 10); + $asset = new TestAsset(new TestVolume(123, $fs)); + $asset->setFilename('upload.jpeg'); + $asset->kind = Asset::KIND_IMAGE; + + $controller->setUploadedAssetMetadataForTest($asset, 'upload.jpeg'); + + $this->assertSame(123, $asset->size); + $this->assertSame(4032, $asset->getWidth()); + $this->assertSame(3024, $asset->getHeight()); + $this->assertSame(1, $fs->readCount); + } + + public function testUploadedAssetMetadataDeletesUploadedObjectOnValidationFailure(): void + { + $maxUploadSize = Craft::$app->getConfig()->getGeneral()->maxUploadFileSize; + + if (!$maxUploadSize) { + $this->markTestSkipped('Max upload size is not configured.'); + } + + $controller = new SizeTestAssetsController('cloud-assets', Craft::$app); + $volume = new TestVolume((int)$maxUploadSize + 1); + $asset = new TestAsset($volume); + $asset->folderPath = 'folder/'; + $asset->setFilename('original.jpeg'); + + try { + $controller->setUploadedAssetMetadataForTest($asset, 'upload.jpeg', 'original.jpeg'); + } catch (BadRequestHttpException) { + } + + $this->assertSame(['folder/upload.jpeg'], $volume->deletedPaths); + } + + public function testUploadedImageDimensionsRetryBoundedRanges(): void + { + $fs = new HeaderTestFs(); + $fs->headers = [ + "\xFF\xD8" . "\xFF\xE1" . pack('n', 4) . 'xx', + "\xFF\xD8" . "\xFF\xE1" . pack('n', 4) . 'xx', + "\xFF\xD8" + . "\xFF\xE1" . pack('n', 4) . 'xx' + . "\xFF\xC0" . pack('n', 17) . "\x08" . pack('n', 3024) . pack('n', 4032) . str_repeat("\0", 10), + ]; + + $this->assertSame([4032, 3024], $fs->getImageDimensions('upload.jpeg')); + $this->assertSame(3, $fs->readCount); + } + private function invokeVolumeSubpath(Volume $volume): string { $controller = new AssetsController('cloud-assets', Craft::$app); @@ -48,3 +107,86 @@ private function invokeVolumeSubpath(Volume $volume): string return $method->invoke($controller, $volume); } } + +class SizeTestAssetsController extends AssetsController +{ + public function setUploadedAssetMetadataForTest(Asset $asset, string $filename, ?string $displayFilename = null): void + { + $this->setUploadedAssetMetadata($asset, $filename, $displayFilename); + } +} + +class TestAsset extends Asset +{ + private TestVolume $volume; + + public function __construct(TestVolume $volume) + { + $this->volume = $volume; + + parent::__construct(); + } + + public function getVolume(): Volume + { + return $this->volume; + } +} + +class HeaderTestFs extends Fs +{ + public string $header; + public array $headers = []; + public int $readCount = 0; + + public static function displayName(): string + { + return 'Header Test'; + } + + public function getFileStreamRange(string $uriPath, int $start, int $end) + { + $this->readCount++; + + $stream = fopen('php://temp', 'r+'); + + if ($stream === false) { + return null; + } + + fwrite($stream, $this->headers[$this->readCount - 1] ?? $this->header); + rewind($stream); + + return $stream; + } +} + +class TestVolume extends Volume +{ + public array $deletedPaths = []; + private ?Fs $fs; + private int $fileSize; + + public function __construct(int $fileSize, ?Fs $fs = null) + { + $this->fileSize = $fileSize; + $this->fs = $fs; + + parent::__construct(); + } + + public function getFileSize(string $uri): int + { + return $this->fileSize; + } + + public function getFs(): Fs + { + return $this->fs ?? new HeaderTestFs(); + } + + public function deleteFile(string $path): void + { + $this->deletedPaths[] = $path; + } +} diff --git a/tests/unit/ImageTransformTest.php b/tests/unit/ImageTransformTest.php index aa2be32..4071ddb 100644 --- a/tests/unit/ImageTransformTest.php +++ b/tests/unit/ImageTransformTest.php @@ -5,10 +5,14 @@ use Codeception\Test\Unit; use Craft; use craft\cloud\Module as CloudModule; +use craft\cloud\fs\AssetsFs; use craft\cloud\imagetransforms\ImageTransformBehavior; use craft\cloud\imagetransforms\ImageTransformer; use craft\elements\Asset; +use craft\events\GenerateTransformEvent; use craft\models\ImageTransform; +use craft\models\Volume; +use ReflectionProperty; class ImageTransformTest extends Unit { @@ -123,6 +127,41 @@ public function testGetTransformUrlDoesNotLeakGravityBetweenAssets(): void $this->assertStringNotContainsString('gravity%5Bx%5D=0.57', $secondUrl); } + public function testEditImageActionUsesNativeTransforms(): void + { + $module = new TestCloudModule('cloud-test'); + $event = new GenerateTransformEvent([ + 'asset' => new TransformDecisionAsset(), + 'transform' => new ImageTransform(['width' => 100]), + ]); + $request = Craft::$app->getRequest(); + $isActionRequest = $request->getIsActionRequest(); + $actionSegments = $request->getActionSegments(); + + try { + $request->setIsActionRequest(true); + $this->setActionSegments(['assets', 'edit-image']); + $this->assertFalse($module->usesAssetCdnTransform($event)); + + $this->setActionSegments(['assets', 'generate-transform']); + $this->assertTrue($module->usesAssetCdnTransform($event)); + + $request->setIsActionRequest(false); + $this->setActionSegments(null); + $this->assertTrue($module->usesAssetCdnTransform($event)); + } finally { + $request->setIsActionRequest($isActionRequest); + $this->setActionSegments($actionSegments); + } + } + + private function setActionSegments(?array $actionSegments): void + { + $property = new ReflectionProperty(Craft::$app->getRequest(), '_actionSegments'); + $property->setAccessible(true); + $property->setValue(Craft::$app->getRequest(), $actionSegments); + } + private function makeAssetStub(array $focalPoint): Asset { return new class($focalPoint) extends Asset { @@ -176,7 +215,7 @@ public function getFocalPoint(bool $asCss = false): array|string|null return $this->focalPointValue; } - public function getWidth(array|string|\craft\models\ImageTransform $transform = null): ?int + public function getWidth(array|string|\craft\models\ImageTransform|null $transform = null): ?int { return $this->widthValue; } @@ -234,3 +273,24 @@ public function buildTransformQuery(Asset $asset, ImageTransform $imageTransform return (string) \League\Uri\Components\Query::fromVariable($behavior->toOptions($gravity)); } } + +class TestCloudModule extends CloudModule +{ + public function usesAssetCdnTransform(GenerateTransformEvent $event): bool + { + return $this->shouldUseAssetCdnTransform($event); + } +} + +class TransformDecisionAsset extends Asset +{ + public function getVolume(): Volume + { + return new class() extends Volume { + public function getFs(): AssetsFs + { + return new AssetsFs(); + } + }; + } +}