Conversation
Summary by CodeRabbit릴리스 노트
WalkthroughTossPayments 웹훅 수신 엔드포인트를 구현했습니다. 웹훅 컨트롤러, 서비스, DTO를 추가하고 결제 상태 전이 로직을 구현했습니다. 인프라 설정(Redis, 네트워크, Promtail)도 업데이트되었습니다. Changes
Sequence DiagramsequenceDiagram
participant Client as 외부 결제<br/>시스템
participant Controller as WebhookController
participant Service as WebhookService
participant PaymentSvc as TosspaymentsPayment<br/>Service
participant TokenSvc as TokenService
participant DB as Database
Client->>Controller: POST /webhooks/tosspayments<br/>(WebhookPayload)
Controller->>Service: handlePaymentStatusChanged(payload)
activate Service
Service->>PaymentSvc: findByPaymentKey(paymentKey)
PaymentSvc->>DB: SELECT payment WHERE paymentKey
DB-->>PaymentSvc: TosspaymentsPayment
PaymentSvc-->>Service: Optional<TosspaymentsPayment>
alt 결제 상태 = DONE
Service->>Service: parseMetadata(metadata)
Service->>DB: INSERT token_purchase
Service->>PaymentSvc: updateState(COMPLETED)
PaymentSvc->>DB: UPDATE payment SET state
Service->>TokenSvc: issueToken(member, quantity)
TokenSvc->>DB: INSERT token
Service->>DB: COMMIT
else 결제 상태 = CANCELED
Service->>PaymentSvc: updateState(CANCELED)
PaymentSvc->>DB: UPDATE payment SET state
else 결제 상태 = EXPIRED/ABORTED
Service->>PaymentSvc: updateState(NOT_NEED_CANCEL)
PaymentSvc->>DB: UPDATE payment SET state
end
deactivate Service
Controller-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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
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 Results 47 files 47 suites 1m 21s ⏱️ Results for commit af4bb74. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 47: The global "*.csv" ignore rule is too broad; narrow it to only
generated/output CSVs by replacing the pattern (found as "*.csv" in .gitignore)
with targeted paths such as "/logs/*.csv" and "/tmp/**/*.csv" or other specific
directories holding generated fixtures, ensuring legitimate project CSVs
(tests/samples) are not accidentally ignored.
In `@docker/prod/api/docker-compose-prod.yml`:
- Around line 94-97: The docker-compose networks block sets prod-kokomen-net as
external while also specifying driver: bridge which is invalid; remove the
driver: bridge line from the networks declaration (leave prod-kokomen-net:
external: true) and ensure the external network is created beforehand (e.g., run
docker network create --driver bridge prod-kokomen-net) so docker-compose can
reference the existing external network.
- Around line 61-71: The kokomen-redis service is missing authentication, a
restart policy, and is exposing port 6379 to the host; update the kokomen-redis
service definition to (1) add an environment variable such as REDIS_PASSWORD (or
the image-specific env/key) and configure the Redis process to require that
password (use the service's command/entrypoint args or image docs to set
requirepass), (2) add a restart policy like restart: unless-stopped (or
on-failure:3) to improve stability, and (3) if Redis is only for internal
services, remove the ports mapping and use expose: - "6379" instead to avoid
publishing the port to the host; reference service name kokomen-redis and the
keys environment, command/entrypoint, restart, ports/expose when making the
changes.
In `@src/main/java/com/samhap/kokomen/payment/controller/WebhookController.java`:
- Line 25: The parameter mapping for the header
tosspayments-webhook-transmission-time in WebhookController currently uses
LocalDateTime (transmissionTime) which may fail due to unknown header format;
change the method to accept the header as String (e.g., rename param to
transmissionTimeStr) and then parse it explicitly using a known
DateTimeFormatter, or inspect the TossPayments examples and add a matching
`@DateTimeFormat`(pattern="...") on the LocalDateTime parameter; update any
downstream logic to use the parsed LocalDateTime variable.
In
`@src/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.java`:
- Around line 11-33: Add validation to the WebhookPaymentData record by
annotating required fields (paymentKey, orderId, status, totalAmount) with
appropriate javax/jakarta.validation constraints (e.g.,
`@NotBlank/`@NotNull/@Positive) and annotate the nested WebhookEasyPay field with
`@Valid` to propagate validation; then implement a compact record constructor for
WebhookPaymentData that performs entity-level checks (null/blank/invalid amount
checks for paymentKey, orderId, status, totalAmount) and throws an
IllegalArgumentException for invalid input so malformed payloads are rejected at
construction time.
- Around line 11-33: Rename the DTO record WebhookPaymentData to follow the
project's DTO naming convention by changing its type name to
WebhookPaymentDataRequest and update the source filename accordingly; then
update all references/usages (variable types, constructors, method parameters,
imports, serializers/deserializers, tests, and any mapping code) from
WebhookPaymentData to WebhookPaymentDataRequest so compilation and JSON
(TossDateTimeDeserializer) annotations remain intact.
- Around line 21-24: TossDateTimeDeserializer currently calls
OffsetDateTime.parse without checking for JSON nulls; update
TossDateTimeDeserializer.deserialize to handle JsonToken.VALUE_NULL (or
parser.getCurrentToken()==JsonToken.VALUE_NULL) and return null (or handle as
appropriate) before calling OffsetDateTime.parse to avoid exceptions when
requestedAt/approvedAt are null. Also rename class WebhookPaymentData to
WebhookPaymentDataResponse and annotate the requestedAt and approvedAt fields
with `@NotNull` to mark them required (update any references/usages/DTO mappings
accordingly).
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java`:
- Around line 96-99: handleCanceled currently unconditionally sets payment state
to CANCELED without validating the current state; add an explicit
allowed-transition check inside handleCanceled (e.g., inspect payment.getState()
or payment.isXxx methods) and only call
payment.updateState(PaymentState.CANCELED) when the current state is one of the
valid predecessors (declare the allowed set such as CREATED, AUTHORIZED, or
whatever business rules require), otherwise log a warning/error and skip the
transition or throw an exception; reference the handleCanceled method,
TosspaymentsStatus parameter, PaymentState.CANCELED enum and
payment.updateState/getState to locate and implement the guard.
- Around line 58-85: handleDone currently skips the same verification performed
in PaymentFacadeService.confirmPayment (validateTosspaymentsResult), so add the
missing checks: compare webhook fields (use WebhookPaymentData.method names like
data.paymentKey()/data.orderId()/data.totalAmount() or data.orderId(),
data.totalAmount() and data.paymentKey if available) against the persisted
TosspaymentsPayment values (payment.getPaymentKey(), payment.getOrderId(),
payment.getTotalAmount()) before proceeding; if any mismatch occurs, log a clear
warning including both sides and abort processing (return or throw) to avoid
issuing tokens or saving TokenPurchase; you can either call the existing
validateTosspaymentsResult helper if accessible or replicate its exact checks
inside handleDone.
In
`@src/main/resources/db/migration/V41__add_unique_constraint_token_purchase_payment_key.sql`:
- Around line 1-3: Before adding the UNIQUE constraint
uk_token_purchase_payment_key on token_purchase.payment_key (and dropping
idx_token_purchase_payment_key), ensure there are no duplicate payment_key
values by running the provided GROUP BY query; if duplicates exist, remove or
consolidate them (e.g., deduplicate or backfill null/unique values) or add a
pre-migration step that identifies and resolves duplicates (delete/merge/update
or archive conflicting rows) so the ALTER TABLE statement will succeed during
deployment.
In `@src/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java`:
- Around line 52-125: Tests in WebhookServiceTest duplicate member/token/payment
setup across methods; extract a reusable helper (e.g., a private method in
WebhookServiceTest) that creates and returns the prepared entities or
identifiers to reduce repetition: factor out the Member/Token creation using
MemberFixtureBuilder and TokenFixtureBuilder and the TosspaymentsPayment
creation using TosspaymentsPaymentFixtureBuilder (including setting state via
payment.updateState(...) when needed), and reuse createWebhookPayload calls by
returning the paymentKey/orderId and metadata so each test just calls the helper
to get prepped memberId/paymentKey and then invokes
webhookService.handlePaymentStatusChanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
.gitignoredocker/dev/promtail/promtail.yamldocker/prod/api/docker-compose-prod.ymldocker/prod/api/promtail/promtail.yamlsrc/main/java/com/samhap/kokomen/payment/controller/WebhookController.javasrc/main/java/com/samhap/kokomen/payment/domain/PaymentState.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.javasrc/main/java/com/samhap/kokomen/payment/service/TosspaymentsPaymentService.javasrc/main/java/com/samhap/kokomen/payment/service/WebhookService.javasrc/main/java/com/samhap/kokomen/payment/service/dto/WebhookPayload.javasrc/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.javasrc/main/resources/db/migration/V41__add_unique_constraint_token_purchase_payment_key.sqlsrc/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java
💤 Files with no reviewable changes (1)
- docker/dev/promtail/promtail.yaml
| /**/bin/ | ||
| .serena | ||
| .claude | ||
| *.csv |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CSV files impact with the *.csv gitignore pattern
echo "== tracked CSV files =="
git ls-files '*.csv'
echo
echo "== csv files present in working tree =="
fd -e csv
echo
echo "== which CSV files are ignored by current rules =="
fd -e csv -x git check-ignore -v {} \;Repository: samhap-soft/kokomen-backend
Length of output: 187
🏁 Script executed:
# Also check the .gitignore file content around line 47
cat -n .gitignore | sed -n '40,55p'Repository: samhap-soft/kokomen-backend
Length of output: 188
🏁 Script executed:
# Check if there are any directories that might contain CSV files
git ls-tree -r HEAD --name-only | grep -i csv || echo "No tracked CSV files found"Repository: samhap-soft/kokomen-backend
Length of output: 143
*.csv 글로벌 패턴은 범위가 과도하므로 특정 경로로 제한 권장
Line 47의 패턴은 모든 디렉터리에서 CSV 파일을 무시합니다. 현재 tracked 또는 working tree의 CSV 파일은 없지만, 테스트 픽스처나 샘플 데이터 CSV를 향후 커밋할 경우 의도치 않게 무시될 수 있습니다. 생성 산출물만 제외하려면 /logs/*.csv, /tmp/**/*.csv 같은 특정 경로로 범위를 좁히는 것이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 47, The global "*.csv" ignore rule is too broad; narrow
it to only generated/output CSVs by replacing the pattern (found as "*.csv" in
.gitignore) with targeted paths such as "/logs/*.csv" and "/tmp/**/*.csv" or
other specific directories holding generated fixtures, ensuring legitimate
project CSVs (tests/samples) are not accidentally ignored.
| kokomen-redis: | ||
| image: valkey/valkey:8.0.1 | ||
| container_name: kokomen-redis | ||
| ports: | ||
| - 6379:6379 | ||
| volumes: | ||
| - kokomen-redis-data:/data | ||
| environment: | ||
| TZ: Asia/Seoul | ||
| networks: | ||
| - prod-kokomen-net |
There was a problem hiding this comment.
프로덕션 Redis 서비스 보안 및 안정성 설정 필요
프로덕션 환경에서 Redis를 운영할 때 다음 사항들을 고려해 주세요:
- 인증 미설정: 비밀번호 없이 Redis가 노출되어 있습니다.
- 재시작 정책 없음: 다른 서비스들은
restart: unless-stopped또는on-failure:3를 사용하고 있습니다. - 포트 노출: 6379 포트가 호스트에 직접 노출되어 있어, 내부 네트워크 통신만 필요한 경우 불필요합니다.
🛡️ 보안 및 안정성 개선 제안
kokomen-redis:
image: valkey/valkey:8.0.1
container_name: kokomen-redis
- ports:
- - 6379:6379
+ expose:
+ - "6379"
volumes:
- kokomen-redis-data:/data
environment:
TZ: Asia/Seoul
+ command: ["valkey-server", "--requirepass", "${REDIS_PASSWORD_PROD}"]
+ restart: unless-stopped
networks:
- prod-kokomen-net내부 서비스 간 통신만 필요한 경우 ports 대신 expose를 사용하고, 환경 변수로 비밀번호를 설정하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/prod/api/docker-compose-prod.yml` around lines 61 - 71, The
kokomen-redis service is missing authentication, a restart policy, and is
exposing port 6379 to the host; update the kokomen-redis service definition to
(1) add an environment variable such as REDIS_PASSWORD (or the image-specific
env/key) and configure the Redis process to require that password (use the
service's command/entrypoint args or image docs to set requirepass), (2) add a
restart policy like restart: unless-stopped (or on-failure:3) to improve
stability, and (3) if Redis is only for internal services, remove the ports
mapping and use expose: - "6379" instead to avoid publishing the port to the
host; reference service name kokomen-redis and the keys environment,
command/entrypoint, restart, ports/expose when making the changes.
| networks: | ||
| prod-kokomen-net: | ||
| external: true | ||
| driver: bridge |
There was a problem hiding this comment.
external: true와 driver: bridge는 함께 사용할 수 없습니다.
외부 네트워크는 Docker Compose 외부에서 이미 생성된 네트워크를 참조하므로, driver 옵션을 지정할 수 없습니다. 이 설정은 docker-compose up 실행 시 오류를 발생시킵니다.
🐛 수정 제안
networks:
prod-kokomen-net:
external: true
- driver: bridge외부 네트워크를 사용하기 전에 먼저 생성해야 합니다:
docker network create --driver bridge prod-kokomen-net📝 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.
| networks: | |
| prod-kokomen-net: | |
| external: true | |
| driver: bridge | |
| networks: | |
| prod-kokomen-net: | |
| external: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/prod/api/docker-compose-prod.yml` around lines 94 - 97, The
docker-compose networks block sets prod-kokomen-net as external while also
specifying driver: bridge which is invalid; remove the driver: bridge line from
the networks declaration (leave prod-kokomen-net: external: true) and ensure the
external network is created beforehand (e.g., run docker network create --driver
bridge prod-kokomen-net) so docker-compose can reference the existing external
network.
|
|
||
| @PostMapping("/tosspayments") | ||
| public ResponseEntity<Void> handleTosspaymentsWebhook( | ||
| @RequestHeader("tosspayments-webhook-transmission-time") LocalDateTime transmissionTime, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "WebhookController.java" | head -5Repository: samhap-soft/kokomen-backend
Length of output: 147
🏁 Script executed:
cat -n src/main/java/com/samhap/kokomen/payment/controller/WebhookController.java | head -40Repository: samhap-soft/kokomen-backend
Length of output: 1813
🌐 Web query:
tosspayments webhook transmission time date format specification
💡 Result:
TossPayments’ official webhook docs define what tosspayments-webhook-transmission-time is, but do not specify an explicit date/time string format for that header value—only that it is “the time the webhook was sent.” In signature verification (for payout.changed, seller.changed), you must use the header value as-is in the signing input: {WEBHOOK_PAYLOAD}:{tosspayments-webhook-transmission-time}. [1]
If you need a documented timestamp format, use the timestamps in the webhook JSON body instead:
createdAt(payment-related events likePAYMENT_STATUS_CHANGED,DEPOSIT_CALLBACK,CANCEL_STATUS_CHANGED, etc.):yyyy-MM-dd'T'HH:mm:ss.SSSSSS(ISO 8601) [1]createdAt(payout/seller events):yyyy-MM-dd'T'HH:mm:ss±hh:mm(ISO 8601) [1]changedAt(inCUSTOMER_STATUS_CHANGED):yyyy-MM-dd'T'HH:mm:ss±hh:mm(ISO 8601) [1]
Sources: TossPayments “웹훅 이벤트” documentation. [1]
[1] https://docs.tosspayments.com/reference/using-api/webhook-events
tosspayments-webhook-transmission-time 헤더 파싱 형식 확인 필요
Tosspayments 공식 문서에서는 tosspayments-webhook-transmission-time 헤더의 명시적 날짜 형식을 정의하지 않습니다. 헤더 형식이 문서화되지 않았으므로, Spring의 기본 LocalDateTime 변환이 실패할 가능성이 있습니다. 다음 중 하나를 선택하세요:
- Tosspayments API 응답 예제에서 실제 형식을 확인하고
@DateTimeFormat어노테이션을 정확히 설정 - 또는 헤더를
String으로 받아 명시적으로 파싱하기 - 또는 문서화된 웹훅 바디의
createdAt또는changedAt타임스탬프(yyyy-MM-dd'T'HH:mm:ss.SSSSSS형식) 사용
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/controller/WebhookController.java`
at line 25, The parameter mapping for the header
tosspayments-webhook-transmission-time in WebhookController currently uses
LocalDateTime (transmissionTime) which may fail due to unknown header format;
change the method to accept the header as String (e.g., rename param to
transmissionTimeStr) and then parse it explicitly using a known
DateTimeFormatter, or inspect the TossPayments examples and add a matching
`@DateTimeFormat`(pattern="...") on the LocalDateTime parameter; update any
downstream logic to use the parsed LocalDateTime variable.
| public record WebhookPaymentData( | ||
| String paymentKey, | ||
| String orderId, | ||
| String orderName, | ||
| String mId, | ||
| String currency, | ||
| String method, | ||
| Long totalAmount, | ||
| Long balanceAmount, | ||
| TosspaymentsStatus status, | ||
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime requestedAt, | ||
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime approvedAt, | ||
| String lastTransactionKey, | ||
| Long suppliedAmount, | ||
| Long vat, | ||
| Long taxFreeAmount, | ||
| Long taxExemptionAmount, | ||
| boolean isPartialCancelable, | ||
| WebhookEasyPay easyPay, | ||
| String country | ||
| ) { |
There was a problem hiding this comment.
웹훅 DTO 필수 필드 검증이 없어 잘못된 payload가 그대로 통과할 수 있습니다.
서비스 로직에서 사용하는 값(paymentKey, orderId, status, totalAmount)에 기본 제약을 두고, 중첩 DTO는 @Valid로 검증 전파를 거는 것이 안전합니다.
🛡️ 제안 수정
+import jakarta.validation.Valid;
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.NotNull;
@@
public record WebhookPaymentData(
- String paymentKey,
- String orderId,
+ `@NotBlank` String paymentKey,
+ `@NotBlank` String orderId,
@@
- Long totalAmount,
+ `@NotNull` Long totalAmount,
@@
- TosspaymentsStatus status,
+ `@NotNull` TosspaymentsStatus status,
@@
- WebhookEasyPay easyPay,
+ `@Valid` WebhookEasyPay easyPay,
String country
) {Also applies to: 35-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.java`
around lines 11 - 33, Add validation to the WebhookPaymentData record by
annotating required fields (paymentKey, orderId, status, totalAmount) with
appropriate javax/jakarta.validation constraints (e.g.,
`@NotBlank/`@NotNull/@Positive) and annotate the nested WebhookEasyPay field with
`@Valid` to propagate validation; then implement a compact record constructor for
WebhookPaymentData that performs entity-level checks (null/blank/invalid amount
checks for paymentKey, orderId, status, totalAmount) and throws an
IllegalArgumentException for invalid input so malformed payloads are rejected at
construction time.
🛠️ Refactor suggestion | 🟠 Major
DTO 네이밍을 Request/Response suffix로 맞추는 것이 좋습니다.
WebhookPaymentData는 저장소 규칙상 DTO 접미사 규칙과 불일치합니다. 예: WebhookPaymentDataRequest.
As per coding guidelines "Name DTOs with 'Request' or 'Response' suffix".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.java`
around lines 11 - 33, Rename the DTO record WebhookPaymentData to follow the
project's DTO naming convention by changing its type name to
WebhookPaymentDataRequest and update the source filename accordingly; then
update all references/usages (variable types, constructors, method parameters,
imports, serializers/deserializers, tests, and any mapping code) from
WebhookPaymentData to WebhookPaymentDataRequest so compilation and JSON
(TossDateTimeDeserializer) annotations remain intact.
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime requestedAt, | ||
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime approvedAt, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'WebhookPaymentData.java'Repository: samhap-soft/kokomen-backend
Length of output: 147
🏁 Script executed:
fd 'TossDateTimeDeserializer.java'Repository: samhap-soft/kokomen-backend
Length of output: 154
🏁 Script executed:
cat -n src/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.javaRepository: samhap-soft/kokomen-backend
Length of output: 1752
🏁 Script executed:
cat -n src/main/java/com/samhap/kokomen/payment/external/dto/TossDateTimeDeserializer.javaRepository: samhap-soft/kokomen-backend
Length of output: 1039
TossDateTimeDeserializer에서 null 값 처리 누락으로 인한 역직렬화 실패 위험
TossDateTimeDeserializer.deserialize() (라인 16-17)는 null 토큰을 검사하지 않으므로, 웹훅 응답의 requestedAt 또는 approvedAt 필드가 null인 경우 OffsetDateTime.parse(null, ...)에서 예외가 발생하고 요청 처리가 실패합니다.
다음과 같이 null 값을 명시적으로 처리하세요:
수정 제안 (TossDateTimeDeserializer.java)
`@Override`
public LocalDateTime deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
+ if (p.currentToken() == com.fasterxml.jackson.core.JsonToken.VALUE_NULL) {
+ return null;
+ }
String dateTimeStr = p.getText();
+ if (dateTimeStr == null || dateTimeStr.isBlank()) {
+ return null;
+ }
OffsetDateTime offsetDateTime = OffsetDateTime.parse(dateTimeStr, DateTimeFormatter.ISO_OFFSET_DATE_TIME);
return offsetDateTime.atZoneSameInstant(ZoneId.of("Asia/Seoul")).toLocalDateTime();
}또한 WebhookPaymentData는 Woowacourse 코딩 가이드라인에 따라 WebhookPaymentDataResponse 등으로 이름을 변경하고, requestedAt과 approvedAt 필드에 @NotNull 주석을 추가하여 필수 필드임을 명시하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/WebhookPaymentData.java`
around lines 21 - 24, TossDateTimeDeserializer currently calls
OffsetDateTime.parse without checking for JSON nulls; update
TossDateTimeDeserializer.deserialize to handle JsonToken.VALUE_NULL (or
parser.getCurrentToken()==JsonToken.VALUE_NULL) and return null (or handle as
appropriate) before calling OffsetDateTime.parse to avoid exceptions when
requestedAt/approvedAt are null. Also rename class WebhookPaymentData to
WebhookPaymentDataResponse and annotate the requestedAt and approvedAt fields
with `@NotNull` to mark them required (update any references/usages/DTO mappings
accordingly).
| private void handleDone(TosspaymentsPayment payment, WebhookPaymentData data) { | ||
| if (payment.getState().canCompleteByWebhook()) { | ||
| PurchaseMetadata metadata = parseMetadata(payment.getMetadata()); | ||
| Long memberId = payment.getMemberId(); | ||
| int tokenCount = metadata.count(); | ||
|
|
||
| payment.updateState(PaymentState.COMPLETED); | ||
|
|
||
| TokenPurchase tokenPurchase = TokenPurchase.builder() | ||
| .memberId(memberId) | ||
| .paymentKey(payment.getPaymentKey()) | ||
| .orderId(payment.getOrderId()) | ||
| .totalAmount(payment.getTotalAmount()) | ||
| .orderName(payment.getOrderName()) | ||
| .productName(metadata.productName()) | ||
| .purchaseCount(tokenCount) | ||
| .unitPrice(metadata.unitPrice()) | ||
| .paymentMethod(data.method()) | ||
| .easyPayProvider(data.easyPay() != null ? data.easyPay().provider() : null) | ||
| .build(); | ||
| tokenPurchaseService.saveTokenPurchase(tokenPurchase); | ||
|
|
||
| tokenService.addPaidTokens(memberId, tokenCount); | ||
|
|
||
| log.info("웹훅으로 토큰 지급 완료 - memberId: {}, paymentKey: {}, tokenCount: {}", | ||
| memberId, payment.getPaymentKey(), tokenCount); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleDone에서 webhook 데이터와 저장된 결제 정보 간 검증 누락
PaymentFacadeService.confirmPayment에서는 validateTosspaymentsResult를 통해 paymentKey, orderId, totalAmount를 검증합니다. 그러나 웹훅 처리에서는 이 검증이 없어 webhook payload의 데이터와 DB 데이터 간 불일치가 발생해도 감지하지 못합니다.
webhook payload에서 온 data.orderId(), data.totalAmount() 등과 payment 엔티티의 값을 비교하는 검증 추가를 권장합니다.
♻️ 검증 로직 추가 제안
private void handleDone(TosspaymentsPayment payment, WebhookPaymentData data) {
if (payment.getState().canCompleteByWebhook()) {
+ payment.validateTosspaymentsResult(data.paymentKey(), data.orderId(), data.totalAmount());
+
PurchaseMetadata metadata = parseMetadata(payment.getMetadata());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java` around
lines 58 - 85, handleDone currently skips the same verification performed in
PaymentFacadeService.confirmPayment (validateTosspaymentsResult), so add the
missing checks: compare webhook fields (use WebhookPaymentData.method names like
data.paymentKey()/data.orderId()/data.totalAmount() or data.orderId(),
data.totalAmount() and data.paymentKey if available) against the persisted
TosspaymentsPayment values (payment.getPaymentKey(), payment.getOrderId(),
payment.getTotalAmount()) before proceeding; if any mismatch occurs, log a clear
warning including both sides and abort processing (return or throw) to avoid
issuing tokens or saving TokenPurchase; you can either call the existing
validateTosspaymentsResult helper if accessible or replicate its exact checks
inside handleDone.
| private void handleCanceled(TosspaymentsPayment payment, TosspaymentsStatus status) { | ||
| payment.updateState(PaymentState.CANCELED); | ||
| log.info("웹훅 취소 처리 - paymentKey: {}, status: {}", payment.getPaymentKey(), status); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleCanceled에서 상태 전이 검증 부재
현재 구현은 isCompleted() 체크 후에 이 메서드가 호출되지만, handleCanceled 자체에는 현재 상태에 대한 검증이 없습니다. 예를 들어 CLIENT_BAD_REQUEST나 SERVER_BAD_REQUEST 상태에서 CANCELED로 전이하는 것이 의도된 동작인지 확인이 필요합니다.
상태 머신 관점에서 유효한 전이만 허용하도록 명시적 검증을 추가하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java` around
lines 96 - 99, handleCanceled currently unconditionally sets payment state to
CANCELED without validating the current state; add an explicit
allowed-transition check inside handleCanceled (e.g., inspect payment.getState()
or payment.isXxx methods) and only call
payment.updateState(PaymentState.CANCELED) when the current state is one of the
valid predecessors (declare the allowed set such as CREATED, AUTHORIZED, or
whatever business rules require), otherwise log a warning/error and skip the
transition or throw an exception; reference the handleCanceled method,
TosspaymentsStatus parameter, PaymentState.CANCELED enum and
payment.updateState/getState to locate and implement the guard.
| ALTER TABLE token_purchase | ||
| DROP INDEX idx_token_purchase_payment_key, | ||
| ADD CONSTRAINT uk_token_purchase_payment_key UNIQUE (payment_key); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/src/main/resources/db/migration/V41*" -type f 2>/dev/null | head -20Repository: samhap-soft/kokomen-backend
Length of output: 162
🏁 Script executed:
# Check if migration directory exists and list migration files
if [ -d "src/main/resources/db/migration" ]; then
echo "=== Migration directory exists ==="
ls -la src/main/resources/db/migration/ | grep -E "^-|^d" | tail -20
else
echo "Migration directory not found at src/main/resources/db/migration"
fiRepository: samhap-soft/kokomen-backend
Length of output: 1908
🏁 Script executed:
# Search for any V41 migration file anywhere in the repo
find . -name "*V41*" -type f 2>/dev/nullRepository: samhap-soft/kokomen-backend
Length of output: 162
🏁 Script executed:
cat -n src/main/resources/db/migration/V41__add_unique_constraint_token_purchase_payment_key.sqlRepository: samhap-soft/kokomen-backend
Length of output: 236
🏁 Script executed:
# Check if there are other migration files related to token_purchase table
grep -l "token_purchase" src/main/resources/db/migration/*.sql 2>/dev/nullRepository: samhap-soft/kokomen-backend
Length of output: 305
🏁 Script executed:
cat -n src/main/resources/db/migration/V17__create_token_purchase_table.sqlRepository: samhap-soft/kokomen-backend
Length of output: 2026
🏁 Script executed:
cat -n src/main/resources/db/migration/V20__add_payment_info_to_token_purchase.sqlRepository: samhap-soft/kokomen-backend
Length of output: 390
마이그레이션 실행 전 중복 payment_key 값 확인 필요
token_purchase 테이블의 payment_key 컬럼에 중복된 값이 존재할 경우 UNIQUE 제약조건 추가 시 마이그레이션이 실패합니다. 프로덕션 배포 전 다음 쿼리로 중복 여부를 확인하세요:
SELECT payment_key, COUNT(*) FROM token_purchase
GROUP BY payment_key HAVING COUNT(*) > 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/resources/db/migration/V41__add_unique_constraint_token_purchase_payment_key.sql`
around lines 1 - 3, Before adding the UNIQUE constraint
uk_token_purchase_payment_key on token_purchase.payment_key (and dropping
idx_token_purchase_payment_key), ensure there are no duplicate payment_key
values by running the provided GROUP BY query; if duplicates exist, remove or
consolidate them (e.g., deduplicate or backfill null/unique values) or add a
pre-migration step that identifies and resolves duplicates (delete/merge/update
or archive conflicting rows) so the ALTER TABLE statement will succeed during
deployment.
| Member member = memberRepository.save(MemberFixtureBuilder.builder().build()); | ||
| Long memberId = member.getId(); | ||
| tokenRepository.save(TokenFixtureBuilder.builder().memberId(memberId).type(TokenType.PAID).tokenCount(0).build()); | ||
|
|
||
| String metadata = objectMapper.writeValueAsString(new PurchaseMetadata("TOKEN_10", 10, 100L)); | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("webhook_test_key") | ||
| .memberId(memberId) | ||
| .orderId("webhook_order_1") | ||
| .totalAmount(1000L) | ||
| .metadata(metadata) | ||
| .build(); | ||
| payment.updateState(PaymentState.NEED_CANCEL); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
|
|
||
| WebhookPayload payload = createWebhookPayload("webhook_test_key", "webhook_order_1", TosspaymentsStatus.DONE, 1000L, "카드", null); | ||
| webhookService.handlePaymentStatusChanged(payload); | ||
|
|
||
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("webhook_test_key").orElseThrow(); | ||
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.COMPLETED); | ||
|
|
||
| Token paidToken = tokenRepository.findByMemberIdAndType(memberId, TokenType.PAID).orElseThrow(); | ||
| assertThat(paidToken.getTokenCount()).isEqualTo(10); | ||
|
|
||
| assertThat(tokenPurchaseRepository.findAll()).hasSize(1); | ||
| TokenPurchase tokenPurchase = tokenPurchaseRepository.findAll().get(0); | ||
| assertThat(tokenPurchase.getMemberId()).isEqualTo(memberId); | ||
| assertThat(tokenPurchase.getPurchaseCount()).isEqualTo(10); | ||
| assertThat(tokenPurchase.getPaymentKey()).isEqualTo("webhook_test_key"); | ||
| assertThat(tokenPurchase.getPaymentMethod()).isEqualTo("카드"); | ||
| } | ||
|
|
||
| @Test | ||
| void CONNECTION_TIMEOUT_상태에서_결제완료_웹훅_수신시_토큰이_지급된다() throws JsonProcessingException { | ||
| Member member = memberRepository.save(MemberFixtureBuilder.builder().build()); | ||
| Long memberId = member.getId(); | ||
| tokenRepository.save(TokenFixtureBuilder.builder().memberId(memberId).type(TokenType.PAID).tokenCount(0).build()); | ||
|
|
||
| String metadata = objectMapper.writeValueAsString(new PurchaseMetadata("TOKEN_20", 20, 100L)); | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("timeout_test_key") | ||
| .memberId(memberId) | ||
| .orderId("timeout_order_1") | ||
| .totalAmount(2000L) | ||
| .metadata(metadata) | ||
| .build(); | ||
| payment.updateState(PaymentState.CONNECTION_TIMEOUT); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
|
|
||
| WebhookPayload payload = createWebhookPayload("timeout_test_key", "timeout_order_1", TosspaymentsStatus.DONE, 2000L, "카드", null); | ||
| webhookService.handlePaymentStatusChanged(payload); | ||
|
|
||
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("timeout_test_key").orElseThrow(); | ||
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.COMPLETED); | ||
|
|
||
| Token paidToken = tokenRepository.findByMemberIdAndType(memberId, TokenType.PAID).orElseThrow(); | ||
| assertThat(paidToken.getTokenCount()).isEqualTo(20); | ||
| } | ||
|
|
||
| @Test | ||
| void NEED_APPROVE_상태에서_결제완료_웹훅_수신시_토큰이_지급된다() throws JsonProcessingException { | ||
| Member member = memberRepository.save(MemberFixtureBuilder.builder().build()); | ||
| Long memberId = member.getId(); | ||
| tokenRepository.save(TokenFixtureBuilder.builder().memberId(memberId).type(TokenType.PAID).tokenCount(0).build()); | ||
|
|
||
| String metadata = objectMapper.writeValueAsString(new PurchaseMetadata("TOKEN_10", 10, 100L)); | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("approve_test_key") | ||
| .memberId(memberId) | ||
| .orderId("approve_order_1") | ||
| .totalAmount(1000L) | ||
| .metadata(metadata) | ||
| .build(); | ||
| tosspaymentsPaymentRepository.save(payment); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
테스트 데이터 준비 코드 중복을 헬퍼로 추출하면 유지보수가 쉬워집니다.
회원/토큰/결제 생성 패턴이 반복되어 시나리오 추가 시 수정 포인트가 늘어납니다. 공통 setup 메서드로 모으면 가독성과 변경 내성이 좋아집니다.
♻️ 제안 예시
+ private Long 회원과유료토큰을생성한다(int 초기토큰수) {
+ Member member = memberRepository.save(MemberFixtureBuilder.builder().build());
+ Long memberId = member.getId();
+ tokenRepository.save(
+ TokenFixtureBuilder.builder()
+ .memberId(memberId)
+ .type(TokenType.PAID)
+ .tokenCount(초기토큰수)
+ .build()
+ );
+ return memberId;
+ }Also applies to: 142-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java`
around lines 52 - 125, Tests in WebhookServiceTest duplicate
member/token/payment setup across methods; extract a reusable helper (e.g., a
private method in WebhookServiceTest) that creates and returns the prepared
entities or identifiers to reduce repetition: factor out the Member/Token
creation using MemberFixtureBuilder and TokenFixtureBuilder and the
TosspaymentsPayment creation using TosspaymentsPaymentFixtureBuilder (including
setting state via payment.updateState(...) when needed), and reuse
createWebhookPayload calls by returning the paymentKey/orderId and metadata so
each test just calls the helper to get prepped memberId/paymentKey and then
invokes webhookService.handlePaymentStatusChanged.
![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
이번 PR은 토스페이먼츠 웹훅 수신을 위한 엔드포인트를 구현하는 내용이네요. 컨트롤러, 서비스, DTO 및 테스트 코드까지 구조적으로 잘 작성되었습니다. 결제 승인 시 분산락을 적용하고 데이터베이스 제약 조건을 강화하는 등 좋은 개선점도 보입니다. 다만, 몇 가지 중요한 점검이 필요합니다. 웹훅 처리 로직에 심각한 보안 취약점이 존재하며, 부분 취소 처리 로직이 불완전해 보입니다. 이 부분들에 대해 상세한 리뷰 의견을 남겼으니 확인 부탁드립니다.
| public void handlePaymentStatusChanged(WebhookPayload payload) { | ||
| WebhookPaymentData data = payload.data(); | ||
| String paymentKey = data.paymentKey(); | ||
| TosspaymentsStatus webhookStatus = data.status(); |
There was a problem hiding this comment.
보안 취약점: 웹훅 페이로드 신뢰
현재 웹훅 핸들러는 토스페이먼츠로부터 받은 웹훅 페이로드의 status 값을 그대로 신뢰하여 결제 처리 로직을 수행하고 있습니다. 이는 심각한 보안 취약점으로 이어질 수 있습니다. 공격자가 paymentKey를 알아낸 후 위조된 'DONE' 상태의 웹훅을 보내면, 실제 결제가 완료되지 않았음에도 토큰이 지급될 수 있습니다.
토스페이먼츠 공식 문서에서는 웹훅 수신 시, 웹훅에 포함된 paymentKey를 사용하여 결제 조회 API를 호출하여 최종 결제 상태를 확인하고 비즈니스 로직을 처리하도록 권장하고 있습니다.
웹훅은 단지 상태 변경 '알림'으로만 사용하고, 실제 상태는 항상 API를 통해 직접 조회하여 확인해야 합니다.
수정 제안:
handlePaymentStatusChanged 메소드 내부에서 payload.data().status()를 사용하기 전에, tosspaymentsClient를 사용하여 토스페이먼츠의 결제 조회 API를 호출하고, 그 응답에 있는 status를 기준으로 로직을 분기하도록 수정해야 합니다.
|
|
||
| switch (webhookStatus) { | ||
| case DONE -> handleDone(payment, data); | ||
| case CANCELED, PARTIAL_CANCELED -> handleCanceled(payment, webhookStatus); |
There was a problem hiding this comment.
부분 취소(PARTIAL_CANCELED) 처리 로직 검토 필요
현재 switch 문에서 PARTIAL_CANCELED 상태가 CANCELED와 동일하게 처리되어 handleCanceled 메소드를 호출하고 있습니다. handleCanceled는 결제 상태를 PaymentState.CANCELED로만 변경합니다.
하지만 부분 취소는 전체 취소와는 다른 비즈니스 로직이 필요할 수 있습니다. 예를 들어,
PaymentState에PARTIAL_CANCELED상태 추가- 부분 취소된 금액만큼
TokenPurchase의remainingCount를 조정하거나, 별도의 환불 내역 기록
현재 구현은 부분 취소가 발생해도 전체가 취소된 것처럼 처리될 수 있어 데이터 불일치를 유발할 수 있습니다. PARTIAL_CANCELED에 대한 정확한 처리 정책을 확인하고 로직을 수정하는 것을 권장합니다.
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
|
|
||
| class WebhookServiceTest extends BaseTest { |
There was a problem hiding this comment.
테스트 케이스 추가 제안: 부분 취소(PARTIAL_CANCELED)
WebhookService의 동작을 검증하기 위해 다양한 테스트 케이스를 작성해주셨습니다. 다만, TosspaymentsStatus.PARTIAL_CANCELED 상태의 웹훅을 수신했을 때의 동작을 확인하는 테스트 케이스가 누락되어 있습니다.
부분 취소 시 의도한 대로 동작하는지, 예를 들어 결제 상태가 어떻게 변경되는지 등을 검증하는 테스트를 추가하면 코드의 안정성을 더욱 높일 수 있습니다.
1 participant
closed #335
작업 내용
스크린샷
참고 사항