SeniorDebuggingCommonNot answered yet
Find the vulnerabilities in a money-transfer service on Spring
This Spring service transfers money between accounts. Review it for security defects and name the fix for each.
Constraints:
transferreads the balance and then writes it in separate steps under concurrencyamountis aBigDecimaltaken from the request with no bounds- the Spring Boot Actuator is exposed (see config below) and the H2 console is enabled
public void transfer(Long fromId, Long toId, BigDecimal amount) {
Account from = repo.findById(fromId); // no ownership check
if (from.getBalance().compareTo(amount) >= 0) { // no amount > 0 check
from.setBalance(from.getBalance().subtract(amount));
to.setBalance(to.getBalance().add(amount));
}
}
// application.yml: management.endpoints.web.exposure.include: "*"Diagnose the causes.
Race: read-check-write isn't atomic — fix with a transaction and pessimistic lock. Broken validation: a negative amount bypasses the balance check. Broken authorization: no ownership check on fromId, plus exposed Actuator (heapdump) and H2 leak data. DoS: unbounded BigDecimal — cap it. Add @ControllerAdvice against stack-trace leaks.
- ✗Fixing the race with synchronized instead of a transaction with a DB lock
- ✗Assuming Actuator and the H2 console are safe by default
- ✗Skipping the account-ownership check and the amount-sign check
- →Why is a pessimistic lock in a transaction more reliable than synchronized for a transfer?
- →How does a negative amount bypass the balance check in this code?
Contents
The vulnerabilities
public void transfer(Long fromId, Long toId, BigDecimal amount) {
Account from = repo.findById(fromId);
if (from.getBalance().compareTo(amount) >= 0) {
from.setBalance(from.getBalance().subtract(amount));
to.setBalance(to.getBalance().add(amount));
}
}- Race condition (TOCTOU). Read-check-write is not atomic: two concurrent transfers both pass the check and overdraw the balance.
- Broken input validation. No
amount > 0check: a negative amount passescompareTo(...) >= 0andsubtractcredits the sender. - Broken authorization. No check that
fromIdbelongs to the caller. Exposing*in Actuator opensheapdump; the H2 console is unauthenticated. - DoS. An unbounded
BigDecimal(huge numbers) degrades the service. - Poor error handling. Exceptions reach the screen with a stack trace.
The fix
@Transactional
public void transfer(Long fromId, Long toId, BigDecimal amount) {
if (amount.signum() <= 0 || amount.compareTo(MAX) > 0) throw new BadRequest();
Account from = repo.findByIdForUpdate(fromId); // pessimistic lock
if (!from.ownedBy(currentUser())) throw new Forbidden();
if (from.getBalance().compareTo(amount) >= 0) { /* debit/credit */ }
}✅ A transaction with a pessimistic lock removes the race; validate amount sign and range; check ownership; restrict Actuator exposure and H2; @ControllerAdvice hides stack traces.
Contents
