Repository navigation
fix: single-quote-escape remote paths in files.mjs shell commands - #17
Conversation
files.mjs's shell-based file operations (listShell, stat, mkdir, remove, move, copy, chmod, diskUsage, find) interpolated caller-supplied paths into double-quoted shell commands with no escaping, letting $, backticks, or an embedded double-quote break out of the command. Adds utils.quoteShellArg() (the standard POSIX '\'' single-quote technique) and applies it at all nine call sites. Closes #13.
cldmv-bot
Bot
added
area: core
There was a problem hiding this comment.
🟡 Changes recommended
Several non-path parameters (e.g., mode, maxDepth, type) are still interpolated into shell commands without validation/coercion, leaving command-injection risk despite the improved path quoting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the file-operations API’s shell-command construction by switching remote path/pattern interpolation in src/api/files.mjs from double-quoted strings to single-quoted, safely-escaped shell arguments, reducing the risk of shell metacharacter injection when interacting with devices via ADB.
Changes:
- Added
quoteShellArg()utility for POSIX-safe single-quote argument quoting. - Updated
files.mjsshell commands (e.g.,mkdir,rm,mv,find,stat,ls) to usequoteShellArg()for remote paths/patterns. - Expanded Vitest coverage to assert correct quoting behavior, including embedded single quotes and metacharacters.
File summaries
| File | Description |
|---|---|
| tests/utils.test.vitest.mjs | Adds unit tests for the new quoteShellArg() helper. |
| tests/files.test.vitest.mjs | Updates expectations to single-quoted commands and adds injection-oriented test cases. |
| src/api/utils.mjs | Introduces quoteShellArg() for safe shell argument quoting. |
| src/api/files.mjs | Migrates shell command construction to use quoteShellArg() for remote paths/patterns. |
Review details
Suppressed comments (1)
src/api/files.mjs:472
chmodinterpolatesmodedirectly into a shell command viamode.toString(8)with no validation/coercion. If a caller passes a non-number value, this can lead to unexpected output or shell injection. Coercemodeto a number and validate it is an integer in a safe range before building the command.
export async function chmod(socket, streamManager, remotePath, mode, recursive = false) {
const flag = recursive ? "-R" : "";
const command = `chmod ${flag} ${mode.toString(8)} ${quoteShellArg(remotePath)}`;
return await self.shell.execute(socket, streamManager, command);
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on PR #17 flagged that mkdir/chmod's mode and find's maxDepth/type were still interpolated into shell commands unvalidated - mode.toString(8) silently skips the octal conversion for a non-number, letting unvalidated input reach the shell. Adds assertValidMode() and inline validation for maxDepth/type, rejecting anything outside a safe range/whitelist instead of interpolating it. Also fixes maxDepth's truthy check, which dropped a valid maxDepth: 0, and corrects quoteShellArg's JSDoc type to match its actual String() coercion.
|
Re: the suppressed finding on Resolves #13 — closed when this ships to the default branch. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently apply safe quoting and input validation across all shell-interpolated arguments in files.mjs, and the added tests cover both security-sensitive edge cases and validation failures.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
🚀 What's Changed
💥 Breaking Changes
No breaking changes
✨ Features
No new features
🐛 Bug Fixes
📦 Dependencies
No dependency updates
🔧 Other Changes
No other changes
👥 Contributors