diff --git a/crates/forgetty-gtk/src/app.rs b/crates/forgetty-gtk/src/app.rs index 9aa6c7b..3b81f91 100644 --- a/crates/forgetty-gtk/src/app.rs +++ b/crates/forgetty-gtk/src/app.rs @@ -6608,7 +6608,6 @@ fn open_config_file() { } } - // --------------------------------------------------------------------------- // Tab right-click context menu (T-M1-extra-009) // --------------------------------------------------------------------------- diff --git a/crates/forgetty-gtk/src/terminal.rs b/crates/forgetty-gtk/src/terminal.rs index e670d65..f8ea7ef 100644 --- a/crates/forgetty-gtk/src/terminal.rs +++ b/crates/forgetty-gtk/src/terminal.rs @@ -129,6 +129,13 @@ pub struct TerminalState { /// Suppress BEL flash until this instant. Set when Ctrl+C is written so the /// shell's readline BEL response (zsh beeps on SIGINT) doesn't cause a flash. pub suppress_bell_until: Option, + /// Suppress the line-discipline echo of the VINTR byte until this instant. + /// Set when Ctrl+C is dispatched so we can drop the upcoming `^X` (or `^C`, + /// `^D`, etc.) caret-form echo from the byte stream before it reaches the + /// VT parser. Without this, users who have remapped VINTR away from 0x03 + /// see e.g. `^X` printed after pressing Ctrl+C, which doesn't match what + /// they pressed (FIX-017 round 2). + pub suppress_intr_echo_until: Option, /// Timestamp of the last PTY data received. Used to detect idle periods /// for calling `malloc_trim(0)` to return freed memory to the OS (T-028). pub last_pty_data: Instant, @@ -255,6 +262,7 @@ pub fn create_terminal( bell_flash_until: None, last_bell: Instant::now() - Duration::from_secs(1), suppress_bell_until: None, + suppress_intr_echo_until: None, last_pty_data: Instant::now(), malloc_trimmed: false, notification_ring: false, @@ -358,10 +366,20 @@ pub fn create_terminal( let (_, offset, _) = s.terminal.scrollbar_state(); s.viewport_offset = offset; + // FIX-017 round 2: if a Ctrl+C was recently dispatched, + // drop the line-discipline ECHOCTL echo of the VINTR + // byte (e.g. `^X` for VINTR=0x18, `^C` for the default). + // The echo arrives as two printable bytes `^` + // from the remote PTY's output stream. We drop the + // first such pair within the 150 ms window so the user + // doesn't see a control-char-name printed after Ctrl+C. + let filtered_data = + drop_first_caret_form_echo(&data, &mut s.suppress_intr_echo_until); + // Scan for OSC notification sequences BEFORE feeding to VT parser. - let osc_notification = scan_osc_notification(&data); + let osc_notification = scan_osc_notification(&filtered_data); - s.terminal.feed(&data); + s.terminal.feed(&filtered_data); s.last_pty_data = Instant::now(); s.malloc_trimmed = false; @@ -612,6 +630,15 @@ pub fn create_terminal( s.cursor_blink_visible = true; s.last_blink_toggle = Instant::now(); s.suppress_bell_until = Some(Instant::now() + Duration::from_millis(300)); + // FIX-017 round 2: also suppress the ECHOCTL echo of + // the VINTR byte that the remote line discipline will + // emit. Without this, the next ~150 ms of output + // shows e.g. `^X` (when VINTR is remapped) printed + // before the shell prompt — visually mismatched with + // what the user pressed. Window is short and only + // drops the FIRST caret-form echo it sees. + s.suppress_intr_echo_until = + Some(Instant::now() + Duration::from_millis(150)); } return glib::Propagation::Stop; } @@ -1431,6 +1458,45 @@ pub fn create_terminal( Ok((vbox, drawing_area, state)) } +/// Drop the first `^` (caret-form control-char echo) from `data` +/// if `suppress_until` is set and hasn't expired. Returns either a borrow +/// of `data` (unmodified) or an owned `Vec` with the two echo bytes +/// removed. Clears `suppress_until` once it either drops a match or +/// observes the deadline has passed. +/// +/// The line-discipline ECHOCTL echo of a control char `c` (0x00..=0x1f or +/// 0x7f) is the two printable bytes `0x5e` (caret) followed by either +/// `c + 0x40` (for 0x00..=0x1f → range 0x40..=0x5f) or `0x3f` (`?`, for +/// 0x7f). We scan for the first such pair within the suppression window; +/// the FIRST match consumes the suppression so a later legitimate `^X` +/// in real output isn't filtered (FIX-017 round 2). +fn drop_first_caret_form_echo<'a>( + data: &'a [u8], + suppress_until: &mut Option, +) -> std::borrow::Cow<'a, [u8]> { + use std::borrow::Cow; + + let Some(deadline) = *suppress_until else { + return Cow::Borrowed(data); + }; + if Instant::now() > deadline { + *suppress_until = None; + return Cow::Borrowed(data); + } + + // Scan for `^` where letter ∈ [0x40, 0x5f] ∪ {0x3f}. + for (i, w) in data.windows(2).enumerate() { + if w[0] == 0x5e && ((0x40..=0x5f).contains(&w[1]) || w[1] == 0x3f) { + *suppress_until = None; + let mut out = Vec::with_capacity(data.len().saturating_sub(2)); + out.extend_from_slice(&data[..i]); + out.extend_from_slice(&data[i + 2..]); + return Cow::Owned(out); + } + } + Cow::Borrowed(data) +} + /// Helper: convert a `Color` to `(r, g, b)` f64 values (0.0..1.0), using /// the given default `Rgba` for `Color::Default`. #[inline] @@ -2913,3 +2979,87 @@ fn draw_rounded_rect(ctx: &cairo::Context, x: f64, y: f64, w: f64, h: f64, r: f6 ctx.arc(x + r, y + r, r, std::f64::consts::PI, 3.0 * std::f64::consts::FRAC_PI_2); ctx.close_path(); } + +#[cfg(test)] +mod tests { + use super::drop_first_caret_form_echo; + use std::time::{Duration, Instant}; + + /// FIX-017 round 2: with an active suppression deadline, the first + /// `^X` (caret + uppercase letter) pair is removed and the deadline + /// is consumed. + #[test] + fn test_drop_caret_x_drops_first_match_and_clears_flag() { + let mut deadline = Some(Instant::now() + Duration::from_secs(1)); + let input = b"^X\r\nprompt$ "; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, b"\r\nprompt$ ", "echo bytes ^X should be removed"); + assert!(deadline.is_none(), "suppression flag should clear after a successful drop"); + } + + /// `^C` (default VINTR echo) is also caret-form and gets dropped. + #[test] + fn test_drop_caret_c_default_vintr_echo() { + let mut deadline = Some(Instant::now() + Duration::from_secs(1)); + let input = b"running\r\n^C\r\n"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, b"running\r\n\r\n"); + assert!(deadline.is_none()); + } + + /// `^?` is the caret-form echo of DEL (0x7f) and must also be dropped. + #[test] + fn test_drop_caret_question_mark_for_del() { + let mut deadline = Some(Instant::now() + Duration::from_secs(1)); + let input = b"^?ok"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, b"ok"); + assert!(deadline.is_none()); + } + + /// No suppression set: bytes are returned unchanged, no `Vec` allocation. + #[test] + fn test_no_suppression_means_no_filtering() { + let mut deadline: Option = None; + let input = b"^X anywhere in output\r\n"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, input); + assert!(deadline.is_none()); + } + + /// Suppression set but deadline has passed: bytes returned unchanged AND + /// the flag is cleared (so subsequent calls short-circuit immediately). + #[test] + fn test_expired_deadline_clears_flag_and_returns_unchanged() { + let mut deadline = Some(Instant::now() - Duration::from_millis(10)); + let input = b"^X should NOT be filtered"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, input); + assert!(deadline.is_none(), "expired deadline must be cleared"); + } + + /// Caret followed by a non-caret-form letter (e.g. lowercase, digit, + /// punctuation) is not an ECHOCTL echo and must NOT be dropped. + #[test] + fn test_caret_followed_by_non_control_letter_not_dropped() { + let mut deadline = Some(Instant::now() + Duration::from_secs(1)); + // ^a (0x5e 0x61) — lowercase 'a' is outside the caret-form letter range. + let input = b"see ^a here"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, input); + assert!(deadline.is_some(), "no match means flag stays active for the rest of the window"); + } + + /// Only the FIRST caret-form pair is dropped within a single call. + /// (A second echo within the same call is exceedingly unlikely from + /// a single Ctrl+C, and the flag is cleared after the first drop so + /// subsequent calls won't filter either.) + #[test] + fn test_only_first_caret_form_dropped() { + let mut deadline = Some(Instant::now() + Duration::from_secs(1)); + let input = b"^X ^Y"; + let out = drop_first_caret_form_echo(input, &mut deadline); + assert_eq!(&*out, b" ^Y", "second ^Y stays — flag is consumed"); + assert!(deadline.is_none()); + } +} diff --git a/crates/forgetty-pty/src/process.rs b/crates/forgetty-pty/src/process.rs index e1aab13..ad17453 100644 --- a/crates/forgetty-pty/src/process.rs +++ b/crates/forgetty-pty/src/process.rs @@ -198,6 +198,36 @@ impl PtyProcess { self.master.process_group_leader() } + /// Get the `VINTR` character (the byte the kernel's line discipline + /// translates to `SIGINT` when `ISIG` is enabled). + /// + /// Reads `c_cc[VINTR]` via `tcgetattr` on the master PTY fd. Returns + /// `None` if the master fd is unavailable or `tcgetattr` fails — callers + /// fall back to the POSIX default `0x03`. + /// + /// Used by the daemon's Ctrl+C path (FIX-017) to write a byte that + /// matches the current `VINTR` setting rather than hardcoded `0x03`. + /// This makes Ctrl+C work for users who have remapped `VINTR` (via + /// `stty intr ` or shell init) and for SSH sessions where the + /// remote PTY inherits the local `VINTR` through ssh's `pty-modes`. + #[cfg(unix)] + pub fn vintr(&self) -> Option { + use std::os::unix::io::RawFd; + let fd: RawFd = self.master.as_raw_fd()?; + let mut termios = unsafe { std::mem::zeroed::() }; + if unsafe { libc::tcgetattr(fd, &mut termios) } != 0 { + return None; + } + Some(termios.c_cc[libc::VINTR]) + } + + /// Non-Unix stub: VINTR is a POSIX termios concept and not meaningful + /// off Unix. Returns `None` so callers use the POSIX default `0x03`. + #[cfg(not(unix))] + pub fn vintr(&self) -> Option { + None + } + /// Kill the child process. pub fn kill(&mut self) -> Result<()> { self.child.kill().map_err(|e| ForgettyError::Pty(format!("failed to kill child: {e}"))) @@ -401,6 +431,21 @@ mod tests { ); } + /// FIX-017 round 2: `vintr()` reads the line discipline's interrupt + /// character from a freshly spawned PTY. Newly created PTYs use the + /// kernel default termios, where `c_cc[VINTR] == 0x03` (Ctrl+C). + /// Verifies the syscall plumbing works end-to-end; it doesn't test the + /// remapping case (that requires `stty intr ` and falls under + /// AC-10's human test). + #[cfg(unix)] + #[test] + fn test_vintr_default_is_etx() { + let proc = PtyProcess::spawn(default_size(), None, None, true).expect("failed to spawn"); + let vintr = proc.vintr().expect("vintr() should succeed on a fresh PTY"); + assert_eq!(vintr, 0x03, "fresh PTY's VINTR should be 0x03 (Ctrl+C), got {vintr:#04x}"); + drop(proc); + } + #[test] fn test_resize_no_panic() { let proc = PtyProcess::spawn(default_size(), None, None, true).expect("failed to spawn"); diff --git a/crates/forgetty-session/src/manager.rs b/crates/forgetty-session/src/manager.rs index 056c396..d9c4f00 100644 --- a/crates/forgetty-session/src/manager.rs +++ b/crates/forgetty-session/src/manager.rs @@ -1395,6 +1395,21 @@ impl SessionManager { // I/O // ----------------------------------------------------------------------- + /// Read the byte the kernel's line discipline treats as the interrupt + /// character (`c_cc[VINTR]`) for the given pane's PTY. + /// + /// Returns `None` if the pane is unknown or the termios read fails; + /// callers should fall back to `0x03` (POSIX default). The daemon's + /// Ctrl+C path uses this so it can write the byte the line discipline + /// will actually translate to `SIGINT` — both locally and (because ssh + /// forwards `VINTR` via `pty-modes`) on the remote end of an SSH + /// session (FIX-017). + pub fn intr_byte(&self, id: PaneId) -> Option { + let inner = self.inner.lock().unwrap_or_else(|e| e.into_inner()); + let pane = inner.panes.get(&id)?; + pane.pty_bridge.pty.vintr() + } + /// Write raw bytes to the PTY master for the given pane. pub fn write_pty(&self, id: PaneId, data: &[u8]) -> Result<()> { let mut inner = self.inner.lock().unwrap_or_else(|e| e.into_inner()); @@ -1747,11 +1762,14 @@ impl SessionManager { /// Send SIGINT to the foreground process group of a pane. /// - /// This is the daemon-side implementation of the Ctrl+C signal path. - /// It does two things: + /// Daemon-side implementation of the Ctrl+C signal path: /// 1. The caller (handle_send_sigint) already wrote 0x03 via write_pty. - /// 2. This method calls kill(-pgid, SIGINT) via tcgetpgrp on the master PTY fd. - /// This is necessary when the child has disabled ISIG (e.g. Node.js, pm2). + /// 2. This method calls kill(-pgid, SIGINT) UNLESS the foreground pgrp + /// leader is a known signal-forwarder (ssh, mosh-client, telnet, rsh). + /// Forwarders read 0x03 from stdin and pass it through; killing them + /// kills the local end of the forward (FIX-017). + /// 3. For non-forwarders, the kill catches raw-mode apps that swallow + /// 0x03 without acting on it (Node.js, pm2 — BUG-001). pub fn send_sigint(&self, id: PaneId) { #[cfg(target_os = "linux")] { @@ -1759,8 +1777,16 @@ impl SessionManager { if let Some(pane) = inner.panes.get(&id) { if let Some(pgid) = pane.pty_bridge.pty.foreground_pgrp() { let my_pid = std::process::id() as libc::pid_t; - if pgid > 0 && pgid != my_pid { + if pgid > 0 && pgid != my_pid && !pgid_is_signal_forwarder(pgid) { unsafe { libc::kill(-(pgid as libc::c_int), libc::SIGINT) }; + } else if pgid > 0 && pgid != my_pid { + debug!( + %id, + pgid, + "send_sigint: foreground is a signal forwarder \ + (e.g. ssh) — skipping kill(-pgid), intr byte \ + already written" + ); } } } @@ -1801,6 +1827,39 @@ fn home_dir_fallback() -> PathBuf { std::env::var("HOME").map(PathBuf::from).unwrap_or_else(|_| PathBuf::from("/")) } +/// Pure matcher: is this `comm` value a known signal-forwarder? +/// +/// Linux `/proc//comm` contents (whitespace-trimmed, max 16 chars). +/// Forwarders read `0x03` from stdin and pass it across a connection +/// (SSH, mosh, telnet, rsh). Sending `kill(-pgid, SIGINT)` to them +/// kills the local end of the forward and severs the connection — see +/// FIX-017. We skip `kill` for these and rely on the unconditional +/// `0x03` byte write to interrupt the remote process. +/// +/// Allowlist contents: +/// - `ssh` — OpenSSH client (covers `sshpass` which exec's into ssh). +/// - `mosh-client` — actual mosh interactive binary; the `mosh` wrapper +/// is a Perl script that exec's into `mosh-client` post-handshake. +/// - `telnet` — legacy but still in use; same forwarding semantics. +/// - `rsh` — vintage but in scope for parity. +fn is_signal_forwarder_comm(comm: &str) -> bool { + matches!(comm, "ssh" | "mosh-client" | "telnet" | "rsh") +} + +/// Read `/proc//comm` and check the allowlist. +/// +/// Returns `false` on any read error (safe default: kill fires, matching +/// the pre-FIX-017 behavior). The unit-testable matcher is +/// `is_signal_forwarder_comm`. +#[cfg(target_os = "linux")] +fn pgid_is_signal_forwarder(pgid: i32) -> bool { + let path = format!("/proc/{pgid}/comm"); + match std::fs::read_to_string(&path) { + Ok(s) => is_signal_forwarder_comm(s.trim()), + Err(_) => false, + } +} + /// Recursively find the `Leaf` node matching `target` in `tree` and replace it /// in-place with a `Split` node containing the original leaf and a new leaf for /// `new_pane`. Returns `true` if the replacement was made. @@ -3875,4 +3934,43 @@ mod tests { session.close_pane(pane1).ok(); } + + // ----------------------------------------------------------------------- + // FIX-017: signal-forwarder allowlist (AC-9) + // + // Pure-function test of the matcher used to gate kill(-pgid) in + // SessionManager::send_sigint. Independent of /proc — exercises the + // string match alone. + // ----------------------------------------------------------------------- + + /// AC-9: known forwarders must match the allowlist exactly. + #[test] + fn test_is_signal_forwarder_comm_known_forwarders() { + assert!(is_signal_forwarder_comm("ssh"), "ssh must be allowlisted"); + assert!(is_signal_forwarder_comm("mosh-client"), "mosh-client must be allowlisted"); + assert!(is_signal_forwarder_comm("telnet"), "telnet must be allowlisted"); + assert!(is_signal_forwarder_comm("rsh"), "rsh must be allowlisted"); + } + + /// AC-9: shells, TUIs, raw-mode apps, and look-alikes must NOT match. + /// kill(-pgid) must still fire for these to preserve BUG-001 behavior. + #[test] + fn test_is_signal_forwarder_comm_non_forwarders() { + for comm in ["node", "python3", "bash", "zsh", "vim", "", "sshd", "my-ssh-wrapper"] { + assert!( + !is_signal_forwarder_comm(comm), + "{comm:?} must NOT be classified as a forwarder" + ); + } + } + + /// I/O wrapper must return false for a pgid with no /proc entry. + /// Safe-default: any read error => kill fires (current pre-FIX-017 behavior). + #[cfg(target_os = "linux")] + #[test] + fn test_pgid_is_signal_forwarder_missing_pid_returns_false() { + // i32::MAX is well outside the kernel's pid_max range on any + // realistic system; the /proc read will fail with ENOENT. + assert!(!pgid_is_signal_forwarder(i32::MAX)); + } } diff --git a/crates/forgetty-socket/src/handlers.rs b/crates/forgetty-socket/src/handlers.rs index 9627cd9..0ab4947 100644 --- a/crates/forgetty-socket/src/handlers.rs +++ b/crates/forgetty-socket/src/handlers.rs @@ -1101,11 +1101,20 @@ fn handle_send_sigint(request: &Request, sm: &SessionManager) -> Response { Err(e) => return e, }; - // Write 0x03 (ETX / Ctrl+C) to the PTY. - // The daemon owns the master PTY fd; it can also do the kill(-pgid, SIGINT). - match sm.write_pty(id, &[0x03]) { + // Resolve the byte the kernel's line discipline treats as the interrupt + // character (c_cc[VINTR]). Falls back to 0x03 (POSIX default) if the + // termios read fails. Writing this byte — instead of hardcoded 0x03 — + // makes Ctrl+C work for local users who have remapped VINTR (e.g., + // `stty intr ^X`), and for ssh sessions where the remote PTY inherits + // the local VINTR via ssh's pty-modes payload (FIX-017). + let intr_byte = sm.intr_byte(id).unwrap_or(0x03); + + match sm.write_pty(id, &[intr_byte]) { Ok(()) => { - // Also send SIGINT to the foreground process group via the session manager. + // For non-forwarder foreground processes (e.g., Node.js, pm2 in + // raw mode that swallow the byte), also kill(-pgid, SIGINT) so + // the signal reaches them via the kernel directly (BUG-001). + // Forwarders (ssh, mosh) are skipped — see `send_sigint`. sm.send_sigint(id); Response::success(request.id.clone(), serde_json::json!({ "ok": true })) } diff --git a/docs/socket-api.md b/docs/socket-api.md index 740c314..bfa22ce 100644 --- a/docs/socket-api.md +++ b/docs/socket-api.md @@ -408,8 +408,13 @@ writes them to the PTY master fd unchanged. Invalid base64 returns #### `send_sigint` -Send SIGINT (`0x03`) to the foreground process group of a pane. Equivalent -to the user pressing Ctrl+C. +Sends `0x03` (ETX/Ctrl+C) to the PTY unconditionally. Additionally calls +`kill(-pgid, SIGINT)` on the foreground process group **unless** the +foreground process is a known signal-forwarder (`ssh`, `mosh-client`, +`telnet`, `rsh`) — those tools forward `0x03` to a remote PTY and would +be killed by a local SIGINT (FIX-017). For shells and most TUIs, the +line discipline handles `0x03` via ISIG; the kill catches raw-mode apps +that swallow `0x03` without acting on it (Node.js, pm2 — BUG-001). **Request:** `{"jsonrpc":"2.0","method":"send_sigint","params":{"pane_id":""},"id":1}`