fix(bash): normalize allowlist path segments - #307
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ThunderTr77, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesPath Traversal Protection via Normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts`:
- Around line 31-38: The normalizeDirectoryPath function incorrectly collapses
consecutive ".." in relative paths; update its ".." handling so it only pops a
previous segment when resolvedSegments is non-empty and the last resolved
segment is not another ".." (i.e., change the condition from
resolvedSegments.length > 0 to resolvedSegments.length > 0 &&
resolvedSegments[resolvedSegments.length - 1] !== '..'); otherwise (and when no
prefix/root) push the ".." segment as before. Ensure this change in
normalizeDirectoryPath (and re-run tests for isWithinAllowedDirectory which
relies on it) preserves absolute/path-with-prefix behavior while keeping
relative "../.." sequences intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e89637cf-fe80-41c8-991c-6bef5cb5b1e0
📒 Files selected for processing (2)
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Frontend Quality
- GitHub Check: CodeQL (rust)
- GitHub Check: CodeQL (javascript-typescript)
🧰 Additional context used
🪛 OpenGrep (1.22.0)
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts
[ERROR] 20-20: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (1)
apps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.ts (1)
97-110: LGTM!
| const normalizedSeparators = path.trim().replace(/\//g, '\\').replace(/\\+$/, ''); | ||
| const driveMatch = /^([a-zA-Z]:)(?:\\(.*))?$/.exec(normalizedSeparators); | ||
| const hasRoot = normalizedSeparators.startsWith('\\'); | ||
| const prefix = driveMatch?.[1]?.toLowerCase() ?? (hasRoot ? '\\' : ''); |
There was a problem hiding this comment.
Potential allowlist bypass for UNC paths: \\server\share is the root of a UNC path, but this normalizer stores the prefix as only \ and then treats both server and share as ordinary segments. That lets .. pop the share name, which does not match Windows path semantics.
For example, with allowedWorkingDirectories: ['\\server\share'], a requested working directory like \\server\other\..\share\secret is normalized here as \\server\share\secret and passes the allowlist, while Windows resolves it under \\server\other\share\secret, outside the allowed share. Could we preserve \\server\share as the UNC prefix/root when resolving .., and add a resolveCommandContext test for this case?
|
This pull request has been inactive for 21 days. Please update it or leave a note if it is still in progress. |
|
需要在 native 执行边界做真实路径校验，前端字符串归一化不够。 Canonical path validation should happen at the native execution boundary; frontend string normalization is not enough. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src-tauri/src/core/built_in_tools/bash.rs`:
- Around line 107-109: Make the native bash authorization path authoritative: in
apps/desktop/src-tauri/src/core/built_in_tools/bash.rs#L107-L109, resolve and
validate the allowlist from native-owned configuration or a validated opaque
policy identifier instead of accepting an empty or renderer-supplied list; in
apps/desktop/src/services/NativeService/types.ts#L18, remove the raw allowlist
from the public execution request; in
apps/desktop/src-tauri/src/core/built_in_tools/types.rs#L17-L18, stop
deserializing caller-provided authorization policy; and in
apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts#L162, stop
forwarding renderer-owned allowlist values as native authorization input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 296a80c3-4de9-4209-a1fe-ce18eee2b4b7
📒 Files selected for processing (7)
apps/desktop/src-tauri/src/core/built_in_tools/bash.rsapps/desktop/src-tauri/src/core/built_in_tools/types.rsapps/desktop/src/services/BuiltInToolService/tools/bash/helper.tsapps/desktop/src/services/BuiltInToolService/tools/bash/index.tsapps/desktop/src/services/NativeService/types.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/index.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Frontend Quality
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
🧰 Additional context used
🪛 OpenGrep (1.25.0)
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts
[ERROR] 20-20: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 21-21: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (3)
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts (1)
20-61: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/bash/index.test.ts (1)
131-147: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.ts (1)
124-137: LGTM!
| if allowed_directories.is_empty() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep the allowlist authoritative at the native boundary.
Line 107 accepts an empty list, but the list is supplied by the same IPC request being validated. A caller can omit it or provide ['C:\\'], bypassing the configured scope. Resolve the policy from trusted native-owned configuration (or a validated opaque policy identifier), rather than trusting a renderer-provided directory list.
apps/desktop/src-tauri/src/core/built_in_tools/bash.rs#L107-L109: validate against native-owned policy, not request data.apps/desktop/src/services/NativeService/types.ts#L18: remove the raw allowlist from the public execution request.apps/desktop/src-tauri/src/core/built_in_tools/types.rs#L17-L18: do not deserialize caller-provided authorization policy.apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts#L162: stop forwarding renderer-owned allowlist values as native authorization input.
📍 Affects 4 files
apps/desktop/src-tauri/src/core/built_in_tools/bash.rs#L107-L109(this comment)apps/desktop/src/services/NativeService/types.ts#L18-L18apps/desktop/src-tauri/src/core/built_in_tools/types.rs#L17-L18apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts#L162-L162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src-tauri/src/core/built_in_tools/bash.rs` around lines 107 -
109, Make the native bash authorization path authoritative: in
apps/desktop/src-tauri/src/core/built_in_tools/bash.rs#L107-L109, resolve and
validate the allowlist from native-owned configuration or a validated opaque
policy identifier instead of accepting an empty or renderer-supplied list; in
apps/desktop/src/services/NativeService/types.ts#L18, remove the raw allowlist
from the public execution request; in
apps/desktop/src-tauri/src/core/built_in_tools/types.rs#L17-L18, stop
deserializing caller-provided authorization policy; and in
apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts#L162, stop
forwarding renderer-owned allowlist values as native authorization input.
2 participants
Summary
.and..segments before comparing them with the allowed-directory list.Related issue or RFC
Related to https://github.com/TouchAI-org/TouchAI
AI assistance disclosure
Testing evidence
corepack pnpm --filter @touchai/desktop exec vitest run --configLoader runner tests/services/BuiltInToolService/tools/bash/helper.test.ts- failed before the fix, then passed after the fix.corepack pnpm --filter @touchai/desktop exec vitest run --configLoader runner tests/services/BuiltInToolService/tools/bash- passed, 56 tests.corepack pnpm --dir apps/desktop exec eslint src/services/BuiltInToolService/tools/bash/helper.ts tests/services/BuiltInToolService/tools/bash/helper.test.ts- passed.corepack pnpm --dir apps/desktop exec prettier --check src/services/BuiltInToolService/tools/bash/helper.ts tests/services/BuiltInToolService/tools/bash/helper.test.ts- passed.corepack pnpm --dir apps/desktop run test:typecheck- passed.pnpm test:prwas not run locally because the directpnpmshim is not available on this machine; focused checks were run throughcorepack pnpm.Risk notes
AgentService, runtime, MCP, or schema impact: none.Screenshots or recordings
Not applicable; tool logic change only.
Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof.