libra/docs/CODE-REVIEW-2026-06.md

Code review — 2026-06-05

Findings from a deep review of the Libra LNbits extension (12k LOC, 14 files). Each finding has file:line references, a one-line fix proposal, and a status tag:

• ✅ fixed — merged in commit listed
• ⏳ outstanding — still needs work
• 🚫 downgraded — initially flagged, verified not a bug on closer read

Triage order at the bottom prioritises blast radius over file location.

2026-07-12 refactor series: findings #2–#19 fixed across PRs #55–#59 + the chore/hygiene branch (stacked; merge in order). LOW items fixed in chore/hygiene except parse_legacy_account_name fragility (documented assumption, internal input only) and the is_active/is_virtual filter inconsistency (still open).

---

CRITICAL

✅ #1 — Mass require_admin_key mis-use → cross-user privilege escalation

Status: fixed in 1201557 (aiolabs/libra main, 2026-06-05) + 4c704e5 (aiolabs/webapp dev).

27 endpoints documented "(admin only)" used require_admin_key, which only checks the caller owns some wallet with its admin key — i.e. any authenticated user. Cluster included receivable/revenue creation, equity-eligibility grant/revoke, account-permission CRUD (grant yourself MANAGE on any account → ledger god mode), role and user-role CRUD, account-sync admin, cross-user reports.

Also deleted the duplicate api_pay_user at views_api.py:1937 (the correctly-gated /api/v1/payables/pay at L2144 replaces it).

Webapp side: deleted orphaned PermissionManager.vue + GrantPermissionDialog.vue admin components that were never imported or routed and whose backing API methods pointed at non-existent paths.

✅ #2 — format_net_settlement_entry ships unbalanced postings on partial payments

beancount_format.py:761-777 emits three postings whose weights sum to net_fiat − total_receivable + total_payable. The docstring example assumes net_fiat == total_receivable − total_payable. But tasks.py:251-258 sets total_receivable = total_prior_balance and net_fiat = invoice_fiat_amount — only equal when the user paid the full balance. Any partial payment ships unbalanced postings; Beancount will reject or apply tolerance silently.

Fix: add assert abs(net_fiat − (receivable − payable)) <= 0.005 inside the formatter and raise on violation. Then fix the caller in tasks.py:218-322 to settle only what the payment covers (likely a two-posting DR Lightning / CR Receivable-for-payment-amount, not net-settlement).

✅ #3 — Migrations not idempotent (violates fork-migrations contract)

migrations.py:347, 377 (ALTER TABLE accounts ADD COLUMN is_active/is_virtual) and migrations.py:441, 472, 510 (CREATE TABLE roles/role_permissions/user_roles) lack idempotency guards. Seed INSERTs at migrations.py:319-330, 400-412, 587-597 have no ON CONFLICT DO NOTHING. Per CLAUDE.md, the cross-DB write between ext_libra and core dbversions is non-atomic — a failed version-bump leaves the migration to re-run on boot and crash with duplicate column / table exists / UNIQUE violation. Bricks the extension until manual dbversions surgery.

Fix: wrap ALTERs with _alter_add_column_safe, switch CREATEs to CREATE TABLE IF NOT EXISTS, gate seed INSERTs with INSERT ... ON CONFLICT DO NOTHING.

✅ #4 — Lightning payment recording has no local idempotency gate

tasks.py:218-322 relies entirely on fava.add_entry_idempotent for dedup, which itself does a read-then-write race on the Fava ledger. On lnbits restart with a persisted invoice queue, the same payment_hash can re-fire; the per-user lock is in-process only and doesn't survive restart. Webhook + poller hitting concurrently both pass the "not present" check and both insert.

Fix: add a processed_payments(payment_hash TEXT PRIMARY KEY) table; INSERT OR IGNORE at the top of on_invoice_paid; only proceed if rowcount == 1.

---

HIGH

✅ #5 — Auth prefix-match in can_access_user_data

auth.py:248-251 uses caller.user_id[:8] == target.user_id[:8]. Eight hex chars = 32 bits; birthday collision at ~65k users.

Fix: require full UUID equality; never resolve by prefix in an authorisation decision.

✅ #6 — can_access_account substring match

auth.py:178-180 does f"User-{short}" in account.name — matches Expenses:Misc-User-deadbeef too.

Fix: split on :, require segment equality.

✅ #7 — ChecksumConflictError never raised by update/delete

fava_client.py:1392-1482: Fava 409/412 propagates as raw HTTPStatusError. The ChecksumConflictError type exists but isn't raised by these methods; callers see stack-trace 500s instead of a clean retry path.

Fix: if status in (409, 412): raise ChecksumConflictError(...).

✅ #8 — float() arithmetic in fiat-rate metadata

views_api.py:1061-1062, 1263-1264, 1364-1365, 1759-1760 compute fiat_rate / btc_rate via float(), then persist into Beancount metadata as the cost basis for that entry. Float drift cascades through reporting.

Fix: keep Decimal end-to-end; only stringify at JSON-serialise time.

✅ #9 — Background loop swallows raise in on_invoice_paid

tasks.py:320-322 does logger.error(...); raise. wait_for_paid_invoices at tasks.py:178-180 has no surrounding try/except, so one unhandled exception kills the listener for the rest of the process lifetime — no further Lightning payments get recorded, no alarm.

