Skip to content

Commit e75c9ac

Browse files
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]>
1 parent bce4f3f commit e75c9ac

File tree

5 files changed

+73
-5
lines changed

5 files changed

+73
-5
lines changed

.changeset/tidy-starfishes-allow.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"svelte": patch
3+
---
4+
5+
fix: only reuse state proxies that belong to the current value

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ export const READONLY_SYMBOL = Symbol('readonly');
3434
export function proxy(value, immutable = true) {
3535
if (typeof value === 'object' && value != null && !is_frozen(value)) {
3636
if (STATE_SYMBOL in value) {
37-
return /** @type {import('./types.js').ProxyMetadata<T>} */ (value[STATE_SYMBOL]).p;
37+
const metadata = /** @type {import('./types.js').ProxyMetadata<T>} */ (value[STATE_SYMBOL]);
38+
// Check that the incoming value is the same proxy that this state symbol was created for:
39+
// If someone copies over the state symbol to a new object (using Reflect.ownKeys) the referenced
40+
// proxy could be stale and we should not return it.
41+
if (metadata.t === value || metadata.p === value) return metadata.p;
3842
}
3943

4044
const prototype = get_prototype_of(value);
@@ -51,7 +55,8 @@ export function proxy(value, immutable = true) {
5155
/** @type {import('./types.js').ProxyStateObject<T>} */ (proxy),
5256
immutable
5357
),
54-
writable: false
58+
writable: true,
59+
enumerable: false
5560
});
5661

5762
return proxy;
@@ -68,7 +73,7 @@ export function proxy(value, immutable = true) {
6873
* @returns {Record<string | symbol, any>}
6974
*/
7075
function unwrap(value, already_unwrapped = new Map()) {
71-
if (typeof value === 'object' && value != null && !is_frozen(value) && STATE_SYMBOL in value) {
76+
if (typeof value === 'object' && value != null && STATE_SYMBOL in value) {
7277
const unwrapped = already_unwrapped.get(value);
7378
if (unwrapped !== undefined) {
7479
return unwrapped;
@@ -100,6 +105,7 @@ function unwrap(value, already_unwrapped = new Map()) {
100105
return obj;
101106
}
102107
}
108+
103109
return value;
104110
}
105111

@@ -124,7 +130,8 @@ function init(value, proxy, immutable) {
124130
v: source(0),
125131
a: is_array(value),
126132
i: immutable,
127-
p: proxy
133+
p: proxy,
134+
t: value
128135
};
129136
}
130137

@@ -171,6 +178,10 @@ const state_proxy_handler = {
171178
if (DEV && prop === READONLY_SYMBOL) {
172179
return Reflect.get(target, READONLY_SYMBOL);
173180
}
181+
if (prop === STATE_SYMBOL) {
182+
return Reflect.get(target, STATE_SYMBOL);
183+
}
184+
174185
const metadata = target[STATE_SYMBOL];
175186
let s = metadata.s.get(prop);
176187

@@ -304,7 +315,13 @@ if (DEV) {
304315
*/
305316
export function readonly(value) {
306317
const proxy = value && value[READONLY_SYMBOL];
307-
if (proxy) return proxy;
318+
if (proxy) {
319+
const metadata = value[STATE_SYMBOL];
320+
// Check that the incoming value is the same proxy that this readonly symbol was created for:
321+
// If someone copies over the readonly symbol to a new object (using Reflect.ownKeys) the referenced
322+
// proxy could be stale and we should not return it.
323+
if (metadata.p === value) return proxy;
324+
}
308325

309326
if (
310327
typeof value === 'object' &&

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,8 @@ export interface ProxyMetadata<T = Record<string | symbol, any>> {
410410
i: boolean;
411411
/** The associated proxy */
412412
p: ProxyStateObject<T> | ProxyReadonlyObject<T>;
413+
/** The original target this proxy was created for */
414+
t: T;
413415
}
414416

415417
export type ProxyStateObject<T = Record<string | symbol, any>> = T & {
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import { test } from '../../test';
2+
3+
export default test({
4+
compileOptions: {
5+
dev: true
6+
},
7+
8+
html: `<button>state1.value: a state2.value: a</button>`,
9+
10+
async test({ assert, target }) {
11+
const btn = target.querySelector('button');
12+
13+
await btn?.click();
14+
assert.htmlEqual(target.innerHTML, `<button>state1.value: b state2.value: b</button>`);
15+
}
16+
});
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<script>
2+
let foo = { value: 'a' }
3+
let state1 = $state(foo);
4+
let state2 = $state(foo);
5+
</script>
6+
7+
<button onclick={() => {
8+
let new_state1 = {};
9+
let new_state2 = {};
10+
// This contains Symbol.$state and Symbol.$readonly and we can't do anything against it,
11+
// because it's called on the original object, not our state proxy
12+
Reflect.ownKeys(foo).forEach(k => {
13+
new_state1[k] = foo[k];
14+
new_state2[k] = foo[k];
15+
});
16+
new_state1.value = 'b';
17+
new_state2.value = 'b';
18+
// $.proxy will see that Symbol.$state exists on this object already, which shouldn't result in a stale value
19+
state1 = new_state1;
20+
// $.proxy can't look into Symbol.$state because of the frozen object
21+
state2 = Object.freeze(new_state2);
22+
}}
23+
>
24+
state1.value: {state1.value}
25+
state2.value: {state2.value}
26+
</button>
27+
28+

0 commit comments

Comments
 (0)