fix(security): add trusted root npm audit - #8131
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
sandl99
added
the
area: security
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe workflow validates production lock reachability, materializes and audits distinct target source graphs, verifies signatures and content hashes, records provenance, and applies shared report-threshold checks. It also validates a bounded audit exception. ChangesSource Graph Audit
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewedNpmWorkflow
participant reviewed-npm-archive
participant materializeSourceGraph
participant auditMaterializedSourceGraph
participant SignatureVerifier
participant ReportValidator
ReviewedNpmWorkflow->>reviewed-npm-archive: validate production lock packages
reviewed-npm-archive-->>ReviewedNpmWorkflow: verified package specifications
ReviewedNpmWorkflow->>materializeSourceGraph: materialize target source graph
materializeSourceGraph-->>ReviewedNpmWorkflow: installed and hash-verified graph
ReviewedNpmWorkflow->>auditMaterializedSourceGraph: audit materialized graph
auditMaterializedSourceGraph->>SignatureVerifier: verify audit signature
SignatureVerifier-->>auditMaterializedSourceGraph: signature result
auditMaterializedSourceGraph-->>ReviewedNpmWorkflow: source-graph audit report
ReviewedNpmWorkflow->>ReportValidator: validate all reports against threshold
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 370fd36 in the TypeScript / code-coverage/cliThe overall coverage in commit 370fd36 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/audit-reviewed-npm-graph.mts (1)
241-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an options object for
materializeSourceGraph.The function takes four positional string paths plus an injected installer. Callers cannot distinguish
sourcePackage,sourceLock, anddestinationat the call site without checking the declaration. The sibling helperauditMaterializedSourceGraphalready uses an options object plus a separatedependenciesargument. Aligning both keeps one shape for the new exported surface.♻️ Proposed signature change
export function materializeSourceGraph( - sourcePackage: string, - sourceLock: string, - destination: string, - registryOrigin: string, - installProductionDependencies: (directory: string) => void = (directory) => - void run("npm", ["ci", "--ignore-scripts", "--omit=dev", "--no-audit", "--no-fund"], directory), + options: Readonly<{ + destination: string; + registryOrigin: string; + sourceLock: string; + sourcePackage: string; + }>, + dependencies: Readonly<{ + installProductionDependencies?: (directory: string) => void; + }> = {}, ): string {🤖 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 `@scripts/audit-reviewed-npm-graph.mts` around lines 241 - 248, Update the exported materializeSourceGraph function to accept an options object containing sourcePackage, sourceLock, destination, registryOrigin, and the injectable installProductionDependencies value, rather than multiple positional arguments. Align its parameter shape with auditMaterializedSourceGraph while preserving the existing defaults and behavior, and update all call sites accordingly.scripts/lib/reviewed-npm-archive.mts (1)
349-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the positional boolean flags with a named options argument.
readReviewedLockPackagesnow takes two trailing booleans. The call site readsrequest.omitDev, true, which does not state which guard thetruedisables. An options object states the intent and prevents a future argument-order mistake.♻️ Proposed refactor
function readReviewedLockPackages( packages: Readonly<Record<string, Record<string, unknown>>>, lockfilePath: string, registryOrigin: string, - omitDev = false, - allowEmpty = false, + options: Readonly<{ allowEmpty?: boolean; omitDev?: boolean }> = {}, ): readonly ReviewedNpmArchiveRequest[] {return readReviewedLockPackages( readReviewedLock(request.lockfilePath), request.lockfilePath, registryOrigin, - request.omitDev, - true, + { allowEmpty: true, omitDev: request.omitDev }, ).map(({ packageSpec }) => packageSpec);Update the internal references to
omitDevandallowEmptyinside the reader, and the existing call at Line 413.🤖 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 `@scripts/lib/reviewed-npm-archive.mts` around lines 349 - 360, Replace the positional boolean arguments passed to readReviewedLockPackages with a named options object, explicitly identifying omitDev and the true value’s allowEmpty meaning. Update readReviewedLockPackages to read omitDev and allowEmpty from that options object, and apply the same change to the existing call near the other reference at line 413.
🤖 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 `@scripts/audit-reviewed-npm-graph.mts`:
- Around line 449-454: Update shouldAuditTargetSourceGraph to normalize both
trustedRepoRoot and targetRepoRoot with fs.realpathSync before comparing them,
replacing the asymmetric path.resolve usage while preserving the boolean
same-tree exemption behavior.
In `@test/reviewed-npm-audit-workflow.test.ts`:
- Around line 187-215: Update the test around materializeSourceGraph to pass a
no-op installer stub, preventing a real npm ci subprocess and preserving the
lockfile assertion. Reuse the existing writeProductionSourceGraph fixture helper
used by the sibling tests instead of duplicating the manifest and lockfile
setup.
---
Nitpick comments:
In `@scripts/audit-reviewed-npm-graph.mts`:
- Around line 241-248: Update the exported materializeSourceGraph function to
accept an options object containing sourcePackage, sourceLock, destination,
registryOrigin, and the injectable installProductionDependencies value, rather
than multiple positional arguments. Align its parameter shape with
auditMaterializedSourceGraph while preserving the existing defaults and
behavior, and update all call sites accordingly.
In `@scripts/lib/reviewed-npm-archive.mts`:
- Around line 349-360: Replace the positional boolean arguments passed to
readReviewedLockPackages with a named options object, explicitly identifying
omitDev and the true value’s allowEmpty meaning. Update readReviewedLockPackages
to read omitDev and allowEmpty from that options object, and apply the same
change to the existing call near the other reference at line 413.
🪄 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: 494db941-de46-4529-9c45-4baf07f25874
📒 Files selected for processing (3)
scripts/audit-reviewed-npm-graph.mtsscripts/lib/reviewed-npm-archive.mtstest/reviewed-npm-audit-workflow.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Sensitive-path security review: FAIL Reviewed PR #8131 at commit SHA Do not merge this commit. A high-severity validation gap allows an untrusted lockfile
Required remediation:
Security categories:
All required checks currently pass for this commit, but passing checks do not resolve this security finding. A new independent documentation and security review is required after remediation. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Reject production-reachable lock records marked dev before npm runs. Preserve non-dev record review and add pre-install regression coverage. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/lib/reviewed-npm-archive.mts (1)
383-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated production-reachable dev check.
The same rule is now written twice: derive
productionLocationswhenomitDevis set, then rejectdev: trueon a reachable location with an identical error message. Both sites are correct today. If the rule changes, the two sites can drift, and a drift in the post-install site would silently weaken the guarantee the security review requires.
scripts/lib/reviewed-npm-archive.mts#L383-L385: replace the inline check with a shared helper, for exampleassertNotProductionDev(productionLocations, location, record).scripts/lib/reviewed-npm-archive.mts#L520-L522: call the same helper instead of repeating the condition and the error string.🤖 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 `@scripts/lib/reviewed-npm-archive.mts` around lines 383 - 385, Extract the duplicated production-reachable dev check into a shared helper, such as assertNotProductionDev(productionLocations, location, record), preserving the existing condition and error message. Update scripts/lib/reviewed-npm-archive.mts lines 383-385 and 520-522 to call the helper instead of containing the inline check.
🤖 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 `@scripts/lib/reviewed-npm-archive.mts`:
- Around line 272-292: Update the dependency aggregation loop in the reviewed
npm lock parsing function so duplicate names retain the strictest
classification: once a name is recorded with optional false from dependencies,
optionalDependencies must not overwrite it with true. Preserve optional true for
names appearing only in optionalDependencies and return the existing dependency
list format.
- Around line 518-523: Guard each `record` in the `packages` loop before
accessing `record.dev`, using the same record-validation guard and descriptive
error behavior as `readReviewedLockPackages`. Preserve the existing
production-dependency check and `omitDev` filtering for valid records.
- Around line 272-275: Update the dependency traversal loop around the
dependencies and optionalDependencies fields to also traverse production
peerDependencies. Use peerDependenciesMeta to mark peer entries as optional when
applicable, while preserving the existing handling for required and optional
dependency fields so peer-only packages are included in the audit.
---
Nitpick comments:
In `@scripts/lib/reviewed-npm-archive.mts`:
- Around line 383-385: Extract the duplicated production-reachable dev check
into a shared helper, such as assertNotProductionDev(productionLocations,
location, record), preserving the existing condition and error message. Update
scripts/lib/reviewed-npm-archive.mts lines 383-385 and 520-522 to call the
helper instead of containing the inline check.
🪄 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: 118a2c71-014d-44e6-904f-1bd9f0d725a4
📒 Files selected for processing (2)
scripts/lib/reviewed-npm-archive.mtstest/reviewed-npm-audit-workflow.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
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 `@test/reviewed-npm-audit-workflow.test.ts`:
- Around line 340-370: Update the reviewed npm audit tests around
materializeSourceGraph so a package present in both dependencies and
optionalDependencies is treated as optional, not required. Remove shared-package
from optionalDependencies in the existing required-dependency test, then add a
passing test covering the duplicate-map case and preserving the expected
materialization behavior.
🪄 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: 37229abe-a98e-49c5-8989-b0508213920d
📒 Files selected for processing (3)
scripts/audit-reviewed-npm-graph.mtsscripts/lib/reviewed-npm-archive.mtstest/reviewed-npm-audit-workflow.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/audit-reviewed-npm-graph.mts
- scripts/lib/reviewed-npm-archive.mts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/npm-audit-exceptions.json (1)
11-14: 🔒 Security & Privacy | 🔵 TrivialConfirm the expiry boundary and remediation handoff.
expiresis2026-08-10, seven days after the current review date, August 3, 2026. The rationale depends on PR#8126, which is still open as of August 3, 2026. (github.com) Confirm whetherparseAuditExceptionRegistrytreats this date as inclusive and ensure the upgrade lands before the cutoff. Otherwise, this high-severity exception may expire before remediation and cause an audit failure.Based on the PR objective, this is a temporary risk acceptance that must be removed with the dependency upgrade.
🤖 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 `@ci/npm-audit-exceptions.json` around lines 11 - 14, Verify that parseAuditExceptionRegistry treats the expires date as inclusive, and ensure the dependency upgrade referenced by trackingIssue PR `#8126` lands before 2026-08-10. Remove this temporary exception and its rationale once the upgrade is applied; otherwise adjust the expiry only through the established exception policy.Source: MCP tools
🤖 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 `@test/reviewed-npm-audit.test.ts`:
- Line 122: Update the test title in the test case beginning “bounds the
OpenClaw brace-expansion exception” to remove the “(`#8126`)” suffix unless a
valid local issue reference is available; do not use the pull request number as
the title suffix.
---
Nitpick comments:
In `@ci/npm-audit-exceptions.json`:
- Around line 11-14: Verify that parseAuditExceptionRegistry treats the expires
date as inclusive, and ensure the dependency upgrade referenced by trackingIssue
PR `#8126` lands before 2026-08-10. Remove this temporary exception and its
rationale once the upgrade is applied; otherwise adjust the expiry only through
the established exception policy.
🪄 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: c31eb959-a84e-4dfc-a601-c2c277572ab5
📒 Files selected for processing (5)
ci/npm-audit-exceptions.jsonscripts/audit-reviewed-npm-graph.mtsscripts/lib/reviewed-npm-archive.mtstest/reviewed-npm-audit-workflow.test.tstest/reviewed-npm-audit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/audit-reviewed-npm-graph.mts
- scripts/lib/reviewed-npm-archive.mts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Approved at exact head bf102dc. Sensitive-path security review: PASS with no findings across (1) secrets and credentials, (2) input validation and sanitization, (3) authentication and authorization, (4) dependencies and third-party libraries, (5) error handling and logging, (6) cryptography and data protection, (7) configuration and security headers, (8) security testing, and (9) system security. Untrusted lock and package records fail closed; install scripts remain disabled; registry, digest, package-name, version, and signature controls are pinned and checked before/after materialization; negative tests cover peer reachability, optional overrides, dev-flag misuse, nested shrinkwraps, malformed records, source drift, identity mismatches, and advisory blocking. CodeRabbit is green with all threads resolved; the exact-head PR Advisor reports merge-as-is; all ordinary code lanes, eight CLI shards, macOS E2E, WSL E2E, DCO, commit verification, and the exact-head documentation writer review pass. The sole expected failure is reviewed-npm-audit: the trusted base intentionally cannot accept this head-created, advisory/package/version/graph/owner/tracking/expiry-bounded bootstrap exception. PR #8126 removes the exception by upgrading brace-expansion to 5.0.9.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security re-review for exact head 1662208e678ebc9fe72c518b6bb1908898ab4280: changes requested.
The branch now has the required audit-only scope, the prior production-reachable dev: true gap is fixed, and the focused reviewed-audit tests pass 55/55. One security blocker remains.
Blocking finding — stale same-tree deferral bypasses the root audit
scripts/audit-reviewed-npm-graph.mts:62-67 says to remove the two deferred digests after the root lock remediation. That remediation landed in #8156. The current main manifest and lock no longer match those digests, but shouldAuditTargetSourceGraph still returns false for the exact old pair at lines 505-515.
A same-tree main run whose package.json and package-lock.json return to those pre-remediation vulnerable identities therefore skips the new nemoclaw-cli audit entirely. PR runs use a separate trusted checkout and still audit, but the main-branch security control remains bypassable for the one graph this temporary exception was created to defer.
Remove the deferred manifest and lock constants, shouldDeferSameTreeSourceGraph, and their positive deferral test. The source graph should now run for same-tree and separate-target executions.
Required cleanup before approval
- Remove the no-op indentation-only change in
ci/reviewed-npm-audit.json. - Refresh the PR body. It still says the PR is not merge-eligible and describes the obsolete same-tree deferral and temporary advisory exception.
- Repeat the exact-head security and documentation reviews and refresh the receipt metadata.
- Let all required checks finish successfully; ten checks are currently pending.
No other security finding remains in the five-file audit-only diff.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
4 participants
Summary
The trusted npm audit now covers the NemoClaw CLI production graph in addition to reviewed archives and pinned runtime graphs. It validates the target lock before installation, verifies installed identities, checks registry signatures, and applies the existing advisory policy without a same-tree exemption.
Related Issue
Related to #8116.
Changes
npm ci, including registry origin, nested shrinkwrap rejection, production reachability, peer dependencies, optional overrides, anddev: truemisuse.Type of Change
Quality Gates
370fd3679584049f87956c2139a9b7c02fede4c9passed all nine categories with no findings.Documentation Writer Review
no-docs-needed370fd3679584049f87956c2139a9b7c02fede4c9against current mainabae71044a3038f7094944be5ae25d8d37b25155. It changes internal npm audit enforcement and regression tests only. No user-visible CLI, configuration, workflow, default, supported behavior, or documentation changes. Changed comments, errors, and test titles follow the writing guide and controlled vocabulary. GitHub CI provides validation for this commit.Security Review
PASS370fd3679584049f87956c2139a9b7c02fede4c9abae71044a3038f7094944be5ae25d8d37b25155PASS— no credentials or secret material are added.PASS— lock structure, dependency names, registry origin, paths, integrity, and installed identities are validated.PASS— no authentication or authorization surface changes.PASS— no dependency is added; production packages remain integrity-pinned and signature-checked.PASS— failures stop the audit with specific errors and do not expose credentials.PASS— lock identities use SHA-256 and npm integrity values use SHA-512.PASS— lifecycle scripts stay disabled during installation and nested shrinkwrap delegation is rejected.PASS— denial paths cover registry drift, dependency reachability, development mislabeling, shrinkwrap, identity mismatch, signatures, and blocking advisories.PASS— the former deferred digest constants and both same-tree deferral functions are removed; the root production graph is audited unconditionally.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — verification is pending after commit370fd3679584049f87956c2139a9b7c02fede4c9is pushed.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — commit hooks passed; pre-push is pending.npm run docsbuilds without warnings (doc changes only) — Not applicable; no documentation file changed.GitHub CI is authoritative.
No duplicate local test, documentation, build, or typecheck suite ran after the merge refresh.
Signed-off-by: San Dang sdang@nvidia.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com