Skip to content

fix(android): quote package names before they reach adb shell - #471

Merged
gmegidish merged 1 commit into
mobile-next:mainfrom
mobile-kevin:fix-android-shell-injection
Sep 30, 2026
Merged

fix(android): quote package names before they reach adb shell#471
gmegidish merged 1 commit into
mobile-next:mainfrom
mobile-kevin:fix-android-shell-injection

Conversation

@mobile-kevin

@mobile-kevin mobile-kevin commented Sep 30, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

Summary

apps launch, apps terminate, apps path and apps clear passed the package name to adb shell unquoted. adb joins everything after shell with spaces and hands the result to the device's sh, so a package name carrying shell syntax ran a second command as the shell user:

$ mobilecli apps launch --device Pixel_9_Pro 'com.mobilenext.playground; touch /sdcard/Download/x'
{"status": "ok", ...}        # and /sdcard/Download/x now exists

Changes

  • devices/android.go: new adbShellArgs / runAdbShell helper that quotes every argument with shellescape.QuoteCommand (the same quoting the fs commands already use), used by the calls that carry a caller-supplied package name: resolve-activity, set-app-locales, am start -n, am force-stop, pm path, pm dump, pm clear.
  • The same payload now fails like any unknown package (no launcher activity found for …, dataDir not found for package …) and nothing extra runs on the device.

mobilecli url has the same root cause and is fixed separately, together with the & truncation it also causes. That PR adds the same helper with identical text, so the two merge in either order.

Testing

  • New e2e tests in test/android.spec.ts ("a package name carrying a shell command"), one per subcommand: each smuggles a touch into the package name and asserts the file does not appear in /sdcard/Download. All four failed before the fix and pass after.
  • New unit test Test_adbShellArgs_keepsAPackageNameFromBecomingASecondCommand.
  • go test ./... -race passes; golangci-lint run --new-from-rev=main reports 0 issues.
  • Full Android e2e suite on an emulator (API 36): 46 passed.

Related Issues

None

Summary by CodeRabbit

  • Bug Fixes
    • Android app commands now handle package names containing shell-sensitive characters as a single argument. This prevents such names from being interpreted as additional shell commands during app launch, termination, path lookup, or app-data clearing, helping ensure these actions affect only the requested package.

@mobile-kevin
fix(android): quote package names before they reach adb shell
f0818b3 
adb joins everything after `shell` with spaces and hands the result to the
device's sh, so a package name such as 'com.example; touch /sdcard/x' ran a
second command as the shell user. apps launch, terminate, path and clear now
send their arguments pre-quoted, the way the fs commands already do.
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: mobile-next/mobilecli/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6620b89b-a3be-4b51-b07b-4d7d0e2b4e08

📥 Commits

Reviewing files that changed from the base of the PR and between 34f65ac and f0818b3.

📒 Files selected for processing (3)
  • devices/android.go
  • devices/android_apps_test.go
  • test/android.spec.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

Walkthrough

The Android device code adds shell argument quoting and uses it for app-related commands. Unit and device tests check that package names containing shell syntax do not execute an appended command.

Changes

Android shell argument quoting

Layer / File(s) Summary
Shell argument quoting helper
devices/android.go, devices/android_apps_test.go		 Adds adbShellArgs to quote command arguments and runAdbShell to execute the quoted command through adb. A unit test checks a package name containing a semicolon and another shell command.
Quoted app commands
devices/android.go, test/android.spec.ts		 Routes launcher resolution, locale configuration, app launch and termination, app path and container queries, and app-data clearing through runAdbShell. Device tests check that crafted package names do not create a marker file.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: gmegidish

Merge Risk: ⚪ Minimal · up to f0818

The change quotes package names before device-shell execution without changing normal app-command behavior. No actionable merge-blocking risk remains after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: quoting Android package names before passing them to adb shell.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@mobile-kevin mobile-kevin mentioned this pull request Sep 30, 2026
Merged
@gmegidish
gmegidish merged commit f7a2405 into mobile-next:main Sep 30, 2026
29 of 31 checks passed
@gmegidish gmegidish mentioned this pull request Oct 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mobile-kevin @gmegidish