Feat: OAuth2 로그인 시 target_url 쿠키를 통한 동적 리다이렉트 기능 구현#179
Conversation
📝 WalkthroughWalkthroughChangesOAuth2 cookie redirect flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig
participant HttpCookieOAuth2AuthorizationRequestRepository
participant CookieUtils
participant OAuth2SuccessHandler
Client->>SecurityConfig: Start OAuth2 login with target_url
SecurityConfig->>HttpCookieOAuth2AuthorizationRequestRepository: Save authorization request
HttpCookieOAuth2AuthorizationRequestRepository->>CookieUtils: Add authorization and target URL cookies
Client->>OAuth2SuccessHandler: Complete OAuth2 authentication
OAuth2SuccessHandler->>CookieUtils: Read target_url cookie
OAuth2SuccessHandler->>HttpCookieOAuth2AuthorizationRequestRepository: Clear authorization cookies
OAuth2SuccessHandler-->>Client: Redirect to target URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/knockin/auth/handler/OAuth2SuccessHandler.java (1)
56-69: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winOpen Redirect vulnerability.
The
targetUrlis read from thetarget_urlcookie (originally populated from an unvalidated user request parameter) and passed directly toresponse.sendRedirect(targetUrl). This allows an attacker to craft a malicious login link that redirects the user to an arbitrary external site after a successful login, facilitating phishing attacks.You must validate the
targetUrlagainst a trusted list of allowed redirect URIs (e.g., your configured CORS origins) before executing the redirect. Ensure the validation logic prevents substring bypasses (like an attacker registeringhttps://yourdomain.com.malicious.com).🛡️ Proposed fix to securely validate the redirect URI
- String targetUrl = CookieUtils.getCookie(request, HttpCookieOAuth2AuthorizationRequestRepository.REDIRECT_URI_PARAM_COOKIE_NAME).map(Cookie::getValue).orElse(knockInProps.getClientSuccessUrl()); + String targetUrl = CookieUtils.getCookie(request, HttpCookieOAuth2AuthorizationRequestRepository.REDIRECT_URI_PARAM_COOKIE_NAME).map(Cookie::getValue).orElse(knockInProps.getClientSuccessUrl()); + + // Validate that the target URL exactly matches an allowed origin or is a safe subpath + boolean isAuthorized = knockInProps.getCorsUrls().stream() + .anyMatch(allowedUrl -> targetUrl.equals(allowedUrl) || targetUrl.startsWith(allowedUrl + "/")); + + if (!isAuthorized && !targetUrl.equals(knockInProps.getClientSuccessUrl())) { + targetUrl = knockInProps.getClientSuccessUrl(); + } boolean secureCookie = knockInProps.getClientSuccessUrl().startsWith("https://"); ResponseCookie accessTokenCookie = ResponseCookie.from(TokenConstants.ACCESS_TOKEN_COOKIE_NAME, accessToken) .httpOnly(true) .secure(secureCookie) .sameSite(secureCookie ? "None" : "Lax") .path("/") .maxAge(TokenProvider.ACCESS_TOKEN_EXPIRE_DURATION) .build(); response.addHeader(HttpHeaders.SET_COOKIE, accessTokenCookie.toString()); httpCookieOAuth2AuthorizationRequestRepository.clearCookies(request, response); response.sendRedirect(targetUrl);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/knockin/auth/handler/OAuth2SuccessHandler.java` around lines 56 - 69, Validate targetUrl in OAuth2SuccessHandler against the configured trusted redirect origins before calling response.sendRedirect, using exact origin/host matching rather than substring checks so lookalike domains such as yourdomain.com.malicious.com are rejected. Fall back to knockInProps.getClientSuccessUrl() or otherwise avoid redirecting when the cookie value is not allowed, while preserving valid trusted redirects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/example/knockin/auth/util/CookieUtils.java`:
- Around line 23-29: Update CookieUtils.addCookie and deleteCookie to use
Spring’s ResponseCookie instead of jakarta.servlet.http.Cookie, configuring
Secure, HttpOnly, and SameSite attributes consistently. Ensure deletion uses
matching attributes and cookie path so browsers accept the clearing cookie in
cross-origin scenarios.
- Around line 45-51: Replace Java serialization in CookieUtils.serialize and
CookieUtils.deserialize with a safe explicit representation for
OAuth2AuthorizationRequest, validating all decoded fields before reconstruction;
alternatively, store and resolve only an opaque server-side identifier. Ensure
deserialize never passes attacker-controlled cookie bytes to
SerializationUtils.deserialize.
---
Outside diff comments:
In `@src/main/java/org/example/knockin/auth/handler/OAuth2SuccessHandler.java`:
- Around line 56-69: Validate targetUrl in OAuth2SuccessHandler against the
configured trusted redirect origins before calling response.sendRedirect, using
exact origin/host matching rather than substring checks so lookalike domains
such as yourdomain.com.malicious.com are rejected. Fall back to
knockInProps.getClientSuccessUrl() or otherwise avoid redirecting when the
cookie value is not allowed, while preserving valid trusted redirects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf89a53a-fbe8-408e-9de0-7477402d18ee
📒 Files selected for processing (4)
src/main/java/org/example/knockin/auth/handler/OAuth2SuccessHandler.javasrc/main/java/org/example/knockin/auth/repository/HttpCookieOAuth2AuthorizationRequestRepository.javasrc/main/java/org/example/knockin/auth/util/CookieUtils.javasrc/main/java/org/example/knockin/config/SecurityConfig.java
| public static void addCookie(HttpServletResponse response, String name, String value, int maxAge) { | ||
| Cookie cookie = new Cookie(name, value); | ||
| cookie.setPath("/"); | ||
| cookie.setHttpOnly(true); | ||
| cookie.setMaxAge(maxAge); | ||
| response.addCookie(cookie); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set Secure and SameSite attributes for cookies.
Cookies are added without the Secure flag. If the application is accessed over HTTPS, sensitive OAuth2 authorization and redirect cookies could be transmitted in plaintext over insecure connections.
Additionally, standard jakarta.servlet.http.Cookie lacks a straightforward way to set the SameSite attribute dynamically. Consider using Spring's ResponseCookie (for both adding and deleting cookies) to securely configure Secure, HttpOnly, and SameSite flags.
🛡️ Proposed fix using `ResponseCookie`
- public static void addCookie(HttpServletResponse response, String name, String value, int maxAge) {
- Cookie cookie = new Cookie(name, value);
- cookie.setPath("/");
- cookie.setHttpOnly(true);
- cookie.setMaxAge(maxAge);
- response.addCookie(cookie);
- }
+ public static void addCookie(HttpServletResponse response, String name, String value, int maxAge) {
+ org.springframework.http.ResponseCookie cookie = org.springframework.http.ResponseCookie.from(name, value)
+ .path("/")
+ .httpOnly(true)
+ .secure(true) // Set dynamically based on environment if needed
+ .sameSite("Lax")
+ .maxAge(maxAge)
+ .build();
+ response.addHeader(org.springframework.http.HttpHeaders.SET_COOKIE, cookie.toString());
+ }(Note: Apply a similar change to deleteCookie to ensure the clearing cookie is accepted by the browser when cross-origin configurations are in place.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void addCookie(HttpServletResponse response, String name, String value, int maxAge) { | |
| Cookie cookie = new Cookie(name, value); | |
| cookie.setPath("/"); | |
| cookie.setHttpOnly(true); | |
| cookie.setMaxAge(maxAge); | |
| response.addCookie(cookie); | |
| } | |
| public static void addCookie(HttpServletResponse response, String name, String value, int maxAge) { | |
| org.springframework.http.ResponseCookie cookie = org.springframework.http.ResponseCookie.from(name, value) | |
| .path("/") | |
| .httpOnly(true) | |
| .secure(true) // Set dynamically based on environment if needed | |
| .sameSite("Lax") | |
| .maxAge(maxAge) | |
| .build(); | |
| response.addHeader(org.springframework.http.HttpHeaders.SET_COOKIE, cookie.toString()); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 27-27: 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).
(cookie-missing-samesite-java)
[warning] 27-27: Ensure cookies have the secure flag
Context: response.addCookie(cookie)
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute.
(cookies-secure-flag)
[warning] 27-27: 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.
(cookie-missing-secure-flag-java)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/example/knockin/auth/util/CookieUtils.java` around lines 23
- 29, Update CookieUtils.addCookie and deleteCookie to use Spring’s
ResponseCookie instead of jakarta.servlet.http.Cookie, configuring Secure,
HttpOnly, and SameSite attributes consistently. Ensure deletion uses matching
attributes and cookie path so browsers accept the clearing cookie in
cross-origin scenarios.
Source: Linters/SAST tools
| public static String serialize(Object object) { | ||
| return Base64.getUrlEncoder().encodeToString(SerializationUtils.serialize(object)); | ||
| } | ||
|
|
||
| public static <T> T deserialize(Cookie cookie, Class<T> cls) { | ||
| return cls.cast(SerializationUtils.deserialize(Base64.getUrlDecoder().decode(cookie.getValue()))); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of SerializationUtils and verify Jackson ObjectMapper existence.
rg 'SerializationUtils\.deserialize'
rg 'ObjectMapper'Repository: prography/11th-1team-BE
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and related auth utilities.
git ls-files 'src/main/java/org/example/knockin/auth/util/CookieUtils.java' \
'src/main/java/org/example/knockin/auth/**' \
'src/main/java/**/OAuth2AuthorizationRequest*' \
'src/main/java/**/Cookie*' | sort
echo '--- CookieUtils outline ---'
ast-grep outline src/main/java/org/example/knockin/auth/util/CookieUtils.java --view expanded || true
echo '--- CookieUtils contents ---'
cat -n src/main/java/org/example/knockin/auth/util/CookieUtils.java
echo '--- Search for CookieUtils usages ---'
rg -n 'CookieUtils\.(serialize|deserialize)|deserialize\(cookie|serialize\(' src/main/java || true
echo '--- Search for OAuth2AuthorizationRequest serialization helpers ---'
rg -n 'OAuth2AuthorizationRequest|OAuth2ClientJackson2Module|Jackson2Module|ObjectMapper|SerializationUtils' src/main/java || trueRepository: prography/11th-1team-BE
Length of output: 10944
Avoid Java deserialization for the cookie payload. CookieUtils.deserialize() feeds attacker-controlled cookie data straight into SerializationUtils.deserialize, which exposes this OAuth2 flow to gadget-based deserialization attacks. Use a safe explicit format for OAuth2AuthorizationRequest or store only an opaque server-side ID in the cookie.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/example/knockin/auth/util/CookieUtils.java` around lines 45
- 51, Replace Java serialization in CookieUtils.serialize and
CookieUtils.deserialize with a safe explicit representation for
OAuth2AuthorizationRequest, validating all decoded fields before reconstruction;
alternatively, store and resolve only an opaque server-side identifier. Ensure
deserialize never passes attacker-controlled cookie bytes to
SerializationUtils.deserialize.
2 participants
PR 타입(하나 이상의 PR 타입을 선택해주세요)
변경 사항
리다이렉트하도록 안전장치(Fallback)를 적용했습니다.
테스트 결과
관련 이슈
Summary by CodeRabbit
New Features
Bug Fixes