Skip to content

Commit e3fbb96

Browse files
devondragonclaude
andcommitted
Address PR feedback: optimize token deletion and clarify password comparison
This commit addresses code review feedback from Copilot AI: **1. Clarify password comparison timing attack concern** - Added detailed comment explaining why equals() is safe for comparing two user-provided strings (newPassword vs confirmPassword) - Timing attacks only matter when comparing against stored secrets - Spring's PasswordEncoder already uses constant-time comparison for actual credential verification **2. Optimize password reset token deletion** - Added @Modifying query method deleteByToken() to PasswordResetTokenRepository - Uses direct DELETE query instead of SELECT + DELETE - Reduces database roundtrips from 2 to 1 - Updated UserService.deletePasswordResetToken() to use optimized method - Maintains logging while improving performance All tests pass (372 tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 3e19f7e commit e3fbb96

File tree

3 files changed

+19
-4
lines changed

3 files changed

+19
-4
lines changed

src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,10 @@ public ResponseEntity<JSONResponse> savePassword(@Valid @RequestBody SavePasswor
192192

193193
try {
194194
// Validate passwords match
195+
// Note: Using equals() is safe here - we're comparing two user-provided strings
196+
// from the same request (not comparing against a stored secret), so timing attacks
197+
// are not a concern. Constant-time comparison is only needed when comparing
198+
// against stored credentials, which is handled by Spring's PasswordEncoder.
195199
if (!savePasswordDto.getNewPassword().equals(savePasswordDto.getConfirmPassword())) {
196200
return buildErrorResponse(messages.getMessage("message.password.mismatch", null, locale), 1,
197201
HttpStatus.BAD_REQUEST);

src/main/java/com/digitalsanctuary/spring/user/persistence/repository/PasswordResetTokenRepository.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,4 +52,15 @@ public interface PasswordResetTokenRepository extends JpaRepository<PasswordRese
5252
@Modifying
5353
@Query("delete from PasswordResetToken t where t.expiryDate <= ?1")
5454
void deleteAllExpiredSince(Date now);
55+
56+
/**
57+
* Delete by token using a direct DELETE query without fetching the entity first.
58+
* More efficient than fetching and then deleting as it executes a single DELETE statement.
59+
*
60+
* @param token the token string to delete
61+
* @return the number of tokens deleted (0 or 1)
62+
*/
63+
@Modifying
64+
@Query("DELETE FROM PasswordResetToken t WHERE t.token = :token")
65+
int deleteByToken(@org.springframework.data.repository.query.Param("token") String token);
5566
}

src/main/java/com/digitalsanctuary/spring/user/service/UserService.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -411,17 +411,17 @@ public Optional<User> getUserByPasswordResetToken(final String token) {
411411

412412
/**
413413
* Deletes a password reset token after it has been used.
414+
* Uses a direct DELETE query for efficiency (no SELECT required).
414415
*
415416
* @param token the token string to delete
416417
*/
417418
public void deletePasswordResetToken(final String token) {
418419
if (token == null) {
419420
return;
420421
}
421-
PasswordResetToken resetToken = passwordTokenRepository.findByToken(token);
422-
if (resetToken != null) {
423-
passwordTokenRepository.delete(resetToken);
424-
log.debug("Deleted password reset token for user: {}", resetToken.getUser().getEmail());
422+
int deletedCount = passwordTokenRepository.deleteByToken(token);
423+
if (deletedCount > 0) {
424+
log.debug("Deleted password reset token: {}", token);
425425
}
426426
}
427427

0 commit comments

Comments
 (0)