aiolabs/libra
8
0
Fork 1
Code Issues 36 Pull requests Projects Releases 6 Packages Wiki Activity Actions

Add centralized authorization module and fix security vulnerabilities #6

Closed
padreug wants to merge 0 commits from fix/authorization-security-refactor into main
pull from: fix/authorization-security-refactor
merge into: aiolabs:main
Conversation 1 Commits - Files changed -
padreug commented 2026-01-07 12:36:25 +00:00
Owner
Copy link

Summary

This PR introduces a centralized authorization system and fixes critical security vulnerabilities in Castle's API endpoints. The changes improve security, reduce code duplication, and establish consistent patterns for access control.

Problem Statement

The previous authorization implementation had several issues:

  1. CRITICAL: Unprotected endpoints - Several endpoints exposing sensitive data had no authentication at all
  2. Duplicated super_user checks - 16+ admin endpoints had copy-pasted authorization boilerplate
  3. Inconsistent identity handling - Mixed use of wallet.wallet.id (wallet ID) vs wallet.wallet.user (user ID)
  4. No centralized auth pattern - Each endpoint implemented authorization differently

Security Vulnerabilities Fixed

CRITICAL - Previously Unprotected Endpoints

These endpoints had zero authentication and exposed sensitive data to anyone:

Endpoint Exposure Fix
GET /api/v1/accounts/{id} Any account details Now requires auth + account access
GET /api/v1/accounts/{id}/balance Any account balance Now requires auth + account access
GET /api/v1/accounts/{id}/transactions All account transactions Now requires auth + account access
GET /api/v1/entries ALL journal entries Now requires super_user
GET /api/v1/balance/{user_id} Any user's balance Now requires self-or-super_user
GET /api/v1/balances/all All user balances Now requires super_user

HIGH - Admin Endpoints Without Super User Check

These endpoints used require_admin_key but didn't verify super_user status, allowing any user with an admin key to access them:

  • Balance assertion CRUD (create, read, check, delete)
  • Expense approval/rejection
  • Manual payment request approval/rejection
  • Reconciliation endpoints
  • Daily reconciliation task
  • User management (/api/v1/users, /api/v1/admin/castle-users)
  • User wallet info (/api/v1/user-wallet/{user_id})

Changes

New File: auth.py

Centralized authorization module providing:

# Core context capturing all auth state
@dataclass
class AuthContext:
    user_id: str
    wallet_id: str
    is_super_user: bool
    wallet: WalletTypeInfo

# FastAPI dependencies
require_authenticated      # Read access (invoice key)
require_authenticated_write # Write access (admin key)  
require_super_user         # Super user only

# Resource-level access checks
can_access_account(auth, account_id, permission_type)
require_account_access(auth, account_id, permission_type)
can_access_user_data(auth, target_user_id)
require_user_data_access(auth, target_user_id)

Modified: views_api.py

Before:

@castle_api_router.get("/api/v1/entries")
async def api_get_journal_entries(limit: int = 100) -> list[dict]:
    # NO AUTH AT ALL - anyone can see all entries!
    ...

@castle_api_router.post("/api/v1/assertions")
async def api_create_balance_assertion(
    data: CreateBalanceAssertion,
    wallet: WalletTypeInfo = Depends(require_admin_key),
) -> BalanceAssertion:
    from lnbits.settings import settings as lnbits_settings
    
    if wallet.wallet.user != lnbits_settings.super_user:
        raise HTTPException(
            status_code=HTTPStatus.FORBIDDEN,
            detail="Only super user can create balance assertions",
        )
    # ... duplicated in 16+ endpoints

After:

@castle_api_router.get("/api/v1/entries")
async def api_get_journal_entries(
    limit: int = 100,
    auth: AuthContext = Depends(require_super_user),  # Protected!
) -> list[dict]:
    ...

@castle_api_router.post("/api/v1/assertions")
async def api_create_balance_assertion(
    data: CreateBalanceAssertion,
    auth: AuthContext = Depends(require_super_user),  # Clean, consistent
) -> BalanceAssertion:
    # No boilerplate - auth is handled by dependency
    ...

Fixed: wallet_id vs user_id

Changed 5 occurrences of wallet.wallet.id to wallet.wallet.user:

# Before (WRONG - wallet_id is NOT user_id)
entry_meta["created-by"] = wallet.wallet.id
created_by=wallet.wallet.id

# After (CORRECT)
entry_meta["created-by"] = wallet.wallet.user
created_by=wallet.wallet.user

Impact

  • Net -89 lines of code by eliminating duplicated authorization boilerplate
  • 6 critical vulnerabilities fixed (unprotected endpoints)
  • 16+ endpoints consolidated to use centralized auth
  • Consistent patterns for future endpoint development

Testing Checklist

  • Verify unprotected endpoints now return 401/403 without auth
  • Verify super_user endpoints work for super user
  • Verify super_user endpoints return 403 for regular admin keys
  • Verify account access checks work for user-owned accounts
  • Verify balance/{user_id} allows self-access but blocks cross-user access
  • Verify journal entry creation records correct user_id (not wallet_id)

Related Issues

Addresses security concerns identified in authorization/roles review.

