chore: organize docs — move 37 files from root into docs/ subfolders
Root now contains only essential files: README.md, CLAUDE.md, CHANGELOG.md, CONTRIBUTING.md Reorganized into: docs/audits/ — all audit reports & checklists (71 files) docs/architecture/ — codebase overview, implementation plan docs/guides/ — auth guide, implementation checklist docs/load-testing/ — k6 load test guides & endpoints docs/security/ — payment & security reviews Also removed 5 untracked debug/investigation files and cleaned up playwright-report/ & test-results/ artifacts. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ho Ngoc Hai
396
docs/security/PAYMENT_SECURITY_CHECKLIST.md
Normal file
396
docs/security/PAYMENT_SECURITY_CHECKLIST.md
Normal file
@@ -0,0 +1,396 @@
|
||||
# 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
|
||||
|
||||
Reference in New Issue
Block a user