Padreug
ec6cac51f0
One pass over the LOW-tier review items plus two folded issues: - Delete validate_journal_entry (dead since the Fava migration; it validated the pre-string-amount model) with its exports, unused crud imports, and tests. Beancount validates entries now. - Migration m006: UNIQUE index on user_roles(user_id, role_id) after deduping; assign_user_role inserts with ON CONFLICT DO NOTHING and returns the existing assignment — closes the auto-assign check-then-act race on concurrent logins. - Extract _get_username_from_user_id (110 lines in views_api, fresh LNbits Database per call inside per-row hot paths) into user_lookup.py with one shared core-DB handle, a 60s TTL cache and a batch get_usernames API (review #18). - Receivable-entry responses report CLEARED, matching the flag the formatter actually writes; PENDING misled the UI (libra-#35). - Replace the remaining print() calls in tasks.py with logger. - get_all_accounts derives valid roots from account_utils.ACCOUNT_TYPE_ROOTS instead of a hardcoded tuple, and the no-op per-test rate-limit reset is gone (libra-#54). - Delete migrations_old.py.bak, MIGRATION_SQUASH_SUMMARY.md, docs/PHASE*_COMPLETE.md and the rendered .html; .gitignore data/ (it holds the runtime .lnbits_auth_key secret). - Track docs/CODE-REVIEW-2026-06.md with finding statuses updated for the PR #55-#59 + chore/hygiene series. - CLAUDE.md notes LNbits pins Pydantic v1: keep .dict(), don't "modernize" to .model_dump(). Note: format_payment_entry's is_payable docstring (flagged in review follow-up) turned out to be consistent with the body — no change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
260 lines
11 KiB
Markdown
260 lines
11 KiB
Markdown
# 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
|
||
`INSERT`s 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 |
|