constructor is not stripped alongside __proto__, so a recursive merge of a decoded argument pollutes Object.prototype

[frenzzy]

Summary

stripOwnProtoKeys removes __proto__ and nothing else. constructor survives decode, and a recursive merge — the other half of the same sink class the guard's own rationale names — reaches Object.prototype through constructor.prototype.

Unlike the __proto__ case, this one actually pollutes Object.prototype.

Tested against next @ 1cc2feb8, built from source, Node 24.19.

Reproduction

payload [{"__proto__":{"polluted":"viaProto"},"n":1}]   -> own keys ["n"]                  ← stripped
payload [{"constructor":{"prototype":{"polluted":"viaCtor"}},"n":1}]
                                                        -> own keys ["constructor","n"]    ← survives

Then the naive deep merge that #3168's rationale describes as the downstream move:

function deepMerge(target, source) {
  for (const k of Object.keys(source)) {
    if (source[k] && typeof source[k] === "object") { target[k] ??= {}; deepMerge(target[k], source[k]); }
    else target[k] = source[k];
  }
  return target;
}

deepMerge({}, decodedArgument);

Object.prototype.polluted  ->  "viaCtor"

For contrast, the same merge on the __proto__ payload is inert, because that key was stripped.

Why this is worth closing alongside #3168

#3168's justification is that Object.assign merges by [[Set]], so a __proto__ key triggers the inherited setter. That reasoning covers a shallow merge. A recursive merge — at least as common in configuration and patch handling — walks into constructor, finds prototype, and writes onto it. Same class of sink, same decode boundary, and the guard already took responsibility for it.

Half-covering the class is worse than not covering it, because the presence of a strip reads as "this boundary is handled". An author who read #3168 and concluded their merge was safe is wrong for the recursive spelling.

It is also strictly more severe than the case that was fixed: __proto__ on a plain object only re-prototypes the merged copy, while constructor.prototype reaches the shared Object.prototype and affects the whole process.

Scope

• Needs a vulnerable recursive merge in application code — so not remotely exploitable against core, same category as Decoded arguments keep __proto__ as an own key, so an ordinary Object.assign merge in a handler re-prototypes the result #3168.
• Needs no enableRichArguments(): the plain-JSON road reproduces it, as does the codec road.
• Object.prototype is reachable here, which the __proto__ road never allowed.

stripOwnProtoKeys is the only strip in the tree — and the other decode roads have none

Two further request-controlled decode boundaries carry no strip at all, so #3168's own defect is still fully open one road over:

decodeFlashCookie  own __proto__ on result: true   keys: ["__proto__","n"]
                   Object.prototype.polluted after a recursive merge: "viaFlash"

The flash cookie is plain unsigned JSON — flash.ts says so: "The payload is plain JSON rather than the wire codec" — read straight off the request's Cookie header and handed to the render as submission.result / submission.input. Anyone who can set a cookie on the domain controls it.

The client's decodeResponse is the same story for a hostile or compromised server.

A fix that touches only stripOwnProtoKeys leaves both, and leaving them is exactly the "this boundary is handled" illusion this issue is about. Whether they belong in this PR or a follow-up is a scoping call, but they should be named.

One correction: the codec's own encoder refuses an own constructor

No honest client can send this key over the codec road — seroval refuses to serialize it:

own constructor key   ENCODER REFUSED: Seroval Error (specific: 1)
own __proto__ key     ENCODED
own prototype key     ENCODED

The codec decode road is still vulnerable, but only to a hand-written frame — which is what a hostile peer writes. Worth knowing because it also means stripping constructor costs the codec road nothing.

Options

1. Strip constructor alongside __proto__ in the same walk. One line, matches the existing decision, and an own key literally named constructor on a decoded argument has no legitimate meaning.
2. Strip the full dangerous set — __proto__, constructor, prototype. Broader; prototype as an own key on a plain decoded object is likewise meaningless.
3. Refuse the payload with a 400 naming the key, as Decoded arguments keep __proto__ as an own key, so an ordinary Object.assign merge in a handler re-prototypes the result #3168's option (2) proposed. Gives the caller a real answer rather than silent repair, and surfaces the data-loss problem the strip otherwise hides (a field literally named constructor disappears with no warning).
4. Return null-prototype objects from decode — makes both keys inert without removing them, and closes the class rather than enumerating it.

Two notes from measuring these. A lone prototype key is inert on its own — it only matters as the second half of the constructor walk — so option (2) buys marginal safety over (1) at the cost of eating a legitimately-named field. And option (4)'s cost is broader than it looks: obj.toString(), obj.hasOwnProperty(...), instanceof Object and any library assuming a prototype all break for every decoded argument, and it needs a change inside the codec's revival rather than at this seam.

I'd suggest (2) as the minimal correction, or (4) if the shape change is acceptable — enumerating dangerous key names is a pattern that keeps needing another entry.

Happy to send a PR with the strip plus a regression test covering __proto__, constructor and prototype on both decode roads, asserting Object.prototype is untouched after a recursive merge, with a legitimately-named field as the control.

Related — the same function has two independent gaps

