fix: require non-empty digest in blueprint verification; fail closed on missing or empty field#765
Open
thebenignhacker wants to merge 2 commits intoNVIDIA:mainfrom
Open
fix: require non-empty digest in blueprint verification; fail closed on missing or empty field#765thebenignhacker wants to merge 2 commits intoNVIDIA:mainfrom
thebenignhacker wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
…on missing or empty field The blueprint.yaml ships with digest: "" which means no integrity verification is performed. Previously there was no verification code at all — an empty digest was silently accepted. Changes: - Add bin/lib/verify-digest.js: SHA-256 verification of the blueprint directory that fails closed when digest is missing or empty - blueprint.yaml: document that digest must be populated before release - Add tests confirming fail-closed behavior on empty digest The verification module supports explicit --skip-verification opt-out for development workflows, but the default is fail-closed. Detection: npx hackmyagent secure . References: - PSIRT disclosure: tickets 6009892-6010011 - CWE-345: Insufficient Verification of Data Authenticity
📝 WalkthroughWalkthroughA new blueprint digest verification module is introduced that computes deterministic SHA-256 digests of blueprint directories and verifies integrity by comparing computed digests against expected values stored in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / build step"
participant Module as "verify-digest module"
participant FS as "Filesystem (`nemoclaw-blueprint`)"
participant Hasher as "SHA-256 hasher"
CLI->>Module: invoke verifyBlueprintDigest(opts)
Module->>FS: read file list (recursive, sorted)
FS-->>Module: file paths & contents
Module->>Module: zero `digest` in blueprint.yaml (in-memory)
Module->>Hasher: hash each relative path + contents
Hasher-->>Module: computedDigest
Module->>FS: read blueprint.yaml -> extract expectedDigest
Module-->>CLI: return { valid, reason?, expectedDigest, actualDigest }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/verify-digest.test.js (1)
32-42: Conditional test body weakens regression coverage.The
if (!expected || expected.trim() === "")guard means this test silently passes without executing any assertions if someone populates the digest field. For a regression test designed to catch the original CWE-345 bypass, consider making it unconditional by mocking or directly testing the fail-closed path:Option 1: Always test with an explicit empty digest
it("verifyBlueprintDigest fails closed on empty digest", () => { - const { verifyBlueprintDigest, readExpectedDigest } = require(verifyDigestPath); - const expected = readExpectedDigest(); - - // The shipped blueprint.yaml has digest: "" — verification must fail - if (!expected || expected.trim() === "") { - const result = verifyBlueprintDigest(); - expect(result.valid).toBe(false); - expect(result.reason).toMatch(/missing or empty/); - } + // Test the fail-closed behavior directly by providing an empty digest scenario. + // Since blueprint.yaml ships with digest: "", this test should always execute. + const { verifyBlueprintDigest, readExpectedDigest } = require(verifyDigestPath); + const expected = readExpectedDigest(); + + // Assert precondition: shipped blueprint.yaml has empty digest + expect(!expected || expected.trim() === "").toBe(true); + + const result = verifyBlueprintDigest(); + expect(result.valid).toBe(false); + expect(result.reason).toMatch(/missing or empty/); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/verify-digest.test.js` around lines 32 - 42, The test currently guards execution with if (!expected || expected.trim() === ""), which lets it skip assertions when a digest exists; change the test to unconditionally exercise the fail-closed path by forcing an empty digest: either stub/mock readExpectedDigest to return "" (so require(verifyDigestPath).readExpectedDigest returns "") or call verifyBlueprintDigest with an explicit empty/undefined digest to assert result.valid is false and result.reason matches /missing or empty/; update the test around verifyBlueprintDigest and readExpectedDigest to ensure the assertions always run.bin/lib/verify-digest.js (2)
24-24: Consider symlink handling for defense-in-depth.
fs.statSyncfollows symlinks, so a malicious symlink in the blueprint directory could cause the digest to include content from outside the directory. While the threat model may already assume directory integrity, usinglstatSync+ explicit symlink rejection would provide defense-in-depth for integrity verification.Optional: Reject symlinks explicitly
const fullPath = path.join(dirPath, entry); const relativePath = prefix ? `${prefix}/${entry}` : entry; - const stat = fs.statSync(fullPath); + const stat = fs.lstatSync(fullPath); + if (stat.isSymbolicLink()) { + throw new Error(`Symlinks not allowed in blueprint directory: ${relativePath}`); + } if (stat.isDirectory()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/verify-digest.js` at line 24, Replace the use of fs.statSync(fullPath) with fs.lstatSync(fullPath) in verify-digest.js and explicitly reject symlinks: call const stat = fs.lstatSync(fullPath) and if (stat.isSymbolicLink()) throw an error (or skip with a clear failure message) so the digest routine (where fullPath is inspected) cannot follow symlinks and include files outside the blueprint directory.
48-51: Regex assumes double-quoted digest value.The regex
/^digest:\s*"[^"]*"/monly matches double-quoted digest values. While the currentblueprint.yamluses this format, YAML also permits single quotes, unquoted values, or flow style. If the YAML format ever changes, the zeroing logic would fail silently (leaving the digest in the hash) and break verification.This is low-risk given you control the file, but consider adding a comment or a stricter check:
Optional: Add a warning or support alternate formats
// Zero the digest field in blueprint.yaml to avoid circular dependency + // NOTE: assumes digest is double-quoted; update regex if YAML style changes if (file === "blueprint.yaml") { content = Buffer.from( content.toString("utf-8").replace(/^digest:\s*"[^"]*"/m, 'digest: ""') ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/verify-digest.js` around lines 48 - 51, The current replacement in the blueprint handling (inside the if (file === "blueprint.yaml") block) uses a regex in content.toString(...).replace(...) that only matches a double-quoted digest value; update that replacement to match single-quoted, double-quoted, or unquoted digest values (e.g., expand the regex used in the replace call to accept ' or " or bare tokens) so the digest is reliably zeroed regardless of quoting, and optionally add a short comment or console.warn when a non-standard format is encountered to surface unexpected YAML styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/verify-digest.test.js`:
- Around line 26-30: The test's assertion assumes readExpectedDigest() always
returns a string but the implementation may return null; update the test in
verify-digest.test.js to accept either a string or null by asserting that
readExpectedDigest() returns null OR typeof result === "string" (referencing the
readExpectedDigest function used in the test) so the test passes when the digest
field is missing or present.
---
Nitpick comments:
In `@bin/lib/verify-digest.js`:
- Line 24: Replace the use of fs.statSync(fullPath) with fs.lstatSync(fullPath)
in verify-digest.js and explicitly reject symlinks: call const stat =
fs.lstatSync(fullPath) and if (stat.isSymbolicLink()) throw an error (or skip
with a clear failure message) so the digest routine (where fullPath is
inspected) cannot follow symlinks and include files outside the blueprint
directory.
- Around line 48-51: The current replacement in the blueprint handling (inside
the if (file === "blueprint.yaml") block) uses a regex in
content.toString(...).replace(...) that only matches a double-quoted digest
value; update that replacement to match single-quoted, double-quoted, or
unquoted digest values (e.g., expand the regex used in the replace call to
accept ' or " or bare tokens) so the digest is reliably zeroed regardless of
quoting, and optionally add a short comment or console.warn when a non-standard
format is encountered to surface unexpected YAML styles.
In `@test/verify-digest.test.js`:
- Around line 32-42: The test currently guards execution with if (!expected ||
expected.trim() === ""), which lets it skip assertions when a digest exists;
change the test to unconditionally exercise the fail-closed path by forcing an
empty digest: either stub/mock readExpectedDigest to return "" (so
require(verifyDigestPath).readExpectedDigest returns "") or call
verifyBlueprintDigest with an explicit empty/undefined digest to assert
result.valid is false and result.reason matches /missing or empty/; update the
test around verifyBlueprintDigest and readExpectedDigest to ensure the
assertions always run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adbc404d-8eeb-488f-b5c3-d6d13119d34d
📒 Files selected for processing (3)
bin/lib/verify-digest.jsnemoclaw-blueprint/blueprint.yamltest/verify-digest.test.js
readExpectedDigest() returns null when the digest field is absent from blueprint.yaml. Update assertion to accept both string and null. Addresses CodeRabbit review feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/verify-digest.test.js (1)
32-42: Conditional test becomes a no-op when digest is populated.This test only runs assertions when the digest field is empty or missing. Once
blueprint.yamlis updated with a valid digest before release, theifbranch is skipped and the test passes vacuously without asserting anything.Consider mocking
readExpectedDigestto return an empty string explicitly, or create a test fixture with a known empty digest to ensure this fail-closed behavior is always tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/verify-digest.test.js` around lines 32 - 42, The test currently only asserts when the real readExpectedDigest() returns empty, making it a no-op if blueprint.yaml has a digest; change the test to stub/mock readExpectedDigest to return an empty string deterministically so verifyBlueprintDigest() is exercised: import or require verifyBlueprintDigest and mock readExpectedDigest (or inject a fixture) to return "" before calling verifyBlueprintDigest(), then assert result.valid is false and result.reason matches /missing or empty/ to ensure the fail-closed behavior is always tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/verify-digest.test.js`:
- Around line 51-58: The test depends on the real blueprint.yaml and will break
if that file contains a valid digest; change the test to explicitly control the
expected-digest source by stubbing/mocking readExpectedDigest (or injecting a
mock) so it returns an empty string for this regression scenario. Locate the
call site where verifyBlueprintDigest is required (verifyBlueprintDigest in
test/verify-digest.test.js using verifyDigestPath) and replace the module
dependency with a mock that makes readExpectedDigest return "" (or provide a
mocked function via dependency injection) before calling verifyBlueprintDigest,
then assert result.valid === false.
---
Nitpick comments:
In `@test/verify-digest.test.js`:
- Around line 32-42: The test currently only asserts when the real
readExpectedDigest() returns empty, making it a no-op if blueprint.yaml has a
digest; change the test to stub/mock readExpectedDigest to return an empty
string deterministically so verifyBlueprintDigest() is exercised: import or
require verifyBlueprintDigest and mock readExpectedDigest (or inject a fixture)
to return "" before calling verifyBlueprintDigest(), then assert result.valid is
false and result.reason matches /missing or empty/ to ensure the fail-closed
behavior is always tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9342e6c9-cb85-48db-869d-7fe97b169887
📒 Files selected for processing (1)
test/verify-digest.test.js
Comment on lines
+51
to
+58
| it("empty string digest must not pass verification", () => { | ||
| // Regression: if (digest && digest !== actual) was the original bug — | ||
| // empty string is falsy in JS, causing silent bypass | ||
| const { verifyBlueprintDigest } = require(verifyDigestPath); | ||
| const result = verifyBlueprintDigest(); | ||
| // With digest: "" in blueprint.yaml, this MUST fail | ||
| expect(result.valid).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Test will fail once blueprint.yaml contains a valid digest.
This test unconditionally expects valid: false, but when the digest field is populated with the correct SHA-256 value before release, verifyBlueprintDigest() will return valid: true and this test will break.
For a regression test specifically targeting the empty-string bypass (CWE-345), consider mocking or stubbing readExpectedDigest to return "" explicitly, decoupling the test from the actual file state.
Proposed fix using dependency injection or mocking
it("empty string digest must not pass verification", () => {
// Regression: if (digest && digest !== actual) was the original bug —
// empty string is falsy in JS, causing silent bypass
- const { verifyBlueprintDigest } = require(verifyDigestPath);
- const result = verifyBlueprintDigest();
- // With digest: "" in blueprint.yaml, this MUST fail
- expect(result.valid).toBe(false);
+ const verifyDigest = require(verifyDigestPath);
+ // Mock readExpectedDigest to simulate the empty-digest regression scenario
+ const originalRead = verifyDigest.readExpectedDigest;
+ verifyDigest.readExpectedDigest = () => "";
+ try {
+ const result = verifyDigest.verifyBlueprintDigest();
+ // Empty string digest MUST fail (fail-closed)
+ expect(result.valid).toBe(false);
+ expect(result.reason).toMatch(/missing or empty/);
+ } finally {
+ verifyDigest.readExpectedDigest = originalRead;
+ }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/verify-digest.test.js` around lines 51 - 58, The test depends on the
real blueprint.yaml and will break if that file contains a valid digest; change
the test to explicitly control the expected-digest source by stubbing/mocking
readExpectedDigest (or injecting a mock) so it returns an empty string for this
regression scenario. Locate the call site where verifyBlueprintDigest is
required (verifyBlueprintDigest in test/verify-digest.test.js using
verifyDigestPath) and replace the module dependency with a mock that makes
readExpectedDigest return "" (or provide a mocked function via dependency
injection) before calling verifyBlueprintDigest, then assert result.valid ===
false.
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
1 participant
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
blueprint.yamlships withdigest: ""and there is no verification codein the codebase. An empty digest means no integrity check is ever performed
on the blueprint directory — any modification to policies, sandbox configs,
or orchestration scripts goes undetected.
If verification code were added with the common pattern
if (digest && digest !== actual), an empty string would evaluate as falsy,silently bypassing the check.
Change
bin/lib/verify-digest.js: SHA-256 verification of the blueprintdirectory that fails closed when digest is missing or empty
blueprint.yaml: document that digest must be populated before release--skip-verificationopt-out for development workflowsDetection
This vulnerability class is detectable via HackMyAgent:
References
Summary by CodeRabbit
New Features
Documentation
Tests