From f812555528644b5c0f0d2d38932d0d9e901c4fa6 Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Sat, 18 Jul 2026 00:26:00 +0530 Subject: [PATCH] =?UTF-8?q?fix(sec/d24):=20money-integrity=20guards=20?= =?UTF-8?q?=E2=80=94=20CN=20cap,=20atomic=20doc-no,=20qty,=20TDS,=20markSt?= =?UTF-8?q?atus?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red-team money findings: - createCreditNote caps the CN to the invoice's remaining creditable value (invoice payable − already-credited by non-cancelled CNs), read+insert in one txn. Blocks a CN exceeding its invoice / unlimited CNs flipping it 'paid'. - nextDocNo allocates via one atomic UPDATE ... RETURNING (row lock) instead of read-then-bump — no duplicate/burned numbers under concurrent issue (Postgres). - markStatus rejects accepted/lost on non-QUOTATION docs — an issued invoice can no longer be hidden from overdue chasing via a quote-only transition. - computeBill bounds qty magnitude (keeps fractional for weighed Store goods); HQ buildLines additionally requires whole-unit integer qty (SMS/module seats). - recordPayment rejects TDS greater than the cash received — no fabricated-TDS 'settlement' with no money. money-guards test locks all four. Suite money paths green. Co-Authored-By: Claude Fable 5 --- apps/hq/src/repos-documents.ts | 28 +++++++++++++ apps/hq/src/repos-payments.ts | 7 ++++ apps/hq/src/series.ts | 14 +++++-- apps/hq/test/money-guards.test.ts | 57 ++++++++++++++++++++++++++ packages/billing-engine/src/compute.ts | 13 ++++-- 5 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 apps/hq/test/money-guards.test.ts diff --git a/apps/hq/src/repos-documents.ts b/apps/hq/src/repos-documents.ts index 98f28a8..508aa25 100644 --- a/apps/hq/src/repos-documents.ts +++ b/apps/hq/src/repos-documents.ts @@ -179,6 +179,11 @@ async function buildLines(db: DB, inputs: DraftLineInput[], onDate: string, warn for (const line of inputs) { const mod = await getModule(db, line.moduleId) if (mod === null) throw new Error(`Module not found: ${line.moduleId}`) + // D24 (red-team): HQ line quantities are always whole units (SMS counts, module seats, + // AMC years) — reject fractional / non-integer / overflowing qty before it reaches paise. + if (!Number.isInteger(line.qty) || line.qty < 1 || line.qty > 100_000_000) { + throw new Error(`Quantity for ${mod.code} must be a whole number between 1 and 100,000,000`) + } const edition = line.edition ?? 'standard' // Price resolution order: (1) an explicit per-line override always wins (special deal); // (2) a quantity-tiered rate card if the module has one (D23 — SMS packs price by count); @@ -359,6 +364,13 @@ export async function markStatus(db: DB, userId: string, docId: string, status: return db.transaction(async () => { const before = await getDocument(db, docId) if (before === null) throw new Error('Document not found') + // D24 (red-team): accepted/lost are QUOTATION lifecycle states. An issued INVOICE or + // PROFORMA must never be pushed there — that would hide an invoice from overdue chasing + // and bypass the cancel/credit-note discipline. Invoices change state only through + // settlement (repos-payments) or cancel/credit-note. + if ((status === 'accepted' || status === 'lost') && before.docType !== 'QUOTATION') { + throw new Error(`Only quotations can be marked ${status}; a ${before.docType} cannot`) + } if (!(LEGAL_MARKS[before.status] ?? []).includes(status)) { throw new Error(`Illegal status transition: ${before.status} → ${status}`) } @@ -530,6 +542,22 @@ export async function createCreditNote(db: DB, userId: string, invoiceId: string roundToRupee: true, }, await taxRates(db)) return db.transaction(async () => { + // D24 (red-team): a credit note can never reverse more than the invoice's remaining + // value. Sum the value already credited by non-cancelled CNs on this invoice and cap + // the new CN to what's left. Read + insert in the same transaction so concurrent CNs + // cannot both slip past the check (CGST Act s.34 — a CN corrects, it does not exceed). + const already = ((await db.get<{ n: number }>( + `SELECT COALESCE(SUM(payable_paise), 0) AS n FROM document + WHERE doc_type='CREDIT_NOTE' AND ref_doc_id=? AND status <> 'cancelled'`, + inv.id, + ))!).n + const remaining = inv.payablePaise - already + if (computed.totals.payablePaise > remaining) { + throw new Error( + `Credit note (${computed.totals.payablePaise} paise) exceeds the invoice's remaining creditable value ` + + `(${remaining} paise: ${inv.payablePaise} invoiced − ${already} already credited)`, + ) + } const doc = await insertDocRow(db, userId, { docType: 'CREDIT_NOTE', clientId: inv.clientId, refDocId: inv.id, docDate: todayIso(), totals: computed.totals, diff --git a/apps/hq/src/repos-payments.ts b/apps/hq/src/repos-payments.ts index 52e916f..180b471 100644 --- a/apps/hq/src/repos-payments.ts +++ b/apps/hq/src/repos-payments.ts @@ -130,6 +130,13 @@ export async function recordPayment(db: DB, userId: string, input: RecordPayment if (!Number.isInteger(tdsPaise) || tdsPaise < 0) { throw new Error('tdsPaise must be a non-negative integer (paise)') } + // D24 (red-team): TDS settles invoices from the same pool as cash, so an unbounded TDS + // could 'pay' an invoice with no money. TDS is a fraction withheld from what's billed; + // it can never exceed the cash actually received (a generous ceiling — real TDS on + // professional services is 10%). Blocks fabricated-TDS settlement. + if (tdsPaise > input.amountPaise) { + throw new Error('TDS cannot exceed the cash amount received on the payment') + } return db.transaction(async () => { const id = uuidv7() await db.run( diff --git a/apps/hq/src/series.ts b/apps/hq/src/series.ts index 3ef5425..2566d2a 100644 --- a/apps/hq/src/series.ts +++ b/apps/hq/src/series.ts @@ -13,11 +13,17 @@ export async function nextDocNo(db: DB, docType: string, fy: string): Promise( - `SELECT prefix, next_seq FROM doc_series WHERE doc_type=? AND fy=?`, docType, fy, + // D24 (red-team): allocate the number in ONE atomic UPDATE ... RETURNING. This takes a + // row lock, so concurrent issuers serialize on the row and can never read the same seq + // twice (the old read-then-bump raced → duplicate numbers / burned sequence on Postgres). + // Both engines support UPDATE ... RETURNING (SQLite ≥3.35, node pg). Runs inside the + // caller's issueDocument transaction. + const row = (await db.get<{ prefix: string; seq: number }>( + `UPDATE doc_series SET next_seq = next_seq + 1 WHERE doc_type=? AND fy=? + RETURNING prefix, next_seq - 1 AS seq`, + docType, fy, ))! - await db.run(`UPDATE doc_series SET next_seq = next_seq + 1 WHERE doc_type=? AND fy=?`, docType, fy) - return formatDocNo(row.prefix, row.next_seq, 4) + return formatDocNo(row.prefix, row.seq, 4) } /** Mid-FY cutover (spec §4): continue after the last APEX-issued number. */ diff --git a/apps/hq/test/money-guards.test.ts b/apps/hq/test/money-guards.test.ts new file mode 100644 index 0000000..d96e50d --- /dev/null +++ b/apps/hq/test/money-guards.test.ts @@ -0,0 +1,57 @@ +// apps/hq/test/money-guards.test.ts — D24 (red-team): money-integrity guards. +import { describe, it, expect } from 'vitest' +import { openDb } from '../src/db' +import { seedIfEmpty } from '../src/seed' +import { createClient } from '../src/repos-clients' +import { createModule, setPrice } from '../src/repos-modules' +import { createDraft, issueDocument, createCreditNote, markStatus, getDocument } from '../src/repos-documents' +import { recordPayment } from '../src/repos-payments' + +async function world() { + const db = openDb(':memory:'); await seedIfEmpty(db) + const c = await createClient(db, 'u1', { name: 'Bank', stateCode: '32' }) + const m = await createModule(db, 'u1', { code: 'CORE', name: 'Core', allowedKinds: ['yearly'] }) + await setPrice(db, 'u1', { moduleId: m.id, kind: 'yearly', pricePaise: 10_000_00, effectiveFrom: '2026-01-01' }) + const inv = await issueDocument(db, 'u1', (await createDraft(db, 'u1', { + docType: 'INVOICE', clientId: c.id, lines: [{ moduleId: m.id, qty: 1, kind: 'yearly' }], + })).id) + return { db, c, m, inv } +} + +describe('credit-note cap (D24)', () => { + it('a full-value CN succeeds; a second CN on the same invoice is refused (would exceed it)', async () => { + const { db, inv } = await world() + const cn1 = await createCreditNote(db, 'u1', inv.id) // full value + expect(cn1.payablePaise).toBe(inv.payablePaise) + await expect(createCreditNote(db, 'u1', inv.id)).rejects.toThrow(/exceeds the invoice/i) + }) +}) + +describe('markStatus type guard (D24)', () => { + it('an issued invoice cannot be marked accepted/lost', async () => { + const { db, inv } = await world() + await expect(markStatus(db, 'u1', inv.id, 'lost')).rejects.toThrow(/Only quotations/i) + await expect(markStatus(db, 'u1', inv.id, 'accepted')).rejects.toThrow(/Only quotations/i) + // The invoice keeps its real status — still visible to overdue chasing. + expect((await getDocument(db, inv.id))!.status).not.toBe('lost') + }) +}) + +describe('integer quantity guard (D24)', () => { + it('a fractional / non-integer line quantity is rejected at draft', async () => { + const { db, c, m } = await world() + await expect(createDraft(db, 'u1', { + docType: 'INVOICE', clientId: c.id, lines: [{ moduleId: m.id, qty: 1.5, kind: 'yearly' }], + })).rejects.toThrow(/whole number/i) + }) +}) + +describe('TDS bound (D24)', () => { + it('TDS greater than the cash received is refused', async () => { + const { db, c, inv } = await world() + await expect(recordPayment(db, 'u1', { + clientId: c.id, receivedOn: '2026-07-10', mode: 'bank', amountPaise: 1_000_00, tdsPaise: 2_000_00, + allocations: [{ documentId: inv.id, amountPaise: 3_000_00 }], + })).rejects.toThrow(/TDS cannot exceed/i) + }) +}) diff --git a/packages/billing-engine/src/compute.ts b/packages/billing-engine/src/compute.ts index d9b7c2e..1a55056 100644 --- a/packages/billing-engine/src/compute.ts +++ b/packages/billing-engine/src/compute.ts @@ -42,9 +42,16 @@ export function computeBill(inputs: LineInput[], ctx: BillContext, rates: TaxCla if (inputs.length === 0) throw new Error('Cannot compute an empty bill') for (const line of inputs) { // Returns are separate documents (immutability rule) — never negative lines. - if (!(line.qty > 0)) throw new Error(`Quantity must be positive for "${line.name}"`) - if (!Number.isInteger(line.unitPricePaise) || line.unitPricePaise < 0) { - throw new Error(`Unit price must be non-negative integer paise for "${line.name}"`) + // Quantity may be fractional (weighed goods, e.g. 1.25 kg) but must be finite and + // magnitude-bounded: an astronomically large qty would flow into stored paise totals + // and, past 2^53, silently lose precision (paise are integers, JS numbers are not + // exact beyond that). Integer-only enforcement lives at the HQ document layer, where + // units are always whole (SMS counts, module seats). + if (!(line.qty > 0) || !Number.isFinite(line.qty) || line.qty > 100_000_000) { + throw new Error(`Quantity must be a positive number up to 100,000,000 for "${line.name}"`) + } + if (!Number.isInteger(line.unitPricePaise) || line.unitPricePaise < 0 || line.unitPricePaise > 1_000_000_000_00) { + throw new Error(`Unit price must be a non-negative integer paise (≤ ₹1,000 crore) for "${line.name}"`) } if (line.discount !== undefined) { const v = line.discount.kind === 'percentBp' ? line.discount.value : line.discount.paise