Skip to content

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

Open
Junior00619 wants to merge 2 commits intoNVIDIA:mainfrom
Junior00619:fix/config-integrity-nonroot
Open

fix(scripts): make config integrity check fatal in non-root mode#1032
Junior00619 wants to merge 2 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 (non-root execution), preventing partial startup.
  • Tests

    • Added tests to verify configuration integrity enforcement and to ensure there is no bypass that allows 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: 4e45777c-a119-4d6c-82c0-17983e4c95d2

📥 Commits

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

📒 Files selected for processing (2)
  • scripts/nemoclaw-start.sh
  • test/nemoclaw-start.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/nemoclaw-start.sh
  • test/nemoclaw-start.test.js

📝 Walkthrough

Walkthrough

Non-root startup path in scripts/nemoclaw-start.sh now refuses to start and exits with exit 1 when verify_config_integrity fails. A new Jest test suite (test/nemoclaw-start.test.js) was added to assert this stricter non-root behavior and absence of the previous bypass message.

Changes

Cohort / File(s) Summary
Non-root Security Hardening
scripts/nemoclaw-start.sh
Non-root branch updated: 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
New tests added to assert non-root block calls verify_config_integrity, does not contain the bypass phrase proceeding anyway, and exits on integrity failure; includes a scan ensuring no ignored-failure code paths remain.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I sniffed the script, I gave a twitch,
No sneaky bypass, no naughty glitch.
When hashes fail, I thump my paw—
"Refuse to start!" is now the law. 🥕

🚥 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 describes the main change: making config integrity check fatal in non-root mode, which directly addresses the primary objective of the PR.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from issue #1013: integrity check is now fatal in non-root mode (exit 1), proceeding anyway bypass is removed, behavior is consistent with root path, and regression tests confirm the fix.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the config integrity check in non-root mode and adding regression tests; no unrelated modifications are present.
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

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

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

1 participant