Auth exact-matching, review-race guards, centralized account-name validation #59

Open

padreug wants to merge 1 commit from fix/auth-and-input-validation into fix/fava-client-hardening

padreug commented

2026-07-12 13:52:54 +00:00

Owner

Fifth PR of the refactor series (stacked on #58 → #57 → #56 → #55; merge in order).

Problems (CODE-REVIEW-2026-06 #5, #6, #16, #17 + libra-#36, libra-#51, libra-#52):

can_access_user_data compared user_id[:8] prefixes — a 32-bit space; any prefix collision (or a crafted short target id) let one user read another's balances/entries.
can_access_account used a substring test (f"User-{short}" in account.name) that also matched accounts merely containing the segment.
Manual-payment approve wrote the ledger entry before claiming the request — two concurrent admins both passed the status check and double-booked.
Account-name validation lived only on the admin add-account endpoint; crud.create_account leaked raw sqlalchemy IntegrityError on duplicates (libra-#36), and get_or_create_user_account string-matched the SQLite error message (never matched on Postgres).

Changes:

Full-ID equality in can_access_user_data; exact :-segment match in can_access_account.
approve/reject_manual_payment_request are status-guarded (WHERE status='pending' + rowcount); the approve endpoint claims first, writes the ledger entry second, and reverts the claim if the write fails — at most one journal entry per request, losers get 409.
validate_account_name centralized in account_utils.py (libra-#51), called from crud.create_account (choke point for every creation path; virtual parents may be a bare root), the admin endpoint, and fava_client.add_account at the writer boundary (libra-#52).
New AccountExistsError domain error; POST /accounts returns 409 on duplicate / 400 on malformed names; the user-account recovery path catches the domain error (Postgres-compatible).

Tests: new tests/test_auth_validation.py — prefix-colliding users isolated; substring account bait denied while the genuinely-owned account passes; concurrent double-approve creates exactly one ledger entry; duplicate create → 409; malformed name → 400. Full suite: 182 passed, 3 skipped.

🤖 Generated with Claude Code

Fifth PR of the refactor series (stacked on #58 → #57 → #56 → #55; merge in order). **Problems** (CODE-REVIEW-2026-06 #5, #6, #16, #17 + libra-#36, libra-#51, libra-#52): - `can_access_user_data` compared `user_id[:8]` prefixes — a 32-bit space; any prefix collision (or a crafted short target id) let one user read another's balances/entries. - `can_access_account` used a substring test (`f"User-{short}" in account.name`) that also matched accounts merely *containing* the segment. - Manual-payment approve wrote the ledger entry **before** claiming the request — two concurrent admins both passed the status check and double-booked. - Account-name validation lived only on the admin add-account endpoint; `crud.create_account` leaked raw sqlalchemy `IntegrityError` on duplicates (libra-#36), and `get_or_create_user_account` string-matched the SQLite error message (never matched on Postgres). **Changes:** - Full-ID equality in `can_access_user_data`; exact `:`-segment match in `can_access_account`. - `approve/reject_manual_payment_request` are status-guarded (`WHERE status='pending'` + rowcount); the approve endpoint claims first, writes the ledger entry second, and reverts the claim if the write fails — at most one journal entry per request, losers get 409. - `validate_account_name` centralized in `account_utils.py` (libra-#51), called from `crud.create_account` (choke point for every creation path; virtual parents may be a bare root), the admin endpoint, and `fava_client.add_account` at the writer boundary (libra-#52). - New `AccountExistsError` domain error; `POST /accounts` returns 409 on duplicate / 400 on malformed names; the user-account recovery path catches the domain error (Postgres-compatible). **Tests:** new `tests/test_auth_validation.py` — prefix-colliding users isolated; substring account bait denied while the genuinely-owned account passes; concurrent double-approve creates exactly one ledger entry; duplicate create → 409; malformed name → 400. Full suite: 182 passed, 3 skipped. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

padreug added 1 commit

2026-07-12 13:52:54 +00:00

fix(auth): exact-match authorization; guard reviews; centralize name validation c0d371036b

Auth + input-validation cluster (CODE-REVIEW-2026-06 #5, #6, #16, #17
+ libra-#36, libra-#51, libra-#52):

- can_access_user_data compares full user ids only. The 8-char prefix
  comparison was a 32-bit space: any prefix collision (or a crafted
  short target id) let one user read another's data.
- can_access_account matches the User-{short} SEGMENT exactly; the
  substring test also matched accounts merely containing it
  (Expenses:Misc-User-deadbeef).
- Manual-payment approve/reject are status-guarded
  (UPDATE ... WHERE status='pending' + rowcount): concurrent admins
  can't double-book. The approve endpoint claims the request BEFORE
  writing the ledger entry and reverts the claim if the write fails,
  so at most one journal entry can exist per request.
- Account-name validation centralized into
  account_utils.validate_account_name (libra-#51) — called from
  crud.create_account (the choke point for every creation path,
  virtual parents allowed a bare root), the admin add-account
  endpoint, and fava_client.add_account at the writer boundary
  (libra-#52).
- crud.create_account translates backend unique-violations into
  AccountExistsError instead of leaking sqlalchemy internals
  (libra-#36); POST /accounts returns 409 on duplicates and 400 on
  malformed names. get_or_create_user_account catches the domain
  error instead of string-matching the SQLite message (which never
  matched on Postgres).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

padreug referenced this pull request

2026-07-12 14:01:56 +00:00

Hygiene sweep: dead code, role-assignment race, cached user lookup, stale files #60

padreug referenced this pull request

2026-07-12 14:13:07 +00:00

Split views_api into a domain package; unshadow /accounts/hierarchy #61

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/auth-and-input-validation:fix/auth-and-input-validation

git switch fix/auth-and-input-validation

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.