fix: serve commit images straight from cloud storage

app/Actions/CreateCommitImageSignedUrl.php

@@ -0,0 +1,50 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Actions;
+
+use App\Models\CommitImage;
+use Aws\Credentials\CredentialsInterface;
+use Illuminate\Filesystem\AwsS3V3Adapter;
+use Illuminate\Support\Facades\Storage;
+
+final readonly class CreateCommitImageSignedUrl
+{
+    /**
+     * How long the URL lets anyone holding it fetch the image, which bounds how long an image stays reachable after its commit is deleted or rejected.
+     */
+    public const int LIFETIME = 1200;
+
+    /**
+     * How long the browser reuses the URL, and so the image behind it, kept well inside its lifetime.
+     */
+    public const int MAX_AGE = 600;
+
+    /**
+     * Only cloud storage serves files itself, and only while the credentials signing the URL outlive it: role credentials can expire first, which would leave the browser holding a dead URL.
+     */
+    public function handle(CommitImage $commitImage, bool $download = false): ?string
+    {
+        $disk = Storage::disk();
+
+        if (! $disk instanceof AwsS3V3Adapter) {
+            return null;
+        }
+
+        $expires = now()->addSeconds(self::LIFETIME);
+        $credentials = $disk->getClient()->getCredentials()->wait();
+
+        if (! $credentials instanceof CredentialsInterface || ($credentials->getExpiration() !== null && $credentials->getExpiration() < $expires->getTimestamp())) {
+            return null;
+        }
+
+        return $disk->temporaryUrl($commitImage->path, $expires, [
+            'ResponseContentType' => 'image/webp',
+            'ResponseContentDisposition' => $download
+                ? 'attachment; filename="'.$commitImage->id.'.webp"'
+                : 'inline',
+            'ResponseCacheControl' => 'private, max-age='.self::MAX_AGE,
+        ]);
+    }
+}

app/Http/Controllers/CommitImageController.php

@@ -4,15 +4,28 @@
 
 namespace App\Http\Controllers;
 
+use App\Actions\CreateCommitImageSignedUrl;
 use App\Http\Requests\ShowCommitImageRequest;
 use App\Models\CommitImage;
