[BE] Sisc1 209 [FIX] redirect URL을 환경에 따라 자동 분기#115
Merged
Conversation
…m/SISC-IT/sisc-web into SISC1-209-BE-redirect-url-프론트로-전달
WalkthroughOAuth2/OIDC 인증 흐름에 환경 인식 쿠키 설정과 쿠키 기반 액세스 토큰 복원을 추가하고, CORS 허용 출처 및 노출 헤더를 업데이트하며 일부 포맷팅 정리를 적용합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig
participant OAuth2Login
participant CustomOidcUserService
participant OAuth2SuccessHandler
participant JwtAuthenticationFilter
Client->>SecurityConfig: OAuth2/OIDC 로그인 요청
SecurityConfig->>OAuth2Login: oauth2Login 진입
OAuth2Login->>CustomOidcUserService: OIDC 사용자 로드 요청
CustomOidcUserService-->>OAuth2Login: 사용자 정보 반환
OAuth2Login->>OAuth2SuccessHandler: 인증 성공 처리
OAuth2SuccessHandler->>OAuth2SuccessHandler: env 확인 (prod 여부)
OAuth2SuccessHandler->>OAuth2SuccessHandler: 쿠키 속성 계산(sameSite, secure, domain)
OAuth2SuccessHandler-->>Client: access/refresh 쿠키 설정 + redirectSuccessBase로 리디렉션
Client->>JwtAuthenticationFilter: 보호된 요청 (Authorization 헤더 없을 수 있음)
JwtAuthenticationFilter->>JwtAuthenticationFilter: Authorization 헤더 확인
alt 헤더 토큰 없음
JwtAuthenticationFilter->>JwtAuthenticationFilter: `access` 쿠키에서 토큰 추출
end
JwtAuthenticationFilter-->>Client: 인증/요청 처리 계속
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (3)
backend/src/main/java/org/sejongisc/backend/auth/entity/AuthProvider.java (1)
6-15: 프로바이더 매핑 유틸 잘 정리되었습니다
from(String providerName)로 registrationId →AuthProvider매핑이 일원화되어 이후 유지보수가 쉬워질 것 같습니다.
추가로 크게 급한 건 아니지만, 방어적으로 사용하려면 아래처럼null/공백 입력에 대한 처리도 고려해 볼 수 있습니다.public static AuthProvider from(String providerName) { if (providerName == null || providerName.isBlank()) { throw new IllegalArgumentException("Provider name must not be null or blank"); } return switch (providerName.toLowerCase()) { case "google" -> GOOGLE; case "kakao" -> KAKAO; case "github" -> GITHUB; default -> throw new IllegalArgumentException("Unsupported provider: " + providerName); }; }backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java (1)
24-93: OIDC(Google)용 사용자 생성 및 attributes 주입 로직은 전체 플로우와 잘 맞습니다
- OIDC 클레임(
sub,name)에서providerUid/name을 추출하고,UserOauthAccount기준으로 User를 조회하거나 새로 생성한 뒤,provider,providerUid,name,userId를 attributes에 넣어주는 구조가 기존CustomOAuth2UserService와 일관되어 SuccessHandler와도 잘 맞습니다.약한 제안 몇 가지입니다.
- provider 문자열
"google"도 enum을 재사용하면 오타 리스크를 줄일 수 있습니다.String provider = AuthProvider.GOOGLE.name().toLowerCase();
- 현재는 Google만 지원하니 문제 없지만, 추후 다른 OIDC 공급자를 추가할 계획이라면
- 이 클래스 이름/구조를 공급자별로 나누거나,
AuthProvider.from(req.getClientRegistration().getRegistrationId())기반으로 공통 처리할 여지가 있습니다.필수 수정 사항은 아니고, 향후 확장 시 참고용으로만 봐주시면 될 것 같습니다.
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
104-131: 쿠키 domain 설정 및 환경별 리다이렉트 구성 정리 제안boolean isProd = Arrays.asList(env.getActiveProfiles()).contains("prod"); String sameSite = isProd ? "None" : "Lax"; boolean secure = isProd; String domain = isProd ? "sisc-web.duckdns.org" : "localhost";여기서
domain변수를 계산하지만, 아래ResponseCookie생성 시.domain(domain)을 지정하지 않아 실제 쿠키의 domain은 요청 Host에만 의존하게 됩니다. 의도했던 동작이 아래 둘 중 어느 쪽인지 한 번 정리해 보는 게 좋겠습니다.
요청 Host 기준으로만 설정하고 싶다면
→domain변수는 사용되지 않으므로 제거하는 편이 코드 가독성에 더 낫습니다.명시적으로 도메인을 고정하고 싶다면
→ 아래와 같이.domain(domain)을 추가해 의도가 코드에 드러나도록 하는 것이 좋습니다.- ResponseCookie accessCookie = ResponseCookie.from("access", accessToken) + ResponseCookie accessCookie = ResponseCookie.from("access", accessToken) .httpOnly(true) .secure(secure) .sameSite(sameSite) + .domain(domain) .path("/") .maxAge(60L * 60) .build(); - ResponseCookie refreshCookie = ResponseCookie.from("refresh", refreshToken) + ResponseCookie refreshCookie = ResponseCookie.from("refresh", refreshToken) .httpOnly(true) .secure(secure) .sameSite(sameSite) + .domain(domain) .path("/") .maxAge(60L * 60 * 24 * 14) .build();참고로, 성공 리다이렉트는
redirectSuccessBase프로퍼티로 이미 환경별 분기가 가능해진 상태라, 동일한 패턴으로 실패 리다이렉트 URL도 프로퍼티로 빼 두면 운영/로컬 환경 전환이 더 수월할 것 같습니다 (선택 사항).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/org/sejongisc/backend/auth/entity/AuthProvider.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java(3 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/SecurityConfig.java(4 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/springsecurity/JwtAuthenticationFilter.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java (2)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
Slf4j(35-146)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java (1)
Slf4j(25-121)
🔇 Additional comments (3)
backend/src/main/java/org/sejongisc/backend/common/auth/config/SecurityConfig.java (1)
37-39: OAuth2/OIDC 설정 및 공개/보호 경로 분리는 전체 플로우와 정합
CustomOAuth2UserService+CustomOidcUserService를userInfoEndpoint에 함께 연결한 부분은 Google OIDC / 나머지 OAuth2 제공자를 분리해 처리하기에 적절해 보입니다./login/**,/oauth2/**를permitAll로 열어둔 것은 Jwt 필터의 EXCLUDE 패턴과도 잘 맞습니다./api/user/**에 대해authenticated()를 요구하도록 한 것도, JWT + 쿠키 기반 인증 구조와 잘 정합입니다.- CORS에서
Authorization헤더를exposed로 추가한 것도, (쿠키 외에) 헤더 기반 토큰을 사용하는 경우 프론트에서 읽을 수 있게 해주는 실용적인 설정입니다.이 파일의 변경 사항 자체는 현재 구조에 잘 맞는 방향으로 보이고, 위에서 언급한 principal 타입 처리/쿠키 보안 이슈만 정리되면 전반적인 시큐리티 플로우는 안정적일 것 같습니다.
Also applies to: 60-63, 89-97, 119-120
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java (2)
38-54: LGTM: OAuth2 사용자 로딩 및 로깅 설정이 적절합니다.위임 패턴이 올바르게 구현되었고, 조건부 디버그 로깅도 적절합니다.
107-118: LGTM: 속성 맵 구조와 principal 키 선택이 적절합니다.이메일이 null일 수 있는 경우를 고려하여
userId를 principal 키로 사용하는 것은 안전한 선택입니다. 새로운 속성 맵 구조도 명확하고 잘 정의되어 있습니다.
Comment on lines
+62
to
+68
| case "kakao" -> { | ||
| providerUid = attrs.get("id").toString(); | ||
| Map<String, Object> kakaoAccount = (Map<String, Object>) attrs.get("kakao_account"); | ||
| email = (String) kakaoAccount.get("email"); // null 가능 | ||
| Map<String, Object> profile = (Map<String, Object>) kakaoAccount.get("profile"); | ||
| name = (String) profile.get("nickname"); | ||
| } |
There was a problem hiding this comment.
Kakao 제공자의 중첩된 맵 접근에서 NPE 위험이 있습니다.
kakao_account와 profile이 null일 경우 NullPointerException이 발생할 수 있습니다. 또한 line 63의 attrs.get("id").toString()도 id가 null이면 NPE가 발생합니다.
다음과 같이 null 안전성을 추가하세요:
case "kakao" -> {
- providerUid = attrs.get("id").toString();
+ Object idObj = attrs.get("id");
+ if (idObj == null) {
+ throw new OAuth2AuthenticationException("Kakao id가 없습니다");
+ }
+ providerUid = idObj.toString();
Map<String, Object> kakaoAccount = (Map<String, Object>) attrs.get("kakao_account");
+ if (kakaoAccount == null) {
+ throw new OAuth2AuthenticationException("Kakao account 정보가 없습니다");
+ }
email = (String) kakaoAccount.get("email"); // null 가능
Map<String, Object> profile = (Map<String, Object>) kakaoAccount.get("profile");
+ if (profile == null) {
+ throw new OAuth2AuthenticationException("Kakao profile 정보가 없습니다");
+ }
name = (String) profile.get("nickname");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "kakao" -> { | |
| providerUid = attrs.get("id").toString(); | |
| Map<String, Object> kakaoAccount = (Map<String, Object>) attrs.get("kakao_account"); | |
| email = (String) kakaoAccount.get("email"); // null 가능 | |
| Map<String, Object> profile = (Map<String, Object>) kakaoAccount.get("profile"); | |
| name = (String) profile.get("nickname"); | |
| } | |
| case "kakao" -> { | |
| Object idObj = attrs.get("id"); | |
| if (idObj == null) { | |
| throw new OAuth2AuthenticationException("Kakao id가 없습니다"); | |
| } | |
| providerUid = idObj.toString(); | |
| Map<String, Object> kakaoAccount = (Map<String, Object>) attrs.get("kakao_account"); | |
| if (kakaoAccount == null) { | |
| throw new OAuth2AuthenticationException("Kakao account 정보가 없습니다"); | |
| } | |
| email = (String) kakaoAccount.get("email"); // null 가능 | |
| Map<String, Object> profile = (Map<String, Object>) kakaoAccount.get("profile"); | |
| if (profile == null) { | |
| throw new OAuth2AuthenticationException("Kakao profile 정보가 없습니다"); | |
| } | |
| name = (String) profile.get("nickname"); | |
| } |
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
around lines 62 to 68, the Kakao branch accesses nested maps and id unsafely
which can throw NullPointerException; guard against nulls by first checking
attrs.get("id") for null (use String.valueOf or Objects.toString with a default)
before calling toString, verify kakao_account is non-null and an instance of Map
before casting, check profile is non-null and an instance of Map before reading
nickname, and set sensible defaults (or leave email/name nullable) when fields
are missing; perform safe casts and null checks so no direct dereference of
possibly-null values occurs.
Comment on lines
+69
to
+73
| case "github" -> { | ||
| providerUid = attrs.get("id").toString(); | ||
| email = (String) attrs.get("email"); | ||
| name = (String) attrs.get("login"); // GitHub은 login이 닉네임 | ||
| } |
There was a problem hiding this comment.
GitHub 제공자의 ID 변환에서 NPE 위험이 있습니다.
Line 70에서 attrs.get("id")가 null을 반환하면 .toString() 호출 시 NullPointerException이 발생합니다.
다음과 같이 수정하세요:
case "github" -> {
- providerUid = attrs.get("id").toString();
+ Object idObj = attrs.get("id");
+ if (idObj == null) {
+ throw new OAuth2AuthenticationException("GitHub id가 없습니다");
+ }
+ providerUid = idObj.toString();
email = (String) attrs.get("email");
name = (String) attrs.get("login");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "github" -> { | |
| providerUid = attrs.get("id").toString(); | |
| email = (String) attrs.get("email"); | |
| name = (String) attrs.get("login"); // GitHub은 login이 닉네임 | |
| } | |
| case "github" -> { | |
| Object idObj = attrs.get("id"); | |
| if (idObj == null) { | |
| throw new OAuth2AuthenticationException("GitHub id가 없습니다"); | |
| } | |
| providerUid = idObj.toString(); | |
| email = (String) attrs.get("email"); | |
| name = (String) attrs.get("login"); // GitHub은 login이 닉네임 | |
| } |
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
around lines 69 to 73, the code calls attrs.get("id").toString() which can throw
a NullPointerException if "id" is missing; replace that direct toString call
with a null-safe conversion or explicit null-check (for example use
Objects.toString(attrs.get("id"), null) or check attrs.get("id") != null before
calling toString) and ensure providerUid is set to null or handled appropriately
when the id is absent; also consider applying the same null-safety for email and
name assignments.
| email = (String) attrs.get("email"); | ||
| name = (String) attrs.get("login"); // GitHub은 login이 닉네임 | ||
| } | ||
| default -> throw new RuntimeException("지원하지 않는 provider: " + provider); |
There was a problem hiding this comment.
지원되지 않는 제공자에 대해 더 구체적인 예외를 사용하세요.
RuntimeException 대신 OAuth2AuthenticationException을 사용하여 Spring Security OAuth2 컨텍스트와 일관성을 유지해야 합니다.
다음과 같이 수정하세요:
-default -> throw new RuntimeException("지원하지 않는 provider: " + provider);
+default -> throw new OAuth2AuthenticationException("지원하지 않는 provider: " + provider);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| default -> throw new RuntimeException("지원하지 않는 provider: " + provider); | |
| default -> throw new OAuth2AuthenticationException(new OAuth2Error("unsupported_provider", "지원하지 않는 provider: " + provider)); |
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
around line 74, replace the generic RuntimeException thrown for unsupported
providers with an OAuth2AuthenticationException: create an OAuth2Error (e.g. new
OAuth2Error("invalid_provider", "지원하지 않는 provider: " + provider, null)) and
throw new OAuth2AuthenticationException(that OAuth2Error) (or pass a custom
message as the second arg), and add the necessary import for
org.springframework.security.oauth2.core.* so the exception integrates with
Spring Security's OAuth2 handling.
| final String fProviderUid = providerUid; | ||
| final String fEmail = email; | ||
| final String fName = name; | ||
| final AuthProvider fAuthProvider = AuthProvider.valueOf(provider.toUpperCase()); |
There was a problem hiding this comment.
사용되지 않는 변수를 제거하세요.
fAuthProvider 변수는 선언되었지만 코드 어디에서도 사용되지 않습니다. Lines 85와 97에서는 대신 AuthProvider.from(provider)를 사용하고 있습니다.
다음과 같이 제거하세요:
final String fProviderUid = providerUid;
final String fEmail = email;
final String fName = name;
-final AuthProvider fAuthProvider = AuthProvider.valueOf(provider.toUpperCase());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final AuthProvider fAuthProvider = AuthProvider.valueOf(provider.toUpperCase()); | |
| final String fProviderUid = providerUid; | |
| final String fEmail = email; | |
| final String fName = name; |
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
around line 82, remove the unused local variable declaration "final AuthProvider
fAuthProvider = AuthProvider.valueOf(provider.toUpperCase());" since the code
uses AuthProvider.from(provider) elsewhere (lines ~85 and ~97); simply delete
that line and ensure there are no remaining references to fAuthProvider.
Comment on lines
88
to
92
| User newUser = User.builder() | ||
| .email(fEmail) | ||
| .name(fName) | ||
| .email(email) | ||
| .name(name) | ||
| .role(Role.TEAM_MEMBER) | ||
| .build(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
람다 내부에서 명시적 final 변수를 사용하세요.
Lines 89-90에서 email과 name을 직접 사용하는 대신 lines 80-81에서 선언한 fEmail과 fName을 사용해야 합니다. 코드가 작동하더라도 명시적 final 변수를 사용하는 것이 의도를 더 명확하게 전달하고 람다 캡처 패턴과 일관성을 유지합니다.
다음과 같이 수정하세요:
User newUser = User.builder()
- .email(email)
- .name(name)
+ .email(fEmail)
+ .name(fName)
.role(Role.TEAM_MEMBER)
.build();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| User newUser = User.builder() | |
| .email(fEmail) | |
| .name(fName) | |
| .email(email) | |
| .name(name) | |
| .role(Role.TEAM_MEMBER) | |
| .build(); | |
| User newUser = User.builder() | |
| .email(fEmail) | |
| .name(fName) | |
| .role(Role.TEAM_MEMBER) | |
| .build(); |
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
around lines 88 to 92, the User builder is using local variables email and name
directly inside a lambda; replace those with the explicitly final variables
fEmail and fName declared on lines 80-81 so the lambda captures the final
variables consistently—update the builder calls to use fEmail and fName instead
of email and name.
Comment on lines
+57
to
79
| if (!(authentication.getPrincipal() instanceof DefaultOAuth2User oauthUser)) { | ||
| throw new IllegalStateException("Unknown principal type: " + authentication.getPrincipal().getClass()); | ||
| } | ||
|
|
||
| // 1. CustomOAuth2UserService에서 넣어준 attributes 가져오기 | ||
| DefaultOAuth2User oauthUser = (DefaultOAuth2User) authentication.getPrincipal(); | ||
| Map<String, Object> attrs = oauthUser.getAttributes(); | ||
|
|
||
| String providerStr = (String) attrs.get("provider"); | ||
| String providerUid = (String) attrs.get("providerUid"); | ||
|
|
||
| if (providerStr == null) { | ||
| throw new IllegalStateException("OAuth provider attribute missing from attributes"); | ||
| } | ||
|
|
||
| String userIdStr = oauthUser.getAttributes().get("id").toString(); | ||
| UUID userId = UUID.fromString(userIdStr); | ||
| String email = (String) oauthUser.getAttributes().get("email"); | ||
| String name = (String) oauthUser.getAttributes().get("name"); | ||
| AuthProvider provider = | ||
| switch (providerStr) { | ||
| case "kakao" -> AuthProvider.KAKAO; | ||
| case "github" -> AuthProvider.GITHUB; | ||
| case "google" -> AuthProvider.GOOGLE; | ||
| default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr); | ||
| }; | ||
|
|
||
| log.info("[OAuth2 Success] userId = {}, email = {}", userId, email); | ||
|
|
There was a problem hiding this comment.
OIDC 로그인 시 principal 타입 처리 버그
현재:
if (!(authentication.getPrincipal() instanceof DefaultOAuth2User oauthUser)) {
throw new IllegalStateException("Unknown principal type: " + authentication.getPrincipal().getClass());
}
Map<String, Object> attrs = oauthUser.getAttributes();로직 때문에, CustomOidcUserService가 반환하는 DefaultOidcUser(OidcUser 구현체)로 로그인하는 경우에는 항상 IllegalStateException이 발생하게 됩니다.
DefaultOAuth2User에만 의존하지 말고, 공통 인터페이스인 OAuth2User 기준으로 처리하는 것이 안전합니다.
또한, provider 매핑은 이미 AuthProvider.from(String)이 있으므로 중복 switch를 제거할 수 있습니다.
예시 수정안:
-import org.springframework.security.oauth2.core.oidc.user.OidcUser;
+import org.springframework.security.oauth2.core.user.OAuth2User;
@@
- if (!(authentication.getPrincipal() instanceof DefaultOAuth2User oauthUser)) {
+ if (!(authentication.getPrincipal() instanceof OAuth2User oauthUser)) {
throw new IllegalStateException("Unknown principal type: " + authentication.getPrincipal().getClass());
}
@@
- String providerStr = (String) attrs.get("provider");
+ String providerStr = (String) attrs.get("provider");
String providerUid = (String) attrs.get("providerUid");
@@
- AuthProvider provider =
- switch (providerStr) {
- case "kakao" -> AuthProvider.KAKAO;
- case "github" -> AuthProvider.GITHUB;
- case "google" -> AuthProvider.GOOGLE;
- default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr);
- };
+ AuthProvider provider = AuthProvider.from(providerStr);이렇게 하면:
- OAuth2(
DefaultOAuth2User)와 OIDC(DefaultOidcUser) 모두 동일한 SuccessHandler를 사용할 수 있고, - provider 문자열 매핑도 한 곳(
AuthProvider.from)에서 관리할 수 있어 추후 공급자 추가/변경 시 유지보수가 쉬워집니다.
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 57 to 79, replace the strict check for DefaultOAuth2User with a
check/cast to the common OAuth2User interface so both DefaultOAuth2User and
DefaultOidcUser are accepted, retrieve attributes from that OAuth2User, and use
the existing AuthProvider.from(String) method instead of the inline switch to
map providerStr (throwing a clear IllegalStateException if providerStr is
missing or AuthProvider.from returns null/invalid); ensure providerUid is read
safely from attributes as before.
Comment on lines
+50
to
53
| "/webjars/**", | ||
| "/login/**", | ||
| "/oauth2/**" | ||
| ); |
There was a problem hiding this comment.
쿠키 기반 access 토큰 허용에 따른 CSRF 리스크 점검 필요
이제 Authorization 헤더가 없을 때 "access" HttpOnly 쿠키의 JWT로도 인증이 되는데, 현재 설정을 보면:
SecurityConfig에서 CSRF가disable상태이고,- prod에서
SameSite = "None",secure = true로 설정되어, - 브라우저는 다른 사이트에서 발생한 요청에도 자동으로 이 쿠키를 첨부합니다.
이 조합이면 공격자가 별도 토큰 탈취 없이도 사용자의 브라우저를 이용해 상태 변경 요청(POST/PUT/DELETE 등)을 보내도록 유도하는 전형적인 CSRF 공격 가능성이 생깁니다.
옵션 예시입니다.
-
가장 확실한 방법
- 쿠키 기반 access 토큰을 유지할 거라면,
- state-changing 엔드포인트에 대해 CSRF 토큰(더블 서브밋, 헤더+쿠키 패턴 등)을 도입하거나,
- 최소한 특정 경로(ex:
/oauth2/**이후 프론트 초기 진입)에서만 쿠키 토큰을 허용하고, 나머지는 여전히 Authorization 헤더만 허용하도록 분리하는 방식을 검토해 주세요.
- 쿠키 기반 access 토큰을 유지할 거라면,
-
단기 완화책
- 당장 구조 변경이 어렵다면,
- 우선
resolveTokenFromCookie사용을 로그인 직후 특정 redirect 처리 등 정말 필요한 최소 경로로 한정하고, - 일반 API 호출에서는 여전히 Authorization 헤더만 허용하는 쪽으로 조정할 수 있습니다.
- 우선
- 당장 구조 변경이 어렵다면,
현재 변경은 보안 모델을 꽤 크게 바꾸는 부분이라, 실제 운영 전 보안 관점에서 한 번 더 논의/검증하는 것을 강하게 추천드립니다.
Also applies to: 77-80, 130-141
🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/springsecurity/JwtAuthenticationFilter.java
around lines 50-53 (and also review similar logic at 77-80 and 130-141), the new
logic that resolves access tokens from an HttpOnly "access" cookie exposes CSRF
risk because CSRF is disabled and SameSite=None with secure=true will send the
cookie on cross-site requests; to fix this either: 1) implement CSRF protection
for state-changing endpoints (e.g., double-submit cookie or header+cookie token
validation) before accepting cookie-based authentication, or 2) restrict
cookie-based token resolution to a very small set of safe endpoints (for example
only on the initial OAuth2 redirect/SPA bootstrap path) and keep all ordinary
API endpoints requiring Authorization header only; as a short-term mitigation,
limit resolveTokenFromCookie usage to post-login redirect handling and ensure
server sets stricter cookie attributes (SameSite=Lax/Strict where feasible) and
document the change for a security review before production rollout.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
57-77: OIDC 로그인 호환성 버그가 여전히 해결되지 않았습니다.이전 리뷰에서 지적된 중대한 문제가 아직 수정되지 않았습니다:
현재 코드는
DefaultOAuth2User만 허용하므로,CustomOidcUserService가 반환하는DefaultOidcUser로 로그인하면 항상IllegalStateException이 발생합니다.또한 provider 매핑을 위한 switch 문이 중복됩니다 -
AuthProvider.from(String)메서드를 사용하여 단일 지점에서 관리해야 합니다.다음 diff를 적용하여 수정하세요:
+import org.springframework.security.oauth2.core.user.OAuth2User; - if (!(authentication.getPrincipal() instanceof DefaultOAuth2User oauthUser)) { + if (!(authentication.getPrincipal() instanceof OAuth2User oauthUser)) { throw new IllegalStateException("Unknown principal type: " + authentication.getPrincipal().getClass()); } // 1. CustomOAuth2UserService에서 넣어준 attributes 가져오기 Map<String, Object> attrs = oauthUser.getAttributes(); String providerStr = (String) attrs.get("provider"); String providerUid = (String) attrs.get("providerUid"); if (providerStr == null) { throw new IllegalStateException("OAuth provider attribute missing from attributes"); } - AuthProvider provider = - switch (providerStr) { - case "kakao" -> AuthProvider.KAKAO; - case "github" -> AuthProvider.GITHUB; - case "google" -> AuthProvider.GOOGLE; - default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr); - }; + AuthProvider provider = AuthProvider.from(providerStr); + if (provider == null) { + throw new IllegalStateException("Unknown OAuth provider: " + providerStr); + }
🧹 Nitpick comments (2)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (2)
104-104: 프로필 체크를 더 효율적인 API로 개선하세요.
Arrays.asList(env.getActiveProfiles()).contains("prod")는 비효율적입니다. Spring의 네이티브 API를 사용하는 것이 더 좋습니다.다음과 같이 리팩토링하세요:
- boolean isProd = Arrays.asList(env.getActiveProfiles()).contains("prod"); + boolean isProd = env.acceptsProfiles(org.springframework.core.env.Profiles.of("prod"));이렇게 하면
Arraysimport도 제거할 수 있습니다.
134-143: 주석 처리된 코드를 제거하세요.설정 가능한
redirectSuccessBase사용은 좋은 개선입니다. 하지만 Lines 136-139의 주석 처리된 코드는 더 이상 필요하지 않으므로 제거하여 코드를 깔끔하게 유지하세요.다음과 같이 정리하세요:
// 7. 프론트로 redirect - // application-local.yml → http://localhost:5173/oauth/success - // application-prod.yml → https://sisc-web.duckdns.org/oauth/success - //String redirectUrl = redirectSuccessBase; -// + "?accessToken=" + accessToken -// + "&name=" + URLEncoder.encode(name, StandardCharsets.UTF_8) -// + "&userId=" + userId; - - // log.info("[OAuth2 Redirect] {}", redirectUrl); - getRedirectStrategy().sendRedirect(request, response, redirectSuccessBase);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java(0 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java(4 hunks)
💤 Files with no reviewable changes (1)
- backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java
🔇 Additional comments (2)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java (1)
69-69: LGTM - 포맷팅 변경주석 처리된 로그 구문의 공백을 정리한 사소한 포맷팅 변경입니다. 기능적 영향은 없습니다.
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
6-6: 새로운 import 추가가 적절합니다.환경 인식 구성을 위한 필요한 import들이 올바르게 추가되었습니다.
Also applies to: 14-14, 31-31
Comment on lines
+44
to
+47
| private final Environment env; | ||
|
|
||
| @Value("${app.oauth2.redirect-success}") | ||
| private String redirectSuccessBase; |
There was a problem hiding this comment.
🧩 Analysis chain
환경별 설정을 위한 필드 추가를 확인하세요.
Environment와 @Value 주입이 적절하게 추가되었습니다. 다만 app.oauth2.redirect-success 속성이 모든 환경 설정 파일(application-local.yml, application-prod.yml 등)에 정의되어 있는지 확인이 필요합니다.
다음 스크립트로 속성 정의를 확인하세요:
🏁 Script executed:
#!/bin/bash
# Description: Verify that app.oauth2.redirect-success property is defined in all environment config files
# Search for the property definition in YAML files
rg -n "redirect-success" --type=yamlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find all application config files
echo "=== Searching for application config files ==="
fd -e yml -e yaml -e properties | head -20
echo "=== Searching for any oauth2 configuration ==="
rg -n "oauth2" --type=yaml --type=properties -i
echo "=== Checking Java config files for OAuth2 ==="
rg -n "oauth2|redirect" --type=java -i -C2 | head -50Length of output: 5925
🏁 Script executed:
#!/bin/bash
# Verify if the field will cause runtime failures
echo "=== Checking for Spring @Value usage patterns in the file ==="
cat backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java | grep -n "@Value" -A2 -B2Length of output: 198
🏁 Script executed:
#!/bin/bash
# Search for any configuration classes or properties
echo "=== Looking for application configuration files ==="
find . -name "application*.yml" -o -name "application*.yaml" -o -name "application*.properties" 2>/dev/null | head -20Length of output: 96
🏁 Script executed:
#!/bin/bash
# Find all application config files and search for OAuth2/redirect configuration
echo "=== All application config files ==="
fd "application.*\.(yml|yaml|properties)$"
echo "=== Search for oauth2 configuration anywhere ==="
rg -i "oauth2|redirect-success" -C2
echo "=== Check the OAuth2SuccessHandler file for @Value annotations ==="
fd "OAuth2SuccessHandler.java$" -x cat {} | grep -n "@Value" -A1 -B1Length of output: 31566
🏁 Script executed:
#!/bin/bash
# Verify Spring @Value behavior with missing properties
echo "=== Understanding @Value with missing property ==="
# When Spring finds @Value with a property that doesn't exist, it throws:
# BeanCreationException or IllegalArgumentException at startup
echo "Missing @Value properties cause startup failure in Spring"Length of output: 108
app.oauth2.redirect-success 속성이 설정 파일에 정의되지 않았습니다. 애플리케이션 시작 실패를 유발합니다.
backend/src/main/resources/application.yml에 app.oauth2.redirect-success 속성이 정의되어 있지 않습니다. Spring의 @Value 주입은 해당 속성이 존재하지 않을 경우 시작 시점에 BeanCreationException을 발생시켜 애플리케이션이 구동되지 않습니다.
application.yml에 다음을 추가하세요:
app:
oauth2:
redirect-success: <실제 리다이렉트 URL>🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 44 to 47, the @Value("${app.oauth2.redirect-success}") injection
fails at startup because the app.oauth2.redirect-success property is missing;
add the property to backend/src/main/resources/application.yml under app.oauth2
with an appropriate redirect URL (e.g., set app.oauth2.redirect-success:
<your-redirect-url>) so Spring can inject the value and the application can
start successfully.
|
|
||
| String sameSite = isProd ? "None" : "Lax"; | ||
| boolean secure = isProd; | ||
| String domain = isProd ? "sisc-web.duckdns.org" : "localhost"; |
There was a problem hiding this comment.
도메인을 하드코딩하지 말고 설정으로 외부화하세요.
도메인 "sisc-web.duckdns.org"가 하드코딩되어 있습니다. 환경이나 도메인이 변경될 때 코드 수정이 필요하므로, application.yml에 속성으로 정의하는 것이 좋습니다.
예시:
+ @Value("${app.oauth2.cookie-domain:localhost}")
+ private String cookieDomain;
+
@Override
public void onAuthenticationSuccess(...) {
- String domain = isProd ? "sisc-web.duckdns.org" : "localhost";
+ String domain = isProd ? cookieDomain : "localhost";또는 환경별 설정 파일에서:
# application-prod.yml
app:
oauth2:
cookie-domain: sisc-web.duckdns.org🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around line 108 the domain "sisc-web.duckdns.org" is hardcoded; externalize it
by adding a property (e.g. app.oauth2.cookie-domain) to
application.yml/application-prod.yml and read it in the class (via
@Value("${app.oauth2.cookie-domain}") or a @ConfigurationProperties bean) with a
sensible default or profile-specific values; replace the hardcoded literal with
the injected property so domain changes are driven by configuration, not code.
Comment on lines
112
to
126
| ResponseCookie accessCookie = ResponseCookie.from("access", accessToken) | ||
| .httpOnly(true) | ||
| .secure(false) // 로컬 개발 | ||
| .sameSite("Lax") // 로컬에서는 None 비추천 | ||
| .secure(secure) // 로컬=false, 배포=true | ||
| .sameSite(sameSite) // 로컬= "Lax", 배포="None" | ||
| .path("/") | ||
| .maxAge(60L * 60) // 1 hour | ||
| .build(); | ||
|
|
||
| ResponseCookie refreshCookie = ResponseCookie.from("refresh", refreshToken) | ||
| .httpOnly(true) | ||
| .secure(false) | ||
| .sameSite("Lax") | ||
| .secure(secure) | ||
| .sameSite(sameSite) | ||
| .path("/") | ||
| .maxAge(60L * 60 * 24 * 14) // 2 weeks | ||
| .build(); |
There was a problem hiding this comment.
쿠키에 domain 속성이 설정되지 않았습니다.
Line 108에서 domain 변수를 계산했지만, 쿠키 생성 시 실제로 적용하지 않았습니다. 이로 인해 쿠키가 의도한 도메인에서 작동하지 않을 수 있습니다.
다음 diff를 적용하여 수정하세요:
ResponseCookie accessCookie = ResponseCookie.from("access", accessToken)
.httpOnly(true)
.secure(secure) // 로컬=false, 배포=true
.sameSite(sameSite) // 로컬= "Lax", 배포="None"
.path("/")
+ .domain(domain)
.maxAge(60L * 60) // 1 hour
.build();
ResponseCookie refreshCookie = ResponseCookie.from("refresh", refreshToken)
.httpOnly(true)
.secure(secure)
.sameSite(sameSite)
.path("/")
+ .domain(domain)
.maxAge(60L * 60 * 24 * 14) // 2 weeks
.build();🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 112 to 126, the ResponseCookie builders for access and refresh
cookies omit the domain setting calculated earlier; update both builders to call
.domain(domain) so the cookies are scoped to the intended domain (ensure the
domain variable is correctly computed/non-null before use, or conditionally
apply .domain(domain) only when valid).
…m/SISC-IT/sisc-web into SISC1-209-BE-redirect-url-프론트로-전달
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
새로운 기능
Chores