Conversation
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明:本评审报告由 AI 自动生成,当前使用 DeepSeek-V4.1-Flash 模型进行分析。
📖 PR #109 — fix(security): 第一波安全止血 — 提权链/admin 收权/注入修复/防爆破/Secret 契约
🎯 结论
结论:🔄 Request Changes
评分:功能 ⭐⭐⭐⭐ | 最小改动 ⭐⭐⭐ | 前向兼容 ⭐⭐⭐ | 方案设计 ⭐⭐⭐⭐
变更:59 文件(+1613 / -762);head=b8e80d2,base=dc2877b(origin/main),自身含 3 个 commit(b75dd6c / 13766be / b8e80d2)
行号说明:均以 head 修订版(
b8e80d2)的文件行号为准,等于 own-delta+侧行号;未出现在本 PR diff 中的文件(如handlers/nps.go、service/easytier/manager.go)为该修订版行号,已单独标注。
整体方案(2-3 句)
路线是对的:把"历史遗留登录旁路 + 任意键写配置"这条提权链连根拔掉(删 legacy 登录、去明文回退、PUT /system/config 收 admin + 键白名单 + 一次性数据迁移),再用 admin 组(JWTAuth+AdminOnly)系统性收编宿主机级写操作,最后用 Secret 类型 + PreserveSecrets 反射回填修掉"保存即清空凭据"的数据破坏 bug。分层也合理:路由层分组做粗粒度授权、handler 做输入校验、service 的 Manager.Start 再做一遍纵深校验(storage),并有源码级路由矩阵守卫 + 中间件单测防回归。主要问题不在方案而在覆盖面:收权清单与 Secret 契约都还留有明确遗漏(见下),且新增的防爆破因 ClientIP() 取值不可信而可被绕过。
核心改动清单
- 提权链阻断:
PUT /system/config移入 admin 组并加updatableConfigKeys白名单(仅 language/theme/speedtest_popup_enabled,system.go:118-148);删除"User 表无记录时经 SystemConfig 登录"路径(auth.go:39-96);CheckPassword去掉明文回退(utils.go:20-25);删除 3 处admin_password同步写入点。 - 存量平滑迁移:
migrateLegacyAdminPassword(db.go:75-107)把遗留口令提升为 admin User 记录(bcrypt 哈希沿用、明文现场哈希)后删除该键,配三态测试(db_test.go)。 - 宿主机级写操作收进 admin 组:port-forward / STUN / frpc / frps / NPS / EasyTier / tunservice / WireGuard(含 hook 字段)/ Caddy / cftunnel(含二进制下载)/ storage / firewall / cron 收窄;
/mesh/proxy收 admin(router.go:109-473)。读接口保留在 auth。 - SMB 配置注入修复:新增
storage.ValidateConfig(validation.go)对共享名/用户名做字符白名单,Create/Update/Start 三处调用,配注入拒绝测试。 - SFTP 禁匿名 + Secret 契约:SFTP 强制用户名密码(validation.go:54-58、manager.go:147-155);新增
model.PreserveSecrets反射回填(secret.go:75-118),接入 10 个 Update 路径;13 个敏感字段迁移到Secret类型(models.go)。 - 登录防爆破 + CI 门禁:
pkg/ratelimit(固定窗口 + 指数退避)、CORS 的*与 credentials 互斥、SQLite 0600、gofmt -l+go test -race、WG hook 文案警示。
问题清单
P0(阻塞合并)
-
[P0]
backend/api/handlers/auth.go:45(配合:58、backend/api/handlers/init.go:62)— 新增的防爆破可被 X-Forwarded-For 直接绕过,且可反过来用于锁死管理员登录- 事实链:
ip := c.ClientIP()作为限流 key;全仓库没有任何SetTrustedProxies调用(已 grep 确认)。gin v1.9.1 的New()默认ForwardedByClientIP: true、RemoteIPHeaders: ["X-Forwarded-For","X-Real-IP"]、trustedProxies: ["0.0.0.0/0","::/0"](gin.go:195-205,已在本地模块缓存核对),即"默认信任所有代理",ClientIP()会直接取客户端可伪造的 XFF 首值。 - 影响:攻击者每次请求带不同 XFF 即可无限次尝试口令(20/min 限流与 5 次退避同时失效,本 PR 第 7 条修复目标落空);反向也可用受害 IP(含管理员)填充限流桶,令其 429 无法登录。
- 修复建议:
并在限流处用
// router.go NewRouter() 内,r := gin.New() 之后 if v := os.Getenv("NETPANEL_TRUSTED_PROXIES"); v != "" { _ = r.SetTrustedProxies(strings.Split(v, ",")) // 显式可信代理 } else { _ = r.SetTrustedProxies(nil) // 直连部署:不信任任何转发头 }
c.RemoteIP()(直连真实来源),或在可信代理场景下由可信反代注入并只取 XFF 最后一跳。/init/setup(init.go:61)同样处理。
- 事实链:
-
[P0]
backend/api/handlers/nps.go:54-71(NpsServerHandler.Update的h.db.Save(&req))— Secret 契约遗漏,NPS 服务端凭据每次保存被清空NpsServerConfig.WebPassword/AuthKey已是Secret(models.go:299-300),GET 返回掩码,Secret.UnmarshalJSON会把掩码转成""(secret.go:44-55);本 handler 没有model.PreserveSecrets,于是前端每次保存都会把真实 Web 密码与 AuthKey 覆盖为空,并可能让 NPS 服务端以空口令重启。这正是本 PR 第 5 条要修的"数据破坏 bug",10 个调用点覆盖了 frp/easytier/ddns/domain/storage/ai/monitor/meshnode,唯独漏了 NPS 服务端(NpsClientConfig.AuthKey 仍是明文string,models.go:317,属同类未迁移)。- 修复建议:与
frp.go:104-112保持一致并补一条"掩码回传不清空 WebPassword/AuthKey"的单测。var existing model.NpsServerConfig if err := h.db.First(&existing, id).Error; err != nil { c.JSON(http.StatusNotFound, gin.H{"code": 404, "message": "配置不存在"}); return } model.PreserveSecrets(&req, &existing)
P1(建议修复)
-
[P1]
backend/api/router.go:217(auth.PUT("/linereg/config"))+backend/api/handlers/linereg.go:179-186— 刚收紧的重绑入口可从旁边绕开
本 PR 把/linereg/rebind-apply收进 admin,但同一资源的写配置仍在 auth:非管理员可写rebind_mode(auto/manual/off)、probe_interval_sec(下界 5s)等,并在 linereg.go:186 立即SetRebindMode生效。auto会经portRebinder"先停后起"重绑端口层服务(linereg.go:65-69、553-587),off则关闭自动选线 —— 与 admin-only 的rebind-apply语义冲突。建议PUT /linereg/config一并收 admin。 -
[P1]
backend/api/router.go:318-325(dnsmasq 全部写操作在 auth)
/dnsmasq/config、/dnsmasq/start|stop会mgr.Start()在宿主机绑定ListenAddr:ListenPort的 UDP DNS 服务并在Config.Enable时自动启动(misc.go:120-159);/dnsmasq/records增删改直接决定内网解析结果(manager.go:106-130 有 IP 校验,但 handler 侧 CreateRecord/UpdateRecord 未校验)。与已收权的 Caddy / port-forward 同类,建议收 admin。 -
[P1]
backend/api/router.go:382-385(access)+backend/api/handlers/domain.go:1858-1893— 间接重启 Caddy 站点
AccessRule的增删改在 auth 且restartBoundSites会对正在运行的站点调用h.caddyMgr.Restart(siteID)(domain.go:1899-1918),等于绕过admin.POST /caddy/:id/start|stop;同时访问控制规则本身是安全策略面。建议写操作收 admin。 -
[P1]
backend/api/router.go:462-471(mesh nodes 写操作在 auth)
/mesh/proxy已收 admin(很好),但/mesh/nodesPOST/PUT/DELETE(含AdminPassword凭据)、POST /mesh/ping、DELETE /mesh/events(清审计事件)仍在 auth。既然把"代理到远程节点"视为高危,节点注册与事件清理同属一个信任链,建议一并收 admin。 -
[P1]
backend/api/router.go:295-306(域名证书全流程在 auth)
POST /domain/certs/:id/apply|renew|step/*|confirm-dns会驱动 ACME 签发并把证书/私钥写入dataDir/certs/<id>(cert/manager.go:555-566,路径按 ID 拼接,无穿越风险)。本 PR 以"改变宿主机反代/证书行为"为由收紧了 Caddy,证书签发属同一范畴,建议收 admin。 -
[P1]
backend/api/router.go:351-363、518-525、424-435、241-248、261-265— 尚未收权的"有对外副作用/可发起出站请求"接口/ipdb/import-url(auth)→downloadAndParseCIDRs(req.URL)由服务端发起到任意 URL 的请求(SSRF 面,domain.go:1634-1650);/monitor/probesCRUD(auth)→ 对TargetAddr/HTTPProbeURL做 TCP/UDP/ICMP/HTTP 探测,等于给非管理员一个内网扫描器;/monitor/servers写(auth)→ 可存 SSH 凭据并驱动出站 SSH 采集(collector.goCollectMetricsViaSSH);/callback/*(auth)→ 使用云端 AK/SK 修改 CDN 回源;/ddns/*(auth)→ 用真实 DNS 凭据改写用户域名解析。
至少前三项建议收 admin(或加目标地址白名单/内网地址拦截),否则 RBAC 语义会前后不一致。
-
[P1]
backend/api/middleware/auth.go:94-109(AdminOnly,本 PR 未改动该实现)— 权限撤回有最长 24h 延迟
判定完全取自 JWT 的is_admin声明,不查库;GenerateToken有效期 24h(api/middleware/auth.go:31)、Logout为空实现(handlers/auth.go:116-118)。因此取消管理员或删除用户后,旧 token 在有效期内仍能通过 AdminOnly。本 PR 大幅扩大 AdminOnly 的保护面(宿主机写操作),这一延迟的实际影响随之放大。建议在 AdminOnly 内用CurrentUserID查库校验enable/is_admin(或用 token 版本号 / User.UpdatedAt 比对),至少缩短 TTL。另:alg混淆已防护(jwt.WithValidMethods([]string{"HS256"})+ HMAC 类型断言,auth.go:41-47),JWT 主密钥来自环境变量或dataDir/jwt_secret(0600 随机生成,pkg/secret/secret.go:42-76),未发现硬编码后门;initDefaultData也不再创建默认管理员(db.go:279-291)——这几项是可靠的。 -
[P1]
backend/model/db.go:75-107(migrateLegacyAdminPassword)— 迁移可能把历史可写凭据固化为管理员账号
迁移仅判断"是否存在 username = 'admin' 的用户"(db.go:83)。若某部署的管理员已改名/不叫 admin,而admin_password键仍存在,升级后会新建一个启用的 admin 用户,口令取自该键 —— 而这个键在旧版本可被任意登录用户通过PUT /system/config写入,若曾被攻击者植入,等于把攻击者口令"转正"成持久管理员。建议:存在任一is_admin = true用户时不要自动创建;创建后强制首次登录改密(或至少打印显著警告 + 记录审计事件),并保留"由操作员确认"的开关。 -
[P1]
backend/service/easytier/manager.go:651— 明文密钥进日志并落库
m.log.Infof("[EasyTier服务端][%d] 启动命令: %s %s", id, m.getBinaryPath(), strings.Join(args, " "))会把含--network-secret、--private-key、--pre-shared-key的完整命令行写出;easytier 用的是 DB logger(main.go:1949-1950 + pkg/logger/logger.go:93-133 的DBHook写库),因此密钥同时进入日志表与GET /admin/logs。建议打印前做脱敏(只输出 flag 名 +***),与本 PR 的脱敏目标对齐。 -
[P1]
backend/service/storage/validation.go:33-34—filepath.Clean严格相等判断会误伤既有配置(升级即失败)
if cleaned := filepath.Clean(root); cleaned != root { return error }会拒绝常见写法/srv/share/(Clean →/srv/share)或/srv//share;再叠加Manager.Start也调用校验(manager.go:71-82),老部署升级后这些存储配置会直接status=error且无法再保存。建议改成"使用 Clean 后的值",只在 Clean 后仍非绝对路径或含..时拒绝:root = filepath.Clean(root) // 直接采用规范化结果 if !filepath.IsAbs(root) || strings.Contains(root, "..") { return err } cfg.RootPath = root
-
[P1]
backend/service/storage/validation.go:59-61+backend/service/storage/manager.go:103-110— 同风险模型下 WebDAV 仍允许匿名
SFTP 已强制用户名密码,但webdav分支不校验Username/Password,且startWebDAV只在cfg.Username != ""时启用 Basic Auth —— 等于把RootPath以匿名方式暴露在ListenAddr(默认 0.0.0.0)上,与刚修的 SFTP 问题同类。建议 WebDAV 同样强制账号密码,或强制ListenAddr为回环/内网并给出明确警示。 -
[P1]
backend/service/wireguard/manager.go:131-134+backend/api/handlers/wireguard.go:31-38、224-255(+model/models.go:1162、1184)— hook 警示无强制,且 WG 私钥仍明文可导出- WG hook 的"警示"仅是本 PR 改的 i18n 文案(
preUpTip/postUpTip),服务端没有任何提示或审计:wg-quick up会以 root 执行PostUp/PreUp(manager.go:131-134),非空时建议至少写一条审计日志/启动前告警,让运维可追溯。 WireguardConfig.PrivateKey(models.go:1162)与WireguardPeer.PresharedKey(1184)仍是明文string,GET /wireguard(auth)整表返回,GET /wireguard/:id/peers/:pid/config|qrcode(auth)返回含本端私钥的完整客户端配置 —— 任一普通用户即可导出隧道私钥(对比wireguard/manager.go:290-315的GetInterfaceDetail已手工写成"***hidden***",可见两处策略不一致)。既然本 PR 已建 Secret 契约,建议把这两个字段纳入(或把 peers/config、qrcode 收 admin)。
- WG hook 的"警示"仅是本 PR 改的 i18n 文案(
-
[P1]
backend/pkg/ratelimit/ratelimit.go:87-88— 退避时长左移溢出后失效
lock := b.base << (e.failures - b.free - 1)无溢出保护;time.Duration为 int64,failures增大到约 68 次后左移溢出为负,lock > b.max不成立 →remaining <= 0→Blocked返回 false,退避自动失效(攻击者只需每次失败后等待 <15min 持续推高计数,长期可无限尝试)。建议改为累乘封顶或先裁剪指数:shift := e.failures - b.free - 1 if shift > 20 { lock = b.max } else { lock = b.base << shift; if lock > b.max { lock = b.max } }
-
[P1]
backend/api/router_matrix_test.go:17-35— 守卫清单不全,容易给出"已系统性收权"的错觉
adminRequiredPrefixes未包含本轮发现的/dnsmasq、/access、/domain/certs、/monitor、/ipdb、/callback、/mesh/nodes等;且它是源码文本匹配(换 group 变量名或用auth.Handle(...)即可绕过)。建议补齐清单,并把"这是约定级、非行为级守卫"的说明再显式一点(文件头已有类似表述,可在失败信息里带上"请同步更新清单"的提示)。
P2(可选优化)
- [P2]
backend/model/secret.go:97-118(PreserveSecrets) — 用fieldIndexByName(ot, f.Name)取 old 侧索引后ov.Field(j),当前 10 个调用点两侧同类型所以安全;但签名是any,一旦两侧类型不同会取错字段或索引越界 panic。建议开头加if cv.Type() != ov.Type() { return }。另外它不处理指针/切片内嵌 Secret(注释已说明)、且"空值=不修改"意味着用户无法再清空一个已保存的凭据(只能删记录重建),建议在前端提示与文档里写明。 - [P2] 仍以明文返回的敏感字段清单:
NpsClientConfig.AuthKey(models.go:317)、NpsTunnel.Password(337)、FrpcConfig.HTTPPassword(186)、DomainAccount.SecretKey(190)、CaddySite.BasicAuthPasswd(706)、CertAccount.EabHmacKey(798)、CallbackAccount.Config/MonitorNotificationChannel.Config(JSON 内含 token)、MonitorServer.AgentToken(1339)。PR 已把部分列入"第二批",建议在描述里给出明确清单与顺序,避免"已脱敏"被读成全覆盖。 - [P2]
backend/api/handlers/system.go:102-116(GetConfig,auth) — 只黑名单过滤admin_password一个键。当前键集(language/theme/speedtest_popup_enabled/probe_*)确实不敏感,但读侧建议与写侧一样用白名单,防后续新增键被动泄露。 - [P2]
backend/api/handlers/auth.go:74-84(用户名枚举与时间侧信道) — 文案已统一(好),但用户不存在时跳过 bcrypt、存在时执行 bcrypt,响应耗时可枚举用户名;backoffKey = ip|username也意味着"换个用户名就换桶"(对固定目标的爆破仍受限)。可选改进:对不存在的用户也跑一次 dummy bcrypt 比较。 - [P2] 前端未同步适配(本 PR 仅改 i18n 文案) — 后端校验是真实存在的(路由分组 + AdminOnly),不是"只隐藏按钮",这点可以放心;但前端没有按角色隐藏/禁用按钮,也没有统一 403 处理,非管理员点击写操作只会弹错误。登录响应与
/admin/users/me均已带is_admin(handlers/auth.go:111、admin.go:115-126、330-338),建议在本次或紧随的批次补上"只读视图 + 403 提示",避免体验倒退。 - [P2]
pr-checks.yml:93的go test -race ./...— race 构建耗时与内存明显上升,且会暴露既有代码里的历史竞态导致偶发红;建议先观察一次实际 CI 时长,必要时对慢包做并发限制(-p)。
安全遗漏清单(本 PR 最有价值部分:仍在 auth 组、但按本 PR 自身标准应属"宿主机级/高危"的接口)
| # | 接口(分组) | 位置 | 实际能力 | 严重度 | 建议 |
|---|---|---|---|---|---|
| 1 | PUT /linereg/config(auth) |
api/router.go:217、handlers/linereg.go:179-186 |
写 rebind_mode(立即生效);auto 触发"先停后起"重绑端口层服务,off 关闭自动选线 |
P1(与刚收权的 rebind-apply 语义冲突,可绕过) |
收 admin |
| 2 | /dnsmasq/config、`/dnsmasq/start |
stop、/dnsmasq/records*`(auth) |
api/router.go:318-325、handlers/misc.go:120-199 |
宿主机绑定 UDP DNS 端口并启停服务;决定内网解析结果 | P1 |
| 3 | /access POST/PUT/DELETE(auth) |
api/router.go:382-385、handlers/domain.go:1858-1893、1899-1918 |
改写访问控制策略并 caddyMgr.Restart() 重启 Caddy 站点 |
P1 | 写操作收 admin |
| 4 | /mesh/nodes POST/PUT/DELETE、/mesh/ping、DELETE /mesh/events(auth) |
api/router.go:462-471 |
增删改远程节点(含 AdminPassword)、清审计事件 |
P1 | 写操作收 admin |
| 5 | `/domain/certs/:id/apply | renew | step/* | confirm-dns`(auth) | api/router.go:295-306、handlers/domain.go:1235-1310 |
| 6 | /monitor/probes*(auth) |
api/router.go:528-532 |
对任意 TargetAddr/HTTPProbeURL 出站探测(内网扫描/SSRF) |
P1 | 收 admin 或目标白名单 |
| 7 | /monitor/servers POST/PUT/DELETE(auth) |
api/router.go:520-523 |
存 SSH 凭据并驱动出站 SSH 采集(CollectMetricsViaSSH) |
P1 | 收 admin |
| 8 | /ipdb/import-url(auth) |
api/router.go:356、handlers/domain.go:1634-1650 |
服务端请求任意 URL(SSRF) | P1 | 收 admin 或限制 http(s)+内网地址黑名单 |
| 9 | /ddns* 写 + run/start/stop(auth) |
api/router.go:241-248 |
用真实 DNS 凭据改写用户域名解析 | P2 | 视产品定位收 admin |
| 10 | `/callback/accounts | tasks*`(auth) | api/router.go:424-435 |
用云端 AK/SK 改 CDN 回源、出站 webhook | P2 |
| 11 | /wol 写 + /wol/:id/wake(auth) |
api/router.go:261-265 |
发送魔术包唤醒内网主机 | P2 | 可选收 admin |
| 12 | GET /wireguard、`GET /wireguard/:id/peers/:pid/config |
qrcode`(auth) | api/router.go:223、232、236-237;model/models.go:1162、1184 |
明文导出 WG 私钥/PSK(等价于拿到整条隧道) | P1 |
| 13 | /ai/*(含 /ai/plugins、/ai/cron-tasks、对话/流式)(auth) |
api/router.go:478-513 |
AI 插件(MCP 工具 + SKILL 包)与定时 AI 任务;插件内容可被替换 | P2(需确认插件包是否允许可执行内容) | 建议插件写操作收 admin;补充插件内容校验说明 |
关于"AI/MCP 工具"这条链路,补充核实结论(供维护者判断):MCP 服务端(
service/mcp/server.go)仅监听 127.0.0.1,且带 Bearer token(NETPANEL_MCP_TOKEN,未配置时随机生成(pkg/config/config.go:36-39),token 不写日志)。它暴露了会改变宿主状态的工具(tunservice_start/stop、portforward_start/stop、probe_config_set直接写 SystemConfig,mcp/server.go:718-730),但这些接口没有角色概念,只有 token。目前未发现可从面板 HTTP 层(auth 组)构造任意 JSON-RPC 请求打到 127.0.0.1:18090 的现成通路(ipdb/import-url为 GET、通知 webhook 为固定结构 body、AI provider 的 body 不完全可控),因此暂记为"疑似风险、需确认";一旦后续有任意可控 body 的出站请求入口(或面板被反代到本机其他容器),P0-1 那类 SSRF 就会升级为"非管理员 →宿主机操作"。建议给 MCP 增加作用域/审计,或把 MCP token 与调用者身份绑定。
其他已核实为"没问题"的点(避免误伤)
- 前端不是"只隐藏按钮":路由分组 +
AdminOnly是真实的服务端校验,且auth_test.go覆盖了无 token/非 admin/伪造 token 三种情况。 - SMB 注入修复是彻底的:
smbNameRe为锚定字符白名单,"ok\nroot preexec = ..."一类换行载荷在TrimSpace后仍被拒绝;useradd/smbpasswd/smbd全部走 argv 或 stdin,无 shell 拼接;ValidateConfig在 handler 与Manager.Start双重调用是正确的纵深防御。 sh -c只有一个使用点(service/cron/manager.go:169-171,cron 任务本身已在 admin 组),其余exec.Command均为 argv 形式(已全量 grep 核对);WG 配置文件路径按 ID 生成(wg%d.conf),无路径穿越。- 证书落盘路径按 ID 拼接(cert/manager.go:555-566),域名不参与路径,无穿越风险。
model/secret.go的掩码/回填契约有单测(secret_test.go),GetConfig、toUserResponse均不泄露口令;SQLite 文件(含 WAL/SHM)收 0600 与 CORS 的*+credentials 互斥处理都正确。
格式化 commit(b8e80d2)风险核查(已实测)
git diff --stat 13766be b8e80d2= 28 文件 +341/-334;加上-w后只剩 4 个文件:.github/workflows/pr-checks.yml(门禁本身)、backend/pkg/svcutil/service_other.go(仅删 2 个空行)、webpage/src/i18n/locales/{en,zh}.ts(WG hook 提示文案)。- 结论:全量格式化 commit 中没有夹带任何行为变更,噪声最大的是
handlers/monitor.go(102)、monitor/task.go(46)、monitor/collector.go(44) 等空白/对齐修改。这点做得非常干净,值得肯定。 - 提交切分建议:格式化单独一个 commit 已是好做法;若能把
svcutil/service_other.go这类纯空行改动也归入格式化 commit 的说明(已是),并避免与 CI 门禁同 commit(现在同一 commit,回滚门禁时会连带回滚格式化),会更便于回滚。
跨 PR 影响(重要,需维护者决策)
-
新增的 gofmt 门禁会让一批 open PR 变红(已实测):
origin/main的backend/有 36 个 .go 文件未通过gofmt,而b8e80d2为 0 个(本机 gofmt 1.22 实测;CI 用 1.25,此类代码差异可忽略——本环境无 1.25 工具链,如需 1.25 的精确结论请在 CI 复核)。CI 在 PR 的合并提交上执行,因此只要某个 PR 改动了这 36 个文件中的任意一个,该 PR 的门禁就会红。命中清单(按own/*.own.diff与 main 未格式化文件求交集):PR 命中文件数 命中文件 #107 6 main.go、model/models.go、callback/manager.go、cftunnel/manager.go、frp/manager.go、tunservice/manager.go #110 4 main.go、pkg/logger/logger.go、cert/manager.go、monitor/manager.go #118 3 cert/manager.go、ddns/manager.go、meshnode/manager.go #115 2 monitor/manager.go、monitor/probe.go #119 2 api/router.go、main.go #100 / #105 / #106 / #117 1 api/router.go #102 1 api/handlers/domain.go #112 1 cftunnel/downloader.go #114 / #116 1 main.go 共 14 个 PR 受影响。建议二选一:(a) 在 #109 里把剩余 36 个文件也一次性 gofmt(彻底消除噪声来源);(b) 门禁先只对"本 PR 新增/改动的文件"做 gofmt 校验(
gofmt -l $(git diff --name-only origin/main...HEAD -- '*.go')),把全仓库格式化作为独立 PR。并在 PR 描述里提示需要 rebase。 -
#110/#111 与 #109 确为真堆叠(已用
git merge-base --is-ancestor b8e80d2 refs/remotes/pr/110|111验证:两者都包含b8e80d2)。它们继承格式化因此门禁不会红;但 GitHub base 都写着main,展示的是累积 diff,会误导 review 范围(#111 自身 16 文件,累积看起来大得多)。main 走 squash,合并 #109 后 #110/#111 rebase 时需留意重复提交与同区冲突:#111 也改webpage/src/i18n/locales/{en,zh}.ts,与 #109 的 WG 提示文案处在同一区块,冲突概率高。 -
#107(2110 行,含 6 个被格式化文件) 与 #109 的格式化会产生大范围空白冲突,建议协调合并顺序(先 #109,其余 rebase)。
-
若合并 #109 后出现 403 报错反馈,很可能是"前端未按角色适配"而非后端错误,建议在 release note 里先写明这条破坏性变更(宿主机级写操作对非管理员返回 403)。
产品视角评估
这条 PR 解决的是真实且严重的问题:旧版本确实存在"任意登录用户一步接管管理员"的路径,以及多个 root 级注入面,优先级高于任何功能开发,方向完全正确,且把"老部署不退线"作为硬约束来设计迁移(保留口令语义、自动提升为 User 记录)体现了对运维场景的体谅。易用性上有一处需要补:收权后非管理员会看到大量可用按钮但点击即 403,错误信息对运维不够可行动(建议统一提示"该操作需要管理员权限(RBAC 角色体系见路线图)"并给出只读态)。需求覆盖度上,家庭宽带/NAT/单管理员部署基本无感,这也是 PR 描述里说"单管理员部署无感知"的合理之处;但"多用户 + 非管理员"这一真实场景下,判定的边界(哪些算宿主机级)还没有收敛成一份明确的清单——本 PR 建立的路由矩阵测试是很好的起点,建议以它为"唯一事实来源"把边界定下来,避免每批都靠人工排查(本次已发现 13 处遗漏)。更小的 MVP 是:先只做"提权链阻断 + Secret 契约 + SMB/SFTP",把大量路由收权与全量格式化拆成独立 PR,评审与回滚都会更可控(当前 59 文件 / 5044 行里约有 340 行是格式化噪声,且收权清单尚不完整,混在一个 PR 里放大了评审成本)。
兼容性/迁移风险
有,共 4 处,均建议写入 release note:
- 非管理员权限收缩(破坏性):宿主机级写操作返回 403,前端未适配 → 需要 UI 只读态与 403 提示。
- SFTP 匿名被硬性禁止(破坏性):
ValidateConfig强制用户名密码,存量"匿名 SFTP"配置升级后Start会失败并置status=error,需要操作员补账号密码;错误文案已足够清晰,建议在 release note 给出一句操作指引。 - 存储
RootPath校验过严(疑似非预期破坏):带尾斜杠/双斜杠的历史配置会被拒绝(validation.go:33-34),建议按上文改为"采用 Clean 后的值"。 - 旧口令迁移(需人工确认风险):
migrateLegacyAdminPassword在"无名为 admin 的用户"时会新建 admin 账号并删除admin_password键(不可自动回滚),建议加"存在其他 admin 时不自动创建 + 强制/提示改密"。
其余方面兼容性良好:API 路径与响应结构未变(PUT /system/config 仅收紧键白名单,language/theme/speedtest_popup_enabled 三个既有键仍可写);DB schema 未破坏性变更;Secret 的 Value()/Scan() 保持原有明文存储格式,与既有数据兼容。
值得肯定的点
- 提权链是"连根拔"而非"打补丁":legacy 登录分支、
CheckPassword明文回退、三处admin_password同步写入点、PUT /system/config的任意键写入 —— 四个环节同时收敛,并配了"遗留口令迁移三态"测试(哈希/明文/无遗留键),既堵住新路径又照顾了老部署,思路很稳。 Secret契约的"空值=不修改"设计 + 反射回填 + 单测,正面解决了"前端回显掩码导致保存即清空凭据"这个隐蔽的数据破坏 bug(虽然 NPS 漏了一个调用点,但机制本身是对的,补一处即可)。- SMB 注入修复做到了纵深防御:字符白名单 + handler/
Manager.Start双重校验 + 注入拒绝测试,且用 argv/stdin 规避了 shell 拼接 —— 这一处修复质量很高。 - 全量格式化没有夹带行为变更,并且用
-w一比就能证明(本报告已核对),加上路由矩阵守卫防回归 —— 这类"可验证的干净改动"对维护者非常友好。
建议操作理由
方案与方向都对、核心链路修复扎实,但防爆破可被 X-Forwarded-For 绕过(P0)、NPS 凭据会被保存清空(P0)、以及 13 处宿主机级接口仍在 auth 组(P1,其中 /linereg/config 可直接绕过本 PR 刚加的门)与新增 gofmt 门禁会影响 14 个 open PR(P1),建议补完这些点(尤其 P0 与收权清单)后再合并。
b8e80d2 to
a7c0611
Compare
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明:本评审报告由 AI 自动生成。
🎯 结论
✅ Approve(附 P1 跟进建议)— 安全修复整体质量高,核心提权链已堵住,有少量遗留点
📖 概要
第一波安全止血 · 提权链 / admin 收权 / 注入修复 / 防爆破 / Secret 契约
核心改动:59 个文件,彻底移除旧版 admin_password 提权路径,实现三层登录防爆破,引入 PreserveSecrets 解决凭据静默清空问题,SMB/SFTP 注入修复,CORS 凭据泄露修复,SQLite 文件收权 0600,路由鉴权矩阵守卫测试
🧭 整体方案
采用"彻底移除历史遗留路径 + 新增基础设施防御"的双线策略:① 完全移除 SystemConfig.admin_password 登录路径(配合 migrateLegacyAdminPassword 平滑升级老部署);② 新增 ratelimit.Backoff 指数退避、storage.ValidateConfig 白名单校验、PreserveSecrets 反射机制、router_matrix_test.go 鉴权矩阵守卫,从根本上解决一批系统性安全债务。方案合理、改动彻底。
📊 变更统计
59 个文件(+1477 / -681 行) | 功能 ⭐⭐⭐⭐ | 安全修复质量 ⭐⭐⭐⭐⭐ | 最小改动 ⭐⭐⭐ | 测试覆盖 ⭐⭐⭐⭐⭐
🚨 关键问题
无 P0 问题。 核心安全修复均已落地:提权路径彻底移除(7 处 admin_password 兼容代码全部清除)、PUT /system/config 收入 admin 组 + 键白名单、登录三层防护(Limiter + IPLimiter + Backoff)、SMB 共享名注入修复(正则白名单 ^[A-Za-z0-9_-]{1,64}$)、SFTP 匿名放行移除、CORS credentials+wildcard 脚枪修复。
P1(建议跟进):
- 💡
backend/pkg/ratelimit/ratelimit.goBackoff.entriesmap 永不清理,长期运行后所有历经爆破攻击的 key 会持续累积(即使已过 max 窗口后新请求到来时单条可以惰性删除,但 map 本身没有定期 GC),建议加定时清理或用sync.Map+ TTL 结合时间戳清理过期条目。 - 💡
backend/api/middleware/auth.goallowedOrigins()从SystemConfig读取 CORS 白名单时,若数据库不可用(如启动初期)静默返回空列表,导致所有跨域请求均被拒绝,建议提供一个合理的默认值(如[]string{})并记录 warning 日志。 - 💡
backend/api/router_matrix_test.go的测试方案是"扫描 router.go 源码字符串"而非"运行时路由表",字符串匹配可能遗漏动态路由注册(如通过 for 循环注册)。整体设计值得肯定,建议在注释中明确这个已知局限。
P2(可选):
- 💡 本 PR 体量大(59 文件),混合了认证安全修复、注入修复、CORS 修复、Secret 契约等四类不相关改动,使得增量审查困难。下次同类安全修复建议按安全域(认证/权限/注入/凭据)拆分为独立 PR。
📂 逐文件分析
backend/api/handlers/auth.go
改动意图:移除旧版 SystemConfig 兼容登录,新增三层防爆破
代码逻辑:登录流程改为只走 User 表 bcrypt 验证;在已有 loginLimiter(5次/分钟)和 ipLimiter(20次/5分钟)基础上新增 loginBackoff(5次免锁后指数退避,封顶 15 分钟),登录成功后同步重置三层计数器
问题分析:Backoff.Blocked 与 RecordFailure 之间存在 TOCTOU 的微小竞态(检查通过后、进入数据库验证前,并发的另一个请求也可能通过检查),但在 mutex 保护下 RecordFailure 会准确记录,实际可利用窗口极小,可接受。整体实现正确。
backend/model/secret.go(新增 PreserveSecrets)
改动意图:解决"查看后保存把凭据清空"的历史 bug
代码逻辑:反射遍历结构体字段,对 Secret 类型的空值字段从旧记录回填,递归处理嵌入结构体
问题分析:实现正确,边界情况(异类型对比、未导出字段)均处理得当,补有完整的测试,质量很高。
backend/service/storage/validation.go(新增)
改动意图:修复 SMB 共享名/用户名注入、SFTP 匿名放行
代码逻辑:正则白名单 + 绝对路径校验 + 路径清洗(filepath.Clean 防 .. 遍历)
问题分析:注释明确说明 SFTP 无文件系统沙箱是已知限制(见 ROADMAP 长期池),这是诚实的设计文档,且现有防线(强制鉴权、RootPath 校验)已是合理当下防御。✅ 无重大问题。
backend/api/router.go + router_matrix_test.go
改动意图:高危宿主机写操作收入 admin 组,守卫测试防回归
代码逻辑:router.go 新增注释明确每个路由组的危险级别;router_matrix_test.go 扫描源码确保白名单内路由不会被误注册到 auth 组
问题分析:admin.GET/PUT("/system/config", ...) 已在本次 diff 中确认(第 129-130 行),提权链已彻底堵住。守卫测试设计思路好,P1 的字符串匹配局限不影响合并。
✅ 待处理清单
- [P1]
Backoff.entries添加定时/惰性 GC 防止长期内存积累 - [P1]
allowedOrigins()数据库不可用时的 fallback 行为需验证
🎯 结论:✅ Approve — 这是本批 PR 中安全质量最高的一个,核心提权链修复彻底、测试覆盖到位、边界条件处理细心。P1 问题不阻塞合并,建议后续 issue 跟进。
P0-1 访问控制整体失效:api/router.go 改为在注册路由之前挂载 AccessManager 中间件(Gin 在注册路由时对中间件链做快照,此前在路由注册完成后才注入,导致 IP 黑白名单/BasicAuth/页面登录对全部 /api/v1 接口不生效);新增 NETPANEL_TRUSTED_PROXIES 配置,默认不信任任何代理,避免伪造 X-Forwarded-For 绕过限流与 IP 名单;访问控制中间件改用 c.ClientIP();新增 service/caddy/acl_module.go 补齐站点级 IP 黑白名单(反代站点由 Caddy 直接服务,不经过面板 Gin 中间件,此前规则中的 Mode/IPList 在 Caddy 侧从未被消费)。 P0-2 权限越界:将可修改宿主状态、可向公网暴露内网、持有凭据的写操作统一收敛到 admin 组(storage、security/firewall、cftunnel、wireguard、ddns、domain/*、callback/*、port-forward、stun、frp*、nps*、easytier*、tunservice、dnsmasq、access、ipdb、ai 等),只读接口保持登录可用;存储新增根目录/凭据/监听地址校验,禁用用户名为空即放行任意登录的 SFTP 匿名访问,WebDAV/SFTP 口令改为常量时间比较,存储列表密码脱敏。 P0-3/P0-4:登录限流新增纯 IP 维度;登出调用 ClearSessionCookie 使 netpanel_session 立即失效(此前登出后该 Cookie 仍有效 24 小时)。 P1 认证与密钥:OAuth 不再按 preferred_username 绑定本地同名账号(IdP 可控该值,构成账号接管),改为仅按 provider+sub 复用身份;WS 终端复用 JWTAuth 口径复核账号状态与 token_version;实现腾讯云 TC3-HMAC-SHA256 与阿里云 ACS3-HMAC-SHA256 回调签名(此前分别为 placeholder 字面量与缺失签名)。 P1 稳定性:修复 easytier StopAll 与 watcher 并发调用 cmd.Wait() 竞态、stderr 缓冲无锁读写、stopping 标志不复位;cftunnel 同类问题;UDP 端口转发会话表提升为实例字段并在 Stop 时关闭上游连接,消除 goroutine 与 socket 泄漏;callback Stop 使用 sync.Once;monitor SSH 连接池加锁并在停机回收,未鉴权 gRPC 改为仅监听 127.0.0.1;cert 新增 StopAll,main.go 补齐 cert/ai 停止回调并真正启动防火墙规则定时同步;防火墙规则新增字段校验以消除 nftables 表达式注入;NPS 未配置口令时生成随机口令且配置文件权限改 0600。 前端:vite 增加 manualChunks 分包(首屏入口由两个 1MB+ chunk 降为 254KB);移除未使用依赖 @ant-design/charts;新增 api/authFetch.ts 修复三处裸 fetch 未携带 Authorization;修复 MonitorTerminal resize 监听器未移除导致的内存泄漏;移除 MainLayout 重复菜单 key。 测试:新增 caddy/acl_module_test.go、firewall/validate_test.go、storage/validate_test.go;go build / go vet / go test 全绿,前端 tsc + vite build 通过。云厂商回调签名按官方算法实现但未接入真实账号验证。
提权链(高危):PUT /system/config 此前任意登录用户可写任意键(含 admin_password),配合旧版"经 SystemConfig 登录"路径可直接接管管理员。 - UpdateConfig 移入 admin 组 + 键白名单(language/theme/speedtest_popup_enabled) - 移除 legacy admin_password 登录路径与全部同步写入点 - CheckPassword 去除明文回退比较 - db 层一次性迁移 migrateLegacyAdminPassword:遗留口令提升为 admin User 记录(bcrypt 现场哈希)后删除该键,老部署平滑升级 admin 组收权:wireguard / firewall / caddy / storage / cftunnel(含 二进制下载)/ easytier / frp / nps / stun / port-forward / tunservice 的写操作与启停、linereg 重绑执行、mesh 远程节点代理,一律要求管理员; 读接口保持登录可用。附源码约定守卫测试防回归。 登录防爆破:新增 pkg/ratelimit(固定窗口限流 + 失败指数退避), 登录按 IP 限 20/min、失败 5 次起按指数锁定至 15min;/init/setup 限频。 CORS:白名单配 * 时不再与 Allow-Credentials 并发(配置脚枪)。 测试:JWT/AdminOnly 中间件、限流/退避、旧口令迁移三态、路由矩阵守卫。
Secret 数据破坏修复:Update 普遍直接 Save 请求体,而 Secret 字段经 GET 回显为掩码,前端原样提交时掩码被转为空串,每次保存都会清空真实 凭据。新增 model.PreserveSecrets(反射回填"空值=不修改"),接入全部 含 Secret 字段的 Update 路径(frp/ddns/easytier/domain/storage/mesh/ ai/monitor)。 敏感字段脱敏迁移(第一批 13 个):Frpc/Frps Token、EasyTier 网络密码/ 私钥/预共享密钥/服务端 Token、DDNS AccessSecret、域名账号 AccessSecret (移除手工"****+尾号"脱敏)、存储密码,改为 Secret 类型——List/Get 不再返回明文,服务端配置生成经 String() 取原文不受影响。 SMB 配置注入(root RCE 面):共享名/用户名白名单字符校验,注入指令 (如换行 + root preexec)被拒绝;RootPath 强制绝对路径且拒绝 ..。 SFTP:移除匿名放行,强制用户名密码(sftp.NewServer 无沙箱,匿名等于 向局域网开放面板权限的整个磁盘);新增 ValidateConfig 在 handler 与 Start 双重校验。附共享名注入/匿名拒绝/路径穿越测试。
- pr-checks 新增 gofmt 检查步骤与 -race 测试,杜绝格式违规与数据竞态 - WireGuard PreUp/PostUp 提示文案改为明确警示"以面板权限执行 shell" - 后端全量 gofmt(此前 11 个文件存在未格式化代码被提交)
a7c0611 to
8a2cb8d
Compare
背景
ROADMAP v2 第一波安全止血。安全调研发现一条真实提权路径与多个 root 级注入面,优先级高于所有功能工作。
修复内容
1. 提权链阻断(高危)
PUT /system/config此前任意登录用户可写任意键(含 admin_password),配合旧版"User 表无记录时经 SystemConfig 明文/哈希登录"路径,非管理员可一步接管管理员:CheckPassword去除明文回退比较2. 宿主机级写操作统一收进 admin 组
WireGuard(PostUp/PreUp = shell 执行)、系统防火墙、Caddy、存储(创建系统用户/写 smb.conf)、EasyTier/FRP/NPS/STUN/端口转发/CF Tunnel 的写与启停、linereg 重绑执行、mesh 远程节点代理——此前任意登录用户可操作宿主机。读接口保持登录可用;附源码约定守卫测试防回归。
3. SMB 配置注入修复(root RCE 面)
共享名未清洗直接写入 smb.conf,换行可注入
root preexec等指令。共享名/用户名改为白名单字符校验。4. SFTP 禁匿名
sftp.NewServer无文件系统沙箱,匿名放行等于向局域网开放面板权限的整个磁盘。现强制用户名密码 + 路径/IP 校验(handler 与 Start 双重校验)。完整沙箱列入路线图长期池。5. Secret 契约执行(数据破坏 bug)
Update 普遍直接 Save 请求体,Secret 字段经 GET 回显为掩码,前端原样提交时掩码被转为空串——每次保存都会清空真实凭据。新增
model.PreserveSecrets反射回填(空值=不修改),接入全部相关 Update 路径。6. 敏感字段脱敏(第一批 13 个字段)
Frpc/Frps Token、EasyTier 密码/私钥/预共享密钥、DDNS/域名账号 AccessSecret、存储密码等迁移到
Secret类型,List/Get 不再返回明文;服务端配置生成不受影响。7. 登录防爆破 + CORS + 数据库收权
pkg/ratelimit:登录按 IP 限 20/min,失败 5 次起指数退避至 15min;/init/setup限频*时不再与 credentials 并发8. CI 门禁
pr-checks 新增 gofmt 检查与
-race测试;后端全量格式化。测试
go build / go vet / go test -race ./...全绿tsc --noEmit通过兼容性说明
已知边界
🤖 Generated with ZCode