fix(security): prevent prototype pollution via unsanitized config path [MEDIUM]#1558
Open
Joshua-Medvinsky wants to merge 1 commit intoNVIDIA:mainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a module-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
wscurran
added
security
Contributor
|
✨ Thanks for submitting this fix, which proposes a way to prevent prototype pollution by validating config path tokens during snapshot restoration. |
cv
requested changes
Apr 8, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security Review — WARNING
The code change is sound — the denylist approach is correct, validation runs upfront before any traversal (no partial mutation), and false positive risk is negligible.
Required change
Add regression tests. A security fix without tests can silently regress. At minimum:
- A
configPathcontaining__proto__throws"Unsafe config path segment" - A
configPathcontainingconstructorthrows - Legitimate paths like
agents.list[0].workspacestill work
setConfigValue is private, so test via prepareSandboxState or export it for direct testing.
What's good
- Threat is real — crafted snapshot manifest can pollute
Object.prototypevia bracket-access traversal - Fix is at the right layer (input validation in
setConfigValuebefore traversal) - All tokens validated upfront in a separate loop — no partial mutation on error
- Error message includes token + path for debugging without leaking sensitive info
DANGEROUS_KEYSas a frozenSetat module scope is clean
…h [MEDIUM] setConfigValue() tokenized dot-separated config paths from snapshot manifests and traversed into objects without checking for dangerous property names. A crafted snapshot with configPath "__proto__.isAdmin" could pollute Object.prototype, corrupting process-wide state. Add UNSAFE_PROPERTY_NAMES denylist check that rejects __proto__, constructor, and prototype before any traversal occurs. Add regression tests covering all three dangerous keys and legitimate path formats. Reported-by: FailSafe Security Researcher Co-Authored-By: Joshua Medvinsky <joshua-medvinsky@users.noreply.github.com>
Joshua-Medvinsky
force-pushed
the
fix/prototype-pollution-config-path
branch
from
5ef98d7 to
deb4a1d
Compare
Contributor
Author
|
Updated per review feedback:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.test.ts`:
- Around line 1425-1440: The tests currently check nested "__proto__" but miss
nested "constructor" and "prototype" cases; update the migration-state tests
that call setConfigValue so they also assert nested unsafe segments are rejected
(e.g., add expectations like expect(() => setConfigValue(doc,
`agents.constructor.isAdmin`, "true")).toThrow(/Unsafe config path segment/) and
expect(() => setConfigValue(doc, `agents.prototype.isAdmin`,
"true")).toThrow(/Unsafe config path segment/)); you can either add two new
it(...) blocks mirroring the "__proto__" nested case or extend the existing
it.each/array of segments used with setConfigValue to include "constructor" and
"prototype" so setConfigValue is validated for all three unsafe segments.
🪄 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: Pro
Run ID: c843840d-e9a3-4ed4-8b51-0da79dab639d
📒 Files selected for processing (2)
nemoclaw/src/commands/migration-state.test.tsnemoclaw/src/commands/migration-state.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw/src/commands/migration-state.ts
Comment on lines
+1425
to
+1440
| it.each(["__proto__", "constructor", "prototype"])( | ||
| "rejects unsafe path segment: %s", | ||
| (segment) => { | ||
| const doc: Record<string, unknown> = {}; | ||
| expect(() => setConfigValue(doc, `${segment}.polluted`, "true")).toThrow( | ||
| /Unsafe config path segment/, | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| it("rejects __proto__ in nested position", () => { | ||
| const doc: Record<string, unknown> = {}; | ||
| expect(() => | ||
| setConfigValue(doc, `agents.__proto__.isAdmin`, "true"), | ||
| ).toThrow(/Unsafe config path segment/); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add nested constructor/prototype cases to match the security test plan.
Current coverage checks nested __proto__, but not nested constructor/prototype paths (for example foo.prototype.bar), which were called out in the PR objective.
Suggested test additions
it("rejects __proto__ in nested position", () => {
const doc: Record<string, unknown> = {};
expect(() =>
setConfigValue(doc, `agents.__proto__.isAdmin`, "true"),
).toThrow(/Unsafe config path segment/);
});
+ it.each(["foo.prototype.bar", "foo.constructor.bar"])(
+ "rejects unsafe segment in nested path: %s",
+ (path) => {
+ const doc: Record<string, unknown> = {};
+ expect(() => setConfigValue(doc, path, "true")).toThrow(
+ /Unsafe config path segment/,
+ );
+ },
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/migration-state.test.ts` around lines 1425 - 1440, The
tests currently check nested "__proto__" but miss nested "constructor" and
"prototype" cases; update the migration-state tests that call setConfigValue so
they also assert nested unsafe segments are rejected (e.g., add expectations
like expect(() => setConfigValue(doc, `agents.constructor.isAdmin`,
"true")).toThrow(/Unsafe config path segment/) and expect(() =>
setConfigValue(doc, `agents.prototype.isAdmin`, "true")).toThrow(/Unsafe config
path segment/)); you can either add two new it(...) blocks mirroring the
"__proto__" nested case or extend the existing it.each/array of segments used
with setConfigValue to include "constructor" and "prototype" so setConfigValue
is validated for all three unsafe segments.
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
4 participants
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.
Security Finding: Prototype Pollution via Unsanitized Config Path
Severity: MEDIUM
Reported by: FailSafe Security Researcher
Component:
nemoclaw/src/commands/migration-state.ts—setConfigValue()Description
The
setConfigValue()function in the snapshot migration module tokenizes a dot-separated configuration path and iteratively traverses or builds an object structure to set a value. The function does not sanitize path tokens against dangerous JavaScript property names such as__proto__,constructor, orprototype, allowing an attacker to polluteObject.prototypethrough a crafted snapshot manifest.The
configPathvalue originates frommanifest.externalRoots[].bindings[].configPathinside the snapshot'ssnapshot.jsonfile. When an operator restores a snapshot from an untrusted or compromised source, the manifest-controlledconfigPathis passed directly tosetConfigValue()without filtering.Exploitation path: A crafted
snapshot.jsonwith a binding containingconfigPath: "__proto__.isAdmin"would causesetConfigValue()to traverse intoObject.prototypeand setisAdminon every object in the Node.js process.Fix
Add a denylist check (
__proto__,constructor,prototype) before using each token as a property key. Throws an error if any token matches a dangerous key.Test plan
configPathvalues still worksconfigPath: "__proto__.isAdmin"throwsUnsafe config path segmentconfigPath: "constructor.polluted"is rejectedconfigPath: "foo.prototype.bar"is rejectedSummary by CodeRabbit