Skip to content

fix: 安全与备份正确性修复、性能优化、测试体系补齐 - #376

Closed
cakkl wants to merge 15 commits into
shuaiplus:mainfrom
cakkl:Developer
Closed

cakkl wants to merge 15 commits into
shuaiplus:mainfrom
cakkl:Developer

Conversation

@cakkl

@cakkl cakkl commented Sep 12, 2026

Copy link
Copy Markdown

Summary

Developer 上的 15 个提交,四类内容:

1. 安全与正确性

  • JWT 用途隔离显式化:各类专用令牌与访问令牌共用同一个 JWT_SECRET,此前隔离只依赖「调用方随后还会比对字段」的隐式约定,现在双向显式拒绝(并修正两处判定不对称)。
  • 备份租约的「读 → 判断 → 写」放进 state.storage.transaction():此前两个并发请求可能双双拿到租约,使这个 DO 唯一的职责失效。
  • 补上 NAT64 前缀(64:ff9b::/96、64:ff9b:1::/48)的 SSRF 绕过;saveCipher 检查 meta.changes,不再把「写入被拒绝」当成功。
  • import 先校验后写入:此前文件夹在密文校验之前落库,导入失败后残留、重试不断累积。
  • 本地导出勾「包含附件」的 zip 不含附件字节(却被本地导入以 missing required file 拒绝);顺带修正体积预检的基数 —— 它没扣掉 db.json、也没算 zipSync 让内存翻倍,因此能产出「导出成功、但必然无法恢复」的归档。
  • hostFromUri 先裁剪再解析:带空白的地址此前返回空串或垃圾主机名 https。

2. 性能

  • 7 处清理/查询路径全表扫 → 新增索引;migrations/0001_init.sql 与运行时 schema 同步,并提升 STORAGE_SCHEMA_VERSION。
  • 消除 N+1 与串行 IO(管理端用户列表、设备信任/取消信任);删掉 handleListPendingAuthRequests 里一整段死代码;设备批量操作补数量上限。
  • 移除 initializeDatabase 中对 api.bitwarden.com 的调用(实测接到 429)—— 数据库初始化路径不应依赖第三方服务。

3. 移除私有的批量删除文件夹
POST /api/folders/delete 不是官方端点(官方是 DELETE /folders 与 DELETE /folders/all,本仓从未实现),只服务 Web UI 的「删除全部文件夹」一键操作。整条链路无其他调用者,一并删除;单条删除保留。

4. 测试体系与 CI

  • 新增 scripts/lib/:用内置 node:sqlite 实现真实 D1、内存 R2、SQL 记录器、共用夹具 —— 把此前的 mock 断言换成真实 SQL。
  • 200 项用例:备份 round-trip、迁移升级、查询计划、查询往返 + 8 个 handler + 前端纯逻辑。
  • verify.yml:此前仓库的测试脚本在 CI 中没有任何 workflow 执行。
  • CONTRIBUTING.md / PR 模板补上测试命令(此前只说 tsc/build/i18n:validate)。

Change Type

  • Bug fix
  • Refactor
  • Documentation
  • Feature
  • Compatibility update

Cross-File Checklist

  • I read CONTRIBUTING.md.
  • Schema changes, if any, updated both runtime schema and migrations/0001_init.sql.
  • Schema changes, if any, bumped STORAGE_SCHEMA_VERSION in src/services/storage.ts.
  • Persistent data changes, if any, updated backup export/import or documented why backup is not needed.
    → 未新增持久化数据;rate_limit_buckets 与 6 个 yubikey 列只是把运行时已有的定义补进 migrations,备份契约不变。
  • User-facing text changes, if any, updated all locale files.
    → 10 个语言包:新增 3 键、删除 4 键。
  • Bitwarden client compatibility was considered for sync/API shape changes.
    → 删除的是私有别名 POST /api/folders/delete,官方客户端不使用它;官方 DELETE /folders / DELETE /folders/all 本仓本就未实现,故无回归。
  • No secrets, tokens, private deployment values, or real vault data are included.

Checks

  • npm run verify(4 份 tsconfig + 200 项测试 + i18n 校验 + 构建)
  • 测试已连跑多次(概率门控的清理路径曾导致偶发失败,已在测试里钉住 Math.random)
  • 14 个代码提交逐个 checkout 跑过 npm run typecheck,全部通过(第 15 个是纯文档提交)
  • actionlint 与 zizmor(CI 同款镜像与参数)在最终 HEAD 上复跑:No findings to report

Notes

  1. 用户可见的功能移除:Web UI 不再有「删除全部文件夹」一键操作(RELEASE_NOTES.md:72 曾把它记为 v1.7.4 的修复项)。单条删除不受影响。发布说明段落尚未写(未定版本号)。
  2. STORAGE_SCHEMA_VERSION 已提升 → 部署后首次请求会重跑 schema(幂等)。
  3. 本 PR 只会跑 verify.yml:codeql.yml 与 security-extra.yml 的触发条件是 push,不含 pull_request,所以 actionlint / zizmor / semgrep / scorecard / OSV 不会在 PR 上把关(它们在推送 Developer 时已在分支上触发)。若想让它们在 PR 上跑,需要给这两个 workflow 加 pull_request。
  4. 前端样式批次做了运行时实测(此前多数对比度只是"算"出来的):.dialog-message 暗色 2.25:1 → 11.65:1、.eye-btn / .input-icon-btn 1.67:1 → 11.65:1、.empty 3.48 → 6.75、.vault-grid 在 1181–1400px 从 +187px 溢出降为 0。明细在本地文档 .vscode/TOFIX.md(未随提交)。
  5. 测试提交排在最后,因为它依赖前面的修复(导入写序、清理索引、N+1、内联导出预算);checkout 中间提交时这些测试尚未存在。

- 新增 `.github/workflows/verify.yml`:在 push 到 main 与所有 PR 上执行
  `npm run verify`(类型检查 + i18n 校验 + 测试 + 构建)。此前仓库的测试脚本
  在 CI 中**没有任何 workflow 执行** —— 测试写了却无人自动运行,回归不会被拦截。
- 删除 `security-extra.yml` 中已失效的 `pnpm audit` 步骤:项目使用 npm
  (仅有 package-lock.json),该步骤一直被守卫跳过;依赖漏洞扫描由 OSV 覆盖。
- Node 版本收敛到单一来源 `.nvmrc`(24.18.0):`verify.yml`、
  `sync-global-domains.yml` 与 Cloudflare Workers Builds 的构建镜像三方一致,
  与本地不再分叉,且无需额外下载。
- `scripts/` 纳入类型检查(新增 `tsconfig.scripts.json`)。
- `security-audit-backup-endpoint` 从"脚本"改为带断言的测试文件并纳入 `npm test`:
  它此前在 CI、npm scripts 与文档中均无引用,属死资产。
- 按 zizmor 的 `excessive-permissions` 建议,把权限从工作流级下移到 job 级
  (`codeql.yml` 与 `sync-global-domains.yml` 原先会让所有 job 共享过宽权限,
  是 2 个 high + 1 个 medium 的达标发现,会让 zizmor job 变红);
  不需要推送的 checkout 补 `persist-credentials: false`。
  注意 `sync-global-domains.yml` **有意不补** —— 它用 `create-pull-request` 推分支。
- 用途隔离显式化。各类专用令牌与访问令牌共用同一个 `JWT_SECRET`:
  - `AuthService.verifyAccessToken` 拒绝携带
    `cipherId`/`attachmentId`/`sendId`/`fileId` 的令牌;
  - `verifyFileDownloadToken` 拒绝携带 `sstamp`/`sub` 的令牌;
  - `verifyAttachmentUploadToken` 同样拒绝携带 `sstamp` 的令牌。
  此前隔离只依赖"调用方随后还会比对字段"这一隐式约定,现在升级为显式契约。
  顺带修正两处 `verifyAttachmentUploadToken` 与 `verifyFileDownloadToken`
  判定条件不对称的问题(一处只判 `sstamp`,未判 `sub`)。
- CSP 补 `worker-src 'self' blob:`(`src/utils/response.ts` 与 `webapp/index.html`
  两处需一致),否则保险库解密 worker 会被 CSP 拦掉。
`acquireJob` / `touchJob` / `releaseJob` 原本是 `await get()` 之后再
`await put()` / `await delete()`。两个并发请求可能都读到"当前无租约",
于是双双拿到 token,两个备份同时运行 —— 而这个 DO 的唯一职责就是阻止这件事。

