Conversation
…tection Co-Authored-By: Achal Channarasappa <achal.channarasappa@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Negative-amount abuse remains in deposit and withdraw
transferAmount now rejects non-positive amounts, but deposit (AccountService.deposit) and withdraw (AccountService.withdraw) still accept any amount. withdraw with a negative value computes balance.subtract(-x), minting money. Same vulnerability class the PR targets, left unaddressed on these endpoints.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and real: withdraw with a negative amount does balance.subtract(-x), minting money, and deposit accepts negatives too. Both are pre-existing and outside this PR's scope (transfer endpoint only), so I'm not expanding the diff unilaterally — flagging for the repo owner to decide whether to fold the same validateAmount guard into deposit/withdraw here or in a follow-up.
| if (fromAccount.getBalance().compareTo(amount) < 0) { | ||
| throw new RuntimeException("Insufficient funds"); | ||
| } | ||
|
|
||
| Account toAccount = accountRepository.findByUsername(toUsername) | ||
| Account toAccount = accountRepository.findByUsername(recipient) | ||
| .orElseThrow(() -> new RuntimeException("Recipient account not found")); | ||
|
|
||
| recordIdempotencyKey(key, fromAccount.getUsername()); | ||
|
|
||
| // Deduct from sender's account | ||
| fromAccount.setBalance(fromAccount.getBalance().subtract(amount)); | ||
| accountRepository.save(fromAccount); |
There was a problem hiding this comment.
📝 Info: Concurrent distinct transfers can overdraw balance
@Transactional adds no row locking, and the idempotency key only dedupes identical requests. Two concurrent transfers from one sender with different tokens both pass the balance check on a fromAccount loaded outside the transaction, then both debit, producing a lost update. Pre-existing; not the double-submit case this PR targets.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate — @Transactional alone gives no row lock, so two concurrent transfers with distinct tokens can both pass the balance check on the detached fromAccount and overdraw. Correctly noted as pre-existing and distinct from the double-submit case this PR targets. The fix would be a pessimistic re-read of the sender (SELECT ... FOR UPDATE via @Lock(PESSIMISTIC_WRITE)) inside the transaction; leaving it out of this diff and flagging for the owner.
| String key = idempotencyKey == null ? "" : idempotencyKey.trim(); | ||
| if (key.isEmpty()) { | ||
| throw new InvalidTransferException("Missing transfer request identifier"); | ||
| } | ||
| if (processedTransferRepository.existsByIdempotencyKey(key)) { | ||
| throw new InvalidTransferException("Duplicate transfer request ignored"); | ||
| } |
There was a problem hiding this comment.
📝 Info: Idempotency key is global, not per user
existsByIdempotencyKey and the unique constraint are not scoped by fromUsername, which is stored but never used for lookup. Keys are client-supplied hidden fields. Random UUIDs make collisions negligible, so this is not a practical issue, but the key namespace is shared across all users.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct — the key namespace is global and from_username is stored but not part of the lookup or the unique constraint. With server-generated random UUIDs collisions are negligible, but scoping is stricter: a (from_username, idempotency_key) unique constraint plus existsByFromUsernameAndIdempotencyKey would keep one user's key from ever affecting another's. Small change if the owner wants it in this PR.
1 participant
Summary
POST /transferpreviously accepted anyBigDecimaland posted it with no validation and no transaction boundary. A negative amount drained the recipient (from.balance.subtract(-10)credits the sender,to.balance.add(-10)debits the recipient), and a double-submit / browser refresh posted the same transfer twice.AccountService.transferAmountnow takes an idempotency key and rejects bad requests before any balance moves:Two layers guard duplicates: the
existsByIdempotencyKeyread handles the common double-submit, and the unique constraint onprocessed_transfer.idempotency_key(claimed viasaveAndFlushbefore balances change) is authoritative for the concurrent race — aDataIntegrityViolationExceptionthere is translated into the same rejection. Adding@Transactionalalso closes the existing hole where a failure between the debit and the credit destroyed money.The key is supplied by the dashboard:
GET /dashboardputs a freshtransferTokenUUID in the model, the transfer form carries it as a hidden field, so resubmitting the same rendered page is rejected rather than posted twice.Rejections use the new
InvalidTransferException(aRuntimeException, so the controller's existing catch renders it as the dashboard error message — no behaviour change for the happy path).AccountServiceTransferTest(13 Mockito tests, no DB) covers each rejected case: negative, zero, null, sub-cent, over-limit, self-transfer (case/whitespace-insensitive), blank recipient, missing key, duplicate key, lost unique-constraint race, insufficient funds, unknown recipient, plus the successful transfer.Verified:
./mvnw clean test→ 14 tests, BUILD SUCCESS (with the local MySQL container running).Devin-Org: engineering
Link to Devin session: https://app.devin.ai/sessions/1c937d5949664b52bb653967367b8770
Requested by: @achalc
Devin Review