♻️ refactor: SMS 인증 무차별 대입 방어 추가#137
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSMS 인증 검증에 Redis 기반 실패 카운터를 도입하여 시도 횟수 초과 시 코드를 무효화하는 브루트포스 방어 기능을 추가했다. 새 에러 코드, RedisUtil의 원자적 increment 메서드, 로그 마스킹, 주석 정정, 관련 단위 테스트가 포함되었다. ChangesSMS 인증 브루트포스 방어
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService
participant RedisUtil
Client->>AuthService: verifySms(전화번호, 코드)
AuthService->>RedisUtil: get(인증코드 키)
alt 코드 없음
AuthService-->>Client: AuthException(VERIFICATION_CODE_EXPIRED)
else 코드 존재
alt 코드 일치
AuthService->>RedisUtil: delete(인증코드/실패카운터 키)
AuthService-->>Client: 검증 성공 응답
else 코드 불일치
AuthService->>RedisUtil: increment(실패 카운터 키)
RedisUtil-->>AuthService: 누적 실패 횟수
alt 한도 도달
AuthService->>RedisUtil: delete(인증코드/실패카운터 키)
AuthService-->>Client: AuthException(VERIFICATION_ATTEMPTS_EXCEEDED)
else 한도 미만
AuthService-->>Client: AuthException(INVALID_VERIFICATION_CODE, 남은 시도 횟수)
end
end
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/example/scoi/domain/auth/service/AuthServiceTest.java (2)
93-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win한도 미만 경계값(4) 테스트 추가 권장.
현재
mismatchUnderLimit()는 failCount=2로 임의 값을 사용합니다.>=비교의 정확한 경계(한도 5 바로 아래인 failCount=4, remainingAttempts=1)를 별도로 검증하면 off-by-one 회귀를 더 확실히 방지할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/scoi/domain/auth/service/AuthServiceTest.java` around lines 93 - 112, Add a dedicated boundary test for the `AuthServiceTest.mismatchUnderLimit` path that uses `authService.verifySms(...)` with the failure counter at 4, not 2, so the `INVALID_VERIFICATION_CODE` branch is validated exactly at the `>= 5` cutoff. Reuse the same `redisUtil.get`, `redisUtil.increment`, and `verify(redisUtil, never())` assertions, and additionally assert that `remainingAttempts` is 1 to cover the off-by-one boundary in `verifySms`.
60-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win성공 테스트에 토큰 저장 검증 추가 권장.
success()는 응답 값과 코드/실패카운터 삭제만 검증하고,redisUtil.set(tokenKey, ...)호출(검증 토큰의 Redis 저장)은 검증하지 않습니다. 토큰 영속화는 이 인증 흐름의 핵심 부분이므로, 키 구성(VERIFICATION_PREFIX + token)과 저장되는 전화번호 값을 함께 검증하면 회귀를 더 잘 잡을 수 있습니다.♻️ 제안 diff
AuthResDTO.SmsVerifyResponse response = authService.verifySms(request(CODE)); // then assertThat(response.verificationToken()).isEqualTo("verification-token"); assertThat(response.isExistingMember()).isTrue(); verify(redisUtil).delete(SMS_KEY); // 코드 삭제 verify(redisUtil).delete(FAIL_KEY); // 실패 카운터 초기화 + verify(redisUtil).set(eq("verification:verification-token"), eq(PHONE), anyLong(), eq(TimeUnit.MINUTES));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/scoi/domain/auth/service/AuthServiceTest.java` around lines 60 - 76, `AuthServiceTest.success()` only checks the response and Redis deletions, but it misses the token persistence step in `AuthService.verifySms`. Update the success test to also verify `redisUtil.set(...)` is called with the verification-token-derived Redis key (using the `VERIFICATION_PREFIX` + token pattern) and the expected phone number value. Keep the existing assertions for `jwtUtil.createVerificationToken`, `redisUtil.delete`, and `memberRepository.existsByPhoneNumber`, and use the `success()` test plus `verifySms` as the main reference points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/scoi/global/redis/RedisUtil.java`:
- Around line 61-70: The Redis increment logic in RedisUtil.increment currently
sets the TTL with a separate expire call after INCR, so a failure between those
steps can leave the counter without expiration. Update increment to make the
increment-and-expire flow atomic, either by wrapping the operations in a Lua
script or a transaction, or by explicitly handling expire failures and
retrying/raising as appropriate. Keep the fix centered on RedisUtil.increment
and preserve the existing validateInput and logging behavior.
---
Nitpick comments:
In `@src/test/java/com/example/scoi/domain/auth/service/AuthServiceTest.java`:
- Around line 93-112: Add a dedicated boundary test for the
`AuthServiceTest.mismatchUnderLimit` path that uses `authService.verifySms(...)`
with the failure counter at 4, not 2, so the `INVALID_VERIFICATION_CODE` branch
is validated exactly at the `>= 5` cutoff. Reuse the same `redisUtil.get`,
`redisUtil.increment`, and `verify(redisUtil, never())` assertions, and
additionally assert that `remainingAttempts` is 1 to cover the off-by-one
boundary in `verifySms`.
- Around line 60-76: `AuthServiceTest.success()` only checks the response and
Redis deletions, but it misses the token persistence step in
`AuthService.verifySms`. Update the success test to also verify
`redisUtil.set(...)` is called with the verification-token-derived Redis key
(using the `VERIFICATION_PREFIX` + token pattern) and the expected phone number
value. Keep the existing assertions for `jwtUtil.createVerificationToken`,
`redisUtil.delete`, and `memberRepository.existsByPhoneNumber`, and use the
`success()` test plus `verifySms` as the main reference points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 20c88764-8003-4878-8aea-46f6a77d100b
📒 Files selected for processing (4)
src/main/java/com/example/scoi/domain/auth/exception/code/AuthErrorCode.javasrc/main/java/com/example/scoi/domain/auth/service/AuthService.javasrc/main/java/com/example/scoi/global/redis/RedisUtil.javasrc/test/java/com/example/scoi/domain/auth/service/AuthServiceTest.java
Comment on lines
+61
to
+70
| public long increment(String key, long timeout, TimeUnit unit) { | ||
| validateInput(key); | ||
|
|
||
| Long count = redisTemplate.opsForValue().increment(key); | ||
| if (count != null && count == 1L) { | ||
| redisTemplate.expire(key, timeout, unit); | ||
| } | ||
| log.debug("Redis 증가: key={}, count={}", key, count); | ||
| return count == null ? 0L : count; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# RedisTemplate<String,String> 빈 및 value serializer 설정 확인
rg -nP -C3 'RedisTemplate<\s*String\s*,\s*String\s*>|setValueSerializer|StringRedisSerializer|GenericJackson2JsonRedisSerializer' --type javaRepository: UMCSCOI/Backend
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Redis-related Java files ==\n'
git ls-files 'src/main/java/**/*.java' | rg 'Redis|redis' || true
printf '\n== RedisUtil outline ==\n'
fd -a 'RedisUtil.java' src/main/java || true
printf '\n== Redis config/search ==\n'
rg -n -C 3 'RedisTemplate|RedisConnectionFactory|StringRedisSerializer|GenericJackson2JsonRedisSerializer|opsForValue\(\)\.increment|expire\(' src/main/java || trueRepository: UMCSCOI/Backend
Length of output: 6670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== RedisUtil.java relevant lines ==\n'
sed -n '1,120p' src/main/java/com/example/scoi/global/redis/RedisUtil.java | cat -n
printf '\n== increment() call sites ==\n'
rg -n -C 2 '\.increment\(' src/main/java || trueRepository: UMCSCOI/Backend
Length of output: 4603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AuthService increment usage ==\n'
sed -n '110,160p' src/main/java/com/example/scoi/domain/auth/service/AuthService.java | cat -nRepository: UMCSCOI/Backend
Length of output: 2794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SMS prefixes and send flow ==\n'
rg -n -C 3 'SMS_VERIFY_FAIL_PREFIX|cooldownKey|failKey|verificationCode|SMS_EXPIRATION_MINUTES|sendSms\(' src/main/java/com/example/scoi/domain/auth/service/AuthService.java || trueRepository: UMCSCOI/Backend
Length of output: 4116
INCR 뒤 TTL을 원자적으로 보장하세요. StringRedisSerializer가 사용되어 직렬화 문제는 없지만, count == 1L일 때만 expire를 별도 호출하면 중간 실패 시 실패 카운터가 TTL 없이 남을 수 있습니다. expire 실패를 처리하거나 Lua/트랜잭션으로 묶어 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/example/scoi/global/redis/RedisUtil.java` around lines 61 -
70, The Redis increment logic in RedisUtil.increment currently sets the TTL with
a separate expire call after INCR, so a failure between those steps can leave
the counter without expiration. Update increment to make the
increment-and-expire flow atomic, either by wrapping the operations in a Lua
script or a transaction, or by explicitly handling expire failures and
retrying/raising as appropriate. Keep the fix centered on RedisUtil.increment
and preserve the existing validateInput and logging behavior.
kingmingyu
approved these changes
Jul 9, 2026
kingmingyu
left a comment
kingmingyu
left a comment
Collaborator
There was a problem hiding this comment.
인증 횟수 제한 좋은 것 같습니다! 고생하셨습니다!
Member
|
@komascode 이제부터 코드리뷰를 코드래빗이 대신 해줄거예요! 해당 리뷰 보고 수정할 사항 있다면 수정하고 별도로 없고 CI 파이프라인 통과하면 바로 머지해도 됩니다~! |
Collaborator
Author
|
코멘트 주신 거 확인했습니다! 감사합니다 |
INCR과 EXPIRE가 별도 왕복이라 중간 실패 시 TTL 없는 카운터가 남을 수 있었다. 두 명령을 단일 스크립트로 묶어 한 번의 왕복으로 실행한다. MULTI/EXEC는 INCR 반환값을 읽어 EXPIRE 여부를 분기할 수 없어 사용하지 않았다. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sendSms가 failKey를 지우지 않아 실패 카운터가 코드 수명을 넘어 이월됐다. 카운터 TTL은 첫 실패 기준, 코드 TTL은 발급 기준이라 두 창이 정렬되지 않아 4회 실패 후 새 코드를 받으면 오답 한 번에 즉시 무효화됐다. 새 코드에 새 시도 예산을 부여한다. 쿨다운(1분)이 재발급 남용을 막는다. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📍 PR 타입 (하나 이상 선택)
❗️ 관련 이슈 링크
Closed #136
📌 개요
/auth/sms/verify)에 시도 횟수 제한이 없어, 5분 유효창 내에 6자리 코드(100만 조합)를 무차별 대입할 수 있었습니다. 실패 카운터를 추가해 이를 차단합니다.🔁 변경 사항
remainingAttempts) 응답sendSms발송 성공 로그에서 인증번호(code)를 제거했습니다. (운영 로그 유출 방지)verifySms분기 로직 단위 테스트 4종 추가 (성공 / 코드 만료 / 한도 미만 / 한도 초과)📸 스크린샷
👀 기타 더 이야기해볼 점
✅ 체크 리스트
Summary by CodeRabbit
New Features
Bug Fixes
Tests