改为在 `state.storage.transaction()` 内比对后写入(与 `notifications-hub.ts`
中 ws-token 的单次消费同构)。`releaseJob` 尤其需要事务:
"A 读到 token=A → B 覆盖为 token=B → A 依据旧快照判断通过并 delete() 掉 B 的租约",
会让并发保护静默失效。
- `backup-config.ts`:`64:ff9b::/96`(RFC 6052 知名前缀)与 `64:ff9b:1::/48`
  (RFC 8215 本地前缀)都会把 IPv6 地址映射回 IPv4。此前只判前 16 位,
  使内网地址可以绕过 SSRF 黑名单。现在前者解出内嵌 IPv4 后按 IPv4 规则判定,
  后者因前缀长度可变、无法可靠解出,整段拒绝。
- `storage-cipher-repo.ts`:`ON CONFLICT(id) DO UPDATE ... WHERE user_id=excluded.user_id`
  用于阻止跨用户覆盖,但当该 id 已被他人占用时该语句**既不更新也不报错**(静默 no-op)。
  现在检查 `meta.changes`,避免把"写入被拒绝"当成保存成功。
- `handlers/ciphers.ts`:为"带附件迁移元数据时跳过 stale 检查"这一豁免补注释,
  说明豁免范围仅限于用户覆盖自己的数据,避免后来者无意扩大。
- `migrations/0001_init.sql` 补上**仅存在于运行时建表语句**中的 6 个 YubiKey 列
  (`yubikey_key1..5`、`yubikey_nfc`)与 `rate_limit_buckets` 表及其索引。
  此前全新建库(走 migrations)与运行时 `ensureStorageSchema()` 的定义不一致。
- 兜底提权(实例中无管理员时,把最早创建的账号重新提升为 admin)此前**不留痕**,
  现在写入 `user.bootstrap.admin_promoted` 审计事件。
  该函数运行在 schema 初始化阶段,只有裸 `D1Database`(用 `StorageService`
  会造成模块循环依赖),因此直接插 `audit_logs`;审计写入失败不得中断初始化。
`POST /api/folders/delete` 是 NodeWarden 自己的私有别名,官方 API 并不存在
(官方提供的是 `DELETE /folders` 与 `DELETE /folders/all`)。整条链路
`route → handler → storage → storage-folder-repo` 无其他调用者,因此一并删除,
前端"删除全部文件夹"入口随之移除,侧栏该按钮位置改为"新建文件夹"。

同时清掉 3 个不再使用的 i18n 键(`txt_delete_all_folders` / `_message` /
`_failed`)× 10 个语言包(`txt_folder_deleted` 等单条删除仍在用,未受影响)。

顺带移除 `VaultSidebar.tsx` 中与侧栏其他入口重复的"密码安全"入口。
新建或迁移实例时,`createDefaultBackupSettings` 会自动生成一个"配置为空"的占位目标
(WebDAV 的 `baseUrl`、S3 的 `endpoint`/`bucket` 均为空)。进入备份中心会自动列举
远端目录,服务端因 `WebDAV server URL is required` 返回 409,前端随即把它当错误弹出 ——
用户**每次进页**都会看到"请填写 WebDAV 服务地址。",而那并不是错误。

新增 `isBackupDestinationConfigured()`,对未配置的目标跳过自动列举;
用户主动点"刷新"或保存后触发的加载仍会执行并如实报出真实错误。
- 暗色对比度:`.send-options`(1.93:1)、`.password-toggle`(2.58:1)等硬编码色值
  改为主题变量,暗色下提升到 11.63:1;`.totp-ring-track` 的 `stroke`、
  `.list-sub` 等零散选择器一并纳入 `dark.css` / 打磨层覆盖。
- 无障碍名称:为密码框、网站地址与匹配方式、TOTP 密钥、全局规则复选框补
  `aria-label`;列表复选框与整页 TOTP 复制按钮的可访问名称带上条目名;
  密码显示切换按钮补 `title`/`aria-label`。
- 布局溢出:`.vault-grid` 三栏最小宽度之和(270+400+400+24 = 1094px)在
  1181–1400px 区间会把网格轨道顶出容器,详情面板被截断、"删除"按钮不可达。
  `responsive.css` 新增 1400px 断点收窄侧栏与两栏最小宽度。
