Fixed FD-56789 - better handle failed writes

app/Http/Controllers/Account/AcceptanceController.php

@@ -172,7 +172,18 @@ class AcceptanceController extends Controller
                 $decoded_image = $this->flattenSignatureBackgroundToWhite($decoded_image);
                 $encodedSignatureImage = base64_encode($decoded_image);
 
-                Storage::put('private_uploads/signatures/'.$sig_filename, (string) $decoded_image);
+                // Storage::put returns false on silent write failures on
+                // non-throwing filesystem drivers. Ignoring the return let
+                // acceptance finalization proceed while the signature file
+                // was absent from disk, producing an "accepted" record whose
+                // evidence file did not exist. Refuse to advance when the
+                // write did not land. Reported by Christopher Finks
+                // (christopherfi-dev) on 2026-08-02.
+                if (! Storage::put('private_uploads/signatures/'.$sig_filename, (string) $decoded_image)) {
+                    Log::warning('Acceptance signature write failed for '.$sig_filename);
+
+                    return redirect()->back()->with('error', trans('admin/users/message.accept_signature_write_failed'));
+                }
 
                 // No image data is present, kick them back.
                 // This mostly only applies to users on super-duper crapola browsers *cough* IE *cough*
@@ -241,7 +252,19 @@ class AcceptanceController extends Controller
 
             // Generate the PDF content
             $pdf_content = $acceptance->generateAcceptancePdf($data, $acceptance);
-            Storage::put('private_uploads/eula-pdfs/'.$pdf_filename, $pdf_content);
+
+            // Storage::put returns false on silent write failures on
+            // non-throwing filesystem drivers. Ignoring the return let
+            // acceptance finalization proceed while the acceptance PDF was
+            // absent from disk, producing an "accepted" record whose
+            // evidence file did not exist. Refuse to advance when the
+            // write did not land. Reported by Christopher Finks
+            // (christopherfi-dev) on 2026-08-02.
+            if (! Storage::put('private_uploads/eula-pdfs/'.$pdf_filename, $pdf_content)) {
+                Log::warning('Acceptance PDF write failed for '.$pdf_filename);
+
+                return redirect()->back()->with('error', trans('admin/users/message.accept_pdf_write_failed'));
+            }
 
             // Log the acceptance
             $acceptance->accept($sig_filename, $item->getEula(), $pdf_filename, $request->input('note'));

app/Http/Controllers/Api/UploadedFilesController.php

