Conversation
WalkthroughThis change refactors the authentication and token management flow by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend
participant Filter as AccessTokenFilter
participant Resolver as TokenResolver
participant Manager as AuthTokenManager
participant Handler as TokenReissueHandler
FE->>Filter: Request to /api/v1/auth/reissue with cookies
Filter->>Resolver: resolveTokens(request)
Resolver-->>Filter: TokenCookies(accessToken, refreshToken)
Filter->>Manager: reissueToken(TokenCookies)
Manager-->>Filter: new Token
Filter->>Handler: onReissueSuccess(request, response, new Token)
Handler-->>FE: Set new cookies, return JSON success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wlgns12370, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors the application's token authentication and reissuance mechanism to better support frontend-driven token refreshes. It introduces new utility classes for robust cookie management and token resolution, centralizes reissuance logic, and updates the main access token filter to accommodate these changes and improve error handling.
Highlights
- Enhanced Token Reissuance Flow: The refresh token authentication process has been updated to allow the frontend to initiate token reissuance upon authentication failure. This involves a new
/api/v1/auth/reissueendpoint handled by theAccessTokenFilter. - Centralized Token Resolution and Management: Introduced
TokenCookiesrecord to encapsulate access and refresh tokens, andTokenResolverto centralize the extraction of these tokens from HTTP requests. TheAuthTokenManagernow usesTokenCookiesfor reissuance. - Refactored Cookie Utility: The
CookieBuilderUtilhas been significantly refactored to provide environment-specific (local, develop, production) methods for creatingResponseCookieandjakarta.servlet.Cookieobjects, ensuring consistent and secure cookie handling across different deployment stages. - Dedicated Reissue Handler: A new
ReissueHandlerinterface and its implementation,TokenReissueHandler, have been added to specifically manage the response logic after a successful token reissuance, including setting new cookies and sending a JSON response. - Updated Access Token Filter Logic: The
AccessTokenFilternow includes specific handling for the new reissuance endpoint, refined logic for signup token validation (expecting a Bearer token in the header), and updated error handling for missing or invalid tokens. Several public API endpoints (/api/v1/event,/api/v1/faq,/api/v1/notice) have been added to the list of URLs that skip token validation.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the token authentication and reissue process, introducing several new components like TokenResolver, ReissueHandler, and a very useful CookieBuilderUtil. The changes are well-structured and improve the modularity of the authentication flow. I've provided some feedback focusing on security best practices (avoiding logging sensitive data), improving correctness in error handling, and enhancing maintainability through safer type casting and the use of DTOs. Overall, this is a solid improvement to the codebase.
| public boolean availableReissue() { | ||
| return getAccessToken().isPresent() && getRefreshToken().isPresent(); | ||
| } |
There was a problem hiding this comment.
The availableReissue method currently requires both the access token and refresh token to be present. For a token reissue operation, typically only the refresh token is necessary to generate a new access token. This implementation could be brittle if the access token is missing for any reason, even with a valid refresh token.
It would be more robust to only check for the presence of the refresh token. With this change, you should also update the logic in AccessTokenFilter:112 to throw NOT_FOUND_REFRESH_TOKEN instead of EXPIRED_REFRESH_TOKEN when this check fails.
| public boolean availableReissue() { | |
| return getAccessToken().isPresent() && getRefreshToken().isPresent(); | |
| } | |
| public boolean availableReissue() { | |
| return refreshToken != null; | |
| } |
| String accessToken = tokenCookies.accessToken(); | ||
| log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken); |
There was a problem hiding this comment.
Logging the full access token can be a security risk, as it may expose sensitive user session information in logs. If these logs are compromised, the tokens could be stolen and used to impersonate users.
It's recommended to avoid logging the full token. If you need to confirm that a token is present, you could log a boolean value, its length, or a truncated/hashed version of it.
| String accessToken = tokenCookies.accessToken(); | |
| log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken); | |
| log.info("스킵된 URL 요청: {} 토큰 존재 여부: {}", requestUri, accessToken != null); |
| try { | ||
| log.info("액세스 토큰 인증 요청: {}", accessToken); | ||
| setAuthentication(accessToken); | ||
| } catch (Exception e) { | ||
| log.error("액세스 토큰 인증 실패: {}", e.getMessage()); | ||
| throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN); | ||
| } |
There was a problem hiding this comment.
The try-catch (Exception e) block is too broad. It catches any exception that might occur during authentication and re-throws it as a generic INVALID_TOKEN error. This hides the original, more specific cause of the failure.
For example, if accessTokenProvider.getAuthentication(accessToken) throws a CustomRuntimeException with ErrorCode.EXPIRED_TOKEN, this information will be lost, and the client will only see INVALID_TOKEN. This makes debugging and client-side error handling more difficult.
You should remove this try-catch block and let the specific exceptions propagate up to your global exception handler (e.g., TokenExceptionFilter), which can then generate a more appropriate error response.
log.info("액세스 토큰 인증 요청: {}", accessToken);
setAuthentication(accessToken);| if (!tokenCookies.availableReissue()) { | ||
| throw new CustomRuntimeException(ErrorCode.EXPIRED_REFRESH_TOKEN); |
There was a problem hiding this comment.
The check !tokenCookies.availableReissue() verifies if the required tokens for reissue are present. If they are not, the code throws CustomRuntimeException(ErrorCode.EXPIRED_REFRESH_TOKEN). This is semantically incorrect. A missing token is not an expired token. This can be misleading when debugging issues.
You should throw an error that more accurately reflects the problem, such as NOT_FOUND_REFRESH_TOKEN.
if (!tokenCookies.availableReissue()) {
throw new CustomRuntimeException(ErrorCode.NOT_FOUND_REFRESH_TOKEN);
}| Map<String, Object> responseBody = new HashMap<>(); | ||
| responseBody.put("status", HttpStatus.OK.value()); | ||
| responseBody.put("message", "reissue를 성공했습니다."); |
There was a problem hiding this comment.
Using a Map<String, Object> for the JSON response body is flexible but lacks type safety and can be less maintainable as the API evolves. It's generally better practice to use a dedicated Data Transfer Object (DTO) for your API responses.
Creating a simple response DTO would make the response structure explicit and self-documenting.
For example, you could define a record:
public record ReissueResponse(int status, String message) {}And then use it here:
ReissueResponse responseBody = new ReissueResponse(HttpStatus.OK.value(), "reissue를 성공했습니다.");
objectMapper.writeValue(response.getWriter(), responseBody);| private static Cookie createBaseServletCookie(String name, String value, Long maxAge) { | ||
| Cookie cookie = new Cookie(name, value); | ||
| cookie.setPath("/"); | ||
| cookie.setMaxAge(maxAge.intValue()); |
There was a problem hiding this comment.
The maxAge parameter is a Long, but jakarta.servlet.http.Cookie.setMaxAge() expects an int. The cast maxAge.intValue() can lead to an integer overflow if maxAge is larger than Integer.MAX_VALUE (about 68 years in seconds), potentially causing unexpected cookie behavior.
While token expiration times are typically much shorter, it's safer to handle this potential overflow, for example by clamping the value to Integer.MAX_VALUE if it exceeds it.
| cookie.setMaxAge(maxAge.intValue()); | |
| cookie.setMaxAge(maxAge > Integer.MAX_VALUE ? Integer.MAX_VALUE : maxAge.intValue()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
26-27: Remove duplicate constants to avoid inconsistency.These constants are duplicated from
CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAMEandCookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME. This creates maintenance burden and potential inconsistency.- private final static String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; - private final static String REFRESH_TOKEN_COOKIE_NAME = "refreshToken";Update the method to use the constants from
CookieBuilderUtil:headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createLocalCookieString( - ACCESS_TOKEN_COOKIE_NAME, + CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, token.accessToken(), token.accessTokenExpireTime() )); headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createLocalCookieString( - REFRESH_TOKEN_COOKIE_NAME, + CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, token.refreshToken(), token.refreshTokenExpireTime() ));
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (2)
26-37: Consider environment-aware cookie creation.The implementation hardcodes
createLocalCookiewhich may not be appropriate for all deployment environments. Consider injecting an environment-specific cookie builder or using a factory pattern.Stream.of( - CookieBuilderUtil.createLocalCookie( + createEnvironmentAwareCookie( CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, newToken.accessToken(), newToken.accessTokenExpireTime() ), - CookieBuilderUtil.createLocalCookie( + createEnvironmentAwareCookie( CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, newToken.refreshToken(), newToken.refreshTokenExpireTime() ) ).forEach(response::addCookie);
39-41: Consider internationalization for response message.The hardcoded Korean message "reissue를 성공했습니다." may not be appropriate for all clients. Consider using a message source or making the message configurable.
Map<String, Object> responseBody = new HashMap<>(); responseBody.put("status", HttpStatus.OK.value()); -responseBody.put("message", "reissue를 성공했습니다."); +responseBody.put("message", "Token reissue successful");src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java (1)
108-117: Consider additional token validationWhile
availableReissue()checks for token presence using Optional, it doesn't validate if the tokens are non-empty strings. Consider adding validation for empty tokens to prevent issues downstream.private void reissueToken(HttpServletRequest request, HttpServletResponse response) throws IOException { final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); if (!tokenCookies.availableReissue()) { throw new CustomRuntimeException(ErrorCode.EXPIRED_REFRESH_TOKEN); } + + // Validate tokens are not empty + if (tokenCookies.accessToken().isEmpty() || tokenCookies.refreshToken().isEmpty()) { + throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN); + } Token newToken = authTokenManager.reissueToken(tokenCookies); reissueHandler.onReissueSuccess(request, response, newToken); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/life/mosu/mosuserver/application/auth/provider/AuthTokenManager.java(2 hunks)src/main/java/life/mosu/mosuserver/application/auth/provider/RefreshTokenProvider.java(2 hunks)src/main/java/life/mosu/mosuserver/global/cookie/TokenCookies.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java(3 hunks)src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java(2 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/ReissueHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java (1)
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (1)
Component(17-49)
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
🔇 Additional comments (19)
src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (3)
10-10: LGTM! Good addition of logging capability.The @slf4j annotation and import addition enables proper logging functionality for this service class.
Also applies to: 13-13
27-27: LGTM! Method signature update aligns with error handling.The updated method signature to declare throwing
CustomRuntimeExceptionis consistent with the existing exception handling logic in the method.
35-36: No sensitive data exposed in the signup token log
TheSignUpTokenrecord definesexpirationas aLongtimestamp (seedomain/auth/signup/SignUpToken.java), and the log statement insrc/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java:35 log.info("signUp {}", signUpToken.expiration());only emits that timestamp. There’s no sensitive information being logged.
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
48-57: LGTM! Standardized cookie creation method usage.The change from
temporaryCookieStringtocreateLocalCookieStringaligns with the broader refactoring to use environment-specific cookie creation methods. This improves code consistency while maintaining the same functionality.src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
45-54: LGTM! Consistent cookie creation method standardization.The change to use
createLocalCookieStringinstead oftemporaryCookieStringis consistent with similar updates across other controllers and improves code standardization.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
49-59: LGTM! Consistent ResponseCookie creation standardization.The change to use
createLocalResponseCookieinstead oftemporaryCookiefollows the same standardization pattern as other files. UsingResponseCookieobjects is appropriate for this handler context.src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java (2)
8-8: LGTM! Added necessary import for cookie constants.The import of
CookieBuilderUtilis needed to access the centralized cookie name constants.
22-25: LGTM! Excellent replacement of magic strings with constants.Replacing hardcoded cookie names with centralized constants from
CookieBuilderUtilimproves maintainability and reduces the risk of typos. This aligns well with the broader token management refactoring.src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
65-74: LGTM! Cookie creation method updated correctly.The change from
createCookietocreateLocalCookieStringaligns with the CookieBuilderUtil refactoring for environment-specific cookie creation methods.src/main/java/life/mosu/mosuserver/application/auth/provider/RefreshTokenProvider.java (3)
17-17: Good refactoring: field ordering improved.Moving the instance field after static final fields follows Java coding conventions.
39-39: Improved error code semantics.Changing from
NOT_EXIST_REFRESH_TOKENtoNOT_FOUND_REFRESH_TOKENis more semantically accurate and follows standard HTTP status code naming conventions.
46-50: LGTM! Formatting improvement.The method parameter formatting is more readable with proper indentation.
src/main/java/life/mosu/mosuserver/global/handler/ReissueHandler.java (1)
8-12: Clean interface design following single responsibility principle.The
ReissueHandlerinterface provides a clear contract for handling successful token reissue events. The method signature appropriately includes the necessary HTTP components and token data, with proper exception handling for I/O operations.src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (1)
43-47: LGTM! Proper HTTP response handling.The response configuration correctly sets status code, content type, and character encoding before writing the JSON response.
src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java (2)
11-33: Excellent implementation with good performance optimization.The token resolution logic is well-implemented:
- Null-safe cookie handling
- Early break optimization when both tokens are found
- Clean return type using TokenCookies record
- Proper separation of concerns
21-24: Cookie Name Constants Match Across TokenCookies and CookieBuilderUtilVerified that both TokenCookies and CookieBuilderUtil define
• ACCESS_TOKEN_* = "accessToken"
• REFRESH_TOKEN_* = "refreshToken"
identically. No inconsistencies remain. Proceed as is.src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
27-34: Well-organized token error codesThe reorganization and addition of token-related error codes provide comprehensive coverage for the new token management flow. The generalized comment and new error codes (MISSING_SIGNUP_TOKEN, EXPIRED_REFRESH_TOKEN) align well with the enhanced authentication handling.
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (2)
30-57: LGTM! Appropriate local environment configurationThe local environment cookie configuration is suitable for development with HttpOnly flag for XSS protection and no Secure flag for HTTP testing.
153-170: Well-structured helper methodsThe private helper methods effectively implement DRY principle by centralizing common cookie attributes configuration.
| public Token reissueToken(final TokenCookies tokenCookies) { | ||
|
|
||
| final Authentication authentication = refreshTokenProvider.getAuthentication( | ||
| refreshTokenString); | ||
| tokenCookies.refreshToken()); |
There was a problem hiding this comment.
Potential NullPointerException when refresh token is null
The method directly uses tokenCookies.refreshToken() without checking if it's null. Since TokenCookies.refreshToken() returns a raw String that can be null, this could cause a NullPointerException when passed to refreshTokenProvider.getAuthentication().
Consider adding validation or ensure the caller validates token presence before invoking this method.
public Token reissueToken(final TokenCookies tokenCookies) {
+ if (tokenCookies.refreshToken() == null) {
+ throw new CustomRuntimeException(ErrorCode.NOT_FOUND_REFRESH_TOKEN);
+ }
final Authentication authentication = refreshTokenProvider.getAuthentication(
tokenCookies.refreshToken());📝 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 Token reissueToken(final TokenCookies tokenCookies) { | |
| final Authentication authentication = refreshTokenProvider.getAuthentication( | |
| refreshTokenString); | |
| tokenCookies.refreshToken()); | |
| public Token reissueToken(final TokenCookies tokenCookies) { | |
| if (tokenCookies.refreshToken() == null) { | |
| throw new CustomRuntimeException(ErrorCode.NOT_FOUND_REFRESH_TOKEN); | |
| } | |
| final Authentication authentication = refreshTokenProvider.getAuthentication( | |
| tokenCookies.refreshToken()); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/auth/provider/AuthTokenManager.java
around lines 28 to 31, the method reissueToken uses tokenCookies.refreshToken()
without null checking, which can cause a NullPointerException if the refresh
token is null. Add a null check for tokenCookies.refreshToken() before passing
it to refreshTokenProvider.getAuthentication(), and handle the null case
appropriately, such as throwing an exception or returning an error response.
| public record TokenCookies( | ||
| String accessToken, | ||
| String refreshToken | ||
| ) { | ||
|
|
||
| public static final String ACCESS_TOKEN_NAME = "accessToken"; | ||
| public static final String REFRESH_TOKEN_NAME = "refreshToken"; | ||
|
|
||
| public Optional<String> getAccessToken() { | ||
| return Optional.ofNullable(accessToken); | ||
| } | ||
|
|
||
| public Optional<String> getRefreshToken() { | ||
| return Optional.ofNullable(refreshToken); | ||
| } | ||
|
|
||
| public boolean availableReissue() { | ||
| return getAccessToken().isPresent() && getRefreshToken().isPresent(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent API design with nullable fields
The record exposes both raw nullable fields through auto-generated accessors (accessToken(), refreshToken()) and safe Optional getters. This dual API is confusing and can lead to NullPointerExceptions, as seen in AuthTokenManager which uses the raw refreshToken() accessor.
Consider either:
- Making the fields non-null with validation in a factory method
- Only exposing Optional-based getters by making the record fields private
Example approach using a factory method with validation:
public record TokenCookies(
String accessToken,
String refreshToken
) {
public static TokenCookies of(String accessToken, String refreshToken) {
return new TokenCookies(
Objects.requireNonNullElse(accessToken, ""),
Objects.requireNonNullElse(refreshToken, "")
);
}
public boolean hasAccessToken() {
return !accessToken.isEmpty();
}
public boolean hasRefreshToken() {
return !refreshToken.isEmpty();
}
}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/cookie/TokenCookies.java lines 5 to
24, the record exposes nullable fields directly via auto-generated accessors and
also provides Optional getters, causing inconsistent API usage and potential
NullPointerExceptions. To fix this, make the record fields private and remove
direct accessors, then provide only Optional-based getters or non-null validated
fields via a factory method that replaces nulls with empty strings. This ensures
consistent, safe access to tokens and prevents null-related errors.
| if (isSkippedUrl(requestUri)) { | ||
| final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); | ||
| String accessToken = tokenCookies.accessToken(); | ||
| log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken); | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Security concern: Avoid logging tokens for skipped URLs
Resolving and logging access tokens for skipped URLs is unnecessary and poses a security risk by exposing tokens in logs. Consider skipping token resolution entirely for these URLs.
if (isSkippedUrl(requestUri)) {
- final TokenCookies tokenCookies = tokenResolver.resolveTokens(request);
- String accessToken = tokenCookies.accessToken();
- log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken);
+ log.info("스킵된 URL 요청: {}", requestUri);
filterChain.doFilter(request, response);
return;
}📝 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.
| if (isSkippedUrl(requestUri)) { | |
| final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); | |
| String accessToken = tokenCookies.accessToken(); | |
| log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| if (isSkippedUrl(requestUri)) { | |
| log.info("스킵된 URL 요청: {}", requestUri); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java
around lines 62 to 68, avoid resolving and logging access tokens for skipped
URLs to prevent exposing sensitive token information in logs. Modify the code to
skip token resolution entirely when the URL is in the skip list, and remove the
log statement that outputs the token. Simply call filterChain.doFilter and
return for skipped URLs without accessing tokens.
| final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); | ||
| String accessToken = tokenCookies.getAccessToken().orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.NOT_FOUND_ACCESS_TOKEN) | ||
| ); | ||
|
|
||
| try { | ||
| log.info("액세스 토큰 인증 요청: {}", accessToken); | ||
| setAuthentication(accessToken); | ||
| } catch (Exception e) { | ||
| log.error("액세스 토큰 인증 실패: {}", e.getMessage()); | ||
| throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN); | ||
| } | ||
|
|
||
| filterChain.doFilter(request, response); |
There was a problem hiding this comment.
Security and error handling improvements needed
- Logging the access token (line 94) exposes sensitive information in logs
- Catching all exceptions (line 96) masks specific errors that could help with debugging
final TokenCookies tokenCookies = tokenResolver.resolveTokens(request);
String accessToken = tokenCookies.getAccessToken().orElseThrow(
() -> new CustomRuntimeException(ErrorCode.NOT_FOUND_ACCESS_TOKEN)
);
try {
- log.info("액세스 토큰 인증 요청: {}", accessToken);
+ log.info("액세스 토큰 인증 요청");
setAuthentication(accessToken);
-} catch (Exception e) {
- log.error("액세스 토큰 인증 실패: {}", e.getMessage());
- throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN);
+} catch (CustomRuntimeException e) {
+ log.error("액세스 토큰 인증 실패: {}", e.getMessage());
+ throw e;
+} catch (Exception e) {
+ log.error("액세스 토큰 인증 중 예상치 못한 오류", e);
+ throw new CustomRuntimeException(ErrorCode.SERVER_ERROR);
}📝 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.
| final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); | |
| String accessToken = tokenCookies.getAccessToken().orElseThrow( | |
| () -> new CustomRuntimeException(ErrorCode.NOT_FOUND_ACCESS_TOKEN) | |
| ); | |
| try { | |
| log.info("액세스 토큰 인증 요청: {}", accessToken); | |
| setAuthentication(accessToken); | |
| } catch (Exception e) { | |
| log.error("액세스 토큰 인증 실패: {}", e.getMessage()); | |
| throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN); | |
| } | |
| filterChain.doFilter(request, response); | |
| final TokenCookies tokenCookies = tokenResolver.resolveTokens(request); | |
| String accessToken = tokenCookies.getAccessToken().orElseThrow( | |
| () -> new CustomRuntimeException(ErrorCode.NOT_FOUND_ACCESS_TOKEN) | |
| ); | |
| try { | |
| log.info("액세스 토큰 인증 요청"); | |
| setAuthentication(accessToken); | |
| } catch (CustomRuntimeException e) { | |
| log.error("액세스 토큰 인증 실패: {}", e.getMessage()); | |
| throw e; | |
| } catch (Exception e) { | |
| log.error("액세스 토큰 인증 중 예상치 못한 오류", e); | |
| throw new CustomRuntimeException(ErrorCode.SERVER_ERROR); | |
| } | |
| filterChain.doFilter(request, response); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java
around lines 88 to 101, remove logging of the raw access token to avoid exposing
sensitive information by either omitting the token value or logging only a
masked or hashed version. Additionally, replace the broad catch of Exception
with more specific exception types related to token validation to improve error
handling and debugging clarity.
| /** | ||
| * [운영 환경용] ResponseCookie 객체를 생성합니다. | ||
| * TODO: 운영 배포 시, 최종 도메인 및 SameSite 정책을 확정해야 합니다. | ||
| * | ||
| * @param name 쿠키의 이름 | ||
| * @param value 쿠키에 저장될 값 | ||
| * @param maxAge 쿠키의 만료 시간 (단위: 초) | ||
| * @return ResponseCookie 객체 | ||
| */ | ||
| public static ResponseCookie createProductionResponseCookie(String name, String value, | ||
| Long maxAge) { | ||
| return createBaseResponseCookieBuilder(name, value, maxAge) | ||
| .secure(true) | ||
| .domain(".mosuedu.com") // TODO: 운영 도메인으로 변경 | ||
| .sameSite("None") // TODO: 운영 정책에 맞게 Strict 또는 Lax로 변경 고려 | ||
| .build(); | ||
| } | ||
|
|
||
| /** | ||
| * [운영 환경용] jakarta.servlet.Cookie 객체를 생성합니다. | ||
| * TODO: 운영 배포 시, 최종 도메인 및 SameSite 정책을 확정해야 합니다. | ||
| * | ||
| * @param name 쿠키의 이름 | ||
| * @param value 쿠키에 저장될 값 | ||
| * @param maxAge 쿠키의 만료 시간 (단위: 초) | ||
| * @return jakarta.servlet.Cookie 객체 | ||
| */ | ||
| public static Cookie createProductionCookie(String name, String value, Long maxAge) { | ||
| Cookie cookie = createBaseServletCookie(name, value, maxAge); | ||
| cookie.setSecure(true); | ||
| cookie.setDomain("api.mosuedu.com"); // TODO: 운영 도메인으로 변경 | ||
| return cookie; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Address production configuration TODOs and domain inconsistency
- The TODO comments indicate production configuration is not finalized
- Domain inconsistency: ResponseCookie uses
.mosuedu.com(line 116) while servlet Cookie usesapi.mosuedu.com(line 133)
The domain configuration should be consistent. Also, ensure the production domain and SameSite policy are finalized before deployment.
public static ResponseCookie createProductionResponseCookie(String name, String value,
Long maxAge) {
return createBaseResponseCookieBuilder(name, value, maxAge)
.secure(true)
- .domain(".mosuedu.com") // TODO: 운영 도메인으로 변경
+ .domain("api.mosuedu.com") // TODO: 운영 도메인으로 변경
.sameSite("None") // TODO: 운영 정책에 맞게 Strict 또는 Lax로 변경 고려
.build();
}Unify production cookie domains and finalize TODOs
There’s an inconsistency in the production cookie methods and lingering TODOs that must be resolved before deployment:
- In
createProductionResponseCookie(src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java:~116), the domain is set to.mosuedu.com - In
createProductionCookie(same file:~133), the domain is set toapi.mosuedu.com - Both methods still have TODOs for the domain and SameSite policy
Please confirm the intended production domain (e.g. api.mosuedu.com vs .mosuedu.com) and the final SameSite setting (None, Lax, or Strict), then update both methods accordingly:
public static ResponseCookie createProductionResponseCookie(String name, String value, Long maxAge) {
return createBaseResponseCookieBuilder(name, value, maxAge)
.secure(true)
- .domain(".mosuedu.com") // TODO: 운영 도메인으로 변경
+ .domain("api.mosuedu.com") // 운영 도메인으로 확정
- .sameSite("None") // TODO: 운영 정책에 맞게 Strict 또는 Lax로 변경 고려
+ .sameSite("None") // SameSite 정책을 최종 확정 (예: Lax 또는 Strict)
.build();
}
public static Cookie createProductionCookie(String name, String value, Long maxAge) {
Cookie cookie = createBaseServletCookie(name, value, maxAge);
cookie.setSecure(true);
- cookie.setDomain("api.mosuedu.com"); // TODO: 운영 도메인으로 변경
+ cookie.setDomain("api.mosuedu.com"); // 운영 도메인으로 확정
return cookie;
}- Ensure both methods use the same domain scope.
- Remove or replace all TODOs once settings are finalized.
📝 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.
| /** | |
| * [운영 환경용] ResponseCookie 객체를 생성합니다. | |
| * TODO: 운영 배포 시, 최종 도메인 및 SameSite 정책을 확정해야 합니다. | |
| * | |
| * @param name 쿠키의 이름 | |
| * @param value 쿠키에 저장될 값 | |
| * @param maxAge 쿠키의 만료 시간 (단위: 초) | |
| * @return ResponseCookie 객체 | |
| */ | |
| public static ResponseCookie createProductionResponseCookie(String name, String value, | |
| Long maxAge) { | |
| return createBaseResponseCookieBuilder(name, value, maxAge) | |
| .secure(true) | |
| .domain(".mosuedu.com") // TODO: 운영 도메인으로 변경 | |
| .sameSite("None") // TODO: 운영 정책에 맞게 Strict 또는 Lax로 변경 고려 | |
| .build(); | |
| } | |
| /** | |
| * [운영 환경용] jakarta.servlet.Cookie 객체를 생성합니다. | |
| * TODO: 운영 배포 시, 최종 도메인 및 SameSite 정책을 확정해야 합니다. | |
| * | |
| * @param name 쿠키의 이름 | |
| * @param value 쿠키에 저장될 값 | |
| * @param maxAge 쿠키의 만료 시간 (단위: 초) | |
| * @return jakarta.servlet.Cookie 객체 | |
| */ | |
| public static Cookie createProductionCookie(String name, String value, Long maxAge) { | |
| Cookie cookie = createBaseServletCookie(name, value, maxAge); | |
| cookie.setSecure(true); | |
| cookie.setDomain("api.mosuedu.com"); // TODO: 운영 도메인으로 변경 | |
| return cookie; | |
| } | |
| public static ResponseCookie createProductionResponseCookie(String name, String value, | |
| Long maxAge) { | |
| return createBaseResponseCookieBuilder(name, value, maxAge) | |
| .secure(true) | |
| .domain("api.mosuedu.com") // 운영 도메인으로 확정 | |
| .sameSite("None") // SameSite 정책을 최종 확정 (예: Lax 또는 Strict) | |
| .build(); | |
| } | |
| public static Cookie createProductionCookie(String name, String value, Long maxAge) { | |
| Cookie cookie = createBaseServletCookie(name, value, maxAge); | |
| cookie.setSecure(true); | |
| cookie.setDomain("api.mosuedu.com"); // 운영 도메인으로 확정 | |
| return cookie; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java around
lines 103 to 135, unify the domain used in both createProductionResponseCookie
and createProductionCookie methods by deciding on a single production domain
(either ".mosuedu.com" or "api.mosuedu.com") and update both methods to use this
domain consistently. Also finalize the SameSite policy in
createProductionResponseCookie by replacing the TODO comment with the chosen
value (None, Lax, or Strict). After confirming these settings, remove all TODO
comments related to domain and SameSite policy in both methods.
| "/api/v1/event", | ||
| "/api/v1/faq", | ||
| "/api/v1/notice" | ||
| ); |
There was a problem hiding this comment.
whiteList 레코드 객체를 만들어서, List 이 아니라 List 로 선언해서 uri랑 method 를 같이 저장하도록 하면 좋을 거 같아요
| if (requestUri.startsWith("/api/v1/auth/signup")) { | ||
| log.info("회원가입 토큰 검증 요청: {}", requestUri); | ||
|
|
||
| String signupToken = resolveBearerTokenFromHeader(request).orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.MISSING_SIGNUP_TOKEN) | ||
| ); |
| Map<String, Object> responseBody = new HashMap<>(); | ||
| responseBody.put("status", HttpStatus.OK.value()); | ||
| responseBody.put("message", "reissue를 성공했습니다."); | ||
|
|
3 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
1. 엔드포인트로 토큰 추출 여부를 판단했는데, 엔드포인트는 같은데 HTTP 메서드가 다른 부분 인증 처리의 고민
Filter쪽에서event나Inquiry에서 등록 로직에 대한 엔드포인트 분기를 어떻게 할지 고민입니다.2. Handler 분리를 했는데, SuccessHandler로 더 분리할지 말지에 대한 고민
/auth/reissue에서response를 처리하는 로직을handler로 분리하였습니다.handler에서 쿠키의reissue와responseJSON 직렬화를 처리했는데,SuccessHandler도 만들어서responseJSON 직렬화 로직을 분리할지 지금 그대로 할지 고민입니다.🎸 기타
MOSU전용 쿠키 유틸을 만들었습니다! 앞으로 쿠키 다룰때 사용해주세요!Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style/Refactor