From 36c2b1a3e326245a63ae7f65460dc5da1891c7f6 Mon Sep 17 00:00:00 2001 From: kipavy Date: Wed, 19 Aug 2026 10:40:59 +0000 Subject: [PATCH 1/3] fix(deep-links): report plugin integrity failures as tampering, not a bad link A plugin-install confirm sheet surfaced every accept failure as the generic "deep link install failed", so PluginHashMismatchError and MinAppVersionError were misreported. A hash mismatch is the user's only tamper signal. Extract pluginInstallErrorMessage(e, t, fallbackKey), matched on error type rather than message, and use it from both the settings install path and the confirm sheet via a new optional errorMessage on ConfirmSpec. Also add explicit .disabled assertions to the confirm-sheet negative tests: they clicked an already-disabled accept button, so the "installs/invites nobody" assertions could not fail. --- .../settings/sections/PluginsSection.tsx | 10 ++--- .../terminal/DeepLinkConfirmModal.test.tsx | 41 +++++++++++++++++-- .../terminal/DeepLinkConfirmModal.tsx | 14 +++++-- .../terminal/deepLinkConfirmSpecs.tsx | 10 +++++ src/plugins/installErrors.ts | 16 ++++++++ 5 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 src/plugins/installErrors.ts diff --git a/src/components/settings/sections/PluginsSection.tsx b/src/components/settings/sections/PluginsSection.tsx index 0b696ec33..155541909 100644 --- a/src/components/settings/sections/PluginsSection.tsx +++ b/src/components/settings/sections/PluginsSection.tsx @@ -7,8 +7,8 @@ import { usePluginRegistryStore } from "@/stores/pluginRegistryStore"; import { useMarketplaceStore, type MarketplacePlugin } from "@/stores/marketplaceStore"; import { useUIStore } from "@/stores/uiStore"; import { useNotificationStore } from "@/stores/notificationStore"; -import { PluginHashMismatchError } from "@/plugins/integrity"; -import { satisfiesMinAppVersion, MinAppVersionError } from "@/plugins/version"; +import { pluginInstallErrorMessage } from "@/plugins/installErrors"; +import { satisfiesMinAppVersion } from "@/plugins/version"; import { availableUpdate, availableSeededUpdate, addedPermissions } from "@/plugins/updates"; import { mergeBrowseCatalog, seededActiveIds as computeSeededActiveIds } from "@/plugins/floor"; import { useSeededTombstoneStore, loadSeededEntries, type SeededEntry } from "@/stores/seededTombstoneStore"; @@ -183,11 +183,7 @@ function usePluginInstaller() { source: { kind: "plugin", id: "system", name: "Voltius" }, type: "toast", severity: "error", - message: e instanceof PluginHashMismatchError - ? t("settings.plugins.install.integrityFailed") - : e instanceof MinAppVersionError - ? t("settings.plugins.install.versionUnsupported", { version: e.required }) - : t("settings.plugins.install.failed"), + message: pluginInstallErrorMessage(e, t, "settings.plugins.install.failed"), duration: 0, }); }; diff --git a/src/components/terminal/DeepLinkConfirmModal.test.tsx b/src/components/terminal/DeepLinkConfirmModal.test.tsx index 99a7d9937..06f72e19c 100644 --- a/src/components/terminal/DeepLinkConfirmModal.test.tsx +++ b/src/components/terminal/DeepLinkConfirmModal.test.tsx @@ -3,6 +3,11 @@ import { render, screen, cleanup, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { DeepLinkConfirmModal } from "./DeepLinkConfirmModal"; import { useDeepLinkStore } from "@/stores/deepLinkStore"; +import { PluginHashMismatchError } from "@/plugins/integrity"; +import { MinAppVersionError } from "@/plugins/version"; + +const acceptButton = (labelKey: string) => + screen.getByText(labelKey).closest("button") as HTMLButtonElement; let teamConnections: Record = {}; let activeLocalSessionId: string | null = null; @@ -171,9 +176,7 @@ test("an invite sheet names the handle and invites the resolved user", async () teamConnections = { "local-1": { sessionKeyBytes: new Uint8Array(32) } }; useDeepLinkStore.setState({ prompt: { route: "invite", handle: "kevin-p" } }); render(); - await waitFor(() => - expect((screen.getByText("terminal.share.deepLinkInviteAction").closest("button") as HTMLButtonElement).disabled).toBe(false), - ); + await waitFor(() => expect(acceptButton("terminal.share.deepLinkInviteAction").disabled).toBe(false)); await userEvent.click(screen.getByText("terminal.share.deepLinkInviteAction")); await waitFor(() => expect(inviteMock).toHaveBeenCalledWith("local-1", expect.objectContaining({ user_id: "u1", handle: "kevin-p" })), @@ -187,6 +190,7 @@ test("an invite link whose handle only fuzzily matches invites nobody", async () useDeepLinkStore.setState({ prompt: { route: "invite", handle: "kevin-p" } }); render(); await waitFor(() => expect(screen.getByText("terminal.share.deepLinkInviteUnknownUser")).toBeTruthy()); + expect(acceptButton("terminal.share.deepLinkInviteAction").disabled).toBe(true); await userEvent.click(screen.getByText("terminal.share.deepLinkInviteAction")); expect(inviteMock).not.toHaveBeenCalled(); }); @@ -198,6 +202,7 @@ test("an invite link with no shareable session names the handle but cannot be ac useDeepLinkStore.setState({ prompt: { route: "invite", handle: "kevin-p" } }); render(); await waitFor(() => expect(screen.getByText("terminal.share.deepLinkInviteNoActiveSession")).toBeTruthy()); + expect(acceptButton("terminal.share.deepLinkInviteAction").disabled).toBe(true); await userEvent.click(screen.getByText("terminal.share.deepLinkInviteAction")); expect(inviteMock).not.toHaveBeenCalled(); }); @@ -223,6 +228,7 @@ test("a snippet-install link naming an entry the catalogue does not list cannot useDeepLinkStore.setState({ prompt: { route: "snippet-install", entryId: "docker-cleanup" } }); render(); await waitFor(() => expect(screen.getByText("snippets.deepLinkInstall.failed")).toBeTruthy()); + expect(acceptButton("snippets.deepLinkInstall.action").disabled).toBe(true); await userEvent.click(screen.getByText("snippets.deepLinkInstall.action")); expect(installEntriesMock).not.toHaveBeenCalled(); }); @@ -251,11 +257,40 @@ test("a plugin-install link naming a source this device does not have installs n useDeepLinkStore.setState({ prompt: { route: "plugin-install", pluginId: "docker", sourceId: "someone-elses" } }); render(); await waitFor(() => expect(screen.getByText("settings.plugins.deepLinkInstall.failed")).toBeTruthy()); + expect(acceptButton("settings.plugins.deepLinkInstall.action").disabled).toBe(true); await userEvent.click(screen.getByText("settings.plugins.deepLinkInstall.action")); expect(installPluginMock).not.toHaveBeenCalled(); expect(fetchManifestMock).not.toHaveBeenCalled(); }); +async function acceptPluginInstall() { + marketplaceSources = [{ id: "voltius", name: "Voltius Marketplace", enabled: true }]; + marketplaceCatalog = [{ id: "docker", name: "Docker", author: "Voltius", version: "1.2.0", sourceId: "voltius" }]; + useDeepLinkStore.setState({ prompt: { route: "plugin-install", pluginId: "docker", sourceId: "voltius" } }); + render(); + await waitFor(() => expect(acceptButton("settings.plugins.deepLinkInstall.action").disabled).toBe(false)); + await userEvent.click(screen.getByText("settings.plugins.deepLinkInstall.action")); +} + +test("a plugin bundle failing its hash check is reported as tampering, not as a bad link", async () => { + installPluginMock.mockRejectedValue(new PluginHashMismatchError("aaa", "bbb")); + await acceptPluginInstall(); + await waitFor(() => expect(screen.getByText("settings.plugins.install.integrityFailed")).toBeTruthy()); + expect(screen.queryByText("settings.plugins.deepLinkInstall.failed")).toBeNull(); +}); + +test("a plugin the running app is too old for names the version requirement", async () => { + installPluginMock.mockRejectedValue(new MinAppVersionError("2.0.0", "1.0.0")); + await acceptPluginInstall(); + await waitFor(() => expect(screen.getByText("settings.plugins.install.versionUnsupported")).toBeTruthy()); +}); + +test("any other plugin-install failure falls back to the deep-link message", async () => { + installPluginMock.mockRejectedValue(new Error("network down")); + await acceptPluginInstall(); + await waitFor(() => expect(screen.getByText("settings.plugins.deepLinkInstall.failed")).toBeTruthy()); +}); + test("a plugin-install link naming a disabled source installs nothing", async () => { marketplaceSources = [{ id: "voltius", name: "Voltius Marketplace", enabled: false }]; marketplaceCatalog = [{ id: "docker", name: "Docker", author: "Voltius", version: "1.2.0", sourceId: "voltius" }]; diff --git a/src/components/terminal/DeepLinkConfirmModal.tsx b/src/components/terminal/DeepLinkConfirmModal.tsx index 19a4bffb3..6194594af 100644 --- a/src/components/terminal/DeepLinkConfirmModal.tsx +++ b/src/components/terminal/DeepLinkConfirmModal.tsx @@ -1,5 +1,6 @@ import { useEffect, useState } from "react"; import { useTranslation } from "react-i18next"; +import type { TFunction } from "i18next"; import { Icon } from "@iconify/react"; import { Modal, ModalCard } from "@/components/shared/Modal"; import { useDeepLinkStore } from "@/stores/deepLinkStore"; @@ -14,6 +15,11 @@ export function DeepLinkConfirmModal() { return prompt ? : null; } +/** Module scope so the load effect can use it without taking it as a dependency. */ +function failureMessage(spec: ConfirmSpec, e: unknown, t: TFunction): string { + return spec.errorMessage?.(e, t, spec.errorKey) ?? t(spec.errorKey); +} + function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { const { t } = useTranslation(); const dismissPrompt = useDeepLinkStore((s) => s.dismissPrompt); @@ -36,10 +42,10 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { .then((value) => { if (!cancelled) setLoaded(value); }) - .catch(() => { + .catch((e: unknown) => { if (cancelled) return; setLoadFailed(true); - setError(t(spec.errorKey)); + setError(failureMessage(spec, e, t)); }) .finally(() => { if (!cancelled) setLoading(false); @@ -61,8 +67,8 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { try { await spec.accept(intent, loaded, t); dismissPrompt(); - } catch { - setError(t(spec.errorKey)); + } catch (e) { + setError(failureMessage(spec, e, t)); } finally { setBusy(false); } diff --git a/src/components/terminal/deepLinkConfirmSpecs.tsx b/src/components/terminal/deepLinkConfirmSpecs.tsx index d3b59fc27..a5b869c4d 100644 --- a/src/components/terminal/deepLinkConfirmSpecs.tsx +++ b/src/components/terminal/deepLinkConfirmSpecs.tsx @@ -13,6 +13,7 @@ import type { CatalogEntry } from "@/services/snippetCatalog"; import { useVaultStore } from "@/stores/vaultStore"; import { PluginPermissionList } from "@/components/settings/sections/PluginPermissionList"; import { useMarketplaceStore, type MarketplacePlugin } from "@/stores/marketplaceStore"; +import { pluginInstallErrorMessage } from "@/plugins/installErrors"; import type { PluginManifest } from "@/plugins/api"; export type ConfirmRoute = ConfirmIntent["route"]; @@ -29,6 +30,12 @@ export interface ConfirmSpec { icon: string; acceptLabelKey: string; errorKey: string; + /** + * Distinguishes failures that mean something specific to the user from the + * generic `errorKey`, which it receives as the fallback. Absent means every + * failure reads the same. + */ + errorMessage?: (e: unknown, t: TFunction, fallbackKey: string) => string; /** * Resolves what the link names. Omitted where the intent already says * everything, so such a sheet paints complete in its first frame rather than @@ -172,6 +179,9 @@ export const CONFIRM_SPECS: { [K in ConfirmRoute]: ConfirmSpec { await useMarketplaceStore.getState().loadSources(); const source = useMarketplaceStore diff --git a/src/plugins/installErrors.ts b/src/plugins/installErrors.ts new file mode 100644 index 000000000..90d1f68ac --- /dev/null +++ b/src/plugins/installErrors.ts @@ -0,0 +1,16 @@ +import type { TFunction } from "i18next"; +import { PluginHashMismatchError } from "./integrity"; +import { MinAppVersionError } from "./version"; + +/** + * The user-facing message for a failed plugin install, matched on the error's + * type rather than its text. A hash mismatch is the user's only tamper signal, + * so it must never collapse into whatever generic failure the caller shows; + * `fallbackKey` covers everything else. + */ +export function pluginInstallErrorMessage(e: unknown, t: TFunction, fallbackKey: string): string { + if (e instanceof PluginHashMismatchError) return t("settings.plugins.install.integrityFailed"); + if (e instanceof MinAppVersionError) + return t("settings.plugins.install.versionUnsupported", { version: e.required }); + return t(fallbackKey); +} From 536136bc81b8468c36ba95d0d3435d25dea5f1bb Mon Sep 17 00:00:00 2001 From: kipavy Date: Wed, 19 Aug 2026 12:28:00 +0000 Subject: [PATCH 2/3] fix(deep-links): stop a locale change re-running a confirm sheet's load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sheet stored its error already translated, which forced `t` into the load effect's dependencies. `t` is a new function on every locale change, so changing language over an open sheet ran `load` a second time — another searchUsers or manifest fetch — without resetting `loading` or `loadFailed`, leaving accept enabled over a stale result. Errors are now held as a key plus its parameters and translated at render, so `t` leaves the deps. pluginInstallErrorMessage returns that descriptor rather than a string, which is also what lets it carry versionUnsupported's `version`. The effect additionally resets its own state, since a redelivered link can change the intent's identity without remounting the sheet. --- .../settings/sections/PluginsSection.tsx | 3 ++- .../terminal/DeepLinkConfirmModal.test.tsx | 13 +++++++++++ .../terminal/DeepLinkConfirmModal.tsx | 23 +++++++++++-------- .../terminal/deepLinkConfirmSpecs.tsx | 9 ++++---- src/plugins/installErrors.ts | 15 ++++++++---- 5 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/components/settings/sections/PluginsSection.tsx b/src/components/settings/sections/PluginsSection.tsx index 155541909..d3f893187 100644 --- a/src/components/settings/sections/PluginsSection.tsx +++ b/src/components/settings/sections/PluginsSection.tsx @@ -179,11 +179,12 @@ function usePluginInstaller() { const busy = new Set([...installing, ...preparing]); const notifyError = (e: unknown) => { + const { key, params } = pluginInstallErrorMessage(e, "settings.plugins.install.failed"); useNotificationStore.getState().addToast({ source: { kind: "plugin", id: "system", name: "Voltius" }, type: "toast", severity: "error", - message: pluginInstallErrorMessage(e, t, "settings.plugins.install.failed"), + message: t(key, params), duration: 0, }); }; diff --git a/src/components/terminal/DeepLinkConfirmModal.test.tsx b/src/components/terminal/DeepLinkConfirmModal.test.tsx index 06f72e19c..f33d53cae 100644 --- a/src/components/terminal/DeepLinkConfirmModal.test.tsx +++ b/src/components/terminal/DeepLinkConfirmModal.test.tsx @@ -183,6 +183,19 @@ test("an invite sheet names the handle and invites the resolved user", async () ); }); +test("a re-render does not re-run load", async () => { + // `useTranslation` hands back a fresh `t` on every render, as the real hook does + // on a locale change. A `t` in the load effect's deps turns that into a second + // searchUsers — or, once the load's own setState re-renders, an unbounded loop. + searchUsersMock.mockResolvedValue([{ user_id: "u1", handle: "kevin-p", is_teammate: false }]); + activeLocalSessionId = "local-1"; + teamConnections = { "local-1": { sessionKeyBytes: new Uint8Array(32) } }; + useDeepLinkStore.setState({ prompt: { route: "invite", handle: "kevin-p" } }); + render(); + await waitFor(() => expect(acceptButton("terminal.share.deepLinkInviteAction").disabled).toBe(false)); + expect(searchUsersMock).toHaveBeenCalledTimes(1); +}); + test("an invite link whose handle only fuzzily matches invites nobody", async () => { searchUsersMock.mockResolvedValue([{ user_id: "u1", handle: "kevin-porter", is_teammate: false }]); activeLocalSessionId = "local-1"; diff --git a/src/components/terminal/DeepLinkConfirmModal.tsx b/src/components/terminal/DeepLinkConfirmModal.tsx index 6194594af..1f06cfe64 100644 --- a/src/components/terminal/DeepLinkConfirmModal.tsx +++ b/src/components/terminal/DeepLinkConfirmModal.tsx @@ -1,11 +1,11 @@ import { useEffect, useState } from "react"; import { useTranslation } from "react-i18next"; -import type { TFunction } from "i18next"; import { Icon } from "@iconify/react"; import { Modal, ModalCard } from "@/components/shared/Modal"; import { useDeepLinkStore } from "@/stores/deepLinkStore"; import { intentKey, type ConfirmIntent } from "@/services/deepLinkUrl"; import { CONFIRM_SPECS, type ConfirmRoute, type ConfirmSpec } from "./deepLinkConfirmSpecs"; +import type { TranslatableMessage } from "@/plugins/installErrors"; export function DeepLinkConfirmModal() { const prompt = useDeepLinkStore((s) => s.prompt); @@ -15,9 +15,8 @@ export function DeepLinkConfirmModal() { return prompt ? : null; } -/** Module scope so the load effect can use it without taking it as a dependency. */ -function failureMessage(spec: ConfirmSpec, e: unknown, t: TFunction): string { - return spec.errorMessage?.(e, t, spec.errorKey) ?? t(spec.errorKey); +function failureMessage(spec: ConfirmSpec, e: unknown): TranslatableMessage { + return spec.errorMessage?.(e, spec.errorKey) ?? { key: spec.errorKey }; } function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { @@ -32,11 +31,17 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { const [loading, setLoading] = useState(!!spec.load); const [loadFailed, setLoadFailed] = useState(false); const [busy, setBusy] = useState(false); - const [error, setError] = useState(null); + const [error, setError] = useState(null); + // `t` is deliberately absent from the deps: it is a new function on every + // locale change, and re-running `load` would fetch a second time without + // resetting the state below, leaving accept live over a stale result. useEffect(() => { if (!spec.load) return; let cancelled = false; + setLoading(true); + setLoadFailed(false); + setError(null); void spec .load(intent) .then((value) => { @@ -45,7 +50,7 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { .catch((e: unknown) => { if (cancelled) return; setLoadFailed(true); - setError(failureMessage(spec, e, t)); + setError(failureMessage(spec, e)); }) .finally(() => { if (!cancelled) setLoading(false); @@ -53,7 +58,7 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { return () => { cancelled = true; }; - }, [intent, spec, t]); + }, [intent, spec]); const details = spec.details(intent, loaded, t); // A sheet that could not name what it is about must never be acceptable. A @@ -68,7 +73,7 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { await spec.accept(intent, loaded, t); dismissPrompt(); } catch (e) { - setError(failureMessage(spec, e, t)); + setError(failureMessage(spec, e)); } finally { setBusy(false); } @@ -90,7 +95,7 @@ function ConfirmSheet({ intent }: { intent: ConfirmIntent }) { {loading &&

{t("common.state.loading")}

} {spec.extra?.(loaded, t)} {details.note &&

{details.note}

} - {error &&

{error}

} + {error &&

{t(error.key, error.params)}

}