diff --git a/plan.md b/plan.md index 446a29c..ecd8bc0 100644 --- a/plan.md +++ b/plan.md @@ -291,6 +291,125 @@ admin.php → http->name('admin') → Common::__construct() (获取$admin, $left --- +## Round 3 安全审计结果(SecurityEngineer) + +### Task S1 — Admin 鉴权覆盖完整性审查 ✅ 验证通过 + +**审查方法**:读取 main 合并结果 + ShopXO Common.php 源码 + +#### 鉴权链分析 + +``` +ThinkPHP 路由 → admin.php (module=admin) + → Common::__construct() + 1. AdminService::LoginInfo() ← 填充 $this->admin(从 session) + 2. AdminPowerService::PowerMenuInit() + 3. ViewInit() + → 插件控制器(如 Ticket/list) + → Base extends Common → parent::__construct() + → 完整继承上述 3 步 + +// 实际执行顺序(ThinkPHP 5/6 约定): +// 子类 __construct() 中的 parent::__construct() 在子类逻辑之前执行 +``` + +**结论**: +- ✅ `Base extends Common` — 登录检查(`$this->admin` 非空)和权限菜单已正确初始化 +- ✅ 所有子控制器(SeatTemplate / Ticket / Verifier / Verification)通过 `extends Base` 自动获得鉴权 +- ✅ BackendArchitect 的 P0 修复(Base extends Common)已合并到 main,**Task S1 验证通过** + +**Defense-in-Depth 建议**(非阻塞): +> 在 `Base::__construct()` 末尾显式调用 `$this->IsLogin()`,确保即使未来有人重写 `__construct()` 也不会绕过鉴权: +```php +public function __construct() +{ + parent::__construct(); + $this->IsLogin(); // 显式鉴权检查 defense-in-depth +} +``` + +--- + +### Task S2 — SQL 注入风险审计 ✅ 无注入风险 + +**审查范围**:Phase 2 所有控制器 + TicketService + +| 控制器 | 查询点 | 输入处理 | 结论 | +|--------|--------|----------|------| +| SeatTemplate::list | `name` like, `status` | `null` + `intval()` | ✅ 安全 | +| SeatTemplate::save | `name`, `seat_map`, `category_id` | `trim()` + `intval()` + JSON 校验 | ✅ 安全 | +| SeatTemplate::delete | `id` | `intval()` | ✅ 安全 | +| Ticket::list | `keywords` multi-field like | `trim()` + 查询构造器绑定 | ✅ 安全 | +| Ticket::detail | `id` | `intval()` | ✅ 安全 | +| Ticket::verify | `ticket_code`, `verifier_id` | `trim()` + `intval()` | ✅ 安全 | +| Ticket::export | `goods_id` | `intval()` | ✅ 安全 | +| Verifier::list | `keywords`, `status` | `trim()` + `intval()` | ✅ 安全 | +| Verification::list | `keywords`, `verifier_id`, date range | `trim()` + `intval()` + `strtotime()` | ✅ 安全 | + +**无原始 SQL 执行,无字符串拼接注入。** ThinkPHP 查询构造器的参数绑定机制已覆盖所有用户输入。 + +⚠️ **新发现 P1 Bug(计划外)**:`Verifier.php:45` 存在 ThinkPHP 语法错误 + +```php +// 当前(已合并到 main): +$users = \Db::name('User') + ->where('id', 'in', $user_ids) + ->column('CONCAT(COALESCE(nickname,""), "/", COALESCE(username,""))', 'id'); +// ↑ ThinkPHP column() 第二个参数是 key 列名,不是 SQL +// 预期错误:ThinkPHP 会把 'id' 作为 value 列名查找,CONCAT 作为 key 列名查找 +// 结果:SQL 语法错误或空结果集 + +// 修复方案(PHP 侧拼接): +$users_raw = \Db::name('User')->where('id', 'in', $user_ids)->select(); +$users = []; +foreach ($users_raw as $u) { + $users[$u['id']] = ($u['nickname'] ?: '') . '/' . ($u['username'] ?: ''); +} +``` + +**此 bug 应在 Task B3 或独立 P1 fix 中修复。** + +--- + +### Task S3 — XSS / CSRF 防护检查 + +| 方面 | 状态 | 说明 | +|------|------|------| +| CSRF Token (POST) | ✅ ShopXO 保护 | AJAX POST 请求由 ShopXO 中间件统一处理 | +| CSRF Token (GET) | ✅ 无状态变更 | GET 请求只读,无需 CSRF 保护 | +| XSS(存储型) | ✅ 低风险 | 后台管理员输入,非用户可见;座位模板名称等字段建议 admin view 层做 `htmlspecialchars()` | +| XSS(反射型) | ✅ 低风险 | ThinkPHP 视图引擎默认转义 | +| 关键操作 guard | ✅ 已覆盖 | `delete()` / `verify()` / `export()` 均有 `IS_AJAX_POST` 检查 | + +**结论**:XSS / CSRF 防护充分,无需额外实现。 + +--- + +### Task S5 — IDOR 水平越权检查 + +| 接口 | 访问控制 | 结论 | +|------|---------|------| +| Ticket::detail (`id`) | 需登录 admin + 插件菜单权限 | ✅ 正常(admin 可见所有票) | +| Verifier::save/delete | 需登录 admin + 插件菜单权限 | ✅ 正常 | +| Verification::list | 需登录 admin + 插件菜单权限 | ✅ 正常(admin 可见所有核销记录) | + +**注**:核销员独立表 `vr_verifiers` 与 ShopXO admin 表 `sx_admin` 分离,权限隔离正确。核销员不登录 ShopXO admin,而是通过独立核销端(Phase 3)。 + +--- + +### 安全任务更新 + +- [x] **Task S1** — 审查 ShopXO 后台鉴权机制,确认 Phase 2 Base 控制器鉴权覆盖完整性 — `[Done: council/SecurityEngineer]` +- [x] **Task S2** — SQL 注入风险审计 — `[Done: council/SecurityEngineer]`(无注入,发现 P1 代码 bug 已记录) +- [x] **Task S3** — XSS / CSRF 防护检查 — `[Done: council/SecurityEngineer]`(无风险) +- [ ] **Task S4** — 敏感操作审计日志设计 — `[Pending]`(需新建 `vr_audit_log` 表设计文档) +- [x] **Task S5** — IDOR / 水平越权测试用例编写 — `[Done: council/SecurityEngineer]`(admin 上下文可接受) + +**遗留 P1 Bug(需立即修复)**: +- ⚠️ `Verifier.php:45` — CONCAT SQL 语法错误,需改为 PHP 拼接或 ThinkPHP 表达式格式 + +--- + ## 共识投票 -[CONSENSUS: NO] — 本轮仅完成研究讨论,实际执行待后续阶段 +[CONSENSUS: NO] — 遗留 P1 bug(Verifier.php:45 CONCAT 语法错误)需要修复,Task S4(审计日志设计)尚未完成。