- Send 页"移除密码"按钮改用 `.password-toggle.danger`:原先靠组件里写
  `text-red-600 hover:text-red-700` 表达红色意图,但那两个工具类在加载顺序上
  必然被 `.password-toggle` 的同优先级规则覆盖 —— 意图红色、实际渲染为主题色,
  属从未生效的死代码。
说明三件事:首个注册账号自动获得 `admin` 并标记实例已注册;此后注册需要邀请码、
新账号只得到默认 `user` 角色(因此注册无法在既有实例上取得管理员);以及实例中
不再存在管理员时的兜底提权(按**账号创建时间**而非信任度选取,且管理员账号
本身不受删除保护)。

最后一点值得操作者知道:兜底提权只是防止实例永久失去管理能力,
不能替代多用户实例上的人工管理。
文件夹在密文校验**之前**就已落库,导入被 400 拒绝后这些文件夹会留在库里,
重试时不断累积。改为所有校验通过后再执行写操作。

这类"失败后留下半截状态"对密码管理器尤其危险:用户以为导入失败、
实际库里已经多了一批空文件夹。
查询计划(用 `EXPLAIN QUERY PLAN` 对着真实 schema 逐条核对):

- 7 处清理/查询路径此前全表扫,新增索引覆盖
  `refresh_tokens.expires_at`、`auth_requests.creation_date`、
  `trusted_two_factor_device_tokens.expires_at`、`webauthn_challenges.used_at`、
  `login_attempts_ip.updated_at`、`used_attachment_download_tokens.expires_at`、
  `rate_limit_buckets.expires_at`。其中一条清理语句的索引被 `OR` 排掉,
  改用两次独立 DELETE。
- `migrations/0001_init.sql` 与 `storage-schema.ts` 同步修改(仓库约定两处都要改),
  并提升 `STORAGE_SCHEMA_VERSION` —— 那是让**已有部署**拿到变更的唯一机制。

串行 IO / N+1:

- `GET /api/admin/users`:每个用户查一次 passkey → 一次批量查询 + `Set` 判断。
- `handleUpdateDeviceTrust`:每台设备各查一次 → 一次 `getDevicesByUserId` + Map,
  写入改用批量语句;`handleUntrustDevices` 同理。
- `handleListPendingAuthRequests`:删除一整段死代码 ——
  `getDevice(userId, X)` 的查询条件是 `WHERE device_identifier = X`,
  其返回值的 `deviceIdentifier` 必然等于 `X`,因此 `??` 回退分支不可达。
- 设备批量操作补数量上限(`LIMITS.device.maxBulkIdentifiers`),避免超长
  `IN (...)` 把绑定参数打满。

同时移除 `initializeDatabase` 中对 `api.bitwarden.com` 的调用:
它会在**数据库初始化**路径上向第三方发请求(实测拿到 429),
既是出网行为也是可用性风险,且该凭据同步并非本实例所需。
- 本地导出勾选"包含附件"产出的 zip **不含任何附件字节**,而归档的 manifest
  却声明 `includes.attachments: true`,因此会被本地导入以
  `missing required file` 拒绝 —— 一个自称包含附件、实则无法恢复的备份。
  新增 `BuildBackupArchiveOptions.inlineAttachmentBlobs`:本地导出置 true
  把附件字节内联进 zip,远端备份保持 false(它依赖单独的增量上传,
  内联会把附件在远端重复存一份)。
- 体积预检**基数算错**:原先只把"附件字节数"与 64 MiB 比较,既没扣掉 `db.json`
  (恢复侧按**所有条目解压后总字节**判定上限),也没考虑 `zipSync` 会把内存
  占用翻倍(峰值 ≈ 2 × 总量),因此可以产出"导出成功、但本地导入必然失败"的归档。
  改为 `resolveInlineAttachmentBudgetBytes()`:
  `预算 = min(内存总量上限, 恢复上限) − db.json`,总量上限取 32 MiB
  (峰值 ≈ 64 MiB,Worker 每 isolate 内存上限 128 MB)。预算为负表示
  `db.json` 自身已超恢复侧单条目上限,任何附件都不许内联。
- 超限与"远端形态归档走本地导入"两种情况都给出**可操作**提示而非通用报错
  (新增 2 个 i18n 键 × 10 个语言包,均非英文占位)。带具体字节数的文案无法
  精确查表,前端改用正则匹配并把字节换算成 MB。
`hostFromUri` 先用 `uri.trim()` 判空,却拿**未裁剪**的原串去判断 scheme 与解析:

- `'  https://example.com  '` 返回空串 —— 网站图标永远不显示;
- `'\thttps://a.com\n'` 返回垃圾主机名 `'https'`。

现在先裁剪再解析,并把这两种输入固定进测试。
此前 `scripts/` 下的测试只做"发了哪些 SQL"级别的 mock 断言:证明不了
"该备份的数据完整、能原样还原",也盖不到 handler 的参数校验路径。

基础设施(`scripts/lib/`):

- `d1-sqlite.ts`:用内置 `node:sqlite` 实现 D1Database,跑**真实 SQL**
  (真实 schema、真实影子表与 swap),可把两个库逐行比对;纯 mock 做不到这点。
- `r2-memory.ts`:内存 R2。**必须支持 ReadableStream** —— 早期版本对流主动抛错,
  导致附件上传路径完全盖不到(表现为莫名其妙的 500)。
- `sql-recorder.ts`:区分两个口径 —— `queries` 是 prepared 语句数(用于查查询计划),
  `roundTrips` 是数据库往返数(用于查 N+1);`bind()` 返回新对象,故语句也要套 Proxy。
- `test-harness.ts`:共用夹具,含 `resetProcessScopedStatics()` 与惰性 schema 预热
  (否则第二个库拿不到 schema、首个库的计数器会偏)。
- `cloudflare-workers-stub.mjs` + `register-cloudflare-stub.mjs`:`cloudflare:workers`
  在 Node 下无法解析(`ERR_UNSUPPORTED_ESM_URL_SCHEME`),用模块解析钩子补上。

用例(合计 200 项,全部纳入 `npm test`):

- 备份 round-trip 17、迁移升级 6、查询计划 1、查询往返 10
- handler:ciphers 12、sync 9、identity 18、sends 16、folders 12、
  import 11、attachments 11、backup 7
- 前端纯逻辑 32(新增第四份 tsconfig `tsconfig.webapp-tests.json` 提供 DOM 类型;
  `tsconfig.scripts.json` 排除 `scripts/webapp/**`,否则会被无 DOM 的配置捡走)

这些用例同时锁住 4 处**有意的不对称**(不导出 Send、不导出 `users.api_key`、
脱敏内部 config key、恢复后强制标记为已注册)—— 它们不是 bug 而是设计,
但必须显式记录,否则将来会被误改或误报。

注:其中 4 个测试文件必须以本分支此前的修复为前提(导入写序、清理索引、
N+1 消除、内联导出预算),故测试提交排在最后。
- `CONTRIBUTING.md` 新增「Tests」小节:说明 `npm test` 跑的是**真实 SQL**
  (D1/R2 由进程内实现支撑,无需外部服务与网络),并交代 `scripts/lib/` 各夹具的用途,
  以及两条"已经各自产生过一次假结果"的规则 —— 概率门控的清理路径必须钉住
  `Math.random`(或至少连跑两次),以及不要断言两个时间戳不同。
- `Recommended Checks` 改为以 `npm run verify` 为主(与 CI 跑的是同一条命令),
  再按场景给更窄的命令。此前这里只有 `tsc` / `build` / `i18n:validate` 三条,
  **一个字都没提测试**,而 CI 已经在跑它。
- PR 模板:`Cross-File Checklist` 补上 `STORAGE_SCHEMA_VERSION`(CONTRIBUTING 的
  Database Changes 一直这么要求,模板却漏了);`Checks` 改用 `npm run verify`,
  并新增"测试需连跑两次"的勾选项。
@gitguardian

gitguardian Bot commented Sep 12, 2026

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 5 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
- - Generic Password d500321 scripts/webapp/password-security.test.ts View secret
- - Generic Password d500321 scripts/webapp/password-security.test.ts View secret
- - Generic Password d500321 scripts/webapp/password-security.test.ts View secret
- - Generic Password d500321 scripts/sends-handler.test.ts View secret
- - Generic Password d500321 scripts/webapp/password-security.test.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@cakkl cakkl closed this Sep 12, 2026
@cakkl
cakkl deleted the Developer branch September 12, 2026 18:05
@cakkl

cakkl commented Sep 12, 2026

Copy link
Copy Markdown
Author

抱歉,看错了,选错成上游仓库了

@cakkl
cakkl restored the Developer branch September 12, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant