Repository navigation
fix(desktop): shell-quote the target of open() and launch() - #2144
PrabalParihar wants to merge 1 commit into
Conversation
open() and launch() interpolated the file, URL, application and URI straight into the shell command they run in the sandbox. A URL with `&` in its query string was cut at the `&` (the rest ran as a separate command), and a path with spaces or quotes was split or failed to parse. Quote them with the quoting helper write() already uses (quoteString in JS, shlex.quote in Python). launch() without a URI still passes only the application.
|
We require contributors to sign our Contributor License Agreement, and we don't have @PrabalParihar on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
🦋 Changeset detectedLatest commit: 597da2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597da2d8e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // function that prints each argument it receives, one per line. | ||
| function argvSeenBy(program: string, command: string): string[] { | ||
| const stub = `${program}() { printf '%s\\n' "$@"; }; ` | ||
| return execFileSync('bash', ['-c', stub + command], { encoding: 'utf8' }) |
There was a problem hiding this comment.
Keep Bash subprocess tests out of the Cloudflare suite
The existing Cloudflare matrix leg in .github/workflows/desktop_js_tests.yml:82-91 runs the desktop suite inside workerd, and vitest.cloudflare.config.mts:75-76 includes this new file—the desktop Cloudflare config excludes only client.test.ts. Workerd cannot spawn Bash through node:child_process, and the Windows-only skip does not exclude that runtime, so these tests make the Cloudflare CI leg fail independently of the quoting behavior. Add this file to the desktop Cloudflare config's exclusions, or use Worker-compatible assertions.
Useful? React with 👍 / 👎.
open() and launch() were putting the file or URL straight into the shell command, so a link with & in the query string got cut off and a path with spaces was split into two arguments. I quoted those values the same way write() already does, in both the JS and Python SDKs.
sandbox.open()andsandbox.launch()in the desktop SDKs (JS and Python) put the file, URL, application and URI straight into the shell command they run in the sandbox, without quoting. So a URL with&in its query string gets cut at the&, and a file path with spaces gets split into several arguments.Problem
Sandbox commands run through
/bin/bash -l -c <cmd>.open()buildsxdg-open ${fileOrUrl}andlaunch()buildsgtk-launch ${application} ${uri}, so bash parses the value itself:open('https://example.com/search?q=e2b&page=2')runsxdg-open https://example.com/search?q=e2bin the background, thenpage=2as a separate command. The browser opens the wrong page.open('/home/user/Q3 report.pdf')passes/home/user/Q3andreport.pdfas two arguments.'or(fails with a bash syntax error.launch('google-chrome', 'https://example.com/?a=1&b=2')has the same&problem.Computer-use agents often pass URLs with query strings to
open(), so this comes up in normal use.Change
open()andlaunch()quote their arguments with the class's existingquoteStringhelper, whichwrite()already uses.shlex.quote(already imported asquote_stringand used bywrite()).launch()with no URI (or an empty one) still passes only the application, as before.@e2b/desktopand@e2b/desktop-python.Usage is the same; values that used to break now work:
Tests
The new tests replace
commands.runwith a recorder, then run the command the SDK built throughbash -c, withxdg-open/gtk-launchstubbed by a shell function that prints each argument it receives. They check what the program would actually get, not the exact quoting.packages/desktop-js/tests/openLaunch.test.ts(4 tests): URL with&and#, path with spaces/quotes/parentheses,launchwith a URI,launchwithout a URI. On main, 3 fail (the URL arrives ashttps://example.com/search?q=e2b, the path gives a bash syntax error); with this change all 4 pass.packages/desktop-python/tests/test_open_launch.py: the same 4 cases. 3 fail on main, all 4 pass here.vitest run tests/openLaunch.test.ts tests/client.test.ts tests/readiness.test.ts: 18 passed.pnpm typecheck,pnpm lint(oxlint) andprettier --check src testsare clean.uv run pytest tests/test_open_launch.py tests/test_client.py tests/test_readiness.py: 17 passed.ruff check,ruff format --checkandty check e2b_desktopare clean.apps,mouse,screen,test_controls) because they need anE2B_API_KEY.Notes
getApplicationWindows/getWindowTitle; fix(desktop): return an empty list when no application window matches #2121 already quotes the application name there.This change was made with AI assistance; the tests listed above were run locally before opening this PR.