fix(desktop): validate credential cache source slugs#5913
Conversation
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: this is a clean security hardening fix that closes the last unvalidated source-slug-to-path call site identified in #5909. Using the shared On approach: the scope is tight — extract the inline credential cache code from One note for the reviewer: the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个干净的安全加固修复，闭合了 #5909 中发现的最后一个未经验证的 source-slug-to-path 调用点。使用共享的 方案：范围紧凑——将 给 reviewer 的一个备注： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: given "last unvalidated source-slug-to-path call site in credential cache reader," I would have made a minimal inline fix — import PR's approach: extracts the credential cache block into a focused Verdict: the PR's approach exceeds the minimal fix in useful ways. The extraction makes the code independently testable, the error handling improvements are genuine (silent ENOENT + logged corrupt JSON is better than silent catch-all), and the test coverage is thorough. Every change in the diff serves the stated goal — no scope creep, no drive-by refactors. No correctness bugs, security holes, or regressions found. The One minor observation (not a blocker): the Real-Scenario TestingThis PR modifies internal credential cache infrastructure in the session MCP server — the code reads
bun is not available in this CI environment, so I couldn't run No tmux before/after applicable — this is non-user-facing infrastructure code. 中文说明代码审查独立方案： 给定"credential cache reader 中最后一个未经验证的 source-slug-to-path 调用点"，我会做一个最小内联修复——导入 PR 的方案： 将 credential cache 代码块提取到独立的 结论： PR 的方案在有用的方面超越了最小修复。提取使代码可以独立测试，错误处理改进是实质性的（静默 ENOENT + 记录损坏的 JSON 优于全静默 catch），测试覆盖很全面。diff 中的每个改动都服务于声明的目标——没有范围蔓延，没有顺手重构。 没有发现正确性 bug、安全漏洞或回归。 真实场景测试这个 PR 修改的是 session MCP server 内部的 credential cache 基础设施——代码读取由 Electron 主进程写入的 当前 CI 环境没有 bun，无法直接运行 tmux before/after 不适用——这是非用户可见的基础设施代码。 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped security fix. The credential cache reader had one remaining call site that built paths from raw The extraction from I'd maintain this happily. Approving. ✅ 中文说明这是一个干净、范围合理的安全修复。credential cache reader 还有一个调用点在未经验证的情况下从原始 从 维护起来没问题。批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local real-runtime verification (maintainer)Built and ran the real desktop package at this PR head ( What the fix does: base built the cache path with a raw Environment: macOS (Darwin), Bun 1.3.14, worktree on 1. PR test plan — green
2. Mutation — the traversal test is non-vacuousReverting The "Received value" is the smoking gun — under the raw join the path resolves outside 3. Real traversal A/B — actual reader, real on-disk fixtures — 10 / 10Ran the real
So invalid slugs now surface as a validation error distinct from a cache miss, exactly as the PR intends, and the secret is never reachable through the credential-cache path. 4. No regression
Notes for the maintainer
🇨🇳 中文版（完整对应）✅ 本地真实运行时验证（维护者）在本 PR head（ 这个修复做了什么: base 用原始 环境: macOS,Bun 1.3.14,worktree 在 1. PR 测试计划 — 全绿
2. 变异测试 —— 遍历测试非空过把 "Received value" 就是铁证 —— 原始拼接下路径解析到了 3. 真实遍历 A/B —— 真实 reader + 真实磁盘 —— 10 / 10用真实
所以非法 slug 现在表现为一个区别于 cache miss 的校验错误(正是 PR 的意图),且 secret 永远无法经 credential-cache 路径触达。 4. 无回归
给维护者的备注
|
✅ Maintainer local verification — PASS (with one important merge caveat)I built and ran this PR locally on a clean worktree at the PR head ( Environment: macOS (Darwin 25.5.0), Bun 1.3.14, Node v22.22.2 · isolated worktree, 1. Reviewer Test Plan — all reproduce
2. Adversarial mutation test — the guard is load-bearingA passing test only matters if it fails when the fix is removed. I reverted Restoring the I also confirmed 3. The "throw on invalid slug" design is safeThis PR intentionally lets an invalid slug throw (the
Net: invalid slugs surface as a loud, diagnostic tool error instead of looking like "no cached credential". That is a defensible choice (see comparison below). 4. Small positives worth noting
|
| Aspect | #5913 (this PR) | #5914 |
|---|---|---|
| Scope | credential-cache call site only | credential-cache + validator path sanitization + source-permissions RPC diagnostics + Bun test-isolation fix |
| Invalid slug → | throws → surfaces as a tool error (loud/diagnostic) | returns null (treated as a missing credential, quiet) |
| Guard | reuses getSourcePath() |
calls assertValidSourceSlug() directly |
| Cache-read diagnostics | injected onReadError logger; ENOENT-vs-corrupt split; logs corrupt JSON |
plain catch → null (no logger) |
| Tests on this file | valid/missing/expired/empty/corrupt/traversal | valid/traversal/null-on-invalid |
| Issue link | "Refs #5909" | "Closes #5909" |
Both are correct and safe — I verified each independently (the #5914 report is on that PR). My suggestion for the maintainer:
- This PR (fix(desktop): validate credential cache source slugs #5913) is the better-isolated, lower-risk credential-cache fix with richer diagnostics and broader cache-read test coverage.
- fix(desktop): harden remaining source path validation #5914 is the superset in scope and formally closes Harden remaining slug-to-path call sites and invalid slug diagnostics #5909, but its credential-cache slice is thinner (no logger, no ENOENT split, fewer tests) and it chooses the quieter
nullbehavior. - Cleanest path: decide the invalid-slug philosophy (loud-throw vs quiet-null), then either (a) merge this PR for the credential-cache file and have fix(desktop): harden remaining source path validation #5914 rebase to drop the duplicate and keep only its validator/RPC/test-isolation parts, or (b) take fix(desktop): harden remaining source path validation #5914 as the base and fold this PR's
onReadError/ENOENT diagnostics + extra tests into it.
CI / merge state
Test (ubuntu-latest, Node 22.x) is green; mac/win/integration legs skipping (named-job gating). mergeStateStatus: BLOCKED = REVIEW_REQUIRED (needs maintainer approval), not a CI failure. Note packages/desktop/ is a separate Bun workspace excluded from root CI, so these changes aren't exercised by the root pipeline — the local run above is the actual evidence.
Verdict
On its own merits, #5913 is correct, safe, well-tested, and ready to merge. The only decision required is #5913 vs #5914 — they cannot both land. 👍
🇨🇳 中文版（完整对应）
✅ 维护者本地验证 —— 通过（但有一个重要的合并提醒）
我在 PR head（ca167ce）的干净 worktree 上、在 macOS 本地构建并运行了本 PR，并做了对抗性变异测试确认 guard 是有效负载。所有结果都与 PR 声明一致。有一个阻塞项，针对的是合并顺序而不是代码本身：本 PR 与 #5914 重叠并冲突——两者只能合一个。详见末尾。
环境： macOS（Darwin 25.5.0）、Bun 1.3.14、Node v22.22.2 · 独立 worktree、ELECTRON_SKIP_BINARY_DOWNLOAD=1 bun install（1560 包，干净）。
1. Reviewer Test Plan —— 全部复现
| 检查 | 结果 |
|---|---|
bun test src/credential-cache.test.ts |
5 通过，0 失败（11 expects）✅ |
bun run build |
通过（打包 391 模块）✅ |
bunx tsc -p tsconfig.json --noEmit --allowImportingTsExtensions |
干净（exit 0） ✅ |
git diff --check HEAD |
无空白字符错误 ✅ |
2. 对抗性变异测试 —— guard 是有效负载
测试通过只有在“把修复删掉后会失败”时才有意义。我把 getCredentialCachePath 回退成旧的直接拼接（join(workspaceRootPath, 'sources', sourceSlug, …)，绕过 getSourcePath）。穿越测试随即失败——路径解析到了 sources/ 目录之外（即植入 secret 的位置），并且不再抛错：
expect(() => getCredentialCachePath(root, '../sessions')).toThrow('Invalid source slug…')
→ Received function did not throw
→ Received value: ".../session-credential-cache-UAvlF4/sessions/.credential-cache.json" // 逃出了 sources/
恢复 getSourcePath() 调用后又会抛错。所以“走已校验的 helper”才是真正挡住穿越的东西。✅
我还确认了 getSourcePath（在 session-tools-core/src/source-helpers.ts）会调用 assertValidSourceSlug（正则 /^[a-z0-9]+(?:-[a-z0-9]+)*$/），所以复用它（而不是重写一遍 guard）是正确的，并且让这个调用点与 getSourceConfigPath/getSourceGuidePath 保持一致。
3. “非法 slug 抛错”的设计是安全的
本 PR 有意让非法 slug 抛错（getCredentialCachePath 调用放在 try 外面），因此 getToken/hasValidCredentials 会把它向上抛而不是返回 null。我追踪了影响范围：
- 抛错流入
source-oauth.ts（ctx.credentialManager.getToken，169/256/334 行），那里没有局部catch…… - ……但顶层
CallToolRequestSchema派发用try/catch包住了每个 handler（index.ts:525），把任何抛错转成优雅的errorResponse("Tool '…' failed: Invalid source slug: …")。不会让子进程崩溃，也不会有 unhandled rejection。 - 正常流程中
source.config.slug在 source 加载时就已校验（source 走的是同一套基于getSourcePath的 helper），所以这个抛错是纵深防御的兜底，不是热路径。
结论：非法 slug 会表现为一个清晰、可诊断的工具错误，而不是被当成“没有缓存凭证”。这是一个站得住脚的选择（见下方对比）。
4. 几个值得肯定的小点
- 用单次
readFileSync+ENOENT判断（isMissingFileError）替代existsSync+readFileSync，消除了一个 TOCTOU 窗口、少一次系统调用——缺失文件保持静默，只有损坏/不可读内容才通过注入的onReadError记日志。 - 对这个切片来说测试覆盖确实全面：valid / missing / expired / empty / 损坏 JSON / 路径穿越，都有断言。
⚠️ 5. 合并提醒 —— 本 PR 与 #5914 冲突（二选一）
两个 PR 都针对 #5909，并且创建了同一个新文件 session-mcp-server/src/credential-cache.ts。真实合并测试（git merge-tree）确认了 3 处冲突：
CONFLICT (add/add): credential-cache.ts
CONFLICT (add/add): credential-cache.test.ts
CONFLICT (content): index.ts
它们互斥——谁先合，另一个就得返工。两者在理念和范围上也不同：
| 维度 | #5913（本 PR） | #5914 |
|---|---|---|
| 范围 | 仅 credential-cache 调用点 | credential-cache + validator 路径脱敏 + source-permissions RPC 诊断 + Bun 测试隔离修复 |
| 非法 slug → | 抛错 → 表现为工具错误（清晰/可诊断） | 返回 null（当成缺失凭证，安静） |
| guard | 复用 getSourcePath() |
直接调 assertValidSourceSlug() |
| cache 读取诊断 | 注入 onReadError logger；区分 ENOENT 与损坏；记录损坏 JSON |
简单 catch → null（无 logger） |
| 该文件的测试 | valid/missing/expired/empty/损坏/穿越 | valid/穿越/非法返回 null |
| issue 链接 | “Refs #5909” | “Closes #5909” |
两者都正确且安全——我分别独立验证过（#5914 的报告在那个 PR 上）。给维护者的建议：
- 本 PR（fix(desktop): validate credential cache source slugs #5913）是隔离性更好、风险更低的 credential-cache 修复，诊断更丰富、cache 读取测试覆盖更广。
- fix(desktop): harden remaining source path validation #5914 在范围上是超集并正式 close Harden remaining slug-to-path call sites and invalid slug diagnostics #5909，但它的 credential-cache 切片更薄（无 logger、不区分 ENOENT、测试更少），且选择了更安静的
null行为。 - 最干净的做法：先定下非法 slug 的理念（抛错-loud vs null-quiet），然后要么 (a) 合并本 PR 拿下 credential-cache 文件，让 fix(desktop): harden remaining source path validation #5914 rebase 掉重复部分、只保留它的 validator/RPC/测试隔离改动；要么 (b) 以 fix(desktop): harden remaining source path validation #5914 为基础，把本 PR 的
onReadError/ENOENT 诊断和额外测试并进去。
CI / 合并状态
Test (ubuntu-latest, Node 22.x) 绿；mac/win/integration 腿 skipping（named-job 占位）。mergeStateStatus: BLOCKED = REVIEW_REQUIRED（等维护者 approve），不是 CI 失败。注意 packages/desktop/ 是被 root CI 排除的独立 Bun workspace，所以这些改动不会被 root 流水线执行——上面的本地运行才是真正的证据。
结论
就其自身而言，#5913 正确、安全、测试充分，可以合并。 唯一需要决策的是 #5913 vs #5914 —— 两者不能都合。👍
|
Looks like #5914 has landed and covers the same credential-cache path. Closing this one to avoid keeping a conflicting duplicate open. Thanks for the verification. |
3 participants
What this PR does
Routes the session MCP server's credential-cache path construction through the existing validated source-directory helper, so unsafe source slugs are rejected before any credential-cache filesystem access.
It also keeps ordinary cache misses quiet while logging corrupt or otherwise unreadable cache files, and adds focused tests for the valid, missing, expired, unreadable, and traversal-slug cases.
Why it's needed
#5909 identified one remaining source-slug-to-path call site in the credential cache reader. The old path construction joined the workspace root,
sources, andsourceSlugdirectly, which left this subprocess path out of sync with the validated source helpers used elsewhere.Invalid slug failures should not look the same as "no cached credential". This PR lets slug validation fail before the cache-read catch block, while preserving the existing null-return behavior for normal cache misses and expired entries.
Reviewer Test Plan
How to verify
From
packages/desktop/packages/session-mcp-server:bun test src/credential-cache.test.ts bun run build bunx tsc -p tsconfig.json --noEmit --allowImportingTsExtensionsFrom the repository root:
Evidence (Before & After)
Before: credential-cache paths were built by directly joining
sourceSlug, so a malformed slug like../sessionscould be resolved before any slug validation.After:
../sessionsthrowsInvalid source slug: "../sessions"before filesystem access; missing cache files still returnnull; corrupt cache JSON returnsnullwith a diagnostic log. The focused Bun test covers these outcomes.Tested on
Environment (optional)
macOS 26.5.1, Bun 1.3.14.
Risk & Scope
null.Linked Issues
Refs #5909
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
这个 PR 让 session MCP server 的 credential-cache 路径构造复用已有的、带 source slug 校验的 source-directory helper，因此不安全的 source slug 会在任何 credential-cache 文件访问前被拒绝。
同时，它保留普通 cache miss 的静默行为，但会为损坏或其它不可读的 cache 文件记录诊断日志，并补充了覆盖有效 cache、缺失 cache、过期 cache、不可读 cache 和路径遍历 slug 的 focused tests。
Why it's needed
#5909 指出了 credential cache reader 里还存在一个 source-slug-to-path 调用点。旧实现直接拼接 workspace root、
sources和sourceSlug，导致这个 subprocess 路径没有和其它地方使用的已校验 source helper 保持一致。非法 slug 失败不应该和“没有缓存凭证”表现得一样。这个 PR 让 slug 校验在 cache-read catch 块之前失败，同时保留普通 cache miss 和过期 cache 返回
null的现有行为。Reviewer Test Plan
How to verify
在
packages/desktop/packages/session-mcp-server下运行：bun test src/credential-cache.test.ts bun run build bunx tsc -p tsconfig.json --noEmit --allowImportingTsExtensions在仓库根目录运行：
Evidence (Before & After)
Before：credential-cache 路径直接拼接
sourceSlug，因此像../sessions这样的异常 slug 可能在任何 slug 校验前被解析成路径。After：
../sessions会在文件系统访问前抛出Invalid source slug: "../sessions"；缺失的 cache 文件仍返回null；损坏的 cache JSON 返回null并记录诊断日志。focused Bun test 覆盖了这些结果。Tested on
Environment (optional)
macOS 26.5.1，Bun 1.3.14。
Risk & Scope
null的现有行为。Linked Issues
Refs #5909
AI Assistance Disclosure
我使用 Codex 审查改动、按现有模式做 sanity-check，并辅助发现潜在边界情况。