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 * ------------------------------------------------------------------------- */