docs: add PR312 security audit report
This commit is contained in:
@@ -0,0 +1,168 @@
|
||||
# PR #312 多轨道混音+字幕渲染+视频拼接 审计报告
|
||||
|
||||
## 总览
|
||||
|
||||
- **结论**：⚠️ 有条件通过（需修复 P1 问题后方可合并）
|
||||
- **问题统计**：P0 0 项，P1 4 项，P2 9 项，P3 5 项
|
||||
- **改动范围**：6 个文件，+2614 行，-2 行
|
||||
- **PR标题**：feat: 多轨道混音 + 字幕渲染引擎 + 视频拼接（后端）
|
||||
- **改动文件**：
|
||||
- `apps/worker/video_processing/multi_track_mixer.py`（新增，+392 行）
|
||||
- `apps/worker/video_processing/subtitle_render_engine.py`（新增，+636 行）
|
||||
- `apps/worker/video_processing/concat_engine.py`（新增，+642 行）
|
||||
- `apps/worker/video_processing/render_audio.py`（修改，+19/-2）
|
||||
- `apps/worker/video_processing/unified_render_service.py`（修改，+2/-0）
|
||||
- `tests/unit/test_multi_track_subtitle_concat.py`（新增，+923 行）
|
||||
|
||||
## 三大重点审计结论
|
||||
|
||||
### 1. FFmpeg 滤镜注入风险
|
||||
|
||||
**总体评价**：✅ 低风险
|
||||
|
||||
- ✅ 所有 FFmpeg 调用均通过 `run_ffmpeg` 使用列表参数传递（`subprocess.run` with list args），无 `shell=True`，不存在命令注入风险
|
||||
- ✅ 数字参数（音量、时长、坐标等）均经过 `float()` / `int()` 转换，并设有范围钳制（如 `volume` 限制在 0.0~2.0）
|
||||
- ✅ 多轨道混音的 `filter_complex` 由受控的数值和内部标签拼接，标签名（`v{i}_in`、`a{i}_in` 等）由代码生成，不可控
|
||||
- ⚠️ `build_subtitle_filter` 的路径转义不完整（未转义 `[` `]`），但当前仅用于服务器生成的路径，风险较低
|
||||
- ⚠️ concat demuxer 的 list 文件中路径仅转义单引号，虽不直接导致命令注入（走 list 文件而非 shell），但若路径中含换行符可能拼接新的 file 行
|
||||
|
||||
### 2. 字幕文件路径安全
|
||||
|
||||
**总体评价**：⚠️ 中等风险（路径来源需关注）
|
||||
|
||||
- ✅ ASS 文件生成位置在 `work_dir` 内（服务器控制），文件名由 `plan_id` 构成，无路径遍历
|
||||
- ✅ ASS 文本内容经过 `_escape_ass_text` 转义（大括号→圆括号、换行→\N），防止 ASS 覆盖标签注入
|
||||
- ✅ 颜色值经过 `_hex_to_ass_color` 校验（非 6 位 hex 降级为白色）
|
||||
- ⚠️ `build_subtitle_filter` 中的字幕文件路径若来自用户输入，可能存在 filter 语法注入风险
|
||||
- ⚠️ 字幕文件无大小上限（极端场景下大量字幕片段可能生成超大 ASS 文件）
|
||||
- ❌ 新增模块未接入任何 `path_security` 校验（注：当前代码库中尚未发现 path_security.py 模块，仅在 __pycache__ 中有缓存文件）
|
||||
|
||||
### 3. 拼接资源消耗控制
|
||||
|
||||
**总体评价**：❌ 高风险（缺少关键限制）
|
||||
|
||||
- ❌ **拼接段数无上限**：`ConcatConfig.from_config_dict` 不限制 segments 数量，大量片段拼接会消耗大量内存和 CPU
|
||||
- ❌ **轨道数量无上限**：`MultiTrackMixConfig.from_config_dict` 不限制 tracks 数量
|
||||
- ❌ **无总时长上限**：拼接后总时长没有任何限制
|
||||
- ❌ **无单段文件大小限制**：输入文件大小没有校验
|
||||
- ⚠️ 使用 concat filter 模式时，所有输入文件同时解码，内存占用与段数成正比
|
||||
- ✅ 有 30 分钟的 FFmpeg 超时机制（`DEFAULT_FFMPEG_TIMEOUT = 1800`）
|
||||
- ✅ worker 级别有 4 并发限制（`worker_concurrency: int = 4`）
|
||||
- ⚠️ 临时文件（track 预处理文件、concat_list.txt）使用后未清理
|
||||
|
||||
## 问题清单（按等级）
|
||||
|
||||
### P0
|
||||
|
||||
无。所有 FFmpeg 调用均使用列表参数，不存在直接的命令注入漏洞。
|
||||
|
||||
### P1
|
||||
|
||||
1. **【multi_track_mixer.py】audio_path 无路径安全校验**
|
||||
- 位置：`AudioTrack.from_dict`（L98）及 `_prepare_single_track`（L173/L240）
|
||||
- 描述：`audio_path` 直接从用户配置中读取字符串，未经过任何路径校验（如路径白名单、路径遍历检测）。用户可通过 `plan.config.audio_tracks.tracks[].audio_path` 传入任意文件路径，ffprobe/ffmpeg 会读取该文件。虽不直接执行命令，但可探测服务器上任意文件的存在性，且音频/视频文件会被转码后进入输出流。
|
||||
- 风险：任意文件读取/探测（信息泄露）
|
||||
- 建议：参照项目既有的 `asset_id → local_path` 解析模式，或引入 `path_security` 模块校验路径必须在指定目录内
|
||||
|
||||
2. **【concat_engine.py】video_path 无路径安全校验**
|
||||
- 位置：`ConcatSegment.from_dict`（L72）及 `_concat_filter`（L319）、`_concat_demuxer`（L281）
|
||||
- 描述：`video_path` 直接从配置读取，无任何路径安全校验。同理，用户可通过拼接配置读取服务器任意文件。
|
||||
- 风险：任意文件读取/探测（信息泄露）
|
||||
- 建议：同上，增加路径安全校验
|
||||
|
||||
3. **【multi_track_mixer.py】无轨道数量上限**
|
||||
- 位置：`MultiTrackMixConfig.from_config_dict`（L119-L137）
|
||||
- 描述：`tracks` 列表无数量上限校验，用户可传入成百上千个轨道，导致 FFmpeg 同时打开大量文件、构建复杂 filter_complex，消耗大量 CPU/内存/文件描述符。
|
||||
- 风险：资源耗尽（DoS）
|
||||
- 建议：设置合理上限（如 `MAX_TRACKS = 16`），超过则截断并记录告警
|
||||
|
||||
4. **【concat_engine.py】无拼接段数上限**
|
||||
- 位置：`ConcatConfig.from_config_dict`（L99-L113）
|
||||
- 描述：`segments` 列表无数量上限校验。concat filter 模式下所有输入文件同时解码，内存占用线性增长。
|
||||
- 风险：资源耗尽（DoS）
|
||||
- 建议：设置合理上限（如 `MAX_SEGMENTS = 50`），超过则截断或报错
|
||||
|
||||
### P2
|
||||
|
||||
5. **【concat_engine.py】无总时长/单段时长上限**
|
||||
- 描述：没有限制拼接后总时长或单段时长，超长视频拼接会长时间占用 worker。
|
||||
- 建议：增加 `MAX_TOTAL_DURATION`（如 2 小时）和 `MAX_SEGMENT_DURATION` 限制
|
||||
|
||||
6. **【multi_track_mixer.py】预处理临时文件未清理**
|
||||
- 位置：`mix_multi_track` 方法（L303）
|
||||
- 描述：`track_{i}_{plan_id}.aac` 等临时文件在混音完成后未被清理，长期运行会占用磁盘空间。
|
||||
- 建议：混音完成后 `try/finally` 清理临时轨道文件
|
||||
|
||||
7. **【concat_engine.py】concat_list.txt 未清理**
|
||||
- 位置：`_concat_demuxer`（L275）
|
||||
- 描述：生成的 concat demuxer 列表文件使用后不删除。
|
||||
- 建议：使用完毕后删除列表文件
|
||||
|
||||
8. **【concat_engine.py】transition 字段无白名单校验**
|
||||
- 位置：`ConcatConfig.from_config_dict`（L132）
|
||||
- 描述：`transition = str(config.get("transition", "none"))` 直接转字符串，无白名单验证。虽然目前仅判断 `!= "none"` 时禁用 stream copy，但未来扩展转场效果（如 xfade）时，transition 名称会进入 filter_complex，存在注入风险。
|
||||
- 建议：使用枚举或白名单列表校验 transition 值
|
||||
|
||||
9. **【subtitle_render_engine.py】字幕滤镜路径转义不完整**
|
||||
- 位置：`build_subtitle_filter`（L633-L636）
|
||||
- 描述：仅转义了 `\`、`:`、`'`，未转义 `[`、`]`。若字幕文件路径中包含方括号，可能破坏 filter_complex 语法导致 FFmpeg 报错。
|
||||
- 当前风险：低（路径由服务器生成，不含方括号）
|
||||
- 建议：补充 `[` → `\\[`、`]` → `\\]` 的转义
|
||||
|
||||
10. **【subtitle_render_engine.py】字幕片段数量/文件大小无上限**
|
||||
- 描述：ASS 文件可包含无限多字幕片段，极端场景下可能生成超大文件。
|
||||
- 建议：增加 `MAX_SEGMENTS` 限制（如 10000 条）
|
||||
|
||||
11. **【concat_engine.py】_concat_filter 大量死代码**
|
||||
- 位置：`_concat_filter` 方法 L330-L418
|
||||
- 描述：方法前半部分（约 100 行）构建的 `filter_parts` 和 `concat_inputs` 在 L418 被 `filter_parts.clear()` 全部清除，然后从 L420 重新构建。前半部分是完全无用的死代码，增加维护成本且容易误读。
|
||||
- 建议：删除前半部分死代码，保留清晰的实现
|
||||
|
||||
12. **【multi_track_mixer.py】track_id/track_type 无字符校验（日志注入）**
|
||||
- 描述：`track_id` 和 `track_type` 是用户可控字符串，直接用于日志输出，存在日志注入风险（如插入伪造日志行）。
|
||||
- 建议：对日志输出的字符串做长度和换行符限制
|
||||
|
||||
13. **【API 层】新增配置字段无 schema 校验**
|
||||
- 位置：`packages/domain/config_schemas.py` 的 `normalize_plan_config`
|
||||
- 描述：`normalize_plan_config` 仅校验和清洗 `cover/title/subtitle/bgm/editing_mode/transition_enabled` 字段。新增加的 `audio_tracks`、`manual_subtitles`、`title_config`、`subtitle_config`（新格式）等字段未纳入校验，任意 JSON 都可以透传到 worker。
|
||||
- 建议：在 config_schemas 中新增对应的数据模型和校验逻辑
|
||||
|
||||
### P3
|
||||
|
||||
14. **【concat_engine.py】未使用的导入**
|
||||
- `import tempfile` 和 `from typing import Any` 未使用
|
||||
|
||||
15. **【subtitle_render_engine.py】未使用的导入**
|
||||
- `dataclasses.field`、`generate_ass_subtitles`、`generate_ass_from_timeline` 未使用（预留导入）
|
||||
|
||||
16. **【subtitle_render_engine.py】尚未集成到主渲染流程**
|
||||
- `SubtitleRenderEngine` 和 `build_subtitles_from_plan` 是独立模块，`unified_render_service.py` 仍使用旧的 `render_subtitles.py` 和 `subtitle_generator.py`，新引擎暂未接入
|
||||
|
||||
17. **【concat_engine.py】尚未集成到主渲染流程**
|
||||
- `ConcatEngine` 仅提供独立的拼接能力，未接入 `UnifiedRenderService` 的渲染管线
|
||||
|
||||
18. **【concat_engine.py】transition 文档标注"预留"但已有逻辑判断**
|
||||
- 字段注释写着"转场效果（none/crossfade）- 预留"，但 `_can_use_stream_copy` 中已有 `if config.transition != "none": return False` 的判断，建议统一描述
|
||||
|
||||
## 亮点
|
||||
|
||||
1. **优秀的降级策略**：三个模块均有完善的降级/容错机制——单轨失败跳过、混音失败回退主音频、参数不一致自动降级到 filter 模式、配置无效返回 None 等，符合"能力不可用时不阻断主流程"的设计原则
|
||||
2. **完善的参数安全转换**：所有数字参数都经过 try/except 包裹的类型转换，失败时降级到默认值；音量、不透明度等有范围钳制
|
||||
3. **统一使用 run_ffmpeg**：所有 FFmpeg 调用均通过 `run_ffmpeg` 工具函数，使用列表参数传递，从根本上避免 shell 注入
|
||||
4. **单测覆盖充分**：52 个新增单测覆盖了配置解析、正常流程、异常场景、降级路径，完整度较高
|
||||
5. **代码结构清晰**：三个模块均遵循统一的风格（dataclass + from_dict + 引擎类 + 便捷函数），符合 UnifiedRenderService 架构风格
|
||||
6. **ASS 文本转义到位**：`_escape_ass_text` 正确转义了大括号（防止 ASS 覆盖标签注入）和换行符
|
||||
|
||||
## 修复建议（优先级排序）
|
||||
|
||||
1. **【P1】** 为 `audio_path` 和 `video_path` 增加路径安全校验（接入 path_security 模块或使用 asset_id 解析模式）
|
||||
2. **【P1】** 增加轨道数量上限（建议 `MAX_TRACKS = 16`）
|
||||
3. **【P1】** 增加拼接段数上限（建议 `MAX_SEGMENTS = 50`）
|
||||
4. **【P2】** 增加拼接总时长上限和单段时长上限
|
||||
5. **【P2】** 清理临时文件（预处理轨道文件、concat_list.txt）
|
||||
6. **【P2】** `transition` 字段增加白名单校验
|
||||
7. **【P2】** 删除 `_concat_filter` 中的死代码
|
||||
8. **【P2】** 字幕滤镜路径转义补充 `[` `]`
|
||||
9. **【P2】** API 层 config_schemas 增加新配置字段的校验模型
|
||||
10. **【P3】** 清理未使用的导入
|
||||
11. **【P3】** 字幕片段数量增加合理上限
|
||||
Reference in New Issue
Block a user