aiolabs/castle
1
0
Fork 0
Code Issues 5 Pull requests 2 Projects Releases 5 Packages Wiki Activity Actions

Add centralized authorization module and fix security vulnerabilities #6

Open
padreug wants to merge 0 commits from fix/authorization-security-refactor into main
pull from: fix/authorization-security-refactor
merge into: aiolabs:main
Conversation 0 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.
padreug added 1 commit 2026-01-07 12:36:25 +00:00 
- 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>
This branch is already included in the target branch. There is nothing to merge.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/authorization-security-refactor:fix/authorization-security-refactor
git checkout fix/authorization-security-refactor

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.

git checkout main
git merge --no-ff fix/authorization-security-refactor
git checkout fix/authorization-security-refactor
git rebase main
git checkout main
git merge --ff-only fix/authorization-security-refactor
git checkout fix/authorization-security-refactor
git rebase main
git checkout main
git merge --no-ff fix/authorization-security-refactor
git checkout main
git merge --squash fix/authorization-security-refactor
git checkout main
git merge --ff-only fix/authorization-security-refactor
git checkout main
git merge fix/authorization-security-refactor
git push origin main
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/castle#6
No description provided.
Delete branch "fix/authorization-security-refactor"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?