+use Illuminate\Http\RedirectResponse;
 use Illuminate\Support\Facades\Storage;
 use Symfony\Component\HttpFoundation\StreamedResponse;
 
 final readonly class CommitImageController
 {
-    public function show(ShowCommitImageRequest $request, CommitImage $commitImage): StreamedResponse
+    /**
+     * Public images on cloud storage are redirected to their signed URL; the rest are streamed from here, once the request has allowed them.
+     */
+    public function show(ShowCommitImageRequest $request, CommitImage $commitImage): RedirectResponse|StreamedResponse
     {
+        $url = $request->signedUrl();
+
+        if ($url !== null) {
+            return redirect()->away($url)->withHeaders([
+                'Cache-Control' => 'private, max-age='.CreateCommitImageSignedUrl::MAX_AGE,
+            ]);
+        }
+
         return Storage::disk()->response($commitImage->path, $commitImage->id.'.webp', headers: [
             'Content-Type' => 'image/webp',
             'Content-Security-Policy' => "default-src 'none'; sandbox",

app/Http/Requests/ShowCommitImageRequest.php

@@ -4,6 +4,7 @@
 
 namespace App\Http\Requests;
 
+use App\Actions\CreateCommitImageSignedUrl;
 use App\Models\Commit;
 use App\Models\CommitImage;
 use Illuminate\Auth\Access\Response;
@@ -12,18 +13,29 @@
 
 final class ShowCommitImageRequest extends FormRequest
 {
+    private ?string $signedUrl = null;
+
     /**
-     * Pending and rejected images never leave the server for anyone but their author, the members of its organization
-     * and moderators, or the temporary URL the API gave one of them, until the commit is deleted.
+     * Pending and rejected images never leave the server for anyone but their author and moderators, or the temporary
+     * URL the API gave one of them, until the commit is deleted. So only images anyone may see get a signed storage URL that works without this check.
+     * Cloud storage answers a missing file behind a signed URL itself, so storage is only asked about files streamed from here.
      */
-    public function authorize(): Response
+    public function authorize(CreateCommitImageSignedUrl $action): Response
     {
         $commitImage = $this->route('commitImage');
         assert($commitImage instanceof CommitImage);
 
         $commit = Commit::query()->find($commitImage->commit_id);
 
-        return $commit instanceof Commit && ($commit->imagesAreVisibleTo($this->user()) || (! $commit->trashed() && $this->hasValidSignature())) && Storage::disk()->exists($commitImage->path)
+        if (! $commit instanceof Commit || ! ($commit->imagesAreVisibleTo($this->user()) || (! $commit->trashed() && $this->hasValidSignature()))) {
+            return Response::denyAsNotFound();
+        }
+
+        $this->signedUrl = $commit->imagesAreVisibleTo(null)
+            ? $action->handle($commitImage, $this->download())
+            : null;
+
+        return $this->signedUrl !== null || Storage::disk()->exists($commitImage->path)
             ? Response::allow()
             : Response::denyAsNotFound();
     }
@@ -36,6 +48,17 @@ public function rules(): array
         return [];
     }
 
+    /**
+     * Where the image can be fetched straight from cloud storage, if it can.
+     */
+    public function signedUrl(): ?string
+    {
+        return $this->signedUrl;
+    }
+
+    /**
+     * Whether the image is wanted as a file to save rather than shown in the page.
+     */
     public function download(): bool
     {
         return $this->boolean('download');

tests/Feature/Controllers/CommitImageControllerTest.php

@@ -5,6 +5,8 @@
 use App\Models\Commit;
 use App\Models\CommitImage;
 use App\Models\User;
+use Aws\Credentials\Credentials;
+use Illuminate\Filesystem\AwsS3V3Adapter;
 use Illuminate\Http\UploadedFile;
 use Illuminate\Support\Facades\Storage;
 
@@ -134,3 +136,95 @@ function storedCommitImage(Commit $commit): CommitImage
 
     expect($response->headers->get('Cache-Control'))->toContain('private')->not->toContain('public');
 });
+
+/**
+ * A real S3 disk, which signs URLs offline, so tests can read what the browser is redirected to without reaching AWS.
+ */
+function cloudDisk(?Credentials $credentials = null): AwsS3V3Adapter
+{
+    $disk = Storage::build([
+        'driver' => 's3',
+        'credentials' => $credentials ?? new Credentials('key', 'secret'),
+        'region' => 'us-east-1',
+        'bucket' => 'images',
+    ]);
+    assert($disk instanceof AwsS3V3Adapter);
+
+    return $disk;
+}
+
+it('redirects public images to a signed cloud storage url, without asking storage about the file', function (): void {
+    $image = CommitImage::factory()->for(Commit::factory())->create();
+    Storage::set(config('filesystems.default'), cloudDisk());
+
+    $response = $this->get(route('commit-image.show', $image));
+
+    $response->assertRedirect();
+
+    $url = (string) $response->headers->get('Location');
+    parse_str((string) parse_url($url, PHP_URL_QUERY), $query);
+
+    expect($url)->toStartWith('https://images.s3.amazonaws.com/'.$image->path.'?')
+        ->and($query)->toHaveKey('X-Amz-Signature')
+        ->and((int) $query['X-Amz-Expires'])->toBeBetween(1190, 1200)
+        ->and($query['response-content-type'])->toBe('image/webp')
+        ->and($query['response-content-disposition'])->toBe('inline')
+        ->and($query['response-cache-control'])->toBe('private, max-age=600')
+        ->and($response->headers->get('Cache-Control'))->toContain('private')->toContain('max-age=600')->not->toContain('public');
+});
+
+it('redirects download requests to a signed cloud storage url with attachment disposition', function (): void {
+    $image = CommitImage::factory()->for(Commit::factory())->create();
+
+    Storage::set(config('filesystems.default'), cloudDisk());
+
+    $response = $this->get(route('commit-image.show', ['commitImage' => $image, 'download' => 1]));
+
+    $response->assertRedirect();
+
+    $url = (string) $response->headers->get('Location');
+    parse_str((string) parse_url($url, PHP_URL_QUERY), $query);
+
+    expect($url)->toStartWith('https://images.s3.amazonaws.com/'.$image->path.'?')
+        ->and($query)->toHaveKey('X-Amz-Signature')
+        ->toHaveKey('response-content-disposition')
+        ->and($query['response-content-disposition'])->toBe('attachment; filename="'.$image->id.'.webp"');
+});
+
+it('streams images of pending commits instead of handing out a signed url', function (): void {
+    $commit = Commit::factory()->pending()->create();
+    $image = CommitImage::factory()->for($commit)->create();
+
+    $disk = Mockery::mock(AwsS3V3Adapter::class);
+    $disk->shouldNotReceive('temporaryUrl');
+    $disk->shouldReceive('exists')->with($image->path)->andReturnFalse();
+    Storage::set(config('filesystems.default'), $disk);
+
+    $this->actingAs($commit->user)
+        ->get(route('commit-image.show', $image))
+        ->assertNotFound();
+});
+
+it('streams images when the storage credentials expire before a signed url would', function (): void {
+    $image = CommitImage::factory()->for(Commit::factory())->create();
+
+    $disk = Mockery::mock(AwsS3V3Adapter::class);
+    $disk->shouldReceive('getClient')->andReturn(cloudDisk(new Credentials('key', 'secret', 'token', now()->addMinutes(5)->getTimestamp()))->getClient());
+    $disk->shouldNotReceive('temporaryUrl');
+    $disk->shouldReceive('exists')->with($image->path)->andReturnFalse();
+    Storage::set(config('filesystems.default'), $disk);
+
+    $this->get(route('commit-image.show', $image))->assertNotFound();
+});
+
+it('streams images when cloud storage would sign a plain http url', function (): void {
+    $image = CommitImage::factory()->for(Commit::factory())->create();
+
+    $disk = Mockery::mock(AwsS3V3Adapter::class);
+    $disk->shouldReceive('getClient')->andReturn(cloudDisk()->getClient());
+    $disk->shouldReceive('temporaryUrl')->andReturn('http://minio.test/images/'.$image->path.'?X-Amz-Signature=abc');
+    $disk->shouldReceive('exists')->with($image->path)->andReturnFalse();
+    Storage::set(config('filesystems.default'), $disk);
+
+    $this->get(route('commit-image.show', $image))->assertNotFound();
+});