fix: prevent prototype hijack in theme safeMerge (CWE-1321)#5027
Open
alanturing881 wants to merge 1 commit into
Open
fix: prevent prototype hijack in theme safeMerge (CWE-1321)#5027alanturing881 wants to merge 1 commit into
alanturing881 wants to merge 1 commit into
Conversation
safeMerge() recursively merges component `theme` overrides using bracket-notation assignment (`out[key] = ...`). Unlike object-literal spread, bracket assignment invokes the `Object.prototype.__proto__` accessor, so a crafted overrides object with an own `__proto__` key (e.g. produced by JSON.parse of an external theme config) hijacks the prototype of the merged object instead of being stored as a normal property. Add a denylist for `__proto__`/`constructor`/`prototype` keys so they are skipped during the merge, matching the function's stated intent of being a *safe* merge. Verified the fix blocks the hijack while preserving legitimate deep-merge behavior and PlatformColor sentinel handling; full existing test suite passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
safeMergeinsrc/theme/provider.tsx(used by every component'sthemeprop override viauseInternalTheme) recursively merges the override object into a fresh copy of the base theme using bracket-notation assignment:Unlike object-literal spread (
{...obj}), which copies a property literally named"__proto__"as an ordinary own data property, bracket-notation assignment invokes theObject.prototype.__proto__accessor. Ifoverrides(e.g. parsed viaJSON.parsefrom an external/remote theme config) has an own key named"__proto__", thenout["__proto__"] = valuecallsObject.setPrototypeOf(out, value)— hijacking the prototype of the objectsafeMergeis building instead of storing it as a normal property.Proof
This is scoped to the specific object
safeMergereturns (it does not reach the shared globalObject.prototype), so the blast radius is limited — but it's still a real CWE-1321 gap in a function whose name and surrounding comment ("Upstreamdeepmergecorrupts PlatformColor objects, so we recurse manually") signal it's meant to be a safe, hardened replacement.Realistic impact
Applications that build per-component
themeoverrides from external/remote JSON (e.g. white-label/dynamic-branding apps) and pass that data straight into a component'sthemeprop without validating its shape are exposed. Because the merge starts from{...base}, keys already present in the real theme schema are shadowed by own properties, so the practical effect is limited to forging fallback values for undefined theme property lookups — not overriding existing, actively-used theme values.Fix
Add a denylist for
__proto__/constructor/prototypesosafeMergenever assigns those keys, closing the hijack regardless of what shapeoverridestakes:Test plan
__proto__key in overrides hijacking the merged object's prototype) is blocked after the fix.colors.primary, adding new keys, leaving untouched keys intact) is unchanged.PlatformColor/DynamicColorIOSsentinel objects are still treated as leaves (not walked into) as before.yarn jest): 732 passed, 1 skipped, 0 failed.tsc --noEmitandeslintclean on the changed file.🤖 Generated with Claude Code