fix: sidebar crash under non-POSIX shells (fish), Alt+digit highlight, and pane-close killing the session#46
Open
panosAthDBX wants to merge 5 commits into
Conversation
spawn_sidebar emitted a POSIX command (FOO=bar exec ...; ${VAR:-default}) that tmux runs via the user's default-command/default-shell. Under a non-POSIX shell (e.g. fish) it fails to parse and the sidebar pane dies with status 127 before start.sh runs. Wrap the command in sh -c '...' (escaping single quotes) so it is interpreted by a POSIX shell regardless of the user's interactive shell.
Co-authored-by: Isaac
Double-quote OPENSESSIONS_SESSION_NAME / WINDOW_ID / REFOCUS_WINDOW values inside the sh -c command so a session name with spaces (or other word-splitting chars) does not break the launcher. Single quotes are still escaped for the sh -c wrapper. Addresses inspect review findings 2, 5. Co-authored-by: Isaac
…index switch_visible_index switched the tmux session but never updated focused_session, so the sidebar highlight (which follows focused_session) stayed on the previously selected session after an Alt+digit switch. Set focused_session to the target, mirroring move_focus. Co-authored-by: Isaac
![inspect-review[bot]](https://avatars.githubusercontent.com/in/3177512?s=60&v=4){kind=small}
There was a problem hiding this comment.
inspect review
Triage: 6 entities analyzed | 0 critical, 0 high, 4 medium, 2 low
Verdict: standard_review
Findings (1)
- [low] Shell injection vulnerability in tmux_provider.rs spawn_sidebar: Single quotes in session_name are escaped, but double quotes are not. If session_name contains a double quote, it will break out of the inner command's double-quoted string and allow arbitrary command injection.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
kill_orphaned_sidebar_panes killed any sidebar whose window had no other panes. When you closed the last work pane in a window, the sidebar was left alone and got killed, leaving the window with zero panes — which destroys the window, and the session if it was the last one (with detach-on-destroy, the client then detaches and it looks like the session 'closed'). Now only reclaim a lone sidebar when its session has other windows; otherwise keep it so the session survives and focus lands on the sidebar. Co-authored-by: Isaac
There was a problem hiding this comment.
inspect review
Triage: 9 entities analyzed | 0 critical, 0 high, 6 medium, 3 low
Verdict: standard_review
Findings (2)
- [low] Shell injection vulnerability in spawn_sidebar: Single quotes in session_name are not properly escaped before being used in the sh -c command. The escape logic
inner.replace('\'', r"'\''")is applied to the entire command string, but session_name is already interpolated intoinnerbefore this escaping happens. An attacker-controlled session name containing single quotes could break out of the sh -c string and execute arbitrary commands. - [low] Logic error in kill_orphaned_sidebar_panes: The code uses
window_session.entry(pane.window_id.clone()).or_insert_with(|| pane.session_name.clone())which means if a window_id appears in multiple sessions (which shouldn't happen in tmux but the code doesn't validate), only the first session_name will be stored. This could lead to incorrect session_has_other_windows checks when determining whether to kill orphaned sidebars.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
panosAthDBX
changed the title
Member
|
thanks for this PR. |
The sidebar spawn command interpolates the tmux session name and window id into double-quoted `OPENSESSIONS_*` env assignments inside `sh -c '...'`. The outer single-quote escaping only guards the `sh -c` wrapper; inside the double quotes `$`, backtick, `"` and `\` stay live, so a session name like `x"; rm -rf ~ #` could break out of the quotes and inject commands. Add `sh_double_quote_escape()` and run the session name and window id through it before interpolation. Backslash is escaped first so the backslashes added for the other metacharacters are not doubled. Regression tests cover a name with `"`, `$` and backtick (must be escaped, breakout sequence must not appear) and a name with a single quote (must survive the outer `'\''` wrap). The existing benign-name test is unchanged, confirming normal session names still render identically. Co-authored-by: Isaac
There was a problem hiding this comment.
inspect review
Triage: 12 entities analyzed | 0 critical, 0 high, 6 medium, 6 low
Verdict: standard_review
Findings (1)
- [low] In kill_orphaned_sidebar_panes, the logic for checking if a session has other windows is incorrect. It checks
is_some_and(|windows| windows.len() > 1)but this will be true even when the session has only one window (the current one), because the HashSet will contain the current window_id. The check should verify that the session has windows OTHER than the current one.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
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
2 participants
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.
Three self-contained bug fixes, independent of the larger Rust-parity work in #45. (These commits also appear in #45; whichever merges first, the other rebases cleanly.)
1. Sidebar pane crashes immediately under a non-POSIX default shell (e.g. fish)
spawn_sidebaremits a POSIX command (FOO=bar exec …; "${VAR:-default}") that tmux runs via the user'sdefault-command/default-shell. Under fish (or csh) it can't be parsed and the sidebar pane dies with status 127 beforestart.shruns — the sidebar never appears. Wrapped insh -c '…'; env-var values are double-quoted so session names with spaces don't word-split.2. Alt+digit session switch doesn't move the sidebar highlight
switch_visible_indexswitched the tmux session but never updatedfocused_session, which is what the sidebar highlight follows — so afterAlt+1/2/…the highlight stayed on the previously focused session (the arrow-keymove_focuspath already updated it). Now setsfocused_sessionto the switched-to session.3. Closing the last work pane in a window kills the whole session
kill_orphaned_sidebar_paneskilled any sidebar whose window had no other panes. Closing the last work pane left the sidebar alone → it was killed → the window had zero panes → tmux destroyed the window, and the session if it was its last (withdetach-on-destroy on, the client then detaches, so it looks like the session "closed"). Now a lone sidebar is only reclaimed when its session has other windows; otherwise it's kept so the session survives and focus lands on the sidebar.Testing
cargo test --workspaceis green. Regression tests added/extended for all three fixes.This pull request and its description were written by Isaac.