goodgo-platform/PAYMENT_SECURITY_CHECKLIST.md

# GoodGo Platform - Payment Module Security Checklist

## Critical Files for Security Review

### 🔴 HIGHEST PRIORITY (Review First)

#### 1. Callback Signature Verification
**Files:**
- `infrastructure/services/vnpay.service.ts` (lines 72-105)
- `infrastructure/services/momo.service.ts` (lines 102-147)
- `infrastructure/services/zalopay.service.ts` (lines 98-144)

**Security Checklist:**
- [ ] Verify crypto.timingSafeEqual() is used for all HMAC comparisons
- [ ] Confirm signature verification keys are correct
- [ ] Check that hash algorithms match provider specs (VNPay: SHA512, MoMo/ZaloPay: SHA256)
- [ ] Verify signature data reconstruction matches provider documentation exactly
- [ ] Test replay attack scenarios - are old callbacks rejected?
- [ ] Confirm parameter ordering in signature is correct
- [ ] Check for timing attacks - all implementations use constant-time compare
- [ ] Verify rawData logging doesn't leak sensitive signature data

---

#### 2. Race Condition Protection - Payment Callbacks
**File:**
- `application/commands/handle-callback/handle-callback.handler.ts` (lines 32-110)
- `infrastructure/repositories/prisma-payment.repository.ts` (lines 84-109)

**Security Checklist:**
- [ ] Confirm updateIfStatus() uses WHERE clause with status IN array
- [ ] Verify Prisma returns null on P2025 error correctly
- [ ] Test concurrent callback scenarios for same payment
- [ ] Verify idempotent response for already-processed payments
- [ ] Confirm events are only emitted once per unique callback
- [ ] Check that PROCESSING status is used as intermediate state
- [ ] Verify no race condition between null check and event publishing

---

#### 3. Race Condition Protection - Escrow Operations
**Files:**
- `application/commands/hold-escrow/hold-escrow.handler.ts` (lines 23-67)
- `application/commands/release-escrow/release-escrow.handler.ts` (lines 24-45)

**Security Checklist:**
- [ ] ❌ CRITICAL: No Redis lock present - concurrent requests can cause state corruption
- [ ] Check if multiple simultaneous hold operations exist in logs
- [ ] Verify no order/escrow can be in two states simultaneously
- [ ] Test: what happens if hold and release called concurrently?
- [ ] Test: what happens if hold called twice quickly?
- [ ] Implement: Redis distributed lock for escrow operations
- [ ] Document: expected behavior under concurrent access

---

#### 4. Financial Amount Validation
**Files:**
- `domain/value-objects/money.vo.ts` (lines 1-21)
- `domain/value-objects/platform-fee.vo.ts` (lines 1-31)

**Security Checklist:**
- [ ] Verify max limit 999_999_999_999 VND is enforced
- [ ] Confirm zero/negative amounts are rejected
- [ ] Test: can amount be set to negative via SQL injection?
- [ ] Verify platform fee calculation: (amount * 5) / 100 = correct
- [ ] Check: does fee calculation handle rounding correctly?
- [ ] Test: edge case of 1 VND order - correct fee?
- [ ] Verify seller payout calculation: amount - fee ≥ 0
- [ ] Test: can seller payout ever be negative?

---

### 🟠 HIGH PRIORITY

#### 5. Order/Escrow State Machine
**Files:**
- `domain/entities/order.entity.ts` (lines 22-32 state machine definition)
- `domain/entities/escrow.entity.ts` (lines 74-148 state transitions)

**Security Checklist:**
- [ ] Verify VALID_TRANSITIONS whitelist is complete and correct
- [ ] Test: can any invalid transition occur?
- [ ] Test: PAYMENT_PENDING → ESCROW_HELD without PAYMENT_CONFIRMED?
- [ ] Verify DISPUTE state can transition to ESCROW_RELEASED or REFUNDED
- [ ] Test: what happens if order tries to transition to invalid state?
- [ ] Check: are all state changes persisted atomically?
- [ ] Verify: timestamp fields updated correctly for each transition
- [ ] Test: out-of-order callbacks don't corrupt state

---

#### 6. Idempotency Protection
**Files:**
- `application/commands/create-order/create-order.handler.ts` (lines 32-38)
- `application/commands/create-payment/create-payment.handler.ts` (handler not fully shown)
- `infrastructure/repositories/prisma-order.repository.ts` (line 18-22)
- `infrastructure/repositories/prisma-payment.repository.ts` (lines 24-29)

**Security Checklist:**
- [ ] Verify idempotencyKey is unique per user/request
- [ ] Test: duplicate requests with same key return same order
- [ ] Test: duplicate requests with same key don't double-charge
- [ ] Check: is idempotencyKey stored in database?
- [ ] Verify: database unique constraint on idempotencyKey
- [ ] Test: what if callback arrives before order created?
- [ ] Test: what if payment created but callback lost?
- [ ] Check: TTL on idempotency keys to prevent bloat

---

