fix: validate retained selection cells atomically

This commit is contained in:
Jonathan Liebig
2026-09-05 17:30:30 +02:00
parent 695aae9830
commit 83827803cd
14 changed files with 317 additions and 21 deletions
+2
View File
@@ -167,6 +167,8 @@ pub enum Method {
PaneEditScrollback(PaneTarget),
#[serde(rename = "pane.selection.read")]
PaneSelectionRead(PaneSelectionReadParams),
#[serde(rename = "pane.selection.read_checked")]
PaneSelectionReadChecked(PaneSelectionReadCheckedParams),
#[serde(rename = "pane.copy_motion")]
PaneCopyMotion(PaneCopyMotionParams),
#[serde(rename = "pane.copy_search")]
+9
View File
@@ -263,6 +263,15 @@ pub struct PaneSelectionReadParams {
pub content_revision: Option<u64>,
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, schemars::JsonSchema)]
pub struct PaneSelectionReadCheckedParams {
pub pane_id: String,
pub anchor: PaneTextPoint,
pub cursor: PaneTextPoint,
/// Selected cell symbols in reading order, including empty wide-character tails.
pub expected_cells: Vec<String>,
}
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, schemars::JsonSchema)]
#[serde(rename_all = "snake_case")]
pub enum PaneCopyMotion {
+1
View File
@@ -435,6 +435,7 @@ pub(crate) fn api_method_name(method: &Method) -> &'static str {
Method::PaneScroll(_) => "pane.scroll",
Method::PaneEditScrollback(_) => "pane.edit_scrollback",
Method::PaneSelectionRead(_) => "pane.selection.read",
Method::PaneSelectionReadChecked(_) => "pane.selection.read_checked",
Method::PaneCopyMotion(_) => "pane.copy_motion",
Method::PaneCopySearch(_) => "pane.copy_search",
Method::PaneList(_) => "pane.list",
+3
View File
@@ -1090,6 +1090,9 @@ impl App {
Method::PaneSelectionRead(params) => {
return self.handle_pane_selection_read(request.id, params);
}
Method::PaneSelectionReadChecked(params) => {
return self.handle_pane_selection_read_checked(request.id, params);
}
Method::PaneCopyMotion(params) => {
return self.handle_pane_copy_motion(request.id, params);
}
+49
View File
@@ -266,6 +266,55 @@ impl App {
}
}
pub(crate) fn pane_selection_text_checked(
&self,
params: &crate::api::schema::PaneSelectionReadCheckedParams,
) -> Result<String, (&'static str, String)> {
let runtime = self
.parse_pane_id(&params.pane_id)
.and_then(|(ws_idx, pane_id)| {
self.state
.runtime_for_pane_in_workspace(&self.terminal_runtimes, ws_idx, pane_id)
.map(|runtime| (pane_id, runtime))
});
let Some((pane_id, runtime)) = runtime else {
return Err((
"pane_not_found",
format!("pane not found: {}", params.pane_id),
));
};
let selection = crate::selection::Selection::absolute_range(
pane_id,
(params.anchor.row, params.anchor.col),
(params.cursor.row, params.cursor.col),
);
runtime
.extract_selection_checked(&selection, &params.expected_cells)
.ok_or_else(|| {
(
"stale_content",
"selected text changed or is unavailable".to_owned(),
)
})
}
pub(super) fn handle_pane_selection_read_checked(
&mut self,
id: String,
params: crate::api::schema::PaneSelectionReadCheckedParams,
) -> String {
match self.pane_selection_text_checked(&params) {
Ok(text) => encode_success(
id,
ResponseResult::PaneSelection {
pane_id: params.pane_id,
text,
},
),
Err((code, message)) => encode_error(id, code, message),
}
}
pub(super) fn handle_pane_copy_motion(
&mut self,
id: String,
+53 -11
View File
@@ -1,6 +1,54 @@
use super::*;
impl ClientShellState {
pub(super) fn request_retained_selection_copy(&mut self, outcome: &mut ClientShellInput) {
let checked = (|| {
let selection = self.selection.as_ref()?;
let surface = self.pane_surface.as_ref()?;
let pane = surface
.panes
.iter()
.find(|pane| pane.pane_id == selection.pane_id)?;
let ((start_row, start_col), (end_row, end_col)) = selection.ordered_cells();
let mut expected_cells = Vec::new();
for row in start_row..=end_row {
let first = if row == start_row { start_col } else { 0 };
let last = if row == end_row {
end_col
} else {
pane.inner_rect.width.checked_sub(1)?
};
let cells = pane_surface_row(surface, pane, row)?
.get(usize::from(first)..=usize::from(last))?;
expected_cells.extend(cells.iter().map(|cell| cell.symbol.clone()));
}
Some(crate::api::schema::Method::PaneSelectionReadChecked(
crate::api::schema::PaneSelectionReadCheckedParams {
pane_id: selection.pane_id.clone(),
anchor: crate::api::schema::PaneTextPoint {
row: start_row,
col: start_col,
},
cursor: crate::api::schema::PaneTextPoint {
row: end_row,
col: end_col,
},
expected_cells,
},
))
})();
if let Some(method) = checked.filter(|method| self.supports_endpoint_method(method)) {
self.push_endpoint_method_with_kind(
method,
PendingEndpointKind::SelectionCopy,
outcome,
);
} else {
// Older endpoints and ranges beyond the retained surface still require an exact revision.
self.request_selection_copy(outcome);
}
}
pub(super) fn record_binding(
&mut self,
binding: crate::input::KeybindMatch,
@@ -245,22 +293,16 @@ impl ClientShellState {
}
pub(super) fn request_selection_copy(&mut self, outcome: &mut ClientShellInput) {
let content_revision = self.selection.as_ref().and_then(|selection| {
let Some(content_revision) = self.selection.as_ref().and_then(|selection| {
self.pane_surface
.as_ref()?
.panes
.iter()
.find(|pane| pane.pane_id == selection.pane_id)
.map(|pane| pane.content_revision)
});
self.request_selection_copy_at_revision(outcome, content_revision);
}
pub(super) fn request_selection_copy_at_revision(
&mut self,
outcome: &mut ClientShellInput,
content_revision: Option<u64>,
) {
}) else {
return;
};
let Some(selection) = self.selection.as_ref() else {
return;
};
@@ -278,7 +320,7 @@ impl ClientShellState {
row: cursor.0,
col: cursor.1,
},
content_revision,
content_revision: Some(content_revision),
},
),
PendingEndpointKind::SelectionCopy,
+1 -2
View File
@@ -520,8 +520,7 @@ impl ClientShellState {
.as_ref()
.is_some_and(crate::selection::Selection::is_visible)
{
// Unrelated live output must not invalidate a retained selection's copy.
self.request_selection_copy_at_revision(outcome, None);
self.request_retained_selection_copy(outcome);
self.selection = None;
self.stop_selection_autoscroll();
self.selection_highlight_clear_deadline = None;
+1 -1
View File
@@ -4,7 +4,7 @@ pub(super) const MIN_TAB_WIDTH: u16 = 8;
pub(super) const NEW_TAB_WIDTH: u16 = 3;
pub(super) const WORKSPACE_HEADER_ROWS: u16 = 2;
fn pane_surface_row<'a>(
pub(super) fn pane_surface_row<'a>(
surface: &'a PaneSurfaceFrame,
pane: &crate::protocol::PaneSurfacePane,
absolute_row: u32,
+64 -6
View File
@@ -218,13 +218,13 @@ async fn retained_mouse_selection_copies_only_on_exact_copy_shortcut() {
assert!(matches!(
&copy.actions[..],
[ClientShellAction::Endpoint { request, .. }]
if matches!(request.method, crate::api::schema::Method::PaneSelectionRead(_))
if matches!(request.method, crate::api::schema::Method::PaneSelectionReadChecked(_))
));
assert!(copy.requests.is_empty());
let ClientShellAction::Endpoint { request, .. } = &copy.actions[0] else {
unreachable!();
};
let crate::api::schema::Method::PaneSelectionRead(params) = &request.method else {
let crate::api::schema::Method::PaneSelectionReadChecked(params) = &request.method else {
unreachable!();
};
let (_tx, rx) = tokio::sync::mpsc::unbounded_channel();
@@ -246,16 +246,29 @@ async fn retained_mouse_selection_copies_only_on_exact_copy_shortcut() {
app.state.insert_test_runtime(pane_id, runtime);
let mut params = params.clone();
params.pane_id = app.public_pane_id(0, pane_id).expect("pane id");
let mut checked = params.clone();
checked.content_revision = Some(2);
let checked = crate::api::schema::PaneSelectionReadParams {
pane_id: params.pane_id.clone(),
anchor: params.anchor,
cursor: params.cursor,
content_revision: Some(2),
};
assert_eq!(
app.pane_selection_text(&checked).unwrap_err().0,
"stale_content"
);
let text = app
.pane_selection_text(&params)
.pane_selection_text_checked(&params)
.expect("copy during output");
assert_eq!(text, "LIV");
// Selected cells changing between the key and extraction must reject the copy.
app.state
.runtime_for_pane_in_workspace(&app.terminal_runtimes, 0, pane_id)
.expect("runtime")
.test_process_pty_bytes(b"\x1b[HOTHER");
assert_eq!(
app.pane_selection_text_checked(&params).unwrap_err().0,
"stale_content"
);
let (_, actions) = state.handle_endpoint_result(
"boot-1",
&request.id,
@@ -314,6 +327,51 @@ fn retained_selection_copy_failures_never_send_terminal_input() {
}
}
#[test]
fn retained_selection_copy_requires_revision_without_checked_read() {
for (offscreen, invalidate) in [(false, false), (true, false), (false, true)] {
let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default()));
state.config.copy_on_select = false;
state.set_snapshot(Box::new(snapshot()));
state.set_pane_surface(surface());
if !offscreen {
state.set_endpoint_methods(Some(vec!["pane.selection.read".into()]));
}
let mut selection = crate::selection::Selection::absolute_range(
"pane_1".to_owned(),
(0, 0),
(if offscreen { 2 } else { 0 }, 2),
);
selection.finish();
state.selection = Some(selection);
if invalidate {
state.invalidate_pane_surface();
}
let copy = state.handle_input_bytes(b"\x03");
if invalidate {
assert!(copy.actions.is_empty());
assert!(copy.requests.is_empty());
continue;
}
let [ClientShellAction::Endpoint { request, .. }] = &copy.actions[..] else {
panic!("copy request");
};
assert!(
matches!(&request.method, crate::api::schema::Method::PaneSelectionRead(params) if params.content_revision == Some(0))
);
let (_, actions) = state.handle_endpoint_result(
"boot-1",
&request.id,
Err(ClientShellEndpointError {
code: Some("stale_content".into()),
message: "pane content changed".into(),
}),
);
assert!(actions.is_empty());
assert!(copy.requests.is_empty());
}
}
#[test]
fn selection_edge_drag_requests_scroll_and_timer_continues_it() {
let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default()));
@@ -1002,7 +1060,7 @@ fn retained_selection_copy_suppresses_key_repeats() {
assert!(press.actions.iter().any(|action| matches!(
action,
ClientShellAction::Endpoint { request, .. }
if matches!(request.method, crate::api::schema::Method::PaneSelectionRead(_))
if matches!(request.method, crate::api::schema::Method::PaneSelectionReadChecked(_))
)));
let repeat = state.handle_raw_events(vec![RawInputEvent::Key(
key.with_kind(crossterm::event::KeyEventKind::Repeat),
+12
View File
@@ -3070,6 +3070,18 @@ impl PaneRuntime {
result
}
pub(crate) fn extract_selection_checked(
&self,
selection: &crate::selection::Selection,
expected_cells: &[String],
) -> Option<String> {
let result = self
.terminal
.extract_selection_checked(selection, expected_cells);
self.compression.wake();
result
}
pub fn render(&self, frame: &mut Frame, area: Rect, show_cursor: bool) {
self.terminal.render(frame, area, show_cursor);
}
+112 -1
View File
@@ -522,6 +522,15 @@ impl PaneTerminal {
self.ghostty.extract_selection(selection)
}
pub(crate) fn extract_selection_checked(
&self,
selection: &crate::selection::Selection,
expected_cells: &[String],
) -> Option<String> {
self.ghostty
.extract_selection_checked(selection, expected_cells)
}
pub fn render(&self, frame: &mut Frame, area: Rect, show_cursor: bool) {
self.ghostty.render(frame, area, show_cursor);
}
@@ -2182,6 +2191,54 @@ impl GhosttyPaneTerminal {
.and_then(|mut core| ghostty_extract_selection(&mut core, selection).ok())
}
pub(crate) fn extract_selection_checked(
&self,
selection: &crate::selection::Selection,
expected_cells: &[String],
) -> Option<String> {
// Compare and extract under one lock: output may change other cells, not this range.
let mut core = self.core.lock().ok()?;
let cols = core.terminal.cols().ok()?;
let ((start_row, start_col), (end_row, end_col)) = selection.ordered_cells();
let mut expected = expected_cells.iter();
for row in start_row..=end_row {
let first = if row == start_row { start_col } else { 0 };
let last = if row == end_row {
end_col
} else {
cols.checked_sub(1)?
};
for col in first..=last {
let expected_symbol = expected.next()?;
let (wide, graphemes) = core.terminal.screen_cell(col, row).ok()?;
let symbol = match wide {
crate::ghostty::CellWide::SpacerTail => String::new(),
crate::ghostty::CellWide::SpacerHead => " ".to_owned(),
_ => {
let text: String =
graphemes.into_iter().filter_map(char::from_u32).collect();
if text.is_empty()
|| (crate::kitty_graphics::is_enabled()
&& text.chars().next().map(u32::from)
== Some(crate::ghostty::KITTY_UNICODE_PLACEHOLDER))
{
" ".to_owned()
} else {
text
}
}
};
if ghostty_normalize_buffer_symbol(&symbol, wide) != *expected_symbol {
return None;
}
}
}
if expected.next().is_some() {
return None;
}
ghostty_extract_selection(&mut core, selection).ok()
}
pub fn visible_hyperlinks(&self, area: Rect) -> Vec<((u16, u16), String, String)> {
self.core
.lock()
@@ -2975,7 +3032,6 @@ pub(super) fn ghostty_blank_symbol_for_width(wide: crate::ghostty::CellWide) ->
}
}
#[cfg(test)]
pub(super) fn ghostty_normalize_buffer_symbol(
symbol: &str,
wide: crate::ghostty::CellWide,
@@ -5319,6 +5375,61 @@ mod tests {
assert_eq!(text, "000003\n000004\n000005");
}
#[test]
fn checked_selection_preserves_terminal_wrapping_and_wide_graphemes() {
for (input, start, end, expected, text) in [
(
"1ABCD2EFGH3IJKL",
(1, 0),
(2, 2),
vec!["2", "E", "F", "G", "H", "3", "I", "J"],
"2EFGH3IJ",
),
(
"abc\r\ndef",
(0, 0),
(1, 2),
vec!["a", "b", "c", " ", " ", "d", "e", "f"],
"abc\ndef",
),
(
"中e\u{301}!",
(0, 0),
(0, 3),
vec!["", "", "e\u{301}", "!"],
"中e\u{301}!",
),
] {
let (tx, _rx) = mpsc::channel(4);
let mut terminal = crate::ghostty::Terminal::new(5, 3, 0).unwrap();
terminal.write(input.as_bytes());
let pane = GhosttyPaneTerminal::new(terminal, tx).unwrap();
let selection =
crate::selection::Selection::absolute_range(PaneId::from_raw(1), start, end);
let expected: Vec<String> = expected.into_iter().map(str::to_owned).collect();
assert_eq!(
pane.extract_selection_checked(&selection, &expected)
.as_deref(),
Some(text)
);
assert!(pane
.extract_selection_checked(&selection, &expected[..expected.len() - 1])
.is_none());
let mut extra = expected.clone();
extra.push(" ".into());
assert!(pane.extract_selection_checked(&selection, &extra).is_none());
// Scrolling with no history replaces the absolute rows beneath the request.
pane.core
.lock()
.unwrap()
.terminal
.write(b"\r\nxxxxx\r\nyyyyy\r\nzzzzz");
assert!(pane
.extract_selection_checked(&selection, &expected)
.is_none());
}
}
#[test]
fn recent_reads_include_viewport_before_scrollback_exists() {
let (tx, _rx) = mpsc::channel(4);
+1
View File
@@ -29,6 +29,7 @@ const CLIENT_SHELL_METHODS: &[&str] = &[
"pane.resize",
"pane.scroll",
"pane.selection.read",
"pane.selection.read_checked",
"pane.split",
"pane.swap",
"pane.zoom",
+8
View File
@@ -407,6 +407,14 @@ impl TerminalRuntime {
self.0.extract_selection(selection)
}
pub(crate) fn extract_selection_checked(
&self,
selection: &crate::selection::Selection,
expected_cells: &[String],
) -> Option<String> {
self.0.extract_selection_checked(selection, expected_cells)
}
pub fn render(&self, frame: &mut Frame, area: Rect, show_cursor: bool) {
self.0.render(frame, area, show_cursor);
}
+1
View File
@@ -15,6 +15,7 @@
"pane.resize": "9fbcc70b8908c43f47b0ff85d0a9040ce18c9179a029ed7d8f2616a05d8c301d",
"pane.scroll": "3229d55230b89730e5231a43d87cefa98cfda24acaaf6365f7c6a3548cc42db1",
"pane.selection.read": "2f7b0fdf4f0fc1f1fe9ab4d962c833dd39a1e9bd866d77bbf0031f8b2c485534",
"pane.selection.read_checked": "7a831a263fb9beda5e287efd9189275a8a55ae6789e98a1a0993f9c3b75ee280",
"pane.split": "5d740322cac5287070194fb522c8135795a6f877b44652ea4959c63b4c67cefd",
"pane.swap": "3146d499d8174d73cfdfa3b6ca44f5d5d0951ab1cdd1808f94a3cf8846dc8057",
"pane.zoom": "0521e444ea892abb1f73d16dc0e10a5667c8a752284eae4cbc0961cd6d96bbfe",