Skip to content

fix(security): prevent prototype pollution via unsanitized config path [MEDIUM] - #1985

Closed
ericksoa wants to merge 4 commits into
mainfrom
fix/prototype-pollution-config-path
Closed

fix(security): prevent prototype pollution via unsanitized config path [MEDIUM]#1985
ericksoa wants to merge 4 commits into
mainfrom
fix/prototype-pollution-config-path

Conversation

@ericksoa

@ericksoa ericksoa commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Security Finding: Prototype Pollution via Unsanitized Config Path

Severity: MEDIUM
Reported by: FailSafe Security Researcher
Component: nemoclaw/src/commands/migration-state.tssetConfigValue()

Description

The setConfigValue() function tokenizes a dot-separated configuration path and iteratively traverses or builds an object structure to set a value. The function did not sanitize path tokens against dangerous JavaScript property names such as __proto__, constructor, or prototype, allowing an attacker to pollute Object.prototype through a crafted snapshot manifest.

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.

Tests

  • Unsafe paths (__proto__, constructor, prototype) in both top-level and nested positions are rejected
  • Prototype stays clean after rejection (no partial mutation)
  • Legitimate paths still work

Supersedes #1558. Closes #1553.

Co-authored-by: failsafesecurity failsafesecurity@users.noreply.github.com
Co-authored-by: Joshua Medvinsky joshua-medvinsky@users.noreply.github.com
Co-authored-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Configuration settings validation now properly rejects invalid path segments to ensure system integrity and stability
  • Tests
    • Added comprehensive test coverage for configuration path validation, including edge cases and boundary conditions

failsafesecurity and others added 4 commits April 16, 2026 14:35
…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 in both top-level and nested
positions, plus legitimate path formats.

Reported-by: FailSafe Security Researcher
Co-Authored-By: Joshua Medvinsky <joshua-medvinsky@users.noreply.github.com>
Replace `as any` cast with typed narrowing and add braces to void
arrow returns to satisfy no-explicit-any and no-confusing-void-expression.

Signed-off-by: Test User <test@example.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ad3b9451-91b2-4c97-8b2c-95ca2cdc0d8f

📥 Commits

Reviewing files that changed from the base of the PR and between a8d0e41 and b5499bb.

📒 Files selected for processing (2)
  • nemoclaw/src/commands/migration-state.test.ts
  • nemoclaw/src/commands/migration-state.ts

📝 Walkthrough

Walkthrough

Added comprehensive prototype pollution defenses to the setConfigValue function by introducing a validation layer that rejects unsafe property paths (__proto__, constructor, prototype) while allowing legitimate configuration updates. Corresponding test suite validates both rejection of unsafe paths and acceptance of safe ones.

Changes

Cohort / File(s) Summary
Prototype Pollution Defense
nemoclaw/src/commands/migration-state.ts, nemoclaw/src/commands/migration-state.test.ts
Exported setConfigValue function with added pre-validation that rejects path segments matching unsafe property names. Comprehensive test suite verifies unsafe paths throw errors and Object.prototype remains unpolluted, while safe paths function correctly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Poem

🐰 Hops with glee through proto-safe code,
No polluted objects on this typed road,
Constructor and __proto__ now barred with care,
Security tests dance through the air! ✨🛡️

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/prototype-pollution-config-path

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

@ericksoa

Copy link
Copy Markdown
Contributor Author

Not needed — reopened the original #1558.

@ericksoa ericksoa closed this Apr 16, 2026
@ericksoa
ericksoa deleted the fix/prototype-pollution-config-path branch April 16, 2026 21:38
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants