fix(blueprint): reject empty digest instead of silently passing verification - #319
fix(blueprint): reject empty digest instead of silently passing verification#319pjt222 wants to merge 2 commits into
Conversation
…ication When a blueprint manifest has an empty or missing digest field, verifyBlueprintDigest() returned valid: true because "" is falsy in JavaScript, causing the digest comparison to be skipped entirely. Both cliLaunch and cliMigrate trust the valid flag and proceed with deployment, so a blueprint without a digest silently bypasses integrity verification. The fix explicitly checks for empty/missing digest and returns an error. Comprehensive tests are added for verifyBlueprintDigest (digest match, mismatch, empty, undefined, multi-file) and checkCompatibility (version satisfied, too old, missing minimum, edge cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors verification logic to treat a missing manifest digest as an explicit error and only compute the directory digest when a manifest digest is present. Adds a comprehensive Vitest test suite validating digest computation, mismatch reporting, nested files, and version compatibility edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nemoclaw/src/blueprint/verify.ts (1)
21-25: Skip hashing when the manifest is already invalid.This branch still walks and hashes the blueprint before reporting the missing-digest error. Moving the hash behind the
manifest.digestguard avoids unnecessary I/O and keeps malformed manifests on the intended failure path.♻️ Proposed adjustment
export function verifyBlueprintDigest( blueprintPath: string, manifest: BlueprintManifest, ): VerificationResult { const errors: string[] = []; - const actualDigest = computeDirectoryDigest(blueprintPath); + let actualDigest = ""; if (!manifest.digest) { errors.push("Blueprint manifest is missing a digest — cannot verify integrity"); - } else if (manifest.digest !== actualDigest) { - errors.push(`Digest mismatch: expected ${manifest.digest}, got ${actualDigest}`); + } else { + actualDigest = computeDirectoryDigest(blueprintPath); + if (manifest.digest !== actualDigest) { + errors.push(`Digest mismatch: expected ${manifest.digest}, got ${actualDigest}`); + } }🤖 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 21 - 25, computeDirectoryDigest(blueprintPath) is called before checking manifest.digest, causing unnecessary hashing even when the manifest is invalid; change the flow so you first check if manifest.digest exists and push the error (errors.push("Blueprint manifest is missing a digest — cannot verify integrity")) and only call computeDirectoryDigest(blueprintPath) when manifest.digest is present, then compare manifest.digest to the computed actualDigest and push the integrity error if they differ.nemoclaw/src/blueprint/verify.test.ts (2)
41-54: Add one nested-directory digest fixture.
mockDirectory()always reports files, so the "multi-file directory" case still only proves flat-file ordering. A nested fixture would exercisecollectFiles()recursion andprefixhashing, which are part of the digest contract.Also applies to: 160-177
🤖 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 41 - 54, The test helper mockDirectory() only fakes a flat listing so it never exercises collectFiles() recursion or prefix hashing; update the tests by adding a nested-directory fixture and enhancing mockDirectory() behavior to simulate directories: make vi.mocked(readdirSync) return directory entries for nested folders (not just file paths), make vi.mocked(statSync) return isDirectory() true for directory paths and false for files, and keep readFileSync returning Buffer content for leaf file paths; then add a test case that supplies a nested file (e.g., "dir/sub.txt") and the expected digest to validate recursive collection and prefix hashing.
115-136: Assert the exact missing-digest error here.Both tests only look for
"missing", so an unrelated failure reason could still satisfy them. Since this PR is pinning a specific guard, it's worth locking the full message down.♻️ Tighten the expectation
expect(result.valid).toBe(false); - expect(result.errors).toHaveLength(1); - expect(result.errors[0]).toContain("missing"); + expect(result.errors).toEqual([ + "Blueprint manifest is missing a digest — cannot verify integrity", + ]);Apply the same assertion shape to the
undefinedcase as well.🤖 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 115 - 136, The tests currently only assert that the error string contains "missing", which is too loose; update both cases in verify.test.ts so they assert the exact missing-digest message returned by verifyBlueprintDigest (replace expect(result.errors[0]).toContain("missing") with an exact equality check, e.g. expect(result.errors[0]).toBe("manifest digest is missing") for both the empty-string and undefined manifest cases), and keep the other expectations the same so the test fails if any other error reason is returned.
🤖 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 41-54: The test helper mockDirectory() only fakes a flat listing
so it never exercises collectFiles() recursion or prefix hashing; update the
tests by adding a nested-directory fixture and enhancing mockDirectory()
behavior to simulate directories: make vi.mocked(readdirSync) return directory
entries for nested folders (not just file paths), make vi.mocked(statSync)
return isDirectory() true for directory paths and false for files, and keep
readFileSync returning Buffer content for leaf file paths; then add a test case
that supplies a nested file (e.g., "dir/sub.txt") and the expected digest to
validate recursive collection and prefix hashing.
- Around line 115-136: The tests currently only assert that the error string
contains "missing", which is too loose; update both cases in verify.test.ts so
they assert the exact missing-digest message returned by verifyBlueprintDigest
(replace expect(result.errors[0]).toContain("missing") with an exact equality
check, e.g. expect(result.errors[0]).toBe("manifest digest is missing") for both
the empty-string and undefined manifest cases), and keep the other expectations
the same so the test fails if any other error reason is returned.
In `@nemoclaw/src/blueprint/verify.ts`:
- Around line 21-25: computeDirectoryDigest(blueprintPath) is called before
checking manifest.digest, causing unnecessary hashing even when the manifest is
invalid; change the flow so you first check if manifest.digest exists and push
the error (errors.push("Blueprint manifest is missing a digest — cannot verify
integrity")) and only call computeDirectoryDigest(blueprintPath) when
manifest.digest is present, then compare manifest.digest to the computed
actualDigest and push the integrity error if they differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d904c9-06cb-4d9f-9f5b-c06a6c87abdf
📒 Files selected for processing (2)
nemoclaw/src/blueprint/verify.test.tsnemoclaw/src/blueprint/verify.ts
Address CodeRabbit review feedback:
- Move computeDirectoryDigest() inside the else branch so malformed
manifests (missing digest) skip unnecessary filesystem I/O
- Replace loose toContain("missing") assertions with exact toEqual()
matching the full error message
- Add test verifying no I/O occurs when digest is missing
- Add nested directory test exercising collectFiles() recursion and
prefix path hashing
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
north-echo
left a comment
north-echo
left a comment
There was a problem hiding this comment.
Security Review, PR #319
The core fix is correct and well-tested. Replacing the falsy guard with an explicit empty-check closes the bypass. The decision to skip hashing when the digest is missing (avoiding unnecessary I/O) is also the right call. The test coverage is thorough for the cases it covers. A few observations on remaining attack surface:
- Whitespace-only digest strings
The guard !manifest.digest is falsy for empty string and undefined, but a whitespace-only string like " " is truthy in JavaScript. parseManifestHeader uses match?.[1]?.trim() ?? "", so a YAML line like digest: (with trailing spaces but no value) would still produce "". But a crafted manifest with digest: " " (quoted whitespace) would pass the guard, trigger a hash computation, and produce a generic "Digest mismatch" error instead of the more informative "missing a digest" message.
Not a bypass (the mismatch would still reject it), but the error message is misleading. A trim() check would catch this:
if (!manifest.digest || !manifest.digest.trim()) {
- No digest format validation
Any non-empty string is accepted as a valid expected digest. A malformed value like "sha256:" (prefix only), "abc", or a truncated hex string would pass the empty-check and produce a generic mismatch error. Validating the format (e.g., /^[a-f0-9]{64}$/ for a bare SHA-256 hex string) would catch malformed manifests earlier with a specific error.
- Timing-safe comparison
The comparison uses !== which is subject to timing side-channel analysis. For digest verification this is lower risk than for auth tokens (the attacker would need to observe many verification attempts with controlled digests), but the standard practice is crypto.timingSafeEqual. Worth considering, especially since the fix is a one-line change:
const match = Buffer.from(manifest.digest).length === Buffer.from(actualDigest).length
&& crypto.timingSafeEqual(Buffer.from(manifest.digest), Buffer.from(actualDigest));
- Failure mode in callers
Both cliLaunch (line 60-62) and cliMigrate (line 93-95) handle verification failure with logger.error() + return. This means the process may exit with code 0 even when verification fails, depending on how the CLI runner handles the return. A process.exit(1) or thrown error would make the failure more visible in CI/CD pipelines and scripts.
- The PR note about checkCompatibility semantics is valuable
The observation that the falsy-guard pattern is correct for minOpenShellVersion/minOpenClawVersion (missing means "no requirement") but incorrect for digest (missing means "cannot verify") is a good architectural note. Worth preserving in a code comment for future contributors.
Overall this is a clean, well-scoped fix. The test suite is the strongest part. I would be comfortable seeing this merged with or without the above suggestions.
Christopher Lusk (christopherdlusk@gmail.com)
…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>
|
Closing this PR — the file it patches ( |
3 participants
Summary
verify.ts(20 tests covering digest verification and version compatibility)Problem
verifyBlueprintDigest()uses a falsy guard onmanifest.digest:When
parseManifestHeaderreturns""for a missingdigest:field (the regexmatch?.[1]?.trim() ?? ""produces empty string), the condition short-circuits tofalse. The function returns{ valid: true }despite performing no integrity check.Both
cliLaunch(line 59) andcliMigrate(line 92) trustverification.validand proceed with deployment. A blueprint without a digest field silently bypasses integrity verification.Fix
Replace the falsy guard with an explicit empty-check:
Note: The same falsy-guard pattern in
checkCompatibilityforminOpenShellVersion/minOpenClawVersionis semantically correct — a missing minimum version means "no requirement." The digest field has opposite semantics: missing means "cannot verify," not "verification unnecessary."Tests added
verifyBlueprintDigest— happy pathverifyBlueprintDigest— mismatchverifyBlueprintDigest— empty digest (bug fix)verifyBlueprintDigest— result fieldsverifyBlueprintDigest— multi-filecheckCompatibility— satisfiedcheckCompatibility— too oldcheckCompatibility— missing minimumcheckCompatibility— edge casesTest plan
npx vitest run— 42 total, 0 failures)status.test.tstests unaffectedverify.ts)status.test.ts🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests