From c6a00df120154de2ee0c983bc30c09d70af1ea72 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 12 Jun 2026 11:02:08 +0200 Subject: [PATCH] fix(security): enforce company scope on notes, doc-convert, and member bulk-delete (#668) v3 port of the v2 authorization fixes. - Notes IDOR (GHSA-85wc): NotePolicy checks the note's company_id and NotesController passes the bound model to authorize() on show/update/destroy. - Estimate<->Invoice convert IDOR (GHSA-j2vg): EstimatesController::convertToInvoice and InvoicesController::convertToEstimate authorize 'view' on the source document before creating the target. - Member bulk-delete (GHSA-wxrv): MembersController scopes ids via User::whereCompany() before MemberService::delete. Adds feature tests for cross-company 403s + same-company happy paths. --- .../Company/Estimate/EstimatesController.php | 3 +++ .../Company/General/NotesController.php | 6 ++--- .../Company/Invoice/InvoicesController.php | 3 +++ .../Company/Members/MembersController.php | 11 +++++++- app/Policies/NotePolicy.php | 18 +++++++------ tests/Feature/Admin/EstimateTest.php | 11 ++++++++ tests/Feature/Admin/InvoiceTest.php | 7 +++++ tests/Feature/Admin/MemberTest.php | 21 +++++++++++++++ tests/Feature/Admin/NotesTest.php | 26 +++++++++++++++++++ 9 files changed, 94 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/Company/Estimate/EstimatesController.php b/app/Http/Controllers/Company/Estimate/EstimatesController.php index 5ac5a3f7..34a6b932 100644 --- a/app/Http/Controllers/Company/Estimate/EstimatesController.php +++ b/app/Http/Controllers/Company/Estimate/EstimatesController.php @@ -121,6 +121,9 @@ class EstimatesController extends Controller public function convertToInvoice(Request $request, Estimate $estimate) { + // Authorize access to the source estimate (tenant isolation) in addition + // to the ability to create an invoice. + $this->authorize('view', $estimate); $this->authorize('create', Invoice::class); $invoice = $this->estimateService->convertToInvoice($estimate); diff --git a/app/Http/Controllers/Company/General/NotesController.php b/app/Http/Controllers/Company/General/NotesController.php index 25239a01..3c217eb7 100644 --- a/app/Http/Controllers/Company/General/NotesController.php +++ b/app/Http/Controllers/Company/General/NotesController.php @@ -61,7 +61,7 @@ class NotesController extends Controller */ public function show(Note $note) { - $this->authorize('view notes'); + $this->authorize('view notes', $note); return new NoteResource($note); } @@ -74,7 +74,7 @@ class NotesController extends Controller */ public function update(NotesRequest $request, Note $note) { - $this->authorize('manage notes'); + $this->authorize('manage notes', $note); $note->update($request->getNotesPayload()); @@ -97,7 +97,7 @@ class NotesController extends Controller */ public function destroy(Note $note) { - $this->authorize('manage notes'); + $this->authorize('manage notes', $note); $note->delete(); diff --git a/app/Http/Controllers/Company/Invoice/InvoicesController.php b/app/Http/Controllers/Company/Invoice/InvoicesController.php index abe48b2a..980c7ec2 100644 --- a/app/Http/Controllers/Company/Invoice/InvoicesController.php +++ b/app/Http/Controllers/Company/Invoice/InvoicesController.php @@ -151,6 +151,9 @@ class InvoicesController extends Controller public function convertToEstimate(Request $request, Invoice $invoice) { + // Authorize access to the source invoice (tenant isolation) in addition + // to the ability to create an estimate. + $this->authorize('view', $invoice); $this->authorize('create', Estimate::class); $estimate = $this->invoiceService->convertToEstimate($invoice); diff --git a/app/Http/Controllers/Company/Members/MembersController.php b/app/Http/Controllers/Company/Members/MembersController.php index cb2e3bea..451e70a7 100644 --- a/app/Http/Controllers/Company/Members/MembersController.php +++ b/app/Http/Controllers/Company/Members/MembersController.php @@ -93,7 +93,16 @@ class MembersController extends Controller $this->authorize('delete multiple users', User::class); if ($request->users) { - $this->memberService->delete($request->users); + // Scope the candidate ids to members of the acting company so a user + // from one company cannot delete accounts belonging to another. + $ids = User::whereCompany() + ->whereIn('id', $request->users) + ->pluck('id') + ->toArray(); + + if ($ids) { + $this->memberService->delete($ids); + } } return response()->json([ diff --git a/app/Policies/NotePolicy.php b/app/Policies/NotePolicy.php index d0981186..2eaa00bf 100644 --- a/app/Policies/NotePolicy.php +++ b/app/Policies/NotePolicy.php @@ -11,21 +11,23 @@ class NotePolicy { use HandlesAuthorization; - public function manageNotes(User $user) + public function manageNotes(User $user, ?Note $note = null) { - if (BouncerFacade::can('manage-all-notes', Note::class)) { - return true; + if (! BouncerFacade::can('manage-all-notes', $note ?? Note::class)) { + return false; } - return false; + // When acting on a specific note, enforce tenant isolation: the note + // must belong to a company the user is a member of. + return $note === null || $user->hasCompany($note->company_id); } - public function viewNotes(User $user) + public function viewNotes(User $user, ?Note $note = null) { - if (BouncerFacade::can('view-all-notes', Note::class)) { - return true; + if (! BouncerFacade::can('view-all-notes', $note ?? Note::class)) { + return false; } - return false; + return $note === null || $user->hasCompany($note->company_id); } } diff --git a/tests/Feature/Admin/EstimateTest.php b/tests/Feature/Admin/EstimateTest.php index 14f1f99d..82eb5ab2 100644 --- a/tests/Feature/Admin/EstimateTest.php +++ b/tests/Feature/Admin/EstimateTest.php @@ -5,6 +5,7 @@ use App\Http\Requests\DeleteEstimatesRequest; use App\Http\Requests\EstimatesRequest; use App\Http\Requests\SendEstimatesRequest; use App\Mail\SendEstimateMail; +use App\Models\Company; use App\Models\Estimate; use App\Models\EstimateItem; use App\Models\Tax; @@ -246,6 +247,16 @@ test('create invoice from estimate', function () { $response->assertStatus(200); }); +test('cannot convert an estimate belonging to another company', function () { + $estimate = Estimate::factory()->create([ + 'company_id' => Company::factory()->create()->id, + 'estimate_date' => now(), + 'expiry_date' => now()->addMonth(), + ]); + + postJson("api/v1/estimates/{$estimate->id}/convert-to-invoice")->assertStatus(403); +}); + test('delete multiple estimates using a form request', function () { $this->assertActionUsesFormRequest( EstimatesController::class, diff --git a/tests/Feature/Admin/InvoiceTest.php b/tests/Feature/Admin/InvoiceTest.php index 597df186..d812de53 100644 --- a/tests/Feature/Admin/InvoiceTest.php +++ b/tests/Feature/Admin/InvoiceTest.php @@ -3,6 +3,7 @@ use App\Http\Controllers\Company\Invoice\InvoicesController; use App\Http\Requests\InvoicesRequest; use App\Mail\SendInvoiceMail; +use App\Models\Company; use App\Models\Invoice; use App\Models\InvoiceItem; use App\Models\Tax; @@ -34,6 +35,12 @@ test('testGetInvoices', function () { $response->assertOk(); }); +test('cannot convert an invoice belonging to another company', function () { + $invoice = Invoice::factory()->create(['company_id' => Company::factory()->create()->id]); + + postJson("api/v1/invoices/{$invoice->id}/convert-to-estimate")->assertStatus(403); +}); + test('create invoice', function () { $invoice = Invoice::factory() ->raw([ diff --git a/tests/Feature/Admin/MemberTest.php b/tests/Feature/Admin/MemberTest.php index 10143597..277d9acc 100644 --- a/tests/Feature/Admin/MemberTest.php +++ b/tests/Feature/Admin/MemberTest.php @@ -2,10 +2,12 @@ use App\Http\Controllers\Company\Members\MembersController; use App\Http\Requests\MemberRequest; +use App\Models\Company; use App\Models\User; use Laravel\Sanctum\Sanctum; use function Pest\Laravel\getJson; +use function Pest\Laravel\postJson; beforeEach(function () { Artisan::call('db:seed', ['--class' => 'DatabaseSeeder', '--force' => true]); @@ -48,3 +50,22 @@ test('update member using a form request', function () { MemberRequest::class ); }); + +test('deletes a member belonging to the current company', function () { + $companyId = User::where('role', 'super admin')->first()->companies()->first()->id; + $user = User::factory()->create(); + $user->companies()->attach($companyId); + + postJson('/api/v1/members/delete', ['users' => [$user->id]])->assertOk(); + + $this->assertDatabaseMissing('users', ['id' => $user->id]); +}); + +test('cannot bulk delete a member belonging to another company', function () { + $user = User::factory()->create(); + $user->companies()->attach(Company::factory()->create()->id); + + postJson('/api/v1/members/delete', ['users' => [$user->id]])->assertOk(); + + $this->assertDatabaseHas('users', ['id' => $user->id]); +}); diff --git a/tests/Feature/Admin/NotesTest.php b/tests/Feature/Admin/NotesTest.php index 3e993770..0dd7d481 100644 --- a/tests/Feature/Admin/NotesTest.php +++ b/tests/Feature/Admin/NotesTest.php @@ -1,5 +1,6 @@ assertModelMissing($note); }); + +test('cannot retrieve a note belonging to another company', function () { + $note = Note::factory()->create(['company_id' => Company::factory()->create()->id]); + + getJson("/api/v1/notes/{$note->id}")->assertStatus(403); +}); + +test('cannot update a note belonging to another company', function () { + $note = Note::factory()->create([ + 'company_id' => Company::factory()->create()->id, + 'name' => 'original-name', + ]); + + putJson("/api/v1/notes/{$note->id}", Note::factory()->raw())->assertStatus(403); + + $this->assertDatabaseHas('notes', ['id' => $note->id, 'name' => 'original-name']); +}); + +test('cannot delete a note belonging to another company', function () { + $note = Note::factory()->create(['company_id' => Company::factory()->create()->id]); + + deleteJson("/api/v1/notes/{$note->id}")->assertStatus(403); + + $this->assertModelExists($note); +});