diff --git a/apps/server-rs/src/lib.rs b/apps/server-rs/src/lib.rs index 9cb6b74..e9e7138 100644 --- a/apps/server-rs/src/lib.rs +++ b/apps/server-rs/src/lib.rs @@ -1243,6 +1243,10 @@ impl ReadOnlyMuxStateSource { return; }; provider.switch_session(&name, client_tty); + // Move the sidebar highlight to the switched-to session, mirroring the + // move_focus path. Without this, Alt+digit switches the tmux session but + // the highlighted selection stays on the previously focused session. + *self.focused_session.lock().unwrap() = Some(name); } fn move_focus(&self, delta: i64, current_session: Option<&str>) -> Option { diff --git a/apps/server-rs/tests/protocol_shell.rs b/apps/server-rs/tests/protocol_shell.rs index 3c6a582..3de8551 100644 --- a/apps/server-rs/tests/protocol_shell.rs +++ b/apps/server-rs/tests/protocol_shell.rs @@ -551,11 +551,16 @@ async fn websocket_switch_index_switches_to_visible_session() { .await .expect("refresh command should send"); - let _ = timeout(Duration::from_secs(1), receiver.next()) + let state = timeout(Duration::from_secs(1), receiver.next()) .await .expect("refresh state should arrive before timeout") .expect("refresh state should arrive") .expect("refresh state should be valid"); + let state_text = state.as_text().expect("state should be text"); + assert!( + state_text.contains(r#""focusedSession":"worker""#), + "switch-index should move the sidebar highlight (focusedSession) to the switched-to session; got: {state_text}" + ); assert_eq!( *mux.switch_calls.lock().unwrap(), vec![("worker".to_string(), None)] diff --git a/packages/runtime-rs/src/tmux_provider.rs b/packages/runtime-rs/src/tmux_provider.rs index 9230fe5..c9eb2c6 100644 --- a/packages/runtime-rs/src/tmux_provider.rs +++ b/packages/runtime-rs/src/tmux_provider.rs @@ -605,6 +605,8 @@ impl MuxProvider for TmuxProvider { let panes = self.client.list_panes(PaneScope::All); let mut window_pane_counts: HashMap = HashMap::new(); let mut sidebars_by_window: HashMap> = HashMap::new(); + let mut window_session: HashMap = HashMap::new(); + let mut windows_by_session: HashMap> = HashMap::new(); let mut seen_pane_ids = HashSet::new(); for pane in panes { @@ -614,6 +616,13 @@ impl MuxProvider for TmuxProvider { *window_pane_counts .entry(pane.window_id.clone()) .or_insert(0) += 1; + window_session + .entry(pane.window_id.clone()) + .or_insert_with(|| pane.session_name.clone()); + windows_by_session + .entry(pane.session_name.clone()) + .or_default() + .insert(pane.window_id.clone()); if pane.title == "opensessions-sidebar" { sidebars_by_window .entry(pane.window_id) @@ -624,8 +633,19 @@ impl MuxProvider for TmuxProvider { for (window_id, sidebars) in sidebars_by_window { if window_pane_counts.get(&window_id) == Some(&1) { - for pane_id in sidebars { - self.client.kill_pane(&pane_id); + // The window holds only the sidebar. Killing it leaves the + // window with zero panes, destroying the window — and the + // session too if this is its last window. Only reclaim it when + // the session has other windows; otherwise keep the sidebar so + // closing the last work pane doesn't close the whole session. + let session_has_other_windows = window_session + .get(&window_id) + .and_then(|session| windows_by_session.get(session)) + .is_some_and(|windows| windows.len() > 1); + if session_has_other_windows { + for pane_id in sidebars { + self.client.kill_pane(&pane_id); + } } continue; } @@ -652,10 +672,27 @@ impl MuxProvider for TmuxProvider { // pane works even when the parent pane's cwd is unrelated to the // workspace (e.g. tmux sessions whose default cwd is `$HOME`). Falls // back to the literal path if the env is unset. - let command = format!( - "OPENSESSIONS_SESSION_NAME={} OPENSESSIONS_WINDOW_ID={window_id} REFOCUS_WINDOW={window_id} exec \"${{OPENSESSIONS_DIR:-.}}\"/{scripts_dir}/start.sh", - target.session_name, + // + // Wrap in `sh -c '...'`: tmux runs pane commands via the user's + // `default-command`/`default-shell`, which may be a non-POSIX shell + // (e.g. fish) that cannot parse `FOO=bar exec` or `${VAR:-default}`. + // Forcing `sh` keeps the launcher portable regardless of the user's + // interactive shell. + // + // Quoting is two-layered: the session name / window id sit inside + // double quotes (so `$`, backtick, `"` and `\` are still live to the + // inner `sh`), so escape those metacharacters first; then the whole + // `inner` is wrapped in single quotes for `sh -c`, with embedded single + // quotes escaped via the standard `'\''` dance. Without the inner + // escape a session name like `x"; rm -rf ~ #` would break out of the + // double quotes and inject commands. + let inner = format!( + "OPENSESSIONS_SESSION_NAME=\"{}\" OPENSESSIONS_WINDOW_ID=\"{}\" REFOCUS_WINDOW=\"{}\" exec \"${{OPENSESSIONS_DIR:-.}}\"/{scripts_dir}/start.sh", + sh_double_quote_escape(&target.session_name), + sh_double_quote_escape(window_id), + sh_double_quote_escape(window_id), ); + let command = format!("sh -c '{}'", inner.replace('\'', r"'\''")); let new_pane = self.client.split_sidebar_pane( &target.id, position == SidebarPosition::Left, @@ -672,6 +709,18 @@ impl MuxProvider for TmuxProvider { } } +/// Escape the characters that stay special inside a POSIX double-quoted +/// string (`\`, `"`, `$`, backtick) so an untrusted value (e.g. a tmux session +/// name) can be interpolated into `FOO="..."` without breaking out of the +/// quotes or triggering command/parameter substitution. Backslash is escaped +/// first so the backslashes added for the others are not doubled. +fn sh_double_quote_escape(s: &str) -> String { + s.replace('\\', "\\\\") + .replace('"', "\\\"") + .replace('$', "\\$") + .replace('`', "\\`") +} + fn session_format() -> &'static str { "#{session_id}\t#{session_name}\t#{session_created}\t#{session_attached}\t#{session_windows}\t#{session_path}" } diff --git a/packages/runtime-rs/tests/tmux_provider.rs b/packages/runtime-rs/tests/tmux_provider.rs index 5ae0294..5197d1a 100644 --- a/packages/runtime-rs/tests/tmux_provider.rs +++ b/packages/runtime-rs/tests/tmux_provider.rs @@ -248,7 +248,7 @@ fn tmux_provider_resolves_focuses_and_kills_agent_panes() { fn tmux_provider_kills_orphaned_and_duplicate_sidebar_panes() { let runner = Arc::new(RecordingRunner::new(HashMap::from([( "list-panes".to_string(), - "%1\talpha\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%1\talpha-2\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%2\tbeta\t@2\t0\t0\t1\t/dev/ttys2\t124\t/repo\tbash\tmain\t94\t24\t26\t119\n%3\tbeta\t@2\t0\t1\t0\t/dev/ttys3\t125\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%4\tbeta\t@2\t0\t2\t0\t/dev/ttys4\t126\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%5\t_os_stash\t@3\t0\t0\t1\t/dev/ttys5\t127\t/tmp\tzsh\topensessions-sidebar\t26\t24\t0\t25" + "%1\talpha\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%1\talpha-2\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%2\tbeta\t@2\t0\t0\t1\t/dev/ttys2\t124\t/repo\tbash\tmain\t94\t24\t26\t119\n%3\tbeta\t@2\t0\t1\t0\t/dev/ttys3\t125\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%4\tbeta\t@2\t0\t2\t0\t/dev/ttys4\t126\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%5\t_os_stash\t@3\t0\t0\t1\t/dev/ttys5\t127\t/tmp\tzsh\topensessions-sidebar\t26\t24\t0\t25\n%6\talpha\t@4\t0\t0\t1\t/dev/ttys6\t128\t/repo\tbash\tmain\t94\t24\t26\t119" .to_string(), )]))); let provider = TmuxProvider::new(runner.clone()); @@ -280,6 +280,30 @@ fn tmux_provider_kills_orphaned_and_duplicate_sidebar_panes() { ); } +#[test] +fn tmux_provider_keeps_lone_sidebar_when_it_is_the_sessions_only_window() { + // A window containing only the sidebar, in a session that has no other + // window, must NOT be killed: doing so would leave the window with zero + // panes, destroying the session (closing the last work pane should not + // close the whole session). + let runner = Arc::new(RecordingRunner::new(HashMap::from([( + "list-panes".to_string(), + "%9\tsolo\t@9\t0\t0\t1\t/dev/ttys9\t130\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25" + .to_string(), + )]))); + let provider = TmuxProvider::new(runner.clone()); + + provider.kill_orphaned_sidebar_panes(); + + let calls = runner.calls.lock().unwrap().clone(); + assert!( + !calls + .iter() + .any(|call| call == &vec!["kill-pane", "-t", "%9"]), + "a lone sidebar that is the session's only window must be kept (killing it would destroy the session)" + ); +} + #[test] fn tmux_provider_spawns_sidebar_against_edge_pane_and_titles_it() { let runner = Arc::new(RecordingRunner::new(HashMap::from([ @@ -316,7 +340,7 @@ fn tmux_provider_spawns_sidebar_against_edge_pane_and_titles_it() { assert_eq!( split_call.last().map(String::as_str), Some( - "OPENSESSIONS_SESSION_NAME=alpha OPENSESSIONS_WINDOW_ID=@1 REFOCUS_WINDOW=@1 exec \"${OPENSESSIONS_DIR:-.}\"//scripts/start.sh" + "sh -c 'OPENSESSIONS_SESSION_NAME=\"alpha\" OPENSESSIONS_WINDOW_ID=\"@1\" REFOCUS_WINDOW=\"@1\" exec \"${OPENSESSIONS_DIR:-.}\"//scripts/start.sh'" ) ); assert!( @@ -326,6 +350,80 @@ fn tmux_provider_spawns_sidebar_against_edge_pane_and_titles_it() { ); } +#[test] +fn tmux_provider_escapes_shell_metacharacters_in_session_name() { + // spawn_sidebar takes the session name from the target pane's listing, so + // the untrusted value lives in the list-panes mock (field 2). A name + // carrying double quotes, `$` and a backtick must not break out of the + // `OPENSESSIONS_SESSION_NAME="..."` double quotes and inject commands. + let evil = r#"x"; touch /tmp/pwned; $(id)`whoami`"#; + let runner = Arc::new(RecordingRunner::new(HashMap::from([ + ( + "list-panes".to_string(), + format!("%1\t{evil}\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tbash\tmain\t80\t24\t0\t79"), + ), + ( + "split-window".to_string(), + format!("%9\t{evil}\t@1\t0\t1\t0\t/dev/ttys9\t999\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25"), + ), + ]))); + let provider = TmuxProvider::new(runner.clone()); + + provider.spawn_sidebar("ignored", "@1", 26, SidebarPosition::Left, "/scripts"); + + let calls = runner.calls.lock().unwrap().clone(); + let split_call = calls + .iter() + .find(|call| call.first().map(String::as_str) == Some("split-window")) + .expect("split-window should be called"); + let command = split_call.last().map(String::as_str).unwrap(); + + // Every metacharacter is backslash-escaped inside the double quotes. + assert!( + command.contains(r#"OPENSESSIONS_SESSION_NAME="x\"; touch /tmp/pwned; \$(id)\`whoami\`""#), + "session name not safely escaped: {command}" + ); + // The raw, unescaped breakout sequence must never appear. + assert!( + !command.contains(r#"="x"; touch"#), + "unescaped double quote broke out of the assignment: {command}" + ); +} + +#[test] +fn tmux_provider_escapes_single_quotes_in_session_name() { + // Single quotes are inert inside the inner double quotes but must survive + // the outer `sh -c '...'` wrap via the `'\''` dance. + let runner = Arc::new(RecordingRunner::new(HashMap::from([ + ( + "list-panes".to_string(), + "%1\to'brien\t@1\t0\t0\t1\t/dev/ttys1\t123\t/repo\tbash\tmain\t80\t24\t0\t79" + .to_string(), + ), + ( + "split-window".to_string(), + "%9\to'brien\t@1\t0\t1\t0\t/dev/ttys9\t999\t/repo\tzsh\topensessions-sidebar\t26\t24\t0\t25" + .to_string(), + ), + ]))); + let provider = TmuxProvider::new(runner.clone()); + + provider.spawn_sidebar("ignored", "@1", 26, SidebarPosition::Left, "/scripts"); + + let calls = runner.calls.lock().unwrap().clone(); + let command = calls + .iter() + .find(|call| call.first().map(String::as_str) == Some("split-window")) + .and_then(|call| call.last()) + .cloned() + .expect("split-window should be called"); + + assert_eq!( + command, + r#"sh -c 'OPENSESSIONS_SESSION_NAME="o'\''brien" OPENSESSIONS_WINDOW_ID="@1" REFOCUS_WINDOW="@1" exec "${OPENSESSIONS_DIR:-.}"//scripts/start.sh'"# + ); +} + #[test] fn std_command_runner_executes_tmux_binary_and_captures_output() { let runner = StdCommandRunner::new("printf");