fix: add path traversal validation to install_app tool (CWE-22)#300
fix: add path traversal validation to install_app tool (CWE-22)#300sebastiondev wants to merge 1 commit intomobile-next:mainfrom
Conversation
…WE-22) Add validateInputPath() and validateFileExtension() checks to the mobile_install_app tool handler so that the user-supplied path parameter is constrained to the current working directory and temp directory, matching the existing validation applied to save_screenshot and start_screen_recording. Without this check, a prompt-injected AI agent could call mobile_install_app with an arbitrary filesystem path, causing installation of a malicious package from outside the expected directories. Changes: - src/utils.ts: refactor validateOutputPath into a shared validatePathAgainstAllowedRoots helper; export new validateInputPath - src/server.ts: add ALLOWED_APP_EXTENSIONS constant; call validateFileExtension and validateInputPath before robot.installApp() - test/utils.ts: new test suite covering path traversal rejection and extension validation for install_app
WalkthroughThis pull request introduces input validation for the mobile app installation feature. The 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
test/utils.ts (1)
10-52: Test coverage is Unix-centric; consider cross-platform and symlink scenarios.The test suite effectively covers common path traversal attack vectors on Unix systems. However:
Windows coverage: Tests use hardcoded Unix paths (
/etc,/usr,/Users). Consider adding platform-conditional tests for Windows paths or skipping Unix-specific tests whenprocess.platform === "win32".Symlink resolution: The shared helper
validatePathAgainstAllowedRootsincludes symlink resolution logic viaresolveWithSymlinks, but no tests verify that a symlink pointing outside allowed roots is correctly rejected.Linux home directory: Line 45 tests
/Users(macOS) but not/home(Linux).♻️ Example symlink test case
it("should reject symlinks pointing outside allowed roots", function() { if (process.platform === "win32") { this.skip(); return; } const linkPath = path.join(process.cwd(), "malicious-link.apk"); try { fs.symlinkSync("/etc/passwd", linkPath); assert.throws(() => validateInputPath(linkPath), ActionableError); } finally { try { fs.unlinkSync(linkPath); } catch {} } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils.ts` around lines 10 - 52, Add cross-platform and symlink tests for validateInputPath: skip or adapt Unix-specific tests when process.platform === "win32" (so tests asserting /etc, /usr, /Users only run on non-Windows), add an additional Linux home test using "/home/..." alongside the existing "/Users/..." case, and add a symlink test that creates a symlink in the cwd pointing to an outside path (e.g., /etc/passwd) and asserts validateInputPath rejects it; reference validateInputPath, validatePathAgainstAllowedRoots and its resolveWithSymlinks behavior to ensure the symlink case exercises resolution logic and gets cleaned up in finally blocks.src/utils.ts (1)
69-88: Unusedlabelparameter invalidatePathAgainstAllowedRoots.The
labelparameter is declared but never referenced in the function body. The error message on line 85 is generic and doesn't distinguish between "input" vs "output" validation contexts.Consider either removing the parameter or incorporating it into the error message for clearer debugging:
♻️ Proposed fix to use the label parameter
function validatePathAgainstAllowedRoots(filePath: string, label: string): void { const resolved = resolveWithSymlinks(filePath); const allowedRoots = getAllowedRoots(); const isWindows = process.platform === "win32"; const isAllowed = allowedRoots.some(root => { if (isWindows) { return isPathUnderRoot(resolved.toLowerCase(), root.toLowerCase()); } return isPathUnderRoot(resolved, root); }); if (!isAllowed) { const dir = path.dirname(resolved); throw new ActionableError( - `"${dir}" is not in the list of allowed directories. Allowed directories include the current directory and the temp directory on this host.` + `The ${label} path "${dir}" is not in the list of allowed directories. Allowed directories include the current directory and the temp directory on this host.` ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 69 - 88, The validatePathAgainstAllowedRoots function currently accepts an unused label parameter; update the function to incorporate label into the thrown ActionableError message (or remove the parameter if callers don't pass context). Specifically, keep the existing resolution logic (resolveWithSymlinks, getAllowedRoots, isPathUnderRoot) but change the error to include the label and the offending directory (for example: `ActionableError(\`${label} "${dir}" is not in the list of allowed directories...\`)`) so callers know whether this was an input or output validation; if you choose to remove the parameter, also remove it from all call sites invoking validatePathAgainstAllowedRoots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils.ts`:
- Around line 69-88: The validatePathAgainstAllowedRoots function currently
accepts an unused label parameter; update the function to incorporate label into
the thrown ActionableError message (or remove the parameter if callers don't
pass context). Specifically, keep the existing resolution logic
(resolveWithSymlinks, getAllowedRoots, isPathUnderRoot) but change the error to
include the label and the offending directory (for example:
`ActionableError(\`${label} "${dir}" is not in the list of allowed
directories...\`)`) so callers know whether this was an input or output
validation; if you choose to remove the parameter, also remove it from all call
sites invoking validatePathAgainstAllowedRoots.
In `@test/utils.ts`:
- Around line 10-52: Add cross-platform and symlink tests for validateInputPath:
skip or adapt Unix-specific tests when process.platform === "win32" (so tests
asserting /etc, /usr, /Users only run on non-Windows), add an additional Linux
home test using "/home/..." alongside the existing "/Users/..." case, and add a
symlink test that creates a symlink in the cwd pointing to an outside path
(e.g., /etc/passwd) and asserts validateInputPath rejects it; reference
validateInputPath, validatePathAgainstAllowedRoots and its resolveWithSymlinks
behavior to ensure the symlink case exercises resolution logic and gets cleaned
up in finally blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 405164c2-adec-47ca-865b-cf9b9c27ce51
📒 Files selected for processing (3)
src/server.tssrc/utils.tstest/utils.ts
1 participant
Vulnerability Summary
CWE-22: Improper Limitation of a Pathname to a Restricted Directory ("Path Traversal")
Severity: Medium
Affected tool:
mobile_install_appData Flow
mobile_install_apptool call with an attacker-controlledpathparameter (type:z.string(), no validation).pathflows directly intorobot.installApp(path).execFileSync(adbPath, ["install", "-r", path])(Android) orexecFileSync("xcrun", ["simctl", "install", uuid, path])(iOS simulator) orios("install", "--path", path)(iOS physical device)..apkor.ipafile from any location on the host filesystem is installed onto the connected mobile device.Exploit Scenario
A prompt-injected AI agent (or malicious MCP client) can sideload arbitrary app packages from anywhere on the host filesystem:
{ "tool": "mobile_install_app", "arguments": { "device": "emulator-5554", "path": "/home/victim/.cache/downloads/malware.apk" } }Or with traversal:
{ "tool": "mobile_install_app", "arguments": { "device": "emulator-5554", "path": "../../tmp/evil-sideload.apk" } }Preconditions
Fix Description
This PR adds two validations to
install_app, bringing it to parity withsave_screenshotandstart_screen_recording(which were fixed in v0.0.49, PR #296):validateFileExtension(path, ALLOWED_APP_EXTENSIONS, "install_app")— restricts to.apk,.ipa,.zip,.appvalidateInputPath(path)— new function that reuses the existingvalidatePathAgainstAllowedRoots()logic to confine paths tocwdandos.tmpdir()Rationale
validateOutputPath()was refactored to extract a sharedvalidatePathAgainstAllowedRoots(filePath, label)helper, andvalidateInputPath()was added as a thin wrapper. This avoids code duplication and keeps the security boundary consistent for both input and output paths.ALLOWED_APP_EXTENSIONSlist (.apk,.ipa,.zip,.app) covers all formats accepted byadb install,simctl install, andgo-ios install.open_urlscheme restriction (also on this branch from upstream commit a0aa689) is a separate upstream fix included in this branch's base.Changes (4 files, +105 / −2 lines)
src/server.tsvalidateFileExtension+validateInputPathcalls ininstall_apphandlersrc/utils.tsvalidatePathAgainstAllowedRootshelper; export newvalidateInputPathtest/utils.tsCHANGELOG.mdTest Results
12 new tests added in
test/utils.ts:validateInputPathtests (6):cwd/etc/passwd)../traversal attempts fromcwd/usr/homeor/UsersoutsidecwdvalidateFileExtensiontests (6):.apkfiles.ipafiles.zipfiles.apppaths.sh)Disprove Analysis
We attempted to invalidate this finding through 9 checks:
npxlocally.pathparameter flows from MCP client →z.string()→robot.installApp(path)→execFileSync(...)with zero validation.pathininstall_app. Adjacent tools (save_screenshot,start_screen_recording) DO validate since v0.0.49.f5e3229fixed path traversal in screenshot/recording.a0aa689restricted URL schemes. Both demonstrate maintainer receptiveness.install_appwas missed during the v0.0.49 path validation effort.Mitigating Factors (documented for completeness)
execFileSyncwith array args prevents shell injection — the issue is strictly path traversal, not RCEadb install/simctl installreject non-valid packages — limits impact to actual.apk/.ipafilesVerdict
CONFIRMED VALID — High confidence. The
install_apptool was simply missed when path validation was added in v0.0.49. This PR brings it to parity.Prior Art
save_screenshotandstart_screen_recordingin the same file, same codebase.open_urlschemes — same security-hardening approach.Thank you for maintaining this project and for the existing security work in v0.0.49. This PR simply extends that same protection to the
install_apptool that was missed.