From 3aeb55b5e7b166054a16aafc1abe20ad4d5a310f Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Tue, 12 May 2026 16:33:55 +0530 Subject: [PATCH] fix(node-pairing): replace changed pending requests [AI] (#80894) * fix: replace changed node pairing requests * fix: replace changed node pairing requests * addressing codex review * addressing codex review * addressing ci * fix: complete node pairing permission handling * addressing review-skill * addressing codex review * addressing codex review * addressing ci * addressing ci * addressing ci * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + .../OpenClawProtocol/GatewayModels.swift | 4 + src/gateway/node-connect-reconcile.test.ts | 151 ++++++++++++++++++ src/gateway/node-connect-reconcile.ts | 134 ++++++++++++++-- src/gateway/protocol/index.test.ts | 22 +++ src/gateway/protocol/schema/nodes.ts | 1 + .../server-methods/nodes.invoke-wake.test.ts | 67 ++++++++ src/gateway/server-methods/nodes.ts | 13 ++ .../server/ws-connection/message-handler.ts | 15 ++ src/infra/node-pairing.test.ts | 150 ++++++++++++++--- src/infra/node-pairing.ts | 108 +++++++++++-- 11 files changed, 620 insertions(+), 46 deletions(-) create mode 100644 src/gateway/node-connect-reconcile.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a76537329b39..b547e875f201 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(node-pairing): replace changed pending requests [AI]. (#80894) Thanks @pgondhi987. - Rate limit Google Chat webhook requests [AI]. (#80974) Thanks @pgondhi987. - fix(feishu): normalize webhook rate-limit client keys [AI]. (#80975) Thanks @pgondhi987. - fix(auth): prevent bootstrap pairing scope changes [AI]. (#80976) Thanks @pgondhi987. diff --git a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift index d1db014896c8..ef401e073cdc 100644 --- a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift +++ b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift @@ -971,6 +971,7 @@ public struct NodePairRequestParams: Codable, Sendable { public let modelidentifier: String? public let caps: [String]? public let commands: [String]? + public let permissions: [String: AnyCodable]? public let remoteip: String? public let silent: Bool? @@ -985,6 +986,7 @@ public struct NodePairRequestParams: Codable, Sendable { modelidentifier: String?, caps: [String]?, commands: [String]?, + permissions: [String: AnyCodable]?, remoteip: String?, silent: Bool?) { @@ -998,6 +1000,7 @@ public struct NodePairRequestParams: Codable, Sendable { self.modelidentifier = modelidentifier self.caps = caps self.commands = commands + self.permissions = permissions self.remoteip = remoteip self.silent = silent } @@ -1013,6 +1016,7 @@ public struct NodePairRequestParams: Codable, Sendable { case modelidentifier = "modelIdentifier" case caps case commands + case permissions case remoteip = "remoteIp" case silent } diff --git a/src/gateway/node-connect-reconcile.test.ts b/src/gateway/node-connect-reconcile.test.ts new file mode 100644 index 000000000000..6e470e27dc0e --- /dev/null +++ b/src/gateway/node-connect-reconcile.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it, vi } from "vitest"; +import type { NodePairingPairedNode, NodePairingRequestInput } from "../infra/node-pairing.js"; +import { reconcileNodePairingOnConnect } from "./node-connect-reconcile.js"; +import type { ConnectParams } from "./protocol/index.js"; + +function makeNodeConnectParams(overrides?: Partial): ConnectParams { + return { + minProtocol: 1, + maxProtocol: 1, + client: { + id: "openclaw-ios", + version: "test", + platform: "ios", + mode: "node", + }, + commands: ["canvas.snapshot"], + ...overrides, + }; +} + +function makePairedNode(overrides?: Partial): NodePairingPairedNode { + return { + nodeId: "openclaw-ios", + token: "token-1", + createdAtMs: 1, + approvedAtMs: 1, + ...overrides, + }; +} + +describe("reconcileNodePairingOnConnect", () => { + it("includes declared permissions in pending node pairing requests", async () => { + const requestPairing = vi.fn(async (input: NodePairingRequestInput) => ({ + status: "pending" as const, + request: { ...input, requestId: "req-1", ts: 1 }, + created: true, + })); + + await reconcileNodePairingOnConnect({ + cfg: {} as never, + connectParams: makeNodeConnectParams({ + permissions: { camera: true, notifications: false }, + }), + pairedNode: null, + requestPairing, + }); + + expect(requestPairing).toHaveBeenCalledWith( + expect.objectContaining({ + nodeId: "openclaw-ios", + permissions: { camera: true, notifications: false }, + }), + ); + }); + + it("requires a fresh pairing request when paired node capabilities change", async () => { + const requestPairing = vi.fn(async (input: NodePairingRequestInput) => ({ + status: "pending" as const, + request: { ...input, requestId: "req-caps", ts: 1 }, + created: true, + })); + + const result = await reconcileNodePairingOnConnect({ + cfg: {} as never, + connectParams: makeNodeConnectParams({ + caps: ["camera", "screen"], + commands: [], + }), + pairedNode: makePairedNode({ + caps: ["camera"], + commands: [], + }), + requestPairing, + }); + + expect(requestPairing).toHaveBeenCalledWith( + expect.objectContaining({ + caps: ["camera", "screen"], + commands: [], + }), + ); + expect(result.effectiveCaps).toEqual(["camera"]); + expect(result.effectiveCommands).toEqual([]); + expect(result.pendingPairing?.request.requestId).toBe("req-caps"); + }); + + it("requires a fresh pairing request when paired node permissions change", async () => { + const requestPairing = vi.fn(async (input: NodePairingRequestInput) => ({ + status: "pending" as const, + request: { ...input, requestId: "req-permissions", ts: 1 }, + created: true, + })); + + const result = await reconcileNodePairingOnConnect({ + cfg: {} as never, + connectParams: makeNodeConnectParams({ + commands: [], + permissions: { camera: true, notifications: false }, + }), + pairedNode: makePairedNode({ + commands: [], + permissions: { camera: true }, + }), + requestPairing, + }); + + expect(requestPairing).toHaveBeenCalledWith( + expect.objectContaining({ + commands: [], + permissions: { camera: true, notifications: false }, + }), + ); + expect(result.effectiveCommands).toEqual([]); + expect(result.effectivePermissions).toEqual({ camera: true, notifications: false }); + expect(result.pendingPairing?.request.requestId).toBe("req-permissions"); + }); + + it("applies declared capability and permission downgrades to the live surface", async () => { + const requestPairing = vi.fn(async (input: NodePairingRequestInput) => ({ + status: "pending" as const, + request: { ...input, requestId: "req-downgrade", ts: 1 }, + created: true, + })); + + const result = await reconcileNodePairingOnConnect({ + cfg: {} as never, + connectParams: makeNodeConnectParams({ + caps: ["camera"], + commands: [], + permissions: { camera: false }, + }), + pairedNode: makePairedNode({ + caps: ["camera", "screen"], + commands: [], + permissions: { camera: true, notifications: true }, + }), + requestPairing, + }); + + expect(requestPairing).toHaveBeenCalledWith( + expect.objectContaining({ + caps: ["camera"], + permissions: { camera: false }, + }), + ); + expect(result.effectiveCaps).toEqual(["camera"]); + expect(result.effectiveCommands).toEqual([]); + expect(result.effectivePermissions).toEqual({ camera: false }); + expect(result.pendingPairing?.request.requestId).toBe("req-downgrade"); + }); +}); diff --git a/src/gateway/node-connect-reconcile.ts b/src/gateway/node-connect-reconcile.ts index 359c48946fbc..58f2800ff4e7 100644 --- a/src/gateway/node-connect-reconcile.ts +++ b/src/gateway/node-connect-reconcile.ts @@ -1,25 +1,22 @@ import type { OpenClawConfig } from "../config/types.openclaw.js"; import type { NodePairingPairedNode, - NodePairingPendingRequest, NodePairingRequestInput, + RequestNodePairingResult, } from "../infra/node-pairing.js"; +import { normalizeArrayBackedTrimmedStringList } from "../shared/string-normalization.js"; import { normalizeDeclaredNodeCommands, resolveNodeCommandAllowlist, } from "./node-command-policy.js"; import type { ConnectParams } from "./protocol/index.js"; -type PendingNodePairingResult = { - status: "pending"; - request: NodePairingPendingRequest; - created: boolean; -}; - export type NodeConnectPairingReconcileResult = { nodeId: string; + effectiveCaps: string[]; effectiveCommands: string[]; - pendingPairing?: PendingNodePairingResult; + effectivePermissions?: Record; + pendingPairing?: RequestNodePairingResult; }; function resolveApprovedReconnectCommands(params: { @@ -32,10 +29,94 @@ function resolveApprovedReconnectCommands(params: { }); } +function normalizeApprovalSurfaceList(value: readonly string[] | undefined): string[] { + return normalizeArrayBackedTrimmedStringList(value) ?? []; +} + +function sameApprovalSurfaceSet( + left: readonly string[] | undefined, + right: readonly string[] | undefined, +): boolean { + const normalizedLeft = new Set(normalizeApprovalSurfaceList(left)); + const normalizedRight = new Set(normalizeApprovalSurfaceList(right)); + if (normalizedLeft.size !== normalizedRight.size) { + return false; + } + for (const entry of normalizedLeft) { + if (!normalizedRight.has(entry)) { + return false; + } + } + return true; +} + +function normalizePermissionMap( + value: Record | undefined, +): Record | undefined { + if (!value) { + return undefined; + } + const entries = Object.entries(value).toSorted(([leftKey], [rightKey]) => + leftKey.localeCompare(rightKey), + ); + return entries.length > 0 ? Object.fromEntries(entries) : undefined; +} + +function samePermissions( + left: Record | undefined, + right: Record | undefined, +): boolean { + const leftEntries = Object.entries(left ?? {}).toSorted(([leftKey], [rightKey]) => + leftKey.localeCompare(rightKey), + ); + const rightEntries = Object.entries(right ?? {}).toSorted(([leftKey], [rightKey]) => + leftKey.localeCompare(rightKey), + ); + if (leftEntries.length !== rightEntries.length) { + return false; + } + return leftEntries.every(([key, value], index) => { + const rightEntry = rightEntries[index]; + return rightEntry !== undefined && rightEntry[0] === key && rightEntry[1] === value; + }); +} + +function intersectApprovalSurfaceList(params: { + approved: readonly string[] | undefined; + declared: readonly string[]; +}): string[] { + const approved = new Set(normalizeApprovalSurfaceList(params.approved)); + return normalizeApprovalSurfaceList(params.declared).filter((entry) => approved.has(entry)); +} + +function intersectPermissionSurface(params: { + approved: Record | undefined; + declared: Record | undefined; +}): Record | undefined { + const entries: Array<[string, boolean]> = []; + for (const [key, declaredValue] of Object.entries(params.declared ?? {})) { + const approvedValue = params.approved?.[key]; + if (!declaredValue) { + entries.push([key, false]); + continue; + } + if (approvedValue === true) { + entries.push([key, true]); + continue; + } + if (approvedValue === false) { + entries.push([key, false]); + } + } + return entries.length > 0 ? Object.fromEntries(entries) : undefined; +} + function buildNodePairingRequestInput(params: { nodeId: string; connectParams: ConnectParams; + caps: string[]; commands: string[]; + permissions?: Record; remoteIp?: string; }): NodePairingRequestInput { return { @@ -45,8 +126,9 @@ function buildNodePairingRequestInput(params: { version: params.connectParams.client.version, deviceFamily: params.connectParams.client.deviceFamily, modelIdentifier: params.connectParams.client.modelIdentifier, - caps: params.connectParams.caps, + caps: params.caps, commands: params.commands, + permissions: params.permissions, remoteIp: params.remoteIp, }; } @@ -56,7 +138,7 @@ export async function reconcileNodePairingOnConnect(params: { connectParams: ConnectParams; pairedNode: NodePairingPairedNode | null; reportedClientIp?: string; - requestPairing: (input: NodePairingRequestInput) => Promise; + requestPairing: (input: NodePairingRequestInput) => Promise; }): Promise { const nodeId = params.connectParams.device?.id ?? params.connectParams.client.id; const allowlist = resolveNodeCommandAllowlist(params.cfg, { @@ -71,19 +153,25 @@ export async function reconcileNodePairingOnConnect(params: { : [], allowlist, }); + const declaredCaps = normalizeApprovalSurfaceList(params.connectParams.caps); + const declaredPermissions = normalizePermissionMap(params.connectParams.permissions); if (!params.pairedNode) { const pendingPairing = await params.requestPairing( buildNodePairingRequestInput({ nodeId, connectParams: params.connectParams, + caps: declaredCaps, commands: declared, + permissions: declaredPermissions, remoteIp: params.reportedClientIp, }), ); return { nodeId, + effectiveCaps: declaredCaps, effectiveCommands: declared, + effectivePermissions: declaredPermissions, pendingPairing, }; } @@ -92,26 +180,48 @@ export async function reconcileNodePairingOnConnect(params: { pairedCommands: params.pairedNode.commands, allowlist, }); + const approvedCaps = normalizeApprovalSurfaceList(params.pairedNode.caps); + const approvedPermissions = normalizePermissionMap(params.pairedNode.permissions); const hasCommandUpgrade = declared.some((command) => !approvedCommands.includes(command)); + const hasCapabilityChange = !sameApprovalSurfaceSet(params.pairedNode.caps, declaredCaps); + const hasPermissionChange = !samePermissions(params.pairedNode.permissions, declaredPermissions); + const effectiveApprovedDeclaredCaps = intersectApprovalSurfaceList({ + approved: approvedCaps, + declared: declaredCaps, + }); + const effectiveApprovedDeclaredCommands = intersectApprovalSurfaceList({ + approved: approvedCommands, + declared, + }); + const effectiveApprovedDeclaredPermissions = intersectPermissionSurface({ + approved: approvedPermissions, + declared: declaredPermissions, + }); - if (hasCommandUpgrade) { + if (hasCommandUpgrade || hasCapabilityChange || hasPermissionChange) { const pendingPairing = await params.requestPairing( buildNodePairingRequestInput({ nodeId, connectParams: params.connectParams, + caps: declaredCaps, commands: declared, + permissions: declaredPermissions ?? (hasPermissionChange ? {} : undefined), remoteIp: params.reportedClientIp, }), ); return { nodeId, - effectiveCommands: approvedCommands, + effectiveCaps: effectiveApprovedDeclaredCaps, + effectiveCommands: effectiveApprovedDeclaredCommands, + effectivePermissions: effectiveApprovedDeclaredPermissions, pendingPairing, }; } return { nodeId, + effectiveCaps: declaredCaps, effectiveCommands: declared, + effectivePermissions: declaredPermissions, }; } diff --git a/src/gateway/protocol/index.test.ts b/src/gateway/protocol/index.test.ts index 732e7412ae93..f89a222cfb08 100644 --- a/src/gateway/protocol/index.test.ts +++ b/src/gateway/protocol/index.test.ts @@ -5,6 +5,7 @@ import { formatValidationErrors, validateModelsListParams, validateNodeEventResult, + validateNodePairRequestParams, validateNodePresenceAlivePayload, validateTasksCancelParams, validateTasksListParams, @@ -507,6 +508,27 @@ describe("validateNodePresenceAlivePayload", () => { }); }); +describe("validateNodePairRequestParams", () => { + it("accepts node pairing permissions", () => { + expect( + validateNodePairRequestParams({ + nodeId: "ios-node-1", + commands: ["canvas.snapshot"], + permissions: { camera: true, notifications: false }, + }), + ).toBe(true); + }); + + it("rejects non-boolean node pairing permissions", () => { + expect( + validateNodePairRequestParams({ + nodeId: "ios-node-1", + permissions: { camera: "yes" }, + }), + ).toBe(false); + }); +}); + describe("validateNodeEventResult", () => { it("accepts structured handled results", () => { expect( diff --git a/src/gateway/protocol/schema/nodes.ts b/src/gateway/protocol/schema/nodes.ts index eed4bf161b92..5cd535fc714b 100644 --- a/src/gateway/protocol/schema/nodes.ts +++ b/src/gateway/protocol/schema/nodes.ts @@ -56,6 +56,7 @@ export const NodePairRequestParamsSchema = Type.Object( modelIdentifier: Type.Optional(NonEmptyString), caps: Type.Optional(Type.Array(NonEmptyString)), commands: Type.Optional(Type.Array(NonEmptyString)), + permissions: Type.Optional(Type.Record(NonEmptyString, Type.Boolean())), remoteIp: Type.Optional(NonEmptyString), silent: Type.Optional(Type.Boolean()), }, diff --git a/src/gateway/server-methods/nodes.invoke-wake.test.ts b/src/gateway/server-methods/nodes.invoke-wake.test.ts index 7ed1c77c74a4..a4ccaa3a34a2 100644 --- a/src/gateway/server-methods/nodes.invoke-wake.test.ts +++ b/src/gateway/server-methods/nodes.invoke-wake.test.ts @@ -33,6 +33,7 @@ const mocks = vi.hoisted(() => ({ sendApnsBackgroundWake: vi.fn(), sendApnsAlert: vi.fn(), shouldClearStoredApnsRegistration: vi.fn(() => false), + requestNodePairing: vi.fn(), })); vi.mock("../../config/io.js", () => ({ @@ -59,6 +60,16 @@ vi.mock("../../infra/push-apns.js", () => ({ shouldClearStoredApnsRegistration: mocks.shouldClearStoredApnsRegistration, })); +vi.mock("../../infra/node-pairing.js", async () => { + const actual = await vi.importActual( + "../../infra/node-pairing.js", + ); + return { + ...actual, + requestNodePairing: mocks.requestNodePairing, + }; +}); + type RespondCall = [ boolean, unknown?, @@ -332,6 +343,62 @@ async function ackPending(nodeId: string, ids: string[], commands?: string[]) { return respond; } +describe("node.pair.request", () => { + it("passes permissions and resolves superseded prompts before broadcasting replacement requests", async () => { + mocks.requestNodePairing.mockResolvedValue({ + status: "pending", + created: true, + request: { + requestId: "req-new", + nodeId: "ios-node-1", + commands: ["canvas.snapshot"], + permissions: { camera: true }, + ts: 1, + }, + superseded: [{ requestId: "req-old", nodeId: "ios-node-1" }], + }); + const respond = vi.fn(); + const broadcast = vi.fn(); + + await nodeHandlers["node.pair.request"]({ + params: { + nodeId: "ios-node-1", + commands: ["canvas.snapshot"], + permissions: { camera: true }, + }, + respond: respond as never, + context: { broadcast } as never, + client: null, + req: { type: "req", id: "req-node-pair", method: "node.pair.request" }, + isWebchatConnect: () => false, + }); + + expect(mocks.requestNodePairing).toHaveBeenCalledWith( + expect.objectContaining({ + nodeId: "ios-node-1", + commands: ["canvas.snapshot"], + permissions: { camera: true }, + }), + ); + expect(broadcast.mock.calls[0]?.[0]).toBe("node.pair.resolved"); + expect(broadcast.mock.calls[0]?.[1]).toEqual({ + requestId: "req-old", + nodeId: "ios-node-1", + decision: "rejected", + ts: expect.any(Number), + }); + expect(broadcast.mock.calls[1]?.[0]).toBe("node.pair.requested"); + expect(broadcast.mock.calls[1]?.[1]).toEqual( + expect.objectContaining({ + requestId: "req-new", + nodeId: "ios-node-1", + permissions: { camera: true }, + }), + ); + expect(respond.mock.calls[0]?.[0]).toBe(true); + }); +}); + describe("node plugin surface refresh", () => { it("refreshes generic plugin surface capability urls", async () => { vi.useFakeTimers(); diff --git a/src/gateway/server-methods/nodes.ts b/src/gateway/server-methods/nodes.ts index bda3e3799872..3ca697560afc 100644 --- a/src/gateway/server-methods/nodes.ts +++ b/src/gateway/server-methods/nodes.ts @@ -667,6 +667,19 @@ export const nodeHandlers: GatewayRequestHandlers = { remoteIp: p.remoteIp, silent: p.silent, }); + const resolvedAt = Date.now(); + for (const superseded of result.superseded ?? []) { + context.broadcast( + "node.pair.resolved", + { + requestId: superseded.requestId, + nodeId: superseded.nodeId, + decision: "rejected", + ts: resolvedAt, + }, + { dropIfSlow: true }, + ); + } if (result.status === "pending" && result.created) { context.broadcast("node.pair.requested", result.request, { dropIfSlow: true, diff --git a/src/gateway/server/ws-connection/message-handler.ts b/src/gateway/server/ws-connection/message-handler.ts index f15a3333b0c7..dc7b0974b915 100644 --- a/src/gateway/server/ws-connection/message-handler.ts +++ b/src/gateway/server/ws-connection/message-handler.ts @@ -1338,11 +1338,26 @@ export function attachGatewayWsMessageHandler(params: GatewayWsMessageHandlerPar }); if (reconciliation.pendingPairing?.created) { const requestContext = buildRequestContext(); + const resolvedAt = Date.now(); + for (const superseded of reconciliation.pendingPairing.superseded ?? []) { + requestContext.broadcast( + "node.pair.resolved", + { + requestId: superseded.requestId, + nodeId: superseded.nodeId, + decision: "rejected", + ts: resolvedAt, + }, + { dropIfSlow: true }, + ); + } requestContext.broadcast("node.pair.requested", reconciliation.pendingPairing.request, { dropIfSlow: true, }); } + connectParams.caps = reconciliation.effectiveCaps; connectParams.commands = reconciliation.effectiveCommands; + connectParams.permissions = reconciliation.effectivePermissions; } const shouldTrackPresence = !isGatewayCliClient(connectParams.client); diff --git a/src/infra/node-pairing.test.ts b/src/infra/node-pairing.test.ts index 01fa3ea1b3dd..c14b9d7b3210 100644 --- a/src/infra/node-pairing.test.ts +++ b/src/infra/node-pairing.test.ts @@ -66,7 +66,7 @@ describe("node pairing tokens", () => { await tempDirs.cleanup(); }); - test("reuses and refreshes pending requests", async () => { + test("reuses pending requests for metadata refreshes", async () => { await withNodePairingDir(async (baseDir) => { const first = await requestNodePairing( { @@ -101,34 +101,43 @@ describe("node pairing tokens", () => { nodeId: "node-2", platform: "darwin", displayName: "Updated Node", - commands: ["canvas.snapshot", "system.run"], - }, - baseDir, - ); - const commandThird = await requestNodePairing( - { - nodeId: "node-2", - platform: "darwin", - displayName: "Updated Node", - commands: ["canvas.snapshot", "system.run", "system.which"], + commands: ["canvas.snapshot"], }, baseDir, ); expect(commandSecond.created).toBe(false); + expect(commandSecond.superseded).toBeUndefined(); expect(commandSecond.request.requestId).toBe(commandFirst.request.requestId); - expect(commandThird.created).toBe(false); - expect(commandThird.request.requestId).toBe(commandSecond.request.requestId); - expect(commandThird.request.displayName).toBe("Updated Node"); - expect(commandThird.request.commands).toEqual([ - "canvas.snapshot", - "system.run", - "system.which", - ]); + expect(commandSecond.request.displayName).toBe("Updated Node"); + expect(commandSecond.request.commands).toEqual(["canvas.snapshot"]); + + const reorderedFirst = await requestNodePairing( + { + nodeId: "node-3", + platform: "darwin", + caps: ["camera", "screen"], + commands: ["canvas.snapshot", "system.run"], + }, + baseDir, + ); + const reorderedSecond = await requestNodePairing( + { + nodeId: "node-3", + platform: "darwin", + caps: ["screen", "camera"], + commands: ["system.run", "canvas.snapshot"], + }, + baseDir, + ); + + expect(reorderedSecond.created).toBe(false); + expect(reorderedSecond.superseded).toBeUndefined(); + expect(reorderedSecond.request.requestId).toBe(reorderedFirst.request.requestId); await requestNodePairing( { - nodeId: "node-3", + nodeId: "node-4", platform: "darwin", commands: ["canvas.present"], }, @@ -136,13 +145,112 @@ describe("node pairing tokens", () => { ); const pairing = await listNodePairing(baseDir); - const pendingNode = findRecordByField(pairing.pending, "nodeId", "node-3"); + const pendingNode = findRecordByField(pairing.pending, "nodeId", "node-4"); expect(pendingNode.commands).toEqual(["canvas.present"]); expect(pendingNode.requiredApproveScopes).toEqual(["operator.pairing", "operator.write"]); expect(pairing.paired).toEqual([]); }); }); + test("supersedes pending requests when the approval surface changes", async () => { + await withNodePairingDir(async (baseDir) => { + const first = await requestNodePairing( + { + nodeId: "node-1", + platform: "darwin", + caps: ["camera"], + commands: ["canvas.snapshot"], + permissions: { camera: true }, + }, + baseDir, + ); + const second = await requestNodePairing( + { + nodeId: "node-1", + platform: "darwin", + commands: ["canvas.snapshot", "system.run"], + }, + baseDir, + ); + + expect(second.created).toBe(true); + expect(second.superseded).toEqual([{ requestId: first.request.requestId, nodeId: "node-1" }]); + expect(second.request.requestId).not.toBe(first.request.requestId); + + const list = await listNodePairing(baseDir); + expect(list.pending).toHaveLength(1); + expect(list.pending[0]?.requestId).toBe(second.request.requestId); + expect(list.pending[0]?.caps).toEqual(["camera"]); + expect(list.pending[0]?.commands).toEqual(["canvas.snapshot", "system.run"]); + expect(list.pending[0]?.permissions).toEqual({ camera: true }); + + await expect( + approveNodePairing( + first.request.requestId, + { callerScopes: ["operator.pairing", "operator.admin"] }, + baseDir, + ), + ).resolves.toBeNull(); + + const approved = await approveNodePairing( + second.request.requestId, + { callerScopes: ["operator.pairing", "operator.admin"] }, + baseDir, + ); + const approvedRecord = requireRecord(approved); + const approvedNode = requireRecord(approvedRecord.node); + expect(approvedRecord.requestId).toBe(second.request.requestId); + expect(approvedNode.caps).toEqual(["camera"]); + expect(approvedNode.commands).toEqual(["canvas.snapshot", "system.run"]); + expect(approvedNode.permissions).toEqual({ camera: true }); + + const capsFirst = await requestNodePairing( + { + nodeId: "node-2", + platform: "darwin", + caps: ["camera"], + }, + baseDir, + ); + const capsSecond = await requestNodePairing( + { + nodeId: "node-2", + platform: "darwin", + caps: ["camera", "screen"], + }, + baseDir, + ); + expect(capsSecond.created).toBe(true); + expect(capsSecond.superseded).toEqual([ + { requestId: capsFirst.request.requestId, nodeId: "node-2" }, + ]); + expect(capsSecond.request.requestId).not.toBe(capsFirst.request.requestId); + + const permissionsFirst = await requestNodePairing( + { + nodeId: "node-3", + platform: "darwin", + permissions: { camera: true }, + }, + baseDir, + ); + const permissionsSecond = await requestNodePairing( + { + nodeId: "node-3", + platform: "darwin", + permissions: { camera: true, screen: true }, + }, + baseDir, + ); + + expect(permissionsSecond.created).toBe(true); + expect(permissionsSecond.superseded).toEqual([ + { requestId: permissionsFirst.request.requestId, nodeId: "node-3" }, + ]); + expect(permissionsSecond.request.requestId).not.toBe(permissionsFirst.request.requestId); + }); + }); + test("recovers when pairing state files were written as arrays", async () => { await withNodePairingDir(async (baseDir) => { const paths = resolvePairingPaths(baseDir, "nodes"); diff --git a/src/infra/node-pairing.ts b/src/infra/node-pairing.ts index 53f25dcc6c36..500c8bc03f13 100644 --- a/src/infra/node-pairing.ts +++ b/src/infra/node-pairing.ts @@ -41,6 +41,15 @@ export type NodePairingPendingRequest = NodePairingRequestInput & { ts: number; }; +export type NodePairingSupersededRequest = Pick; + +export type RequestNodePairingResult = { + status: "pending"; + request: NodePairingPendingRequest; + created: boolean; + superseded?: NodePairingSupersededRequest[]; +}; + type NodePairingPendingEntry = NodePairingPendingRequest & { requiredApproveScopes: NodeApprovalScope[]; }; @@ -116,6 +125,82 @@ function refreshPendingNodePairingRequest( }; } +function normalizeApprovalSurfaceList(value: string[] | undefined): string[] { + return normalizeArrayBackedTrimmedStringList(value) ?? []; +} + +function sameApprovalSurfaceSet(left: string[] | undefined, right: string[] | undefined): boolean { + const normalizedLeft = new Set(normalizeApprovalSurfaceList(left)); + const normalizedRight = new Set(normalizeApprovalSurfaceList(right)); + if (normalizedLeft.size !== normalizedRight.size) { + return false; + } + for (const entry of normalizedLeft) { + if (!normalizedRight.has(entry)) { + return false; + } + } + return true; +} + +function samePermissions( + left: Record | undefined, + right: Record | undefined, +): boolean { + const leftEntries = Object.entries(left ?? {}).toSorted(([leftKey], [rightKey]) => + leftKey.localeCompare(rightKey), + ); + const rightEntries = Object.entries(right ?? {}).toSorted(([leftKey], [rightKey]) => + leftKey.localeCompare(rightKey), + ); + if (leftEntries.length !== rightEntries.length) { + return false; + } + return leftEntries.every(([key, value], index) => { + const rightEntry = rightEntries[index]; + return rightEntry !== undefined && rightEntry[0] === key && rightEntry[1] === value; + }); +} + +function samePendingApprovalSurface( + existing: NodePairingPendingRequest, + incoming: NodePairingRequestInput, +): boolean { + const incomingCaps = normalizeArrayBackedTrimmedStringList(incoming.caps) ?? existing.caps; + const incomingCommands = + normalizeArrayBackedTrimmedStringList(incoming.commands) ?? existing.commands; + const incomingPermissions = incoming.permissions ?? existing.permissions; + return ( + sameApprovalSurfaceSet(existing.caps, incomingCaps) && + sameApprovalSurfaceSet(existing.commands, incomingCommands) && + samePermissions(existing.permissions, incomingPermissions) + ); +} + +function mergeNodePairingReplacementInput(params: { + existing: readonly NodePairingPendingRequest[]; + incoming: NodePairingRequestInput; +}): NodePairingRequestInput { + const latest = params.existing[0]; + return { + nodeId: params.incoming.nodeId, + displayName: params.incoming.displayName ?? latest?.displayName, + platform: params.incoming.platform ?? latest?.platform, + version: params.incoming.version ?? latest?.version, + coreVersion: params.incoming.coreVersion ?? latest?.coreVersion, + uiVersion: params.incoming.uiVersion ?? latest?.uiVersion, + deviceFamily: params.incoming.deviceFamily ?? latest?.deviceFamily, + modelIdentifier: params.incoming.modelIdentifier ?? latest?.modelIdentifier, + caps: params.incoming.caps ?? latest?.caps, + commands: params.incoming.commands ?? latest?.commands, + permissions: params.incoming.permissions ?? latest?.permissions, + remoteIp: params.incoming.remoteIp ?? latest?.remoteIp, + silent: Boolean( + params.incoming.silent && params.existing.every((pending) => pending.silent === true), + ), + }; +} + function resolveNodeApprovalRequiredScopes( pending: NodePairingPendingRequest, ): NodeApprovalScope[] { @@ -186,11 +271,7 @@ export async function getPairedNode( export async function requestNodePairing( req: NodePairingRequestInput, baseDir?: string, -): Promise<{ - status: "pending"; - request: NodePairingPendingRequest; - created: boolean; -}> { +): Promise { return await withLock(async () => { const state = await loadState(baseDir); const nodeId = normalizeNodeId(req.nodeId); @@ -200,26 +281,27 @@ export async function requestNodePairing( const pendingForNode = Object.values(state.pendingById) .filter((pending) => pending.nodeId === nodeId) .toSorted((left, right) => right.ts - left.ts); - return await reconcilePendingPairingRequests({ + const result = await reconcilePendingPairingRequests({ pendingById: state.pendingById, existing: pendingForNode, incoming: { ...req, nodeId, }, - canRefreshSingle: () => true, + canRefreshSingle: (existing, incoming) => samePendingApprovalSurface(existing, incoming), refreshSingle: (existing, incoming) => refreshPendingNodePairingRequest(existing, incoming), buildReplacement: ({ existing, incoming }) => buildPendingNodePairingRequest({ - req: { - ...incoming, - silent: Boolean( - incoming.silent && existing.every((pending) => pending.silent === true), - ), - }, + req: mergeNodePairingReplacementInput({ existing, incoming }), }), persist: async () => await persistState(state, baseDir), }); + const superseded = result.created + ? pendingForNode + .filter((pending) => pending.requestId !== result.request.requestId) + .map((pending) => ({ requestId: pending.requestId, nodeId: pending.nodeId })) + : []; + return superseded.length > 0 ? { ...result, superseded } : result; }); }