From d0ec7792d94b233e22bda0d23238ccaefb121719 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Wed, 6 May 2026 12:24:41 +0000 Subject: [PATCH 1/2] fix(credentials): clean up empty legacy credentials.json on upgrade Pre-gateway NemoClaw versions wrote credentials to ~/.nemoclaw/credentials.json. The current onboard completion path migrates and securely unlinks that file, but only when stageLegacyCredentialsToEnv() returned at least one staged key. On upgrades from older versions where the file was already empty ({}), or held only keys outside KNOWN_CREDENTIAL_ENV_KEYS, or only blank/whitespace values, the stage call returned [] and the file was silently left on disk indefinitely. Add removeLegacyCredentialsFileIfEmpty() that re-inspects the legacy file under the same symlink/O_NOFOLLOW/size guards as the migration path and secureUnlink()s it iff zero allowlisted keys carry a non-empty string value. Wire it into a new tiny host-artifact-cleanup module (cleanupStaleHostFiles) that runs at the end of the onboard success path, so future stale-file leftovers can be added by appending one entry without touching onboard.ts. Tests cover empty {}, unknown-keys-only, blank-value-only, real-credential preserved, missing file, symlink-refusal, corrupt-JSON, and zero-fill-on- unlink for defence-in-depth, plus end-to-end coverage of the runner. Signed-off-by: Tinson Lai --- src/lib/credentials.ts | 69 +++++++++++++++ src/lib/host-artifact-cleanup.ts | 59 +++++++++++++ src/lib/onboard.ts | 9 ++ test/credentials.test.ts | 134 ++++++++++++++++++++++++++++- test/host-artifact-cleanup.test.ts | 92 ++++++++++++++++++++ 5 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 src/lib/host-artifact-cleanup.ts create mode 100644 test/host-artifact-cleanup.test.ts diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index dde86d11cb4..ba68c67546a 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -383,6 +383,75 @@ export function removeLegacyCredentialsFile(): void { secureUnlink(legacyFile); } +/** + * Securely remove the legacy plaintext credentials.json *iff* it carries + * no migratable credential payload — i.e. it's an empty `{}`, contains + * only keys outside `KNOWN_CREDENTIAL_ENV_KEYS`, or every allowlisted key + * has a blank/non-string value. Used by the onboard completion path to + * clean up the stale empty file left behind on upgrades from pre-gateway + * NemoClaw versions (#3105). + * + * Refuses to act on a missing/symlinked/oversized/non-JSON/non-object + * file — those are either nothing-to-do or "leave for inspection" cases + * that the regular migration path already handles. Returns `true` only + * if the file was actually removed. + */ +export function removeLegacyCredentialsFileIfEmpty(): boolean { + const legacyFile = getCredsFile(); + + try { + rejectSymlinksOnPath(path.dirname(legacyFile)); + } catch { + return false; + } + + let fd: number; + try { + fd = fs.openSync(legacyFile, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); + } catch { + return false; + } + + let raw: string; + try { + const stat = fs.fstatSync(fd); + if (!stat.isFile()) return false; + if (stat.size > LEGACY_CREDS_FILE_MAX_BYTES) return false; + raw = fs.readFileSync(fd, "utf-8"); + } catch { + return false; + } finally { + try { + fs.closeSync(fd); + } catch { + /* fd already closed; ignore */ + } + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return false; + } + + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return false; + } + + const allowed = new Set(KNOWN_CREDENTIAL_ENV_KEYS); + for (const [key, value] of Object.entries(parsed as Record)) { + if (!allowed.has(key)) continue; + if (typeof value !== "string") continue; + if (normalizeCredentialValue(value)) { + return false; + } + } + + secureUnlink(legacyFile); + return true; +} + /** * Read a secret value from a TTY without echoing typed characters * (asterisks are written instead). Resolves to the trimmed answer or diff --git a/src/lib/host-artifact-cleanup.ts b/src/lib/host-artifact-cleanup.ts new file mode 100644 index 00000000000..17d51b6b5e4 --- /dev/null +++ b/src/lib/host-artifact-cleanup.ts @@ -0,0 +1,59 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Post-upgrade stale-file cleanup. +// +// As NemoClaw evolves, host-side state migrates: older versions wrote files +// under the user's home that newer versions no longer use. Each entry in +// STALE_FILES owns the safety logic for one specific stale file and reports +// back when it actually removed something. The runner invokes them all once +// during the onboard completion path so a fresh post-upgrade onboard sweeps +// every leftover in a single uniform pass. Adding a future cleanup is one +// entry on STALE_FILES; the loop does not change. If a future leftover is a +// folder or symlink instead of a file, widen the shape at that point. + +import { removeLegacyCredentialsFileIfEmpty } from "./credentials"; + +interface StaleHostFile { + /** Human-readable description for the success log line. */ + readonly description: string; + /** + * Atomically inspect-and-remove the file iff its safety guards + * permit it. Returns true iff the file was actually removed. + * Splitting "is-removable?" from "remove" would be TOCTOU-unsafe, + * so each entry exposes a single combined operation. + */ + readonly tryRemove: () => boolean; +} + +const STALE_FILES: readonly StaleHostFile[] = [ + { + description: "~/.nemoclaw/credentials.json (no migratable credentials)", + tryRemove: removeLegacyCredentialsFileIfEmpty, + }, +]; + +/** + * Sweep every registered stale host file left behind by older NemoClaw + * versions. Best-effort: a failure inside one entry is logged to stderr + * but never aborts the others or the surrounding onboard. Logs a single + * line to stdout per file actually removed so the user can audit what + * changed. Safe to call after a successful onboard regardless of which + * migration paths fired earlier — every entry is a no-op when its + * target doesn't exist or doesn't satisfy the entry's safety rules. + */ +export function cleanupStaleHostFiles(): void { + for (const file of STALE_FILES) { + try { + if (file.tryRemove()) { + console.log(` Removed stale ${file.description}.`); + } + } catch (error) { + console.error( + ` Skipped stale-file cleanup ${file.description}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + } +} diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index c4ca1772fca..01a3b6b6830 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -272,6 +272,9 @@ const { saveCredential, } = credentials; const { hashCredential }: typeof import("./credential-hash") = require("./credential-hash"); +const { + cleanupStaleHostFiles, +}: typeof import("./host-artifact-cleanup") = require("./host-artifact-cleanup"); const registry: typeof import("./registry") = require("./registry"); const nim: typeof import("./nim") = require("./nim"); const onboardSession: typeof import("./onboard-session") = require("./onboard-session"); @@ -9482,6 +9485,12 @@ async function onboard(opts: OnboardOptions = {}): Promise { `providers/channels enabled to migrate them, then the file is removed automatically.`, ); } + // Sweep stale host files left over from older NemoClaw versions — + // e.g. an empty/orphaned ~/.nemoclaw/credentials.json from upgrades + // before the credentials-gateway move (issue #3105). Each registered + // entry enforces its own safety guards; this call is a no-op when + // every target is already clean. + cleanupStaleHostFiles(); printDashboard(sandboxName, model, provider, nimContainer, agent); } finally { releaseOnboardLock(); diff --git a/test/credentials.test.ts b/test/credentials.test.ts index 11588f30971..a8f08312647 100644 --- a/test/credentials.test.ts +++ b/test/credentials.test.ts @@ -20,7 +20,8 @@ function isCredentialsModule(value: object | null): value is CredentialsModule { typeof Reflect.get(value, "getCredential") === "function" && typeof Reflect.get(value, "saveCredential") === "function" && typeof Reflect.get(value, "stageLegacyCredentialsToEnv") === "function" && - typeof Reflect.get(value, "removeLegacyCredentialsFile") === "function" + typeof Reflect.get(value, "removeLegacyCredentialsFile") === "function" && + typeof Reflect.get(value, "removeLegacyCredentialsFileIfEmpty") === "function" ); } @@ -432,6 +433,137 @@ describe("legacy credentials.json migration (two-phase: stage then remove)", () }); }); +describe("removeLegacyCredentialsFileIfEmpty (post-upgrade cleanup, #3105)", () => { + it("removes an empty {} legacy file (regression #3105)", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, "{}", { mode: 0o600 }); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + expect(fs.existsSync(legacyFile)).toBe(false); + }); + + it("removes a file containing only unknown keys", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync( + legacyFile, + JSON.stringify({ FOO: "bar", PATH: "/etc/passwd" }), + { mode: 0o600 }, + ); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + expect(fs.existsSync(legacyFile)).toBe(false); + }); + + it("removes a file where every allowlisted value is blank/whitespace", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync( + legacyFile, + JSON.stringify({ NVIDIA_API_KEY: "", OPENAI_API_KEY: " \r\n\t " }), + { mode: 0o600 }, + ); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + expect(fs.existsSync(legacyFile)).toBe(false); + }); + + it("keeps a file with at least one non-empty allowlisted credential", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + const payload = JSON.stringify({ NVIDIA_API_KEY: "nvapi-real-secret", FOO: "bar" }); + fs.writeFileSync(legacyFile, payload, { mode: 0o600 }); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(false); + expect(fs.existsSync(legacyFile)).toBe(true); + expect(fs.readFileSync(legacyFile, "utf-8")).toBe(payload); + }); + + it("returns false when no legacy file exists", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(false); + }); + + it("refuses to act on a symlinked legacy path (target untouched)", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + + const victimFile = path.join(home, "victim.json"); + fs.writeFileSync(victimFile, "{}", { mode: 0o600 }); + fs.symlinkSync(victimFile, legacyFile); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(false); + expect(fs.existsSync(legacyFile)).toBe(true); + expect(fs.existsSync(victimFile)).toBe(true); + }); + + it("leaves a corrupt legacy file in place for inspection", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, "{not-json", { mode: 0o600 }); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(false); + expect(fs.existsSync(legacyFile)).toBe(true); + }); + + it("zero-fills an empty file before unlinking (defence in depth)", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + const cleartext = "{}"; + fs.writeFileSync(legacyFile, cleartext, { mode: 0o600 }); + + const originalUnlink = fs.unlinkSync; + const captured: { bytes: Buffer | null } = { bytes: null }; + const spy = vi.spyOn(fs, "unlinkSync").mockImplementation((p) => { + if (typeof p === "string" && p === legacyFile && captured.bytes === null) { + try { + captured.bytes = fs.readFileSync(p); + } catch { + /* file already gone */ + } + } + return originalUnlink(p); + }); + + try { + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + } finally { + spy.mockRestore(); + } + + const bytesAtUnlink = captured.bytes; + expect(bytesAtUnlink).not.toBeNull(); + if (bytesAtUnlink !== null) { + expect(bytesAtUnlink.length).toBe(Buffer.byteLength(cleartext)); + expect(bytesAtUnlink.every((b) => b === 0)).toBe(true); + } + expect(fs.existsSync(legacyFile)).toBe(false); + }); +}); + describe("prompt machinery (unchanged)", () => { it("exits cleanly when answers are staged through a pipe", () => { const script = ` diff --git a/test/host-artifact-cleanup.test.ts b/test/host-artifact-cleanup.test.ts new file mode 100644 index 00000000000..6dd3624c8f1 --- /dev/null +++ b/test/host-artifact-cleanup.test.ts @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { KNOWN_CREDENTIAL_ENV_KEYS } from "../dist/lib/credentials.js"; +import { cleanupStaleHostFiles } from "../dist/lib/host-artifact-cleanup.js"; + +const TRACKED_ENV_KEYS = [...KNOWN_CREDENTIAL_ENV_KEYS]; + +function clearTrackedEnv() { + for (const key of TRACKED_ENV_KEYS) { + delete process.env[key]; + } +} + +function captureConsole(fn: () => T): { result: T; stdout: string[]; stderr: string[] } { + const stdout: string[] = []; + const stderr: string[] = []; + const origLog = console.log; + const origErr = console.error; + console.log = (...args: unknown[]) => { + stdout.push(args.map((a) => String(a)).join(" ")); + }; + console.error = (...args: unknown[]) => { + stderr.push(args.map((a) => String(a)).join(" ")); + }; + try { + const result = fn(); + return { result, stdout, stderr }; + } finally { + console.log = origLog; + console.error = origErr; + } +} + +beforeEach(() => { + clearTrackedEnv(); +}); + +afterEach(() => { + clearTrackedEnv(); + vi.restoreAllMocks(); + vi.unstubAllEnvs(); +}); + +describe("cleanupStaleHostFiles (post-upgrade sweep, #3105)", () => { + it("removes an empty legacy credentials.json and logs the removal", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cleanup-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, "{}", { mode: 0o600 }); + vi.stubEnv("HOME", home); + + const { stdout, stderr } = captureConsole(() => cleanupStaleHostFiles()); + + expect(fs.existsSync(legacyFile)).toBe(false); + expect(stdout.join("\n")).toMatch(/Removed stale .*credentials\.json/); + expect(stderr).toEqual([]); + }); + + it("keeps a credentials.json carrying real credentials and logs nothing", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cleanup-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + const payload = JSON.stringify({ NVIDIA_API_KEY: "nvapi-real" }); + fs.writeFileSync(legacyFile, payload, { mode: 0o600 }); + vi.stubEnv("HOME", home); + + const { stdout, stderr } = captureConsole(() => cleanupStaleHostFiles()); + + expect(fs.existsSync(legacyFile)).toBe(true); + expect(fs.readFileSync(legacyFile, "utf-8")).toBe(payload); + expect(stdout).toEqual([]); + expect(stderr).toEqual([]); + }); + + it("is a no-op (and emits no log) when no stale file exists", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cleanup-")); + vi.stubEnv("HOME", home); + + const { stdout, stderr } = captureConsole(() => cleanupStaleHostFiles()); + + expect(stdout).toEqual([]); + expect(stderr).toEqual([]); + }); +}); From 0a089dfdef5c10b02abb609e337786b20d6ad62b Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Wed, 6 May 2026 13:34:21 +0000 Subject: [PATCH 2/2] fix(credentials): widen empty-file detection and verify unlink success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two CodeRabbit findings on the legacy credentials cleanup helper: 1. A 0-byte or whitespace-only credentials.json now matches the empty-{} path instead of throwing inside JSON.parse and getting stuck on disk. Pre-gateway versions or partial writes can produce these shapes; they carry no migratable payload, so fall through to the unlink. 2. secureUnlink is best-effort and swallows errors. Verify the file is actually gone with lstatSync (treating ENOENT as success) before returning true — otherwise the runner would log "Removed stale ..." on a permission-denied unlink, misleading the user. Add regression tests for both edge cases. Signed-off-by: Tinson Lai --- src/lib/credentials.ts | 48 ++++++++++++++++++++++++++-------------- test/credentials.test.ts | 45 +++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index ba68c67546a..c35602397aa 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -428,28 +428,44 @@ export function removeLegacyCredentialsFileIfEmpty(): boolean { } } - let parsed: unknown; - try { - parsed = JSON.parse(raw); - } catch { - return false; - } - - if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { - return false; - } + // A 0-byte or whitespace-only file is functionally identical to an + // empty {} — there's no migratable payload, so skip JSON.parse (which + // would throw on the empty input) and fall through to the unlink. + if (raw.trim() !== "") { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return false; + } - const allowed = new Set(KNOWN_CREDENTIAL_ENV_KEYS); - for (const [key, value] of Object.entries(parsed as Record)) { - if (!allowed.has(key)) continue; - if (typeof value !== "string") continue; - if (normalizeCredentialValue(value)) { + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { return false; } + + const allowed = new Set(KNOWN_CREDENTIAL_ENV_KEYS); + for (const [key, value] of Object.entries(parsed as Record)) { + if (!allowed.has(key)) continue; + if (typeof value !== "string") continue; + if (normalizeCredentialValue(value)) { + return false; + } + } } + // secureUnlink is best-effort and swallows errors. Verify the file is + // actually gone before claiming a successful removal — otherwise the + // runner would log "Removed stale ..." on a permission-denied unlink. secureUnlink(legacyFile); - return true; + try { + fs.lstatSync(legacyFile); + } catch (error) { + if (isErrnoException(error) && error.code === "ENOENT") { + return true; + } + return false; + } + return false; } /** diff --git a/test/credentials.test.ts b/test/credentials.test.ts index a8f08312647..3f5b3883688 100644 --- a/test/credentials.test.ts +++ b/test/credentials.test.ts @@ -526,6 +526,51 @@ describe("removeLegacyCredentialsFileIfEmpty (post-upgrade cleanup, #3105)", () expect(fs.existsSync(legacyFile)).toBe(true); }); + it("removes a 0-byte legacy file (CodeRabbit nit: whitespace-only doesn't throw)", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, "", { mode: 0o600 }); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + expect(fs.existsSync(legacyFile)).toBe(false); + }); + + it("removes a whitespace-only legacy file", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, " \n\t\r\n ", { mode: 0o600 }); + + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(true); + expect(fs.existsSync(legacyFile)).toBe(false); + }); + + it("returns false when the secure unlink silently fails (CodeRabbit nit)", async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); + const credsDir = path.join(home, ".nemoclaw"); + const legacyFile = path.join(credsDir, "credentials.json"); + fs.mkdirSync(credsDir, { recursive: true }); + fs.writeFileSync(legacyFile, "{}", { mode: 0o600 }); + + // Simulate a swallowed unlink failure: secureUnlink internally calls + // fs.unlinkSync with try/catch, so a no-op stub leaves the file intact. + // The helper must detect this and return false rather than lying. + const spy = vi.spyOn(fs, "unlinkSync").mockImplementation(() => undefined); + try { + const credentials = await importCredentialsModule(home); + expect(credentials.removeLegacyCredentialsFileIfEmpty()).toBe(false); + } finally { + spy.mockRestore(); + } + + expect(fs.existsSync(legacyFile)).toBe(true); + }); + it("zero-fills an empty file before unlinking (defence in depth)", async () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-creds-")); const credsDir = path.join(home, ".nemoclaw");