Skip to content

Commit 3b66543

Browse files
committed
fix(security): escape rejected names in CLI validation diagnostics
The CLI name validators echoed rejected input verbatim, so a name carrying ANSI escape or control bytes reached the terminal and CI logs unchanged. Reuse the canonical bounded ASCII preview already applied to blueprint names. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
1 parent 264564e commit 3b66543

12 files changed

Lines changed: 188 additions & 12 deletions

docs/reference/troubleshooting.mdx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,9 @@ Sandbox names must be lowercase, start with a letter, contain only letters, numb
663663
The CLI rejects names that do not match these rules.
664664
It prints a `Try: <suggested-slug>` recovery line whenever it can derive a valid lowercase, hyphen-separated form from the input, so passing `--name MyAssistant` reports `Try: myassistant` and you can rerun with the suggested slug.
665665

666+
The error repeats the rejected value as a quoted preview rather than the raw input.
667+
Characters outside printable ASCII appear as `\uXXXX` escapes and the preview stops after 80 characters, so a name that carries control or escape sequences cannot change your terminal or a CI log.
668+
666669
Names that collide with global CLI commands are also rejected.
667670
Reserved names include `onboard`, `list`, `deploy`, `setup`, `start`, `stop`, `status`, `debug`, `uninstall`, `credentials`, and `help`.
668671
Using a reserved name would cause the CLI to route to the global command instead of the sandbox.

