Handle malformed wallet deposit bodies#187
Conversation
Greptile SummaryThis PR hardens the wallet deposit endpoint against malformed input by extracting body parsing into a helper that catches JSON parse errors and rejects non-object payloads, and by tightening the
Confidence Score: 5/5Safe to merge — the changes are narrowly scoped to input validation and do not touch any payment, balance, or persistence logic. The body-parsing helper correctly handles all problematic inputs (NaN, Infinity, floats, null, arrays, missing key, parse errors). The amount guard is stricter than the original and covers every edge case. Tests are accurate, mocks are well-isolated, and no existing behaviour is altered. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /api/wallet/deposit] --> B[getAuthContext]
B -->|null| C[401 Unauthorized]
B -->|user| D[parseDepositRequestBody]
D -->|parse error / non-object| E[400 Invalid request body]
D -->|valid object| F{amount_sats valid?\nnumber, integer,\n1-1,000,000}
F -->|no| G[400 Invalid amount]
F -->|yes| H[createServiceClient]
H --> I[getUserLnWallet]
I -->|null| J[400 No Lightning wallet found]
I -->|wallet| K[createInvoice]
K --> L[getLnBalance]
L --> M{wallet row exists?}
M -->|no| N[insert wallets row]
M -->|yes| O[insert wallet_transactions pending]
N --> O
O --> P[200 ok + payment_request + payment_hash]
Reviews (3): Last reviewed commit: "Handle malformed wallet deposit bodies" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("rejects non-integer amounts without wallet lookup", async () => { | ||
| const res = await POST(makeRequest({ amount_sats: 100.5 })); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| const data = await res.json(); | ||
| expect(data.error).toBe("Invalid amount (1-1,000,000 sats)"); | ||
| expect(mockCreateServiceClient).not.toHaveBeenCalled(); | ||
| expect(mockGetUserLnWallet).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The amount tests only cover a float (100.5), but amount_sats <= 0 means both 0 and any negative integer slip through the test suite without explicit coverage. For a financial endpoint, 0, -1, and 1 (the valid lower boundary) should each be tested directly — a future maintainer could accidentally invert the comparison and the existing tests would not catch it.
| it("creates an invoice for a valid deposit amount", async () => { | ||
| const walletSelectSingle = vi.fn().mockResolvedValue({ | ||
| data: { id: "wallet-1", balance_sats: 250 }, | ||
| }); | ||
| const walletEq = vi.fn().mockReturnValue({ single: walletSelectSingle }); | ||
| const walletSelect = vi.fn().mockReturnValue({ eq: walletEq }); | ||
| const transactionInsert = vi.fn().mockResolvedValue({ data: null }); | ||
| const admin = { | ||
| from: vi.fn((table: string) => { | ||
| if (table === "wallets") { | ||
| return { | ||
| select: walletSelect, | ||
| insert: vi.fn().mockResolvedValue({ data: null }), | ||
| }; | ||
| } | ||
| if (table === "wallet_transactions") { | ||
| return { insert: transactionInsert }; | ||
| } | ||
| return {}; | ||
| }), | ||
| }; | ||
| mockCreateServiceClient.mockReturnValue(admin); | ||
|
|
||
| const res = await POST(makeRequest({ amount_sats: 100 })); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| const data = await res.json(); | ||
| expect(data).toEqual({ | ||
| ok: true, | ||
| payment_request: "lnbc123", | ||
| payment_hash: "hash-123", | ||
| amount_sats: 100, | ||
| }); | ||
| expect(mockCreateInvoice).toHaveBeenCalledWith( | ||
| "invoice-key-123", | ||
| 100, | ||
| "ugig.net deposit (user-123)", | ||
| ); | ||
| expect(transactionInsert).toHaveBeenCalledWith({ | ||
| user_id: "user-12345678", | ||
| type: "deposit", | ||
| amount_sats: 100, | ||
| balance_after: 250, | ||
| bolt11: "lnbc123", | ||
| payment_hash: "hash-123", | ||
| status: "pending", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The code path at line 49–53 of route.ts (if (!lnWallet)) returns a 400 with "No Lightning wallet found" but has no corresponding test. If getUserLnWallet is changed to return null/undefined for new users, a regression here would be silently missed.
2 participants
Summary
amount_satsto be a whole-number sat amount before wallet/invoice workFixes #186
uGig task
Submitted for the active uGig repo testing task: https://ugig.net/gigs/4741218f-a723-46bb-82cb-6516120331ae
No payout details included; those can be provided after acceptance.
Tests