From a624cd423ea44d1a00165e9e0623a40c6d8c2946 Mon Sep 17 00:00:00 2001 From: dmmulroy Date: Fri, 5 Dec 2025 12:07:20 -0500 Subject: [PATCH 1/2] fix: serialize @Persist state for RPC returns Fixes #99 - @Persist state returned RpcStub objects instead of data Problem: Cloudflare RPC uses structured clone which can't serialize Proxy objects. @Persist wraps values in Proxy for mutation tracking. Solution: Unwrap proxies before RPC serialization via _wrapMethodsForRpc(). Optimized to skip non-proxied values (most RPC returns) for performance. Key changes in persist.ts: - Add containsProxy() to detect IS_PROXIED symbol recursively - unwrapProxy() returns original when no proxy detected (fast path) - Only recreate objects with null prototype when proxy present ```typescript // Before: always recreated all objects export function unwrapProxy(value: T): T { // ... always traverse and recreate with Object.create(null) } // After: fast path for non-proxied values export function unwrapProxy(value: T): T { if (!containsProxy(value)) return value; // identity return // ... only unwrap when proxy detected } ``` --- packages/core/src/index.ts | 39 +++++++- packages/core/src/persist.test.ts | 157 ++++++++++++++++++++++++++++++ packages/core/src/persist.ts | 137 ++++++++++++++++++++++++-- 3 files changed, 321 insertions(+), 12 deletions(-) create mode 100644 packages/core/src/persist.test.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 69bec8b..e412274 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2,7 +2,7 @@ import { env, DurableObject, WorkerEntrypoint } from "cloudflare:workers"; import { Storage } from "../../storage/src/index"; import { Alarms } from "../../alarms/src/index"; import { Sockets } from "../../sockets/src/index"; -import { Persist, PERSISTED_VALUES, initializePersistedProperties, persistProperty } from "./persist"; +import { Persist, PERSISTED_VALUES, initializePersistedProperties, persistProperty, unwrapProxy } from "./persist"; export { Persist }; @@ -183,8 +183,43 @@ export abstract class Actor extends DurableObject { if (!this.name) { this._name = DEFAULT_ACTOR_NAME; } + + // Wrap RPC methods to unwrap proxy values for serialization + this._wrapMethodsForRpc(); } - + + /** + * Wraps all public methods to unwrap proxy return values for RPC serialization. + * @private + */ + private _wrapMethodsForRpc(): void { + const skipMethods = new Set([ + 'constructor', '_wrapMethodsForRpc', '_initializePersistedProperties', + '_waitForSetName', '_persistProperty', 'setName' + ]); + + let proto = Object.getPrototypeOf(this); + while (proto && proto !== Object.prototype) { + for (const name of Object.getOwnPropertyNames(proto)) { + if (skipMethods.has(name) || name.startsWith('_')) continue; + + const descriptor = Object.getOwnPropertyDescriptor(proto, name); + if (!descriptor || typeof descriptor.value !== 'function') continue; + + const original = descriptor.value; + const self = this; + (this as Record)[name] = function(...args: unknown[]) { + const result = original.apply(self, args); + if (result instanceof Promise) { + return result.then((v: unknown) => unwrapProxy(v)); + } + return unwrapProxy(result); + }; + } + proto = Object.getPrototypeOf(proto); + } + } + /** * Initializes the persisted properties table and loads any stored values. * This is called during construction to ensure properties are loaded before any code uses them. diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts new file mode 100644 index 0000000..9179f15 --- /dev/null +++ b/packages/core/src/persist.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, it } from "vitest"; +import { unwrapProxy, IS_PROXIED } from "./persist"; + +describe("unwrapProxy", () => { + describe("prototype pollution prevention", () => { + it("ignores __proto__ key in proxied objects", () => { + const malicious = JSON.parse('{"__proto__": {"polluted": true}, "safe": 1}'); + malicious[IS_PROXIED] = true; + const result = unwrapProxy(malicious); + + expect(result.safe).toBe(1); + expect(result.__proto__).toBeUndefined(); + expect(({} as Record).polluted).toBeUndefined(); + }); + + it("ignores constructor key in proxied objects", () => { + const malicious = { constructor: { prototype: { polluted: true } }, safe: 1, [IS_PROXIED]: true }; + const result = unwrapProxy(malicious); + + expect(result.safe).toBe(1); + expect(result.constructor).toBeUndefined(); + }); + + it("ignores prototype key in proxied objects", () => { + const malicious = { prototype: { polluted: true }, safe: 1, [IS_PROXIED]: true }; + const result = unwrapProxy(malicious); + + expect(result.safe).toBe(1); + expect(result.prototype).toBeUndefined(); + }); + + it("filters dangerous keys in nested proxied objects", () => { + const nested = JSON.parse('{"__proto__": {"polluted": true}, "valid": 2}'); + nested[IS_PROXIED] = true; + const malicious = { + nested, + safe: 1, + }; + const result = unwrapProxy(malicious); + + expect(result.safe).toBe(1); + expect(result.nested.valid).toBe(2); + expect(result.nested.__proto__).toBeUndefined(); + }); + + it("proxied object result has null prototype", () => { + const input = { a: 1, [IS_PROXIED]: true }; + const result = unwrapProxy(input); + + expect(Object.getPrototypeOf(result)).toBeNull(); + }); + + it("non-proxied objects returned unchanged (fast path)", () => { + const input = { a: 1 }; + const result = unwrapProxy(input); + + expect(result).toBe(input); + expect(Object.getPrototypeOf(result)).toBe(Object.prototype); + }); + + it("nested proxy triggers full unwrap", () => { + const input = { outer: { inner: { [IS_PROXIED]: true, val: 1 } } }; + const result = unwrapProxy(input); + + expect(result).not.toBe(input); + expect(result.outer.inner.val).toBe(1); + expect(Object.getPrototypeOf(result)).toBeNull(); + }); + + it("non-proxied array returned unchanged", () => { + const input = [1, 2, { a: 3 }]; + const result = unwrapProxy(input); + + expect(result).toBe(input); + }); + + it("array containing proxy triggers unwrap", () => { + const input = [1, { [IS_PROXIED]: true, val: 2 }]; + const result = unwrapProxy(input); + + expect(result).not.toBe(input); + expect(Array.isArray(result)).toBe(true); + }); + }); + + describe("basic functionality", () => { + it("handles primitives", () => { + expect(unwrapProxy(null)).toBeNull(); + expect(unwrapProxy(undefined)).toBeUndefined(); + expect(unwrapProxy(42)).toBe(42); + expect(unwrapProxy("str")).toBe("str"); + expect(unwrapProxy(true)).toBe(true); + }); + + it("handles arrays", () => { + const input = [1, { a: 2 }, [3]]; + const result = unwrapProxy(input); + + expect(result).toEqual([1, { a: 2 }, [3]]); + expect(Array.isArray(result)).toBe(true); + }); + + it("handles nested objects", () => { + const input = { a: { b: { c: 1 } } }; + const result = unwrapProxy(input); + + expect(result.a.b.c).toBe(1); + }); + + it("handles circular references", () => { + const obj: Record = { a: 1 }; + obj.self = obj; + + const result = unwrapProxy(obj); + + expect(result.a).toBe(1); + expect(result.self).toBe(result); + }); + + it("handles Map", () => { + const input = new Map([["key", { value: 1 }]]); + const result = unwrapProxy(input); + + expect(result instanceof Map).toBe(true); + expect(result.get("key")).toEqual({ value: 1 }); + }); + + it("handles Set", () => { + const input = new Set([1, 2, { a: 3 }]); + const result = unwrapProxy(input); + + expect(result instanceof Set).toBe(true); + expect(result.size).toBe(3); + }); + + it("preserves Date instances", () => { + const date = new Date("2024-01-01"); + const result = unwrapProxy(date); + + expect(result).toBe(date); + }); + + it("preserves RegExp instances", () => { + const regex = /test/gi; + const result = unwrapProxy(regex); + + expect(result).toBe(regex); + }); + + it("preserves Error instances", () => { + const error = new Error("test"); + const result = unwrapProxy(error); + + expect(result).toBe(error); + }); + }); +}); diff --git a/packages/core/src/persist.ts b/packages/core/src/persist.ts index dc78742..7688b16 100644 --- a/packages/core/src/persist.ts +++ b/packages/core/src/persist.ts @@ -15,6 +15,114 @@ export type Constructor = { [PERSISTED_PROPERTIES]?: Set; }; +/** + * Checks if a value contains any proxied objects (marked with IS_PROXIED symbol). + * Used to short-circuit unwrapProxy when no proxy is present. + * @internal + */ +function containsProxy(value: unknown, seen = new WeakSet()): boolean { + if (value === null || value === undefined || typeof value !== 'object') { + return false; + } + if (value instanceof Date || value instanceof RegExp || value instanceof Error) { + return false; + } + if (seen.has(value)) { + return false; + } + seen.add(value); + + // Check if this object is proxied + try { + if ((value as Record)[IS_PROXIED] === true) { + return true; + } + } catch { + // Ignore errors from accessing symbol on exotic objects + } + + if (Array.isArray(value)) { + return value.some(v => containsProxy(v, seen)); + } + if (value instanceof Map) { + for (const v of value.values()) { + if (containsProxy(v, seen)) return true; + } + return false; + } + if (value instanceof Set) { + for (const v of value) { + if (containsProxy(v, seen)) return true; + } + return false; + } + + for (const key of Object.keys(value)) { + if (containsProxy((value as Record)[key], seen)) { + return true; + } + } + return false; +} + +/** + * Recursively unwraps proxy objects to get raw data for RPC serialization. + * Handles circular references by tracking visited objects. + * @internal + */ +export function unwrapProxy(value: T, seen = new WeakMap()): T { + if (value === null || value === undefined || typeof value !== 'object') { + return value; + } + if (value instanceof Date || value instanceof RegExp || value instanceof Error) { + return value; + } + + // Fast path: if no proxy detected in value tree, return original unchanged + if (!containsProxy(value)) { + return value; + } + + // Handle circular references + const cached = seen.get(value); + if (cached !== undefined) { + return cached as T; + } + + if (Array.isArray(value)) { + const result: unknown[] = []; + seen.set(value, result); + for (let i = 0; i < value.length; i++) { + result[i] = unwrapProxy(value[i], seen); + } + return result as T; + } + if (value instanceof Map) { + const result = new Map(); + seen.set(value, result); + for (const [k, v] of value) { + result.set(k, unwrapProxy(v, seen)); + } + return result as T; + } + if (value instanceof Set) { + const result = new Set(); + seen.set(value, result); + for (const v of value) { + result.add(unwrapProxy(v, seen)); + } + return result as T; + } + + const result = Object.create(null) as Record; + seen.set(value, result); + for (const key of Object.keys(value)) { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue; + result[key] = unwrapProxy((value as Record)[key], seen); + } + return result as T; +} + /** * Creates a deep proxy for objects to track nested property changes * @param value The value to potentially proxy @@ -57,13 +165,19 @@ function createDeepProxy(value: any, instance: any, propertyKey: string, trigger if (key === IS_PROXIED) return true; // Handle special cases and built-in methods - if (typeof key === 'symbol' || - key === 'toString' || - key === 'valueOf' || + if (typeof key === 'symbol' || + key === 'toString' || + key === 'valueOf' || key === 'constructor' || - key === 'toJSON') { + key === '__proto__' || + key === 'prototype') { return Reflect.get(target, key); } + + // Provide toJSON to enable RPC serialization + if (key === 'toJSON') { + return () => unwrapProxy(target); + } try { // Check if the property exists @@ -141,14 +255,17 @@ function createDeepProxy(value: any, instance: any, propertyKey: string, trigger const currentValue = Reflect.get(target, key); // Handle different type transition scenarios - if (currentValue !== null && - typeof currentValue === 'object' && - newValue !== null && - typeof newValue === 'object' && - !Array.isArray(currentValue) && + if (currentValue !== null && + typeof currentValue === 'object' && + newValue !== null && + typeof newValue === 'object' && + !Array.isArray(currentValue) && !Array.isArray(newValue)) { // Case 1: Both values are objects - merge them instead of replacing - Object.assign(currentValue, newValue); + for (const k of Object.keys(newValue)) { + if (k === '__proto__' || k === 'constructor' || k === 'prototype') continue; + (currentValue as Record)[k] = (newValue as Record)[k]; + } } else if (newValue !== null && typeof newValue === 'object' && !Object.isFrozen(newValue)) { // Case 2: New value is an object but current value is not (or doesn't exist) // Create a new proxied object From f27bf3579901ac939de9422672434c34a8b81ec8 Mon Sep 17 00:00:00 2001 From: dmmulroy Date: Fri, 5 Dec 2025 13:11:56 -0500 Subject: [PATCH 2/2] fix: preserve Object.prototype on unwrapped objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changed Object.create(null) to {} so returned objects retain standard prototype methods (hasOwnProperty, toString, etc). Updated tests to verify malicious keys aren't copied as own properties while allowing inherited prototype methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- packages/core/src/persist.test.ts | 18 ++++++++++++------ packages/core/src/persist.ts | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/core/src/persist.test.ts b/packages/core/src/persist.test.ts index 9179f15..0c53d5a 100644 --- a/packages/core/src/persist.test.ts +++ b/packages/core/src/persist.test.ts @@ -9,7 +9,9 @@ describe("unwrapProxy", () => { const result = unwrapProxy(malicious); expect(result.safe).toBe(1); - expect(result.__proto__).toBeUndefined(); + // Malicious __proto__ value not copied as own property + expect(Object.prototype.hasOwnProperty.call(result, '__proto__')).toBe(false); + // Global Object.prototype not polluted expect(({} as Record).polluted).toBeUndefined(); }); @@ -18,7 +20,10 @@ describe("unwrapProxy", () => { const result = unwrapProxy(malicious); expect(result.safe).toBe(1); - expect(result.constructor).toBeUndefined(); + // Malicious constructor value not copied as own property + expect(Object.prototype.hasOwnProperty.call(result, 'constructor')).toBe(false); + // Inherited constructor is standard Object constructor + expect(result.constructor).toBe(Object); }); it("ignores prototype key in proxied objects", () => { @@ -40,14 +45,15 @@ describe("unwrapProxy", () => { expect(result.safe).toBe(1); expect(result.nested.valid).toBe(2); - expect(result.nested.__proto__).toBeUndefined(); + // Malicious __proto__ value not copied as own property + expect(Object.prototype.hasOwnProperty.call(result.nested, '__proto__')).toBe(false); }); - it("proxied object result has null prototype", () => { + it("proxied object result has standard prototype", () => { const input = { a: 1, [IS_PROXIED]: true }; const result = unwrapProxy(input); - expect(Object.getPrototypeOf(result)).toBeNull(); + expect(Object.getPrototypeOf(result)).toBe(Object.prototype); }); it("non-proxied objects returned unchanged (fast path)", () => { @@ -64,7 +70,7 @@ describe("unwrapProxy", () => { expect(result).not.toBe(input); expect(result.outer.inner.val).toBe(1); - expect(Object.getPrototypeOf(result)).toBeNull(); + expect(Object.getPrototypeOf(result)).toBe(Object.prototype); }); it("non-proxied array returned unchanged", () => { diff --git a/packages/core/src/persist.ts b/packages/core/src/persist.ts index 7688b16..2970f3c 100644 --- a/packages/core/src/persist.ts +++ b/packages/core/src/persist.ts @@ -114,7 +114,7 @@ export function unwrapProxy(value: T, seen = new WeakMap()): return result as T; } - const result = Object.create(null) as Record; + const result: Record = {}; seen.set(value, result); for (const key of Object.keys(value)) { if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue;