fix(desktop): reject unsafe source slugs before deletion#5829
Conversation
08cb279 to
9e992e4
Compare
|
WSL validation completed with local paths redacted. Note: the committed PR diff now contains only the source deletion guard; the temporary focused test file was removed from the PR as requested. |
|
Added Windows validation with local paths redacted, then marked the PR ready for review. The PR body now lists Windows as tested and keeps the prior WSL validation as Linux coverage. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional notes (not line-specific):
- Regex inconsistency:
validators.tsuses^[a-z0-9-]+$(allows leading/trailing/consecutive hyphens) while this PR uses the stricter^[a-z0-9]+(?:-[a-z0-9]+)*$. Worth aligning to a single shared constant. - Duplicate
getSourcePathinsession-tools-core/source-helpers.tsalso lacks slug validation — pre-existing architectural issue that would benefit from consolidation. - No tests for
assertValidSourceSlugordeleteSource— a few unit tests covering valid slugs, path-traversal attempts, and edge cases would guard against regressions.
| * Delete a source from a workspace | ||
| */ | ||
| export function deleteSource(workspaceRootPath: string, sourceSlug: string): void { | ||
| assertValidSourceSlug(sourceSlug); |
There was a problem hiding this comment.
[Suggestion] Validation is applied only to deleteSource. All other slug-accepting functions (loadSourceConfig, saveSourceConfig, loadSource, sourceExists, saveSourcePermissions) pass sourceSlug to path.join() via getSourcePath() without validation. Consider moving the assertion into getSourcePath() itself so every caller is protected through a single chokepoint:
| assertValidSourceSlug(sourceSlug); | |
| export function getSourcePath(workspaceRootPath: string, sourceSlug: string): string { | |
| assertValidSourceSlug(sourceSlug); | |
| return join(getWorkspaceSourcesPath(workspaceRootPath), sourceSlug); | |
| } |
The narrow scope (delete only) is defensible since rmSync({recursive: true}) is the highest-risk operation, but this leaves read and write paths unprotected.
Also note: generateSourceSlug (~line 432) applies .replace(/^-|-$/g, '') before .substring(0, 50). A name producing a slug where position 50 lands right after a non-alphanumeric character yields a trailing hyphen after truncation (e.g., 49 as + !b → "aaa...a-"). That source directory is created on disk but deleteSource rejects it, making it undeletable. Fix: re-trim after truncation:
let slug = name
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.substring(0, 50)
.replace(/^-|-$/g, '');— qwen3.7-max via Qwen Code /review
| function assertValidSourceSlug(sourceSlug: string): void { | ||
| if (!SOURCE_SLUG_REGEX.test(sourceSlug)) { | ||
| throw new Error('Invalid source slug'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The error message 'Invalid source slug' provides no context about which slug was rejected. Including the value would help diagnose failures (especially the truncation edge case in generateSourceSlug):
| } | |
| throw new Error(`Invalid source slug: ${JSON.stringify(sourceSlug)}`); |
JSON.stringify ensures the value is visible even for empty strings or whitespace-containing inputs.
— qwen3.7-max via Qwen Code /review
|
Thanks for the careful review and the concrete suggestions. I updated the PR to address the review feedback:
Validation run locally: Appreciate the review, especially the truncation edge case callout. |
|
|
||
| const SOURCE_SLUG_REGEX = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; | ||
|
|
||
| function assertValidSourceSlug(sourceSlug: string): void { |
There was a problem hiding this comment.
[Suggestion] assertValidSourceSlug is the sole defense against a CWE-22 path traversal, but has no test coverage. No tests verify that path-traversal payloads (../sessions, ..\sessions, absolute paths) are rejected, or that valid slugs (good-source, my-source-123) are accepted. deleteSource itself has never had tests.
Without regression tests, a future change to the regex or validation logic could silently re-introduce the vulnerability. Consider adding a test file (e.g., storage.test.ts in sources/__tests__/) covering rejection of traversal payloads, acceptance of valid slugs, and no-op behavior for non-existent valid slugs.
— qwen3.7-max via Qwen Code /review
|
Thanks for the follow-up. I added the missing regression coverage you called out:
Validation run locally: Appreciate the extra test coverage callout. |
| * Get path to a source folder within a workspace | ||
| */ | ||
| export function getSourcePath(workspaceRootPath: string, sourceSlug: string): string { | ||
| assertValidSourceSlug(sourceSlug); |
There was a problem hiding this comment.
[Suggestion] Adding assertValidSourceSlug to getSourcePath means the throw now fires before the try/catch blocks in callers like loadSourceConfig (line ~67) and loadSourceGuide (line ~194). Both functions document a null-return contract ("returns null if the config doesn't exist"), but an invalid slug now produces an uncaught exception instead of null.
This also affects loadWorkspaceSources (~line 346): it iterates readdirSync entries and passes each entry.name to loadSource → getSourcePath. The old Zod regex (/^[a-z0-9-]+$/) was more permissive — it allowed leading/trailing/consecutive hyphens. Any source directory created under that regex (or by the old generateSourceSlug truncation bug that could leave trailing hyphens) will now crash the entire source-listing pipeline.
Consider either:
- Wrapping the
getSourcePathcall inside the existing try/catch inloadSourceConfig/loadSourceGuideto preserve the null-return contract, or - Adding a try/catch around
loadSourcein theloadWorkspaceSourcesloop with adebug()log to skip non-conforming directories gracefully.
— qwen3.7-max via Qwen Code /review
| import { join } from 'node:path'; | ||
| import type { SourceConfig } from './types.ts'; | ||
|
|
||
| export const SOURCE_SLUG_REGEX = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; |
There was a problem hiding this comment.
[Suggestion] SOURCE_SLUG_REGEX and assertValidSourceSlug are defined identically in both source-helpers.ts and validators.ts (shared). The duplication is architecturally motivated (session-tools-core doesn't depend on shared), but neither site references the other. A future maintainer may update one copy without realizing the twin exists, silently creating validation divergence between the two getSourcePath implementations.
Consider adding a one-line cross-reference comment at each definition site, e.g.:
// Keep in sync with shared/src/config/validators.ts — session-tools-core cannot import from shared— qwen3.7-max via Qwen Code /review
|
Thanks for the follow-up review. I updated the PR to preserve the existing null-return/listing behavior while keeping the stricter source slug guard in place. Changes in this update:
Validation run locally: Appreciate the catch on the legacy-directory/null-contract behavior. |
Maintainer verification report — local macOS build & real-filesystem testsVerdict: the security fix is correct and safe to merge. The path-traversal deletion was reproduced on macOS (the platform the PR marked " Environment: macOS (darwin, arm64) · bun 1.3.14 · node v22.22.2 · PR head 1. Path traversal reproduced & fixed on macOS (real
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
The core path traversal fix is correct — the regex is ReDoS-safe, generateSourceSlug reorder is a proper bug fix, and tests pass (8/8). However, getSourcePath now throws on invalid slugs, and several callers that assumed it never fails were not updated. These are the same class of issue already fixed for loadSourceConfig/loadSourceGuide in shared/storage.ts, but applied to functions that were missed:
[Critical] Unchanged callers that now throw unhandled:
exportSources(resource-bundle.ts:190) — passesreaddirSyncdirectory names togetSourcePathwith no try/catch. A single legacy directory name (e.g.,my-source-, valid under old/^[a-z0-9-]+$/regex) crashes the entire workspace export.validateAllPermissions(validators.ts:1506) — same readdirSync iteration pattern, crashes workspace validation.sourceExists(storage.ts:573,source-helpers.ts:52) — now throws instead of returningfalsefor invalid slugs. Callers likesource-test.ts:72use it as a boolean guard and don't expect a throw.- session-tools-core
loadSourceConfig(source-helpers.ts:70) —getSourceConfigPathis outside the try/catch. The shared version was correctly fixed in this PR but this parallel implementation was missed.
[Suggestion] Additional callers missing try/catch:
loadSourcePermissionsConfig(permissions-config.ts:473) andloadRawSourcePermissions(permissions-config.ts:510) —getSourcePermissionsPathcalled before/outside try/catch.- RPC
sources.GET_PERMISSIONShandler (server-core/src/handlers/rpc/sources.ts:100) — slug comes from RPC parameters,getSourcePermissionsPathcalled before try/catch. validateSource(validators.ts:535-593) — constructs paths viajoin(sourcesDir, slug, ...)without callingassertValidSourceSlug(defense-in-depth gap; the function is already imported in the same file).
Suggested pattern for readdirSync callers:
try {
sourcePath = getSourcePath(workspaceRootPath, slug);
} catch {
warnings.push(`Source '${slug}' has invalid slug format, skipping`);
continue;
}For boolean/null-returning callers (sourceExists, loadSourceConfig, etc.): wrap in try/catch returning the default value (false / null).
— qwen3.7-max via Qwen Code /review
| * Get the path to a source's directory | ||
| */ | ||
| export function getSourcePath(workspaceRootPath: string, sourceSlug: string): string { | ||
| assertValidSourceSlug(sourceSlug); |
There was a problem hiding this comment.
[Critical] Adding assertValidSourceSlug here means every caller of getSourcePath in this package now potentially throws. The loadSourceConfig at line 70 calls getSourceConfigPath → getSourcePath outside its try/catch (line 74), and sourceExists at line 52 has no try/catch at all. The shared storage.ts versions were correctly fixed in this PR — please apply the same pattern here:
| assertValidSourceSlug(sourceSlug); | |
| assertValidSourceSlug(sourceSlug); | |
| return join(workspaceRootPath, 'sources', sourceSlug); | |
| } |
Also wrap loadSourceConfig's getSourceConfigPath call inside the existing try/catch (matching shared/storage.ts), and add try/catch to sourceExists returning false on invalid slug.
— qwen3.7-max via Qwen Code /review
| if (source) { | ||
| sources.push(source); | ||
| } else { | ||
| debug(`[loadWorkspaceSources] Skipping invalid or unreadable source directory: ${entry.name}`); |
There was a problem hiding this comment.
[Critical] This else correctly makes loadWorkspaceSources tolerate directories whose name fails the new stricter SOURCE_SLUG_REGEX (e.g. legacy legacy-source- — the exact dir your storage-source-slug.test.ts "legacy" case creates). But getSourcePath now throws on such names, and two sibling loops that iterate the same on-disk directory names were not given this treatment:
resources/resource-bundle.ts:190exportSources(whenselection === 'all', slugs come fromreaddirSync(sourcesDir)) callsgetSourcePath(workspaceRootPath, slug)outside any try/catch. One legacy/invalid directory name now throws and aborts the entire workspace export (RPCresources.EXPORT,server-core/.../rpc/resources.ts:32, no try/catch), instead of the pre-existingwarnings.push('… skipping'); continue.config/validators.ts:1502validateAllPermissions→validateSourcePermissions(workspaceRoot, entry)→getSourcePermissionsPath→getSourcePath(no try/catch) — one invalid dir name aborts all-permissions validation.
Verified: SOURCE_SLUG_REGEX.test('legacy-source-') === false, so getSourcePath('…','legacy-source-') throws — yet loadWorkspaceSources (here) and your test treat that same directory as a tolerated, skippable legacy source. The export / validation paths are now inconsistent with it.
Fix: mirror this skip-and-warn in those loops, e.g. in exportSources:
for (const slug of slugs) {
let sourcePath: string
try {
sourcePath = getSourcePath(workspaceRootPath, slug)
} catch {
warnings.push(`Source '${slug}' has an invalid slug, skipping`)
continue
}
if (!existsSync(sourcePath)) { /* … */ }
}(The related session-tools-core loadSourceConfig/sourceExists/sourceConfigExists throws — already flagged at source-helpers.ts:31 — are the same root cause; in that path the MCP dispatcher's catch at session-mcp-server/src/index.ts:559 downgrades them to an error response rather than a crash, so they are less severe than the two cases above.)
中文
这个 else 分支让 loadWorkspaceSources 能容忍名字不满足新版更严格 SOURCE_SLUG_REGEX 的目录（例如旧的 legacy-source-，正是你 storage-source-slug.test.ts 里 legacy 用例创建的目录）。但 getSourcePath 现在对这类名字会抛异常，而另外两处遍历同样的磁盘目录名的循环没有做同样处理：
resource-bundle.ts:190的exportSources（当selection === 'all'时 slug 来自readdirSync(sourcesDir)）在 try/catch 之外调用getSourcePath。一个旧的/非法目录名现在会抛异常并中断整个 workspace 导出（RPCresources.EXPORT，rpc/resources.ts:32无 try/catch），而原行为是warnings.push('…skipping'); continue跳过。validators.ts:1502的validateAllPermissions→validateSourcePermissions→getSourcePermissionsPath→getSourcePath（无 try/catch）——一个非法目录名会中断整个权限校验。
已验证：SOURCE_SLUG_REGEX.test('legacy-source-') === false，所以 getSourcePath('…','legacy-source-') 抛异常；但此处的 loadWorkspaceSources 和你的测试都把同一个目录当作可跳过的 legacy source。导出/校验路径与之不一致。
修复：在那些循环里照搬这里的跳过+告警逻辑（见上面 exportSources 示例）。
（session-tools-core 里 loadSourceConfig/sourceExists 的抛异常——已在 source-helpers.ts:31 标注——属于同一根因；但该路径会被 session-mcp-server/src/index.ts:559 的 dispatcher catch 兜底降级为错误响应而非崩溃，所以严重性低于上面两处。）
— claude-opus-4-8 via Claude Code /qreview
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28210285720)._ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The path traversal fix is correctly implemented at the getSourcePath chokepoint, with proper try/catch wrapping for null-return callers and solid test coverage.
Note: the same CWE-22 pattern exists in skill handlers (skills.ts) and render-template.ts — worth addressing in a follow-up.
— qwen3.7-max via Qwen Code /review
|
@doudouOUC @wenshao thanks again for the detailed reviews. I have updated the PR according to your feedback. The latest update addresses the previously missed callers around invalid source slugs, including export/validation paths, boolean/null-returning helpers, session-tools-core consistency, permissions config loading, RPC permission lookup handling, and additional defense-in-depth validation where appropriate. I also re-ran the focused validation locally: Appreciate the careful callouts, especially around preserving existing null/false-return contracts and avoiding legacy source directories breaking broader workflows. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-review of 2d313bd9e (preserve invalid source slug fallbacks) — the previously-raised concerns are resolved. ✅
The getSourcePath / getSourcePermissionsPath throw is now tolerated everywhere a legacy / invalid on-disk source directory name can surface, so the stricter slug guard no longer turns those callers into hard failures:
resources/resource-bundle.tsexportSources→ try/catch → warn + skip (was: aborts the whole export)config/validators.tsvalidateAllPermissions(warn + skip),validateSourcePermissions/validateSource(clean error)session-tools-core/source-helpers.tsloadSourceConfig/sourceExists/sourceConfigExists→ return null/false (the priorsource-helpers.ts:31[Critical])- shared
sourceExists,loadSourcePermissionsConfig,loadRawSourcePermissions, and thesourcesRPC permissions handler
The headline deleteSource guard still rejects ../sessions before rmSync, and the resource-import loop already records invalid entries as failures rather than throwing. New regression tests cover the exact legacy-source- directory scenario for export, permissions validation, and the session-tools-core loaders — I ran the desktop slug suites locally (all green) and CI is passing.
LGTM. — claude-opus-4-8 via Claude Code /qreview
中文
对 2d313bd9e（preserve invalid source slug fallbacks）的复审——之前提的问题都已解决。 ✅
getSourcePath / getSourcePermissionsPath 的抛异常现在在所有可能遇到旧的/非法磁盘目录名的调用点都做了兜底，更严格的 slug 校验不再把这些调用变成硬失败：
resource-bundle.ts的exportSources→ try/catch → 告警并跳过（原先会中断整个导出）validators.ts的validateAllPermissions（告警+跳过）、validateSourcePermissions/validateSource（返回干净的错误）session-tools-core/source-helpers.ts的loadSourceConfig/sourceExists/sourceConfigExists→ 返回 null/false（即之前source-helpers.ts:31的 [Critical]）- shared 的
sourceExists、loadSourcePermissionsConfig、loadRawSourcePermissions，以及sources的 RPC 权限处理器
头号修复 deleteSource 仍会在 rmSync 之前拒绝 ../sessions；资源导入循环本就把非法条目记为 failed 而非抛异常。新增的回归测试精确覆盖了 legacy-source- 目录在导出、权限校验、session-tools-core 加载器中的场景——我在本地实跑了 desktop 的 slug 测试套件（全绿），CI 也通过。
🔍 Maintainer local verification report — PR #5829I built and tested this PR locally in isolated git worktrees (PR head TL;DR
1. Security fix verification (core)I wrote a standalone end-to-end reproduction that imports the real Baseline ( PR head (#5829, with fix) — BLOCKED: The documented RPC chain is confirmed real:
2. Test resultsEach new/changed PR test file — all green:
Full package suites:
3. The one finding: order-dependent test failure
|
| 测试文件 | 结果 |
|---|---|
sources/__tests__/storage-source-slug.test.ts |
6 pass |
config/__tests__/source-slug-validation.test.ts |
3 pass |
agent/__tests__/permissions-config-source-slug.test.ts |
1 pass |
resources/__tests__/resource-bundle.test.ts |
50 pass |
session-tools-core/src/source-helpers.test.ts |
3 pass |
全量包套件:
session-tools-core:51 pass / 0 failshared:2341 pass / 11 skip / 2 fail(连续 3 次运行结果一致)SAFE_MODE_CONFIG > should have display properties——origin/main上早已失败(基线也挂)。tests/mode-manager.test.ts:659断言displayName === 'Explore',但实际已是'Plan mode'。这是某次模式改名遗留的陈旧测试,与本 PR 无关。source storage slug validation > preserves null-return ...—— 本 PR 新增 → 见 §3。
- 基线
shared(origin/main,无 PR):2331 pass / 1 fail(只有 SAFE_MODE 那个)→ 证明本 PR 恰好只新增一个失败。
另有两个 OAuth "metadata discovery" E2E 测试偶发失败一次(我离线沙箱里 5s 网络超时);非确定性,且与本 PR 无关。
3. 唯一发现:依赖顺序的测试失败 ⚠️
storage-source-slug.test.ts 单独跑通过(6/6),但在全量 bun test 中确定性失败:
单独跑 -> 6 pass, 0 fail
+ credential-manager-renew.test.ts(先跑) -> 1 fail
失败断言(第 112 行):loadWorkspaceSources(ws) 返回 [],而非 ['valid-source']。
根因 —— 在本 PR 未触碰的文件里: sources/__tests__/credential-manager-renew.test.ts 在模块顶层、无任何还原地调用:
mock.module('../storage.ts', () => ({ loadSourceConfig: mock(() => null), ... }))bun 的 mock.module 是进程级全局、且不会自动还原。文件按字母序执行,credential-manager-renew 先于 storage-source-slug 运行;泄漏的 mock 让 loadSourceConfig → null,于是 loadWorkspaceSources → loadSource → loadSourceConfig 连合法 source 都返回 null → []。本 PR 的新测试只是第一个在该污染源之后读取 loadWorkspaceSources 的测试,从而踩中了这颗早已埋下的雷。
影响:
- 不会阻塞 CI: root
package.json的 workspaces 排除了 desktop("!packages/desktop"),test:ci跑--workspaces,且desktop-release.yml只在workflow_dispatch触发 —— 没有任何 PR workflow 会跑 desktop 的bun test。 - 但它确实是真实缺陷:任何人在本地
packages/shared跑bun test(或未来 desktop 接入 CI)都会撞上。
修复选项(任选其一):
- 最佳(根因): 让
credential-manager-renew.test.ts还原它的 module mock(如afterAll(() => mock.restore())/ 收窄作用域),不再污染后续文件。超出本 PR 当前范围,但这是正确的修法。 - PR 内的逃生通道: 改名为
storage-source-slug.isolated.ts—— desktop 测试脚本会把*.isolated.ts各自单独进程运行(项目已有此约定,如pre-tool-use-checks.isolated.ts),从而避开共享进程污染。
4. 代码审计补充(均干净)
- 两份重复的
SOURCE_SLUG_REGEX(sharedvalidators.ts与session-tools-core/source-helpers.ts)逐字节一致(/^[a-z0-9]+(?:-[a-z0-9]+)*$/)。手工"保持同步"是个轻微的未来可维护性风险,但当前正确。 getSourcePath由全函数变为偏函数(非法 slug 抛错)。我追踪了shared、session-tools-core、server-core里的全部调用方:- 读取/遍历类与返回 boolean/null 的 helper(
loadSourceConfig、loadSourceGuide、sourceExists、sourceConfigExists、loadWorkspaceSources、exportSources、validateAllPermissions、loadSource*Permissions、server-coreGET_PERMISSIONS)→ 均已包裹为返回 null/false/跳过并告警。✓ - 写入/动作类(
deleteSource、saveSourceConfig、saveSourceGuide、downloadSourceIcon、createSource)→ 非法即抛错(fail-closed,正确);createSource会先校验 config。✓ session-tools-core/handlers/source-test.ts(4 处未包裹调用,文件未被本 PR 修改)→ 全部位于入口处if (!sourceExists(...)) return errorResponse(...)守卫之后,而本 PR 已加固sourceExists对非法 slug 返回 false。无新增崩溃路径。 ✓- resource-bundle 导入循环 → 通用
catch记录失败(恶意导入的 slug 被捕获,而非穿越)。✓
- 读取/遍历类与返回 boolean/null 的 helper(
generateSourceSlug顺序调整(先截断后 trim)正确:折叠连续非字母数字使内部双连字符不可能出现,尾部 trim 现在在截断之后执行,空值回退为'source',唯一性后缀${slug}-${n}仍然合法。tsc --noEmit在shared、session-tools-core、server-core均通过。
建议
安全改动正确、有效、范围合理 —— 从安全/正确性角度 👍 可合并。唯一的小问题是 §3 的顺序依赖测试,它不阻塞 CI;建议处理一下(理想是选项 1,或在本 PR 内用选项 2),让 desktop 套件在本地运行及未来 desktop CI 中保持绿色。
Verified locally on isolated worktrees (PR head 13d320c8c vs origin/main 87da4ef33), bun 1.3.14. Reproduction imported the real storage.ts to confirm baseline-vulnerable → fixed.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
The core CWE-22 fix is correct — all 3 Critical findings from the prior review are resolved. 63/63 tests pass. No Critical findings remain.
Remaining defense-in-depth Suggestions (all on unchanged code outside the diff):
-
getCredentialCachePathbypasses slug validation —session-mcp-server/src/index.ts:95-96constructs a filesystem path using the slug directly without routing throughgetSourcePathor callingassertValidSourceSlug. This is the only slug-to-path construction that bypasses the new validation. Exploitability is low, but pre-existing configs from before this fix could still be read. -
validateAllSourcesreports legacy dirs as errors vsvalidateAllPermissionsskips with warnings —validators.ts:656iterates all subdirectories and callsvalidateSource, which returnsvalid: falsefor legacy invalid-slug directories, making workspace validation fail.validateAllPermissionsgracefully skips the same dirs with a warning. The inconsistency means a workspace with a legacy directory failsvalidateAllbut passesvalidateAllPermissions. -
config-validate.tsfallback path returns generic error —handlers/config-validate.ts:96callsgetSourceConfigPathwithout try/catch. The throw is caught by the MCP server's global handler, returning a generic error instead of a structured validation result. No test covers this path. -
deleteSourcethrow propagates to RPC handler —storage.tsdeleteSourcecallsgetSourcePathwithout try/catch, andrpc/sources.ts:55has no local try/catch either. TheWsRpcServerglobal catch handles it, but the error is generic. Other slug-accepting handlers wrap calls in try/catch. -
source-test.tsuses config.jsonslugfield for path construction —handlers/source-test.ts:317passessource.slug(from config.json, JSON-parsed but not slug-validated) togetSourceGuidePath. Defense-in-depth gap.
— qwen3.7-max via Qwen Code /review
| try { | ||
| const content = readFileSync(path, 'utf-8') | ||
| return safeJsonParse(content) | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] The catch block now also catches assertValidSourceSlug throws from getSourcePermissionsPath (line 101), but the log message 'Error reading permissions config:' frames it as an I/O error. For slug validation failures, this misdirects debugging — an oncall engineer would chase disk permissions instead of a data validation issue.
Consider distinguishing the error type:
} catch (error) {
if (error instanceof Error && error.message.startsWith('Invalid source slug')) {
log.warn('Invalid source slug for permissions:', error.message)
} else {
log.error('Error reading permissions config:', error)
}
return null
}— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
2 Critical issues found (both in files not modified by this PR but broken by the new assertValidSourceSlug guard):
[Critical] config-validate.ts:96 — fallback path throws on invalid slugs
The fallback case 'sources' in config-validate.ts (when ctx.validators is falsy) calls getSourceConfigPath(ctx.workspacePath, sourceSlug) without try/catch. getSourceConfigPath now throws on invalid slugs via assertValidSourceSlug. This unhandled throw propagates from the fallback handler. The full-validators path handles this correctly via validateSource's own try/catch.
Suggested fix: wrap the fallback getSourceConfigPath call in try/catch, consistent with the other call sites.
[Critical] source-test.ts:318 — SLUG_REGEX vs SOURCE_SLUG_REGEX mismatch
checkCompleteness uses source.slug (from loaded config) to call getSourceGuidePath. But validateSourceConfigBasic validates the slug with SLUG_REGEX (/^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$/ — allows consecutive hyphens), while getSourceGuidePath enforces SOURCE_SLUG_REGEX (/^[a-z0-9]+(?:-[a-z0-9]+)*$/ — rejects consecutive hyphens). A config with slug: "my--source" passes validateSourceConfigBasic but throws at getSourceGuidePath with no try/catch. Fix: align SLUG_REGEX with SOURCE_SLUG_REGEX, or use the already-validated sourceSlug parameter instead of source.slug.
— qwen3.7-max via Qwen Code /review
| const content = readFileSync(filePath, 'utf-8'); | ||
| const json = safeJsonParse(content); | ||
| const result = PermissionsConfigSchema.safeParse(json); | ||
| return result.success ? result.data : null; |
There was a problem hiding this comment.
[Suggestion] Missing debug log in catch block
The new catch {} returns null silently, while the sibling loadSourcePermissionsConfig (line 483) logs debug('[Permissions] Error loading source config:', error). Consider adding debug('[Permissions] Error loading raw source permissions:', error) for consistency and easier debugging.
— qwen3.7-max via Qwen Code /review
| assertValidSourceSlug(slug); | ||
| } catch (error) { | ||
| return { | ||
| valid: false, |
There was a problem hiding this comment.
[Suggestion] Unsanitized slug in validation error file field
file is set to `sources/${slug}/config.json` before assertValidSourceSlug(slug) runs. If slug is ../sessions, the file field becomes sources/../sessions/config.json in the error response. Consider using a sanitized placeholder (e.g. 'sources/<invalid>/config.json') or constructing file after the assertion passes.
— qwen3.7-max via Qwen Code /review
|
Thanks, @doudouOUC. I appreciate the careful follow-up review and the concrete hardening suggestions. Since the core CWE-22 fix in this PR has been addressed and merged, I’ll keep the remaining slug-validation, fallback-error, and diagnostics items out of this PR’s scope and track them in follow-up issue(s) with targeted PR(s). That should make each change easier to review and avoid mixing the main path-traversal fix with broader defense-in-depth cleanup. I’ll split the follow-up work into:
|
4 participants
What this PR does
This PR prevents source deletion from accepting path-like slugs. Invalid source identifiers are rejected before the delete path is resolved, so inputs like
../sessionsor..\sessionscannot move the deletion target from the workspacesourcesdirectory to a neighboring workspace directory.Change
good-source.../sessionsand..\sessionsbefore any recursive filesystem deletion is attempted.Why it's needed
This hardens the desktop source-deletion path against path traversal caused by treating a caller-provided source slug as a filesystem path segment. The affected value is
sourceSlug, which is expected to be a source identifier, but the deletion path previously accepted path-like values and then resolved them with the workspace sources directory.This is a CWE-22 style path traversal issue because
..segments in a caller-provided slug can make the resolved deletion target escape the intendedsourcesdirectory.The vulnerable path shape is:
Because path joining normalizes
..segments, a path-like slug can resolve outside the intendedsourcesdirectory. If that neighboring workspace directory exists, the delete operation can remove it recursively.A local isolated reproduction showed this behavior:
This PR is scoped to the source deletion path. It does not claim arbitrary filesystem deletion or unauthenticated remote access.
Possible call chain / impact
The relevant call chain is:
The impact is limited to callers that can invoke source deletion for a workspace. A path-like
sourceSlugcould redirect the recursive delete target fromsources/to a neighboring workspace directory such assessions. Valid source slugs continue to delete normally, and this PR does not change source creation, skill deletion, or other slug-based storage helpers.Reviewer Test Plan
How to verify
Create a temporary workspace with
sources/good-sourceand a neighboringsessions/session-1directory. Attempt to delete a source with slug../sessionsand..\sessions. The invalid slugs should be rejected, andsessions/session-1should remain intact. Then deletegood-sourceand confirm normal source deletion still works.Evidence (Before & After)
The bug can be reproduced with a crafted
sourceSlugcontaining path traversal segments:The root cause is that
sourceSlugis intended to be a name, such asgood-source, but the previous code treated it as a filesystem path segment when constructing the deletion target:Node's
path.join()normalizes the joined path, including..segments. As a result,sources/../sessionsresolves back to the neighboringsessionsdirectory, and the previous code then continued with recursive deletion. This matches the CWE-22 class of issue: external input is used to construct a path intended to stay under a restricted directory, but special path elements are not constrained correctly.After the fix, Windows and WSL validation rejected both
../sessionsand..\sessions, kept the neighboringsessions/session-1/marker.txtfile intact, and confirmedgood-sourcestill deletes normally.Tested on
Environment (optional)
Windows PowerShell 5.1 and WSL Ubuntu-22.04 with Bash 5.1.16. Local paths were redacted from validation output.
Windows validation:
WSL validation:
Risk & Scope
Linked Issues
Fixes #5834
中文说明
这个 PR 做了什么
这个 PR 防止 source 删除逻辑接受路径型 slug。删除路径解析前会先拒绝非法 source 标识符，避免
../sessions或..\sessions这类输入把删除目标从 workspace 的sources目录移动到相邻 workspace 目录。Change
good-source这类正常的小写连字符 source 标识符。../sessions和..\sessions这类路径型或非法标识符。为什么需要
这个 PR 加固了 desktop 的 source 删除路径，防止调用方传入的 source slug 被当作文件系统路径片段处理后造成路径穿越。受影响的值是
sourceSlug，它语义上应当是 source 标识符，但此前删除路径会接受路径型值，并把它和 workspace sources 目录一起解析。这属于 CWE-22 风格的路径穿越问题，因为调用方提供的 slug 中的
..片段可能让解析后的删除目标逃逸出预期的sources目录。风险路径形态如下：
由于路径拼接会归一化
..片段，路径型 slug 可能解析到预期的sources目录之外。如果该相邻 workspace 目录存在，删除操作可能递归移除它。本地隔离复现显示：
这个 PR 只覆盖 source 删除路径，不声称任意文件系统删除或未认证远程访问。
Possible call chain / impact
相关调用链如下：
影响范围限于能够对 workspace 调用 source 删除的路径。路径型
sourceSlug可能把递归删除目标从sources/重定向到相邻 workspace 目录，例如sessions。合法 source slug 仍会正常删除；本 PR 不更改 source 创建、skill 删除或其他基于 slug 的 storage helper。Reviewer Test Plan
如何验证
创建一个临时 workspace，包含
sources/good-source和相邻目录sessions/session-1。尝试用../sessions和..\sessions作为 source slug 删除 source。非法 slug 应被拒绝，且sessions/session-1应保持存在。然后删除good-source，确认正常 source 删除仍然可用。Evidence (Before & After)
可以通过精心设计的
sourceSlug复现这个 bug。核心 payload 是路径穿越片段：原因很直接：
sourceSlug本来应该是一个名字，例如good-source；但旧代码把它当作路径片段拼进删除目标：Node 的
path.join()会把路径片段拼接后归一化，也就是会解析..。所以sources/../sessions会回退到相邻的sessions目录，随后旧代码继续执行递归删除。MITRE 的 CWE-22 描述的正是这类问题：外部输入被用于构造本应限制在特定目录下的路径，但没有正确限制特殊路径元素，导致解析后的路径逃逸到目录外。修复后：Windows 和 WSL 验证拒绝了
../sessions和..\sessions，保留相邻的sessions/session-1/marker.txt文件，并确认good-source仍可正常删除。Tested on
Environment (optional)
Windows PowerShell 5.1，WSL Ubuntu-22.04 / Bash 5.1.16。验证输出中的本地路径已脱敏。
Windows 验证：
WSL 验证：
Risk & Scope
Linked Issues
Fixes #5834