From 7575192d0ea8a808401a5f8eff873fa7bf465609 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Sat, 15 Aug 2026 23:36:48 +0200 Subject: [PATCH] fix: harden cockpit operation resolution C1-A01 (P2). `resolveJobOperation` resolved operation names through `OPERATION_LOOKUP.get(value)`. `Map.prototype.get` is looked up at call time, so a hostile replacement installed after module initialization could map any requested name onto a repair-authorizable one. Reproduced from the parent baseline: with `Map.prototype.get` returning `source.edit`, a valid repair-job envelope resolved `merge` to `source.edit` and produced ALLOW_ONCE / WITHIN_JOB_ENVELOPE with an execution permit issued. The same corruption applied to `auto_merge.enable` and to unmodeled names such as `shell.exec`. Remove the Map lookup entirely. Resolution is now an exact membership test against the existing frozen vocabularies via `containsValue`, which touches no prototype method, and the value returned on a hit is the caller's own string rather than one produced by a container. The resolver can therefore return only the exact requested name when it is modeled, or UNKNOWN_JOB_OPERATION. No runtime mechanism can substitute one operation name for another. Adds focused adversarial regression coverage under poisoned `Map.prototype.get`, restoring the captured descriptor in a finally block: merge stays merge, auto_merge.enable stays auto_merge.enable, shell.exec stays unknown, source.edit stays source.edit, merge cannot reach ALLOW_ONCE, unknown cannot reach ALLOW_ONCE, and a legitimate source.edit still authorizes byte-identically to its unpoisoned baseline. Co-Authored-By: Claude Opus 5 (1M context) --- src/domain/job-operation.ts | 41 +++-- .../job-authorization-invariants.test.ts | 146 ++++++++++++++++++ 2 files changed, 171 insertions(+), 16 deletions(-) diff --git a/src/domain/job-operation.ts b/src/domain/job-operation.ts index 9f5ed17..ccb6144 100644 --- a/src/domain/job-operation.ts +++ b/src/domain/job-operation.ts @@ -154,21 +154,6 @@ export type JobOperation = | ForbiddenJobOperation | UnknownJobOperation; -/** - * Membership is backed by a `Map`, not a plain object. - * - * A plain-object lookup inherits `Object.prototype`, so `'toString'`, - * `'constructor'`, and `'__proto__'` would resolve to a truthy entry. A `Map` - * has no prototype chain for keys. Same reasoning as PR 002's taxonomy. - */ -const OPERATION_LOOKUP: ReadonlyMap = - new Map([ - ...REPAIR_AUTHORIZABLE_OPERATIONS.map( - (operation) => [operation, operation] as const, - ), - ...FORBIDDEN_OPERATIONS.map((operation) => [operation, operation] as const), - ]); - /** * Resolve an untrusted operation name to a modeled member. * @@ -179,12 +164,36 @@ const OPERATION_LOOKUP: ReadonlyMap(body: () => T): T { + const saved = Object.getOwnPropertyDescriptor(Map.prototype, 'get'); + Object.defineProperty(Map.prototype, 'get', { + value: function poisonedGet(): string { + return JOB_OPERATION.SOURCE_EDIT; + }, + writable: true, + configurable: true, + }); + try { + return body(); + } finally { + if (saved === undefined) { + Reflect.deleteProperty(Map.prototype, 'get'); + } else { + Object.defineProperty(Map.prototype, 'get', saved); + } + } +} + +describe('poisoning Map.prototype.get cannot re-resolve an operation', () => { + it('proves the poisoning is actually in effect', () => { + // Without this the whole section could pass vacuously, on a runtime that + // was never hostile in the first place. + const observed = withPoisonedMapGet(() => new Map().get('anything')); + + expect(observed).toBe(JOB_OPERATION.SOURCE_EDIT); + }); + + it('keeps merge resolving to merge', () => { + const resolved = withPoisonedMapGet(() => resolveJobOperation(FORBIDDEN_OPERATION.MERGE)); + + expect(resolved).toBe(FORBIDDEN_OPERATION.MERGE); + }); + + it('keeps auto_merge.enable resolving to auto_merge.enable', () => { + const resolved = withPoisonedMapGet(() => + resolveJobOperation(FORBIDDEN_OPERATION.AUTO_MERGE_ENABLE), + ); + + expect(resolved).toBe(FORBIDDEN_OPERATION.AUTO_MERGE_ENABLE); + }); + + it('keeps an unmodeled operation resolving to unknown', () => { + const resolved = withPoisonedMapGet(() => resolveJobOperation('shell.exec')); + + expect(resolved).toBe(UNKNOWN_JOB_OPERATION); + }); + + it('keeps source.edit resolving to source.edit', () => { + const resolved = withPoisonedMapGet(() => resolveJobOperation(JOB_OPERATION.SOURCE_EDIT)); + + expect(resolved).toBe(JOB_OPERATION.SOURCE_EDIT); + }); + + it('resolves every modeled operation to itself and nothing else', () => { + const modeled = [...REPAIR_AUTHORIZABLE_OPERATIONS, ...FORBIDDEN_OPERATIONS]; + + const resolved = withPoisonedMapGet(() => modeled.map((o) => resolveJobOperation(o))); + + expect(resolved).toStrictEqual(modeled); + }); + + it('does not let a merge request reach ALLOW_ONCE', () => { + // The request carries valid `source.edit` operands, so nothing earlier in + // the evaluator can refuse it on an operand ground. The only thing standing + // between it and a permit is that `merge` still resolves as `merge`. + const decision = withPoisonedMapGet(() => + authorizeJobOperation( + buildJob(), + buildEdit({ operation: FORBIDDEN_OPERATION.MERGE }), + ), + ); + + expect(decision.operation).toBe(FORBIDDEN_OPERATION.MERGE); + expect(decision.decision).toBe(JOB_AUTHORIZATION.OPERATOR_REQUIRED); + expect(decision.reason).toBe(JOB_AUTHORIZATION_REASON.MERGE_IS_OPERATOR_ONLY); + expect(decision.mayExecuteOnce).toBe(false); + expect(decision.permit).toBeNull(); + }); + + it('does not let an auto-merge request reach ALLOW_ONCE', () => { + const decision = withPoisonedMapGet(() => + authorizeJobOperation( + buildJob(), + buildEdit({ operation: FORBIDDEN_OPERATION.AUTO_MERGE_ENABLE }), + ), + ); + + expect(decision.operation).toBe(FORBIDDEN_OPERATION.AUTO_MERGE_ENABLE); + expect(decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(decision.reason).toBe(JOB_AUTHORIZATION_REASON.OPERATION_FORBIDDEN); + expect(decision.mayExecuteOnce).toBe(false); + expect(decision.permit).toBeNull(); + }); + + it('does not let an unmodeled request reach ALLOW_ONCE', () => { + const decision = withPoisonedMapGet(() => + authorizeJobOperation(buildJob(), buildEdit({ operation: 'shell.exec' })), + ); + + expect(decision.operation).toBe(UNKNOWN_JOB_OPERATION); + expect(decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(decision.reason).toBe(JOB_AUTHORIZATION_REASON.OPERATION_UNKNOWN); + expect(decision.mayExecuteOnce).toBe(false); + expect(decision.permit).toBeNull(); + }); + + it('still authorizes a legitimate source.edit under the same poisoning', () => { + // Fail-closed is not enough on its own: a repair that refused everything + // would satisfy every assertion above and break the boundary instead. + const baseline = authorizeJobOperation(buildJob(), buildEdit()); + const poisoned = withPoisonedMapGet(() => authorizeJobOperation(buildJob(), buildEdit())); + + expect(poisoned.decision).toBe(JOB_AUTHORIZATION.ALLOW_ONCE); + expect(poisoned.reason).toBe(JOB_AUTHORIZATION_REASON.WITHIN_JOB_ENVELOPE); + expect(poisoned.mayExecuteOnce).toBe(true); + expect(JSON.stringify(poisoned)).toBe(JSON.stringify(baseline)); + }); + + it('restores Map.prototype.get afterwards', () => { + withPoisonedMapGet(() => undefined); + + expect(new Map([['k', 'v']]).get('k')).toBe('v'); + }); +}); + /* ------------------------------------------------------------------------- * Cross-boundary conventions * ------------------------------------------------------------------------- */