From 87223760daee4873c6458a4762a336f5edb2d7c1 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Mon, 24 Aug 2026 09:12:10 +0200 Subject: [PATCH] fix: insulate workflow descriptor objects PR9-WF-F1: descriptor construction in workflow.ts::append and the inline objectDefineProperty sites in workflow-transitions.ts::noteRevisionSpan used ordinary prototype-inheriting PropertyDescriptor literals. Under a poisoned inherited Object.prototype.get/set, ToPropertyDescriptor observes the inherited accessor fields and throws TypeError, so workflow evaluation could throw instead of returning the intended deterministic applied state or rejection. Capture Object.setPrototypeOf at module load beside the existing intrinsics and null-prototype each descriptor before the captured Object.defineProperty consumes it. Descriptor flags, index semantics, and revision/sequence ordering are unchanged; the only behavioural change is that prototype-poison-induced TypeError becomes the already-intended fail-closed result. Co-Authored-By: Claude Opus 4.8 --- src/domain/workflow-transitions.ts | 17 +- src/domain/workflow.ts | 13 +- tests/domain/workflow-invariants.test.ts | 189 ++++++++++++++++++++++ tests/domain/workflow-transitions.test.ts | 131 +++++++++++++++ 4 files changed, 344 insertions(+), 6 deletions(-) diff --git a/src/domain/workflow-transitions.ts b/src/domain/workflow-transitions.ts index 6de0b3b..803c73c 100644 --- a/src/domain/workflow-transitions.ts +++ b/src/domain/workflow-transitions.ts @@ -105,6 +105,7 @@ const objectFreeze = Object.freeze; const objectHasOwn = Object.hasOwn; const arrayIsArray = Array.isArray; const objectDefineProperty = Object.defineProperty; +const objectSetPrototypeOf = Object.setPrototypeOf; const objectIsFrozen = Object.isFrozen; const reflectIsExtensible = Reflect.isExtensible; const reflectOwnKeys = Reflect.ownKeys; @@ -318,20 +319,28 @@ function noteRevisionSpan( const low = lowest[index]; const high = highest[index]; if (low !== undefined && sequence < low) { - objectDefineProperty(lowest, index, { + // Null-prototype the descriptor before `defineProperty` reads it, so a + // poisoned inherited `get`/`set` cannot be observed by + // `ToPropertyDescriptor` and turn this stamp into a thrown `TypeError`. + // Same insulation as `workflow.ts::append`, kept inline here on purpose. + const descriptor: PropertyDescriptor = { value: sequence, writable: true, enumerable: true, configurable: true, - }); + }; + objectSetPrototypeOf(descriptor, null); + objectDefineProperty(lowest, index, descriptor); } if (high !== undefined && sequence > high) { - objectDefineProperty(highest, index, { + const descriptor: PropertyDescriptor = { value: sequence, writable: true, enumerable: true, configurable: true, - }); + }; + objectSetPrototypeOf(descriptor, null); + objectDefineProperty(highest, index, descriptor); } return; } diff --git a/src/domain/workflow.ts b/src/domain/workflow.ts index 107058f..bcc4949 100644 --- a/src/domain/workflow.ts +++ b/src/domain/workflow.ts @@ -60,6 +60,7 @@ import type { InvocationReportResult } from './agent-invocation-report.js'; */ const objectFreeze = Object.freeze; const objectDefineProperty = Object.defineProperty; +const objectSetPrototypeOf = Object.setPrototypeOf; const objectHasOwn = Object.hasOwn; const objectIs = Object.is; const numberIsInteger = Number.isInteger; @@ -90,12 +91,20 @@ function containsValue(list: readonly string[], value: unknown): boolean { /** Append by defining an own element, bypassing inherited index setters. */ export function append(list: T[], value: T): void { - objectDefineProperty(list, list.length, { + // The descriptor is null-prototyped before `defineProperty` consumes it. + // `ToPropertyDescriptor` tests `get`/`set` with `HasProperty`, which walks the + // prototype chain: an ordinary literal inherits from `Object.prototype`, so a + // poisoned inherited `get`/`set` would be observed and throw `TypeError` + // instead of appending. Severing the prototype leaves only the own data + // fields visible, so conversion sees exactly what is written here. + const descriptor: PropertyDescriptor = { value, writable: true, enumerable: true, configurable: true, - }); + }; + objectSetPrototypeOf(descriptor, null); + objectDefineProperty(list, list.length, descriptor); } /** diff --git a/tests/domain/workflow-invariants.test.ts b/tests/domain/workflow-invariants.test.ts index d76c55f..f352f90 100644 --- a/tests/domain/workflow-invariants.test.ts +++ b/tests/domain/workflow-invariants.test.ts @@ -27,6 +27,7 @@ import { type WorkflowEvent, type WorkflowState, } from '../../src/domain/index.js'; +import { append } from '../../src/domain/workflow.js'; import { admitEvidence, admitReview, @@ -127,6 +128,46 @@ function withPoisoned(target: object, key: PropertyKey, value: unknown, body: () } } +/** + * Plant inherited accessor fields on `Object.prototype` and restore them exactly. + * + * A poisoned `get`/`set` is what makes `ToPropertyDescriptor` throw on any + * ordinary descriptor literal: it sees an inherited, callable accessor sitting + * beside the literal's own `value`/`writable`, the one combination the intrinsic + * refuses. The installing descriptor is itself null-prototyped, so poisoning + * `set` while `get` is already poisoned does not disrupt the very + * `defineProperty` call that installs it — the harness stays valid under the + * exact condition it exercises, and cannot mask the defect with its own throw. + */ +function withAccessorPoison(keys: readonly PropertyKey[], body: () => void): void { + const proto = Object.prototype; + const saved = keys.map((key) => Object.getOwnPropertyDescriptor(proto, key)); + const poison: PropertyDescriptor = { + value(): unknown { + return undefined; + }, + writable: true, + enumerable: false, + configurable: true, + }; + Object.setPrototypeOf(poison, null); + try { + for (const key of keys) { + Object.defineProperty(proto, key, poison); + } + body(); + } finally { + keys.forEach((key, index) => { + const original = saved[index]; + if (original === undefined) { + Reflect.deleteProperty(proto, key); + } else { + Object.defineProperty(proto, key, original); + } + }); + } +} + describe('group H — provider, purpose, and reported status are inert', () => { /** Replace the three recorded label fields, so only they may differ. */ function withoutLabels(state: WorkflowState): unknown { @@ -2822,3 +2863,151 @@ describe('group M — forbidden vocabulary', () => { } }); }); + +describe('group N — PR9-WF-F1: descriptor objects survive prototype poisoning', () => { + // `append` builds a PropertyDescriptor and hands it to `Object.defineProperty`. + // `ToPropertyDescriptor` probes `get`/`set` with `HasProperty`, which walks the + // prototype chain, so an inherited poison on `Object.prototype` was observed by + // the conversion and threw `TypeError` — turning the layer's intended + // deterministic result into an unexpected throw. Every case here must complete + // without throwing and must leave the realm exactly as it found it. + + // The realm is always restored before any assertion runs: a matcher such as + // `toEqual` builds descriptor objects of its own, which would themselves throw + // under the poison and mask what is being tested (Section 14). Every case + // captures plain values inside the poisoned block and asserts once outside it. + + it('appends under a poisoned Object.prototype.get without throwing', () => { + const list: number[] = []; + withAccessorPoison(['get'], () => { + append(list, 7); + }); + expect(list).toHaveLength(1); + expect(list[0]).toBe(7); + }); + + it('appends under a poisoned Object.prototype.set without throwing', () => { + const list: string[] = []; + withAccessorPoison(['set'], () => { + append(list, 'x'); + }); + expect(list).toEqual(['x']); + }); + + it('appends under a poisoned get *and* set without throwing', () => { + const list: number[] = []; + withAccessorPoison(['get', 'set'], () => { + append(list, 1); + append(list, 2); + }); + expect(list).toEqual([1, 2]); + }); + + it('preserves append index and flag semantics under poison', () => { + const list: number[] = []; + let descriptor: PropertyDescriptor | undefined; + withAccessorPoison(['get', 'set'], () => { + append(list, 42); + descriptor = Object.getOwnPropertyDescriptor(list, 0); + }); + expect(descriptor).toEqual({ + value: 42, + writable: true, + enumerable: true, + configurable: true, + }); + }); + + it.each([['get'], ['set'], ['get', 'set']] as const)( + 'reaches append through applyWorkflowEvent under %s poison and still applies', + (...keys) => { + // `requested()` carries one invocation, so validating it re-runs the + // snapshot append path before the report is applied. Compare against the + // clean evaluation: same input semantics must yield the same output. + const clean = applyWorkflowEvent(requested(), reportInvocation()); + let poisoned: unknown; + + withAccessorPoison([...keys], () => { + poisoned = applyWorkflowEvent(requested(), reportInvocation()); + }); + + expect(poisoned).toEqual(clean); + expect((poisoned as typeof clean).outcome).toBe('APPLIED'); + expect((poisoned as typeof clean).state.invocations[0]?.state).toBe('REPORTED'); + }, + ); + + it('returns the identical prior state on a rejection reached under poison', () => { + const prior = requested(); + let result: ReturnType | undefined; + + withAccessorPoison(['get', 'set'], () => { + // A duplicate invocation id rejects, and the snapshot of `prior` reaches + // `append` on the way to that rejection. + result = applyWorkflowEvent(prior, requestInvocation()); + }); + + expect(result?.outcome).toBe('REJECTED'); + expect(result?.rejection).toBe('DUPLICATE_INVOCATION_ID'); + expect(result?.state).toBe(prior); + }); + + it.each([['get'], ['set']] as const)( + 'survives an Object.prototype.%s poison installed mid-evaluation before a later append', + (key) => { + const proto = Object.prototype; + const saved = Object.getOwnPropertyDescriptor(proto, key); + const base = requested(); + const hostile = { ...base } as Record; + const poison: PropertyDescriptor = { + value(): unknown { + return undefined; + }, + writable: true, + enumerable: false, + configurable: true, + }; + Object.setPrototypeOf(poison, null); + // The bound commit is read early in the snapshot; arming the poison from + // its getter guarantees the poison is live before the invocation list's + // `append` calls run later in the same evaluation. + Object.defineProperty(hostile, 'boundCommitSha', { + get(): string { + Object.defineProperty(proto, key, poison); + return base.boundCommitSha; + }, + enumerable: true, + configurable: true, + }); + + let outcome: string | undefined; + let invocationState: string | undefined; + try { + const result = applyWorkflowEvent( + hostile as unknown as WorkflowState, + reportInvocation(), + ); + outcome = result.outcome; + invocationState = result.state.invocations[0]?.state; + } finally { + if (saved === undefined) { + Reflect.deleteProperty(proto, key); + } else { + Object.defineProperty(proto, key, saved); + } + } + + // Assert only after the realm is restored, so the matcher itself runs + // against a clean `Object.prototype`. + expect(outcome).toBe('APPLIED'); + expect(invocationState).toBe('REPORTED'); + }, + ); + + it('leaves Object.prototype.get and Object.prototype.set untouched afterwards', () => { + // Every case above restores in `finally`; this pins that the realm is clean + // once the group has run, so no later test inherits a poisoned prototype. + expect(Object.getOwnPropertyDescriptor(Object.prototype, 'get')).toBeUndefined(); + expect(Object.getOwnPropertyDescriptor(Object.prototype, 'set')).toBeUndefined(); + }); +}); diff --git a/tests/domain/workflow-transitions.test.ts b/tests/domain/workflow-transitions.test.ts index 8b47a1e..5a4c9df 100644 --- a/tests/domain/workflow-transitions.test.ts +++ b/tests/domain/workflow-transitions.test.ts @@ -1283,3 +1283,134 @@ describe('applyWorkflowEvent — group N, end-to-end lifecycle replay', () => { expect(label(REVIEW_B)).toContain(REVIEW_B); }); }); + +/** + * Plant inherited accessor fields on `Object.prototype`, restoring them exactly. + * + * The installing descriptor is null-prototyped so that poisoning `set` while + * `get` is already poisoned does not disrupt the very `defineProperty` that + * installs it — the harness stays valid under the same condition it exercises. + */ +function withAccessorPoison(keys: readonly PropertyKey[], body: () => void): void { + const proto = Object.prototype; + const saved = keys.map((key) => Object.getOwnPropertyDescriptor(proto, key)); + const poison: PropertyDescriptor = { + value(): unknown { + return undefined; + }, + writable: true, + enumerable: false, + configurable: true, + }; + Object.setPrototypeOf(poison, null); + try { + for (const key of keys) { + Object.defineProperty(proto, key, poison); + } + body(); + } finally { + keys.forEach((key, index) => { + const original = saved[index]; + if (original === undefined) { + Reflect.deleteProperty(proto, key); + } else { + Object.defineProperty(proto, key, original); + } + }); + } +} + +describe('PR9-WF-F1: noteRevisionSpan inline descriptors survive prototype poisoning', () => { + // `noteRevisionSpan` stamps its lowest/highest slots with `Object.defineProperty` + // over an inline descriptor. Those calls are on the public evaluation path: + // `applyWorkflowEvent` -> `snapshotWorkflow` -> `noteRevisionSpan`. An inherited + // `get`/`set` poison made `ToPropertyDescriptor` throw there, so a hostile realm + // turned an intended apply/rejection into an unexpected `TypeError`. + + /** One invocation requested then reported at the same revision. */ + function reportedInvocation(): WorkflowState { + return applyOrThrow(withRequestedInvocation(), reportInvocation()); + } + + /** + * A state whose two same-revision invocation records are ordered so the + * second-listed carries the lower sequence — driving `noteRevisionSpan` + * through its lowest-slot inline descriptor. Built from real transitions, + * then reordered; lists are refrozen to stay faithful to a produced state. + */ + function reachesLowestSpanSite(): WorkflowState { + let state = openedWorkflow(); + state = applyOrThrow(state, requestInvocation(buildInvocation({ invocationId: INVOCATION_A }))); + state = applyOrThrow(state, requestInvocation(buildInvocation({ invocationId: INVOCATION_B }))); + const invocations = Object.freeze([ + Object.freeze({ ...state.invocations[0], requestedAtSequence: 2 }), + Object.freeze({ ...state.invocations[1], requestedAtSequence: 1 }), + ]); + return Object.freeze({ ...state, invocations }) as WorkflowState; + } + + const deeplyFrozen = (state: WorkflowState): boolean => + Object.isFrozen(state) && + Object.isFrozen(state.invocations) && + Object.isFrozen(state.evidence) && + Object.isFrozen(state.reviews); + + it.each([['get'], ['set'], ['get', 'set']] as const)( + 'reaches the highest-slot inline descriptor under %s poison and applies unchanged', + (...keys) => { + const prior = reportedInvocation(); + const clean = applyWorkflowEvent(prior, admitEvidence()); + let poisoned: ReturnType | undefined; + + withAccessorPoison([...keys], () => { + poisoned = applyWorkflowEvent(prior, admitEvidence()); + }); + + expect(poisoned).toEqual(clean); + expect(poisoned?.outcome).toBe('APPLIED'); + // Chronology, revision, and sequence accounting are all unchanged. + expect(poisoned?.state.revision).toBe(clean.state.revision); + expect(poisoned?.state.sequence).toBe(clean.state.sequence); + expect(poisoned ? deeplyFrozen(poisoned.state) : false).toBe(true); + }, + ); + + it.each([['get'], ['set'], ['get', 'set']] as const)( + 'reaches the lowest-slot inline descriptor under %s poison with identical outcome', + (...keys) => { + const prior = reachesLowestSpanSite(); + const clean = applyWorkflowEvent(prior, admitEvidence()); + let poisoned: ReturnType | undefined; + + withAccessorPoison([...keys], () => { + poisoned = applyWorkflowEvent(prior, admitEvidence()); + }); + + // Whether the reordered state reads as applicable or as a deterministic + // rejection, the poisoned run must reproduce the clean run exactly. + expect(poisoned).toEqual(clean); + expect(poisoned?.outcome).toBe(clean.outcome); + expect(poisoned?.rejection).toBe(clean.rejection); + }, + ); + + it('preserves prior-state identity on a rejection reached through noteRevisionSpan', () => { + const prior = reportedInvocation(); + let poisoned: ReturnType | undefined; + + withAccessorPoison(['get', 'set'], () => { + // A duplicate invocation id rejects, but the snapshot of `prior` reaches + // `noteRevisionSpan` first. + poisoned = applyWorkflowEvent(prior, requestInvocation()); + }); + + expect(poisoned?.outcome).toBe('REJECTED'); + expect(poisoned?.rejection).toBe('DUPLICATE_INVOCATION_ID'); + expect(poisoned?.state).toBe(prior); + }); + + it('leaves the realm clean after exercising the inline descriptors', () => { + expect(Object.getOwnPropertyDescriptor(Object.prototype, 'get')).toBeUndefined(); + expect(Object.getOwnPropertyDescriptor(Object.prototype, 'set')).toBeUndefined(); + }); +});