diff --git a/app/Http/Requests/EstimatesRequest.php b/app/Http/Requests/EstimatesRequest.php index 3c3dd45b..870e134c 100644 --- a/app/Http/Requests/EstimatesRequest.php +++ b/app/Http/Requests/EstimatesRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Models\CompanySetting; use App\Models\Customer; use App\Models\Estimate; +use App\Support\DocumentTotals; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -119,18 +120,34 @@ class EstimatesRequest extends FormRequest $exchange_rate = $company_currency != $current_currency ? $this->exchange_rate : 1; $currency = Customer::find($this->customer_id)->currency_id; + $tax_per_item = CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO '; + $discount_per_item = CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO'; + + // Recompute totals server-side from the line items (GHSA-8c69). + $totals = DocumentTotals::compute( + $this->items ?? [], + $this->taxes ?? [], + $this->discount_val, + $tax_per_item, + (bool) $this->tax_included, + $discount_per_item + ); + return collect($this->except('items', 'taxes')) ->merge([ 'creator_id' => $this->user()->id ?? null, 'status' => $this->has('estimateSend') ? Estimate::STATUS_SENT : Estimate::STATUS_DRAFT, 'company_id' => $this->header('company'), - 'tax_per_item' => CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO ', - 'discount_per_item' => CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO', + 'tax_per_item' => $tax_per_item, + 'discount_per_item' => $discount_per_item, + 'sub_total' => $totals['sub_total'], + 'total' => $totals['total'], + 'tax' => $totals['tax'], 'exchange_rate' => $exchange_rate, 'base_discount_val' => $this->discount_val * $exchange_rate, - 'base_sub_total' => $this->sub_total * $exchange_rate, - 'base_total' => $this->total * $exchange_rate, - 'base_tax' => $this->tax * $exchange_rate, + 'base_sub_total' => $totals['sub_total'] * $exchange_rate, + 'base_total' => $totals['total'] * $exchange_rate, + 'base_tax' => $totals['tax'] * $exchange_rate, 'currency_id' => $currency, ]) ->toArray(); diff --git a/app/Http/Requests/InvoicesRequest.php b/app/Http/Requests/InvoicesRequest.php index 48c5ac10..561db0f3 100644 --- a/app/Http/Requests/InvoicesRequest.php +++ b/app/Http/Requests/InvoicesRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Models\CompanySetting; use App\Models\Customer; use App\Models\Invoice; +use App\Support\DocumentTotals; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -118,23 +119,41 @@ class InvoicesRequest extends FormRequest $exchange_rate = $company_currency != $current_currency ? $this->exchange_rate : 1; $currency = Customer::find($this->customer_id)->currency_id; + $tax_per_item = CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO '; + $discount_per_item = CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO'; + + // Recompute the document totals server-side from the line items so a + // tampered total/sub_total/tax/due_amount in the request is ignored + // (GHSA-8c69). + $totals = DocumentTotals::compute( + $this->items ?? [], + $this->taxes ?? [], + $this->discount_val, + $tax_per_item, + (bool) $this->tax_included, + $discount_per_item + ); + return collect($this->except('items', 'taxes')) ->merge([ 'creator_id' => $this->user()->id ?? null, 'status' => $this->has('invoiceSend') ? Invoice::STATUS_SENT : Invoice::STATUS_DRAFT, 'paid_status' => Invoice::STATUS_UNPAID, 'company_id' => $this->header('company'), - 'tax_per_item' => CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO ', - 'discount_per_item' => CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO', - 'due_amount' => $this->total, + 'tax_per_item' => $tax_per_item, + 'discount_per_item' => $discount_per_item, + 'sub_total' => $totals['sub_total'], + 'total' => $totals['total'], + 'tax' => $totals['tax'], + 'due_amount' => $totals['total'], 'sent' => (bool) $this->sent ?? false, 'viewed' => (bool) $this->viewed ?? false, 'exchange_rate' => $exchange_rate, - 'base_total' => $this->total * $exchange_rate, + 'base_total' => $totals['total'] * $exchange_rate, 'base_discount_val' => $this->discount_val * $exchange_rate, - 'base_sub_total' => $this->sub_total * $exchange_rate, - 'base_tax' => $this->tax * $exchange_rate, - 'base_due_amount' => $this->total * $exchange_rate, + 'base_sub_total' => $totals['sub_total'] * $exchange_rate, + 'base_tax' => $totals['tax'] * $exchange_rate, + 'base_due_amount' => $totals['total'] * $exchange_rate, 'currency_id' => $currency, ]) ->toArray(); diff --git a/app/Http/Requests/RecurringInvoiceRequest.php b/app/Http/Requests/RecurringInvoiceRequest.php index f9792832..e5cd7d08 100644 --- a/app/Http/Requests/RecurringInvoiceRequest.php +++ b/app/Http/Requests/RecurringInvoiceRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Models\CompanySetting; use App\Models\Customer; use App\Models\RecurringInvoice; +use App\Support\DocumentTotals; use Illuminate\Foundation\Http\FormRequest; class RecurringInvoiceRequest extends FormRequest @@ -106,15 +107,35 @@ class RecurringInvoiceRequest extends FormRequest $nextInvoiceAt = RecurringInvoice::getNextInvoiceDate($this->frequency, $this->starts_at); + $tax_per_item = CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO '; + $discount_per_item = CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO'; + + // Recompute totals server-side from the line items (GHSA-8c69). The + // recurring template totals propagate to every generated invoice. + $totals = DocumentTotals::compute( + $this->items ?? [], + $this->taxes ?? [], + $this->discount_val, + $tax_per_item, + (bool) $this->tax_included, + $discount_per_item + ); + return collect($this->except('items', 'taxes')) ->merge([ 'creator_id' => $this->user()->id, 'company_id' => $this->header('company'), 'next_invoice_at' => $nextInvoiceAt, - 'tax_per_item' => CompanySetting::getSetting('tax_per_item', $this->header('company')) ?? 'NO ', - 'discount_per_item' => CompanySetting::getSetting('discount_per_item', $this->header('company')) ?? 'NO', - 'due_amount' => $this->total, + 'tax_per_item' => $tax_per_item, + 'discount_per_item' => $discount_per_item, + 'sub_total' => $totals['sub_total'], + 'total' => $totals['total'], + 'tax' => $totals['tax'], + 'due_amount' => $totals['total'], 'exchange_rate' => $exchange_rate, + 'base_sub_total' => $totals['sub_total'] * $exchange_rate, + 'base_total' => $totals['total'] * $exchange_rate, + 'base_tax' => $totals['tax'] * $exchange_rate, 'currency_id' => $currency, ]) ->toArray(); diff --git a/app/Models/Estimate.php b/app/Models/Estimate.php index 8ebe1f15..3806859b 100644 --- a/app/Models/Estimate.php +++ b/app/Models/Estimate.php @@ -8,6 +8,7 @@ use App\Facades\PDF; use App\Mail\SendEstimateMail; use App\Services\SerialNumberFormatter; use App\Space\PdfTemplateUtils; +use App\Support\DocumentTotals; use App\Support\SafeOrderBy; use App\Traits\GeneratesPdfTrait; use App\Traits\HasCustomFieldsTrait; @@ -317,6 +318,8 @@ class Estimate extends Model implements HasMedia foreach ($estimateItems as $estimateItem) { $estimateItem['company_id'] = $request->header('company'); $estimateItem['exchange_rate'] = $exchange_rate; + // Recompute the item total from price/quantity (GHSA-8c69). + $estimateItem['total'] = DocumentTotals::itemTotal($estimateItem, $estimate->discount_per_item === 'YES'); $estimateItem['base_price'] = $estimateItem['price'] * $exchange_rate; $estimateItem['base_discount_val'] = $estimateItem['discount_val'] * $exchange_rate; $estimateItem['base_tax'] = $estimate['tax'] * $exchange_rate; diff --git a/app/Models/Invoice.php b/app/Models/Invoice.php index e92f24ec..f8f1c9d6 100644 --- a/app/Models/Invoice.php +++ b/app/Models/Invoice.php @@ -8,6 +8,7 @@ use App\Facades\PDF; use App\Mail\SendInvoiceMail; use App\Services\SerialNumberFormatter; use App\Space\PdfTemplateUtils; +use App\Support\DocumentTotals; use App\Support\SafeOrderBy; use App\Traits\GeneratesPdfTrait; use App\Traits\HasCustomFieldsTrait; @@ -392,12 +393,12 @@ class Invoice extends Model implements HasMedia return 'customer_cannot_be_changed_after_payment_is_added'; } - if ($request->total >= 0 && $request->total < $total_paid_amount) { + if ($data['total'] >= 0 && $data['total'] < $total_paid_amount) { return 'total_invoice_amount_must_be_more_than_paid_amount'; } - if ($oldTotal != $request->total) { - $oldTotal = (int) round($request->total) - (int) $oldTotal; + if ($oldTotal != $data['total']) { + $oldTotal = (int) round($data['total']) - (int) $oldTotal; } else { $oldTotal = 0; } @@ -506,6 +507,9 @@ class Invoice extends Model implements HasMedia foreach ($invoiceItems as $invoiceItem) { $invoiceItem['company_id'] = $invoice->company_id; $invoiceItem['exchange_rate'] = $exchange_rate; + // Recompute the item total from price/quantity so a tampered item + // total can't desync from the recomputed document totals (GHSA-8c69). + $invoiceItem['total'] = DocumentTotals::itemTotal($invoiceItem, $invoice->discount_per_item === 'YES'); $invoiceItem['base_price'] = $invoiceItem['price'] * $exchange_rate; $invoiceItem['base_discount_val'] = $invoiceItem['discount_val'] * $exchange_rate; $invoiceItem['base_tax'] = $invoiceItem['tax'] * $exchange_rate; diff --git a/app/Models/RecurringInvoice.php b/app/Models/RecurringInvoice.php index d44f5e36..0695102f 100644 --- a/app/Models/RecurringInvoice.php +++ b/app/Models/RecurringInvoice.php @@ -5,6 +5,7 @@ namespace App\Models; use App\Facades\Hashids; use App\Http\Requests\RecurringInvoiceRequest; use App\Services\SerialNumberFormatter; +use App\Support\DocumentTotals; use App\Support\SafeOrderBy; use App\Traits\HasCustomFieldsTrait; use Carbon\Carbon; @@ -248,6 +249,8 @@ class RecurringInvoice extends Model { foreach ($invoiceItems as $invoiceItem) { $invoiceItem['company_id'] = $recurringInvoice->company_id; + // Recompute the item total from price/quantity (GHSA-8c69). + $invoiceItem['total'] = DocumentTotals::itemTotal($invoiceItem, $recurringInvoice->discount_per_item === 'YES'); $item = $recurringInvoice->items()->create($invoiceItem); if (array_key_exists('taxes', $invoiceItem) && $invoiceItem['taxes']) { foreach ($invoiceItem['taxes'] as $tax) { diff --git a/app/Support/DocumentTotals.php b/app/Support/DocumentTotals.php new file mode 100644 index 00000000..d626ec01 --- /dev/null +++ b/app/Support/DocumentTotals.php @@ -0,0 +1,70 @@ + $subTotal, + 'tax' => $totalTax, + 'total' => $total, + ]; + } + + /** + * Authoritative per-item total: round(price * quantity) minus the item + * discount (only applied when discount is configured per item). + */ + public static function itemTotal(array $item, bool $perItemDiscount): int + { + $price = (float) ($item['price'] ?? 0); + $quantity = (float) ($item['quantity'] ?? 0); + $discount = $perItemDiscount ? (int) round((float) ($item['discount_val'] ?? 0)) : 0; + + return (int) round($price * $quantity) - $discount; + } + + protected static function sumTaxAmounts(array $taxes): int + { + $sum = 0; + foreach ($taxes as $tax) { + $sum += (int) round((float) ($tax['amount'] ?? 0)); + } + + return $sum; + } +} diff --git a/tests/Feature/Admin/EstimateTest.php b/tests/Feature/Admin/EstimateTest.php index ef42c27d..fb0057ad 100644 --- a/tests/Feature/Admin/EstimateTest.php +++ b/tests/Feature/Admin/EstimateTest.php @@ -57,12 +57,9 @@ test('create estimate', function () { 'estimate_number' => $estimate['estimate_number'], 'discount_type' => $estimate['discount_type'], 'discount_val' => $estimate['discount_val'], - 'sub_total' => $estimate['sub_total'], 'discount' => $estimate['discount'], 'customer_id' => $estimate['customer_id'], - 'total' => $estimate['total'], 'notes' => $estimate['notes'], - 'tax' => $estimate['tax'], ]); }); @@ -115,12 +112,9 @@ test('update estimate', function () { 'estimate_number' => $estimate2['estimate_number'], 'discount_type' => $estimate2['discount_type'], 'discount_val' => $estimate2['discount_val'], - 'sub_total' => $estimate2['sub_total'], 'discount' => $estimate2['discount'], 'customer_id' => $estimate2['customer_id'], - 'total' => $estimate2['total'], 'notes' => $estimate2['notes'], - 'tax' => $estimate2['tax'], ]); $this->assertDatabaseHas('estimate_items', [ @@ -319,12 +313,9 @@ test('create estimate with tax per item', function () { 'estimate_number' => $estimate['estimate_number'], 'discount_type' => $estimate['discount_type'], 'discount_val' => $estimate['discount_val'], - 'sub_total' => $estimate['sub_total'], 'discount' => $estimate['discount'], 'customer_id' => $estimate['customer_id'], - 'total' => $estimate['total'], 'notes' => $estimate['notes'], - 'tax' => $estimate['tax'], ]); $this->assertDatabaseHas('estimate_items', [ @@ -378,12 +369,9 @@ test('create estimate with EUR currency', function () { 'estimate_number' => $estimate['estimate_number'], 'discount_type' => $estimate['discount_type'], 'discount_val' => $estimate['discount_val'], - 'sub_total' => $estimate['sub_total'], 'discount' => $estimate['discount'], 'customer_id' => $estimate['customer_id'], - 'total' => $estimate['total'], 'notes' => $estimate['notes'], - 'tax' => $estimate['tax'], ]); $this->assertDatabaseHas('taxes', [ diff --git a/tests/Feature/Admin/InvoiceTest.php b/tests/Feature/Admin/InvoiceTest.php index 99d9c4dc..42e203a3 100644 --- a/tests/Feature/Admin/InvoiceTest.php +++ b/tests/Feature/Admin/InvoiceTest.php @@ -48,11 +48,7 @@ test('create invoice', function () { $this->assertDatabaseHas('invoices', [ 'template_name' => $invoice['template_name'], 'invoice_number' => $invoice['invoice_number'], - 'sub_total' => $invoice['sub_total'], - 'discount' => $invoice['discount'], 'customer_id' => $invoice['customer_id'], - 'total' => $invoice['total'], - 'tax' => $invoice['tax'], ]); $this->assertDatabaseHas('invoice_items', [ @@ -61,6 +57,44 @@ test('create invoice', function () { ]); }); +test('server recomputes invoice totals and ignores client-supplied amounts', function () { + // Well-formed item (10 x 10000 = 100000) but every client-supplied total is + // tampered to 1 — the server must recompute from price/quantity (GHSA-8c69). + $item = InvoiceItem::factory()->raw([ + 'price' => 10000, + 'quantity' => 10, + 'total' => 1, + 'discount_val' => 0, + 'tax' => 0, + 'taxes' => [], + ]); + + $invoice = Invoice::factory()->raw([ + 'items' => [$item], + 'taxes' => [], + 'discount_val' => 0, + 'tax_included' => false, + 'sub_total' => 1, + 'total' => 1, + 'tax' => 0, + 'due_amount' => 1, + ]); + + postJson('api/v1/invoices', $invoice)->assertOk(); + + $this->assertDatabaseHas('invoices', [ + 'invoice_number' => $invoice['invoice_number'], + 'sub_total' => 100000, + 'total' => 100000, + 'due_amount' => 100000, + ]); + + $this->assertDatabaseHas('invoice_items', [ + 'name' => $item['name'], + 'total' => 100000, + ]); +}); + test('create invoice with negative and zero item quantities', function () { $invoice = Invoice::factory()->raw([ 'items' => [ @@ -77,6 +111,7 @@ test('create invoice with negative and zero item quantities', function () { 'price' => 75, ]), ], + 'discount_val' => 0, 'sub_total' => -150, 'total' => -150, ]); @@ -135,10 +170,6 @@ test('create invoice as sent', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $invoice['invoice_number'], - 'sub_total' => $invoice['sub_total'], - 'total' => $invoice['total'], - 'tax' => $invoice['tax'], - 'discount' => $invoice['discount'], 'customer_id' => $invoice['customer_id'], 'template_name' => $invoice['template_name'], ]); @@ -173,10 +204,6 @@ test('update invoice', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $invoice2['invoice_number'], - 'sub_total' => $invoice2['sub_total'], - 'total' => $invoice2['total'], - 'tax' => $invoice2['tax'], - 'discount' => $invoice2['discount'], 'customer_id' => $invoice2['customer_id'], 'template_name' => $invoice2['template_name'], ]); @@ -332,10 +359,6 @@ test('create invoice with negative tax', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $invoice['invoice_number'], - 'sub_total' => $invoice['sub_total'], - 'total' => $invoice['total'], - 'tax' => $invoice['tax'], - 'discount' => $invoice['discount'], 'customer_id' => $invoice['customer_id'], ]); @@ -368,10 +391,6 @@ test('create invoice with tax per item', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $invoice['invoice_number'], - 'sub_total' => $invoice['sub_total'], - 'total' => $invoice['total'], - 'tax' => $invoice['tax'], - 'discount' => $invoice['discount'], 'customer_id' => $invoice['customer_id'], ]); diff --git a/tests/Unit/DocumentTotalsTest.php b/tests/Unit/DocumentTotalsTest.php new file mode 100644 index 00000000..c1f09358 --- /dev/null +++ b/tests/Unit/DocumentTotalsTest.php @@ -0,0 +1,79 @@ + 10000, 'quantity' => 10, 'total' => 1, 'taxes' => []]], + [], 0, 'NO', false, 'NO' + ); + + expect($totals['sub_total'])->toBe(100000) + ->and($totals['total'])->toBe(100000) + ->and($totals['tax'])->toBe(0); +}); + +test('applies document discount and document-level tax', function () { + $totals = DocumentTotals::compute( + [['price' => 1000, 'quantity' => 2]], + [['amount' => 150]], + 500, 'NO', false, 'NO' + ); + + // 2000 - 500 + 150 + expect($totals['sub_total'])->toBe(2000) + ->and($totals['tax'])->toBe(150) + ->and($totals['total'])->toBe(1650); +}); + +test('uses per-item taxes (not document taxes) when tax_per_item is YES', function () { + $totals = DocumentTotals::compute( + [ + ['price' => 1000, 'quantity' => 1, 'taxes' => [['amount' => 100]]], + ['price' => 2000, 'quantity' => 1, 'taxes' => [['amount' => 200]]], + ], + [['amount' => 9999]], + 0, 'YES', false, 'YES' + ); + + expect($totals['sub_total'])->toBe(3000) + ->and($totals['tax'])->toBe(300) + ->and($totals['total'])->toBe(3300); +}); + +test('excludes tax from total when tax_included', function () { + $totals = DocumentTotals::compute( + [['price' => 1000, 'quantity' => 1]], + [['amount' => 100]], + 0, 'NO', true, 'NO' + ); + + expect($totals['total'])->toBe(1000)->and($totals['tax'])->toBe(100); +}); + +test('applies per-item discount only when discount_per_item is YES', function () { + $with = DocumentTotals::compute( + [['price' => 1000, 'quantity' => 2, 'discount_val' => 300]], + [], 0, 'NO', false, 'YES' + ); + $without = DocumentTotals::compute( + [['price' => 1000, 'quantity' => 2, 'discount_val' => 300]], + [], 0, 'NO', false, 'NO' + ); + + expect($with['sub_total'])->toBe(1700) + ->and($without['sub_total'])->toBe(2000); +}); + +test('supports negative quantities', function () { + $totals = DocumentTotals::compute( + [ + ['price' => 100, 'quantity' => -2], + ['price' => 50, 'quantity' => 1], + ['price' => 75, 'quantity' => 0], + ], + [], 0, 'NO', false, 'NO' + ); + + expect($totals['sub_total'])->toBe(-150)->and($totals['total'])->toBe(-150); +}); diff --git a/tests/Unit/EstimateTest.php b/tests/Unit/EstimateTest.php index 316d9a8b..4d0e1425 100644 --- a/tests/Unit/EstimateTest.php +++ b/tests/Unit/EstimateTest.php @@ -66,12 +66,9 @@ test('create estimate', function () { 'estimate_number' => $estimate['estimate_number'], 'customer_id' => $estimate['customer_id'], 'template_name' => $estimate['template_name'], - 'sub_total' => $estimate['sub_total'], - 'total' => $estimate['total'], 'discount' => $estimate['discount'], 'discount_type' => $estimate['discount_type'], 'discount_val' => $estimate['discount_val'], - 'tax' => $estimate['tax'], 'notes' => $estimate['notes'], ]); }); @@ -114,12 +111,9 @@ test('update estimate', function () { 'estimate_number' => $newEstimate['estimate_number'], 'customer_id' => $newEstimate['customer_id'], 'template_name' => $newEstimate['template_name'], - 'sub_total' => $newEstimate['sub_total'], - 'total' => $newEstimate['total'], 'discount' => $newEstimate['discount'], 'discount_type' => $newEstimate['discount_type'], 'discount_val' => $newEstimate['discount_val'], - 'tax' => $newEstimate['tax'], 'notes' => $newEstimate['notes'], ]); }); diff --git a/tests/Unit/InvoiceTest.php b/tests/Unit/InvoiceTest.php index e5ecb92e..68db6b90 100644 --- a/tests/Unit/InvoiceTest.php +++ b/tests/Unit/InvoiceTest.php @@ -82,9 +82,6 @@ test('create invoice', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $invoice['invoice_number'], - 'sub_total' => $invoice['sub_total'], - 'total' => $invoice['total'], - 'tax' => $invoice['tax'], 'discount' => $invoice['discount'], 'notes' => $invoice['notes'], 'customer_id' => $invoice['customer_id'], @@ -133,9 +130,6 @@ test('update invoice', function () { $this->assertDatabaseHas('invoices', [ 'invoice_number' => $newInvoice['invoice_number'], - 'sub_total' => $newInvoice['sub_total'], - 'total' => $newInvoice['total'], - 'tax' => $newInvoice['tax'], 'discount' => $newInvoice['discount'], 'notes' => $newInvoice['notes'], 'customer_id' => $newInvoice['customer_id'],