Skip to content

Commit 5f60724

Browse files
ly-wang19ly-wang19
authored
fix(nodes): guard node scalar fields against malformed entries
1 parent 685b95b commit 5f60724

5 files changed

Lines changed: 296 additions & 27 deletions

File tree

‎src/cli/nodes-cli/pairing-render.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Pending pairing render tests cover crash-safety for malformed pairing-list scalars.
2+
import { describe, expect, it } from "vitest";
3+
import { parsePairingList } from "../../shared/node-list-parse.js";
4+
import { renderPendingPairingRequestsTable } from "./pairing-render.js";
5+
6+
const theme = {
7+
heading: (text: string) => text,
8+
warn: (text: string) => text,
9+
muted: (text: string) => text,
10+
};
11+
12+
describe("cli/nodes-cli/pairing-render", () => {
13+
it("renders pending rows parsed from malformed scalars without throwing", () => {
14+
// node.pair.list rows are blind-cast from the pairing file; non-string requestId/nodeId/
15+
// displayName/remoteIp once crashed the renderer's .trim()/sanitizeTerminalText calls.
16+
const { pending } = parsePairingList({
17+
pending: [{ requestId: 7, nodeId: {}, displayName: 42, remoteIp: 99, ts: 1 }],
18+
});
19+
20+
expect(() =>
21+
renderPendingPairingRequestsTable({ pending, now: 1000, tableWidth: 80, theme }),
22+
).not.toThrow();
23+
});
24+
25+
it("keeps a valid pending label after normalization", () => {
26+
const { pending } = parsePairingList({
27+
pending: [
28+
{ requestId: "r1", nodeId: "n1", displayName: "Phone", remoteIp: "10.0.0.1", ts: 1 },
29+
],
30+
});
31+
32+
const { table } = renderPendingPairingRequestsTable({
33+
pending,
34+
now: 1000,
35+
tableWidth: 80,
36+
theme,
37+
});
38+
expect(table).toContain("Phone");
39+
expect(table).toContain("10.0.0.1");
40+
});
41+
});

‎src/gateway/node-catalog.test.ts‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,4 +444,91 @@ describe("gateway/node-catalog", () => {
444444
expect(nodes[0]?.caps).toEqual(["camera"]);
445445
expect(nodes[0]?.commands).toEqual(["system.run"]);
446446
});
447+
448+
it("normalizes non-string scalar fields from malformed pairing records (no nodes-status crash)", () => {
449+
// Paired/pending records are blind-cast from disk, so every formatter-facing scalar can be a
450+
// non-string; before this guard a `nodes status` formatter (.trim() / sanitizeTerminalText) threw.
451+
const catalog = createKnownNodeCatalog({
452+
pairedDevices: [
453+
pairedDevice({
454+
deviceId: "mac-1",
455+
displayName: 654 as unknown as string,
456+
clientId: 456 as unknown as string,
457+
clientMode: 789 as unknown as string,
458+
}),
459+
],
460+
pairedNodes: [
461+
pairedNode({
462+
nodeId: "mac-1",
463+
version: 123 as unknown as string,
464+
displayName: 321 as unknown as string,
465+
platform: 11 as unknown as string,
466+
remoteIp: 22 as unknown as string,
467+
deviceFamily: 33 as unknown as string,
468+
modelIdentifier: 44 as unknown as string,
469+
}),
470+
],
471+
connectedNodes: [],
472+
});
473+
474+
const node = getKnownNode(catalog, "mac-1");
475+
expect(node?.version).toBeUndefined();
476+
expect(node?.displayName).toBeUndefined();
477+
expect(node?.clientId).toBeUndefined();
478+
expect(node?.clientMode).toBeUndefined();
479+
expect(node?.platform).toBeUndefined();
480+
expect(node?.remoteIp).toBeUndefined();
481+
expect(node?.deviceFamily).toBeUndefined();
482+
expect(node?.modelIdentifier).toBeUndefined();
483+
});
484+
485+
it("falls through a non-string higher-priority scalar to a valid lower-priority value", () => {
486+
// A corrupted live (higher-priority) scalar must be treated as ABSENT so the valid paired
487+
// (lower-priority) value still surfaces, instead of the malformed value suppressing it.
488+
const catalog = createKnownNodeCatalog({
489+
pairedDevices: [pairedDevice({ deviceId: "mac-1" })],
490+
pairedNodes: [
491+
pairedNode({ nodeId: "mac-1", displayName: "Node Display", platform: "linux" }),
492+
],
493+
connectedNodes: [
494+
{
495+
nodeId: "mac-1",
496+
connId: "conn-1",
497+
client: {} as never,
498+
displayName: 42 as unknown as string,
499+
platform: {} as unknown as string,
500+
declaredCaps: [],
501+
caps: [],
502+
declaredCommands: [],
503+
commands: [],
504+
connectedAtMs: 1,
505+
},
506+
],
507+
});
508+
509+
const node = getKnownNode(catalog, "mac-1");
510+
expect(node?.displayName).toBe("Node Display");
511+
expect(node?.platform).toBe("linux");
512+
});
513+
514+
it("drops blind-cast pairing entries whose required id is not a string", () => {
515+
const catalog = createKnownNodeCatalog({
516+
pairedDevices: [
517+
pairedDevice({ deviceId: "good-node" }),
518+
// A corrupted pairing file can carry a non-string id; without the catalog id guard
519+
// these would key the maps and crash listKnownNodes' nodeId.localeCompare sort.
520+
pairedDevice({ deviceId: 7 as unknown as string }),
521+
],
522+
pairedNodes: [
523+
pairedNode({ nodeId: "good-node" }),
524+
pairedNode({ nodeId: {} as unknown as string }),
525+
],
526+
pendingNodes: [pendingNode({ nodeId: [] as unknown as string })],
527+
connectedNodes: [],
528+
});
529+
530+
const nodes = listKnownNodes(catalog);
531+
expect(nodes.map((entry) => entry.nodeId)).toEqual(["good-node"]);
532+
expect(getKnownNode(catalog, "good-node")).not.toBeNull();
533+
});
447534
});

