From f92ee52f03db977451506cf8c975be53fc559512 Mon Sep 17 00:00:00 2001 From: shihuili1218 Date: Thu, 3 Sep 2026 21:38:55 +0800 Subject: [PATCH 1/3] review: plugin follow-ups from bot review of #266 - ensure_zip_b64_within_cap now counts '=' padding: 10 MiB % 3 == 1, so a cap-sized zip encodes with '==' and the old estimate rejected a legal payload by up to 2 bytes (contradicting its own 'exact' claim) - headless dispatcher passes the area argument through to install_impl; hardcoding None silently skipped the region-button mismatch validation - isExecPayload rejects NaN/Infinity/non-positive timeoutMs before it hits the invoke boundary (where NaN serializes to null) - manager hover-revealed actions also appear on :focus-within - AppShell comment now matches behavior: disconnect keeps iframes mounted but hides the region; it never showed a plugin-side disconnected state Not taken: movePluginTo reorder index (code, comment, test and the moveTab primitive all agree on take-the-target's-slot semantics), iframe self-navigation hardening (no capability gain over what an installed plugin can already do), keyboard reorder and per-tab size keying (deferred). --- src-tauri/src/commands/plugin.rs | 49 ++++++++++++++++++++++------ src-tauri/src/server.rs | 12 ++++++- src/lib/components/AppShell.svelte | 6 ++-- src/lib/plugins/PluginManager.svelte | 6 +++- src/lib/plugins/bridge.test.ts | 6 ++++ src/lib/plugins/bridge.ts | 5 ++- 6 files changed, 69 insertions(+), 15 deletions(-) diff --git a/src-tauri/src/commands/plugin.rs b/src-tauri/src/commands/plugin.rs index e3ae3590..4961b51c 100644 --- a/src-tauri/src/commands/plugin.rs +++ b/src-tauri/src/commands/plugin.rs @@ -530,16 +530,29 @@ pub fn plugins_root(state: State<'_, AppState>) -> AppResult { /// Reject encoded input that could decode beyond MAX_ZIP_BYTES — decoding /// first would allocate the whole payload in memory. Call before decode. pub fn ensure_zip_b64_within_cap(encoded: &str) -> AppResult<()> { - // Decoded upper bound: 3 bytes per full 4-char group plus the 1-2 bytes - // of a trailing partial group (padding decodes to nothing). Exact, so it - // agrees with install_impl's check on the decoded bytes. - let len = encoded.trim().len(); - let decoded = len / 4 * 3 - + match len % 4 { - 2 => 1, - 3 => 2, - _ => 0, - }; + // Exact decoded length: 3 bytes per full 4-char group, 1-2 bytes for an + // unpadded tail group, minus trailing '=' (they decode to nothing), so + // the estimate agrees with install_impl's check on the decoded bytes. + // Padding matters: 10 MiB % 3 == 1, so a cap-sized payload ends in "==" + // and ignoring that overcounts by 2 bytes and rejects a legal zip. + let s = encoded.trim(); + let len = s.len(); + let mut decoded = len / 4 * 3; + match len % 4 { + 2 => decoded += 1, + 3 => decoded += 2, + _ => {} + } + // At most the final group pads; saturate so garbage like "====" cannot + // underflow (the caller's decode rejects it anyway). + let pad = s + .as_bytes() + .iter() + .rev() + .take(2) + .take_while(|&b| *b == b'=') + .count(); + decoded = decoded.saturating_sub(pad); if decoded > MAX_ZIP_BYTES { return Err(AppError::config( "plugin_zip_invalid", @@ -676,6 +689,22 @@ mod tests { zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap() } + #[test] + fn zip_b64_cap_accepts_padded_input_at_exactly_the_cap() { + use base64::{engine::general_purpose::STANDARD, Engine}; + // 10 MiB % 3 == 1, so a cap-sized payload encodes with "==" padding; + // ignoring the padding overcounted by 2 and rejected a legal zip. + let encoded = STANDARD.encode(vec![b'x'; MAX_ZIP_BYTES]); + assert!(ensure_zip_b64_within_cap(&encoded).is_ok()); + } + + #[test] + fn zip_b64_cap_rejects_one_byte_over() { + use base64::{engine::general_purpose::STANDARD, Engine}; + let encoded = STANDARD.encode(vec![b'x'; MAX_ZIP_BYTES + 1]); + assert!(ensure_zip_b64_within_cap(&encoded).is_err()); + } + #[cfg(desktop)] #[tokio::test] async fn local_exec_runs_command_and_captures_output() { diff --git a/src-tauri/src/server.rs b/src-tauri/src/server.rs index 90986ca6..29576915 100644 --- a/src-tauri/src/server.rs +++ b/src-tauri/src/server.rs @@ -336,6 +336,12 @@ fn dispatch( "install_plugin" => { use base64::{engine::general_purpose::STANDARD, Engine}; let b64: String = arg(&args, "base64Zip")?; + // Same region-button contract as the Tauri command — passing None + // here would silently skip the area mismatch validation. + let area: Option = match args.get("area") { + Some(serde_json::Value::String(s)) => Some(s.clone()), + _ => None, + }; crate::commands::plugin::ensure_zip_b64_within_cap(&b64).map_err(err_value)?; let bytes = STANDARD.decode(b64.trim()).map_err(|e| { err_value(AppError::config( @@ -343,7 +349,11 @@ fn dispatch( json!({ "err": e.to_string() }), )) })?; - ok(crate::commands::plugin::install_impl(state, &bytes, None)) + ok(crate::commands::plugin::install_impl( + state, + &bytes, + area.as_deref(), + )) } "list_plugins" => ok(crate::db::plugin::list(&state.db)), "set_plugin_enabled" => { diff --git a/src/lib/components/AppShell.svelte b/src/lib/components/AppShell.svelte index ca41f0aa..d3cbe32c 100644 --- a/src/lib/components/AppShell.svelte +++ b/src/lib/components/AppShell.svelte @@ -439,8 +439,10 @@ .filter((tab) => open(tab.id) && (tab.type === "ssh" || tab.type === "local")) - // sessionId 可为空串：断线时 iframe 保活（图表不丢），exec 报 - // plugin_no_exec 由插件显示断连态；重连后 sessionId 恢复。 + // sessionId may be "" on a disconnected tab: the tab stays listed + // so its iframes stay mounted (charts survive the drop; exec would + // answer plugin_no_exec), but pluginAreaActive hides the region + // itself until the session comes back. .map((tab) => ({tabId: tab.id, sessionId: app.sessionIdForTab(tab.id) ?? ""})); } let pluginSideTabs = $derived(pluginTabsFor(plugins.isSideOpen)); diff --git a/src/lib/plugins/PluginManager.svelte b/src/lib/plugins/PluginManager.svelte index b728ae39..92e0d7d4 100644 --- a/src/lib/plugins/PluginManager.svelte +++ b/src/lib/plugins/PluginManager.svelte @@ -644,8 +644,12 @@ opacity: 0; transition: opacity 0.12s; } + /* focus-within: the hover-revealed enable/uninstall controls must also be + visible when a keyboard user tabs into them. */ .block:hover .cell-hover, - .seg:hover .cell-hover { + .block:focus-within .cell-hover, + .seg:hover .cell-hover, + .seg:focus-within .cell-hover { opacity: 1; } .cell-label { diff --git a/src/lib/plugins/bridge.test.ts b/src/lib/plugins/bridge.test.ts index 181d02a4..1f96dc8e 100644 --- a/src/lib/plugins/bridge.test.ts +++ b/src/lib/plugins/bridge.test.ts @@ -54,6 +54,12 @@ describe("isPluginRequest", () => { expect(isPluginRequest(req({payload: {command: "ls", timeoutMs: "3000"}}))).toBe(false); }); + it("rejects non-finite and non-positive timeoutMs", () => { + for (const timeoutMs of [Number.NaN, Number.POSITIVE_INFINITY, 0, -3000]) + expect(isPluginRequest(req({payload: {command: "ls", timeoutMs}}))).toBe(false); + expect(isPluginRequest(req({payload: {command: "ls", timeoutMs: 3000}}))).toBe(true); + }); + it("rejects non-object payloads", () => { expect(isPluginRequest(req({payload: null}))).toBe(false); expect(isPluginRequest("exec")).toBe(false); diff --git a/src/lib/plugins/bridge.ts b/src/lib/plugins/bridge.ts index 07897601..97949583 100644 --- a/src/lib/plugins/bridge.ts +++ b/src/lib/plugins/bridge.ts @@ -68,7 +68,10 @@ function isExecPayload(p: unknown): boolean { const req = p as Record; if (typeof req.command !== "string" || req.command.length === 0) return false; if (req.command.length > MAX_COMMAND_LENGTH) return false; - if (req.timeoutMs !== undefined && typeof req.timeoutMs !== "number") return false; + // Finite and positive: NaN/Infinity serialize to null at the invoke boundary, + // and a non-positive timeout is meaningless (the backend clamps the rest). + if (req.timeoutMs !== undefined && !(Number.isFinite(req.timeoutMs) && req.timeoutMs > 0)) + return false; return true; } From ae3f7321475f3b13ad7c3ed4e4a4b168ca0b1c8d Mon Sep 17 00:00:00 2001 From: shihuili1218 Date: Thu, 3 Sep 2026 22:00:10 +0800 Subject: [PATCH 2/3] =?UTF-8?q?review:=20address=20#267=20feedback=20?= =?UTF-8?q?=E2=80=94=20type-safe=20timeout=20check,=20reuse=20optional=5Fs?= =?UTF-8?q?tring=5Farg?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - isExecPayload compared an `unknown` with `>`, which does not type-check (svelte-check: possibly null / operator cannot be applied). A typeof guard narrows it; the short-circuit already made the runtime Symbol claim moot - the headless dispatcher's hand-rolled area match reinvented the existing optional_string_arg helper; wrong-typed values now error instead of silently skipping the area validation Not taken: constructing the cap-test base64 strings without allocating — the tests deliberately pin the real encoder path (STANDARD.encode of a cap-sized payload), and a few transient MiB in a unit test is nothing. --- src-tauri/src/server.rs | 5 +---- src/lib/plugins/bridge.test.ts | 2 +- src/lib/plugins/bridge.ts | 5 ++++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/server.rs b/src-tauri/src/server.rs index 29576915..3541fbf2 100644 --- a/src-tauri/src/server.rs +++ b/src-tauri/src/server.rs @@ -338,10 +338,7 @@ fn dispatch( let b64: String = arg(&args, "base64Zip")?; // Same region-button contract as the Tauri command — passing None // here would silently skip the area mismatch validation. - let area: Option = match args.get("area") { - Some(serde_json::Value::String(s)) => Some(s.clone()), - _ => None, - }; + let area = optional_string_arg(&args, "area")?; crate::commands::plugin::ensure_zip_b64_within_cap(&b64).map_err(err_value)?; let bytes = STANDARD.decode(b64.trim()).map_err(|e| { err_value(AppError::config( diff --git a/src/lib/plugins/bridge.test.ts b/src/lib/plugins/bridge.test.ts index 1f96dc8e..1c96ea42 100644 --- a/src/lib/plugins/bridge.test.ts +++ b/src/lib/plugins/bridge.test.ts @@ -55,7 +55,7 @@ describe("isPluginRequest", () => { }); it("rejects non-finite and non-positive timeoutMs", () => { - for (const timeoutMs of [Number.NaN, Number.POSITIVE_INFINITY, 0, -3000]) + for (const timeoutMs of [Number.NaN, Number.POSITIVE_INFINITY, 0, -3000, null]) expect(isPluginRequest(req({payload: {command: "ls", timeoutMs}}))).toBe(false); expect(isPluginRequest(req({payload: {command: "ls", timeoutMs: 3000}}))).toBe(true); }); diff --git a/src/lib/plugins/bridge.ts b/src/lib/plugins/bridge.ts index 97949583..2fa47280 100644 --- a/src/lib/plugins/bridge.ts +++ b/src/lib/plugins/bridge.ts @@ -70,7 +70,10 @@ function isExecPayload(p: unknown): boolean { if (req.command.length > MAX_COMMAND_LENGTH) return false; // Finite and positive: NaN/Infinity serialize to null at the invoke boundary, // and a non-positive timeout is meaningless (the backend clamps the rest). - if (req.timeoutMs !== undefined && !(Number.isFinite(req.timeoutMs) && req.timeoutMs > 0)) + // typeof narrows the unknown so the comparison type-checks; Number.isFinite + // never throws or coerces, so symbols/bigints fail the typeof check instead. + const timeoutMs = req.timeoutMs; + if (timeoutMs !== undefined && (typeof timeoutMs !== "number" || !Number.isFinite(timeoutMs) || timeoutMs <= 0)) return false; return true; } From d8579785cbfa9cf997410f85128134a4568a3073 Mon Sep 17 00:00:00 2001 From: shihuili1218 Date: Thu, 3 Sep 2026 22:25:51 +0800 Subject: [PATCH 3/3] =?UTF-8?q?review:=20require=20an=20integer=20timeoutM?= =?UTF-8?q?s=20=E2=80=94=20backend=20reads=20u64=20on=20both=20transports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fractional timeout (3000.5) passed the finite/positive check, then errored as invalid args on Tauri (Option) while headless's Value::as_u64 silently turned it into the default timeout. Number .isSafeInteger at the bridge gives both transports one contract. --- src/lib/plugins/bridge.test.ts | 4 ++-- src/lib/plugins/bridge.ts | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/lib/plugins/bridge.test.ts b/src/lib/plugins/bridge.test.ts index 1c96ea42..993d996e 100644 --- a/src/lib/plugins/bridge.test.ts +++ b/src/lib/plugins/bridge.test.ts @@ -54,8 +54,8 @@ describe("isPluginRequest", () => { expect(isPluginRequest(req({payload: {command: "ls", timeoutMs: "3000"}}))).toBe(false); }); - it("rejects non-finite and non-positive timeoutMs", () => { - for (const timeoutMs of [Number.NaN, Number.POSITIVE_INFINITY, 0, -3000, null]) + it("rejects non-finite, fractional and non-positive timeoutMs", () => { + for (const timeoutMs of [Number.NaN, Number.POSITIVE_INFINITY, 0, -3000, null, 3000.5]) expect(isPluginRequest(req({payload: {command: "ls", timeoutMs}}))).toBe(false); expect(isPluginRequest(req({payload: {command: "ls", timeoutMs: 3000}}))).toBe(true); }); diff --git a/src/lib/plugins/bridge.ts b/src/lib/plugins/bridge.ts index 2fa47280..e4e51fa6 100644 --- a/src/lib/plugins/bridge.ts +++ b/src/lib/plugins/bridge.ts @@ -68,12 +68,13 @@ function isExecPayload(p: unknown): boolean { const req = p as Record; if (typeof req.command !== "string" || req.command.length === 0) return false; if (req.command.length > MAX_COMMAND_LENGTH) return false; - // Finite and positive: NaN/Infinity serialize to null at the invoke boundary, - // and a non-positive timeout is meaningless (the backend clamps the rest). - // typeof narrows the unknown so the comparison type-checks; Number.isFinite - // never throws or coerces, so symbols/bigints fail the typeof check instead. + // A safe positive integer: the backend reads u64 on both transports, so a + // fractional value would error on Tauri but silently become the default + // timeout on headless — one contract at the bridge. typeof narrows the + // unknown so the comparison type-checks; isSafeInteger never throws or + // coerces, so symbols/bigints fail the typeof check instead. const timeoutMs = req.timeoutMs; - if (timeoutMs !== undefined && (typeof timeoutMs !== "number" || !Number.isFinite(timeoutMs) || timeoutMs <= 0)) + if (timeoutMs !== undefined && (typeof timeoutMs !== "number" || !Number.isSafeInteger(timeoutMs) || timeoutMs <= 0)) return false; return true; }