From f9eae0c2762dbd646afc6cbc41a34354af3ef1ac Mon Sep 17 00:00:00 2001 From: "Vyctor H. Brzezowski" Date: Sun, 9 Aug 2026 21:13:19 -0300 Subject: [PATCH] fix(ui): confirm Devices pairing rejects and token revokes in-app (#121279) * fix(ui): confirm Devices pairing rejects and token revokes in-app Reject device pairing, reject node pairing, and revoke device token asked for confirmation through native window.confirm from the shared Nodes operations module. Embedded webviews without a dialog bridge return false for that call, so all three actions ended with no dialog, no request, and no recorded reason. Move the confirmations to DevicesPage, the visual owner, onto the canonical showConfirmDialog helper already used for inventory removal, and give the copy real i18n keys. The shared operations module now presents no UI at all. * test(ui): drive the revoke confirmation through the in-app dialog device-token-reconnect drove the revoke prompt through Playwright's native dialog event, which no longer fires now that the page owns the confirmation. --- ui/src/e2e/device-token-reconnect.e2e.test.ts | 16 +-- ui/src/i18n/locales/en.ts | 5 + ui/src/lib/nodes/index.ts | 17 +-- ui/src/pages/devices/devices-page.test.ts | 110 +++++++++++++-- ui/src/pages/devices/devices-page.ts | 132 +++++++++++------- 5 files changed, 195 insertions(+), 85 deletions(-) diff --git a/ui/src/e2e/device-token-reconnect.e2e.test.ts b/ui/src/e2e/device-token-reconnect.e2e.test.ts index 35b3b66e6eda..81cc25f1a96d 100644 --- a/ui/src/e2e/device-token-reconnect.e2e.test.ts +++ b/ui/src/e2e/device-token-reconnect.e2e.test.ts @@ -247,15 +247,13 @@ describeControlUiE2e("Control UI device-token reconnect E2E", () => { await revokeButton.waitFor({ state: "visible" }); await revokeButton.scrollIntoViewIfNeeded(); await captureProof(wilfredDevices.page, "wilfred-before-revoke.png"); - const dialogPromise = wilfredDevices.page.waitForEvent("dialog"); - await Promise.all([ - dialogPromise.then(async (dialog) => { - expect(dialog.type()).toBe("confirm"); - expect(dialog.message()).toBe(`Revoke token for ${deviceId} (operator)?`); - await dialog.accept(); - }), - revokeButton.click(), - ]); + await revokeButton.click(); + // Revoke confirms in-page, not through window.confirm: webviews without a dialog + // bridge silently answer false and would drop the action with no visible outcome. + const revokeConfirm = wilfredDevices.page.locator("openclaw-modal-dialog"); + await revokeConfirm.getByText("Revoke the operator token?").waitFor(); + await revokeConfirm.getByText(`Device ID: ${deviceId}`).waitFor(); + await revokeConfirm.getByRole("button", { name: "Revoke", exact: true }).click(); const revoke = await wilfredDevices.gateway.waitForRequest("device.token.revoke"); expect(revoke.params).toEqual({ deviceId, role: "operator" }); const wilfredStoreKey = diff --git a/ui/src/i18n/locales/en.ts b/ui/src/i18n/locales/en.ts index fd3f064a0206..38a491dc7599 100644 --- a/ui/src/i18n/locales/en.ts +++ b/ui/src/i18n/locales/en.ts @@ -575,6 +575,11 @@ export const en: TranslationMap = { removeStalePromptTitle: "Remove {count} stale pairings?", removeStalePromptTitleOne: "Remove 1 stale pairing?", removeStalePromptBody: "Affected clients re-pair silently on their next connection.", + rejectDevicePromptTitle: "Reject this device pairing request?", + rejectNodePromptTitle: "Reject this node pairing request?", + rejectPromptBody: "The client must send a new pairing request before it can connect.", + revokePromptTitle: "Revoke the {role} token?", + revokePromptBody: "This token stops working immediately and cannot be restored.", gateway: "gateway", unpaired: "unpaired", unknownClient: "unknown client", diff --git a/ui/src/lib/nodes/index.ts b/ui/src/lib/nodes/index.ts index d781d41caeb5..718ffab79cea 100644 --- a/ui/src/lib/nodes/index.ts +++ b/ui/src/lib/nodes/index.ts @@ -1,4 +1,7 @@ // Shared Nodes operations used by the Control UI page and Gateway event hooks. +// Presentation-free by contract: destructive confirmations belong to the owning page, +// because native window.confirm silently returns false in webviews with no dialog bridge +// and would end the action with no outcome and no recorded reason. import { getPublicKeyAsync, signAsync, utils } from "@noble/ed25519"; import { gatewayCredentialScope } from "@openclaw/gateway-client/browser"; import { @@ -291,10 +294,6 @@ export async function rejectDevicePairing(state: DevicesState, requestId: string if (!client || !state.connected) { return; } - const confirmed = window.confirm("Reject this device pairing request?"); - if (!confirmed) { - return; - } const generation = state.requestGeneration; try { await client.request("device.pair.reject", { requestId }); @@ -343,8 +342,6 @@ async function reloadInventory(state: InventoryState, opts?: { error?: string }) } } -// Confirmation for these removals lives in the page (in-page dialog): native -// window.confirm silently returns false in webviews without a dialog bridge. export async function removeInventoryEntry(state: InventoryState, entry: InventoryRemovalRequest) { const client = state.client; if (!client || !state.connected) { @@ -400,10 +397,6 @@ export async function rejectNodePairingRequest(state: InventoryState, requestId: if (!state.client || !state.connected) { return; } - const confirmed = window.confirm("Reject this node pairing request?"); - if (!confirmed) { - return; - } try { await state.client.request("node.pair.reject", { requestId }); await reloadInventory(state); @@ -467,10 +460,6 @@ export async function revokeDeviceToken( if (!client || !state.connected) { return; } - const confirmed = window.confirm(`Revoke token for ${params.deviceId} (${params.role})?`); - if (!confirmed) { - return; - } const generation = state.requestGeneration; try { const { gatewayUrl, ...requestParams } = params; diff --git a/ui/src/pages/devices/devices-page.test.ts b/ui/src/pages/devices/devices-page.test.ts index 2d4e55a873e6..c20640aa2764 100644 --- a/ui/src/pages/devices/devices-page.test.ts +++ b/ui/src/pages/devices/devices-page.test.ts @@ -1,20 +1,22 @@ /* @vitest-environment jsdom */ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { GatewayBrowserClient } from "../../api/gateway.ts"; import type { PresenceEntry } from "../../api/types.ts"; import type { ApplicationContext, ApplicationGatewaySnapshot } from "../../app/context.ts"; -import { showConfirmDialog } from "../../components/confirm-dialog.ts"; +import { t } from "../../i18n/index.ts"; import { createInitialDevicesState, loadNodes, type InventoryRemovalRequest, } from "../../lib/nodes/index.ts"; +import { + installDialogPolyfill, + waitForRenderedModalDialog, +} from "../../test-helpers/modal-dialog.ts"; import type { DevicesRouteData } from "./devices-page.ts"; import "./devices-page.ts"; -vi.mock("../../components/confirm-dialog.ts", () => ({ showConfirmDialog: vi.fn() })); - type TestDevicesPage = HTMLElement & { context: ApplicationContext; pageState: ReturnType; @@ -39,8 +41,30 @@ type TestDevicesPage = HTMLElement & { kind: "entry"; entry: InventoryRemovalRequest; }) => Promise; + confirmPairingReject: (target: "device" | "node", requestId: string) => Promise; + confirmTokenRevoke: (deviceId: string, role: string) => Promise; }; +function clickDialogButton(label: string) { + const button = [...document.body.querySelectorAll("button")].find( + (candidate) => candidate.textContent?.trim() === label, + ); + if (!(button instanceof HTMLButtonElement)) { + throw new Error(`Expected ${label} button`); + } + button.click(); +} + +function createConnectedPage(client: GatewayBrowserClient) { + const page = document.createElement("openclaw-devices-page") as TestDevicesPage; + page.context = { + gateway: { connection: { gatewayUrl: "http://gateway.test" } }, + runtimeConfig: { state: { configSnapshot: null, configLoading: false } }, + } as unknown as ApplicationContext; + applyGatewaySnapshot(page, gatewaySnapshot(client, true)); + return page; +} + function applyGatewaySnapshot( page: TestDevicesPage, snapshot: ApplicationGatewaySnapshot, @@ -94,6 +118,17 @@ function gateway(client: GatewayBrowserClient | null): ApplicationContext["gatew } describe("DevicesPage gateway lifecycle", () => { + let restoreDialogPolyfill: () => void; + + beforeEach(() => { + restoreDialogPolyfill = installDialogPolyfill(); + }); + + afterEach(() => { + document.body.replaceChildren(); + restoreDialogPolyfill(); + }); + it("preserves matching initial route data, then resets it on provider replacement", () => { const client = null; const currentGateway = gateway(client); @@ -250,25 +285,70 @@ describe("DevicesPage gateway lifecycle", () => { it("cancels a pending removal confirmation when the connection resets", async () => { const request = vi.fn(); const client = { request } as unknown as GatewayBrowserClient; - const confirmation = deferred(); - vi.mocked(showConfirmDialog).mockReturnValueOnce(confirmation.promise); - const page = document.createElement("openclaw-devices-page") as TestDevicesPage; - page.pageState = createInitialDevicesState({ client, connected: true }); - page.context = { - runtimeConfig: { state: { configSnapshot: null, configLoading: false } }, - } as unknown as ApplicationContext; + const page = createConnectedPage(client); + const pending = page.confirmInventoryRemoval({ kind: "entry", entry: { id: "device-1", name: "Browser", removeNode: false, removeDevice: true }, }); - await Promise.resolve(); - const signal = vi.mocked(showConfirmDialog).mock.calls[0]?.[0].signal; + await waitForRenderedModalDialog(document.body); applyGatewaySnapshot(page, gatewaySnapshot(client, false)); - confirmation.resolve(true); await pending; - expect(signal?.aborted).toBe(true); expect(request).not.toHaveBeenCalled(); + expect(document.body.querySelector("openclaw-modal-dialog")).toBeNull(); + }); + + it("rejects a device pairing request after the in-app dialog is confirmed", async () => { + const request = vi.fn().mockResolvedValue({}); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmPairingReject("device", "request-1"); + const { dialog } = await waitForRenderedModalDialog(document.body); + expect(dialog.getAttribute("aria-label")).toBe(t("devices.inventory.rejectDevicePromptTitle")); + + clickDialogButton(t("devices.inventory.reject")); + await pending; + + expect(request).toHaveBeenCalledWith("device.pair.reject", { requestId: "request-1" }); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); + }); + + it("issues no node pairing request when the dialog is cancelled", async () => { + const request = vi.fn(); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmPairingReject("node", "request-2"); + await waitForRenderedModalDialog(document.body); + + clickDialogButton(t("common.cancel")); + await pending; + + expect(request).not.toHaveBeenCalled(); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); + }); + + it("drops a confirmed token revoke when the request generation moved on", async () => { + const request = vi.fn(); + const client = { request } as unknown as GatewayBrowserClient; + const page = createConnectedPage(client); + + const pending = page.confirmTokenRevoke("device-1", "operator"); + const { dialog } = await waitForRenderedModalDialog(document.body); + expect(dialog.getAttribute("aria-label")).toBe( + t("devices.inventory.revokePromptTitle", { role: "operator" }), + ); + // The awaited dialog is a real suspension point: a generation bump during it means the + // captured scope no longer owns the connection, so the revoke must not reach the server. + page.pageState.requestGeneration += 1; + + clickDialogButton(t("devices.inventory.revoke")); + await pending; + + expect(request).not.toHaveBeenCalled(); + applyGatewaySnapshot(page, gatewaySnapshot(client, false)); }); }); diff --git a/ui/src/pages/devices/devices-page.ts b/ui/src/pages/devices/devices-page.ts index a7b45731123a..3d1a831fcf6b 100644 --- a/ui/src/pages/devices/devices-page.ts +++ b/ui/src/pages/devices/devices-page.ts @@ -10,7 +10,7 @@ import { type ApplicationGatewaySnapshot, } from "../../app/context.ts"; import { hasOperatorAdminAccess } from "../../app/operator-access.ts"; -import { showConfirmDialog } from "../../components/confirm-dialog.ts"; +import { showConfirmDialog, type ConfirmDialogOptions } from "../../components/confirm-dialog.ts"; import { renderDocsLink } from "../../components/settings-ui.ts"; import { renderSettingsWorkspace } from "../../components/settings-workspace.ts"; import { t } from "../../i18n/index.ts"; @@ -89,7 +89,7 @@ class DevicesPage extends OpenClawLightDomElement { @state() private canPairDevice = false; @state() private execApprovalsTarget: "gateway" | "node" = "gateway"; @state() private execApprovalsTargetNodeId: string | null = null; - private inventoryRemovalConfirmation: AbortController | null = null; + private pendingConfirmation: AbortController | null = null; private routeDataInitialized = false; private readonly gateway = new GatewayPageController(this, { @@ -181,7 +181,7 @@ class DevicesPage extends OpenClawLightDomElement { } override disconnectedCallback() { - this.cancelInventoryRemovalConfirmation(); + this.cancelPendingConfirmation(); this.subscriptions.clear(); void this.presenceTask.run([null, null]); this.presence = []; @@ -245,7 +245,7 @@ class DevicesPage extends OpenClawLightDomElement { } private resetServerState(snapshot: ApplicationGatewaySnapshot) { - this.cancelInventoryRemovalConfirmation(); + this.cancelPendingConfirmation(); this.pageState.requestGeneration += 1; const next = createInitialDevicesState({ client: snapshot.client, @@ -313,45 +313,32 @@ class DevicesPage extends OpenClawLightDomElement { return this.presenceTask.run([gateway, client]); } - private cancelInventoryRemovalConfirmation() { - this.inventoryRemovalConfirmation?.abort(); - this.inventoryRemovalConfirmation = null; + private cancelPendingConfirmation() { + this.pendingConfirmation?.abort(); + this.pendingConfirmation = null; } - private async confirmInventoryRemoval(prompt: InventoryRemovalPrompt) { - if (this.inventoryRemovalConfirmation) { + // Every destructive Devices action confirms here, never through window.confirm: the + // awaited dialog lets the gateway reconnect or swap clients mid-prompt, so the captured + // scope is revalidated before the operation runs against a different server. + private async confirmDestructiveAction( + prompt: Omit, + run: (pageState: DevicesPageDataState) => unknown, + ) { + if (this.pendingConfirmation) { return; } const controller = new AbortController(); - this.inventoryRemovalConfirmation = controller; + this.pendingConfirmation = controller; const generation = this.requestGeneration; const client = this.gateway.client; - const title = - prompt.kind === "entry" - ? t("devices.inventory.removePromptTitle", { name: prompt.entry.name }) - : t( - prompt.entries.length === 1 - ? "devices.inventory.removeStalePromptTitleOne" - : "devices.inventory.removeStalePromptTitle", - { count: String(prompt.entries.length) }, - ); const confirmed = await showConfirmDialog({ - title, - message: t( - prompt.kind === "entry" - ? "devices.inventory.removePromptBody" - : "devices.inventory.removeStalePromptBody", - ), - details: - prompt.kind === "entry" - ? t("devices.inventory.deviceId", { id: prompt.entry.id }) - : undefined, - confirmLabel: t("devices.inventory.remove"), + ...prompt, danger: true, signal: controller.signal, }); - if (this.inventoryRemovalConfirmation === controller) { - this.inventoryRemovalConfirmation = null; + if (this.pendingConfirmation === controller) { + this.pendingConfirmation = null; } if ( !confirmed || @@ -362,11 +349,71 @@ class DevicesPage extends OpenClawLightDomElement { ) { return; } + await this.runPageTask(run); + } + + private confirmInventoryRemoval(prompt: InventoryRemovalPrompt) { if (prompt.kind === "entry") { - void this.runPageTask((pageState) => removeInventoryEntry(pageState, prompt.entry)); - return; + const entry = prompt.entry; + return this.confirmDestructiveAction( + { + title: t("devices.inventory.removePromptTitle", { name: entry.name }), + message: t("devices.inventory.removePromptBody"), + details: t("devices.inventory.deviceId", { id: entry.id }), + confirmLabel: t("devices.inventory.remove"), + }, + (pageState) => removeInventoryEntry(pageState, entry), + ); } - void this.runPageTask((pageState) => removeStaleInventoryEntries(pageState, prompt.entries)); + const entries = prompt.entries; + return this.confirmDestructiveAction( + { + title: t( + entries.length === 1 + ? "devices.inventory.removeStalePromptTitleOne" + : "devices.inventory.removeStalePromptTitle", + { count: String(entries.length) }, + ), + message: t("devices.inventory.removeStalePromptBody"), + confirmLabel: t("devices.inventory.remove"), + }, + (pageState) => removeStaleInventoryEntries(pageState, entries), + ); + } + + private confirmPairingReject(target: "device" | "node", requestId: string) { + return this.confirmDestructiveAction( + { + title: t( + target === "device" + ? "devices.inventory.rejectDevicePromptTitle" + : "devices.inventory.rejectNodePromptTitle", + ), + message: t("devices.inventory.rejectPromptBody"), + confirmLabel: t("devices.inventory.reject"), + }, + (pageState) => + target === "device" + ? rejectDevicePairing(pageState, requestId) + : rejectNodePairingRequest(pageState, requestId), + ); + } + + private confirmTokenRevoke(deviceId: string, role: string) { + return this.confirmDestructiveAction( + { + title: t("devices.inventory.revokePromptTitle", { role }), + message: t("devices.inventory.revokePromptBody"), + details: t("devices.inventory.deviceId", { id: deviceId }), + confirmLabel: t("devices.inventory.revoke"), + }, + (pageState) => + revokeDeviceToken(pageState, { + deviceId, + gatewayUrl: this.context.gateway.connection.gatewayUrl, + role, + }), + ); } private resolveExecApprovalsTarget(): ExecApprovalsTarget { @@ -420,12 +467,10 @@ class DevicesPage extends OpenClawLightDomElement { onDevicePairSetupOpen: () => void this.context.overlays.openDevicePairSetup(), onDeviceApprove: (requestId) => void this.runPageTask((pageState) => approveDevicePairing(pageState, requestId)), - onDeviceReject: (requestId) => - void this.runPageTask((pageState) => rejectDevicePairing(pageState, requestId)), + onDeviceReject: (requestId) => void this.confirmPairingReject("device", requestId), onNodeApprove: (requestId) => void this.runPageTask((pageState) => approveNodePairingRequest(pageState, requestId)), - onNodeReject: (requestId) => - void this.runPageTask((pageState) => rejectNodePairingRequest(pageState, requestId)), + onNodeReject: (requestId) => void this.confirmPairingReject("node", requestId), onInventoryRemove: (entry) => void this.confirmInventoryRemoval({ kind: "entry", entry }), onInventoryCleanup: (entries) => { if (entries.length > 0) { @@ -441,14 +486,7 @@ class DevicesPage extends OpenClawLightDomElement { scopes, }), ), - onDeviceRevoke: (deviceId, role) => - void this.runPageTask((pageState) => - revokeDeviceToken(pageState, { - deviceId, - gatewayUrl: this.context.gateway.connection.gatewayUrl, - role, - }), - ), + onDeviceRevoke: (deviceId, role) => void this.confirmTokenRevoke(deviceId, role), onLoadConfig: () => void this.context.runtimeConfig.refresh({ discardPendingChanges: true }), onLoadExecApprovals: () =>