[FEAT] 인가 코드 하이브리드 방식 - #67
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough소셜 로그인에 OAuth 인가 코드 콜백과 제공자별 토큰 교환이 추가되었습니다. 자격 증명과 리다이렉트 URI 검증이 도입되었고, 로그인 결과는 설정된 프론트엔드 주소로 전달됩니다. ChangesOAuth 로그인 흐름
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant OAuthClientService
participant OAuthProvider
Client->>AuthController: OAuth callback code
AuthController->>AuthService: socialLogin(credential, redirectUri)
AuthService->>OAuthClientService: getUserInfo(credential, redirectUri)
OAuthClientService->>OAuthProvider: exchange code for access token
OAuthProvider-->>OAuthClientService: OAuthTokenResponse
OAuthClientService->>OAuthProvider: request user information
OAuthProvider-->>AuthService: OAuthUserInfo
AuthController-->>Client: frontend redirect with login result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
🧹 Nitpick comments (3)
src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java (1)
30-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win핵심 신규 로직(인가 코드 교환)에 대한 테스트가 없습니다.
setUp()에서OAuthProperties를null로 주입하기 때문에,exchangeKakaoCode/exchangeGoogleCode는 매 테스트에서 clientId 체크로 즉시 early-return하고 원본 코드 값을 그대로 access token으로 사용합니다. 결과적으로 이번 PR의 핵심 기능인 "인가 코드 → 토큰 교환" 흐름(카카오/구글 토큰 엔드포인트 POST, 응답 검증, 예외 시 폴백)은 어떤 테스트에서도 실제로 실행되지 않습니다.
OAuthProperties를 채운 인스턴스로 별도 테스트를 추가하고MockRestServiceServer로 카카오/구글 토큰 엔드포인트 응답을 목킹해서 성공/실패 케이스를 검증하는 걸 추천합니다. 원하시면 테스트 코드를 같이 작성해드릴 수 있어요!🤖 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/mr/domain/auth/service/OAuthClientServiceTest.java` around lines 30 - 35, Update OAuthClientServiceTest so setUp initializes OAuthProperties with non-null Kakao and Google client IDs, then construct OAuthClientService with those properties. Add MockRestServiceServer tests for exchangeKakaoCode and exchangeGoogleCode covering successful token-endpoint POST responses, response validation, and fallback behavior on endpoint failures, ensuring the actual authorization-code-to-access-token flow executes.src/main/java/com/mr/domain/auth/service/OAuthClientService.java (1)
55-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKakao/Google 교환 로직 중복.
exchangeKakaoCode와exchangeGoogleCode는 provider 설정 조회 → redirectUri 결정 → form body 구성 → POST → accessToken 검증 → catch-fallback까지 구조가 거의 동일합니다. 공통 헬퍼(토큰 엔드포인트 URL, client_secret 처리 방식만 매개변수화)로 추출하면 유지보수가 쉬워집니다. 참고로 Kakao는client_secret이 blank가 아닐 때만 추가하고 Google은 항상 추가하는 처리 방식 차이도 함께 정리할 좋은 기회입니다.Also applies to: 129-163
🤖 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/mr/domain/auth/service/OAuthClientService.java` around lines 55 - 92, Extract the duplicated provider token-exchange flow shared by exchangeKakaoCode and exchangeGoogleCode into a common helper. Parameterize the token endpoint and client_secret inclusion policy, preserving Kakao’s non-blank-only behavior and Google’s always-included behavior, while retaining each method’s provider configuration, redirect URI selection, access-token validation, and code fallback.src/main/java/com/mr/domain/auth/service/AuthService.java (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueredirectUri 분기 로직 중복.
OAuthClientService.getUserInfo(socialType, codeOrToken, customRedirectUri)가 이미customRedirectUri가 null/blank일 때 2-arg 버전으로 위임하므로,AuthService에서 동일한 조건을 다시 검사할 필요가 없습니다.socialLogin과linkSocialAccount모두 항상 3-arg 오버로드만 호출하도록 단순화할 수 있습니다.♻️ 개선 제안 예시 (socialLogin)
public AuthResponseDTO.LoginResponse socialLogin(SocialType socialType, String codeOrToken, String redirectUri, String deviceInfo) { - OAuthUserInfo userInfo = (redirectUri != null && !redirectUri.isBlank()) - ? oAuthClientService.getUserInfo(socialType, codeOrToken, redirectUri) - : oAuthClientService.getUserInfo(socialType, codeOrToken); + OAuthUserInfo userInfo = oAuthClientService.getUserInfo(socialType, codeOrToken, redirectUri);Also applies to: 136-139
🤖 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/mr/domain/auth/service/AuthService.java` around lines 39 - 42, Remove the redirectUri null/blank branching in AuthService methods socialLogin and linkSocialAccount, and always call OAuthClientService.getUserInfo(socialType, codeOrToken, redirectUri) using the 3-argument overload. Preserve the existing arguments and flow while relying on OAuthClientService to delegate when redirectUri is absent.
🤖 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/mr/domain/auth/dto/req/AuthRequestDTO.java`:
- Around line 9-27: Update SocialLoginRequest and getEffectiveCodeOrToken so the
selected input type is preserved alongside its trimmed value, distinguishing
code/authorizationCode from accessToken with an explicit enum or result type.
Update OAuthClientService to use that type and skip the provider code-exchange
request for access-token inputs while retaining exchange behavior for code-based
inputs.
In `@src/main/java/com/mr/domain/auth/service/OAuthClientService.java`:
- Around line 76-92: Update the exception handling around the OAuth token
exchange in OAuthClientService so the caught exception e is included in the
warning log with stack-trace details. Distinguish expected invalid-code/provider
4xx failures from configuration, network, and 5xx failures: only fall back to
returning code for the former, while propagating the latter instead of treating
them as AccessTokens; apply the same behavior to the corresponding catch block
noted around the alternate provider flow.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/auth/service/AuthService.java`:
- Around line 39-42: Remove the redirectUri null/blank branching in AuthService
methods socialLogin and linkSocialAccount, and always call
OAuthClientService.getUserInfo(socialType, codeOrToken, redirectUri) using the
3-argument overload. Preserve the existing arguments and flow while relying on
OAuthClientService to delegate when redirectUri is absent.
In `@src/main/java/com/mr/domain/auth/service/OAuthClientService.java`:
- Around line 55-92: Extract the duplicated provider token-exchange flow shared
by exchangeKakaoCode and exchangeGoogleCode into a common helper. Parameterize
the token endpoint and client_secret inclusion policy, preserving Kakao’s
non-blank-only behavior and Google’s always-included behavior, while retaining
each method’s provider configuration, redirect URI selection, access-token
validation, and code fallback.
In `@src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java`:
- Around line 30-35: Update OAuthClientServiceTest so setUp initializes
OAuthProperties with non-null Kakao and Google client IDs, then construct
OAuthClientService with those properties. Add MockRestServiceServer tests for
exchangeKakaoCode and exchangeGoogleCode covering successful token-endpoint POST
responses, response validation, and fallback behavior on endpoint failures,
ensuring the actual authorization-code-to-access-token flow executes.
🪄 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: 28f01d6d-cc1d-4437-9f55-9d9ac7a2ac03
📒 Files selected for processing (11)
src/main/java/com/mr/domain/auth/controller/AuthController.javasrc/main/java/com/mr/domain/auth/dto/req/AuthRequestDTO.javasrc/main/java/com/mr/domain/auth/dto/res/GoogleUserResponse.javasrc/main/java/com/mr/domain/auth/dto/res/KakaoUserResponse.javasrc/main/java/com/mr/domain/auth/dto/res/OAuthTokenResponse.javasrc/main/java/com/mr/domain/auth/service/AuthService.javasrc/main/java/com/mr/domain/auth/service/OAuthClientService.javasrc/main/java/com/mr/domain/notification/service/NotificationEventListener.javasrc/main/java/com/mr/global/config/OAuthProperties.javasrc/main/java/com/mr/global/config/OAuthRestClientConfig.javasrc/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/mr/domain/auth/service/OAuthClientService.java (1)
77-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
clientId미설정 시 인가 코드를 그대로 반환하는 fallback, 설정 오류를 숨깁니다.
exchangeKakaoCode(그리고 동일 패턴의exchangeGoogleCode, 153-158번째 줄)는resolveAccessToken에서credential.type() == AUTHORIZATION_CODE일 때만 호출됩니다. 즉 이 시점에는 이미 "토큰 교환이 필요한 인가 코드"라는 사실이 확정된 상태인데,clientId가 비어 있으면 조용히code를 그대로 반환해 뒤이어 Kakao/Google 사용자 정보 API에 Bearer 토큰처럼 전송됩니다. 실제로는 401 등 원격 API 오류로만 드러나 "OAuth 설정 누락"이라는 진짜 원인을 파악하기 어렵게 만듭니다.설정 누락은 조용히 넘기지 말고 명확한 예외로 실패시키는 것이 디버깅과 운영에 유리합니다.
🛡️ 제안 수정
private String exchangeKakaoCode(String code, String customRedirectUri) { OAuthProperties.ProviderProperties kakaoProps = oAuthProperties != null ? oAuthProperties.kakao() : null; String clientId = kakaoProps != null ? kakaoProps.clientId() : null; if (clientId == null || clientId.isBlank()) { - return code; + throw new GeneralException(AuthErrorStatus.OAUTH_CLIENT_ERROR); }
exchangeGoogleCode(153-158번째 줄)에도 동일하게 적용해 주세요.🤖 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/mr/domain/auth/service/OAuthClientService.java` around lines 77 - 82, Update exchangeKakaoCode and exchangeGoogleCode so a missing or blank clientId throws a clear configuration-related exception instead of returning the authorization code; preserve the existing token-exchange flow when clientId is configured.
🧹 Nitpick comments (3)
src/main/java/com/mr/domain/auth/service/AuthService.java (2)
50-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fetchOAuthUserInfo의 분기 로직,OAuthClientService와 중복됩니다.
OAuthClientService.getUserInfo(SocialType, OAuthCredential, String)(3-인자 버전)은 이미credential.type() == ACCESS_TOKEN여부와customRedirectUri의 null/blank 여부를 내부에서 완전히 처리합니다(예:exchangeKakaoCode/exchangeGoogleCode내부의customRedirectUri != null && !customRedirectUri.isBlank()검사). 그래서fetchOAuthUserInfo가 동일한 조건을 다시 검사해 4갈래로 분기할 필요가 없습니다. redirectUri가null이든 빈 문자열이든 결과는 동일하므로, 아래처럼 한 줄로 줄여도 동작이 같습니다.♻️ 제안 리팩터
private OAuthUserInfo fetchOAuthUserInfo(SocialType socialType, OAuthCredential credential, String redirectUri) { - if (credential.type() == OAuthCredential.CredentialType.ACCESS_TOKEN) { - if (redirectUri != null && !redirectUri.isBlank()) { - return oAuthClientService.getUserInfo(socialType, credential.value(), redirectUri); - } - return oAuthClientService.getUserInfo(socialType, credential.value()); - } - if (redirectUri != null && !redirectUri.isBlank()) { - return oAuthClientService.getUserInfo(socialType, credential, redirectUri); - } - return oAuthClientService.getUserInfo(socialType, credential); + return oAuthClientService.getUserInfo(socialType, credential, redirectUri); }DRY(Don't Repeat Yourself) 원칙 관점에서 서비스 계층의 책임을 클라이언트 계층에 위임하는 편이 유지보수에 유리합니다.
🤖 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/mr/domain/auth/service/AuthService.java` around lines 50 - 61, Update fetchOAuthUserInfo to delegate directly to OAuthClientService.getUserInfo(SocialType, OAuthCredential, String) without branching on credential.type() or redirectUri null/blank status. Preserve the existing inputs so OAuthClientService remains responsible for selecting the credential flow and handling redirect URI normalization.
63-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win레거시
codeOrToken계열 메서드들을 제거하세요.현재 컨트롤러는 모두
OAuthCredential계열 시그니처로 호출합니다. 이 오버로드들은codeOrToken이름과 인증 코드 문맥을 유지하지만CredentialType.ACCESS_TOKEN으로 강제 랩핑하기 때문에, 만약 실제 인가 코드가 들어오면 토큰 교환 없이 사용자 정보 조회 경로를 타게 됩니다. OAuth 인가코드/엑세스토큰 처리를 명확히 구분한 하나의 메서드 흐름을 남겨 두는 편이 좋습니다.🤖 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/mr/domain/auth/service/AuthService.java` around lines 63 - 69, Remove the legacy socialLogin overloads accepting codeOrToken from AuthService, including the variants with and without redirectUri. Preserve the OAuthCredential-based socialLogin flow as the sole entry point, and update any remaining callers to construct and pass the appropriate credential type explicitly.src/main/java/com/mr/domain/auth/dto/OAuthCredential.java (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win정적 팩토리 메서드로 반복 생성 패턴 정리 추천 (칭찬 먼저: 하이브리드 인증 계약을 record 하나로 깔끔하게 표현한 점 좋습니다 👍)
new OAuthCredential(CredentialType.ACCESS_TOKEN, value)형태가AuthService.java(64, 196번째 줄)와OAuthClientService.java(58, 59번째 줄)에서 반복 등장합니다.ofAccessToken/ofAuthorizationCode같은 정적 팩토리 메서드를 추가하면 호출부 가독성이 좋아지고, 향후CredentialType처리 로직이 바뀌어도 한 곳만 수정하면 됩니다.♻️ 제안 리팩터
public record OAuthCredential( CredentialType type, String value ) { public enum CredentialType { AUTHORIZATION_CODE, ACCESS_TOKEN } + + public static OAuthCredential ofAccessToken(String value) { + return new OAuthCredential(CredentialType.ACCESS_TOKEN, value); + } + + public static OAuthCredential ofAuthorizationCode(String value) { + return new OAuthCredential(CredentialType.AUTHORIZATION_CODE, value); + } }🤖 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/mr/domain/auth/dto/OAuthCredential.java` around lines 3 - 11, Update the OAuthCredential record with static factory methods such as ofAccessToken and ofAuthorizationCode that create credentials with the corresponding CredentialType, then replace direct constructor calls in AuthService and OAuthClientService with these methods.
🤖 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/mr/domain/auth/service/OAuthClientService.java`:
- Around line 110-116: Update the warning logs in the Kakao authorization-code
exchange catch block and the corresponding exchangeGoogleCode catch block to
stop recording the raw code value. Preserve the original exception details and
replace the code placeholder with non-sensitive context, such as a generic
exchange-failure message or a safely redacted identifier.
---
Outside diff comments:
In `@src/main/java/com/mr/domain/auth/service/OAuthClientService.java`:
- Around line 77-82: Update exchangeKakaoCode and exchangeGoogleCode so a
missing or blank clientId throws a clear configuration-related exception instead
of returning the authorization code; preserve the existing token-exchange flow
when clientId is configured.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/auth/dto/OAuthCredential.java`:
- Around line 3-11: Update the OAuthCredential record with static factory
methods such as ofAccessToken and ofAuthorizationCode that create credentials
with the corresponding CredentialType, then replace direct constructor calls in
AuthService and OAuthClientService with these methods.
In `@src/main/java/com/mr/domain/auth/service/AuthService.java`:
- Around line 50-61: Update fetchOAuthUserInfo to delegate directly to
OAuthClientService.getUserInfo(SocialType, OAuthCredential, String) without
branching on credential.type() or redirectUri null/blank status. Preserve the
existing inputs so OAuthClientService remains responsible for selecting the
credential flow and handling redirect URI normalization.
- Around line 63-69: Remove the legacy socialLogin overloads accepting
codeOrToken from AuthService, including the variants with and without
redirectUri. Preserve the OAuthCredential-based socialLogin flow as the sole
entry point, and update any remaining callers to construct and pass the
appropriate credential type explicitly.
🪄 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: ca1bbbe0-f2e3-4bf1-a6f5-7050c027e69f
📒 Files selected for processing (5)
src/main/java/com/mr/domain/auth/controller/AuthController.javasrc/main/java/com/mr/domain/auth/dto/OAuthCredential.javasrc/main/java/com/mr/domain/auth/dto/req/AuthRequestDTO.javasrc/main/java/com/mr/domain/auth/service/AuthService.javasrc/main/java/com/mr/domain/auth/service/OAuthClientService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/mr/domain/auth/controller/AuthController.java
- src/main/java/com/mr/domain/auth/dto/req/AuthRequestDTO.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java (1)
158-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGoogle 분기와 실제 토큰 교환 성공 경로도 테스트로 고정해 주세요.
현재 신규 테스트는 Kakao의 설정 누락만 검증합니다. Google의 동일한
client_id누락 분기와 authorization-code 요청의redirect_uri, form body,access_token응답 매핑은 회귀 시 감지되지 않습니다. Google 대응 테스트와 각 provider의 성공적인MockRestServiceServer교환 테스트를 추가해 주세요.🤖 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/mr/domain/auth/service/OAuthClientServiceTest.java` around lines 158 - 168, OAuthClientServiceTest의 exchangeCode_missingConfig_throwsOauthServerError만으로는 Google 분기와 성공 경로가 검증되지 않으므로, Google의 client_id 누락 시 OAUTH_SERVER_ERROR가 발생하는 테스트를 추가하세요. 또한 Kakao와 Google 각각에 MockRestServiceServer를 사용한 authorization-code 토큰 교환 성공 테스트를 추가해 redirect_uri와 form body가 올바르게 전송되고 access_token 응답이 정상 매핑되는지 검증하세요.
🤖 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/mr/domain/auth/service/OAuthClientService.java`:
- Around line 81-82: OAuth token exchange must reject missing redirect URIs. In
the flows around the Kakao and Google redirectUri calculations, validate the
resolved redirectUri immediately afterward for null or blank values, log the
configuration error consistently, and throw AuthErrorStatus.OAUTH_SERVER_ERROR
before issuing the token request.
---
Nitpick comments:
In `@src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java`:
- Around line 158-168: OAuthClientServiceTest의
exchangeCode_missingConfig_throwsOauthServerError만으로는 Google 분기와 성공 경로가 검증되지
않으므로, Google의 client_id 누락 시 OAUTH_SERVER_ERROR가 발생하는 테스트를 추가하세요. 또한 Kakao와
Google 각각에 MockRestServiceServer를 사용한 authorization-code 토큰 교환 성공 테스트를 추가해
redirect_uri와 form body가 올바르게 전송되고 access_token 응답이 정상 매핑되는지 검증하세요.
🪄 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: b412b0b3-aa8a-42d3-807c-5706e252c4b8
📒 Files selected for processing (2)
src/main/java/com/mr/domain/auth/service/OAuthClientService.javasrc/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/mr/domain/auth/service/AuthService.java (2)
64-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win두 String 오버로드가 인가 코드 입력을 지원하지 않습니다. 두 메서드 모두
codeOrToken을 무조건ACCESS_TOKEN으로 포장하므로, 인가 코드가 전달되면 잘못된 제공자 API 경로로 라우팅됩니다.
src/main/java/com/mr/domain/auth/service/AuthService.java#L64-L69: Access Token 전용 API로 명확히 하거나OAuthCredential오버로드를 사용하세요.src/main/java/com/mr/domain/auth/service/AuthService.java#L196-L197: 계정 연동도 동일하게OAuthCredential과redirectUri를 유지하세요.🤖 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/mr/domain/auth/service/AuthService.java` around lines 64 - 69, Update AuthService.socialLogin at src/main/java/com/mr/domain/auth/service/AuthService.java:64-69 to make the String overload explicitly access-token-only or delegate through the OAuthCredential overload without claiming authorization-code support; update the account-linking flow at src/main/java/com/mr/domain/auth/service/AuthService.java:196-197 to preserve OAuthCredential and redirectUri so authorization-code requests use the correct provider API path.
40-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOAuth 호출과 retry를 비트랜잭션으로 분리하세요.
socialLogin에 붙은@Transactional은 클래스의@Transactional(readOnly = true)를 덮어쓰지만, 여전히TransactionTemplate의 기본 전파REQUIRED와 같은 트랜잭션에 참여합니다. 여기서 중복 소셜 ID가 발생하면DataIntegrityViolationException후 해당 트랜잭션이rollback-only상태가 되므로, catch 후 실행되는 기존 사용자 retry도 같은 트랜잭션 커밋 시점에UnexpectedRollbackException으로 실패할 수 있습니다.
@Transactional(propagation = Propagation.NOT_SUPPORTED)으로 오버런 메서드를 비트랜잭션으로 두고, 각 저장/재시도 경로를 독립TransactionTemplate(REQUIRES_NEW)또는 별도 트랜잭션 경계로 구성하세요. 해당 트랜잭션 전파 설정과 중복 소셜 가입 fallback은 통합 테스트로 확인해 주세요.🤖 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/mr/domain/auth/service/AuthService.java` around lines 40 - 48, Update AuthService.socialLogin to use `@Transactional`(propagation = Propagation.NOT_SUPPORTED), keeping OAuth retrieval and retry execution outside a surrounding transaction. Configure the transaction boundaries used by executeSocialLogin and executeSocialLoginForExistingUser to run independently with REQUIRES_NEW, so a DataIntegrityViolationException cannot mark the fallback transaction rollback-only. Add an integration test covering duplicate social signup fallback and successful retry.
🤖 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.
Outside diff comments:
In `@src/main/java/com/mr/domain/auth/service/AuthService.java`:
- Around line 64-69: Update AuthService.socialLogin at
src/main/java/com/mr/domain/auth/service/AuthService.java:64-69 to make the
String overload explicitly access-token-only or delegate through the
OAuthCredential overload without claiming authorization-code support; update the
account-linking flow at
src/main/java/com/mr/domain/auth/service/AuthService.java:196-197 to preserve
OAuthCredential and redirectUri so authorization-code requests use the correct
provider API path.
- Around line 40-48: Update AuthService.socialLogin to use
`@Transactional`(propagation = Propagation.NOT_SUPPORTED), keeping OAuth retrieval
and retry execution outside a surrounding transaction. Configure the transaction
boundaries used by executeSocialLogin and executeSocialLoginForExistingUser to
run independently with REQUIRES_NEW, so a DataIntegrityViolationException cannot
mark the fallback transaction rollback-only. Add an integration test covering
duplicate social signup fallback and successful retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e12a57e-8ece-437c-b73a-30a770335384
📒 Files selected for processing (1)
src/main/java/com/mr/domain/auth/service/AuthService.java
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/com/mr/domain/auth/service/OAuthClientService.java (2)
40-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getUserInfo오버로드 간 분기 로직이 중복됩니다.2-인자
getUserInfo(SocialType, OAuthCredential)(라인 50-55)가credential.type()을 다시 검사한 뒤 3-인자 버전으로 위임하는데, 3-인자getUserInfo(SocialType, OAuthCredential, String)(라인 40-48)가 이미 동일한 타입 분기를 내부적으로 처리합니다.customRedirectUri가null이어도resolveRedirectUri가 기본값(allowedUris.get(0))으로 폴백하므로, 그냥null을 넘겨 3-인자 버전에 위임해도 동작은 동일합니다. 지금처럼 같은 로직이 두 곳에 있으면 한쪽만 수정되고 다른 쪽이 방치되는 사고가 나기 쉽습니다.♻️ 개선 제안
public OAuthUserInfo getUserInfo(SocialType socialType, OAuthCredential credential) { - if (credential.type() == OAuthCredential.CredentialType.ACCESS_TOKEN) { - return getUserInfo(socialType, credential.value()); - } - return getUserInfo(socialType, credential, null); + return getUserInfo(socialType, credential, null); }🤖 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/mr/domain/auth/service/OAuthClientService.java` around lines 40 - 66, Remove the redundant credential-type branch from the two-argument getUserInfo(SocialType, OAuthCredential) overload and delegate directly to the three-argument getUserInfo(SocialType, OAuthCredential, String) with a null customRedirectUri, preserving the existing default redirect resolution.
264-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFQN을 매번 풀네임으로 쓰는 대신 import를 추가하는 게 어떨까요?
org.springframework.web.util.UriComponentsBuilder,com.mr.domain.auth.dto.res.AuthResponseDTO가 파일 곳곳에서 완전정규명으로 인라인 사용됩니다. 기능엔 영향 없지만 가독성을 위해 상단에import로 올려주시면 좋겠습니다.🤖 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/mr/domain/auth/service/OAuthClientService.java` around lines 264 - 277, Replace the fully qualified AuthResponseDTO and UriComponentsBuilder references used in OAuthClientService, including buildFrontendRedirectUrl, with imports declared at the top of the file; preserve the existing behavior and signatures.src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java (1)
159-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winredirect_uri 화이트리스트 테스트, 꼼꼼하게 잘 짜셨네요! 👍
missingConfig/invalidCustomRedirectUri케이스를 정확히 짚었습니다. 다만 이번에 새로 추가된getAuthorizationUrl,buildFrontendRedirectUrl에 대한 테스트가 보이지 않습니다. 특히getAuthorizationUrl은 URL 인코딩(scope 공백)과 (향후 추가될)state파라미터 검증에,buildFrontendRedirectUrl은 쿼리파라미터 구성 검증에 활용도가 높으니 추가해 주시면 회귀 방지에 도움이 될 것 같습니다.🤖 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/mr/domain/auth/service/OAuthClientServiceTest.java` around lines 159 - 190, OAuthClientServiceTest에 새로 추가된 getAuthorizationUrl과 buildFrontendRedirectUrl 테스트를 보강하세요. getAuthorizationUrl은 scope의 공백이 올바르게 URL 인코딩되는지와 state 파라미터가 포함되는지 검증하고, buildFrontendRedirectUrl은 반환 URL에 필요한 쿼리 파라미터가 정확히 구성되는지 검증하는 테스트를 추가하세요.src/main/java/com/mr/domain/auth/service/AuthService.java (2)
52-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fetchOAuthUserInfo의 분기가OAuthClientService.getUserInfo와 중복됩니다.
credential.type()과redirectUri공백 여부로 4갈래 분기를 하고 있는데,oAuthClientService.getUserInfo(socialType, credential, redirectUri)(3-인자,OAuthCredential버전)가 이미ACCESS_TOKEN/AUTHORIZATION_CODE분기와redirectUri가null/blank일 때의 기본값 처리를 내부적으로 다 수행합니다. 그냥 위임해도 동일하게 동작합니다.♻️ 개선 제안
private OAuthUserInfo fetchOAuthUserInfo(SocialType socialType, OAuthCredential credential, String redirectUri) { - if (credential.type() == OAuthCredential.CredentialType.ACCESS_TOKEN) { - if (redirectUri != null && !redirectUri.isBlank()) { - return oAuthClientService.getUserInfo(socialType, credential.value(), redirectUri); - } - return oAuthClientService.getUserInfo(socialType, credential.value()); - } - if (redirectUri != null && !redirectUri.isBlank()) { - return oAuthClientService.getUserInfo(socialType, credential, redirectUri); - } - return oAuthClientService.getUserInfo(socialType, credential); + return oAuthClientService.getUserInfo(socialType, credential, redirectUri); }🤖 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/mr/domain/auth/service/AuthService.java` around lines 52 - 63, Update fetchOAuthUserInfo to delegate directly to the three-argument OAuthClientService.getUserInfo(socialType, credential, redirectUri) overload, removing the credential type and redirectUri branching while preserving the service’s existing default handling.
65-67: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
self-invocation에서@Transactional을 회피하지 않도록 분리하세요.3-인자
socialLogin(SocialType, String, String)은 내부 4-인자 메서드로this.socialLogin(...)를 호출합니다. Spring의 AOP 프록시는 다른 내부 메서드 호출에서는 동작하지 않으므로, 이 경로가 실행되면 4-인자@Transactional설정이 적용되지 않고 클래스 기본값처럼 동작할 수 있습니다. 분리 서비스 빈 호출이나 자기 주입 자원을 통해 외부 호출처럼 프록시를 타게 하면 Spring AOP 트랜잭션 문서에서 권장하는 방식으로 안전하게 고칠 수 있습니다.🤖 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/mr/domain/auth/service/AuthService.java` around lines 65 - 67, Update the 3-argument socialLogin(SocialType, String, String) path so it delegates to the transactional 4-argument socialLogin through a separate Spring service bean or injected self-proxy, rather than direct self-invocation. Preserve the existing OAuthCredential construction and arguments while ensuring the `@Transactional` method is invoked through the AOP proxy.
🤖 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/mr/domain/auth/controller/AuthController.java`:
- Around line 62-86: Update AuthController.oAuthCallback so invalid OAuth
callbacks (non-null error or missing/blank code) redirect to the frontend
instead of throwing GeneralException, passing an appropriate error code as a
query parameter while preserving normal login redirects. Follow the existing
startOAuthLogin redirect/error-handling pattern and keep the state-parameter
validation concern separate.
In `@src/main/java/com/mr/domain/auth/service/OAuthClientService.java`:
- Around line 264-277: Update buildFrontendRedirectUrl so it never places
accessToken or refreshToken in the redirect query parameters. Replace the
raw-token parameters with a single-use exchange code that the frontend can
redeem through the established secure HTTPS POST flow, or deliver refreshToken
through an HttpOnly Secure cookie while retaining only non-sensitive response
data in the URL.
- Around line 235-262: Update getAuthorizationUrl to generate a
cryptographically random, unpredictable state before building the Kakao or
Google authorization URL, store it with a short TTL keyed to the login attempt,
and include it as the state query parameter. Extend oAuthCallback to retrieve
and compare the stored state with the callback value, rejecting missing or
mismatched values before processing the authorization code.
---
Nitpick comments:
In `@src/main/java/com/mr/domain/auth/service/AuthService.java`:
- Around line 52-63: Update fetchOAuthUserInfo to delegate directly to the
three-argument OAuthClientService.getUserInfo(socialType, credential,
redirectUri) overload, removing the credential type and redirectUri branching
while preserving the service’s existing default handling.
- Around line 65-67: Update the 3-argument socialLogin(SocialType, String,
String) path so it delegates to the transactional 4-argument socialLogin through
a separate Spring service bean or injected self-proxy, rather than direct
self-invocation. Preserve the existing OAuthCredential construction and
arguments while ensuring the `@Transactional` method is invoked through the AOP
proxy.
In `@src/main/java/com/mr/domain/auth/service/OAuthClientService.java`:
- Around line 40-66: Remove the redundant credential-type branch from the
two-argument getUserInfo(SocialType, OAuthCredential) overload and delegate
directly to the three-argument getUserInfo(SocialType, OAuthCredential, String)
with a null customRedirectUri, preserving the existing default redirect
resolution.
- Around line 264-277: Replace the fully qualified AuthResponseDTO and
UriComponentsBuilder references used in OAuthClientService, including
buildFrontendRedirectUrl, with imports declared at the top of the file; preserve
the existing behavior and signatures.
In `@src/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java`:
- Around line 159-190: OAuthClientServiceTest에 새로 추가된 getAuthorizationUrl과
buildFrontendRedirectUrl 테스트를 보강하세요. getAuthorizationUrl은 scope의 공백이 올바르게 URL
인코딩되는지와 state 파라미터가 포함되는지 검증하고, buildFrontendRedirectUrl은 반환 URL에 필요한 쿼리 파라미터가
정확히 구성되는지 검증하는 테스트를 추가하세요.
🪄 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: ee83c145-fe38-406a-acf5-94c90e370f3e
📒 Files selected for processing (7)
src/main/java/com/mr/domain/auth/controller/AuthController.javasrc/main/java/com/mr/domain/auth/dto/req/AuthRequestDTO.javasrc/main/java/com/mr/domain/auth/service/AuthService.javasrc/main/java/com/mr/domain/auth/service/OAuthClientService.javasrc/main/java/com/mr/global/config/OAuthProperties.javasrc/main/java/com/mr/global/security/SecurityConfig.javasrc/test/java/com/mr/domain/auth/service/OAuthClientServiceTest.java
| @GetMapping("/{socialType}/callback") | ||
| public void oAuthCallback( | ||
| @Parameter(description = "소셜 로그인 제공자 (KAKAO, GOOGLE)", example = "KAKAO") | ||
| @PathVariable(name = "socialType") SocialType socialType, | ||
| @RequestParam(name = "code", required = false) String code, | ||
| @RequestParam(name = "error", required = false) String error, | ||
| @RequestParam(name = "redirectUri", required = false) String customRedirectUri, | ||
| @RequestHeader(value = HttpHeaders.USER_AGENT, defaultValue = "Unknown Device") String deviceInfo, | ||
| HttpServletResponse response | ||
| ) throws IOException { | ||
| if (error != null || code == null || code.isBlank()) { | ||
| throw new GeneralException(AuthErrorStatus.INVALID_AUTH_REQUEST); | ||
| } | ||
|
|
||
| OAuthCredential credential = new OAuthCredential(OAuthCredential.CredentialType.AUTHORIZATION_CODE, code); | ||
| AuthResponseDTO.LoginResponse loginResponse = authService.socialLogin( | ||
| socialType, | ||
| credential, | ||
| customRedirectUri, | ||
| deviceInfo | ||
| ); | ||
|
|
||
| String targetFrontendUrl = oAuthClientService.buildFrontendRedirectUrl(loginResponse); | ||
| response.sendRedirect(targetFrontendUrl); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
콜백 실패 시에도 프론트엔드로 리다이렉트해 주세요.
error가 있거나 code가 비어있을 때 GeneralException을 던지는데, 이 엔드포인트는 사용자의 브라우저가 직접 GET으로 도달하는 리다이렉트 타겟입니다(302로 이 URL에 도착). 예외를 던지면 브라우저에 원시 JSON 에러 응답이 그대로 표시되어(전역 예외 핸들러 응답 형식 그대로), 사용자는 "로그인 취소" 같은 정상적인 흐름에서도 개발자용 에러 화면을 보게 됩니다.
startOAuthLogin처럼 프론트엔드 URL로 sendRedirect하되, 에러 코드를 쿼리파라미터로 실어 보내는 방식을 권장합니다.
또한 이 콜백은 OAuth 표준의 state 파라미터 검증이 빠져 있어 인가 코드 CSRF에 취약합니다(별도로도 짚었습니다).
🛠️ 개선 제안 예시
if (error != null || code == null || code.isBlank()) {
- throw new GeneralException(AuthErrorStatus.INVALID_AUTH_REQUEST);
+ String errorRedirectUrl = oAuthClientService.buildFrontendErrorRedirectUrl(error != null ? error : "invalid_request");
+ response.sendRedirect(errorRedirectUrl);
+ return;
}🤖 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/mr/domain/auth/controller/AuthController.java` around lines
62 - 86, Update AuthController.oAuthCallback so invalid OAuth callbacks
(non-null error or missing/blank code) redirect to the frontend instead of
throwing GeneralException, passing an appropriate error code as a query
parameter while preserving normal login redirects. Follow the existing
startOAuthLogin redirect/error-handling pattern and keep the state-parameter
validation concern separate.
| public String getAuthorizationUrl(SocialType socialType, String customRedirectUri) { | ||
| OAuthProperties.ProviderProperties props = oAuthProperties != null ? switch (socialType) { | ||
| case KAKAO -> oAuthProperties.kakao(); | ||
| case GOOGLE -> oAuthProperties.google(); | ||
| } : null; | ||
|
|
||
| String clientId = props != null ? props.clientId() : null; | ||
| if (clientId == null || clientId.isBlank()) { | ||
| log.error("{} OAuth configuration is missing (client_id is not set)", socialType); | ||
| throw new GeneralException(AuthErrorStatus.OAUTH_SERVER_ERROR); | ||
| } | ||
|
|
||
| String redirectUri = resolveRedirectUri(props, customRedirectUri, socialType.name()); | ||
|
|
||
| return switch (socialType) { | ||
| case KAKAO -> org.springframework.web.util.UriComponentsBuilder.fromUriString("https://kauth.kakao.com/oauth/authorize") | ||
| .queryParam("response_type", "code") | ||
| .queryParam("client_id", clientId) | ||
| .queryParam("redirect_uri", redirectUri) | ||
| .build().toUriString(); | ||
| case GOOGLE -> org.springframework.web.util.UriComponentsBuilder.fromUriString("https://accounts.google.com/o/oauth2/v2/auth") | ||
| .queryParam("response_type", "code") | ||
| .queryParam("client_id", clientId) | ||
| .queryParam("redirect_uri", redirectUri) | ||
| .queryParam("scope", "email profile openid") | ||
| .build().toUriString(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
OAuth 2.0 authorization code flow state parameter CSRF protection best practice
💡 Result:
To protect against Cross-Site Request Forgery (CSRF) in the OAuth 2.0 authorization code flow, the industry-standard best practice is to ensure that every authorization request is cryptographically bound to the user-agent session [1][2][3]. According to the latest OAuth 2.0 Security Best Current Practice (RFC 9700), clients must implement one of the following mechanisms to prevent CSRF [1][2][3]: 1. Proof Key for Code Exchange (PKCE): If a client has confirmed that the authorization server supports PKCE, it may rely on the CSRF protection provided by PKCE [1][2][3]. The PKCE code challenge and verifier process ensures that the authorization code is bound to the specific client transaction [4]. 2. OpenID Connect Nonce: In OpenID Connect (OIDC) flows, the 'nonce' parameter serves as a cryptographically bound, transaction-specific value that provides CSRF protection [1][2][3]. 3. State Parameter: If PKCE or OIDC 'nonce' is not used (or if the authorization server does not support them), the client must use the 'state' parameter [1][2][3]. When using the 'state' parameter, follow these best practices: - Uniqueness and Opacity: The value must be a high-entropy, unpredictable, and transaction-specific string (a CSRF token) [5][6]. - Binding: The value must be securely bound to the user-agent session (e.g., stored in a cryptographically signed cookie or an encrypted server-side session) [1][7][5]. - Validation: Upon receiving the authorization response, the client must verify that the 'state' value returned by the authorization server matches the value it stored in the user-agent session [5][6][8]. If the values do not match, the client must reject the request as a potential CSRF attack [5][6]. - Tamper Resistance: If the 'state' parameter is used to carry additional application state, ensure the integrity of the data is protected against tampering and swapping, for example, by signing or encrypting the contents [2][9]. In summary, while the 'state' parameter was the traditional method for CSRF protection, modern implementations favor PKCE as a robust, built-in mechanism for securing the authorization code flow [1][7][2]. Always ensure the authorization server supports the chosen protection mechanism before relying on it [1][2][9].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc9700.pdf
- 2: https://www.rfc-editor.org/info/rfc9700/
- 3: https://datatracker.ietf.org/doc/draft-ietf-oauth-security-topics/29/
- 4: https://cheatsheetseries.owasp.org/cheatsheets/OAuth2_Cheat_Sheet.html
- 5: https://auth0.com/docs/secure/attack-protection/state-parameters
- 6: https://learn.microsoft.com/en-us/linkedin/shared/api-guide/best-practices/secure-applications
- 7: https://drafts.oauth.net/oauth-browser-based-apps/draft-ietf-oauth-browser-based-apps.html
- 8: https://shibboleth.atlassian.net/wiki/spaces/DEV/pages/5230952453/OAuth+2.0+and+OIDC+State+Handling
- 9: https://datatracker.ietf.org/doc/rfc9700/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -i 'OAuthClientService.java|OauthCallback|OAuth.*Callback|Auth|Security|Config' . | sed -n '1,120p'
echo
echo "== search state/code/callback usages =="
rg -n "OAuthClientService|getAuthorizationUrl|redirect_uri|authorization_code|state|scope|code|oauth|oauth2|OauthCallback|OAuthCallback" -S . \
-g '!build' -g '!target' -g '!node_modules' | sed -n '1,240p'
echo
echo "== Inspect OAuthClientService relevant section =="
FILE="$(fd -i 'OAuthClientService.java' . | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
echo "-- $FILE lines 1-340 --"
cat -n "$FILE" | sed -n '1,340p'
fi
echo
echo "== Inspect callback/controller/service candidates =="
for f in $(fd -i '*Callback*.java|*OAuth*Controller.java|*OAuth*Service.java|*AuthService.java' . | sed -n '1,40p'); do
echo "-- $f line count --"
wc -l "$f"
echo "-- $f outline --"
ast-grep outline "$f" --view compact 2>/dev/null | sed -n '1,120p' || true
doneRepository: Musereview/BE
Length of output: 2008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pwd =="
pwd
echo
echo "== top-level entries =="
ls -la | sed -n '1,120p'
echo
echo "== git availability =="
git status || true
echo
echo "== locate relevant files =="
fd -i 'OAuthClientService.java|OauthCallback|OAuth.*Callback|Auth|Security|Config' . | sed -n '1,160p' || true
echo
echo "== candidate Java files for OAuth handling =="
fd -i '(OAuthClientService|OAuth.*Callback|Oauth.*Callback|.*OAuth.*Service|.*OAuth.*Controller|.*Auth.*Service|.*Auth.*Controller).*\.java' . | sed -n '1,200p' || true
echo
echo "== exact target =="
if [ -f src/main/java/com/mr/domain/auth/service/OAuthClientService.java ]; then
wc -l src/main/java/com/mr/domain/auth/service/OAuthClientService.java
cat -n src/main/java/com/mr/domain/auth/service/OAuthClientService.java | sed -n '1,360p'
else
echo "target file not found at expected path or repository unavailable"
fiRepository: Musereview/BE
Length of output: 2008
🌐 Web query:
Spring Framework UriComponentsBuilder queryParam encode queryParameters build encode documentation
💡 Result:
In the Spring Framework, UriComponentsBuilder provides flexible ways to handle URI encoding, particularly when working with query parameters. Encoding Options UriComponentsBuilder exposes two primary levels of encoding, which determine how and when characters are processed: 1. UriComponentsBuilder#encode: This is generally recommended for most use cases [1][2]. It pre-encodes the URI template at build time and applies strict encoding to URI variables separately when they are expanded [3][1]. Because it treats URI variables as opaque data, it ensures they are fully encoded, including characters with reserved meaning [1][4]. 2. UriComponents#encode: This method is called after building the components (post-expansion) [1][2]. It encodes URI components after URI variables have been expanded [4]. This approach is preferred if you intentionally need to include reserved characters within your URI variables or if you are not expanding variables at all [1][5]. Handling Query Parameters When using queryParam(String name, Object... values), you can include URI template variables (e.g., "?q={value}") [3][6]. The encoding behavior depends on the chosen method: - If you call UriComponentsBuilder#encode, Spring will perform pre-encoding on the template and then strictly encode the expanded variable values [1][4]. - Note that standard encoding applied via the builder only targets characters illegal in a query parameter name or value (such as "=" or "&") [3][7]. Characters that are legal according to RFC 3986 are typically not encoded automatically, which includes the "+" character [3][6]. To achieve stricter encoding for specific values, you should use URI template variables [3][7]. Common Workflow The standard pattern for building a URI with encoded query parameters is: UriComponents uriComponents = UriComponentsBuilder.fromUriString("https://example.com/search").queryParam("q", "{queryValue}").encode // Pre-encodes template and sets strategy for strict variable encoding.buildAndExpand("my search value").toUri; DefaultUriBuilderFactory In many Spring applications (such as those using WebClient or RestTemplate), the DefaultUriBuilderFactory is used to manage encoding consistently [1][4]. It allows you to set an EncodingMode, such as TEMPLATE_AND_VALUES (which uses UriComponentsBuilder#encode) or URI_COMPONENT (which uses UriComponents#encode) [4][8]. Defaulting behavior may vary by Spring version and client implementation [8].
Citations:
- 1: https://docs.spring.io/spring-framework/reference/web/webflux/uri-building.html
- 2: https://docs.spring.io/spring/reference/6.2-SNAPSHOT/web/webmvc/mvc-uri-building.html
- 3: https://docs.spring.io/spring-framework/docs/6.2.18/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 4: https://docs.spring.io/spring/reference/web/webmvc/mvc-uri-building.html
- 5: https://docs.spring.io/spring-framework/docs/6.2.16/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 6: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 7: https://docs.spring.io/spring-framework/docs/7.0.8/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 8: https://docs.spring.io/spring/reference/7.0-SNAPSHOT/web/webflux/uri-building.html
🌐 Web query:
Spring Framework UriComponentsBuilder encode queryParam queryParameters spaces percent encoding Java documentation
💡 Result:
In the Spring Framework, UriComponentsBuilder handles query parameter encoding according to RFC 3986 [1]. When using queryParam, the default encoding behavior is selective: it only encodes characters that are considered illegal in a query parameter name or value (such as "=" or "&") [2][3]. Important details regarding spaces and character encoding include: 1. Selective Encoding: Many characters that are technically legal within the RFC 3986 syntax for query components are not automatically encoded [2][3]. Because the "+" character is considered legal in query parameters, it is not automatically converted to an encoded space (e.g., %20) [2][3]. This can lead to ambiguity if the receiving end interprets "+" as a space [4]. 2. Stricter Encoding: To achieve stricter encoding—where URI variables are treated as opaque data and fully encoded—you should use URI template variables in conjunction with the encode method [5][2][6]. Example: UriComponents uri = UriComponentsBuilder.fromUriString("https://example.com/search").queryParam("q", "{q}").encode.buildAndExpand("a space").toUri; This approach ensures that the variable "a space" is properly encoded (e.g., as "a%20space") [5][6]. 3. Manual vs. Framework Encoding: Spring expects you to be aware of whether the data you provide to queryParam is already encoded [4]. If you provide raw, unencoded strings that contain characters requiring specific encoding, using URI template variables combined with.encode is the recommended practice for robust, framework-managed encoding [5][4]. For further guidance, consult the URI Encoding section of the official Spring Framework reference documentation [5][4].
Citations:
- 1: https://stackoverflow.com/questions/47217647/encoding-query-parameters-with-uricomponentsbuilder
- 2: https://docs.spring.io/spring-framework/docs/6.2.18/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 3: https://docs.spring.io/spring-framework/docs/7.0.7/javadoc-api/org/springframework/web/util/UriComponentsBuilder.html
- 4: UriComponents allows "+" in a query param spring-projects/spring-framework#31363
- 5: https://docs.spring.io/spring/reference/web/webmvc/mvc-uri-building.html
- 6: https://docs.spring.io/spring-framework/reference/7.1-SNAPSHOT/web/webmvc/mvc-uri-building.html
🌐 Web query:
Java URI class equals query parameter spaces raw URI malformed URI exception
💡 Result:
When using the java.net.URI class, characters that are not legal URI characters, such as spaces, must be properly encoded to avoid a URISyntaxException [1][2][3]. Malformed URI Exceptions If you pass a string containing unencoded spaces (or other illegal characters) directly into the single-argument URI constructor (e.g., new URI("http://example.com/search?q=my query")), the constructor will throw a URISyntaxException because it attempts to parse the string as a strict RFC 2396/3986 URI [3][4]. To avoid this, you should either encode the input string before passing it to the constructor or use the multi-argument URI constructors, which perform the necessary encoding automatically [3][4][5]. Multi-Argument Constructors The recommended approach for building URIs in Java is to use the multi-argument constructors [4][5]. These constructors accept the individual components of the URI—such as scheme, authority, path, query, and fragment—as separate arguments [6][5]. The URI class then handles the correct percent-encoding for each component [7][5]. For example, to include a space in a query parameter, you should separate the components rather than constructing the entire URL string yourself: URI uri = new URI("http", "example.com", "/search", "q=my query", null); The resulting URI string will automatically represent the space as %20, producing: http://example.com/search?q=my%20query [7][5]. Equality and Comparison The equals method in the java.net.URI class compares URIs based on their raw, unencoded components [1][8]. For two hierarchical URIs to be considered equal, their paths must be equal and their queries must either both be undefined or else be equal [1][9]. Because the comparison happens on the raw forms (rather than the encoded forms), the URI class effectively compares the underlying data before any percent-encoding is applied [1][10]. Therefore, two URI objects are considered equal if their components are identical, regardless of the percent-encoding scheme used for those components [1][8]. Distinction from URLEncoder It is important to note that java.net.URLEncoder is a legacy class designed primarily for application/x-www-form-urlencoded data (such as HTML form submissions) [7][5]. A key difference is that URLEncoder converts spaces to plus signs (+), whereas the URI class, which follows RFC 3986, correctly converts spaces to %20 [7][5]. For REST APIs and modern web standards, you should prefer the java.net.URI class [5].
Citations:
- 1: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/net/URI.html
- 2: https://developer.android.com/reference/java/net/URI
- 3: https://stackoverflow.com/questions/749709/how-to-deal-with-the-urisyntaxexception
- 4: https://stackoverflow.com/questions/304806/encode-and-decode-rfc2396-urls
- 5: https://www.devformatters.com/blog/urlencoder--Java-URL-Encoding
- 6: https://www.baeldung.com/java-url-vs-uri
- 7: https://www.proofbyexample.com/url-and-uri-encoding-in-java.html
- 8: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URI.html
- 9: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/net/URI.html
- 10: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/net/URI.java
OAuth 인가 요청에 CSRF 보호용 state를 포함시키세요.
getAuthorizationUrl은 카카오/구글 인가 URL을 만들지만 CSRF 보호를 위한 state를 전혀 포함하지 않습니다. OAuth 2.0 인가 코드 흐름에서는 state가 요청과 콜백을 바인딩해 피크업 계정으로 로그인하는 공격을 막는 표준 기제입니다(RFC 6749 §10.12, OAuth Security Best Current Practice).
개선 방향: 인가 요청 전에 랜덤하고 예측 불가능한 state를 생성해 짧은 TTL로 저장한 뒤, 카카오/구글 반환 URL의 state를 같은 값과 비교하는 검증 로직을 oAuthCallback 에 추가하세요. PKCE나 OpenID Connect nonce를 사용할 수도 있지만, 이 흐름에서는 state가 최소 요건입니다.
🤖 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/mr/domain/auth/service/OAuthClientService.java` around
lines 235 - 262, Update getAuthorizationUrl to generate a cryptographically
random, unpredictable state before building the Kakao or Google authorization
URL, store it with a short TTL keyed to the login attempt, and include it as the
state query parameter. Extend oAuthCallback to retrieve and compare the stored
state with the callback value, rejecting missing or mismatched values before
processing the authorization code.
| public String buildFrontendRedirectUrl(com.mr.domain.auth.dto.res.AuthResponseDTO.LoginResponse loginResponse) { | ||
| String baseUrl = (oAuthProperties != null && oAuthProperties.frontendRedirectUri() != null && !oAuthProperties.frontendRedirectUri().isBlank()) | ||
| ? oAuthProperties.frontendRedirectUri() | ||
| : "http://localhost:3000/oauth/callback"; | ||
|
|
||
| return org.springframework.web.util.UriComponentsBuilder.fromUriString(baseUrl) | ||
| .queryParam("accessToken", loginResponse.tokenInfo().accessToken()) | ||
| .queryParam("refreshToken", loginResponse.tokenInfo().refreshToken()) | ||
| .queryParam("userId", loginResponse.userId()) | ||
| .queryParam("isNewUser", loginResponse.isNewUser()) | ||
| .queryParam("isOnboardingCompleted", loginResponse.isOnboardingCompleted()) | ||
| .build() | ||
| .toUriString(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate OAuthClientService.java =="
fd -a 'OAuthClientService\.java$' .
echo "== relevant lines =="
file="$(fd 'OAuthClientService\.java$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '230,285p' "$file" | cat -n
sed -n '1,90p' "$file" | cat -n
fi
echo "== search frontendRedirectUri usages =="
rg -n "frontendRedirectUri|buildFrontendRedirectUrl|refreshToken|accessToken|OAuthServer|OAUTH_SERVER_ERROR|resolveRedirectUri" .Repository: Musereview/BE
Length of output: 2008
🌐 Web query:
RFC 6749 section 10.16 access token in query string fragment URL exposure OAuth tokens
💡 Result:
RFC 6749 Section 10.16 addresses the "Misuse of Access Token to Impersonate Resource Owner in Implicit Flow," noting that when access tokens are exposed in the URI (such as via the fragment component in the Implicit Grant flow), they are susceptible to leakage through various channels [1][2]. The security concerns regarding access tokens in URLs—whether in query parameters or fragments—are well-documented and represent a significant security risk [3][4]. Placing sensitive tokens in URIs exposes them to unintended discovery through [5][6]: - Browser History: Tokens contained in a URL remain in the browser's history, where they can be accessed by other users of the same device or via local malicious software [3][4]. - Server and Proxy Logs: URIs (including query parameters) are frequently logged by web servers, load balancers, and reverse proxies, making the tokens visible to administrators and any entity with access to these logs [7][5]. - Referer Headers: When a page containing a token in its URL navigates to another site, the full URL—including the token—may be sent to the destination server via the HTTP Referer header [5][6]. Because of these risks, modern OAuth 2.0 security standards, including RFC 9700, strictly advise against passing access tokens in URI query parameters [3][4]. Furthermore, the Implicit Grant flow (which relied on URL fragments to return tokens) is now deprecated and removed from OAuth 2.1 in favor of the Authorization Code Grant with PKCE, which avoids exposing access tokens in the front-channel URL [8][9][6]. Developers are strongly encouraged to transmit access tokens using secure mechanisms, such as the Authorization request header (e.g., Authorization: Bearer ) [5][6].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc6749.html
- 2: https://datatracker.ietf.org/doc/html/rfc6749
- 3: https://datatracker.ietf.org/doc/html/rfc9700
- 4: https://datatracker.ietf.org/doc/rfc9700/
- 5: https://auditbuffet.com/patterns/ab-002210
- 6: https://justappsec.com/guides/oauth-2-security-best-practices
- 7: https://stackoverflow.com/questions/16733863/oauth2-0-implicit-grant-flow-why-use-url-hash-fragments
- 8: https://cheatsheetseries.owasp.org/cheatsheets/OAuth2_Cheat_Sheet.html
- 9: https://cheatsheetseries.owasp.org/cheatsheets/OAuth2%5FCheat%5FSheet.html
🔴 토큰 원문을 URL 쿼리파라미터로 넘기지 마세요.
buildFrontendRedirectUrl이 accessToken/refreshToken을 프론트엔드 리다이렉트 URL에 노출하므로 브라우저 히스토리·서버/프록시 로그·Referer 헤더 경로를 통해 유출될 수 있습니다. refreshToken은 특히 장수명 시신분 계정의 직접 탈취로 이어질 수 있어 보안상 허용되지 않습니다(OAuth 2.0은 URI에서 토큰을 넘기는 방식 사용을 권장하지 않습니다; RFC 6749 §10.16, OWASP OAuth 2.0 Cheat Sheet 참고).
토큰 원문 대신 1회용 교환 코드를 쿼리로 전달하고, 프론트엔드가 별도 HTTPS + 검증된 POST API로 토큰을 교환하도록 하는 방향, 또는 refreshToken은 HttpOnly Secure 쿠키로 전달하는 방식을 사용하는 편이 안전합니다.
🤖 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/mr/domain/auth/service/OAuthClientService.java` around
lines 264 - 277, Update buildFrontendRedirectUrl so it never places accessToken
or refreshToken in the redirect query parameters. Replace the raw-token
parameters with a single-use exchange code that the frontend can redeem through
the established secure HTTPS POST flow, or deliver refreshToken through an
HttpOnly Secure cookie while retaining only non-sensitive response data in the
URL.
3 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit
새로운 기능
버그 수정