#### 7. Authorization & Ownership Verification
**Files:**
- `presentation/controllers/orders.controller.ts` (lines 44-116)
- `presentation/controllers/payments.controller.ts` (lines 52-139)

**Security Checklist:**
- [ ] Verify JwtAuthGuard on all user endpoints
- [ ] Check: can user view other user's orders/payments?
- [ ] Verify: buyer authorization checked in order queries
- [ ] Verify: only seller/buyer can access their transactions
- [ ] Test: IDOR vulnerabilities - user A accessing user B's order
- [ ] Check: admin-only endpoints use RolesGuard
- [ ] Test: non-admin user can't call hold/release escrow
- [ ] Verify: user.sub (JWT subject) properly extracted

---

#### 8. Refund Security
**Files:**
- `application/commands/refund-payment/refund-payment.handler.ts` (not fully shown)
- `infrastructure/services/vnpay.service.ts` (lines 107-169)
- `infrastructure/services/momo.service.ts` (lines 149-202)
- `infrastructure/services/zalopay.service.ts` (lines 146-197)

**Security Checklist:**
- [ ] Only ADMIN role can initiate refunds
- [ ] Verify: refund amount ≤ original payment amount
- [ ] Check: can refund amount be negative?
- [ ] Test: can payment be refunded multiple times?
- [ ] Verify: refund status tracking in Payment entity
- [ ] Check: refund provider response validation
- [ ] Test: partial refunds - are multiple refunds tracked?
- [ ] Verify: funds actually sent back to customer (not to app)

---

#### 9. Rate Limiting on Callbacks
**File:**
- `presentation/controllers/payments.controller.ts` (lines 75-89)

**Security Checklist:**
- [ ] Confirm: @Throttle decorator with 20 requests per 60s
- [ ] Check: @EndpointRateLimit with 100 requests per 60s
- [ ] Verify: rate limit key is IP-based
- [ ] Test: callback flooding attack mitigated?
- [ ] Check: admin bypass disabled for callbacks
- [ ] Verify: rate limit storage mechanism (in-memory? Redis?)
- [ ] Test: legitimate callback bursts (payment provider retries)
- [ ] Check: rate limit errors logged appropriately

---

### 🟡 MEDIUM PRIORITY

#### 10. Configuration & Secrets Management
**Files:**
- `infrastructure/services/vnpay.service.ts` (lines 27-32)
- `infrastructure/services/momo.service.ts` (lines 27-31)
- `infrastructure/services/zalopay.service.ts` (lines 27-30)

**Security Checklist:**
- [ ] Verify: all secrets loaded from ConfigService (not hardcoded)
- [ ] Check: .env file is in .gitignore
- [ ] Confirm: secrets aren't logged anywhere
- [ ] Verify: hash secrets are properly long (recommend 32+ chars)
- [ ] Check: sandbox/production URLs separated by env
- [ ] Test: missing config throws error early (not at payment time)
- [ ] Verify: secret rotation mechanism exists or planned
- [ ] Check: env variables encrypted at rest in CI/CD

---

#### 11. Database Constraints
**Files:**
- Check Prisma schema for order/escrow/payment models

**Security Checklist:**
- [ ] Verify: unique constraints on idempotencyKey per user
- [ ] Check: NOT NULL constraints on critical fields
- [ ] Verify: ONE-TO-ONE relationship order ↔ escrow
- [ ] Check: foreign key constraints prevent orphans
- [ ] Verify: status fields have CHECK constraints or enums
- [ ] Check: amount fields are proper numeric types (not strings)
- [ ] Verify: no direct user-provided IDs used without validation
- [ ] Check: database indexes on frequently queried fields

---

#### 12. Error Handling & Information Disclosure
**Files:**
- All handlers catch errors and log appropriately

**Security Checklist:**
- [ ] Verify: error messages don't leak sensitive data
- [ ] Check: stack traces not exposed to clients
- [ ] Verify: generic error messages for failed operations
- [ ] Check: error codes are documented (E.g., OrderNotFound)
- [ ] Test: invalid amount shows appropriate error
- [ ] Verify: failed callbacks logged with provider context
- [ ] Check: refund failures don't expose retry mechanism
- [ ] Verify: no SQL queries exposed in error messages

---

#### 13. Logging & Audit Trail
**Files:**
- All handlers use logger.log() and logger.error()

**Security Checklist:**
- [ ] Verify: critical operations logged (payment, refund, escrow changes)
- [ ] Check: logs include user context (userId, orderId, etc)
- [ ] Verify: logs include timestamp and status transition
- [ ] Check: no sensitive data logged (payment secrets, full CC info)
- [ ] Verify: log rotation configured
- [ ] Check: logs are tamper-proof (signed/hashed)
- [ ] Test: audit trail shows complete order/payment lifecycle
- [ ] Verify: invalid callbacks logged with details for investigation

---

### 🟢 LOWER PRIORITY

#### 14. Test Coverage
**Files:**
- `application/__tests__/handle-callback-edge-cases.handler.spec.ts` (edge cases)
- All `__tests__` files

