Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion KNOWLEDGE_BASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ Sources: [stash hook](resources/js/hooks/use-stash.ts), [image storage](resource

### Images and descriptions

Attach up to **three JPEG, PNG, or WebP images** to a commit or reply. Each image can be at most **10 MB** and **6,000 pixels in either dimension**. Add an image description of up to **1,000 characters** for accessibility. Add images with the picker, by pasting, or by dropping them on the composer. Every image is re-encoded to WebP, at most 1,600 pixels wide, which strips metadata such as location. Images can be posted without accompanying text. Images on a pending commit are visible only to its author and admins. Image attachments and a poll cannot be combined in the same commit. Open an attached photo to view it in the photo viewer, move through the commit's images, read its description, or download it. Terminal commits can attach images without descriptions; the signed API cannot attach images.
Attach up to **three JPEG, PNG, GIF, or WebP images** to a commit or reply. Each image can be at most **10 MB** and **6,000 pixels in either dimension**. Add an image description of up to **1,000 characters** for accessibility. Add images with the picker, by pasting, or by dropping them on the composer. Every image is re-encoded to WebP, at most 1,600 pixels wide, which strips metadata such as location. GIFs are the exception: they are kept as GIFs at their original size so they stay animated. Messages and terminal commits do not take GIFs. Images can be posted without accompanying text. Images on a pending commit are visible only to its author and admins. Image attachments and a poll cannot be combined in the same commit. Open an attached photo to view it in the photo viewer, move through the commit's images, read its description, or download it. Terminal commits can attach images without descriptions; the signed API cannot attach images.

Sources: [image picker](resources/js/components/commit-image-picker.tsx), [image limits](app/Actions/CreateCommitImages.php), [attachment validation](app/Http/Requests/CreateCommitRequest.php), [photo controller](app/Http/Controllers/CommitPhotoController.php).

Expand Down
32 changes: 22 additions & 10 deletions app/Actions/CreateCommitImageFiles.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,26 @@
namespace App\Actions;

use Illuminate\Http\UploadedFile;
use Illuminate\Image\Image as ProcessedImage;
use Illuminate\Image\ImageException;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Image;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
use Illuminate\Validation\ValidationException;
use Intervention\Gif\Decoder as GifDecoder;
use Intervention\Gif\Exceptions\GifException;

final readonly class CreateCommitImageFiles
{
private const int MAX_STORED_DIMENSION = 1600;

private const int QUALITY = 60;

private const array ALLOWED_TYPES = [IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_WEBP];
private const array ALLOWED_TYPES = [IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_WEBP, IMAGETYPE_GIF];

/**
* Every image is decoded, scaled down, and re-encoded as WebP: metadata such as GPS location is dropped, and anything smuggled alongside the pixels is not kept.
* A GIF keeps its animation instead: re-encoding every frame takes gigabytes of memory, so its blocks are only parsed and written back, which drops bytes outside them.
* All of them are encoded before any is stored, so a bad one leaves nothing behind, and a stored one is erased again if the transaction saving it rolls back.
*
* @param list<UploadedFile> $images
Expand All @@ -36,15 +38,15 @@ public function handle(array $images, array $descriptions = [], string $director
$files = [];

foreach ($optimized as $position => $image) {
$path = $image->storeAs($directory, Str::uuid7()->toString().'.webp');
assert(is_string($path));
$path = $directory.'/'.Str::uuid7()->toString().'.'.$image['extension'];
Storage::disk()->put($path, $image['bytes']);

DB::afterRollBack(fn (): bool => Storage::disk()->delete($path));

$files[] = [
'path' => $path,
'width' => $image->width(),
'height' => $image->height(),
'width' => $image['width'],
'height' => $image['height'],
'description' => $descriptions[$position] ?? null,
'position' => $position,
];
Expand All @@ -55,8 +57,10 @@ public function handle(array $images, array $descriptions = [], string $director

/**
* GD is used explicitly since, unlike Imagick, it never writes the source's metadata back out.
*
* @return array{extension: string, bytes: string, width: int, height: int}
*/
private function optimize(UploadedFile $file): ProcessedImage
private function optimize(UploadedFile $file): array
{
$info = @getimagesize($file->getRealPath());

Expand All @@ -67,25 +71,33 @@ private function optimize(UploadedFile $file): ProcessedImage
throw $this->invalid();
}

if ($info[2] === IMAGETYPE_GIF) {
try {
return ['extension' => 'gif', 'bytes' => GifDecoder::decode($file->getRealPath())->encode(), 'width' => $info[0], 'height' => $info[1]];
} catch (GifException) {
throw $this->invalid();
}
}

$image = Image::fromUpload($file)
->usingGd()
->orient()
->scale(self::MAX_STORED_DIMENSION, self::MAX_STORED_DIMENSION)
->optimize('webp', self::QUALITY);

try {
$image->toBytes();
$bytes = $image->toBytes();
} catch (ImageException) {
throw $this->invalid();
}

return $image;
return ['extension' => 'webp', 'bytes' => $bytes, 'width' => $image->width(), 'height' => $image->height()];
}

private function invalid(): ValidationException
{
return ValidationException::withMessages([
'images' => 'Images must be JPEG, PNG, or WebP files of at most '.CreateCommitImages::MAX_SOURCE_DIMENSION.' pixels per side.',
'images' => 'Images must be JPEG, PNG, GIF, or WebP files of at most '.CreateCommitImages::MAX_SOURCE_DIMENSION.' pixels per side.',
]);
}
}
2 changes: 1 addition & 1 deletion app/Actions/CreateUserFeed.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ private function entry(XMLWriter $xml, User $user, Commit $commit): void
$this->link($xml, 'alternate', route('commit.show', [$user, $commit]), 'text/html');

