diff --git a/plans/README.md b/plans/README.md index c263bd73..1bb65f2c 100644 --- a/plans/README.md +++ b/plans/README.md @@ -10,7 +10,7 @@ Generated by the improve skill on 2026-09-01 at commit `2d978f84`. Execute in th | 002 | Require authentication on route-based MCP | P1 | M | 001 | TODO | | 003 | Enforce shared-state exposure policy on direct MCP reads | P1 | S | 002 | TODO | | 004 | Contain remote asset materialization | P1 | S | - | TODO | -| 005 | Block Data Inspector prototype-chain writes | P1 | S | - | TODO | +| 005 | Block Data Inspector prototype-chain writes | P1 | S | - | DONE | | 006 | Validate request-derived authentication-link origins | P1 | M | - | TODO | | 007 | Reject pre-existing symlink escapes from filesystem roots | P2 | M | - | TODO | diff --git a/plugins/data-inspector/src/engine/normalize.ts b/plugins/data-inspector/src/engine/normalize.ts index 1bf750a8..c1045d32 100644 --- a/plugins/data-inspector/src/engine/normalize.ts +++ b/plugins/data-inspector/src/engine/normalize.ts @@ -103,7 +103,11 @@ export function navigate(value: unknown, path: NodePath, options: Pick)[at] + // Own properties only — mirrors the walker, which never descends into + // inherited properties, and keeps live re-navigation off the prototype chain. + cur = cur instanceof Map + ? cur.get(at) + : Object.hasOwn(cur, at) ? (cur as Record)[at] : undefined break case 'i': { const arr = cur as unknown[] diff --git a/plugins/data-inspector/src/engine/write.ts b/plugins/data-inspector/src/engine/write.ts index 423b44d2..dcdcea1c 100644 --- a/plugins/data-inspector/src/engine/write.ts +++ b/plugins/data-inspector/src/engine/write.ts @@ -22,6 +22,29 @@ class WriteError extends Error { } } +/** + * Property names that reach or replace a shared prototype through ordinary + * property access (`__proto__`, `constructor.prototype`, …). Plain-object + * set/add/rename destinations reject these; Map keys are data, not property + * names, and never go through this check. + */ +const UNSAFE_OBJECT_KEYS = new Set(['__proto__', 'prototype', 'constructor']) + +/** Reject a plain-object property name that could reach a shared prototype. */ +function assertSafeObjectKey(key: string): void { + if (UNSAFE_OBJECT_KEYS.has(key)) + throw new WriteError('InvalidKey', `"${key}" is a prototype-sensitive property name`) +} + +/** + * Create an own data property with a plain descriptor, bypassing any setter + * inherited from the prototype chain. Used for every write that introduces a + * property name the target doesn't already own (`add`, `rename`'s new key). + */ +function defineOwnDataProperty(target: object, key: string, value: unknown): void { + Object.defineProperty(target, key, { configurable: true, enumerable: true, writable: true, value }) +} + /** Decode a discriminated wire value into the raw JS value to write. */ function decode(value: WriteValue): unknown { return value.kind === 'undefined' ? undefined : value.value @@ -86,9 +109,15 @@ function setAt(parent: object, seg: PathSegment, value: unknown, opts: WriteAppl } assertMutableObject(parent) const key = at as string - const desc = Object.getOwnPropertyDescriptor(parent, key) - if (desc && !desc.writable && !desc.set) + assertSafeObjectKey(key) + if (!Object.hasOwn(parent, key)) + throw new WriteError('PathNotFound', `property "${key}" does not exist`) + const desc = Object.getOwnPropertyDescriptor(parent, key)! + if (!desc.writable && !desc.set) throw new WriteError('ReadonlyProperty', `property "${key}" has no setter`) + // The property is verified own, so bracket assignment can only run + // this object's own setter (or write its own data slot) — never one + // inherited from a shared prototype. const record = parent as Record record[key] = value return @@ -195,8 +224,11 @@ function addTo(container: object, key: WriteValue | undefined, value: unknown, o const propKey = decodeKey(key, 'add') if (typeof propKey !== 'string') throw new WriteError('InvalidKey', 'an object property key must be a string') - const record = container as Record - record[propKey] = value + assertSafeObjectKey(propKey) + // A fresh own data property, never a bracket assignment: the key is new to + // this object, so assignment would otherwise walk the prototype chain and + // could run an inherited setter. + defineOwnDataProperty(container, propKey, value) } function renameAt(parent: object, seg: PathSegment, newKey: unknown): void { @@ -218,11 +250,14 @@ function renameAt(parent: object, seg: PathSegment, newKey: unknown): void { throw new WriteError('PathNotFound', `property "${key}" does not exist`) if (typeof newKey !== 'string') throw new WriteError('InvalidKey', 'an object property key must be a string') + assertSafeObjectKey(newKey) if (newKey === key) return const value = (parent as Record)[key] delete (parent as Record)[key] - ;(parent as Record)[newKey] = value + // The new key is fresh to this object; define it directly rather than + // assigning through the prototype chain. + defineOwnDataProperty(parent, newKey, value) return } if (kind === 'mk' || kind === 'mv') { diff --git a/plugins/data-inspector/test/write.test.ts b/plugins/data-inspector/test/write.test.ts index 1566b842..03059e87 100644 --- a/plugins/data-inspector/test/write.test.ts +++ b/plugins/data-inspector/test/write.test.ts @@ -181,6 +181,108 @@ describe('applyWrite — rename', () => { }) }) +describe('applyWrite — prototype-chain safety', () => { + const unsafeKeys = ['__proto__', 'prototype', 'constructor'] as const + + it('rejects set of prototype-sensitive destination keys', () => { + for (const key of unsafeKeys) { + const root = {} + const out = applyWrite(root, { op: 'set', path: [['k', key]], value: json({ polluted: true }) }) + expect(out).toMatchObject({ ok: false, error: { name: 'InvalidKey' } }) + } + expect(Object.prototype).not.toHaveProperty('polluted') + }) + + it('rejects add of prototype-sensitive destination keys', () => { + for (const key of unsafeKeys) { + const out = applyWrite({}, { op: 'add', path: [], key: json(key), value: json({ polluted: true }) }) + expect(out).toMatchObject({ ok: false, error: { name: 'InvalidKey' } }) + } + expect(Object.prototype).not.toHaveProperty('polluted') + }) + + it('rejects rename onto a prototype-sensitive destination key', () => { + for (const key of unsafeKeys) { + const root = { a: 1 } + const out = applyWrite(root, { op: 'rename', path: [['k', 'a']], key: json(key) }) + expect(out).toMatchObject({ ok: false, error: { name: 'InvalidKey' } }) + expect(root).toEqual({ a: 1 }) + } + expect(Object.prototype).not.toHaveProperty('polluted') + }) + + it('treats an inherited property as absent, reporting nested set as PathNotFound', () => { + const proto = { shared: { secret: 1 } } + const root = Object.create(proto) as Record + // `shared` is inherited, not an own property of `root`. + const out = applyWrite(root, { op: 'set', path: [['k', 'shared'], ['k', 'secret']], value: json(2) }) + expect(out).toMatchObject({ ok: false, error: { name: 'PathNotFound' } }) + expect(proto.shared.secret).toBe(1) + }) + + it('treats an inherited property as absent, reporting delete/rename as PathNotFound', () => { + const proto = { shared: 1 } + const root = Object.create(proto) as Record + expect(applyWrite(root, { op: 'delete', path: [['k', 'shared']] })).toMatchObject({ ok: false, error: { name: 'PathNotFound' } }) + expect(applyWrite(root, { op: 'rename', path: [['k', 'shared']], key: json('renamed') })).toMatchObject({ ok: false, error: { name: 'PathNotFound' } }) + expect(proto.shared).toBe(1) + }) + + it('add creates an own data property without invoking an inherited setter', () => { + const proto: Record = {} + let setterCalls = 0 + const bumpSetterCalls = () => setterCalls++ + Object.defineProperty(proto, 'name', { configurable: true, enumerable: true, get: () => 'proto-value', set: bumpSetterCalls }) + try { + const root: Record = Object.create(proto) + const out = applyWrite(root, { op: 'add', path: [], key: json('name'), value: json('own-value') }) + expect(out.ok).toBe(true) + expect(setterCalls).toBe(0) + expect(Object.hasOwn(root, 'name')).toBe(true) + expect(root.name).toBe('own-value') + } + finally { + delete proto.name + } + }) + + it('rename creates an own data property at the destination without invoking an inherited setter', () => { + const proto: Record = {} + let setterCalls = 0 + const bumpSetterCalls = () => setterCalls++ + Object.defineProperty(proto, 'name', { configurable: true, enumerable: true, get: () => 'proto-value', set: bumpSetterCalls }) + try { + const root: Record = Object.create(proto) + root.oldKey = 'own-value' + const out = applyWrite(root, { op: 'rename', path: [['k', 'oldKey']], key: json('name') }) + expect(out.ok).toBe(true) + expect(setterCalls).toBe(0) + expect(Object.hasOwn(root, 'name')).toBe(true) + expect(root.name).toBe('own-value') + } + finally { + delete proto.name + } + }) + + it('lets a Map use __proto__/prototype/constructor as ordinary data keys', () => { + const map = new Map() + for (const key of unsafeKeys) { + const out = applyWrite(map, { op: 'add', path: [], key: json(key), value: json(`value:${key}`) }) + expect(out.ok).toBe(true) + } + for (const key of unsafeKeys) + expect(map.get(key)).toBe(`value:${key}`) + + expect(applyWrite(map, { op: 'set', path: [['k', '__proto__']], value: json('updated') })).toMatchObject({ ok: true }) + expect(map.get('__proto__')).toBe('updated') + + expect(applyWrite(map, { op: 'rename', path: [['k', 'prototype']], key: json('renamed-prototype') })).toMatchObject({ ok: true }) + expect(map.get('renamed-prototype')).toBe('value:prototype') + expect(map.has('prototype')).toBe(false) + }) +}) + describe('applyWrite — request typing', () => { it('round-trips through JSON (wire-safety of the request shape)', () => { const request: WriteRequest = { op: 'set', path: [['k', 'a'], ['i', 0]], value: { kind: 'undefined' } }