fix: don't reuse proxies when state symbol refers to stale value (#10343)

* fix: don't reuse proxies when state symbol refers to stale value

When somebody copies over the state symbol property onto a new object (you can retrieve the symbol by using `Reflect.ownKeys(...)`), it was wrongfully assumed that it always relates to the current value. This PR adds an additional check that this is actually the case.
This also adds a dev time warning when an object is frozen but contains a state property, which hints at a bug in user land.
fixes #10316

* lint

* rename

* remove warning

* update test

* changeset

---------

Co-authored-by: Rich Harris <[email protected]>
Co-authored-by: Rich Harris <[email protected]>

Author: dummdidumm

.changeset/tidy-starfishes-allow.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: only reuse state proxies that belong to the current value

packages/svelte/src/internal/client/proxy.js

@@ -34,7 +34,11 @@ export const READONLY_SYMBOL = Symbol('readonly');
 export function proxy(value, immutable = true) {
 	if (typeof value === 'object' && value != null && !is_frozen(value)) {
 		if (STATE_SYMBOL in value) {
-			return /** @type {import('./types.js').ProxyMetadata<T>} */ (value[STATE_SYMBOL]).p;
+			const metadata = /** @type {import('./types.js').ProxyMetadata<T>} */ (value[STATE_SYMBOL]);
+			// Check that the incoming value is the same proxy that this state symbol was created for:
+			// If someone copies over the state symbol to a new object (using Reflect.ownKeys) the referenced
+			// proxy could be stale and we should not return it.
+			if (metadata.t === value || metadata.p === value) return metadata.p;
 		}
 
 		const prototype = get_prototype_of(value);
@@ -51,7 +55,8 @@ export function proxy(value, immutable = true) {
 					/** @type {import('./types.js').ProxyStateObject<T>} */ (proxy),
 					immutable
 				),
-				writable: false
+				writable: true,
+				enumerable: false
 			});
 
 			return proxy;
@@ -68,7 +73,7 @@ export function proxy(value, immutable = true) {
  * @returns {Record<string | symbol, any>}
  */
 function unwrap(value, already_unwrapped = new Map()) {
-	if (typeof value === 'object' && value != null && !is_frozen(value) && STATE_SYMBOL in value) {
+	if (typeof value === 'object' && value != null && STATE_SYMBOL in value) {
 		const unwrapped = already_unwrapped.get(value);
 		if (unwrapped !== undefined) {
 			return unwrapped;
@@ -100,6 +105,7 @@ function unwrap(value, already_unwrapped = new Map()) {
 			return obj;
 		}
 	}
+
 	return value;
 }
 
@@ -124,7 +130,8 @@ function init(value, proxy, immutable) {
 		v: source(0),
 		a: is_array(value),
 		i: immutable,
-		p: proxy
+		p: proxy,
+		t: value
 	};
 }
 
@@ -171,6 +178,10 @@ const state_proxy_handler = {
 		if (DEV && prop === READONLY_SYMBOL) {
 			return Reflect.get(target, READONLY_SYMBOL);
 		}
+		if (prop === STATE_SYMBOL) {
+			return Reflect.get(target, STATE_SYMBOL);
+		}
+
 		const metadata = target[STATE_SYMBOL];
 		let s = metadata.s.get(prop);
 
@@ -304,7 +315,13 @@ if (DEV) {
  */
 export function readonly(value) {
 	const proxy = value && value[READONLY_SYMBOL];
-	if (proxy) return proxy;
+	if (proxy) {
+		const metadata = value[STATE_SYMBOL];
+		// Check that the incoming value is the same proxy that this readonly symbol was created for:
+		// If someone copies over the readonly symbol to a new object (using Reflect.ownKeys) the referenced
+		// proxy could be stale and we should not return it.
+		if (metadata.p === value) return proxy;
+	}
 
 	if (
 		typeof value === 'object' &&

packages/svelte/src/internal/client/types.d.ts

@@ -410,6 +410,8 @@ export interface ProxyMetadata<T = Record<string | symbol, any>> {
 	i: boolean;
 	/** The associated proxy */
 	p: ProxyStateObject<T> | ProxyReadonlyObject<T>;
+	/** The original target this proxy was created for */
+	t: T;
 }
 
 export type ProxyStateObject<T = Record<string | symbol, any>> = T & {

packages/svelte/tests/runtime-runes/samples/state-reuse/_config.js

@@ -0,0 +1,16 @@
+import { test } from '../../test';
+
+export default test({
+	compileOptions: {
+		dev: true
+	},
+
+	html: `<button>state1.value: a state2.value: a</button>`,
+
+	async test({ assert, target }) {
+		const btn = target.querySelector('button');
+
+		await btn?.click();
+		assert.htmlEqual(target.innerHTML, `<button>state1.value: b state2.value: b</button>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/state-reuse/main.svelte

@@ -0,0 +1,28 @@
+<script>
+	let foo = { value: 'a' }
+	let state1 = $state(foo);
+	let state2 = $state(foo);
+</script>
+
+<button onclick={() => {
+		let new_state1 = {};
+		let new_state2 = {};
+		// This contains Symbol.$state and Symbol.$readonly and we can't do anything against it,
+		// because it's called on the original object, not our state proxy
+		Reflect.ownKeys(foo).forEach(k => {
+			new_state1[k] = foo[k];
+			new_state2[k] = foo[k];
+		});
+		new_state1.value = 'b';
+		new_state2.value = 'b';
+		// $.proxy will see that Symbol.$state exists on this object already, which shouldn't result in a stale value
+		state1 = new_state1;
+		// $.proxy can't look into Symbol.$state because of the frozen object
+		state2 = Object.freeze(new_state2);
+	}}
+>
+state1.value: {state1.value}
+state2.value: {state2.value}
+</button>
+
+