diff --git a/app/Http/Controllers/Account/AcceptanceController.php b/app/Http/Controllers/Account/AcceptanceController.php index 34dee44257..10c51e3caf 100644 --- a/app/Http/Controllers/Account/AcceptanceController.php +++ b/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')); diff --git a/app/Http/Controllers/Api/UploadedFilesController.php b/app/Http/Controllers/Api/UploadedFilesController.php index cb367ace8c..60b892a8f8 100644 --- a/app/Http/Controllers/Api/UploadedFilesController.php +++ b/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)) { diff --git a/app/Http/Controllers/UploadedFilesController.php b/app/Http/Controllers/UploadedFilesController.php index 0db36b9a2a..46a446fc47 100644 --- a/app/Http/Controllers/UploadedFilesController.php +++ b/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)) { diff --git a/resources/lang/en-US/admin/users/message.php b/resources/lang/en-US/admin/users/message.php index d20c990e50..c88a7173ce 100644 --- a/resources/lang/en-US/admin/users/message.php +++ b/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.', diff --git a/tests/Feature/CheckoutAcceptances/AcceptanceStorageFailureTest.php b/tests/Feature/CheckoutAcceptances/AcceptanceStorageFailureTest.php new file mode 100644 index 0000000000..6f6222a18a --- /dev/null +++ b/tests/Feature/CheckoutAcceptances/AcceptanceStorageFailureTest.php @@ -0,0 +1,111 @@ +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'); + } +} diff --git a/tests/Feature/FileUploads/UploadDeleteStorageFailureTest.php b/tests/Feature/FileUploads/UploadDeleteStorageFailureTest.php new file mode 100644 index 0000000000..259f598b4d --- /dev/null +++ b/tests/Feature/FileUploads/UploadDeleteStorageFailureTest.php @@ -0,0 +1,100 @@ +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, + ]); + } +}