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/ReportsController.php b/app/Http/Controllers/ReportsController.php index 0f2458dce0..0566bfbe39 100644 --- a/app/Http/Controllers/ReportsController.php +++ b/app/Http/Controllers/ReportsController.php @@ -1326,6 +1326,15 @@ class ReportsController extends Controller $itemsForReport = $query->get() ->filter(fn ($unaccepted) => $unaccepted->checkoutable) + // FMCS scope, mirrors sentAssetAcceptanceReminder + deleteAssetAcceptance. + // CheckoutAcceptance has no company_id column and does not use + // CompanyableTrait / CompanyableChildTrait, so it is not covered + // by the CompanyableScope global scope. Without this per-row + // check, a reports.view user scoped to Company A sees pending + // acceptances for items owned by Company B in both the page + // render and the CSV export. Same helper the two mutating + // actions already use. + ->filter(fn ($unaccepted) => $this->currentUserCanAccessAcceptance($unaccepted)) ->map(fn ($unaccepted) => Checkoutable::fromAcceptance($unaccepted)); return view('reports/unaccepted_assets', compact('itemsForReport', 'showDeleted')); @@ -1513,6 +1522,11 @@ class ReportsController extends Controller $itemsForReport = $acceptances->get() ->filter(fn ($unaccepted) => $unaccepted->checkoutable) + // FMCS scope, same rationale as getAssetAcceptanceReport. + // The CSV export path had the same missing filter as the page + // render, so a reports.view user scoped to Company A could + // download pending acceptances for Company B items. + ->filter(fn ($unaccepted) => $this->currentUserCanAccessAcceptance($unaccepted)) ->map(fn ($unaccepted) => Checkoutable::fromAcceptance($unaccepted)); $rows = []; @@ -1531,6 +1545,16 @@ class ReportsController extends Controller $header = array_map('trim', $header); $rows[] = implode(',', $header); + // Formula-escape data rows using the same helper + setting as the + // sibling exports in this file. Row values (company / category / + // model / item name / asset tag / assignee display name) are all + // user-editable free-text fields that a low-privilege user could + // set to a spreadsheet formula. Without escaping, the payload + // evaluates when a reports.view user opens the downloaded CSV in + // Excel / LibreOffice / Google Sheets. Same backtick prefix used + // by every other export in ReportsController. + $formatter = new EscapeFormula('`'); + foreach ($itemsForReport as $item) { if ($item != null) { @@ -1544,6 +1568,11 @@ class ReportsController extends Controller $row[] = str_replace(',', '', $item->plain_text_name); $row[] = str_replace(',', '', $item->asset_tag); $row[] = str_replace(',', '', ($item->acceptance->assignedto) ? $item->acceptance->assignedto->display_name : trans('admin/reports/general.deleted_user')); + + if (config('app.escape_formulas') !== false) { + $row = $formatter->escapeRecord($row); + } + $rows[] = implode(',', $row); } } 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/app/Models/SnipeModel.php b/app/Models/SnipeModel.php index 5ace6563e2..f59078840b 100644 --- a/app/Models/SnipeModel.php +++ b/app/Models/SnipeModel.php @@ -206,24 +206,68 @@ class SnipeModel extends Model public function getEula() { + // Resolve the raw eula text from the appropriate source, then hand + // it to sanitizeEulaForRender before returning. See that method for + // the security rationale behind the sanitize step. + $raw = null; // This is - for now - only for assets, where the asset model is the thing tied to the category if (($this->model) && ($this->model->category)) { if (($this->model->category->eula_text) && ($this->model->category->use_default_eula == 0)) { - return $this->model->category->eula_text; + $raw = $this->model->category->eula_text; } elseif ($this->model->category->use_default_eula == 1) { - return Setting::getSettings()->default_eula_text; + $raw = Setting::getSettings()->default_eula_text; } else { return false; } // For everything else, just check the category for EULA info } elseif (($this->category) && ($this->category->eula_text)) { - return $this->category->eula_text; + $raw = $this->category->eula_text; } elseif ((Setting::getSettings()->default_eula_text) && (($this->category) && ($this->category->use_default_eula == '1'))) { - return Setting::getSettings()->default_eula_text; + $raw = Setting::getSettings()->default_eula_text; } - return null; + return $this->sanitizeEulaForRender($raw); + } + + /** + * Sanitize raw eula_text before it lands in any renderer. This method + * is invoked by getEula and mirrors the shape Category::getEula uses on + * the web path (Helper::parseEscapedMarkedown = strip_tags + Parsedown + * safe mode) with one addition: an strip on the Parsedown output. + * + * The extra strip is what closes the LFR + SSRF primitive reported + * by W1nterFr3ak (Chris Byron Otieno) on 2026-08-02. Every checkout mail + * template embeds this via `{!! $eula !!}` into a Markdown mailable, + * whose HTML output is walked by laravel-mail-auto-embed, which fetches + * every server-side (file_get_contents for local paths, + * curl with TLS verification disabled for remote URLs) and attaches the + * bytes to the outgoing mail. Any low-privilege user with categories.edit + * could set eula_text to `![x](/var/www/html/.env)` or a raw tag, + * check the asset out to themselves, and receive the file contents (or + * the response body of any URL, including cloud instance metadata) as + * a MIME attachment. + * + * strip_tags kills raw HTML the user might have typed directly. + * Parsedown safe mode converts markdown to HTML. The second img-strip + * removes markdown-syntax images that Parsedown converted + * (e.g. `![x](url)` becoming ``). BlockImagesMarkdownExtension + * on the mail Markdown parser (see config/mail.php) is defense in depth + * for anything that slips past this pre-sanitize. + */ + protected function sanitizeEulaForRender(?string $raw): ?string + { + if ($raw === null || $raw === '') { + return null; + } + + $rendered = Helper::parseEscapedMarkedown($raw); + + if ($rendered === null || $rendered === '') { + return null; + } + + return preg_replace('/]*>/i', '', $rendered); } public function getImageUrl($path = null) 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/resources/views/mail/markdown/bulk-asset-checkout-mail.blade.php b/resources/views/mail/markdown/bulk-asset-checkout-mail.blade.php index 52aa4ef052..d19563fc1e 100644 --- a/resources/views/mail/markdown/bulk-asset-checkout-mail.blade.php +++ b/resources/views/mail/markdown/bulk-asset-checkout-mail.blade.php @@ -68,7 +68,12 @@ @if (!$singular_eula && $group->first()->eula)
-{{ $group->first()->eula }} +{{-- eula is pre-sanitized by SnipeModel::getEula (strip_tags + Parsedown + safe mode + strip) before being loaded into $asset->eula in + BulkAssetCheckoutMail::content, so emitting the resulting HTML raw + preserves formatting without reintroducing the mail-auto-embed + LFR/SSRF vector. --}} +{!! $group->first()->eula !!} @endif @@ -76,7 +81,7 @@ @if ($singular_eula) -{{ $singular_eula }} +{!! $singular_eula !!} @endif 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/CheckoutAcceptances/EulaMailAutoEmbedInjectionTest.php b/tests/Feature/CheckoutAcceptances/EulaMailAutoEmbedInjectionTest.php new file mode 100644 index 0000000000..c8ea72fe89 --- /dev/null +++ b/tests/Feature/CheckoutAcceptances/EulaMailAutoEmbedInjectionTest.php @@ -0,0 +1,139 @@ +` + * server-side (file_get_contents for local paths, curl with TLS + * verification disabled for remote URLs) and attached the bytes to the + * outbound mail. A low-privilege user with categories.create/edit + + * assets.checkout could set eula_text to `![x](/var/www/html/.env)` or a + * raw `` tag, check the asset out to themselves, and receive the + * file contents (or any URL's response body, including cloud metadata) as + * a MIME attachment. + * + * The fix sanitizes at the model boundary: `SnipeModel::getEula` now + * pipes through `Helper::parseEscapedMarkedown` (strip_tags + Parsedown + * safe mode) and additionally strips `` from the Parsedown output, + * killing both attack vectors before eula content reaches any mail + * template. `BlockImagesMarkdownExtension` on the mail Markdown parser + * from GHSA-f3vq-g24v-xc2g remains defense in depth. + * + * These tests exercise the model-layer sanitizer directly and the + * end-to-end mailable render so both surfaces are pinned. + */ +class EulaMailAutoEmbedInjectionTest extends TestCase +{ + private function assetWithEula(string $eulaText): Asset + { + $category = Category::factory()->assetLaptopCategory()->create([ + 'eula_text' => $eulaText, + 'use_default_eula' => 0, + ]); + $model = AssetModel::factory()->create(['category_id' => $category->id]); + + return Asset::factory()->create(['model_id' => $model->id]); + } + + public function test_get_eula_strips_markdown_syntax_image_pointing_at_local_file() + { + $asset = $this->assetWithEula('![logo](/var/www/html/.env)'); + + $rendered = $asset->getEula(); + + $this->assertStringNotContainsString('assertStringNotContainsString('/var/www/html/.env', (string) $rendered); + } + + public function test_get_eula_strips_raw_html_img_pointing_at_local_file() + { + $asset = $this->assetWithEula('logo'); + + $rendered = $asset->getEula(); + + $this->assertStringNotContainsString('assertStringNotContainsString('/var/www/html/.env', (string) $rendered); + } + + public function test_get_eula_strips_markdown_syntax_image_pointing_at_ssrf_target() + { + $asset = $this->assetWithEula('![x](http://169.254.169.254/latest/meta-data/iam/security-credentials/)'); + + $rendered = $asset->getEula(); + + $this->assertStringNotContainsString('assertStringNotContainsString('169.254.169.254', (string) $rendered); + } + + public function test_get_eula_strips_raw_html_img_pointing_at_loopback_ssrf_target() + { + $asset = $this->assetWithEula('ssrf'); + + $rendered = $asset->getEula(); + + $this->assertStringNotContainsString('assertStringNotContainsString('127.0.0.1', (string) $rendered); + } + + public function test_get_eula_preserves_legitimate_markdown_formatting() + { + $asset = $this->assetWithEula("**Terms** apply.\n\n- item one\n- item two"); + + $rendered = (string) $asset->getEula(); + + $this->assertStringContainsString('Terms', $rendered); + $this->assertStringContainsString('
  • item one
    • ', $rendered); + } + + public function test_checkout_asset_mail_render_omits_poisoned_img_from_eula() + { + $asset = $this->assetWithEula('![logo](/var/www/html/.env)'); + $target = User::factory()->create(); + $admin = User::factory()->create(); + + $mail = new CheckoutAssetMail( + $asset, + $target, + $admin, + null, + null, + ); + + $rendered = (string) $mail->render(); + + $this->assertStringNotContainsString('assertStringNotContainsString('/var/www/html/.env', $rendered); + } + + public function test_checkout_asset_mail_render_omits_raw_html_img_from_eula() + { + $asset = $this->assetWithEula('logo'); + $target = User::factory()->create(); + $admin = User::factory()->create(); + + $mail = new CheckoutAssetMail( + $asset, + $target, + $admin, + null, + null, + ); + + $rendered = (string) $mail->render(); + + $this->assertStringNotContainsString('assertStringNotContainsString('/etc/hostname', $rendered); + } +} 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, + ]); + } +} diff --git a/tests/Feature/Modals/Ui/ShowModalsTest.php b/tests/Feature/Modals/Ui/ShowModalsTest.php index 2dc77ccd34..9951459eeb 100644 --- a/tests/Feature/Modals/Ui/ShowModalsTest.php +++ b/tests/Feature/Modals/Ui/ShowModalsTest.php @@ -9,7 +9,18 @@ class ShowModalsTest extends TestCase { public function test_user_modal_renders() { - $admin = User::factory()->createUsers()->create(); + // Force distinctive attribute values here rather than accepting the + // Faker defaults. assertDontSee does a substring check on the entire + // response body, so a Faker-generated first name of "Gene" collides + // with legitimate modal text like "Generate Password" and produces a + // false-positive failure. The strings below cannot appear inside any + // translated label, class name, or DOM attribute in the modal. + $admin = User::factory()->createUsers()->create([ + 'first_name' => 'ZzModalTestFirst', + 'last_name' => 'ZzModalTestLast', + 'email' => 'zz-modal-test@example.invalid', + ]); + $response = $this->actingAs($admin) ->get('modals/user') ->assertOk(); diff --git a/tests/Feature/Reporting/AcceptanceReportCsvFormulaEscapeTest.php b/tests/Feature/Reporting/AcceptanceReportCsvFormulaEscapeTest.php new file mode 100644 index 0000000000..86066b4e64 --- /dev/null +++ b/tests/Feature/Reporting/AcceptanceReportCsvFormulaEscapeTest.php @@ -0,0 +1,81 @@ +create(); + $asset = Asset::factory()->create(['name' => $assetName, 'company_id' => $company->id]); + + return CheckoutAcceptance::factory()->pending()->for($asset, 'checkoutable')->create(); + } + + public function test_data_rows_with_formula_prefix_are_escaped_by_default() + { + $this->seedPendingAcceptanceWithAssetNamed('=HYPERLINK("http://attacker.test","click")'); + + $body = $this->actingAs(User::factory()->superuser()->create()) + ->post(route('reports/export/unaccepted_assets')) + ->assertOk() + ->getContent(); + + $this->assertStringNotContainsString('=HYPERLINK("http://attacker.test","click")', $body); + $this->assertStringContainsString('`=HYPERLINK', $body); + } + + public function test_data_rows_with_plus_and_at_prefixes_are_escaped() + { + $this->seedPendingAcceptanceWithAssetNamed('+cmd|/c calc'); + $this->seedPendingAcceptanceWithAssetNamed('@SUM(A1:A9)'); + + $body = $this->actingAs(User::factory()->superuser()->create()) + ->post(route('reports/export/unaccepted_assets')) + ->assertOk() + ->getContent(); + + $this->assertStringContainsString('`+cmd|', $body); + $this->assertStringContainsString('`@SUM(', $body); + } + + public function test_data_rows_are_not_escaped_when_setting_disabled() + { + // Matches how the sibling exports in ReportsController behave when + // operators intentionally disable escaping. + config(['app.escape_formulas' => false]); + + $this->seedPendingAcceptanceWithAssetNamed('=SUM(A1:A9)'); + + $body = $this->actingAs(User::factory()->superuser()->create()) + ->post(route('reports/export/unaccepted_assets')) + ->assertOk() + ->getContent(); + + $this->assertStringContainsString('=SUM(A1:A9)', $body); + $this->assertStringNotContainsString('`=SUM(A1:A9)', $body); + } +} diff --git a/tests/Feature/Reporting/AcceptanceReportFmcsScopeTest.php b/tests/Feature/Reporting/AcceptanceReportFmcsScopeTest.php new file mode 100644 index 0000000000..13d62fb550 --- /dev/null +++ b/tests/Feature/Reporting/AcceptanceReportFmcsScopeTest.php @@ -0,0 +1,127 @@ +create(['company_id' => $company->id, 'name' => 'Asset-'.$company->id]); + $acceptance = CheckoutAcceptance::factory()->pending()->for($asset, 'checkoutable')->create(); + + return [$asset, $acceptance]; + } + + public function test_page_render_hides_other_company_pending_acceptances_under_fmcs() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + [$assetA] = $this->seedPendingAcceptanceOwnedBy($companyA); + [$assetB] = $this->seedPendingAcceptanceOwnedBy($companyB); + + $reporterA = User::factory()->canViewReports()->forCompany($companyA)->create(); + + $response = $this->actingAs($reporterA) + ->get(route('reports/unaccepted_assets')) + ->assertOk(); + + $this->assertStringContainsString($assetA->name, $response->getContent()); + $this->assertStringNotContainsString($assetB->name, $response->getContent()); + } + + public function test_csv_export_hides_other_company_pending_acceptances_under_fmcs() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + [$assetA] = $this->seedPendingAcceptanceOwnedBy($companyA); + [$assetB] = $this->seedPendingAcceptanceOwnedBy($companyB); + + $reporterA = User::factory()->canViewReports()->forCompany($companyA)->create(); + + $response = $this->actingAs($reporterA) + ->post(route('reports/export/unaccepted_assets')) + ->assertOk(); + + $body = $response->getContent(); + $this->assertStringContainsString($assetA->name, $body); + $this->assertStringNotContainsString($assetB->name, $body); + } + + public function test_superuser_sees_all_company_pending_acceptances_in_page() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + [$assetA] = $this->seedPendingAcceptanceOwnedBy($companyA); + [$assetB] = $this->seedPendingAcceptanceOwnedBy($companyB); + + $superuser = User::factory()->superuser()->forCompany($companyA)->create(); + + $response = $this->actingAs($superuser) + ->get(route('reports/unaccepted_assets')) + ->assertOk(); + + $this->assertStringContainsString($assetA->name, $response->getContent()); + $this->assertStringContainsString($assetB->name, $response->getContent()); + } + + public function test_superuser_sees_all_company_pending_acceptances_in_csv() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + [$assetA] = $this->seedPendingAcceptanceOwnedBy($companyA); + [$assetB] = $this->seedPendingAcceptanceOwnedBy($companyB); + + $superuser = User::factory()->superuser()->forCompany($companyA)->create(); + + $response = $this->actingAs($superuser) + ->post(route('reports/export/unaccepted_assets')) + ->assertOk(); + + $body = $response->getContent(); + $this->assertStringContainsString($assetA->name, $body); + $this->assertStringContainsString($assetB->name, $body); + } + + public function test_fmcs_disabled_leaves_report_unscoped() + { + // With FMCS off the helper short-circuits and every row passes. + // Guard against future refactors that accidentally add scoping on + // installs that do not have FMCS enabled. + [$companyA, $companyB] = Company::factory()->count(2)->create(); + [$assetA] = $this->seedPendingAcceptanceOwnedBy($companyA); + [$assetB] = $this->seedPendingAcceptanceOwnedBy($companyB); + + $reporterA = User::factory()->canViewReports()->forCompany($companyA)->create(); + + $response = $this->actingAs($reporterA) + ->get(route('reports/unaccepted_assets')) + ->assertOk(); + + $this->assertStringContainsString($assetA->name, $response->getContent()); + $this->assertStringContainsString($assetB->name, $response->getContent()); + } +}