From fb8e20ddb6b23b759167fd95ead33ec0f564f087 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 4 Apr 2026 23:19:34 +0900 Subject: [PATCH] fix: harden paired-device management authz (#50627) (thanks @coygeek) --- CHANGELOG.md | 1 + src/gateway/server-methods/devices.test.ts | 141 ++++++++++++++++++ src/gateway/server-methods/devices.ts | 87 ++++++++++- .../server.device-token-rotate-authz.test.ts | 126 ++++++++++++++++ 4 files changed, 349 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f828c49dda4..c4bbca42500 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,7 @@ Docs: https://docs.openclaw.ai - Mattermost/config schema: accept `groups.*.requireMention` again so existing Mattermost configs no longer fail strict validation after upgrade. (#58271) Thanks @MoerAI. - Providers/OpenRouter failover: classify `403 "Key limit exceeded"` spending-limit responses as billing so model fallback continues instead of stopping on generic auth. (#59892) Thanks @rockcent. - Device pairing/security: keep non-operator device scope checks bound to the requested role prefix so bootstrap verification cannot redeem `operator.*` scopes through `node` auth. (#57258) Thanks @jlapenna. +- Gateway/device pairing: require non-admin paired-device sessions to manage only their own device for token rotate/revoke and paired-device removal, blocking cross-device token theft inside pairing-scoped sessions. (#50627) Thanks @coygeek. ## 2026.4.2 diff --git a/src/gateway/server-methods/devices.test.ts b/src/gateway/server-methods/devices.test.ts index 3c61596067d..3f7028f7c05 100644 --- a/src/gateway/server-methods/devices.test.ts +++ b/src/gateway/server-methods/devices.test.ts @@ -27,6 +27,15 @@ vi.mock("../../infra/device-pairing.js", async () => { }; }); +function createClient(scopes: string[], deviceId?: string) { + return { + connect: { + scopes, + ...(deviceId ? { device: { id: deviceId } } : {}), + }, + } as never; +} + function createOptions( method: string, params: Record, @@ -86,6 +95,41 @@ describe("deviceHandlers", () => { ); }); + it("rejects removing another device from a non-admin device session", async () => { + const opts = createOptions( + "device.pair.remove", + { deviceId: "device-2" }, + { client: createClient(["operator.pairing"], "device-1") }, + ); + + await deviceHandlers["device.pair.remove"](opts); + + expect(removePairedDeviceMock).not.toHaveBeenCalled(); + expect(opts.respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ message: "device pairing removal denied" }), + ); + }); + + it("treats normalized device ids as self-owned for paired device removal", async () => { + removePairedDeviceMock.mockResolvedValue({ deviceId: "device-1", removedAtMs: 123 }); + const opts = createOptions( + "device.pair.remove", + { deviceId: " device-1 " }, + { client: createClient(["operator.pairing"], "device-1") }, + ); + + await deviceHandlers["device.pair.remove"](opts); + + expect(removePairedDeviceMock).toHaveBeenCalledWith(" device-1 "); + expect(opts.respond).toHaveBeenCalledWith( + true, + { deviceId: "device-1", removedAtMs: 123 }, + undefined, + ); + }); + it("disconnects active clients after revoking a device token", async () => { revokeDeviceTokenMock.mockResolvedValue({ role: "operator", revokedAtMs: 456 }); const opts = createOptions("device.token.revoke", { @@ -110,6 +154,48 @@ describe("deviceHandlers", () => { ); }); + it("allows admin-scoped callers to revoke another device's token", async () => { + revokeDeviceTokenMock.mockResolvedValue({ role: "operator", revokedAtMs: 456 }); + const opts = createOptions( + "device.token.revoke", + { deviceId: "device-2", role: "operator" }, + { client: createClient(["operator.admin"], "device-1") }, + ); + + await deviceHandlers["device.token.revoke"](opts); + + expect(revokeDeviceTokenMock).toHaveBeenCalledWith({ + deviceId: "device-2", + role: "operator", + }); + expect(opts.respond).toHaveBeenCalledWith( + true, + { deviceId: "device-2", role: "operator", revokedAtMs: 456 }, + undefined, + ); + }); + + it("treats normalized device ids as self-owned for token revocation", async () => { + revokeDeviceTokenMock.mockResolvedValue({ role: "operator", revokedAtMs: 456 }); + const opts = createOptions( + "device.token.revoke", + { deviceId: " device-1 ", role: "operator" }, + { client: createClient(["operator.pairing"], "device-1") }, + ); + + await deviceHandlers["device.token.revoke"](opts); + + expect(revokeDeviceTokenMock).toHaveBeenCalledWith({ + deviceId: " device-1 ", + role: "operator", + }); + expect(opts.respond).toHaveBeenCalledWith( + true, + { deviceId: "device-1", role: "operator", revokedAtMs: 456 }, + undefined, + ); + }); + it("disconnects active clients after rotating a device token", async () => { getPairedDeviceMock.mockResolvedValue({ deviceId: "device-1", @@ -175,6 +261,61 @@ describe("deviceHandlers", () => { ); }); + it("treats normalized device ids as self-owned for token rotation", async () => { + getPairedDeviceMock.mockResolvedValue({ + deviceId: "device-1", + role: "operator", + roles: ["operator"], + scopes: ["operator.pairing"], + tokens: { + operator: { + token: "old-token", + role: "operator", + scopes: ["operator.pairing"], + createdAtMs: 123, + }, + }, + }); + rotateDeviceTokenMock.mockResolvedValue({ + ok: true, + entry: { + token: "new-token", + role: "operator", + scopes: ["operator.pairing"], + createdAtMs: 456, + rotatedAtMs: 789, + }, + }); + const opts = createOptions( + "device.token.rotate", + { + deviceId: " device-1 ", + role: "operator", + scopes: ["operator.pairing"], + }, + { client: createClient(["operator.pairing"], "device-1") }, + ); + + await deviceHandlers["device.token.rotate"](opts); + + expect(rotateDeviceTokenMock).toHaveBeenCalledWith({ + deviceId: " device-1 ", + role: "operator", + scopes: ["operator.pairing"], + }); + expect(opts.respond).toHaveBeenCalledWith( + true, + { + deviceId: " device-1 ", + role: "operator", + token: "new-token", + scopes: ["operator.pairing"], + rotatedAtMs: 789, + }, + undefined, + ); + }); + it("rejects rotating a token for a role that was never approved", async () => { getPairedDeviceMock.mockResolvedValue({ deviceId: "device-1", diff --git a/src/gateway/server-methods/devices.ts b/src/gateway/server-methods/devices.ts index 228d11175fb..559f0e4593f 100644 --- a/src/gateway/server-methods/devices.ts +++ b/src/gateway/server-methods/devices.ts @@ -24,7 +24,7 @@ import { validateDeviceTokenRevokeParams, validateDeviceTokenRotateParams, } from "../protocol/index.js"; -import type { GatewayRequestHandlers } from "./types.js"; +import type { GatewayClient, GatewayRequestHandlers } from "./types.js"; const DEVICE_TOKEN_ROTATION_DENIED_MESSAGE = "device token rotation denied"; @@ -33,6 +33,13 @@ type DeviceTokenRotateTarget = { normalizedRole: string; }; +type DeviceManagementAuthz = { + callerDeviceId: string | null; + callerScopes: string[]; + isAdminCaller: boolean; + normalizedTargetDeviceId: string; +}; + function redactPairedDevice( device: { tokens?: Record } & Record, ) { @@ -47,7 +54,11 @@ function logDeviceTokenRotationDenied(params: { log: { warn: (message: string) => void }; deviceId: string; role: string; - reason: RotateDeviceTokenDenyReason | "caller-missing-scope" | "unknown-device-or-role"; + reason: + | RotateDeviceTokenDenyReason + | "caller-missing-scope" + | "unknown-device-or-role" + | "device-ownership-mismatch"; scope?: string | null; }) { const suffix = params.scope ? ` scope=${params.scope}` : ""; @@ -75,6 +86,32 @@ async function loadDeviceTokenRotateTarget(params: { return { pairedDevice, normalizedRole }; } +function resolveDeviceManagementAuthz( + client: GatewayClient | null, + targetDeviceId: string, +): DeviceManagementAuthz { + const callerScopes = Array.isArray(client?.connect?.scopes) ? client.connect.scopes : []; + const rawCallerDeviceId = client?.connect?.device?.id; + const callerDeviceId = + typeof rawCallerDeviceId === "string" && rawCallerDeviceId.trim() + ? rawCallerDeviceId.trim() + : null; + return { + callerDeviceId, + callerScopes, + isAdminCaller: callerScopes.includes("operator.admin"), + normalizedTargetDeviceId: targetDeviceId.trim(), + }; +} + +function deniesCrossDeviceManagement(authz: DeviceManagementAuthz): boolean { + return Boolean( + authz.callerDeviceId && + authz.callerDeviceId !== authz.normalizedTargetDeviceId && + !authz.isAdminCaller, + ); +} + export const deviceHandlers: GatewayRequestHandlers = { "device.pair.list": async ({ params, respond }) => { if (!validateDevicePairListParams(params)) { @@ -176,7 +213,7 @@ export const deviceHandlers: GatewayRequestHandlers = { ); respond(true, rejected, undefined); }, - "device.pair.remove": async ({ params, respond, context }) => { + "device.pair.remove": async ({ params, respond, context, client }) => { if (!validateDevicePairRemoveParams(params)) { respond( false, @@ -191,6 +228,18 @@ export const deviceHandlers: GatewayRequestHandlers = { return; } const { deviceId } = params as { deviceId: string }; + const authz = resolveDeviceManagementAuthz(client, deviceId); + if (deniesCrossDeviceManagement(authz)) { + context.logGateway.warn( + `device pairing removal denied device=${deviceId} reason=device-ownership-mismatch`, + ); + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, "device pairing removal denied"), + ); + return; + } const removed = await removePairedDevice(deviceId); if (!removed) { respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "unknown deviceId")); @@ -221,6 +270,21 @@ export const deviceHandlers: GatewayRequestHandlers = { role: string; scopes?: string[]; }; + const authz = resolveDeviceManagementAuthz(client, deviceId); + if (deniesCrossDeviceManagement(authz)) { + logDeviceTokenRotationDenied({ + log: context.logGateway, + deviceId, + role, + reason: "device-ownership-mismatch", + }); + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, DEVICE_TOKEN_ROTATION_DENIED_MESSAGE), + ); + return; + } const rotateTarget = await loadDeviceTokenRotateTarget({ deviceId, role, @@ -235,14 +299,13 @@ export const deviceHandlers: GatewayRequestHandlers = { return; } const { pairedDevice, normalizedRole } = rotateTarget; - const callerScopes = Array.isArray(client?.connect?.scopes) ? client.connect.scopes : []; const requestedScopes = normalizeDeviceAuthScopes( scopes ?? pairedDevice.tokens?.[normalizedRole]?.scopes ?? pairedDevice.scopes, ); const missingScope = resolveMissingRequestedScope({ role, requestedScopes, - allowedScopes: callerScopes, + allowedScopes: authz.callerScopes, }); if (missingScope) { logDeviceTokenRotationDenied({ @@ -293,7 +356,7 @@ export const deviceHandlers: GatewayRequestHandlers = { context.disconnectClientsForDevice?.(deviceId.trim(), { role: entry.role }); }); }, - "device.token.revoke": async ({ params, respond, context }) => { + "device.token.revoke": async ({ params, respond, context, client }) => { if (!validateDeviceTokenRevokeParams(params)) { respond( false, @@ -308,6 +371,18 @@ export const deviceHandlers: GatewayRequestHandlers = { return; } const { deviceId, role } = params as { deviceId: string; role: string }; + const authz = resolveDeviceManagementAuthz(client, deviceId); + if (deniesCrossDeviceManagement(authz)) { + context.logGateway.warn( + `device token revocation denied device=${deviceId} role=${role} reason=device-ownership-mismatch`, + ); + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, "device token revocation denied"), + ); + return; + } const entry = await revokeDeviceToken({ deviceId, role }); if (!entry) { respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "unknown deviceId/role")); diff --git a/src/gateway/server.device-token-rotate-authz.test.ts b/src/gateway/server.device-token-rotate-authz.test.ts index 4ae55502679..2b75126842e 100644 --- a/src/gateway/server.device-token-rotate-authz.test.ts +++ b/src/gateway/server.device-token-rotate-authz.test.ts @@ -107,6 +107,132 @@ async function waitForMacrotasks(): Promise { await new Promise((resolve) => setImmediate(resolve)); } +async function issuePairingScopedTokenForAdminApprovedDevice(name: string): Promise<{ + deviceId: string; + identityPath: string; + pairingToken: string; +}> { + const issued = await issueOperatorToken({ + name, + approvedScopes: ["operator.admin"], + tokenScopes: ["operator.pairing"], + clientId: GATEWAY_CLIENT_NAMES.TEST, + clientMode: GATEWAY_CLIENT_MODES.TEST, + }); + return { + deviceId: issued.deviceId, + identityPath: issued.identityPath, + pairingToken: issued.token, + }; +} + +describe("gateway device.token.rotate/revoke ownership guard (IDOR)", () => { + test("rejects a device-token caller rotating another device's token", async () => { + const started = await startServerWithClient("secret"); + const deviceA = await issuePairingScopedTokenForAdminApprovedDevice("idor-device-a"); + const deviceB = await issuePairingScopedTokenForAdminApprovedDevice("idor-device-b"); + + let pairingWs: WebSocket | undefined; + try { + pairingWs = await connectPairingScopedOperator({ + port: started.port, + identityPath: deviceA.identityPath, + deviceToken: deviceA.pairingToken, + }); + + const rotate = await rpcReq(pairingWs, "device.token.rotate", { + deviceId: deviceB.deviceId, + role: "operator", + scopes: ["operator.pairing"], + }); + expect(rotate.ok).toBe(false); + expect(rotate.error?.message).toBe("device token rotation denied"); + + const pairedB = await getPairedDevice(deviceB.deviceId); + expect(pairedB?.tokens?.operator?.token).toBe(deviceB.pairingToken); + } finally { + pairingWs?.close(); + started.ws.close(); + await started.server.close(); + started.envSnapshot.restore(); + } + }); + + test("allows an admin-scoped caller to rotate another device's token", async () => { + const started = await startServerWithClient("secret"); + const device = await issuePairingScopedTokenForAdminApprovedDevice("idor-admin-rotate"); + + try { + await connectOk(started.ws); + + const rotate = await rpcReq<{ token?: string }>(started.ws, "device.token.rotate", { + deviceId: device.deviceId, + role: "operator", + scopes: ["operator.pairing"], + }); + expect(rotate.ok).toBe(true); + expect(rotate.payload?.token).toBeTruthy(); + } finally { + started.ws.close(); + await started.server.close(); + started.envSnapshot.restore(); + } + }); + + test("rejects a device-token caller revoking another device's token", async () => { + const started = await startServerWithClient("secret"); + const deviceA = await issuePairingScopedTokenForAdminApprovedDevice("idor-revoke-a"); + const deviceB = await issuePairingScopedTokenForAdminApprovedDevice("idor-revoke-b"); + + let pairingWs: WebSocket | undefined; + try { + pairingWs = await connectPairingScopedOperator({ + port: started.port, + identityPath: deviceA.identityPath, + deviceToken: deviceA.pairingToken, + }); + + const revoke = await rpcReq(pairingWs, "device.token.revoke", { + deviceId: deviceB.deviceId, + role: "operator", + }); + expect(revoke.ok).toBe(false); + expect(revoke.error?.message).toBe("device token revocation denied"); + + const pairedB = await getPairedDevice(deviceB.deviceId); + expect(pairedB?.tokens?.operator?.revokedAtMs).toBeUndefined(); + } finally { + pairingWs?.close(); + started.ws.close(); + await started.server.close(); + started.envSnapshot.restore(); + } + }); + + test("allows an admin-scoped caller to revoke another device's token", async () => { + const started = await startServerWithClient("secret"); + const device = await issuePairingScopedTokenForAdminApprovedDevice("idor-admin-revoke"); + + try { + await connectOk(started.ws); + + const revoke = await rpcReq<{ revokedAtMs?: number }>(started.ws, "device.token.revoke", { + deviceId: device.deviceId, + role: "operator", + }); + expect(revoke.ok).toBe(true); + expect(revoke.payload?.revokedAtMs).toBeTypeOf("number"); + + const paired = await getPairedDevice(device.deviceId); + expect(paired?.tokens?.operator?.revokedAtMs).toBeTypeOf("number"); + } finally { + started.ws.close(); + await started.server.close(); + started.envSnapshot.restore(); + } + }); +}); + describe("gateway device.token.rotate caller scope guard", () => { test("rejects rotating an admin-approved device token above the caller session scopes", async () => { const started = await startServerWithClient("secret");