From defbfc6406d9b1a7b9c86ac66e42194a17510193 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 3 Apr 2026 13:56:34 +0200 Subject: [PATCH 1/4] Fix CustomerPolicy missing hasCompany() check (IDOR) (#604) * Fix CustomerPolicy missing hasCompany() check (cross-company IDOR) Add $user->hasCompany($customer->company_id) check to view, update, delete, restore, and forceDelete methods in CustomerPolicy, matching the pattern used by all other policies (InvoicePolicy, PaymentPolicy, EstimatePolicy, etc.). Without this check, a user in Company A with view-customer ability could access customers belonging to Company B by providing the target customer's ID. Add cross-company authorization tests to verify the fix. Closes #565 * Scope bulk delete to current company to prevent cross-company deletion Filter customer IDs through whereCompany() before passing to deleteCustomers(), ensuring users cannot delete customers belonging to other companies via the bulk delete endpoint. --- .../V1/Admin/Customer/CustomersController.php | 6 ++- app/Policies/CustomerPolicy.php | 10 ++--- tests/Feature/Admin/CustomerTest.php | 38 +++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/V1/Admin/Customer/CustomersController.php b/app/Http/Controllers/V1/Admin/Customer/CustomersController.php index d0a73186..3dbe5846 100644 --- a/app/Http/Controllers/V1/Admin/Customer/CustomersController.php +++ b/app/Http/Controllers/V1/Admin/Customer/CustomersController.php @@ -92,7 +92,11 @@ class CustomersController extends Controller { $this->authorize('delete multiple customers'); - Customer::deleteCustomers($request->ids); + $ids = Customer::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Customer::deleteCustomers($ids); return response()->json([ 'success' => true, diff --git a/app/Policies/CustomerPolicy.php b/app/Policies/CustomerPolicy.php index aa2feccb..02ad2fa3 100644 --- a/app/Policies/CustomerPolicy.php +++ b/app/Policies/CustomerPolicy.php @@ -32,7 +32,7 @@ class CustomerPolicy */ public function view(User $user, Customer $customer): bool { - if (BouncerFacade::can('view-customer', $customer)) { + if (BouncerFacade::can('view-customer', $customer) && $user->hasCompany($customer->company_id)) { return true; } @@ -60,7 +60,7 @@ class CustomerPolicy */ public function update(User $user, Customer $customer): bool { - if (BouncerFacade::can('edit-customer', $customer)) { + if (BouncerFacade::can('edit-customer', $customer) && $user->hasCompany($customer->company_id)) { return true; } @@ -74,7 +74,7 @@ class CustomerPolicy */ public function delete(User $user, Customer $customer): bool { - if (BouncerFacade::can('delete-customer', $customer)) { + if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) { return true; } @@ -88,7 +88,7 @@ class CustomerPolicy */ public function restore(User $user, Customer $customer): bool { - if (BouncerFacade::can('delete-customer', $customer)) { + if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) { return true; } @@ -102,7 +102,7 @@ class CustomerPolicy */ public function forceDelete(User $user, Customer $customer): bool { - if (BouncerFacade::can('delete-customer', $customer)) { + if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) { return true; } diff --git a/tests/Feature/Admin/CustomerTest.php b/tests/Feature/Admin/CustomerTest.php index 33f3ec09..731a4e34 100644 --- a/tests/Feature/Admin/CustomerTest.php +++ b/tests/Feature/Admin/CustomerTest.php @@ -2,6 +2,7 @@ use App\Http\Controllers\V1\Admin\Customer\CustomersController; use App\Http\Requests\CustomerRequest; +use App\Models\Company; use App\Models\Customer; use App\Models\Invoice; use App\Models\User; @@ -157,3 +158,40 @@ test('delete multiple customer', function () { 'success' => true, ]); }); + +test('cannot view customer from another company', function () { + $otherCompany = Company::factory()->create(); + $otherCustomer = Customer::factory()->create([ + 'company_id' => $otherCompany->id, + ]); + + getJson("api/v1/customers/{$otherCustomer->id}") + ->assertForbidden(); +}); + +test('cannot update customer from another company', function () { + $otherCompany = Company::factory()->create(); + $otherCustomer = Customer::factory()->create([ + 'company_id' => $otherCompany->id, + ]); + + putJson("api/v1/customers/{$otherCustomer->id}", [ + 'name' => 'Hacked Name', + 'email' => 'hacked@example.com', + ])->assertForbidden(); +}); + +test('cannot bulk delete customer from another company', function () { + $otherCompany = Company::factory()->create(); + $otherCustomer = Customer::factory()->create([ + 'company_id' => $otherCompany->id, + ]); + + postJson('api/v1/customers/delete', [ + 'ids' => [$otherCustomer->id], + ])->assertOk(); + + $this->assertDatabaseHas('customers', [ + 'id' => $otherCustomer->id, + ]); +}); From 1adebe85b9e316be3efd5b6ebcbb63bab4fcaf1f Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:16:42 +0200 Subject: [PATCH 2/4] Scope all bulk deletes to current company and fix inverted ownership transfer (#605) Bulk delete: filter IDs through whereCompany() before deleting in all controllers (Invoices, Payments, Items, Expenses, Estimates, Recurring Invoices). Previously, any user could delete records from other companies by providing cross-company IDs. Transfer ownership: fix inverted hasCompany() check that allowed transferring company ownership to users who do NOT belong to the company, while blocking users who DO belong. Ref #567 --- .../Controllers/V1/Admin/Company/CompaniesController.php | 4 ++-- .../Controllers/V1/Admin/Estimate/EstimatesController.php | 6 +++++- .../Controllers/V1/Admin/Expense/ExpensesController.php | 6 +++++- .../Controllers/V1/Admin/Invoice/InvoicesController.php | 6 +++++- app/Http/Controllers/V1/Admin/Item/ItemsController.php | 6 +++++- .../Controllers/V1/Admin/Payment/PaymentsController.php | 6 +++++- .../Admin/RecurringInvoice/RecurringInvoiceController.php | 6 +++++- 7 files changed, 32 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/V1/Admin/Company/CompaniesController.php b/app/Http/Controllers/V1/Admin/Company/CompaniesController.php index 05163008..77964d5d 100644 --- a/app/Http/Controllers/V1/Admin/Company/CompaniesController.php +++ b/app/Http/Controllers/V1/Admin/Company/CompaniesController.php @@ -61,10 +61,10 @@ class CompaniesController extends Controller $company = Company::find($request->header('company')); $this->authorize('transfer company ownership', $company); - if ($user->hasCompany($company->id)) { + if (! $user->hasCompany($company->id)) { return response()->json([ 'success' => false, - 'message' => 'User does not belongs to this company.', + 'message' => 'User does not belong to this company.', ]); } diff --git a/app/Http/Controllers/V1/Admin/Estimate/EstimatesController.php b/app/Http/Controllers/V1/Admin/Estimate/EstimatesController.php index 31a415da..fabd2a17 100644 --- a/app/Http/Controllers/V1/Admin/Estimate/EstimatesController.php +++ b/app/Http/Controllers/V1/Admin/Estimate/EstimatesController.php @@ -68,7 +68,11 @@ class EstimatesController extends Controller { $this->authorize('delete multiple estimates'); - Estimate::destroy($request->ids); + $ids = Estimate::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Estimate::destroy($ids); return response()->json([ 'success' => true, diff --git a/app/Http/Controllers/V1/Admin/Expense/ExpensesController.php b/app/Http/Controllers/V1/Admin/Expense/ExpensesController.php index dd42f123..c62e8c1d 100644 --- a/app/Http/Controllers/V1/Admin/Expense/ExpensesController.php +++ b/app/Http/Controllers/V1/Admin/Expense/ExpensesController.php @@ -81,7 +81,11 @@ class ExpensesController extends Controller { $this->authorize('delete multiple expenses'); - Expense::destroy($request->ids); + $ids = Expense::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Expense::destroy($ids); return response()->json([ 'success' => true, diff --git a/app/Http/Controllers/V1/Admin/Invoice/InvoicesController.php b/app/Http/Controllers/V1/Admin/Invoice/InvoicesController.php index 9c3c1629..630d4736 100644 --- a/app/Http/Controllers/V1/Admin/Invoice/InvoicesController.php +++ b/app/Http/Controllers/V1/Admin/Invoice/InvoicesController.php @@ -100,7 +100,11 @@ class InvoicesController extends Controller { $this->authorize('delete multiple invoices'); - Invoice::deleteInvoices($request->ids); + $ids = Invoice::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Invoice::deleteInvoices($ids); return response()->json([ 'success' => true, diff --git a/app/Http/Controllers/V1/Admin/Item/ItemsController.php b/app/Http/Controllers/V1/Admin/Item/ItemsController.php index 93e21bec..7c63a61a 100644 --- a/app/Http/Controllers/V1/Admin/Item/ItemsController.php +++ b/app/Http/Controllers/V1/Admin/Item/ItemsController.php @@ -90,7 +90,11 @@ class ItemsController extends Controller { $this->authorize('delete multiple items'); - Item::destroy($request->ids); + $ids = Item::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Item::destroy($ids); return response()->json([ 'success' => true, diff --git a/app/Http/Controllers/V1/Admin/Payment/PaymentsController.php b/app/Http/Controllers/V1/Admin/Payment/PaymentsController.php index 6f7c2dc3..ee6d0ff7 100644 --- a/app/Http/Controllers/V1/Admin/Payment/PaymentsController.php +++ b/app/Http/Controllers/V1/Admin/Payment/PaymentsController.php @@ -73,7 +73,11 @@ class PaymentsController extends Controller { $this->authorize('delete multiple payments'); - Payment::deletePayments($request->ids); + $ids = Payment::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + Payment::deletePayments($ids); return response()->json([ 'success' => true, diff --git a/app/Http/Controllers/V1/Admin/RecurringInvoice/RecurringInvoiceController.php b/app/Http/Controllers/V1/Admin/RecurringInvoice/RecurringInvoiceController.php index b859392c..e3becd7b 100644 --- a/app/Http/Controllers/V1/Admin/RecurringInvoice/RecurringInvoiceController.php +++ b/app/Http/Controllers/V1/Admin/RecurringInvoice/RecurringInvoiceController.php @@ -84,7 +84,11 @@ class RecurringInvoiceController extends Controller { $this->authorize('delete multiple recurring invoices'); - RecurringInvoice::deleteRecurringInvoice($request->ids); + $ids = RecurringInvoice::whereCompany() + ->whereIn('id', $request->ids) + ->pluck('id'); + + RecurringInvoice::deleteRecurringInvoice($ids); return response()->json([ 'success' => true, From 3d871604aef5b02ac8af6dada0aaee81d7248cd2 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:32:12 +0200 Subject: [PATCH 3/4] Add company ownership check to clone endpoints (#606) Verify the source record belongs to the current company before cloning. Previously, users could clone invoices/estimates from other companies, leaking sensitive data (amounts, customer details, items, taxes, notes). The view policy already includes hasCompany() check, so authorizing view on the source record gates both ability and company ownership. Ref #574 --- .../Controllers/V1/Admin/Estimate/CloneEstimateController.php | 1 + app/Http/Controllers/V1/Admin/Invoice/CloneInvoiceController.php | 1 + 2 files changed, 2 insertions(+) diff --git a/app/Http/Controllers/V1/Admin/Estimate/CloneEstimateController.php b/app/Http/Controllers/V1/Admin/Estimate/CloneEstimateController.php index 546b4fcf..64a57f64 100644 --- a/app/Http/Controllers/V1/Admin/Estimate/CloneEstimateController.php +++ b/app/Http/Controllers/V1/Admin/Estimate/CloneEstimateController.php @@ -21,6 +21,7 @@ class CloneEstimateController extends Controller */ public function __invoke(Request $request, Estimate $estimate) { + $this->authorize('view', $estimate); $this->authorize('create', Estimate::class); $date = Carbon::now(); diff --git a/app/Http/Controllers/V1/Admin/Invoice/CloneInvoiceController.php b/app/Http/Controllers/V1/Admin/Invoice/CloneInvoiceController.php index 77596d64..e3775781 100644 --- a/app/Http/Controllers/V1/Admin/Invoice/CloneInvoiceController.php +++ b/app/Http/Controllers/V1/Admin/Invoice/CloneInvoiceController.php @@ -21,6 +21,7 @@ class CloneInvoiceController extends Controller */ public function __invoke(Request $request, Invoice $invoice) { + $this->authorize('view', $invoice); $this->authorize('create', Invoice::class); $date = Carbon::now(); From 7d9fdb79cccc5fefb084c832bdac046a8e32d31a Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:34:33 +0200 Subject: [PATCH 4/4] Scope users listing and search to current company (#607) Add scopeWhereCompany() to User model using whereHas through the user_company pivot table. Apply it in UsersController::index() and SearchController so users only see members of their current company. Previously, the users page showed ALL users across all companies. Ref #574 --- app/Http/Controllers/V1/Admin/General/SearchController.php | 3 ++- app/Http/Controllers/V1/Admin/Users/UsersController.php | 5 +++-- app/Models/User.php | 7 +++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/V1/Admin/General/SearchController.php b/app/Http/Controllers/V1/Admin/General/SearchController.php index f5179e44..a81b5c89 100644 --- a/app/Http/Controllers/V1/Admin/General/SearchController.php +++ b/app/Http/Controllers/V1/Admin/General/SearchController.php @@ -25,7 +25,8 @@ class SearchController extends Controller ->paginate(10); if ($user->isOwner()) { - $users = User::applyFilters($request->only(['search'])) + $users = User::whereCompany() + ->applyFilters($request->only(['search'])) ->latest() ->paginate(10); } diff --git a/app/Http/Controllers/V1/Admin/Users/UsersController.php b/app/Http/Controllers/V1/Admin/Users/UsersController.php index bf0df019..03eb3628 100644 --- a/app/Http/Controllers/V1/Admin/Users/UsersController.php +++ b/app/Http/Controllers/V1/Admin/Users/UsersController.php @@ -25,14 +25,15 @@ class UsersController extends Controller $user = $request->user(); - $users = User::applyFilters($request->all()) + $users = User::whereCompany() + ->applyFilters($request->all()) ->where('id', '<>', $user->id) ->latest() ->paginate($limit); return UserResource::collection($users) ->additional(['meta' => [ - 'user_total_count' => User::count(), + 'user_total_count' => User::whereCompany()->count(), ]]); } diff --git a/app/Models/User.php b/app/Models/User.php index b5c575cc..c778ab93 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -213,6 +213,13 @@ class User extends Authenticatable implements HasMedia return $query->where('email', 'LIKE', '%'.$email.'%'); } + public function scopeWhereCompany($query) + { + return $query->whereHas('companies', function ($q) { + $q->where('company_id', request()->header('company')); + }); + } + public function scopePaginateData($query, $limit) { if ($limit == 'all') {