From 73d49c3741f24397d980d98579e85c32093daa97 Mon Sep 17 00:00:00 2001 From: nickyleach <331803+nickyleach@users.noreply.github.com> Date: Fri, 11 Sep 2026 21:09:17 +0000 Subject: [PATCH] fix(agents): compare inherited Claude OAuth version at claim time The hire-inheritance gate re-read the parent's current fixed Claude OAuth binding inside the write transaction, but only checked its type and key. It did not compare the binding to the reference the route had already copied onto the child before the transaction started. A concurrent version change on the parent between that copy and the transaction could pass the gate while the child kept a stale version. Compare the parent's current reference against the child's copied reference, including the version selector, and reject the claim on any mismatch. Co-authored-by: Paperclip --- .../agents-claude-oauth-binding.test.ts | 62 ++++++++++++++++++- server/src/services/agents.ts | 38 ++++++++---- server/src/services/secrets.ts | 25 ++++++++ 3 files changed, 111 insertions(+), 14 deletions(-) diff --git a/server/src/__tests__/agents-claude-oauth-binding.test.ts b/server/src/__tests__/agents-claude-oauth-binding.test.ts index ae8b6e5982..2c906dc644 100644 --- a/server/src/__tests__/agents-claude-oauth-binding.test.ts +++ b/server/src/__tests__/agents-claude-oauth-binding.test.ts @@ -27,6 +27,7 @@ import { assertClaudeOAuthBindingInvariant, CLAUDE_OAUTH_CLAIM_REJECTED, CLAUDE_OAUTH_CREDENTIAL_CONFLICT, + claudeOAuthBindingsMatchExactly, claudeOAuthClaimRejectedError, isFixedClaudeOAuthBinding, secretService, @@ -188,6 +189,22 @@ describe("assertClaudeOAuthBindingInvariant", () => { expect(error.status).toBe(409); expect(error.message).toBe(CLAUDE_OAUTH_CLAIM_REJECTED); }); + + it("matches two fixed bindings only when their version selectors are exactly equal", () => { + expect(claudeOAuthBindingsMatchExactly(FIXED_BINDING, FIXED_BINDING)).toBe(true); + expect( + claudeOAuthBindingsMatchExactly({ ...FIXED_BINDING, version: 5 }, { ...FIXED_BINDING, version: 5 }), + ).toBe(true); + expect( + claudeOAuthBindingsMatchExactly({ ...FIXED_BINDING, version: 5 }, { ...FIXED_BINDING, version: 2 }), + ).toBe(false); + expect( + claudeOAuthBindingsMatchExactly({ ...FIXED_BINDING, version: 5 }, { ...FIXED_BINDING, version: "latest" }), + ).toBe(false); + // Neither side needs the exact fixed shape only; both sides do. + expect(claudeOAuthBindingsMatchExactly(FIXED_BINDING, { type: "plain", value: "x" })).toBe(false); + expect(claudeOAuthBindingsMatchExactly(null, FIXED_BINDING)).toBe(false); + }); }); // --- The stored-session claim on the create and hire paths (Postgres) -------- @@ -801,7 +818,7 @@ describeEmbeddedPostgres("agent service Claude OAuth binding claim", () => { async function seedParentAgent( parentScope: Scope, - options: { adapterType?: string; holdsFixedBinding?: boolean } = {}, + options: { adapterType?: string; holdsFixedBinding?: boolean; version?: number } = {}, ) { const [row] = await db .insert(agents) @@ -812,7 +829,14 @@ describeEmbeddedPostgres("agent service Claude OAuth binding claim", () => { status: "idle", adapterType: options.adapterType ?? "claude_local", adapterConfig: { - env: options.holdsFixedBinding === false ? {} : { CLAUDE_CODE_OAUTH_TOKEN: { ...FIXED_BINDING } }, + env: + options.holdsFixedBinding === false + ? {} + // A binding written through the normal persistence path always + // carries a resolved version, "latest" by default. Match that + // shape here, so only `options.version` simulates a pinned + // version, or a version change since the child copied it. + : { CLAUDE_CODE_OAUTH_TOKEN: { ...FIXED_BINDING, version: options.version ?? "latest" } }, }, runtimeConfig: {}, }) @@ -841,6 +865,40 @@ describeEmbeddedPostgres("agent service Claude OAuth binding claim", () => { expect(await countDeclarationsForAgent(created.id)).toBe(1); }); + it("binds the inherited reference when the child's copied version still matches the parent's current version", async () => { + const scope = await seedScope(); + const parent = await seedParentAgent(scope, { version: 5 }); + + const created = await agentService(db).create( + scope.companyId, + createInput(scope, { CLAUDE_CODE_OAUTH_TOKEN: { ...FIXED_BINDING, version: 5 } }), + { claudeLogin: { inheritedFromAgentId: parent.id } }, + ); + + const persisted = created.adapterConfig as { env: Record }; + expect(persisted.env.CLAUDE_CODE_OAUTH_TOKEN).toMatchObject({ ...FIXED_BINDING, version: 5 }); + expect(await countDeclarationsForAgent(created.id)).toBe(1); + }); + + it("rejects an inherited claim when the parent's version moved after the route copied the child's reference", async () => { + const scope = await seedScope(); + // The parent now holds version 5. The child's reference, copied before this + // transaction, still names version 2 — a concurrent parent rotation moved + // the parent's version between the copy and this write. + const parent = await seedParentAgent(scope, { version: 5 }); + + await expect( + agentService(db).create( + scope.companyId, + createInput(scope, { CLAUDE_CODE_OAUTH_TOKEN: { ...FIXED_BINDING, version: 2 } }), + { claudeLogin: { inheritedFromAgentId: parent.id } }, + ), + ).rejects.toMatchObject({ message: CLAUDE_OAUTH_CLAIM_REJECTED }); + // Only the seeded parent exists; the rejected create inserted no child. + expect(await countAgents(scope.companyId)).toBe(1); + expect(await countDeclarationsForCompany(scope.companyId)).toBe(0); + }); + it("rejects an inherited claim when the named parent holds no fixed binding", async () => { const scope = await seedScope(); const parent = await seedParentAgent(scope, { holdsFixedBinding: false }); diff --git a/server/src/services/agents.ts b/server/src/services/agents.ts index a3b4dfb60d..1a75fd9d71 100644 --- a/server/src/services/agents.ts +++ b/server/src/services/agents.ts @@ -40,9 +40,10 @@ import { normalizeAgentPermissions } from "./agent-permissions.js"; import { REDACTED_EVENT_VALUE, sanitizeRecord } from "../redaction.js"; import { assertClaudeOAuthBindingInvariant, + claudeOAuthBindingsMatchExactly, claudeOAuthClaimRejectedError, CLAUDE_LOCAL_ADAPTER_TYPE, - isFixedClaudeOAuthBinding, + readClaudeOAuthBinding, secretService, type ClaudeOAuthBindingInvariantDecision, } from "./secrets.js"; @@ -569,12 +570,15 @@ export function agentService(db: Db) { * The hire-inheritance path (`inheritedFromAgentId`) binds the fixed * reference with no login round trip and no stored owner value, because the * owning user resolves per run, not from a value stored against this agent. - * The gate re-reads the named parent agent inside this transaction and - * permits the bind only when the parent exists, is in the same company, is a - * `claude_local` agent, and already holds the exact fixed binding. The route - * derives this identifier from the authenticated agent actor, never from the - * request body, so the gate treats it as a claim to verify, not a trusted - * value. + * The route copies the parent's reference onto the child before this + * transaction starts, so a concurrent version change on the parent can + * leave the child holding a stale version. The gate re-reads the named + * parent agent inside this transaction and permits the bind only when the + * parent exists, is in the same company, is a `claude_local` agent, and its + * current reference matches the child's copied reference exactly, including + * the version selector. The route derives the parent identifier from the + * authenticated agent actor, never from the request body, so the gate + * treats it as a claim to verify, not a trusted value. * * A controlled internal override skips the claim for a migration or an * administrator repair. The function creates the fixed user-secret definition @@ -589,6 +593,13 @@ export function agentService(db: Db) { consume: boolean; environmentId: string | null; claudeLogin?: ClaudeLoginContext; + /** + * The adapter config the write is about to persist. The + * `inheritedFromAgentId` path reads the child's copied + * `CLAUDE_CODE_OAUTH_TOKEN` reference from it, to compare against the + * parent's current reference. + */ + childAdapterConfig?: unknown; }, ): Promise { const ownerUserId = input.claudeLogin?.ownerUserId ?? null; @@ -610,6 +621,10 @@ export function agentService(db: Db) { } else if (input.claudeLogin?.inheritedFromAgentId) { // The hire-inheritance path. Re-read the named parent inside this // transaction; a caller-supplied identifier never binds on its own. + // Compare the parent's current reference against the reference + // already copied onto the child, including the version selector, so + // a concurrent version change on the parent cannot leave the child + // bound to a stale version. const parentId = input.claudeLogin.inheritedFromAgentId; const parent = await txDb .select({ @@ -620,15 +635,13 @@ export function agentService(db: Db) { .from(agents) .where(eq(agents.id, parentId)) .then((rows) => rows[0] ?? null); - const parentAdapterConfig = parent && isPlainRecord(parent.adapterConfig) ? parent.adapterConfig : null; - const parentEnv = - parentAdapterConfig && isPlainRecord(parentAdapterConfig.env) ? parentAdapterConfig.env : null; - const parentBinding = parentEnv ? parentEnv.CLAUDE_CODE_OAUTH_TOKEN : null; + const parentBinding = readClaudeOAuthBinding(parent?.adapterConfig ?? null); + const childBinding = readClaudeOAuthBinding(input.childAdapterConfig ?? null); if ( !parent || parent.companyId !== input.companyId || parent.adapterType !== CLAUDE_LOCAL_ADAPTER_TYPE || - !isFixedClaudeOAuthBinding(parentBinding) + !claudeOAuthBindingsMatchExactly(parentBinding, childBinding) ) { throw claudeOAuthClaimRejectedError(); } @@ -886,6 +899,7 @@ export function agentService(db: Db) { consume: true, environmentId: (data.defaultEnvironmentId as string | null | undefined) ?? null, claudeLogin: options?.claudeLogin, + childAdapterConfig: adapterConfig, }); const created = await tx .insert(agents) diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index dd6fa92712..c5facc4964 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -190,6 +190,31 @@ export function isFixedClaudeOAuthBinding(binding: unknown): boolean { return record.type === "user_secret_ref" && record.key === CLAUDE_CODE_OAUTH_TOKEN_KEY; } +/** + * Reads the `CLAUDE_CODE_OAUTH_TOKEN` binding from an adapter config, or + * `null` when the config carries no such key. + */ +export function readClaudeOAuthBinding(config: unknown): unknown { + const value = readAdapterEnvRecord(config)[CLAUDE_CODE_OAUTH_TOKEN_KEY]; + return value === undefined ? null : value; +} + +/** + * Returns true when both bindings are the exact fixed Claude Code OAuth + * reference and select the exact same secret version. The hire-inheritance + * gate compares the parent's current reference, re-read inside the write + * transaction, against the reference already copied onto the child before the + * transaction started. A concurrent version change on the parent must fail + * this check, so the child never keeps a stale version under a claim the gate + * treats as current. + */ +export function claudeOAuthBindingsMatchExactly(parentBinding: unknown, childBinding: unknown): boolean { + if (!isFixedClaudeOAuthBinding(parentBinding) || !isFixedClaudeOAuthBinding(childBinding)) return false; + const parentVersion = (parentBinding as Record).version; + const childVersion = (childBinding as Record).version; + return parentVersion === childVersion; +} + /** True when the config carries the exact fixed OAuth binding. */ function hasFixedClaudeOAuthBinding(config: unknown): boolean { return isFixedClaudeOAuthBinding(readAdapterEnvRecord(config)[CLAUDE_CODE_OAUTH_TOKEN_KEY]);