3
0
mirror of https://github.com/snipe/snipe-it.git synced 2026-08-18 11:15:42 +00:00

Merge remote-tracking branch 'origin/develop'

This commit is contained in:
snipe
2026-08-02 12:58:22 +01:00
13 changed files with 716 additions and 14 deletions

View File

@ -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'));

View File

@ -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)) {

View File

@ -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);
}
}

View File

@ -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)) {

View File

@ -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 <img> strip on the Parsedown output.
*
* The extra <img> 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 <img src=""> 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 <img> 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 <img> 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 `<img src=url>`). 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('/<img\b[^>]*>/i', '', $rendered);
}
public function getImageUrl($path = null)

View File

@ -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.',

View File

@ -68,7 +68,12 @@
@if (!$singular_eula && $group->first()->eula)
<hr>
{{ $group->first()->eula }}
{{-- eula is pre-sanitized by SnipeModel::getEula (strip_tags + Parsedown
safe mode + <img> 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
</x-mail::panel>
@ -76,7 +81,7 @@
@if ($singular_eula)
<x-mail::panel>
{{ $singular_eula }}
{!! $singular_eula !!}
</x-mail::panel>
@endif

View File

@ -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');
}
}

View File

@ -0,0 +1,139 @@
<?php
namespace Tests\Feature\CheckoutAcceptances;
use App\Mail\CheckoutAssetMail;
use App\Models\Asset;
use App\Models\AssetModel;
use App\Models\Category;
use App\Models\User;
use Illuminate\Support\Facades\Mail;
use Tests\TestCase;
/**
* Regression coverage for the arbitrary local-file read + SSRF reported by
* W1nterFr3ak (Chris Byron Otieno) on 2026-08-02. Category eula_text was
* passed raw through SnipeModel::getEula, echoed via `{!! $eula !!}` into
* every checkout mail template, and the resulting HTML was walked by
* `laravel-mail-auto-embed`, which fetched every `<img src="">`
* 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 `<img>` 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 `<img>` 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('<img', (string) $rendered);
$this->assertStringNotContainsString('/var/www/html/.env', (string) $rendered);
}
public function test_get_eula_strips_raw_html_img_pointing_at_local_file()
{
$asset = $this->assetWithEula('<img src="/var/www/html/.env" alt="logo">');
$rendered = $asset->getEula();
$this->assertStringNotContainsString('<img', (string) $rendered);
$this->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('<img', (string) $rendered);
$this->assertStringNotContainsString('169.254.169.254', (string) $rendered);
}
public function test_get_eula_strips_raw_html_img_pointing_at_loopback_ssrf_target()
{
$asset = $this->assetWithEula('<img src="http://127.0.0.1:9999/secret" alt="ssrf">');
$rendered = $asset->getEula();
$this->assertStringNotContainsString('<img', (string) $rendered);
$this->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('<strong>Terms</strong>', $rendered);
$this->assertStringContainsString('<li>item one</li>', $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('<img', $rendered);
$this->assertStringNotContainsString('/var/www/html/.env', $rendered);
}
public function test_checkout_asset_mail_render_omits_raw_html_img_from_eula()
{
$asset = $this->assetWithEula('<img src="/etc/hostname" alt="logo">');
$target = User::factory()->create();
$admin = User::factory()->create();
$mail = new CheckoutAssetMail(
$asset,
$target,
$admin,
null,
null,
);
$rendered = (string) $mail->render();
$this->assertStringNotContainsString('<img', $rendered);
$this->assertStringNotContainsString('/etc/hostname', $rendered);
}
}

View File

@ -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,
]);
}
}

View File

@ -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();

View File

@ -0,0 +1,81 @@
<?php
namespace Tests\Feature\Reporting;
use App\Models\Asset;
use App\Models\CheckoutAcceptance;
use App\Models\Company;
use App\Models\User;
use Tests\TestCase;
/**
* Regression coverage for the CSV formula injection reported by Arpit Jain
* (arpitjain099) on 2026-08-02. postAssetAcceptanceReport built its CSV
* by hand and str_replace(',', '', ...) each cell before joining with
* implode(','). Stripping commas kept the manual join from breaking but
* did nothing about formulas. Every other CSV export in
* ReportsController used League\Csv\EscapeFormula gated on
* config('app.escape_formulas'); this one did not.
*
* The fix adds the same EscapeFormula pass in the loop with the same
* gating, so a low-privilege user who plants a formula in one of the
* asset / company / user free-text fields no longer sees the payload
* execute when a reports.view user opens the CSV in Excel / LibreOffice
* / Google Sheets.
*/
class AcceptanceReportCsvFormulaEscapeTest extends TestCase
{
private function seedPendingAcceptanceWithAssetNamed(string $assetName): CheckoutAcceptance
{
// Company + asset are wired so the resulting row surfaces the
// poisoned name in the report's Name column.
$company = Company::factory()->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);
}
}

View File

@ -0,0 +1,127 @@
<?php
namespace Tests\Feature\Reporting;
use App\Models\Asset;
use App\Models\CheckoutAcceptance;
use App\Models\Company;
use App\Models\User;
use Tests\TestCase;
/**
* Regression coverage for the FMCS scope gap reported by Arpit Jain
* (arpitjain099) on 2026-08-02. Both getAssetAcceptanceReport (the page)
* and postAssetAcceptanceReport (the CSV export) ran
* CheckoutAcceptance::pending() with no company scope. CheckoutAcceptance
* has no company_id column and does not use CompanyableTrait /
* CompanyableChildTrait, so it is not covered by the CompanyableScope
* global scope. Companion read-side bug to GHSA-p5wx-p3vv-g6p2, which
* fixed the same scope gap on the mutating actions.
*
* Both read paths now filter their result set through
* currentUserCanAccessAcceptance(), matching the pattern the mutating
* actions on the same page use.
*/
class AcceptanceReportFmcsScopeTest extends TestCase
{
private function seedPendingAcceptanceOwnedBy(Company $company): array
{
$asset = Asset::factory()->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());
}
}