chore: hygiene sweep — dead code, role race, user lookup, stale files
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>
This commit is contained in:
parent
c0d371036b
commit
ec6cac51f0
19 changed files with 454 additions and 2942 deletions
260
docs/CODE-REVIEW-2026-06.md
Normal file
260
docs/CODE-REVIEW-2026-06.md
Normal file
|
|
@ -0,0 +1,260 @@
|
|||
# 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 |
|
||||
Loading…
Add table
Add a link
Reference in a new issue