Fix five correctness and robustness issues in serialization and transport handling#190
Open
taylorodell wants to merge 2 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: fd36ad1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
- serialize.ts: filter inherited Object.prototype keys (and toJSON) out of an error's own-property bag during deserialization, matching the plain-object path. Prevents keys like __proto__/toString/valueOf from being copied onto deserialized errors. - rpc.ts (ImportTableEntry.resolve): ignore and dispose a duplicate resolution instead of overwriting and leaking the previous one. - rpc.ts (abort handler): pass the unwrapped payload.value to abort(), matching the adjacent reject handler, so error handlers receive the reason rather than the payload wrapper. - websocket.ts (abort): truncate the close reason to the 123-byte limit on a UTF-8 character boundary so close() does not throw on long reasons. Adds focused unit tests for each fix.
The error deserializer looks up the wire-supplied error type name (value[1]) in ERROR_TYPES, which was a plain object literal inheriting Object.prototype. A name like "constructor" resolved to Object.prototype.constructor (i.e. Object) -- which is truthy, so the `|| Error` fallback was skipped. `new Object(message)` then produced a String wrapper rather than an Error, defeating downstream `instanceof Error` checks while still carrying the sender's own-property bag. A name like "toString" resolved to a non-constructor and threw, aborting the session. Give ERROR_TYPES a null prototype so unknown or inherited names resolve to undefined and correctly fall back to Error. Adds a regression test covering the "constructor"/"toString"/etc. cases alongside the existing prototype-key filtering test.
dad80cd to
fd36ad1
Compare
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.
This PR fixes five small, independent correctness/robustness bugs in serialization and transport handling, and adds a focused unit test for each.
1. Error type names should not resolve to
Object.prototypememberssrc/serialize.ts(the"error"case): the wire-supplied error type name (value[1]) is looked up inERROR_TYPES, which was a plain object literal inheritingObject.prototype. A name like"constructor"resolved toObject.prototype.constructor(i.e.Object) — which is truthy, so the|| Errorfallback was skipped — andnew Object(message)then produced aStringwrapper rather than anError, defeating downstreaminstanceof Errorchecks while still carrying the sender's own-property bag. A name like"toString"resolved to a non-constructor and threw, aborting the session.ERROR_TYPESnow has a null prototype, so unknown or inherited names resolve toundefinedand correctly fall back toError.2. Error deserialization should filter
Object.prototypekeyssrc/serialize.ts(the"error"case): the plain-object deserializer already skips keys that shadowObject.prototypemembers (andtoJSON), but the error path only skippedname/message/stack. As a result, keys like__proto__,toString,valueOf,hasOwnProperty, ortoJSONin an error's own-property bag were assigned as own properties on the deserialized error (and__proto__in particular went through the prototype setter). This makes the error path consistent with the plain-object path: such keys are skipped, while their inner values are still evaluated so any stubs are released.3. Guard against double-resolving an import
src/rpc.ts,ImportTableEntry.resolve(): ifresolve()is ever called a second time,this.resolutionwas overwritten without disposing the previous hook, leaking it. Added a guard at the top that disposes the incoming hook and returns when a resolution already exists.4.
aborthandler passes the wrong valuesrc/rpc.ts, the"abort"case inreadLoop(): it passed theRpcPayloadwrapper tothis.abort()instead of the unwrapped reason. The adjacent"reject"handler already usespayload.value; this change makesabortconsistent, so error handlers receive the actual reason rather than the wrapper.5. Truncate WebSocket close reason to 123 bytes
src/websocket.ts,WebSocketTransport.abort():WebSocket.close(code, reason)requires the reason to be ≤ 123 UTF-8 bytes; a longer reason makesclose()throw and escape the abort path. The reason is now truncated on a UTF-8 character boundary (using a streamingTextDecoderso no partial code unit is emitted) before callingclose().Testing
Added one unit test per fix in
__tests__/index.test.ts(existing test style/harness). All existing and new tests pass viavitest run(nodeandworkerdprojects locally; the@vitest/browserprojects run on CI). A patch changeset is included.