## Summary This PR introduces a centralized authorization system and fixes critical security vulnerabilities in Castle's API endpoints. The changes improve security, reduce code duplication, and establish consistent patterns for access control. ## Problem Statement The previous authorization implementation had several issues: 1. **CRITICAL: Unprotected endpoints** - Several endpoints exposing sensitive data had no authentication at all 2. **Duplicated super_user checks** - 16+ admin endpoints had copy-pasted authorization boilerplate 3. **Inconsistent identity handling** - Mixed use of `wallet.wallet.id` (wallet ID) vs `wallet.wallet.user` (user ID) 4. **No centralized auth pattern** - Each endpoint implemented authorization differently ## Security Vulnerabilities Fixed ### CRITICAL - Previously Unprotected Endpoints These endpoints had **zero authentication** and exposed sensitive data to anyone: | Endpoint | Exposure | Fix | |----------|----------|-----| | `GET /api/v1/accounts/{id}` | Any account details | Now requires auth + account access | | `GET /api/v1/accounts/{id}/balance` | Any account balance | Now requires auth + account access | | `GET /api/v1/accounts/{id}/transactions` | All account transactions | Now requires auth + account access | | `GET /api/v1/entries` | **ALL journal entries** | Now requires super_user | | `GET /api/v1/balance/{user_id}` | Any user's balance | Now requires self-or-super_user | | `GET /api/v1/balances/all` | All user balances | Now requires super_user | ### HIGH - Admin Endpoints Without Super User Check These endpoints used `require_admin_key` but didn't verify `super_user` status, allowing any user with an admin key to access them: - Balance assertion CRUD (create, read, check, delete) - Expense approval/rejection - Manual payment request approval/rejection - Reconciliation endpoints - Daily reconciliation task - User management (`/api/v1/users`, `/api/v1/admin/castle-users`) - User wallet info (`/api/v1/user-wallet/{user_id}`) ## Changes ### New File: `auth.py` Centralized authorization module providing: ```python # Core context capturing all auth state @dataclass class AuthContext: user_id: str wallet_id: str is_super_user: bool wallet: WalletTypeInfo # FastAPI dependencies require_authenticated # Read access (invoice key) require_authenticated_write # Write access (admin key) require_super_user # Super user only # Resource-level access checks can_access_account(auth, account_id, permission_type) require_account_access(auth, account_id, permission_type) can_access_user_data(auth, target_user_id) require_user_data_access(auth, target_user_id) ``` ### Modified: `views_api.py` **Before:** ```python @castle_api_router.get("/api/v1/entries") async def api_get_journal_entries(limit: int = 100) -> list[dict]: # NO AUTH AT ALL - anyone can see all entries! ... @castle_api_router.post("/api/v1/assertions") async def api_create_balance_assertion( data: CreateBalanceAssertion, wallet: WalletTypeInfo = Depends(require_admin_key), ) -> BalanceAssertion: from lnbits.settings import settings as lnbits_settings if wallet.wallet.user != lnbits_settings.super_user: raise HTTPException( status_code=HTTPStatus.FORBIDDEN, detail="Only super user can create balance assertions", ) # ... duplicated in 16+ endpoints ``` **After:** ```python @castle_api_router.get("/api/v1/entries") async def api_get_journal_entries( limit: int = 100, auth: AuthContext = Depends(require_super_user), # Protected! ) -> list[dict]: ... @castle_api_router.post("/api/v1/assertions") async def api_create_balance_assertion( data: CreateBalanceAssertion, auth: AuthContext = Depends(require_super_user), # Clean, consistent ) -> BalanceAssertion: # No boilerplate - auth is handled by dependency ... ``` ### Fixed: wallet_id vs user_id Changed 5 occurrences of `wallet.wallet.id` to `wallet.wallet.user`: ```python # Before (WRONG - wallet_id is NOT user_id) entry_meta["created-by"] = wallet.wallet.id created_by=wallet.wallet.id # After (CORRECT) entry_meta["created-by"] = wallet.wallet.user created_by=wallet.wallet.user ``` ## Impact - **Net -89 lines of code** by eliminating duplicated authorization boilerplate - **6 critical vulnerabilities fixed** (unprotected endpoints) - **16+ endpoints consolidated** to use centralized auth - **Consistent patterns** for future endpoint development ## Testing Checklist - [ ] Verify unprotected endpoints now return 401/403 without auth - [ ] Verify super_user endpoints work for super user - [ ] Verify super_user endpoints return 403 for regular admin keys - [ ] Verify account access checks work for user-owned accounts - [ ] Verify balance/{user_id} allows self-access but blocks cross-user access - [ ] Verify journal entry creation records correct user_id (not wallet_id) ## Related Issues Addresses security concerns identified in authorization/roles review. 
- Create auth.py with AuthContext, require_super_user, require_authenticated
- Fix 6 CRITICAL unprotected endpoints exposing sensitive data
- Consolidate 16+ admin endpoints with duplicated super_user checks
- Standardize on user_id (wallet.wallet.user) instead of wallet_id

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
padreug commented 2026-05-07 06:12:06 +00:00
Author
Owner
Copy link

Closing — this work landed on main via merge commit 5eb007b ("Merge branch 'fix/authorization-security-refactor'"), bringing in ca0cee7 ("Add centralized authorization module and fix security vulnerabilities").

Verified locally:

  • auth.py exists with AuthContext, require_authenticated, require_authenticated_write, require_super_user, can_access_account, require_account_access, can_access_user_data, require_user_data_access
  • views_api.py endpoints use the centralized dependencies
$ git branch --contains ca0cee7
* main

No further action needed.

Closing — this work landed on `main` via merge commit `5eb007b` ("Merge branch 'fix/authorization-security-refactor'"), bringing in `ca0cee7` ("Add centralized authorization module and fix security vulnerabilities"). Verified locally: - `auth.py` exists with `AuthContext`, `require_authenticated`, `require_authenticated_write`, `require_super_user`, `can_access_account`, `require_account_access`, `can_access_user_data`, `require_user_data_access` - `views_api.py` endpoints use the centralized dependencies ``` $ git branch --contains ca0cee7 * main ``` No further action needed.
padreug closed this pull request 2026-05-07 06:12:17 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aiolabs/libra!6
No description provided.