fix(cua-driver): percent-encode non-ASCII paths in launch_app urls - #1574
fix(cua-driver): percent-encode non-ASCII paths in launch_app urls#1574hippoley wants to merge 2 commits into
Conversation
Fixes trycua#1519. launch_app crashed the daemon when urls contained CJK / Cyrillic / accented characters (e.g. /Users/me/器材控/templates/file.key). Root cause: resolveLaunchURL() used URL(string:) to detect the scheme, which returns nil for bare non-ASCII characters. The fallback URL(fileURLWithPath:) path was reached, but without .standardizedFileURL the resulting URL could carry unencoded bytes that LaunchServices rejected mid-request, disconnecting the daemon. Fix: - Extract resolveLaunchURL into CuaDriverCore/Apps/URLResolver.swift as a public function so it can be unit-tested independently. - For plain file paths: use URL(fileURLWithPath:).standardizedFileURL, which correctly percent-encodes non-ASCII before handing to LaunchServices. - For file:// URLs: strip scheme, decode existing percent-encoding, then re-encode via fileURLWithPath for a clean round-trip. - For http/https URLs: addingPercentEncoding before URL(string:) so non-ASCII query params don't return nil. - LaunchAppTool.resolveLaunchURL now delegates to the core function. Tests (URLResolverTests): - CJK absolute path (/Users/me/器材控/...) - Cyrillic path - Accented Latin path - Emoji path - ASCII path (regression guard) - Tilde expansion - file:// passthrough - file:// with percent-encoded CJK - http/https URLs - Non-ASCII in HTTPS query string - Empty string returns nil
|
@nishantpurohit04 is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR fixes daemon crashes when ChangesNon-ASCII URL Resolution for App Launching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/Sources/CuaDriverCore/Apps/URLResolver.swift`:
- Around line 31-55: The code currently only detects schemes via "://", so
inputs like "about:blank" fall through to the file-path branch; change scheme
detection to look for the first ':' before any '/' (e.g., find
raw.firstIndex(of: ":") and ensure it's before any "/" or not followed by "//")
and extract scheme = raw[..<colon].lowercased(); then handle schemes explicitly
(if scheme == "http" || "https" → percent-encode and return URL(string:), if
scheme == "file" → perform the existing fileURLWithPath flow) and for any other
detected scheme return URL(string: raw) (or percent-encode where appropriate)
instead of treating them as plain file paths; update uses of
schemeRange/scheme/pathPart/decoded to the new colon-based indices so non- "://"
schemes like "about:" are preserved.
In `@libs/cua-driver/Tests/URLResolverTests/URLResolverTests.swift`:
- Around line 91-98: Update the testAboutBlank to assert the resolved value
instead of ignoring it: call resolveLaunchURL("about:blank") and add assertions
that the returned URL is non-nil and that its scheme and/or absoluteString match
the expected "about" and "about:blank" respectively (use XCTAssertNotNil and
XCTAssertEqual on result and result?.scheme/result?.absoluteString). This will
verify resolveLaunchURL's behavior and catch regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99a6f263-609e-4615-a0dd-3f37628d551c
📒 Files selected for processing (4)
libs/cua-driver/Package.swiftlibs/cua-driver/Sources/CuaDriverCore/Apps/URLResolver.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swiftlibs/cua-driver/Tests/URLResolverTests/URLResolverTests.swift
| if let schemeRange = raw.range(of: "://") { | ||
| let scheme = raw[raw.startIndex..<schemeRange.lowerBound].lowercased() | ||
|
|
||
| if scheme == "http" || scheme == "https" { | ||
| // Percent-encode non-ASCII so URL(string:) doesn't return nil. | ||
| if let encoded = raw.addingPercentEncoding( | ||
| withAllowedCharacters: .urlQueryAllowed), | ||
| let url = URL(string: encoded) | ||
| { | ||
| return url | ||
| } | ||
| // Already encoded by caller — try as-is. | ||
| return URL(string: raw) | ||
| } | ||
|
|
||
| if scheme == "file" { | ||
| // Strip "file://" prefix, decode any existing percent-encoding, | ||
| // then re-encode via fileURLWithPath for a clean round-trip. | ||
| let pathPart = String(raw[schemeRange.upperBound...]) | ||
| let decoded = pathPart.removingPercentEncoding ?? pathPart | ||
| return URL(fileURLWithPath: decoded).standardizedFileURL | ||
| } | ||
| } | ||
|
|
||
| // Plain path (absolute or tilde-prefixed). |
There was a problem hiding this comment.
Handle non-:// schemes before file-path fallback.
At Line 31, scheme detection only matches ://. Inputs like about:blank skip URL-scheme handling and fall into the plain-path branch at Line 55, which changes them into file paths instead of preserving scheme semantics.
Suggested fix
public func resolveLaunchURL(_ raw: String) -> URL? {
guard !raw.isEmpty else { return nil }
+ // Detect generic URI schemes first (e.g. about:, mailto:, etc.).
+ if let colon = raw.firstIndex(of: ":") {
+ let schemeCandidate = String(raw[..<colon])
+ let isValidScheme = schemeCandidate.range(
+ of: #"^[A-Za-z][A-Za-z0-9+\-.]*$"#,
+ options: .regularExpression
+ ) != nil
+
+ if isValidScheme {
+ let scheme = schemeCandidate.lowercased()
+ if scheme == "http" || scheme == "https" {
+ if let encoded = raw.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed),
+ let url = URL(string: encoded) {
+ return url
+ }
+ return URL(string: raw)
+ }
+ if scheme == "file", let schemeRange = raw.range(of: "://") {
+ let pathPart = String(raw[schemeRange.upperBound...])
+ let decoded = pathPart.removingPercentEncoding ?? pathPart
+ return URL(fileURLWithPath: decoded).standardizedFileURL
+ }
+ return URL(string: raw)
+ }
+ }
+
- // Detect explicit URL schemes (http / https / file).
- if let schemeRange = raw.range(of: "://") {
+ // Backward-compatible handling for explicit URL schemes using ://
+ if let schemeRange = raw.range(of: "://") {
let scheme = raw[raw.startIndex..<schemeRange.lowerBound].lowercased()
...
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/Sources/CuaDriverCore/Apps/URLResolver.swift` around lines 31
- 55, The code currently only detects schemes via "://", so inputs like
"about:blank" fall through to the file-path branch; change scheme detection to
look for the first ':' before any '/' (e.g., find raw.firstIndex(of: ":") and
ensure it's before any "/" or not followed by "//") and extract scheme =
raw[..<colon].lowercased(); then handle schemes explicitly (if scheme == "http"
|| "https" → percent-encode and return URL(string:), if scheme == "file" →
perform the existing fileURLWithPath flow) and for any other detected scheme
return URL(string: raw) (or percent-encode where appropriate) instead of
treating them as plain file paths; update uses of
schemeRange/scheme/pathPart/decoded to the new colon-based indices so non- "://"
schemes like "about:" are preserved.
| func testAboutBlank() throws { | ||
| // about:blank is used by browsers — not a file/http/https scheme, | ||
| // so it falls through to the plain-path branch. Verify it doesn't crash. | ||
| // (NSWorkspace handles about:blank natively for browsers.) | ||
| let result = resolveLaunchURL("about:blank") | ||
| // We don't assert a specific value — just that it doesn't crash/throw. | ||
| _ = result | ||
| } |
There was a problem hiding this comment.
testAboutBlank currently can’t catch regressions.
At Line 96, no behavioral assertion is made, so this test passes even if resolveLaunchURL("about:blank") resolves incorrectly. Please assert expected scheme/value.
Suggested test assertion update
- func testAboutBlank() throws {
- // about:blank is used by browsers — not a file/http/https scheme,
- // so it falls through to the plain-path branch. Verify it doesn't crash.
- // (NSWorkspace handles about:blank natively for browsers.)
- let result = resolveLaunchURL("about:blank")
- // We don't assert a specific value — just that it doesn't crash/throw.
- _ = result
- }
+ func testAboutBlank() throws {
+ let url = try XCTUnwrap(resolveLaunchURL("about:blank"))
+ XCTAssertEqual(url.scheme, "about")
+ XCTAssertEqual(url.absoluteString, "about:blank")
+ }📝 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.
| func testAboutBlank() throws { | |
| // about:blank is used by browsers — not a file/http/https scheme, | |
| // so it falls through to the plain-path branch. Verify it doesn't crash. | |
| // (NSWorkspace handles about:blank natively for browsers.) | |
| let result = resolveLaunchURL("about:blank") | |
| // We don't assert a specific value — just that it doesn't crash/throw. | |
| _ = result | |
| } | |
| func testAboutBlank() throws { | |
| let url = try XCTUnwrap(resolveLaunchURL("about:blank")) | |
| XCTAssertEqual(url.scheme, "about") | |
| XCTAssertEqual(url.absoluteString, "about:blank") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/Tests/URLResolverTests/URLResolverTests.swift` around lines
91 - 98, Update the testAboutBlank to assert the resolved value instead of
ignoring it: call resolveLaunchURL("about:blank") and add assertions that the
returned URL is non-nil and that its scheme and/or absoluteString match the
expected "about" and "about:blank" respectively (use XCTAssertNotNil and
XCTAssertEqual on result and result?.scheme/result?.absoluteString). This will
verify resolveLaunchURL's behavior and catch regressions.
Address CodeRabbit review comments on PR trycua#1574. 1. Scheme detection now uses firstIndex(of: ':') + RFC 3986 regex instead of range(of: '://'), so single-colon schemes like about:blank, mailto:, data: are correctly passed through via URL(string:) rather than falling into the file-path branch. 2. testAboutBlank now asserts url.scheme == 'about' and url.absoluteString == 'about:blank' instead of being a no-op. Added testMailtoScheme to cover the same code path. No behaviour change for http/https/file/plain-path inputs.
|
Good catches, @coderabbitai — both fixed in the latest commit (3e61888). 1. Non- 2. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me take a quick look at the updated files to verify the fixes. 🏁 Script executed: Length of output: 7314
🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
closed as #2077 switched the rust backend to avoid the broken url(string:) behavior |
2 participants
Fixes #1519.
launch_app crashed the daemon when urls contained CJK / Cyrillic / accented characters (e.g. /Users/me/器材控/templates/file.key).
Root cause: resolveLaunchURL() used URL(string:) to detect the scheme, which returns nil for bare non-ASCII characters. The fallback URL(fileURLWithPath:) path was reached, but without .standardizedFileURL the resulting URL could carry unencoded bytes that LaunchServices rejected mid-request, disconnecting the daemon.
Fix:
Tests (URLResolverTests):
Summary by CodeRabbit
New Features
Refactor
Tests