Feature : Github OAuth 구현, OAuth 통합 컨트롤러, AES 암호화, Github access token 저장 #105
Conversation
…ithubUrl이 자동 추가되도록함
minjee2758
requested review from
Kimminu7,
NCookies,
chat26666,
pokerbearkr and
thezz9
minjee2758
added
documentation
|
""" Walkthrough이 변경사항은 GitHub OAuth2 로그인을 지원하기 위해 여러 계층에 걸쳐 기능을 추가하고, AES 암호화 유틸리티를 도입하며, OAuth2 인증 성공 시 GitHub 액세스 토큰을 암호화하여 저장하는 로직을 구현합니다. 또한, OAuth2 관련 API 엔드포인트와 설정이 추가 및 확장되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2Controller
participant SpringSecurity
participant OAuthProvider as "GitHub/Google"
participant CustomSuccessHandler
participant AESUtil
participant UserDomainService
participant UserRepository
Client->>OAuth2Controller: /api/oauth2/authorize/{provider}?redirect_uri=
OAuth2Controller->>Client: Redirect /oauth2/authorization/{provider}
Client->>SpringSecurity: /oauth2/authorization/{provider}
SpringSecurity->>OAuthProvider: OAuth2 인증 요청
OAuthProvider-->>SpringSecurity: 인증 응답 (access token 등)
SpringSecurity->>CustomSuccessHandler: 인증 성공 콜백
CustomSuccessHandler->>AESUtil: (GitHub일 경우) access token 암호화
AESUtil-->>CustomSuccessHandler: 암호화된 토큰 반환
CustomSuccessHandler->>UserDomainService: 암호화 토큰 User에 저장 요청
UserDomainService->>UserRepository: updateUserGithubAccessToken
UserRepository-->>UserDomainService: 저장 완료
CustomSuccessHandler->>Client: JWT access/refresh 토큰 JSON 반환 (또는 redirect)
Possibly related PRs
Suggested reviewers
Poem
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
src/main/java/org/ezcode/codetest/domain/user/model/entity/CustomOAuth2User.java (1)
33-37: GitHub ID 기반 이메일 생성 시 검증 로직 추가 고려null 이메일 처리 로직이 올바르게 구현되었지만, 다음 사항들을 고려해보세요:
- GitHub ID가 특수문자를 포함할 경우 유효하지 않은 이메일이 생성될 수 있습니다.
@github.com도메인을 사용하는 것이 실제 GitHub 이메일과 혼동을 일으킬 수 있습니다.더 안전한 구현을 위해 다음과 같이 개선을 제안합니다:
public String getEmail() { if (oAuth2Response.getEmail() == null) { - return oAuth2Response.getGithubId()+"@github.com"; + return oAuth2Response.getGithubId().replaceAll("[^a-zA-Z0-9]", "") + "@github.local"; } else { return oAuth2Response.getEmail(); } }src/main/java/org/ezcode/codetest/domain/user/model/entity/UserFactory.java (1)
8-29: 팩토리 메서드 개선 제안현재 구현은 좋지만 몇 가지 개선점이 있습니다:
- 지원하지 않는 provider에 대한 명시적 예외 처리
- 패스워드 생성 로직 중복 제거
- 메서드 파라미터 검증
+private static final Set<String> SUPPORTED_PROVIDERS = Set.of("github", "google"); + +private static String generateRandomPassword() { + return UUID.randomUUID().toString(); +} + public static User createSocialUser( OAuth2Response response, String nickname, String provider ) { + if (provider == null || provider.trim().isEmpty()) { + throw new IllegalArgumentException("Provider cannot be null or empty"); + } + + String normalizedProvider = provider.toLowerCase(); + if (!SUPPORTED_PROVIDERS.contains(normalizedProvider)) { + throw new IllegalArgumentException("Unsupported OAuth2 provider: " + provider); + } + - return switch (provider.toLowerCase()) { + return switch (normalizedProvider) { case "github" -> User.githubUser( response.getEmail(), response.getName(), nickname, - UUID.randomUUID().toString(), + generateRandomPassword(), response.getGithubUrl() ); default -> User.socialUser( response.getEmail(), response.getName(), nickname, - UUID.randomUUID().toString() + generateRandomPassword() ); }; }src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GithubOAuth2Response.java (1)
29-35: 이메일 처리 로직 개선현재 이메일 fallback 로직이 명확하지만, provider 체크가 불필요하고 null 안전성을 개선할 수 있습니다.
@Override public String getEmail() { - if (attributes.get("email") == null && getProvider().equals("github")) { - return getGithubId()+"@github.com"; - } else { - return (String) attributes.get("email"); - } + Object email = attributes.get("email"); + if (email == null) { + return getGithubId() + "@github.com"; + } + return email.toString(); }src/main/java/org/ezcode/codetest/presentation/usermanagement/OAuth2Controller.java (1)
25-31: API 문서 예시 URL 업데이트 필요API 문서의 예시 URL이 실제 도메인과 일치하지 않을 수 있습니다. 실제 서비스 도메인으로 업데이트하거나 예시임을 명확히 표시하는 것이 좋습니다.
@Operation(summary = "GitHub & Google OAuth2 로그인 API", description = """ 프론트엔드에서 GitHub 로그인 버튼 클릭 시 이 API를 먼저 호출 redirect_uri는 로그인 완료 후 accessToken과 refreshToken을 전달받을 프론트의 콜백 URL - 예시: GET /api/oauth2/authorize/github?redirect_uri=https://ezcode.my/oauth/callback + 예시: GET /api/oauth2/authorize/github?redirect_uri=https://your-domain.com/oauth/callback """)src/main/java/org/ezcode/codetest/common/security/hander/CustomSuccessHandler.java (1)
61-78: GitHub 액세스 토큰 만료 처리 고려사항현재 구현은 잘 되어 있으나, PR에서 언급하신 토큰 만료 처리에 대해 다음을 제안드립니다:
- GitHub 액세스 토큰은 만료되지 않지만, 사용자가 권한을 취소할 수 있습니다
- 토큰 사용 시 401/403 에러 발생하면 재인증을 유도하는 로직 추가를 고려하세요
- 토큰과 함께 저장 시간을 기록하여 주기적인 갱신을 고려할 수 있습니다
src/main/java/org/ezcode/codetest/domain/user/model/entity/User.java (1)
75-76: 필드 접근 제어자 일관성
githubAccessToken필드를 다른 필드들과 일관성을 위해private으로 선언하는 것이 좋겠습니다.-private String githubAccessToken; //깃허브 access_token 값 +@Column(name = "github_access_token", columnDefinition = "TEXT") +private String githubAccessToken; //깃허브 access_token 값src/main/java/org/ezcode/codetest/domain/user/service/CustomOAuth2UserService.java (1)
97-101: 문자열 비교 일관성 개선
createResponse메서드에서는toLowerCase()를 사용하는데, 여기서는 소문자로만 비교합니다. 일관성을 위해 대소문자 구분 없이 비교하는 것이 좋겠습니다.-if ("github".equals(provider) && user.getGithubUrl()==null) { +if ("github".equalsIgnoreCase(provider) && user.getGithubUrl() == null) { user.setGithubUrl(response.getGithubUrl()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
build.gradle(2 hunks)src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GithubOAuth2Response.java(1 hunks)src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GoogleOAuth2Response.java(1 hunks)src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/OAuth2Response.java(1 hunks)src/main/java/org/ezcode/codetest/application/usermanagement/user/service/UserService.java(0 hunks)src/main/java/org/ezcode/codetest/common/security/config/SecurityConfig.java(1 hunks)src/main/java/org/ezcode/codetest/common/security/hander/CustomSuccessHandler.java(4 hunks)src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java(1 hunks)src/main/java/org/ezcode/codetest/common/security/util/JwtFilter.java(0 hunks)src/main/java/org/ezcode/codetest/common/security/util/SecurityPath.java(1 hunks)src/main/java/org/ezcode/codetest/domain/user/exception/code/AuthExceptionCode.java(1 hunks)src/main/java/org/ezcode/codetest/domain/user/model/entity/CustomOAuth2User.java(2 hunks)src/main/java/org/ezcode/codetest/domain/user/model/entity/User.java(5 hunks)src/main/java/org/ezcode/codetest/domain/user/model/entity/UserFactory.java(1 hunks)src/main/java/org/ezcode/codetest/domain/user/repository/UserRepository.java(1 hunks)src/main/java/org/ezcode/codetest/domain/user/service/CustomOAuth2UserService.java(2 hunks)src/main/java/org/ezcode/codetest/domain/user/service/UserDomainService.java(1 hunks)src/main/java/org/ezcode/codetest/infrastructure/persistence/repository/user/UserRepositoryImpl.java(1 hunks)src/main/java/org/ezcode/codetest/presentation/usermanagement/OAuth2Controller.java(1 hunks)src/main/resources/application.properties(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/org/ezcode/codetest/application/usermanagement/user/service/UserService.java
- src/main/java/org/ezcode/codetest/common/security/util/JwtFilter.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GithubOAuth2Response.java (1)
src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GoogleOAuth2Response.java (1)
Schema(7-52)
src/main/java/org/ezcode/codetest/domain/user/service/CustomOAuth2UserService.java (1)
src/main/java/org/ezcode/codetest/domain/user/model/entity/UserFactory.java (1)
UserFactory(7-30)
🪛 ast-grep (0.38.1)
src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java
[warning] 19-19: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 27-27: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 19-19: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 27-27: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🔇 Additional comments (11)
build.gradle (1)
117-118: JAXB API 버전 확인이 필요합니다.AES 암호화를 위해 추가된 JAXB API 2.3.1 버전이 최신 버전인지, 보안 취약점이 없는지 확인해주세요.
JAXB API 최신 버전과 2.3.1 버전의 보안 취약점 여부를 확인해주세요.src/main/java/org/ezcode/codetest/domain/user/exception/code/AuthExceptionCode.java (1)
22-22: 토큰 인코딩 실패 예외 코드가 적절하게 추가되었습니다.AES 암호화 실패 시 사용할 예외 코드가 기존 패턴을 따라 올바르게 구현되었습니다.
src/main/java/org/ezcode/codetest/common/security/config/SecurityConfig.java (1)
58-60: 매개변수 이름 변경으로 가독성이 개선되었습니다.OAuth2 설정에서 람다 매개변수 이름을
userInfo로 변경하여 코드 가독성이 향상되었습니다.src/main/java/org/ezcode/codetest/domain/user/repository/UserRepository.java (1)
23-23: GitHub 액세스 토큰 업데이트 메서드가 적절히 추가되었습니다.GitHub OAuth 통합을 위한 토큰 업데이트 메서드가 리포지토리 패턴을 따라 올바르게 정의되었습니다.
src/main/java/org/ezcode/codetest/infrastructure/persistence/repository/user/UserRepositoryImpl.java (1)
50-53: GitHub 액세스 토큰 업데이트 메서드가 올바르게 구현되었습니다.JPA save를 통한 간단하고 효과적인 구현으로 기존 리포지토리 패턴을 일관성 있게 따르고 있습니다.
src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/GoogleOAuth2Response.java (1)
41-49: 인터페이스 확장에 대한 적절한 플레이스홀더 구현Google OAuth2 응답에 GitHub 관련 데이터가 없는 상황에서 빈 문자열을 반환하는 것은 적절한 구현입니다. 인터페이스 계약을 올바르게 준수하고 있습니다.
src/main/java/org/ezcode/codetest/domain/user/service/UserDomainService.java (1)
131-134: GitHub 액세스 토큰 업데이트 메서드 구현 승인메서드가 기존 클래스 패턴을 잘 따르고 있습니다:
- 적절한 로깅 추가
- 리포지토리 계층으로의 올바른 위임
- 다른 메서드들과 일관된 구조
src/main/java/org/ezcode/codetest/application/usermanagement/user/dto/response/OAuth2Response.java (1)
16-20: 인터페이스 확장 구현 승인GitHub 특화 메서드 추가가 깔끔하게 구현되었습니다:
- 명확한 메서드 명명과 주석
- 단일 책임 원칙 준수
- 다양한 OAuth 제공자에 대한 다형성 지원
src/main/java/org/ezcode/codetest/common/security/util/SecurityPath.java (1)
8-10: OAuth2 엔드포인트에 대한 보안 경로 설정 승인OAuth2 컨트롤러 지원을 위한 public path 추가가 적절히 구현되었습니다:
- 기존 OAuth 경로에 명확한 주석 추가
- 새로운 API OAuth2 경로 추가
- OAuth 플로우에 필요한 공개 접근 허용
src/main/java/org/ezcode/codetest/domain/user/service/CustomOAuth2UserService.java (2)
34-56: 리팩토링이 잘 되었습니다!
loadUser메서드가 메서드 추출을 통해 가독성이 크게 향상되었습니다. 각 메서드가 단일 책임을 가지도록 잘 분리되었네요.
87-96: 기존 유저의 소셜 로그인 추가 처리가 잘 구현되었습니다기존 유저가 처음으로 소셜 로그인을 추가할 때 verified 상태와 reviewToken을 부여하는 로직이 PR 목적에 맞게 잘 구현되었습니다.
src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java
Outdated
Show resolved
Hide resolved
| public String decrypt(String encryptedInput) throws Exception { | ||
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | ||
| Cipher cipher = Cipher.getInstance(ALGORITHM); | ||
| cipher.init(Cipher.DECRYPT_MODE, keySpec); | ||
| byte[] encryptedBytes = Base64.getDecoder().decode(encryptedInput); | ||
| byte[] decryptedBytes = cipher.doFinal(encryptedBytes); | ||
| return new String(decryptedBytes); | ||
| } |
There was a problem hiding this comment.
복호화 메서드도 보안 모드로 업데이트 필요
암호화 메서드와 동일하게 CBC 모드를 사용하도록 수정해야 합니다.
-public String decrypt(String encryptedInput) throws Exception {
- SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM);
- Cipher cipher = Cipher.getInstance(ALGORITHM);
- cipher.init(Cipher.DECRYPT_MODE, keySpec);
- byte[] encryptedBytes = Base64.getDecoder().decode(encryptedInput);
- byte[] decryptedBytes = cipher.doFinal(encryptedBytes);
- return new String(decryptedBytes);
+public String decrypt(String encryptedInput) throws Exception {
+ if (SECRET_KEY.getBytes().length != 16 && SECRET_KEY.getBytes().length != 24 && SECRET_KEY.getBytes().length != 32) {
+ throw new IllegalArgumentException("AES key must be 16, 24, or 32 bytes");
+ }
+
+ byte[] combined = Base64.getDecoder().decode(encryptedInput);
+
+ // IV 추출 (처음 16바이트)
+ byte[] iv = new byte[16];
+ byte[] encryptedBytes = new byte[combined.length - 16];
+ System.arraycopy(combined, 0, iv, 0, 16);
+ System.arraycopy(combined, 16, encryptedBytes, 0, encryptedBytes.length);
+
+ SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM);
+ Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
+ IvParameterSpec ivSpec = new IvParameterSpec(iv);
+ cipher.init(Cipher.DECRYPT_MODE, keySpec, ivSpec);
+
+ byte[] decryptedBytes = cipher.doFinal(encryptedBytes);
+ return new String(decryptedBytes, "UTF-8");
}필요한 import 추가:
+import javax.crypto.spec.IvParameterSpec;
+import java.nio.charset.StandardCharsets;📝 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.
| public String decrypt(String encryptedInput) throws Exception { | |
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | |
| Cipher cipher = Cipher.getInstance(ALGORITHM); | |
| cipher.init(Cipher.DECRYPT_MODE, keySpec); | |
| byte[] encryptedBytes = Base64.getDecoder().decode(encryptedInput); | |
| byte[] decryptedBytes = cipher.doFinal(encryptedBytes); | |
| return new String(decryptedBytes); | |
| } | |
| // Add at top of the file: | |
| + import javax.crypto.spec.IvParameterSpec; | |
| + import java.nio.charset.StandardCharsets; | |
| public String decrypt(String encryptedInput) throws Exception { | |
| // Validate key length | |
| int keyLen = SECRET_KEY.getBytes().length; | |
| if (keyLen != 16 && keyLen != 24 && keyLen != 32) { | |
| throw new IllegalArgumentException("AES key must be 16, 24, or 32 bytes"); | |
| } | |
| // Decode IV + ciphertext | |
| byte[] combined = Base64.getDecoder().decode(encryptedInput); | |
| // Extract IV (first 16 bytes) | |
| byte[] iv = new byte[16]; | |
| byte[] encryptedBytes = new byte[combined.length - 16]; | |
| System.arraycopy(combined, 0, iv, 0, 16); | |
| System.arraycopy(combined, 16, encryptedBytes, 0, encryptedBytes.length); | |
| // Initialize cipher in CBC mode with PKCS5 padding | |
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | |
| Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); | |
| IvParameterSpec ivSpec = new IvParameterSpec(iv); | |
| cipher.init(Cipher.DECRYPT_MODE, keySpec, ivSpec); | |
| byte[] decryptedBytes = cipher.doFinal(encryptedBytes); | |
| return new String(decryptedBytes, StandardCharsets.UTF_8); | |
| } |
🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 27-27: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 27-27: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🤖 Prompt for AI Agents
In src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java around
lines 26 to 33, the decrypt method uses the default cipher mode instead of CBC
mode like the encrypt method. Update the Cipher.getInstance call to specify
"AES/CBC/PKCS5Padding" to match the encryption mode. Also, initialize the cipher
with an IvParameterSpec for the CBC mode using the same IV as encryption. Add
necessary imports for IvParameterSpec and any other required classes.
| public String encrypt(String input) throws Exception { | ||
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | ||
| Cipher cipher = Cipher.getInstance(ALGORITHM); | ||
| cipher.init(Cipher.ENCRYPT_MODE, keySpec); | ||
| byte[] encryptedBytes = cipher.doFinal(input.getBytes()); | ||
| return Base64.getEncoder().encodeToString(encryptedBytes); | ||
| } |
There was a problem hiding this comment.
치명적인 보안 취약점: ECB 모드 사용
AES ECB 모드는 보안상 안전하지 않습니다. 같은 평문이 같은 암호문을 생성하므로 패턴 분석이 가능합니다. CBC 또는 GCM 모드를 사용해야 합니다.
-public String encrypt(String input) throws Exception {
- SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM);
- Cipher cipher = Cipher.getInstance(ALGORITHM);
- cipher.init(Cipher.ENCRYPT_MODE, keySpec);
- byte[] encryptedBytes = cipher.doFinal(input.getBytes());
- return Base64.getEncoder().encodeToString(encryptedBytes);
+public String encrypt(String input) throws Exception {
+ if (SECRET_KEY.getBytes().length != 16 && SECRET_KEY.getBytes().length != 24 && SECRET_KEY.getBytes().length != 32) {
+ throw new IllegalArgumentException("AES key must be 16, 24, or 32 bytes");
+ }
+
+ SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM);
+ Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
+ cipher.init(Cipher.ENCRYPT_MODE, keySpec);
+
+ byte[] iv = cipher.getIV();
+ byte[] encryptedBytes = cipher.doFinal(input.getBytes("UTF-8"));
+
+ // IV와 암호문을 함께 저장
+ byte[] combined = new byte[iv.length + encryptedBytes.length];
+ System.arraycopy(iv, 0, combined, 0, iv.length);
+ System.arraycopy(encryptedBytes, 0, combined, iv.length, encryptedBytes.length);
+
+ return Base64.getEncoder().encodeToString(combined);
}📝 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.
| public String encrypt(String input) throws Exception { | |
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | |
| Cipher cipher = Cipher.getInstance(ALGORITHM); | |
| cipher.init(Cipher.ENCRYPT_MODE, keySpec); | |
| byte[] encryptedBytes = cipher.doFinal(input.getBytes()); | |
| return Base64.getEncoder().encodeToString(encryptedBytes); | |
| } | |
| public String encrypt(String input) throws Exception { | |
| // Ensure the key is a valid AES length | |
| if (SECRET_KEY.getBytes().length != 16 | |
| && SECRET_KEY.getBytes().length != 24 | |
| && SECRET_KEY.getBytes().length != 32) { | |
| throw new IllegalArgumentException("AES key must be 16, 24, or 32 bytes"); | |
| } | |
| SecretKeySpec keySpec = new SecretKeySpec(SECRET_KEY.getBytes(), ALGORITHM); | |
| Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); | |
| cipher.init(Cipher.ENCRYPT_MODE, keySpec); | |
| // Retrieve the randomly generated IV | |
| byte[] iv = cipher.getIV(); | |
| byte[] encryptedBytes = cipher.doFinal(input.getBytes("UTF-8")); | |
| // Prepend IV to ciphertext so it can be used for decryption | |
| byte[] combined = new byte[iv.length + encryptedBytes.length]; | |
| System.arraycopy(iv, 0, combined, 0, iv.length); | |
| System.arraycopy(encryptedBytes, 0, combined, iv.length, encryptedBytes.length); | |
| return Base64.getEncoder().encodeToString(combined); | |
| } |
🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 19-19: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 19-19: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🤖 Prompt for AI Agents
In src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java around
lines 18 to 24, the encryption method uses AES in ECB mode, which is insecure
due to pattern leakage. Change the Cipher instance to use AES in CBC or GCM
mode, initialize it with an IV (initialization vector), and update the
encryption logic to handle the IV properly. Ensure the key and IV are securely
generated and managed.
...a/org/ezcode/codetest/application/usermanagement/user/dto/response/GithubOAuth2Response.java
Show resolved
Hide resolved
| public void redirectToProvider( | ||
| @PathVariable String provider, | ||
| HttpServletRequest request, | ||
| HttpServletResponse response, | ||
| @RequestParam(required = false) String redirect_uri | ||
| ) throws IOException { | ||
| if (redirect_uri != null) { | ||
| request.getSession().setAttribute("redirect_uri", redirect_uri); | ||
| } | ||
|
|
||
| response.sendRedirect("/oauth2/authorization/" + provider); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
입력 검증 및 보안 강화 필요
현재 구현에서 provider 파라미터와 redirect_uri에 대한 검증이 없어 보안 위험이 있습니다.
+private static final Set<String> ALLOWED_PROVIDERS = Set.of("github", "google");
+private static final Pattern REDIRECT_URI_PATTERN = Pattern.compile("^https?://[a-zA-Z0-9.-]+(/.*)?$");
+
@GetMapping("/authorize/{provider}")
public void redirectToProvider(
@PathVariable String provider,
HttpServletRequest request,
HttpServletResponse response,
@RequestParam(required = false) String redirect_uri
) throws IOException {
+ // Provider 검증
+ if (!ALLOWED_PROVIDERS.contains(provider.toLowerCase())) {
+ response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Unsupported OAuth2 provider");
+ return;
+ }
+
+ // Redirect URI 검증 (제공된 경우)
+ if (redirect_uri != null) {
+ if (!REDIRECT_URI_PATTERN.matcher(redirect_uri).matches()) {
+ response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid redirect URI format");
+ return;
+ }
+ request.getSession().setAttribute("redirect_uri", redirect_uri);
+ }
+
- if (redirect_uri != null) {
- request.getSession().setAttribute("redirect_uri", redirect_uri);
- }
-
- response.sendRedirect("/oauth2/authorization/" + provider);
+ response.sendRedirect("/oauth2/authorization/" + provider.toLowerCase());
}필요한 import 추가:
+import java.util.Set;
+import java.util.regex.Pattern;
+import jakarta.servlet.http.HttpServletResponse;🤖 Prompt for AI Agents
In
src/main/java/org/ezcode/codetest/presentation/usermanagement/OAuth2Controller.java
around lines 36 to 47, the method redirectToProvider lacks validation for the
provider and redirect_uri parameters, posing security risks. Add input
validation to ensure the provider matches allowed OAuth2 providers and validate
redirect_uri to prevent open redirect vulnerabilities. Also, include necessary
imports for validation utilities or annotations used.
src/main/java/org/ezcode/codetest/common/security/hander/CustomSuccessHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java (2)
18-24: 치명적인 보안 취약점: ECB 모드 사용 (이전 리뷰와 동일한 이슈)AES ECB 모드는 보안상 안전하지 않습니다. 동일한 평문이 동일한 암호문을 생성하므로 패턴 분석이 가능합니다. 이전 리뷰 코멘트에서 제시된 CBC 모드로 변경해야 합니다.
26-33: 복호화 메서드도 보안 모드로 업데이트 필요 (이전 리뷰와 동일한 이슈)암호화 메서드와 동일하게 CBC 모드를 사용하도록 수정해야 합니다. 이전 리뷰에서 제시된 해결책을 적용해주세요.
🧹 Nitpick comments (1)
src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java (1)
18-33: 예외 처리 개선 필요현재 메서드들이 일반적인
Exception을 던지고 있어 호출자가 구체적인 오류 상황을 파악하기 어렵습니다. 더 구체적인 예외 타입을 사용하거나 커스텀 예외를 정의하는 것이 좋습니다.-public String encrypt(String input) throws Exception { +public String encrypt(String input) throws IllegalArgumentException, GeneralSecurityException { // 구현 내용 } -public String decrypt(String encryptedInput) throws Exception { +public String decrypt(String encryptedInput) throws IllegalArgumentException, GeneralSecurityException { // 구현 내용 }필요한 import 추가:
+import java.security.GeneralSecurityException;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/ezcode/codetest/common/security/hander/CustomSuccessHandler.java(4 hunks)src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java(1 hunks)src/main/java/org/ezcode/codetest/domain/user/exception/code/AuthExceptionCode.java(1 hunks)src/main/resources/application.properties(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/application.properties
- src/main/java/org/ezcode/codetest/common/security/hander/CustomSuccessHandler.java
🧰 Additional context used
🪛 ast-grep (0.38.1)
src/main/java/org/ezcode/codetest/common/security/util/AESUtil.java
[warning] 19-19: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 27-27: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 19-19: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 27-27: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
🔇 Additional comments (2)
src/main/java/org/ezcode/codetest/domain/user/exception/code/AuthExceptionCode.java (2)
22-23: 새로운 예외 코드 추가 확인OAuth2 구현과 AES 암호화 기능을 위한 새로운 예외 코드들이 적절히 추가되었습니다.
TOKEN_ENCODE_FAIL: AES 암호화 실패 시 사용REDIRECT_URI_NOT_FOUND: OAuth2 리다이렉트 URI 누락 시 사용두 예외 모두 적절한 HTTP 상태 코드(BAD_REQUEST)와 명확한 한국어 메시지를 가지고 있습니다.
21-23: ```shell
#!/bin/bash
echo "=== AESUtil references ==="
rg --type java "AESUtil"echo -e "\n=== encodedGithubToken context ==="
rg -A5 -B5 "encodedGithubToken" --type java</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
작업 내용
변경 사항
CustomOAuth2UserService의 3중 if문을 processOAuth2User() 메서드로 추출
깃허브 이메일 처리 (GitHub 이메일이 null일 경우 처리)
소셜 로그인 추가 시 AI리뷰 토큰 부여
통합 OAuth 컨트롤러
/api/oauth2/authorize/{provider}로 provider RequestParam 값에 따라 이동깃허브 AccessToken 저장
해결해야 할 문제
참고 사항
OAuth 해보고싶으시면
http://localhost:8080/api/oauth2/authorize/githubhttp://localhost:8080/api/oauth2/authorize/google를 크롬창에 넣고 엔터 누르시면 됩니다
참고 이미지
(1) github 이메일 제공 관련
이렇게 풀려있어야 넘어옵니다
(2) DB에 등록되는 화면
그냥 이메일로만 가입햇을때 (verified = false, github_url=null)
github 등록하면
코드 리뷰 전 확인 체크리스트
type :)Summary by CodeRabbit
신규 기능
버그 수정
환경설정
리팩토링 및 개선
문서화