diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt index 49828e1..a496e19 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -285,8 +285,18 @@ data class Importable( val inUse: String, ) +/** + * What a machine has that could be continued. + * + * The slowest call this app makes, and it was the only expensive one left on the 5 second default — + * which is how it came to time out against a server that was answering perfectly well. Listing + * means reading every transcript Claude Code has ever written: about four seconds against a + * gigabyte of them before the tunnel adds anything, and that figure grows with every session + * anybody has. A timeout is for a server that has stopped answering, so it is set well clear of how + * long the work takes rather than just above it. + */ fun fetchImportable(settings: ServerSettings, setup: String): List = - requestFromServer(settings, "/setups/$setup/importable") { + requestFromServer(settings, "/setups/$setup/importable", readTimeoutMs = 60000) { it.jsonObjects { session -> Importable( id = session.getString("id"), diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt index 01f9d36..67786cb 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt @@ -19,8 +19,6 @@ import androidx.compose.ui.graphics.ColorMatrix import androidx.compose.ui.graphics.Paint import androidx.compose.ui.graphics.drawscope.drawIntoCanvas import androidx.compose.ui.graphics.graphicsLayer -import androidx.compose.ui.input.pointer.PointerEventPass -import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.unit.dp /** @@ -36,29 +34,19 @@ import androidx.compose.ui.unit.dp * a *word* because a spinner alone cannot say which operation this is — deleting and importing are * different in kind, and losing a session to the wrong one is not recoverable by waiting. * - * Inert by consuming pointer events above the content rather than by asking every caller to disable - * its own click handler: the row is covered, so there is nothing left to remember. + * It does **not** make the row inert; the caller disables its own click handling while it passes a + * label. That was the other way round at first — an overlay consuming pointer events, so no caller + * had to remember — and it swallowed the drag along with the tap, which meant a list could not be + * scrolled while anything in it was busy. Consuming taps but not drags means re-deciding what a + * gesture is above the components that already decide it; disabling the click is the platform's own + * answer and leaves the scroll where it belongs. */ @Composable fun BusyItem(label: String?, content: @Composable () -> Unit) { Box { Box(Modifier.busy(label != null)) { content() } if (label != null) { - Box( - Modifier.matchParentSize().pointerInput(Unit) { - // Consumed on the initial pass, so nothing underneath sees the gesture at - // all -- a press that produced a ripple on a row that cannot be pressed - // would say the opposite of everything else here. - awaitPointerEventScope { - while (true) { - awaitPointerEvent(PointerEventPass.Initial).changes.forEach { - it.consume() - } - } - } - }, - contentAlignment = Alignment.Center, - ) { + Box(Modifier.matchParentSize(), contentAlignment = Alignment.Center) { Row(verticalAlignment = Alignment.CenterVertically) { CircularProgressIndicator( modifier = Modifier.width(16.dp).height(16.dp), diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt index eb789ee..a2101fd 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt @@ -455,6 +455,11 @@ private fun ImportableList( Modifier.fillMaxWidth() .padding(vertical = 4.dp) .combinedClickable( + // Off while something is happening to this row -- + // see [BusyItem], which draws that but deliberately + // leaves the gestures alone so the list still + // scrolls. + enabled = running[session.id] == null, onClick = { if (settling(session.id)) return@combinedClickable // In selection mode a tap is a selection, so the diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt index 775e1ad..d0476c8 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt @@ -279,7 +279,15 @@ private fun SessionCard( ) { BusyItem(label = if (deleting) "deleting" else null) { Card( - Modifier.fillMaxWidth().combinedClickable(onClick = onOpen, onLongClick = onLongPress) + // Off while the delete is in flight: a card that still opens a session it is + // deleting is a race the reader can start by tapping. On the card rather than in + // [BusyItem], which leaves gestures alone so the list still scrolls. + Modifier.fillMaxWidth() + .combinedClickable( + enabled = !deleting, + onClick = onOpen, + onLongClick = onLongPress, + ) ) { Column(Modifier.padding(16.dp)) { Row( diff --git a/server/src/session/import.rs b/server/src/session/import.rs index fb69684..7513db8 100644 --- a/server/src/session/import.rs +++ b/server/src/session/import.rs @@ -17,7 +17,7 @@ //! reason: an enrolled token must not be able to turn into "read me this //! arbitrary path". -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail, ensure}; use serde::Serialize; use serde_json::Value; @@ -618,19 +618,63 @@ fn push_assistant(events: &mut Vec, content: &Value) { /// chance of resuming that conversation, including from an ai-app session /// that was already importing it. pub async fn delete(transport: &Transport, id: &str) -> Result<()> { - let found = list(transport).await?; - let chosen = found - .into_iter() - .find(|candidate| candidate.id == id) - .with_context(|| format!("no Claude Code session {id} on that machine"))?; - let launch = Launch::new("rm", vec!["-f".to_string(), chosen.path.clone()], None); - transport + // The file name *is* the id, so the machine can find it by name. This + // used to call `list` and search its output, which is correct and costs + // a full read of every transcript on the machine -- around four seconds + // against a gigabyte of them, per delete, so a batch of ten took the + // best part of a minute doing nothing but re-reading the same files. + // `context_of` below already resolved an id the cheap way; this is the + // same lookup, and the two now agree. + ensure!(is_session_id(id), "not a Claude Code session id: {id}"); + let script = r#" +for f in "$HOME"/.claude/projects/*/"$1".jsonl; do + [ -f "$f" ] || continue + rm -f "$f" || exit 1 + printf '%s\n' "$f" + exit 0 +done +"#; + let launch = Launch::new( + "sh", + vec![ + "-c".to_string(), + script.to_string(), + "sh".to_string(), + id.to_string(), + ], + None, + ); + // Nothing on stdout means the loop found no such file. Said here rather + // than by exiting non-zero, because a non-zero exit is reported as the + // machine being unreachable -- which is a different thing from the + // session not being there, and only one of them is worth retrying. + let removed = transport .capture(&launch) .await - .with_context(|| format!("deleting {}", chosen.path))?; + .with_context(|| format!("deleting Claude Code session {id}"))?; + if removed.trim().is_empty() { + bail!("no Claude Code session {id} on that machine"); + } Ok(()) } +/// Whether an id is one of ours to put in a shell glob. +/// +/// Both places that resolve an id to a file interpolate it into +/// `$HOME/.claude/projects/*/"$1".jsonl`. That is an argument rather than +/// script text, so a shell cannot be talked into running something -- but a +/// `/` or a `..` inside it still walks the glob out of the directory the id +/// is supposed to name. [`delete`] is where that would be fatal, because it +/// removes whatever it lands on, and it is exactly the reason `delete` used +/// to resolve ids by searching a listing instead. +/// +/// Claude Code names each transcript with a uuid, so hex and dashes is the +/// whole alphabet. Refused rather than escaped: an id that is not one of +/// these did not come from the list this app showed. +fn is_session_id(id: &str) -> bool { + !id.is_empty() && id.len() <= 64 && id.bytes().all(|b| b.is_ascii_hexdigit() || b == b'-') +} + /// How often an imported session checks whether its source file grew. /// /// A poll rather than a watch, because the file may be on another machine @@ -709,6 +753,12 @@ pub async fn line_count(transport: &Transport, path: &str) -> Result { /// Not knowing is a state the status row draws, so there is nothing to be /// gained by inventing a number here. pub async fn context_of(transport: &Transport, session_id: &str) -> Option { + // The same guard `delete` explains, applied to the other member of the + // set: this one only reads, but a glob that can leave the directory is + // worth closing in both places rather than in the dangerous one only. + if !is_session_id(session_id) { + return None; + } // The id crosses as an argument rather than as script text: it comes // from the CLI, but it reaches a shell on a machine that may not be // this one, and the rule there is that data never becomes syntax. @@ -756,6 +806,31 @@ pub async fn replay_after( mod tests { use super::*; + /// The guard on the only thing this module ever puts in a glob. + /// + /// Worth a test of its own because what it protects is a `rm`: `delete` + /// resolves an id straight to `$HOME/.claude/projects/*/"$1".jsonl`, so + /// an id that can contain a slash or a `..` is an id that can name a + /// file outside the directory and have it removed. + #[test] + fn a_session_id_cannot_walk_out_of_the_projects_directory() { + assert!(is_session_id("5ecf21da-d53f-4a11-9c0d-000000000100")); + assert!(is_session_id("deadbeef")); + + assert!(!is_session_id("../../../etc/passwd")); + assert!(!is_session_id("a/b")); + assert!(!is_session_id("..")); + assert!(!is_session_id("a.b")); + assert!(!is_session_id("a*")); + assert!(!is_session_id("a b")); + // Empty would glob to the directory itself, and a long one is not a + // uuid whatever else it is. + assert!(!is_session_id("")); + assert!(!is_session_id(&"a".repeat(65))); + } + + use super::*; + /// A 1x1 PNG, base64 -- the smallest thing with a real header. const PNG: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk\ YPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==";