diff --git a/app/Actions/CreateCommitImageSignedUrl.php b/app/Actions/CreateCommitImageSignedUrl.php new file mode 100644 index 000000000..2c4a34eeb --- /dev/null +++ b/app/Actions/CreateCommitImageSignedUrl.php @@ -0,0 +1,50 @@ +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, + ]); + } +} diff --git a/app/Http/Controllers/CommitImageController.php b/app/Http/Controllers/CommitImageController.php index 943b25213..3205206e2 100644 --- a/app/Http/Controllers/CommitImageController.php +++ b/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", diff --git a/app/Http/Requests/ShowCommitImageRequest.php b/app/Http/Requests/ShowCommitImageRequest.php index 218e2f9f0..81ec6f27f 100644 --- a/app/Http/Requests/ShowCommitImageRequest.php +++ b/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 and moderators, or the temporary - * URL the API gave one of them, until the commit is deleted. + * 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'); diff --git a/tests/Feature/Controllers/CommitImageControllerTest.php b/tests/Feature/Controllers/CommitImageControllerTest.php index 9314912ce..d643fe761 100644 --- a/tests/Feature/Controllers/CommitImageControllerTest.php +++ b/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(); +});