fix(shellEnv): scrub AppImage env from shell probe#1680
Open
raulpineda wants to merge 2 commits intogeneralaction:mainfrom
Open
fix(shellEnv): scrub AppImage env from shell probe#1680raulpineda wants to merge 2 commits intogeneralaction:mainfrom
raulpineda wants to merge 2 commits intogeneralaction:mainfrom
Conversation
Route the env passed to `$SHELL -ilc` through `buildExternalToolEnv()` so AppImage packaging vars (APPIMAGE, APPDIR, ARGV0, OWD, etc.) and `/tmp/.mount_*` entries in PATH / LD_LIBRARY_PATH / XDG_DATA_DIRS are stripped before the probed login shell inherits them. Without this, a Linux AppImage install leaks those vars into the login shell startup. If the user's `.zshrc` / `.bash_profile` exec's a binary by name through PATH (starship version modules, mise activation hooks, oh-my-zsh completion plugins all do some flavor of this), the exec can resolve back into the AppImage mount and re-enter Emdash's main process, which runs this probe again and spawns another shell — a fork bomb. The same remediation helper is already used by ptyManager.ts (rationale comment there introduced by generalaction#485) and appIpc.ts (generalaction#872). shellEnv.ts was the remaining call site where process.env was passed wholesale to a child shell. Adds a regression test that mocks execSync and asserts the spawned env has AppImage keys and `/tmp/.mount_*` PATH entries scrubbed. Verified by stash-and-rerun: the test fails without the fix and passes with it. Fixes generalaction#1679 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@raulpineda is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRoutes shell-probing child processes through Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MainProcess as Main\nProcess
participant EnvBuilder as buildExternalToolEnv
participant ShellProbe as Login\nShell (execSync)
participant ChildShell as Probed\nShell
User->>MainProcess: trigger shell env initialization
MainProcess->>EnvBuilder: buildExternalToolEnv(process.env)
EnvBuilder-->>MainProcess: sanitizedEnv (no APPIMAGE, no mount paths)
MainProcess->>ShellProbe: execSync("-ilc ...", { env: sanitizedEnv })
ShellProbe->>ChildShell: start with sanitized environment
ChildShell-->>ShellProbe: produce probe output (env/list)
ShellProbe-->>MainProcess: probe output
MainProcess->>MainProcess: parse and set application shell env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
src/main/utils/__tests__/shellEnv.test.ts (1)
349-351: AddXDG_DATA_DIRSassertion to fully lock the sanitization contract.
buildExternalToolEnv()also strips AppImage mount entries fromXDG_DATA_DIRS; this regression test currently checks onlyPATHandLD_LIBRARY_PATH.🧪 Suggested test extension
process.env.PATH = `/usr/local/bin:${appDir}/usr/bin:/usr/bin`; process.env.LD_LIBRARY_PATH = `${appDir}/usr/lib:/usr/local/cuda/lib64`; + process.env.XDG_DATA_DIRS = `/usr/local/share:${appDir}/usr/share:/usr/share`; @@ expect(env.LD_LIBRARY_PATH).toBeDefined(); expect(env.LD_LIBRARY_PATH).not.toContain(appDir); expect(env.LD_LIBRARY_PATH).not.toContain('/tmp/.mount_'); + expect(env.XDG_DATA_DIRS).toBeDefined(); + expect(env.XDG_DATA_DIRS).not.toContain(appDir); + expect(env.XDG_DATA_DIRS).not.toContain('/tmp/.mount_'); }Also applies to: 378-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/__tests__/shellEnv.test.ts` around lines 349 - 351, The test currently only asserts PATH and LD_LIBRARY_PATH sanitization; extend it to also assert that buildExternalToolEnv() removes AppImage mount entries from XDG_DATA_DIRS by setting process.env.XDG_DATA_DIRS to include an AppImage-style path and verifying the returned env.XDG_DATA_DIRS no longer contains that mount entry; update both occurrences of the test (around the blocks that set process.env.PATH / LD_LIBRARY_PATH at the given ranges) so they include the same XDG_DATA_DIRS setup and corresponding assertion referencing buildExternalToolEnv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/utils/__tests__/shellEnv.test.ts`:
- Around line 369-373: The test currently asserts options.env for every
mockedExecSync.mock.calls entry which fails when detectSshAuthSock() triggers a
launchctl getenv call without an env option; update the loop in shellEnv.test.ts
to first filter calls to only those executing the shell probe (e.g., where
typeof call[0] === 'string' and the command string includes the shell-probe
probe command or a known probe substring), then perform the options/env
assertions on those filtered calls (reference mockedExecSync.mock.calls and
detectSshAuthSock to locate the relevant test code).
---
Nitpick comments:
In `@src/main/utils/__tests__/shellEnv.test.ts`:
- Around line 349-351: The test currently only asserts PATH and LD_LIBRARY_PATH
sanitization; extend it to also assert that buildExternalToolEnv() removes
AppImage mount entries from XDG_DATA_DIRS by setting process.env.XDG_DATA_DIRS
to include an AppImage-style path and verifying the returned env.XDG_DATA_DIRS
no longer contains that mount entry; update both occurrences of the test (around
the blocks that set process.env.PATH / LD_LIBRARY_PATH at the given ranges) so
they include the same XDG_DATA_DIRS setup and corresponding assertion
referencing buildExternalToolEnv.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d827b82a-2a73-4024-bac2-a42dde156e2d
📒 Files selected for processing (2)
src/main/utils/__tests__/shellEnv.test.tssrc/main/utils/shellEnv.ts
The AppImage-env regression test iterated over every mockedExecSync
call and asserted `options.env` was defined. That breaks on macOS,
where detectSshAuthSock() calls `execSync('launchctl getenv ...')`
with no `env` option — the loop would trip the `expect(env).toBeDefined()`
assertion on the launchctl call before it ever reached the shell probe.
Filter the call list to entries whose command string contains `-ilc`
(the flag both getShellEnvVar and getShellLocaleVars use) so the
assertions only run against the actual probe calls. Behavior on Linux
is unchanged (launchctl branch is skipped there anyway).
Verified by reverting shellEnv.ts to HEAD~1 and re-running: the test
still fails on `expected '/home/user/emdash.AppImage' to be undefined`,
confirming the filter didn't weaken the regression guard.
Addresses CodeRabbit review on generalaction#1680.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/main/utils/shellEnv.tsspawned$SHELL -ilc '<probe>'withprocess.envpassed wholesale, leakingAPPIMAGE/APPDIR//tmp/.mount_*PATHentries into the child shell on Linux AppImage installs. Any shell-init code that exec's a binary by name throughPATH(starship version modules, mise activation hooks, oh-my-zsh completion plugins) could then re-enter the AppImage binary and fork-bomb Emdash. See [bug]: AppImage launch fork-bombs on Linux #1679 for the failure mode.buildExternalToolEnv()fromsrc/main/utils/childProcessEnv.ts— the same helper already used byptyManager.ts(introduced by Python scripts fail in Emdash worktree terminal when using virtual environments (PYTHONHOME conflict) #485) andappIpc.ts(Fix AppImage env leakage in Open In launches #872).shellEnv.tswas the remainingprocess.env-wholesale call site.execSyncand asserts the spawned env hasAPPIMAGE/APPDIR/ARGV0/OWDstripped and no/tmp/.mount_*entries left inPATH/LD_LIBRARY_PATH.Fixes
Fixes #1679
Test plan
pnpm run format— cleanpnpm run lint— 0 errorspnpm run type-check— cleanpnpm exec vitest run— 878 passing, 1 skipped, 82 filesexpected '/home/user/emdash.AppImage' to be undefined), passes with itsystemd-run --scope+zsh -ilc+ PATH-based version probe) confirmed the upstream cause and is available on requestSummary by CodeRabbit
Bug Fixes
Tests