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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent 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). (5)
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughnormalizeDirectoryPath now performs full Windows-aware path normalization (drive prefixes, backslashes, collapse separators, resolve ChangesPath Traversal Protection via Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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?
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.