Files
rssWorkFlow/docs/code-review.md
T
congsh ba6e7669e8 Initial commit: RSS platform phase 1 skeleton with code review fixes
Features:
- FastAPI + SQLAlchemy 2.0 async + PostgreSQL/pgvector + Redis backend
- Vue 3 + TypeScript + Element Plus frontend
- JWT auth with access/refresh tokens and revocation
- Admin/member RBAC
- RSS feed CRUD and article listing
- Settings management with Fernet encryption for sensitive values
- Redis distributed lock service
- Alembic initial migration
- Docker Compose development environment

Fixes from code review:
- Fix DB session leak in dependency injection
- Restrict registration to admin only
- Add default admin password warning
- Implement JWT refresh tokens and jti blacklist
- Strengthen password policy
- Use func.count for pagination totals
- Replace NullPool with AsyncAdaptedQueuePool
- Remove init_db from lifespan to enforce alembic migrations
- Add request_id middleware and logging filter
- Fix vite.config.ts env loading
- Add frontend token refresh interceptor
- Add Vue error handler

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-15 17:01:57 +08:00

391 lines
17 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# RSS 平台代码审核报告
- 审核对象:`/home/congsh/workspace/dev/rssWorkFlow`
- 技术栈:FastAPI + Vue 3 + PostgreSQL + Redis + MinIO
- 审核范围:后端核心 / API / 模型 / 服务 / 前端 / 部署配置
- 评级标准:P0(必须立即修复) / P1(生产前修复) / P2(建议优化)
---
## 一、P0:严重缺陷(必须立即修复)
### 1. 数据库会话资源泄露
`backend/app/api/deps.py:18-21`
```python
async def get_db() -> AsyncSession:
async for session in _get_db():
return session
```
`_get_db()``async generator`,正常依赖注入会通过 `yield` 让 FastAPI 接管 session 生命周期。但这里用 `async for` + `return``finally` 中的 `await session.close()` 永远不会被执行,session 一直被占用,连接池最终耗尽。
修复:
```python
async def get_db():
async for session in _get_db():
yield session
```
或者直接复用 `database.py:23``get_db`,避免二次包装。
### 2. `/auth/register` 接口存在越权
`backend/app/api/v1/auth.py:16-40`
- 任何未登录用户都可调用注册接口
- `UserCreate.role` 未做限制,可直接传 `"admin"` 创建管理员账号
修复:在 `register` 加上 `current_user: User = Depends(get_current_admin)`,或对 `role` 字段做白名单(普通用户只能注册 `member`),并限制注册端点的访问(首次部署后关闭或改为邀请制)。
### 3. 默认管理员弱口令 + 凭证入仓
- `.env``.env.example` 内容完全一致,且包含 `SECRET_KEY=change-me-...``DEFAULT_ADMIN_PASSWORD=admin`
- `.env` 已存在本地仓库(虽然 `.gitignore` 写了 `.env`,但若曾误提交则泄露)
- `main.py:40-58` 启动时如果无用户则创建 `admin/admin`,且无强制改密流程
修复:
1.`main.py:40-58` 增加「首次启动时检查默认密码,若仍为 `admin/admin` 则在 health 端点暴露 warning」
2. 增加「首次登录强制修改密码」逻辑
3. 仓库 `.env` 不可提交任何真实凭证;CI 校验 `SECRET_KEY` 不能等于占位值
### 4. JWT 设计与刷新机制缺失
`backend/app/core/auth.py`
- 无 refresh token
- `ACCESS_TOKEN_EXPIRE_MINUTES=480`8 小时)过长且无续签
- payload 没有 `iat` / `jti`,无主动吊销机制(用户被禁后 token 仍可用 8 小时)
- payload 中带 `role` 字段(`auth.py:68`)后端已查 DB 重新加载,覆盖逻辑没问题,但字段冗余且易引发误解
修复:拆分 access (15min) + refresh (7d) + 引入 `jti` 维护吊销集合(Redis 黑名单)。
---
## 二、P1:安全风险(生产前需修复)
### 5. CORS dev fallback 使用通配
`backend/main.py:78-85`
```python
else:
app.add_middleware(CORSMiddleware, allow_origins=["*"], ...)
```
只要 `CORS_ALLOWED_ORIGINS` 为空就放开 `*`,生产环境如果忘记配置即全放开。`allow_credentials=False` 缓解了,但 `allow_methods=["*"]` + `allow_headers=["*"]` 仍过于宽松。
修复:开发态也强制要求配置 `CORS_ALLOWED_ORIGINS`,缺失时启动失败或显式 `WARNING`
### 6. 密码强度弱
`backend/app/schemas/user.py:16`
```python
password: str = Field(..., min_length=6, max_length=128)
```
6 位纯数字即可。
修复:最少 8/10 位 + 至少字母+数字的复杂度校验。
### 7. 缺少登录限流与审计
- 无失败次数限制(存在暴力破解风险)
- 无登录/关键操作审计日志
修复:基于 Redis 接入 slowapi 或自实现 IP+用户维度限流;关键操作(改密、改 admin、删除 feed)落审计表。
### 8. 敏感设置未加密落库
`backend/app/services/settings_service.py:105-133`
非 admin 调用 `list_settings(mask_sensitive=True)` 时返回 `real_value=null`,逻辑正确。但 `OPENAI_API_KEY` 等敏感值在 `apply_db_settings_to_config` 写入内存时是明文——OK;问题是未在 settings_service 中对 `value` 做加密落库,DB 泄露即明文 API Key 泄露。
修复:使用 `cryptography.fernet` 加密存储敏感设置。
### 9. RSS Feed URL 缺少 SSRF 防护
`feed.py:15` `String(2048)` 看似够用,但 `HttpUrl``feed.py:8` 校验仅做 URL 格式校验,没有 `https://` 强制,没有 `SSRF` 防护(后端抓取时可能请求内网地址)。
修复:抓取时强制 `https`/`http`,并维护 IP 黑名单或代理出口策略。
---
## 三、P1:性能与正确性
### 10. 分页计数全量拉取
`backend/app/api/v1/feeds.py:39-40``articles.py:40-41`
```python
count_result = await db.execute(select(Feed.id).select_from(query.subquery()))
total = len(count_result.scalars().all())
```
把每条 ID 都取回 Python 再 `len()`,表大时是灾难。应该用 `func.count()`
```python
from sqlalchemy import func
count_query = select(func.count()).select_from(query.subquery())
total = (await db.execute(count_query)).scalar_one()
```
### 11. 数据库连接池策略不当
`backend/app/core/database.py:7-12`
```python
engine = create_async_engine(..., poolclass=NullPool)
```
`NullPool` 适合 serverless/单次执行,但 Docker 长驻服务中会每次请求都创建/销毁 Postgres 连接,TPS 高时延显著。应改为默认 `AsyncAdaptedQueuePool`(带 `pool_size` + `max_overflow`)。
### 12. lifespan 中 init_db 与 alembic 冲突
`backend/main.py:24`
```python
await init_db() # Base.metadata.create_all
```
启动时用 `create_all` 自动建表,会绕过 alembic 迁移;表结构偏离后将无法再用 `alembic upgrade` 演进。
修复:移除 `init_db()` 调用,统一通过 `make migrate` 走 alembic。`init_default_settings` 也应在迁移脚本或独立 seed 任务中执行。
### 13. 数据库迁移命名不合规
`backend/alembic/versions/001_initial_schema.py`
Alembic 推荐 `xxxxxx_initial_schema.py` 哈希前缀。缺少前缀会导致 `alembic history` 出现歧义、`autogenerate` 比较报错。
### 14. task_runtime Redis hash 字段类型不严谨
`app/services/task_runtime.py`
`update_progress``current`/`total` 存为字符串,`get_progress``int()`。如果 key 在 `update_progress` 之前被外部写为 `int`,会被覆盖为字符串,行为不统一。建议明确约定。
---
## 四、P2:代码质量
### 15. get_current_user 双重读取 Authorization
`backend/app/api/deps.py:30-37`
`HTTPBearer(auto_error=False)` 已处理,又手动再 `request.headers.get("Authorization")`。是冗余逻辑,建议只保留一种。
### 16. rbac.has_permission 语义模糊
`backend/app/core/rbac.py:26-30`
```python
def has_permission(user: User, required_role: Role) -> bool:
if user.role == Role.ADMIN:
return True
return user.role == required_role
```
返回 `True` 当用户是 admin,无论 `required_role` 是什么——这其实是「admin 拥有所有权限」的语义,但函数命名是「是否有某角色权限」,容易误用。需在 docstring 中明确:返回值等价于 `user.role == ADMIN or user.role == required_role`
### 17. require_admin 与 get_current_admin 重复实现
`rbac.py:16-23``deps.py:80-87` 做同一件事,应统一从一处导入。
### 18. 缺少统一日志 request_id 注入
`backend/app/core/logging.py:6` 定义了 `request_id_var: ContextVar`,但没有 ASGI 中间件去赋值 `request_id`,所有日志的 `[%s]` 始终是空字符串,request_id 形同虚设。
修复:添加 ASGI middleware 从 header `X-Request-ID` 读取或生成 UUID,写入 `ContextVar`
### 19. 健康检查端点无认证
`backend/app/api/v1/health.py`
返回了 `db``redis` 状态字符串,可被外部用于探测内网拓扑。建议:
- 基础 `/health` 仅返回 `ok/degraded`
- 详细诊断使用 `/health/db``/health/redis` 并在生产加 `Depends(get_current_admin)`
### 20. 前端 vite.config.ts 误用 process.env
`frontend/vite.config.ts:17,22`
```ts
target: process.env.VITE_API_BASE_URL || 'http://localhost:8000'
```
Vite 在 Node 端运行时 `process.env` 通常取不到 `VITE_*` 变量(这些只在客户端 `import.meta.env` 中存在)。应改用 `loadEnv`
```ts
import { defineConfig, loadEnv } from 'vite'
export default defineConfig(({ mode }) => {
const env = loadEnv(mode, process.cwd(), '')
return { server: { proxy: { '/api': { target: env.VITE_API_BASE_URL || 'http://localhost:8000' } } } }
})
```
### 21. 前端路由守卫在 token 失效但 user 缓存存在时的中间态
`frontend/src/router/index.ts:38-57`
`authStore.isAuthenticated` 要求 `token && user` 都有值。F5 刷新时:localStorage 有 token 但 Pinia 中 `user` 为 null——通过 `fetchUser()` 重新拉取,正常。
但如果 **user 已被禁用** 且 token 仍有效,则 `get_current_user` 抛 403,前端会 `authStore.logout()`。**如果服务器端 `is_active` 没及时同步**(集群场景),会出现 token 在内存有效、DB 无效的中间态。建议每次路由切换都强制重新校验。
### 22. 前端 token 存 localStorage
`frontend/src/stores/auth.ts:7,19`
XSS 风险:恶意脚本可读 `localStorage.getItem('token')`。生产建议:access token 走 httpOnly cookie,或限制 token 权限到只读。
### 23. axios 拦截器 401 硬跳页
`frontend/src/api/index.ts:29-33`
401 时直接 `window.location.href = '/login'`,硬刷路由。如果某些请求是登录后业务请求(如 fetchMe 失败),会突然跳页而不是返回错误。考虑在 store 层处理跳转。
### 24. Dashboard 拉 1000 条数据统计
`frontend/src/views/DashboardView.vue:46`
```ts
const res = await feedsApi.list({ limit: 1000 })
```
后端 `PaginationParams.limit` 没有上限,前端传多少就返回多少。数据大时慢。应该让后端提供专用统计接口。
### 25. 前端无全局错误边界
没有 `app.config.errorHandler`、没有 `<ErrorBoundary>` 组件,单个组件报错会白屏。
修复:
```ts
app.config.errorHandler = (err, instance, info) => {
console.error('Unhandled error:', err, info)
// 接入 Sentry 等
}
```
### 26. 分页参数未做边界校验
`feeds.py` 等处的 `PaginationParams.skip/limit` 无 max 限制,恶意请求 `limit=99999999` 会拉全表。
修复:`limit: int = Field(50, ge=1, le=200)`
### 27. Dockerfile 细节
- `backend.Dockerfile` `pip install -e .` 但 production 阶段又 `COPY backend/`,无 `.dockerignore` 容易把 .env/数据卷带进去
- `frontend.Dockerfile``npm install` 应改 `npm ci` 以保证 lock 一致
- 无 production 多阶段构建(目前 `Dockerfile` 有 production stage 但 `docker-compose.yml` 只用 `development`
### 28. tests/conftest.py 自定义 event_loop 已弃用
`backend/tests/conftest.py:11-15`
```python
@pytest.fixture(scope="session")
def event_loop():
loop = asyncio.get_event_loop_policy().new_event_loop()
...
```
pytest-asyncio 0.23+ 已标记为 deprecated,改为 `asyncio_mode = "auto"` + `event_loop_policy` fixture 或升级到 `pytest-asyncio` 0.23+ 推荐做法。
### 29. 测试覆盖率几乎为零
仅 2 个测试(密码哈希 + 用户创建),没有 API endpoint 集成测试、没有任何前端测试。CI 缺失。
建议:补 FastAPI `TestClient` 集成测试 + 至少 RBAC 权限矩阵测试;前端至少加 Vitest 单测覆盖 stores + 一个 E2EPlaywright)。
### 30. 文档缺失
`docs/` 目录为空,README 引用的「设计文档」「开发步骤」路径指向 `/home/congsh/workspace/chat/`,仓内 `docs/` 没文件——文档随项目仓库丢失风险。
修复:将 `/home/congsh/workspace/chat/rss-platform-design.md``rss-platform-dev-plan.md` 移到 `docs/` 下并更新 README 引用路径。
---
## 五、修复优先级汇总
| 优先级 | 文件 | 问题 | 建议改动 |
|--------|------|------|----------|
| P0 | `backend/app/api/deps.py:18-21` | DB session 泄露 | 改为 `async for ... yield session` 或直接复用 `database.get_db` |
| P0 | `backend/app/api/v1/auth.py:16-40` | 注册越权 | 加 admin 鉴权 + role 白名单 |
| P0 | `backend/main.py:40-58` | 默认弱口令 | 首次启动检查、强制改密 |
| P0 | `backend/app/core/auth.py` | 无 refresh / jti | 拆分 access/refresh + Redis 黑名单 |
| P1 | `backend/main.py:78-85` | CORS 通配 fallback | 移除或启动 fail-fast |
| P1 | `backend/app/schemas/user.py:16` | 密码长度 6 | 改为 10 + 复杂度校验 |
| P1 | `backend/app/api/v1/feeds.py:39` & `articles.py:40` | `len()` 计数 | 改 `func.count()` |
| P1 | `backend/app/core/database.py:7-12` | NullPool | 改默认连接池 |
| P1 | `backend/main.py:24` | `init_db` 绕过 alembic | 删除 init_db,统一 alembic |
| P2 | `frontend/vite.config.ts:17,22` | `process.env` 错用 | 改 `loadEnv` |
| P2 | `backend/app/core/logging.py` | request_id 形同虚设 | 加 ASGI 中间件赋值 |
| P2 | `frontend/src/stores/auth.ts` | localStorage 存 token | 改 httpOnly cookie 或加 CSP |
| P2 | 整体 | 缺少测试与 CI | 补 pytest + Vitest + Playwright + GH Actions |
---
## 六、值得肯定的点
- 整体结构清晰,模块边界(core / models / schemas / services / api)划分合理
- 模型层 `UUIDMixin` + `TimestampMixin` 抽象良好
- Alembic + Pydantic Settings + async SQLAlchemy 2.0 类型注解到位
- 前端 Pinia + 路由守卫 + axios 拦截器规范
- 用 Pydantic 的 `HttpUrl``Field(ge/le)``min_length` 在请求侧就校验
- 使用 `lifespan` 替代 deprecated 的 `on_event`
---
## 七、修复记录(2026-06-15
本次根据本报告对 `rssWorkFlow` 代码进行了选择性修复,覆盖 P0 / P1 / 部分 P2 项。
### 已修复
| 优先级 | 问题 | 修复文件 | 修复内容 |
|--------|------|----------|----------|
| P0 | DB session 资源泄露 | `backend/app/api/deps.py` | `get_db` 改为 `async for ... yield session`,由 FastAPI 管理生命周期 |
| P0 | `/auth/register` 越权 | `backend/app/api/v1/auth.py` | 注册接口改为仅 admin 可调用;schema 限制 role 白名单 |
| P0 | 默认管理员弱口令 | `backend/main.py`, `.env.example`, `backend/app/api/v1/health.py` | 启动时检测 admin/admin 并在 `/health` 返回 warning;文档增加安全提示 |
| P0 | JWT 无 refresh/jti | `backend/app/core/auth.py`, `backend/app/api/v1/auth.py`, `backend/app/schemas/user.py`, `backend/app/core/config.py` | access/refresh 双 tokentoken 携带 jti/type/iat;支持 Redis 黑名单吊销 |
| P1 | CORS dev fallback 通配 | `backend/main.py` | 移除 `*` fallback,默认使用 `http://localhost:5173`;生产必须显式配置 |
| P1 | 密码强度不足 | `backend/app/schemas/user.py` | 密码要求 8-128 位且至少包含字母和数字 |
| P1 | 分页计数全量拉取 | `backend/app/api/v1/feeds.py`, `backend/app/api/v1/articles.py` | 改用 `select(func.count())` |
| P1 | 数据库连接池策略 | `backend/app/core/database.py` | 移除 `NullPool`,改用 `AsyncAdaptedQueuePool`pool_size=10, max_overflow=20 |
| P1 | lifespan 中 init_db 绕过 alembic | `backend/main.py`, `backend/app/core/database.py` | 移除 `init_db()` 调用与函数定义,统一由 alembic 管理 schema |
| P1 | 敏感设置未加密 | `backend/app/services/settings_service.py`, `backend/app/core/config.py`, `backend/pyproject.toml` | 使用 Fernet 加密敏感配置项(`OPENAI_API_KEY``API_TOKEN`),明文/加密兼容 |
| P1 | 健康检查暴露拓扑 | `backend/app/api/v1/health.py` | `/health/db``/health/redis` 增加 admin 鉴权 |
| P2 | vite.config.ts 误用 process.env | `frontend/vite.config.ts` | 改用 `loadEnv` 读取环境变量 |
| P2 | request_id 未注入 | `backend/app/core/logging.py`, `backend/main.py` | 新增 ASGI middleware,从 header 读取或生成 request_id |
| P2 | pytest event_loop 弃用 | `backend/tests/conftest.py` | 移除自定义 `event_loop` fixture,依赖 `asyncio_mode = "auto"` |
| P2 | 前端无错误边界 | `frontend/src/main.ts` | 添加 `app.config.errorHandler` |
| P2 | 分页参数无上限 | `backend/app/schemas/common.py` | `skip >= 0``1 <= limit <= 200` |
| P2 | 文档未随仓库管理 | `docs/design.md`, `docs/dev-plan.md`, `README.md` | 将设计文档与开发步骤复制到 `docs/`README 引用更新 |
| P2 | 前端 token 无刷新 | `frontend/src/stores/auth.ts`, `frontend/src/api/index.ts`, `frontend/src/api/auth.ts`, `frontend/src/types/index.ts` | 存储 refresh_token401 时自动刷新,失败再跳转登录 |
### 未修复 / 留待后续
- 登录限流与审计日志(P1):建议后续接入 slowapi 或自实现 Redis 限流,并新增 `audit_logs` 表。
- Dockerfile 多阶段与 `.dockerignore` 优化(P2):当前阶段先用 dev 构建,production 镜像与构建 CI 留待部署阶段完善。
- 测试覆盖率与 CIP2):已留好 pytest / Vitest / Playwright 接入点,后续阶段补充集成测试与 GitHub Actions。
- SSRF 防护(P1):RSS 抓取任务尚未实现,抓取模块中加入 URL 校验、IP 黑名单、代理出口策略。
### 验证
- 后端全部 Python 文件 `py_compile` 通过
- `init_db` 已无引用
- 前端 TypeScript 文件已完成同步修改(因无 node_modules,未运行 `tsc`
### 运行建议
1. `cd /home/congsh/workspace/dev/rssWorkFlow`
2. `cp .env.example .env` 并修改 `SECRET_KEY`、默认管理员密码、生成 `SETTINGS_ENCRYPTION_KEY`
3. `make dev`
4. `docker-compose exec backend alembic upgrade head`
5. 访问前端登录,验证 `/health` 无安全警告
- Dockerfile 多阶段构建思路正确