‎src/gateway/node-catalog.ts‎

Lines changed: 86 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
// Gateway node catalog builder.
22
// Merges paired devices, approved node records, and live websocket sessions.
3-
import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/string-coerce";
3+
import {
4+
normalizeLowercaseStringOrEmpty,
5+
normalizeOptionalString,
6+
} from "@openclaw/normalization-core/string-coerce";
47
import { normalizeSortedUniqueTrimmedStringList } from "@openclaw/normalization-core/string-normalization";
58
import { hasEffectivePairedDeviceRole, type PairedDevice } from "../infra/device-pairing.js";
69
import {
@@ -77,6 +80,28 @@ function uniqueSortedStrings(...items: Array<readonly unknown[] | undefined>): s
7780
return normalizeSortedUniqueTrimmedStringList(items.flatMap((item) => item ?? []));
7881
}
7982

83+
// Catalog scalars come from blind-cast pairing records, so coerce every formatter-facing optional
84+
// string scalar to a trimmed string or undefined: a non-string would crash `nodes status`/`nodes
85+
// list` formatters (.trim(), sanitizeTerminalText/stripAnsi). Scalar analog of uniqueSortedStrings.
86+
function firstNormalizedString(...values: unknown[]): string | undefined {
87+
// Treat a non-string (or empty) higher-priority value as ABSENT and fall through, instead of
88+
// letting `find` pick the first non-null value and normalize it to undefined — which would
89+
// suppress a valid lower-priority string. Return the first value that yields a trimmed string.
90+
for (const value of values) {
91+
const normalized = normalizeOptionalString(value);
92+
if (normalized !== undefined) {
93+
return normalized;
94+
}
95+
}
96+
return undefined;
97+
}
98+
99+
// Blind-cast pairing records can carry a non-string id; a node with no addressable string
100+
// id is unusable and would crash the id-based catalog sort/format, so drop it entirely.
101+
function hasAddressableId(value: unknown): boolean {
102+
return normalizeOptionalString(value) !== undefined;
103+
}
104+
80105
function buildDevicePairingSource(entry: PairedDevice): KnownNodeDevicePairingSource {
81106
return {
82107
nodeId: entry.deviceId,
@@ -182,7 +207,7 @@ function resolveEffectiveLastSeen(params: {
182207
}
183208
return {
184209
lastSeenAtMs: newest.atMs,
185-
lastSeenReason: newest.reason,
210+
lastSeenReason: normalizeOptionalString(newest.reason),
186211
};
187212
}
188213

@@ -197,30 +222,59 @@ function buildEffectiveKnownNode(entry: {
197222
const lastSeen = resolveEffectiveLastSeen({ live, devicePairing, nodePairing });
198223
return {
199224
nodeId,
200-
displayName:
201-
live?.displayName ??
202-
nodePairing?.displayName ??
203-
devicePairing?.displayName ??
225+
displayName: firstNormalizedString(
226+
live?.displayName,
227+
nodePairing?.displayName,
228+
devicePairing?.displayName,
204229
pendingNodePairing?.displayName,
205-
platform:
206-
live?.platform ??
207-
nodePairing?.platform ??
208-
devicePairing?.platform ??
230+
),
231+
platform: firstNormalizedString(
232+
live?.platform,
233+
nodePairing?.platform,
234+
devicePairing?.platform,
209235
pendingNodePairing?.platform,
210-
version: live?.version ?? nodePairing?.version ?? pendingNodePairing?.version,
211-
coreVersion: live?.coreVersion ?? nodePairing?.coreVersion ?? pendingNodePairing?.coreVersion,
212-
uiVersion: live?.uiVersion ?? nodePairing?.uiVersion ?? pendingNodePairing?.uiVersion,
213-
clientId: live?.clientId ?? devicePairing?.clientId ?? pendingNodePairing?.clientId,
214-
clientMode: live?.clientMode ?? devicePairing?.clientMode ?? pendingNodePairing?.clientMode,
215-
deviceFamily:
216-
live?.deviceFamily ?? nodePairing?.deviceFamily ?? pendingNodePairing?.deviceFamily,
217-
modelIdentifier:
218-
live?.modelIdentifier ?? nodePairing?.modelIdentifier ?? pendingNodePairing?.modelIdentifier,
219-
remoteIp:
220-
live?.remoteIp ??
221-
nodePairing?.remoteIp ??
222-
devicePairing?.remoteIp ??
236+
),
237+
version: firstNormalizedString(
238+
live?.version,
239+
nodePairing?.version,
240+
pendingNodePairing?.version,
241+
),
242+
coreVersion: firstNormalizedString(
243+
live?.coreVersion,
244+
nodePairing?.coreVersion,
245+
pendingNodePairing?.coreVersion,
246+
),
247+
uiVersion: firstNormalizedString(
248+
live?.uiVersion,
249+
nodePairing?.uiVersion,
250+
pendingNodePairing?.uiVersion,
251+
),
252+
clientId: firstNormalizedString(
253+
live?.clientId,
254+
devicePairing?.clientId,
255+
pendingNodePairing?.clientId,
256+
),
257+
clientMode: firstNormalizedString(
258+
live?.clientMode,
259+
devicePairing?.clientMode,
260+
pendingNodePairing?.clientMode,
261+
),
262+
deviceFamily: firstNormalizedString(
263+
live?.deviceFamily,
264+
nodePairing?.deviceFamily,
265+
pendingNodePairing?.deviceFamily,
266+
),
267+
modelIdentifier: firstNormalizedString(
268+
live?.modelIdentifier,
269+
nodePairing?.modelIdentifier,
270+
pendingNodePairing?.modelIdentifier,
271+
),
272+
remoteIp: firstNormalizedString(
273+
live?.remoteIp,
274+
nodePairing?.remoteIp,
275+
devicePairing?.remoteIp,
223276
pendingNodePairing?.remoteIp,
277+
),
224278
caps: live ? uniqueSortedStrings(live.caps) : uniqueSortedStrings(nodePairing?.caps),
225279
commands: live
226280
? uniqueSortedStrings(live.commands)
@@ -271,15 +325,22 @@ export function createKnownNodeCatalog(params: {
271325
}): KnownNodeCatalog {
272326
const devicePairingById = new Map(
273327
params.pairedDevices
274-
.filter((entry) => hasEffectivePairedDeviceRole(entry, "node"))
328+
.filter(
329+
(entry) => hasAddressableId(entry.deviceId) && hasEffectivePairedDeviceRole(entry, "node"),
330+
)
275331
.map((entry) => [entry.deviceId, buildDevicePairingSource(entry)]),
276332
);
277333
const nodePairingById = new Map(
278-
(params.pairedNodes ?? []).map((entry) => [entry.nodeId, buildApprovedNodeSource(entry)]),
334+
(params.pairedNodes ?? [])
335+
.filter((entry) => hasAddressableId(entry.nodeId))
336+
.map((entry) => [entry.nodeId, buildApprovedNodeSource(entry)]),
279337
);
280338
const pendingNodePairingById = new Map<string, KnownNodePendingSource>();
281339
// listNodePairing returns newest requests first; keep the current approval action per node.
282340
for (const entry of params.pendingNodes ?? []) {
341+
if (!hasAddressableId(entry.nodeId)) {
342+
continue;
343+
}
283344
if (!pendingNodePairingById.has(entry.nodeId)) {
284345
pendingNodePairingById.set(entry.nodeId, buildPendingNodeSource(entry));
285346
}

‎src/shared/node-list-parse.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,30 @@ describe("shared/node-list-parse", () => {
6060
paired: [{ nodeId: "n1" }],
6161
});
6262
});
63+
64+
it("drops pairing rows with a non-string or empty required id instead of emitting empty-id sentinels", () => {
65+
const { pending, paired } = parsePairingList({
66+
pending: [
67+
{ requestId: 7, nodeId: {}, ts: 1 }, // non-string required ids -> dropped
68+
{ requestId: " ", nodeId: "n0", ts: 2 }, // whitespace-only requestId -> dropped
69+
{ requestId: "r1", nodeId: "n1", displayName: 42, remoteIp: 99, platform: true, ts: 3 },
70+
],
71+
paired: [
72+
{ nodeId: 5, token: 3 }, // non-string nodeId -> dropped
73+
{ nodeId: "n2", displayName: { x: 1 }, remoteIp: [], lastSeenReason: 0 },
74+
],
75+
});
76+
// A malformed required id drops the whole row (no "" sentinel a consumer would trust); the valid
77+
// row survives with optional scalars normalized to undefined so renderers never trim a non-string.
78+
expect(pending).toHaveLength(1);
79+
expect(pending[0]).toMatchObject({ requestId: "r1", nodeId: "n1" });
80+
expect(pending[0].displayName).toBeUndefined();
81+
expect(pending[0].remoteIp).toBeUndefined();
82+
expect(pending[0].platform).toBeUndefined();
83+
expect(paired).toHaveLength(1);
84+
expect(paired[0]).toMatchObject({ nodeId: "n2" });
85+
expect(paired[0].displayName).toBeUndefined();
86+
expect(paired[0].remoteIp).toBeUndefined();
87+
expect(paired[0].lastSeenReason).toBeUndefined();
88+
});
6389
});

‎src/shared/node-list-parse.ts‎

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,66 @@
11
// Node list parsing helpers normalize node inventory records from CLI output.
22
import { asRecord } from "@openclaw/normalization-core/record-coerce";
3+
import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce";
34
import type { NodeListNode, PairedNode, PairingList, PendingRequest } from "./node-list-types.js";
45

6+
// pending/paired rows are blind-cast from a permissive pairing file, so any scalar can be
7+
// non-string. CLI renderers call `.trim()`/`sanitizeTerminalText` on them (these rows bypass
8+
// the gateway node catalog), so normalize at this shared parse boundary to keep every
9+
// consumer crash-safe.
10+
// A pending/paired row needs an addressable string id to be approved, keyed, or rendered. A
11+
// non-string required id drops the row entirely rather than becoming an empty-string sentinel that
12+
// downstream consumers would treat as a real id.
13+
function normalizePendingRequest(row: PendingRequest): PendingRequest | null {
14+
const requestId = normalizeOptionalString(row.requestId);
15+
const nodeId = normalizeOptionalString(row.nodeId);
16+
if (requestId === undefined || nodeId === undefined) {
17+
return null;
18+
}
19+
return {
20+
...row,
21+
requestId,
22+
nodeId,
23+
displayName: normalizeOptionalString(row.displayName),
24+
platform: normalizeOptionalString(row.platform),
25+
version: normalizeOptionalString(row.version),
26+
coreVersion: normalizeOptionalString(row.coreVersion),
27+
uiVersion: normalizeOptionalString(row.uiVersion),
28+
remoteIp: normalizeOptionalString(row.remoteIp),
29+
};
30+
}
31+
32+
function normalizePairedNode(row: PairedNode): PairedNode | null {
33+
const nodeId = normalizeOptionalString(row.nodeId);
34+
if (nodeId === undefined) {
35+
return null;
36+
}
37+
return {
38+
...row,
39+
nodeId,
40+
token: normalizeOptionalString(row.token),
41+
displayName: normalizeOptionalString(row.displayName),
42+
platform: normalizeOptionalString(row.platform),
43+
version: normalizeOptionalString(row.version),
44+
coreVersion: normalizeOptionalString(row.coreVersion),
45+
uiVersion: normalizeOptionalString(row.uiVersion),
46+
remoteIp: normalizeOptionalString(row.remoteIp),
47+
lastSeenReason: normalizeOptionalString(row.lastSeenReason),
48+
};
49+
}
50+
551
/** Extracts pending and paired node arrays from permissive node.pair.list payloads. */
652
export function parsePairingList(value: unknown): PairingList {
753
const obj = asRecord(value);
8-
const pending = Array.isArray(obj.pending) ? (obj.pending as PendingRequest[]) : [];
9-
const paired = Array.isArray(obj.paired) ? (obj.paired as PairedNode[]) : [];
54+
const pending = Array.isArray(obj.pending)
55+
? (obj.pending as PendingRequest[])
56+
.map(normalizePendingRequest)
57+
.filter((row): row is PendingRequest => row !== null)
58+
: [];
59+
const paired = Array.isArray(obj.paired)
60+
? (obj.paired as PairedNode[])
61+
.map(normalizePairedNode)
62+
.filter((row): row is PairedNode => row !== null)
63+
: [];
1064
return { pending, paired };
1165
}
1266

0 commit comments

Comments
 (0)