stripOwnProtoKeys carries both defects, and closing one leaves the class open:

} else {
  const proto = Object.getPrototypeOf(v);
  if (proto !== Object.prototype && proto !== null) continue;   // ← #3200: skips the whole subtree
  if (Object.prototype.hasOwnProperty.call(v, "__proto__")) {   // ← #3202: one key name only
    delete v["__proto__"];
  }

• #3168's __proto__ strip is bypassed by wrapping the payload in a codec-revived Error #3200 — the continue conflates "do not rebuild this container" with "do not look inside it", so anything under a codec-revived Error keeps its keys.
• constructor is not stripped alongside __proto__, so a recursive merge of a decoded argument pollutes Object.prototype #3202 — only __proto__ is checked, so constructor survives and a recursive merge reaches Object.prototype.

They are orthogonal: fixing the walk does not strip constructor, and stripping constructor does not make the walk descend. A fix should address both, and one regression test can cover the matrix (key name × carrier × decode road).

Worth noting what already works and must keep working: Map keys, Map values and Set members are walked (the branches above), and a Map whose key is the literal string "__proto__" round-trips undamaged.

Provenance

Introduced by c42fc3fe — fix(web): strip own __proto__ keys at server-function argument decode (#3168), at packages/web/server-functions/src/server.ts:1132.

Worth weighing against Solid's preference for primitives over enumerated special cases: a denylist of dangerous key names is a list that keeps needing another entry (__proto__, then constructor, then prototype). Returning null-prototype objects from decode makes all of them inert with no list at all — a smaller rule, and one that cannot be outgrown. It changes the shape handlers receive, so it is a design call rather than a patch, but it is the option that closes the class instead of enumerating it.

One change closes this and its sibling — implemented and verified

#3200 and #3202 are two gaps in one function, and the prototype check turns out to protect nothing: stripOwnProtoKeys mutates in place and rebuilds nothing, so continue-ing on a non-plain prototype was never guarding a rebuild — it only stopped the walk, which is the whole of #3200.

     } else {
-      const proto = Object.getPrototypeOf(v);
-      if (proto !== Object.prototype && proto !== null) continue;
-      if (Object.prototype.hasOwnProperty.call(v, "__proto__")) {
-        delete v["__proto__"];
-      }
+      for (const key of UNSAFE_ARGUMENT_KEYS) {
+        if (Object.prototype.hasOwnProperty.call(v, key)) delete v[key];
+      }
       for (const key of Object.keys(v)) stack.push(v[key]);
     }

with const UNSAFE_ARGUMENT_KEYS = ["__proto__", "constructor", "prototype"]; beside the function.

10 insertions, 4 deletions, six of the insertions being the comment — so the executable change is smaller than the code it replaces, and a special case disappears.

Verified

constructor payload      own keys ["n"]      (was ["constructor","n"])
Object.prototype.polluted after a recursive merge: undefined   (was "viaCtor")
Error-carrier bypass     merged.isAdmin undefined, prototype hijacked false   (was true/true)

control — a Map keyed by the literal string "__proto__":
  is a Map: true   size: 2   get("__proto__"): "v"   get("other"): "w"    ← intact

Full tracked suite with the change: 53 files, 586 passed, 2 skipped — no existing test moved.

What this does not cover

The two other request-controlled decode boundaries still have no strip: decodeFlashCookie (unsigned JSON off the Cookie header) and the client's decodeResponse. Verified still open:

decodeFlashCookie -> own __proto__ on result: true, keys ["__proto__","n"]
                     Object.prototype.polluted after a recursive merge: "viaFlash"

Whether those belong in the same PR is a scoping call, but a fix that stops at stripOwnProtoKeys leaves #3168's own defect live one road over.

Measured scope — three of four idiomatic copies are already safe

Worth stating plainly, because it narrows this issue and I did not say it when filing. Measured on Node 24.19:

JSON.parse makes it an own key    : true
Object.keys sees it               : ["__proto__","a"]

Object.assign({}, src)   prototype replaced: true      ← the only unsafe one
{ ...src }               prototype replaced: false
structuredClone(src)     prototype replaced: false
Object.fromEntries(...)  prototype replaced: false

Object.prototype itself polluted  : false

Only assignment is unsafe, because it goes through [[Set]] and wakes the inherited setter; spread and fromEntries define rather than set. And the __proto__ road never reaches Object.prototype itself — it replaces the prototype of the copy.

The constructor road narrows the same way:

recursive merge via t[k] = v          -> Object.prototype.polluted = "viaCtor"
recursive merge via defineProperty    -> Object.prototype.polluted = undefined

So what this issue asks the runtime to do is protect application code from a footgun the application can avoid by writing spread instead of Object.assign. That is a boundary-of-responsibility decision rather than an unambiguous defect, and it deserves to be decided as one.

The case for the runtime taking it: this boundary is already hardened in other ways — decode depth is capped, RegExp revival is disabled, the argument count is bounded — and a large application multiplies the chance that one handler somewhere merges by assignment.

The case against, also measured: the strip is lossy. A field honestly named constructor disappears with no error and no warning. Refusing the payload with a 400 that names the key (option 3 below) keeps the protection without the silent data loss.

[ryansolid]

Fixed by #3203.