From f8eb259664fd2ae15eb1792daedd41665e63ced7 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Fri, 21 Aug 2026 13:51:44 +0200 Subject: [PATCH] fix(c1): snapshot trusted state before hostile reads --- src/domain/execution-permit.ts | 12 +++-- src/domain/job-authorization.ts | 7 ++- tests/domain/execution-permit.test.ts | 53 +++++++++++++++++++ .../job-authorization-invariants.test.ts | 30 +++++++++++ 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/src/domain/execution-permit.ts b/src/domain/execution-permit.ts index 4abfdb8..0c529a3 100644 --- a/src/domain/execution-permit.ts +++ b/src/domain/execution-permit.ts @@ -398,6 +398,14 @@ export function operatorMergeAuthorizes( return false; } + // The supplied target is captured *before* any candidate property is read. + // Reading the untrusted candidate runs its own getters and Proxy traps, which + // could otherwise mutate the still-live target before its fields are captured + // and make a stale candidate match a target it was rewritten to fit. + const targetRepositoryId = readExactIdentifier(readOwnProperty(targetRecord, 'repositoryId')); + const targetPullRequestId = readExactIdentifier(readOwnProperty(targetRecord, 'pullRequestId')); + const targetHeadSha = readExactIdentifier(readOwnProperty(targetRecord, 'currentHeadSha')); + const authorizationId = readExactIdentifier(readOwnProperty(record, 'authorizationId')); const operatorId = readExactIdentifier(readOwnProperty(record, 'operatorId')); const repositoryId = readExactIdentifier(readOwnProperty(record, 'repositoryId')); @@ -406,10 +414,6 @@ export function operatorMergeAuthorizes( const authorizedAt = readExactIdentifier(readOwnProperty(record, 'authorizedAt')); const singleUse = readOwnProperty(record, 'singleUse'); - const targetRepositoryId = readExactIdentifier(readOwnProperty(targetRecord, 'repositoryId')); - const targetPullRequestId = readExactIdentifier(readOwnProperty(targetRecord, 'pullRequestId')); - const targetHeadSha = readExactIdentifier(readOwnProperty(targetRecord, 'currentHeadSha')); - if ( authorizationId === null || operatorId === null || diff --git a/src/domain/job-authorization.ts b/src/domain/job-authorization.ts index dcb7e57..a55704e 100644 --- a/src/domain/job-authorization.ts +++ b/src/domain/job-authorization.ts @@ -349,8 +349,13 @@ export function authorizeJobOperation( job: RepairJobAuthorization, request: JobOperationRequest, ): JobAuthorizationDecision { - const operation = readJobOperation(request); + // The trusted job is snapshotted into a frozen copy *before* the untrusted + // request is read. Reading the request runs its own getters and Proxy traps, + // which could otherwise mutate the still-live trusted job before it is + // captured; taking the snapshot first means every later check reads only the + // frozen `snapshot`, never a value request-side code could still change. const jobRead = readRepairJobAuthorization(job); + const operation = readJobOperation(request); const snapshot = jobRead.snapshot; const kind: JobOperation = operation.operation; diff --git a/tests/domain/execution-permit.test.ts b/tests/domain/execution-permit.test.ts index fc63a6f..9333c61 100644 --- a/tests/domain/execution-permit.test.ts +++ b/tests/domain/execution-permit.test.ts @@ -3,9 +3,12 @@ import { describe, expect, it } from 'vitest'; import { authorizeJobOperation, JOB_AUTHORIZATION, + operatorMergeAuthorizes, permitAuthorizes, type ExecutionPermit, type JobOperationRequest, + type MergeTarget, + type OperatorMergeAuthorization, type RepairJobAuthorization, } from '../../src/domain/index.js'; import { @@ -14,13 +17,16 @@ import { buildJob, buildPush, buildRequest, + HEAD_A, HEAD_B, JOB_B, NON_OBJECTS, + PARENT_PR_A, PARENT_PR_B, PARENT_REF, REPAIR_BRANCH, REPAIR_WORKTREE, + REPO_A, REPO_B, SECOND_AUTHORIZED_PATH, throwingRecord, @@ -407,3 +413,50 @@ describe('permit identity cannot be collided by operand content', () => { expect(withStowaway.operands.commandClass).toBeNull(); }); }); + +describe('the merge target is captured before the candidate is read', () => { + const buildTarget = (): MergeTarget => ({ + repositoryId: REPO_A, + pullRequestId: PARENT_PR_A, + currentHeadSha: HEAD_A, + }); + const buildAuthorization = (): OperatorMergeAuthorization => ({ + authorizationId: 'auth-0001', + operatorId: 'operator-1', + repositoryId: REPO_A, + pullRequestId: PARENT_PR_A, + // A stale/wrong HEAD: does not match the target's authoritative SHA. + headSha: HEAD_B, + authorizedAt: '2026-01-01T00:00:00Z', + singleUse: true, + }); + + it('rejects a candidate whose HEAD SHA does not match the supplied target', () => { + expect(operatorMergeAuthorizes(buildAuthorization(), buildTarget())).toBe(false); + }); + + it('a candidate getter cannot rewrite the supplied target to match itself', () => { + // The candidate carries a getter that, while the candidate is being read, + // rewrites the still-live target's HEAD to the candidate's stale SHA. If the + // target were captured *after* the candidate were read, that mutation would + // turn a false into a true. + let getterRan = false; + const target = buildTarget(); // currentHeadSha === HEAD_A (authoritative) + const hostile = { + ...buildAuthorization(), // headSha === HEAD_B (stale) + get authorizationId() { + getterRan = true; + (target as { currentHeadSha: string }).currentHeadSha = HEAD_B; + return 'auth-0001'; + }, + } as unknown as OperatorMergeAuthorization; + + const result = operatorMergeAuthorizes(hostile, target); + + // The getter must actually have executed, or the test proves nothing. + expect(getterRan).toBe(true); + // The authoritative target SHA was already captured, so the stale candidate + // never matches. + expect(result).toBe(false); + }); +}); diff --git a/tests/domain/job-authorization-invariants.test.ts b/tests/domain/job-authorization-invariants.test.ts index 968bdbc..854c435 100644 --- a/tests/domain/job-authorization-invariants.test.ts +++ b/tests/domain/job-authorization-invariants.test.ts @@ -690,6 +690,36 @@ describe('a value read twice cannot differ between validation and use', () => { expect(after.decision).toBe(JOB_AUTHORIZATION.ALLOW_ONCE); expect(Object.isFrozen(before.invalidJobFields)).toBe(true); }); + + it('snapshots the trusted job before the hostile request is read', () => { + // A `repair.push` naming the protected parent ref. Honestly evaluated this + // is a protected-ref mutation and must be denied. The request carries a + // getter that, while the request is being read, mutates the still-live + // trusted job so the protected ref becomes the repair branch. If the job + // were snapshotted *after* the request were read, that mutation would flip + // DENY into ALLOW_ONCE and issue a permit to push the protected branch. + let getterRan = false; + const job = buildJob(); + const hostile = { + ...buildPush({ ref: PARENT_REF }), + get operation() { + getterRan = true; + (job as { repairBranch: string }).repairBranch = PARENT_REF; + (job as { protectedParentRef: string }).protectedParentRef = REPAIR_BRANCH; + return JOB_OPERATION.REPAIR_PUSH; + }, + } as unknown as JobOperationRequest; + + const decision = authorizeJobOperation(job, hostile); + + // The getter must actually have executed, or the test proves nothing. + expect(getterRan).toBe(true); + // The trusted job was already captured, so the mutation changed no authority. + expect(decision.decision).toBe(JOB_AUTHORIZATION.DENY); + expect(decision.reason).toBe(JOB_AUTHORIZATION_REASON.PROTECTED_REF_MUTATION); + expect(decision.permit).toBeNull(); + expect(decision.mayExecuteOnce).toBe(false); + }); }); describe('prototype pollution and inherited properties create no authority', () => {