From ec56dd311620209395a90a86cee306615c49534f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 26 Apr 2026 02:49:34 +0100 Subject: [PATCH] fix(pairing): preserve corrupt pairing stores --- CHANGELOG.md | 3 +++ src/commands/doctor-device-pairing.test.ts | 28 +++++++++++++++++++++ src/commands/doctor-device-pairing.ts | 17 ++++++++++++- src/infra/device-pairing.test.ts | 20 +++++++++++++++ src/infra/device-pairing.ts | 6 ++--- src/infra/json-files.test.ts | 26 ++++++++++++++++++- src/infra/json-files.ts | 29 ++++++++++++++++++++++ src/infra/node-pairing.test.ts | 21 ++++++++++++++++ src/infra/node-pairing.ts | 6 ++--- src/infra/pairing-files.ts | 7 +++++- 10 files changed, 154 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dac6e16f5e..2c407f84add 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,9 @@ Docs: https://docs.openclaw.ai - Diagnostics/OTEL: treat normal early model stream cleanup as a completed model call instead of exporting a misleading `StreamAbandoned` error span. Thanks @vincentkoc. +- Gateway/pairing: stop corrupt or unreadable device/node pairing stores from + being treated as empty state, preserving `paired.json` for repair instead of + overwriting approved pairings. Fixes #71873. Thanks @iret77. - ACP: wait for the configured runtime backend to become healthy before startup identity reconciliation, avoiding transient acpx warnings during Gateway boot. Fixes #40566. diff --git a/src/commands/doctor-device-pairing.test.ts b/src/commands/doctor-device-pairing.test.ts index de219404151..92ade27fcfe 100644 --- a/src/commands/doctor-device-pairing.test.ts +++ b/src/commands/doctor-device-pairing.test.ts @@ -104,6 +104,34 @@ describe("noteDevicePairingHealth", () => { }); }); + it("warns when local pairing state is corrupt instead of treating it as empty", async () => { + await withTempDir("openclaw-doctor-device-pairing-", async (stateDir) => { + await withEnvAsync( + { + OPENCLAW_STATE_DIR: stateDir, + OPENCLAW_TEST_FAST: "1", + }, + async () => { + const pairedPath = path.join(stateDir, "devices", "paired.json"); + await fs.mkdir(path.dirname(pairedPath), { recursive: true }); + await fs.writeFile(pairedPath, "{not-json}", "utf8"); + + await noteDevicePairingHealth({ + cfg: { gateway: { mode: "local" } }, + healthOk: false, + }); + + expect(noteMock).toHaveBeenCalledTimes(1); + const message = String(noteMock.mock.calls[0]?.[0] ?? ""); + expect(noteMock.mock.calls[0]?.[1]).toBe("Device pairing"); + expect(message).toContain("paired.json"); + expect(message).toContain("refused to treat it as empty"); + expect(await fs.readFile(pairedPath, "utf8")).toBe("{not-json}"); + }, + ); + }); + }); + it("warns when the local cached device token predates the gateway rotation", async () => { await withApprovedOperatorPairing(async ({ stateDir, identity }) => { storeDeviceAuthToken({ diff --git a/src/commands/doctor-device-pairing.ts b/src/commands/doctor-device-pairing.ts index 2c00d492eb4..ecf5607b7f4 100644 --- a/src/commands/doctor-device-pairing.ts +++ b/src/commands/doctor-device-pairing.ts @@ -12,6 +12,7 @@ import { type DevicePairingPendingRequest, type PairedDevice, } from "../infra/device-pairing.js"; +import { JsonFileReadError } from "../infra/json-files.js"; import type { DeviceAuthStore } from "../shared/device-auth.js"; import { normalizeDeviceAuthScopes } from "../shared/device-auth.js"; import { roleScopesAllow } from "../shared/operator-scope-compat.js"; @@ -510,11 +511,25 @@ function collectLocalDeviceAuthIssues(snapshot: DoctorPairingSnapshot): string[] return lines; } +function formatPairingStoreReadIssue(error: JsonFileReadError): string { + const problem = error.reason === "parse" ? "contains invalid JSON" : "could not be read"; + return `- Device pairing store ${error.filePath} ${problem}. OpenClaw refused to treat it as empty to avoid overwriting approved pairings. Fix the JSON or file permissions, or move it aside and re-pair devices.`; +} + export async function noteDevicePairingHealth(params: { cfg: OpenClawConfig; healthOk: boolean; }): Promise { - const snapshot = await loadDoctorPairingSnapshot(params); + let snapshot: DoctorPairingSnapshot | null; + try { + snapshot = await loadDoctorPairingSnapshot(params); + } catch (error) { + if (error instanceof JsonFileReadError) { + note(formatPairingStoreReadIssue(error), "Device pairing"); + return; + } + throw error; + } if (!snapshot) { return; } diff --git a/src/infra/device-pairing.test.ts b/src/infra/device-pairing.test.ts index a6845eb32d7..27359dde0fb 100644 --- a/src/infra/device-pairing.test.ts +++ b/src/infra/device-pairing.test.ts @@ -1053,6 +1053,26 @@ describe("device pairing tokens", () => { await expect(getPairedDevice("device-1", baseDir)).resolves.toBeNull(); }); + test("refuses to overwrite corrupt paired device state", async () => { + const baseDir = await makeDevicePairingDir(); + const request = await requestDevicePairing( + { + deviceId: "device-1", + publicKey: "public-key-1", + role: "node", + scopes: [], + }, + baseDir, + ); + const { pairedPath } = resolvePairingPaths(baseDir, "devices"); + await writeFile(pairedPath, "{not-json}", "utf8"); + + await expect( + approveDevicePairing(request.request.requestId, { callerScopes: [] }, baseDir), + ).rejects.toThrow(/paired\.json/); + await expect(readFile(pairedPath, "utf8")).resolves.toBe("{not-json}"); + }); + test("clears paired device state by device id", async () => { const baseDir = await makeDevicePairingDir(); await setupPairedOperatorDevice(baseDir, ["operator.read"]); diff --git a/src/infra/device-pairing.ts b/src/infra/device-pairing.ts index 7c5a787411b..00a0816b844 100644 --- a/src/infra/device-pairing.ts +++ b/src/infra/device-pairing.ts @@ -12,7 +12,7 @@ import { import { createAsyncLock, pruneExpiredPending, - readJsonFile, + readDurableJsonFile, reconcilePendingPairingRequests, resolvePairingPaths, writeJsonAtomic, @@ -143,8 +143,8 @@ export function formatDevicePairingForbiddenMessage(result: DevicePairingForbidd async function loadState(baseDir?: string): Promise { const { pendingPath, pairedPath } = resolvePairingPaths(baseDir, "devices"); const [pending, paired] = await Promise.all([ - readJsonFile>(pendingPath), - readJsonFile>(pairedPath), + readDurableJsonFile>(pendingPath), + readDurableJsonFile>(pairedPath), ]); const state: DevicePairingStateFile = { pendingById: pending ?? {}, diff --git a/src/infra/json-files.test.ts b/src/infra/json-files.test.ts index c1166ec5f49..50ea62541f4 100644 --- a/src/infra/json-files.test.ts +++ b/src/infra/json-files.test.ts @@ -3,7 +3,14 @@ import path from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; import { afterEach, describe, expect, it, vi } from "vitest"; import { withTempDir } from "../test-helpers/temp-dir.js"; -import { createAsyncLock, readJsonFile, writeJsonAtomic, writeTextAtomic } from "./json-files.js"; +import { + JsonFileReadError, + createAsyncLock, + readDurableJsonFile, + readJsonFile, + writeJsonAtomic, + writeTextAtomic, +} from "./json-files.js"; const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, "platform"); @@ -45,6 +52,23 @@ describe("json file helpers", () => { }); }); + it("reads durable json strictly while allowing missing files", async () => { + await withTempDir({ prefix: "openclaw-json-files-" }, async (base) => { + const validPath = path.join(base, "valid.json"); + const invalidPath = path.join(base, "invalid.json"); + const missingPath = path.join(base, "missing.json"); + await fs.writeFile(validPath, '{"ok":true}', "utf8"); + await fs.writeFile(invalidPath, "{not-json}", "utf8"); + + await expect(readDurableJsonFile(validPath)).resolves.toEqual({ ok: true }); + await expect(readDurableJsonFile(missingPath)).resolves.toBeNull(); + await expect(readDurableJsonFile(invalidPath)).rejects.toMatchObject({ + filePath: invalidPath, + reason: "parse", + } satisfies Partial); + }); + }); + it("writes json atomically with pretty formatting and optional trailing newline", async () => { await withTempDir({ prefix: "openclaw-json-files-" }, async (base) => { const filePath = path.join(base, "nested", "config.json"); diff --git a/src/infra/json-files.ts b/src/infra/json-files.ts index b79cb1d845e..8ca5802eee0 100644 --- a/src/infra/json-files.ts +++ b/src/infra/json-files.ts @@ -7,6 +7,18 @@ function getErrorCode(err: unknown): string | undefined { return err instanceof Error ? (err as NodeJS.ErrnoException).code : undefined; } +export class JsonFileReadError extends Error { + readonly filePath: string; + readonly reason: "read" | "parse"; + + constructor(filePath: string, reason: "read" | "parse", cause: unknown) { + super(`Failed to ${reason} JSON file: ${filePath}`, { cause }); + this.name = "JsonFileReadError"; + this.filePath = filePath; + this.reason = reason; + } +} + async function replaceFileWithWindowsFallback(tempPath: string, filePath: string, mode: number) { try { await fs.rename(tempPath, filePath); @@ -43,6 +55,23 @@ export async function readJsonFile(filePath: string): Promise { } } +export async function readDurableJsonFile(filePath: string): Promise { + let raw: string; + try { + raw = await fs.readFile(filePath, "utf8"); + } catch (err) { + if (getErrorCode(err) === "ENOENT") { + return null; + } + throw new JsonFileReadError(filePath, "read", err); + } + try { + return JSON.parse(raw) as T; + } catch (err) { + throw new JsonFileReadError(filePath, "parse", err); + } +} + export function readJsonFileSync(filePath: string): unknown { try { const raw = readFileSync(filePath, "utf8"); diff --git a/src/infra/node-pairing.test.ts b/src/infra/node-pairing.test.ts index eba5d95294d..062cd8bf1d6 100644 --- a/src/infra/node-pairing.test.ts +++ b/src/infra/node-pairing.test.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import { afterAll, beforeAll, describe, expect, test } from "vitest"; import { createSuiteTempRootTracker } from "../test-helpers/temp-dir.js"; import { @@ -7,6 +8,7 @@ import { requestNodePairing, verifyNodeToken, } from "./node-pairing.js"; +import { resolvePairingPaths } from "./pairing-files.js"; async function setupPairedNode(baseDir: string): Promise { const request = await requestNodePairing( @@ -202,4 +204,23 @@ describe("node pairing tokens", () => { }); }); }); + + test("refuses to overwrite corrupt paired node state when requesting pairing", async () => { + await withNodePairingDir(async (baseDir) => { + const { dir, pairedPath } = resolvePairingPaths(baseDir, "nodes"); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(pairedPath, "{not-json}", "utf8"); + + await expect( + requestNodePairing( + { + nodeId: "node-1", + platform: "darwin", + }, + baseDir, + ), + ).rejects.toThrow(/paired\.json/); + await expect(fs.readFile(pairedPath, "utf8")).resolves.toBe("{not-json}"); + }); + }); }); diff --git a/src/infra/node-pairing.ts b/src/infra/node-pairing.ts index 0a371615178..ceb956f4913 100644 --- a/src/infra/node-pairing.ts +++ b/src/infra/node-pairing.ts @@ -5,7 +5,7 @@ import { type NodeApprovalScope, resolveNodePairApprovalScopes } from "./node-pa import { createAsyncLock, pruneExpiredPending, - readJsonFile, + readDurableJsonFile, reconcilePendingPairingRequests, resolvePairingPaths, writeJsonAtomic, @@ -134,8 +134,8 @@ type ApproveNodePairingResult = ApprovedNodePairingResult | ForbiddenNodePairing async function loadState(baseDir?: string): Promise { const { pendingPath, pairedPath } = resolvePairingPaths(baseDir, "nodes"); const [pending, paired] = await Promise.all([ - readJsonFile>(pendingPath), - readJsonFile>(pairedPath), + readDurableJsonFile>(pendingPath), + readDurableJsonFile>(pairedPath), ]); const state: NodePairingStateFile = { pendingById: pending ?? {}, diff --git a/src/infra/pairing-files.ts b/src/infra/pairing-files.ts index 06e2f729812..a7f38b43517 100644 --- a/src/infra/pairing-files.ts +++ b/src/infra/pairing-files.ts @@ -1,7 +1,12 @@ import path from "node:path"; import { resolveStateDir } from "../config/paths.js"; -export { createAsyncLock, readJsonFile, writeJsonAtomic } from "./json-files.js"; +export { + createAsyncLock, + readDurableJsonFile, + readJsonFile, + writeJsonAtomic, +} from "./json-files.js"; export function resolvePairingPaths(baseDir: string | undefined, subdir: string) { const root = baseDir ?? resolveStateDir();