Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughJwt 예외 처리를 통합했습니다: JwtTokenProvider는 검증 오류로 GlobalException을 던지도록 변경되고, JwtTokenAuthenticationFilter는 예외를 BaseErrorResponse로 매핑하여 응답을 반환하며, EXPIRED_TOKEN의 HTTP 상태 코드가 FORBIDDEN으로 변경되었습니다. GlobalExceptionControllerAdvice에 import가 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Filter as JwtTokenAuthenticationFilter
participant Provider as JwtTokenProvider
participant Response as ResponseWriter
Client->>Filter: HTTP Request (JWT)
Filter->>Provider: validateToken(jwt)
alt 토큰 유효
Provider-->>Filter: Claims
Filter->>Filter: 인증 생성 및 SecurityContext 설정
Filter-->>Client: 다음 필터 체인 진행
else 토큰 오류(만료/형식/기타)
Provider-->>Filter: throw GlobalException
Filter->>Filter: resolveTokenException() -> BaseExceptionResponseStatus
Filter->>Response: setStatus(SC_OK) 및 write(BaseErrorResponse)
Response-->>Client: JSON 에러 응답
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 483f436. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java (1)
29-45:⚠️ Potential issue | 🔴 Critical
doFilter의 예외 포착 범위가 너무 넓어 비JWT 예외까지 토큰 오류로 변환됩니다.Line [44]의
chain.doFilter가try내부라서, 하위 레이어 예외도 Line [45]에서 잡혀 JWT 오류 응답으로 바뀔 수 있습니다. JWT 처리 예외만 잡고 나머지는 전파해야 합니다.🔧 제안 수정안
+import ku_rum.backend.global.exception.global.GlobalException; @@ - try { + try { StringBuffer path = ((HttpServletRequest) request).getRequestURL(); @@ if (token != null && jwtTokenProvider.validateToken(token) && isNotLogout(token)) { Authentication authentication = jwtTokenProvider.getAuthentication(token); SecurityContextHolder.getContext().setAuthentication(authentication); } - chain.doFilter(request, response); - } catch (Exception e) { + } catch (GlobalException e) { HttpServletResponse httpServletResponse = (HttpServletResponse) response; httpServletResponse.setContentType("application/json;charset=UTF-8"); @@ BaseErrorResponse errorResponse = new BaseErrorResponse(status); httpServletResponse.getWriter().write(new ObjectMapper().writeValueAsString(errorResponse)); + return; } + chain.doFilter(request, response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java` around lines 29 - 45, In JwtTokenAuthenticationFilter.doFilter, the try/catch currently wraps the entire filter including chain.doFilter causing non-JWT exceptions to be converted into token errors; narrow the scope so only JWT-specific operations (resolveToken, jwtTokenProvider.validateToken, isNotLogout, jwtTokenProvider.getAuthentication and SecurityContextHolder.setAuthentication) are wrapped and catch only JWT-related exceptions (e.g., JwtException or your provider's specific exception), then allow other exceptions thrown by chain.doFilter(request, response) to propagate (or rethrow them) instead of converting them to token errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java`:
- Around line 49-50: The filter JwtTokenAuthenticationFilter currently overrides
the HTTP status to HttpServletResponse.SC_OK in the exception path; change the
logic that handles exceptions from resolveTokenException(e) so that the response
uses the HTTP status from the returned BaseExceptionResponseStatus (e.g.,
status.getHttpStatus()/getStatusCode()) rather than hard-coding 200, and ensure
httpServletResponse.setStatus(...) is called with that status value so
EXPIRED_TOKEN(403) and other statuses are preserved.
- Around line 57-69: The resolveTokenException method currently parses exception
messages; instead, replace that logic with instanceof checks: if e is null
return JWT_ERROR; if e instanceof GlobalException return the embedded
BaseExceptionResponseStatus from the GlobalException (e.g., call its accessor to
retrieve the status); if e instanceof ExpiredJwtException (or JwtException
subtype representing expiration) return EXPIRED_TOKEN; if e instanceof
MalformedJwtException return MALFORMED_TOKEN; otherwise return INVALID_TOKEN.
Update resolveTokenException and ensure JwtTokenProvider.validateToken()’s
thrown GlobalException is handled via instanceof rather than message parsing.
In `@src/main/java/ku_rum/backend/global/security/JwtTokenProvider.java`:
- Around line 86-89: In JwtTokenProvider replace the current instanceof
SecurityException check with handling for
io.jsonwebtoken.security.SignatureException (or add an explicit catch for
SignatureException) so signature verification failures are mapped to
BaseExceptionResponseStatus.INVALID_TOKEN; specifically detect
SignatureException (e.g., if (e instanceof SignatureException)), log a clear
"[SignatureException] 잘못된 토큰" message and throw new
GlobalException(BaseExceptionResponseStatus.INVALID_TOKEN) just like the
intended SecurityException handling.
---
Outside diff comments:
In
`@src/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java`:
- Around line 29-45: In JwtTokenAuthenticationFilter.doFilter, the try/catch
currently wraps the entire filter including chain.doFilter causing non-JWT
exceptions to be converted into token errors; narrow the scope so only
JWT-specific operations (resolveToken, jwtTokenProvider.validateToken,
isNotLogout, jwtTokenProvider.getAuthentication and
SecurityContextHolder.setAuthentication) are wrapped and catch only JWT-related
exceptions (e.g., JwtException or your provider's specific exception), then
allow other exceptions thrown by chain.doFilter(request, response) to propagate
(or rethrow them) instead of converting them to token errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ku_rum/backend/global/handler/GlobalExceptionControllerAdvice.javasrc/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.javasrc/main/java/ku_rum/backend/global/security/JwtTokenProvider.javasrc/main/java/ku_rum/backend/global/support/status/BaseExceptionResponseStatus.java
src/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java
Show resolved
Hide resolved
src/main/java/ku_rum/backend/global/security/JwtTokenAuthenticationFilter.java
Show resolved
Hide resolved
| if (e instanceof SecurityException) { | ||
| log.debug("[SecurityException] 잘못된 토큰"); | ||
| throw new JwtException("[SecurityException] 잘못된 토큰입니다."); | ||
| throw new GlobalException(BaseExceptionResponseStatus.INVALID_TOKEN); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "JwtTokenProvider.java" -type fRepository: KU-rum/backend
Length of output: 126
🏁 Script executed:
cat -n src/main/java/ku_rum/backend/global/security/JwtTokenProvider.javaRepository: KU-rum/backend
Length of output: 8550
🌐 Web query:
JJWT 0.12.3 parseSignedClaims exception handling signature validation
💡 Result:
In JJWT 0.12.3, JwtParser.parseSignedClaims(...) is the “I expect a signed JWS with Claims” entry point, and it performs signature verification as part of parsing (assuming you configured a verification key/locator).
Correct usage (signature is validated)
try {
Jws<Claims> jws = Jwts.parser()
.verifyWith(key) // SecretKey or PublicKey
.build()
.parseSignedClaims(token);
Claims claims = jws.getPayload(); // only reached if signature + validations passed
} catch (JwtException e) {
// invalid / untrusted token (includes bad signature, expired, malformed, etc.)
}The project README explicitly notes that if signature verification fails, SignatureException (a JwtException) is thrown. [1]
What to catch / how to distinguish failures
parseSignedClaims(CharSequence) in 0.12.3 declares:
throws JwtException(umbrella for parse/validation problems, including signature failure)throws IllegalArgumentException(null/empty/blank input)
and documentsUnsupportedJwtExceptionwhen the input is not a signed Claims JWT. [2]
So typically:
catch (SignatureException e) { /* signature invalid */ }
catch (ExpiredJwtException e) { /* exp in past */ }
catch (UnsupportedJwtException e) { /* not a signed Claims JWS (wrong token type/method) */ }
catch (MalformedJwtException e) { /* bad compact serialization / invalid structure */ }
catch (SecurityException e) { /* actually a JWE and decryption failed (when using parse(..) variants) */ }
catch (JwtException e) { /* anything else JWT-related */ }
catch (IllegalArgumentException e) { /* null/blank token */ }(Those specific exception types are the standard JJWT parse/validate failures surfaced by the parser APIs; parse(CharSequence) lists them explicitly, while parseSignedClaims groups them under JwtException.) [2]
Why parseSignedClaims (vs parse)
parseSignedClaimswill throwUnsupportedJwtExceptionif the token isn’t a signed Claims JWS, which helps avoid accidentally accepting an unexpected token kind. [2]- A long-standing concern is that the generic
parse(...)can successfully parse tokens that are not signed (depending on configuration/what’s encountered), so using the more specific parse method is safer when you require a signature. [3]
Sources:
[1] JJWT GitHub README (signature verification + SignatureException guidance)
[2] JJWT 0.12.3 Javadoc: JwtParser.parseSignedClaims and parser exception contracts
[3] JJWT GitHub issue #775 discussion about parse behavior and preferring signed-parse methods
🌐 Web query:
JJWT 0.12.3 SignatureException vs SecurityException signature verification parseSignedClaims
💡 Result:
In JJWT 0.12.3, parseSignedClaims(...) is for JWS (signed) JWTs. When something goes wrong during verification/parsing, you’ll typically see one of these (both are JwtExceptions):
io.jsonwebtoken.security.SignatureException
Thrown when a JWS signature is present but cannot be verified (e.g., wrong key, token was tampered with, algorithm/key mismatch for verification). The JJWT README explicitly calls out that signature verification failure during parseSignedClaims results in a SignatureException. [1]
io.jsonwebtoken.security.SecurityException
A broader “crypto/security” JwtException category: problems with security-related elements like keys/algorithms/JCA provider, etc. (i.e., not necessarily “the signature doesn’t match”, but “something security/crypto-related failed”). [2]
Also, in the parser API docs, SecurityException is explicitly used when the compact token is actually a JWE (encrypted JWT) and decryption fails. [3]
Practical guidance
- If you only care “token signature invalid vs everything else”, catch:
SignatureException→ treat as bad/forged tokenJwtException(orSecurityException) → treat as other invalid token / crypto config problem
- If you unexpectedly see
SecurityExceptionwhile you believe you’re parsing a JWS, double-check you’re not receiving a JWE (encrypted token) or misconfigured crypto/provider. [3]
Sources: JJWT README (verification + parseSignedClaims) [1], JJWT security package summary (exception meanings) [2], JJWT 0.12.3 JwtParser javadoc (SecurityException for JWE decrypt failure) [3].
서명 검증 실패 예외가 INVALID_TOKEN으로 매핑되지 않습니다.
Line 86의 SecurityException은 JJWT 0.12.3에서 서명 검증 실패 시 발생하는 io.jsonwebtoken.security.SignatureException을 받지 못합니다. 현재 코드는 서명 오류를 처리하지 않아 JWT_ERROR로 폴(fall-through)됩니다.
🔧 제안 수정안
+import io.jsonwebtoken.security.SignatureException;
} catch (Exception e) {
+ if (e instanceof SignatureException) {
+ log.debug("[SignatureException] 잘못된 토큰");
+ throw new GlobalException(BaseExceptionResponseStatus.INVALID_TOKEN);
+
+ } else if (e instanceof SecurityException) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/ku_rum/backend/global/security/JwtTokenProvider.java` around
lines 86 - 89, In JwtTokenProvider replace the current instanceof
SecurityException check with handling for
io.jsonwebtoken.security.SignatureException (or add an explicit catch for
SignatureException) so signature verification failures are mapped to
BaseExceptionResponseStatus.INVALID_TOKEN; specifically detect
SignatureException (e.g., if (e instanceof SignatureException)), log a clear
"[SignatureException] 잘못된 토큰" message and throw new
GlobalException(BaseExceptionResponseStatus.INVALID_TOKEN) just like the
intended SecurityException handling.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ku_rum/backend/global/security/jwt/JwtTokenProviderTest.java (1)
145-147:⚠️ Potential issue | 🟠 MajorJWT 관련 예외 타입 불일치:
createToken과validateToken의 예외 통일이 필요합니다.현재 코드에서 JWT 관련 메서드들이 서로 다른 예외 타입을 던지고 있습니다:
validateToken(): 모든 토큰 검증 실패 시GlobalException사용 (INVALID_TOKEN, MALFORMED_TOKEN, EXPIRED_TOKEN 등)createToken(): Redis 저장 실패 시JwtException사용두 메서드 모두 JWT 작업 실패이므로, 예외 타입을 통일하여
GlobalException으로 정렬하거나, 명시적인 예외 전략을 문서화해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ku_rum/backend/global/security/jwt/JwtTokenProviderTest.java` around lines 145 - 147, The test and implementation are inconsistent: createToken throws JwtException while validateToken uses GlobalException; unify them by changing createToken to throw GlobalException on Redis save failures (use the same INVALID_TOKEN / relevant enum or create a new JWT-related GlobalError) and update JwtTokenProviderTest (the assertion on createToken) to expect GlobalException with the appropriate message ("레디스에 리프레시 토큰 저장 실패"); locate createToken and its Redis-save error handling and replace JwtException usage with GlobalException construction consistent with validateToken's pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/ku_rum/backend/global/security/jwt/JwtTokenProviderTest.java`:
- Around line 112-113: Update the assertion in JwtTokenProviderTest so it not
only checks that jwtTokenProvider.validateToken(expiredToken) throws
GlobalException but also asserts the exception's error identifier equals the JWT
expiration code (e.g., EXPIRED_TOKEN or the corresponding error code/property).
Capture the thrown GlobalException from the validateToken call, then assert on
its errorCode/errorStatus/getErrorId (whichever accessor exists) to match the
expected EXPIRED_TOKEN constant to ensure the test verifies both type and
specific error identity.
---
Outside diff comments:
In `@src/test/java/ku_rum/backend/global/security/jwt/JwtTokenProviderTest.java`:
- Around line 145-147: The test and implementation are inconsistent: createToken
throws JwtException while validateToken uses GlobalException; unify them by
changing createToken to throw GlobalException on Redis save failures (use the
same INVALID_TOKEN / relevant enum or create a new JWT-related GlobalError) and
update JwtTokenProviderTest (the assertion on createToken) to expect
GlobalException with the appropriate message ("레디스에 리프레시 토큰 저장 실패"); locate
createToken and its Redis-save error handling and replace JwtException usage
with GlobalException construction consistent with validateToken's pattern.
| assertThatThrownBy(() -> jwtTokenProvider.validateToken(expiredToken)) | ||
| .isInstanceOf(JwtException.class); | ||
| .isInstanceOf(GlobalException.class); |
There was a problem hiding this comment.
만료 토큰 케이스는 예외 “타입” 외에 에러 식별자까지 검증해 주세요.
현재는 GlobalException 여부만 확인해서, 다른 원인의 GlobalException도 통과할 수 있습니다. JWT 예외 통일 목적에 맞게 EXPIRED_TOKEN(또는 대응되는 에러 코드/상태)까지 함께 검증하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/ku_rum/backend/global/security/jwt/JwtTokenProviderTest.java`
around lines 112 - 113, Update the assertion in JwtTokenProviderTest so it not
only checks that jwtTokenProvider.validateToken(expiredToken) throws
GlobalException but also asserts the exception's error identifier equals the JWT
expiration code (e.g., EXPIRED_TOKEN or the corresponding error code/property).
Capture the thrown GlobalException from the validateToken call, then assert on
its errorCode/errorStatus/getErrorId (whichever accessor exists) to match the
expected EXPIRED_TOKEN constant to ensure the test verifies both type and
specific error identity.
|
1 participant
#️⃣ 연관된 이슈
closes #448
📝작업 내용
작업 상세 내용
상세 내용을 입력해주세요.
💬리뷰 요구사항
Summary by CodeRabbit
릴리스 노트
Bug Fixes
Refactor
Tests
Chores