nemoclaw/src/shared/sandbox-name.cts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ const INVALID_NAME_PREVIEW_MAX_LENGTH = 80;
4444
* terminal or CI log. Escaping every non-ASCII UTF-16 code unit also covers
4545
* C1 controls, line separators, bidi formatting, and unpaired surrogates.
4646
*/
47-
function diagnosticPreview(value: unknown): string {
47+
export function diagnosticPreview(value: unknown): string {
4848
const raw = String(value);
4949
const prefix = raw.slice(0, INVALID_NAME_PREVIEW_MAX_LENGTH);
5050
let escaped = '"';
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
import { describe, expect, it } from "vitest";
5+
6+
import {
7+
validateMcpServerName,
8+
validatePersistedMcpCredentialEnvName,
9+
validateSandboxName,
10+
} from "./mcp-bridge-validation";
11+
12+
const ESC = String.fromCharCode(27);
13+
14+
function messageFrom(reject: () => void): string {
15+
try {
16+
reject();
17+
} catch (error) {
18+
return (error as Error).message;
19+
}
20+
throw new Error("expected the validator to reject the name");
21+
}
22+
23+
describe("MCP bridge name diagnostics", () => {
24+
it("escapes control characters in a rejected sandbox name (#7796)", () => {
25+
const message = messageFrom(() => validateSandboxName(`bad${ESC}[31mX`));
26+
27+
expect(message).toContain(String.raw`Invalid sandbox name "bad\u001b[31mX"`);
28+
expect(message).not.toContain(ESC);
29+
});
30+
31+
it("escapes control characters in a rejected MCP server name (#7796)", () => {
32+
const message = messageFrom(() => validateMcpServerName(`srv${ESC}]0;title`));
33+
34+
expect(message).toContain(String.raw`Invalid MCP server name "srv\u001b]0;title"`);
35+
expect(message).not.toContain(ESC);
36+
});
37+
38+
it("escapes control characters in a rejected credential environment name (#7796)", () => {
39+
const message = messageFrom(() => validatePersistedMcpCredentialEnvName(`TOKEN${ESC}[2J`));
40+
41+
expect(message).toContain(String.raw`Invalid environment variable name "TOKEN\u001b[2J"`);
42+
expect(message).not.toContain(ESC);
43+
});
44+
45+
it("bounds an over-length rejected name to a truncated preview (#7796)", () => {
46+
const message = messageFrom(() => validateSandboxName(`Bad${"x".repeat(200)}`));
47+
48+
expect(message).toContain(`"Bad${"x".repeat(77)}..."`);
49+
expect(message.length).toBeLessThan(200);
50+
});
51+
});

src/lib/actions/sandbox/mcp-bridge-status-state.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ registry.registerSandbox({ name: "openclaw-sandbox", agent: "openclaw" });
219219
}>;
220220
};
221221
expect(payload.invalid.exitCode).toBe(2);
222-
expect(payload.invalid.message).toContain("Invalid MCP server name '__proto__'");
222+
expect(payload.invalid.message).toContain('Invalid MCP server name "__proto__"');
223223
expect(payload.inherited).toHaveLength(1);
224224
expect(payload.inherited[0]).toMatchObject({
225225
server: "constructor",

src/lib/actions/sandbox/mcp-bridge-validation.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { type SpawnSyncReturns, spawnSync } from "node:child_process";
55
import crypto from "node:crypto";
66

77
import { resolveOpenshell } from "../../adapters/openshell/resolve";
8+
import { diagnosticPreview } from "../../name-validation";
89
import type { McpBridgeEntry } from "../../state/registry";
910
import { buildSubprocessEnv, isSubprocessEnvNameAllowed } from "../../subprocess-env";
1011
import {
@@ -162,7 +163,7 @@ const MCP_PROVIDER_HASH_BYTES = 8;
162163
export function validateSandboxName(name: string): void {
163164
if (!name || name.length > 63 || !VALID_SANDBOX_RE.test(name)) {
164165
throw new McpBridgeError(
165-
`Invalid sandbox name '${name}'. Names must be 1-63 lowercase alphanumeric characters with optional internal hyphens.`,
166+
`Invalid sandbox name ${diagnosticPreview(name)}. Names must be 1-63 lowercase alphanumeric characters with optional internal hyphens.`,
166167
2,
167168
);
168169
}
@@ -171,7 +172,7 @@ export function validateSandboxName(name: string): void {
171172
export function validateMcpServerName(name: string): void {
172173
if (!VALID_SERVER_RE.test(name)) {
173174
throw new McpBridgeError(
174-
`Invalid MCP server name '${name}'. Names must start with a letter and contain only letters, digits, hyphens, and underscores.`,
175+
`Invalid MCP server name ${diagnosticPreview(name)}. Names must start with a letter and contain only letters, digits, hyphens, and underscores.`,
175176
2,
176177
);
177178
}
@@ -218,7 +219,7 @@ export function validateMcpCredentialEnvName(name: string): void {
218219
export function validatePersistedMcpCredentialEnvName(name: string): void {
219220
if (!VALID_ENV_RE.test(name)) {
220221
throw new McpBridgeError(
221-
`Invalid environment variable name '${name}'. Names must match [A-Za-z_][A-Za-z0-9_]*.`,
222+
`Invalid environment variable name ${diagnosticPreview(name)}. Names must match [A-Za-z_][A-Za-z0-9_]*.`,
222223
2,
223224
);
224225
}

src/lib/deploy/index.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ describe("executeDeploy", () => {
272272
await expect(executeDeploy(fixture.options)).rejects.toThrow("exit:1");
273273

274274
const errorText = fixture.errors.join("\n");
275-
expect(errorText).toContain("Invalid sandbox name: 'bad name'");
275+
expect(errorText).toContain('Invalid sandbox name: "bad name"');
276276
expect(errorText).toContain("Sandbox names cannot contain spaces.");
277277
expect(errorText).toContain(
278278
"Allowed format: 1-63 characters, lowercase, starts with a letter, letters/numbers/internal hyphens only, ends with letter/number.",

src/lib/name-validation.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
NAME_ALLOWED_FORMAT as CANONICAL_NAME_ALLOWED_FORMAT,
66
NAME_MAX_LENGTH as CANONICAL_NAME_MAX_LENGTH,
77
NAME_VALID_PATTERN as CANONICAL_NAME_VALID_PATTERN,
8+
diagnosticPreview as canonicalDiagnosticPreview,
89
} from "../../nemoclaw/dist/shared/sandbox-name.cjs";
910

1011
// sourceOfTruth: nemoclaw/src/shared/sandbox-name.cts
@@ -15,6 +16,7 @@ import {
1516
export const NAME_MAX_LENGTH = CANONICAL_NAME_MAX_LENGTH;
1617
export const NAME_ALLOWED_FORMAT = CANONICAL_NAME_ALLOWED_FORMAT;
1718
export const NAME_VALID_PATTERN = CANONICAL_NAME_VALID_PATTERN;
19+
export const diagnosticPreview = canonicalDiagnosticPreview;
1820

1921
function validationSubject(label: string): string {
2022
const normalized = label.trim().toLowerCase();

src/lib/runner.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ import path from "node:path";
1111

1212
import { redirectInheritedChildStdoutToStderr } from "./cli/stdout-guard";
1313
import { shellQuote } from "./core/shell-quote";
14-
import { NAME_ALLOWED_FORMAT, NAME_MAX_LENGTH, NAME_VALID_PATTERN } from "./name-validation";
14+
import {
15+
diagnosticPreview,
16+
NAME_ALLOWED_FORMAT,
17+
NAME_MAX_LENGTH,
18+
NAME_VALID_PATTERN,
19+
} from "./name-validation";
1520
import { detectDockerHost } from "./platform";
1621
import { redact, redactError, writeRedactedResult } from "./security/redact";
1722
import { buildSubprocessEnv } from "./subprocess-env";
@@ -380,11 +385,13 @@ function validateName(name: string, label = "name"): string {
380385
}
381386
if (name.length > NAME_MAX_LENGTH) {
382387
throw new Error(
383-
`${label} too long (max ${NAME_MAX_LENGTH} chars): '${name.slice(0, 20)}...'. Allowed format: ${NAME_ALLOWED_FORMAT}.`,
388+
`${label} too long (max ${NAME_MAX_LENGTH} chars): ${diagnosticPreview(name)}. Allowed format: ${NAME_ALLOWED_FORMAT}.`,
384389
);
385390
}
386391
if (!NAME_VALID_PATTERN.test(name)) {
387-
throw new Error(`Invalid ${label}: '${name}'. Allowed format: ${NAME_ALLOWED_FORMAT}.`);
392+
throw new Error(
393+
`Invalid ${label}: ${diagnosticPreview(name)}. Allowed format: ${NAME_ALLOWED_FORMAT}.`,
394+
);
388395
}
389396
return name;
390397
}

src/lib/shields/transition-lock.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,20 @@ describe("host shields transition lock", () => {
106106
fs.writeFileSync(guardPath, JSON.stringify(value), { mode: 0o600 });
107107
}
108108

109+
it("escapes control characters in a rejected sandbox name (#7796)", () => {
110+
const hostileName = `bad${String.fromCharCode(27)}[31mX`;
111+
112+
let message = "";
113+
try {
114+
shieldsTransitionLockPath(hostileName, stateDir);
115+
} catch (error) {
116+
message = (error as Error).message;
117+
}
118+
119+
expect(message).toContain(String.raw`Invalid sandbox name: "bad\u001b[31mX".`);
120+
expect(message).not.toContain(String.fromCharCode(27));
121+
});
122+
109123
it("atomically creates a regular owner file and removes it after the callback", () => {
110124
const locker = manager();
111125
const lockPath = shieldsTransitionLockPath("alpha", stateDir);

src/lib/shields/transition-lock.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,12 @@ import { randomBytes } from "node:crypto";
66
import fs from "node:fs";
77
import path from "node:path";
88

9-
import { NAME_ALLOWED_FORMAT, NAME_MAX_LENGTH, NAME_VALID_PATTERN } from "../name-validation";
9+
import {
10+
diagnosticPreview,
11+
NAME_ALLOWED_FORMAT,
12+
NAME_MAX_LENGTH,
13+
NAME_VALID_PATTERN,
14+
} from "../name-validation";
1015
import { resolveNemoclawStateDir } from "../state/paths";
1116
import { isProcessAlive, readProcessStartIdentity } from "./timer-control";
1217

@@ -171,7 +176,9 @@ function validateSandboxName(name: string): string {
171176
);
172177
}
173178
if (!NAME_VALID_PATTERN.test(name)) {
174-
throw new Error(`Invalid sandbox name: '${name}'. Allowed format: ${NAME_ALLOWED_FORMAT}.`);
179+
throw new Error(
180+
`Invalid sandbox name: ${diagnosticPreview(name)}. Allowed format: ${NAME_ALLOWED_FORMAT}.`,
181+
);
175182
}
176183
return name;
177184
}

0 commit comments

Comments
 (0)