diff --git a/.changeset/proto-key-removal-4672.md b/.changeset/proto-key-removal-4672.md new file mode 100644 index 000000000..905b0f0fb --- /dev/null +++ b/.changeset/proto-key-removal-4672.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +A store whose own `__proto__` data is removed — by `reconcile`, a setter's `delete`, or a projection — now reads `undefined`, as a store that never had it does, for tracked and untracked readers alike (#4672, core R30). The key's node used to be told the backing's inherited value, so the read served `Object.prototype` wrapped as a store. Tracked readers are notified of the change; `__proto__` assignment stays inert. diff --git a/packages/signals/docs/RULES-INDEX.md b/packages/signals/docs/RULES-INDEX.md index 611159b04..324fdb079 100644 --- a/packages/signals/docs/RULES-INDEX.md +++ b/packages/signals/docs/RULES-INDEX.md @@ -28,7 +28,7 @@ Status legend: **live** stated and standing · **ruled** carries an explicit rul | C | 4 | 0 | 3 | 1 | | INV | 11 | 4 | 7 | 4 | | RUL | 13 | 3 | 6 | 6 | -| R (CS) | 59 | 15 | 19 | 30 | +| R (CS) | 59 | 15 | 20 | 30 | | R (OL) | 37 | 4 | 1 | 32 | | R (OS) | 46 | 0 | 0 | 46 | | R (PJ) | 37 | 2 | 5 | 32 | @@ -153,7 +153,7 @@ Status legend: **live** stated and standing · **ruled** carries an explicit rul | CS-R9 | live | `docs/rules-mining/core-store.md:42` | store.ts×2 target.ts×1 | — | Proxy identity per logical slot is stable across writes and reconciles** (mapArray keyed flows reuse rows across refetch/reconcile). | | CS-R10 | live | `docs/rules-mining/core-store.md:48` | — | next-smoke.test.ts×1 | Per-property tracking; same-value writes (direct or functional path setter returning prev) do not re-trigger.** | | CS-R11 | live | `docs/rules-mining/core-store.md:52` | — | visibility-oracle-store.states.ts×3 visibility-oracle.states.ts×2 | Per-path tracking: reading `state.user.firstName` subscribes to that leaf; reading the reference `store[0]` does not subscribe to `store[0].i`.** | -| CS-R12 | live | `docs/rules-mining/core-store.md:56` | store.ts×1 | — | Reading an absent key subscribes to that key: other-key changes don't trigger; defining it later (assignment or defineProperty) does.** | +| CS-R12 | live | `docs/rules-mining/core-store.md:56` | store.ts×1 | proto-key-removal-4672.test.ts×1 | Reading an absent key subscribes to that key: other-key changes don't trigger; defining it later (assignment or defineProperty) does.** | | CS-R13 | live | `docs/rules-mining/core-store.md:60` | store.ts×2 target.ts×1 | — | `in` tracks presence, not value: undefined-write doesn't retrigger; delete does; adding absent key does. `in`/`has` never invokes source getters.** | | CS-R14 | live | `docs/rules-mining/core-store.md:64` | — | — | `Object.keys` / `for…in` subscribe to key-set membership (root and nested) — distinct from property nodes.** Aligned: key-set node. | | CS-R15 | live | `docs/rules-mining/core-store.md:68` | store.ts×1 | — | Array structural tracking is uniform across idioms: indexed length loop, `for…of`, mapArray ($TRACK) all re-run exactly once per flush on add/update/removal.** | @@ -171,7 +171,7 @@ Status legend: **live** stated and standing · **ruled** carries an explicit rul | CS-R27 | live | `docs/rules-mining/core-store.md:121` | store.ts×2 | — | The setter may return a replacement value that swaps the root wholesale; symbol keys on the replacement preserved.** | | CS-R28 | live | `docs/rules-mining/core-store.md:125` | reconcile.ts×1 | — | `storePath` addressing: string keys, numeric indices, index arrays, predicate filters ((value, index)), ranges, trailing functional setters address and update intended paths + trigger per-path subscri… | | CS-R29 | live | `docs/rules-mining/core-store.md:129` | store.ts×1 | overlay.test.ts×1 | Merge/replacement preserve accessor descriptors and keep getters LIVE (re-evaluated per read, reactive reads track), for pre-existing and new keys.** | -| CS-R30 | live | `docs/rules-mining/core-store.md:134` | store.ts×3 | store-prototype-4670.test.ts×2 | Prototype pollution fully guarded: `__proto__` assignment inert; reading `constructor` on the draft returns undefined; storePath refuses `__proto__`/`constructor`/`prototype` segments; skips unsafe ow… | +| CS-R30 | live | `docs/rules-mining/core-store.md:134` | reconcile.ts×1 store.ts×4 | proto-key-removal-4672.test.ts×3 store-prototype-4670.test.ts×2 | Prototype pollution fully guarded: `__proto__` assignment inert; reading `constructor` on the draft returns undefined; storePath refuses `__proto__`/`constructor`/`prototype` segments; skips unsafe ow… | | CS-R31 | live | `docs/rules-mining/core-store.md:138` | store.ts×1 | a34-writes-then-derivations.test.ts×1 createMemo.test.ts×6 derived-write-then-derivation-3733.test.ts×1 held-derivation-not-a-proposal-3612.test.ts×3 createStore.test.ts×3 reconcile-resend-identity.test.ts×1 | Derived-store manual writes apply before the derivation.** A manual `setStore` lands at once, staged with any live transaction. When a source of the fold changes, in the same update or later, the fold… | | CS-R32 | live | `docs/rules-mining/core-store.md:147` | — | visibility-oracle-store.states.ts×1 | A setter-staged replacement followed by reconcile lands the reconciled value — staged writes fold into the diff.** Aligned: O7's resolution (a test already exists). | | CS-R33 | live | `docs/rules-mining/core-store.md:151` | — | visibility-oracle-store.states.ts×4 | Action/async lane semantics on store properties: a write held by an action makes isPending true for that property (per-property, not whole-store) while showing the committed value; applies on settle.*… | diff --git a/packages/signals/src/store/reconcile.ts b/packages/signals/src/store/reconcile.ts index ea52e4d27..1eed03ee3 100644 --- a/packages/signals/src/store/reconcile.ts +++ b/packages/signals/src/store/reconcile.ts @@ -272,7 +272,16 @@ function applyAdopt(t: StoreTarget, incoming: any, keyFn: KeyFn | null, proj = f // Keys the incoming object no longer has still have nodes. if (nodes !== null && nodesHit < t.nc) { for (const key of Reflect.ownKeys(nodes)) { - if (!hasOwnP.call(incoming, key)) notifyKeyDiff(nodes[key as any], key, old, incoming, false); + // A removed `__proto__` reads absent, not the prototype (core R30, + // #4672). + if (!hasOwnP.call(incoming, key)) + notifyKeyDiff( + nodes[key as any], + key, + old, + key === "__proto__" ? Object.create(null) : incoming, + false + ); } } notifyFoldTail(t, old, incoming); diff --git a/packages/signals/src/store/store.ts b/packages/signals/src/store/store.ts index bf69eeaea..ade047728 100644 --- a/packages/signals/src/store/store.ts +++ b/packages/signals/src/store/store.ts @@ -1220,9 +1220,10 @@ function notifyWrites(t: StoreTarget): void { for (const key of keys) { const node = nodes[key as any]; if (node === undefined) continue; + let own = false; if ( (node as any).acc === true || - (hasOwn.call(pb, key) && lookupGetter.call(pb, key) !== undefined) + ((own = hasOwn.call(pb, key)) && lookupGetter.call(pb, key) !== undefined) ) { // Accessor keys: the node is linked for shape-change notification, // its value is never served (the getter runs with the proxy @@ -1241,7 +1242,10 @@ function notifyWrites(t: StoreTarget): void { if (od?.get || od?.set || !isEqual(od?.value, nd.value)) setSignal(node, () => nd.value); continue; } - const nv = t.del?.has(key) ? undefined : pb[key as any]; + // A removed `__proto__` is told `undefined`, never the prototype (core + // R30, #4672); an own key pays nothing. (An overlay's written keys are + // own on it or in `del`.) + const nv = own || !(t.del?.has(key) || key === "__proto__") ? pb[key as any] : undefined; setSignal(node, () => nv); if (t._fam !== null) { // The derive's write is a derivation; a user setter's is a proposal diff --git a/packages/signals/tests/store/proto-key-removal-4672.test.ts b/packages/signals/tests/store/proto-key-removal-4672.test.ts new file mode 100644 index 000000000..bdf162e3d --- /dev/null +++ b/packages/signals/tests/store/proto-key-removal-4672.test.ts @@ -0,0 +1,236 @@ +/** + * #4672 — a store with own `__proto__` data (JSON rows can carry it) whose + * data was then removed read `Object.prototype` through the store: a + * store-wrapped object with 12 own keys, to tracked and untracked readers + * alike. The key's node had been told the backing's inherited value. + * + * Core R30: an inherited `__proto__` is never served, so once the own data is + * gone the read answers what a store that never had it answers: `undefined`. + * F-10: the change is visible at the flush to every channel, so tracked + * readers re-run onto it. R30 also keeps `__proto__` assignment inert, so a + * write after the removal neither lands nor pollutes `Object.prototype`. + */ +import { describe, expect, it } from "vitest"; +import { + createProjection, + createRenderEffect, + createRoot, + createStore, + flush, + reconcile +} from "../../src/index.js"; + +const withOwn = (key: string, value: unknown, base: Record = { id: "a" }) => { + const o: any = { ...base }; + Object.defineProperty(o, key, { value, enumerable: true, writable: true, configurable: true }); + return o; +}; + +function track(read: () => unknown) { + const seen: unknown[] = []; + createRoot(() => + createRenderEffect(read, v => { + seen.push(v); + }) + ); + flush(); + return seen; +} + +const removals: Record { q: any; remove: () => void }> = { + reconcile: key => { + const [q, setQ] = createStore(withOwn(key, 0)); + return { q, remove: () => setQ(reconcile({ id: "a" }, () => undefined) as any) }; + }, + "setter delete": key => { + const [q, setQ] = createStore(withOwn(key, 0)); + return { + q, + remove: () => + setQ((s: any) => { + delete s[key]; + }) + }; + }, + "projection draft delete": key => { + const [src, setSrc] = createStore({ has: true }); + const q = createProjection( + (draft: any) => { + if (src.has) + Object.defineProperty(draft, key, { + value: 0, + enumerable: true, + writable: true, + configurable: true + }); + else delete draft[key]; + }, + withOwn(key, 0) + ); + return { + q, + remove: () => + setSrc(s => { + s.has = false; + }) + }; + }, + "projection returned value": key => { + const [src, setSrc] = createStore({ has: true }); + const q = createProjection(() => (src.has ? withOwn(key, 0) : { id: "a" }), { id: "a" }); + return { + q, + remove: () => + setSrc(s => { + s.has = false; + }) + }; + }, + "derived store returned value": key => { + const [src, setSrc] = createStore({ has: true }); + const [q] = createStore(() => (src.has ? withOwn(key, 0) : { id: "a" }), { id: "a" }); + return { + q, + remove: () => + setSrc(s => { + s.has = false; + }) + }; + } +}; + +describe("#4672 own __proto__ data removed", () => { + it("the issue's repro: reconcile removal reads undefined, tracked and untracked", () => { + const [q, setQ] = createStore(withOwn("__proto__", 0)); + const seen = track(() => q["__proto__"]); + expect(q["__proto__"]).toBe(0); + expect(seen).toEqual([0]); + + setQ(reconcile({ id: "a" }, () => undefined) as any); + flush(); + + expect(q["__proto__"]).toBeUndefined(); + expect(seen).toEqual([0, undefined]); + expect(Object.keys(q)).toEqual(["id"]); + }); + + it("matches a store that never had own __proto__ data", () => { + const [never] = createStore({ id: "a" }); + const seen = track(() => never["__proto__"]); + expect(never["__proto__"]).toBeUndefined(); + expect(seen).toEqual([undefined]); + }); + + for (const [path, make] of Object.entries(removals)) { + it(`${path}: reads undefined and tells the tracked reader`, () => { + const { q, remove } = make("__proto__"); + const seen = track(() => q["__proto__"]); + expect(q["__proto__"]).toBe(0); + remove(); + flush(); + expect(q["__proto__"]).toBeUndefined(); + expect(seen).toEqual([0, undefined]); + expect(Object.keys(q)).toEqual(["id"]); + }); + + it(`${path}: with no tracked reader reads undefined`, () => { + const { q, remove } = make("__proto__"); + expect(q["__proto__"]).toBe(0); + remove(); + flush(); + expect(q["__proto__"]).toBeUndefined(); + }); + } + + it("setting it to undefined is inert (R30): the own data stays", () => { + const [q, setQ] = createStore(withOwn("__proto__", 0)); + const seen = track(() => q["__proto__"]); + setQ((s: any) => { + s["__proto__"] = undefined; + }); + flush(); + expect(q["__proto__"]).toBe(0); + expect(seen).toEqual([0]); + }); + + it("a write after removal stays inert and pollutes nothing", () => { + const [q, setQ] = createStore(withOwn("__proto__", 0)); + const seen = track(() => q["__proto__"]); + setQ((s: any) => { + delete s["__proto__"]; + }); + flush(); + setQ((s: any) => { + s["__proto__"] = { polluted4672: true }; + }); + flush(); + expect(q["__proto__"]).toBeUndefined(); + expect(seen).toEqual([0, undefined]); + expect(({} as any).polluted4672).toBeUndefined(); + }); + + it("a wide record (overlay pending backing): own __proto__ survives sibling writes, then removal reads undefined", () => { + const base: Record = {}; + for (let i = 0; i < 40; i++) base["k" + i] = i; + const [q, setQ] = createStore(withOwn("__proto__", 0, base)); + const seen = track(() => [q["__proto__"], q.k1]); + for (let r = 0; r < 3; r++) { + setQ((s: any) => { + s.k1 = 100 + r; + }); + flush(); + } + expect(q["__proto__"]).toBe(0); + setQ((s: any) => { + delete s["__proto__"]; + s.k2 = 5; + }); + flush(); + expect(q["__proto__"]).toBeUndefined(); + setQ((s: any) => { + s.k3 = 5; + }); + flush(); + expect(q["__proto__"]).toBeUndefined(); + expect(seen.map((v: any) => v[0])).toEqual([0, 0, 0, 0, undefined]); + }); + + // The reverse direction is outside #4672: a tracked reader of an absent + // `__proto__` gets no node (the get trap answers it before subscribing), so + // own data landing later reaches untracked reads only. Pinned as it stands. + it("reverse, untracked: own __proto__ data landing reads as data", () => { + const [q, setQ] = createStore({ id: "a" }); + setQ(reconcile(withOwn("__proto__", 0), () => undefined) as any); + flush(); + expect(q["__proto__"]).toBe(0); + }); + + it.fails("reverse, tracked: own __proto__ data landing reaches the tracked reader (R12)", () => { + const [q, setQ] = createStore({ id: "a" }); + const seen = track(() => q["__proto__"]); + setQ(reconcile(withOwn("__proto__", 0), () => undefined) as any); + flush(); + expect(seen).toEqual([undefined, 0]); + }); + + // Pins for the sibling keys (#4674 ruling): a removed own `constructor` + // reads the inherited class, as an object that never had one does; own + // `prototype` data never leaked (no built-in prototype has one). + it("pin: a removed own constructor reads the inherited class", () => { + const { q, remove } = removals.reconcile("constructor"); + const seen = track(() => q.constructor); + remove(); + flush(); + expect(q.constructor).toBe(Object); + expect(seen).toEqual([0, Object]); + }); + + it("pin: a removed own prototype reads undefined", () => { + const { q, remove } = removals["setter delete"]("prototype"); + const seen = track(() => q.prototype); + remove(); + flush(); + expect(q.prototype).toBeUndefined(); + expect(seen).toEqual([0, undefined]); + }); +});