Conversation
…nt prompt injection Device-controlled strings (app names, UI element labels) were interpolated into natural language sentences, making them indistinguishable from instructions to the LLM. Returning pure JSON ensures device data is treated as data, not as commands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Critical: - C1: Validate screenshot save path — resolve and check parent dir exists - C2: Validate install app path — check file exists and has valid extension - C3: Add optional MCP_TOKEN bearer auth for SSE server Medium: - M1: Add MOBILE_MCP_DISABLE_TELEMETRY=1 env var to opt out of telemetry - M2: Wrap JSON.parse calls in mobile-device.ts with try-catch + ActionableError - M3: Wrap XML parser.parse in android.ts with try-catch + ActionableError - M4: Wrap JSON.parse calls in ios.ts (IosRobot + IosManager) with try-catch - M5: Add .min(0) bounds to all x/y coordinate Zod schemas Low: - L3: Remove stack trace from fatal error log message - L4: Run npm audit fix — resolved high/medium dep vulnerabilities Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis pull request enhances error handling and validation across the mobile device automation framework. It wraps JSON parsing and XML parsing operations in try/catch blocks to throw ActionableError exceptions with descriptive messages instead of propagating raw exceptions. Additional validation is added for file paths during app installation, including extension checks and existence verification. Response formats are standardized to return JSON structures for list operations, and screen interaction coordinates are validated to ensure non-negative values. Telemetry can now be disabled via an environment variable, and screenshot operations include directory existence checks. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mobile-device.ts (1)
76-86:⚠️ Potential issue | 🟡 MinorReturning zero dimensions may cause downstream issues.
When
screenSizeis missing from the response, the function returns{ width: 0, height: 0, scale: 1.0 }. This could cause division-by-zero or incorrect calculations in methods likeswipe()andswipeFromCoordinate()that compute center positions based on screen dimensions.Consider throwing an
ActionableErrorinstead of returning invalid dimensions.🛡️ Proposed fix to fail explicitly
if (response.data.device.screenSize) { return response.data.device.screenSize; } - return { width: 0, height: 0, scale: 1.0 }; + throw new ActionableError("Device did not report screen size");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mobile-device.ts` around lines 76 - 86, The function that parses device info currently returns a zero-sized screen when response.data.device.screenSize is missing; instead, update the method that calls this.runCommand(["device", "info"]) (the code that casts to DeviceInfoResponse) to throw an ActionableError if response.data.device.screenSize is undefined or null, with a clear message indicating that screen size is missing from mobilecli output; reference the DeviceInfoResponse type, the runCommand call, and ensure callers such as swipe() and swipeFromCoordinate() no longer receive {width:0,height:0,scale:1.0} by failing fast so downstream calculations avoid divide-by-zero or invalid positioning.src/ios.ts (2)
277-290:⚠️ Potential issue | 🟡 MinorPotential unhandled exception from
getDeviceNamein the map callback.
listDevicesnow catches JSON parse failures and returns an empty array. However, at line 286,getDeviceName(device)is called within.map(), andgetDeviceNamethrows anActionableErroron parse failure (lines 252-257). This would cause the entirelistDevicescall to throw, bypassing the graceful empty-array fallback.Consider wrapping the map callback or using a filter to handle individual device failures gracefully.
🛡️ Proposed fix to handle individual device failures
const devices = json.deviceList.map(device => ({ deviceId: device, - deviceName: this.getDeviceName(device), - })); + deviceName: (() => { + try { + return this.getDeviceName(device); + } catch { + return device; // fallback to deviceId + } + })(), + })); return devices;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ios.ts` around lines 277 - 290, listDevices currently maps json.deviceList calling getDeviceName(device) which can throw an ActionableError and bubble up, defeating the graceful fallback; update the mapping to handle per-device failures by wrapping the getDeviceName call in a try/catch (inside the map callback) or using a filter+map so that when getDeviceName throws you catch the error, optionally log it, and skip that device (returning only successfully-resolved devices) so listDevices never throws due to a single bad device.
299-316:⚠️ Potential issue | 🟡 MinorSame issue:
getDeviceInfothrows but isn't caught in the map callback.Similar to
listDevices, thelistDevicesWithDetailsmethod catches JSON parse failures but the.map()at line 306 callsgetDeviceInfo(device)which throwsActionableErroron failure. This could cause the entire method to throw unexpectedly.🛡️ Proposed fix to handle individual device failures
- const devices = json.deviceList.map(device => { - const info = this.getDeviceInfo(device); - return { - deviceId: device, - deviceName: info.DeviceName, - version: info.ProductVersion, - }; - }); + const devices = json.deviceList + .map(device => { + try { + const info = this.getDeviceInfo(device); + return { + deviceId: device, + deviceName: info.DeviceName, + version: info.ProductVersion, + }; + } catch { + return null; + } + }) + .filter((d): d is NonNullable<typeof d> => d !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ios.ts` around lines 299 - 316, The map in listDevicesWithDetails currently calls getDeviceInfo(device) which can throw ActionableError and will abort the whole method; update the mapping so each device call to getDeviceInfo is wrapped in a try/catch (inside the mapping or by using a for/forEach to build the array) and on error log/skip that device and continue; ensure the returned device objects still include deviceId, deviceName, and version for successful entries and that any ActionableError from getDeviceInfo does not bubble out of listDevicesWithDetails.
🧹 Nitpick comments (1)
src/index.ts (1)
13-22: Consider using timing-safe comparison for token validation.The current string comparison
req.headers["authorization"] !== \Bearer ${token}`` is vulnerable to timing attacks. While this is a low-risk concern for most use cases, using a constant-time comparison is a security best practice for authentication tokens.Additionally, the
authorizationheader lookup is case-sensitive. HTTP headers are case-insensitive, so clients sendingAuthorization(capitalized) will work, but this relies on Express normalizing headers to lowercase.🔒 Proposed fix using timing-safe comparison
+import crypto from "node:crypto"; + const startSseServer = async (port: number) => { const app = express(); const server = createMcpServer(); const token = process.env.MCP_TOKEN; if (token) { app.use((req, res, next) => { - if (req.headers["authorization"] !== `Bearer ${token}`) { + const authHeader = req.headers["authorization"] ?? ""; + const expected = `Bearer ${token}`; + const isValid = authHeader.length === expected.length && + crypto.timingSafeEqual(Buffer.from(authHeader), Buffer.from(expected)); + if (!isValid) { res.status(401).json({ error: "Unauthorized" }); return; } next(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 13 - 22, The middleware currently compares req.headers["authorization"] directly to `Bearer ${token}` which is susceptible to timing attacks and relies on header case; update the auth check in the app.use middleware to: (1) retrieve the header case-insensitively (use req.get('authorization') or normalize req.headers['authorization']), (2) verify it starts with the "Bearer " prefix and extract the incoming token string, and (3) perform a constant-time comparison between the extracted token and process.env.MCP_TOKEN using a timing-safe API (e.g., Node's crypto.timingSafeEqual) by converting both to Buffers of equal length (handle differing lengths safely by comparing against a zero-filled buffer of the token length or falling back to a constant-time safe path) before returning 401; update references in the middleware where `token`, `MCP_TOKEN`, and the authorization header are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server.ts`:
- Around line 366-378: The current extension check in the async handler allows
files with no extension to bypass validation; update the validation around
ext/validExtensions in the anonymous async ({ device, path: appPath }) => { ...
} block so that you require a non-empty extension that is one of
validExtensions: if ext is empty or not included in validExtensions, throw the
ActionableError. Keep the existing file existence check
(fs.existsSync(resolvedAppPath)) and then proceed to getRobotFromDevice(device)
and robot.installApp(resolvedAppPath) only after the stricter extension
validation.
---
Outside diff comments:
In `@src/ios.ts`:
- Around line 277-290: listDevices currently maps json.deviceList calling
getDeviceName(device) which can throw an ActionableError and bubble up,
defeating the graceful fallback; update the mapping to handle per-device
failures by wrapping the getDeviceName call in a try/catch (inside the map
callback) or using a filter+map so that when getDeviceName throws you catch the
error, optionally log it, and skip that device (returning only
successfully-resolved devices) so listDevices never throws due to a single bad
device.
- Around line 299-316: The map in listDevicesWithDetails currently calls
getDeviceInfo(device) which can throw ActionableError and will abort the whole
method; update the mapping so each device call to getDeviceInfo is wrapped in a
try/catch (inside the mapping or by using a for/forEach to build the array) and
on error log/skip that device and continue; ensure the returned device objects
still include deviceId, deviceName, and version for successful entries and that
any ActionableError from getDeviceInfo does not bubble out of
listDevicesWithDetails.
In `@src/mobile-device.ts`:
- Around line 76-86: The function that parses device info currently returns a
zero-sized screen when response.data.device.screenSize is missing; instead,
update the method that calls this.runCommand(["device", "info"]) (the code that
casts to DeviceInfoResponse) to throw an ActionableError if
response.data.device.screenSize is undefined or null, with a clear message
indicating that screen size is missing from mobilecli output; reference the
DeviceInfoResponse type, the runCommand call, and ensure callers such as swipe()
and swipeFromCoordinate() no longer receive {width:0,height:0,scale:1.0} by
failing fast so downstream calculations avoid divide-by-zero or invalid
positioning.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 13-22: The middleware currently compares
req.headers["authorization"] directly to `Bearer ${token}` which is susceptible
to timing attacks and relies on header case; update the auth check in the
app.use middleware to: (1) retrieve the header case-insensitively (use
req.get('authorization') or normalize req.headers['authorization']), (2) verify
it starts with the "Bearer " prefix and extract the incoming token string, and
(3) perform a constant-time comparison between the extracted token and
process.env.MCP_TOKEN using a timing-safe API (e.g., Node's
crypto.timingSafeEqual) by converting both to Buffers of equal length (handle
differing lengths safely by comparing against a zero-filled buffer of the token
length or falling back to a constant-time safe path) before returning 401;
update references in the middleware where `token`, `MCP_TOKEN`, and the
authorization header are used.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/android.tssrc/index.tssrc/ios.tssrc/mobile-device.tssrc/server.ts
| async ({ device, path: appPath }) => { | ||
| const resolvedAppPath = nodePath.resolve(appPath); | ||
| const ext = nodePath.extname(resolvedAppPath).toLowerCase(); | ||
| const validExtensions = [".apk", ".ipa", ".zip", ".app"]; | ||
| if (ext !== "" && !validExtensions.includes(ext)) { | ||
| throw new ActionableError(`Invalid app file extension "${ext}". Expected .apk, .ipa, .zip, or .app`); | ||
| } | ||
| if (!fs.existsSync(resolvedAppPath)) { | ||
| throw new ActionableError(`App file not found: ${resolvedAppPath}`); | ||
| } | ||
| const robot = getRobotFromDevice(device); | ||
| await robot.installApp(path); | ||
| return `Installed app from ${path}`; | ||
| await robot.installApp(resolvedAppPath); | ||
| return `Installed app from ${nodePath.basename(resolvedAppPath)}`; |
There was a problem hiding this comment.
Extension validation can be bypassed for files without an extension.
The condition ext !== "" && !validExtensions.includes(ext) allows files without any extension to pass validation. A file like /tmp/malicious would have ext === "" and bypass the extension check entirely.
🛡️ Proposed fix to require valid extension
- if (ext !== "" && !validExtensions.includes(ext)) {
+ if (!validExtensions.includes(ext)) {
throw new ActionableError(`Invalid app file extension "${ext}". Expected .apk, .ipa, .zip, or .app`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async ({ device, path: appPath }) => { | |
| const resolvedAppPath = nodePath.resolve(appPath); | |
| const ext = nodePath.extname(resolvedAppPath).toLowerCase(); | |
| const validExtensions = [".apk", ".ipa", ".zip", ".app"]; | |
| if (ext !== "" && !validExtensions.includes(ext)) { | |
| throw new ActionableError(`Invalid app file extension "${ext}". Expected .apk, .ipa, .zip, or .app`); | |
| } | |
| if (!fs.existsSync(resolvedAppPath)) { | |
| throw new ActionableError(`App file not found: ${resolvedAppPath}`); | |
| } | |
| const robot = getRobotFromDevice(device); | |
| await robot.installApp(path); | |
| return `Installed app from ${path}`; | |
| await robot.installApp(resolvedAppPath); | |
| return `Installed app from ${nodePath.basename(resolvedAppPath)}`; | |
| async ({ device, path: appPath }) => { | |
| const resolvedAppPath = nodePath.resolve(appPath); | |
| const ext = nodePath.extname(resolvedAppPath).toLowerCase(); | |
| const validExtensions = [".apk", ".ipa", ".zip", ".app"]; | |
| if (!validExtensions.includes(ext)) { | |
| throw new ActionableError(`Invalid app file extension "${ext}". Expected .apk, .ipa, .zip, or .app`); | |
| } | |
| if (!fs.existsSync(resolvedAppPath)) { | |
| throw new ActionableError(`App file not found: ${resolvedAppPath}`); | |
| } | |
| const robot = getRobotFromDevice(device); | |
| await robot.installApp(resolvedAppPath); | |
| return `Installed app from ${nodePath.basename(resolvedAppPath)}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` around lines 366 - 378, The current extension check in the
async handler allows files with no extension to bypass validation; update the
validation around ext/validExtensions in the anonymous async ({ device, path:
appPath }) => { ... } block so that you require a non-empty extension that is
one of validExtensions: if ext is empty or not included in validExtensions,
throw the ActionableError. Keep the existing file existence check
(fs.existsSync(resolvedAppPath)) and then proceed to getRobotFromDevice(device)
and robot.installApp(resolvedAppPath) only after the stricter extension
validation.
|
@quantappm thank you so much for this pull request. It includes in itself about 3 different fixes. I'll manually check this over the weekend and merge to production. Thanks again, you rock! 🚀 |
2 participants
No description provided.