# Alva Requor Store POS — Security, RBAC & Audit Hardening Report

**Date:** 2026-08-13
**Scope:** Full application audit — authorization, roles, sensitive-action gating, audit logging, injection/XSS/CSRF, mass assignment, IDOR, file uploads, error exposure, financial-transaction integrity.
**Method:** Manual inspection of the existing implementation (routes, policies, controllers, services, models, FormRequests, views) cross-referenced against the RBAC seeder and test suite, followed by targeted fixes and new regression tests. This was an audit of what already exists, not a feature-addition pass — findings below are marked **Verified clean**, **Fixed**, or **Recommended (not implemented)**.

---

## 1. Executive Summary

The existing implementation was already built on a disciplined, security-conscious architecture: every financial mutation flows through a single trusted service class wrapped in `DB::transaction()`, every FormRequest validates foreign keys with tenant-scoped `Rule::exists()`, every route-bound model access checks business ownership, and no raw SQL or unescaped output exists anywhere in the codebase. The audit found **three real gaps** (not architectural flaws) and fixed all three:

1. **Discounting had no dedicated permission** — any Cashier could grant an arbitrary discount, gated only by the same `sales.create` permission used for ringing up a normal sale.
2. **Purchasing had no audit trail** — `PurchasingService` (confirm/cancel/payment/supplier-return) never wrote to `audit_logs`, unlike its sales-side equivalent.
3. **The audit trail had no viewer** — `audit-logs.view` has existed as a permission since the RBAC seeder was first written, and entries have been recorded since early phases, but no page ever read them back.

No SQL injection, XSS, CSRF, mass-assignment, or IDOR vulnerabilities were found. Everything else in this report marked "Verified clean" was actively checked, not assumed.

---

## 2. Authorization — Routes, Policies, Gates, Permission Checks

**Verified clean.** Every one of the ~35 controllers' state-changing and record-viewing actions calls `$this->authorize(...)` — either against a registered Policy (`Product`, `Sale`, `Purchase`, `Expense`, `CashierShift`, `User` — registered in `AuthServiceProvider`) or a bare `spatie/laravel-permission` permission string (`'brands.create'`, `'inventory.adjust'`, etc.) for models without a dedicated policy. There is no controller action that mutates data without an authorization check.

Six policies exist (`app/Policies/`): `ProductPolicy`, `SalePolicy`, `PurchasePolicy`, `ExpensePolicy`, `CashierShiftPolicy`, `UserPolicy`. Each `view`/`update`/`cancel`/etc. method checks both the permission **and** `$user->business_id === $model->business_id`, so a policy-gated action can never cross a tenant boundary even if the permission check passes.

**Manual URL entry cannot bypass authorization.** This was tested directly, not inferred: guests are redirected to `/login` from every protected route (verified with a sweep across all 25 report routes, the audit log route, and the dashboard); under-permissioned roles receive `403`, not a redirect or a silently-empty page.

---

## 3. Roles

All four roles seed correctly (`RolesAndPermissionsSeeder`) and were re-verified against `RbacTest.php` plus the new `SecurityAuditTest.php`:

| Role | Scope |
|---|---|
| **Super Admin** | Every permission, including `users.*`, `roles.*`, `settings.manage`, `audit-logs.view` (the four admin-only permissions no other role gets). |
| **Manager** | Every permission *except* the four admin-only ones above — computed as `allPermissions.diff(adminOnlyPermissions)`, so any new permission added to the map is automatically available to Manager without the seeder needing a manual update. |
| **Cashier** | An explicit allow-list: POS access, sale creation/return, customer create/view/pay, product view, expense create/view, own-shift open/close. Notably **excludes** `sales.cancel`, `sales.discount` (new), `products.edit`, `inventory.adjust`, `expenses.delete`. |
| **Storekeeper** | An explicit allow-list: inventory view/adjust/count/receive, purchase view/receive, product/category/brand/unit view, `reports.inventory` only (not `reports.sales/purchases/profit`). Notably **excludes** POS access and `sales.*` entirely. |

