feat(cua-driver): require explicit user consent before enabling JS from Apple Events - #1387
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughA new cross-browser JavaScript execution system is introduced with two parallel execution paths: Apple Events/AppleScript for native browser tabs (Chrome, Safari, Firefox, etc.) and Chrome DevTools Protocol for Electron apps. Documentation describes enabling "Allow JavaScript from Apple Events" across different browsers. A new MCP tool unifies both execution approaches and supports DOM querying and text extraction across browser types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PageTool
participant BrowserJS
participant OSAScript as /usr/bin/osascript
participant Browser
participant ElectronJS
participant Inspector as Inspector Port
participant CDP as CDP WebSocket
participant ElectronApp as Electron App
Client->>PageTool: execute_javascript(pid, window_id, js_code)
PageTool->>PageTool: Resolve bundleId via NSWorkspace
alt Browser-based (Chrome, Safari, Firefox)
PageTool->>BrowserJS: execute(javascript, bundleId, windowId)
BrowserJS->>BrowserJS: Enumerate CGWindows to resolve window title
BrowserJS->>BrowserJS: Generate browser-specific AppleScript
BrowserJS->>OSAScript: Execute AppleScript with JS code
OSAScript->>Browser: AppleScript → JavaScript execution
Browser->>OSAScript: Return result/stderr
OSAScript->>BrowserJS: Capture stdout/error output
BrowserJS->>PageTool: Return result or throw error
else Electron-based Application
PageTool->>ElectronJS: execute(javascript, pid)
ElectronJS->>ElectronJS: Verify Electron Framework in bundle
ElectronJS->>Inspector: Scan common CDP ports (9222, 40353)
alt Inspector not found
ElectronJS->>ElectronApp: Send SIGUSR1 to activate inspector
ElectronJS->>Inspector: Poll newly opened inspector port
end
ElectronJS->>CDP: Establish WebSocket to debuggable target
ElectronJS->>CDP: POST Runtime.evaluate (returnByValue, awaitPromise)
CDP->>ElectronApp: Execute JavaScript in context
ElectronApp->>CDP: Return evaluation result
CDP->>ElectronJS: Parse response
ElectronJS->>PageTool: Return stringified result
end
PageTool->>Client: Result or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 9
🧹 Nitpick comments (3)
libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift (3)
16-89: Tool description advertises capabilities that aren't gated.The description states "Requires 'Allow JavaScript from Apple Events' enabled — see WEB_APPS.md." but the tool itself never points at the consent flow when the prereq is missing — it just bubbles up the AppleScript error string from
BrowserJS. Once the consent gate lands (see top‑level inconsistency comment), this description should also call out theenable_javascript_apple_eventsfollow‑up so the agent knows the recovery path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift` around lines 16 - 89, The tool description for the "page" Tool (created in the static let handler -> Tool(name: "page", description: ...)) advertises that "Allow JavaScript from Apple Events" is required but does not direct the agent to the recovery flow when that prereq is missing; update the description text to explicitly mention the follow-up action `enable_javascript_apple_events` (or refer to the handler's consent flow) so callers know to invoke the consent helper when BrowserJS returns the AppleScript consent error; ensure the description string mentions `enable_javascript_apple_events` and a short hint like "run enable_javascript_apple_events to prompt consent" so the agent can discover the recovery path.
193-209: Routing order is fine, but the fall‑through error is misleading.When neither
BrowserJS.supportsnorElectronJS.isElectronmatches (e.g. a Tauri/WKWebView app, whichWEB_APPS.mdexplicitly mentions, or an Electron app whose framework directory was missed), this throwsBrowserJS.Error.unsupportedBrowser(bundleId)whose description is "Browser '…' does not support JavaScript via Apple Events. Supported: Chrome, Brave, Edge, Safari." That's wrong for, say, Slack — the correct diagnostic is "not a supported browser and not detected as Electron". Consider a dedicated error case here so the agent gets actionable feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift` around lines 193 - 209, The current executeJS function returns BrowserJS.Error.unsupportedBrowser when neither BrowserJS.supports(bundleId:) nor ElectronJS.isElectron(pid:) match, which mislabels non-Apple-Events non-Electron apps; update executeJS to throw a new, distinct error when both checks fail (e.g. BrowserJS.Error.unsupportedEnvironment or a new enum case like Execution.Error.notSupportedByBrowserOrElectron) so callers get a clear message such as "Not a supported Apple-Events browser and not detected as Electron"; modify the error thrown in executeJS and add the new error case to the BrowserJS/Error (or a shared Execution/Error) enum so the message can include bundleId and pid for diagnostics, and update any places that pattern-match on the old error if needed.
50-82: Consent‑gate action described in the PR is missing from this tool.The PR title/description introduce a
page(action=enable_javascript_apple_events, bundle_id, user_has_confirmed_enabling=true, …)action plus anenableJavaScriptAppleEvents(bundleId:)implementation that quits Chrome, patches every profile'sPreferencesJSON, and relaunches. None of that appears here:
inputSchema.action.enum(line 64) only listsexecute_javascript,get_text,query_dom.- The schema has no
bundle_idoruser_has_confirmed_enablingproperties.- The
switch actionblock (lines 112‑174) has noenable_javascript_apple_eventscase.- The tool description (lines 19‑49) advertises only "Three actions".
BrowserJS.Error.javascriptNotEnabledreturns the agent toWEB_APPS.mdrather than instructing it to ask the user and re‑invokepagewithuser_has_confirmed_enabling=true.If this PR is intended to land the consent gate as described, the tool surface, schema, error message, and consent dispatcher are all unimplemented. If the consent gate is being deferred, the PR description and
WEB_APPS.mdshould be aligned with what's actually shipping (the manual shell/Python flow).Want me to draft the missing
enable_javascript_apple_eventscase (schema entry,user_has_confirmed_enablingguard, and aCuaDriverCore.BrowserJS.enableJavaScriptAppleEvents(bundleId:)that quits the browser, walks the profilePreferencesJSON files, and relaunches)?Also applies to: 112-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift` around lines 50 - 82, The page tool is missing the consent-gate action: add "enable_javascript_apple_events" to inputSchema.action.enum, add "bundle_id" (string) and "user_has_confirmed_enabling" (boolean, default false) properties to inputSchema, and update the tool description to advertise the new action; in the action dispatch (the switch on action) add a case for "enable_javascript_apple_events" that requires user_has_confirmed_enabling==true (or returns a structured prompt flow if false) and calls CuaDriverCore.BrowserJS.enableJavaScriptAppleEvents(bundleId:), and modify BrowserJS.Error.javascriptNotEnabled to instruct the agent to ask the user and re-invoke page with user_has_confirmed_enabling=true instead of returning to WEB_APPS.md; finally implement CuaDriverCore.BrowserJS.enableJavaScriptAppleEvents(bundleId:) to quit the browser, iterate over profile Preferences JSON files and apply the required patches, and relaunch the browser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/cua-driver/Skills/cua-driver/WEB_APPS.md`:
- Around line 304-320: The sleep(5) in the Chrome relaunch recipe is too close
to the ~4s settle threshold and can produce flaky verification; update the step
that runs sleep 5 to either increase the wait to sleep 7 (or a similarly safe
margin) or add a short retry loop/note before the osascript verification that
reruns the execute javascript "1+1" check if it fails (refer to the "sleep 5"
line and the subsequent osascript block that executes javascript to locate and
change the text).
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/BrowserJS.swift`:
- Around line 189-211: The cgWindowTitle(for:) function currently returns nil
for windows with an empty title which causes callers to treat existing windows
as missing; change its behavior to return the empty String when a window title
is present but empty (i.e., replace both occurrences of "return name.isEmpty ?
nil : name" with simply "return name") so that callers like
chromiumSpec/safariSpec can perform their AppleScript empty-string matching and
fallback to the front window as intended.
- Around line 143-185: runAppleScript can hang indefinitely because
withCheckedThrowingContinuation only resumes from the process
terminationHandler; add the same bounded-deadline pattern used in SetValueTool
(10s) to abort osascript: start a deadline timer when you call proc.run(), and
if the timer fires before terminationHandler, kill the Process, read any partial
stdout/stderr, and resume the continuation with a timeout Error (use or add an
appropriate Error.timeout or Error.executionFailed("timed out")). Ensure
terminationHandler and the timer coordinate so the continuation is resumed
exactly once (invalidate/cancel the timer in terminationHandler and guard
against double-resume), and preserve existing behavior for
Error.javascriptNotEnabled and other execution failures.
- Around line 173-174: The err.contains("AppleScript is turned off") clause is
redundant and the substring approach is brittle; in the error-handling block
where you call continuation.resume(throwing:
Error.javascriptNotEnabled(appName)) (the place with the err variable), remove
the redundant clause and replace the single substring check with a per-browser
detection: parse any numeric error code from err (e.g. look for "error code 12")
and treat code 12 as Chrome’s JS-disabled case, add explicit checks for known
AppleScript numeric errors Safari may emit (e.g. -609, -1723) or, if a browser
identifier is available, branch on browserName to apply browser-specific
detection logic, and fall back to the generic substring check only as a last
resort so Error.javascriptNotEnabled(appName) is raised reliably for Chrome and
handled separately for Safari.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift`:
- Around line 80-82: pageTarget() currently returns the first port advertising a
"page" target and execute(...) (via cdpEvaluate) uses it without ensuring that
port belongs to the intended process pid, allowing cross-process JS execution;
fix this by limiting candidate ports to those owned by the target pid using
listeningPorts(pid:), i.e. either modify pageTarget(pid:) to accept a pid and
only probe ports from listeningPorts(pid:) or, after getting a port from
pageTarget(), verify that the port is present in listeningPorts(pid:) before
calling cdpEvaluate (apply the same check/adjustment for the other call sites
referenced around the 133-144 range); ensure execute(...)/cdpEvaluate only run
against ports validated as owned by pid.
- Around line 270-275: The current extractor in the ElectronJS.swift evaluate
result path treats JS booleans as NSNumber and returns "1"/"0"; update the
inner["value"] handling to detect CFBoolean first (e.g. using CFGetTypeID(value
as CFTypeRef) == CFBooleanGetTypeID() or CFBooleanGetTypeID()) and return the
string "true" or "false" accordingly before the NSNumber branch, then keep the
existing NSNumber, JSONSerialization, and fallback branches (refer to the
inner["value"] handling in the function in this file).
- Around line 90-122: The kill(pid, SIGUSR1) call's return value isn't checked
so failures (permission denied or process gone) become a misleading
inspectorNotAvailable error; update the code around the kill(pid, SIGUSR1) call
in ElectronJS.swift to check the return value, and if it returns -1 capture
errno (and optional strerror) and throw a more specific Error (or rethrow
Error.inspectorNotAvailable with appended errno/description) before entering the
polling loop; reference the existing symbols listeningPorts, isInspectorPort,
scanInspectorPorts and Error.inspectorNotAvailable when implementing this check
so the signal failure is surfaced immediately instead of timing out.
- Around line 227-251: The current use of a single ws.receive in the function
that sends the CDP payload (inside the withCheckedThrowingContinuation block)
can mis-handle out-of-order frames and hang indefinitely; modify the
implementation to (1) send suppression commands before evaluate by dispatching
"Runtime.disable" and "Page.disable" on the same WebSocket prior to sending the
evaluate payload, (2) replace the single ws.receive callback with a receive loop
that parses incoming text frames and dispatches by JSON "id" (responses) vs
"method" (events), using the existing parseCDPResult logic only for responses
that match the request id (and reject/ignore events), and (3) add timeout
protection around the receive loop (e.g., use Task.sleep with cancellation or a
withTimeout wrapper) to resume the continuation with a timeout error if no
matching response arrives; locate changes around the webSocketTask creation, the
ws.send completion handler, the ws.receive usage and parseCDPResult invocation
to implement these fixes.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift`:
- Around line 149-163: The attrJS string is built by raw-quoting attribute names
which allows unescaped quotes/backslashes to break the generated JS; change the
construction of attrJS in PageTool.swift to escape each attribute via the
existing jsonString(_:) helper (e.g., use attrs.map { jsonString($0)
}.joined(separator: ", ") and keep the "[]" fallback) so the injected attributes
are safely encoded before embedding into the js template that already uses
jsonString(selector).
---
Nitpick comments:
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift`:
- Around line 16-89: The tool description for the "page" Tool (created in the
static let handler -> Tool(name: "page", description: ...)) advertises that
"Allow JavaScript from Apple Events" is required but does not direct the agent
to the recovery flow when that prereq is missing; update the description text to
explicitly mention the follow-up action `enable_javascript_apple_events` (or
refer to the handler's consent flow) so callers know to invoke the consent
helper when BrowserJS returns the AppleScript consent error; ensure the
description string mentions `enable_javascript_apple_events` and a short hint
like "run enable_javascript_apple_events to prompt consent" so the agent can
discover the recovery path.
- Around line 193-209: The current executeJS function returns
BrowserJS.Error.unsupportedBrowser when neither BrowserJS.supports(bundleId:)
nor ElectronJS.isElectron(pid:) match, which mislabels non-Apple-Events
non-Electron apps; update executeJS to throw a new, distinct error when both
checks fail (e.g. BrowserJS.Error.unsupportedEnvironment or a new enum case like
Execution.Error.notSupportedByBrowserOrElectron) so callers get a clear message
such as "Not a supported Apple-Events browser and not detected as Electron";
modify the error thrown in executeJS and add the new error case to the
BrowserJS/Error (or a shared Execution/Error) enum so the message can include
bundleId and pid for diagnostics, and update any places that pattern-match on
the old error if needed.
- Around line 50-82: The page tool is missing the consent-gate action: add
"enable_javascript_apple_events" to inputSchema.action.enum, add "bundle_id"
(string) and "user_has_confirmed_enabling" (boolean, default false) properties
to inputSchema, and update the tool description to advertise the new action; in
the action dispatch (the switch on action) add a case for
"enable_javascript_apple_events" that requires user_has_confirmed_enabling==true
(or returns a structured prompt flow if false) and calls
CuaDriverCore.BrowserJS.enableJavaScriptAppleEvents(bundleId:), and modify
BrowserJS.Error.javascriptNotEnabled to instruct the agent to ask the user and
re-invoke page with user_has_confirmed_enabling=true instead of returning to
WEB_APPS.md; finally implement
CuaDriverCore.BrowserJS.enableJavaScriptAppleEvents(bundleId:) to quit the
browser, iterate over profile Preferences JSON files and apply the required
patches, and relaunch the browser.
🪄 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: 4a7f9895-b044-499c-8a7b-c4ca759be0b5
📒 Files selected for processing (4)
libs/cua-driver/Skills/cua-driver/WEB_APPS.mdlibs/cua-driver/Sources/CuaDriverCore/Browser/BrowserJS.swiftlibs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift
| # 3. Relaunch Chrome and wait for sync to stabilise. | ||
| # Chrome sync fires ~1-2 s after launch and may briefly pull | ||
| # an older value from the server before Chrome pushes our local | ||
| # True back. Either test before sync fires (<1 s) or after it | ||
| # settles (>4 s). Waiting exactly ~2 s lands in the race window | ||
| # and is the most likely way to see a false negative. | ||
| open -a "Google Chrome" | ||
| sleep 5 | ||
|
|
||
| # 4. Verify | ||
| osascript -e 'tell application "Google Chrome" | ||
| tell active tab of front window | ||
| execute javascript "1+1" | ||
| end tell | ||
| end tell' | ||
| # → 2 | ||
| ``` |
There was a problem hiding this comment.
Sync‑race text recommends "wait 5s" but says ~2s is the false‑negative window.
The narrative here (lines 304‑311) tells the reader that the race window is around 2 s and that they should test either before sync fires (<1 s) or after settle (>4 s), but the example uses sleep 5 immediately followed by an osascript … execute javascript "1+1" verification. That's right at the edge of the "settled" window and varies with network conditions; users hitting flaky verification will assume the write didn't take. Either bump the sleep to ~7 s or add a note that the verification may need a retry on slow networks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Skills/cua-driver/WEB_APPS.md` around lines 304 - 320, The
sleep(5) in the Chrome relaunch recipe is too close to the ~4s settle threshold
and can produce flaky verification; update the step that runs sleep 5 to either
increase the wait to sleep 7 (or a similarly safe margin) or add a short retry
loop/note before the osascript verification that reruns the execute javascript
"1+1" check if it fails (refer to the "sleep 5" line and the subsequent
osascript block that executes javascript to locate and change the text).
| private static func runAppleScript( | ||
| _ script: String, | ||
| appName: String | ||
| ) async throws -> String { | ||
| // Write to a temp file to avoid shell-quoting the script entirely. | ||
| let tmpURL = URL(fileURLWithPath: NSTemporaryDirectory()) | ||
| .appendingPathComponent(UUID().uuidString + ".applescript") | ||
| try script.write(to: tmpURL, atomically: true, encoding: .utf8) | ||
| defer { try? FileManager.default.removeItem(at: tmpURL) } | ||
|
|
||
| return try await withCheckedThrowingContinuation { continuation in | ||
| let proc = Process() | ||
| proc.executableURL = URL(fileURLWithPath: "/usr/bin/osascript") | ||
| proc.arguments = [tmpURL.path] | ||
| let stdout = Pipe() | ||
| let stderr = Pipe() | ||
| proc.standardOutput = stdout | ||
| proc.standardError = stderr | ||
| proc.terminationHandler = { p in | ||
| let out = String( | ||
| data: stdout.fileHandleForReading.readDataToEndOfFile(), | ||
| encoding: .utf8 | ||
| )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let err = String( | ||
| data: stderr.fileHandleForReading.readDataToEndOfFile(), | ||
| encoding: .utf8 | ||
| )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
|
|
||
| if p.terminationStatus == 0 { | ||
| continuation.resume(returning: out) | ||
| } else if err.contains("turned off") || err.contains("AppleScript is turned off") { | ||
| continuation.resume(throwing: Error.javascriptNotEnabled(appName)) | ||
| } else { | ||
| continuation.resume(throwing: Error.executionFailed(err.isEmpty ? out : err)) | ||
| } | ||
| } | ||
| do { | ||
| try proc.run() | ||
| } catch { | ||
| continuation.resume(throwing: Error.executionFailed(error.localizedDescription)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
No timeout on osascript — request thread can hang indefinitely.
withCheckedThrowingContinuation here only resumes from terminationHandler, so if osascript blocks (Apple Events TCC prompt, an unresponsive Safari renderer, a stuck Chromium permission dialog), the MCP tool handler is wedged forever. The codebase already addresses this exact pitfall in SetValueTool.swift (lines 245‑290) with a 10‑second polling deadline — the same pattern should apply here, especially since BrowserJS is called on every page invocation.
🔒 Suggested fix — bound osascript with a deadline (mirrors SetValueTool)
- return try await withCheckedThrowingContinuation { continuation in
- let proc = Process()
- proc.executableURL = URL(fileURLWithPath: "/usr/bin/osascript")
- proc.arguments = [tmpURL.path]
- let stdout = Pipe()
- let stderr = Pipe()
- proc.standardOutput = stdout
- proc.standardError = stderr
- proc.terminationHandler = { p in
- let out = String(
- data: stdout.fileHandleForReading.readDataToEndOfFile(),
- encoding: .utf8
- )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
- let err = String(
- data: stderr.fileHandleForReading.readDataToEndOfFile(),
- encoding: .utf8
- )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
-
- if p.terminationStatus == 0 {
- continuation.resume(returning: out)
- } else if err.contains("turned off") || err.contains("AppleScript is turned off") {
- continuation.resume(throwing: Error.javascriptNotEnabled(appName))
- } else {
- continuation.resume(throwing: Error.executionFailed(err.isEmpty ? out : err))
- }
- }
- do {
- try proc.run()
- } catch {
- continuation.resume(throwing: Error.executionFailed(error.localizedDescription))
- }
- }
+ let proc = Process()
+ proc.executableURL = URL(fileURLWithPath: "/usr/bin/osascript")
+ proc.arguments = [tmpURL.path]
+ let stdout = Pipe()
+ let stderr = Pipe()
+ proc.standardOutput = stdout
+ proc.standardError = stderr
+ do {
+ try proc.run()
+ } catch {
+ throw Error.executionFailed(error.localizedDescription)
+ }
+ let deadline = Date().addingTimeInterval(10.0)
+ while proc.isRunning && Date() < deadline {
+ try? await Task.sleep(nanoseconds: 50_000_000)
+ }
+ if proc.isRunning {
+ proc.terminate()
+ throw Error.executionFailed("osascript timed out after 10 seconds")
+ }
+ let out = (String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "")
+ .trimmingCharacters(in: .whitespacesAndNewlines)
+ let err = (String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "")
+ .trimmingCharacters(in: .whitespacesAndNewlines)
+ if proc.terminationStatus == 0 { return out }
+ if err.contains("turned off") { throw Error.javascriptNotEnabled(appName) }
+ throw Error.executionFailed(err.isEmpty ? out : err)📝 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.
| private static func runAppleScript( | |
| _ script: String, | |
| appName: String | |
| ) async throws -> String { | |
| // Write to a temp file to avoid shell-quoting the script entirely. | |
| let tmpURL = URL(fileURLWithPath: NSTemporaryDirectory()) | |
| .appendingPathComponent(UUID().uuidString + ".applescript") | |
| try script.write(to: tmpURL, atomically: true, encoding: .utf8) | |
| defer { try? FileManager.default.removeItem(at: tmpURL) } | |
| return try await withCheckedThrowingContinuation { continuation in | |
| let proc = Process() | |
| proc.executableURL = URL(fileURLWithPath: "/usr/bin/osascript") | |
| proc.arguments = [tmpURL.path] | |
| let stdout = Pipe() | |
| let stderr = Pipe() | |
| proc.standardOutput = stdout | |
| proc.standardError = stderr | |
| proc.terminationHandler = { p in | |
| let out = String( | |
| data: stdout.fileHandleForReading.readDataToEndOfFile(), | |
| encoding: .utf8 | |
| )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| let err = String( | |
| data: stderr.fileHandleForReading.readDataToEndOfFile(), | |
| encoding: .utf8 | |
| )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| if p.terminationStatus == 0 { | |
| continuation.resume(returning: out) | |
| } else if err.contains("turned off") || err.contains("AppleScript is turned off") { | |
| continuation.resume(throwing: Error.javascriptNotEnabled(appName)) | |
| } else { | |
| continuation.resume(throwing: Error.executionFailed(err.isEmpty ? out : err)) | |
| } | |
| } | |
| do { | |
| try proc.run() | |
| } catch { | |
| continuation.resume(throwing: Error.executionFailed(error.localizedDescription)) | |
| } | |
| } | |
| } | |
| private static func runAppleScript( | |
| _ script: String, | |
| appName: String | |
| ) async throws -> String { | |
| // Write to a temp file to avoid shell-quoting the script entirely. | |
| let tmpURL = URL(fileURLWithPath: NSTemporaryDirectory()) | |
| .appendingPathComponent(UUID().uuidString + ".applescript") | |
| try script.write(to: tmpURL, atomically: true, encoding: .utf8) | |
| defer { try? FileManager.default.removeItem(at: tmpURL) } | |
| let proc = Process() | |
| proc.executableURL = URL(fileURLWithPath: "/usr/bin/osascript") | |
| proc.arguments = [tmpURL.path] | |
| let stdout = Pipe() | |
| let stderr = Pipe() | |
| proc.standardOutput = stdout | |
| proc.standardError = stderr | |
| do { | |
| try proc.run() | |
| } catch { | |
| throw Error.executionFailed(error.localizedDescription) | |
| } | |
| let deadline = Date().addingTimeInterval(10.0) | |
| while proc.isRunning && Date() < deadline { | |
| try? await Task.sleep(nanoseconds: 50_000_000) | |
| } | |
| if proc.isRunning { | |
| proc.terminate() | |
| throw Error.executionFailed("osascript timed out after 10 seconds") | |
| } | |
| let out = (String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "") | |
| .trimmingCharacters(in: .whitespacesAndNewlines) | |
| let err = (String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "") | |
| .trimmingCharacters(in: .whitespacesAndNewlines) | |
| if proc.terminationStatus == 0 { return out } | |
| if err.contains("turned off") { throw Error.javascriptNotEnabled(appName) } | |
| throw Error.executionFailed(err.isEmpty ? out : err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/BrowserJS.swift` around lines
143 - 185, runAppleScript can hang indefinitely because
withCheckedThrowingContinuation only resumes from the process
terminationHandler; add the same bounded-deadline pattern used in SetValueTool
(10s) to abort osascript: start a deadline timer when you call proc.run(), and
if the timer fires before terminationHandler, kill the Process, read any partial
stdout/stderr, and resume the continuation with a timeout Error (use or add an
appropriate Error.timeout or Error.executionFailed("timed out")). Ensure
terminationHandler and the timer coordinate so the continuation is resumed
exactly once (invalidate/cancel the timer in terminationHandler and guard
against double-resume), and preserve existing behavior for
Error.javascriptNotEnabled and other execution failures.
| } else if err.contains("turned off") || err.contains("AppleScript is turned off") { | ||
| continuation.resume(throwing: Error.javascriptNotEnabled(appName)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What stderr message does osascript return when "Allow JavaScript from Apple Events" is disabled in Google Chrome and Safari, and is there a stable AppleScript error number associated with it?
💡 Result:
When "Allow JavaScript from Apple Events" is disabled in Google Chrome, osascript returns an execution error on stderr with the message: "Google Chrome got an error: Executing JavaScript through AppleScript is turned off. To turn it on, from the menu bar, go to View > Developer > Allow JavaScript from Apple Events. For more information: https://support.google.com/chrome/?p=applescript". This is prefixed with a line and column indicator, such as "78:98: execution error: " followed by the message, and ends with the error number (12). For Safari, no specific stderr message is documented in the sources when "Allow JavaScript from Apple Events" (Develop menu) is disabled. Scripts attempting "do JavaScript" fail silently or with generic AppleScript errors (e.g., connection invalid or access not allowed, like -609 or -1723), but the exact message varies by context and is not consistently reported. Developers check for the setting via defaults read or menu state rather than a unique error string. The error number 12 for Chrome is not a standard stable AppleScript/OSAScript error code from Apple's documentation (which lists codes like -1700 to -1760); it appears to be a custom error code returned by Chrome's AppleScript suite specifically for this denial. No stable universal AppleScript error number is associated across both browsers.
Citations:
- 1: https://stackoverflow.com/questions/5135609/can-applescript-access-browser-tabs-and-execute-javascript-in-them
- 2: Google Chrome: Executing JavaScript through AppleScript is no longer supported thalesmello/webcomplete.vim#3
- 3: [YouTube Music] Commands not working... raycast/extensions#25177
- 4: https://developer.apple.com/library/archive/documentation/AppleScript/Conceptual/AppleScriptLangGuide/reference/ASLR_error_codes.html
- 5: Chrome AppleScript: Misleading error when 'Allow JavaScript from Apple Events' is disabled in Chrome modelcontextprotocol/mcpb#34
The redundant clause is correct, but the error number suggestion is inaccurate and the locale-fragility concern is incomplete.
The second condition err.contains("AppleScript is turned off") is indeed redundant since it's a substring of err.contains("turned off"). More importantly, this approach is fragile: Chrome consistently returns "Executing JavaScript through AppleScript is turned off" with custom error code 12, but Safari doesn't produce this message reliably—it returns generic AppleScript errors like -609 or -1723 depending on context, making the substring check ineffective for Safari. Matching on Chrome's error number 12 is not feasible as a cross-browser solution. The original suggestion to use -10000 / -1728 family error codes is incorrect; those are standard AppleScript codes not returned by Chrome's AppleScript suite.
To handle this robustly, either (1) extract and match Chrome's error code 12 with a note that Safari handling may require a different mechanism, or (2) maintain per-browser error detection logic rather than a unified substring approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/BrowserJS.swift` around lines
173 - 174, The err.contains("AppleScript is turned off") clause is redundant and
the substring approach is brittle; in the error-handling block where you call
continuation.resume(throwing: Error.javascriptNotEnabled(appName)) (the place
with the err variable), remove the redundant clause and replace the single
substring check with a per-browser detection: parse any numeric error code from
err (e.g. look for "error code 12") and treat code 12 as Chrome’s JS-disabled
case, add explicit checks for known AppleScript numeric errors Safari may emit
(e.g. -609, -1723) or, if a browser identifier is available, branch on
browserName to apply browser-specific detection logic, and fall back to the
generic substring check only as a last resort so
Error.javascriptNotEnabled(appName) is raised reliably for Chrome and handled
separately for Safari.
| private static func cgWindowTitle(for windowId: UInt32) -> String? { | ||
| // Enumerate all on-screen windows and find the one with matching ID. | ||
| // CGWindowListCopyWindowInfo([.optionIncludingWindow], id) is unreliable | ||
| // for windows not yet in the compositing list — enumerating all is safer. | ||
| let list = CGWindowListCopyWindowInfo( | ||
| [.optionOnScreenOnly, .excludeDesktopElements], kCGNullWindowID | ||
| ) as? [[String: Any]] ?? [] | ||
| for entry in list { | ||
| guard let id = entry[kCGWindowNumber as String] as? Int, | ||
| UInt32(id) == windowId else { continue } | ||
| let name = entry[kCGWindowName as String] as? String ?? "" | ||
| return name.isEmpty ? nil : name | ||
| } | ||
| // Fall back to off-screen windows (minimized, hidden). | ||
| let all = CGWindowListCopyWindowInfo([], kCGNullWindowID) as? [[String: Any]] ?? [] | ||
| for entry in all { | ||
| guard let id = entry[kCGWindowNumber as String] as? Int, | ||
| UInt32(id) == windowId else { continue } | ||
| let name = entry[kCGWindowName as String] as? String ?? "" | ||
| return name.isEmpty ? nil : name | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Empty‑titled windows surface as windowNotFound.
return name.isEmpty ? nil : name (lines 200, 208) drops legitimately empty window titles (new‑tab page, about:blank, freshly‑opened windows whose title hasn't propagated yet). The caller then throws .windowNotFound(windowId) even though the window does exist — and the AppleScript whose name contains match for an empty string would actually match the first window (which is the desired fallback). Returning the empty string lets the existing fallback in chromiumSpec/safariSpec (if matchedWindow is missing value then set matchedWindow to front window) take over.
🔧 Proposed fix
- for entry in list {
- guard let id = entry[kCGWindowNumber as String] as? Int,
- UInt32(id) == windowId else { continue }
- let name = entry[kCGWindowName as String] as? String ?? ""
- return name.isEmpty ? nil : name
- }
+ for entry in list {
+ guard let id = entry[kCGWindowNumber as String] as? Int,
+ UInt32(id) == windowId else { continue }
+ return entry[kCGWindowName as String] as? String ?? ""
+ }
@@
- for entry in all {
- guard let id = entry[kCGWindowNumber as String] as? Int,
- UInt32(id) == windowId else { continue }
- let name = entry[kCGWindowName as String] as? String ?? ""
- return name.isEmpty ? nil : name
- }
+ for entry in all {
+ guard let id = entry[kCGWindowNumber as String] as? Int,
+ UInt32(id) == windowId else { continue }
+ return entry[kCGWindowName as String] as? String ?? ""
+ }📝 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.
| private static func cgWindowTitle(for windowId: UInt32) -> String? { | |
| // Enumerate all on-screen windows and find the one with matching ID. | |
| // CGWindowListCopyWindowInfo([.optionIncludingWindow], id) is unreliable | |
| // for windows not yet in the compositing list — enumerating all is safer. | |
| let list = CGWindowListCopyWindowInfo( | |
| [.optionOnScreenOnly, .excludeDesktopElements], kCGNullWindowID | |
| ) as? [[String: Any]] ?? [] | |
| for entry in list { | |
| guard let id = entry[kCGWindowNumber as String] as? Int, | |
| UInt32(id) == windowId else { continue } | |
| let name = entry[kCGWindowName as String] as? String ?? "" | |
| return name.isEmpty ? nil : name | |
| } | |
| // Fall back to off-screen windows (minimized, hidden). | |
| let all = CGWindowListCopyWindowInfo([], kCGNullWindowID) as? [[String: Any]] ?? [] | |
| for entry in all { | |
| guard let id = entry[kCGWindowNumber as String] as? Int, | |
| UInt32(id) == windowId else { continue } | |
| let name = entry[kCGWindowName as String] as? String ?? "" | |
| return name.isEmpty ? nil : name | |
| } | |
| return nil | |
| } | |
| private static func cgWindowTitle(for windowId: UInt32) -> String? { | |
| // Enumerate all on-screen windows and find the one with matching ID. | |
| // CGWindowListCopyWindowInfo([.optionIncludingWindow], id) is unreliable | |
| // for windows not yet in the compositing list — enumerating all is safer. | |
| let list = CGWindowListCopyWindowInfo( | |
| [.optionOnScreenOnly, .excludeDesktopElements], kCGNullWindowID | |
| ) as? [[String: Any]] ?? [] | |
| for entry in list { | |
| guard let id = entry[kCGWindowNumber as String] as? Int, | |
| UInt32(id) == windowId else { continue } | |
| return entry[kCGWindowName as String] as? String ?? "" | |
| } | |
| // Fall back to off-screen windows (minimized, hidden). | |
| let all = CGWindowListCopyWindowInfo([], kCGNullWindowID) as? [[String: Any]] ?? [] | |
| for entry in all { | |
| guard let id = entry[kCGWindowNumber as String] as? Int, | |
| UInt32(id) == windowId else { continue } | |
| return entry[kCGWindowName as String] as? String ?? "" | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/BrowserJS.swift` around lines
189 - 211, The cgWindowTitle(for:) function currently returns nil for windows
with an empty title which causes callers to treat existing windows as missing;
change its behavior to return the empty String when a window title is present
but empty (i.e., replace both occurrences of "return name.isEmpty ? nil : name"
with simply "return name") so that callers like chromiumSpec/safariSpec can
perform their AppleScript empty-string matching and fallback to the front window
as intended.
| if let pagePort = await pageTarget() { | ||
| return try await cdpEvaluate(port: pagePort, javascript: javascript) | ||
| } |
There was a problem hiding this comment.
pageTarget() ignores the target pid — JS can execute in the wrong app.
pageTarget() probes a fixed list of ports (9222, 9223, 9224, 9225, 9230) and returns the first one serving any "page" target, then execute(...) immediately uses it (line 81) without verifying the target belongs to pid. If any other Electron/Chromium process on the machine is already listening on 9222 with --remote-debugging-port (very common: a manually launched Chrome dev session, another agent, a different VS Code instance), BrowserJS.execute(...) will run the agent's JavaScript in that unrelated process. That's a silent cross‑process correctness bug and a real security/privacy concern (e.g., leaking page contents from a user's personal Chrome window).
The fix is to constrain pageTarget() (or its result) to ports owned by pid — you already have listeningPorts(pid:) for exactly this.
🔒 Sketch
- if let pagePort = await pageTarget() {
+ if let pagePort = await pageTarget(pid: pid) {
return try await cdpEvaluate(port: pagePort, javascript: javascript)
}
@@
- private static func pageTarget() async -> Int? {
- for port in [9222, 9223, 9224, 9225, 9230] {
+ private static func pageTarget(pid: Int32) async -> Int? {
+ let owned = await listeningPorts(pid: pid)
+ for port in [9222, 9223, 9224, 9225, 9230] where owned.contains(port) {
...
}
return nil
}Also applies to: 133-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift` around lines
80 - 82, pageTarget() currently returns the first port advertising a "page"
target and execute(...) (via cdpEvaluate) uses it without ensuring that port
belongs to the intended process pid, allowing cross-process JS execution; fix
this by limiting candidate ports to those owned by the target pid using
listeningPorts(pid:), i.e. either modify pageTarget(pid:) to accept a pid and
only probe ports from listeningPorts(pid:) or, after getting a port from
pageTarget(), verify that the port is present in listeningPorts(pid:) before
calling cdpEvaluate (apply the same check/adjustment for the other call sites
referenced around the 133-144 range); ensure execute(...)/cdpEvaluate only run
against ports validated as owned by pid.
| // Snapshot TCP LISTEN ports before signalling so we can diff afterwards. | ||
| let portsBefore = await listeningPorts(pid: pid) | ||
|
|
||
| // Activate the V8 inspector — same mechanism as `node --inspect`. | ||
| kill(pid, SIGUSR1) | ||
|
|
||
| // Poll up to 2 s for a new LISTEN port to appear. | ||
| var inspectorPort: Int? | ||
| for _ in 0..<10 { | ||
| try await Task.sleep(for: .milliseconds(200)) | ||
| let portsAfter = await listeningPorts(pid: pid) | ||
| let newPorts = portsAfter.subtracting(portsBefore) | ||
| for port in newPorts { | ||
| if await isInspectorPort(port) { | ||
| inspectorPort = port | ||
| break | ||
| } | ||
| } | ||
| if inspectorPort != nil { break } | ||
|
|
||
| // Fallback: scan the common Node inspector range in case lsof | ||
| // didn't resolve the pid association in time. | ||
| if let port = await scanInspectorPorts() { | ||
| inspectorPort = port | ||
| break | ||
| } | ||
| } | ||
|
|
||
| guard let port = inspectorPort else { | ||
| throw Error.inspectorNotAvailable( | ||
| "no CDP endpoint appeared on localhost within 2 s after SIGUSR1" | ||
| ) | ||
| } |
There was a problem hiding this comment.
kill(pid, SIGUSR1) return value ignored; failure shows up as a confusing inspectorNotAvailable.
If the process exited between isElectron and the signal call (race) or the calling user lacks permission, kill returns -1 and the subsequent 2‑second poll just times out with a generic “no CDP endpoint appeared” error. Cheap to surface the real cause (errno) and helps debugging dramatically.
🔧 Sketch
- // Activate the V8 inspector — same mechanism as `node --inspect`.
- kill(pid, SIGUSR1)
+ // Activate the V8 inspector — same mechanism as `node --inspect`.
+ if kill(pid, SIGUSR1) != 0 {
+ throw Error.inspectorNotAvailable("kill(SIGUSR1) failed: \(String(cString: strerror(errno)))")
+ }📝 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.
| // Snapshot TCP LISTEN ports before signalling so we can diff afterwards. | |
| let portsBefore = await listeningPorts(pid: pid) | |
| // Activate the V8 inspector — same mechanism as `node --inspect`. | |
| kill(pid, SIGUSR1) | |
| // Poll up to 2 s for a new LISTEN port to appear. | |
| var inspectorPort: Int? | |
| for _ in 0..<10 { | |
| try await Task.sleep(for: .milliseconds(200)) | |
| let portsAfter = await listeningPorts(pid: pid) | |
| let newPorts = portsAfter.subtracting(portsBefore) | |
| for port in newPorts { | |
| if await isInspectorPort(port) { | |
| inspectorPort = port | |
| break | |
| } | |
| } | |
| if inspectorPort != nil { break } | |
| // Fallback: scan the common Node inspector range in case lsof | |
| // didn't resolve the pid association in time. | |
| if let port = await scanInspectorPorts() { | |
| inspectorPort = port | |
| break | |
| } | |
| } | |
| guard let port = inspectorPort else { | |
| throw Error.inspectorNotAvailable( | |
| "no CDP endpoint appeared on localhost within 2 s after SIGUSR1" | |
| ) | |
| } | |
| // Snapshot TCP LISTEN ports before signalling so we can diff afterwards. | |
| let portsBefore = await listeningPorts(pid: pid) | |
| // Activate the V8 inspector — same mechanism as `node --inspect`. | |
| if kill(pid, SIGUSR1) != 0 { | |
| throw Error.inspectorNotAvailable("kill(SIGUSR1) failed: \(String(cString: strerror(errno)))") | |
| } | |
| // Poll up to 2 s for a new LISTEN port to appear. | |
| var inspectorPort: Int? | |
| for _ in 0..<10 { | |
| try await Task.sleep(for: .milliseconds(200)) | |
| let portsAfter = await listeningPorts(pid: pid) | |
| let newPorts = portsAfter.subtracting(portsBefore) | |
| for port in newPorts { | |
| if await isInspectorPort(port) { | |
| inspectorPort = port | |
| break | |
| } | |
| } | |
| if inspectorPort != nil { break } | |
| // Fallback: scan the common Node inspector range in case lsof | |
| // didn't resolve the pid association in time. | |
| if let port = await scanInspectorPorts() { | |
| inspectorPort = port | |
| break | |
| } | |
| } | |
| guard let port = inspectorPort else { | |
| throw Error.inspectorNotAvailable( | |
| "no CDP endpoint appeared on localhost within 2 s after SIGUSR1" | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift` around lines
90 - 122, The kill(pid, SIGUSR1) call's return value isn't checked so failures
(permission denied or process gone) become a misleading inspectorNotAvailable
error; update the code around the kill(pid, SIGUSR1) call in ElectronJS.swift to
check the return value, and if it returns -1 capture errno (and optional
strerror) and throw a more specific Error (or rethrow
Error.inspectorNotAvailable with appended errno/description) before entering the
polling loop; reference the existing symbols listeningPorts, isInspectorPort,
scanInspectorPorts and Error.inspectorNotAvailable when implementing this check
so the signal failure is surfaced immediately instead of timing out.
| return try await withCheckedThrowingContinuation { continuation in | ||
| let ws = URLSession.shared.webSocketTask(with: wsURL) | ||
| ws.resume() | ||
| ws.send(.string(payloadStr)) { sendError in | ||
| if let err = sendError { | ||
| ws.cancel() | ||
| continuation.resume(throwing: Error.cdpConnectionFailed(err.localizedDescription)) | ||
| return | ||
| } | ||
| ws.receive { result in | ||
| ws.cancel() | ||
| switch result { | ||
| case .failure(let err): | ||
| continuation.resume(throwing: Error.cdpConnectionFailed(err.localizedDescription)) | ||
| case .success(let message): | ||
| guard case .string(let str) = message else { | ||
| continuation.resume(throwing: Error.cdpConnectionFailed("binary WebSocket frame")) | ||
| return | ||
| } | ||
| continuation.resume(with: Result { try Self.parseCDPResult(str) }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Chrome DevTools Protocol: does Runtime.evaluate guarantee the response frame is the next message after the request, or can other events arrive on a fresh WebSocket before the response with the matching id?
💡 Result:
No, the Chrome DevTools Protocol (CDP) does not guarantee that the response to Runtime.evaluate is the next message after the request on a fresh WebSocket connection. Other events can arrive before the response with the matching ID. In CDP, all messages—commands, responses, and events—are sent over WebSocket as JSON objects. Responses match requests by 'id' field, while events lack 'id'. Message dispatching is asynchronous: the browser can generate and send events (e.g., console messages, network events) concurrently with processing commands like Runtime.evaluate. Authoritative sources confirm this: - aslushnikov's getting-started-with-cdp README: "Message order is important in CDP. In case of Target.setDiscoverTargets, it is (implicitly) guaranteed that all current targets will be reported before the response." This implies no general order guarantee for other commands like Runtime.evaluate; only specific cases are noted. - Chromium-discuss group thread on "Chrome devtools protocol and event order guarantees" raises the question for Runtime.evaluate promises but receives no affirmative guarantee response in visible snippets. - Multiple issues (e.g., Network events arriving out of expected order relative to commands) show events interleaving with responses. Clients must handle raw WebSocket messages, dispatching by 'id' for responses and 'method' for events, without assuming sequential responses. To implement correctly on a fresh WebSocket: send request, process all incoming messages until one with matching 'id' arrives (ignoring events), then handle it.
Citations:
- 1: https://github.com/aslushnikov/getting-started-with-cdp/blob/HEAD/README.md
- 2: https://groups.google.com/a/chromium.org/g/chromium-discuss/c/TS4EhwF5lWo
- 3: https://chromedevtools.github.io/devtools-protocol
- 4: network.requestWillBeSent triggered to late ChromeDevTools/devtools-protocol#73
- 5: [Bug]: CDP's websocket events inconsistent behavior puppeteer/puppeteer#11456
🏁 Script executed:
# First, let's read the specific file and lines mentioned to understand the full context
cat -n libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift | sed -n '200,270p'Repository: trycua/cua
Length of output: 3746
🏁 Script executed:
# Search for the parseCDPResult function definition
rg -A 20 "func parseCDPResult" libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swiftRepository: trycua/cua
Length of output: 1184
🏁 Script executed:
# Search for timeout-related code
rg -i "timeout|withTimeout|Task\." libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift | head -20Repository: trycua/cua
Length of output: 248
Handle out-of-order WebSocket messages and add timeout protection.
The single ws.receive call assumes the next frame is the Runtime.evaluate response, but Chrome DevTools Protocol does not guarantee this. On a fresh WebSocket connection, unsolicited frames (target lifecycle events, Inspector.targetReloadedAfterCrash, etc.) can arrive before the response with matching id. The code closes the socket on the first received frame. When a non-response frame arrives first, parseCDPResult silently misparsses it (returning raw JSON to the caller) because it never validates the id field.
Additionally, with awaitPromise: true, if the JavaScript promise never resolves, the ws.receive call will hang indefinitely with no timeout protection.
Recommended fixes:
- Loop on
ws.receiveand dispatch frames by theiridfield (responses) vs.methodfield (events) - Send
Runtime.disableandPage.disablebefore the evaluate request to suppress unsolicited events - Wrap the receive operation in a timeout mechanism (e.g.,
Task.sleepwith cancellation orwithTimeout)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift` around lines
227 - 251, The current use of a single ws.receive in the function that sends the
CDP payload (inside the withCheckedThrowingContinuation block) can mis-handle
out-of-order frames and hang indefinitely; modify the implementation to (1) send
suppression commands before evaluate by dispatching "Runtime.disable" and
"Page.disable" on the same WebSocket prior to sending the evaluate payload, (2)
replace the single ws.receive callback with a receive loop that parses incoming
text frames and dispatches by JSON "id" (responses) vs "method" (events), using
the existing parseCDPResult logic only for responses that match the request id
(and reject/ignore events), and (3) add timeout protection around the receive
loop (e.g., use Task.sleep with cancellation or a withTimeout wrapper) to resume
the continuation with a timeout error if no matching response arrives; locate
changes around the webSocketTask creation, the ws.send completion handler, the
ws.receive usage and parseCDPResult invocation to implement these fixes.
| if let value = inner["value"] { | ||
| if let str = value as? String { return str } | ||
| if let num = value as? NSNumber { return num.stringValue } | ||
| if let data = try? JSONSerialization.data(withJSONObject: value), | ||
| let str = String(data: data, encoding: .utf8) { return str } | ||
| return "\(value)" |
There was a problem hiding this comment.
Booleans get serialized as "1"/"0".
JSONSerialization materializes JSON booleans as NSNumber bridging from CFBoolean, so value as? NSNumber matches true/false before the JSONSerialization.data(...) branch. The consequence is that evaluate("true") returns "1" and evaluate("false") returns "0" to the agent — confusing and easy to misread as integers. Branch on CFBooleanGetTypeID() first (or use CFGetTypeID(value as CFTypeRef) == CFBooleanGetTypeID()) before falling into the NSNumber path.
🔧 Proposed fix
if let value = inner["value"] {
if let str = value as? String { return str }
+ if CFGetTypeID(value as CFTypeRef) == CFBooleanGetTypeID() {
+ return (value as? Bool) == true ? "true" : "false"
+ }
if let num = value as? NSNumber { return num.stringValue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Browser/ElectronJS.swift` around lines
270 - 275, The current extractor in the ElectronJS.swift evaluate result path
treats JS booleans as NSNumber and returns "1"/"0"; update the inner["value"]
handling to detect CFBoolean first (e.g. using CFGetTypeID(value as CFTypeRef)
== CFBooleanGetTypeID() or CFBooleanGetTypeID()) and return the string "true" or
"false" accordingly before the NSNumber branch, then keep the existing NSNumber,
JSONSerialization, and fallback branches (refer to the inner["value"] handling
in the function in this file).
| let attrJS = attrs.isEmpty | ||
| ? "[]" | ||
| : "[\(attrs.map { "\"\($0)\"" }.joined(separator: ", "))]" | ||
| let js = """ | ||
| (() => { | ||
| const attrs = \(attrJS); | ||
| return JSON.stringify( | ||
| Array.from(document.querySelectorAll(\(jsonString(selector)))).map(el => { | ||
| const obj = { tag: el.tagName.toLowerCase(), text: el.innerText?.trim() }; | ||
| for (const a of attrs) obj[a] = el.getAttribute(a); | ||
| return obj; | ||
| }) | ||
| ); | ||
| })() | ||
| """ |
There was a problem hiding this comment.
attributes aren't escaped when injected into the generated JS.
attrs.map { "\"\($0)\"" }.joined(separator: ", ") interpolates each attribute name raw into a JS array literal. Quote/backslash characters in an attribute name (an agent typo, a stray data-foo"; alert(1);//) break the surrounding JS or get directly executed in the page. You already have jsonString(_:) right below — use it.
🔧 Proposed fix
- let attrJS = attrs.isEmpty
- ? "[]"
- : "[\(attrs.map { "\"\($0)\"" }.joined(separator: ", "))]"
+ let attrJS = "[\(attrs.map(jsonString).joined(separator: ", "))]"📝 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.
| let attrJS = attrs.isEmpty | |
| ? "[]" | |
| : "[\(attrs.map { "\"\($0)\"" }.joined(separator: ", "))]" | |
| let js = """ | |
| (() => { | |
| const attrs = \(attrJS); | |
| return JSON.stringify( | |
| Array.from(document.querySelectorAll(\(jsonString(selector)))).map(el => { | |
| const obj = { tag: el.tagName.toLowerCase(), text: el.innerText?.trim() }; | |
| for (const a of attrs) obj[a] = el.getAttribute(a); | |
| return obj; | |
| }) | |
| ); | |
| })() | |
| """ | |
| let attrJS = "[\(attrs.map(jsonString).joined(separator: ", "))]" | |
| let js = """ | |
| (() => { | |
| const attrs = \(attrJS); | |
| return JSON.stringify( | |
| Array.from(document.querySelectorAll(\(jsonString(selector)))).map(el => { | |
| const obj = { tag: el.tagName.toLowerCase(), text: el.innerText?.trim() }; | |
| for (const a of attrs) obj[a] = el.getAttribute(a); | |
| return obj; | |
| }) | |
| ); | |
| })() | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/PageTool.swift` around lines
149 - 163, The attrJS string is built by raw-quoting attribute names which
allows unescaped quotes/backslashes to break the generated JS; change the
construction of attrJS in PageTool.swift to escape each attribute via the
existing jsonString(_:) helper (e.g., use attrs.map { jsonString($0)
}.joined(separator: ", ") and keep the "[]" fallback) so the injected attributes
are safely encoded before embedding into the js template that already uses
jsonString(selector).
- BrowserJS.cgWindowTitle: return "" (not nil) when window found but has empty title (new tabs, about:blank) to avoid false windowNotFound errors. AppleScript `name contains ""` matches any window and falls through to the front window fallback correctly. - PageTool.query_dom: use jsonString() helper to escape attribute names before injecting into the generated JS array literal, preventing XSS via malformed attribute names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ElectronJS pageTarget() was probing fixed ports (9222-9225, 9230) and returning the first "page" target found, regardless of which process owned the port. If another Electron/Chromium app was already listening on port 9222, JS would execute in the wrong app. Use lsof to get ports actually owned by the target pid and limit probing to those. Falls back to all candidate ports when lsof returns empty (race window before SIGUSR1 activates the inspector). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3054839 to
dbcc536
Compare
1 participant
Summary
Previously, SKILL.md documented a runbook for agents to autonomously edit Chrome's
PreferencesJSON to enableallow_javascript_apple_events. This is problematic: the flag allows any automation tool to execute JavaScript in the user's browser tabs, Chrome syncs it to Google's servers, and doing it silently violates user trust.This PR adds a programmatic consent gate:
BrowserJS.javascriptNotEnablederror now includes a structured message telling the agent to stop, present a specific consent question to the user, and only proceed withuser_has_confirmed_enabling=truepage(action=enable_javascript_apple_events)is a new tool action that quits the browser, patches all profile Preferences files, and relaunches — but refuses unlessuser_has_confirmed_enabling=trueis explicitly setWEB_APPS.mdreplaces the raw shell/Python scripts with the tool invocation and a clear "do not enable autonomously" ruleConsent flow
page(action=execute_javascript, ...)pagereturns structured error with exact question to ask userpage(action=enable_javascript_apple_events, bundle_id="com.google.Chrome", user_has_confirmed_enabling=true, ...)Test plan
swift buildpassesenable_javascript_apple_eventswithoutuser_has_confirmed_enabling=true→ erroruser_has_confirmed_enabling=true→ Chrome relaunches with flag set🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation