Skip to content

Commit b63b50c

Browse files
steipetesteipete
committed
fix(gateway): stop device pairing approval alert floods from retrying devices
A device retrying with a broken token minted a new pending requestId (and approval alert broadcast) every 5-minute TTL window because refresh preserved ts and expiry keyed on ts. Pending requests now stay alive via an internal refreshedAtMs keepalive (ts still owns ordering/--latest), superseded device requests broadcast device.pair.resolved like node pairing already did, and the Mac app keeps one alert per device, closes superseded visible alerts, and resyncs its queue when approving a stale request. Closes #100974
1 parent 5733fb0 commit b63b50c

10 files changed

Lines changed: 190 additions & 21 deletions

File tree

‎apps/macos/Sources/OpenClaw/DevicePairingApprovalPrompter.swift‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,11 @@ final class DevicePairingApprovalPrompter {
146146

147147
switch response {
148148
case .alertFirstButtonReturn:
149-
_ = await self.approve(requestId: request.requestId)
149+
if !(await self.approve(requestId: request.requestId)) {
150+
// Stale request (expired or superseded on the gateway): re-sync the
151+
// queue with gateway truth so accumulated stale alerts collapse at once.
152+
await self.loadPendingRequestsFromGateway()
153+
}
150154
case .alertSecondButtonReturn:
151155
shouldRemove = false
152156
if let idx = self.queue.firstIndex(of: request) {
@@ -211,9 +215,29 @@ final class DevicePairingApprovalPrompter {
211215
}
212216
}
213217

218+
// The gateway keeps at most one live pending request per device, so a new
219+
// requestId for the same device supersedes anything still queued for it.
220+
// Without this, missed/dropped resolve pushes pile up as alerts whose
221+
// approval can no longer succeed. Returns nil when the request is already queued.
222+
static func coalescedQueue(_ queue: [PendingRequest], adding req: PendingRequest) -> [PendingRequest]? {
223+
guard !queue.contains(where: { $0.requestId == req.requestId }) else { return nil }
224+
return queue.filter { $0.deviceId != req.deviceId } + [req]
225+
}
226+
214227
private func enqueue(_ req: PendingRequest) {
215-
guard !self.queue.contains(req) else { return }
216-
self.queue.append(req)
228+
guard let next = Self.coalescedQueue(self.queue, adding: req) else { return }
229+
let supersededActiveId = self.alertState.activeRequestId.flatMap { activeId in
230+
self.queue.contains(where: {
231+
$0.requestId == activeId && $0.deviceId == req.deviceId
232+
}) ? activeId : nil
233+
}
234+
self.queue = next
235+
if let supersededActiveId {
236+
// The visible alert is for a superseded requestId; close it so the
237+
// fresh request presents instead of an approve that would no-op.
238+
self.resolvedByRequestId.insert(supersededActiveId)
239+
self.endActiveAlert()
240+
}
217241
self.updatePendingCounts()
218242
self.presentNextIfNeeded()
219243
}

‎apps/macos/Sources/OpenClaw/NodePairingApprovalPrompter.swift‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,11 @@ final class NodePairingApprovalPrompter {
265265
}
266266

267267
private func enqueue(_ req: PendingRequest) {
268-
if self.queue.contains(req) { return }
268+
if self.queue.contains(where: { $0.requestId == req.requestId }) { return }
269+
// The gateway keeps at most one live pending request per node; a newer
270+
// request supersedes queued ones so missed resolve pushes cannot stack
271+
// stale alerts. The actively presented request stays owned by its alert.
272+
self.queue.removeAll { $0.nodeId == req.nodeId && $0.requestId != self.alertState.activeRequestId }
269273
self.queue.append(req)
270274
self.updatePendingCounts()
271275
self.presentNextIfNeeded()

‎apps/macos/Tests/OpenClawIPCTests/NodePairingApprovalPrompterTests.swift‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,37 @@ struct NodePairingApprovalPrompterTests {
9191
#expect(accessory.frame.width >= 380)
9292
#expect(accessory.frame.height > 80)
9393
}
94+
95+
@Test func `a newer device pairing request supersedes queued requests for the same device`() {
96+
func request(_ requestId: String, deviceId: String, ts: Double) -> DevicePairingApprovalPrompter
97+
.PendingRequest
98+
{
99+
DevicePairingApprovalPrompter.PendingRequest(
100+
requestId: requestId,
101+
deviceId: deviceId,
102+
publicKey: "pub",
103+
displayName: nil,
104+
platform: "MacIntel",
105+
clientId: nil,
106+
clientMode: nil,
107+
role: "node",
108+
scopes: nil,
109+
remoteIp: nil,
110+
silent: nil,
111+
isRepair: true,
112+
ts: ts)
113+
}
114+
115+
let stale1 = request("req-1", deviceId: "device-a", ts: 1)
116+
let stale2 = request("req-2", deviceId: "device-a", ts: 2)
117+
let other = request("req-3", deviceId: "device-b", ts: 3)
118+
let fresh = request("req-4", deviceId: "device-a", ts: 4)
119+
120+
// Stale requests for the same device collapse; other devices are untouched.
121+
let coalesced = DevicePairingApprovalPrompter.coalescedQueue([stale1, stale2, other], adding: fresh)
122+
#expect(coalesced?.map(\.requestId) == ["req-3", "req-4"])
123+
124+
// Re-delivery of an already queued requestId is a no-op.
125+
#expect(DevicePairingApprovalPrompter.coalescedQueue([stale1, other], adding: stale1) == nil)
126+
}
94127
}

‎docs/gateway/pairing.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@ handshake. Only clients that explicitly call `node.pair.*` use this flow.
3030
4. On approval, the Gateway issues a **new token** (tokens rotate on re-pair).
3131
5. The node reconnects using the token and is now paired.
3232

33-
Pending requests expire automatically after **5 minutes**.
33+
Pending requests expire automatically **5 minutes after the node's last
34+
retry** — an actively reconnecting node keeps its one pending request alive
35+
rather than generating a fresh request (and approval prompt) per attempt.
3436

3537
## CLI workflow (headless friendly)
3638

‎docs/nodes/index.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ openclaw nodes status
2929
openclaw nodes describe --node <idOrNameOrIp>
3030
```
3131

32-
Pending pairing requests expire after 5 minutes; see [Gateway-owned pairing](/gateway/pairing) for the full request/approve/token lifecycle. If a node retries with changed auth details (role/scopes/public key), the prior pending request is superseded and a new `requestId` is created — re-run `openclaw devices list` before approving.
32+
Pending pairing requests expire 5 minutes after the device's last retry — a device that keeps reconnecting keeps its one pending request (and `requestId`) alive instead of minting a new prompt every few minutes; see [Gateway-owned pairing](/gateway/pairing) for the full request/approve/token lifecycle. If a node retries with changed auth details (role/scopes/public key), the prior pending request is superseded and a new `requestId` is created — clients get a `device.pair.resolved` event for the superseded request, and you should re-run `openclaw devices list` before approving.
3333

3434
- `nodes status` marks a node as **paired** when its device pairing role includes `node`.
3535
- The device pairing record is the durable approved-role contract. Token rotation stays inside that contract; it cannot upgrade a paired node into a role that pairing approval never granted.

‎src/gateway/server/ws-connection/message-handler.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1443,6 +1443,21 @@ export function attachGatewayWsMessageHandler(params: GatewayWsMessageHandlerPar
14431443
allowSetupCodeMobileBootstrapPairing,
14441444
});
14451445
const context = buildRequestContext();
1446+
// A replacement request obsoletes older pending requestIds; tell approval
1447+
// UIs so they drop the stale prompts instead of stacking alerts forever.
1448+
const supersededResolvedAt = Date.now();
1449+
for (const superseded of pairing.superseded ?? []) {
1450+
context.broadcast(
1451+
"device.pair.resolved",
1452+
{
1453+
requestId: superseded.requestId,
1454+
deviceId: superseded.deviceId,
1455+
decision: "rejected",
1456+
ts: supersededResolvedAt,
1457+
},
1458+
{ dropIfSlow: true },
1459+
);
1460+
}
14461461
let approved: Awaited<ReturnType<typeof approveDevicePairing>> | undefined;
14471462
let resolvedByConcurrentApproval = false;
14481463
let recoveryRequestId: string | undefined;

‎src/infra/device-pairing-churn.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,9 @@ describe("device pairing requestId churn", () => {
9292

9393
expect(approveReconnect.created).toBe(true);
9494
expect(approveReconnect.request.requestId).not.toBe(readRepair.request.requestId);
95+
expect(approveReconnect.superseded).toEqual([
96+
{ requestId: readRepair.request.requestId, deviceId: DEVICE_ID },
97+
]);
9598

9699
const staleApprove = await approveDevicePairing(
97100
readRepair.request.requestId,

‎src/infra/device-pairing.test.ts‎

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,50 @@ describe("device pairing tokens", () => {
293293
expect(second.request.ts).toBe(originalTs);
294294
});
295295

296+
test("re-requests keep one pending request alive past the pending TTL without churning requestIds", async () => {
297+
// Regression: a device retrying all night must not mint a new requestId (and a
298+
// new approval prompt broadcast) every TTL window. Refreshes stamp refreshedAtMs
299+
// as a TTL keepalive while ts stays the creation time for approval ordering.
300+
const baseDir = await makeDevicePairingDir();
301+
const req = {
302+
deviceId: "device-1",
303+
publicKey: "public-key-1",
304+
role: "operator" as const,
305+
scopes: ["operator.read"],
306+
};
307+
const first = await requestDevicePairing(req, baseDir);
308+
const refreshed = await requestDevicePairing(req, baseDir);
309+
expect(refreshed.created).toBe(false);
310+
311+
// Simulate hours of aging since creation while retries kept the keepalive fresh.
312+
const paths = resolvePairingPaths(baseDir, "devices");
313+
const pendingById = JSON.parse(await readFile(paths.pendingPath, "utf8")) as Record<
314+
string,
315+
{ ts: number; refreshedAtMs?: number }
316+
>;
317+
const pending = requireValue(
318+
pendingById[first.request.requestId],
319+
"expected pending pairing request",
320+
);
321+
expect(pending.refreshedAtMs).toBeGreaterThanOrEqual(pending.ts);
322+
const createdTs = Date.now() - 60 * 60 * 1000;
323+
pending.ts = createdTs;
324+
await writeFile(paths.pendingPath, JSON.stringify(pendingById, null, 2));
325+
326+
const third = await requestDevicePairing(req, baseDir);
327+
expect(third.created).toBe(false);
328+
expect(third.request.requestId).toBe(first.request.requestId);
329+
expect(third.request.ts).toBe(createdTs);
330+
// The keepalive is store-internal; it must not leak into protocol payloads.
331+
expect("refreshedAtMs" in third.request).toBe(false);
332+
333+
// A stale keepalive still expires the request.
334+
pending.refreshedAtMs = createdTs;
335+
pendingById[first.request.requestId] = pending;
336+
await writeFile(paths.pendingPath, JSON.stringify(pendingById, null, 2));
337+
expect((await listDevicePairing(baseDir)).pending).toHaveLength(0);
338+
});
339+
296340
test("supersedes pending requests when requested roles/scopes change", async () => {
297341
const baseDir = await makeDevicePairingDir();
298342
const first = await requestDevicePairing(
@@ -316,6 +360,9 @@ describe("device pairing tokens", () => {
316360

317361
expect(second.created).toBe(true);
318362
expect(second.request.requestId).not.toBe(first.request.requestId);
363+
expect(second.superseded).toEqual([
364+
{ requestId: first.request.requestId, deviceId: "device-1" },
365+
]);
319366
expect(second.request.role).toBe("operator");
320367
expectArrayIncludesAll(second.request.roles, ["node", "operator"], "request roles");
321368
expectArrayIncludesAll(
@@ -1934,5 +1981,4 @@ describe("device pairing tokens", () => {
19341981
).rejects.toThrow(/paired\.json/);
19351982
await expect(readFile(pairedPath, "utf8")).resolves.toBe("{not-json}");
19361983
});
1937-
19381984
});

‎src/infra/device-pairing.ts‎

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,26 @@ export type DevicePairingPendingRequest = {
4343
ts: number;
4444
};
4545

46+
// Internal pending record. refreshedAtMs is a TTL keepalive stamped on refresh so an
47+
// actively retrying device keeps one pending request (and requestId) alive instead of
48+
// minting a new request every TTL window and flooding operator approval UIs. It never
49+
// crosses the protocol boundary, and ordering/--latest still use ts.
50+
type DevicePairingPendingRecord = DevicePairingPendingRequest & { refreshedAtMs?: number };
51+
52+
/** Pending request summary returned when a replacement supersedes older requests. */
53+
export type DevicePairingSupersededRequest = Pick<
54+
DevicePairingPendingRequest,
55+
"requestId" | "deviceId"
56+
>;
57+
58+
/** Result for creating or refreshing a pending device pairing request. */
59+
export type RequestDevicePairingResult = {
60+
status: "pending";
61+
request: DevicePairingPendingRequest;
62+
created: boolean;
63+
superseded?: DevicePairingSupersededRequest[];
64+
};
65+
4666
/** Bearer token issued to one paired device role. */
4767
export type DeviceAuthToken = {
4868
token: string;
@@ -155,7 +175,7 @@ export type ApproveDevicePairingResult =
155175
| null;
156176

157177
type DevicePairingStateFile = {
158-
pendingById: Record<string, DevicePairingPendingRequest>;
178+
pendingById: Record<string, DevicePairingPendingRecord>;
159179
pairedByDeviceId: Record<string, PairedDevice>;
160180
};
161181

@@ -192,7 +212,7 @@ async function loadState(baseDir?: string): Promise<DevicePairingStateFile> {
192212
readJsonIfExists<unknown>(pairedPath),
193213
]);
194214
const state: DevicePairingStateFile = {
195-
pendingById: coercePairingStateRecord<DevicePairingPendingRequest>(pending),
215+
pendingById: coercePairingStateRecord<DevicePairingPendingRecord>(pending),
196216
pairedByDeviceId: coercePairingStateRecord<PairedDevice>(paired),
197217
};
198218
pruneExpiredPending(state.pendingById, Date.now(), PENDING_TTL_MS);
@@ -395,10 +415,10 @@ function incomingApprovalCoveredByExisting(
395415
}
396416

397417
function refreshPendingDevicePairingRequest(
398-
existing: DevicePairingPendingRequest,
418+
existing: DevicePairingPendingRecord,
399419
incoming: Omit<DevicePairingPendingRequest, "requestId" | "ts" | "isRepair">,
400420
isRepair: boolean,
401-
): DevicePairingPendingRequest {
421+
): DevicePairingPendingRecord {
402422
return {
403423
...existing,
404424
publicKey: incoming.publicKey,
@@ -415,6 +435,8 @@ function refreshPendingDevicePairingRequest(
415435
// request's queue position. Using Date.now() here would let an attacker silently
416436
// refresh recency and win the implicit --latest approval race.
417437
ts: existing.ts,
438+
// Keepalive for the pending TTL only (see pruneExpiredPending); never affects ordering.
439+
refreshedAtMs: Date.now(),
418440
};
419441
}
420442

@@ -427,6 +449,13 @@ function resolveSupersededPendingSilent(params: {
427449
);
428450
}
429451

452+
function toPublicPendingDevicePairingRequest(
453+
pending: DevicePairingPendingRecord,
454+
): DevicePairingPendingRequest {
455+
const { refreshedAtMs: _refreshedAtMs, ...request } = pending;
456+
return request;
457+
}
458+
430459
function buildPendingDevicePairingRequest(params: {
431460
requestId?: string;
432461
deviceId: string;
@@ -613,7 +642,9 @@ function scopesWithinApprovedDeviceBaseline(params: {
613642

614643
export async function listDevicePairing(baseDir?: string): Promise<DevicePairingList> {
615644
const state = await loadState(baseDir);
616-
const pending = Object.values(state.pendingById).toSorted((a, b) => b.ts - a.ts);
645+
const pending = Object.values(state.pendingById)
646+
.map(toPublicPendingDevicePairingRequest)
647+
.toSorted((a, b) => b.ts - a.ts);
617648
const paired = Object.values(state.pairedByDeviceId).toSorted(
618649
(a, b) => b.approvedAtMs - a.approvedAtMs,
619650
);
@@ -635,18 +666,15 @@ export async function getPendingDevicePairing(
635666
baseDir?: string,
636667
): Promise<DevicePairingPendingRequest | null> {
637668
const state = await loadState(baseDir);
638-
return state.pendingById[requestId] ?? null;
669+
const pending = state.pendingById[requestId];
670+
return pending ? toPublicPendingDevicePairingRequest(pending) : null;
639671
}
640672

641673
/** Create or refresh a pending device pairing request for owner approval. */
642674
export async function requestDevicePairing(
643675
req: Omit<DevicePairingPendingRequest, "requestId" | "ts" | "isRepair">,
644676
baseDir?: string,
645-
): Promise<{
646-
status: "pending";
647-
request: DevicePairingPendingRequest;
648-
created: boolean;
649-
}> {
677+
): Promise<RequestDevicePairingResult> {
650678
return await withLock(async () => {
651679
const state = await loadState(baseDir);
652680
const deviceId = normalizeDeviceId(req.deviceId);
@@ -657,7 +685,7 @@ export async function requestDevicePairing(
657685
const pendingForDevice = Object.values(state.pendingById)
658686
.filter((pending) => pending.deviceId === deviceId)
659687
.toSorted((left, right) => right.ts - left.ts);
660-
return await reconcilePendingPairingRequests({
688+
const result = await reconcilePendingPairingRequests({
661689
pendingById: state.pendingById,
662690
existing: pendingForDevice,
663691
incoming: req,
@@ -696,6 +724,18 @@ export async function requestDevicePairing(
696724
},
697725
persist: async () => await persistState(state, baseDir, "pending"),
698726
});
727+
// Surface superseded requestIds so callers can broadcast their resolution;
728+
// clients otherwise keep prompting for requests that can no longer be approved.
729+
const superseded = result.created
730+
? pendingForDevice
731+
.filter((pending) => pending.requestId !== result.request.requestId)
732+
.map((pending) => ({ requestId: pending.requestId, deviceId: pending.deviceId }))
733+
: [];
734+
const publicResult = {
735+
...result,
736+
request: toPublicPendingDevicePairingRequest(result.request),
737+
};
738+
return superseded.length > 0 ? { ...publicResult, superseded } : publicResult;
699739
});
700740
}
701741

‎src/infra/pairing-files.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@ export function coercePairingStateRecord<T>(value: unknown): Record<string, T> {
2424
}
2525

2626
/** Remove pending requests older than the caller's pairing TTL. */
27-
export function pruneExpiredPending<T extends { ts: number }>(
27+
export function pruneExpiredPending<T extends { ts: number; refreshedAtMs?: number }>(
2828
pendingById: Record<string, T>,
2929
nowMs: number,
3030
ttlMs: number,
3131
) {
3232
for (const [id, req] of Object.entries(pendingById)) {
33-
if (nowMs - req.ts > ttlMs) {
33+
// refreshedAtMs is a TTL keepalive: expiry counts from the device's last
34+
// re-request, while ts stays the creation time for approval ordering.
35+
if (nowMs - (req.refreshedAtMs ?? req.ts) > ttlMs) {
3436
delete pendingById[id];
3537
}
3638
}

0 commit comments

Comments
 (0)