fix: sanitize AppImage environment before opening URLs#710
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR replaces the external Changes
Sequence DiagramsequenceDiagram
participant Caller as IDE/CLI Caller
participant DevPodOpen as devpodopen.Run()
participant IsAppImage as isAppImage()
participant Sanitize as sanitizedEnv()
participant OpenURL as openURLSanitized(url)
participant XdgOpen as xdg-open (system)
participant StdOpen as open.Run() (stdlib)
Caller->>DevPodOpen: Run(url)
DevPodOpen->>IsAppImage: isAppImage()?
alt Running as AppImage
IsAppImage-->>DevPodOpen: true
DevPodOpen->>OpenURL: openURLSanitized(url)
OpenURL->>Sanitize: sanitizedEnv()
Sanitize-->>OpenURL: filtered env (no APPIMAGE vars)
OpenURL->>XdgOpen: exec with sanitized env
XdgOpen-->>OpenURL: success/error
OpenURL-->>DevPodOpen: error or nil
else Not AppImage
IsAppImage-->>DevPodOpen: false
DevPodOpen->>StdOpen: open.Run(url)
StdOpen->>XdgOpen: standard system handler
XdgOpen-->>StdOpen: result
StdOpen-->>DevPodOpen: error or nil
end
DevPodOpen-->>Caller: return error (wrapped if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 docstrings
🧪 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)
pkg/open/appimage_other.go (1)
9-10: Prefer returning an error instead of panicking in non-Linux stub.This path should be unreachable, but panic makes accidental calls crash the process unnecessarily.
♻️ Suggested change
+import "errors" + func openURLSanitized(_ string) error { - panic("openURLSanitized is only available on Linux") + return errors.New("openURLSanitized is only available on Linux") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/open/appimage_other.go` around lines 9 - 10, The non-Linux stub openURLSanitized should return an error instead of panicking; change the implementation of openURLSanitized to return a descriptive error (e.g., using errors.New or fmt.Errorf) stating that openURLSanitized is only available on Linux so accidental calls don't crash the process, and ensure callers receive that error instead of causing a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/open/open.go`:
- Line 33: tryOpen is swallowing opener errors which leads Open/Run to log
"opened url" on failures; update tryOpen (and its callers Open/Run) so any error
returned by the opener is propagated upward instead of being dropped: locate
tryOpen, ensure it returns the opener error instead of returning nil on failure,
and adjust Open/Run callers to check and return that error (and only log "opened
url" after confirming no error) so failures stop retries and are reported
correctly.
---
Nitpick comments:
In `@pkg/open/appimage_other.go`:
- Around line 9-10: The non-Linux stub openURLSanitized should return an error
instead of panicking; change the implementation of openURLSanitized to return a
descriptive error (e.g., using errors.New or fmt.Errorf) stating that
openURLSanitized is only available on Linux so accidental calls don't crash the
process, and ensure callers receive that error instead of causing a panic.
🪄 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: fce1789f-d334-48dd-997f-1f81d8ba36bd
📒 Files selected for processing (11)
cmd/pro/start.gocmd/up.gopkg/client/clientimplementation/daemonclient/client.gopkg/ide/jetbrains/generic.gopkg/ide/vscode/open.gopkg/ide/zed/zed.gopkg/ide/zed/zed_linux.gopkg/open/appimage_linux.gopkg/open/appimage_other.gopkg/open/open.gopkg/platform/client/client.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/open/open.go (1)
33-36:⚠️ Potential issue | 🟠 MajorReturn opener failures instead of retrying them forever.
Line 33 now gets the wrapped
open url:error fromtryOpen, but this loop still treats it like a transient reachability failure. Ifxdg-openkeeps failing,Openwill spin until the context expires and then returnnil, so callers still can't tell that nothing was opened. Please stop retrying once the failure is coming from the opener itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/open/open.go` around lines 33 - 36, The loop in Open retries forever even when tryOpen returns an opener failure; change Open so that when tryOpen(ctx, url, Run, log) returns a non-nil error that indicates the opener itself failed (the wrapped "open url:" error from tryOpen), Open returns that error immediately instead of continuing to retry until context deadline; locate the retry loop in Open and add a condition to detect the opener error returned by tryOpen and return it directly (do not swallow and continue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/open/open.go`:
- Around line 33-36: The loop in Open retries forever even when tryOpen returns
an opener failure; change Open so that when tryOpen(ctx, url, Run, log) returns
a non-nil error that indicates the opener itself failed (the wrapped "open url:"
error from tryOpen), Open returns that error immediately instead of continuing
to retry until context deadline; locate the retry loop in Open and add a
condition to detect the opener error returned by tryOpen and return it directly
(do not swallow and continue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89d1486e-1be6-46ff-a76d-0e1f7b578d10
📒 Files selected for processing (2)
pkg/open/appimage_other.gopkg/open/open.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/open/appimage_other.go
When running inside a Linux AppImage, the AppRun script prepends bundled library paths to LD_LIBRARY_PATH. Child processes like xdg-open and gio then load these bundled libraries instead of the system ones, causing symbol lookup errors (e.g. "undefined symbol: g_unix_mount_entry_get_options"). This centralizes all URL-opening through pkg/open.Run(), which detects the AppImage environment and strips injected variables (LD_LIBRARY_PATH, LD_PRELOAD, APPDIR, etc.) before spawning xdg-open. It also uses /usr/bin/xdg-open by absolute path to bypass Tauri's bundled wrapper. References: - AppImage/AppImageKit#396 - AppImage/AppImageKit#616 - tauri-apps/tauri#10617
skevetter
force-pushed
the
investigate-issue-127
branch
from
7a02738 to
820648d
Compare
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
Fixes #127
pkg/open.Run(), which detects AppImage environments and sanitizes the process environment before spawningxdg-openLD_LIBRARY_PATH,LD_PRELOAD,APPDIR, and other AppImage-injected variables that cause library conflicts for child processes/usr/bin/xdg-openby absolute path to bypass Tauri's bundled wrapperBackground
When running inside a Linux AppImage, the
AppRunscript prepends bundled library paths toLD_LIBRARY_PATH. Child processes likexdg-openandgiothen load these bundled libraries instead of the system ones, causing symbol lookup errors (e.g.undefined symbol: g_unix_mount_entry_get_options). This prevents JetBrains Gateway and other applications from opening via custom URL schemes.References:
Summary by CodeRabbit
Bug Fixes
Refactor