No privilege-escalation path exists: `User.$fillable` excludes anything role-related (roles live in Spatie's separate pivot tables, never touched by mass assignment), and `ProfileController::update()` (self-service profile edit) only accepts `name`/`email`/`phone` — a user cannot elevate their own role or reassign themselves to another business via the profile form.

---

## 4. Sensitive Actions

| Action | Gate | Cashier | Storekeeper | Status |
|---|---|---|---|---|
| Price changes | `products.edit` (policy) | ✗ | ✗ | Verified clean |
| **Discounts** | `sales.discount` | ✗ | n/a | **Fixed** — previously ungated (see §7.1) |
| Stock adjustments | `inventory.adjust` | ✗ | ✓ (their job) | Verified clean |
| Sale cancellation | `sales.cancel` (policy) | ✗ | n/a | Verified clean |
| Refunds / returns | `sales.return` (policy) | ✓ (routine retail duty) | ✗ | Verified clean — see note below |
| Expense deletion/cancellation | `expenses.delete` (policy) | ✗ | n/a | Verified clean |
| User management | `users.*` (admin-only) | ✗ | ✗ | Verified clean, see §9.1 |
| Role management | `roles.*` (admin-only) | ✗ | ✗ | Verified clean, see §9.1 |
| System settings | `settings.manage` (admin-only) | ✗ | ✗ | Verified clean, see §9.1 |

**Note on refunds:** Cashier retaining `sales.return` was a deliberate decision from an earlier phase (routine retail returns are normal cashier duty in most stores), not an oversight — Storekeeper, who has no customer-facing role, correctly lacks it entirely. This was left as-is rather than "fixed," since narrowing it would be a behavior change outside this audit's mandate.

### 4.1 Fixed: `sales.discount`

Discount-granting was previously bundled into `sales.create`, meaning **any Cashier could apply an arbitrary discount** (up to the line/invoice subtotal) with no additional authorization. Added a dedicated `sales.discount` permission, granted to Manager and Super Admin, withheld from Cashier and Storekeeper. Enforced in `SalesService::completeSale()`:

```php
if (bccomp($discountAmount, '0', 2) === 1 && $cashier->cannot('sales.discount')) {
    throw new InvalidArgumentException('You are not authorized to apply a discount to this sale.');
}
```

Checked in the service (not the controller or a FormRequest) because it depends on the *computed* discount total, not raw request input — the exact same reasoning already used for the pre-existing `sales.credit` check three lines below it in the same method. A sale with zero discount never triggers the check, so this doesn't affect the (much more common) no-discount checkout path at all.

---

## 5. Audit Logs

`AuditLogService::log()` writes `business_id`, `branch_id`, `user_id`, `action`, `module`, `description`, `old_values`/`new_values` (JSON), `ip_address`, and `user_agent` for every call site. Coverage by required category:

| Category | Coverage |
|---|---|
| Authentication | Login, logout (`LoginController`) |
| Product changes | Create, update, delete (`ProductController`) |
| Price changes | A **distinct** `price_changed` action, separate from the generic `updated` entry, fired only when `purchase_price` or `selling_price` actually changed (`ProductController::update()`) |
| Inventory changes | Captured via the immutable `inventory_transactions` ledger (type, quantity, `stock_before`/`stock_after`, user, reference) rather than the generic audit log — see note below |
| Sales | Created, cancelled (`SalesService`) |
| Refunds / returns | `returned` action, includes refund method and amount in the description (`SalesService::recordSaleReturn()`) |
| Expenses | Create, update, cancel (`ExpenseController`) |
| **Purchases** | **Fixed** — was entirely unlogged; now confirmed/cancelled/payment/supplier-return all logged (see §5.1) |
| Permissions | No code path currently changes a user's role/permissions at runtime (see §9.1) — nothing to log yet |

**Note on inventory changes:** `InventoryService` deliberately does not call `AuditLogService` — every stock mutation (opening stock, receiving, sale, return, adjustment, damage, expiry write-off, stock-count variance) already writes an immutable `InventoryTransaction` row with `user_id`, exact `stock_before`/`stock_after`, `batch_number`, `reference_type`/`reference_id`, and free-text notes. This is a *more* precise audit trail for that domain than a generic log entry would be, and was a deliberate architectural choice from the module's original design, not a gap. It is queryable today via **Reports → Inventory → Stock Movement**.

### 5.1 Fixed: Purchasing had no audit trail

`PurchasingService` was already correct on transactional integrity (every mutating method wrapped in `DB::transaction()`, matching `SalesService`'s rigor) but never wrote to `audit_logs` — a purchase could be confirmed, cancelled, paid against, or returned to a supplier with zero trace outside the domain tables. Added logging to all four methods, mirroring the exact pattern and `action` naming already used by `SalesService` (`confirmed`, `cancelled`, `payment_made`, `returned`).

### 5.2 Fixed: The audit trail had no viewer

`audit-logs.view` has existed as an admin-only permission since the RBAC seeder was first written; `AuditLogService` has been writing entries since the authentication module was built. But **no route or controller ever read the `audit_logs` table back** — the infrastructure to *record* accountability existed without any way to *exercise* it. Added a minimal `AuditLogController@index` (filterable by module/action/user/date range, business-scoped, paginated) at `/audit-logs`, gated by `audit-logs.view` — Super Admin only, matching the existing permission scope (Manager does not get this permission, by original design).

---

## 6. Security Hygiene

| Check | Finding |
|---|---|
| **SQL injection** | Verified clean. Every `whereRaw`/`selectRaw`/`orderByRaw`/`DB::raw` call site in the app (7 total) uses a hardcoded literal expression (`COUNT(*)`, `CASE WHEN ... THEN ... END`, `expiry_date IS NULL, expiry_date ASC`) — none interpolate request-controlled input into raw SQL. The reporting module (built in an earlier phase) deliberately avoids SQL aggregation entirely in favor of PHP-side `bcmath` folding, for financial-precision reasons unrelated to injection risk, which incidentally makes it doubly safe. |
| **XSS** | Verified clean. Zero instances of Blade's unescaped `{!! !!}` output anywhere in `resources/views`. All user-supplied data (product names, descriptions, audit log JSON payloads, etc.) renders through `{{ }}`, which HTML-escapes by default. |
| **CSRF** | Verified clean. `VerifyCsrfToken::$except` is empty — every POST/PUT/PATCH/DELETE route requires a valid token, with zero exceptions carved out. |
| **Mass assignment** | Verified clean. Every model defines `$fillable` explicitly (no `$guarded = []` anywhere). Grepped the entire `app/Http/Controllers` tree for `::create($request->all())` / `->update($request->all())` — zero matches. Every create/update path uses a FormRequest's `->validated()` array or an explicitly-constructed array. |
| **IDOR** | Verified clean. Every controller action that receives a route-bound model checks `$model->business_id === $user->business_id`, either via a Policy method or an explicit `abort_unless(...)` (32 call sites checked). Every FormRequest that validates a foreign key (`branch_id`, `product_id`, `customer_id`, `supplier_id`, `category_id`, `payment_method_id`, etc. — 31 call sites across every Store/Update request) scopes the existence check with `Rule::exists(...)->where('business_id', $businessId)`, so submitting another business's ID in a form fails validation before it ever reaches a service. |
| **File uploads** | Verified clean. Product images (`StoreProductRequest`/`UpdateProductRequest`) and expense receipts (`StoreExpenseRequest`/`UpdateExpenseRequest`) both whitelist MIME types (`jpg,jpeg,png,webp` / `+pdf` for receipts) and cap size (2MB / 4MB). Laravel's `->store()` generates randomized 40-character filenames — uploaded files are not sequentially guessable. **Minor recommendation (not implemented):** expense receipts are stored on the `public` disk like product images; since receipts can contain vendor/payment details, consider serving them through an authenticated route instead of a public URL, even though the randomized filename already makes them impractical to enumerate. |
| **Unsafe redirects** | Verified clean. Zero instances of `redirect()->to($request->...)` or `->away(...)` anywhere in the app — no code path redirects to a user-supplied URL. |
| **Sensitive error messages** | Verified clean in application code: zero broad `catch (\Throwable)` / `catch (\Exception)` blocks anywhere in `app/Http/Controllers` — every catch targets a specific, safe-to-display domain exception (`InvalidArgumentException`, `InsufficientStockException`, `ExpiredStockException`). **Environment-level finding:** the local `.env` has `APP_DEBUG=true` (appropriate for local development — left untouched). `.env.example` now carries an explicit warning comment that this **must** be `false` in production, since Laravel's default behavior with debug mode on renders full stack traces, file paths, and raw `QueryException` messages (including SQL) to the browser. |
| **Missing validation** | Verified clean. Every controller accepting POST/PUT data does so through a dedicated FormRequest class or an explicit inline `$request->validate([...])` call (`StockCountController`) — no controller reads unvalidated input into a create/update call. |

---

## 7. Financial Integrity

**Verified clean.** Every service that mutates money or stock wraps its writes in `DB::transaction()`:

- `InventoryService` — 6 transaction boundaries (opening stock, receiving, FEFO sale, batch return, batch adjustment, and the core `applyTransaction()` write path all lock the relevant `product_stocks`/`product_batches` row with `lockForUpdate()` before validating and writing).
- `SalesService` — 5 boundaries (`completeSale`, `cancelSale`, `recordSaleReturn`, `recordPayment`, `adjustCustomerBalance`).
- `PurchasingService` — 5 boundaries (`confirmPurchase`, `cancelPurchase`, `recordPayment`, `recordSupplierReturn`, `adjustSupplierBalance`).
- `CashierShiftService` — 4 boundaries (open, movement, close, approve).
- `StockCountController::approve()` wraps its entire per-item variance loop **and** the count's status update in one outer transaction, so a mid-loop failure can never leave some items applied and the count still marked in-progress.

No financial mutation path was found that writes multiple related rows (e.g., a ledger entry and a balance update) outside a transaction boundary. Concurrent-write safety is enforced via `lockForUpdate()` on the balance/stock row being modified, not merely by the transaction boundary alone — re-verified against the existing `test_concurrent_sales_cannot_oversell_the_same_product` and `test_sequential_sales_never_allow_total_sold_to_exceed_available_stock` tests.

---

## 8. Fixes Implemented

| # | Fix | Files |
|---|---|---|
| 1 | Added `sales.discount` permission; enforced in `SalesService::completeSale()`; granted to Manager/Super Admin only | `RolesAndPermissionsSeeder.php`, `SalesService.php` |
| 2 | Added audit logging to `PurchasingService` (confirm, cancel, payment, supplier return) | `PurchasingService.php` |
| 3 | Built `AuditLogController` + filterable, business-scoped view, gated by `audit-logs.view` | `AuditLogController.php`, `audit-logs/index.blade.php`, `routes/web.php`, nav link |
| 4 | Added a production-debug warning comment to `.env.example` | `.env.example` |
| 5 | Fixed three existing tests that exercised the now-gated discount behavior with a Cashier actor (granted the permission explicitly where the test's intent was discount math, not authorization) | `SalesServiceTest.php`, `FefoSaleTest.php`, `SalesReportServiceTest.php` |

## 9. Recommendations Not Implemented

Per the audit's explicit instruction to inspect rather than expand the implementation, the following gaps were identified but deliberately **not built**, since doing so would be adding new features rather than hardening existing ones:

### 9.1 User management, role management, and system settings have no UI

`UserController` currently exposes only a read-only `index()` — there is no create/edit/deactivate flow, despite `UserPolicy` already defining `create`/`update`/`delete` methods (unused). There is no `RoleController` or `SettingController` at all, despite `roles.*` and `settings.manage` permissions being fully seeded and admin-scoped. Since none of these actions exist as reachable routes, there is currently nothing to exploit — but they represent incomplete functionality relative to what the permission structure anticipates. Building them is a legitimate follow-up phase, not a same-pass hardening fix.

### 9.2 Expense receipts on the public disk

Low-severity; noted in §6. The randomized filename already makes casual enumeration impractical, but an authenticated-route alternative would be strictly better for a document that can contain vendor/payment details.

### 9.3 POS discount field is not conditionally hidden for Cashiers

The backend now correctly rejects a Cashier-applied discount (§4.1), but the POS screen's discount input is not conditionally hidden based on the `sales.discount` permission — a Cashier can still type a discount and submit, and will receive a clear server-side rejection rather than the field simply being absent. This is a UX polish item, not a security gap (the enforcement is server-side and cannot be bypassed from the client), left out to avoid an unrequested POS UI change in a security-focused pass.

---

## 10. New Test Coverage

`tests/Feature/SecurityAuditTest.php` (15 tests) plus 4 new tests in `SalesServiceTest.php`:

- Guest redirected from every report route, the audit log route, and the reports hub.
- Cashier forbidden from every report route and the audit log route; confirmed access to the (permission-gated-per-link) reports hub itself.
- Storekeeper confirmed limited to `reports.inventory.*` only.
- Manager confirmed access to every report except the audit log.
- Only Super Admin can reach the audit log; confirmed business-tenant isolation (another business's log entry never appears).
- `sales.discount`: Cashier rejected on both line- and invoice-level discounts (via the real `/sales` HTTP endpoint, not just the service layer); a zero discount never requires the permission; Manager succeeds.
- All four new `PurchasingService` audit log entries (confirm, cancel, payment, supplier return) verified present in `audit_logs`.

**Full suite: 446 tests passing, zero regressions** (427 pre-existing + 19 new). Verified against the real MySQL/MariaDB database via `migrate:fresh --seed`, plus a manual HTTP smoke test of the audit log viewer's permission gate and the reports hub's role-aware visibility.