Repository navigation
[FLYW-210] 번호인증 구현 - #258
Merged
Hidden character warning
The head ref may contain hidden characters: "FLYW-210-\ubc88\ud638\uc778\uc99d-\uad6c\ud604"
Merged
[FLYW-210] 번호인증 구현 #258
Conversation
📝 WalkthroughWalkthrough회원가입에 전화번호 SMS 인증을 도입합니다. 서버 사이드 SmsVerificationService·도메인·매퍼·마이그레이션과 SMS API 컨트롤러를 추가하고, SignUpService에서 전화번호 인증 여부를 확인하도록 변경하며 프론트엔드에 인증 UI/JS를 추가했습니다. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant Controller as SmsVerificationApiController
participant Service as SmsVerificationService
participant DB as SmsVerificationMapper (DB)
participant Gateway as SMS Gateway
Client->>Controller: POST /api/sms/send (phoneNumber)
Controller->>Service: sendCode(phoneNumber)
Service->>DB: countRecentByPhone(phoneNumber, since)
DB-->>Service: count
alt rate limit exceeded
Service-->>Controller: throw Exception
Controller-->>Client: 400 Bad Request
else within limit
Service->>Service: generate code & id, set expiresAt
Service->>DB: insert(SmsVerification)
DB-->>Service: OK
Service->>Gateway: send SMS(code)
Gateway-->>Service: Sent
Service-->>Controller: success
Controller-->>Client: ApiResponse<Void>
end
Client->>Controller: POST /api/sms/verify (phoneNumber, code)
Controller->>Service: verify(phoneNumber, code)
Service->>DB: findLatestByPhone(phoneNumber)
DB-->>Service: SmsVerification
alt expired or mismatch
Service-->>Controller: false
Controller-->>Client: ApiResponse<Boolean> false
else valid
Service->>DB: markVerified(id, timestamp)
DB-->>Service: OK
Service-->>Controller: true
Controller-->>Client: ApiResponse<Boolean> true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/resources/mapper/user/UserProfileMapper.xml (1)
10-17:⚠️ Potential issue | 🟠 Majorphone_number 컬럼 NOT NULL 제약으로 인한 INSERT 실패 위험
phone_number 컬럼이 스키마상 NOT NULL 제약이 있습니다 (V1, V19 마이그레이션 확인). COALESCE 제거로 인해 #{phoneNumber}가 NULL일 경우, DEFAULT 값이 있어도 명시적 NULL 삽입은 INSERT 실패를 초래합니다. phone_number가 선택사항이라면 COALESCE를 복원하거나 Java 단에서 null 체크를 추가해야 합니다.
src/test/java/com/flyway/auth/service/SignUpServiceImplTest.java (1)
61-94:⚠️ Potential issue | 🔴 Critical🚨 테스트 실패: 기존 테스트들이 SMS 인증 검증으로 인해 실패합니다
SignUpServiceImpl.signUp()메서드가 이제 SMS 인증을 요구하지만, 기존 테스트들이 업데이트되지 않았습니다:
EmailSignUpRequest에phoneNumber가 설정되지 않음 →validateRequest()에서USER_INVALID_INPUT발생smsVerificationService.isVerified()에 대한 mock 설정이 없음 →USER_PHONE_NOT_VERIFIED발생🐛 테스트 수정 예시
`@Test` `@DisplayName`("회원가입 성공 시 사용자/아이덴티티/프로필이 생성된다") void signUp_success_createsUserAndIdentity() { EmailSignUpRequest request = EmailSignUpRequest.builder() .name("Tester") .email("test@example.com") .rawPassword("password") .attemptId("attempt-1") + .phoneNumber("01012345678") .build(); + when(smsVerificationService.isVerified("01012345678")).thenReturn(true); when(signUpAttemptRepository.consumeIfVerified(eq("attempt-1"), eq("test@example.com"), any())) .thenReturn(1); when(userIdentityRepository.existsEmailIdentity("test@example.com")).thenReturn(false); when(passwordEncoder.encode("password")).thenReturn("encoded"); signUpService.signUp(request);다른 테스트들도 동일하게
phoneNumber설정과isVerified()mock이 필요합니다. 또한 SMS 인증 실패 시나리오에 대한 테스트 케이스 추가를 권장합니다.src/test/java/com/flyway/auth/service/UserAuthServiceImplTest.java (1)
60-92:⚠️ Potential issue | 🔴 Critical🚨 테스트 실패: SignUpServiceImplTest와 동일한 문제
SignUpServiceImplTest와 마찬가지로, 이 테스트 클래스의 모든signUp()호출 테스트들이 실패합니다:
EmailSignUpRequest에phoneNumber미설정smsVerificationService.isVerified()mock 미설정예를 들어
signUp_duplicateEmail_throwBusinessException테스트는USER_EMAIL_ALREADY_EXISTS를 기대하지만, 실제로는 SMS 인증 검증 단계에서 먼저 실패합니다.🐛 수정이 필요한 테스트 목록
다음 테스트들에
phoneNumber설정과smsVerificationService.isVerified()mock 추가가 필요합니다:
signUp_duplicateEmail_throwBusinessExceptionsignUp_success_saveAllsignUp_passwordEncodingMissing_throwBusinessExceptionsignUp_always_checks_duplicate_first`@Test` `@DisplayName`("중복 이메일이면 BusinessException(USER_EMAIL_ALREADY_EXISTS) 발생하고 저장 로직은 수행되지 않는다") void signUp_duplicateEmail_throwBusinessException() { // given EmailSignUpRequest req = new EmailSignUpRequest(); req.setName("홍길동"); req.setEmail("dup@example.com"); req.setRawPassword("password1234"); req.setAttemptId("AttemptId"); + req.setPhoneNumber("01012345678"); + when(smsVerificationService.isVerified("01012345678")).thenReturn(true); when(userIdentityRepository.existsEmailIdentity("dup@example.com")) .thenReturn(true);
🤖 Fix all issues with AI agents
In
`@src/main/java/com/flyway/sender/controller/SmsVerificationApiController.java`:
- Around line 17-25: SmsVerificationApiController.send currently accepts any
phoneNumber and catches Exception broadly; add input validation for phoneNumber
(null/blank and regex format check) at the start of send and throw or return a
meaningful bad-request ApiResponse when invalid, referencing the method send and
ApiResponse; replace the single catch(Exception e) with specific handling: catch
validation/format errors (e.g., IllegalArgumentException or a custom
ValidationException) to return ResponseEntity.badRequest() with a clear error
code/message, and keep a separate generic catch(Exception e) that logs the full
exception and returns ResponseEntity.status(500) with
ApiResponse.error("SERVER_ERROR", "Internal server error") so unexpected errors
aren't masked; ensure service.sendCode(phoneNumber) is only called after
validation.
In `@src/main/java/com/flyway/sender/mapper/SmsVerificationMapper.java`:
- Around line 20-22: The comment for SmsVerificationMapper.existsVerifiedPhone
incorrectly states "5분 내" while SmsVerificationService.isVerified uses a
10-minute window; update the comment to match the actual behavior or remove the
hardcoded time note since the time limit is handled dynamically in
SmsVerificationService.isVerified, and ensure both
SmsVerificationMapper.existsVerifiedPhone and SmsVerificationService.isVerified
remain consistent about which timeframe is enforced or documented.
In `@src/main/java/com/flyway/sender/service/SmsService.java`:
- Around line 114-118: The log at the end of the ticket-sending block currently
prints userPhone in cleartext; change it to log a masked version of the phone
number to avoid PII exposure by adding/using a phone-masking helper (e.g.,
maskPhoneNumber) or inline masking logic and call that when logging; update the
log.info line in SmsService (the block that calls buildTicketSmsContent(...) and
sendSms(...)) to pass the masked phone value instead of userPhone, and also
ensure sendSms and any internal SMS-related logs use the same mask helper for
consistency.
In `@src/main/java/com/flyway/sender/service/SmsVerificationService.java`:
- Line 49: The line using v.getCode().equals(code) in SmsVerificationService
should be replaced with a constant-time comparison to prevent timing attacks:
convert both strings to byte[] with a fixed charset (e.g., UTF-8), guard against
nulls, and use MessageDigest.isEqual(byte[], byte[]) (or another constant-time
utility) to compare the stored code (v.getCode()) and the supplied code
parameter, returning the result of that comparison instead of String.equals().
- Line 30: The code in SmsVerificationService generates the OTP using
java.util.Random (String code = String.format("%06d", new
Random().nextInt(1000000))); which is predictable; replace it with
java.security.SecureRandom (e.g., a static final SecureRandom instance used to
call nextInt(1_000_000)) and update the import; ensure the replacement occurs
where the code variable is constructed so the formatted 6-digit string is
produced from SecureRandom.nextInt(1_000_000).
In `@src/main/webapp/resources/signup/js/signup.js`:
- Around line 366-386: Fix the syntax error and unreachable SMS-check: remove
the stray characters causing the invalid token ("}cd") and correct the closing
brace so the block compiles; then move or merge the phone verification logic
(phoneVerifiedHidden check) into the same pre-submit validation flow inside the
signupForm.submit listener (before any unconditional return) so both
emailVerifiedHidden and phoneVerifiedHidden are validated, calling
e.preventDefault() and showToast/alert for each failing check in sequence within
the handler (refer to signupForm submit listener, emailVerifiedHidden,
phoneVerifiedHidden, showToast).
🧹 Nitpick comments (9)
src/main/resources/mapper/sender/SmsMapper.xml (1)
33-35: 중복 주석 한 줄 정리 제안
동일 주석이 연속으로 들어가 있어 하나만 남기는 편이 깔끔합니다.🧹 제안 변경안
- <!-- 탑승객별 티켓 정보 (좌석/기내식/수하물 ) --> - <!-- 탑승객별 티켓 정보 (좌석/기내식/수하물 ) --> + <!-- 탑승객별 티켓 정보 (좌석/기내식/수하물 ) -->src/main/java/com/flyway/security/config/SecurityConfigApi.java (1)
73-77: /api/sms/ 공개 범위를 실제 사용하는 HTTP 메서드(POST)로 제한 권장**
현재 모든 HTTP 메서드가 공개되어 있으나, SmsVerificationApiController에서 실제 사용하는 메서드는 POST(/send, /verify)뿐입니다. 불필요한 메서드(GET, PUT, DELETE 등)를 허용하지 않도록 좁혀 공격 면을 줄일 것을 권장합니다.🔧 제안 변경안
.antMatchers( "/api/auth/**", "/api/public/**", - "/api/sms/**" - ).permitAll() + ).permitAll() + .antMatchers(HttpMethod.POST, "/api/sms/**").permitAll()src/main/resources/db/migration/V21__create_sms_verification_table.sql (1)
1-10: 스키마 설계는 적절하나, 몇 가지 고려사항이 있습니다.
동일 전화번호에 대한 중복 인증 요청 처리: 현재 스키마에서는 동일 전화번호로 여러 인증 요청이 쌓일 수 있습니다. 서비스 레이어에서 처리하거나, 인덱스 전략을 조정하는 것을 고려해 주세요.
만료된 레코드 정리:
expires_at을 활용한 주기적 정리 작업(batch job 또는 scheduled task)이 필요합니다.레코드 조회 최적화:
phone_number단독 조회가 빈번할 경우 별도 인덱스 추가를 고려해 주세요.♻️ 선택적 개선안
CREATE TABLE sms_verification ( sms_verification_id CHAR(36) NOT NULL, phone_number VARCHAR(20) NOT NULL, code VARCHAR(6) NOT NULL, expires_at DATETIME NOT NULL, verified_at DATETIME NULL, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (sms_verification_id), - INDEX idx_sms_phone (phone_number, created_at) + INDEX idx_sms_phone (phone_number, created_at), + INDEX idx_sms_expires (expires_at) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;src/main/webapp/resources/signup/js/signup.js (1)
135-144: 들여쓰기 스타일 불일치새로 추가된 SMS 관련 DOM 요소 선언들이 기존 코드와 다른 들여쓰기 스타일(4칸 추가 들여쓰기)을 사용하고 있습니다. 기존 코드 스타일과 일관성을 유지해 주세요.
src/main/java/com/flyway/auth/service/SignUpServiceImpl.java (1)
80-80: 불필요한 빈 줄
userRepository.save(user);이후에 불필요한 빈 줄이 추가되었습니다.src/main/resources/mapper/sender/SmsVerificationMapper.xml (1)
13-20:findLatestByPhone에서created_at컬럼 누락
SmsVerification도메인 객체에createdAt필드가 있다면, SELECT 절에도created_at AS createdAt을 포함해야 합니다. 현재는 조회 시 해당 필드가 null로 반환됩니다.♻️ 수정 제안
<select id="findLatestByPhone" resultType="com.flyway.sender.domain.SmsVerification"> SELECT sms_verification_id AS smsVerificationId, phone_number AS phoneNumber, code, - expires_at AS expiresAt, verified_at AS verifiedAt + expires_at AS expiresAt, verified_at AS verifiedAt, + created_at AS createdAt FROM sms_verification WHERE phone_number = #{phoneNumber} AND verified_at IS NULL ORDER BY created_at DESC LIMIT 1 </select>src/main/java/com/flyway/sender/controller/SmsVerificationApiController.java (1)
27-34:/verify엔드포인트 입력값 검증 필요
phoneNumber와code파라미터에 대한 null/빈 문자열 검증이 없습니다. 잘못된 입력이 서비스 레이어로 전달되어 예기치 않은 동작을 유발할 수 있습니다.🛡️ 수정 제안
`@PostMapping`("/verify") public ResponseEntity<ApiResponse<Boolean>> verify( `@RequestParam` String phoneNumber, `@RequestParam` String code) { + if (phoneNumber == null || phoneNumber.isBlank() || code == null || code.isBlank()) { + return ResponseEntity.badRequest() + .body(ApiResponse.error("INVALID_INPUT", "전화번호와 인증번호를 입력해주세요.")); + } boolean ok = service.verify(phoneNumber, code); String msg = ok ? "인증되었습니다." : "인증번호가 일치하지 않습니다."; return ResponseEntity.ok(ApiResponse.success(ok, msg)); }src/main/java/com/flyway/sender/service/SmsVerificationService.java (2)
27-28: 커스텀 비즈니스 예외 사용 권장
RuntimeException은 너무 일반적이며 클라이언트에서 적절한 에러 핸들링이 어렵습니다. 프로젝트의BusinessException과ErrorCode를 사용하여 일관된 예외 처리를 하세요.♻️ 수정 제안
+import com.flyway.common.exception.BusinessException; +import com.flyway.common.exception.ErrorCode; if (count >= 3) { - throw new RuntimeException("잠시 후 다시 시도해주세요."); + throw new BusinessException(ErrorCode.SMS_RATE_LIMIT_EXCEEDED); }
44-53: 인증 실패 시 로깅 추가 권장인증 실패 사유(코드 없음, 만료, 불일치)에 대한 로깅이 없어 문제 발생 시 디버깅이 어렵습니다. 보안 감사를 위해 실패 케이스도 로깅하는 것이 좋습니다.
📝 수정 제안
`@Transactional` public boolean verify(String phoneNumber, String code) { SmsVerification v = mapper.findLatestByPhone(phoneNumber); - if (v == null) return false; - if (v.getExpiresAt().isBefore(LocalDateTime.now())) return false; - if (!v.getCode().equals(code)) return false; + String masked = phoneNumber.length() >= 7 + ? phoneNumber.substring(0, 3) + "****" + phoneNumber.substring(7) + : phoneNumber; + if (v == null) { + log.warn("[SMS] 인증 실패 - 요청 없음: {}", masked); + return false; + } + if (v.getExpiresAt().isBefore(LocalDateTime.now())) { + log.warn("[SMS] 인증 실패 - 만료됨: {}", masked); + return false; + } + if (!constantTimeEquals(v.getCode(), code)) { + log.warn("[SMS] 인증 실패 - 코드 불일치: {}", masked); + return false; + } mapper.markVerified(v.getSmsVerificationId(), LocalDateTime.now()); + log.info("[SMS] 인증 성공: {}", masked); return true; }
Comment on lines
+17
to
+25
| public ResponseEntity<ApiResponse<Void>> send(@RequestParam String phoneNumber) { | ||
| try { | ||
| service.sendCode(phoneNumber); | ||
| return ResponseEntity.ok(ApiResponse.success(null, "인증번호가 발송되었습니다.")); | ||
| } catch (Exception e) { | ||
| return ResponseEntity.badRequest() | ||
| .body(ApiResponse.error("FAIL", e.getMessage())); | ||
| } | ||
| } |
There was a problem hiding this comment.
입력값 검증 및 예외 처리 개선 필요
phoneNumber에 대한 유효성 검증이 없습니다. null, 빈 문자열, 또는 잘못된 형식의 전화번호가 서비스 레이어까지 전달될 수 있습니다.Exception을 포괄적으로 catch하면 예상치 못한 오류(예: NullPointerException, DB 연결 오류)가 일반적인 "FAIL" 응답으로 처리되어 디버깅이 어려워집니다.
🛡️ 수정 제안
`@PostMapping`("/send")
-public ResponseEntity<ApiResponse<Void>> send(`@RequestParam` String phoneNumber) {
+public ResponseEntity<ApiResponse<Void>> send(`@RequestParam` String phoneNumber) {
+ if (phoneNumber == null || phoneNumber.isBlank()) {
+ return ResponseEntity.badRequest()
+ .body(ApiResponse.error("INVALID_INPUT", "전화번호를 입력해주세요."));
+ }
try {
service.sendCode(phoneNumber);
return ResponseEntity.ok(ApiResponse.success(null, "인증번호가 발송되었습니다."));
- } catch (Exception e) {
+ } catch (IllegalArgumentException e) {
return ResponseEntity.badRequest()
.body(ApiResponse.error("FAIL", e.getMessage()));
}
}🤖 Prompt for AI Agents
In `@src/main/java/com/flyway/sender/controller/SmsVerificationApiController.java`
around lines 17 - 25, SmsVerificationApiController.send currently accepts any
phoneNumber and catches Exception broadly; add input validation for phoneNumber
(null/blank and regex format check) at the start of send and throw or return a
meaningful bad-request ApiResponse when invalid, referencing the method send and
ApiResponse; replace the single catch(Exception e) with specific handling: catch
validation/format errors (e.g., IllegalArgumentException or a custom
ValidationException) to return ResponseEntity.badRequest() with a clear error
code/message, and keep a separate generic catch(Exception e) that logs the full
exception and returns ResponseEntity.status(500) with
ApiResponse.error("SERVER_ERROR", "Internal server error") so unexpected errors
aren't masked; ensure service.sendCode(phoneNumber) is only called after
validation.
Comment on lines
+20
to
+22
| // 인증 완료 여부 확인 (5분 내) | ||
| int existsVerifiedPhone(@Param("phoneNumber") String phoneNumber, | ||
| @Param("since") LocalDateTime since); |
There was a problem hiding this comment.
주석과 실제 사용이 불일치
주석에는 "5분 내"라고 되어 있지만, SmsVerificationService.isVerified()에서는 10분을 사용합니다. 주석을 정확하게 수정하거나, 시간 제한이 동적이므로 주석을 제거하세요.
📝 수정 제안
- // 인증 완료 여부 확인 (5분 내)
+ // 인증 완료 여부 확인 (since 시간 이후)
int existsVerifiedPhone(`@Param`("phoneNumber") String phoneNumber,
`@Param`("since") LocalDateTime since);🤖 Prompt for AI Agents
In `@src/main/java/com/flyway/sender/mapper/SmsVerificationMapper.java` around
lines 20 - 22, The comment for SmsVerificationMapper.existsVerifiedPhone
incorrectly states "5분 내" while SmsVerificationService.isVerified uses a
10-minute window; update the comment to match the actual behavior or remove the
hardcoded time note since the time limit is handled dynamically in
SmsVerificationService.isVerified, and ensure both
SmsVerificationMapper.existsVerifiedPhone and SmsVerificationService.isVerified
remain consistent about which timeframe is enforced or documented.
Comment on lines
114
to
+118
| try { | ||
| String content = buildTicketSmsContent(firstInfo, segments, paxInfoList); | ||
| sendSms(firstInfo.getPhoneNumber(), content); | ||
| log.info("[SMS] 티켓 발송 - passenger: {} {}, phone: {}", | ||
| firstInfo.getFirstName(), firstInfo.getLastName(), firstInfo.getPhoneNumber()); | ||
| sendSms(userPhone, content); // 회원 번호로 발송 | ||
| log.info("[SMS] 티켓 발송 - passenger: {} {}, to userPhone: {}", | ||
| firstInfo.getFirstName(), firstInfo.getLastName(), userPhone); |
There was a problem hiding this comment.
전화번호 로그 마스킹 필요(PII 노출 위험)
새 로그가 사용자 전화번호를 그대로 노출합니다. 운영 로그 접근 범위를 고려하면 마스킹이 필요합니다. (가능하면 sendSms 내부 로그도 동일하게 마스킹 권장)
🔒 제안 변경안
- log.info("[SMS] 티켓 발송 - passenger: {} {}, to userPhone: {}",
- firstInfo.getFirstName(), firstInfo.getLastName(), userPhone);
+ log.info("[SMS] 티켓 발송 - passenger: {} {}, to userPhone: {}",
+ firstInfo.getFirstName(), firstInfo.getLastName(), maskPhone(userPhone));+ private String maskPhone(String phone) {
+ if (phone == null) return null;
+ int len = phone.length();
+ if (len <= 4) return "****";
+ return phone.substring(0, Math.min(3, len)) + "****" + phone.substring(len - 4);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| String content = buildTicketSmsContent(firstInfo, segments, paxInfoList); | |
| sendSms(firstInfo.getPhoneNumber(), content); | |
| log.info("[SMS] 티켓 발송 - passenger: {} {}, phone: {}", | |
| firstInfo.getFirstName(), firstInfo.getLastName(), firstInfo.getPhoneNumber()); | |
| sendSms(userPhone, content); // 회원 번호로 발송 | |
| log.info("[SMS] 티켓 발송 - passenger: {} {}, to userPhone: {}", | |
| firstInfo.getFirstName(), firstInfo.getLastName(), userPhone); | |
| try { | |
| String content = buildTicketSmsContent(firstInfo, segments, paxInfoList); | |
| sendSms(userPhone, content); // 회원 번호로 발송 | |
| log.info("[SMS] 티켓 발송 - passenger: {} {}, to userPhone: {}", | |
| firstInfo.getFirstName(), firstInfo.getLastName(), maskPhone(userPhone)); | |
| } | |
| private String maskPhone(String phone) { | |
| if (phone == null) return null; | |
| int len = phone.length(); | |
| if (len <= 4) return "****"; | |
| return phone.substring(0, Math.min(3, len)) + "****" + phone.substring(len - 4); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/flyway/sender/service/SmsService.java` around lines 114 -
118, The log at the end of the ticket-sending block currently prints userPhone
in cleartext; change it to log a masked version of the phone number to avoid PII
exposure by adding/using a phone-masking helper (e.g., maskPhoneNumber) or
inline masking logic and call that when logging; update the log.info line in
SmsService (the block that calls buildTicketSmsContent(...) and sendSms(...)) to
pass the masked phone value instead of userPhone, and also ensure sendSms and
any internal SMS-related logs use the same mask helper for consistency.
| throw new RuntimeException("잠시 후 다시 시도해주세요."); | ||
| } | ||
|
|
||
| String code = String.format("%06d", new Random().nextInt(1000000)); |
There was a problem hiding this comment.
java.util.Random 대신 SecureRandom 사용 필요
인증 코드 생성에 java.util.Random을 사용하고 있습니다. 이는 예측 가능한 시드를 사용하므로 공격자가 다음 인증 코드를 예측할 수 있습니다. 보안에 민감한 코드 생성에는 SecureRandom을 사용해야 합니다.
🔒 수정 제안
import java.util.Random;
+import java.security.SecureRandom;
import java.util.UUID;
`@Slf4j`
`@Service`
`@RequiredArgsConstructor`
public class SmsVerificationService {
private final SmsVerificationMapper mapper;
private final SmsService smsService;
+ private final SecureRandom secureRandom = new SecureRandom();
`@Transactional`
public void sendCode(String phoneNumber) {
// 5분 내 3회 제한
int count = mapper.countRecentByPhone(phoneNumber, LocalDateTime.now().minusMinutes(5));
if (count >= 3) {
throw new RuntimeException("잠시 후 다시 시도해주세요.");
}
- String code = String.format("%06d", new Random().nextInt(1000000));
+ String code = String.format("%06d", secureRandom.nextInt(1000000));🤖 Prompt for AI Agents
In `@src/main/java/com/flyway/sender/service/SmsVerificationService.java` at line
30, The code in SmsVerificationService generates the OTP using java.util.Random
(String code = String.format("%06d", new Random().nextInt(1000000))); which is
predictable; replace it with java.security.SecureRandom (e.g., a static final
SecureRandom instance used to call nextInt(1_000_000)) and update the import;
ensure the replacement occurs where the code variable is constructed so the
formatted 6-digit string is produced from SecureRandom.nextInt(1_000_000).
| SmsVerification v = mapper.findLatestByPhone(phoneNumber); | ||
| if (v == null) return false; | ||
| if (v.getExpiresAt().isBefore(LocalDateTime.now())) return false; | ||
| if (!v.getCode().equals(code)) return false; |
There was a problem hiding this comment.
타이밍 공격 방지를 위한 상수 시간 비교 필요
String.equals()는 첫 번째 불일치 문자에서 즉시 반환하므로 응답 시간 차이를 통해 인증 코드를 유추할 수 있는 타이밍 공격에 취약합니다. MessageDigest.isEqual() 또는 상수 시간 비교를 사용하세요.
🔒 수정 제안
+import java.security.MessageDigest;
+import java.nio.charset.StandardCharsets;
`@Transactional`
public boolean verify(String phoneNumber, String code) {
SmsVerification v = mapper.findLatestByPhone(phoneNumber);
if (v == null) return false;
if (v.getExpiresAt().isBefore(LocalDateTime.now())) return false;
- if (!v.getCode().equals(code)) return false;
+ if (!constantTimeEquals(v.getCode(), code)) return false;
mapper.markVerified(v.getSmsVerificationId(), LocalDateTime.now());
return true;
}
+ private boolean constantTimeEquals(String a, String b) {
+ if (a == null || b == null) return false;
+ return MessageDigest.isEqual(
+ a.getBytes(StandardCharsets.UTF_8),
+ b.getBytes(StandardCharsets.UTF_8)
+ );
+ }🤖 Prompt for AI Agents
In `@src/main/java/com/flyway/sender/service/SmsVerificationService.java` at line
49, The line using v.getCode().equals(code) in SmsVerificationService should be
replaced with a constant-time comparison to prevent timing attacks: convert both
strings to byte[] with a fixed charset (e.g., UTF-8), guard against nulls, and
use MessageDigest.isEqual(byte[], byte[]) (or another constant-time utility) to
compare the stored code (v.getCode()) and the supplied code parameter, returning
the result of that comparison instead of String.equals().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/webapp/resources/signup/js/signup.js`:
- Around line 509-521: The SMS send flow uses await res.json() without handling
parse errors; update the send SMS handler so you only attempt JSON parsing when
response content-type is JSON or wrap res.json() in a try-catch (in the same
async function that calls fetch) and handle failures by showing
smsErrorStatus.textContent with a fallback message (e.g., data?.message if
parsed, otherwise a generic "인증번호 발송에 실패했습니다." or the raw res.text()) and ensure
smsErrorStatus.classList.remove("hidden") is called on parse or network errors;
reference the fetch call and the smsErrorStatus usage in the SMS send handler to
locate where to add the defensive parsing and error display.
🧹 Nitpick comments (4)
src/main/java/com/flyway/auth/service/SignUpServiceImpl.java (1)
164-167: SMS 인증 확인 순서가 일관성 없습니다.
signUp()메서드에서는 SMS 인증을 DB 작업 전에 먼저 확인하지만, 이 메서드에서는 이메일 업데이트(line 159)와 상태 업데이트(line 162) 이후에 확인합니다. 트랜잭션이 롤백되긴 하지만, 검증 실패 시 불필요한 DB 작업이 발생하고 코드 일관성이 떨어집니다.♻️ SMS 인증을 먼저 수행하도록 순서 변경 제안
`@Override` `@Transactional` public void completeOauthSignUp(String userId, EmailSignUpRequest request) { validateOauthRequest(request); if (userId == null || userId.isBlank()) { throw new BusinessException(ErrorCode.USER_INVALID_INPUT); } User user = userRepository.findById(userId); if (user == null || user.getStatus() != AuthStatus.ONBOARDING) { throw new BusinessException(ErrorCode.USER_INVALID_INPUT); } + // SMS 인증 확인 + if (!smsVerificationService.isVerified(request.getPhoneNumber())) { + throw new BusinessException(ErrorCode.USER_PHONE_NOT_VERIFIED); + } + String email = request.getEmail(); if (email != null && !email.isBlank()) { userRepository.updateEmail(userId, email); } userRepository.updateStatus(userId, AuthStatus.ACTIVE); - if (!smsVerificationService.isVerified(request.getPhoneNumber())) { - throw new BusinessException(ErrorCode.USER_PHONE_NOT_VERIFIED); - } - UserProfile profile = UserProfile.builder() .userId(userId) .name(request.getName()) .phoneNumber(request.getPhoneNumber()) .build(); userProfileRepository.updateProfile(profile); }src/main/java/com/flyway/security/jwt/JwtApiAuthFilter.java (1)
37-46:/api/sms(슬래시 없음) 경로 누락 가능성
startsWith("/api/sms/")만으로는/api/sms요청이 제외되지 않아 필터가 적용될 수 있습니다. 의도적으로 공개해야 한다면 정확 매치도 포함하는 편이 안전합니다.✅ 제안 수정안
- return path.startsWith("/api/public/") - || path.startsWith("/api/auth/") - || path.startsWith("/api/sms/"); + return path.startsWith("/api/public/") + || path.startsWith("/api/auth/") + || path.equals("/api/sms") + || path.startsWith("/api/sms/");src/main/java/com/flyway/security/filter/OnboardingAccessFilter.java (1)
17-29:/api/sms(슬래시 없음) 경로가 허용 목록에서 누락될 수 있음
"/api/sms/"만 허용하면/api/sms호출은 차단됩니다. 공개 경로라면 정확 매치도 포함하는 것을 권장합니다.✅ 제안 수정안
private static final String[] ALLOWED_PREFIXES = { "/signup", "/auth/", "/resources/", "/css/", "/js/", "/images/", "/favicon.ico", "/error", "/api/auth/", "/api/public/", - "/api/sms/" + "/api/sms", + "/api/sms/" };src/main/webapp/resources/signup/js/signup.js (1)
135-144: 들여쓰기가 파일의 나머지 부분과 일치하지 않습니다.새로 추가된 SMS 관련 변수 선언(lines 135-144)은 4칸 들여쓰기를 사용하지만, 기존 코드(lines 113-133)는 2칸 들여쓰기를 사용합니다. 이 파일 전체에서 일관된 들여쓰기 스타일(2칸)을 유지해 주세요.
♻️ 들여쓰기 수정 제안
- const sendSmsBtn = document.getElementById("sendSmsBtn"); - const resendSmsBtn = document.getElementById("resendSmsBtn"); - const smsStatus = document.getElementById("smsStatus"); - const smsErrorStatus = document.getElementById("smsErrorStatus"); - const smsSentBox = document.getElementById("smsSentBox"); - const smsSuccessBox = document.getElementById("smsSuccessBox"); - const smsCodeInput = document.getElementById("smsCode"); - const verifySmsBtn = document.getElementById("verifySmsBtn"); - const smsVerifyStatus = document.getElementById("smsVerifyStatus"); - const phoneVerifiedHidden = document.getElementById("phoneVerified"); + const sendSmsBtn = document.getElementById("sendSmsBtn"); + const resendSmsBtn = document.getElementById("resendSmsBtn"); + const smsStatus = document.getElementById("smsStatus"); + const smsErrorStatus = document.getElementById("smsErrorStatus"); + const smsSentBox = document.getElementById("smsSentBox"); + const smsSuccessBox = document.getElementById("smsSuccessBox"); + const smsCodeInput = document.getElementById("smsCode"); + const verifySmsBtn = document.getElementById("verifySmsBtn"); + const smsVerifyStatus = document.getElementById("smsVerifyStatus"); + const phoneVerifiedHidden = document.getElementById("phoneVerified");
Comment on lines
+509
to
+521
| try { | ||
| const res = await fetch(`${contextPath}/api/sms/send?phoneNumber=${encodeURIComponent(phone)}`, { | ||
| method: "POST" | ||
| }); | ||
| const data = await res.json(); | ||
|
|
||
| if (!res.ok) { | ||
| if (smsErrorStatus) { | ||
| smsErrorStatus.textContent = data?.message || "인증번호 발송에 실패했습니다."; | ||
| smsErrorStatus.classList.remove("hidden"); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
JSON 파싱 오류 처리가 누락되었습니다.
res.json() 호출이 try-catch 없이 직접 사용됩니다. 서버가 JSON이 아닌 응답(예: 500 에러 HTML 페이지)을 반환하면 파싱 오류가 발생하여 사용자에게 적절한 피드백을 제공하지 못할 수 있습니다.
이메일 인증 코드(lines 254-263)와 같은 방어적 패턴을 적용하는 것을 권장합니다.
🛡️ 수정 제안
try {
const res = await fetch(`${contextPath}/api/sms/send?phoneNumber=${encodeURIComponent(phone)}`, {
method: "POST"
});
- const data = await res.json();
+
+ let data = null;
+ try {
+ data = await res.json();
+ } catch (e) {
+ data = {};
+ }
if (!res.ok) {🤖 Prompt for AI Agents
In `@src/main/webapp/resources/signup/js/signup.js` around lines 509 - 521, The
SMS send flow uses await res.json() without handling parse errors; update the
send SMS handler so you only attempt JSON parsing when response content-type is
JSON or wrap res.json() in a try-catch (in the same async function that calls
fetch) and handle failures by showing smsErrorStatus.textContent with a fallback
message (e.g., data?.message if parsed, otherwise a generic "인증번호 발송에 실패했습니다."
or the raw res.text()) and ensure smsErrorStatus.classList.remove("hidden") is
called on parse or network errors; reference the fetch call and the
smsErrorStatus usage in the SMS send handler to locate where to add the
defensive parsing and error display.
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 설명
번호인증 구현
✅ 완료한 기능 명세
📸 스크린샷
💭 고민과 해결과정
🔗 관련 이슈
Closes #255
Summary by CodeRabbit
New Features
Chores
Tests