Back to DevLog

Fixing payment reconciliation and loan term validation

4 min read

I fixed two security findings from the audit and then discovered the findings themselves were wrong.

The audit undercounted both problems

The audit claimed one Square capture path. I re-derived the surface and found eight capture sites across six routes plus one cron. It claimed seven idempotency key sites. I found thirteen.

I replaced all time-based idempotency keys with sha256 over scope and intent components, truncated to 28 hex characters to stay under Square's 45-character limit with margin. Twelve sites converted. The thirteenth was a duplicate route that I deleted.

The highest-impact bug was in the recurring billing cron. It keyed on recurring_${tenant.id}_${Date.now()}, so any cron re-run inside a billing period re-charged every tenant. Now keyed on next_payment_date.

Payment capture is now transactional

I wrapped all eight post-capture write sequences in real transactions via a new runPostCapture() helper that owns a pooled connection. On write failure it rolls back, then records the orphaned charge in a new payment_reconciliation table using a separate pooled connection so the row survives the rollback.

I fixed five broken transactions that used db.pool.query('BEGIN'). That function picks a free connection per statement, so BEGIN never bound the writes. They auto-committed and ROLLBACK was a no-op.

I found and fixed a path where an orphaned capture actively harmed a paying customer. The cron's generic catch marked the tenant past due, wrote a failed payment row, and emailed "payment failed" after the card was successfully charged.

Loan term validation

POST /api/loans accepted any downPaymentPercentage. A request with 0.01 bought a $15,000 property for $100.50 today. termMonths was also unbounded. Zero produced monthlyPayment: Infinity and totalAmount: NaN written to the loans table. 6000 produced a 500-year loan.

I added a purchase-terms resolver on the existing tenant to global to siteConfig chain, plus exact-match validation that runs before the Square capture. The validator throws a 400 if the client-supplied terms do not match a configured tier.

I rewrote calculateFinancing to take resolved terms and throw rather than select a tier. I removed both fall-through branches.

I fixed a real money mismatch. calculateFinancing read processingFee and minimumMonthlyPayment from siteConfig, not tenant settings. A tenant that configured its own fee had the UI quote its fee and the server charge siteConfig's $99.

I rewrote the client calculator to render exactly the configured tiers and terms, resolving them with the same precedence as the server. The calculator had offered 20/25/35/50% but only 20% and 50% were configured tiers. For 25% down the client picked 12% APR and the server picked 8% APR. The customer was shown $376.95 per month and originated at $355.64 per month.

Active and overdue loan counts

The Active and Overdue counts disagreed between /admin/dashboard and /admin/loans in both directions. I found thirteen definitions of active and overdue across the codebase, not the two described in the request.

The dashboard counted overdue as next_payment_date < CURRENT_DATE - INTERVAL '7 days', baking the grace period into the overdue flag itself. The loans page treated active and overdue as mutually exclusive and subtracted overdue from active. The All tab counted archived loans that its own filter hides.

I added SQL mirrors to server.js sitting directly beside the JS helpers: LOAN_IS_ACTIVE_SQL, LOAN_DAYS_PAST_DUE_SQL, LOAN_IS_OVERDUE_SQL, LOAN_LATE_FEE_WAIVED_SQL, loanPastGracePeriodSql(), loanLateFeeAppliesSql().

GET /api/admin/loans now returns is_active, is_overdue, days_past_due, late_fee_applies, past_grace_period, late_fee_waived. AdminLoans.js renders those flags rather than recomputing, so its stat cards and tab counts are the same numbers.

Active and overdue overlap. A loan that is overdue is also active. A payment due today is not overdue. Grace changes the fee only.

Multi-tenant leak in PDF export

/api/admin/reports/export ran all eight of its queries with no tenant filter. Portfolio totals, active loans, revenue, tax, HOA and outstanding balances all summed every tenant on the platform into one admin's downloadable PDF. It survived the C-4 sweep because it takes no :id parameter. The C-4 pass found IDOR-shaped leaks, not all leaks.

Master Admin counter columns are dead

tenants.active_loans_count, total_properties_count and admin_users_count are written by nothing in the codebase. They had sat at DEFAULT 0 since the table was created. All three call sites now count live from the source tables.

Share this post