Fix shell injection risk via malicious QR code URL passed to adb#4
Fix shell injection risk via malicious QR code URL passed to adb#4jackfreem merged 5 commits intoblock:mainfrom
Conversation
jackfreem
force-pushed
the
jf/SanitizeURLs
branch
from
e3f7f7e to
9033d79
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate Android shell injection when opening URLs (e.g., from scanned QR codes) by validating/sanitizing the URL before passing it into adb shell am start -d.
Changes:
- Added a URL sanitizer helper intended to reject malformed/unsupported schemes and shell metacharacters.
- Wired URL sanitization into
AndroidEmulatorHelper.openUrlso only a validated URL is passed toadb. - Added user-facing error messages describing why a URL was rejected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/qrgo/Helpers/URLSanitizer.swift | Introduces sanitizeUrlForAndroidShell to validate scheme and reject dangerous characters. |
| Sources/qrgo/Helpers/AndroidEmulatorHelper.swift | Adds validation wrapper and uses the sanitized URL in the adb shell am start invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
… allowlist
- Replace broad shell-metacharacter denylist with single-quote approach:
sanitizeUrlForAndroidShell now only rejects characters that can break
out of POSIX single quotes (', \0, \r, \n), allowing legitimate URLs
with & in query strings to pass through safely
- Wrap the sanitized URL in single quotes in the adb shell invocation
so the Android /bin/sh treats & ; | etc. as literal URL data
- Extract allowedUrlSchemes constant so sanitizeUrlForAndroidShell and
validateAndSanitizeUrl share a single source of truth for the allowlist
- Update PR description to reflect actual package structure (no qrgoLib
library target; sanitizer lives alongside other helpers in qrgo target)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Pass am start as a single command string to adb shell so the Android device's /bin/sh interprets the single-quote wrapping around the URL, rather than relying on adb to preserve embedded quotes in argv - Clarify URLSanitizer doc: the function does not reject shell metacharacters like & ; | — it relies on the caller single-quoting the URL in the command string
| printError("ADB not found. Please install Android SDK or ensure ADB is in your PATH.") | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
P1: The old code passed arguments as separate array elements to adb. The new code concatenates everything into a single shell command string: "am start ... -d '\(safeUrlString)'". While the single-quoting + sanitizer approach is correct, the security now depends entirely on sanitizeUrlForAndroidShell never having a bug. Could this keep the separate-argument style and add shell-escaping just for the URL argument? Or at minimum, could a comment be added here explaining the security invariant — that the sanitizer guarantees no single-quote breakout, and that this string must always be constructed with single quotes around the URL?
| @@ -107,13 +107,34 @@ class AndroidEmulatorHelper { | |||
| return props | |||
| } | |||
There was a problem hiding this comment.
P2: When sanitizeUrlForAndroidShell returns nil, this method re-parses the URL with URL(string:) to determine which error to show. Would it be cleaner for the sanitizer to return an error enum (e.g., .malformed, .badScheme(String), .dangerousCharacters) instead of a bare nil? That would avoid duplicated parsing and keep error messages in sync with rejection reasons.
There was a problem hiding this comment.
Good call - switched.
| import Foundation | ||
|
|
||
| /// Allowed URL schemes for opening on Android devices. | ||
| let allowedUrlSchemes: Set<String> = ["http", "https", "cashme"] |
There was a problem hiding this comment.
P2: allowedUrlSchemes is a module-level let with internal access. Since it's shared between URLSanitizer.swift and AndroidEmulatorHelper.swift this works, but is it intentional that any file in the module can read this set? If this should be the canonical allowlist, consider documenting that intent or noting it's the single source of truth for scheme validation.
| printError("ADB not found. Please install Android SDK or ensure ADB is in your PATH.") | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
P3: Worth a brief inline comment noting that when adb shell receives a single argument, it passes it to sh -c on the device — which is why single-quoting the URL is the correct mitigation. This context would help future maintainers understand the security model.
- Return Result<String, URLSanitizationError> from sanitizeUrlForAndroidShell instead of String? so callers get a typed reason for rejection (.malformed, .disallowedScheme, .dangerousCharacters) without re-parsing the URL - Update validateAndSanitizeUrl to switch on the error cases directly, removing the duplicated URL(string:) re-parse for error message selection - Document allowedUrlSchemes as the single source of truth for scheme validation across the module - Add inline comment above the adb shell invocation explaining the security model: adb shell passes the string to /bin/sh -c on device, single quotes neutralize metacharacters, and the sanitizer ensures no single-quote breakout
Closes #3
Why
URLs from scanned QR codes were passed unsanitized to
adb shell am start -d <url>, allowing a malicious QR code containing shell metacharacters (e.g.;,|,$()) to execute arbitrary commands on the connected Android device.What
sanitizeUrlForAndroidShell()to Helpers/URLSanitizer.swift; validates URL scheme against an allowlist (http,https,cashme) and rejects shell meta characters after URL re-serializationAndroidEmulatorHelper.openUrlvia avalidateAndSanitizeUrlhelper before the URL is passed toadbReferences
%3B) pass safely — they appear as literal%3Bin the serialized string and are decoded as URL data by Android'sam, not as shell syntaxTests