From 2d63de7a97ad416c42de2f9112c9b0f09b7384ef Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 16 Jul 2026 20:28:21 -0300 Subject: [PATCH] Address review: dedupe memory-budget logic, fix stale doc, cover crop errors - Extract estimatedDecodeMemory() + a MAX_DECODE_MEMORY_BYTES constant so optimizeImage (fallback) and the crop/fit guard (throw) share one estimate instead of duplicating the formula and threshold. - Correct the cropFailureException docblock: it now covers download, crop, and story-fit failures, not just downloads. - Add a Facebook crop process-failure test and an Instagram cropped-temp-leak test so the crop path matches the fit path's error coverage. --- app/Services/Media/MediaOptimizer.php | 46 +++++++++---------- .../Concerns/CropsImageForAspectRatio.php | 4 +- .../Services/Social/FacebookPublisherTest.php | 18 ++++++++ .../Social/InstagramPublisherTest.php | 31 +++++++++++++ 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/app/Services/Media/MediaOptimizer.php b/app/Services/Media/MediaOptimizer.php index abbcb427..63baaba1 100644 --- a/app/Services/Media/MediaOptimizer.php +++ b/app/Services/Media/MediaOptimizer.php @@ -12,6 +12,8 @@ class MediaOptimizer { + private const MAX_DECODE_MEMORY_BYTES = 256 * 1024 * 1024; + private ImageManager $manager; public function __construct() @@ -27,29 +29,18 @@ public function optimizeImage(string $filePath, Platform $platform): string { $config = $this->getImageConfig($platform); - // Check image dimensions to prevent GD memory overflow $imageInfo = @getimagesize($filePath); - if ($imageInfo) { - $width = $imageInfo[0]; - $height = $imageInfo[1]; - $channels = $imageInfo['channels'] ?? 4; - $estimatedMemory = $width * $height * $channels * 1.5; // 1.5x safety margin + if ($imageInfo !== false && $this->estimatedDecodeMemory($imageInfo) > self::MAX_DECODE_MEMORY_BYTES) { + Log::warning('MediaOptimizer: Image too large for GD processing', [ + 'width' => $imageInfo[0], + 'height' => $imageInfo[1], + 'platform' => $platform->value, + ]); - // If image would use more than 256MB of RAM, skip optimization and return as-is - if ($estimatedMemory > 256 * 1024 * 1024) { - Log::warning('MediaOptimizer: Image too large for GD processing', [ - 'width' => $width, - 'height' => $height, - 'estimated_memory' => $estimatedMemory, - 'platform' => $platform->value, - ]); + $tempFile = tempnam(sys_get_temp_dir(), 'media_opt_'); + copy($filePath, $tempFile); - // Copy original file to temp location and return - $tempFile = tempnam(sys_get_temp_dir(), 'media_opt_'); - copy($filePath, $tempFile); - - return $tempFile; - } + return $tempFile; } $image = $this->manager->decodePath($filePath); @@ -186,6 +177,17 @@ public function fitToCanvas(string $filePath, int $width, int $height): string return $tempFile; } + /** + * Estimated GD memory (bytes) needed to decode an image, from its + * getimagesize() metadata. + * + * @param array{0: int, 1: int, channels?: int} $imageInfo + */ + private function estimatedDecodeMemory(array $imageInfo): float + { + return $imageInfo[0] * $imageInfo[1] * ($imageInfo['channels'] ?? 4) * 1.5; + } + /** * Reject a source whose pixel dimensions would blow the GD memory budget, * before it is decoded — a small-byte, huge-dimension image would otherwise @@ -200,9 +202,7 @@ private function assertWithinMemoryBudget(string $filePath): void return; } - $estimatedMemory = $imageInfo[0] * $imageInfo[1] * ($imageInfo['channels'] ?? 4) * 1.5; - - if ($estimatedMemory > 256 * 1024 * 1024) { + if ($this->estimatedDecodeMemory($imageInfo) > self::MAX_DECODE_MEMORY_BYTES) { throw new RuntimeException("Image dimensions ({$imageInfo[0]}x{$imageInfo[1]}) exceed the safe processing budget."); } } diff --git a/app/Services/Social/Concerns/CropsImageForAspectRatio.php b/app/Services/Social/Concerns/CropsImageForAspectRatio.php index a6e5a632..99028461 100644 --- a/app/Services/Social/Concerns/CropsImageForAspectRatio.php +++ b/app/Services/Social/Concerns/CropsImageForAspectRatio.php @@ -97,8 +97,8 @@ protected function aspectRatioToFloat(string $ratio): float } /** - * The platform-specific exception thrown when the source image cannot be - * downloaded for cropping. + * The platform-specific exception thrown when an image can't be prepared for + * publishing — a download, crop, or story-fit failure. */ abstract protected function cropFailureException(string $message): SocialPublishException; } diff --git a/tests/Feature/Services/Social/FacebookPublisherTest.php b/tests/Feature/Services/Social/FacebookPublisherTest.php index a5e8e37e..e6d432a1 100644 --- a/tests/Feature/Services/Social/FacebookPublisherTest.php +++ b/tests/Feature/Services/Social/FacebookPublisherTest.php @@ -765,6 +765,24 @@ function facebookJpegBytes(int $width = 1200, int $height = 800): string ->toThrow(FacebookPublishException::class, 'Failed to download image for cropping'); }); +test('facebook image post throws a clean exception when the crop source is not decodable', function () { + Storage::fake(); + + $this->postPlatform->update(['meta' => ['aspect_ratio' => '4:5']]); + $this->post->update([ + 'media' => [ + ['id' => 'm1', 'path' => 'media/a.jpg', 'url' => 'https://example.com/media/a.jpg', 'mime_type' => 'image/jpeg', 'original_filename' => 'a.jpg'], + ], + ]); + + Http::fake([ + 'https://example.com/media/a.jpg' => Http::response('error', 200, ['Content-Type' => 'text/html']), + ]); + + expect(fn () => $this->publisher->publish($this->postPlatform)) + ->toThrow(FacebookPublishException::class, 'Failed to process image for cropping'); +}); + test('facebook single image post without aspect ratio uploads the original image (no crop)', function () { Storage::fake(); diff --git a/tests/Feature/Services/Social/InstagramPublisherTest.php b/tests/Feature/Services/Social/InstagramPublisherTest.php index 2eb87a89..1639c57b 100644 --- a/tests/Feature/Services/Social/InstagramPublisherTest.php +++ b/tests/Feature/Services/Social/InstagramPublisherTest.php @@ -975,6 +975,37 @@ function fakeJpegBytes(int $width = 1200, int $height = 800): string ->toThrow(InstagramPublishException::class, 'Failed to process image for cropping'); }); +test('instagram publisher does not leak the cropped temp file when hosting the feed image fails', function () { + $this->postPlatform->update(['meta' => ['aspect_ratio' => '4:5']]); + + $this->post->update([ + 'media' => [ + ['id' => 'm1', 'path' => 'media/a.jpg', 'url' => 'https://example.com/media/a.jpg', 'mime_type' => 'image/jpeg', 'original_filename' => 'a.jpg'], + ], + ]); + + Http::fake([ + 'https://example.com/media/a.jpg' => Http::response(fakeJpegBytes(1200, 800), 200), + ]); + + $croppedPath = null; + $mockOptimizer = Mockery::mock(MediaOptimizer::class); + $mockOptimizer->shouldReceive('cropToAspectRatio')->once()->andReturnUsing(function (string $tempFile) use (&$croppedPath) { + $croppedPath = tempnam(sys_get_temp_dir(), 'media_crop_'); + copy($tempFile, $croppedPath); + + return $croppedPath; + }); + app()->instance(MediaOptimizer::class, $mockOptimizer); + + Storage::shouldReceive('put')->once()->andThrow(new RuntimeException('disk full')); + + expect(fn () => $this->publisher->publish($this->postPlatform))->toThrow(RuntimeException::class); + + expect($croppedPath)->not->toBeNull() + ->and(file_exists($croppedPath))->toBeFalse(); +}); + test('feed image with original aspect ratio bypasses crop', function () { Storage::fake();