fix: Implicit latest-device approval can pair the wrong requester (#64160)

* fix: require confirmation before implicit device approval

Keep re-requested pairing entries from jumping the queue and force operators to confirm implicit latest-request approval so a refreshed attacker request cannot be silently approved.

* fix: require exact device pairing approval

* fix: stabilize reply CI checks

---------

Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
Coy Geek
2026-04-10 18:55:01 -07:00
committed by GitHub
parent f2065a7651
commit 192ee081e7
13 changed files with 315 additions and 77 deletions

View File

@@ -119,6 +119,7 @@ Docs: https://docs.openclaw.ai
- Heartbeat: stop top-level `interval:` and `prompt:` fields outside the `tasks:` block from bleeding into the last parsed heartbeat task. (#64488) Thanks @Rahulkumar070.
- Agents/OpenAI replay: preserve malformed function-call arguments in stored assistant history, avoid double-encoding preserved raw strings on replay, and coerce replayed string args back to objects at Anthropic and Google provider boundaries. (#61956) Thanks @100yenadmin.
- Heartbeat/config: accept and honor `agents.defaults.heartbeat.timeoutSeconds` and per-agent heartbeat timeout overrides for heartbeat agent turns. (#64491) Thanks @cedillarack.
- CLI/devices: make implicit `openclaw devices approve` selection preview-only and require approving the exact request ID, preventing latest-request races during device pairing. (#64160) Thanks @coygeek.
## 2026.4.9

View File

@@ -49,8 +49,10 @@ openclaw devices clear --yes --pending --json
### `openclaw devices approve [requestId] [--latest]`
Approve a pending device pairing request. If `requestId` is omitted, OpenClaw
automatically approves the most recent pending request.
Approve a pending device pairing request by exact `requestId`. If `requestId`
is omitted or `--latest` is passed, OpenClaw only prints the selected pending
request and exits; rerun approval with the exact request ID after verifying
the details.
Note: if a device retries pairing with changed auth details (role/scopes/public
key), OpenClaw supersedes the previous pending entry and issues a new
@@ -126,7 +128,7 @@ Pass `--token` or `--password` explicitly. Missing explicit credentials is an er
`operator.admin`.
- `devices clear` is intentionally gated by `--yes`.
- If pairing scope is unavailable on local loopback (and no explicit `--url` is passed), list/approve can use a local pairing fallback.
- `devices approve` picks the newest pending request automatically when you omit `requestId` or pass `--latest`.
- `devices approve` requires an explicit request ID before minting tokens; omitting `requestId` or passing `--latest` only previews the newest pending request.
## Token drift recovery checklist

View File

@@ -852,7 +852,7 @@ Subcommands:
Notes:
- `devices list` and `devices approve` can fall back to local pairing files on local loopback when direct pairing scope is unavailable.
- `devices approve` auto-selects the newest pending request when no `requestId` is passed or `--latest` is set.
- `devices approve` requires an explicit request ID before minting tokens; omitting `requestId` or passing `--latest` only previews the newest pending request.
- Stored-token reconnects reuse the token's cached approved scopes; explicit
`devices rotate --scope ...` updates that stored scope set for future
cached-token reconnects.

View File

@@ -96,13 +96,12 @@ export const handleCompactCommand: CommandHandler = async (params) => {
}
const sessionAgentId = params.sessionKey
? resolveSessionAgentId({ sessionKey: params.sessionKey, config: params.cfg })
: params.agentId;
const shouldResolveSessionAgentDir =
sessionAgentId !== undefined &&
(!params.agentDir || (params.agentId !== undefined && sessionAgentId !== params.agentId));
const sessionAgentDir = shouldResolveSessionAgentDir
? resolveAgentDir(params.cfg, sessionAgentId)
: params.agentDir;
: (params.agentId ?? "main");
const currentAgentId = params.agentId ?? "main";
const sessionAgentDir =
sessionAgentId === currentAgentId && params.agentDir
? params.agentDir
: resolveAgentDir(params.cfg, sessionAgentId);
const customInstructions = extractCompactInstructions({
rawBody: params.ctx.CommandBody ?? params.ctx.RawBody ?? params.ctx.Body,
ctx: params.ctx,

View File

@@ -14,7 +14,7 @@ const modelCatalogMocks = vi.hoisted(() => ({
}));
const modelAuthLabelMocks = vi.hoisted(() => ({
resolveModelAuthLabel: vi.fn(() => undefined),
resolveModelAuthLabel: vi.fn<(params: unknown) => string | undefined>(() => undefined),
}));
vi.mock("../../agents/model-catalog.js", () => ({
@@ -22,7 +22,7 @@ vi.mock("../../agents/model-catalog.js", () => ({
}));
vi.mock("../../agents/model-auth-label.js", () => ({
resolveModelAuthLabel: (...args: unknown[]) => modelAuthLabelMocks.resolveModelAuthLabel(...args),
resolveModelAuthLabel: (params: unknown) => modelAuthLabelMocks.resolveModelAuthLabel(params),
}));
const telegramModelsTestPlugin: ChannelPlugin = {
@@ -256,11 +256,15 @@ describe("handleModelsCommand", () => {
sessionKey: "agent:support:main",
});
params.sessionEntry = {
sessionId: "wrapper-session",
updatedAt: Date.now(),
providerOverride: "wrapper-provider",
modelOverride: "wrapper-model",
} as HandleCommandsParams["sessionEntry"];
params.sessionStore = {
"agent:support:main": {
sessionId: "target-session",
updatedAt: Date.now(),
providerOverride: "target-provider",
modelOverride: "target-model",
},

View File

@@ -392,8 +392,12 @@ export const handleModelsCommand: CommandHandler = async (params, allowTextComma
sessionKey: params.sessionKey,
config: params.cfg,
})
: params.agentId;
const modelsAgentDir = modelsAgentId ? resolveAgentDir(params.cfg, modelsAgentId) : undefined;
: (params.agentId ?? "main");
const currentAgentId = params.agentId ?? "main";
const modelsAgentDir =
modelsAgentId === currentAgentId && params.agentDir
? params.agentDir
: resolveAgentDir(params.cfg, modelsAgentId);
const targetSessionEntry = params.sessionStore?.[params.sessionKey] ?? params.sessionEntry;
const reply = await resolveModelsCommandReply({

View File

@@ -171,6 +171,7 @@ describe("resolveCommandsSystemPromptBundle", () => {
const params = makeParams();
params.sessionEntry = {
sessionId: "wrapper-session",
updatedAt: Date.now(),
groupId: "wrapper-group",
groupChannel: "#wrapper",
space: "wrapper-space",
@@ -179,6 +180,7 @@ describe("resolveCommandsSystemPromptBundle", () => {
params.sessionStore = {
"agent:target:telegram:direct:target-session": {
sessionId: "target-session",
updatedAt: Date.now(),
groupId: "target-group",
groupChannel: "#target",
space: "target-space",

View File

@@ -84,6 +84,39 @@ describe("devices cli approve", () => {
);
});
it("prints selected details and exits when implicit approval is used", async () => {
callGateway.mockResolvedValueOnce({
pending: [
{
requestId: "req-abc",
deviceId: "device-9",
displayName: "Device Nine",
role: "operator",
scopes: ["operator.admin"],
remoteIp: "10.0.0.9",
ts: 1000,
},
],
});
await runDevicesApprove([]);
expect(callGateway).toHaveBeenCalledTimes(1);
expect(callGateway).toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.list" }),
);
const logOutput = runtime.log.mock.calls.map((c) => readRuntimeCallText(c)).join("\n");
expect(logOutput).toContain("req-abc");
expect(logOutput).toContain("Device Nine");
expect(runtime.error).toHaveBeenCalledWith(
expect.stringContaining("openclaw devices approve req-abc"),
);
expect(runtime.exit).toHaveBeenCalledWith(1);
expect(callGateway).not.toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.approve" }),
);
});
it.each([
{
name: "id is omitted",
@@ -103,12 +136,10 @@ describe("devices cli approve", () => {
],
expectedRequestId: "req-3",
},
])("uses latest pending request when $name", async ({ args, pending, expectedRequestId }) => {
callGateway
.mockResolvedValueOnce({
pending,
})
.mockResolvedValueOnce({ device: { deviceId: "device-2" } });
])("previews latest pending request when $name", async ({ args, pending, expectedRequestId }) => {
callGateway.mockResolvedValueOnce({
pending,
});
await runDevicesApprove(args);
@@ -116,12 +147,84 @@ describe("devices cli approve", () => {
1,
expect.objectContaining({ method: "device.pair.list" }),
);
expect(callGateway).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
method: "device.pair.approve",
params: { requestId: expectedRequestId },
}),
expect(callGateway).not.toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.approve" }),
);
expect(runtime.error).toHaveBeenCalledWith(
expect.stringContaining(`openclaw devices approve ${expectedRequestId}`),
);
});
it("falls back to device id when selected pending display name is blank", async () => {
callGateway.mockResolvedValueOnce({
pending: [
{
requestId: "req-blank",
deviceId: "device-9",
displayName: " ",
ts: 1000,
},
],
});
await runDevicesApprove([]);
const logOutput = runtime.log.mock.calls.map((c) => readRuntimeCallText(c)).join("\n");
expect(logOutput).toContain("device-9");
expect(runtime.error).toHaveBeenCalledWith(
expect.stringContaining("openclaw devices approve req-blank"),
);
expect(callGateway).not.toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.approve" }),
);
});
it("includes explicit gateway flags in the rerun approval command", async () => {
callGateway.mockResolvedValueOnce({
pending: [{ requestId: "req-url", deviceId: "device-9", ts: 1000 }],
});
await runDevicesApprove([
"--latest",
"--url",
"ws://gateway.example:18789/openclaw?cluster=qa lab",
"--timeout",
"3000",
"--token",
"secret-token",
]);
const errorOutput = runtime.error.mock.calls.map((c) => readRuntimeCallText(c)).join("\n");
expect(errorOutput).toContain(
"openclaw devices approve req-url --url 'ws://gateway.example:18789/openclaw?cluster=qa lab' --timeout 3000",
);
expect(errorOutput).toContain("Reuse the same --token option when rerunning.");
expect(errorOutput).not.toContain("secret-token");
expect(callGateway).not.toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.approve" }),
);
});
it("returns JSON for implicit approval preview in JSON mode", async () => {
callGateway.mockResolvedValueOnce({
pending: [{ requestId: "req-json", deviceId: "device-json", ts: 1000 }],
});
await runDevicesApprove(["--latest", "--json", "--url", "ws://gateway.example:18789"]);
expect(runtime.log).not.toHaveBeenCalled();
expect(runtime.error).not.toHaveBeenCalled();
expect(runtime.writeJson).toHaveBeenCalledWith({
selected: { requestId: "req-json", deviceId: "device-json", ts: 1000 },
approveCommand: "openclaw devices approve req-json --url ws://gateway.example:18789 --json",
requiresAuthFlags: {
token: false,
password: false,
},
});
expect(runtime.exit).toHaveBeenCalledWith(1);
expect(callGateway).not.toHaveBeenCalledWith(
expect.objectContaining({ method: "device.pair.approve" }),
);
});
@@ -269,13 +372,7 @@ describe("devices cli local fallback", () => {
});
it("falls back to local approve when gateway returns pairing required on loopback", async () => {
callGateway
.mockRejectedValueOnce(new Error("gateway closed (1008): pairing required"))
.mockRejectedValueOnce(new Error("gateway closed (1008): pairing required"));
listDevicePairing.mockResolvedValueOnce({
pending: [{ requestId: "req-latest", deviceId: "device-1", publicKey: "pk", ts: 2 }],
paired: [],
});
callGateway.mockRejectedValueOnce(new Error("gateway closed (1008): pairing required"));
approveDevicePairing.mockResolvedValueOnce({
requestId: "req-latest",
device: {
@@ -287,7 +384,7 @@ describe("devices cli local fallback", () => {
});
summarizeDeviceTokens.mockReturnValue(undefined);
await runDevicesApprove(["--latest"]);
await runDevicesApprove(["req-latest"]);
expect(approveDevicePairing).toHaveBeenCalledWith("req-latest", {
callerScopes: ["operator.admin"],

View File

@@ -69,13 +69,18 @@ type DevicePairingList = {
};
const FALLBACK_NOTICE = "Direct scope access failed; using local fallback.";
const DEFAULT_DEVICES_TIMEOUT_MS = 10_000;
const devicesCallOpts = (cmd: Command, defaults?: { timeoutMs?: number }) =>
cmd
.option("--url <url>", "Gateway WebSocket URL (defaults to gateway.remote.url when configured)")
.option("--token <token>", "Gateway token (if required)")
.option("--password <password>", "Gateway password (password auth)")
.option("--timeout <ms>", "Timeout in ms", String(defaults?.timeoutMs ?? 10_000))
.option(
"--timeout <ms>",
"Timeout in ms",
String(defaults?.timeoutMs ?? DEFAULT_DEVICES_TIMEOUT_MS),
)
.option("--json", "Output JSON", false);
const callGatewayCli = async (method: string, opts: DevicesRpcOpts, params?: unknown) =>
@@ -92,7 +97,7 @@ const callGatewayCli = async (method: string, opts: DevicesRpcOpts, params?: unk
password: opts.password,
method,
params,
timeoutMs: Number(opts.timeout ?? 10_000),
timeoutMs: Number(opts.timeout ?? DEFAULT_DEVICES_TIMEOUT_MS),
clientName: GATEWAY_CLIENT_NAMES.CLI,
mode: GATEWAY_CLIENT_MODES.CLI,
}),
@@ -236,6 +241,47 @@ function formatPendingScopes(request: PendingDevice): string {
return scopes.join(", ");
}
function formatPendingDeviceIdentity(request: PendingDevice): string {
return normalizeOptionalString(request.displayName) ?? request.deviceId;
}
function quoteCliArg(value: string): string {
if (/^[A-Za-z0-9_/:=.,@%+-]+$/.test(value)) {
return value;
}
return `'${value.replaceAll("'", "'\\''")}'`;
}
function buildExplicitApproveCommand(opts: DevicesRpcOpts, requestId: string): string {
const args = ["openclaw", "devices", "approve", requestId];
const url = normalizeOptionalString(opts.url);
if (url) {
args.push("--url", url);
}
const timeout = normalizeOptionalString(opts.timeout);
if (timeout && timeout !== String(DEFAULT_DEVICES_TIMEOUT_MS)) {
args.push("--timeout", timeout);
}
if (opts.json === true) {
args.push("--json");
}
return args.map(quoteCliArg).join(" ");
}
function formatAuthFlagReminder(opts: DevicesRpcOpts): string {
const flags: string[] = [];
if (normalizeOptionalString(opts.token)) {
flags.push("--token");
}
if (normalizeOptionalString(opts.password)) {
flags.push("--password");
}
if (flags.length === 0) {
return "";
}
return `Reuse the same ${flags.join("/")} option${flags.length === 1 ? "" : "s"} when rerunning.`;
}
function resolveRequiredDeviceRole(
opts: DevicesRpcOpts,
): { deviceId: string; role: string } | null {
@@ -401,18 +447,62 @@ export function registerDevicesCli(program: Command) {
.command("approve")
.description("Approve a pending device pairing request")
.argument("[requestId]", "Pending request id")
.option("--latest", "Approve the most recent pending request", false)
.option("--latest", "Show the most recent pending request to approve explicitly", false)
.action(async (requestId: string | undefined, opts: DevicesRpcOpts) => {
let resolvedRequestId = requestId?.trim();
if (!resolvedRequestId || opts.latest) {
const latest = selectLatestPendingRequest((await listPairingWithFallback(opts)).pending);
resolvedRequestId = latest?.requestId?.trim();
const usingImplicitSelection = !resolvedRequestId || Boolean(opts.latest);
let selectedRequest: PendingDevice | null = null;
if (usingImplicitSelection) {
selectedRequest = selectLatestPendingRequest(
(await listPairingWithFallback(opts)).pending,
);
resolvedRequestId = selectedRequest?.requestId?.trim();
}
if (!resolvedRequestId) {
defaultRuntime.error("No pending device pairing requests to approve");
defaultRuntime.exit(1);
return;
}
if (usingImplicitSelection) {
// Keep implicit selection preview-only. A second command with the exact
// requestId binds the approval to the request the operator inspected.
const req = selectedRequest!;
const approveCommand = buildExplicitApproveCommand(opts, req.requestId);
const authReminder = formatAuthFlagReminder(opts);
if (opts.json) {
defaultRuntime.writeJson({
selected: req,
approveCommand,
requiresAuthFlags: {
token: Boolean(normalizeOptionalString(opts.token)),
password: Boolean(normalizeOptionalString(opts.password)),
},
});
defaultRuntime.exit(1);
return;
}
defaultRuntime.log(
`${theme.warn("Selected pending device request")} ${theme.command(req.requestId)}`,
);
defaultRuntime.log(` Device: ${formatPendingDeviceIdentity(req)}`);
const role = formatPendingRoles(req);
if (role) {
defaultRuntime.log(` Role: ${role}`);
}
const scopes = formatPendingScopes(req);
if (scopes) {
defaultRuntime.log(` Scopes: ${scopes}`);
}
if (req.remoteIp) {
defaultRuntime.log(` IP: ${req.remoteIp}`);
}
defaultRuntime.error(`Approve this exact request with: ${approveCommand}`);
if (authReminder) {
defaultRuntime.error(authReminder);
}
defaultRuntime.exit(1);
return;
}
const result = await approvePairingWithFallback(opts, resolvedRequestId);
if (!result) {
defaultRuntime.error("unknown requestId");

View File

@@ -21,7 +21,7 @@ import type { CronEvent } from "./state.js";
import { createCronServiceState } from "./state.js";
import { onTimer } from "./timer.js";
const FAST_TIMEOUT_SECONDS = 0.0025;
const FAST_TIMEOUT_SECONDS = 1;
const opsRegressionFixtures = setupCronRegressionFixtures({
prefix: "cron-service-ops-regressions-",
});
@@ -198,39 +198,44 @@ describe("cron service ops regressions", () => {
});
it("applies timeoutSeconds to manual cron.run isolated executions", async () => {
const store = opsRegressionFixtures.makeStorePath();
const scheduledAt = Date.parse("2026-02-15T13:00:00.000Z");
const job = createIsolatedRegressionJob({
id: "manual-timeout",
name: "manual timeout",
scheduledAt,
schedule: { kind: "every", everyMs: 60_000, anchorMs: scheduledAt },
payload: { kind: "agentTurn", message: "work", timeoutSeconds: FAST_TIMEOUT_SECONDS },
state: { nextRunAtMs: scheduledAt },
});
await writeCronJobs(store.storePath, [job]);
vi.useFakeTimers();
try {
const store = opsRegressionFixtures.makeStorePath();
const scheduledAt = Date.parse("2026-02-15T13:00:00.000Z");
const job = createIsolatedRegressionJob({
id: "manual-timeout",
name: "manual timeout",
scheduledAt,
schedule: { kind: "every", everyMs: 60_000, anchorMs: scheduledAt },
payload: { kind: "agentTurn", message: "work", timeoutSeconds: FAST_TIMEOUT_SECONDS },
state: { nextRunAtMs: scheduledAt },
});
await writeCronJobs(store.storePath, [job]);
const abortAwareRunner = createAbortAwareIsolatedRunner();
const state = createCronServiceState({
cronEnabled: false,
storePath: store.storePath,
log: noopLogger,
enqueueSystemEvent: vi.fn(),
requestHeartbeatNow: vi.fn(),
runIsolatedAgentJob: abortAwareRunner.runIsolatedAgentJob,
});
const abortAwareRunner = createAbortAwareIsolatedRunner();
const state = createCronServiceState({
cronEnabled: false,
storePath: store.storePath,
log: noopLogger,
enqueueSystemEvent: vi.fn(),
requestHeartbeatNow: vi.fn(),
runIsolatedAgentJob: abortAwareRunner.runIsolatedAgentJob,
});
const resultPromise = run(state, job.id, "force");
await abortAwareRunner.waitForStart();
await vi.advanceTimersByTimeAsync(10);
const result = await resultPromise;
expect(result).toEqual({ ok: true, ran: true });
expect(abortAwareRunner.getObservedAbortSignal()?.aborted).toBe(true);
const resultPromise = run(state, job.id, "force");
await abortAwareRunner.waitForStart();
await vi.advanceTimersByTimeAsync(Math.ceil(FAST_TIMEOUT_SECONDS * 1_000) + 10);
const result = await resultPromise;
expect(result).toEqual({ ok: true, ran: true });
expect(abortAwareRunner.getObservedAbortSignal()?.aborted).toBe(true);
const updated = state.store?.jobs.find((entry) => entry.id === job.id);
expect(updated?.state.lastStatus).toBe("error");
expect(updated?.state.lastError).toContain("timed out");
expect(updated?.state.runningAtMs).toBeUndefined();
const updated = state.store?.jobs.find((entry) => entry.id === job.id);
expect(updated?.state.lastStatus).toBe("error");
expect(updated?.state.lastError).toContain("timed out");
expect(updated?.state.runningAtMs).toBeUndefined();
} finally {
vi.useRealTimers();
}
});
it("#17554: run() clears stale runningAtMs and executes the job", async () => {

View File

@@ -25,7 +25,7 @@ import {
runMissedJobs,
} from "./timer.js";
const FAST_TIMEOUT_SECONDS = 0.0025;
const FAST_TIMEOUT_SECONDS = 1;
const timerRegressionFixtures = setupCronRegressionFixtures({
prefix: "cron-service-timer-regressions-",
});
@@ -957,7 +957,7 @@ describe("cron service timer regressions", () => {
try {
const store = timerRegressionFixtures.makeStorePath();
const scheduledAt = Date.parse("2026-02-15T13:00:00.000Z");
const timeoutSeconds = 0.03;
const timeoutSeconds = 1;
const cronJob = createIsolatedRegressionJob({
id: "timeout-fraction-29774",
name: "timeout fraction regression",
@@ -1010,10 +1010,10 @@ describe("cron service timer regressions", () => {
const timerPromise = onTimer(state);
await started.promise;
await vi.advanceTimersByTimeAsync(15);
await vi.advanceTimersByTimeAsync(500);
expect(abortWallMs).toBeUndefined();
await vi.advanceTimersByTimeAsync(20);
await vi.advanceTimersByTimeAsync(600);
await timerPromise;
const elapsedMs = (abortWallMs ?? Date.now()) - wallStart;

View File

@@ -161,6 +161,37 @@ describe("device pairing tokens", () => {
expect(second.request.requestId).toBe(first.request.requestId);
});
test("re-requesting with identical params preserves the original ts to prevent queue-jumping", async () => {
// Regression: refreshPendingDevicePairingRequest must not bump ts to Date.now().
// An attacker who reconnects with the same key/role/scopes could otherwise
// silently move their request to the top of the implicit --latest approval queue.
const baseDir = await makeDevicePairingDir();
const first = await requestDevicePairing(
{
deviceId: "device-1",
publicKey: "public-key-1",
role: "operator",
scopes: ["operator.read"],
},
baseDir,
);
const originalTs = first.request.ts;
await new Promise((resolve) => setTimeout(resolve, 20));
const second = await requestDevicePairing(
{
deviceId: "device-1",
publicKey: "public-key-1",
role: "operator",
scopes: ["operator.read"],
},
baseDir,
);
expect(second.created).toBe(false);
expect(second.request.requestId).toBe(first.request.requestId);
expect(second.request.ts).toBe(originalTs);
});
test("supersedes pending requests when requested roles/scopes change", async () => {
const baseDir = await makeDevicePairingDir();
const first = await requestDevicePairing(

View File

@@ -322,7 +322,10 @@ function refreshPendingDevicePairingRequest(
// If either request is interactive, keep the pending request visible for approval.
silent: Boolean(existing.silent && incoming.silent),
isRepair: existing.isRepair || isRepair,
ts: Date.now(),
// Preserve the original creation timestamp so that reconnects cannot bump this
// request's queue position. Using Date.now() here would let an attacker silently
// refresh recency and win the implicit --latest approval race.
ts: existing.ts,
};
}