bug: reject non-positive amounts in deposit/withdraw/transfer

[devin-ai-integration]

Summary

deposit, withdraw, and transferAmount never validated the sign of amount. The insufficient-funds guard is balance.compareTo(amount) < 0, which is always false for a negative amount, so any authenticated user could POST /transfer with toUsername=<victim>&amount=-100: from.balance.subtract(-100) credited the attacker and to.balance.add(-100) debited an arbitrary victim account. withdraw with a negative amount likewise inflated the caller's own balance.

Fix is a single guard in the service layer (the only place all three mutations converge), executed before any balance read or repository write:

private static void validateAmount(BigDecimal amount) {
    if (amount == null || amount.compareTo(BigDecimal.ZERO) <= 0) {
        throw new RuntimeException("Amount must be greater than zero");
    }
}

called at the top of deposit, withdraw, and transferAmount. Zero and null are rejected too.

Controller error path. BankController.deposit previously had no try/catch, so the new exception would have surfaced as a 500. All three mutating endpoints now handle failure the same way: on RuntimeException they redirect:/dashboard with the message as a flash attribute (RedirectAttributes.addFlashAttribute("error", …)) instead of rendering dashboard directly with the in-request Account object. Rendering in-request could show a balance that had been mutated in memory before a repository write failed; the redirect forces GET /dashboard to re-read the account from the database. dashboard.html already renders ${error}.

Atomicity. deposit, withdraw, and transferAmount are @Transactional, so the balance save(s) and the transaction-history write commit or roll back as one unit; a failure after the balance write no longer leaves a balance without its history row (or, for transfers, one side moved and the other not).

New AccountServiceAmountValidationTest (Mockito, no Spring context — the existing app test needs a live MySQL) covers the theft path (negative transfer throws and touches neither repository), zero/null, negative withdraw/deposit, plus happy paths for all three operations.

Verification: JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 mvn -Dtest=AccountServiceAmountValidationTest test — Tests run: 8, Failures: 0, Errors: 0, Skipped: 0. The repository has no test CI (the only check is Snyk), so this local run is the test evidence.

Re-run on the PR head (fc622dd, after the redirect and @Transactional commits): mvn -B -Dtest=AccountServiceAmountValidationTest test — Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 (surefire report TEST-com.[REDACTED SECRET].bankapp.service.AccountServiceAmountValidationTest.xml).

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/d40b21770ac144aa9a39abb9cfca43e3
Open in Devin Desktop: https://app.devin.ai/desktop/session/d40b21770ac144aa9a39abb9cfca43e3?variant=devin
Requested by: @achalc

---