Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughexternal 모듈에 Spring AOP 의존성을 추가하고, TosspaymentsClient의 로깅/예외 처리 코드를 제거한 뒤 이를 대체하는 TosspaymentsLoggingAspect를 새로 도입하여 TosspaymentsClient의 공개 메서드를 AOP로 가로챕니다. Changes
Sequence Diagram(s)sequenceDiagram
participant ClientApp as Client App
participant TosspayClient as TosspaymentsClient
participant Aspect as TosspaymentsLoggingAspect
participant ExternalAPI as Toss API
ClientApp->>TosspayClient: invoke confirmPayment(...)
TosspayClient->>Aspect: around advice intercepts (ProceedingJoinPoint)
Aspect->>Aspect: log request, start StopWatch
Aspect->>TosspayClient: proceed()
TosspayClient->>ExternalAPI: HTTP call
ExternalAPI-->>TosspayClient: HTTP response
TosspayClient-->>Aspect: return response
Aspect->>Aspect: stop StopWatch, log response + elapsed
Aspect-->>ClientApp: return response
Estimated code review effort🎯 3 (중간) | ⏱️ ~20분 Possibly related PRs
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)
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 |
Summary of ChangesHello @unifolio0, 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! 이 PR은 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit 607688d. ♻️ This comment has been updated with latest results. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR is a good refactoring that separates the logging logic of RestClient using AOP, making the TosspaymentsClient code cleaner through separation of concerns. However, the current implementation in TosspaymentsLoggingAspect logs full request arguments, response bodies, and error response bodies, which contain sensitive transaction identifiers (e.g., paymentKey) and Personally Identifiable Information (e.g., orderName). Logging such data poses a significant security and compliance risk. It is crucial to address this by making the exception handling logic more robust and ensuring sensitive information is not logged. Consider implementing redaction/masking for sensitive fields or logging only non-sensitive metadata.
| String methodName = joinPoint.getSignature().getName(); | ||
| Object[] args = joinPoint.getArgs(); | ||
|
|
||
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); |
There was a problem hiding this comment.
The logging statement includes the full request arguments (args), which contain sensitive information such as paymentKey and orderId. Logging these identifiers can lead to the exposure of sensitive transaction data and violates security policies. It is recommended to log only non-sensitive metadata or to implement a masking mechanism for sensitive fields, perhaps by overriding the toString() method of related DTOs.
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); | |
| log.info("[토스페이먼츠 API 요청] {}", methodName); |
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), result); |
There was a problem hiding this comment.
The logging statement includes the full response body (result), which contains Personally Identifiable Information (PII) such as orderName and sensitive transaction details like paymentKey. Logging such data can violate privacy regulations and increase the risk of exposure. It is recommended to avoid logging the entire response object and instead log only necessary, non-sensitive fields. Additionally, ensure the try-catch block is robust enough to handle all Throwables, not just HttpClientErrorException, to prevent logging omissions and ensure stopWatch always stops.
log.info("[토스페이먼츠 API 응답] {} - {}ms", methodName, stopWatch.getTotalTimeMillis());| log.error("[토스페이먼츠 API 실패] {} - {}ms - status: {}, body: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), | ||
| e.getStatusCode(), e.getResponseBodyAsString(), e); |
There was a problem hiding this comment.
The error logging statement includes the full error response body (e.getResponseBodyAsString()), which may contain PII or sensitive system information returned by the external API during a failure. Logging raw error responses can lead to unintended data leakage. It is recommended to log the status code and exception details without including the full response body.
log.error("[토스페이먼츠 API 실패] {} - {}ms - status: {}",
methodName, stopWatch.getTotalTimeMillis(),
e.getStatusCode(), e);| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), result); |
There was a problem hiding this comment.
응답 객체 전체를 로깅하면 민감한 정보가 포함될 수 있고 로그가 너무 길어질 수 있습니다. 요청 로깅과 마찬가지로, 응답 객체의 toString()을 재정의하여 필요한 정보만 로깅하거나 민감한 정보를 마스킹하는 것이 좋습니다.
![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
🤖 Fix all issues with AI agents
In
`@external/src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`:
- Line 23: The logging in TosspaymentsLoggingAspect currently prints raw request
arguments and responses (args and result), which can expose sensitive payment
data; update the code to sanitize/redact sensitive fields before logging by
introducing or using a sanitizer utility (e.g., redactSensitiveFields(Object) or
a TosspaymentsLoggingSanitizer) and call it where methodName and args are logged
and where result is logged, ensuring sensitive keys (amounts, card numbers,
keys, tokens, PII) are masked or omitted and that you do not rely on raw
toString() of request/response objects.
- Around line 12-15: Reorder the class annotations so Spring annotations come
before Lombok: move `@Aspect`, `@Component` and `@Order`(1) to appear above `@Slf4j` in
the TosspaymentsLoggingAspect class; this ensures the Spring annotations
(`@Aspect`, `@Component`, `@Order`) are declared before the Lombok `@Slf4j` to comply
with the project coding guideline.
- Around line 27-39: The current TosspaymentsLoggingAspect catch only handles
HttpClientErrorException so server-side errors (HttpServerErrorException),
timeouts/connection failures (ResourceAccessException) and other
RestClientExceptions aren't logged and stopWatch.stop() may not run; update the
try/catch in the method where joinPoint.proceed() is invoked to ensure
stopWatch.stop() is always called (move stopWatch.stop() to a finally block) and
add a broader catch for RestClientException (or Exception) to log the error with
methodName, stopWatch.getTotalTimeMillis(), and relevant exception details
before rethrowing; reference the joinPoint.proceed(), stopWatch.stop(),
methodName, and HttpClientErrorException in your changes.
| @Slf4j | ||
| @Order(1) | ||
| @Aspect | ||
| @Component |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
어노테이션 순서: Spring 어노테이션을 Lombok 어노테이션보다 앞에 배치해주세요.
코딩 가이드라인에 따르면 Spring 어노테이션이 Lombok 어노테이션보다 먼저 위치해야 합니다.
♻️ 수정 제안
-@Slf4j
`@Order`(1)
`@Aspect`
`@Component`
+@Slf4j
public class TosspaymentsLoggingAspect {As per coding guidelines, "Use Lombok for constructors, getters/setters with Spring annotations before Lombok annotations".
📝 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.
| @Slf4j | |
| @Order(1) | |
| @Aspect | |
| @Component | |
| `@Order`(1) | |
| `@Aspect` | |
| `@Component` | |
| `@Slf4j` |
🤖 Prompt for AI Agents
In
`@external/src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
around lines 12 - 15, Reorder the class annotations so Spring annotations come
before Lombok: move `@Aspect`, `@Component` and `@Order`(1) to appear above `@Slf4j` in
the TosspaymentsLoggingAspect class; this ensures the Spring annotations
(`@Aspect`, `@Component`, `@Order`) are declared before the Lombok `@Slf4j` to comply
with the project coding guideline.
| String methodName = joinPoint.getSignature().getName(); | ||
| Object[] args = joinPoint.getArgs(); | ||
|
|
||
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); |
There was a problem hiding this comment.
결제 요청 인자(args)를 그대로 로깅하면 민감한 결제 정보가 노출될 수 있습니다.
args 배열에는 결제 금액, 키 등 민감 데이터가 포함될 수 있습니다. 마찬가지로 Line 31의 result 응답 객체도 민감 정보를 포함할 수 있습니다. 민감 필드를 마스킹하거나, toString()에서 민감 필드를 제외하는 방식을 고려해주세요.
🤖 Prompt for AI Agents
In
`@external/src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
at line 23, The logging in TosspaymentsLoggingAspect currently prints raw
request arguments and responses (args and result), which can expose sensitive
payment data; update the code to sanitize/redact sensitive fields before logging
by introducing or using a sanitizer utility (e.g., redactSensitiveFields(Object)
or a TosspaymentsLoggingSanitizer) and call it where methodName and args are
logged and where result is logged, ensuring sensitive keys (amounts, card
numbers, keys, tokens, PII) are masked or omitted and that you do not rely on
raw toString() of request/response objects.
| try { | ||
| Object result = joinPoint.proceed(); | ||
| stopWatch.stop(); | ||
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), result); | ||
| return result; | ||
| } catch (HttpClientErrorException e) { | ||
| stopWatch.stop(); | ||
| log.error("[토스페이먼츠 API 실패] {} - {}ms - status: {}, body: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), | ||
| e.getStatusCode(), e.getResponseBodyAsString(), e); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
HttpClientErrorException만 캐치하면 서버 에러(5xx) 및 타임아웃이 로깅되지 않습니다.
HttpServerErrorException(5xx), ResourceAccessException(타임아웃/커넥션 에러) 등 다른 예외가 발생하면 로깅 없이 전파됩니다. 또한 이 경우 stopWatch.stop()도 호출되지 않습니다.
전체 예외를 로깅하려면 catch (Exception e) 또는 최소한 catch (RestClientException e)를 추가하거나, finally 블록에서 처리하는 것을 권장합니다.
🐛 수정 제안
try {
Object result = joinPoint.proceed();
stopWatch.stop();
log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}",
methodName, stopWatch.getTotalTimeMillis(), result);
return result;
} catch (HttpClientErrorException e) {
stopWatch.stop();
log.error("[토스페이먼츠 API 실패] {} - {}ms - status: {}, body: {}",
methodName, stopWatch.getTotalTimeMillis(),
e.getStatusCode(), e.getResponseBodyAsString(), e);
throw e;
+ } catch (Exception e) {
+ if (stopWatch.isRunning()) {
+ stopWatch.stop();
+ }
+ log.error("[토스페이먼츠 API 실패] {} - {}ms", methodName, stopWatch.getTotalTimeMillis(), e);
+ throw e;
}🤖 Prompt for AI Agents
In
`@external/src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
around lines 27 - 39, The current TosspaymentsLoggingAspect catch only handles
HttpClientErrorException so server-side errors (HttpServerErrorException),
timeouts/connection failures (ResourceAccessException) and other
RestClientExceptions aren't logged and stopWatch.stop() may not run; update the
try/catch in the method where joinPoint.proceed() is invoked to ensure
stopWatch.stop() is always called (move stopWatch.stop() to a finally block) and
add a broader catch for RestClientException (or Exception) to log the error with
methodName, stopWatch.getTotalTimeMillis(), and relevant exception details
before rethrowing; reference the joinPoint.proceed(), stopWatch.stop(),
methodName, and HttpClientErrorException in your changes.
1 participant
closed #16
작업 내용
참고 사항
Summary by CodeRabbit