Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plans/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

Expand Down
6 changes: 5 additions & 1 deletion plugins/data-inspector/src/engine/normalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ export function navigate(value: unknown, path: NodePath, options: Pick<Normalize
return undefined
switch (kind) {
case 'k':
cur = cur instanceof Map ? cur.get(at) : (cur as Record<string, unknown>)[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<string, unknown>)[at] : undefined
break
case 'i': {
const arr = cur as unknown[]
Expand Down
45 changes: 40 additions & 5 deletions plugins/data-inspector/src/engine/write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<string, unknown>
record[key] = value
return
Expand Down Expand Up @@ -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<string, unknown>
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 {
Expand All @@ -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<string, unknown>)[key]
delete (parent as Record<string, unknown>)[key]
;(parent as Record<string, unknown>)[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') {
Expand Down
102 changes: 102 additions & 0 deletions plugins/data-inspector/test/write.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>
// `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<string, unknown>
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<string, unknown> = {}
let setterCalls = 0
const bumpSetterCalls = () => setterCalls++
Object.defineProperty(proto, 'name', { configurable: true, enumerable: true, get: () => 'proto-value', set: bumpSetterCalls })
try {
const root: Record<string, unknown> = 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<string, unknown> = {}
let setterCalls = 0
const bumpSetterCalls = () => setterCalls++
Object.defineProperty(proto, 'name', { configurable: true, enumerable: true, get: () => 'proto-value', set: bumpSetterCalls })
try {
const root: Record<string, unknown> = 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<unknown, unknown>()
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' } }
Expand Down
Loading