docs: 添加系统性优化方案 (P1-P2)
This commit is contained in:
238
docs/code-review/OPTIMIZATION_PLAN_2026-04-03.md
Normal file
238
docs/code-review/OPTIMIZATION_PLAN_2026-04-03.md
Normal file
@@ -0,0 +1,238 @@
|
|||||||
|
# 系统性优化方案 - 2026-04-03
|
||||||
|
|
||||||
|
## 待处理问题
|
||||||
|
|
||||||
|
| # | 类别 | 问题 | 严重度 | 优先级 |
|
||||||
|
|---|------|------|--------|--------|
|
||||||
|
| 1 | 前端 | OAuth auth_url 缺少 origin 验证 | MEDIUM | P1 |
|
||||||
|
| 2 | 前端 | API 响应缺少运行时类型验证 | MEDIUM | P2 |
|
||||||
|
| 3 | 后端 | 缓存击穿 (cache stampede) 风险 | MEDIUM | P2 |
|
||||||
|
| 4 | 后端 | Webhook 服务 Goroutine 生命周期管理 | MEDIUM | P2 |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. OAuth auth_url 缺少 origin 验证
|
||||||
|
|
||||||
|
### 问题描述
|
||||||
|
`window.location.assign(result.auth_url)` 直接使用后端返回的 `auth_url`,未验证其 origin 是否属于可信域。
|
||||||
|
|
||||||
|
**风险**: 后端被攻击或配置错误时可能返回恶意 URL 导致用户被重定向到钓鱼站点。
|
||||||
|
|
||||||
|
**受影响文件**:
|
||||||
|
- `frontend/admin/src/pages/auth/LoginPage/LoginPage.tsx:137-141`
|
||||||
|
- `frontend/admin/src/pages/admin/ProfileSecurityPage/ProfileSecurityPage.tsx:195-205`
|
||||||
|
|
||||||
|
### 修复方案
|
||||||
|
|
||||||
|
在 `oauth.ts` 添加 origin 验证函数:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
export function validateOAuthUrl(authUrl: string): boolean {
|
||||||
|
try {
|
||||||
|
const url = new URL(authUrl)
|
||||||
|
// 仅允许同源或已知可信 OAuth 提供商
|
||||||
|
const trustedOrigins = [
|
||||||
|
'https://github.com',
|
||||||
|
'https://google.com',
|
||||||
|
'https://facebook.com',
|
||||||
|
// ... 其他可信提供商
|
||||||
|
]
|
||||||
|
return url.origin === window.location.origin ||
|
||||||
|
trustedOrigins.includes(url.origin)
|
||||||
|
} catch {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
然后在调用前验证:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const result = await getOAuthAuthorizationUrl(provider, ...)
|
||||||
|
if (!validateOAuthUrl(result.auth_url)) {
|
||||||
|
throw new Error('Invalid OAuth authorization URL')
|
||||||
|
}
|
||||||
|
window.location.assign(result.auth_url)
|
||||||
|
```
|
||||||
|
|
||||||
|
### 验证方式
|
||||||
|
- 单元测试覆盖恶意 URL 场景
|
||||||
|
- E2E 测试验证重定向行为
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. API 响应缺少运行时类型验证
|
||||||
|
|
||||||
|
### 问题描述
|
||||||
|
`parseJsonResponse` 使用 TypeScript 类型断言 (`as ApiResponse<T>`),不验证运行时响应结构。`result.data` 可能是 `undefined`。
|
||||||
|
|
||||||
|
**受影响文件**:
|
||||||
|
- `frontend/admin/src/lib/http/client.ts:87-89, 240-245`
|
||||||
|
|
||||||
|
### 修复方案
|
||||||
|
|
||||||
|
添加运行时验证:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
interface ApiResponse<T> {
|
||||||
|
code: number
|
||||||
|
message: string
|
||||||
|
data?: T
|
||||||
|
}
|
||||||
|
|
||||||
|
function isApiResponse(obj: unknown): obj is ApiResponse<unknown> {
|
||||||
|
if (typeof obj !== 'object' || obj === null) return false
|
||||||
|
const r = obj as Record<string, unknown>
|
||||||
|
return typeof r.code === 'number' && typeof r.message === 'string'
|
||||||
|
}
|
||||||
|
|
||||||
|
async function parseJsonResponse<T>(response: Response): Promise<T> {
|
||||||
|
const raw = await response.json()
|
||||||
|
|
||||||
|
if (!isApiResponse(raw)) {
|
||||||
|
throw new Error('Invalid API response structure')
|
||||||
|
}
|
||||||
|
|
||||||
|
if (raw.code !== 0) {
|
||||||
|
throw AppError.fromResponse(raw, response.status)
|
||||||
|
}
|
||||||
|
|
||||||
|
if (raw.data === undefined) {
|
||||||
|
throw new Error('API response missing data field')
|
||||||
|
}
|
||||||
|
|
||||||
|
return raw.data as T
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### 验证方式
|
||||||
|
- 用 mock 测试验证无效响应被正确拒绝
|
||||||
|
- 用有效响应验证数据正确解析
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. 缓存击穿 (Cache Stampede) 风险
|
||||||
|
|
||||||
|
### 问题描述
|
||||||
|
`AuthMiddleware.isJTIBlacklisted` 在 L1 cache miss 时,所有并发请求同时打 L2 (Redis),无 singleflight 保护。
|
||||||
|
|
||||||
|
**受影响文件**:
|
||||||
|
- `internal/api/middleware/auth.go:113-130`
|
||||||
|
|
||||||
|
### 修复方案
|
||||||
|
|
||||||
|
使用 `golang.org/x/sync/singleflight`:
|
||||||
|
|
||||||
|
```go
|
||||||
|
import "golang.org/x/sync/singleflight"
|
||||||
|
|
||||||
|
var sfGroup singleflight.Group
|
||||||
|
|
||||||
|
func (m *AuthMiddleware) isJTIBlacklisted(jti string) bool {
|
||||||
|
if jti == "" {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
key := "jwt_blacklist:" + jti
|
||||||
|
if _, ok := m.l1Cache.Get(key); ok {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
if m.cacheManager != nil {
|
||||||
|
// 使用 singleflight 防止缓存击穿
|
||||||
|
val, err := sfGroup.Do(key, func() (interface{}, error) {
|
||||||
|
return m.cacheManager.Get(context.Background(), key)
|
||||||
|
})
|
||||||
|
if err == nil && val != nil {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### 验证方式
|
||||||
|
- 并发测试验证 singleflight 正确聚合请求
|
||||||
|
- 单元测试验证缓存未命中时行为正确
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Webhook 服务 Goroutine 生命周期管理
|
||||||
|
|
||||||
|
### 问题描述
|
||||||
|
`WebhookService` 启动 worker pool 和 `time.AfterFunc` 重试,无 `Shutdown()` 方法。服务器关闭时 goroutine 被强制终止,可能丢失 delivery 记录。
|
||||||
|
|
||||||
|
**受影响文件**:
|
||||||
|
- `internal/service/webhook.go:110-123, 235-254`
|
||||||
|
|
||||||
|
### 修复方案
|
||||||
|
|
||||||
|
添加 `Shutdown` 方法:
|
||||||
|
|
||||||
|
```go
|
||||||
|
// Shutdown 优雅关闭 Webhook 服务
|
||||||
|
// 等待所有处理中的 delivery 完成,最多等待 timeout
|
||||||
|
func (s *WebhookService) Shutdown(ctx context.Context) error {
|
||||||
|
// 1. 停止接收新任务
|
||||||
|
close(s.queue)
|
||||||
|
|
||||||
|
// 2. 等待现有 worker 完成
|
||||||
|
done := make(chan struct{})
|
||||||
|
go func() {
|
||||||
|
s.wg.Wait()
|
||||||
|
close(done)
|
||||||
|
}()
|
||||||
|
|
||||||
|
select {
|
||||||
|
case <-done:
|
||||||
|
// 正常完成
|
||||||
|
case <-ctx.Done():
|
||||||
|
return ctx.Err()
|
||||||
|
}
|
||||||
|
|
||||||
|
// 3. 清理 AfterFunc 重试(可选:等待或取消)
|
||||||
|
// time.AfterFunc 无法直接取消,但 goroutine 会在下一次执行时检查 shutdown 状态
|
||||||
|
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
在 `main.go` 的服务器关闭逻辑中调用:
|
||||||
|
|
||||||
|
```go
|
||||||
|
// 创建带超时的 context
|
||||||
|
shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||||
|
defer shutdownCancel()
|
||||||
|
|
||||||
|
// 关闭 Webhook 服务
|
||||||
|
if err := webhookSvc.Shutdown(shutdownCtx); err != nil {
|
||||||
|
log.Printf("Webhook service shutdown error: %v", err)
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### 验证方式
|
||||||
|
- 单元测试验证 Shutdown 等待 worker 完成
|
||||||
|
- 集成测试验证关闭时 delivery 记录不丢失
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 优先级排序
|
||||||
|
|
||||||
|
| 优先级 | 问题 | 理由 |
|
||||||
|
|--------|------|------|
|
||||||
|
| P1 | OAuth origin 验证 | 直接影响用户安全,有实际攻击面 |
|
||||||
|
| P2 | 缓存击穿 | 高并发场景下影响性能,不是立即安全问题 |
|
||||||
|
| P2 | API 响应验证 | 数据一致性,但现有代码已有基础校验 |
|
||||||
|
| P2 | Webhook 生命周期 | 关闭时可能丢数据,但不是高频场景 |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 验证矩阵
|
||||||
|
|
||||||
|
| 问题 | go build | go test | npm run build | 手动验证 |
|
||||||
|
|------|----------|---------|---------------|----------|
|
||||||
|
| OAuth origin | - | 需要补充测试 | - | E2E |
|
||||||
|
| API 响应验证 | - | 需要补充测试 | - | 单元测试 |
|
||||||
|
| 缓存击穿 | ✅ | 需要补充测试 | - | 并发测试 |
|
||||||
|
| Webhook shutdown | ✅ | 需要补充测试 | - | 集成测试 |
|
||||||
Reference in New Issue
Block a user