Fix: wrap the iteration body in try/except Exception: logger.exception(...); never raise from on_invoice_paid.

✅ #10 — record-payment dedup is non-atomic AND exception-swallowing

views_api.py:1841-1924 (per subagent report — needs verification) catches all exceptions in the dedup window with a 5-second timeout and treats Fava errors as "not duplicate", producing double-entries on transient Fava blips.

Fix: fail closed on transport error; narrow the exception type catch to httpx.HTTPError only.

✅ #11 — validate_journal_entry is stale

core/validation.py:21-93 validates the pre-string-amount model — sums one bag of integers, doesn't balance per currency. Doesn't match production data shape post-Fava migration.

Fix: rewrite to parse "X CCY" strings and balance per currency, or delete if Beancount-side validation is now considered sufficient.

---

MEDIUM

✅ #12 — m001_initial seed INSERT into accounts non-idempotent

migrations.py:319-330 — same shape as #3.

✅ #13 — format_posting_at_average_cost emits {} when cost_currency=None

beancount_format.py:256 — <sats> SATS {} isn't valid Beancount syntax; drop the braces when cost is unset.

✅ #14 — Per-call httpx.AsyncClient instantiation in fava_client

~30 sites build a new httpx.AsyncClient per call. TCP handshake every time.

Fix: construct once on FavaClient.__init__, expose aclose().

✅ #15 — BQL string interpolation without quoting

fava_client.py:250, 621, 770-775, 853-858 interpolate account_name / user-id-prefix raw into BQL WHERE account = '{...}'. The 8-char hex prefix is safe in practice; arbitrary account_name input is not.

Fix: validate against ^[A-Za-z0-9:_-]+$ before interpolation.

✅ #16 — approve_manual_payment_request not status-guarded

crud.py:559-579 overwrites status='approved' regardless of current state. Two concurrent admins → two journal entries.

Fix: UPDATE ... WHERE id=:id AND status='pending', check rowcount == 1.

✅ #17 — Account name not validated on receivable/revenue/expense

views_api.py:1066-1074, 1224-1236, 1369-1376, 1477-1494 accept free-string data.expense_account (etc.) with no Beancount-syntax check before lookup.

Fix: enforce ^[A-Z][A-Za-z0-9:-]*$.

✅ #18 — _get_username_from_user_id creates a fresh LNbits DB per call

views_api.py:697-708 opens an LNbits DB inside a per-row hot path.

Fix: cache the Database instance at module load; batch-load usernames once per request via single IN (…) query.

✅ #19 — get_user_balance regex rejects decimal SATS

fava_client.py:346, 503 patterns require (-?\d+) SATS. Fava's @@→@ normalisation can emit decimal SATS.

Fix: (-?[\d.]+).

⏳ #20 — fava_url default http://localhost:3333 and sandboxed lnbits

Loopback breaks if the lnbits service unit gains PrivateNetwork=true. Not a code bug — worth documenting in deploy assumptions.

---

LOW

• ⏳ tasks.py mixes print() with logger.* (:61, 65-69, 81, 89, 94, 162).
• ⏳ Dead model imports in crud.py:21-27 (JournalEntry, EntryLine) after entry_lines table dropped.
• ⏳ auto_assign_default_role check-then-act race (crud.py:1609-1641) — add UNIQUE constraint on user_roles(user_id, role_id).
• ⏳ Pydantic v1 .dict() calls (crud.py:381, 400, 411, 450) if upstream is on v2.
• ⏳ account_utils.parse_legacy_account_name splits on - — fragile if ever called on user input.
• ⏳ Account.is_active vs is_virtual default-filter inconsistency hides virtual parents in permission-grant UI (crud.py:146-167).

---

🚫 Downgraded (initially flagged, verified not a bug)

Subagent A — "inverted balance sign in tasks.py:251-258"

fava_client.py:303 docstring says positive = user owes libra (bookkeeper perspective). tasks.py:249-250 matches. CLAUDE.md describes the user's perspective (positive = libra owes user), which is consistent at the UI layer. Both representations are internally coherent — no bug, just a doc-vs-code perspective collision.

Subagent A — "fava_ledger_slug default doesn't match deploy"

Verified empirically by user: Fava in single-ledger mode appears to accept arbitrary slugs against the JSON API, and the deploy's seed title "Libra Ledger" → slugify → libra-ledger matches the extension default in models.py:159 anyway. False alarm.

---

Triage order (when picking the next item)

1. #2 (unbalanced net settlement) + #4 (idempotency) — silently corrupts the ledger on every partial Lightning payment + every restart with a persisted invoice queue. Real-money blast radius.
2. #3 (migrations) + #12 — guaranteed boot crash on the documented failure mode; bricks the extension.
3. #8 (float in fiat metadata) — every entry written today carries float-drift cost basis into Beancount.
4. #9 (silent listener death) — operational; the kind of bug discovered when nobody can pay for a week.
5. #5, #6 (auth narrowing) — residual privilege risk; smaller blast than #1 (already fixed) but worth closing.
6. Everything else, in arbitrary order; mostly hygiene.

---

Commits applied

Commit	Repo / branch	What
1201557	aiolabs/libra main	Gate cross-user admin endpoints behind require_super_user; delete duplicate api_pay_user
4c704e5	aiolabs/webapp dev	Delete orphaned PermissionManager.vue + GrantPermissionDialog.vue + 3 API methods + 4 dead types