-
Notifications
You must be signed in to change notification settings - Fork 1
♻️refactor: 소셜 로그인 성공 시 리다이렉트 URI를 프론트쪽으로 변경 #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update the OAuth2 authentication success handler to redirect users to a configurable frontend base URI, directing them either to the home page or profile completion page based on their profile status. Additionally, cookie SameSite attributes are set to "None," and a new configuration property for the frontend base URI is introduced. The previously used OAuth2 redirect endpoint in the controller is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OAuth2Provider
participant Server (OAuth2AuthenticationSuccessHandler)
participant Frontend
User->>OAuth2Provider: Initiate OAuth2 login
OAuth2Provider->>Server (OAuth2AuthenticationSuccessHandler): Callback with authentication
Server (OAuth2AuthenticationSuccessHandler)->>Server (OAuth2AuthenticationSuccessHandler): Retrieve Member and check profile status
alt Profile completed
Server (OAuth2AuthenticationSuccessHandler)->>User: Redirect to {baseUri}/home
else Profile incomplete
Server (OAuth2AuthenticationSuccessHandler)->>User: Redirect to {baseUri}/profile
end
User->>Frontend: Land on redirected page
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java (1)
33-41: Deletion cookie missing Secure; align attributes for reliable removalWhen deleting, mirror the security attributes used when setting the cookie. Add
cookie.setSecure(true)to ensure consistent handling across browsers.Apply this diff:
Cookie cookie = new Cookie(cookieName, null); cookie.setMaxAge(0); cookie.setPath("/"); cookie.setHttpOnly(true); + cookie.setSecure(true); cookie.setAttribute("SameSite", "None"); response.addCookie(cookie);
🧹 Nitpick comments (2)
src/main/resources/application-oauth2.yml (1)
1-5: Make base URI robust: provide a sensible default and document format
- Provide a local default to reduce misconfig risk.
- Document whether trailing slash is allowed; current code appends a path and could produce double slashes depending on the value.
Suggested change:
- base-uri: ${FRONTEND_BASE_URI} + base-uri: ${FRONTEND_BASE_URI:http://localhost:3000}Also verify CORS is configured to allow credentials (no wildcard origin) for the frontend origin.
src/main/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java (1)
29-31: Validate configuration at startupFail fast if
app.oauth2.redirect.base-uriis missing/blank to avoid runtime surprises.Example:
if (baseUri == null || baseUri.isBlank()) { throw new IllegalStateException("app.oauth2.redirect.base-uri must be configured"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java(2 hunks)src/main/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java(2 hunks)src/main/resources/application-oauth2.yml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java (1)
src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java (1)
Component(8-42)
🪛 ast-grep (0.38.6)
src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java
[warning] 39-39: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
[warning] 39-39: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
🔇 Additional comments (2)
src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java (1)
14-14: Confirm SameSite=None support and enforce CSRF protectionsThe
cookie.setAttribute("SameSite", "None")call depends on your servlet container and Jakarta Servlet API version supporting arbitrary cookie attributes. If you’re on an older container (e.g. Servlet 5.x in Spring Boot 3.0), this may be ignored. Please:• Verify your container and servlet API version support
Cookie#setAttribute("SameSite", "None").
• If not supported, fall back to manually emitting the header:response.addHeader("Set-Cookie", String.format("%s=%s; Path=/; Max-Age=%d; HttpOnly; Secure; SameSite=None", cookieName, token, maxAge));• Because
SameSite=Nonepermits cross-site cookie delivery, ensure you have a CSRF protection strategy in place (e.g. Spring Security CSRF tokens or a double-submit cookie pattern).Locations to review:
- src/main/java/checkmo/domain/member/service/security/jwt/JwtCookieUtil.java –
addTokenToCookie(...)at lines 13–17 and 36–39.src/main/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java (1)
38-41: Guard the principal castIf this handler is ever reused with another auth flow, a direct cast can throw. Add an
instanceofguard or constrain the handler mapping to only the OAuth2 flow.Minimal guard example:
if (!(authentication.getPrincipal() instanceof PrincipalDetails principalDetails)) { getRedirectStrategy().sendRedirect(request, response, baseUri); return; } Member member = principalDetails.getMember();
...n/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java
Outdated
Show resolved
Hide resolved
...n/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java
Show resolved
Hide resolved
...n/java/checkmo/domain/member/service/security/oauth2/OAuth2AuthenticationSuccessHandler.java
Show resolved
Hide resolved
zjhj0814
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다~! 저한테는 좋네요
🚀 변경사항
프론트쪽으로 리다이렉트하도록 변경
/profile(프로필 추가 정보 입력 페이지)로 이동/home(홈화면 페이지)로 이동🔗 관련 이슈
✅ 체크리스트
Summary by CodeRabbit
New Features
Bug Fixes
Chores