security(blueprint): harden digest verification with format validation and timing-safe comparison#448
Conversation
…n and timing-safe comparison
The existing falsy guard on manifest.digest silently passes verification
when the digest field is empty, allowing unverified blueprint artifacts
to execute through both cliLaunch and cliMigrate.
- Reject empty, undefined, and whitespace-only digests with clear error
- Validate digest format against /^[a-f0-9]{64}$/ (SHA-256 hex)
- Skip directory hashing when digest is missing or malformed (no wasted I/O)
- Use crypto.timingSafeEqual for digest comparison
- Add test suite covering all rejection and acceptance cases
Related: NVIDIA#319
Signed-off-by: Christopher Lusk <christopherdlusk@gmail.com>
Assisted-by: Claude (Anthropic)
Signed-off-by: Christopher Lusk <122107484+north-echo@users.noreply.github.com>
📝 WalkthroughWalkthroughAdded a new Vitest test file and tightened blueprint digest verification: manifest digest format is validated, constant-time comparison is used, missing/invalid digests produce explicit errors, and directory hashing is skipped when appropriate. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nemoclaw/src/blueprint/verify.test.ts (1)
30-33: Align test sort semantics with production implementation.
expectedDigestuseslocaleCompare, while production digesting uses default string.sort(). Aligning them avoids locale-sensitive drift in future edge-case filenames.♻️ Suggested tweak
function expectedDigest(files: MockFile[]): string { const hash = createHash("sha256"); - const sorted = [...files].sort((a, b) => a.path.localeCompare(b.path)); + const sorted = [...files].sort((a, b) => + a.path < b.path ? -1 : a.path > b.path ? 1 : 0, + ); for (const f of sorted) { hash.update(f.path); hash.update(f.content); } return hash.digest("hex"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/verify.test.ts` around lines 30 - 33, The test's expectedDigest uses localeCompare which introduces locale-sensitive ordering; change the sort in expectedDigest to use the same default string ordering as production (i.e., compare paths with simple string comparison rather than localeCompare). Update the sort on the sorted array in expectedDigest (the [...files].sort((a, b) => ... ) call) to use a straightforward comparison like checking a.path < b.path / a.path > b.path (returning -1/1/0) so test ordering matches production digesting.nemoclaw/src/blueprint/verify.ts (1)
29-31: The current code is type-safe and does not have a runtime crash risk at line 29. The short-circuit evaluation (!manifest.digest ||) ensures.trim()is only called on truthy values; falsy values likeundefinedare caught first and never reach.trim(). Tests confirm the code handles undefined gracefully.While the suggested type guard is reasonable defensive programming to prevent potential issues if non-string values somehow reach the function, the TypeScript type system already enforces that
BlueprintManifest.digestis a string, and the parsing logic inresolve.tsguarantees string values at the source. No unsafe deserialization or type casting was found that could bypass this contract.The fix proposed is optional defensive hardening, not a critical issue fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/verify.ts` around lines 29 - 31, The manifest digest check should defensively ensure digest is a string before calling .trim(); update the condition in verify (the block that currently checks manifest.digest and then calls manifest.digest.trim()) to first assert typeof manifest.digest === "string" (e.g., replace the short-circuit expression with an explicit string-type check), then call .trim() and test against DIGEST_PATTERN; keep the existing error push message when missing or invalid and retain the subsequent DIGEST_PATTERN.test(manifest.digest) branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/blueprint/verify.test.ts`:
- Around line 30-33: The test's expectedDigest uses localeCompare which
introduces locale-sensitive ordering; change the sort in expectedDigest to use
the same default string ordering as production (i.e., compare paths with simple
string comparison rather than localeCompare). Update the sort on the sorted
array in expectedDigest (the [...files].sort((a, b) => ... ) call) to use a
straightforward comparison like checking a.path < b.path / a.path > b.path
(returning -1/1/0) so test ordering matches production digesting.
In `@nemoclaw/src/blueprint/verify.ts`:
- Around line 29-31: The manifest digest check should defensively ensure digest
is a string before calling .trim(); update the condition in verify (the block
that currently checks manifest.digest and then calls manifest.digest.trim()) to
first assert typeof manifest.digest === "string" (e.g., replace the
short-circuit expression with an explicit string-type check), then call .trim()
and test against DIGEST_PATTERN; keep the existing error push message when
missing or invalid and retain the subsequent
DIGEST_PATTERN.test(manifest.digest) branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1ee9f6c-769d-4632-88f8-28ef5f39cb2d
📒 Files selected for processing (2)
nemoclaw/src/blueprint/verify.test.tsnemoclaw/src/blueprint/verify.ts
Use simple string comparison instead of localeCompare in the test helper to match the production collectFiles().sort() behavior. Signed-off-by: Christopher Lusk <christopherdlusk@gmail.com> Assisted-by: Claude (Anthropic) Signed-off-by: Christopher Lusk <122107484+north-echo@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/blueprint/verify.test.ts (1)
49-53: Consider stricter path matching in mock.Using
p.endsWith(f.path)could theoretically match unintended files if paths overlap (e.g.,"a.txt"would match both/fake/path/a.txtand/fake/path/ba.txt). This is fine for the current test data where paths are distinct, but could cause subtle issues if tests are extended.♻️ Optional: Use exact basename matching
vi.mocked(readFileSync).mockImplementation((filePath: unknown) => { const p = String(filePath); - const file = files.find((f) => p.endsWith(f.path)); + const file = files.find((f) => p.endsWith("/" + f.path) || p === f.path); return Buffer.from(file?.content ?? ""); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/verify.test.ts` around lines 49 - 53, The mock for readFileSync uses p.endsWith(f.path) which can produce false positives; update the implementation in the test (the vi.mocked(readFileSync) callback) to perform stricter matching by comparing the normalized basename or full normalized path instead of endsWith — e.g., derive the basename via path.basename(p) and match against f.path basename, or normalize both and use exact equality (compare path.normalize(p) === path.normalize(f.path)) so files.find(...) only returns the intended file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/blueprint/verify.test.ts`:
- Around line 49-53: The mock for readFileSync uses p.endsWith(f.path) which can
produce false positives; update the implementation in the test (the
vi.mocked(readFileSync) callback) to perform stricter matching by comparing the
normalized basename or full normalized path instead of endsWith — e.g., derive
the basename via path.basename(p) and match against f.path basename, or
normalize both and use exact equality (compare path.normalize(p) ===
path.normalize(f.path)) so files.find(...) only returns the intended file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ceed9ffa-0045-4b60-b71d-9d60cd5cd413
📒 Files selected for processing (1)
nemoclaw/src/blueprint/verify.test.ts
psorensen-nvidia
added
bug
|
Closing — PR #492 removed If blueprint integrity verification is reintroduced in the host CLI or elsewhere, the patterns here (format validation, |
3 participants
Summary
Hardens blueprint integrity verification to fail closed on empty, malformed, and whitespace-only digests. Adds format validation and timing-safe comparison.
Problem
verifyBlueprintDigest() uses a falsy guard on manifest.digest:
When parseManifestHeader returns "" for a missing digest: field (the regex match?.[1]?.trim() ?? "" produces empty string), the condition short-circuits to false. The function returns { valid: true } despite performing no integrity check.
Both cliLaunch (line 59) and cliMigrate (line 92) trust verification.valid and proceed with deployment. A blueprint without a digest field silently bypasses integrity verification.
This is a supply chain integrity issue: an attacker who can influence blueprint resolution (DNS poisoning, MITM, compromised registry) could substitute a malicious blueprint that executes without verification.
Changes
Relationship to PR #319
PR #319 (pjt222) fixes the core bypass (empty digest rejection) and adds comprehensive tests. This PR goes further with:
If #319 merges first, this PR can be rebased to layer the additional hardening on top. The two are complementary.
Testing
Christopher Lusk (christopherdlusk@gmail.com)
Summary by CodeRabbit
Tests
Bug Fixes