foreach ($commit->images as $image) {
$this->link($xml, 'enclosure', $image->url(), 'image/webp');
$this->link($xml, 'enclosure', $image->url(), $image->mimeType());
}

$xml->writeElement('published', $commit->created_at->toAtomString());
Expand Down
71 changes: 69 additions & 2 deletions app/Actions/UpdateCommitAiDecision.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
use App\Models\CommitImage;
use App\Services\OpenAI;
use Illuminate\Support\Facades\Storage;
use Intervention\Gif\Decoder as GifDecoder;
use Intervention\Gif\DisposalMethod;

final readonly class UpdateCommitAiDecision
{
Expand All @@ -17,6 +19,11 @@
*/
private const float AUTO_REJECT_THRESHOLD = 0.95;

/**
* How many frames of a GIF are checked, spread evenly from its first to its last.
*/
private const int GIF_FRAMES = 10;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing a bit, 10 seemed fine but I can imagine for longer gifs more could be wanted.
If this feature is wanted at all


public function __construct(
private OpenAI $openAI,
private UpdateCommitStatuses $updateCommitStatuses,
Expand All @@ -32,8 +39,8 @@ public function __construct(
*/
public function handle(Commit $commit): void
{
$images = array_values($commit->images->map(
fn (CommitImage $image): string => 'data:image/webp;base64,'.base64_encode((string) Storage::get($image->path)),
$images = array_values($commit->images->flatMap(
$this->dataUrls(...),
)->all());

$scores = $this->openAI->moderate($commit->body, $images);
Expand Down Expand Up @@ -61,4 +68,64 @@ public function handle(Commit $commit): void
$this->updateCommitStatuses->handle([$commit->id], CommitStatus::Approved, onlyPending: true);
}
}

/**
* The AI does not take animated images, so a GIF is checked by its frames instead.
*
* @return list<string>
*/
private function dataUrls(CommitImage $image): array
{
$bytes = (string) Storage::get($image->path);

if ($image->mimeType() !== 'image/gif') {
return ['data:image/webp;base64,'.base64_encode($bytes)];
}

return array_map(
fn (string $frame): string => 'data:image/png;base64,'.base64_encode($frame),
$this->gifFrames($bytes),
);
}

/**
* Most frames only redraw part of the picture, so each is drawn over the ones before it, as a browser shows it.
*
* @return list<string>
*/
private function gifFrames(string $bytes): array

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely something that I am unsure about. this would mean for a gif at least ten frames are sent to OpenAI

{
$gif = GifDecoder::decode($bytes);
$frames = $gif->frames();
$last = count($frames) - 1;
$checked = array_map(fn (int $step): int => (int) round($step * $last / (self::GIF_FRAMES - 1)), range(0, self::GIF_FRAMES - 1));

// A GIF may claim an empty screen, but its frames still draw onto at least one pixel.
$canvas = imagecreatetruecolor(max(1, $gif->logicalScreenDescriptor()->width()), max(1, $gif->logicalScreenDescriptor()->height()));
$pngs = [];

foreach ($frames as $index => $frame) {
$descriptor = $frame->imageDescriptor();
$disposal = $frame->graphicControlExtension()?->disposalMethod();
$previous = $disposal === DisposalMethod::PREVIOUS ? imagecrop($canvas, ['x' => 0, 'y' => 0, 'width' => imagesx($canvas), 'height' => imagesy($canvas)]) : false;

$image = imagecreatefromstring((clone $gif)->setFrames([$frame])->encode());
assert($image !== false);
imagecopy($canvas, $image, $descriptor->left(), $descriptor->top(), 0, 0, imagesx($image), imagesy($image));

if (in_array($index, $checked, true)) {
ob_start();
imagepng($canvas);
$pngs[] = (string) ob_get_clean();
}

if ($disposal === DisposalMethod::BACKGROUND) {
imagefilledrectangle($canvas, $descriptor->left(), $descriptor->top(), $descriptor->left() + $descriptor->width() - 1, $descriptor->top() + $descriptor->height() - 1, 0);
} elseif ($previous !== false) {
$canvas = $previous;
}
}

return $pngs;
}
}
4 changes: 2 additions & 2 deletions app/Http/Controllers/CommitImageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
{
public function show(ShowCommitImageRequest $request, CommitImage $commitImage): StreamedResponse
{
return Storage::disk()->response($commitImage->path, $commitImage->id.'.webp', headers: [
'Content-Type' => 'image/webp',
return Storage::disk()->response($commitImage->path, $commitImage->id.'.'.pathinfo($commitImage->path, PATHINFO_EXTENSION), headers: [
'Content-Type' => $commitImage->mimeType(),
'Content-Security-Policy' => "default-src 'none'; sandbox",
'X-Content-Type-Options' => 'nosniff',
'Cache-Control' => 'private, max-age=3600',
Expand Down
2 changes: 1 addition & 1 deletion app/Http/Requests/CreateCommitReplyRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function rules(): array
'images.*' => [
'required',
'image',
File::types(['image/jpeg', 'image/png', 'image/webp'])->max(CreateCommitImages::MAX_UPLOAD_SIZE),
File::types(['image/jpeg', 'image/png', 'image/webp', 'image/gif'])->max(CreateCommitImages::MAX_UPLOAD_SIZE),
Rule::dimensions()
->maxWidth(CreateCommitImages::MAX_SOURCE_DIMENSION)
->maxHeight(CreateCommitImages::MAX_SOURCE_DIMENSION),
Expand Down
6 changes: 3 additions & 3 deletions app/Http/Requests/CreateCommitRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static function normalizeBody(FormRequest $request): void
*
* @return array<string, array<int, string|ValidationRule|File|Dimensions|Enum>>
*/
public static function attachmentRules(): array
public static function attachmentRules(bool $allowsGif = false): array
{
return [
'poll' => ['nullable', 'array:options,duration', 'prohibits:parent_id,images'],
Expand All @@ -60,7 +60,7 @@ public static function attachmentRules(): array
'images.*' => [
'required',
'image',
File::types(['image/jpeg', 'image/png', 'image/webp'])->max(CreateCommitImages::MAX_UPLOAD_SIZE),
File::types(['image/jpeg', 'image/png', 'image/webp', ...($allowsGif ? ['image/gif'] : [])])->max(CreateCommitImages::MAX_UPLOAD_SIZE),
Rule::dimensions()
->maxWidth(CreateCommitImages::MAX_SOURCE_DIMENSION)
->maxHeight(CreateCommitImages::MAX_SOURCE_DIMENSION),
Expand Down Expand Up @@ -88,7 +88,7 @@ public function rules(): array
'body' => self::bodyRules($this->hasFile('images')),
'quoted_commit_id' => ['nullable', 'uuid', 'prohibits:parent_id', new VisibleCommit($user, 'That commit is no longer available to quote.')],
'link_preview_url' => ['nullable', 'string', Rule::in(is_string($body) ? resolve(Urls::class)->all($body) : [])],
...self::attachmentRules(),
...self::attachmentRules(allowsGif: true),
];
}

Expand Down
8 changes: 8 additions & 0 deletions app/Models/CommitImage.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,12 @@ public function url(): string
{
return route('commit-image.show', $this);
}

/**
* Images are stored as WebP, apart from GIFs, which are kept as they are to stay animated.
*/
public function mimeType(): string
{
return str_ends_with($this->path, '.gif') ? 'image/gif' : 'image/webp';
}
}
2 changes: 1 addition & 1 deletion app/Services/PageMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ private function commitImages(Commit $commit, User $user): array
'url' => $image->url(),
'width' => $image->width,
'height' => $image->height,
'type' => 'image/webp',
'type' => $image->mimeType(),
'alt' => $count === 1
? "Image attached to the commit of {$user->nameWithHandle()}"
: 'Image '.($index + 1)." of {$count} attached to the commit of {$user->nameWithHandle()}",
Expand Down
9 changes: 7 additions & 2 deletions resources/js/lib/read-picked-images.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@ export class PickedImageError extends Error {}

export const maxImages = 3;
export const maxImageBytes = 10 * 1024 * 1024;
export const acceptedImageTypes = ['image/jpeg', 'image/png', 'image/webp'];
export const acceptedImageTypes = [
'image/jpeg',
'image/png',
'image/webp',
'image/gif',
];
export const invalidImageMessage =
'Only JPEG, PNG, or WebP images up to 10 MB.';
'Only JPEG, PNG, GIF, or WebP images up to 10 MB.';

/**
* Copy provider-backed files before reading metadata or creating preview URLs.
Expand Down
3 changes: 1 addition & 2 deletions tests/Feature/Controllers/CommitControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1409,7 +1409,7 @@
expect($user->commits()->count())->toBe(0);
});

it('may only attach jpeg, png or webp images to a commit', function (UploadedFile $file): void {
it('may only attach jpeg, png, gif or webp images to a commit', function (UploadedFile $file): void {
Storage::fake();

$user = User::factory()->create();
Expand All @@ -1427,7 +1427,6 @@
'svg' => fn (): UploadedFile => UploadedFile::fake()->createWithContent('image.svg', '<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>'),
'html disguised as png' => fn (): UploadedFile => UploadedFile::fake()->createWithContent('image.png', '<html><script>alert(1)</script></html>'),
'bmp' => fn (): UploadedFile => UploadedFile::fake()->image('image.bmp', 10, 10),
'gif' => fn (): UploadedFile => UploadedFile::fake()->image('image.gif', 10, 10),
'pdf' => fn (): UploadedFile => UploadedFile::fake()->create('document.pdf', 10, 'application/pdf'),
]);

Expand Down
12 changes: 12 additions & 0 deletions tests/Feature/Controllers/CommitImageControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ function storedCommitImage(Commit $commit): CommitImage
->and($response->streamedContent())->toBe(Storage::disk()->get($image->path));
})->with(['default' => [[]], 'download disabled' => [['download' => '0']]]);

it('serves gifs as gifs', function (): void {
$path = 'commit-images/'.fake()->uuid().'.gif';
Storage::disk()->put($path, (string) UploadedFile::fake()->image('image.gif', 10, 10)->getContent());
$image = CommitImage::factory()->for(Commit::factory()->create())->create(['path' => $path]);

$response = $this->get(route('commit-image.show', ['commitImage' => $image, 'download' => 1]));

$response->assertOk()
->assertDownload($image->id.'.gif')
->assertHeader('Content-Type', 'image/gif');
});

it('downloads the stored image with a webp filename', function (): void {
$image = storedCommitImage(Commit::factory()->create());

Expand Down
15 changes: 15 additions & 0 deletions tests/Feature/Controllers/ConversationMessageControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,21 @@
->and($response->json('message.images.0.url'))->toBe(route('message-image.show', Message::query()->sole()->images->first()));
})->with(['look at this', '']);

it('does not take gifs, which only commits keep', function (): void {
Storage::fake();

$mark = User::factory()->withRemotes()->create();
$nuno = User::factory()->withRemotes()->create();

$this->actingAs($mark)->post(route('conversation-message.store', $nuno), [
'body' => '',
'client_id' => Str::uuid()->toString(),
'images' => [UploadedFile::fake()->image('animation.gif', 10, 10)],
], ['Accept' => 'application/json'])->assertJsonValidationErrors('images.0');

expect(Storage::disk()->allFiles())->toBe([]);
});

it('stores the images of a retried message only once', function (): void {
Storage::fake();

Expand Down
27 changes: 25 additions & 2 deletions tests/Unit/Actions/CreateCommitImagesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,34 @@
]);
})->throws(ValidationException::class);

it('refuses formats other than jpeg, png and webp', function (string $name): void {
it('keeps gifs as gifs, dropping anything smuggled after them', function (): void {
$file = UploadedFile::fake()->image('animation.gif', 30, 20);
file_put_contents($file->getRealPath(), '<?php echo "pwned"; ?>', FILE_APPEND);

$commit = Commit::factory()->create();

resolve(CreateCommitImages::class)->handle($commit, [$file]);

$image = $commit->images()->sole();
$contents = (string) Storage::disk()->get($image->path);

expect($image->path)->toStartWith('commit-images/')->toEndWith('.gif')
->and($image->only(['width', 'height']))->toBe(['width' => 30, 'height' => 20])
->and(getimagesizefromstring($contents)['mime'] ?? null)->toBe('image/gif')
->and($contents)->not->toContain('<?php');
});

it('refuses gifs that end before their frames do', function (): void {
$file = UploadedFile::fake()->createWithContent('broken.gif', "GIF89a\x0a\x00\x0a\x00\x00\x00\x00");

resolve(CreateCommitImages::class)->handle(Commit::factory()->create(), [$file]);
})->throws(ValidationException::class);

it('refuses formats other than jpeg, png, gif and webp', function (string $name): void {
$file = UploadedFile::fake()->image($name, 10, 10);

resolve(CreateCommitImages::class)->handle(Commit::factory()->create(), [$file]);
})->throws(ValidationException::class)->with(['image.bmp', 'image.gif']);
})->throws(ValidationException::class)->with(['image.bmp']);

it('refuses images too large to decode safely', function (): void {
resolve(CreateCommitImages::class)->handle(Commit::factory()->create(), [UploadedFile::fake()->image('huge.png', 6001, 10)]);
Expand Down
Loading