From d9f143cdb349354929dd9cee65cc076cfb78206f Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 09:31:17 -0700 Subject: [PATCH] Marketplace gh login: tear down the container login on every failure A lost stream or a failed Enter write returned without killing the in-container gh, leaving it polling with a temp GH_CONFIG_DIR that would receive the token. The output loop is now drive_login (generic over the stream, writer and emitter, so it is unit-tested without Docker), and its result goes through cleanup_on_error, so every ending except a token read back runs the pkill. Neutral wording for the shared ANSI stripper's overflow warning. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/auth_token_commands.rs | 2 +- app/src-tauri/src/marketplace/gh_login.rs | 324 +++++++++++++++--- 2 files changed, 273 insertions(+), 53 deletions(-) diff --git a/app/src-tauri/src/commands/auth_token_commands.rs b/app/src-tauri/src/commands/auth_token_commands.rs index 6e057c5..c56545a 100644 --- a/app/src-tauri/src/commands/auth_token_commands.rs +++ b/app/src-tauri/src/commands/auth_token_commands.rs @@ -644,7 +644,7 @@ impl AnsiStripper { // fresh chunk, which re-enters here. if self.carry.len() > MAX_ANSI_CARRY { log::warn!( - "`claude setup-token` emitted an unterminated control sequence \ + "the command emitted an unterminated control sequence \ longer than {} bytes — treating it as text", MAX_ANSI_CARRY ); diff --git a/app/src-tauri/src/marketplace/gh_login.rs b/app/src-tauri/src/marketplace/gh_login.rs index 97c9ccf..22ca482 100644 --- a/app/src-tauri/src/marketplace/gh_login.rs +++ b/app/src-tauri/src/marketplace/gh_login.rs @@ -5,9 +5,10 @@ use std::time::Duration; -use futures_util::StreamExt; +use bollard::container::LogOutput; +use futures_util::{Stream, StreamExt}; use tauri::{AppHandle, Emitter}; -use tokio::io::AsyncWriteExt; +use tokio::io::{AsyncWrite, AsyncWriteExt}; use tokio::sync::oneshot; use crate::commands::auth_token_commands::{push_capped_tail, AnsiStripper, SUBMIT_ENTER_DELAY}; @@ -134,43 +135,49 @@ fn failure_tail(transcript: &str) -> String { lines[lines.len().saturating_sub(5)..].join("\n") } -/// Pre-flight N9: stop the in-container login after a cancel or timeout. +/// Pre-flight N9 / review fix 1: stop the in-container login after any +/// failed attempt. async fn kill_container_login(container_id: &str) { let cmd = CANCEL_PKILL.iter().map(|s| s.to_string()).collect(); let _ = exec_oneshot_as(container_id, "claude", cmd, vec![]).await; } -/// Run `gh auth login --web` in the container and return the token it minted. -pub async fn run_gh_container_login( - app: &AppHandle, - account_id: &str, - container_id: &str, - host: &str, - mut cancel: oneshot::Receiver<()>, -) -> Result { - if !valid_host(host) { - return Err(format!("{host:?} is not a valid host name.")); +/// Hand `result` back, running `cleanup` first when it is a failure. +/// +/// Review fix 1: a tty exec keeps running after its attach is dropped, so any +/// login that ends without a token — cancel, timeout, a lost stream, a failed +/// write, gh exiting without one — must stop the in-container login, or gh +/// keeps polling and its temp `GH_CONFIG_DIR` (which receives the token if the +/// user finishes in the browser) outlives the attempt. +async fn cleanup_on_error(result: Result, cleanup: C) -> Result +where + C: FnOnce() -> Fut, + Fut: std::future::Future, +{ + if result.is_err() { + cleanup().await; } - let AttachedExec { - exec_id, - mut output, - mut input, - } = create_attached_exec_as( - container_id, - vec![ - "sh".to_string(), - "-c".to_string(), - GH_LOGIN_SCRIPT.to_string(), - "triple-c-gh-login".to_string(), - host.to_string(), - ], - true, - "claude", - "/home/claude", - ) - .await?; + result +} - let deadline = tokio::time::Instant::now() + LOGIN_TIMEOUT; +/// Pump gh's output until the exec ends, emitting code and display events and +/// pressing Enter at gh's prompt. `Ok` is the transcript of a stream that ended +/// normally; every other ending is `Err`. Takes the attach halves by value, so +/// they are closed by the time this returns. +async fn drive_login( + mut output: S, + mut input: W, + cancel: &mut oneshot::Receiver<()>, + deadline: tokio::time::Instant, + account_id: &str, + host: &str, + mut emit: E, +) -> Result +where + S: Stream> + Unpin, + W: AsyncWrite + Unpin, + E: FnMut(&'static str, serde_json::Value), +{ let mut stripper = AnsiStripper::default(); let mut transcript = String::new(); let mut pending = String::new(); @@ -179,18 +186,12 @@ pub async fn run_gh_container_login( loop { let next = tokio::select! { - _ = &mut cancel => { - drop(output); - drop(input); - kill_container_login(container_id).await; + _ = &mut *cancel => { return Err("GitHub sign-in cancelled. Nothing was stored.".to_string()); } next = tokio::time::timeout_at(deadline, output.next()) => match next { Ok(next) => next, Err(_) => { - drop(output); - drop(input); - kill_container_login(container_id).await; return Err(format!( "Timed out after {} minutes waiting for the GitHub sign-in. Nothing was stored.", LOGIN_TIMEOUT.as_secs() / 60 @@ -205,21 +206,21 @@ pub async fn run_gh_container_login( "Lost the connection to gh: {e}. Nothing was stored." )) } - None => break, + None => return Ok(transcript), }; let text = stripper.push(&frame.into_bytes()); push_capped_tail(&mut transcript, &text, MAX_TRANSCRIPT); let shown = take_display_lines(&mut pending, &text); if !shown.is_empty() { - let _ = app.emit( + emit( OUTPUT_EVENT, serde_json::json!({ "account_id": account_id, "chunk": shown }), ); } if !code_sent { if let Some((code, url)) = parse_device_prompt(&transcript, host) { - let _ = app.emit( + emit( CODE_EVENT, serde_json::json!({ "account_id": account_id, "code": code, "url": url }), ); @@ -238,18 +239,70 @@ pub async fn run_gh_container_login( enter_sent = true; } } +} - let status = wait_for_exec_exit(&exec_id).await; - if let Some(token) = extract_token(&transcript) { - return Ok(token); +/// Run `gh auth login --web` in the container and return the token it minted. +/// +/// Once the exec exists there is exactly one way out: the result of the inner +/// block goes through [`cleanup_on_error`], so only a token read back skips +/// the in-container kill. +pub async fn run_gh_container_login( + app: &AppHandle, + account_id: &str, + container_id: &str, + host: &str, + mut cancel: oneshot::Receiver<()>, +) -> Result { + if !valid_host(host) { + return Err(format!("{host:?} is not a valid host name.")); } - Err(format!( - "gh did not complete the sign-in (exit status {}). Nothing was stored.\n{}", - status - .map(|c| c.to_string()) - .unwrap_or_else(|| "unknown".to_string()), - failure_tail(&transcript) - )) + let AttachedExec { + exec_id, + output, + input, + } = create_attached_exec_as( + container_id, + vec![ + "sh".to_string(), + "-c".to_string(), + GH_LOGIN_SCRIPT.to_string(), + "triple-c-gh-login".to_string(), + host.to_string(), + ], + true, + "claude", + "/home/claude", + ) + .await?; + + let deadline = tokio::time::Instant::now() + LOGIN_TIMEOUT; + let result = async { + let transcript = drive_login( + output, + input, + &mut cancel, + deadline, + account_id, + host, + |event, payload| { + let _ = app.emit(event, payload); + }, + ) + .await?; + if let Some(token) = extract_token(&transcript) { + return Ok(token); + } + let status = wait_for_exec_exit(&exec_id).await; + Err(format!( + "gh did not complete the sign-in (exit status {}). Nothing was stored.\n{}", + status + .map(|c| c.to_string()) + .unwrap_or_else(|| "unknown".to_string()), + failure_tail(&transcript) + )) + } + .await; + cleanup_on_error(result, || kill_container_login(container_id)).await } #[cfg(test)] @@ -378,6 +431,173 @@ mod tests { assert!(GH_LOGIN_SCRIPT.contains(CANCEL_PKILL[2])); } + /// Review fix 1: every failed login tears the container side down, and a + /// successful one does not. + mod teardown { + use super::super::*; + use bollard::container::LogOutput; + use futures_util::stream; + use std::pin::Pin; + use std::sync::{Arc, Mutex}; + use std::task::{Context, Poll}; + + type Frame = Result; + + fn out(s: &'static str) -> Frame { + Ok(LogOutput::StdOut { message: s.into() }) + } + + fn lost() -> Frame { + Err(bollard::errors::Error::DockerResponseServerError { + status_code: 500, + message: "connection reset".to_string(), + }) + } + + /// Records every write separately; or fails every write. + #[derive(Clone, Default)] + struct Keys { + writes: Arc>>>, + broken: bool, + } + + impl tokio::io::AsyncWrite for Keys { + fn poll_write( + self: Pin<&mut Self>, + _: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + if self.broken { + return Poll::Ready(Err(std::io::Error::other("pipe closed"))); + } + self.writes.lock().unwrap().push(buf.to_vec()); + Poll::Ready(Ok(buf.len())) + } + fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + fn poll_shutdown( + self: Pin<&mut Self>, + _: &mut Context<'_>, + ) -> Poll> { + Poll::Ready(Ok(())) + } + } + + const PROMPT: &str = "! First copy your one-time code: 4F2A-9C1B\r\nPress Enter to open github.com in your browser... "; + + async fn drive( + frames: S, + keys: Keys, + cancel: &mut oneshot::Receiver<()>, + deadline: tokio::time::Instant, + ) -> ( + Result, + Vec<(&'static str, serde_json::Value)>, + ) + where + S: futures_util::Stream + Unpin, + { + let mut events = Vec::new(); + let r = drive_login( + frames, + keys, + cancel, + deadline, + "acct-1", + "github.com", + |e, p| events.push((e, p)), + ) + .await; + (r, events) + } + + fn far() -> tokio::time::Instant { + tokio::time::Instant::now() + LOGIN_TIMEOUT + } + + #[tokio::test] + async fn cleanup_runs_on_every_failure_and_never_on_success() { + let runs = Arc::new(Mutex::new(0)); + let count = || { + let runs = runs.clone(); + async move { *runs.lock().unwrap() += 1 } + }; + let ok: Result = Ok("test-token-not-real".into()); + assert!(cleanup_on_error(ok, count).await.is_ok()); + assert_eq!(*runs.lock().unwrap(), 0); + let err: Result = Err("boom".into()); + assert_eq!(cleanup_on_error(err, count).await, Err("boom".into())); + assert_eq!(*runs.lock().unwrap(), 1); + } + + #[tokio::test(start_paused = true)] + async fn a_complete_login_returns_the_transcript_and_presses_enter_alone() { + let keys = Keys::default(); + let (_tx, mut cancel) = oneshot::channel(); + let frames = stream::iter(vec![ + out(PROMPT), + out("\r\n\u{2713} Logged in\r\n"), + out("__TRIPLEC_TOKEN_BEGIN__test-token-"), + out("not-real__TRIPLEC_TOKEN_END__\r\n"), + ]); + let (r, events) = drive(frames, keys.clone(), &mut cancel, far()).await; + let transcript = r.unwrap(); + assert_eq!( + extract_token(&transcript), + Some("test-token-not-real".into()) + ); + assert_eq!(*keys.writes.lock().unwrap(), vec![b"\r".to_vec()]); + assert!(events.contains(&( + CODE_EVENT, + serde_json::json!({ + "account_id": "acct-1", + "code": "4F2A-9C1B", + "url": "https://github.com/login/device" + }) + ))); + for (_, payload) in &events { + assert!(!payload.to_string().contains("test-token-not-real")); + } + } + + #[tokio::test] + async fn a_lost_stream_is_a_failure() { + let (_tx, mut cancel) = oneshot::channel(); + let frames = stream::iter(vec![out(PROMPT), lost()]); + let (r, _) = drive(frames, Keys::default(), &mut cancel, far()).await; + assert!(r.unwrap_err().contains("Lost the connection")); + } + + #[tokio::test(start_paused = true)] + async fn a_failed_enter_is_a_failure() { + let keys = Keys { + broken: true, + ..Default::default() + }; + let (_tx, mut cancel) = oneshot::channel(); + let frames = stream::iter(vec![out(PROMPT)]); + let (r, _) = drive(frames, keys, &mut cancel, far()).await; + assert!(r.unwrap_err().contains("Could not answer")); + } + + #[tokio::test] + async fn a_cancel_is_a_failure() { + let (tx, mut cancel) = oneshot::channel(); + tx.send(()).unwrap(); + let (r, _) = drive(stream::pending(), Keys::default(), &mut cancel, far()).await; + assert!(r.unwrap_err().contains("cancelled")); + } + + #[tokio::test(start_paused = true)] + async fn a_timeout_is_a_failure() { + let (_tx, mut cancel) = oneshot::channel(); + let deadline = tokio::time::Instant::now() + Duration::from_secs(1); + let (r, _) = drive(stream::pending(), Keys::default(), &mut cancel, deadline).await; + assert!(r.unwrap_err().contains("Timed out")); + } + } + /// The script end to end against a stand-in `gh`, as a login would run it /// inside the container (minus Docker). #[cfg(unix)]