[refactor] 소셜 로그인 동적 리다이렉트되도록 변경 #307
Conversation
WalkthroughOAuth2 동적 리다이렉트와 리퀘스트/세션 기반 리다이렉트 도메인 처리 추가, 허용 도메인 검증 및 관련 에러코드 추가, 로그인 토큰 저장소를 인터페이스로 분리하고 메모리/레디스 구현을 도입, 리졸버 패키지 정리 및 Security/CORS 설정을 WebDomainProperties로 동적 구성. (50단어 이내) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FE as Frontend
participant BE as Backend
participant AR as CustomAuthorizationRequestResolver
participant WD as WebDomainProperties
participant OP as OAuth2 Provider
participant ST as LoginTokenStorage
FE->>BE: GET /oauth2/authorization/{client}?redirect_url=...
BE->>AR: resolve(request)
AR->>WD: isAllowed(redirect_url)?
alt 허용됨
AR->>BE: session[REDIRECT_SESSION_KEY] = origin
else 미허용/없음
AR->>BE: session.remove(REDIRECT_SESSION_KEY)
end
BE->>OP: 리다이렉트 (OAuth2 인증)
OP-->>BE: 콜백
BE->>BE: CustomSuccessHandler.onAuthenticationSuccess
BE->>BE: session에서 origin 복원
BE->>WD: isAllowed(origin)?
alt 허용목록 비어있음
BE-->>FE: 에러 (WEB_DOMAIN_ORIGIN_EMPTY)
else 허용됨
BE->>ST: put(key,type,token,ttl)
BE-->>FE: 최종 리다이렉트 (토큰/쿠키 포함)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Test Results474 tests 474 ✅ 44s ⏱️ Results for commit 2052fcf. ♻️ This comment has been updated with latest results. |
![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 (2)
src/main/java/konkuk/thip/common/security/oauth2/auth/AuthController.java (2)
159-220: userId 신뢰 문제: 임의 계정 토큰 발급 가능성exchangeTempToken에서 body로 받은 userId를 신뢰해 액세스 토큰을 발급합니다. oauth2Id와 userId의 소유 관계 검증이 없어 계정 탈취 리스크가 있습니다. Deprecated라도 방치하면 취약점입니다. 즉시 검증 추가 또는 엔드포인트 제거를 권장합니다.
검증을 유지하려면 다음과 같이 매핑 확인을 추가하세요(예시):
- Long userId = Long.valueOf(userIdStr); + Long userId = Long.valueOf(userIdStr); + var user = userJpaRepository.findById(userId) + .orElseThrow(() -> new BusinessException(ErrorCode.USER_NOT_FOUND)); + if (!oauth2Id.equals(user.getOauth2Id())) { + return ResponseEntity.status(HttpStatus.FORBIDDEN).build(); + }
106-153: Deprecated 엔드포인트(쿠키 설정·임시 토큰 교환) 즉시 비활성화 — 운영환경 노출 금지
- 문제: src/main/java/konkuk/thip/common/security/oauth2/auth/AuthController.java (약 106–153행)에 @deprecated만 붙은 setCookie 및 exchangeTempToken 엔드포인트가 남아 있으며 컨트롤러에 @Profile/@conditional이 없어 운영환경에서 그대로 노출됩니다.
- 조치(택1): 메서드 또는 컨트롤러에 @Profile("local|test") 또는 @ConditionalOnProperty(name="thip.deprecated-api.enabled", havingValue="true") 추가해 로컬/테스트에서만 활성화.
- 조치(택2): 또는 src/main/java/konkuk/thip/config/SecurityConfig.java의 filterChain에 requestMatchers("/auth/set-cookie","/auth/exchange-temp-token").denyAll() 또는 적절한 권한 제한을 추가해 운영환경에서 접근 차단.
- 참고: LoginTokenStorage 구현체는 @Profile로 분리되어 있으나(예: MemoryLoginTokenStorage @Profile("test"), RedisLoginTokenStorage @Profile({"!test"})) 스토리지 프로파일만으로는 엔드포인트 비노출을 보장하지 않습니다.
🧹 Nitpick comments (15)
src/main/java/konkuk/thip/common/security/resolver/Oauth2IdArgumentResolver.java (1)
32-36: 매직 스트링 제거 제안: AuthParameters 사용요청 속성 키 "oauth2Id"를 상수로 치환하면 변경 내구성이 좋아집니다.
+import static konkuk.thip.common.security.constant.AuthParameters.JWT_SIGNUP_TOKEN_KEY; - Object oauth2Id = ((HttpServletRequest) webRequest.getNativeRequest()).getAttribute("oauth2Id"); + Object oauth2Id = ((HttpServletRequest) webRequest.getNativeRequest()) + .getAttribute(JWT_SIGNUP_TOKEN_KEY.getValue());src/main/java/konkuk/thip/common/security/resolver/AuthTokenArgumentResolver.java (1)
34-38: 매직 스트링 제거 및 상수 사용요청 속성 "token"은 AuthParameters.JWT_TOKEN_ATTRIBUTE로 통일하는 게 안전합니다.
+import static konkuk.thip.common.security.constant.AuthParameters.JWT_TOKEN_ATTRIBUTE; ... - Object token = ((HttpServletRequest) webRequest.getNativeRequest()).getAttribute("token"); + Object token = ((HttpServletRequest) webRequest.getNativeRequest()) + .getAttribute(JWT_TOKEN_ATTRIBUTE.getValue());src/main/java/konkuk/thip/common/security/resolver/UserIdArgumentResolver.java (1)
32-37: 타입 캐스팅 안전성 강화필터/인터셉터에서 userId를 Number/String으로 넣는 변형에 대비해 안전 캐스팅을 권장합니다. 또한 키 상수 사용을 제안합니다.
+import static konkuk.thip.common.security.constant.AuthParameters.JWT_ACCESS_TOKEN_KEY; ... - Object userId = ((HttpServletRequest) webRequest.getNativeRequest()).getAttribute("userId"); + Object userId = ((HttpServletRequest) webRequest.getNativeRequest()) + .getAttribute(JWT_ACCESS_TOKEN_KEY.getValue()); if (userId == null) { throw new AuthException(AUTH_TOKEN_NOT_FOUND); } - return (Long) userId; + if (userId instanceof Long l) return l; + if (userId instanceof Number n) return n.longValue(); + if (userId instanceof String s) return Long.parseLong(s); + throw new AuthException(AUTH_TOKEN_NOT_FOUND);src/main/java/konkuk/thip/common/security/oauth2/CustomSuccessHandler.java (1)
69-77: URL 문자열 결합 대신 UriComponentsBuilder 사용직접 문자열 결합은 슬래시 중복/누락과 인젝션 리스크가 있습니다. 안전한 URL 빌더를 사용하세요.
- getRedirectStrategy().sendRedirect(request, response, webRedirectDomain + REDIRECT_SIGNUP_URL.getValue() + "?loginTokenKey=" + loginTokenKey); + String signupUrl = UriComponentsBuilder.fromHttpUrl(webRedirectDomain) + .path(REDIRECT_SIGNUP_URL.getValue()) + .queryParam("loginTokenKey", loginTokenKey) + .build(true) + .toUriString(); + getRedirectStrategy().sendRedirect(request, response, signupUrl); ... - getRedirectStrategy().sendRedirect(request, response, webRedirectDomain + REDIRECT_HOME_URL.getValue() + "?loginTokenKey=" + loginTokenKey); + String homeUrl = UriComponentsBuilder.fromHttpUrl(webRedirectDomain) + .path(REDIRECT_HOME_URL.getValue()) + .queryParam("loginTokenKey", loginTokenKey) + .build(true) + .toUriString(); + getRedirectStrategy().sendRedirect(request, response, homeUrl);src/main/java/konkuk/thip/common/security/oauth2/auth/AuthController.java (3)
95-101: 토큰 타입 분기 명시화 및 예외 처리 추가 권장현재 else 분기에서 TEMP 외의 알 수 없는 타입도 허용됩니다. 미지의 타입을 조용히 신규 유저로 처리하면 보안·로직 리스크가 있습니다. switch/else-if로 명시화하고, 그 외에는 예외를 던지도록 수정하세요.
- if (entry.type() == ACCESS) { - token = entry.token(); - isNewUser = false; - } else { - token = entry.token(); - isNewUser = true; - } + switch (entry.type()) { + case ACCESS -> { + token = entry.token(); + isNewUser = false; + } + case TEMP -> { + token = entry.token(); + isNewUser = true; + } + default -> throw new AuthException(API_INVALID_PARAM, + new IllegalArgumentException("Unsupported token type: " + entry.type())); + }
130-148: 쿠키 분기에도 미지의 타입 방어 코드 추가set-cookie 엔드포인트는 Deprecated지만, 동일하게 미지 타입 방어 로직을 넣어두는 편이 안전합니다.
- if (entry.type() == ACCESS) { + if (entry.type() == ACCESS) { cookie = ResponseCookie.from(COOKIE_ACCESS_TOKEN.getValue(), entry.token()) .httpOnly(true) .secure(true) .sameSite("None") .path("/") .maxAge(Duration.ofDays(30)) .build(); type = ACCESS.getValue(); - } else { + } else if (entry.type() == TEMP) { cookie = ResponseCookie.from(COOKIE_TEMP_TOKEN.getValue(), entry.token()) .httpOnly(true) .secure(true) .sameSite("None") .path("/") .maxAge(Duration.ofMinutes(10)) .build(); type = TEMP.getValue(); + } else { + throw new AuthException(API_INVALID_PARAM, + new IllegalArgumentException("Unsupported token type: " + entry.type())); }
77-85: 요청 DTO 명명 혼선/auth/token에서 AuthSetCookieRequest를 사용합니다. 의미상 AuthTokenKeyRequest 등으로 분리하면 API 의도가 명확합니다. 호환성 영향이 크면 주석으로 구분을 명시하세요.
src/main/java/konkuk/thip/config/properties/WebDomainProperties.java (1)
18-18: 동적 로컬 IP 허용 요구사항 반영(개발/테스트 한정)QA 시 현재 Wi‑Fi IP로의 리다이렉트를 허용하려면, dev/test 프로파일에서 RFC1918(10/172.16–31/192.168)·링크로컬(169.254)·로컬호스트를 허용하는 선택적 규칙을 지원하는 게 실용적입니다. 프로퍼티에 패턴/대역을 명시하는 방식을 고려하세요(예: server.web-domain-urls-patterns).
현재 yml에 동적 IP가 반영되지 않으면 실제 요구사항을 만족하기 어렵습니다. 운영/개발 별 허용 목록을 확인 부탁드립니다.
src/main/java/konkuk/thip/config/SecurityConfig.java (2)
108-112: 프로퍼티 리스트를 직접 변형함 → 사이드이펙트 및 중복 가능성webDomainProperties의 내부 리스트를 mutate하면 다른 빈에서 동일 리스트를 참조할 때 부작용이 생길 수 있습니다. null/공백 필터 및 중복 제거도 권장합니다.
- List<String> allowedOrigins = webDomainProperties.getWebDomainUrls(); - allowedOrigins.addAll(List.of(prodServerUrl, devServerUrl)); - - config.setAllowedOrigins(allowedOrigins); + List<String> allowedOrigins = + new java.util.ArrayList<>( + webDomainProperties.getWebDomainUrls().stream() + .filter(o -> o != null && !o.isBlank()) + .toList() + ); + if (prodServerUrl != null && !prodServerUrl.isBlank()) allowedOrigins.add(prodServerUrl); + if (devServerUrl != null && !devServerUrl.isBlank()) allowedOrigins.add(devServerUrl); + // 중복 제거 + allowedOrigins = allowedOrigins.stream().distinct().toList(); + + config.setAllowedOrigins(allowedOrigins);
106-123: 동적 IP CORS 대응동적 로컬 IP를 허용해야 한다면 setAllowedOriginPatterns 사용을 고려하세요(와일드카드 패턴). 단, 보안상 개발/테스트 프로파일로 제한해야 합니다.
운영 프로파일에서 와일드카드가 활성화되지 않도록 설정을 확인해 주세요.
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/MemoryLoginTokenStorage.java (1)
24-27: TTL 유효성 검증 추가 권장음수/0 TTL이 들어오면 즉시 만료/오동작할 수 있습니다. 사전에 파라미터 검증을 추가하세요.
@Override public void put(String key, TokenType type, String token, Duration ttl) { + if (key == null || key.isBlank() || type == null || token == null || ttl == null || !ttl.isPositive()) { + throw new IllegalArgumentException("Invalid arguments for MemoryLoginTokenStorage.put"); + } long expiredAt = Instant.now().plus(ttl).toEpochMilli(); store.put(key, new InternalEntry(type, token, expiredAt)); }src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/RedisLoginTokenStorage.java (2)
30-43: GETDEL 후 파싱 로직으로 변경하여 프로파일 간 일관성 확보문자열로 저장하면 어떤 시리얼라이저에서도 안정적으로 동작합니다.
- public Entry consume(String key) { - String redisKey = toRedisKey(key); - Object value = redisTemplate.opsForValue().getAndDelete(redisKey); - if (value == null) { - return null; - } - - if (value instanceof Entry entry) { - return entry; - } - - throw new AuthException(ErrorCode.JSON_PROCESSING_ERROR); - } + public Entry consume(String key) { + String redisKey = toRedisKey(key); + Object value = redisTemplate.opsForValue().getAndDelete(redisKey); + if (value == null) return null; + String s = (value instanceof String) ? (String) value : String.valueOf(value); + int idx = s.indexOf(':'); + if (idx <= 0 || idx == s.length() - 1) { + throw new AuthException(ErrorCode.JSON_PROCESSING_ERROR); + } + TokenType type = TokenType.valueOf(s.substring(0, idx)); + String token = s.substring(idx + 1); + return new Entry(type, token); + }
13-19: 권고: 타입 안정성 보강 (Serializer는 이미 설정됨)RedisConfig에서 RedisTemplate<String, Object>의 값 직렬화에 GenericJackson2JsonRedisSerializer가 설정되어 있습니다. Entry는 record로 선언되어 java.io.Serializable을 구현하지 않습니다.
- 권장: RedisLoginTokenStorage가 사용하는 redisTemplate을 RedisTemplate<String, Entry>로 구체화하거나 전용 RedisTemplate<String, Entry> Bean을 등록해 컴파일타임 타입 안정성 확보. (src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/RedisLoginTokenStorage.java, src/main/java/konkuk/thip/config/RedisConfig.java)
- 대안: Entry에 java.io.Serializable 구현 추가 — JDK 직렬화를 선택할 경우 필요. 현재는 GenericJackson2JsonRedisSerializer로 JSON 직렬화가 적용되어 있어 필수는 아님. (src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/LoginTokenStorage.java)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (2)
45-56: provider 별 분리 저장 권장(registrationId 키 포함)요구사항에 “registrationId 기반” 저장이 언급되었으나, 현재 단일 키를 사용합니다. 다중 프로바이더 동시 플로우에서 꼬임을 방지하려면 키를 분리하세요.
- String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); + String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); + String regId = base.getClientRegistrationId(); + String sessionKey = REDIRECT_SESSION_KEY.getValue() + ":" + regId; - var session = request.getSession(true); + var session = request.getSession(true); - if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { - session.setAttribute(REDIRECT_SESSION_KEY.getValue(), redirectUrl); + if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { + session.setAttribute(sessionKey, redirectUrl); } else { - session.removeAttribute(REDIRECT_SESSION_KEY.getValue()); + session.removeAttribute(sessionKey); }
49-56: additionalParameters 복사 불필요추가 파라미터를 수정하지 않으므로 Map 복사는 불필요합니다. 오버헤드 제거를 권장합니다.
- Map<String, Object> additional = new HashMap<>(base.getAdditionalParameters()); - return OAuth2AuthorizationRequest.from(base) - .additionalParameters(additional) - .build(); + return OAuth2AuthorizationRequest.from(base).build();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
src/main/java/konkuk/thip/common/exception/code/ErrorCode.java(1 hunks)src/main/java/konkuk/thip/common/security/constant/AuthParameters.java(1 hunks)src/main/java/konkuk/thip/common/security/oauth2/CustomSuccessHandler.java(3 hunks)src/main/java/konkuk/thip/common/security/oauth2/LoginTokenStorage.java(0 hunks)src/main/java/konkuk/thip/common/security/oauth2/auth/AuthController.java(4 hunks)src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/LoginTokenStorage.java(1 hunks)src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/MemoryLoginTokenStorage.java(1 hunks)src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/RedisLoginTokenStorage.java(1 hunks)src/main/java/konkuk/thip/common/security/resolver/AuthTokenArgumentResolver.java(1 hunks)src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java(1 hunks)src/main/java/konkuk/thip/common/security/resolver/Oauth2IdArgumentResolver.java(1 hunks)src/main/java/konkuk/thip/common/security/resolver/UserIdArgumentResolver.java(1 hunks)src/main/java/konkuk/thip/config/SecurityConfig.java(4 hunks)src/main/java/konkuk/thip/config/WebMvcConfig.java(1 hunks)src/main/java/konkuk/thip/config/properties/WebDomainProperties.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/konkuk/thip/common/security/oauth2/LoginTokenStorage.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/MemoryLoginTokenStorage.java (1)
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/RedisLoginTokenStorage.java (1)
Profile(13-48)
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/RedisLoginTokenStorage.java (1)
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/MemoryLoginTokenStorage.java (1)
Profile(12-44)
🔇 Additional comments (10)
src/main/java/konkuk/thip/common/security/resolver/Oauth2IdArgumentResolver.java (1)
1-1: 패키지 이동 LGTM패키지 경로 변경만 이뤄졌고 동작 영향 없음.
src/main/java/konkuk/thip/config/WebMvcConfig.java (2)
3-5: 패키지 경로 정리 LGTM리졸버 패키지 이동에 맞춘 import 업데이트 적절합니다.
21-26: 리졸버 등록 순서 확인 요청현재는 서로 다른 애노테이션 기반이라 순서 영향이 없겠지만, 이후 조건 확장 시 우선순위 이슈가 생길 수 있습니다. 필요 시 명시적 정렬 고려 바랍니다.
src/main/java/konkuk/thip/common/security/resolver/AuthTokenArgumentResolver.java (1)
1-1: 패키지 이동 LGTMsrc/main/java/konkuk/thip/common/exception/code/ErrorCode.java (1)
30-31: 허용 도메인 목록 미설정 에러코드 추가 LGTM운영 설정 누락 시 500으로 처리하는 선택 합리적입니다. 코드 충돌 없는지 전역적으로 한 번만 확인 부탁드립니다.
src/main/java/konkuk/thip/common/security/oauth2/CustomSuccessHandler.java (1)
43-58: 세션에 저장된 리디렉트 값이 '오리진'인지(아니면 전체 URL인지) 확인 필요CustomSuccessHandler (src/main/java/konkuk/thip/common/security/oauth2/CustomSuccessHandler.java — L43-58)가 세션에서 꺼낸 값을 오리진으로 가정해 검증합니다. Resolver 단계에서 REDIRECT_SESSION_KEY에 저장되는 값이 "http(s)://host[:port]"(origin)인지 확인하세요. 전체 URL(path/query/fragment)이 저장될 수 있다면 저장 전에 origin만 추출하거나, 여기에서 URI로 파싱해 scheme+host+port로 정규화한 뒤 검증하도록 수정해야 합니다.
조치
- 확인 대상: CustomAuthorizationRequestResolver 또는 OAuth2 인증 요청 생성 지점에서 REDIRECT_SESSION_KEY에 어떤 값이 저장되는지 수동 확인.
- 권장 보완(택1):
- Resolver에서 origin만 저장(권장)
- 또는 CustomSuccessHandler에서 URI.parse → scheme+host+port로 정규화 후 webDomainProperties.isAllowed로 비교
로컬에서 관련 참조를 찾는 명령(저장소 루트에서 실행):
rg -n -uu -S -C3 'REDIRECT_SESSION_KEY|REDIRECT_URL_KEY|setAttribute|getParameter' || truesrc/main/java/konkuk/thip/config/SecurityConfig.java (2)
56-62: 커스텀 AuthorizationRequestResolver/세션 저장소 연동 LGTMOAuth2 인가 요청 커스터마이징과 세션 저장소 연결 방식 적절합니다.
66-88: STATELESS + HttpSession 사용 관련 동작 확인 요청코드에서 OAuth2 인가 요청을 HttpSessionOAuth2AuthorizationRequestRepository로 저장하고 있으며 sessionCreationPolicy는 STATELESS로 설정되어 있습니다 (src/main/java/konkuk/thip/config/SecurityConfig.java:64,75,87). 운영 인프라(리버스 프록시/로드밸런서)에서 JSESSIONID가 클라이언트에 유지·전달되는지, 스티키 세션이 필요한지 확인하세요. 필요하면 세션 기반 저장소를 쿠키 기반(HttpCookieOAuth2AuthorizationRequestRepository 등)으로 변경하거나 인프라에서 세션 스티키성을 보장하도록 구성하십시오.
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/MemoryLoginTokenStorage.java (1)
33-43: 원샷 소비 로직 LGTMConcurrentHashMap.remove로 원자적 소비, 만료 확인 로직 적절합니다.
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (1)
48-52: 파라미터 명칭 확인 필요 — 코드에서는 'redirect_url' 사용 중, FE/문서와 일치 여부 확인
- 검증 결과: AuthParameters.REDIRECT_URL_KEY = "redirect_url"로 정의되어 있으며 CustomAuthorizationRequestResolver에서 request.getParameter(REDIRECT_URL_KEY.getValue())로 사용됩니다. 코드베이스에서 "redirect_uri"는 발견되지 않았습니다. (참조: AuthParameters.java, CustomAuthorizationRequestResolver.java, CustomSuccessHandler.java)
- 조치: FE와 키를 'redirect_url'로 합의하거나 요약 문서의 'redirect_uri' 표기를 'redirect_url'로 수정하세요. 만약 FE가 'redirect_uri' 사용을 요구하면 AuthParameters와 해당 사용처(Resolver 및 SuccessHandler의 세션 연동)를 함께 변경해 주세요.
| REDIRECT_URL_KEY("redirect_url"), | ||
| REDIRECT_SESSION_KEY("oauth2_return_to"), | ||
|
|
There was a problem hiding this comment.
❓ Verification inconclusive
파라미터 키 불일치 가능성(redirect_url vs redirect_uri)
PR 요약/가이드에선 redirect_uri를 예시로 들었습니다. 현재 상수는 redirect_url입니다. 클라이언트와 불일치 시 로그인 플로우가 깨집니다. 호환성 위해 둘 다 지원하는 방안을 추천합니다.
COOKIE_TEMP_TOKEN("temp_token"),
- REDIRECT_URL_KEY("redirect_url"),
+ REDIRECT_URL_KEY("redirect_url"),
+ REDIRECT_URI_KEY("redirect_uri"),
REDIRECT_SESSION_KEY("oauth2_return_to"),추가로 Resolver에서 두 키 중 하나를 우선순위로 읽는 로직을 반영해 주세요.
필수: redirect_uri 파라미터 호환성 추가
PR 예시가 redirect_uri인 반면 코드엔 redirect_url만 있어 클라이언트 불일치 시 OAuth 로그인 흐름이 깨질 수 있습니다. 다음 변경 적용 필요 (파일: src/main/java/konkuk/thip/common/security/constant/AuthParameters.java):
COOKIE_TEMP_TOKEN("temp_token"),
- REDIRECT_URL_KEY("redirect_url"),
+ REDIRECT_URL_KEY("redirect_url"),
+ REDIRECT_URI_KEY("redirect_uri"),
REDIRECT_SESSION_KEY("oauth2_return_to"),Resolver에서 REDIRECT_URI_KEY를 우선 읽고 없으면 REDIRECT_URL_KEY를 사용하도록 우선순위 로직 반영.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/konkuk/thip/common/security/constant/AuthParameters.java around
lines 22 to 24, the enum currently only defines REDIRECT_URL_KEY("redirect_url")
which is incompatible with clients sending redirect_uri; add a new enum entry
REDIRECT_URI_KEY("redirect_uri") and then update the OAuth resolver logic to
check for REDIRECT_URI_KEY first and fall back to REDIRECT_URL_KEY when
REDIRECT_URI_KEY is absent so redirect_uri is honored with backward
compatibility.
There was a problem hiding this comment.
pr 의 예시가 오타인 거야. FE와 'redirect_url' 을 key로 설정하는 것으로 결정했어.
There was a problem hiding this comment.
@seongjunnoh 감사합니다! PR 설명의 예시가 오타였다는 점을 명확히 해주셔서 이해했습니다.
FE와 redirect_url을 키로 사용하기로 합의하셨다면 현재 구현이 올바르네요. 제가 제안한 REDIRECT_URI_KEY 추가와 우선순위 로직은 불필요합니다.
현재 REDIRECT_URL_KEY("redirect_url") 구현이 FE와의 합의된 사양에 맞게 잘 되어 있습니다.
🐰✨
╰( ◕ ᗜ ◕ )╯
✏️ Learnings added
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#307
File: src/main/java/konkuk/thip/common/security/constant/AuthParameters.java:22-24
Timestamp: 2025-09-20T09:02:02.821Z
Learning: seongjunnoh는 PR 설명과 실제 구현 간 차이에 대해 명확한 설명을 제공하며, FE와의 협업에서 구체적인 기술 사양(파라미터 키명 등)을 미리 합의하는 방식을 선호한다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#113
File: src/main/java/konkuk/thip/recentSearch/adapter/out/persistence/RecentSearchCommandPersistenceAdapter.java:38-44
Timestamp: 2025-07-30T14:05:04.945Z
Learning: seongjunnoh는 코드 최적화 제안에 대해 구체적인 기술적 근거와 효율성 차이를 이해하고 싶어하며, 성능 개선 방식에 대한 상세한 설명을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#93
File: src/main/java/konkuk/thip/room/adapter/out/persistence/RoomQueryPersistenceAdapter.java:49-114
Timestamp: 2025-07-28T16:44:31.224Z
Learning: seongjunnoh는 코드 중복 문제에 대한 리팩토링 제안을 적극적으로 수용하고 함수형 인터페이스를 활용한 해결책을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#285
File: src/main/java/konkuk/thip/room/adapter/out/jpa/RoomStatus.java:1-7
Timestamp: 2025-08-31T05:25:14.845Z
Learning: seongjunnoh는 enum 의존성에 대해 유연한 접근을 선호하며, 도메인→어댑터 레이어 참조와 같은 아키텍처 layering 원칙보다 실용적인 구현을 우선시한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#195
File: src/main/java/konkuk/thip/feed/application/mapper/FeedQueryMapper.java:0-0
Timestamp: 2025-08-13T05:22:32.287Z
Learning: seongjunnoh는 데이터 무결성과 중복 방지에 대한 고민이 깊으며, LinkedHashSet을 활용한 중복 제거와 순서 보장을 동시에 달성하는 솔루션을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#305
File: src/main/java/konkuk/thip/notification/adapter/out/persistence/repository/NotificationQueryRepository.java:9-13
Timestamp: 2025-09-17T06:40:49.863Z
Learning: seongjunnoh는 메서드 네이밍 시 구현 세부사항보다 비즈니스 의도를 반영하는 것을 선호하며, 미래 확장성과 아키텍처 변화를 고려한 설계 철학을 가지고 있다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#180
File: src/main/java/konkuk/thip/room/application/service/AttendanceCheckCreateService.java:22-40
Timestamp: 2025-08-14T09:15:31.371Z
Learning: seongjunnoh는 기술적 완벽성보다 실제 비즈니스 시나리오와 사용자 행동 패턴을 우선 고려하며, 발생 가능성이 낮은 엣지 케이스보다는 실용적인 구현을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#278
File: src/main/java/konkuk/thip/TestTokenController.java:0-0
Timestamp: 2025-08-24T09:33:52.982Z
Learning: seongjunnoh는 Spring의 ConditionalOnProperty 동작 원리를 정확히 이해하고 있으며, 보안 이슈에 대해서도 실질적인 위험성을 기준으로 판단하는 실용적 접근을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#278
File: src/main/java/konkuk/thip/common/exception/code/ErrorCode.java:210-211
Timestamp: 2025-08-24T09:40:48.595Z
Learning: seongjunnoh는 HTTP 상태 코드 선택에 대해 기술적 근거와 코드베이스 내 일관성을 중요하게 생각하며, 구체적인 사례 분석을 통한 설명을 선호한다.
Learnt from: seongjunnoh
PR: THIP-TextHip/THIP-Server#112
File: src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/FeedQueryRepositoryImpl.java:272-272
Timestamp: 2025-07-30T10:44:34.115Z
Learning: seongjunnoh는 피드 커서 페이지네이션에서 LocalDateTime 단일 커서 방식을 선호하며, 복합 키 기반 커서보다 구현 단순성과 성능을 우선시한다.
| // Resolver에서 세션에 저장한 origin을 복원 | ||
| String webRedirectDomain = null; | ||
| if (request.getSession(false) != null) { | ||
| webRedirectDomain = (String) request.getSession(false).getAttribute(REDIRECT_SESSION_KEY.getValue()); | ||
| request.getSession(false).removeAttribute(REDIRECT_SESSION_KEY.getValue()); // 사용했으면 제거(일회성) | ||
| } | ||
|
|
||
| // 허용 오리진 검증 및 폴백 | ||
| if (!webDomainProperties.isAllowed(Objects.toString(webRedirectDomain, ""))) { | ||
| List<String> origins = webDomainProperties.getWebDomainUrls(); | ||
| if (origins == null || origins.isEmpty()) { | ||
| throw new AuthException(ErrorCode.WEB_DOMAIN_ORIGIN_EMPTY); | ||
| } | ||
| webRedirectDomain = origins.get(0); | ||
| } | ||
|
|
There was a problem hiding this comment.
리다이렉트 도메인 검증 강화 및 Origin 정규화
현재 문자열 기반 검증/결합은 오리진이 아닌 전체 URL이 들어오거나 경계 케이스(중복 슬래시, 포트/IPv6, 국제화 도메인)에서 취약할 수 있습니다. URI 파싱으로 오리진을 정규화한 뒤 허용 목록과 비교하도록 개선하세요.
+import org.springframework.web.util.UriComponentsBuilder;
+import java.net.URI;
...
- // 허용 오리진 검증 및 폴백
- if (!webDomainProperties.isAllowed(Objects.toString(webRedirectDomain, ""))) {
+ // 허용 오리진 검증 및 폴백
+ String candidate = Objects.toString(webRedirectDomain, "");
+ if (!candidate.isEmpty()) {
+ try {
+ URI parsed = URI.create(candidate).normalize();
+ candidate = parsed.getScheme() + "://" + parsed.getAuthority();
+ } catch (IllegalArgumentException e) {
+ candidate = "";
+ }
+ }
+ if (!webDomainProperties.isAllowed(candidate)) {
List<String> origins = webDomainProperties.getWebDomainUrls();
if (origins == null || origins.isEmpty()) {
throw new AuthException(ErrorCode.WEB_DOMAIN_ORIGIN_EMPTY);
}
- webRedirectDomain = origins.get(0);
+ candidate = origins.get(0);
}
+ // 최종 오리진
+ webRedirectDomain = candidate;📝 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.
| // Resolver에서 세션에 저장한 origin을 복원 | |
| String webRedirectDomain = null; | |
| if (request.getSession(false) != null) { | |
| webRedirectDomain = (String) request.getSession(false).getAttribute(REDIRECT_SESSION_KEY.getValue()); | |
| request.getSession(false).removeAttribute(REDIRECT_SESSION_KEY.getValue()); // 사용했으면 제거(일회성) | |
| } | |
| // 허용 오리진 검증 및 폴백 | |
| if (!webDomainProperties.isAllowed(Objects.toString(webRedirectDomain, ""))) { | |
| List<String> origins = webDomainProperties.getWebDomainUrls(); | |
| if (origins == null || origins.isEmpty()) { | |
| throw new AuthException(ErrorCode.WEB_DOMAIN_ORIGIN_EMPTY); | |
| } | |
| webRedirectDomain = origins.get(0); | |
| } | |
| // Resolver에서 세션에 저장한 origin을 복원 | |
| String webRedirectDomain = null; | |
| if (request.getSession(false) != null) { | |
| webRedirectDomain = (String) request.getSession(false).getAttribute(REDIRECT_SESSION_KEY.getValue()); | |
| request.getSession(false).removeAttribute(REDIRECT_SESSION_KEY.getValue()); // 사용했으면 제거(일회성) | |
| } | |
| // 허용 오리진 검증 및 폴백 | |
| String candidate = Objects.toString(webRedirectDomain, ""); | |
| if (!candidate.isEmpty()) { | |
| try { | |
| URI parsed = URI.create(candidate).normalize(); | |
| candidate = parsed.getScheme() + "://" + parsed.getAuthority(); | |
| } catch (IllegalArgumentException e) { | |
| candidate = ""; | |
| } | |
| } | |
| if (!webDomainProperties.isAllowed(candidate)) { | |
| List<String> origins = webDomainProperties.getWebDomainUrls(); | |
| if (origins == null || origins.isEmpty()) { | |
| throw new AuthException(ErrorCode.WEB_DOMAIN_ORIGIN_EMPTY); | |
| } | |
| candidate = origins.get(0); | |
| } | |
| // 최종 오리진 | |
| webRedirectDomain = candidate; |
🤖 Prompt for AI Agents
In src/main/java/konkuk/thip/common/security/oauth2/CustomSuccessHandler.java
around lines 43 to 58, the current plain-string redirect-domain check is
vulnerable to full-URL inputs and edge cases; parse and normalize the stored
redirect value as a URI and extract its origin (scheme + host and explicit port
if non-default) before comparing against the allowed origins. Specifically:
retrieve and remove the session attribute as now, then if non-null attempt to
parse it with java.net.URI (or java.net.URL), build a normalized origin string
(lowercase host, include explicit port when not default for scheme), and only
use that normalized origin for isAllowed checks; also normalize the configured
allowed origins the same way (or pre-normalize when loading properties), and
keep the fallback: if allowed list is empty throw AuthException; finally if
parsing fails, treat as invalid and fall back to the first allowed origin or
throw per current policy.
| /** | ||
| * 저장된 토큰을 1회용으로 소비 후 삭제한다. | ||
| * 존재하지 않으면 null 반환. | ||
| */ | ||
| Entry consume(String key); | ||
|
|
||
| record Entry(TokenType type, String token) { | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
소비(consume) 원자성 명시 필요
보안상 “1회용”은 get+delete가 원자적이어야 합니다. 인터페이스 주석에 원자성/스레드 안전성 계약을 명문화해 구현체 일관성을 보장하세요.
- /**
- * 저장된 토큰을 1회용으로 소비 후 삭제한다.
- * 존재하지 않으면 null 반환.
- */
+ /**
+ * 저장된 토큰을 1회용으로 원자적으로(atomic) 소비 후 삭제한다.
+ * - 구현체는 getAndDelete(예: Redis GETDEL) 등으로 동시성 환경에서도 중복 소비가 불가능해야 한다.
+ * - key가 존재하지 않으면 null을 반환한다.
+ * - put의 ttl은 0보다 커야 한다.
+ */
Entry consume(String key);📝 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.
| /** | |
| * 저장된 토큰을 1회용으로 소비 후 삭제한다. | |
| * 존재하지 않으면 null 반환. | |
| */ | |
| Entry consume(String key); | |
| record Entry(TokenType type, String token) { | |
| } | |
| } | |
| /** | |
| * 저장된 토큰을 1회용으로 원자적으로(atomic) 소비 후 삭제한다. | |
| * - 구현체는 getAndDelete(예: Redis GETDEL) 등으로 동시성 환경에서도 중복 소비가 불가능해야 한다. | |
| * - key가 존재하지 않으면 null을 반환한다. | |
| * - put의 ttl은 0보다 커야 한다. | |
| */ | |
| Entry consume(String key); | |
| record Entry(TokenType type, String token) { | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/konkuk/thip/common/security/oauth2/tokenstorage/LoginTokenStorage.java
around lines 11 to 19, the Javadoc for consume(String key) must declare
atomicity and thread-safety: state that consume performs an atomic
get-and-delete (only one caller will receive the token for a given key), that
concurrent calls must not return the same token, and that null is returned when
no token exists; require implementations to guarantee this (e.g., via
synchronization, database transaction, or atomic store operations) and mention
expected behavior on failure/exception. Ensure the contract is explicit so all
implementations preserve one-time-use semantics.
| private final RedisTemplate<String, Object> redisTemplate; | ||
|
|
||
| private static final String PREFIX = "auth:login-token:"; | ||
|
|
||
| @Override | ||
| public void put(String key, TokenType type, String token, Duration ttl) { | ||
| String redisKey = toRedisKey(key); | ||
| Entry entry = new Entry(type, token); | ||
|
|
||
| redisTemplate.opsForValue().set(redisKey, entry, ttl); | ||
| } |
There was a problem hiding this comment.
직렬화 호환성 이슈: Entry 객체를 값으로 저장 시 역직렬화 실패 가능
RedisTemplate<String, Object>의 기본 시리얼라이저 설정에 따라 Entry 레코드가 Map/바이트로 역직렬화되어 instanceof Entry가 실패할 수 있습니다. 또한 JDK 직렬화를 쓰면 Entry가 Serializable이어야 합니다. 구현 간 일관성을 위해 문자열 값으로 저장하는 방식을 권장합니다.
- public void put(String key, TokenType type, String token, Duration ttl) {
- String redisKey = toRedisKey(key);
- Entry entry = new Entry(type, token);
-
- redisTemplate.opsForValue().set(redisKey, entry, ttl);
- }
+ public void put(String key, TokenType type, String token, Duration ttl) {
+ String redisKey = toRedisKey(key);
+ String value = type.name() + ":" + token; // 직렬화 독립
+ redisTemplate.opsForValue().set(redisKey, value, ttl);
+ }📝 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.
| private final RedisTemplate<String, Object> redisTemplate; | |
| private static final String PREFIX = "auth:login-token:"; | |
| @Override | |
| public void put(String key, TokenType type, String token, Duration ttl) { | |
| String redisKey = toRedisKey(key); | |
| Entry entry = new Entry(type, token); | |
| redisTemplate.opsForValue().set(redisKey, entry, ttl); | |
| } | |
| private final RedisTemplate<String, Object> redisTemplate; | |
| private static final String PREFIX = "auth:login-token:"; | |
| @Override | |
| public void put(String key, TokenType type, String token, Duration ttl) { | |
| String redisKey = toRedisKey(key); | |
| String value = type.name() + ":" + token; // 직렬화 독립 | |
| redisTemplate.opsForValue().set(redisKey, value, ttl); | |
| } |
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java
Outdated
Show resolved
Hide resolved
| public boolean isAllowed(String target) { | ||
| try { | ||
| URI uri = URI.create(target); | ||
| String origin = uri.getScheme() + "://" + uri.getHost() + (uri.getPort() > 0 ? ":" + uri.getPort() : ""); | ||
| return webDomainUrls.stream().anyMatch(o -> o.equalsIgnoreCase(origin)); | ||
| } catch (Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
오리진 정규화 부족: 기본 포트/IPv6/IDN 처리로 인해 합법 도메인이 거부될 수 있음
현재 origin 생성이 단순 문자열 결합이라 다음 이슈가 있습니다:
- http:80 / https:443 명시 시 불일치
- IPv6 오리진 괄호 미포함 가능성
- 국제화 도메인(IDN) 대소문/표기 불일치
- 스킴 화이트리스트 미적용
정규화 함수를 도입해 비교하세요.
- public boolean isAllowed(String target) {
- try {
- URI uri = URI.create(target);
- String origin = uri.getScheme() + "://" + uri.getHost() + (uri.getPort() > 0 ? ":" + uri.getPort() : "");
- return webDomainUrls.stream().anyMatch(o -> o.equalsIgnoreCase(origin));
- } catch (Exception e) {
- return false;
- }
- }
+ public boolean isAllowed(String target) {
+ try {
+ if (target == null || target.isBlank()) return false;
+ URI uri = URI.create(target);
+ String normalized = normalizeOrigin(uri);
+ if (normalized == null) return false;
+ for (String allowed : webDomainUrls) {
+ String a = normalizeOrigin(URI.create(allowed));
+ if (a != null && a.equalsIgnoreCase(normalized)) return true;
+ }
+ return false;
+ } catch (Exception e) {
+ return false;
+ }
+ }아래 헬퍼를 같은 클래스에 추가하세요:
// helper
private String normalizeOrigin(URI uri) {
if (uri == null) return null;
String scheme = uri.getScheme();
if (!"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) return null;
String host = uri.getHost();
if (host == null || host.isBlank()) return null;
// IDN 정규화
try {
host = java.net.IDN.toASCII(host);
} catch (Exception ignore) { return null; }
int port = uri.getPort();
int defaultPort = "https".equalsIgnoreCase(scheme) ? 443 : 80;
int effectivePort = (port <= 0 || port == defaultPort) ? -1 : port;
try {
// URI를 통해 IPv6 괄호 포함 오리진 생성
URI origin = new URI(scheme, null, host, effectivePort, null, null, null);
return origin.toString();
} catch (Exception e) {
return null;
}
}🤖 Prompt for AI Agents
In src/main/java/konkuk/thip/config/properties/WebDomainProperties.java around
lines 20 to 28, the current origin construction is fragile (doesn't handle
default ports, IPv6 bracket syntax, IDN, or scheme white-listing); add the
provided normalizeOrigin(URI) helper to the same class and update
isAllowed(String target) to: parse the target to a URI, call
normalizeOrigin(uri), return false if null, and compare the normalized origin
against webDomainUrls using case-insensitive equality; ensure normalizeOrigin
enforces only http/https, converts host via IDN.toASCII, treats default ports
(80/443) as absent, builds the origin via new URI(...) to correctly include IPv6
brackets, and return null on any invalid input so isAllowed returns false.
| devServerUrl | ||
| )); | ||
|
|
||
| List<String> allowedOrigins = webDomainProperties.getWebDomainUrls(); |
| @@ -1,4 +1,4 @@ | |||
| package konkuk.thip.common.security.argument_resolver; | |||
| package konkuk.thip.common.security.resolver; | |||
| import static konkuk.thip.common.security.constant.AuthParameters.REDIRECT_SESSION_KEY; | ||
| import static konkuk.thip.common.security.constant.AuthParameters.REDIRECT_URL_KEY; | ||
|
|
||
| @Slf4j |
| String webRedirectDomain = null; | ||
| if (request.getSession(false) != null) { | ||
| webRedirectDomain = (String) request.getSession(false).getAttribute(REDIRECT_SESSION_KEY.getValue()); | ||
| request.getSession(false).removeAttribute(REDIRECT_SESSION_KEY.getValue()); // 사용했으면 제거(일회성) |
| import java.util.List; | ||
|
|
||
| @Getter | ||
| @Setter |
There was a problem hiding this comment.
안 쓰는 것 같네요 수정하겠슴다!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/konkuk/thip/config/properties/WebDomainProperties.java (1)
18-26: 오리진 정규화/화이트리스트 검증 보강 필요 — 기본 포트·IPv6·IDN·스킴 처리현재 문자열 결합으로 만든 오리진 비교는 기본 포트(80/443), IPv6 괄호, IDN, 스킴 화이트리스트를 다루지 못해 합법 URL이 거부되거나 환경별로 불안정할 수 있습니다. 정규화 후 비교하도록 수정하세요.
아래 패치를 적용하면 해결됩니다.
@@ - public boolean isAllowed(String target) { - try { - URI uri = URI.create(target); - String origin = uri.getScheme() + "://" + uri.getHost() + (uri.getPort() > 0 ? ":" + uri.getPort() : ""); - return webDomainUrls.stream().anyMatch(o -> o.equalsIgnoreCase(origin)); - } catch (Exception e) { - return false; - } - } + public boolean isAllowed(String target) { + try { + if (target == null || target.isBlank()) return false; + URI uri = URI.create(target); + String normalized = normalizeOrigin(uri); + if (normalized == null) return false; + for (String allowed : webDomainUrls) { + String a = normalizeOrigin(URI.create(allowed)); + if (a != null && a.equalsIgnoreCase(normalized)) return true; + } + return false; + } catch (Exception e) { + return false; + } + } + + // helper + private String normalizeOrigin(URI uri) { + if (uri == null) return null; + String scheme = uri.getScheme(); + if (!"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) return null; + + String host = uri.getHost(); + if (host == null || host.isBlank()) return null; + + // IDN 정규화 + try { + host = java.net.IDN.toASCII(host); + } catch (Exception ignore) { return null; } + + int port = uri.getPort(); + int defaultPort = "https".equalsIgnoreCase(scheme) ? 443 : 80; + int effectivePort = (port <= 0 || port == defaultPort) ? -1 : port; + + try { + // IPv6 포함 안전한 오리진 생성 + URI origin = new URI(scheme, null, host, effectivePort, null, null, null); + return origin.toString(); + } catch (Exception e) { + return null; + } + }src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (1)
46-50: redirect_url 미제공/불허 시, 이전 세션 값 제거 + registrationId 단위로 스코프파라미터가 없거나 불허일 때 기존 세션 값이 남아 예기치 않은 리다이렉트가 발생할 수 있고, 다중 프로바이더 동시 로그인 시 키 충돌 위험이 있습니다. registrationId별로 저장하고, 미제공/불허 시 제거하세요.
@@ - String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); - Map<String, Object> additional = new HashMap<>(base.getAdditionalParameters()); - if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { - request.getSession(true).setAttribute(REDIRECT_SESSION_KEY.getValue(), redirectUrl); - } + String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); + String sessionKey = REDIRECT_SESSION_KEY.getValue() + ":" + base.getClientRegistrationId(); + if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { + // 저장 필요 시에만 세션 생성 + request.getSession(true).setAttribute(sessionKey, redirectUrl); + } else { + // 존재할 때만 제거(세션 생성 X) + var existing = request.getSession(false); + if (existing != null) { + existing.removeAttribute(sessionKey); + } + } @@ - return OAuth2AuthorizationRequest.from(base) - .additionalParameters(additional) - .build(); + return base;성공 핸들러가 동일한 키(REDIRECT_SESSION_KEY + ":" + registrationId)를 읽도록 되어있는지 확인 바랍니다. 아래 스크립트로 사용처를 점검하세요.
#!/bin/bash # REDIRECT_SESSION_KEY 사용처와 SuccessHandler 접근 키 점검 rg -n -C2 'REDIRECT_SESSION_KEY' rg -n -C2 'getClientRegistrationId\(' rg -n -C3 -P '(CustomSuccessHandler|SuccessHandler).*REDIRECT_SESSION_KEY'
🧹 Nitpick comments (2)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (2)
52-55: 불필요한 객체 생성 제거additional 파라미터를 수정하지 않으므로 복사/리빌드 불필요합니다. 위 패치처럼 base를 그대로 반환하면 할당을 줄일 수 있습니다.
18-19: @requiredargsconstructor 제거 권고SecurityConfig.java에서 new CustomAuthorizationRequestResolver(...)로 직접 인스턴스화(src/main/java/konkuk/thip/config/SecurityConfig.java:57)하고 있어 Lombok의 @requiredargsconstructor는 불필요하거나 혼선을 초래합니다. @requiredargsconstructor 삭제를 권장합니다 — 리포지토리 검색 결과 다른 빈 등록/테스트 참조 없음.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java(1 hunks)src/main/java/konkuk/thip/config/properties/WebDomainProperties.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
확인했습니다!!
securityConfig 에서 oauth 로그인 요청시에 authorizationRequestResolver 을 통해서 세션에 로그인 요청의 redirect url 값을 저장하는 방식으로 구현하셨군요!!
그리고 이건 현재 pr과는 다른 얘기긴 한데,
현재 코드 구현에서 저희 서버로 api 요청이 들어오는 경우
- filter 내부에서 분기처리를 통해 저희가 정의한 AuthException 을 throw (이때 exception이 발생할 경우 SecurityContext에 Authentication 은 세팅되지 못함)
- filter 내부에서 발생한 모든 exception을 catch 후 request에 set
- securityConfig 체이닝의
.authorizeHttpRequests(auth -> auth
.requestMatchers(SecurityWhitelist.patterns()).permitAll()
.anyRequest().authenticated()
)에 의해서 화이트리스트에 포함되지 않으면서 authenticated 되지 않은 경우(= SecurityContext에 Authentication이 존재하지 않는 경우) 에는
.exceptionHandling(handler -> handler.authenticationEntryPoint(jwtAuthenticationEntryPoint))에 의해서 JwtAuthenticationEntryPoint 가 동작함
4. JwtAuthenticationEntryPoint 내부에서는
@Override
public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) throws IOException, ServletException {
Exception e = (Exception) request.getAttribute("exception");
if(e == null){
e = authException;
}
resolver.resolveException(request, response, null, e);
}를 통해 request 에서 get 한 exception을 HandlerExceptionResolver 에게 맡김
5. HandlerExceptionResolver 는 우리가 정의한 GlobalExceptionHandler 을 호출함으로써 예외처리 진행
이런 플로우로 동작을 합니다.
그런데 이때 1번 과정에서 저희가 정의한 분기처리에 해당하는 경우가 아니라 NullPointerException, ArrayIndexOutOfBoundsException, SignatureException 등 다른 exception이 발생할 경우,
5번 과정에서 GlobalExceptionHandler 로 전달되는 exception은 AuthException이 아니라 다른 exception 이고, 따라서 filter 내부에서 exception이 발생했지만 401 error 가 아니라 500 error 가 나오게 됩니다.
따라서 filter 에서 request에 set 하는 exception을 모두 AuthException 으로 세팅하는 것처럼 필터링 과정에서 발생하는 예외처리에 대한 다른 방안이 필요해보이는데 어떻게 생각하시나요?
(모든 발생 가능한 상황을 분기처리하면 좋겠지만, 저희 예상에서 벗어나는 상황이 발생할 수 있으니 필터링 과정에서 발생하는 모든 예외에 대해서 AuthException으로 강제 매핑하는 방법을 예시로 들었슴다)
@buzz0331 @hd0rable
| .authorizationEndpoint(authorizationEndpointConfig -> authorizationEndpointConfig | ||
| .authorizationRequestResolver(resolver) | ||
| .authorizationRequestRepository(authReqRepo) | ||
| ) |
There was a problem hiding this comment.
OAuth2 로그인 시작시에 resolver 가 로그인 요청을 가로채서 redirect url을 세션에 저장하도록 설정하셨군요! 확인했습니다
| @Profile({"!test"}) | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class RedisLoginTokenStorage implements LoginTokenStorage { |
| @Profile("test") | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class MemoryLoginTokenStorage implements LoginTokenStorage{ |
| @Value("${server.web-redirect-url}") | ||
| private String webRedirectUrl; | ||
| private final WebDomainProperties webDomainProperties; |
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java
Outdated
Show resolved
Hide resolved
네넵 예외처리가 의도된대로 처리되지못하는 부분은 개선이필요해보이네욧.. 이 방식이면 인증 플로우에서 발생하는 예외는 일관되게 401로 처리되고, 실제 서버 내부 장애 등은 여전히 500으로 분리해서 처리할 수 있으니 관리가 더 쉬울 것 같네요 👍🏻👍🏻 |
인증 과정에서 저희가 예상하지 못한 예외까지 핸들링하는 접근은 좋은 방향인 것 같습니다. 개인적으로는 JwtAuthenticationEntryPoint에서 request에 예외가 전달되지 않은 경우, 기본적으로 AuthException으로 감싸서 전달하는 방식이 적절할 것 같은데 어떻게 생각하시나요? package konkuk.thip.common.security.filter;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import konkuk.thip.common.exception.AuthException;
import konkuk.thip.common.exception.code.ErrorCode;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.web.AuthenticationEntryPoint;
import org.springframework.stereotype.Component;
import org.springframework.web.servlet.HandlerExceptionResolver;
import java.io.IOException;
@Component
public class JwtAuthenticationEntryPoint implements AuthenticationEntryPoint {
private final HandlerExceptionResolver resolver;
public JwtAuthenticationEntryPoint(@Qualifier("handlerExceptionResolver")
HandlerExceptionResolver resolver) {
this.resolver = resolver;
}
@Override
public void commence(HttpServletRequest request,
HttpServletResponse response,
AuthenticationException authException) throws IOException, ServletException {
// 필터에서 set한 예외 우선
Exception original = (Exception) request.getAttribute("exception");
if (original == null) {
original = authException;
}
Exception mapped = wrapAsAuthException(original);
resolver.resolveException(request, response, null, mapped);
}
// 모든 예외를 AuthException(401)으로 감싸는 메서드
private Exception wrapAsAuthException(Exception e) {
if (e instanceof AuthException) {
return e;
}
return new AuthException(ErrorCode.AUTH_INTERNAL_SERVER_ERROR, e);
}
}다만 이 방식은 어디까지나 임시방편으로, 예상치 못한 예외를 모두 AuthException(401)으로 감싸주는 것에 불과한 것 같긴합니다. 근본적인 문제 해결을 위해서는 실제 발생 원인을 찾아서, 필요하다면 필터 단계에서 분기 처리를 명확히 해주는 것이 바람직하다고 생각합니다. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (2)
46-53: [P2] 세션 키를 registrationId별로 분리 + 불필요한 세션 생성 방지동시 탭/복수 프로바이더 로그인 시 덮어쓰기/경합을 막기 위해 세션 키를
registrationId별로 분리하고, 제거만 필요할 때는 세션을 새로 만들지 않도록 개선이 필요합니다.다음 diff 적용 제안:
- String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); - Map<String, Object> additional = new HashMap<>(base.getAdditionalParameters()); - var session = request.getSession(true); - if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { - session.setAttribute(REDIRECT_SESSION_KEY.getValue(), redirectUrl); - } else { - session.removeAttribute(REDIRECT_SESSION_KEY.getValue()); - } + String redirectUrl = request.getParameter(REDIRECT_URL_KEY.getValue()); + final String registrationId = base.getClientRegistrationId(); + final String attrKey = REDIRECT_SESSION_KEY.getValue() + ":" + registrationId; + + var session = request.getSession(false); + if (StringUtils.hasText(redirectUrl) && webDomainProperties.isAllowed(redirectUrl)) { + if (session == null) session = request.getSession(true); + session.setAttribute(attrKey, redirectUrl); + } else if (session != null) { + session.removeAttribute(attrKey); + }해당 변경을 채택할 경우, LoginSuccessHandler 등에서 조회/삭제하는 세션 키도 동일 규칙으로 맞춰주세요.
47-47: [P3] 불필요한 additionalParameters 복사/재빌드 제거현재 추가 파라미터를 수정하지 않으므로 Map 복사와
from(base)재빌드가 불필요합니다. 단순히base를 반환해도 동일 동작입니다.- Map<String, Object> additional = new HashMap<>(base.getAdditionalParameters()); ... - return OAuth2AuthorizationRequest.from(base) - .additionalParameters(additional) - .build(); + return base;Also applies to: 54-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/konkuk/thip/common/security/resolver/CustomAuthorizationRequestResolver.java (3)
46-53: [P1] 리다이렉트 URL 검증 보강 필요 (오픈 리다이렉트/도메인 스푸핑 대응)
webDomainProperties.isAllowed(redirectUrl)가 “엄격한 오리진 기반 화이트리스트”를 보장하는지 확인이 필요합니다. 아래를 반드시 차단/정규화하세요:
- 스킴 강제: https(필요 시 http는 환경 한정)만 허용, 스킴 누락/비정상 스킴 거부
- 호스트 정규화: IDN(Punycode) 변환, 기본 포트(80/443) 처리, 대소문자/트레일링 점/서브도메인 스푸핑(
allowed.com.evil) 방지- 유저정보(
user@host), 프래그먼트(#...), 빈 경로, 파일 스킴 등 비허용 구성 거부- IP 허용 시 사설/루프백/IPv6 리터럴 정책 분리
- 단순 prefix 비교가 아닌 “정확한 host(+port) 일치” 또는 명시 서브도메인 매칭
테스트 케이스 예시:
- 허용: https://app.example.com, https://app.example.com:443
- 거부: http://app.example.com (http 금지 시), https://app.example.com.evil, https://[email protected], https://app.example.com#frag, //app.example.com
49-53: [P3] redirect_url 미제공 시 세션 정리 적용 확인 — 좋습니다파라미터 부재/비허용 시 세션 키를 제거하도록 반영되어, 이전 세션 값 잔존 이슈가 해소됩니다. 👍
43-45: [P3] 널 가드 OK
base == null조기 반환으로 불필요 처리 방지되어 적절합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/konkuk/thip/user/adapter/in/web/UserVerifyNicknameControllerTest.java (3)
150-151: DisplayName 한글 띄어쓰기 보정 제안 (가독성)"포함하지않고" → "포함하지 않고"로 띄어쓰기 보정하면 더 자연스럽습니다. 우선순위: P3
- @DisplayName("회원 탈퇴한(= soft delete 처리된) 유저의 닉네임 정보를 포함하지않고 중복 검증을 수행한다.") + @DisplayName("회원 탈퇴한(= soft delete 처리된) 유저의 닉네임 정보를 포함하지 않고 중복 검증을 수행한다.")
165-173: JSON 파싱 없이 jsonPath로 값까지 단정하도록 단순화
.exists()+ 수동 JSON 파싱 대신.value(true)로 바로 단정하면 테스트가 간결하고 의도가 명확해집니다. 우선순위: P3- result.andExpect(status().isOk()) - .andExpect(jsonPath("$.data.isVerified").exists()); - - String json = result.andReturn().getResponse().getContentAsString(); - JsonNode jsonNode = objectMapper.readTree(json); - boolean isVerified = jsonNode.path("data").path("isVerified").asBoolean(); - - assertThat(isVerified).isTrue(); + result.andExpect(status().isOk()) + .andExpect(jsonPath("$.data.isVerified").value(true));
154-157: JDBC로 직접 업데이트 후 1차 캐시 정리 필요 (잠재적 플래키성 완화)동일 트랜잭션 내 JPA 1차 캐시에
deleteUser가 남아있을 수 있어 상태 필드가 갱신되지 않은 엔티티가 재사용될 여지가 있습니다.entityManager.clear()로 캐시를 비워 DB 상태를 확정적으로 반영하도록 권장합니다. 우선순위: P3jdbcTemplate.update( "UPDATE users SET status = ? WHERE user_id = ?", StatusType.INACTIVE.name(), deleteUser.getUserId()); + entityManager.clear(); // JDBC 갱신 후 1차 캐시 비우기클래스 상단에 EntityManager 주입(프로젝트 버전에 맞는 패키지 선택)도 추가해주세요:
// Spring Boot 3+ (Jakarta) 사용 시 import jakarta.persistence.EntityManager; import jakarta.persistence.PersistenceContext; // Spring Boot 2.x (javax) 사용 시 // import javax.persistence.EntityManager; // import javax.persistence.PersistenceContext; @PersistenceContext private EntityManager entityManager;해당 중복검증 쿼리가 soft delete(INACTIVE) 레코드를 명시적으로 제외하는 조건을 포함하는지도 한번 확인 부탁드립니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/test/java/konkuk/thip/user/adapter/in/web/UserVerifyNicknameControllerTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| boolean isVerified = jsonNode.path("data").path("isVerified").asBoolean(); | ||
|
|
||
| assertThat(isVerified).isFalse(); // 닉네임 중복으로 인해 isVerified == false | ||
| assertThat(isVerified).isTrue(); |
넵 현준님 코멘트처럼 JwtAuthenticationEntryPoint 의 코드를 수정하는 것도 좋아보입니다!! 그러면 코멘트 주신 것처럼 JwtAuthenticationEntryPoint 를 수정해주시나요?? |
|
@seongjunnoh 넵 수정완료했습니다. 로그는 어차피 handler에서 모두 출력해주고 있기 때문에 굳이 resolver에서는 출력해줄 필요 없을 것 같습니다! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/konkuk/thip/common/security/filter/JwtAuthenticationEntryPoint.java (1)
42-47: 예외 래핑 로직의 로깅 추가를 고려해보세요.AuthException이 아닌 예외를 AUTH_INTERNAL_SERVER_ERROR로 래핑할 때, 원본 예외 정보를 로그에 남기는 것을 고려해보세요. PR 댓글에서 언급된 대로 이후 500 에러 원인 분석에 도움이 될 것입니다.
// 모든 예외를 AuthException(401)으로 감싸는 메서드 private Exception wrapAsAuthException(Exception e) { if (e instanceof AuthException) { return e; } + // TODO: 원본 예외 원인 분석을 위한 로깅 추가 검토 return new AuthException(ErrorCode.AUTH_INTERNAL_SERVER_ERROR, e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/konkuk/thip/common/exception/code/ErrorCode.java(1 hunks)src/main/java/konkuk/thip/common/security/filter/JwtAuthenticationEntryPoint.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/konkuk/thip/common/exception/code/ErrorCode.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/main/java/konkuk/thip/common/security/filter/JwtAuthenticationEntryPoint.java (4)
6-7: 새로운 import 추가가 적절합니다.AuthException과 ErrorCode import가 새로운 예외 래핑 로직에 필요하며, 올바르게 추가되었습니다.
20-22: 파라미터 포맷팅이 개선되었습니다.생성자 파라미터의 포맷팅이 더 읽기 쉽게 변경되었습니다.
26-39: 예외 처리 로직이 개선되었습니다.필터에서 설정한 예외를 우선적으로 처리하고, AuthException으로 래핑하는 로직이 올바르게 구현되었습니다. 이는 PR 목표에서 언급된 필터 단계의 예외들을 일관되게 401로 처리하는 요구사항을 만족합니다.
46-46: 확인: ErrorCode.AUTH_INTERNAL_SERVER_ERROR 정의됨스크립트 출력에서 ErrorCode에 AUTH_INTERNAL_SERVER_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, 40108, "인증 처리 중 서버 오류가 발생했습니다.")로 정의된 것을 확인했습니다.
| // 모든 예외를 AuthException(401)으로 감싸는 메서드 | ||
| private Exception wrapAsAuthException(Exception e) { | ||
| if (e instanceof AuthException) { | ||
| return e; | ||
| } | ||
| resolver.resolveException(request, response, null, e); | ||
| return new AuthException(ErrorCode.AUTH_INTERNAL_SERVER_ERROR, e); | ||
| } |
#️⃣ 연관된 이슈
📝 작업 내용
기존 소셜 로그인에서는 웹 도메인을 개발, 운영 서버를 각각
localhost,웹 도메인으로 리다이렉트 되도록 도메인을 yml 파일에 정적으로 선언해두고 사용했습니다.하지만, 프론트 개발자 분들께서 로컬 모바일 웹 환경에서 QA를 진행할때 리다이렉트를 현재 사용중인 와이파이의 ip 주소로 받아야하는 이슈가 발생해서 어쩔 수 없이 동적으로 리다이렉트를 받아야 하는 상황이 생겼습니다.
쿼리 파라미터로 리다이렉트 도메인을 받더라도 카카오에 소셜 로그인을 요청을 보내고 다시 서버로 리다이렉트 받는 동작을 수행하고 나면 쿼리 파라미터가 사라져 있기 때문에 이를 잠깐 보관해야 했습니다.
따라서, 저는 registrationId 기반으로 소셜 로그인 세션동안 잠깐 쿼리 파라미터로 전달된 redirect_url을 저장하고자 ClientRegistrationRepository를 도입하였습니다.
동작 흐름
추가적으로 기존 메모리 기반 LoginTokenStorage를 메모리 기반이 아닌 Redis 기반으로 수정하였습니다.
(테스트 환경에서는 기존대로 메모리 기반 유지)
📸 스크린샷
쿼리 파라미터로 전달한 도메인으로 리다이렉트
토큰 발급 제대로 되는 것까지 확인완료
💬 리뷰 요구사항
📌 PR 진행 시 이러한 점들을 참고해 주세요
Summary by CodeRabbit
신기능
버그 수정
리팩토링
테스트