From e432e4e62f5aa52238e3ff3fc37a0c78588eec47 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 12 Jun 2026 09:17:52 +0200 Subject: [PATCH] fix(security): enforce company scope on notes, estimate-convert, and user bulk-delete (#661) - Notes IDOR (GHSA-85wc): NotePolicy::viewNotes/manageNotes now receive the Note and require hasCompany($note->company_id); NotesController passes the bound model to authorize() on show/update/destroy. - Estimate->Invoice IDOR (GHSA-j2vg): ConvertEstimateController authorizes 'view' on the source estimate before creating the invoice. - User bulk-delete (GHSA-wxrv): UsersController scopes candidate ids via User::whereCompany() before deletion so cross-company accounts are protected. Adds feature tests for cross-company 403s plus same-company happy paths. --- .../Estimate/ConvertEstimateController.php | 4 +++ .../V1/Admin/General/NotesController.php | 6 ++--- .../V1/Admin/Users/UsersController.php | 11 +++++++- app/Policies/NotePolicy.php | 18 +++++++------ tests/Feature/Admin/EstimateTest.php | 11 ++++++++ tests/Feature/Admin/NotesTest.php | 26 +++++++++++++++++++ tests/Feature/Admin/UserTest.php | 24 ++++++++++++----- 7 files changed, 81 insertions(+), 19 deletions(-) diff --git a/app/Http/Controllers/V1/Admin/Estimate/ConvertEstimateController.php b/app/Http/Controllers/V1/Admin/Estimate/ConvertEstimateController.php index 95b10dda..331fd79d 100644 --- a/app/Http/Controllers/V1/Admin/Estimate/ConvertEstimateController.php +++ b/app/Http/Controllers/V1/Admin/Estimate/ConvertEstimateController.php @@ -23,6 +23,10 @@ class ConvertEstimateController extends Controller */ public function __invoke(Request $request, Estimate $estimate, Invoice $invoice) { + // Authorize access to the source estimate (tenant isolation) in addition + // to the ability to create an invoice — otherwise any estimate id from + // another company could be converted and disclosed. + $this->authorize('view', $estimate); $this->authorize('create', Invoice::class); $estimate->load(['items', 'items.taxes', 'customer', 'taxes']); diff --git a/app/Http/Controllers/V1/Admin/General/NotesController.php b/app/Http/Controllers/V1/Admin/General/NotesController.php index 5cb0a2e8..0d7878e9 100644 --- a/app/Http/Controllers/V1/Admin/General/NotesController.php +++ b/app/Http/Controllers/V1/Admin/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/V1/Admin/Users/UsersController.php b/app/Http/Controllers/V1/Admin/Users/UsersController.php index 03eb3628..119321c8 100644 --- a/app/Http/Controllers/V1/Admin/Users/UsersController.php +++ b/app/Http/Controllers/V1/Admin/Users/UsersController.php @@ -90,7 +90,16 @@ class UsersController extends Controller $this->authorize('delete multiple users', User::class); if ($request->users) { - User::deleteUsers($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) { + User::deleteUsers($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 96bee25d..ef42c27d 100644 --- a/tests/Feature/Admin/EstimateTest.php +++ b/tests/Feature/Admin/EstimateTest.php @@ -6,6 +6,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; @@ -247,6 +248,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/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); +}); diff --git a/tests/Feature/Admin/UserTest.php b/tests/Feature/Admin/UserTest.php index 0e322f10..ac5e6639 100644 --- a/tests/Feature/Admin/UserTest.php +++ b/tests/Feature/Admin/UserTest.php @@ -2,6 +2,7 @@ use App\Http\Controllers\V1\Admin\Users\UsersController; use App\Http\Requests\UserRequest; +use App\Models\Company; use App\Models\User; use Laravel\Sanctum\Sanctum; @@ -86,12 +87,21 @@ test('update user using a form request', function () { // ]); // }); -// test('delete users', function () { -// $user = User::factory()->create(); -// $data['users'] = [$user->id]; +test('deletes a user 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/users/delete", $data) -// ->assertOk(); + postJson('/api/v1/users/delete', ['users' => [$user->id]])->assertOk(); -// $this->assertModelMissing($user); -// }); + $this->assertDatabaseMissing('users', ['id' => $user->id]); +}); + +test('cannot bulk delete a user belonging to another company', function () { + $user = User::factory()->create(); + $user->companies()->attach(Company::factory()->create()->id); + + postJson('/api/v1/users/delete', ['users' => [$user->id]])->assertOk(); + + $this->assertDatabaseHas('users', ['id' => $user->id]); +});