mirror of
https://github.com/snipe/snipe-it.git
synced 2026-08-18 11:15:42 +00:00
Fixed FD-56821 - better scoping on acceptance report
This commit is contained in:
@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
127
tests/Feature/Reporting/AcceptanceReportFmcsScopeTest.php
Normal file
127
tests/Feature/Reporting/AcceptanceReportFmcsScopeTest.php
Normal 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());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user