From 14b4fb3674c6409bc29ca2061221e0a5b2030b7a Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Sun, 16 Aug 2026 06:32:14 +0200 Subject: [PATCH] fix: reject inherited repair list elements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readList` obtained each authorization-list entry with an ordinary indexed read, which walks the prototype chain. At a sparse hole that resolved whatever a custom array prototype — or `Array.prototype` itself — carried at that numeric key, so a value the operator never supplied could enter the trusted `RepairJobAuthorization` snapshot as an authorized path or command class and reach `ALLOW_ONCE` with an `ExecutionPermit` bound to the fabricated operand. Entries are now obtained through `readOwnElement`, which gates the read behind the module's already-captured `Object.hasOwn` and reports absence with a module-private sentinel rather than collapsing it into `undefined`, so the list refuses a missing element itself instead of relying on the element reader. A sparse hole rejects the whole list: never skipped, defaulted, or filled from the prototype chain. Dense own lists are unaffected. The guarantee is documented at the strength the code proves. It holds for any array whose own-property introspection is truthful; a Proxy defines the observable result of both the own check and the read, so one that misreports ownership can still pass an inherited value through. That widens nothing — such a caller can supply the same value as a dense own element — and the comment and architecture text now say so rather than claiming an atomic observation. The sentinel is a bare object literal, so it adds no call into a mutable global and keeps the module's captured-intrinsic discipline. Merge stays OPERATOR_REQUIRED, auto-merge stays DENY, and C1-A01 and C1-A02 are untouched. Co-Authored-By: Claude Opus 5 (1M context) --- docs/architecture/C1-repair-job-authority.md | 25 +- src/domain/repair-job.ts | 83 ++++- .../job-authorization-invariants.test.ts | 317 +++++++++++++++++- 3 files changed, 416 insertions(+), 9 deletions(-) diff --git a/docs/architecture/C1-repair-job-authority.md b/docs/architecture/C1-repair-job-authority.md index 582402e..a83c7b1 100644 --- a/docs/architecture/C1-repair-job-authority.md +++ b/docs/architecture/C1-repair-job-authority.md @@ -522,8 +522,29 @@ patterns PR 004, PR 005, and PR 006 established: real object and a cut branch name can name a different ref. **C1 truncates nothing at all, because C1 has no prose field.** An oversized list is likewise rejected, not shortened. -- **Sparse arrays reject.** A hole reads as `undefined`, which no reader accepts, - so sparseness rejects rather than collapsing. +- **List entries are own elements.** Every entry of an authorization list is + read as an **own** indexed property, so only an element the supplied object + reports as its own can become an authorized path or command class. For any + array whose own-property introspection is truthful — every ordinary array, + however its prototype chain is arranged — that is exactly the elements the job + configuration supplied: an index with no own element is a sparse hole, and a + hole rejects the whole list rather than collapsing, shortening, or taking a + default, so an inherited numeric property planted on a custom array prototype + or on `Array.prototype` is refused however well-formed its value looks. + Provenance decides, not shape. A hole is not authorization, and prototype + state is not authorization. + + The guarantee stops where the runtime's own-property report does, and the + boundary is documented rather than papered over. A Proxy *defines* the + observable result of `Object.hasOwn` and of the subsequent read, so one whose + `getOwnPropertyDescriptor` trap claims a hole is own while the read forwards + through the target's prototype will pass an inherited value through. The + reader performs one own check and one guarded read and re-validates nothing + afterwards, but those are two observations rather than one atomic one, and a + Proxy may answer them inconsistently. C1 establishes provenance no further + than the supplied object's own report, and claims no more. This widens no + authority: a caller able to supply such a Proxy can supply the same value as a + dense own element instead, which is configuration, not an attack. - **Array building avoids the prototype.** Appends define an own indexed property rather than using `push` or indexed assignment, so an inherited index setter is not on the path. diff --git a/src/domain/repair-job.ts b/src/domain/repair-job.ts index af9715c..4a39216 100644 --- a/src/domain/repair-job.ts +++ b/src/domain/repair-job.ts @@ -106,6 +106,70 @@ export function readOwnProperty(target: object, key: string): unknown { } } +/** + * Absence marker for {@link readOwnElement}. + * + * A bare object literal, so producing it calls no global function: this module + * captures every intrinsic it relies on at load, and a sentinel is not a reason + * to add a fresh dependency on a mutable global. It is used only through + * reference identity, never inspected, and never frozen because nothing reads + * it. + * + * The claim is exactly this and no more: **the reference is module-private.** + * It is not exported and no entry point returns it, so it is not among the + * values a caller ordinarily has to hand. That is why `undefined` was not used + * instead — `undefined` is also a legitimate, and rejected, element *value*, + * and an authorization list must refuse a missing element on its own rather + * than depend on the element reader to refuse whatever turned up. + */ +const NO_OWN_ELEMENT = {}; + +/** + * Read one **own** indexed element of an untrusted array. + * + * The same own-only discipline as {@link readOwnProperty}, through the same + * captured `Object.hasOwn`, except that absence is reported as + * {@link NO_OWN_ELEMENT} instead of collapsing into `undefined`. + * + * An ordinary `elements[index]` walks the prototype chain, so at a sparse hole + * it resolves whatever a custom array prototype — or `Array.prototype` itself — + * carries at that numeric key. Provenance is what decides authority here, not + * the value's shape: an inherited entry can be a perfectly well-formed + * repository path or a genuine command class, and a value nobody put in the + * operator's array is still not authorization. + * + * **What this proves, exactly:** for any array whose own-property introspection + * is truthful — every ordinary array, however its prototype chain is arranged — + * a sparse hole and an inherited numeric property are both refused, because + * `Object.hasOwn` answers `false` and the index is never read at all. + * + * **What it does not prove**, and must not be claimed to: that a value which + * survives came from a real own element. A Proxy *defines* the observable + * result of both operations, so one whose `getOwnPropertyDescriptor` trap + * claims an index is own while the read forwards through the target's + * prototype will pass an inherited value through. There is one own check and + * one read, and nothing re-validates afterwards — but they are two separate + * observations, not one atomic one, and a Proxy may answer them + * inconsistently. C1 takes an object's own-property report at face value and + * establishes provenance no further than that report. This grants no authority + * a caller did not already have: anyone able to supply such a Proxy can supply + * the same value as a dense own element instead, which is not an attack but a + * configuration. + * + * Both operations are guarded, because a getter or a Proxy trap may throw. + * Either way the answer is absence, never an exception. + */ +function readOwnElement(elements: object, index: number): unknown { + try { + if (!objectHasOwn(elements, index)) { + return NO_OWN_ELEMENT; + } + return (elements as Record)[index]; + } catch { + return NO_OWN_ELEMENT; + } +} + /** V1 bounds. Every unbounded dimension is capped before iteration. */ export const JOB_BOUNDS = objectFreeze({ /** @@ -331,8 +395,15 @@ export function readRepositoryRelativePath(value: unknown): string | null { * wrote, and silently keeping a prefix of a list an operator got wrong is not * an improvement on refusing it. * - * A sparse array yields `undefined` at the holes, which no reader accepts, so - * sparseness rejects rather than collapsing. + * Every entry is obtained through {@link readOwnElement}, so a list entry can + * only ever be one the supplied object reports as its **own**. For an ordinary + * array that is exactly the elements the job configuration actually supplied: + * an index with no own element is a sparse hole, and a hole rejects the whole + * list — it is never skipped, defaulted, or filled from the prototype chain — + * so an inherited numeric property planted on a custom array prototype or on + * `Array.prototype` is refused however well-formed its value looks. A Proxy + * that misreports ownership is the documented limit of that guarantee, and it + * widens nothing; see {@link readOwnElement}. */ function readList( value: unknown, @@ -367,10 +438,10 @@ function readList( const parsed: T[] = []; for (let index = 0; index < rawLength; index += 1) { - let element: unknown; - try { - element = elements[index]; - } catch { + const element = readOwnElement(elements, index); + if (element === NO_OWN_ELEMENT) { + // A sparse hole, or an own-check or read that threw. Either way this + // index carries no own element, so the whole list is refused. return null; } const value_ = read(element); diff --git a/tests/domain/job-authorization-invariants.test.ts b/tests/domain/job-authorization-invariants.test.ts index 24f2a16..60e2ce1 100644 --- a/tests/domain/job-authorization-invariants.test.ts +++ b/tests/domain/job-authorization-invariants.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { APPROVAL_STATE, @@ -18,11 +18,14 @@ import { resolveJobOperation, satisfiesIndependentValidator, UNKNOWN_JOB_OPERATION, + findInvalidRepairJobFields, + readRepairJobAuthorization, type ApprovalRecord, type JobOperationRequest, type OperatorMergeAuthorization, type RepairJobAuthorization, type ValidatorClaim, + type VerificationCommandClass, } from '../../src/domain/index.js'; import { AUTHORIZED_PATH, @@ -898,6 +901,318 @@ describe('poisoning Map.prototype.get cannot re-resolve an operation', () => { }); }); +/** + * Build a length-2 array holding `own` at index 0 and a genuine **hole** at 1. + * + * `Array.isArray` still answers `true` and `length` is still 2, so every bound + * and shape check upstream is satisfied; the only thing wrong with index 1 is + * that nothing ever put an own element there. + */ +function withHoleAtOne(own: string): string[] { + const sparse: string[] = []; + sparse[0] = own; + sparse.length = 2; + return sparse; +} + +/** The same hole, with `victim` reachable at index 1 through a custom prototype. */ +function sparseWithInheritedElement(own: string, victim: string): string[] { + const sparse = withHoleAtOne(own); + Object.setPrototypeOf(sparse, { 1: victim }); + return sparse; +} + +/** Run `body` with `victim` planted at `Array.prototype[1]`, then restore. */ +function withArrayPrototypeElement(victim: string, body: () => T): T { + const saved = Object.getOwnPropertyDescriptor(Array.prototype, 1); + Object.defineProperty(Array.prototype, 1, { + value: victim, + writable: true, + configurable: true, + enumerable: false, + }); + try { + return body(); + } finally { + if (saved === undefined) { + Reflect.deleteProperty(Array.prototype, 1); + } else { + Object.defineProperty(Array.prototype, 1, saved); + } + } +} + +describe('an inherited numeric property cannot become an authorization entry', () => { + it('proves the custom-prototype setup is actually active', () => { + // Without this the section could pass vacuously, on an array that was never + // hostile: the hole must be a hole, and the inherited value must be there. + const sparse = sparseWithInheritedElement(AUTHORIZED_PATH, UNAUTHORIZED_PATH); + + expect(Array.isArray(sparse)).toBe(true); + expect(sparse.length).toBe(2); + expect(Object.hasOwn(sparse, 1)).toBe(false); + // An ordinary indexed read — the thing the reader must not do — resolves it. + expect(sparse[1]).toBe(UNAUTHORIZED_PATH); + }); + + it('rejects an authorized-path list whose hole is filled by a custom prototype', () => { + const job = buildJob({ + authorizedPaths: sparseWithInheritedElement(AUTHORIZED_PATH, UNAUTHORIZED_PATH), + }); + + expect(readRepairJobAuthorization(job).snapshot).toBeNull(); + expect(findInvalidRepairJobFields(job)).toContain('authorizedPaths'); + }); + + it('does not let a custom-prototype path reach ALLOW_ONCE or a permit', () => { + const job = buildJob({ + authorizedPaths: sparseWithInheritedElement(AUTHORIZED_PATH, UNAUTHORIZED_PATH), + }); + + const decision = authorizeJobOperation(job, buildEdit({ path: UNAUTHORIZED_PATH })); + + expect(decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(decision.reason).toBe(JOB_AUTHORIZATION_REASON.JOB_ENVELOPE_INVALID); + expect(decision.mayExecuteOnce).toBe(false); + expect(decision.permit).toBeNull(); + // The fabricated path is nowhere in the answer, not even as an echo. + expect(JSON.stringify(decision)).not.toContain(UNAUTHORIZED_PATH); + }); + + it('proves the Array.prototype pollution is actually active', () => { + const observed = withArrayPrototypeElement(UNAUTHORIZED_PATH, () => { + const sparse = withHoleAtOne(AUTHORIZED_PATH); + return { own: Object.hasOwn(sparse, 1), read: sparse[1], length: sparse.length }; + }); + + expect(observed.own).toBe(false); + expect(observed.read).toBe(UNAUTHORIZED_PATH); + expect(observed.length).toBe(2); + }); + + it('rejects an authorized-path list whose hole is filled by Array.prototype', () => { + const outcome = withArrayPrototypeElement(UNAUTHORIZED_PATH, () => { + const job = buildJob({ authorizedPaths: withHoleAtOne(AUTHORIZED_PATH) }); + + return { + snapshot: readRepairJobAuthorization(job).snapshot, + invalidFields: findInvalidRepairJobFields(job), + decision: authorizeJobOperation(job, buildEdit({ path: UNAUTHORIZED_PATH })), + }; + }); + + expect(outcome.snapshot).toBeNull(); + expect(outcome.invalidFields).toContain('authorizedPaths'); + expect(outcome.decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(outcome.decision.reason).toBe(JOB_AUTHORIZATION_REASON.JOB_ENVELOPE_INVALID); + expect(outcome.decision.mayExecuteOnce).toBe(false); + expect(outcome.decision.permit).toBeNull(); + }); + + it('restores Array.prototype afterwards', () => { + withArrayPrototypeElement(UNAUTHORIZED_PATH, () => undefined); + + expect(Object.hasOwn(Array.prototype, 1)).toBe(false); + expect(withHoleAtOne(AUTHORIZED_PATH)[1]).toBeUndefined(); + }); + + it('does not let an inherited command class enter the trusted snapshot', () => { + // `test` is a genuine, well-formed verification class. Shape is not the + // question; nobody put it in this job's list. + const sparse = sparseWithInheritedElement('lint', 'test'); + const job = buildJob({ + authorizedCommandClasses: sparse as unknown as readonly VerificationCommandClass[], + }); + + expect(Object.hasOwn(sparse, 1)).toBe(false); + expect(readRepairJobAuthorization(job).snapshot).toBeNull(); + expect(findInvalidRepairJobFields(job)).toContain('authorizedCommandClasses'); + }); + + it('does not let an inherited command class gain command-class authority', () => { + const job = buildJob({ + authorizedCommandClasses: sparseWithInheritedElement( + 'lint', + 'test', + ) as unknown as readonly VerificationCommandClass[], + }); + + const decision = authorizeJobOperation( + job, + buildRequest({ + operation: 'verification.run', + worktreeId: REPAIR_WORKTREE, + commandClass: 'test', + }), + ); + + expect(decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(decision.mayExecuteOnce).toBe(false); + expect(decision.permit).toBeNull(); + }); + + it('still accepts dense own lists, and still reaches ALLOW_ONCE inside them', () => { + const job = buildJob({ + authorizedPaths: [AUTHORIZED_PATH, UNAUTHORIZED_PATH], + authorizedCommandClasses: ['lint', 'test'], + }); + + expect(findInvalidRepairJobFields(job)).toEqual([]); + expect(readRepairJobAuthorization(job).snapshot?.authorizedPaths).toEqual([ + AUTHORIZED_PATH, + UNAUTHORIZED_PATH, + ]); + + // The same path that had to be refused when it was merely inherited is + // authorized the moment an operator actually puts it in the list. + const edit = authorizeJobOperation(job, buildEdit({ path: UNAUTHORIZED_PATH })); + expect(edit.decision).toBe(JOB_AUTHORIZATION.ALLOW_ONCE); + expect(edit.reason).toBe(JOB_AUTHORIZATION_REASON.WITHIN_JOB_ENVELOPE); + expect(edit.mayExecuteOnce).toBe(true); + expect(edit.permit).not.toBeNull(); + + const verify = authorizeJobOperation( + job, + buildRequest({ + operation: 'verification.run', + worktreeId: REPAIR_WORKTREE, + commandClass: 'test', + }), + ); + expect(verify.decision).toBe(JOB_AUTHORIZATION.ALLOW_ONCE); + expect(verify.mayExecuteOnce).toBe(true); + }); + + it('keeps a plain sparse hole failing closed with nothing inherited at all', () => { + const job = buildJob({ authorizedPaths: withHoleAtOne(AUTHORIZED_PATH) }); + + expect(findInvalidRepairJobFields(job)).toContain('authorizedPaths'); + expect(authorizeJobOperation(job, buildEdit()).mayExecuteOnce).toBe(false); + }); + + it('rejects rather than throws when the own check itself is hostile', () => { + // A Proxy whose `getOwnPropertyDescriptor` trap throws: the own-only read + // must fail closed, and the entry point must stay total. + const hostile = new Proxy([AUTHORIZED_PATH, AUTHORIZED_PATH], { + getOwnPropertyDescriptor(): PropertyDescriptor { + throw new Error('hostile own-property trap'); + }, + }); + const job = buildJob({ authorizedPaths: hostile }); + + expect(() => findInvalidRepairJobFields(job)).not.toThrow(); + expect(findInvalidRepairJobFields(job)).toContain('authorizedPaths'); + expect(authorizeJobOperation(job, buildEdit()).mayExecuteOnce).toBe(false); + }); + + it('rejects rather than throws when an element getter is hostile', () => { + const hostile: string[] = [AUTHORIZED_PATH, AUTHORIZED_PATH]; + Object.defineProperty(hostile, 1, { + get(): string { + throw new Error('hostile element getter'); + }, + configurable: true, + enumerable: true, + }); + const job = buildJob({ authorizedPaths: hostile }); + + expect(() => authorizeJobOperation(job, buildEdit())).not.toThrow(); + expect(findInvalidRepairJobFields(job)).toContain('authorizedPaths'); + expect(authorizeJobOperation(job, buildEdit()).mayExecuteOnce).toBe(false); + }); + + it('pins the documented limit: a Proxy that misreports ownership widens nothing', () => { + // The honest boundary, recorded so the guarantee is not read as stronger + // than it is. A Proxy *defines* the observable result of `Object.hasOwn` + // and of the read, so one that claims a hole is own while the read forwards + // through the target's prototype passes the inherited value through. No + // reader can tell it apart from a truthful object, and this test does not + // pretend otherwise — it pins the *consequence*, which is that nothing is + // widened. + const target = withHoleAtOne(AUTHORIZED_PATH); + Object.setPrototypeOf(target, { 1: UNAUTHORIZED_PATH }); + const liar = new Proxy(target, { + getOwnPropertyDescriptor(t, key): PropertyDescriptor | undefined { + if (key === '1') { + return { value: UNAUTHORIZED_PATH, writable: true, enumerable: true, configurable: true }; + } + return Reflect.getOwnPropertyDescriptor(t, key); + }, + }); + + // The lie is in effect: the object reports the hole as its own. + expect(Object.hasOwn(target, 1)).toBe(false); + expect(Object.hasOwn(liar, 1)).toBe(true); + + const viaProxy = authorizeJobOperation( + buildJob({ authorizedPaths: liar }), + buildEdit({ path: UNAUTHORIZED_PATH }), + ); + // The same caller, supplying the same value as a plain dense own element — + // which needs no Proxy and no lie at all. + const viaDenseArray = authorizeJobOperation( + buildJob({ authorizedPaths: [AUTHORIZED_PATH, UNAUTHORIZED_PATH] }), + buildEdit({ path: UNAUTHORIZED_PATH }), + ); + + // Byte-identical: the Proxy reaches exactly what configuration already + // reaches, so it is not an escalation, and the documentation says so. + expect(JSON.stringify(viaProxy)).toBe(JSON.stringify(viaDenseArray)); + }); + + it('needs no global Symbol call to evaluate, so the sentinel depends on no mutable global', async () => { + // The absence marker is a bare object literal. A module that built it with + // `Symbol(...)` would call the global factory during evaluation; this + // module must not, so a replaced `Symbol` is not on its load path at all. + const saved = globalThis.Symbol; + let calls = 0; + // A Proxy over the real Symbol keeps every own property — `Symbol.iterator` + // and friends — so the module loader itself is unaffected. + const counting = new Proxy(saved, { + apply(target, thisArg, args: readonly unknown[]): unknown { + calls += 1; + return Reflect.apply(target as (...a: readonly unknown[]) => unknown, thisArg, args); + }, + }); + + let fresh: typeof import('../../src/domain/repair-job.js'); + try { + globalThis.Symbol = counting; + vi.resetModules(); + // Awaited inside the try, so the module actually evaluates while the + // counting Symbol is installed. + fresh = await import('../../src/domain/repair-job.js'); + } finally { + globalThis.Symbol = saved; + } + + expect(calls).toBe(0); + // And the freshly evaluated module still behaves. + expect(fresh.findInvalidRepairJobFields(buildJob())).toEqual([]); + expect( + fresh.findInvalidRepairJobFields(buildJob({ authorizedPaths: withHoleAtOne(AUTHORIZED_PATH) })), + ).toContain('authorizedPaths'); + }); + + it('leaves the merge and auto-merge barriers exactly where they were', () => { + // The A03 repair touches list-element provenance and nothing else. + const job = buildJob({ + authorizedPaths: sparseWithInheritedElement(AUTHORIZED_PATH, UNAUTHORIZED_PATH), + }); + + const merge = authorizeJobOperation(job, buildRequest({ operation: 'merge' })); + expect(merge.decision).toBe(JOB_AUTHORIZATION.OPERATOR_REQUIRED); + expect(merge.reason).toBe(JOB_AUTHORIZATION_REASON.MERGE_IS_OPERATOR_ONLY); + expect(merge.mayExecuteOnce).toBe(false); + expect(merge.permit).toBeNull(); + + const auto = authorizeJobOperation(job, buildRequest({ operation: 'auto_merge.enable' })); + expect(auto.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(auto.reason).toBe(JOB_AUTHORIZATION_REASON.OPERATION_FORBIDDEN); + expect(auto.permit).toBeNull(); + }); +}); + /* ------------------------------------------------------------------------- * Cross-boundary conventions * ------------------------------------------------------------------------- */