From c0d371036ba05c96a1457126bdc356dfe24629f7 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 15:52:29 +0200 Subject: [PATCH] fix(auth): exact-match authorization; guard reviews; centralize name validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- account_utils.py | 52 +++++++++ auth.py | 18 +-- crud.py | 142 +++++++++++++++-------- fava_client.py | 7 ++ tests/test_auth_validation.py | 207 ++++++++++++++++++++++++++++++++++ views_api.py | 99 ++++++++-------- 6 files changed, 422 insertions(+), 103 deletions(-) create mode 100644 tests/test_auth_validation.py diff --git a/account_utils.py b/account_utils.py index 8520d59..d797796 100644 --- a/account_utils.py +++ b/account_utils.py @@ -17,6 +17,58 @@ ACCOUNT_TYPE_ROOTS = { AccountType.EXPENSE: "Expenses", } +VALID_ACCOUNT_PREFIXES = ("Assets:", "Liabilities:", "Equity:", "Income:", "Expenses:") + + +def is_valid_account_component(component: str, *, is_root: bool) -> bool: + """Validate one ':'-separated account component against Beancount's grammar. + + Mirrors core/account.py: a root component matches ``[\\p{Lu}][\\p{L}\\p{Nd}-]*`` + (must start with an uppercase letter); a sub component matches + ``[\\p{Lu}\\p{Nd}][\\p{L}\\p{Nd}-]*`` (may also start with a digit). Body + chars are letters, decimal digits, or hyphen. Implemented with Unicode-aware + str methods (libra's runtime has no beancount — Fava is a separate service), + so non-ASCII letters are accepted exactly as Beancount accepts them. + """ + if not component: + return False + first, rest = component[0], component[1:] + first_ok = (first.isalpha() and first.isupper()) or ( + not is_root and first.isdecimal() + ) + if not first_ok: + return False + return all(ch == "-" or ch.isalpha() or ch.isdecimal() for ch in rest) + + +def validate_account_name(name: str, *, allow_root_only: bool = False) -> None: + """Raise ValueError if ``name`` is not a syntactically valid Beancount account. + + The single source of truth for account-name validation (libra-#51): + every path that writes an account name — admin add-account, direct + account create, user-account derivation — funnels through here before + the name can reach the ledger source. + + Args: + name: Hierarchical account name (e.g. "Expenses:Food"). + allow_root_only: Accept a bare root component ("Expenses") — + only virtual parent accounts are allowed this shape. + """ + parts = name.split(":") + min_parts = 1 if allow_root_only else 2 + valid = ( + len(parts) >= min_parts + and is_valid_account_component(parts[0], is_root=True) + and all(is_valid_account_component(p, is_root=False) for p in parts[1:]) + ) + if not valid: + raise ValueError( + f"Invalid account name {name!r}: each ':'-separated part must be " + "letters/digits/hyphens, the root starting with an uppercase " + "letter (sub-accounts may start with a digit), with at least one " + "sub-account (e.g. Expenses:Food)." + ) + def format_hierarchical_account_name( account_type: AccountType, diff --git a/auth.py b/auth.py index 8cc9c17..34ba8c6 100644 --- a/auth.py +++ b/auth.py @@ -172,11 +172,14 @@ async def can_access_account( if auth.is_super_user: return True - # Check if this is the user's own account + # Check if this is the user's own account. Match the User-{short} + # segment exactly — a substring test also matched account names that + # merely CONTAIN it (e.g. "Expenses:Misc-User-deadbeef"), granting + # access to unrelated accounts. account = await get_account(account_id) if account: - user_short = auth.user_id[:8] - if f"User-{user_short}" in account.name: + user_segment = f"User-{auth.user_id[:8]}" + if user_segment in account.name.split(":"): return True # Check explicit permissions @@ -242,14 +245,13 @@ async def can_access_user_data(auth: AuthContext, target_user_id: str) -> bool: if auth.is_super_user: return True - # Users can access their own data - compare full ID or short ID + # Users can access their own data. Full-ID equality ONLY: an 8-char + # prefix comparison is a 32-bit space, and any prefix collision (or a + # deliberately crafted short target id) let one user read another's + # data. Callers must pass full user ids. if auth.user_id == target_user_id: return True - # Also allow if short IDs match (8 char prefix) - if auth.user_id[:8] == target_user_id[:8]: - return True - return False diff --git a/crud.py b/crud.py index b49bb9b..5ea93f0 100644 --- a/crud.py +++ b/crud.py @@ -66,7 +66,21 @@ PERMISSION_CACHE_TTL = 60 # 1 minute # ===== ACCOUNT OPERATIONS ===== +class AccountExistsError(Exception): + """Raised when creating an account whose name is already taken.""" + + def __init__(self, name: str): + super().__init__(f"Account already exists: {name}") + self.name = name + + async def create_account(data: CreateAccount) -> Account: + # Single validation choke point for every account-creation path + # (libra-#51). Virtual parents may be a bare root ("Expenses"). + from .account_utils import validate_account_name + + validate_account_name(data.name, allow_root_only=data.is_virtual) + account_id = urlsafe_short_hash() account = Account( id=account_id, @@ -77,7 +91,17 @@ async def create_account(data: CreateAccount) -> Account: is_virtual=data.is_virtual, created_at=datetime.now(), ) - await db.insert("accounts", account) + try: + await db.insert("accounts", account) + except Exception as e: + # Translate backend-specific unique-violation errors (SQLite: + # "UNIQUE constraint failed", Postgres: "duplicate key value") + # into a domain error instead of leaking sqlalchemy internals + # (libra-#36). + msg = str(e).lower() + if "unique" in msg or "duplicate" in msg: + raise AccountExistsError(data.name) from e + raise # Invalidate cache for this account (Cache class doesn't have delete method, use pop) account_cache._values.pop(f"account:id:{account_id}", None) @@ -305,44 +329,39 @@ async def get_or_create_user_account( user_id=user_id, ) ) - except Exception as e: - # Handle UNIQUE constraint error - account already exists - if "UNIQUE constraint failed" in str(e) and "accounts.name" in str(e): - logger.warning(f"[LIBRA DB] Account already exists (UNIQUE constraint), fetching by name: {account_name}") - # Fetch existing account by name only (ignore user_id in query) - account = await db.fetchone( - """ - SELECT * FROM accounts - WHERE name = :name - """, - {"name": account_name}, - Account, - ) - if account: - logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})") - # Update user_id if it's NULL or different - if account.user_id != user_id: - logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}") - await db.execute( - """ - UPDATE accounts - SET user_id = :user_id - WHERE name = :name - """, - {"user_id": user_id, "name": account_name} - ) - # Refresh account from DB - account = await db.fetchone( - """ - SELECT * FROM accounts - WHERE name = :name - """, - {"name": account_name}, - Account, - ) - else: - # Re-raise if it's a different error - raise + except AccountExistsError: + logger.warning(f"[LIBRA DB] Account already exists, fetching by name: {account_name}") + # Fetch existing account by name only (ignore user_id in query) + account = await db.fetchone( + """ + SELECT * FROM accounts + WHERE name = :name + """, + {"name": account_name}, + Account, + ) + if account: + logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})") + # Update user_id if it's NULL or different + if account.user_id != user_id: + logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}") + await db.execute( + """ + UPDATE accounts + SET user_id = :user_id + WHERE name = :name + """, + {"user_id": user_id, "name": account_name} + ) + # Refresh account from DB + account = await db.fetchone( + """ + SELECT * FROM accounts + WHERE name = :name + """, + {"name": account_name}, + Account, + ) else: logger.info(f"[LIBRA DB] Account already exists in Libra DB: {account_name}") @@ -563,14 +582,20 @@ async def get_all_manual_payment_requests( async def approve_manual_payment_request( request_id: str, reviewed_by: str, journal_entry_id: str ) -> Optional["ManualPaymentRequest"]: - """Approve a manual payment request""" - from .models import ManualPaymentRequest + """Approve a manual payment request. - await db.execute( + Status-guarded: only a 'pending' request can be approved, so two + concurrent admins can't both win (the loser gets None and must not + create a second journal entry). + + Returns: + The approved request, or None if it wasn't pending anymore. + """ + result = await db.execute( """ UPDATE manual_payment_requests SET status = 'approved', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by, journal_entry_id = :journal_entry_id - WHERE id = :id + WHERE id = :id AND status = 'pending' """, { "id": request_id, @@ -579,21 +604,42 @@ async def approve_manual_payment_request( "journal_entry_id": journal_entry_id, }, ) + if result.rowcount == 0: + return None return await get_manual_payment_request(request_id) +async def revert_manual_payment_request(request_id: str) -> None: + """Roll an approved request back to pending. + + Compensation for the approve flow: the status is claimed BEFORE the + journal entry is written (so concurrent admins can't double-book); + if the ledger write then fails, the claim must be released. + """ + await db.execute( + """ + UPDATE manual_payment_requests + SET status = 'pending', reviewed_at = NULL, reviewed_by = NULL, journal_entry_id = NULL + WHERE id = :id AND status = 'approved' + """, + {"id": request_id}, + ) + + async def reject_manual_payment_request( request_id: str, reviewed_by: str ) -> Optional["ManualPaymentRequest"]: - """Reject a manual payment request""" - from .models import ManualPaymentRequest + """Reject a manual payment request. - await db.execute( + Status-guarded like approve_manual_payment_request; returns None when + the request wasn't pending anymore. + """ + result = await db.execute( """ UPDATE manual_payment_requests SET status = 'rejected', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by - WHERE id = :id + WHERE id = :id AND status = 'pending' """, { "id": request_id, @@ -601,6 +647,8 @@ async def reject_manual_payment_request( "reviewed_by": reviewed_by, }, ) + if result.rowcount == 0: + return None return await get_manual_payment_request(request_id) diff --git a/fava_client.py b/fava_client.py index faad8f7..057562a 100644 --- a/fava_client.py +++ b/fava_client.py @@ -1775,6 +1775,13 @@ class FavaClient: """ from datetime import date as date_type + # Defense in depth at the writer boundary (libra-#52): the name is + # written verbatim into ledger source below, so validate it HERE, + # not only in the endpoints that happen to call this today. + from .account_utils import validate_account_name + + validate_account_name(account_name, allow_root_only=True) + if opening_date is None: opening_date = date_type.today() diff --git a/tests/test_auth_validation.py b/tests/test_auth_validation.py new file mode 100644 index 0000000..90d278f --- /dev/null +++ b/tests/test_auth_validation.py @@ -0,0 +1,207 @@ +"""Auth narrowing + input validation. + +Covers the PR-5 fixes: + - `can_access_user_data`: full-ID equality only (an 8-char prefix + comparison let prefix-colliding users read each other's data). + - `can_access_account`: exact User-{short} SEGMENT match (a substring + test also matched accounts merely containing it). + - Manual-payment approve/reject: status-guarded claim — concurrent + admins can't double-book (CODE-REVIEW-2026-06 #16). + - POST /accounts: duplicate → 409 instead of a leaked + IntegrityError 500 (libra-#36); malformed name → 400 (libra-#51). +""" +import asyncio +import importlib +from uuid import uuid4 + +import pytest + +from .helpers import submit_manual_payment_request + +pytestmark = pytest.mark.anyio + + +def _module(name: str): + for prefix in ("lnbits.extensions.libra", "libra"): + try: + return importlib.import_module(f"{prefix}.{name}") + except ModuleNotFoundError: + continue + raise ModuleNotFoundError(f"libra.{name}: tried both import paths") + + +auth = _module("auth") +libra_crud = _module("crud") +mdl = _module("models") + + +def _ctx(user_id: str) -> "auth.AuthContext": + return auth.AuthContext( + user_id=user_id, wallet_id="w", is_super_user=False, wallet=None, + ) + + +# --------------------------------------------------------------------------- +# can_access_user_data — full-ID equality +# --------------------------------------------------------------------------- + + +async def test_prefix_colliding_user_cannot_access_other_users_data(client): + caller = "deadbeef" + uuid4().hex[8:] + victim = "deadbeef" + uuid4().hex[8:] # same 8-char prefix, different id + assert victim != caller + + assert await auth.can_access_user_data(_ctx(caller), caller) is True + assert await auth.can_access_user_data(_ctx(caller), victim) is False + # A crafted short target id must not match either. + assert await auth.can_access_user_data(_ctx(caller), caller[:8]) is False + + +# --------------------------------------------------------------------------- +# can_access_account — exact segment match +# --------------------------------------------------------------------------- + + +async def test_account_access_requires_exact_user_segment(client): + user_id = "deadbeef" + uuid4().hex[8:] + suffix = uuid4().hex[:6] + + # An account whose LAST SEGMENT merely contains "User-deadbeef". + lookalike = await libra_crud.create_account( + mdl.CreateAccount( + name=f"Expenses:Misc-User-deadbeef-{suffix}", + account_type=mdl.AccountType.EXPENSE, + description="substring-match bait", + ) + ) + owned = await libra_crud.create_account( + mdl.CreateAccount( + name=f"Assets:Receivable-{suffix}:User-deadbeef", + account_type=mdl.AccountType.ASSET, + description="genuinely owned", + user_id=user_id, + ) + ) + + ctx = _ctx(user_id) + assert await auth.can_access_account( + ctx, lookalike.id, mdl.PermissionType.READ + ) is False, "substring-only match must not grant access" + assert await auth.can_access_account( + ctx, owned.id, mdl.PermissionType.READ + ) is True + + +# --------------------------------------------------------------------------- +# Manual payment approve/reject — status-guarded claim +# --------------------------------------------------------------------------- + + +async def test_concurrent_approvals_create_exactly_one_entry( + client, super_user_headers, configured_user, +): + _, wallet = configured_user + submitted = await submit_manual_payment_request( + client, + wallet_inkey=wallet.inkey, + amount_sats=10_000, + description=f"race {uuid4().hex[:6]}", + ) + + r1, r2 = await asyncio.gather( + client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ), + client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ), + ) + statuses = sorted([r1.status_code, r2.status_code]) + assert statuses[0] == 200, f"one approval must win: {statuses} {r1.text} {r2.text}" + assert statuses[1] in (400, 409), ( + f"the losing approval must fail cleanly, got {statuses}" + ) + + # Exactly one ledger entry references this request. + listing = await client.get( + "/libra/api/v1/entries/user", + headers={"X-Api-Key": wallet.inkey}, + ) + assert listing.status_code == 200 + link = f"MPR-{submitted['id']}" + matching = [ + e for e in listing.json()["entries"] if link in (e.get("links") or []) + ] + assert len(matching) == 1, ( + f"expected exactly one journal entry for {link}, got {len(matching)}" + ) + + +async def test_reject_after_approve_conflicts( + client, super_user_headers, configured_user, +): + _, wallet = configured_user + submitted = await submit_manual_payment_request( + client, + wallet_inkey=wallet.inkey, + amount_sats=5_000, + description=f"approve-then-reject {uuid4().hex[:6]}", + ) + + r = await client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ) + assert r.status_code == 200, r.text + + r = await client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/reject", + headers=super_user_headers, + ) + assert r.status_code in (400, 409), ( + f"rejecting an approved request must fail, got {r.status_code}" + ) + + +# --------------------------------------------------------------------------- +# POST /accounts — duplicate and malformed names +# --------------------------------------------------------------------------- + + +async def test_create_duplicate_account_returns_409( + client, super_user_headers, +): + name = f"Expenses:DupTest-{uuid4().hex[:6]}" + body = {"name": name, "account_type": "expense", "description": "dup test"} + + r = await client.post( + "/libra/api/v1/accounts", headers=super_user_headers, json=body, + ) + assert r.status_code == 201, r.text + + r = await client.post( + "/libra/api/v1/accounts", headers=super_user_headers, json=body, + ) + assert r.status_code == 409, ( + f"duplicate create must 409, not leak an IntegrityError: " + f"{r.status_code} {r.text}" + ) + + +async def test_create_account_with_invalid_name_returns_400( + client, super_user_headers, +): + r = await client.post( + "/libra/api/v1/accounts", + headers=super_user_headers, + json={ + "name": 'Expenses:bad"name\nfoo', + "account_type": "expense", + }, + ) + assert r.status_code == 400, ( + f"malformed account name must 400 before reaching the ledger: " + f"{r.status_code} {r.text}" + ) diff --git a/views_api.py b/views_api.py index 1c3269f..b41e6b6 100644 --- a/views_api.py +++ b/views_api.py @@ -13,6 +13,7 @@ from lnbits.decorators import ( ) from lnbits.utils.exchange_rates import allowed_currencies, fiat_amount_as_satoshis +from .account_utils import VALID_ACCOUNT_PREFIXES, validate_account_name from .beancount_format import fiat_rate_metadata from .crud import ( approve_manual_payment_request, @@ -295,7 +296,19 @@ async def api_create_account( auth: AuthContext = Depends(require_super_user), ) -> Account: """Create a new account (super user only)""" - return await create_account(data) + from .crud import AccountExistsError + + try: + return await create_account(data) + except ValueError as e: + raise HTTPException( + status_code=HTTPStatus.BAD_REQUEST, detail=str(e) + ) + except AccountExistsError: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail=f"Account {data.name} already exists", + ) @libra_api_router.get("/api/v1/accounts/{account_id}") @@ -2856,15 +2869,30 @@ async def api_approve_manual_payment_request( settled_entry_links=settled_links ) - # Submit to Fava - result = await fava.add_entry(entry) - logger.info(f"Manual payment entry submitted to Fava: {result.get('data', 'Unknown')}") - - # Approve the request with Fava entry reference - entry_id = f"fava-{datetime.now().timestamp()}" - return await approve_manual_payment_request( - request_id, auth.user_id, entry_id + # Claim the request BEFORE writing the ledger entry — the + # status-guarded UPDATE makes exactly one concurrent admin win, so + # only one journal entry can ever be created for this request. + approved = await approve_manual_payment_request( + request_id, auth.user_id, f"MPR-{request.id}" ) + if approved is None: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Request was already reviewed by another admin", + ) + + try: + result = await fava.add_entry(entry) + logger.info(f"Manual payment entry submitted to Fava: {result.get('data', 'Unknown')}") + except BaseException: + # Ledger write failed — release the claim so the request can be + # approved again. + from .crud import revert_manual_payment_request + + await revert_manual_payment_request(request_id) + raise + + return approved @libra_api_router.post("/api/v1/manual-payment-requests/{request_id}/reject") @@ -2887,7 +2915,13 @@ async def api_reject_manual_payment_request( detail=f"Request already {request.status}", ) - return await reject_manual_payment_request(request_id, auth.user_id) + rejected = await reject_manual_payment_request(request_id, auth.user_id) + if rejected is None: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Request was already reviewed by another admin", + ) + return rejected # ===== EXPENSE APPROVAL ENDPOINTS ===== @@ -3650,52 +3684,21 @@ async def api_get_account_hierarchy( # ===== ACCOUNT SYNC ENDPOINTS ===== -_VALID_ACCOUNT_PREFIXES = ("Assets:", "Liabilities:", "Equity:", "Income:", "Expenses:") - - -def _is_valid_account_component(component: str, *, is_root: bool) -> bool: - """Validate one ':'-separated account component against Beancount's grammar. - - Mirrors core/account.py: a root component matches ``[\\p{Lu}][\\p{L}\\p{Nd}-]*`` - (must start with an uppercase letter); a sub component matches - ``[\\p{Lu}\\p{Nd}][\\p{L}\\p{Nd}-]*`` (may also start with a digit). Body - chars are letters, decimal digits, or hyphen. Implemented with Unicode-aware - str methods (libra's runtime has no beancount — Fava is a separate service), - so non-ASCII letters are accepted exactly as Beancount accepts them. - """ - if not component: - return False - first, rest = component[0], component[1:] - first_ok = (first.isalpha() and first.isupper()) or ( - not is_root and first.isdecimal() - ) - if not first_ok: - return False - return all(ch == "-" or ch.isalpha() or ch.isdecimal() for ch in rest) +_VALID_ACCOUNT_PREFIXES = VALID_ACCOUNT_PREFIXES def _validate_account_name(name: str) -> None: """Raise HTTP 400 if ``name`` is not a syntactically valid Beancount account. - The UI guards this client-side, but the endpoint is reachable directly via - API, so this is the load-bearing check before the name is written into the - ledger source. Requires a root plus at least one sub-component. + Thin HTTP wrapper around account_utils.validate_account_name — the + single source of truth for account-name syntax (libra-#51). """ - parts = name.split(":") - valid = ( - len(parts) >= 2 - and _is_valid_account_component(parts[0], is_root=True) - and all(_is_valid_account_component(p, is_root=False) for p in parts[1:]) - ) - if not valid: + try: + validate_account_name(name) + except ValueError as e: raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, - detail=( - f"Invalid account name {name!r}: each ':'-separated part must be " - "letters/digits/hyphens, the root starting with an uppercase " - "letter (sub-accounts may start with a digit), with at least one " - "sub-account (e.g. Expenses:Food)." - ), + detail=str(e), )