Skip to content

fix(scripts): make config integrity check fatal in non-root mode#1032

Closed
Junior00619 wants to merge 3 commits intoNVIDIA:mainfrom
Junior00619:fix/config-integrity-nonroot
Closed

fix(scripts): make config integrity check fatal in non-root mode#1032
Junior00619 wants to merge 3 commits intoNVIDIA:mainfrom
Junior00619:fix/config-integrity-nonroot

Conversation

@Junior00619
Copy link
Copy Markdown
Contributor

@Junior00619 Junior00619 commented Mar 27, 2026

Summary

Makes the config integrity check fatal in non-root mode. Previously, if
verify_config_integrity() failed in non-root mode, the script logged a
warning but continued to start the gateway with potentially tampered config.

Related Issue

Fixes #1013

Changes

  • Non-root path now exits with code 1 on integrity check failure (matches
    root path behavior)
  • Removed "proceeding anyway" bypass
  • Added regression tests verifying the non-root block exits on failure and
    no code path bypasses the integrity check

Type of Change

Code change for a new feature, bug fix, or refactor.

Testing

  • npm test — 479 passed
  • npx prek run --all-files — all hooks passed (including shellcheck)
  • New tests: config integrity check (2 assertions)

Summary by CodeRabbit

  • Bug Fixes

    • Startup now refuses to continue and terminates when configuration integrity checks fail in non-root mode, preventing partial startup and unintended initialization.
  • Tests

    • Added tests to verify configuration integrity enforcement and to ensure there is no bypass that would allow startup to proceed after integrity failures.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dd61fe75-cda5-4954-a192-4adf5e785099

📥 Commits

Reviewing files that changed from the base of the PR and between 72bef9c and 877f006.

📒 Files selected for processing (1)
  • scripts/nemoclaw-start.sh

📝 Walkthrough

Walkthrough

Non-root startup in scripts/nemoclaw-start.sh now treats verify_config_integrity failures as fatal: it logs a refusal and exits with status 1. A new Jest test suite (test/nemoclaw-start.test.js) validates the non-root branch contains the integrity check and exit behavior and no bypass message.

Changes

Cohort / File(s) Summary
Non-root Security Hardening
scripts/nemoclaw-start.sh
Non-root branch changed: on verify_config_integrity failure it logs a security refusal and terminates with exit 1 instead of warning and continuing.
Config Integrity Validation Tests
test/nemoclaw-start.test.js
Added tests that extract the non-root block to assert presence of verify_config_integrity, absence of the bypass phrase proceeding anyway, and presence of exit 1; also scans script for any ignored-failure patterns.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I hopped the lines and gave a stare,
No sneaky bypass hiding there.
When hashes fail, I stamp and shout,
"No start for you" — I stand about. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: making the config integrity check fatal in non-root mode, which is the primary objective of the PR.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from issue #1013: makes integrity check fatal in non-root mode, removes the bypass warning, ensures consistency between code paths, and adds regression tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #1013: the script fix changes the non-root path behavior, and the test additions verify the fix without introducing unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/nemoclaw-start.test.js (1)

25-27: Consider a more explicit comment on the regex matching strategy.

The regex correctly matches only the outer fi because inner fi statements are indented (e.g., fi) while the block-closing fi at column 0 matches \nfi\n. This is clever but relies on consistent indentation. A brief inline comment explaining this would help future maintainers understand why the regex works.

📝 Suggested comment addition
     // Extract the non-root block (between "id -u -ne 0" and the matching "fi")
+    // Note: The \nfi\n pattern matches only the unindented outer "fi" at column 0,
+    // skipping inner "fi" statements that are indented with leading whitespace.
     const nonRootMatch = src.match(
       /if \[ "\$\(id -u\)" -ne 0 \]; then\n([\s\S]*?)\nfi\n/
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/nemoclaw-start.test.js` around lines 25 - 27, Add an inline comment
above the regex assignment to nonRootMatch explaining the matching strategy:
note that the pattern /\nfi\n/ deliberately targets the closing "fi" at column 0
so it matches only the outer block (inner "fi" lines are indented like "  fi"),
and mention the caveat that this relies on consistent indentation in the source;
reference the nonRootMatch variable and the regex /if \[ "\$\(id -u\)" -ne 0 \];
then\n([\s\S]*?)\nfi\n/ so future maintainers understand why the expression
safely selects the outer block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/nemoclaw-start.test.js`:
- Around line 25-27: Add an inline comment above the regex assignment to
nonRootMatch explaining the matching strategy: note that the pattern /\nfi\n/
deliberately targets the closing "fi" at column 0 so it matches only the outer
block (inner "fi" lines are indented like "  fi"), and mention the caveat that
this relies on consistent indentation in the source; reference the nonRootMatch
variable and the regex /if \[ "\$\(id -u\)" -ne 0 \]; then\n([\s\S]*?)\nfi\n/ so
future maintainers understand why the expression safely selects the outer block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ef4136d0-a0ce-487c-8963-a27e4935c57d

📥 Commits

Reviewing files that changed from the base of the PR and between 6f9d530 and 9458309.

📒 Files selected for processing (2)
  • scripts/nemoclaw-start.sh
  • test/nemoclaw-start.test.js

In nemoclaw-start.sh, the non-root code path caught verify_config_integrity
failures and continued with a warning, bypassing the security model that
protects openclaw.json from tampering. The root code path correctly treated
the check as fatal (exits under set -euo pipefail).

The integrity check is now fatal in both code paths. If the config hash
doesn't match, the sandbox refuses to start regardless of whether it's
running as root or non-root.

Adds regression tests verifying:
- The non-root block exits on integrity failure
- No code path bypasses verify_config_integrity

Fixes NVIDIA#1013
@Junior00619 Junior00619 force-pushed the fix/config-integrity-nonroot branch from 9458309 to 18f12c6 Compare March 27, 2026 19:51
@kjw3
Copy link
Copy Markdown
Contributor

kjw3 commented Mar 30, 2026

Review note: I compared #1032 and #1071 directly against current main. Both fixed the same issue from #1013 by making the non-root config-integrity check fatal in scripts/nemoclaw-start.sh, and both added regression coverage in test/nemoclaw-start.test.js. I preferred #1032 on branch hygiene and test clarity, but #1071 has now been merged, so this PR is no longer needed. Closing as duplicate/superseded by merged PR #1071.

@kjw3
Copy link
Copy Markdown
Contributor

kjw3 commented Mar 30, 2026

Closing as duplicate/superseded by merged PR #1071, which resolves the same issue.

@kjw3 kjw3 closed this Mar 30, 2026
@wscurran wscurran added priority: high Important issue that should be resolved in the next release NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). labels Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). priority: high Important issue that should be resolved in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NemoClaw] nemoclaw-start.sh: config integrity check bypassed in non-root mode — script continues after SHA256 failure

3 participants