**Security Checklist:**
- [ ] Check: test coverage for callback signature verification
- [ ] Verify: tests for race condition scenarios
- [ ] Check: tests for idempotency edge cases
- [ ] Verify: tests for invalid state transitions
- [ ] Check: tests for authorization failures
- [ ] Verify: tests for concurrent escrow operations
- [ ] Check: tests for amount validation edge cases
- [ ] Verify: tests for provider failure scenarios

---

#### 15. API Documentation
**Files:**
- Controller decorators (@ApiOperation, @ApiResponse)

**Security Checklist:**
- [ ] Verify: endpoints documented with security requirements
- [ ] Check: response schemas don't include secrets
- [ ] Verify: rate limits documented
- [ ] Check: authorization requirements clearly stated
- [ ] Verify: error responses documented
- [ ] Check: webhook signature verification explained
- [ ] Verify: callback retry behavior documented
- [ ] Check: provider-specific behavior differences noted

---

## Attack Scenarios to Test

### Scenario 1: Callback Flooding
```
Attack: Send 1000 callbacks per second
Expected: Rate limiter blocks after 100 per 60s
Expected: Payment status unchanged after first successful callback
Check: No double-charging
```

### Scenario 2: Replay Attack
```
Attack: Resend old successful callback
Expected: Payment already in terminal state, idempotent response
Expected: No double-charging
Check: Logs show replay attempt
```

### Scenario 3: Concurrent Escrow Release
```
Attack: Call /orders/{id}/escrow/release twice simultaneously
Expected: One succeeds, one fails with ESCROW_INVALID_STATE
Current Risk: ⚠️ Could succeed twice without Redis lock
```

### Scenario 4: Forged Callback
```
Attack: Send callback with invalid HMAC signature
Expected: Validation exception, payment rejected
Check: Signature verification uses constant-time compare
```

### Scenario 5: Order/Escrow State Desync
```
Attack: Order in PAYMENT_CONFIRMED, Escrow in RELEASED
Expected: Invalid state machine - shouldn't be possible
Check: Are order + escrow updates atomic?
```

### Scenario 6: Integer Overflow
```
Attack: Send payment for 999_999_999_999 VND
Expected: Money VO rejects (max limit)
Attack: Send fee calculation for large amount
Expected: No integer overflow, correct 5% fee calculated
```

### Scenario 7: Authorization Bypass
```
Attack: Get another user's order ID, call /orders/{theirID}
Expected: 404 or Forbidden (not found to prevent enumeration)
Check: Ownership verified in query handler
```

### Scenario 8: Double Refund
```
Attack: Call /payments/{id}/refund twice
Expected: Second call fails (payment already REFUNDED)
Check: State machine prevents invalid transition
```

---

## Security Metrics

| Metric | Status | Target |
|--------|--------|--------|
| All callbacks verify HMAC signature | ✅ YES | 100% |
| Race conditions protected with locks/atomicity | ⚠️ PARTIAL | 100% (escrow ops need locks) |
| Idempotency keys enforced | ✅ YES | 100% |
| Authorization on all endpoints | ✅ YES | 100% |
| Amount validation (min/max) | ✅ YES | 100% |
| Rate limiting on callbacks | ✅ YES | 100% |
| Error messages don't leak secrets | ⚠️ NEEDS REVIEW | 100% |
| Logging captures audit trail | ✅ YES | 100% |
| Test coverage for security | ⚠️ PARTIAL | >80% |
| Database constraints | ⚠️ NEEDS VERIFICATION | 100% |

---

## Recommended Actions

### CRITICAL (Do Before Production)
1. [ ] **Implement Redis distributed lock for escrow hold/release operations**
   - Use `@nestjs/common` or external lock service
   - Prevent concurrent state mutations
   
2. [ ] **Add database constraints validation**
   - Verify Prisma schema has proper constraints
   - Add unique index on (userId, idempotencyKey)
   
3. [ ] **Audit all error messages**
   - Ensure no secrets leak in responses
   - Test error cases manually

### HIGH (Before First Deployment)
4. [ ] **Add comprehensive test suite**
   - Race condition tests
   - Callback replay tests
   - IDOR tests
   
5. [ ] **Secrets audit**
   - Verify no hardcoded values
   - Check .env/.gitignore
   - Document secret rotation procedure
   
6. [ ] **Stress test callbacks**
   - Simulate provider retry storms
   - Verify rate limiting works

### MEDIUM (Near-term)
7. [ ] **Add more detailed audit logging**
   - Payment status transitions
   - Failed callback attempts
   - Refund requests/approvals

8. [ ] **Create incident response playbook**
   - Double payment detection
   - Stuck order recovery
   - Provider integration issues

### NICE-TO-HAVE
9. [ ] **Field-level encryption for sensitive data**
   - Payment callback data
   - Provider transaction IDs
   
10. [ ] **Webhook signature verification monitoring**
    - Alert on verification failures
    - Track provider replay attempts