diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index 6b3b37b..486c029 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -7,6 +7,7 @@ import { deserialize, serialize, RpcSession, type RpcSessionOptions, RpcTranspor type RpcTransportWithCustomEncoding, RpcTarget, RpcStub, newWebSocketRpcSession, newMessagePortRpcSession, newHttpBatchRpcSession} from "../src/index.js" +import { MAX_CLOSE_REASON_BYTES } from "../src/websocket.js" import { Counter, TestTarget } from "./test-util.js"; type CustomEncodingLevel = RpcTransportWithCustomEncoding["encodingLevel"]; @@ -3451,67 +3452,7 @@ describe("deserialization and transport correctness", () => { expect(String(error.message)).toContain("bad RPC message"); }); - it("handles a duplicate resolve message for an import without leaking or double-releasing", async () => { - // A recording transport pair so we can observe outgoing messages and re-inject one. - class RecTransport implements RpcTransport { - sent: string[] = []; - private incoming: string[] = []; - partner!: RecTransport; - private waiter?: () => void; - - async send(message: string): Promise { - message = message.replaceAll("$remove$", ""); - this.sent.push(message); - this.partner.deliver(message); - } - - deliver(message: string) { - this.incoming.push(message); - this.waiter?.(); - this.waiter = undefined; - } - - async receive(): Promise { - while (this.incoming.length === 0) { - await new Promise(resolve => { this.waiter = resolve; }); - } - return this.incoming.shift()!; - } - } - - let clientT = new RecTransport(); - let serverT = new RecTransport(); - clientT.partner = serverT; - serverT.partner = clientT; - - let client = new RpcSession(clientT); - // Keep a reference so the session is not collected; the server's main is a Counter. - let _server = new RpcSession(serverT, new Counter(5)); - - // A normal call creates and then resolves+releases a client import. - let value = await client.getRemoteMain().increment(1); - expect(value).toBe(6); - - let resolveMsg = serverT.sent.find(m => m.startsWith('["resolve"')); - expect(resolveMsg).toBeDefined(); - - let releasesBefore = clientT.sent.filter(m => m.startsWith('["release"')).length; - let statsBefore = client.getStats(); - - // Re-inject the resolve for the (now already-resolved-and-released) import. This must be - // handled cleanly: no duplicate release is emitted, the resolution hook is disposed rather - // than leaked, and the session is not aborted. The guard added in ImportTableEntry.resolve() - // is the defensive backstop for this scenario; via the public protocol the duplicate lands - // on the "import not found" path because the table entry is removed after the first - // resolution. - clientT.deliver(resolveMsg!); - await pumpMicrotasks(); - - expect(clientT.sent.filter(m => m.startsWith('["release"')).length).toBe(releasesBefore); - expect(client.getStats()).toStrictEqual(statsBefore); - }); - - it("truncates an over-long WebSocket close reason to 123 UTF-8 bytes", async () => { + it("truncates an over-long WebSocket close reason on a code-point boundary", async () => { let closeArgs: { code: number, reason: string }[] = []; let closeThrew = false; let listeners: Record void)[]> = {}; @@ -3523,8 +3464,8 @@ describe("deserialization and transport correctness", () => { }, send(_message: string) { return Promise.resolve(); }, close(code: number, reason: string) { - // Mimic the real WebSocket contract: a reason longer than 123 UTF-8 bytes throws. - if (new TextEncoder().encode(reason).length > 123) { + // Mimic the real contract: an over-long reason throws. + if (new TextEncoder().encode(reason).length > MAX_CLOSE_REASON_BYTES) { closeThrew = true; throw new Error("close reason too long"); } @@ -3534,23 +3475,19 @@ describe("deserialization and transport correctness", () => { newWebSocketRpcSession(fakeWs); - // A malformed message whose resulting "bad RPC message: ..." error far exceeds 123 UTF-8 - // bytes and contains multibyte characters, so truncation must land on a character boundary. - let huge = "ABC" + "\u{1F4A5}".repeat(80); - let badMessage = JSON.stringify([huge]); - for (let cb of listeners["message"] || []) { - cb({ data: badMessage }); - } + // Peer-supplied abort reason that straddles the limit with a multi-byte character: the 2-byte + // "é" begins at the last allowed byte, so truncation must drop the partial "é" rather than emit + // a replacement character (itself 3 bytes, which would re-exceed the limit). + let reason = "a".repeat(MAX_CLOSE_REASON_BYTES - 1) + "\u00e9"; + for (let cb of listeners["message"] || []) cb({ data: JSON.stringify(["abort", reason]) }); await pumpMicrotasks(); expect(closeThrew).toBe(false); expect(closeArgs.length).toBe(1); expect(closeArgs[0].code).toBe(3000); - - let reason = closeArgs[0].reason; - expect(new TextEncoder().encode(reason).length).toBeLessThanOrEqual(123); - // The truncated reason is still valid (no replacement char from a split multibyte char). - expect(reason).not.toContain("\uFFFD"); - expect(reason).toBe(new TextDecoder().decode(new TextEncoder().encode(reason))); + let sentReason = closeArgs[0].reason; + expect(new TextEncoder().encode(sentReason).length).toBeLessThanOrEqual(MAX_CLOSE_REASON_BYTES); + expect(sentReason).not.toContain("\uFFFD"); + expect(sentReason).toBe("a".repeat(MAX_CLOSE_REASON_BYTES - 1)); }); }); diff --git a/src/rpc.ts b/src/rpc.ts index c248de5..1ea76c1 100644 --- a/src/rpc.ts +++ b/src/rpc.ts @@ -205,13 +205,6 @@ class ImportTableEntry { // a forwarded call. The caller is responsible for ensuring the last of these is handed off // before direct calls can be delivered. - if (this.resolution) { - // Already resolved. A duplicate resolution would overwrite (and leak) the - // existing one, so dispose the incoming hook and ignore it. - resolution.dispose(); - return; - } - if (this.localRefcount == 0) { // Already disposed (canceled), so ignore the resolution and don't send a redundant release. resolution.dispose(); diff --git a/src/serialize.ts b/src/serialize.ts index fed1f90..ad8ff76 100644 --- a/src/serialize.ts +++ b/src/serialize.ts @@ -121,19 +121,14 @@ async function streamToBlob(stream: ReadableStream, type: string): Promise return b.type === type ? b : b.slice(0, b.size, type); } -// Maps error name to error class for deserialization. -// -// Uses a null prototype so that an attacker-supplied error type name from the wire -// (`value[1]`) cannot resolve to an inherited `Object.prototype` member. Without this, -// e.g. `ERROR_TYPES["constructor"]` would resolve to `Object` (truthy, so the `|| Error` -// fallback below is skipped), producing `new Object(message)` -- a `String` wrapper rather -// than an `Error` (type confusion / `instanceof Error` bypass), and a name like `"toString"` -// would resolve to a non-constructor and throw. With a null prototype, every non-own key -// resolves to `undefined` and correctly falls back to `Error`. -const ERROR_TYPES: Record = Object.assign(Object.create(null), { +// Maps error name to error class for deserialization. Null-prototype so a wire-supplied (untrusted) +// name can't resolve to an inherited member like `constructor` (which would build a `String` +// wrapper, not an `Error`) or `toString`; unknown names fall back to `Error`. +const ERROR_TYPES: Record = { + __proto__: null, Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError, URIError, AggregateError, // TODO: DOMError? Others? -}); +}; // Converts fully-hydrated messages into object trees that are JSON-serializable for sending over // the wire. This is used to implement serialization -- but it doesn't take the last step of diff --git a/src/websocket.ts b/src/websocket.ts index 0140c61..fa7a4ee 100644 --- a/src/websocket.ts +++ b/src/websocket.ts @@ -7,6 +7,9 @@ import { RpcStub } from "./core.js"; import { RpcSession, RpcSessionOptions } from "./rpc.js"; +/** Close-frame reason max UTF-8 bytes: 125 payload − 2-byte status (RFC 6455 §5.5). */ +export const MAX_CLOSE_REASON_BYTES = 125 - 2; + export function newWebSocketRpcSession( webSocket: WebSocket | string, localMain?: any, options?: RpcSessionOptions): RpcStub { if (typeof webSocket === "string") { @@ -124,13 +127,10 @@ export class WebSocketTransport { } else { message = `${reason}`; } - // WebSocket close reasons are limited to 123 UTF-8 bytes; a longer string makes - // close() throw. Truncate on a character boundary to stay within the limit. + // `stream: true` drops a trailing partial code point rather than emitting a replacement char. let reasonBytes = new TextEncoder().encode(message); - if (reasonBytes.length > 123) { - // Decoding with { stream: true } drops any trailing partial code unit, so the - // result is guaranteed to be valid and ≤ 123 bytes. - message = new TextDecoder().decode(reasonBytes.subarray(0, 123), { stream: true }); + if (reasonBytes.length > MAX_CLOSE_REASON_BYTES) { + message = new TextDecoder().decode(reasonBytes.subarray(0, MAX_CLOSE_REASON_BYTES), { stream: true }); } this.#webSocket.close(3000, message);