mirror of
https://github.com/snipe/snipe-it.git
synced 2026-08-18 03:06:23 +00:00
Fixed FD-56789 - upload file delete ordering
This commit is contained in:
snipe
@ -112,10 +112,19 @@ class ImageUploadRequest extends Request
|
||||
$ext = $image->guessExtension();
|
||||
$file_name = $type.'-'.$form_fieldname.($item->id ?? '-'.$item->id).'-'.str_random(10).'.'.$ext;
|
||||
|
||||
// Track whether the new file actually landed on disk. Storage::put
|
||||
// can return false without throwing (disks default to non-throwing
|
||||
// mode). Before, the ordering was put -> deleteExistingImage ->
|
||||
// reassign, all unconditional, so a failed put still destroyed
|
||||
// the current image and left the model referencing a file that
|
||||
// was never written. Keep the old image intact unless we confirm
|
||||
// the new one is there.
|
||||
$wroteNewFile = false;
|
||||
|
||||
if (($image->getMimeType() == 'image/vnd.microsoft.icon') || ($image->getMimeType() == 'image/x-icon') || ($image->getMimeType() == 'image/avif') || ($image->getMimeType() == 'image/webp')) {
|
||||
// If the file is an icon, webp or avif, we need to just move it since gd doesn't support resizing
|
||||
// icons or avif, and webp support and needs to be compiled into gd for resizing to be available
|
||||
Storage::disk('public')->put($prefix.$file_name, file_get_contents($image));
|
||||
$wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, file_get_contents($image));
|
||||
|
||||
} elseif ($image->getMimeType() == 'image/svg+xml') {
|
||||
// If the file is an SVG, we need to clean it and NOT encode it
|
||||
@ -124,7 +133,7 @@ class ImageUploadRequest extends Request
|
||||
$cleanSVG = $sanitizer->sanitize($dirtySVG);
|
||||
|
||||
try {
|
||||
Storage::disk('public')->put($prefix.$file_name, $cleanSVG);
|
||||
$wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, $cleanSVG);
|
||||
} catch (\Exception $e) {
|
||||
Log::debug($e);
|
||||
}
|
||||
@ -145,13 +154,25 @@ class ImageUploadRequest extends Request
|
||||
}
|
||||
|
||||
// This requires a string instead of an object, so we use ($string)
|
||||
Storage::disk('public')->put($prefix.$file_name, (string) $upload->encode());
|
||||
$wroteNewFile = (bool) Storage::disk('public')->put($prefix.$file_name, (string) $upload->encode());
|
||||
|
||||
}
|
||||
|
||||
// Remove Current image if exists
|
||||
$item = $this->deleteExistingImage($item, $path, $db_fieldname);
|
||||
$item->{$db_fieldname} = $file_name;
|
||||
if ($wroteNewFile) {
|
||||
// Only touch the existing image and the model reference AFTER
|
||||
// confirming the new file is on disk. deleteExistingImage
|
||||
// itself now also refuses to null the model when the delete
|
||||
// fails, so a partial cleanup does not leave the model
|
||||
// pointing at a phantom file either way.
|
||||
$item = $this->deleteExistingImage($item, $path, $db_fieldname);
|
||||
$item->{$db_fieldname} = $file_name;
|
||||
} else {
|
||||
Log::warning('Image upload failed to write to disk; keeping existing image reference intact.', [
|
||||
'item_type' => $type,
|
||||
'item_id' => $item->id ?? null,
|
||||
'target_path' => $prefix.$file_name,
|
||||
]);
|
||||
}
|
||||
|
||||
// If the user isn't uploading anything new but wants to delete their old image, do so
|
||||
} elseif ($this->input('image_delete') == '1') {
|
||||
@ -166,13 +187,28 @@ class ImageUploadRequest extends Request
|
||||
|
||||
if ($item->{$db_fieldname} != '') {
|
||||
try {
|
||||
// Same path normalization as handleImages — branding callers
|
||||
// Same path normalization as handleImages. Branding callers
|
||||
// pass '' for the disk root, and we don't want to produce a
|
||||
// leading-slash key on S3.
|
||||
$path = trim((string) $path, '/');
|
||||
$key = $path === '' ? $item->{$db_fieldname} : $path.'/'.$item->{$db_fieldname};
|
||||
Storage::disk('public')->delete($key);
|
||||
$item->{$db_fieldname} = null;
|
||||
$deleted = Storage::disk('public')->delete($key);
|
||||
|
||||
// Only null the model reference if the delete actually
|
||||
// succeeded. Before, the field was cleared unconditionally
|
||||
// even when Storage::delete returned false (silent-fail
|
||||
// mode on the default local disk). The result was a model
|
||||
// row that reported "no image" while the file remained on
|
||||
// disk, orphaned.
|
||||
if ($deleted) {
|
||||
$item->{$db_fieldname} = null;
|
||||
} else {
|
||||
Log::warning('Storage delete returned false; keeping model reference so operators can retry.', [
|
||||
'item_type' => class_basename(get_class($item)),
|
||||
'item_id' => $item->id ?? null,
|
||||
'key' => $key,
|
||||
]);
|
||||
}
|
||||
} catch (\Exception $e) {
|
||||
Log::debug($e);
|
||||
}
|
||||
|
||||
146
tests/Feature/Uploads/ImageUploadRequestFailurePathsTest.php
Normal file
146
tests/Feature/Uploads/ImageUploadRequestFailurePathsTest.php
Normal file
@ -0,0 +1,146 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature\Uploads;
|
||||
|
||||
use App\Models\Manufacturer;
|
||||
use App\Models\User;
|
||||
use Illuminate\Http\UploadedFile;
|
||||
use Illuminate\Support\Facades\Storage;
|
||||
use Mockery;
|
||||
use Tests\TestCase;
|
||||
|
||||
/**
|
||||
* Regression coverage for the Storage write/delete failure ordering bug
|
||||
* Christopher Finks reported as Issue 4.
|
||||
*
|
||||
* Pre-fix flow inside ImageUploadRequest::handleImages:
|
||||
* Storage::put(new file) // return value discarded
|
||||
* deleteExistingImage() // unconditional
|
||||
* $item->image = $new; // unconditional
|
||||
*
|
||||
* If put returned false (silent-fail mode on the default local disk), the
|
||||
* old file was destroyed and the model row ended up referencing a file
|
||||
* that never landed on disk. The fix captures the put return, only
|
||||
* proceeds to delete + reassign when the write succeeded, and mirrors the
|
||||
* same check inside deleteExistingImage so a failed delete does not null
|
||||
* out the model reference either.
|
||||
*
|
||||
* We exercise the fix through the manufacturers update endpoint because it
|
||||
* has a straightforward image field and the fewest confounding variables
|
||||
* of the many handleImages callers, but the fix applies to every model
|
||||
* whose controller ultimately routes through ImageUploadRequest.
|
||||
*/
|
||||
class ImageUploadRequestFailurePathsTest extends TestCase
|
||||
{
|
||||
public function test_failed_new_image_write_preserves_existing_image_reference(): void
|
||||
{
|
||||
Storage::fake('public');
|
||||
$publicDisk = Storage::disk('public');
|
||||
|
||||
// Seed a real "existing image" file so the test can assert it still
|
||||
// exists on disk after the failed write.
|
||||
$publicDisk->put('manufacturers/manufacturer-image-pre-existing.png', 'original bytes');
|
||||
|
||||
$manufacturer = Manufacturer::factory()->create([
|
||||
'image' => 'manufacturer-image-pre-existing.png',
|
||||
]);
|
||||
|
||||
// Proxy the public disk so put() returns false (simulating a write
|
||||
// failure that Storage does not throw on) while every other call
|
||||
// still passes through to the real fake disk.
|
||||
$proxy = Mockery::mock($publicDisk);
|
||||
$proxy->shouldReceive('put')->andReturn(false);
|
||||
$proxy->shouldReceive('exists')->andReturnUsing(fn (...$a) => $publicDisk->exists(...$a));
|
||||
$proxy->shouldReceive('delete')->andReturnUsing(fn (...$a) => $publicDisk->delete(...$a));
|
||||
$proxy->shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $publicDisk->makeDirectory(...$a));
|
||||
Storage::shouldReceive('disk')->with('public')->andReturn($proxy);
|
||||
|
||||
$this->actingAs(User::factory()->superuser()->create())
|
||||
->put(route('manufacturers.update', $manufacturer), [
|
||||
'name' => $manufacturer->name,
|
||||
'image' => UploadedFile::fake()->image('replacement.png'),
|
||||
])
|
||||
->assertStatus(302)
|
||||
->assertSessionHasNoErrors();
|
||||
|
||||
$manufacturer->refresh();
|
||||
$this->assertEquals(
|
||||
'manufacturer-image-pre-existing.png',
|
||||
$manufacturer->image,
|
||||
'A failed new-image write must not clear or replace the existing image reference on the model.',
|
||||
);
|
||||
$this->assertTrue(
|
||||
$publicDisk->exists('manufacturers/manufacturer-image-pre-existing.png'),
|
||||
'A failed new-image write must not delete the existing image from disk.',
|
||||
);
|
||||
}
|
||||
|
||||
public function test_failed_delete_preserves_model_reference(): void
|
||||
{
|
||||
// Directly exercises ImageUploadRequest::deleteExistingImage in
|
||||
// isolation because the ManufacturersController path nulls the
|
||||
// model reference itself before handing off to handleImages (see
|
||||
// ManufacturersController::update line 153). The controller-level
|
||||
// pre-null defeats the deleteExistingImage guard for that specific
|
||||
// caller. Every other caller (ProfileController::update for the
|
||||
// user avatar, and any future callsite that relies on
|
||||
// deleteExistingImage to null the field) benefits from the guard.
|
||||
Storage::fake('public');
|
||||
$publicDisk = Storage::disk('public');
|
||||
$publicDisk->put('manufacturers/manufacturer-image-still-there.png', 'still here');
|
||||
|
||||
$manufacturer = Manufacturer::factory()->create([
|
||||
'image' => 'manufacturer-image-still-there.png',
|
||||
]);
|
||||
|
||||
$proxy = Mockery::mock($publicDisk);
|
||||
$proxy->shouldReceive('delete')->andReturn(false);
|
||||
$proxy->shouldReceive('exists')->andReturnUsing(fn (...$a) => $publicDisk->exists(...$a));
|
||||
$proxy->shouldReceive('put')->andReturnUsing(fn (...$a) => $publicDisk->put(...$a));
|
||||
$proxy->shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $publicDisk->makeDirectory(...$a));
|
||||
Storage::shouldReceive('disk')->with('public')->andReturn($proxy);
|
||||
|
||||
$request = app(\App\Http\Requests\ImageUploadRequest::class);
|
||||
$result = $request->deleteExistingImage($manufacturer, 'manufacturers', 'image');
|
||||
|
||||
$this->assertEquals(
|
||||
'manufacturer-image-still-there.png',
|
||||
$result->image,
|
||||
'A failed delete must not null the model image reference - the file may still be on disk and orphaning it is worse than leaving the reference intact.',
|
||||
);
|
||||
}
|
||||
|
||||
public function test_successful_write_still_replaces_existing_image(): void
|
||||
{
|
||||
// Sanity: the fix must not regress the happy path. Successful writes
|
||||
// continue to delete the old file and update the model reference.
|
||||
Storage::fake('public');
|
||||
$publicDisk = Storage::disk('public');
|
||||
$publicDisk->put('manufacturers/manufacturer-image-old.png', 'old bytes');
|
||||
|
||||
$manufacturer = Manufacturer::factory()->create([
|
||||
'image' => 'manufacturer-image-old.png',
|
||||
]);
|
||||
|
||||
$this->actingAs(User::factory()->superuser()->create())
|
||||
->put(route('manufacturers.update', $manufacturer), [
|
||||
'name' => $manufacturer->name,
|
||||
'image' => UploadedFile::fake()->image('new.png'),
|
||||
])
|
||||
->assertStatus(302)
|
||||
->assertSessionHasNoErrors();
|
||||
|
||||
$manufacturer->refresh();
|
||||
$this->assertNotEquals('manufacturer-image-old.png', $manufacturer->image);
|
||||
$this->assertNotNull($manufacturer->image);
|
||||
$this->assertStringContainsString('Manufacturer-image', $manufacturer->image);
|
||||
$this->assertFalse(
|
||||
$publicDisk->exists('manufacturers/manufacturer-image-old.png'),
|
||||
'On a successful new write, the old image file is deleted from disk.',
|
||||
);
|
||||
$this->assertTrue(
|
||||
$publicDisk->exists('manufacturers/'.$manufacturer->image),
|
||||
'On a successful new write, the new file lands at the expected path.',
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user