fix(credentials): clean up empty legacy credentials.json on upgrade - #3119
Merged
Conversation
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 <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a safety-first post-upgrade stale-file sweep: a new credentials helper that conditionally and securely removes legacy ChangesLegacy Credentials Cleanup
Sequence DiagramsequenceDiagram
participant Onboard as onboard.ts
participant Cleaner as cleanupStaleHostFiles()
participant Creds as removeLegacyCredentialsFileIfEmpty()
participant FS as Filesystem
Onboard->>Cleaner: finalize onboarding
Cleaner->>Creds: attempt removal of legacy credentials.json
Creds->>FS: open with O_NOFOLLOW / reject symlinked ancestor paths
FS-->>Creds: open/stat result
alt unsafe (symlink/path reject or open failure)
Creds-->>Cleaner: false (skip)
Cleaner->>Cleaner: log skip/error
else safe regular file
Creds->>FS: stat size
FS-->>Creds: size result
alt size > cap or parse error
Creds-->>Cleaner: false (skip)
Cleaner->>Cleaner: log skip
else size OK and JSON object
Creds->>Creds: check KNOWN_CREDENTIAL_ENV_KEYS for non-blank strings
alt usable credentials present
Creds-->>Cleaner: false (preserve)
Cleaner->>Cleaner: log preserved
else empty/unknown-only
Creds->>FS: zero-fill file then unlink
FS-->>Creds: removed
Creds-->>Cleaner: true (removed)
Cleaner->>Cleaner: log removal
end
end
end
Cleaner-->>Onboard: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
9488-9493: Run targeted onboarding E2Es for this path change.Given this executes in the core onboarding finalization path, I recommend running the listed onboarding-related nightly jobs before merge (
cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e,messaging-compatible-endpoint-e2e,hermes-discord-e2e).As per coding guidelines,
src/lib/onboard.tschanges should be validated with the recommended E2E set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` around lines 9488 - 9493, This change adds a cleanup call in the onboarding finalization path (cleanupStaleHostFiles()), so before merging run the targeted onboarding E2E suites to validate behavior: execute cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e, messaging-compatible-endpoint-e2e, and hermes-discord-e2e against the onboarding flow to ensure cleanupStaleHostFiles() doesn't regress registrations, credentials handling, or downstream integrations; if any test fails, investigate the onboarding finalization sequence around the cleanupStaleHostFiles() invocation and adjust guards or ordering accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/credentials.ts`:
- Around line 431-435: Before calling JSON.parse(raw), detect zero-byte or
whitespace-only legacy files by checking raw.trim() length; if empty, treat the
file as having no migratable payload (skip JSON.parse) and return the same
success/cleanup signal you use for stale artifacts so the cleanup can delete it.
Update the logic surrounding JSON.parse(raw) and the variables parsed/raw so
whitespace-only files don't throw and get stuck.
- Around line 451-452: The code returns true unconditionally after calling
secureUnlink(legacyFile) which swallows failures, causing
cleanupStaleHostFiles() to report removal even if the file remains; modify the
logic so that after calling secureUnlink(legacyFile) you verify the file's
absence (e.g., stat or exists check and treat ENOENT as success) and only return
true if the file is actually gone (or change secureUnlink to return a boolean
indicating removal and use that result). Ensure you reference secureUnlink and
cleanupStaleHostFiles and base the success return on the post-unlink existence
check or the new secureUnlink return value for legacyFile.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 9488-9493: This change adds a cleanup call in the onboarding
finalization path (cleanupStaleHostFiles()), so before merging run the targeted
onboarding E2E suites to validate behavior: execute cloud-e2e,
sandbox-operations-e2e, rebuild-openclaw-e2e, messaging-compatible-endpoint-e2e,
and hermes-discord-e2e against the onboarding flow to ensure
cleanupStaleHostFiles() doesn't regress registrations, credentials handling, or
downstream integrations; if any test fails, investigate the onboarding
finalization sequence around the cleanupStaleHostFiles() invocation and adjust
guards or ordering accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c02ee8e-8ae6-4d37-a95a-c5a6cfe9e8ed
📒 Files selected for processing (5)
src/lib/credentials.tssrc/lib/host-artifact-cleanup.tssrc/lib/onboard.tstest/credentials.test.tstest/host-artifact-cleanup.test.ts
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 <tinsonl@nvidia.com>
…p-empty-legacy-credentials # Conflicts: # src/lib/onboard.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/credentials.ts`:
- Around line 415-459: You are currently validating the file by inode (using fd
and stat) but then unconditionally call secureUnlink(legacyFile) after closing
fd, which can delete a different file if the path was replaced; instead preserve
and compare the original inode before deleting: capture the original stat (the
existing stat variable from fstatSync(fd)), close the fd, then before calling
secureUnlink(legacyFile) re-stat the pathname (fs.statSync(legacyFile)) and
compare key identifiers (dev and ino) to the original stat — only call
secureUnlink when they match, otherwise abort and do not unlink; reference the
existing symbols legacyFile, fd, stat (from fstatSync), and secureUnlink in your
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef57adc5-73c6-49e5-9ba7-af4f635375a9
📒 Files selected for processing (2)
src/lib/credentials.tssrc/lib/onboard.ts
Comment on lines
+415
to
+459
| 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 */ | ||
| } | ||
| } | ||
|
|
||
| // 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; | ||
| } | ||
|
|
||
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | ||
| return false; | ||
| } | ||
|
|
||
| const allowed = new Set<string>(KNOWN_CREDENTIAL_ENV_KEYS); | ||
| for (const [key, value] of Object.entries(parsed as Record<string, unknown>)) { | ||
| 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); |
Contributor
There was a problem hiding this comment.
Don't inspect one inode and delete another.
The emptiness check is done on the fd-backed inode you read here, but Line 459 deletes by pathname after that fd has been closed. If another process replaces credentials.json in that window, this can classify inode A as empty and then wipe inode B instead — including a newly written real credential file. Please keep the validated inode pinned through cleanup, or abort when the current path no longer matches the inode you inspected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/credentials.ts` around lines 415 - 459, You are currently validating
the file by inode (using fd and stat) but then unconditionally call
secureUnlink(legacyFile) after closing fd, which can delete a different file if
the path was replaced; instead preserve and compare the original inode before
deleting: capture the original stat (the existing stat variable from
fstatSync(fd)), close the fd, then before calling secureUnlink(legacyFile)
re-stat the pathname (fs.statSync(legacyFile)) and compare key identifiers (dev
and ino) to the original stat — only call secureUnlink when they match,
otherwise abort and do not unlink; reference the existing symbols legacyFile,
fd, stat (from fstatSync), and secureUnlink in your change.
ericksoa
approved these changes
May 7, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed and fixed on top of the PR branch. Resolved the current-main conflict by keeping the stale host-file cleanup, current ./state/registry import, and main's post-deployment verification before printDashboard(). CodeRabbit is clean on head a6561ad and all PR checks are green. Local validation passed: npm run build:cli, npm run typecheck:cli, npm run source-shape:check, git diff --check, focused credential/host-artifact cleanup tests, and the legacy credentials onboard test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Securely remove a stale
~/.nemoclaw/credentials.jsonleft on host disk after upgrading from pre-gateway versions when the file has nothing to migrate (empty{}, only unknown keys, or only blank values). Closes the security-hygiene gap where cleartext API keys could linger indefinitely after the gateway-only migration in #2554.Related Issue
Closes #3105
Changes
removeLegacyCredentialsFileIfEmpty()insrc/lib/credentials.tsthat re-inspects the legacy file under the same symlink-rejection / O_NOFOLLOW / size-cap guards used bystageLegacyCredentialsToEnv()andsecureUnlinks it iff zero allowlisted keys carry a non-empty string value.src/lib/host-artifact-cleanup.tsmodule exposingcleanupStaleHostFiles()— a tiny runner over aSTALE_FILESconst list. Today it has one entry (the legacy credentials file); future stale-file leftovers can be added by appending another entry without touchingonboard.ts. Cleaner-level errors are isolated so a single failure can't abort onboard completion.cleanupStaleHostFiles()into the onboard success path immediately after the existing migrate/warn branches, so a fresh post-upgrade onboard sweeps any leftover artifacts in one uniform pass.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Improvements
Tests