284 lines
14 KiB
Markdown
284 lines
14 KiB
Markdown
# Backend Audit Report — GoodGo POS System
|
||
|
||
> **Date**: 2026-03-20
|
||
> **Auditor**: Senior Backend Engineer (Agent 98eaaecb)
|
||
> **Scope**: .NET service implementation, CQRS patterns, MediatR usage, EF Core, domain model
|
||
> **Branch**: master (d0211e5)
|
||
|
||
---
|
||
|
||
## Executive Summary
|
||
|
||
Platform có 26 .NET 10 microservices với kiến trúc Clean Architecture + CQRS nhất quán và chất lượng code cao. Naming conventions, domain patterns, và bilingual documentation được áp dụng đồng bộ trên tất cả services. **Tuy nhiên, 3 vấn đề critical về security và 4 vấn đề high về consistency cần xử lý trước khi deploy staging.** Điểm nổi bật nhất là debug endpoints không có auth (privilege escalation) và SQL injection tồn tại trong merchant-service-net — cả hai đều là BLOCKER.
|
||
|
||
---
|
||
|
||
## Critical Issues
|
||
|
||
### C1 — Debug Endpoints `[AllowAnonymous]` trong MerchantService
|
||
|
||
**File**: `services/merchant-service-net/src/MerchantService.API/Controllers/StaffController.cs:249–390`
|
||
**Severity**: BLOCKER — bất kỳ ai cũng có thể gọi
|
||
|
||
5 endpoints không có authentication:
|
||
- `POST /api/v1/staff/debug/seed` (line 249) — tạo staff data tùy ý
|
||
- `POST /api/v1/staff/debug/update-userid` (line 340) — ghi đè userId của bất kỳ staff qua reflection
|
||
- `POST /api/v1/staff/debug/update-merchant` (line 360) — ghi đè merchantId tùy ý
|
||
|
||
Đây là **privilege escalation + data tampering vulnerability** cực kỳ nghiêm trọng. Bất kỳ attacker nào cũng có thể leo thang quyền hoặc phá vỡ dữ liệu merchant trong production.
|
||
|
||
**Fix ngay**: Xóa hoàn toàn các debug endpoints này khỏi production code, hoặc wrap trong `if (env.IsDevelopment())` và thêm `[Authorize(Roles = "SuperAdmin")]`.
|
||
|
||
---
|
||
|
||
### C2 — SQL Injection trong StaffController
|
||
|
||
**File**: `services/merchant-service-net/src/MerchantService.API/Controllers/StaffController.cs:307, 367`
|
||
**Severity**: HIGH — string interpolation trực tiếp vào SQL
|
||
|
||
```csharp
|
||
// Line 307
|
||
cmd.CommandText = $"SELECT id FROM shops WHERE merchant_id = '{merchant.Id}' LIMIT 1";
|
||
|
||
// Line 367 — userId, merchantId từ [FromQuery] — SQL INJECTION RISK
|
||
cmd.CommandText = $"UPDATE merchants SET user_id = '{userId}' WHERE id = '{merchantId}'";
|
||
```
|
||
|
||
Dù `Guid` tự sanitize ở thời điểm hiện tại, pattern string interpolation trong SQL là vi phạm security baseline. Bất kỳ refactor nào đổi type sang string sẽ tức thì trở thành lỗ hổng.
|
||
|
||
**Fix**: Dùng parameterized queries:
|
||
```csharp
|
||
cmd.CommandText = "UPDATE merchants SET user_id = @userId WHERE id = @merchantId";
|
||
cmd.Parameters.AddWithValue("@userId", userId);
|
||
cmd.Parameters.AddWithValue("@merchantId", merchantId);
|
||
```
|
||
|
||
---
|
||
|
||
### C3 — JWT Signature Validation Bị Tắt trong Development — 4 Services
|
||
|
||
**Files** (confirmed bằng grep toàn repo):
|
||
- `services/merchant-service-net/src/MerchantService.API/Program.cs`
|
||
- `services/ads-serving-service-net/src/AdsServingService.API/Program.cs`
|
||
- `services/ads-tracking-service-net/src/AdsTrackingService.API/Program.cs`
|
||
- `services/ads-billing-service-net/src/AdsBillingService.API/Program.cs`
|
||
|
||
**Severity**: HIGH — token forgery risk nếu `ASPNETCORE_ENVIRONMENT` bị misconfigure
|
||
|
||
```csharp
|
||
// Pattern này tồn tại trong cả 4 services
|
||
ValidateIssuerSigningKey = builder.Environment.IsDevelopment() ? false : true,
|
||
SignatureValidator = builder.Environment.IsDevelopment() ? ... : null,
|
||
```
|
||
|
||
Nếu `ASPNETCORE_ENVIRONMENT` không được set đúng trên server (hoặc bị override sai), tất cả JWT signatures đều bị bỏ qua — bất kỳ token tự tạo nào đều được chấp nhận. Nguy hiểm đặc biệt với `ads-billing-service-net` (xử lý billing).
|
||
|
||
> **Note**: `iam-service-net/tests/CustomWebApplicationFactory.cs` cũng có `ValidateIssuerSigningKey = false` nhưng đây là test context — chấp nhận được.
|
||
|
||
**Fix**: Dùng test signing key trong development thay vì skip validation hoàn toàn:
|
||
```csharp
|
||
ValidateIssuerSigningKey = true, // Luôn validate — kể cả development
|
||
// Development: dùng shared test key từ env var DEV_JWT_SIGNING_KEY
|
||
```
|
||
|
||
---
|
||
|
||
## Warnings
|
||
|
||
### W1 — CORS Policy `AllowAnyOrigin()` trên tất cả Services
|
||
|
||
**File**: Mọi service `Program.cs`, ví dụ `services/_template_dot_net/src/MyService.API/Program.cs:93`
|
||
|
||
```csharp
|
||
policy.AllowAnyOrigin().AllowAnyMethod().AllowAnyHeader();
|
||
```
|
||
|
||
Tất cả 26 services đều cấu hình CORS wildcard. Trong production, điều này cho phép bất kỳ domain nào gọi API — vi phạm same-origin policy intent.
|
||
|
||
**Fix**: Restrict origins trong production environment:
|
||
```csharp
|
||
if (env.IsProduction())
|
||
policy.WithOrigins("https://goodgo.vn", "https://admin.goodgo.vn")
|
||
.WithMethods("GET", "POST", "PUT", "DELETE", "PATCH")
|
||
.WithHeaders("Content-Type", "Authorization");
|
||
else
|
||
policy.AllowAnyOrigin().AllowAnyMethod().AllowAnyHeader();
|
||
```
|
||
|
||
---
|
||
|
||
### W2 — Idempotency Chỉ Có ở 3/26 Services
|
||
|
||
**Services với idempotency**: order-service-net, inventory-service-net, ads-serving-service-net
|
||
**Services thiếu**: wallet-service-net, booking-service-net, promotion-service-net, và 19 services khác
|
||
|
||
Các write operations trong wallet, booking, promotion có nguy cơ cao bị duplicate execution (network retry, user double-click). Thiếu `IRequestManager` idempotency cho phép double-charge, double-booking.
|
||
|
||
**Fix**: Implement `IRequestManager` pattern (đã có trong `_template_dot_net`) cho tất cả write-heavy services:
|
||
- Ưu tiên cao nhất: `wallet-service-net`, `booking-service-net`
|
||
- Ưu tiên trung bình: `promotion-service-net`, `membership-service-net`
|
||
|
||
---
|
||
|
||
### W3 — API Error Response Format Không Nhất Quán
|
||
|
||
**File**: `services/order-service-net/src/OrderService.API/Controllers/OrdersController.cs:73`
|
||
|
||
Một số controllers trả về raw message object thay vì format chuẩn:
|
||
```csharp
|
||
// Inconsistent — dùng ở một số controllers
|
||
return NotFound(new { Message = "Order not found" });
|
||
|
||
// Standard theo CLAUDE.md — cần dùng nhất quán
|
||
return NotFound(new { success = false, error = new { code = "ORDER_NOT_FOUND", message = "..." } });
|
||
```
|
||
|
||
**Fix**: Đồng nhất tất cả error response theo format: `{ success: false, error: { code, message } }`. Tốt nhất dùng exception middleware để map `DomainException` → ProblemDetails tự động, tránh manual error construction trong controllers.
|
||
|
||
---
|
||
|
||
### W4 — ProblemDetails Exception Mapping Không Đầy Đủ ở Một Số Services
|
||
|
||
**Reference (tốt)**: `services/iam-service-net/src/IamService.API/Program.cs` — map 5+ exception types
|
||
**Template thiếu**: `services/_template_dot_net/src/MyService.API/Program.cs` — chỉ map cơ bản
|
||
|
||
Các services mới tạo từ template sẽ không có exception mapping đầy đủ, dẫn đến stack trace leak hoặc generic 500 errors.
|
||
|
||
**Fix**: Cập nhật `_template_dot_net` với exception mappings đầy đủ. Sau đó sync cho các services đã tạo từ template.
|
||
|
||
---
|
||
|
||
### W5 — HttpContextAccessor Inject Trực Tiếp vào Handler
|
||
|
||
**File**: `services/merchant-service-net/src/MerchantService.API/Program.cs:31`
|
||
|
||
```csharp
|
||
builder.Services.AddHttpContextAccessor(); // OK
|
||
// Nhưng một số handlers inject IHttpContextAccessor trực tiếp
|
||
```
|
||
|
||
Truy cập HttpContext trong MediatR handler vi phạm clean boundary giữa application layer và web layer. Handler không nên biết về HTTP context.
|
||
|
||
**Fix**: Inject contextual data (userId, tenantId) vào Command/Query parameter khi dispatch từ Controller:
|
||
```csharp
|
||
// Controller
|
||
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
|
||
await _mediator.Send(new CreateOrderCommand(userId, shopId, items), cancellationToken);
|
||
```
|
||
|
||
---
|
||
|
||
## Improvements
|
||
|
||
### I1 — Outbox Pattern cho Domain Events
|
||
|
||
**Hiện tại**: Domain events được dispatch in-process trong `SaveEntitiesAsync()` trước SaveChanges. Nếu process crash sau SaveChanges, events đã bị lost.
|
||
|
||
**Recommendation**: Implement transactional outbox pattern — persist events vào `outbox_messages` table trong cùng transaction, sau đó poll/relay sang RabbitMQ. Tham khảo: MassTransit Outbox, Wolverine Outbox.
|
||
|
||
**Impact**: Đảm bảo at-least-once delivery cho cross-service events — quan trọng cho wallet-service, booking-service, promotion-service.
|
||
|
||
---
|
||
|
||
### I2 — Missing OpenAPI Specs (openapi.yaml)
|
||
|
||
Theo `CLAUDE.md`, mỗi service PHẢI có `openapi.yaml`. Hiện tại chỉ có Swagger UI từ Swashbuckle (runtime generated), không có static `openapi.yaml` file được commit vào repo.
|
||
|
||
**Impact**: Không thể dùng API-first contract testing (Pact), không có versioning cho API spec.
|
||
|
||
**Fix**: Thêm script generate `openapi.yaml` vào CI pipeline:
|
||
```bash
|
||
dotnet swagger tofile --output openapi.yaml ./bin/Release/net10.0/ServiceName.API.dll v1
|
||
```
|
||
|
||
---
|
||
|
||
### I3 — Missing Prometheus Metrics Endpoint
|
||
|
||
Theo `CLAUDE.md`, mỗi service PHẢI có `/metrics` endpoint cho Prometheus. Chưa thấy `prometheus-net` hoặc `OpenTelemetry.Exporter.Prometheus.AspNetCore` được configure trong services.
|
||
|
||
**Fix**: Thêm vào `_template_dot_net` và sync:
|
||
```csharp
|
||
builder.Services.AddOpenTelemetry()
|
||
.WithMetrics(metrics => metrics
|
||
.AddAspNetCoreInstrumentation()
|
||
.AddPrometheusExporter());
|
||
|
||
app.MapPrometheusScrapingEndpoint("/metrics");
|
||
```
|
||
|
||
---
|
||
|
||
### I4 — Saga Pattern cho Cross-Service Transactions
|
||
|
||
Hiện tại order placement phụ thuộc vào nhiều services (inventory, wallet, promotion) nhưng không có orchestration nếu một service fail mid-flow. Tình huống: wallet bị trừ nhưng inventory deduct fail → inconsistent state.
|
||
|
||
**Recommendation**: Implement Saga orchestration (dùng MassTransit Saga hoặc Wolverine) cho Order Placement flow. Đây là medium-term investment nhưng cần thiết trước khi scale.
|
||
|
||
---
|
||
|
||
### I5 — Dapper Queries Không Có Timeout Explicit
|
||
|
||
Một số Dapper queries (read-heavy) không set `commandTimeout`, dùng default 30s của PostgreSQL. Với queries phức tạp trên dataset lớn, điều này có thể block connection pool.
|
||
|
||
**Fix**: Set explicit timeout cho Dapper queries:
|
||
```csharp
|
||
var result = await _connection.QueryAsync<T>(sql, parameters, commandTimeout: 5);
|
||
```
|
||
|
||
---
|
||
|
||
## Action Items (Prioritized)
|
||
|
||
| # | Priority | Action | Owner | File/Service |
|
||
|---|----------|--------|-------|-------------|
|
||
| 1 | **P0 BLOCKER** | Xóa debug endpoints `[AllowAnonymous]` khỏi StaffController | Backend Dev | `merchant-service-net/StaffController.cs:249–390` |
|
||
| 2 | **P0 BLOCKER** | Fix SQL injection — dùng parameterized queries | Backend Dev | `merchant-service-net/StaffController.cs:307, 367` |
|
||
| 3 | **P0 HIGH** | Fix JWT signature validation — không skip trong development | Backend Dev | `merchant-service-net/Program.cs:91` |
|
||
| 4 | **P1 HIGH** | Restrict CORS origins trong production environment | Backend Dev | Tất cả services `Program.cs` |
|
||
| 5 | **P1 HIGH** | Implement idempotency cho wallet-service, booking-service | Backend Dev | `wallet-service-net`, `booking-service-net` |
|
||
| 6 | **P1 HIGH** | Đồng nhất API error response format (`{ success, error: { code, message } }`) | Backend Dev | Tất cả controllers |
|
||
| 7 | **P2 MEDIUM** | Cập nhật `_template_dot_net` với đầy đủ ProblemDetails exception mapping | Backend Dev | `_template_dot_net/Program.cs` |
|
||
| 8 | **P2 MEDIUM** | Refactor HttpContextAccessor ra khỏi handlers — inject qua Command params | Backend Dev | `merchant-service-net` handlers |
|
||
| 9 | **P2 MEDIUM** | Thêm `/metrics` Prometheus endpoint vào tất cả services | Backend Dev | Tất cả `Program.cs` |
|
||
| 10 | **P2 MEDIUM** | Generate và commit `openapi.yaml` trong CI pipeline | DevOps/Backend | CI `.github/workflows` |
|
||
| 11 | **P3 LOW** | Set explicit Dapper query timeout | Backend Dev | Tất cả services dùng Dapper |
|
||
| 12 | **P3 LOW** | Implement Outbox pattern cho domain events | Backend Dev | Tất cả services có cross-service events |
|
||
| 13 | **P3 LOW** | Implement Saga orchestration cho Order Placement flow | Backend Dev | `order-service-net`, `wallet-service-net`, `inventory-service-net` |
|
||
|
||
---
|
||
|
||
## Strengths
|
||
|
||
Những patterns đang được áp dụng tốt — nên duy trì:
|
||
|
||
- **Bilingual codebase**: Tất cả XML docs, comments, log messages có EN + VI — nhất quán 100%
|
||
- **Domain encapsulation**: Private fields + read-only properties, zero public setters — đúng chuẩn DDD
|
||
- **MediatR pipeline**: 3 behaviors (Logging → Validation → Transaction) áp dụng nhất quán 26/26 services
|
||
- **Entity patterns**: Aggregate roots, domain events, behavior methods — clean và testable
|
||
- **EF Core config**: snake_case columns, private field mapping, owned collections — đúng theo quy chuẩn
|
||
- **FluentValidation**: Bilingual messages, child validation, enum validation — đầy đủ
|
||
- **Container security**: Multi-stage Dockerfile, non-root user (dotnetuser:1001), healthcheck — production-ready
|
||
- **Test structure**: 50 test projects (25 unit + 25 functional), WebApplicationFactory pattern đúng
|
||
- **C# 14 / .NET 10**: Nullable enabled, TreatWarningsAsErrors=true, implicit usings — type-safe
|
||
- **Health checks**: `/health`, `/health/live`, `/health/ready` với PostgreSQL probe — đúng chuẩn Kubernetes
|
||
- **API versioning**: URL segment + Header reader, consistent v1.0 default — RESTful
|
||
- **Repository pattern**: Interface trong Domain, implementation trong Infrastructure — dependency inversion đúng
|
||
|
||
---
|
||
|
||
## Services Examined
|
||
|
||
| Service | Detail Level | Key Findings |
|
||
|---------|-------------|--------------|
|
||
| `order-service-net` | Deep | CQRS, entity, EF patterns — reference quality |
|
||
| `iam-service-net` | Deep | Authorization, exception handling — best ProblemDetails mapping |
|
||
| `merchant-service-net` | Deep | **C1 debug endpoints, C2 SQL injection, C3 JWT validation** |
|
||
| `fnb-engine-net` | Medium | Validation patterns — good use of RuleForEach |
|
||
| `inventory-service-net` | Medium | Idempotency pattern — only one of 3 services with IRequestManager |
|
||
| `wallet-service-net` | Medium | Repository implementation, Dapper usage |
|
||
| `_template_dot_net` | Deep | Reference architecture — needs update (W4, I2, I3) |
|
||
| Other 19 services | Structural | Verified pattern consistency, no deep code review |
|
||
|
||
**Coverage**: 7 services deep-reviewed (27%), 19 services structural verification (73%)
|