feat: custom exception 추가#10
Open
jhlarry1109 wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthrough커스텀 예외 계층과 전역 예외 처리기가 추가되고, 서비스 검증·토큰·다운스트림 클라이언트의 예외 타입이 변경되었다. API 스펙과 문서가 이에 맞게 갱신되며, ECS 배포용 GitHub Actions 워크플로도 추가되었다. Changes커스텀 예외 처리 인프라 및 서비스 통합
ECS 배포 CI/CD 워크플로
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (3)
.github/workflows/deploy.yml (3)
15-15: ⚡ Quick win배포 추적성 개선:
latest대신 고유 태그 사용 권장
IMAGE_TAG: latest를 사용하면 어떤 커밋이 배포되었는지 추적이 어렵고, 롤백 시 이전 이미지를 식별할 수 없습니다. Git SHA나 타임스탬프 기반 태그를 권장합니다.♻️ Git SHA 기반 태그 사용 예시
env: AWS_REGION: ap-northeast-2 AWS_ACCOUNT_ID: 727452759104 ECR_REPOSITORY: momentlit/auth ECS_CLUSTER: default ECS_SERVICE: momentlit-auth-service - IMAGE_TAG: latest + IMAGE_TAG: ${{ github.sha }}또는
latest와 SHA 태그를 함께 푸시하여 추적성과 편의성을 모두 확보할 수 있습니다:- name: Build and push Docker image run: | IMAGE_URI_BASE=${AWS_ACCOUNT_ID}.dkr.ecr.${AWS_REGION}.amazonaws.com/${ECR_REPOSITORY} docker buildx build \ --platform linux/amd64 \ --provenance=false \ -t ${IMAGE_URI_BASE}:${{ github.sha }} \ -t ${IMAGE_URI_BASE}:latest \ . \ --push🤖 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 @.github/workflows/deploy.yml at line 15, The IMAGE_TAG environment variable is set to "latest" which does not provide deployment traceability since multiple commits will all point to the same tag, making it impossible to identify which specific commit was deployed or to properly rollback. Replace the IMAGE_TAG environment variable with a unique identifier such as github.sha (the commit hash). Additionally, when building and pushing the Docker image in the docker buildx build step, tag the image with both the unique SHA-based tag and the "latest" tag to maintain both traceability for debugging and a convenient latest reference for quick deployments.
58-63: 💤 Low value타임아웃 및 실패 처리 고려
aws ecs wait services-stable은 기본적으로 최대 40분(40회 폴링 × 60초)까지 대기합니다. 배포 실패 시 긴 대기 후에야 워크플로가 실패하므로, 적절한 타임아웃 설정을 고려해 주세요.⏱️ 타임아웃 설정 예시
- name: Wait for ECS service stable + timeout-minutes: 15 run: | aws ecs wait services-stable \ --cluster $ECS_CLUSTER \ --services $ECS_SERVICE \ --region $AWS_REGION🤖 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 @.github/workflows/deploy.yml around lines 58 - 63, The aws ecs wait services-stable command has a default maximum wait time of 40 minutes, which means the workflow will take a long time to fail if the ECS service fails to stabilize. Add a timeout-minutes property to the "Wait for ECS service stable" step to set a reasonable timeout limit (e.g., 10-15 minutes), ensuring the workflow fails faster in case of deployment issues. This can be added at the same indentation level as the run property in the YAML step definition.
24-37: 공급망 보안: GitHub Actions를 SHA 해시로 고정하기현재 모든 GitHub Actions가 버전 태그(
@v4,@v5등)로 참조되어 있습니다. 악의적인 태그 변조에 대비해 다음과 같이 SHA 해시로 고정하는 것이 보안 모범 사례입니다:
actions/checkout@v4→actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5aws-actions/configure-aws-credentials@v5→aws-actions/configure-aws-credentials@61815dcd50bd041e203e49132bacad1fd04d2708aws-actions/amazon-ecr-login@v2→aws-actions/amazon-ecr-login@d539f0932e70871a027e9d5a9d8fc38589180a64docker/setup-buildx-action@v3→docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f🤖 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 @.github/workflows/deploy.yml around lines 24 - 37, The GitHub Actions in the deploy workflow are currently pinned to version tags (such as `@v4`, `@v5`, `@v2`, `@v3`) which are vulnerable to tag tampering attacks. Replace all version tag references with their corresponding SHA hash values for supply chain security: update actions/checkout from `@v4` to the provided SHA hash, update aws-actions/configure-aws-credentials from `@v5` to its SHA hash, update aws-actions/amazon-ecr-login from `@v2` to its SHA hash, and update docker/setup-buildx-action from `@v3` to its SHA hash. This ensures the workflow uses immutable commit references instead of mutable tags.Source: Linters/SAST tools
🤖 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 @.github/workflows/deploy.yml:
- Around line 17-24: The GitHub Actions workflow needs security hardening to
follow the principle of least privilege. Add a top-level permissions block to
the workflow (at the same level as the jobs section) to explicitly declare
minimal required permissions, and then add persist-credentials: false parameter
to the checkout@v4 step to prevent Git credentials from being stored in the
runner environment. This ensures the workflow only requests and retains the
minimum permissions necessary for the build and deploy operation.
In `@API_SPEC.yaml`:
- Around line 25-61: The error response examples in the responses section
(BadRequestError, UnauthorizedError, GoogleOauthError, InternalServerError)
contain error codes embedded in the message field as prefixes like "[ERROR:
Request/BadRequest]". Extract these error code prefixes from the message field
and create a separate `code` field for each error response example. Update
BadRequestError to have code "REQUEST_BAD_REQUEST", UnauthorizedError to have
code "AUTH_UNAUTHORIZED", GoogleOauthError to have code "AUTH_OAUTH_GOOGLE", and
InternalServerError to have an appropriate code value. Keep only the Korean
error message text (without the prefix) in the message field for each example.
- Around line 13-24: The ErrorResponse schema in API_SPEC.yaml violates the
coding guideline requiring error responses to have both 'code' and 'message'
fields as separate properties. Currently, the error code is embedded as a string
prefix in the 'message' field (e.g., "[ERROR: Request/BadRequest]"), forcing
clients to parse strings instead of using structured data. Add a new 'code'
property to the ErrorResponse schema as a string type, remove the error code
prefix from the message example, and update the 'required' array to include both
'code' and 'message'. Additionally, ensure that the ApiResponse DTO class and
GlobalExceptionHandler's response creation logic are modified to populate the
separate 'code' and 'message' fields independently rather than combining them
into a single message string.
In `@src/main/java/com/example/auth/global/dto/ApiResponse.java`:
- Around line 3-9: The ApiResponse record currently lacks a code field, and the
fail method only accepts a message parameter with error codes embedded as string
prefixes. Add a code field to the ApiResponse record to store error codes
separately, then update the fail method signature to accept both code and
message as distinct parameters. This will ensure error responses follow a
consistent structure with properly separated code and message fields, allowing
clients to parse error codes structurally rather than relying on string parsing.
In `@src/main/java/com/example/auth/service/TokenService.java`:
- Around line 52-56: Remove the unnecessary `IllegalArgumentException` from the
catch block in the TokenService token validation logic. The
`jwtProvider.validateToken()` method only throws `JwtException` and its
subclasses through JJWT's parseSignedClaims() method, so catching
`IllegalArgumentException` is redundant and can mask actual bugs. Change the
catch clause to only catch `JwtException`, keeping the same
UnauthorizedException throw statement that handles the validation failure.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Line 15: The IMAGE_TAG environment variable is set to "latest" which does not
provide deployment traceability since multiple commits will all point to the
same tag, making it impossible to identify which specific commit was deployed or
to properly rollback. Replace the IMAGE_TAG environment variable with a unique
identifier such as github.sha (the commit hash). Additionally, when building and
pushing the Docker image in the docker buildx build step, tag the image with
both the unique SHA-based tag and the "latest" tag to maintain both traceability
for debugging and a convenient latest reference for quick deployments.
- Around line 58-63: The aws ecs wait services-stable command has a default
maximum wait time of 40 minutes, which means the workflow will take a long time
to fail if the ECS service fails to stabilize. Add a timeout-minutes property to
the "Wait for ECS service stable" step to set a reasonable timeout limit (e.g.,
10-15 minutes), ensuring the workflow fails faster in case of deployment issues.
This can be added at the same indentation level as the run property in the YAML
step definition.
- Around line 24-37: The GitHub Actions in the deploy workflow are currently
pinned to version tags (such as `@v4`, `@v5`, `@v2`, `@v3`) which are vulnerable to tag
tampering attacks. Replace all version tag references with their corresponding
SHA hash values for supply chain security: update actions/checkout from `@v4` to
the provided SHA hash, update aws-actions/configure-aws-credentials from `@v5` to
its SHA hash, update aws-actions/amazon-ecr-login from `@v2` to its SHA hash, and
update docker/setup-buildx-action from `@v3` to its SHA hash. This ensures the
workflow uses immutable commit references instead of mutable tags.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cd01533e-96ae-4e6b-bfb4-f1198e0debf5
📒 Files selected for processing (14)
.github/workflows/deploy.ymlAPI_SPEC.yamldocs/service-overview.mddocs/service-policy.mdsrc/main/java/com/example/auth/global/dto/ApiResponse.javasrc/main/java/com/example/auth/global/exception/AuthException.javasrc/main/java/com/example/auth/global/exception/BadRequestException.javasrc/main/java/com/example/auth/global/exception/GlobalExceptionHandler.javasrc/main/java/com/example/auth/global/exception/GoogleOauthException.javasrc/main/java/com/example/auth/global/exception/TokenNotFoundException.javasrc/main/java/com/example/auth/global/exception/UnauthorizedException.javasrc/main/java/com/example/auth/service/AuthService.javasrc/main/java/com/example/auth/service/AuthValidator.javasrc/main/java/com/example/auth/service/TokenService.java
Comment on lines
+17
to
+24
| jobs: | ||
| deploy: | ||
| name: Build and Deploy Auth | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout source code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
보안 강화: permissions 블록 추가 및 persist-credentials: false 설정 필요
정적 분석에서 지적한 대로, 최소 권한 원칙을 위해 명시적 권한 블록을 추가하고, checkout 단계에서 자격 증명 지속을 비활성화해야 합니다.
🛡️ 권장 수정 사항
jobs:
deploy:
name: Build and Deploy Auth
runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ id-token: write
steps:
- name: Checkout source code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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
| jobs: | |
| deploy: | |
| name: Build and Deploy Auth | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout source code | |
| uses: actions/checkout@v4 | |
| jobs: | |
| deploy: | |
| name: Build and Deploy Auth | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| id-token: write | |
| steps: | |
| - name: Checkout source code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/deploy.yml around lines 17 - 24, The GitHub Actions
workflow needs security hardening to follow the principle of least privilege.
Add a top-level permissions block to the workflow (at the same level as the jobs
section) to explicitly declare minimal required permissions, and then add
persist-credentials: false parameter to the checkout@v4 step to prevent Git
credentials from being stored in the runner environment. This ensures the
workflow only requests and retains the minimum permissions necessary for the
build and deploy operation.
Source: Linters/SAST tools
Comment on lines
+13
to
+24
| ErrorResponse: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: "[ERROR: Request/BadRequest] Refresh Token을 입력해주세요." | ||
| data: | ||
| nullable: true | ||
| example: null | ||
| required: | ||
| - message | ||
| - data |
There was a problem hiding this comment.
에러 응답 스키마가 코딩 가이드라인을 위반합니다.
코딩 가이드라인은 "API error responses should have a consistent format with 'code' and 'message' fields"를 요구하지만, 현재 ErrorResponse 스키마는 code 필드가 없고 message와 data 필드만 있습니다. 에러 코드가 메시지 문자열에 접두사로 포함되어 있어(예: "[ERROR: Request/BadRequest]") 클라이언트가 구조화된 데이터 대신 문자열 파싱을 해야 합니다.
다음과 같이 구조화된 스키마로 변경하는 것을 권장합니다:
ErrorResponse:
type: object
properties:
code:
type: string
example: "REQUEST_BAD_REQUEST"
message:
type: string
example: "Refresh Token을 입력해주세요."
data:
nullable: true
example: null
required:
- code
- message이 변경은 ApiResponse DTO와 GlobalExceptionHandler의 응답 생성 로직도 함께 수정해야 합니다.
As per coding guidelines, "API error responses should have a consistent format with 'code' and 'message' fields."
🤖 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 `@API_SPEC.yaml` around lines 13 - 24, The ErrorResponse schema in
API_SPEC.yaml violates the coding guideline requiring error responses to have
both 'code' and 'message' fields as separate properties. Currently, the error
code is embedded as a string prefix in the 'message' field (e.g., "[ERROR:
Request/BadRequest]"), forcing clients to parse strings instead of using
structured data. Add a new 'code' property to the ErrorResponse schema as a
string type, remove the error code prefix from the message example, and update
the 'required' array to include both 'code' and 'message'. Additionally, ensure
that the ApiResponse DTO class and GlobalExceptionHandler's response creation
logic are modified to populate the separate 'code' and 'message' fields
independently rather than combining them into a single message string.
Source: Coding guidelines
Comment on lines
+25
to
+61
| responses: | ||
| BadRequestError: | ||
| description: "Invalid request input." | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/ErrorResponse" | ||
| example: | ||
| message: "[ERROR: Request/BadRequest] Refresh Token을 입력해주세요." | ||
| data: null | ||
| UnauthorizedError: | ||
| description: "Authentication failed or token is invalid." | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/ErrorResponse" | ||
| example: | ||
| message: "[ERROR: Auth/Unauthorized] 유효하지 않은 Refresh Token입니다." | ||
| data: null | ||
| GoogleOauthError: | ||
| description: "Google OAuth request failed." | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/ErrorResponse" | ||
| example: | ||
| message: "[ERROR: Auth/Oauth/Google] Google Access Token을 발급받을 수 없습니다." | ||
| data: null | ||
| InternalServerError: | ||
| description: "Unexpected server error." | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/ErrorResponse" | ||
| example: | ||
| message: "[ERROR: ?/?] 서버 내부 오류가 발생했습니다." | ||
| data: null |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
에러 응답 컴포넌트의 예시가 구조화되지 않은 에러 코드를 포함합니다.
각 에러 응답 예시의 message 필드에 "[ERROR: Request/BadRequest]" 같은 접두사가 포함되어 있습니다. 위에서 언급한 대로 code 필드를 별도로 분리하면, 예시도 다음과 같이 업데이트되어야 합니다:
BadRequestError:
description: "Invalid request input."
content:
application/json:
schema:
$ref: "`#/components/schemas/ErrorResponse`"
example:
code: "REQUEST_BAD_REQUEST"
message: "Refresh Token을 입력해주세요."
data: null🤖 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 `@API_SPEC.yaml` around lines 25 - 61, The error response examples in the
responses section (BadRequestError, UnauthorizedError, GoogleOauthError,
InternalServerError) contain error codes embedded in the message field as
prefixes like "[ERROR: Request/BadRequest]". Extract these error code prefixes
from the message field and create a separate `code` field for each error
response example. Update BadRequestError to have code "REQUEST_BAD_REQUEST",
UnauthorizedError to have code "AUTH_UNAUTHORIZED", GoogleOauthError to have
code "AUTH_OAUTH_GOOGLE", and InternalServerError to have an appropriate code
value. Keep only the Korean error message text (without the prefix) in the
message field for each example.
Source: Coding guidelines
Comment on lines
3
to
+9
| public record ApiResponse<T> ( | ||
| String message, | ||
| T data | ||
| ){} | ||
| ){ | ||
| public static <T> ApiResponse<T> fail(String message) { | ||
| return new ApiResponse<>(message, null); | ||
| } |
There was a problem hiding this comment.
에러 응답 포맷을 code/message 분리 구조로 맞춰주세요.
현재 fail(String message)는 message와 data=null만 반환해서, 에러 코드를 문자열 접두사로 붙이는 구현에 의존하게 됩니다. 이 구조는 전역 예외 응답의 계약을 불안정하게 만들고, 클라이언트가 code를 구조적으로 파싱할 수 없습니다.
변경 예시
-public record ApiResponse<T> (
- String message,
- T data
-){
- public static <T> ApiResponse<T> fail(String message) {
- return new ApiResponse<>(message, null);
- }
+public record ApiResponse<T> (
+ String code,
+ String message,
+ T data
+){
+ public static <T> ApiResponse<T> fail(String code, String message) {
+ return new ApiResponse<>(code, message, null);
+ }
}As per coding guidelines, "API error responses should have a consistent format with 'code' and 'message' fields."
🤖 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/com/example/auth/global/dto/ApiResponse.java` around lines 3 -
9, The ApiResponse record currently lacks a code field, and the fail method only
accepts a message parameter with error codes embedded as string prefixes. Add a
code field to the ApiResponse record to store error codes separately, then
update the fail method signature to accept both code and message as distinct
parameters. This will ensure error responses follow a consistent structure with
properly separated code and message fields, allowing clients to parse error
codes structurally rather than relying on string parsing.
Source: Coding guidelines
Comment on lines
+52
to
+56
| try { | ||
| jwtProvider.validateToken(refreshToken); | ||
| } catch (JwtException | IllegalArgumentException exception) { | ||
| throw new UnauthorizedException("유효하지 않은 Refresh Token입니다."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# JwtProvider.validateToken() 메서드의 예외 선언 확인
rg -nP -A 10 'void\s+validateToken\s*\(' --type=java -g '*JwtProvider*'Repository: MomentLit/Auth
Length of output: 40
🌐 Web query:
JJWT 0.12.6 JwtParser parseSignedClaims exceptions
💡 Result:
In JJWT 0.12.6, the JwtParser.parseSignedClaims(CharSequence) method is designed to parse and verify signed JWTs (JWS). Because this operation involves both parsing and cryptographic verification, it can throw various exceptions derived from the base io.jsonwebtoken.JwtException class [1][2]. Common exceptions include: 1. SignatureException: Thrown if the JWT signature verification fails [3]. 2. ExpiredJwtException: Thrown if the JWT has expired (e.g., the 'exp' claim is in the past) [4][3]. 3. UnsupportedJwtException: Thrown if the parsed JWT does not match the expected format or type (e.g., if you call parseSignedClaims on a token that is not a JWS) [1][2][3]. 4. MalformedJwtException: Thrown if the JWT string is not correctly formatted or is otherwise unparseable [1][2]. 5. MissingClaimException: Thrown if a required claim is missing. 6. PrematureJwtException: Thrown if the JWT is not yet valid (e.g., the 'nbf' claim is in the future). Since these exceptions all extend from JwtException, it is recommended to wrap the parseSignedClaims call in a try-catch block to handle authentication or validation failures [1][2][3]: try { Claims claims = Jwts.parser.verifyWith(secretKey).build.parseSignedClaims(token).getPayload; // Token is valid and trusted } catch (JwtException e) { // Token is invalid, expired, or failed signature verification } Note that in version 0.12.6, JwtParser is immutable, and you must use the Jwts.parser method to obtain a JwtParserBuilder, configure it, and call build before parsing [5][6][7]. Using the incorrect method for the token type (e.g., trying to parse an unsigned JWT with parseSignedClaims) will trigger an UnsupportedJwtException [1][2].
Citations:
- 1: https://github.com/jwtk/jjwt
- 2: https://github.com/jwtk/jjwt/blob/0b34b211/README.adoc
- 3: https://github.com/jwtk/jjwt/blob/ca02283e523b745a0f4fb146d314cdf9f86be753/README.md
- 4: SignatureException could include the header and claims jwtk/jjwt#969
- 5: Having trouble upgrading from an old version of jjwt jwtk/jjwt#955
- 6: https://stackoverflow.com/questions/73486900/how-to-fix-deprecated-parser-and-setsigningkeyjava-security-key-usage-in-jjw
- 7: https://stackoverflow.com/questions/78805779/issue-with-parserbuilder-method-in-jjwt-library-for-jwt-token-validation
🏁 Script executed:
fd -type f -name "*.java" | head -20Repository: MomentLit/Auth
Length of output: 229
🏁 Script executed:
find . -type f -name "*JwtProvider*" 2>/dev/nullRepository: MomentLit/Auth
Length of output: 124
🏁 Script executed:
rg "class JwtProvider" --type=java -A 5Repository: MomentLit/Auth
Length of output: 615
🏁 Script executed:
cat -n src/main/java/com/example/auth/service/TokenService.java | head -80Repository: MomentLit/Auth
Length of output: 2958
🏁 Script executed:
cat -n src/main/java/com/example/auth/global/security/JwtProvider.javaRepository: MomentLit/Auth
Length of output: 4740
IllegalArgumentException 포착을 제거하세요.
JwtProvider.validateToken()은 JJWT의 parseSignedClaims()을 통해 오직 JwtException 및 그 하위 예외만 던집니다. IllegalArgumentException은 이 검증 과정에서 발생하지 않으므로, 이를 포착하는 것은 불필요하며 실제 버그를 은폐할 수 있습니다. catch (JwtException exception)으로 변경하세요.
🤖 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/com/example/auth/service/TokenService.java` around lines 52 -
56, Remove the unnecessary `IllegalArgumentException` from the catch block in
the TokenService token validation logic. The `jwtProvider.validateToken()`
method only throws `JwtException` and its subclasses through JJWT's
parseSignedClaims() method, so catching `IllegalArgumentException` is redundant
and can mask actual bugs. Change the catch clause to only catch `JwtException`,
keeping the same UnauthorizedException throw statement that handles the
validation failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/example/auth/global/client/UserServiceClient.java (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ObjectMapper를 빈으로 주입 권장
new ObjectMapper()대신 Spring의 자동 구성된ObjectMapper빈을 생성자 주입받아 사용하는 것을 권장합니다. 애플리케이션 전역의 직렬화/역직렬화 설정 일관성을 보장하고 테스트 용이성이 향상됩니다.♻️ 제안: ObjectMapper 빈 주입
private final RestClient restClient; - private final ObjectMapper objectMapper = new ObjectMapper(); + private final ObjectMapper objectMapper; public UserServiceClient( `@Value`("${user-service.base-url:http://localhost:8081}") String userServiceBaseUrl, + ObjectMapper objectMapper ) { this.restClient = RestClient.builder() .baseUrl(userServiceBaseUrl) .build(); + this.objectMapper = objectMapper; }🤖 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/com/example/auth/global/client/UserServiceClient.java` at line 27, `UserServiceClient`의 `objectMapper`를 `new ObjectMapper()`로 직접 생성하지 말고, Spring이 관리하는 `ObjectMapper` 빈을 생성자 주입으로 받도록 변경하세요. `UserServiceClient` 생성자와 `objectMapper` 사용 지점을 함께 수정해 애플리케이션의 전역 직렬화/역직렬화 설정을 그대로 따르도록 하고, 테스트에서도 주입 가능한 형태로 유지하세요.
🤖 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/com/example/auth/global/exception/GlobalExceptionHandler.java`:
- Around line 46-84: The `downstreamServiceHandleException` handler in
`GlobalExceptionHandler` is logging `e.getResponseBody()` again, which
duplicates the body already logged in
`UserServiceClient.convertToDownstreamException` and may expose PII. Update the
`log.warn` and `log.error` calls in this handler to log only the service name
and status code, removing the response body from all messages while keeping the
existing response handling intact.
- Around line 50-60: The 4xx branch in GlobalExceptionHandler currently returns
ApiResponse.fail(e.getMessage()) without the same error prefix used for 502/503
responses. Update the 4xx handling in the exception handler to build the
response message with the same [ERROR: Auth/Downstream/...] prefix format used
elsewhere, keeping the logic in GlobalExceptionHandler consistent across all
downstream failures.
---
Nitpick comments:
In `@src/main/java/com/example/auth/global/client/UserServiceClient.java`:
- Line 27: `UserServiceClient`의 `objectMapper`를 `new ObjectMapper()`로 직접 생성하지
말고, Spring이 관리하는 `ObjectMapper` 빈을 생성자 주입으로 받도록 변경하세요. `UserServiceClient` 생성자와
`objectMapper` 사용 지점을 함께 수정해 애플리케이션의 전역 직렬화/역직렬화 설정을 그대로 따르도록 하고, 테스트에서도 주입 가능한
형태로 유지하세요.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f4f3f77-96ac-4311-bd85-32f96c7083be
📒 Files selected for processing (3)
src/main/java/com/example/auth/global/client/UserServiceClient.javasrc/main/java/com/example/auth/global/exception/DownstreamServiceException.javasrc/main/java/com/example/auth/global/exception/GlobalExceptionHandler.java
Comment on lines
+46
to
+84
| @ExceptionHandler(DownstreamServiceException.class) | ||
| public ResponseEntity<ApiResponse<String>> downstreamServiceHandleException( | ||
| DownstreamServiceException e | ||
| ) { | ||
| if (e.getStatusCode().is4xxClientError()) { | ||
| log.warn( | ||
| "Downstream service client error. service={}, status={}, body={}", | ||
| e.getServiceName(), | ||
| e.getStatusCode(), | ||
| e.getResponseBody() | ||
| ); | ||
|
|
||
| return ResponseEntity.status(e.getStatusCode()) | ||
| .body(ApiResponse.fail(e.getMessage())); | ||
| } | ||
|
|
||
| if (e.getStatusCode().isSameCodeAs(HttpStatus.SERVICE_UNAVAILABLE)) { | ||
| log.error( | ||
| "Downstream service unavailable. service={}, body={}", | ||
| e.getServiceName(), | ||
| e.getResponseBody() | ||
| ); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE) | ||
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | ||
| + "] " + e.getServiceName() + " 서비스에 연결할 수 없습니다.")); | ||
| } | ||
|
|
||
| log.error( | ||
| "Downstream service server error. service={}, status={}, body={}", | ||
| e.getServiceName(), | ||
| e.getStatusCode(), | ||
| e.getResponseBody() | ||
| ); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | ||
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | ||
| + "] " + e.getServiceName() + " 서비스 호출 중 오류가 발생했습니다.")); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
응답 본문 이중 로깅 및 PII 노출 우려
UserServiceClient.convertToDownstreamException에서 이미 responseBody를 로깅하므로, 핸들러에서 다시 로깅하면 동일한 응답 본문이 중복 기록됩니다. 또한 사용자 서비스의 에러 응답 본문에 이메일 등 PII가 포함될 수 있어 보안상 우려가 있습니다.
핸들러의 로그에서 e.getResponseBody()를 제거하고 서비스명과 상태코드만 로깅하는 것을 제안합니다.
🔒️ 제안: 핸들러에서 응답 본문 로깅 제거
if (e.getStatusCode().is4xxClientError()) {
log.warn(
- "Downstream service client error. service={}, status={}, body={}",
+ "Downstream service client error. service={}, status={}",
e.getServiceName(),
- e.getStatusCode(),
- e.getResponseBody()
+ e.getStatusCode()
); if (e.getStatusCode().isSameCodeAs(HttpStatus.SERVICE_UNAVAILABLE)) {
log.error(
- "Downstream service unavailable. service={}, body={}",
+ "Downstream service unavailable. service={}",
- e.getServiceName(),
- e.getResponseBody()
+ e.getServiceName()
); log.error(
- "Downstream service server error. service={}, status={}, body={}",
+ "Downstream service server error. service={}, status={}",
e.getServiceName(),
- e.getStatusCode(),
- e.getResponseBody()
+ e.getStatusCode()
);📝 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
| @ExceptionHandler(DownstreamServiceException.class) | |
| public ResponseEntity<ApiResponse<String>> downstreamServiceHandleException( | |
| DownstreamServiceException e | |
| ) { | |
| if (e.getStatusCode().is4xxClientError()) { | |
| log.warn( | |
| "Downstream service client error. service={}, status={}, body={}", | |
| e.getServiceName(), | |
| e.getStatusCode(), | |
| e.getResponseBody() | |
| ); | |
| return ResponseEntity.status(e.getStatusCode()) | |
| .body(ApiResponse.fail(e.getMessage())); | |
| } | |
| if (e.getStatusCode().isSameCodeAs(HttpStatus.SERVICE_UNAVAILABLE)) { | |
| log.error( | |
| "Downstream service unavailable. service={}, body={}", | |
| e.getServiceName(), | |
| e.getResponseBody() | |
| ); | |
| return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE) | |
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | |
| + "] " + e.getServiceName() + " 서비스에 연결할 수 없습니다.")); | |
| } | |
| log.error( | |
| "Downstream service server error. service={}, status={}, body={}", | |
| e.getServiceName(), | |
| e.getStatusCode(), | |
| e.getResponseBody() | |
| ); | |
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | |
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | |
| + "] " + e.getServiceName() + " 서비스 호출 중 오류가 발생했습니다.")); | |
| } | |
| `@ExceptionHandler`(DownstreamServiceException.class) | |
| public ResponseEntity<ApiResponse<String>> downstreamServiceHandleException( | |
| DownstreamServiceException e | |
| ) { | |
| if (e.getStatusCode().is4xxClientError()) { | |
| log.warn( | |
| "Downstream service client error. service={}, status={}", | |
| e.getServiceName(), | |
| e.getStatusCode() | |
| ); | |
| return ResponseEntity.status(e.getStatusCode()) | |
| .body(ApiResponse.fail(e.getMessage())); | |
| } | |
| if (e.getStatusCode().isSameCodeAs(HttpStatus.SERVICE_UNAVAILABLE)) { | |
| log.error( | |
| "Downstream service unavailable. service={}", | |
| e.getServiceName() | |
| ); | |
| return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE) | |
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | |
| "] " + e.getServiceName() + " 서비스에 연결할 수 없습니다.")); | |
| } | |
| log.error( | |
| "Downstream service server error. service={}, status={}", | |
| e.getServiceName(), | |
| e.getStatusCode() | |
| ); | |
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | |
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | |
| "] " + e.getServiceName() + " 서비스 호출 중 오류가 발생했습니다.")); | |
| } |
🤖 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/com/example/auth/global/exception/GlobalExceptionHandler.java`
around lines 46 - 84, The `downstreamServiceHandleException` handler in
`GlobalExceptionHandler` is logging `e.getResponseBody()` again, which
duplicates the body already logged in
`UserServiceClient.convertToDownstreamException` and may expose PII. Update the
`log.warn` and `log.error` calls in this handler to log only the service name
and status code, removing the response body from all messages while keeping the
existing response handling intact.
Comment on lines
+50
to
+60
| if (e.getStatusCode().is4xxClientError()) { | ||
| log.warn( | ||
| "Downstream service client error. service={}, status={}, body={}", | ||
| e.getServiceName(), | ||
| e.getStatusCode(), | ||
| e.getResponseBody() | ||
| ); | ||
|
|
||
| return ResponseEntity.status(e.getStatusCode()) | ||
| .body(ApiResponse.fail(e.getMessage())); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
4xx 다운스트림 에러의 메시지 형식 불일치
4xx 에러는 e.getMessage()를 그대로 반환하지만, 503/502 에러는 [ERROR: Auth/Downstream/...] 접두사를 사용합니다. 모든 에러 응답 형식의 일관성을 위해 4xx에도 동일한 접두사를 추가하는 것을 제안합니다.
♻️ 제안: 4xx 응답에 접두사 추가
return ResponseEntity.status(e.getStatusCode())
- .body(ApiResponse.fail(e.getMessage()));
+ .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName()
+ + "] " + e.getMessage()));📝 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
| if (e.getStatusCode().is4xxClientError()) { | |
| log.warn( | |
| "Downstream service client error. service={}, status={}, body={}", | |
| e.getServiceName(), | |
| e.getStatusCode(), | |
| e.getResponseBody() | |
| ); | |
| return ResponseEntity.status(e.getStatusCode()) | |
| .body(ApiResponse.fail(e.getMessage())); | |
| } | |
| if (e.getStatusCode().is4xxClientError()) { | |
| log.warn( | |
| "Downstream service client error. service={}, status={}, body={}", | |
| e.getServiceName(), | |
| e.getStatusCode(), | |
| e.getResponseBody() | |
| ); | |
| return ResponseEntity.status(e.getStatusCode()) | |
| .body(ApiResponse.fail("[ERROR: Auth/Downstream/" + e.getServiceName() | |
| "] " + e.getMessage())); | |
| } |
🤖 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/com/example/auth/global/exception/GlobalExceptionHandler.java`
around lines 50 - 60, The 4xx branch in GlobalExceptionHandler currently returns
ApiResponse.fail(e.getMessage()) without the same error prefix used for 502/503
responses. Update the 4xx handling in the exception handler to build the
response message with the same [ERROR: Auth/Downstream/...] prefix format used
elsewhere, keeping the logic in GlobalExceptionHandler consistent across all
downstream failures.
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
1 participant
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
릴리스 노트
New Features
Bug Fixes
Documentation
Chores