@@ -217,9 +217,24 @@ class UploadedFilesController extends Controller
             ->first();
 
         if ($log) {
-            // Check the file actually exists, and delete it
+            // Check the file actually exists, and delete it.
+            //
+            // Storage::delete returns false on silent delete failures on
+            // non-throwing filesystem drivers. Ignoring the return let a
+            // failed physical delete produce an "upload deleted" action-log
+            // entry, which HasUploads::uploads uses to exclude the row from
+            // normal listings. Net effect: bytes still on disk, action log
+            // shows the file as deleted, admin sees a success response, and
+            // the file is invisible through the ordinary UI. Refuse to log
+            // the deletion when the physical delete did not succeed.
+            // Reported by Christopher Finks (christopherfi-dev) on
+            // 2026-08-02.
             if (Storage::exists(parent::getMapStoragePath()[$object_type].$log->filename)) {
-                Storage::delete(parent::getMapStoragePath()[$object_type].$log->filename);
+                if (! Storage::delete(parent::getMapStoragePath()[$object_type].$log->filename)) {
+                    \Log::warning('File storage delete failed for '.$log->filename.' on '.parent::getMapObjectType()[$object_type].' id '.$id);
+
+                    return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('general.file_upload_status.delete.error', 1)), 500);
+                }
             }
             // Delete the record of the file
             if ($log->logUploadDelete($object, $log->filename)) {

app/Http/Controllers/UploadedFilesController.php

@@ -146,9 +146,24 @@ class UploadedFilesController extends Controller
             ->where('item_id', $object->id)->first();
 
         if ($log) {
-            // Check the file actually exists, and delete it
+            // Check the file actually exists, and delete it.
+            //
+            // Storage::delete returns false on silent delete failures on
+            // non-throwing filesystem drivers. Ignoring the return let a
+            // failed physical delete produce an "upload deleted" action-log
+            // entry, which HasUploads::uploads uses to exclude the row from
+            // normal listings. Net effect: bytes still on disk, action log
+            // shows the file as deleted, admin sees a success response, and
+            // the file is invisible through the ordinary UI. Refuse to log
+            // the deletion when the physical delete did not succeed.
+            // Reported by Christopher Finks (christopherfi-dev) on
+            // 2026-08-02.
             if (Storage::exists(parent::getMapStoragePath()[$object_type].$log->filename)) {
-                Storage::delete(parent::getMapStoragePath()[$object_type].$log->filename);
+                if (! Storage::delete(parent::getMapStoragePath()[$object_type].$log->filename)) {
+                    \Log::warning('File storage delete failed for '.$log->filename.' on '.parent::getMapObjectType()[$object_type].' id '.$id);
+
+                    return redirect()->back()->withFragment('files')->with('error', trans_choice('general.file_upload_status.delete.error', 1));
+                }
             }
             // Delete the record of the file
             if ($log->logUploadDelete($object, $log->filename)) {

resources/lang/en-US/admin/users/message.php

@@ -4,6 +4,8 @@ return [
 
     'accepted' => 'You have successfully accepted this item.',
     'declined' => 'You have successfully declined this item.',
+    'accept_signature_write_failed' => 'Your acceptance signature could not be saved to storage. Acceptance was not recorded. Please contact your administrator.',
+    'accept_pdf_write_failed' => 'The acceptance PDF could not be saved to storage. Acceptance was not recorded. Please contact your administrator.',
     'bulk_manager_warn' => 'Your users have been successfully updated, however your manager entry was not saved because the manager you selected was also in the user list to be edited, and users may not be their own manager. Please select your users again, excluding the manager.',
     'user_exists' => 'User already exists!',
     'cannot_delete' => 'User does not exist or you do not have permission to delete them.',

tests/Feature/CheckoutAcceptances/AcceptanceStorageFailureTest.php

@@ -0,0 +1,111 @@
+<?php
+
+namespace Tests\Feature\CheckoutAcceptances;
+
+use App\Models\Asset;
+use App\Models\CheckoutAcceptance;
+use App\Models\Setting;
+use App\Models\User;
+use Illuminate\Support\Facades\Storage;
+use Mockery;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for Christopher Finks (christopherfi-dev) Issue 6:
+ * AcceptanceController::store used Storage::put for the signature image and
+ * for the acceptance PDF without checking the return value. On non-throwing
+ * filesystem configurations a silent put() failure returned false, but the
+ * flow continued into $acceptance->accept(), populating accepted_at, the
+ * signature/EULA filename fields, the "accepted" action log, and the
+ * downstream notifications, even though the evidence files were absent
+ * from storage. The application then presented a completed acceptance
+ * whose evidence file did not exist.
+ *
+ * The fix checks both put() returns explicitly and short-circuits with a
+ * user-visible error before advancing acceptance state.
+ */
+class AcceptanceStorageFailureTest extends TestCase
+{
+    private function pendingAcceptance(User $target): CheckoutAcceptance
+    {
+        $asset = Asset::factory()->assignedToUser($target)->create();
+
+        return CheckoutAcceptance::factory()->pending()->for($asset, 'checkoutable')->create([
+            'assigned_to_id' => $target->id,
+        ]);
+    }
+
+    private function acceptPayloadWithSignature(): array
+    {
+        // Minimal valid signature payload: a data URI containing base64-encoded
+        // bytes. flattenSignatureBackgroundToWhite is tolerant of arbitrary
+        // input because it treats non-decodable data as opaque bytes.
+        $body = base64_encode('signature-bytes');
+
+        return [
+            'asset_acceptance' => 'accepted',
+            'signature_output' => 'data:image/png;base64,'.$body,
+        ];
+    }
+
+    private function mockStorageToFailPut(): void
+    {
+        // Storage::put on the default disk returns false; every other
+        // Storage call passes through so exists / makeDirectory continue
+        // to work for the pre-flight directory checks in AcceptanceController.
+        Storage::fake();
+        $default = Storage::disk();
+
+        $proxy = Mockery::mock($default);
+        $proxy->shouldReceive('put')->andReturn(false);
+        $proxy->shouldReceive('exists')->andReturnUsing(fn (...$a) => $default->exists(...$a));
+        $proxy->shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $default->makeDirectory(...$a));
+
+        Storage::shouldReceive('disk')->andReturn($proxy);
+        Storage::shouldReceive('exists')->andReturnUsing(fn (...$a) => $default->exists(...$a));
+        Storage::shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $default->makeDirectory(...$a));
+        Storage::shouldReceive('put')->andReturn(false);
+    }
+
+    public function test_failed_signature_write_does_not_finalize_acceptance(): void
+    {
+        // Require signatures so the signature write path fires.
+        $settings = Setting::query()->firstOrFail();
+        $settings->require_accept_signature = 1;
+        $settings->save();
+        Setting::$_cache = null;
+
+        $target = User::factory()->create();
+        $acceptance = $this->pendingAcceptance($target);
+
+        $this->mockStorageToFailPut();
+
+        $response = $this->actingAs($target)
+            ->post(route('account.store-acceptance', $acceptance), $this->acceptPayloadWithSignature());
+
+        $response->assertSessionHas('error');
+
+        $acceptance->refresh();
+        $this->assertNull($acceptance->accepted_at, 'accepted_at must not populate when the signature write failed');
+        $this->assertNull($acceptance->signature_filename, 'signature_filename must not populate when the signature write failed');
+    }
+
+    public function test_failed_pdf_write_does_not_finalize_acceptance(): void
+    {
+        // Signature not required; only the PDF write path fires.
+        $target = User::factory()->create();
+        $acceptance = $this->pendingAcceptance($target);
+
+        $this->mockStorageToFailPut();
+
+        $response = $this->actingAs($target)
+            ->post(route('account.store-acceptance', $acceptance), [
+                'asset_acceptance' => 'accepted',
+            ]);
+
+        $response->assertSessionHas('error');
+
+        $acceptance->refresh();
+        $this->assertNull($acceptance->accepted_at, 'accepted_at must not populate when the PDF write failed');
+    }
+}

tests/Feature/FileUploads/UploadDeleteStorageFailureTest.php

@@ -0,0 +1,100 @@
+<?php
+
+namespace Tests\Feature\FileUploads;
+
+use App\Models\Actionlog;
+use App\Models\Asset;
+use App\Models\User;
+use Illuminate\Http\UploadedFile;
+use Illuminate\Support\Facades\Storage;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for Christopher Finks (christopherfi-dev) Issue 7:
+ * both UploadedFilesController and Api\UploadedFilesController called
+ * Storage::delete() without checking the return value, then created an
+ * "upload deleted" action log entry and returned success unconditionally.
+ * HasUploads::uploads excludes rows whose filename matches an "upload
+ * deleted" log, so a silently failed physical delete produced: file still
+ * on disk, action log states it's gone, admin sees a success response, and
+ * the file is invisible through the ordinary UI listing.
+ *
+ * The fix checks Storage::delete's return and refuses to create the
+ * deletion log (or return success) when the physical delete failed.
+ */
+class UploadDeleteStorageFailureTest extends TestCase
+{
+    private function seedAssetWithUpload(User $actor): array
+    {
+        $asset = Asset::factory()->create();
+
+        // Upload a real file so a corresponding "uploaded" action log
+        // exists for the destroy path to find.
+        $this->actingAs($actor)
+            ->post(route('ui.files.store', ['object_type' => 'assets', 'id' => $asset->id]), [
+                'file' => [UploadedFile::fake()->create('test.pdf', 10)],
+            ])
+            ->assertRedirect();
+
+        $log = Actionlog::where('item_type', Asset::class)
+            ->where('item_id', $asset->id)
+            ->where('action_type', 'uploaded')
+            ->latest()
+            ->first();
+
+        return [$asset, $log];
+    }
+
+    private function mockStorageToFailDelete(): void
+    {
+        // Storage::exists returns true (file is present), Storage::delete
+        // returns false (silent-fail). All other calls pass through.
+        Storage::fake();
+        $default = Storage::disk();
+
+        Storage::shouldReceive('exists')->andReturn(true);
+        Storage::shouldReceive('delete')->andReturn(false);
+        Storage::shouldReceive('disk')->andReturn($default);
+        Storage::shouldReceive('makeDirectory')->andReturnUsing(fn (...$a) => $default->makeDirectory(...$a));
+    }
+
+    public function test_web_delete_does_not_log_deletion_when_physical_delete_fails(): void
+    {
+        $user = User::factory()->superuser()->create();
+        [$asset, $log] = $this->seedAssetWithUpload($user);
+
+        $this->mockStorageToFailDelete();
+
+        $response = $this->actingAs($user)
+            ->delete(route('ui.files.destroy', ['object_type' => 'assets', 'id' => $asset->id, 'file_id' => $log->id]));
+
+        $response->assertSessionHas('error');
+
+        $this->assertDatabaseMissing('action_logs', [
+            'item_type' => Asset::class,
+            'item_id' => $asset->id,
+            'action_type' => 'upload deleted',
+            'filename' => $log->filename,
+        ]);
+    }
+
+    public function test_api_delete_does_not_log_deletion_when_physical_delete_fails(): void
+    {
+        $user = User::factory()->superuser()->create();
+        [$asset, $log] = $this->seedAssetWithUpload($user);
+
+        $this->mockStorageToFailDelete();
+
+        $response = $this->actingAsForApi($user)
+            ->deleteJson(route('api.files.destroy', ['object_type' => 'assets', 'id' => $asset->id, 'file_id' => $log->id]));
+
+        $response->assertStatus(500);
+
+        $this->assertDatabaseMissing('action_logs', [
+            'item_type' => Asset::class,
+            'item_id' => $asset->id,
+            'action_type' => 'upload deleted',
+            'filename' => $log->filename,
+        ]);
+    }
+}