Skip to content

Fix five correctness and robustness issues in serialization and transport handling#190

Open
taylorodell wants to merge 2 commits into
cloudflare:mainfrom
taylorodell:fix/serialize-rpc-websocket-correctness
Open

Fix five correctness and robustness issues in serialization and transport handling#190
taylorodell wants to merge 2 commits into
cloudflare:mainfrom
taylorodell:fix/serialize-rpc-websocket-correctness

Conversation

@taylorodell

@taylorodell taylorodell commented Jun 5, 2026

Copy link
Copy Markdown

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.prototype members

src/serialize.ts (the "error" case): the wire-supplied error type name (value[1]) is looked up 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 — and 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. ERROR_TYPES now has a null prototype, so unknown or inherited names resolve to undefined and correctly fall back to Error.

2. Error deserialization should filter Object.prototype keys

src/serialize.ts (the "error" case): the plain-object deserializer already skips keys that shadow Object.prototype members (and toJSON), but the error path only skipped name/message/stack. As a result, keys like __proto__, toString, valueOf, hasOwnProperty, or toJSON in 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(): if resolve() is ever called a second time, this.resolution was 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. abort handler passes the wrong value

src/rpc.ts, the "abort" case in readLoop(): it passed the RpcPayload wrapper to this.abort() instead of the unwrapped reason. The adjacent "reject" handler already uses payload.value; this change makes abort consistent, 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 makes close() throw and escape the abort path. The reason is now truncated on a UTF-8 character boundary (using a streaming TextDecoder so no partial code unit is emitted) before calling close().

Testing

Added one unit test per fix in __tests__/index.test.ts (existing test style/harness). All existing and new tests pass via vitest run (node and workerd projects locally; the @vitest/browser projects run on CI). A patch changeset is included.

@changeset-bot

changeset-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fd36ad1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
capnweb Patch

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

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@taylorodell

Copy link
Copy Markdown
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.
@taylorodell taylorodell force-pushed the fix/serialize-rpc-websocket-correctness branch from dad80cd to fd36ad1 Compare June 27, 2026 14:33
github-actions Bot added a commit that referenced this pull request Jun 27, 2026
@taylorodell taylorodell changed the title Fix four correctness issues in serialization and transport handling Fix five correctness and robustness issues in serialization and transport handling Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant