From 9064cfd09edc6afb710d1d93c24e7d282ca56571 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Wed, 2 Sep 2026 14:46:53 -0700 Subject: [PATCH] feat(codex-local): give each Codex account its own home and path secret (#12709) ## Thinking Path > - Paperclip is the control plane for companies that use AI agents for work > - Local adapters connect Paperclip agents to provider command line tools > - The Codex adapter stores login data in a shared company home > - A shared home cannot keep credentials for more than one Codex account > - This pull request gives each account a safe home and a matching company secret > - The benefit is that one company can use multiple Codex accounts at the same time ## Linked Issues or Issue Description **Problem or motivation** A company can hold only one Codex subscription credential because device login uses one shared home. A second account cannot log in without replacing or conflicting with the first credential. **Proposed solution** This change validates the vendor account identifier, stores each credential in its own home, and creates a company secret that points to that home. Repeat login calls return success when the matching secret already exists. **Roadmap alignment** The change supports the roadmap goal for centrally managed secrets with scoped access and audited resolution. **Additional context** The security review returned approve with no blocking finding. The branch adds shared account-handle validation and tests for device login and the Codex local adapter. ## What Changed - Add strict allowlist validation for Codex account handles. - Store each Codex account credential in a separate home under the Codex cache root. - Verify that the resolved account home stays inside the cache root. - Create the `CODEX_HOME_` company secret for each account. - Keep repeat and concurrent login calls safe and idempotent. - Add shared helper and route, adapter, and validation tests. ## Verification - `pnpm --filter @paperclipai/adapter-codex-local test` passes with 343 tests. - `pnpm --filter @paperclipai/server test src/__tests__/agent-device-login-routes.test.ts` passes with 25 tests. - The adapter suite passes with 23 tests. - The shared package and Codex adapter typechecks pass. - Continuous integration must pass on every check before merge. ## Risks The account handle becomes part of a directory path and secret name. The strict allowlist and root containment check reduce path traversal risk. Existing single-account homes remain unchanged unless a new device login creates an account-specific home. ## Model Used OpenAI GPT-5 (exact runtime model ID: gpt-5), with tool use and code execution. The runtime context window is not exposed in this run. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../src/server/adapter-auth-promotion.test.ts | 321 ++++- .../src/server/adapter-auth-promotion.ts | 204 +-- .../src/server/codex-auth-cache.test.ts | 225 ++++ .../src/server/codex-auth-cache.ts | 224 +++- .../adapters/codex-local/src/server/index.ts | 7 + .../daytona/src/plugin.test.ts | 8 +- packages/shared/src/account-handle.test.ts | 73 ++ packages/shared/src/account-handle.ts | 44 + packages/shared/src/index.ts | 1 + .../agent-device-login-routes.test.ts | 730 ++++++++++- .../__tests__/device-login-service.test.ts | 78 ++ server/src/__tests__/secrets-service.test.ts | 452 +++++++ server/src/routes/agents.ts | 395 +++++- server/src/services/device-login-service.ts | 52 +- server/src/services/secrets.ts | 1158 ++++++++++------- 15 files changed, 3261 insertions(+), 711 deletions(-) create mode 100644 packages/shared/src/account-handle.test.ts create mode 100644 packages/shared/src/account-handle.ts diff --git a/packages/adapters/codex-local/src/server/adapter-auth-promotion.test.ts b/packages/adapters/codex-local/src/server/adapter-auth-promotion.test.ts index 260f8b2a06..da57c9ae98 100644 --- a/packages/adapters/codex-local/src/server/adapter-auth-promotion.test.ts +++ b/packages/adapters/codex-local/src/server/adapter-auth-promotion.test.ts @@ -23,8 +23,8 @@ const TOKEN_SENTINEL = "SENTINEL_TOKEN_XYZ"; // This suite proves the device-login credential promotion helper. The helper // runs an independent readiness check on the exact staged credential first, then -// validates it with the export rules, then writes only the company-scoped -// credential home and the company-scoped cache. It writes only while the session +// validates it with the export rules, then writes this account's own home and, +// as a fallback, the company default home. It writes only while the session // holds the sole active claim on the slot (the conditional check). It never // writes the instance-global host, and it never logs secret bytes. describe("device-login credential promotion", () => { @@ -83,11 +83,7 @@ describe("device-login credential promotion", () => { return path.join(resolveManagedCodexHomeDir(env, companyId), "auth.json"); } - async function readIfPresent(target: string): Promise { - return readFile(target, "utf8").catch(() => null); - } - - it("a failed readiness check rejects and writes neither the company home nor the cache", async () => { + it("a failed readiness check rejects and writes neither the company home nor the account home", async () => { const home = await makeInstanceRoot(); const env = envFor(home); const logs: string[] = []; @@ -119,10 +115,10 @@ describe("device-login credential promotion", () => { expect(logs.join("\n")).not.toContain(ACCOUNT); }); - it("a user login seeds an empty company home and cache slot for the new identity", async () => { + it("a user login seeds this account's own home and the company default home", async () => { const home = await makeInstanceRoot(); const env = envFor(home); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), companyId: COMPANY_A, userInitiated: true, @@ -131,22 +127,22 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("promoted"); + expect(result.outcome).toBe("promoted"); const homeAuth = await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8"); expect(JSON.parse(homeAuth).tokens.account_id).toBe(ACCOUNT); - const cacheAuth = await readFile( + const accountAuth = await readFile( resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A), "utf8", ); - expect(JSON.parse(cacheAuth).tokens.account_id).toBe(ACCOUNT); + expect(JSON.parse(accountAuth).tokens.account_id).toBe(ACCOUNT); // The instance-global host was never seeded. await expect( lstat(path.join(resolveSharedCodexHomeDir(env), "auth.json")), ).rejects.toThrow(); }); - it("a strictly-newer same-identity login updates the company home", async () => { + it("a strictly-newer login updates this account's own home", async () => { const home = await makeInstanceRoot(); const env = envFor(home); await promoteDeviceLoginCredential({ @@ -158,7 +154,7 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "new" }), companyId: COMPANY_A, userInitiated: true, @@ -167,13 +163,15 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("promoted"); - const homeAuth = JSON.parse(await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8")); - expect(homeAuth.last_refresh).toBe(NEWER); - expect(homeAuth.tokens.refresh_token).toContain("new"); + expect(result.outcome).toBe("promoted"); + const accountAuth = JSON.parse( + await readFile(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A), "utf8"), + ); + expect(accountAuth.last_refresh).toBe(NEWER); + expect(accountAuth.tokens.refresh_token).toContain("new"); }); - it("an older same-identity login keeps the company home", async () => { + it("an older login keeps this account's own home", async () => { const home = await makeInstanceRoot(); const env = envFor(home); await promoteDeviceLoginCredential({ @@ -185,7 +183,7 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: OLDER, marker: "older" }), companyId: COMPANY_A, userInitiated: true, @@ -194,12 +192,14 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("kept"); - const homeAuth = JSON.parse(await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8")); - expect(homeAuth.tokens.refresh_token).toContain("keep"); + expect(result.outcome).toBe("kept"); + const accountAuth = JSON.parse( + await readFile(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A), "utf8"), + ); + expect(accountAuth.tokens.refresh_token).toContain("keep"); }); - it("a different-identity login keeps the home and reports a foreign-identity outcome", async () => { + it("promotion writes the account home for a second, different account", async () => { const home = await makeInstanceRoot(); const env = envFor(home); await promoteDeviceLoginCredential({ @@ -211,7 +211,7 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: OTHER_ACCOUNT, lastRefresh: NEWER, marker: "other" }), companyId: COMPANY_A, userInitiated: true, @@ -220,23 +220,19 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - // The home keeps the first identity. The login did not install the other - // account, so the outcome is `kept_foreign_identity`, not a plain `kept`: the - // caller must fail the session instead of a report of `authenticated`. The - // other identity still lands in its own per-identity cache slot. - expect(outcome).toBe("kept_foreign_identity"); - const homeAuth = JSON.parse(await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8")); - expect(homeAuth.tokens.account_id).toBe(ACCOUNT); - const otherCache = JSON.parse( + // A second, different account gets its own home, so the second login + // succeeds instead of failing behind the first account's home. + expect(result.outcome).toBe("promoted"); + const otherAccountAuth = JSON.parse( await readFile(resolveCodexAuthCacheEntryPath(env, OTHER_ACCOUNT, COMPANY_A), "utf8"), ); - expect(otherCache.tokens.account_id).toBe(OTHER_ACCOUNT); + expect(otherAccountAuth.tokens.account_id).toBe(OTHER_ACCOUNT); }); - it("the cache off-switch skips the cache slot but still seeds the company home", async () => { + it("promotion returns the account id and the account home path", async () => { const home = await makeInstanceRoot(); - const env = envFor(home, { PAPERCLIP_CODEX_AUTH_CACHE: "off" }); - const outcome = await promoteDeviceLoginCredential({ + const env = envFor(home); + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), companyId: COMPANY_A, userInitiated: true, @@ -245,29 +241,231 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("promoted"); - // The company home was seeded. - expect(await readIfPresent(companyHomeAuthPath(env, COMPANY_A))).toContain(ACCOUNT); - // The cache slot was not written. - await expect( - lstat(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A)), - ).rejects.toThrow(); + expect(result.accountId).toBe(ACCOUNT); + expect(result.accountHomeDir).toBe( + path.dirname(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A)), + ); }); - it("a cache write failure keeps the promotion successful and the company home durable", async () => { + it("promotion reports accountHomeCreated true for a first login of an account", async () => { const home = await makeInstanceRoot(); const env = envFor(home); - // Force the per-identity cache write to fail. Plant a regular file where the - // cache expects the identity directory, so the private-directory guard throws - // before the cache slot is written. The company home write runs first, so the - // credential is already durable when the cache write fails. + const result = await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + expect(result.outcome).toBe("promoted"); + expect(result.accountHomeCreated).toBe(true); + }); + + it("promotion reports accountHomeCreated false for a repeat login of the same account", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: OLDER, marker: "first" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + const result = await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "second" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + expect(result.outcome).toBe("promoted"); + expect(result.accountHomeCreated).toBe(false); + }); + + it("two concurrent promotions for the same account report only one accountHomeCreated true", async () => { + // Two different logins for the SAME Codex account run their own + // promotion slot, so they can promote at the same time. Without a lock + // around the absence check and the directory creation, both calls could + // see the directory as absent and both report `created: true`; a caller + // that later deletes the directory on `created: true` would then delete + // a home the other call's login still uses. + const home = await makeInstanceRoot(); + const env = envFor(home); + const [first, second] = await Promise.all([ + promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "racer-a" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }), + promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "racer-b" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }), + ]); + // Both racers still authenticate; only the timestamp-comparison outcome + // (`promoted` vs. `kept`) can differ, because they carry the same + // `lastRefresh` timestamp. + expect(["promoted", "kept"]).toContain(first.outcome); + expect(["promoted", "kept"]).toContain(second.outcome); + const createdFlags = [first.accountHomeCreated, second.accountHomeCreated]; + expect(createdFlags.filter(Boolean)).toHaveLength(1); + await expect( + lstat(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A)), + ).resolves.toBeDefined(); + }); + + it("promotion keeps the company default home when it holds another account", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "first" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + const before = await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8"); + await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: OTHER_ACCOUNT, lastRefresh: NEWER, marker: "other" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + // The company default home fallback still names the first account: a + // second account never clobbers it once some account has claimed it. + expect(await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8")).toBe(before); + }); + + it("promotion seeds the company default home when it holds no usable credential", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + const result = await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + expect(result.outcome).toBe("promoted"); + const homeAuth = await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8"); + expect(JSON.parse(homeAuth).tokens.account_id).toBe(ACCOUNT); + }); + + it("promotion fails the login when the account identifier cannot become a handle", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + await expect( + promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: "acct 42", lastRefresh: NEWER }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }), + ).rejects.toThrow(); + await expect(lstat(companyHomeAuthPath(env, COMPANY_A))).rejects.toThrow(); + }); + + it("a whitespace-padded account identifier never shares a home with, or reports as authenticated against, the unpadded account", async () => { + // A distinct identifier with surrounding whitespace must reject, not + // alias onto the unpadded account's home. Promote the unpadded account + // first, then attempt a second login whose identifier differs only by a + // leading space. The second login must fail, and it must never read or + // report a "kept" outcome against the first account's home. + const home = await makeInstanceRoot(); + const env = envFor(home); + await promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER, marker: "original" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }); + await expect( + promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ` ${ACCOUNT}`, lastRefresh: NEWER, marker: "padded" }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }), + ).rejects.toThrow(); + // The unpadded account's home still holds only the credential the first + // login wrote. The padded login never read it, never wrote it, and never + // received a "kept" outcome that would report it as authenticated. + const accountAuth = JSON.parse( + await readFile(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A), "utf8"), + ); + expect(accountAuth.tokens.refresh_token).toContain("original"); + }); + + it("a broken account-home directory fails the whole login", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + // Force this account's own home write to fail. Plant a regular file where + // the account expects its own directory, so the private-directory guard + // throws before any write. That write is now fail-loud (it is the durable + // result), so the whole login fails, and the best-effort company default + // home fallback is never reached. const cacheDir = resolveCodexAuthCacheDir(env, COMPANY_A); await mkdir(cacheDir, { recursive: true, mode: 0o700 }); const entryPath = resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A); await writeFile(path.dirname(entryPath), "not-a-directory"); + await expect( + promoteDeviceLoginCredential({ + authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), + companyId: COMPANY_A, + userInitiated: true, + checkReadiness: ready, + isSoleActiveOwner: soleOwner, + env, + log: noopLog, + }), + ).rejects.toThrow(); + await expect(lstat(companyHomeAuthPath(env, COMPANY_A))).rejects.toThrow(); + }); + + it("a company default home write failure keeps the promotion successful and this account's own home durable", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + // Force the company default home write to fail. Plant a regular file where + // the company home directory belongs, so mkdir fails. This account's own + // home write already ran and is durable, so the login still succeeds. + const companyHome = resolveManagedCodexHomeDir(env, COMPANY_A); + await mkdir(path.dirname(companyHome), { recursive: true }); + await writeFile(companyHome, "not-a-directory"); + const logs: string[] = []; - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), companyId: COMPANY_A, userInitiated: true, @@ -279,14 +477,13 @@ describe("device-login credential promotion", () => { }, }); - // The company home holds a usable credential, so the promotion still reports - // success rather than a failed login. - expect(outcome).toBe("promoted"); - const homeAuth = JSON.parse(await readFile(companyHomeAuthPath(env, COMPANY_A), "utf8")); - expect(homeAuth.tokens.account_id).toBe(ACCOUNT); - // The cache failure is observable and carries no secret bytes. + expect(result.outcome).toBe("promoted"); + const accountAuth = JSON.parse( + await readFile(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A), "utf8"), + ); + expect(accountAuth.tokens.account_id).toBe(ACCOUNT); const haystack = logs.join("\n"); - expect(haystack).toContain("the per-identity cache write failed"); + expect(haystack).toContain("seeding the company default home failed"); expect(haystack).not.toContain(TOKEN_SENTINEL); expect(haystack).not.toContain(ACCOUNT); }); @@ -324,7 +521,7 @@ describe("device-login credential promotion", () => { it("a promotion whose session is no longer the sole active owner writes nothing", async () => { const home = await makeInstanceRoot(); const env = envFor(home); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), companyId: COMPANY_A, userInitiated: true, @@ -334,17 +531,17 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("not_sole_owner"); + expect(result.outcome).toBe("not_sole_owner"); await expect(lstat(companyHomeAuthPath(env, COMPANY_A))).rejects.toThrow(); await expect( lstat(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A)), ).rejects.toThrow(); }); - it("an automatic background login never seeds an empty company slot", async () => { + it("an automatic background login never seeds an empty home", async () => { const home = await makeInstanceRoot(); const env = envFor(home); - const outcome = await promoteDeviceLoginCredential({ + const result = await promoteDeviceLoginCredential({ authBytes: subscriptionAuth({ accountId: ACCOUNT, lastRefresh: NEWER }), companyId: COMPANY_A, // Not a user-initiated login. @@ -354,7 +551,7 @@ describe("device-login credential promotion", () => { env, log: noopLog, }); - expect(outcome).toBe("background_skipped"); + expect(result.outcome).toBe("background_skipped"); await expect(lstat(companyHomeAuthPath(env, COMPANY_A))).rejects.toThrow(); await expect( lstat(resolveCodexAuthCacheEntryPath(env, ACCOUNT, COMPANY_A)), diff --git a/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts b/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts index 27d4a012fc..6c532bc20c 100644 --- a/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts +++ b/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts @@ -1,11 +1,10 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { toAccountHandle } from "@paperclipai/shared"; import { - ensureCodexAuthCacheEntryDir, - isCodexAuthCacheEnabled, + ensureCodexAuthCacheEntryDirExclusive, readSubscriptionAccountId, - writeCodexAuthCacheEntry, } from "./codex-auth-cache.js"; import { writeCredentialSeedOrNewer } from "./codex-auth-seed-write.js"; import { codexHomeHasUsableAuth, resolveManagedCodexHomeDir } from "./codex-home.js"; @@ -18,13 +17,18 @@ import { assertUsableSubscriptionShape } from "./device-login-export.js"; // // The helper never writes the instance-global host (`CODEX_HOME` or `~/.codex`), // because that source has no `companyId`. It writes two company-scoped targets: -// the company credential home and the company per-identity cache slot. The -// company home write is the first-login host rule: the cache vend never seeds an -// empty host, so a first login must seed the company home directly. +// this account's own home, and — only as a fallback for an agent with no bound +// secret — the company default home. +// +// This account's own home is the durable result of a login: the write is fail +// loud, and the caller names it with a company secret so any agent can bind to +// it. The company default home is a best-effort fallback: it is seeded only the +// first time any account logs in for the company (while it holds no usable +// credential yet), and a write failure there never fails the login. // // Two decisions gate the write: -// - Decision C: only a user-initiated login seeds the company slot. An -// automatic background path never seeds an empty slot. +// - Decision C: only a user-initiated login seeds a home. An automatic +// background path never seeds one. // - Decision H: the helper writes only while the session still holds the sole // active claim on `(company_id, adapter_type)`. This is defense in depth over // the partial unique index, so a second session cannot race the same slot. @@ -93,30 +97,45 @@ export async function checkStagedCredentialReadiness( /** * The promotion outcome. * - * - `promoted`: the helper wrote the company home (a seed or a strictly-newer - * same-identity update). The helper then tries the per-identity cache slot when - * the cache is on. The cache write is best-effort: a cache failure keeps the - * `promoted` outcome, because the company home is already durable. - * - `kept`: the login carried the SAME identity as the company home, and the home - * already held a newer same-identity credential, so the home was kept. This is a - * successful authentication: a later run vends and uses the same account. The - * helper still tries the best-effort cache write. - * - `kept_foreign_identity`: the login carried a DIFFERENT identity than the - * company home. The helper never clobbers an occupied home, so it kept the other - * account and installed nothing durable for this login. The identity-anchored - * vend reads the home identity first, so a later run can never select this - * login. This is NOT a successful authentication; the caller must fail the - * session instead of a report of `authenticated`. + * - `promoted`: the helper wrote this account's own home (a seed for a first + * login of this account, or a strictly-newer update for a repeat login). The + * helper also seeds the company default home when it holds no usable + * credential yet. + * - `kept`: the login carried a credential that is not newer than what this + * account's own home already holds, so the home was kept as-is. This is + * still a successful authentication: a later run reads the same home. * - `not_sole_owner`: Decision H rejected the write. Nothing was written. * - `background_skipped`: Decision C rejected the write (an automatic background - * path never seeds a company slot). Nothing was written. + * path never seeds a home). Nothing was written. */ -export type PromoteDeviceLoginCredentialOutcome = - | "promoted" - | "kept" - | "kept_foreign_identity" - | "not_sole_owner" - | "background_skipped"; +export type PromoteDeviceLoginCredentialOutcome = "promoted" | "kept" | "not_sole_owner" | "background_skipped"; + +/** + * The promotion result. `accountId` and `accountHomeDir` are set once the + * credential's identity is known — every outcome below the handle-validation + * step carries `accountId`; only `promoted` and `kept` reach the write step and + * carry `accountHomeDir`. + * + * `accountHomeCreated` is true only when this account's own home directory was + * absent right before this call created it. The check and the creation run + * under one lock, so a concurrent promotion for the same account (a second + * login for the same Codex account) never also reports `created: true` for a + * directory the first call already made. A caller that must undo a later + * failure (for example, a failed secret write) should remove the directory + * only when `accountHomeCreated` is true, and should still re-check that no + * company secret now names this account's home before it deletes: the + * absence of a company secret at the time this call started is not proof + * that this login is the only login that used the directory. A user can also + * delete the secret and keep the account home, so a repeat login then reads + * no secret. `accountHomeCreated` is false for every outcome that does not + * reach the write step (`not_sole_owner`, `background_skipped`). + */ +export interface PromoteDeviceLoginCredentialResult { + outcome: PromoteDeviceLoginCredentialOutcome; + accountId: string | null; + accountHomeDir: string | null; + accountHomeCreated: boolean; +} export interface PromoteDeviceLoginCredentialInput { /** The exact staged credential bytes the login sandbox produced. */ @@ -167,13 +186,14 @@ function requireSafeCompanyId(companyId: string): string { /** * Promotes a device-login credential into the company scope. The order is fixed: - * readiness check, credential validation, Decision C, Decision H, then the writes. - * The readiness check and the writes run while the caller still holds the active - * claim, so a second session cannot race the same slot. + * readiness check, credential validation, account-handle validation, Decision C, + * Decision H, then the writes. The readiness check and the writes run while the + * caller still holds the active claim, so a second session cannot race the same + * slot. */ export async function promoteDeviceLoginCredential( input: PromoteDeviceLoginCredentialInput, -): Promise { +): Promise { const { authBytes, userInitiated, checkReadiness, isSoleActiveOwner, log } = input; const env = input.env ?? process.env; const companyId = requireSafeCompanyId(input.companyId); @@ -191,84 +211,92 @@ export async function promoteDeviceLoginCredential( const accountId = readSubscriptionAccountId(authBytes); if (!accountId) { // The shape gate above already guarantees a subscription identity; this guard - // keeps the account_id non-null for the cache key without a non-null cast. + // keeps the account_id non-null for the handle conversion without a non-null + // cast. throw new Error("device-login promotion: the credential has no subscription identity"); } - // 3. Decision C: only a user-initiated login seeds the company slot. + // 2b. Convert the identity into a safe account handle. The handle names both + // this account's own home directory and its company secret, so a login + // whose identity cannot form one must fail before any write. + const accountHandle = toAccountHandle(accountId); + if (!accountHandle) { + throw new Error("device-login promotion: the account identifier cannot form a valid account handle"); + } + + // 3. Decision C: only a user-initiated login seeds a home. if (!userInitiated) { - await log("[paperclip] Codex device-login promotion: skipped (an automatic background login never seeds a company slot)."); - return "background_skipped"; + await log("[paperclip] Codex device-login promotion: skipped (an automatic background login never seeds a home)."); + return { outcome: "background_skipped", accountId, accountHomeDir: null, accountHomeCreated: false }; } // 4. Decision H: write only while the session still owns the active slot. const soleOwner = await isSoleActiveOwner(); if (!soleOwner) { await log("[paperclip] Codex device-login promotion: skipped (the session no longer holds the sole active claim on the slot)."); - return "not_sole_owner"; + return { outcome: "not_sole_owner", accountId, accountHomeDir: null, accountHomeCreated: false }; } - // 5a. First-login host rule: seed or update the company credential home. The - // shared writer seeds an empty home and applies a strictly-newer - // same-identity update; it keeps a newer same-identity or a different - // identity. It never touches the instance-global host. - const companyHome = resolveManagedCodexHomeDir(env, companyId); - await mkdir(companyHome, { recursive: true, mode: PRIVATE_DIR_MODE }); - const companyHomeAuthPath = path.join(companyHome, AUTH_FILE_NAME); - const homeOutcome = await writeCredentialSeedOrNewer({ + // 5a. This account's own home is the durable result of a login: each account + // handle addresses exactly one home, so this write can never collide with + // a different identity, and a write failure here fails the whole + // promotion (fail loud, unlike the company default home fallback below). + // Two different logins can promote the same account at the same time + // (each login owns its own promotion slot), so the absence check and the + // directory creation run inside one lock: only the caller that truly + // finds the directory absent gets `created: true`. A plain + // check-then-create sequence here would let two concurrent callers for + // the same account both see the directory as absent and both believe + // they created it. + const { entryPath: accountHomeAuthPath, created: accountHomeCreated } = + await ensureCodexAuthCacheEntryDirExclusive(env, accountHandle, companyId); + const accountHomeDir = path.dirname(accountHomeAuthPath); + const accountHomeOutcome = await writeCredentialSeedOrNewer({ sourceBytes: authBytes, - destinationPath: companyHomeAuthPath, + destinationPath: accountHomeAuthPath, seedIfDestAbsent: true, log, - writtenLine: "[paperclip] Codex device-login promotion: wrote the company credential home at mode 0600.", - keptLine: "[paperclip] Codex device-login promotion: kept the company credential home (the login is not a seed or a strictly-newer same-identity credential).", - tempPrefix: "auth.json.promotion-home", + writtenLine: "[paperclip] Codex device-login promotion: wrote this account's own home at mode 0600.", + keptLine: "[paperclip] Codex device-login promotion: kept this account's own home (the login is not a seed or a strictly-newer credential).", + tempPrefix: "auth.json.promotion-account-home", errorLabel: "codex device-login promotion", env, }); - // A kept home has two very different meanings. The writer keeps the home when - // it already holds a newer SAME-identity credential (a genuine success: a later - // run vends and uses the same account), and it also keeps the home when the home - // holds a DIFFERENT identity (the writer never clobbers an occupied home). The - // second case installed nothing durable for this login: the home still holds the - // other account, and the identity-anchored vend reads the home identity first, - // so it can never select this login. Compare the login identity with the kept - // home identity, so the caller can fail the session instead of a report of - // `authenticated`. A home that this helper cannot read as the same identity is - // treated as a foreign identity; the caller fails closed. - let foreignIdentityKeep = false; - if (homeOutcome === "kept") { - const homeBytes = await readFile(companyHomeAuthPath).catch(() => null); - const homeAccountId = homeBytes ? readSubscriptionAccountId(homeBytes) : null; - foreignIdentityKeep = homeAccountId !== accountId; - } - - // 5b. Record the credential in its per-identity company cache slot, so a later - // run can vend a strictly-newer copy. The cache write respects the - // off-switch. It is company-scoped and per identity, so it never crosses a - // company boundary and never clobbers a different identity. - // - // The company home write above already made the credential durable, so a - // later run authenticates from the home even when the cache slot is absent. - // The cache is only a vend optimization. So a cache write failure (a - // permission error, a full disk, or a lock timeout) must not fail the - // promotion, or the operator sees a failed login for a credential that is - // already usable. Log the failure and keep the home outcome; a later run - // re-seeds the cache slot from the durable home. - if (isCodexAuthCacheEnabled(env)) { + // 5b. Company default home fallback, for an agent with no bound secret. Seed + // it only the first time any account logs in for the company, i.e. only + // while it holds no usable credential yet; a login for a second account + // must never touch it once some account has claimed it. This write is + // best-effort: this account's own home above is already durable, so a + // failure here (a permission error, a full disk, a lock timeout) must not + // fail the promotion. + const companyHome = resolveManagedCodexHomeDir(env, companyId); + if (!(await codexHomeHasUsableAuth(companyHome))) { try { - const cacheEntryPath = await ensureCodexAuthCacheEntryDir(env, accountId, companyId); - await writeCodexAuthCacheEntry({ sandboxAuthBytes: authBytes, cacheEntryPath, log, env }); + await mkdir(companyHome, { recursive: true, mode: PRIVATE_DIR_MODE }); + const companyHomeAuthPath = path.join(companyHome, AUTH_FILE_NAME); + await writeCredentialSeedOrNewer({ + sourceBytes: authBytes, + destinationPath: companyHomeAuthPath, + seedIfDestAbsent: true, + log, + writtenLine: "[paperclip] Codex device-login promotion: seeded the company default home.", + keptLine: "[paperclip] Codex device-login promotion: kept the company default home.", + tempPrefix: "auth.json.promotion-home", + errorLabel: "codex device-login promotion", + env, + }); } catch { await log( - "[paperclip] Codex device-login promotion: the per-identity cache write failed; the company credential home is durable, so the login stays successful.", + "[paperclip] Codex device-login promotion: seeding the company default home failed; this account's own home is durable, so the login stays successful.", ); } } - if (homeOutcome === "written") { - return "promoted"; - } - return foreignIdentityKeep ? "kept_foreign_identity" : "kept"; + return { + outcome: accountHomeOutcome === "written" ? "promoted" : "kept", + accountId, + accountHomeDir, + accountHomeCreated, + }; } diff --git a/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts b/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts index 014093bd65..36d19bbe1d 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { + assertAccountHomeCacheDirStillValid, clearCodexAuthCache, clearCodexAuthCacheEntry, ensureCodexAuthCacheEntryDir, @@ -12,6 +13,8 @@ import { resolveCodexAuthCacheEntryPath, selectVendCredential, toCacheKey, + withAccountHomeSecretMutationLock, + withCodexAccountHomePromotionLock, } from "./codex-auth-cache.js"; import { resolveSharedCodexHomeDir } from "./codex-home.js"; @@ -127,6 +130,17 @@ describe("codex auth cache store", () => { ); }); + it("resolveCodexAuthCacheEntryPath rejects an identifier that toAccountHandle rejects", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + // A space, a plus sign, and a leading hyphen all fail the stricter + // account-handle allowlist, even though the older denylist alone would + // accept some of them. + expect(() => resolveCodexAuthCacheEntryPath(env, "acct 42", "company-a")).toThrow(); + expect(() => resolveCodexAuthCacheEntryPath(env, "acct+42", "company-a")).toThrow(); + expect(() => resolveCodexAuthCacheEntryPath(env, "-rf", "company-a")).toThrow(); + }); + it("resolveCodexAuthCacheEntryPath verifies the resolved path stays under the cache root", async () => { const home = await makeInstanceRoot(); const env = envFor(home); @@ -335,4 +349,215 @@ describe("codex auth cache store", () => { expect(isCodexAuthCacheEnabled({ PAPERCLIP_CODEX_AUTH_CACHE: "no" })).toBe(false); }); }); + + describe("Phase 6: whole-promotion serialization for one company", () => { + it("withCodexAccountHomePromotionLock never overlaps two concurrent callers for the same company", async () => { + // The device-login route holds this lock across its whole promotion + // sequence: the account-home directory decision, the credential write, + // and the secret bind or cleanup that follows. Two different logins for + // the SAME Codex account must run that whole sequence one at a time, so + // a login can never delete the shared account-home directory while + // another login's own sequence is still writing its credential or + // binding its own secret to that same directory. This proves the lock + // itself enforces that: two concurrent callers for one company never run + // their callbacks at the same time, whichever caller goes first. + const home = await makeInstanceRoot(); + const env = envFor(home); + const events: string[] = []; + let releaseFirstCaller!: () => void; + const firstCallerGate = new Promise((resolve) => { + releaseFirstCaller = resolve; + }); + + const firstCall = withCodexAccountHomePromotionLock(env, "company-shared", async () => { + events.push("first-enter"); + await firstCallerGate; + events.push("first-exit"); + return "first"; + }); + // Give the first caller a chance to acquire the lock and enter its + // callback before the second caller starts racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + const secondCall = withCodexAccountHomePromotionLock(env, "company-shared", async () => { + events.push("second-enter"); + events.push("second-exit"); + return "second"; + }); + // The second caller must stay blocked on the lock while the first + // caller still holds it: it must never log `second-enter` before the + // first caller's `first-exit`. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["first-enter"]); + + releaseFirstCaller(); + const [first, second] = await Promise.all([firstCall, secondCall]); + expect(first).toBe("first"); + expect(second).toBe("second"); + expect(events).toEqual(["first-enter", "first-exit", "second-enter", "second-exit"]); + }); + + it("withCodexAccountHomePromotionLock keeps two different companies independent", async () => { + // The lock is per company, so two different companies' device logins + // never wait on each other. + const home = await makeInstanceRoot(); + const env = envFor(home); + const events: string[] = []; + let releaseCompanyA!: () => void; + const companyAGate = new Promise((resolve) => { + releaseCompanyA = resolve; + }); + + const companyACall = withCodexAccountHomePromotionLock(env, "company-a", async () => { + events.push("a-enter"); + await companyAGate; + events.push("a-exit"); + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const companyBCall = withCodexAccountHomePromotionLock(env, "company-b", async () => { + events.push("b-enter"); + events.push("b-exit"); + }); + await companyBCall; + // Company B's callback ran and finished while company A's callback was + // still waiting on its own gate, so the two locks never contended. + expect(events).toEqual(["a-enter", "b-enter", "b-exit"]); + + releaseCompanyA(); + await companyACall; + expect(events).toEqual(["a-enter", "b-enter", "b-exit", "a-exit"]); + }); + }); + + describe("Phase 7: account-home secret-mutation serialization for one company", () => { + it("withAccountHomeSecretMutationLock never overlaps two concurrent callers for the same company", async () => { + // The secrets service holds this lock for the whole of a `local_encrypted` + // secret's create or rotate call, and an account-home cleanup's claimant + // scan holds it for the whole of its final check-and-delete step. This + // proves the lock itself enforces mutual exclusion between any two + // holders for one company, whichever caller goes first. + const home = await makeInstanceRoot(); + const env = envFor(home); + const events: string[] = []; + let releaseFirstCaller!: () => void; + const firstCallerGate = new Promise((resolve) => { + releaseFirstCaller = resolve; + }); + + const firstCall = withAccountHomeSecretMutationLock(env, "company-shared", async () => { + events.push("first-enter"); + await firstCallerGate; + events.push("first-exit"); + return "first"; + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const secondCall = withAccountHomeSecretMutationLock(env, "company-shared", async () => { + events.push("second-enter"); + events.push("second-exit"); + return "second"; + }); + // The second caller must stay blocked on the lock while the first + // caller still holds it: it must never log `second-enter` before the + // first caller's `first-exit`. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["first-enter"]); + + releaseFirstCaller(); + const [first, second] = await Promise.all([firstCall, secondCall]); + expect(first).toBe("first"); + expect(second).toBe("second"); + expect(events).toEqual(["first-enter", "first-exit", "second-enter", "second-exit"]); + }); + + it("withAccountHomeSecretMutationLock keeps two different companies independent", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + const events: string[] = []; + let releaseCompanyA!: () => void; + const companyAGate = new Promise((resolve) => { + releaseCompanyA = resolve; + }); + + const companyACall = withAccountHomeSecretMutationLock(env, "company-a", async () => { + events.push("a-enter"); + await companyAGate; + events.push("a-exit"); + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const companyBCall = withAccountHomeSecretMutationLock(env, "company-b", async () => { + events.push("b-enter"); + events.push("b-exit"); + }); + await companyBCall; + expect(events).toEqual(["a-enter", "b-enter", "b-exit"]); + + releaseCompanyA(); + await companyACall; + expect(events).toEqual(["a-enter", "b-enter", "b-exit", "a-exit"]); + }); + + it("withAccountHomeSecretMutationLock does not contend with withCodexAccountHomePromotionLock for the same company", async () => { + // The two locks use separate lock directories (Security condition: no + // shared lock key), so a device-login promotion that holds + // `withCodexAccountHomePromotionLock` for its whole sequence can still + // call into a secrets-service write that takes this lock without + // deadlocking on itself. + const home = await makeInstanceRoot(); + const env = envFor(home); + const events: string[] = []; + + await withCodexAccountHomePromotionLock(env, "company-shared", async () => { + events.push("promotion-enter"); + await withAccountHomeSecretMutationLock(env, "company-shared", async () => { + events.push("mutation-enter"); + events.push("mutation-exit"); + }); + events.push("promotion-exit"); + }); + + expect(events).toEqual(["promotion-enter", "mutation-enter", "mutation-exit", "promotion-exit"]); + }); + }); + + describe("Phase 8: account-home directory validity before a secret write commits", () => { + it("resolves for a value naming a directory that still exists under the cache root", async () => { + const home = await makeInstanceRoot(); + const env = envFor(home); + const companyId = "company-still-exists"; + const entryPath = await ensureCodexAuthCacheEntryDir(env, "acct-still-exists", companyId); + const accountHomeDir = path.dirname(entryPath); + + await expect( + assertAccountHomeCacheDirStillValid(env, companyId, accountHomeDir), + ).resolves.toBeUndefined(); + }); + + it("rejects a value naming a directory under the cache root that no longer exists", async () => { + // This is the shape an account-home cleanup leaves behind: a value that + // once named a real directory, now removed. A create or a rotate that + // is about to commit this exact value must fail instead of writing a + // secret that points at nothing. + const home = await makeInstanceRoot(); + const env = envFor(home); + const companyId = "company-removed"; + const entryPath = await ensureCodexAuthCacheEntryDir(env, "acct-removed", companyId); + const accountHomeDir = path.dirname(entryPath); + await rm(accountHomeDir, { recursive: true, force: true }); + + await expect(assertAccountHomeCacheDirStillValid(env, companyId, accountHomeDir)).rejects.toThrow( + /no longer exists/, + ); + }); + + it("leaves a value alone when it does not sit under this company's cache root", async () => { + // Most `local_encrypted` secret values (an API key, a token, a + // hand-typed string) never sit under the cache root, so this check must + // never reject a write that only happens to name a missing path. + const home = await makeInstanceRoot(); + const env = envFor(home); + + await expect( + assertAccountHomeCacheDirStillValid(env, "company-unrelated", "/some/unrelated/missing/path"), + ).resolves.toBeUndefined(); + }); + }); }); diff --git a/packages/adapters/codex-local/src/server/codex-auth-cache.ts b/packages/adapters/codex-local/src/server/codex-auth-cache.ts index 5d9755a278..cc399cfeba 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-cache.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-cache.ts @@ -3,15 +3,19 @@ import path from "node:path"; import { randomUUID } from "node:crypto"; import { resolvePaperclipInstanceRootForAdapter } from "@paperclipai/adapter-utils/server-utils"; import { withDirectoryMergeLock } from "@paperclipai/adapter-utils/workspace-restore-merge"; +import { toAccountHandle } from "@paperclipai/shared"; import { USE_SOURCE_EXIT, decideCodexAuthMerge } from "./codex-auth-merge-decision.js"; import { writeCredentialSeedOrNewer } from "./codex-auth-seed-write.js"; -// The identity-keyed host credential cache keeps one usable subscription -// credential per identity (`account_id`) in a SEPARATE host store, outside the -// shared Codex home and outside the symlink allowlist. The cache is additive: it +// The identity-keyed host credential store keeps one usable subscription +// credential per identity (`account_id`) in a SEPARATE host tree, outside the +// shared Codex home and outside the symlink allowlist. Each identity's entry +// directory doubles as that account's addressable home: the device-login +// promotion writes the durable credential there, and a company secret names the +// directory by path. The store also still backs the identity-anchored vend: it // never changes the copy-back path, the fail-closed decision predicate, or the -// host default store overwrite. It only refreshes an identity the host already -// holds. It never seeds an empty host store and never picks a credential at +// host default store overwrite, it only refreshes an identity the host already +// holds, it never seeds an empty host store, and it never picks a credential at // random. const CACHE_DIR_NAME = "codex-auth-cache"; @@ -110,18 +114,25 @@ export function resolveCodexAuthCacheDir( /** * Resolves the entry path for one identity: `//auth.json`. - * The `account_id` is sanitized by {@link toCacheKey}. After the join, this - * verifies the resolved entry path stays under the cache root and ends at exactly - * `/auth.json`. This function does no filesystem work; it is safe - * for a read path (the vend and the clear). (Security condition 3.) + * The `account_id` is validated first by {@link toAccountHandle} (a strict + * allowlist), the entry point of this function, then sanitized again by + * {@link toCacheKey} (a denylist) as a second, independent layer. After the + * join, this verifies the resolved entry path stays under the cache root and + * ends at exactly `/auth.json`. This function does no filesystem + * work; it is safe for a read path (the vend and the clear). (Security + * condition 3.) */ export function resolveCodexAuthCacheEntryPath( env: NodeJS.ProcessEnv = process.env, accountId: string, companyId: string, ): string { + const handle = toAccountHandle(accountId); + if (!handle) { + throw new Error("codex auth cache: account_id is not a valid account handle"); + } const resolvedRoot = resolveCodexAuthCacheDir(env, companyId); - const safeKey = toCacheKey(accountId); + const safeKey = toCacheKey(handle); const entryDir = path.resolve(resolvedRoot, safeKey); const entryPath = path.resolve(entryDir, CACHE_ENTRY_FILE); const expectedEntryPath = path.join(resolvedRoot, safeKey, CACHE_ENTRY_FILE); @@ -156,8 +167,9 @@ async function ensurePrivateDir(dir: string): Promise { /** * Resolves the entry path and creates the cache root and the entry directory - * private (mode 0700), each guarded by `lstat`. Use this on the write path - * before the cache slot is written. + * private (mode 0700), each guarded by `lstat`. The `account_id` is validated + * at the entry point of this function through {@link resolveCodexAuthCacheEntryPath}. + * Use this on the write path before the entry is written. */ export async function ensureCodexAuthCacheEntryDir( env: NodeJS.ProcessEnv = process.env, @@ -170,11 +182,194 @@ export async function ensureCodexAuthCacheEntryDir( return entryPath; } +export interface EnsureCodexAuthCacheEntryDirExclusiveResult { + /** The entry path: `/auth.json`. */ + entryPath: string; + /** True only when this exact call found the account's own home directory + * absent and created it. */ + created: boolean; +} + +/** + * Same as {@link ensureCodexAuthCacheEntryDir}, but the absence check and the + * directory creation run inside one lock keyed to the company-scoped cache + * root. Two different logins for the SAME Codex account can promote at the + * same time (each login owns its own promotion slot), so a plain + * check-then-create sequence lets both concurrent callers see the slot as + * absent and both report `created: true`. Under this lock, only the caller + * that truly finds the slot absent gets `created: true`; the other caller + * correctly reports `created: false` and so never deletes a home the first + * caller's login already wrote to and named with a company secret. + */ +export async function ensureCodexAuthCacheEntryDirExclusive( + env: NodeJS.ProcessEnv = process.env, + accountId: string, + companyId: string, +): Promise { + const cacheRoot = resolveCodexAuthCacheDir(env, companyId); + await ensurePrivateDir(cacheRoot); + return withDirectoryMergeLock( + cacheRoot, + async () => { + const entryPath = resolveCodexAuthCacheEntryPath(env, accountId, companyId); + const entryDir = path.dirname(entryPath); + const created = await lstat(entryDir) + .then(() => false) + .catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return true; + throw error; + }); + await ensurePrivateDir(entryDir); + return { entryPath, created }; + }, + env, + ); +} + +/** + * Resolves a per-company, named lock target directory under the same + * instance-scoped `companies//` tree the cache root and the + * promotion lock use. Two callers that pass the same `lockName` for the same + * `companyId` always resolve the same directory, so they share one lock; two + * different `lockName` values never share a lock key, so a caller that holds + * one of these named locks can still call {@link ensureCodexAuthCacheEntryDirExclusive} + * (which locks the cache root) or another named lock without nesting a lock + * inside itself. + */ +function resolveCodexAuthCacheNamedLockDir( + env: NodeJS.ProcessEnv = process.env, + companyId: string, + lockName: string, +): string { + const safeCompanyId = toSafePathSegment(companyId, "companyId"); + const instanceRoot = resolvePaperclipInstanceRootForAdapter({ + homeDir: nonEmpty(env.PAPERCLIP_HOME) ?? undefined, + instanceId: nonEmpty(env.PAPERCLIP_INSTANCE_ID) ?? undefined, + env, + }); + return path.resolve(instanceRoot, "companies", safeCompanyId, lockName); +} + +/** + * Serializes one company's whole device-login promotion sequence: the + * account-home directory create-or-reuse decision, the credential write, and + * the secret bind or cleanup that follows. Two different logins for the SAME + * Codex account run this sequence one at a time under this lock, so no login + * can decide to delete the shared account-home directory while another + * login's flow is still mid-way through writing its credential or binding + * its own secret to that same directory. + * + * `ensureCodexAuthCacheEntryDirExclusive` alone is not enough: it locks only + * the short directory-creation step, so its lock is already released by the + * time a login reaches the secret bind. A second login can then write its + * credential and be about to bind its own secret while the first login's + * later, unrelated secret-write failure decides to remove the directory both + * logins now share, deleting a credential home the second login still needs. + * Holding this lock across the full sequence for every login closes that + * window: a login that must clean up its own directory always finishes that + * cleanup, including the directory-recreation of any later login that lands + * on the now-empty slot, before the next login's sequence starts. + */ +export async function withCodexAccountHomePromotionLock( + env: NodeJS.ProcessEnv = process.env, + companyId: string, + fn: () => Promise, +): Promise { + const lockDir = resolveCodexAuthCacheNamedLockDir(env, companyId, "codex-auth-cache-promotion-lock"); + await ensurePrivateDir(lockDir); + return withDirectoryMergeLock(lockDir, fn, env); +} + +/** + * Serializes one company's writes to the literal value of any + * `local_encrypted` company secret against the account-home cleanup's + * claimant scan in {@link withCodexAccountHomePromotionLock}'s caller. Only a + * `local_encrypted` secret can hold a literal directory path, so this is the + * one provider a cleanup claimant scan must trust. Without this lock, a + * secret create or rotation can commit its new value after the scan's last + * pass finds no claimant but before the cleanup deletes the directory: the + * scan and the write never observe each other, so the write's new value + * survives past a directory the delete has already removed. + * + * The secrets service holds this lock for the full duration of a + * `local_encrypted` secret's create or rotate call. A cleanup claimant scan + * holds it for the full duration of its final check-and-delete step. The two + * critical sections can then never interleave: a write that starts first + * finishes (and becomes visible to the scan) before the scan's lock-holding + * check runs, and a write that starts after the scan's lock-holding check + * finishes only commits once the scan (and any delete it decided on) is + * done, so it can never name a directory the delete just removed without the + * scan having had a chance to see it first. + * + * This is a separate lock directory from + * {@link withCodexAccountHomePromotionLock}'s, so a device-login promotion + * (which holds that lock for its whole sequence, including its own secret + * create) can still call into a secrets-service write that takes this lock + * without deadlocking on itself. + */ +export async function withAccountHomeSecretMutationLock( + env: NodeJS.ProcessEnv = process.env, + companyId: string, + fn: () => Promise, +): Promise { + const lockDir = resolveCodexAuthCacheNamedLockDir(env, companyId, "codex-account-home-secret-mutation-lock"); + await ensurePrivateDir(lockDir); + return withDirectoryMergeLock(lockDir, fn, env); +} + +/** + * Confirms a `local_encrypted` secret value that names a directory under this + * company's Codex account-home cache root still exists on disk. Call this + * inside {@link withAccountHomeSecretMutationLock}, before a create or a + * rotate commits the value. + * + * The lock alone stops a write and the account-home cleanup's + * check-and-delete from interleaving; it does not stop them from running in + * either order. When the cleanup's check-and-delete runs first — it finds no + * secret names the directory, because this write had not committed yet, and + * removes the directory — a write that was only queued behind the lock still + * goes on, once the lock frees, to commit the very directory the cleanup + * just removed. That would leave an active secret naming a directory that + * does not exist. This check closes that window: a write whose value would + * name a directory the cleanup already removed fails instead of committing. + * + * A value outside this company's cache root is left alone. Only a value + * under the cache root can ever be an account-home directory, so this never + * rejects an unrelated `local_encrypted` secret write, such as an API key or + * a hand-typed token. + */ +export async function assertAccountHomeCacheDirStillValid( + env: NodeJS.ProcessEnv = process.env, + companyId: string, + value: string, +): Promise { + const cacheRoot = resolveCodexAuthCacheDir(env, companyId); + if (!value.startsWith(cacheRoot + path.sep)) return; + const exists = await lstat(value) + .then(() => true) + .catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return false; + throw error; + }); + if (!exists) { + throw new Error( + "codex auth cache: account-home directory no longer exists; refusing to write a secret that names it", + ); + } +} + /** * Reads the usable subscription `account_id` from an `auth.json` payload. Returns * `null` for an absent, unusable, or api-key credential (no subscription * identity). This mirrors `parseAuth` in `codex-auth-merge-decision.cjs`; keep * the two in step when the auth format changes. + * + * The returned value is the exact, untrimmed `account_id` string. A caller + * passes it on to {@link toAccountHandle} unchanged: that function is the + * one place that decides whether surrounding whitespace is acceptable, and it + * must see the raw value to reject an identifier that differs from another + * one only by whitespace. Trimming here, before that check runs, would let + * two distinct identifiers collapse onto the same account handle. */ export function readSubscriptionAccountId(bytes: Buffer): string | null { let parsed: unknown; @@ -196,7 +391,10 @@ export function readSubscriptionAccountId(bytes: Buffer): string | null { return null; } const tokenRecord = tokens as Record; - const accountId = typeof tokenRecord.account_id === "string" ? tokenRecord.account_id.trim() : ""; + const rawAccountId = typeof tokenRecord.account_id === "string" ? tokenRecord.account_id : ""; + // A blank-or-whitespace-only value is absent; a value with real content + // keeps its exact, untrimmed form. + const accountId = rawAccountId.trim().length > 0 ? rawAccountId : ""; const hasTokenMaterial = ["id_token", "access_token", "refresh_token"].some((key) => { const value = tokenRecord[key]; return typeof value === "string" && value.trim().length > 0; diff --git a/packages/adapters/codex-local/src/server/index.ts b/packages/adapters/codex-local/src/server/index.ts index 9db6ea0bb1..cdbfb01822 100644 --- a/packages/adapters/codex-local/src/server/index.ts +++ b/packages/adapters/codex-local/src/server/index.ts @@ -40,7 +40,14 @@ export { type CredentialReadinessResult, type PromoteDeviceLoginCredentialInput, type PromoteDeviceLoginCredentialOutcome, + type PromoteDeviceLoginCredentialResult, } from "./adapter-auth-promotion.js"; +export { + withCodexAccountHomePromotionLock, + withAccountHomeSecretMutationLock, + assertAccountHomeCacheDirStillValid, + resolveCodexAuthCacheDir, +} from "./codex-auth-cache.js"; export { parseCodexJsonl, isCodexHarnessCrash, isCodexProviderQuotaError, isCodexTransientUpstreamError, isCodexUnknownSessionError } from "./parse.js"; export { getQuotaWindows, diff --git a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts index c2efc24b04..41da01fb5a 100644 --- a/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts +++ b/packages/plugins/sandbox-providers/daytona/src/plugin.test.ts @@ -2806,7 +2806,13 @@ describe("Daytona sandbox provider plugin", () => { ], }); // Let syncIn register on the activity gate and reach the hung upload. - await new Promise((resolve) => setTimeout(resolve, 0)); + // The real `fs.stat` and `mkdir` round trip run before the upload call, + // so a fixed tick count can race ahead of them on a slower or busier + // host. Poll for the actual upload call instead of guessing a tick + // count, so this assertion never fires before syncIn reaches the hang. + await vi.waitFor(() => { + expect(sandbox.fs.uploadFiles).toHaveBeenCalled(); + }); const cancelPromise = plugin.definition.onEnvironmentCancelInteractiveSetup?.({ driverKey: "daytona", diff --git a/packages/shared/src/account-handle.test.ts b/packages/shared/src/account-handle.test.ts new file mode 100644 index 0000000000..9e6ca78971 --- /dev/null +++ b/packages/shared/src/account-handle.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from "vitest"; +import { ACCOUNT_HANDLE_MAX_LENGTH, toAccountHandle } from "./account-handle.js"; +import { createUserSecretDefinitionSchema } from "./validators/secret.js"; + +describe("toAccountHandle", () => { + it("returns the value for a plain identifier", () => { + expect(toAccountHandle("acct-42")).toBe("acct-42"); + }); + + it("returns null for a value that holds a space, such as \"a b\"", () => { + expect(toAccountHandle("a b")).toBeNull(); + }); + + it("returns null for a value with surrounding whitespace instead of trimming it", () => { + // Trimming a value before validation would let " acct-42" and "acct-42" + // resolve to the same handle. Two distinct identifiers must never share + // one handle, so surrounding whitespace is a rejection, not something to + // strip. + expect(toAccountHandle(" acct-42")).toBeNull(); + expect(toAccountHandle("acct-42 ")).toBeNull(); + expect(toAccountHandle(" acct-42 ")).toBeNull(); + expect(toAccountHandle("\tacct-42\n")).toBeNull(); + // The plain identifier with no surrounding whitespace still passes. + expect(toAccountHandle("acct-42")).toBe("acct-42"); + }); + + it("returns null for an empty string", () => { + expect(toAccountHandle("")).toBeNull(); + }); + + it("returns null for a value that holds a plus sign, such as \"a+b\"", () => { + expect(toAccountHandle("a+b")).toBeNull(); + }); + + it("returns null for a value that holds a colon, such as \"a:b\"", () => { + expect(toAccountHandle("a:b")).toBeNull(); + }); + + it("returns null for a value longer than 100 characters", () => { + const tooLong = "a".repeat(ACCOUNT_HANDLE_MAX_LENGTH + 1); + expect(toAccountHandle(tooLong)).toBeNull(); + const atLimit = "a".repeat(ACCOUNT_HANDLE_MAX_LENGTH); + expect(toAccountHandle(atLimit)).toBe(atLimit); + }); + + it("returns null for \".\" and for \"..\"", () => { + expect(toAccountHandle(".")).toBeNull(); + expect(toAccountHandle("..")).toBeNull(); + }); + + it("returns null for a value that starts with a hyphen, such as \"-rf\"", () => { + expect(toAccountHandle("-rf")).toBeNull(); + }); + + it("returns null for a value that holds a shell metacharacter, such as \"$(id)\"", () => { + expect(toAccountHandle("$(id)")).toBeNull(); + }); + + it("returns null for a value that holds a path separator or a NUL byte", () => { + expect(toAccountHandle("a/b")).toBeNull(); + expect(toAccountHandle("a\\b")).toBeNull(); + expect(toAccountHandle("a\0b")).toBeNull(); + }); + + it("every accepted handle also passes the secret key schema", () => { + const candidates = ["acct-42", "acct_42", "ACCT.42", "a".repeat(ACCOUNT_HANDLE_MAX_LENGTH)]; + for (const candidate of candidates) { + const handle = toAccountHandle(candidate); + expect(handle).not.toBeNull(); + expect(createUserSecretDefinitionSchema.shape.key.safeParse(handle).success).toBe(true); + } + }); +}); diff --git a/packages/shared/src/account-handle.ts b/packages/shared/src/account-handle.ts new file mode 100644 index 0000000000..4bc5d6ec16 --- /dev/null +++ b/packages/shared/src/account-handle.ts @@ -0,0 +1,44 @@ +// A canonical account handle names one login account inside one company. Two +// downstream sinks each guard the raw account identifier with a different +// validator: a directory-segment denylist, and the secret-key allowlist +// (`packages/shared/src/validators/secret.ts`). `toAccountHandle` accepts a +// value only when it satisfies both, so one handle can safely name both a +// directory and a secret. + +/** The longest accepted account handle. The secret key limit is 120 + * characters, and the longest name prefix a caller adds is `CODEX_HOME_` at + * 11 characters, so 100 leaves headroom. */ +export const ACCOUNT_HANDLE_MAX_LENGTH = 100; + +// The same character class the secret key schema uses. +const ACCOUNT_HANDLE_PATTERN = /^[a-zA-Z0-9_.-]+$/; + +/** + * Converts a raw account identifier into a safe account handle. Returns + * `null` when the value cannot become one instead of throwing, so a caller + * decides how to fail the operation. + * + * The identifier is the account's identity key: it names one directory and + * one secret, so two distinct identifiers must never resolve to the same + * handle. A caller must never normalize the value before this check (for + * example, by trimming it) — normalizing first can alias a distinct + * identifier onto an already-claimed handle and let a login accept a + * different account's credential home as its own. + * + * An input becomes a handle only when every rule below is true: + * + * - It carries no leading or trailing whitespace. + * - It matches `/^[a-zA-Z0-9_.-]+$/`. + * - It is {@link ACCOUNT_HANDLE_MAX_LENGTH} characters or shorter. + * - It is neither `.` nor `..`. + * - It does not start with `-` (a leading `-` reads as a command-line option). + */ +export function toAccountHandle(rawAccountId: string): string | null { + if (typeof rawAccountId !== "string" || rawAccountId.length === 0) return null; + if (rawAccountId.trim() !== rawAccountId) return null; + if (!ACCOUNT_HANDLE_PATTERN.test(rawAccountId)) return null; + if (rawAccountId.length > ACCOUNT_HANDLE_MAX_LENGTH) return null; + if (rawAccountId === "." || rawAccountId === "..") return null; + if (rawAccountId.startsWith("-")) return null; + return rawAccountId; +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index bd898d4f16..3afb7521ec 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -2726,3 +2726,4 @@ export { isPaperclipDevRunnerCommand, rewriteUrlHostToLoopback, } from "./runtime-exposure/loopback-bind.js"; +export { ACCOUNT_HANDLE_MAX_LENGTH, toAccountHandle } from "./account-handle.js"; diff --git a/server/src/__tests__/agent-device-login-routes.test.ts b/server/src/__tests__/agent-device-login-routes.test.ts index 23d094f340..2fd3657310 100644 --- a/server/src/__tests__/agent-device-login-routes.test.ts +++ b/server/src/__tests__/agent-device-login-routes.test.ts @@ -1,6 +1,10 @@ import express from "express"; import request from "supertest"; +import { lstat, mkdtemp, rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { withAccountHomeSecretMutationLock } from "@paperclipai/adapter-codex-local/server"; import { AdapterAuthSessionConflictError } from "../services/device-login-service.js"; import type { AdapterAuthSessionRow, @@ -68,6 +72,26 @@ const mockSecretService = vi.hoisted(() => ({ secretKeys: new Set(), manifest: [], })), + // The account-home secret lookup and creation. Absent by default, so a + // successful promotion creates it; a test overrides either to drive the + // idempotent-repeat-login and secret-creation-failure paths. + getByName: vi.fn(async (_companyId: string, _name: string) => null as Record | null), + create: vi.fn(async (_companyId: string, _input: Record) => ({ id: "secret-1" })), + // The company-wide secret listing the cleanup-safety scan reads. Empty by + // default, so a cleanup that finds no other claimant removes the + // directory; a test overrides it with a differently named secret to prove + // the scan finds a claimant a name-only check would miss. + list: vi.fn( + async (_companyId: string) => [] as Array<{ id: string; name: string; provider: string }>, + ), + // The pre-existing-secret value check. Defaults to a value that never + // matches an account home, so a test that leaves this unset and still + // reaches an existing-secret branch fails loud instead of passing by + // accident; a test overrides it to the account home under test. + resolveSecretValueForDeviceLoginCheck: vi.fn( + async (_companyId: string, _secretId: string, _context: { configPath: string }) => + "/unset-mock-value" as string, + ), })); const mockEnvironmentService = vi.hoisted(() => ({ @@ -341,10 +365,20 @@ const loginPath = (companyId: string, type = "codex_local") => `/api/companies/${companyId}/adapters/${type}/login-sessions`; describe("adapter device-login routes", () => { + // Real temp directories a test plants as an account home, cleaned up in + // `afterEach` whether or not the route removed them itself. + const accountHomeTestDirs: string[] = []; + beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); - mockDeviceLoginPromotion.mockResolvedValue("promoted"); + mockDeviceLoginPromotion.mockResolvedValue({ + outcome: "promoted", + accountId: "acct-default", + accountHomeDir: "/tmp/paperclip-codex-account-home/acct-default", + }); + mockSecretService.getByName.mockResolvedValue(null); + mockSecretService.create.mockResolvedValue({ id: "secret-1" }); harness.store = createMemoryStore(); harness.runtime = createFakeRuntime(); harness.acquisitions = []; @@ -376,9 +410,14 @@ describe("adapter device-login routes", () => { ); }); - afterEach(() => { + afterEach(async () => { // Release the gate, so every in-flight login run ends and clears its timer. harness.releaseGate(); + while (accountHomeTestDirs.length > 0) { + const dir = accountHomeTestDirs.pop(); + if (!dir) continue; + await rm(dir, { recursive: true, force: true }).catch(() => undefined); + } }); it("requires a board actor and rejects an agent-token start", async () => { @@ -726,6 +765,12 @@ describe("adapter device-login routes", () => { }); it("lets a second owner start an active login in the same company", async () => { + // Both logins bind the default account home, and the login service + // reconfirms the bound secret's value a second time right before it + // commits the terminal `authenticated` state. + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue( + "/tmp/paperclip-codex-account-home/acct-default", + ); const app = await createApp(); const first = await request(app) @@ -744,9 +789,31 @@ describe("adapter device-login routes", () => { expect(second.body.sessionId).not.toBe(first.body.sessionId); // Each owner's start acquires its own lease. expect(harness.acquisitions).toHaveLength(2); + // Drain both sessions to a terminal status before the test ends. Neither + // session's promotion has run yet (both still wait on the gate), so an + // undrained session keeps running after this test returns and can consume + // a later test's own mocked promotion result. A status read is owner + // scoped, so read each session back as the owner that started it. + harness.releaseGate(); + await vi.waitFor(async () => { + currentActor = boardActor(OWNER_A); + const firstStatus = await request(app).get(`${loginPath(COMPANY_1)}/${first.body.sessionId}`); + expect(firstStatus.body.status).toBe("authenticated"); + }); + await vi.waitFor(async () => { + currentActor = boardActor(OWNER_B); + const secondStatus = await request(app).get(`${loginPath(COMPANY_1)}/${second.body.sessionId}`); + expect(secondStatus.body.status).toBe("authenticated"); + }); }); it("returns 409 for a second active start in a different environment", async () => { + // The login binds the default account home, and the login service + // reconfirms the bound secret's value a second time right before it + // commits the terminal `authenticated` state. + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue( + "/tmp/paperclip-codex-account-home/acct-default", + ); const app = await createApp(); const first = await request(app) @@ -761,12 +828,24 @@ describe("adapter device-login routes", () => { .send({ environmentId: SANDBOX_ENV_2 }); expect(second.status, JSON.stringify(second.body)).toBe(409); expect(harness.acquisitions).toHaveLength(1); + // Drain the first session to a terminal status before the test ends. An + // undrained session keeps running after this test returns and can consume + // a later test's own mocked promotion result. + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${first.body.sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); }); it("fails closed when promotion loses the sole-owner claim", async () => { // This is the expiry/reaper-race result from Decision H. It is a normal // resolved adapter outcome, but it must never be accepted as authentication. - mockDeviceLoginPromotion.mockResolvedValueOnce("not_sole_owner"); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "not_sole_owner", + accountId: null, + accountHomeDir: null, + }); const app = await createApp(); const start = await request(app) @@ -790,12 +869,176 @@ describe("adapter device-login routes", () => { expect(mockDeviceLoginPromotion).toHaveBeenCalledTimes(1); }); - it("fails closed when the login is a different account than the company home", async () => { - // The promotion keeps the occupied company home and installs nothing durable - // for a different-identity login. The identity-anchored vend can never select - // this login, so a later run keeps the existing account. The route must fail - // the session instead of a report of `authenticated`. - mockDeviceLoginPromotion.mockResolvedValueOnce("kept_foreign_identity"); + it("a successful Codex login creates the company secret naming the account home", async () => { + // A second, different account no longer fails behind an occupied company + // home: each account gets its own home, named by a company secret. + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue( + "/tmp/paperclip-codex-account-home/acct-second", + ); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-second", + accountHomeDir: "/tmp/paperclip-codex-account-home/acct-second", + }); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); + + expect(mockSecretService.getByName).toHaveBeenCalledWith(COMPANY_1, "CODEX_HOME_acct-second"); + expect(mockSecretService.create).toHaveBeenCalledWith( + COMPANY_1, + expect.objectContaining({ + name: "CODEX_HOME_acct-second", + value: "/tmp/paperclip-codex-account-home/acct-second", + }), + expect.anything(), + ); + }); + + it("a repeat login for the same account creates no second secret", async () => { + // The secret already names this account's home, so the route reads it, + // confirms the value still matches, and creates nothing new: a repeat + // login is idempotent. + const accountHomeDir = "/tmp/paperclip-codex-account-home/acct-default"; + mockSecretService.getByName.mockResolvedValue({ id: "existing-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "kept", + accountId: "acct-default", + accountHomeDir, + }); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); + + expect(mockSecretService.create).not.toHaveBeenCalled(); + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).toHaveBeenCalledWith( + COMPANY_1, + "existing-secret", + expect.objectContaining({ configPath: "secrets.CODEX_HOME_acct-default" }), + ); + }); + + it("holds the account-home secret mutation lock while checking a pre-existing secret's value", async () => { + // A `local_encrypted` secret rotate takes `withAccountHomeSecretMutationLock` + // for the whole of its write. The pre-existing-secret check must run + // inside the same lock and return right after: otherwise a rotate could + // commit a new value in the gap between the check and the login + // reporting success, and the login would report success for a value + // that no longer names the account's own home. This proves the check + // waits for a lock held elsewhere, and only reads the secret's value + // once that lock frees. + const accountHomeDir = "/tmp/paperclip-codex-account-home/acct-default"; + mockSecretService.getByName.mockResolvedValue({ id: "existing-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "kept", + accountId: "acct-default", + accountHomeDir, + }); + + let releaseHeldLock!: () => void; + const heldLockGate = new Promise((resolve) => { + releaseHeldLock = resolve; + }); + const lockHolder = withAccountHomeSecretMutationLock(undefined, COMPANY_1, () => heldLockGate); + // Give the held lock a chance to actually acquire before the login + // starts racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + + const app = await createApp(); + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + harness.releaseGate(); + + // The login's check must stay blocked behind the held lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).not.toHaveBeenCalled(); + + releaseHeldLock(); + await lockHolder; + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).toHaveBeenCalled(); + }); + + it("reconfirms the account-home secret a second time immediately before the terminal authenticated commit", async () => { + // The early check inside `promote` releases its lock well before the + // login service records the terminal `authenticated` state, so it alone + // cannot prove the value still matches at that later point. The service + // must run the same check again, right before that commit, under a + // fresh lock acquisition: this test proves the second check runs, not + // only the first. + const accountHomeDir = "/tmp/paperclip-codex-account-home/acct-default"; + mockSecretService.getByName.mockResolvedValue({ id: "existing-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "kept", + accountId: "acct-default", + accountHomeDir, + }); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); + + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).toHaveBeenCalledTimes(2); + }); + + it("fails closed when the account-home secret is rotated after validation but before the login service commits its terminal state", async () => { + // A board user can rotate the matching `CODEX_HOME` secret after + // `promote`'s own check passes and before the login service commits its + // terminal state, because `promote`'s lock is fully released by then. + // The service must catch this with a second check it runs right before + // that commit, or it would report `authenticated` for a value that no + // longer names the account's own home. + const accountHomeDir = "/tmp/paperclip-codex-account-home/acct-default"; + mockSecretService.getByName.mockResolvedValue({ id: "existing-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck + // `promote`'s own early check: still matches. + .mockResolvedValueOnce(accountHomeDir) + // The rotation lands in the gap. The final check, immediately before + // the terminal commit, now reads the rotated value. + .mockResolvedValueOnce("/rotated/away/from/the/account/home"); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "kept", + accountId: "acct-default", + accountHomeDir, + }); const app = await createApp(); const start = await request(app) @@ -811,12 +1054,471 @@ describe("adapter device-login routes", () => { expect(status.body.failure?.reason).toBe("promotion_failed"); }); - const row = await (harness.store as ReturnType).getByPublicId( - sessionId, - COMPANY_1, + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).toHaveBeenCalledTimes(2); + }); + + it("fails closed when a pre-existing secret names a different account home", async () => { + // A stale or hand-entered secret at this name must not let the login + // report success while a bound agent reads the wrong credential home. + mockSecretService.getByName.mockResolvedValue({ id: "stale-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue( + "/some/other/path/left-over-from-before", ); - expect(row?.status).toBe("failed"); - expect(mockDeviceLoginPromotion).toHaveBeenCalledTimes(1); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "kept", + accountId: "acct-stale", + accountHomeDir: "/tmp/paperclip-codex-account-home/acct-stale", + }); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + expect(mockSecretService.create).not.toHaveBeenCalled(); + }); + + it("fails closed and removes the account home when secret creation fails", async () => { + // This login created the account home, so a secret-creation failure must + // fail the login and remove the directory it just created, so the + // operation stays atomic. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-broken-secret", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).rejects.toThrow(); + }); + + it("a secret creation failure keeps an account home that already existed", async () => { + // The account home directory pre-dates this login (a user deleted the + // secret but kept the directory, or an earlier login already wrote it). + // A secret-creation failure must still fail the login, but it must never + // remove a directory this login did not create. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-existing-home", + accountHomeDir, + accountHomeCreated: false, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("a secret name conflict succeeds and keeps the account home", async () => { + // Two logins for one account can both read no secret and then race the + // create call. The loser gets a 409 conflict, which proves the secret + // already exists. A repeat login for the same account must be idempotent, + // so the loser must confirm the winning secret's value, report a + // successful login, and must never remove the account home the winner + // just wrote. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-conflict", + accountHomeDir, + accountHomeCreated: true, + }); + // `vi.resetModules()` in `beforeEach` gives the route module a fresh copy + // of `../errors.js`. Import the conflict helper from that same fresh copy, + // so the route's `err instanceof HttpError` check sees a matching class. + const { conflict: freshConflict } = + await vi.importActual("../errors.js"); + mockSecretService.create.mockRejectedValueOnce( + freshConflict("a secret with this name already exists"), + ); + // The first read (before `create`) finds no secret; the second read + // (after the conflict) finds the winner's secret. + mockSecretService.getByName + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: "winning-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue(accountHomeDir); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("authenticated"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("fails closed when the secret that won the create race names a different account home", async () => { + // The 409 conflict proves some secret now holds this name, but that + // secret could belong to unrelated stale state, not the winner of a + // genuine same-account race. The login must not report success without + // checking the winning secret's value. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-conflict-mismatch", + accountHomeDir, + accountHomeCreated: true, + }); + const { conflict: freshConflict } = + await vi.importActual("../errors.js"); + mockSecretService.create.mockRejectedValueOnce( + freshConflict("a secret with this name already exists"), + ); + mockSecretService.getByName + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: "winning-secret" }); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValue( + "/some/other/path/left-over-from-before", + ); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + // The mismatch is a validation failure on the winner's secret, not this + // call's own directory, so this call must still never delete it. + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("does not remove an account home a concurrent login already claimed", async () => { + // This call created the account home directory (`accountHomeCreated: + // true`), but a concurrent login for the same account can still create + // the company secret before this call's own, unrelated secret-creation + // attempt fails. The directory now belongs to that other login's secret, + // so the cleanup scan must find it by value and keep the directory. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-concurrent-claim", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + // The first read (before `create`) finds no secret. + mockSecretService.getByName.mockResolvedValueOnce(null); + // The cleanup scan lists every company secret and resolves each one's + // value. The concurrent login's secret carries this call's own generated + // name, so this proves the scan still catches a same-name claimant. + mockSecretService.list.mockResolvedValueOnce([ + { id: "concurrent-secret", name: "CODEX_HOME_acct-concurrent-claim", provider: "local_encrypted" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValueOnce(accountHomeDir); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("does not remove an account home a differently named secret still references", async () => { + // A user can bind a hand-named secret to an account home directly, with + // no relation to the generated `CODEX_HOME_` name. A cleanup that + // checks only the generated name would miss this secret and delete a + // directory it still needs; the scan must catch it by value instead. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-aliased", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + mockSecretService.getByName.mockResolvedValueOnce(null); + mockSecretService.list.mockResolvedValueOnce([ + // An unrelated secret that resolves to a different value. The scan + // must not stop at the first entry and must not treat every secret as + // a match. + { id: "unrelated-secret", name: "SOME_OTHER_SECRET", provider: "local_encrypted" }, + { id: "hand-named-secret", name: "MY_CODEX_HOME", provider: "local_encrypted" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockImplementation( + async (_companyId: string, secretId: string, _context: { configPath: string }) => + secretId === "hand-named-secret" ? accountHomeDir : "/some/unrelated/path", + ); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("does not remove an account home an AWS Secrets Manager-backed secret still references", async () => { + // A secret's value is a plain string regardless of its provider, so a + // hand-named secret bound to this account home through AWS Secrets + // Manager is just as real a claimant as a `local_encrypted` one. The + // scan must resolve it too, not skip it for its provider. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-aws-claim", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + mockSecretService.getByName.mockResolvedValueOnce(null); + mockSecretService.list.mockResolvedValueOnce([ + { id: "aws-secret", name: "MY_CODEX_HOME", provider: "aws_secrets_manager" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValueOnce(accountHomeDir); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + expect(mockSecretService.resolveSecretValueForDeviceLoginCheck).toHaveBeenCalledWith( + COMPANY_1, + "aws-secret", + expect.objectContaining({ configPath: "secrets.MY_CODEX_HOME" }), + ); + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("removes the account home when the cleanup scan finds no claimant", async () => { + // No secret, under any name, resolves to this directory, so the cleanup + // must still remove it — the broader scan must not make cleanup any less + // eager than the old name-only check when nothing claims the directory. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-unclaimed", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + mockSecretService.getByName.mockResolvedValueOnce(null); + mockSecretService.list.mockResolvedValueOnce([ + { id: "unrelated-secret", name: "SOME_OTHER_SECRET", provider: "local_encrypted" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockResolvedValueOnce("/some/unrelated/path"); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).rejects.toThrow(); + }); + + it("preserves the account home when the cleanup scan cannot resolve a secret's value", async () => { + // A resolve failure on one secret is not proof that secret names a + // different directory: the failure can hit the exact secret that would + // have matched. The cleanup must keep the directory rather than treat an + // unresolved secret as a non-match, even when every secret that DID + // resolve named a different directory. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-unresolvable", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + mockSecretService.getByName.mockResolvedValueOnce(null); + mockSecretService.list.mockResolvedValueOnce([ + { id: "unrelated-secret", name: "SOME_OTHER_SECRET", provider: "local_encrypted" }, + { id: "unresolvable-secret", name: "MAYBE_CODEX_HOME", provider: "local_encrypted" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockImplementation( + async (_companyId: string, secretId: string, _context: { configPath: string }) => { + if (secretId === "unresolvable-secret") throw new Error("decryption key unavailable"); + return "/some/unrelated/path"; + }, + ); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("preserves the account home when a secret appears after the scan starts", async () => { + // The scan lists secrets once, then resolves each one in turn. A new + // secret can appear after that first list call, while the scan is still + // resolving an earlier secret. The scan must re-list and check that new + // secret too, instead of trusting its first, now-stale list. + const accountHomeDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-account-home-test-")); + accountHomeTestDirs.push(accountHomeDir); + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct-late-claim", + accountHomeDir, + accountHomeCreated: true, + }); + mockSecretService.create.mockRejectedValueOnce(new Error("db unavailable")); + mockSecretService.getByName.mockResolvedValueOnce(null); + // The first pass sees only the unrelated secret. By the time the scan + // re-lists, a second, differently named secret now names this account + // home; the scan must catch it on that later pass. + mockSecretService.list + .mockResolvedValueOnce([{ id: "unrelated-secret", name: "SOME_OTHER_SECRET", provider: "local_encrypted" }]) + .mockResolvedValueOnce([ + { id: "unrelated-secret", name: "SOME_OTHER_SECRET", provider: "local_encrypted" }, + { id: "late-secret", name: "LATE_CODEX_HOME", provider: "local_encrypted" }, + ]); + mockSecretService.resolveSecretValueForDeviceLoginCheck.mockImplementation( + async (_companyId: string, secretId: string, _context: { configPath: string }) => + secretId === "late-secret" ? accountHomeDir : "/some/unrelated/path", + ); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + await expect(lstat(accountHomeDir)).resolves.toBeDefined(); + }); + + it("fails closed when the account identifier cannot form a valid account home name", async () => { + // The promotion helper itself already rejects an identifier that cannot + // become an account handle; a defensive re-check in the route covers a + // promotion result the route did not fully trust. + mockDeviceLoginPromotion.mockResolvedValueOnce({ + outcome: "promoted", + accountId: "acct with space", + accountHomeDir: "/tmp/paperclip-codex-account-home/unused", + }); + const app = await createApp(); + + const start = await request(app) + .post(loginPath(COMPANY_1)) + .send({ environmentId: SANDBOX_ENV_1 }); + expect(start.status, JSON.stringify(start.body)).toBe(201); + const sessionId = start.body.sessionId as string; + + harness.releaseGate(); + await vi.waitFor(async () => { + const status = await request(app).get(`${loginPath(COMPANY_1)}/${sessionId}`); + expect(status.body.status).toBe("failed"); + expect(status.body.failure?.reason).toBe("promotion_failed"); + }); + + expect(mockSecretService.create).not.toHaveBeenCalled(); }); it("omits the URL, code, credential bytes, and lease id from logs and activity", async () => { diff --git a/server/src/__tests__/device-login-service.test.ts b/server/src/__tests__/device-login-service.test.ts index 73dea1d5dd..ad7c479dc2 100644 --- a/server/src/__tests__/device-login-service.test.ts +++ b/server/src/__tests__/device-login-service.test.ts @@ -382,6 +382,84 @@ describe("device login service", () => { expect(JSON.stringify(activity)).not.toContain(PROMPT_CODE); }); + it("wraps the terminal authenticated commit in a promotion's runTerminalCommit", async () => { + // A promotion that defines `runTerminalCommit` gets the chance to wrap the + // service's own terminal write, so it can hold a lock across a check it + // already ran once earlier in `promote` and the write that publishes + // `authenticated`. + const store = createMemoryStore(); + const { runtime } = createFakeRuntime({ + exec: execSuccess, + authBytes: Buffer.from('{"token":"secret"}'), + }); + const companyId = randomUUID(); + const wrapCalls: unknown[] = []; + const service = makeService({ + store, + runtime, + promotion: { + promote: () => {}, + async runTerminalCommit(commit, context) { + wrapCalls.push(context); + return commit(); + }, + }, + }); + const { session, completed } = await service.start({ + companyId, + environmentId: randomUUID(), + adapterType: ADAPTER_TYPE, + startedByUserId: OWNER_A, + }); + + const outcome = await completed; + expect(outcome.status).toBe("authenticated"); + expect(wrapCalls).toHaveLength(1); + const row = await store.getByPublicId(session.sessionId, companyId); + expect(row?.status).toBe("authenticated"); + }); + + it("records a failed terminal, and never authenticates, when runTerminalCommit rejects", async () => { + // A promotion's `runTerminalCommit` rejects when its own re-check, run + // immediately before the terminal write, finds the value it validated + // earlier no longer holds (for example, a rotation landed in the gap). + // The service must fail the login the same way a `promote` rejection + // does, never publishing `authenticated` for the stale value. + const store = createMemoryStore(); + const { runtime, deleteCalls } = createFakeRuntime({ + exec: execSuccess, + authBytes: Buffer.from('{"token":"secret"}'), + }); + const companyId = randomUUID(); + const service = makeService({ + store, + runtime, + promotion: { + promote: () => {}, + async runTerminalCommit() { + // A real promotion would run its re-check here, find the bound + // value stale, and throw before ever calling `commit`. + throw new Error("the bound value no longer matches"); + }, + }, + }); + const { session, completed } = await service.start({ + companyId, + environmentId: randomUUID(), + adapterType: ADAPTER_TYPE, + startedByUserId: OWNER_A, + }); + + const outcome = await completed; + expect(outcome.status).toBe("failed"); + const row = await store.getByPublicId(session.sessionId, companyId); + expect(row?.status).toBe("failed"); + expect(row?.failureReason).toBe("promotion_failed"); + // The service still deletes the sandbox on this failure path, the same + // as a `promote` rejection. + expect(deleteCalls).toHaveLength(1); + }); + it("gives a Grok prompt to a grok_local session, and the session surfaces the Grok code and URL", async () => { // This proves the Grok parser ran: the profile map resolves the parser from // the trusted adapter type, so a `grok_local` session runs the Grok parser, diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index bd30d873c2..53e73b2ff8 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -1,9 +1,11 @@ import { randomUUID } from "node:crypto"; import { mkdirSync, rmSync } from "node:fs"; +import { mkdir, rm } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { and, eq } from "drizzle-orm"; +import { resolveCodexAuthCacheDir, withAccountHomeSecretMutationLock } from "@paperclipai/adapter-codex-local/server"; import { activityLog, agents, @@ -88,6 +90,16 @@ describeEmbeddedPostgres("secretService", () => { return companyId; } + // Creates a real directory under this company's Codex account-home cache + // root, so a test can prove the write-time directory-validity check reads + // an actual filesystem entry, the same way the production cleanup and the + // production secret write do. + async function makeAccountHomeDir(companyId: string, accountHandle: string): Promise { + const accountHomeDir = path.join(resolveCodexAuthCacheDir(undefined, companyId), accountHandle); + await mkdir(accountHomeDir, { recursive: true }); + return accountHomeDir; + } + async function seedCompanyMember( companyId: string, userId: string, @@ -360,6 +372,446 @@ describeEmbeddedPostgres("secretService", () => { ).rejects.toThrow(/already exists/i); }); + it("serializes two local_encrypted secret creates in the same company so their writes never overlap", async () => { + // An account-home cleanup's claimant scan and a `local_encrypted` secret + // write share one lock (`withAccountHomeSecretMutationLock`), so a write + // can never commit inside the exact window the scan already used to + // decide no secret claims a directory it is about to delete. This proves + // the lock itself enforces that: two `local_encrypted` creates in the + // SAME company never run their provider write at the same time, + // whichever caller goes first. + const companyId = await seedCompany(); + const svc = secretService(db); + const events: string[] = []; + let releaseFirstWrite!: () => void; + const firstWriteGate = new Promise((resolve) => { + releaseFirstWrite = resolve; + }); + const originalCreateSecret = localEncryptedProvider.createSecret.bind(localEncryptedProvider); + vi.spyOn(localEncryptedProvider, "createSecret") + .mockImplementationOnce(async (input) => { + events.push("first-provider-enter"); + await firstWriteGate; + events.push("first-provider-exit"); + return originalCreateSecret(input); + }) + .mockImplementationOnce(async (input) => { + events.push("second-provider-enter"); + return originalCreateSecret(input); + }); + + const firstCreate = svc.create(companyId, { + name: `account-home-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-a", + }); + // Give the first call a chance to acquire the lock and enter its provider + // write before the second call starts racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + const secondCreate = svc.create(companyId, { + name: `hand-named-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-a", + }); + // The second call must stay blocked on the lock while the first call + // still holds it: it must never enter its own provider write before the + // first call's provider write exits. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["first-provider-enter"]); + + releaseFirstWrite(); + await Promise.all([firstCreate, secondCreate]); + expect(events).toEqual(["first-provider-enter", "first-provider-exit", "second-provider-enter"]); + }); + + it("serializes a local_encrypted secret rotate against a concurrent create of a different secret in the same company", async () => { + // Same lock, the other write path: a rotate that writes a new + // `local_encrypted` value must serialize against a concurrent create the + // same way a create serializes against another create. + const companyId = await seedCompany(); + const svc = secretService(db); + const existing = await svc.create(companyId, { + name: `existing-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-b", + }); + const events: string[] = []; + let releaseRotateWrite!: () => void; + const rotateWriteGate = new Promise((resolve) => { + releaseRotateWrite = resolve; + }); + const originalCreateVersion = localEncryptedProvider.createVersion.bind(localEncryptedProvider); + vi.spyOn(localEncryptedProvider, "createVersion").mockImplementationOnce(async (input) => { + events.push("rotate-provider-enter"); + await rotateWriteGate; + events.push("rotate-provider-exit"); + return originalCreateVersion(input); + }); + const originalCreateSecret = localEncryptedProvider.createSecret.bind(localEncryptedProvider); + vi.spyOn(localEncryptedProvider, "createSecret").mockImplementationOnce(async (input) => { + events.push("create-provider-enter"); + return originalCreateSecret(input); + }); + + const rotateCall = svc.rotate(existing.id, { value: "/company/codex-home/acct-b-rotated" }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const createCall = svc.create(companyId, { + name: `hand-named-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-b", + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["rotate-provider-enter"]); + + releaseRotateWrite(); + await Promise.all([rotateCall, createCall]); + expect(events).toEqual(["rotate-provider-enter", "rotate-provider-exit", "create-provider-enter"]); + }); + + it("fails a queued local_encrypted create when an account-home cleanup removes its directory first", async () => { + // The mutation lock alone stops a write and an account-home cleanup's + // check-and-delete from interleaving; it does not stop them from running + // in either order. When the cleanup wins the lock first, deletes the + // directory, and releases the lock, a create that was only queued behind + // it must not go on to commit that now-deleted directory as a secret + // value. It must fail instead. + const companyId = await seedCompany(); + const svc = secretService(db); + const accountHomeDir = await makeAccountHomeDir(companyId, "acct-queued-create"); + + const events: string[] = []; + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + events.push("cleanup-enter"); + await cleanupGate; + await rm(accountHomeDir, { recursive: true, force: true }); + events.push("cleanup-exit"); + }); + // Give the cleanup a chance to acquire the lock before the create starts + // racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + const createCall = svc.create(companyId, { + name: `account-home-${randomUUID()}`, + provider: "local_encrypted", + value: accountHomeDir, + }); + // The create must stay queued behind the held lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["cleanup-enter"]); + + releaseCleanup(); + await cleanupCall; + await expect(createCall).rejects.toThrow(/no longer exists/); + }); + + it("fails a queued local_encrypted rotate when an account-home cleanup removes its directory first", async () => { + // Same race, the other write path: a rotate that would commit a + // now-deleted account-home directory must fail the same way a create + // does. + const companyId = await seedCompany(); + const svc = secretService(db); + const existing = await svc.create(companyId, { + name: `hand-named-${randomUUID()}`, + provider: "local_encrypted", + value: "/some/unrelated/placeholder/value", + }); + const accountHomeDir = await makeAccountHomeDir(companyId, "acct-queued-rotate"); + + const events: string[] = []; + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + events.push("cleanup-enter"); + await cleanupGate; + await rm(accountHomeDir, { recursive: true, force: true }); + events.push("cleanup-exit"); + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const rotateCall = svc.rotate(existing.id, { value: accountHomeDir }); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["cleanup-enter"]); + + releaseCleanup(); + await cleanupCall; + await expect(rotateCall).rejects.toThrow(/no longer exists/); + }); + + it("serializes an aws_secrets_manager secret create against a concurrent account-home mutation lock holder", async () => { + // A plain string value can equal a Codex account-home path regardless of + // which provider stores it. Prove a non-local (`aws_secrets_manager`) + // create now takes the SAME `withAccountHomeSecretMutationLock` a + // `local_encrypted` create takes, not only when the create's own + // provider is `local_encrypted`. + const companyId = await seedCompany(); + const svc = secretService(db); + const awsVault = await svc.createProviderConfig(companyId, { + provider: "aws_secrets_manager", + displayName: "AWS production", + config: { region: "us-east-1", namespace: "prod-use1" }, + }); + vi.spyOn(awsSecretsManagerProvider, "createSecret").mockResolvedValue({ + material: { + scheme: "aws_secrets_manager_v1", + secretId: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret", + versionId: "aws-version-1", + source: "managed", + }, + valueSha256: "value-sha-1", + fingerprintSha256: "fingerprint-sha-1", + externalRef: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret", + providerVersionRef: "aws-version-1", + }); + + const events: string[] = []; + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + events.push("cleanup-enter"); + await cleanupGate; + events.push("cleanup-exit"); + }); + // Give the cleanup a chance to acquire the lock before the AWS create + // starts racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + const createCall = svc.create(companyId, { + name: `aws-secret-${randomUUID()}`, + provider: "aws_secrets_manager", + providerConfigId: awsVault.id, + value: "runtime-secret", + }); + // The AWS create must stay queued behind the held lock, the same way a + // `local_encrypted` create does. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["cleanup-enter"]); + + releaseCleanup(); + await cleanupCall; + const created = await createCall; + expect(events).toEqual(["cleanup-enter", "cleanup-exit"]); + expect(created.status).toBe("active"); + }); + + it("serializes an aws_secrets_manager secret rotate against a concurrent account-home mutation lock holder", async () => { + // Same reasoning as the create test above, the other write path: a + // non-local rotate must also take the lock, not only a + // `local_encrypted` rotate. + const companyId = await seedCompany(); + const svc = secretService(db); + const awsVault = await svc.createProviderConfig(companyId, { + provider: "aws_secrets_manager", + displayName: "AWS production", + config: { region: "us-east-1", namespace: "prod-use1" }, + }); + vi.spyOn(awsSecretsManagerProvider, "createSecret").mockResolvedValue({ + material: { + scheme: "aws_secrets_manager_v1", + secretId: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret-rotate", + versionId: "aws-version-1", + source: "managed", + }, + valueSha256: "value-sha-1", + fingerprintSha256: "fingerprint-sha-1", + externalRef: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret-rotate", + providerVersionRef: "aws-version-1", + }); + vi.spyOn(awsSecretsManagerProvider, "createVersion").mockResolvedValue({ + material: { + scheme: "aws_secrets_manager_v1", + secretId: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret-rotate", + versionId: "aws-version-2", + source: "managed", + }, + valueSha256: "value-sha-2", + fingerprintSha256: "fingerprint-sha-2", + externalRef: "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/aws-secret-rotate", + providerVersionRef: "aws-version-2", + }); + const existing = await svc.create(companyId, { + name: `aws-secret-rotate-${randomUUID()}`, + provider: "aws_secrets_manager", + providerConfigId: awsVault.id, + value: "runtime-secret", + }); + + const events: string[] = []; + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + events.push("cleanup-enter"); + await cleanupGate; + events.push("cleanup-exit"); + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + const rotateCall = svc.rotate(existing.id, { value: "rotated-runtime-secret" }); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["cleanup-enter"]); + + releaseCleanup(); + await cleanupCall; + const rotated = await rotateCall; + expect(events).toEqual(["cleanup-enter", "cleanup-exit"]); + expect(rotated.latestVersion).toBe(2); + }); + + it("serializes a rename, archive, or disable update against a concurrent account-home mutation lock holder", async () => { + // A rename or a status change to `archived` or `disabled` can stop the + // generated account-home secret from resolving to the value a + // device-login promotion already validated. Prove `update` now holds + // the SAME lock a cleanup (or a promotion's terminal-commit re-check) + // holds for its whole critical section, so an update can never land + // inside that section. + const companyId = await seedCompany(); + const svc = secretService(db); + const secret = await svc.create(companyId, { + name: `CODEX_HOME_update-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-update", + }); + + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + await cleanupGate; + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + + let updateResolved = false; + const updateCall = svc.update(secret.id, { status: "archived" }).then((result) => { + updateResolved = true; + return result; + }); + // The update must stay queued behind the held lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(updateResolved).toBe(false); + + releaseCleanup(); + await cleanupCall; + const updated = await updateCall; + expect(updateResolved).toBe(true); + expect(updated?.status).toBe("archived"); + }); + + it("serializes a secret delete against a concurrent account-home mutation lock holder", async () => { + // Same reasoning as the update test above: a delete must also take the + // lock, so it can never land inside a cleanup's, or a promotion's + // terminal-commit re-check's, critical section. + const companyId = await seedCompany(); + const svc = secretService(db); + const secret = await svc.create(companyId, { + name: `CODEX_HOME_delete-${randomUUID()}`, + provider: "local_encrypted", + value: "/company/codex-home/acct-delete", + }); + + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + await cleanupGate; + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + + let removeResolved = false; + const removeCall = svc.remove(secret.id).then((result) => { + removeResolved = true; + return result; + }); + // The delete must stay queued behind the held lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(removeResolved).toBe(false); + + releaseCleanup(); + await cleanupCall; + await removeCall; + expect(removeResolved).toBe(true); + const remaining = await svc.getById(secret.id); + expect(remaining).toBeNull(); + }); + + it("commits a local_encrypted secret naming an account-home directory that still exists", async () => { + // A regression guard for the check above: a create whose value names a + // directory that is still present must keep succeeding, unblocked. + const companyId = await seedCompany(); + const svc = secretService(db); + const accountHomeDir = await makeAccountHomeDir(companyId, "acct-still-present"); + + const created = await svc.create(companyId, { + name: `account-home-${randomUUID()}`, + provider: "local_encrypted", + value: accountHomeDir, + }); + expect(created.status).toBe("active"); + }); + + it("fails a queued adapter-schema-secret create when an account-home cleanup removes its directory first", async () => { + // `normalizeAdapterConfigForPersistence` creates an adapter config secret + // through a separate managed-create path (`createManagedLocalSecret`), + // not `svc.create`. It must run the same queued-behind-the-lock + // directory check as `svc.create` and `svc.rotate`, so an adapter + // schema secret can never commit an account-home directory a cleanup + // already removed while this call waited for the lock. + const companyId = await seedCompany(); + const svc = secretService(db); + const accountHomeDir = await makeAccountHomeDir(companyId, "acct-queued-adapter-secret"); + + const events: string[] = []; + let releaseCleanup!: () => void; + const cleanupGate = new Promise((resolve) => { + releaseCleanup = resolve; + }); + const cleanupCall = withAccountHomeSecretMutationLock(undefined, companyId, async () => { + events.push("cleanup-enter"); + await cleanupGate; + await rm(accountHomeDir, { recursive: true, force: true }); + events.push("cleanup-exit"); + }); + // Give the cleanup a chance to acquire the lock before the adapter + // config normalization starts racing for the same lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + const normalizeCall = svc.normalizeAdapterConfigForPersistence( + companyId, + { apiKey: accountHomeDir }, + { adapterType: "hermes_gateway" }, + ); + // The queued create must stay blocked behind the held lock. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["cleanup-enter"]); + + releaseCleanup(); + await cleanupCall; + await expect(normalizeCall).rejects.toThrow(/no longer exists/); + }); + + it("keeps an adapter-schema-secret create working for a value outside the account-home cache root", async () => { + // A regression guard for the check above: an adapter schema secret whose + // value is not an account-home directory (an ordinary API key, for + // example) must keep succeeding, unblocked. + const companyId = await seedCompany(); + const svc = secretService(db); + + const normalized = await svc.normalizeAdapterConfigForPersistence( + companyId, + { apiKey: `plain-api-key-${randomUUID()}` }, + { adapterType: "hermes_gateway" }, + ); + const apiKeyBinding = (normalized as Record).apiKey as { type: string; secretId: string }; + expect(apiKeyBinding.type).toBe("secret_ref"); + const secret = await svc.getById(apiKeyBinding.secretId); + expect(secret?.status).toBe("active"); + }); + it("validates the access namespace as agent-only with env-style aliases", async () => { const companyId = await seedCompany(); const svc = secretService(db); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 3d9afc549c..d13477d0a9 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -1,5 +1,6 @@ import { Router, type NextFunction, type Request, type Response } from "express"; import { generateKeyPairSync, randomUUID } from "node:crypto"; +import { rm } from "node:fs/promises"; import path from "node:path"; import type { Db } from "@paperclipai/db"; import { agents as agentsTable, companies, heartbeatRuns, issues as issuesTable, projects as projectsTable } from "@paperclipai/db"; @@ -32,6 +33,7 @@ import { startAdapterAuthSessionRequestSchema, startClaudeSetupTokenSessionRequestSchema, submitBrowserCodeRequestSchema, + toAccountHandle, type AgentAdapterType, } from "@paperclipai/shared"; import { @@ -174,6 +176,8 @@ import { import { checkStagedCredentialReadiness, promoteDeviceLoginCredential, + withAccountHomeSecretMutationLock, + withCodexAccountHomePromotionLock, } from "@paperclipai/adapter-codex-local/server"; import { checkStagedGrokCredentialReadiness, @@ -273,6 +277,119 @@ function readRunIssueId(context: Record | null) { return typeof nestedIssueId === "string" && isUuidLike(nestedIssueId) ? nestedIssueId : null; } +// Confirms a pre-existing `CODEX_HOME_` secret still names this +// account's own home before a device login treats the secret's presence as a +// successful, idempotent login. The secret name alone is not proof of a +// match: a stale value from before the cache root moved, or a value a user +// entered by hand, would otherwise let the login report success while a +// bound agent reads the wrong (or a missing) credential home. Fails loud on +// a mismatch, so the login fails instead of silently pointing agents at the +// wrong home. +// +// Each caller must run this function inside `withAccountHomeSecretMutationLock`, +// the same lock a `local_encrypted` secret rotate holds for its whole write. +// A caller that only checks once, early in the promotion, and then reports +// success later is not enough on its own: the lock this call held is fully +// released by the time it returns, so a rotate queued behind it can commit a +// new value before the login service records its terminal `authenticated` +// state, which happens well after this call returns (see `runTerminalCommit` +// below, which runs this same check again, under a fresh lock acquisition, +// immediately before that terminal state commits). +async function assertAccountHomeSecretMatches( + secretsSvc: { resolveSecretValueForDeviceLoginCheck: (companyId: string, secretId: string, context: { configPath: string }) => Promise }, + companyId: string, + secret: { id: string }, + secretName: string, + expectedAccountHomeDir: string, +): Promise { + const storedValue = await secretsSvc.resolveSecretValueForDeviceLoginCheck(companyId, secret.id, { + configPath: `secrets.${secretName}`, + }); + if (storedValue !== expectedAccountHomeDir) { + throw new Error( + `device-login credential promotion rejected: the existing ${secretName} secret does not name this account's own home`, + ); + } +} + +// Confirms no company secret, under any name or provider, still names this +// account home before a failed promotion deletes the directory. The +// generated `CODEX_HOME_` name is not the only secret that can +// reference this directory: a user can bind a hand-named secret to the same +// account home, so a check that reads only the generated name misses that +// secret and deletes a directory it still needs. A bound agent then reads a +// `CODEX_HOME` value that points at nothing. A secret's value is a plain +// string regardless of its provider, so an AWS Secrets Manager-backed secret +// (or any other provider) can equal this directory's path just as a +// `local_encrypted` secret can; the scan resolves every secret's value, not +// only `local_encrypted` ones. +// +// A secret whose value fails to resolve is NOT proof that secret names a +// different directory: the resolve call can fail for a secret that would +// have matched. Treating that failure as a non-match would let the cleanup +// delete a directory a secret still needs. So the scan fails closed: any +// resolution failure makes the whole scan report a claim, even when every +// secret that DID resolve named a different directory. +// +// The scan lists every secret once, then resolves each secret's value in +// turn, and each resolve call is its own round trip. A new secret can enter +// the company between the initial list and the last resolve call, so a +// single pass can finish, find no claimant among the secrets it read, and +// still miss a secret that named this directory moments later. So the scan +// re-lists after every pass and resolves only the secrets it has not yet +// checked, and it only reports "no claimant" once a pass finds nothing new +// to check. A scan that keeps finding new secrets on every pass fails +// closed after a bounded number of passes, so a fast stream of concurrent +// secret creation cannot force an unsafe delete. +// +// The scan alone still cannot rule out a secret write that commits after the +// scan's own last pass finishes but before the caller's delete runs: the +// scan and that write are two separate operations with no shared state, so +// neither can see the other. The caller closes that window by running the +// scan and the delete inside `withAccountHomeSecretMutationLock`, the same +// lock every `local_encrypted` secret create or rotate holds for its whole +// write. That lock is the atomic protection; the multi-pass scan above stays +// as a defense-in-depth check for a write path that has not taken the lock. +const ACCOUNT_HOME_CLAIM_SCAN_MAX_PASSES = 5; + +async function anySecretNamesAccountHome( + secretsSvc: { + list: (companyId: string) => Promise>; + resolveSecretValueForDeviceLoginCheck: ( + companyId: string, + secretId: string, + context: { configPath: string }, + ) => Promise; + }, + companyId: string, + accountHomeDir: string, +): Promise { + const checkedSecretIds = new Set(); + for (let pass = 0; pass < ACCOUNT_HOME_CLAIM_SCAN_MAX_PASSES; pass += 1) { + const secrets = await secretsSvc.list(companyId); + const uncheckedSecrets = secrets.filter((secret) => !checkedSecretIds.has(secret.id)); + if (uncheckedSecrets.length === 0) return false; + let resolutionFailed = false; + for (const secret of uncheckedSecrets) { + checkedSecretIds.add(secret.id); + const storedValue = await secretsSvc + .resolveSecretValueForDeviceLoginCheck(companyId, secret.id, { + configPath: `secrets.${secret.name}`, + }) + .catch(() => { + resolutionFailed = true; + return null; + }); + if (storedValue === accountHomeDir) return true; + } + if (resolutionFailed) return true; + } + // Every pass found a secret it had not yet checked. Fail closed: an + // endless stream of new secrets is not proof that none of them claims + // this directory. + return true; +} + export function agentRoutes( db: Db, options: { @@ -542,6 +659,21 @@ export function agentRoutes( // process owns one instance, so the in-memory prompt and the cancellation // controllers persist across requests. const adapterLoginStore = createDbAdapterAuthSessionStore(db); + + // The account-home secret a Codex login validated and bound, keyed by + // session id, queued for `runTerminalCommit` to reconfirm right before the + // login service commits its terminal `authenticated` write. `promote`'s own + // check runs under a lock that is fully released by the time `promote` + // returns, so a rotation can still land after that check and before the + // terminal write; `runTerminalCommit` closes that gap by re-running the + // same check under a fresh lock acquisition that it holds across the + // terminal write itself. `runTerminalCommit` deletes the entry it reads, so + // nothing outlives one login attempt. + const pendingAccountHomeSecretCommits = new Map< + string, + { secretId: string; secretName: string; accountHomeDir: string } + >(); + const adapterLoginService = createDeviceLoginService({ store: adapterLoginStore, runtime: createProductionLoginSessionRuntime({ @@ -577,59 +709,222 @@ export function agentRoutes( // promotion function. promotionByAdapterType: { codex_local: { + // Hold one lock across the whole promotion sequence below: the + // credential write, the existing-secret check, the secret create, and + // the cleanup a create failure can trigger. Two different logins for + // the SAME Codex account run this whole sequence one at a time, so a + // login can never decide to delete the shared account-home directory + // while another login's own sequence is still mid-way through writing + // its credential or binding its own secret to that same directory. A + // lock around only the directory-creation step is not enough: that + // lock is already released by the time a login reaches the secret + // bind, so a second login can write its credential and be about to + // bind its own secret while the first login's later, unrelated + // secret-write failure removes the directory both logins now share. async promote(authBytes, context) { - // Hold the promotion critical-section lock across the ownership check and - // the credential write. The reaper takes the same lock before it reclaims - // a stale `promoting` row. So a reclaim never interleaves with a live - // write: the reaper either wins the lock first and the ownership check - // then reads a reclaimed row and writes nothing, or the write finishes - // first under the lock and the reaper reclaims only after it completes. A - // read-only fence is not enough, because the filesystem write can start - // after the fence; the lock spans the whole section. - const outcome = await adapterLoginStore.withCompanyAdapterPromotionLock( - context.companyId, - context.startedByUserId, - context.adapterType, - () => - promoteDeviceLoginCredential({ - authBytes, - companyId: context.companyId, - userInitiated: true, - checkReadiness: (bytes) => checkStagedCredentialReadiness(bytes), - isSoleActiveOwner: async () => { - // The partial unique index allows one active row per company and - // adapter. So a `promoting` row for this session is the sole - // active owner of the company credential slot. The read runs - // inside the lock, so it observes a reaper reclaim that committed - // before this section acquired the lock. - const row = await adapterLoginStore.get(context.sessionId); - return row?.status === "promoting" && row.companyId === context.companyId; - }, - log: (line) => { - // The promotion lines carry no token bytes and no raw account id, - // so it is safe to log them with the session identifier. - logger.info({ sessionId: context.sessionId }, line); - }, - }), - ); - // A resolved promotion is not necessarily an accepted promotion. In - // particular, a reaper/expiry race can revoke this session's sole - // ownership between the service transition and Decision H. Fail closed: - // only a credential write or a deliberate safe keep can authenticate. - if (outcome === "kept_foreign_identity") { - // The login produced a different account than the one the company - // credential home already holds. The promotion never clobbers an - // occupied home, so this login installed nothing durable, and the - // identity-anchored vend can never select it: a later run keeps the - // existing account. Fail the session, so the operator never sees a - // false `authenticated` for an account the system will not use. - throw new Error( - "device-login credential promotion rejected: the login is a different account than the one already set for this company; the existing account was kept", + return withCodexAccountHomePromotionLock(undefined, context.companyId, async () => { + // Hold the promotion critical-section lock across the ownership check + // and the credential write. The reaper takes the same lock before it + // reclaims a stale `promoting` row. So a reclaim never interleaves with + // a live write: the reaper either wins the lock first and the + // ownership check then reads a reclaimed row and writes nothing, or + // the write finishes first under the lock and the reaper reclaims only + // after it completes. A read-only fence is not enough, because the + // filesystem write can start after the fence; the lock spans the whole + // section. + const result = await adapterLoginStore.withCompanyAdapterPromotionLock( + context.companyId, + context.startedByUserId, + context.adapterType, + () => + promoteDeviceLoginCredential({ + authBytes, + companyId: context.companyId, + userInitiated: true, + checkReadiness: (bytes) => checkStagedCredentialReadiness(bytes), + isSoleActiveOwner: async () => { + // The partial unique index allows one active row per company and + // adapter. So a `promoting` row for this session is the sole + // active owner of the company credential slot. The read runs + // inside the lock, so it observes a reaper reclaim that committed + // before this section acquired the lock. + const row = await adapterLoginStore.get(context.sessionId); + return row?.status === "promoting" && row.companyId === context.companyId; + }, + log: (line) => { + // The promotion lines carry no token bytes and no raw account id, + // so it is safe to log them with the session identifier. + logger.info({ sessionId: context.sessionId }, line); + }, + }), ); + // A resolved promotion is not necessarily an accepted promotion. In + // particular, a reaper/expiry race can revoke this session's sole + // ownership between the service transition and Decision H. Fail closed: + // only a credential write or a deliberate safe keep can authenticate. + if (result.outcome !== "promoted" && result.outcome !== "kept") { + throw new Error(`device-login credential promotion rejected: ${result.outcome}`); + } + // The account's own home is durable at this point (the promotion above + // wrote it fail-loud). Name it with a company secret, so any agent can + // bind to it. Reading the secret by name first keeps a repeat login for + // the same account idempotent: `create` throws a conflict when the name + // already exists. + const handle = result.accountId ? toAccountHandle(result.accountId) : null; + if (!handle || !result.accountHomeDir) { + throw new Error( + "device-login credential promotion rejected: the promotion carried no account home", + ); + } + const secretName = `CODEX_HOME_${handle}`; + const accountHomeDir = result.accountHomeDir; + const existingSecret = await secretsSvc.getByName(context.companyId, secretName); + if (existingSecret) { + // A same-name secret already exists. Confirm it still names this + // account's own home before treating a repeat login as a success: + // the name alone is not proof of a match. + // + // Run the check inside the same lock a `local_encrypted` secret + // rotate holds for its whole write, the same lock a rotate + // takes. This is an early fail-fast only: the lock is fully + // released once this call returns, well before the login + // service commits its terminal state, so queue the same check + // for `runTerminalCommit` to run again right before that + // commit, under a fresh lock acquisition it holds across the + // commit itself. + await withAccountHomeSecretMutationLock(undefined, context.companyId, () => + assertAccountHomeSecretMatches(secretsSvc, context.companyId, existingSecret, secretName, accountHomeDir), + ); + pendingAccountHomeSecretCommits.set(context.sessionId, { + secretId: existingSecret.id, + secretName, + accountHomeDir, + }); + return; + } + try { + const createdSecret = await secretsSvc.create( + context.companyId, + { + name: secretName, + provider: "local_encrypted", + value: accountHomeDir, + description: `CODEX_HOME generated by logging into account ${handle}`, + }, + { userId: context.startedByUserId, agentId: null }, + ); + // The value just committed is correct at this instant, but a + // rotate queued behind the create's own lock can still commit a + // different value before the login service records its + // terminal state. Queue the same reconfirm `runTerminalCommit` + // runs for the two branches above. + pendingAccountHomeSecretCommits.set(context.sessionId, { + secretId: createdSecret.id, + secretName, + accountHomeDir, + }); + } catch (err) { + if (err instanceof HttpError && err.status === 409) { + // A conflict means a concurrent login for the same account won the + // create race. Confirm the winning secret still names this + // account's own home before treating the race as a successful, + // idempotent login. + const winningSecret = await secretsSvc.getByName(context.companyId, secretName); + if (!winningSecret) { + throw new Error( + `device-login credential promotion rejected: the ${secretName} secret conflict could not be resolved`, + ); + } + // Same lock and the same reasoning as the pre-existing-secret + // check above: an early fail-fast only, so also queue the + // same check for `runTerminalCommit` to run again, under a + // fresh lock acquisition it holds across the terminal commit. + await withAccountHomeSecretMutationLock(undefined, context.companyId, () => + assertAccountHomeSecretMatches(secretsSvc, context.companyId, winningSecret, secretName, accountHomeDir), + ); + pendingAccountHomeSecretCommits.set(context.sessionId, { + secretId: winningSecret.id, + secretName, + accountHomeDir, + }); + return; + } + // The account home write failed for a reason other than a naming + // conflict. Remove the directory only when this exact login created + // it AND no secret, under any name, still names it. The lock + // around this whole method already rules out another + // SAME-ACCOUNT LOGIN from being mid-sequence here, but it does + // not rule out a secret a different path created at this exact + // directory in between the read above and this failure — for + // example, a concurrent login for the same account that won a + // race on this exact name, or a user who names a secret by hand + // under a different name entirely. The re-check is this call's + // only signal for that case, so it stays even under the lock: + // `accountHomeCreated` alone is not proof no such secret now + // claims the directory, and a same-name check alone is not proof + // either, because the claiming secret can carry any name. + // + // The check and the delete run inside `withAccountHomeSecretMutationLock`, + // the same lock the secrets service holds for the whole of a + // `local_encrypted` secret's create or rotate call. That closes the + // window `anySecretNamesAccountHome`'s own multi-pass scan cannot: a + // secret write that commits after this check's last pass but before + // the delete runs. Under the shared lock, a write either finishes + // (and becomes visible to the check) before this section acquires the + // lock, or it waits for this section to finish before it can commit. + if (result.accountHomeCreated) { + await withAccountHomeSecretMutationLock(undefined, context.companyId, async () => { + const claimed = await anySecretNamesAccountHome(secretsSvc, context.companyId, accountHomeDir); + if (!claimed) { + await rm(accountHomeDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + } + throw new Error( + "device-login credential promotion rejected: failed to record the account home secret", + ); + } + }); + }, + // The login service calls this immediately before it commits its + // terminal `authenticated` write, wrapping that write in the + // callback it hands in as `commit`. `promote` above already + // validated the bound account-home secret once, early, but its own + // lock is fully released by the time `promote` returns — well before + // this runs. Re-run the same check here, and hold the SAME lock + // across both the check and `commit`, so a rotate cannot land in the + // gap between the validated value and the terminal write that + // reports it as authenticated: a rotate either finishes (and this + // check reads its new value, and rejects) before this section + // acquires the lock, or it waits for this section — including the + // terminal commit — to finish first. + async runTerminalCommit(commit, context) { + const pending = pendingAccountHomeSecretCommits.get(context.sessionId); + pendingAccountHomeSecretCommits.delete(context.sessionId); + if (!pending) { + // `promote` never reached a secret bind for this session (for + // example, a rejected promotion already failed the login before + // the service ever reaches this call). Nothing to reconfirm. + return commit(); } - if (outcome !== "promoted" && outcome !== "kept") { - throw new Error(`device-login credential promotion rejected: ${outcome}`); - } + return withAccountHomeSecretMutationLock(undefined, context.companyId, async () => { + // Resolve by the secret's id, not its name: a rotate changes the + // value under the same id, so re-resolving this id picks up a + // rotation the same way the very first check would have, with no + // need to re-look the secret up by name. A deleted secret makes + // this resolve call itself fail (unlike a value mismatch, which + // `assertAccountHomeSecretMatches` turns into its own error), and + // that failure propagates the same way: the login never + // authenticates. + await assertAccountHomeSecretMatches( + secretsSvc, + context.companyId, + { id: pending.secretId }, + pending.secretName, + pending.accountHomeDir, + ); + return commit(); + }); }, }, grok_local: { diff --git a/server/src/services/device-login-service.ts b/server/src/services/device-login-service.ts index f19ba75b5b..4e722319ea 100644 --- a/server/src/services/device-login-service.ts +++ b/server/src/services/device-login-service.ts @@ -163,6 +163,16 @@ export interface CredentialPromotionContext { */ export interface CredentialPromotion { promote(authBytes: Buffer, context: CredentialPromotionContext): void | Promise; + /** + * Wraps the service's terminal "authenticated" commit, immediately before + * the service runs it. A promotion that already bound and validated a + * value in `promote` can hold the SAME lock across this call, so a write + * that would invalidate that value cannot land in the gap between the + * earlier validation and this terminal commit. When the wrapper throws, + * the service records a failed login instead of `authenticated` and never + * runs `commit`. A promotion that omits this runs `commit` directly. + */ + runTerminalCommit?(commit: () => Promise, context: CredentialPromotionContext): Promise; } /** The redacted lifecycle phases. Each phase carries no secret data. */ @@ -1082,15 +1092,41 @@ export function createDeviceLoginService(deps: DeviceLoginServiceDeps) { // Publish `authenticated` only when the final conditional write still finds // the held claim. A lost write means the claim expired and the reaper // reclaimed the row, so the service never publishes `authenticated`. - return await terminate({ + const commitAuthenticated = () => + terminate({ + sessionId, + lease, + terminal: "authenticated", + reason: null, + expectedStatuses: ["promoting"], + conditionalTransition, + activity, + }); + const promotionContext: CredentialPromotionContext = { sessionId, - lease, - terminal: "authenticated", - reason: null, - expectedStatuses: ["promoting"], - conditionalTransition, - activity, - }); + companyId: input.companyId, + startedByUserId: input.startedByUserId, + adapterType: input.adapterType, + }; + try { + return profile.promotion.runTerminalCommit + ? await profile.promotion.runTerminalCommit(commitAuthenticated, promotionContext) + : await commitAuthenticated(); + } catch { + // The wrapped final check rejected the value `promote` bound earlier + // (for example, a rotation landed after that validation). The login + // did not finish, so fail closed the same way a `promote` rejection + // does: never publish `authenticated`, still delete the sandbox. + return await terminate({ + sessionId, + lease, + terminal: "failed", + reason: "promotion_failed", + expectedStatuses: ["promoting"], + conditionalTransition, + activity, + }); + } } const terminal: AdapterAuthSessionStatus = diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index ca3bc328f9..dd6fa92712 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -65,6 +65,25 @@ import { isSecretProviderClientError } from "../secrets/types.js"; import { authorizationDeniedDetails, authorizationService } from "./authorization.js"; import { findActiveServerAdapter } from "../adapters/index.js"; import { logActivity } from "./activity-log.js"; +// Only a `local_encrypted` secret can hold a literal directory path, so only a +// `local_encrypted` secret can ever name a Codex account-home directory. A +// create or a rotate that writes a new `local_encrypted` value runs inside +// this lock for its whole call, so it can never commit a value in the exact +// window an account-home cleanup's claimant scan already decided "no +// claimant" but has not yet deleted the directory. See +// `withAccountHomeSecretMutationLock`'s own comment in the codex-local +// adapter for the full race this closes. +// +// The lock alone still lets a queued write commit a directory the cleanup +// already removed, once the cleanup's own lock-holding section runs first and +// frees the lock. `assertAccountHomeCacheDirStillValid` closes that window: a +// create or a rotate calls it inside the same lock, right before it commits, +// so a value that named a directory the cleanup just deleted fails instead of +// writing a secret that points at nothing. +import { + assertAccountHomeCacheDirStillValid, + withAccountHomeSecretMutationLock, +} from "@paperclipai/adapter-codex-local/server"; const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; const AGENT_ACCESS_CONFIG_PATH_PREFIX = "access."; @@ -72,6 +91,11 @@ const AGENT_ACCESS_CONFIG_PATH_PREFIX = "access."; // resolves the recorded connection secret under this id so the audit trail // marks the read as an orphan-sandbox teardown, not a normal environment read. export const SANDBOX_CLEANUP_CONSUMER_ID = "environment-sandbox-cleanup"; +// System consumer id for a device-login account-home secret check. A device +// login resolves a pre-existing secret's value under this id, so the audit +// trail marks the read as a same-account idempotency check, not a normal +// runtime bind. +export const DEVICE_LOGIN_SECRET_CHECK_CONSUMER_ID = "device-login-secret-check"; const SENSITIVE_ENV_KEY_RE = /(api[-_]?key|access[-_]?token|auth(?:_?token)?|authorization|bearer|secret|passwd|password|credential|jwt|private[-_]?key|cookie|connectionstring)/i; const REDACTED_SENTINEL = "***REDACTED***"; @@ -1508,6 +1532,32 @@ export function secretService(db: Db | DbTransaction) { ).value; } + // Resolve a pre-existing `CODEX_HOME_` (or equivalent) secret's + // current value for a device-login idempotency check. A device login that + // finds a secret already at its expected name must compare the stored + // value against the account home it just resolved, not trust the name + // alone: a stale or foreign secret at that name would otherwise let the + // login report success while a bound agent reads the wrong credential + // home. The read is audit-only (no `bindingContext`), because the secret + // may carry no environment binding yet. + async function resolveSecretValueForDeviceLoginCheck( + companyId: string, + secretId: string, + context: { configPath: string }, + ): Promise { + return ( + await resolveSecretValueInternal(companyId, secretId, "latest", { + accessContext: { + consumerType: "system", + consumerId: DEVICE_LOGIN_SECRET_CHECK_CONSUMER_ID, + actorType: "system", + actorId: null, + configPath: context.configPath, + }, + }) + ).value; + } + async function resolveSecretValueForAgentAccess( companyId: string, secretId: string, @@ -1949,6 +1999,13 @@ export function secretService(db: Db | DbTransaction) { }; } + // Every `local_encrypted` secret this function creates runs under + // `withAccountHomeSecretMutationLock`, so its write can never commit inside + // the window a Codex account-home cleanup already used to decide no secret + // claims the directory it is about to delete. A create queued behind the + // lock can still win it after the cleanup already removed that directory, + // so check the directory's existence inside the same lock, right before + // the write, the same way `create:` and `rotate:` below do. async function createManagedLocalSecret( companyId: string, input: { @@ -1958,6 +2015,22 @@ export function secretService(db: Db | DbTransaction) { description?: string | null; }, actor?: { userId?: string | null; agentId?: string | null }, + ) { + return withAccountHomeSecretMutationLock(undefined, companyId, async () => { + await assertAccountHomeCacheDirStillValid(undefined, companyId, input.value); + return createManagedLocalSecretUnlocked(companyId, input, actor); + }); + } + + async function createManagedLocalSecretUnlocked( + companyId: string, + input: { + name: string; + key: string; + value: string; + description?: string | null; + }, + actor?: { userId?: string | null; agentId?: string | null }, ) { const existing = await getByName(companyId, input.name); if (existing) throw conflict(`Secret already exists: ${input.name}`); @@ -2067,6 +2140,540 @@ export function secretService(db: Db | DbTransaction) { } } + // The body `create:` below runs, unchanged. Extracted to a named function so + // `create:` can wrap it in `withAccountHomeSecretMutationLock` only for the + // one provider (`local_encrypted`) that can hold a literal directory path, + // without duplicating this whole write sequence. + async function createSecretUnlocked( + companyId: string, + input: { + name: string; + provider: SecretProvider; + providerConfigId?: string | null; + value?: string | null; + key?: string | null; + managedMode?: "paperclip_managed" | "external_reference"; + description?: string | null; + externalRef?: string | null; + providerVersionRef?: string | null; + providerMetadata?: Record | null; + }, + actor?: { userId?: string | null; agentId?: string | null }, + ) { + const existing = await getByName(companyId, input.name); + if (existing) throw conflict(`Secret already exists: ${input.name}`); + const key = normalizeSecretKey(input.key ?? input.name); + if (!key) throw unprocessable("Secret key is required"); + const duplicateKey = await db + .select() + .from(companySecrets) + .where(and( + eq(companySecrets.companyId, companyId), + eq(companySecrets.scope, "company"), + eq(companySecrets.key, key), + ne(companySecrets.status, "deleted"), + )) + .then((rows) => rows[0] ?? null); + if (duplicateKey) throw conflict(`Secret key already exists: ${key}`); + + const managedMode = input.managedMode ?? "paperclip_managed"; + const provider = getSecretProvider(input.provider); + const providerConfig = await getSelectableRuntimeProviderConfig({ + companyId, + provider: input.provider, + providerConfigId: input.providerConfigId, + }); + if (managedMode === "external_reference" && !input.externalRef?.trim()) { + throw unprocessable("External reference secrets require externalRef"); + } + if (managedMode === "paperclip_managed" && input.externalRef?.trim()) { + throw unprocessable("Managed secrets cannot override externalRef"); + } + if (managedMode === "paperclip_managed" && !input.value?.trim()) { + throw unprocessable("Managed secrets require value"); + } + const providerWriteContext = { + companyId, + secretKey: key, + secretName: input.name, + version: 1, + }; + const reservedSecret = await db + .insert(companySecrets) + .values({ + companyId, + key, + name: input.name, + provider: input.provider, + providerConfigId: input.providerConfigId ?? null, + status: "archived", + managedMode, + externalRef: null, + providerMetadata: input.providerMetadata ?? null, + latestVersion: 0, + description: input.description ?? null, + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }) + .returning() + .then((rows) => rows[0]); + + let prepared: PreparedSecretVersion; + try { + prepared = + managedMode === "external_reference" + ? await provider.linkExternalSecret({ + externalRef: input.externalRef ?? "", + providerVersionRef: input.providerVersionRef ?? null, + providerConfig, + context: providerWriteContext, + }) + : await provider.createSecret({ + value: input.value ?? "", + externalRef: null, + providerConfig, + context: providerWriteContext, + }); + } catch (error) { + throw await throwProviderWriteOrReservedRowRollbackError({ + error, + rollbackReservedRow: () => db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)), + companyId, + provider: provider.id, + providerConfigId: input.providerConfigId ?? null, + providerConfig, + operation: "secret.create", + }); + } + + try { + await db + .update(companySecrets) + .set({ + externalRef: prepared.externalRef, + latestVersion: 1, + updatedAt: new Date(), + }) + .where(eq(companySecrets.id, reservedSecret.id)); + await db.insert(companySecretVersions).values({ + secretId: reservedSecret.id, + version: 1, + material: prepared.material, + valueSha256: prepared.valueSha256, + fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, + providerVersionRef: prepared.providerVersionRef ?? null, + status: "disabled", + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }); + } catch (error) { + if (managedMode === "paperclip_managed") { + const cleaned = await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "delete", + operation: "create.prepare_rollback", + }); + if (!cleaned) { + throwProviderCleanupFailedAfterCreateRollback({ + companyId, + provider: provider.id, + providerConfigId: input.providerConfigId ?? null, + providerConfig, + operation: "create.prepare_rollback", + }); + } + } + await deleteLocalSecretCreateReservationOrThrow({ + db, + secretId: reservedSecret.id, + companyId, + provider: provider.id, + providerConfigId: input.providerConfigId ?? null, + providerConfig, + operation: "create.prepare_rollback", + }); + throw error; + } + + try { + return await db.transaction(async (tx) => { + await tx + .update(companySecretVersions) + .set({ status: "current" }) + .where(and( + eq(companySecretVersions.secretId, reservedSecret.id), + eq(companySecretVersions.version, 1), + )); + + const secret = await tx + .update(companySecrets) + .set({ + status: "active", + externalRef: prepared.externalRef, + latestVersion: 1, + lastRotatedAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(companySecrets.id, reservedSecret.id)) + .returning() + .then((rows) => rows[0]); + + if (!secret) throw notFound("Secret not found"); + return secret; + }); + } catch (error) { + if (managedMode === "paperclip_managed") { + const cleaned = await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "delete", + operation: "create.rollback", + }); + if (!cleaned) { + throwProviderCleanupFailedAfterCreateRollback({ + companyId, + provider: provider.id, + providerConfigId: input.providerConfigId ?? null, + providerConfig, + operation: "create.rollback", + }); + } + } + await deleteLocalSecretCreateReservationOrThrow({ + db, + secretId: reservedSecret.id, + companyId, + provider: provider.id, + providerConfigId: input.providerConfigId ?? null, + providerConfig, + operation: "create.rollback", + }); + throw error; + } + } + + // The body of `rotate:` below, unchanged. Extracted to a named function so + // `rotate:` can wrap it in `withAccountHomeSecretMutationLock` only when the + // secret being rotated is `local_encrypted`, without duplicating this whole + // write sequence. + async function rotateUnlocked( + secretId: string, + input: { + value?: string | null; + externalRef?: string | null; + providerVersionRef?: string | null; + providerConfigId?: string | null; + // The optional owner-bound compare-and-set guard. When set, the final + // update matches the latest version, so a concurrent rotation between the + // read and the write cannot pass. A mismatch throws a 409 conflict. + expectedLatestVersion?: number; + }, + actor?: { userId?: string | null; agentId?: string | null }, + ) { + const secret = await getById(secretId); + if (!secret) throw notFound("Secret not found"); + if (secret.status !== "active") throw unprocessable("Cannot rotate a non-active secret"); + if (input.expectedLatestVersion !== undefined && secret.latestVersion !== input.expectedLatestVersion) { + throw conflict(SECRET_VERSION_STALE_CONFLICT); + } + const providerId = secret.provider as SecretProvider; + const provider = getSecretProvider(providerId); + const providerConfigId = + input.providerConfigId === undefined ? secret.providerConfigId : input.providerConfigId; + const providerConfig = await getSelectableRuntimeProviderConfig({ + companyId: secret.companyId, + provider: providerId, + providerConfigId, + }); + const nextVersion = secret.latestVersion + 1; + const externalValueWrite = + secret.managedMode === "external_reference" && Boolean(input.value?.trim()); + if (externalValueWrite) { + const currentRef = secret.externalRef?.trim(); + if (!currentRef) { + throw unprocessable("External reference secrets require externalRef"); + } + if (input.externalRef?.trim() && input.externalRef.trim() !== currentRef) { + throw unprocessable( + "Provide either a new value or a new external reference, not both", + ); + } + if (input.providerVersionRef?.trim()) { + throw unprocessable("Value updates cannot pin providerVersionRef"); + } + if (!provider.updateExternalSecretValue) { + throw unprocessable( + `${provider.descriptor().label} does not support writing values to external reference secrets`, + ); + } + } + if (secret.managedMode === "external_reference" && !(input.externalRef ?? secret.externalRef)?.trim()) { + throw unprocessable("External reference secrets require externalRef"); + } + if (secret.managedMode !== "external_reference" && input.externalRef?.trim()) { + throw unprocessable("Managed secrets cannot override externalRef"); + } + if (secret.managedMode !== "external_reference" && !input.value?.trim()) { + throw unprocessable("Managed secrets require value"); + } + const providerWriteContext = { + companyId: secret.companyId, + secretKey: secret.key, + secretName: secret.name, + version: nextVersion, + }; + let prepared: PreparedSecretVersion; + try { + prepared = externalValueWrite + ? await provider.updateExternalSecretValue!({ + externalRef: secret.externalRef ?? "", + value: input.value ?? "", + providerConfig, + context: providerWriteContext, + }) + : secret.managedMode === "external_reference" + ? await provider.linkExternalSecret({ + externalRef: input.externalRef ?? secret.externalRef ?? "", + providerVersionRef: input.providerVersionRef ?? null, + providerConfig, + context: providerWriteContext, + }) + : await provider.createVersion({ + value: input.value ?? "", + externalRef: secret.externalRef ?? null, + providerConfig, + context: providerWriteContext, + }); + } catch (error) { + throw remoteProviderWriteHttpError(error, { + companyId: secret.companyId, + provider: provider.id, + providerConfigId, + providerConfig, + operation: "secret.rotate", + }); + } + + try { + await db.insert(companySecretVersions).values({ + secretId: secret.id, + version: nextVersion, + material: prepared.material, + valueSha256: prepared.valueSha256, + fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, + providerVersionRef: prepared.providerVersionRef ?? null, + status: "disabled", + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }); + } catch (error) { + if (secret.managedMode !== "external_reference" || externalValueWrite) { + await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "archive", + operation: "rotate.prepare_rollback", + }); + } + // A guarded concurrent rotation inserted the same next version first, so + // this insert fails the (secretId, version) unique index. The loser never + // reaches the compare-and-set guard below. Normalize only that one + // collision to the same stale conflict, so a guarded rotation always + // returns one fixed 409. Re-throw every other error unchanged. + if ( + input.expectedLatestVersion !== undefined && + isUniqueConstraintViolation(error, COMPANY_SECRET_VERSION_UNIQUE_CONSTRAINT) + ) { + throw conflict(SECRET_VERSION_STALE_CONFLICT); + } + throw error; + } + + try { + return await db.transaction(async (tx) => { + await tx + .update(companySecretVersions) + .set({ status: "previous" }) + .where(and( + eq(companySecretVersions.secretId, secret.id), + ne(companySecretVersions.version, nextVersion), + )); + await tx + .update(companySecretVersions) + .set({ status: "current" }) + .where(and( + eq(companySecretVersions.secretId, secret.id), + eq(companySecretVersions.version, nextVersion), + )); + + const updated = await tx + .update(companySecrets) + .set({ + latestVersion: nextVersion, + externalRef: prepared.externalRef, + providerConfigId, + lastRotatedAt: new Date(), + updatedAt: new Date(), + }) + .where(and( + eq(companySecrets.id, secret.id), + // The compare-and-set guard. It matches the latest version, so a + // concurrent rotation that already bumped it fails this predicate. + input.expectedLatestVersion === undefined + ? undefined + : eq(companySecrets.latestVersion, input.expectedLatestVersion), + )) + .returning() + .then((rows) => rows[0] ?? null); + + if (!updated) { + // The predicate matched no row. A supplied expected version means a + // concurrent rotation won the race; return the stale conflict. An + // unguarded rotation means the secret is gone. + throw input.expectedLatestVersion === undefined + ? notFound("Secret not found") + : conflict(SECRET_VERSION_STALE_CONFLICT); + } + return updated; + }); + } catch (error) { + if (secret.managedMode !== "external_reference" || externalValueWrite) { + const cleaned = await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "archive", + operation: "rotate.rollback", + }); + if (cleaned) { + await db + .delete(companySecretVersions) + .where(and( + eq(companySecretVersions.secretId, secret.id), + eq(companySecretVersions.version, nextVersion), + )) + .catch(() => undefined); + } + } + throw error; + } + } + + // The body of `update:` below, unchanged. Extracted to a named function so + // `update:` can wrap it in `withAccountHomeSecretMutationLock`, keyed by the + // company the pre-check read, without duplicating this whole patch + // sequence. + async function updateUnlocked( + secretId: string, + patch: { + name?: string; + key?: string; + status?: "active" | "disabled" | "archived" | "deleted"; + providerConfigId?: string | null; + description?: string | null; + externalRef?: string | null; + providerMetadata?: Record | null; + }, + ) { + const secret = await getById(secretId); + if (!secret) throw notFound("Secret not found"); + if (secret.status === "deleted") throw notFound("Secret not found"); + + if (patch.name && patch.name !== secret.name) { + const duplicate = await getByName(secret.companyId, patch.name); + if (duplicate && duplicate.id !== secret.id) { + throw conflict(`Secret already exists: ${patch.name}`); + } + } + const nextKey = patch.key ? normalizeSecretKey(patch.key) : secret.key; + if (!nextKey) throw unprocessable("Secret key is required"); + if (nextKey !== secret.key) { + const duplicateKey = await db + .select() + .from(companySecrets) + .where(and( + eq(companySecrets.companyId, secret.companyId), + eq(companySecrets.scope, "company"), + eq(companySecrets.key, nextKey), + ne(companySecrets.status, "deleted"), + )) + .then((rows) => rows[0] ?? null); + if (duplicateKey && duplicateKey.id !== secret.id) { + throw conflict(`Secret key already exists: ${nextKey}`); + } + } + const deleting = patch.status === "deleted"; + if (deleting && secret.managedMode === "paperclip_managed") { + throw unprocessable("Managed secrets must be deleted through DELETE /secrets/:id"); + } + if (secret.managedMode !== "external_reference" && patch.externalRef !== undefined) { + throw unprocessable("Managed secrets cannot override externalRef"); + } + if ( + secret.managedMode === "external_reference" && + patch.externalRef !== undefined && + patch.externalRef !== secret.externalRef + ) { + throw unprocessable( + "External reference secrets cannot be retargeted through generic update", + ); + } + if ( + secret.managedMode === "external_reference" && + patch.providerConfigId !== undefined && + patch.providerConfigId !== secret.providerConfigId + ) { + throw unprocessable( + "External reference secrets cannot change provider vault through generic update", + ); + } + if ( + secret.managedMode === "paperclip_managed" && + patch.providerConfigId !== undefined && + patch.providerConfigId !== secret.providerConfigId + ) { + throw unprocessable( + "Managed secrets cannot change provider vault through PATCH; use rotate() to migrate to a new vault", + ); + } + if (patch.providerConfigId !== undefined) { + await assertProviderConfigForSecret( + secret.companyId, + secret.provider as SecretProvider, + patch.providerConfigId, + ); + } + + return db + .update(companySecrets) + .set({ + key: deleting ? `${secret.key}__deleted__${secret.id}` : nextKey, + name: deleting ? `${secret.name}__deleted__${secret.id}` : patch.name ?? secret.name, + status: patch.status ?? secret.status, + providerConfigId: + patch.providerConfigId === undefined ? secret.providerConfigId : patch.providerConfigId, + description: + patch.description === undefined ? secret.description : patch.description, + externalRef: + patch.externalRef === undefined ? secret.externalRef : patch.externalRef, + providerMetadata: + patch.providerMetadata === undefined ? secret.providerMetadata : patch.providerMetadata, + deletedAt: deleting ? new Date() : secret.deletedAt, + updatedAt: new Date(), + }) + .where(eq(companySecrets.id, secret.id)) + .returning() + .then((rows) => rows[0] ?? null); + } + function collectTargetIds( bindings: Array, targetType: SecretBindingTargetType, @@ -2715,7 +3322,27 @@ export function secretService(db: Db | DbTransaction) { return { secretId: existing.id, latestVersion: existing.latestVersion }; } + // A delete can stop the generated account-home secret from resolving to + // the value a device-login promotion already validated, the same way a + // rename, an archive, or a disable can (see `update:`'s comment). Run every + // delete inside `withAccountHomeSecretMutationLock`, keyed by the company a + // pre-check read, so a delete can never land in the gap between the + // promotion's terminal-commit re-check and the commit it guards. + // `removeSecretUnlocked` re-reads the secret under the lock, so a delete + // that lost a race between the pre-check and the lock still sees current + // state, not the pre-check's stale read. async function removeSecretInternal(secretId: string) { + const preCheckSecret = await getById(secretId); + if (!preCheckSecret) return null; + return withAccountHomeSecretMutationLock(undefined, preCheckSecret.companyId, () => + removeSecretUnlocked(secretId), + ); + } + + // The body of `removeSecretInternal` above, unchanged. Extracted to a named + // function so that wrapper can hold the lock across this whole delete + // sequence without duplicating it. + async function removeSecretUnlocked(secretId: string) { const secret = await getById(secretId); if (!secret) return null; const versionRow = await getSecretVersion(secret.id, secret.latestVersion); @@ -3872,7 +4499,16 @@ export function secretService(db: Db | DbTransaction) { listAgentSecretAccess, resolveSecretValueForEphemeralAccess, resolveSecretValueForSandboxCleanup, + resolveSecretValueForDeviceLoginCheck, + // A plain string value can equal a Codex account-home path regardless of + // which provider stores it: an AWS Secrets Manager-backed secret (or any + // other provider) resolves to the same literal string a `local_encrypted` + // secret would. So every provider's create runs inside + // `withAccountHomeSecretMutationLock`, not only `local_encrypted`'s. This + // closes the account-home cleanup race for every provider: a create can no + // longer commit a value between the cleanup's final claimant scan and its + // delete just because it named a provider the old check treated as safe. create: async ( companyId: string, input: { @@ -3889,203 +4525,28 @@ export function secretService(db: Db | DbTransaction) { }, actor?: { userId?: string | null; agentId?: string | null }, ) => { - const existing = await getByName(companyId, input.name); - if (existing) throw conflict(`Secret already exists: ${input.name}`); - const key = normalizeSecretKey(input.key ?? input.name); - if (!key) throw unprocessable("Secret key is required"); - const duplicateKey = await db - .select() - .from(companySecrets) - .where(and( - eq(companySecrets.companyId, companyId), - eq(companySecrets.scope, "company"), - eq(companySecrets.key, key), - ne(companySecrets.status, "deleted"), - )) - .then((rows) => rows[0] ?? null); - if (duplicateKey) throw conflict(`Secret key already exists: ${key}`); - - const managedMode = input.managedMode ?? "paperclip_managed"; - const provider = getSecretProvider(input.provider); - const providerConfig = await getSelectableRuntimeProviderConfig({ - companyId, - provider: input.provider, - providerConfigId: input.providerConfigId, + return withAccountHomeSecretMutationLock(undefined, companyId, async () => { + // A queued create can win the lock after an account-home cleanup + // already removed the directory this value names. Check the + // directory's existence inside the same lock, right before the + // write, so that order fails instead of committing a dangling path. + // `assertAccountHomeCacheDirStillValid` is a no-op for a value that is + // not under the account-home cache root, so this never rejects an + // unrelated secret write. + if (input.value) { + await assertAccountHomeCacheDirStillValid(undefined, companyId, input.value); + } + return createSecretUnlocked(companyId, input, actor); }); - if (managedMode === "external_reference" && !input.externalRef?.trim()) { - throw unprocessable("External reference secrets require externalRef"); - } - if (managedMode === "paperclip_managed" && input.externalRef?.trim()) { - throw unprocessable("Managed secrets cannot override externalRef"); - } - if (managedMode === "paperclip_managed" && !input.value?.trim()) { - throw unprocessable("Managed secrets require value"); - } - const providerWriteContext = { - companyId, - secretKey: key, - secretName: input.name, - version: 1, - }; - const reservedSecret = await db - .insert(companySecrets) - .values({ - companyId, - key, - name: input.name, - provider: input.provider, - providerConfigId: input.providerConfigId ?? null, - status: "archived", - managedMode, - externalRef: null, - providerMetadata: input.providerMetadata ?? null, - latestVersion: 0, - description: input.description ?? null, - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, - }) - .returning() - .then((rows) => rows[0]); - - let prepared: PreparedSecretVersion; - try { - prepared = - managedMode === "external_reference" - ? await provider.linkExternalSecret({ - externalRef: input.externalRef ?? "", - providerVersionRef: input.providerVersionRef ?? null, - providerConfig, - context: providerWriteContext, - }) - : await provider.createSecret({ - value: input.value ?? "", - externalRef: null, - providerConfig, - context: providerWriteContext, - }); - } catch (error) { - throw await throwProviderWriteOrReservedRowRollbackError({ - error, - rollbackReservedRow: () => db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)), - companyId, - provider: provider.id, - providerConfigId: input.providerConfigId ?? null, - providerConfig, - operation: "secret.create", - }); - } - - try { - await db - .update(companySecrets) - .set({ - externalRef: prepared.externalRef, - latestVersion: 1, - updatedAt: new Date(), - }) - .where(eq(companySecrets.id, reservedSecret.id)); - await db.insert(companySecretVersions).values({ - secretId: reservedSecret.id, - version: 1, - material: prepared.material, - valueSha256: prepared.valueSha256, - fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, - providerVersionRef: prepared.providerVersionRef ?? null, - status: "disabled", - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, - }); - } catch (error) { - if (managedMode === "paperclip_managed") { - const cleaned = await cleanupPreparedProviderWrite({ - provider, - prepared, - providerConfig, - context: providerWriteContext, - mode: "delete", - operation: "create.prepare_rollback", - }); - if (!cleaned) { - throwProviderCleanupFailedAfterCreateRollback({ - companyId, - provider: provider.id, - providerConfigId: input.providerConfigId ?? null, - providerConfig, - operation: "create.prepare_rollback", - }); - } - } - await deleteLocalSecretCreateReservationOrThrow({ - db, - secretId: reservedSecret.id, - companyId, - provider: provider.id, - providerConfigId: input.providerConfigId ?? null, - providerConfig, - operation: "create.prepare_rollback", - }); - throw error; - } - - try { - return await db.transaction(async (tx) => { - await tx - .update(companySecretVersions) - .set({ status: "current" }) - .where(and( - eq(companySecretVersions.secretId, reservedSecret.id), - eq(companySecretVersions.version, 1), - )); - - const secret = await tx - .update(companySecrets) - .set({ - status: "active", - externalRef: prepared.externalRef, - latestVersion: 1, - lastRotatedAt: new Date(), - updatedAt: new Date(), - }) - .where(eq(companySecrets.id, reservedSecret.id)) - .returning() - .then((rows) => rows[0]); - - if (!secret) throw notFound("Secret not found"); - return secret; - }); - } catch (error) { - if (managedMode === "paperclip_managed") { - const cleaned = await cleanupPreparedProviderWrite({ - provider, - prepared, - providerConfig, - context: providerWriteContext, - mode: "delete", - operation: "create.rollback", - }); - if (!cleaned) { - throwProviderCleanupFailedAfterCreateRollback({ - companyId, - provider: provider.id, - providerConfigId: input.providerConfigId ?? null, - providerConfig, - operation: "create.rollback", - }); - } - } - await deleteLocalSecretCreateReservationOrThrow({ - db, - secretId: reservedSecret.id, - companyId, - provider: provider.id, - providerConfigId: input.providerConfigId ?? null, - providerConfig, - operation: "create.rollback", - }); - throw error; - } }, + // Same reasoning as `create:` above: a rotate's new value can equal a + // Codex account-home path under any provider, so every provider's rotate + // runs inside `withAccountHomeSecretMutationLock`, not only + // `local_encrypted`'s. The pre-check reads the secret only to find its + // company for the lock; `rotateUnlocked` re-reads the secret under the + // lock, so a rotation that lost a race between the pre-check and the lock + // still sees current state, not the pre-check's stale read. rotate: async ( secretId: string, input: { @@ -4093,205 +4554,36 @@ export function secretService(db: Db | DbTransaction) { externalRef?: string | null; providerVersionRef?: string | null; providerConfigId?: string | null; - // The optional owner-bound compare-and-set guard. When set, the final - // update matches the latest version, so a concurrent rotation between the - // read and the write cannot pass. A mismatch throws a 409 conflict. expectedLatestVersion?: number; }, actor?: { userId?: string | null; agentId?: string | null }, ) => { - const secret = await getById(secretId); - if (!secret) throw notFound("Secret not found"); - if (secret.status !== "active") throw unprocessable("Cannot rotate a non-active secret"); - if (input.expectedLatestVersion !== undefined && secret.latestVersion !== input.expectedLatestVersion) { - throw conflict(SECRET_VERSION_STALE_CONFLICT); - } - const providerId = secret.provider as SecretProvider; - const provider = getSecretProvider(providerId); - const providerConfigId = - input.providerConfigId === undefined ? secret.providerConfigId : input.providerConfigId; - const providerConfig = await getSelectableRuntimeProviderConfig({ - companyId: secret.companyId, - provider: providerId, - providerConfigId, + const preCheckSecret = await getById(secretId); + if (!preCheckSecret) throw notFound("Secret not found"); + return withAccountHomeSecretMutationLock(undefined, preCheckSecret.companyId, async () => { + // Same reasoning as `create:` above: check the new value's directory + // inside the same lock the write commits under, so a rotate queued + // behind an account-home cleanup cannot commit a directory the + // cleanup already removed. + if (input.value) { + await assertAccountHomeCacheDirStillValid(undefined, preCheckSecret.companyId, input.value); + } + return rotateUnlocked(secretId, input, actor); }); - const nextVersion = secret.latestVersion + 1; - const externalValueWrite = - secret.managedMode === "external_reference" && Boolean(input.value?.trim()); - if (externalValueWrite) { - const currentRef = secret.externalRef?.trim(); - if (!currentRef) { - throw unprocessable("External reference secrets require externalRef"); - } - if (input.externalRef?.trim() && input.externalRef.trim() !== currentRef) { - throw unprocessable( - "Provide either a new value or a new external reference, not both", - ); - } - if (input.providerVersionRef?.trim()) { - throw unprocessable("Value updates cannot pin providerVersionRef"); - } - if (!provider.updateExternalSecretValue) { - throw unprocessable( - `${provider.descriptor().label} does not support writing values to external reference secrets`, - ); - } - } - if (secret.managedMode === "external_reference" && !(input.externalRef ?? secret.externalRef)?.trim()) { - throw unprocessable("External reference secrets require externalRef"); - } - if (secret.managedMode !== "external_reference" && input.externalRef?.trim()) { - throw unprocessable("Managed secrets cannot override externalRef"); - } - if (secret.managedMode !== "external_reference" && !input.value?.trim()) { - throw unprocessable("Managed secrets require value"); - } - const providerWriteContext = { - companyId: secret.companyId, - secretKey: secret.key, - secretName: secret.name, - version: nextVersion, - }; - let prepared: PreparedSecretVersion; - try { - prepared = externalValueWrite - ? await provider.updateExternalSecretValue!({ - externalRef: secret.externalRef ?? "", - value: input.value ?? "", - providerConfig, - context: providerWriteContext, - }) - : secret.managedMode === "external_reference" - ? await provider.linkExternalSecret({ - externalRef: input.externalRef ?? secret.externalRef ?? "", - providerVersionRef: input.providerVersionRef ?? null, - providerConfig, - context: providerWriteContext, - }) - : await provider.createVersion({ - value: input.value ?? "", - externalRef: secret.externalRef ?? null, - providerConfig, - context: providerWriteContext, - }); - } catch (error) { - throw remoteProviderWriteHttpError(error, { - companyId: secret.companyId, - provider: provider.id, - providerConfigId, - providerConfig, - operation: "secret.rotate", - }); - } - - try { - await db.insert(companySecretVersions).values({ - secretId: secret.id, - version: nextVersion, - material: prepared.material, - valueSha256: prepared.valueSha256, - fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, - providerVersionRef: prepared.providerVersionRef ?? null, - status: "disabled", - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, - }); - } catch (error) { - if (secret.managedMode !== "external_reference" || externalValueWrite) { - await cleanupPreparedProviderWrite({ - provider, - prepared, - providerConfig, - context: providerWriteContext, - mode: "archive", - operation: "rotate.prepare_rollback", - }); - } - // A guarded concurrent rotation inserted the same next version first, so - // this insert fails the (secretId, version) unique index. The loser never - // reaches the compare-and-set guard below. Normalize only that one - // collision to the same stale conflict, so a guarded rotation always - // returns one fixed 409. Re-throw every other error unchanged. - if ( - input.expectedLatestVersion !== undefined && - isUniqueConstraintViolation(error, COMPANY_SECRET_VERSION_UNIQUE_CONSTRAINT) - ) { - throw conflict(SECRET_VERSION_STALE_CONFLICT); - } - throw error; - } - - try { - return await db.transaction(async (tx) => { - await tx - .update(companySecretVersions) - .set({ status: "previous" }) - .where(and( - eq(companySecretVersions.secretId, secret.id), - ne(companySecretVersions.version, nextVersion), - )); - await tx - .update(companySecretVersions) - .set({ status: "current" }) - .where(and( - eq(companySecretVersions.secretId, secret.id), - eq(companySecretVersions.version, nextVersion), - )); - - const updated = await tx - .update(companySecrets) - .set({ - latestVersion: nextVersion, - externalRef: prepared.externalRef, - providerConfigId, - lastRotatedAt: new Date(), - updatedAt: new Date(), - }) - .where(and( - eq(companySecrets.id, secret.id), - // The compare-and-set guard. It matches the latest version, so a - // concurrent rotation that already bumped it fails this predicate. - input.expectedLatestVersion === undefined - ? undefined - : eq(companySecrets.latestVersion, input.expectedLatestVersion), - )) - .returning() - .then((rows) => rows[0] ?? null); - - if (!updated) { - // The predicate matched no row. A supplied expected version means a - // concurrent rotation won the race; return the stale conflict. An - // unguarded rotation means the secret is gone. - throw input.expectedLatestVersion === undefined - ? notFound("Secret not found") - : conflict(SECRET_VERSION_STALE_CONFLICT); - } - return updated; - }); - } catch (error) { - if (secret.managedMode !== "external_reference" || externalValueWrite) { - const cleaned = await cleanupPreparedProviderWrite({ - provider, - prepared, - providerConfig, - context: providerWriteContext, - mode: "archive", - operation: "rotate.rollback", - }); - if (cleaned) { - await db - .delete(companySecretVersions) - .where(and( - eq(companySecretVersions.secretId, secret.id), - eq(companySecretVersions.version, nextVersion), - )) - .catch(() => undefined); - } - } - throw error; - } }, + // A patch can rename a secret (`patch.name`) or move it out of the + // `active` status (archive, disable, or a soft delete for an + // `external_reference` secret) — any of which can stop the generated + // account-home secret from resolving to the value a device-login + // promotion already validated. Run every update inside + // `withAccountHomeSecretMutationLock`, the same lock the promotion's + // terminal-commit re-check holds across its own resolve-and-commit + // section, so a rename, an archive, a disable, or a soft delete can never + // land in the gap between that re-check and the commit it guards: it + // either finishes (and the re-check observes it and rejects) before that + // section acquires the lock, or it waits for that section to finish + // first. update: async ( secretId: string, patch: { @@ -4304,95 +4596,11 @@ export function secretService(db: Db | DbTransaction) { providerMetadata?: Record | null; }, ) => { - const secret = await getById(secretId); - if (!secret) throw notFound("Secret not found"); - if (secret.status === "deleted") throw notFound("Secret not found"); - - if (patch.name && patch.name !== secret.name) { - const duplicate = await getByName(secret.companyId, patch.name); - if (duplicate && duplicate.id !== secret.id) { - throw conflict(`Secret already exists: ${patch.name}`); - } - } - const nextKey = patch.key ? normalizeSecretKey(patch.key) : secret.key; - if (!nextKey) throw unprocessable("Secret key is required"); - if (nextKey !== secret.key) { - const duplicateKey = await db - .select() - .from(companySecrets) - .where(and( - eq(companySecrets.companyId, secret.companyId), - eq(companySecrets.scope, "company"), - eq(companySecrets.key, nextKey), - ne(companySecrets.status, "deleted"), - )) - .then((rows) => rows[0] ?? null); - if (duplicateKey && duplicateKey.id !== secret.id) { - throw conflict(`Secret key already exists: ${nextKey}`); - } - } - const deleting = patch.status === "deleted"; - if (deleting && secret.managedMode === "paperclip_managed") { - throw unprocessable("Managed secrets must be deleted through DELETE /secrets/:id"); - } - if (secret.managedMode !== "external_reference" && patch.externalRef !== undefined) { - throw unprocessable("Managed secrets cannot override externalRef"); - } - if ( - secret.managedMode === "external_reference" && - patch.externalRef !== undefined && - patch.externalRef !== secret.externalRef - ) { - throw unprocessable( - "External reference secrets cannot be retargeted through generic update", - ); - } - if ( - secret.managedMode === "external_reference" && - patch.providerConfigId !== undefined && - patch.providerConfigId !== secret.providerConfigId - ) { - throw unprocessable( - "External reference secrets cannot change provider vault through generic update", - ); - } - if ( - secret.managedMode === "paperclip_managed" && - patch.providerConfigId !== undefined && - patch.providerConfigId !== secret.providerConfigId - ) { - throw unprocessable( - "Managed secrets cannot change provider vault through PATCH; use rotate() to migrate to a new vault", - ); - } - if (patch.providerConfigId !== undefined) { - await assertProviderConfigForSecret( - secret.companyId, - secret.provider as SecretProvider, - patch.providerConfigId, - ); - } - - return db - .update(companySecrets) - .set({ - key: deleting ? `${secret.key}__deleted__${secret.id}` : nextKey, - name: deleting ? `${secret.name}__deleted__${secret.id}` : patch.name ?? secret.name, - status: patch.status ?? secret.status, - providerConfigId: - patch.providerConfigId === undefined ? secret.providerConfigId : patch.providerConfigId, - description: - patch.description === undefined ? secret.description : patch.description, - externalRef: - patch.externalRef === undefined ? secret.externalRef : patch.externalRef, - providerMetadata: - patch.providerMetadata === undefined ? secret.providerMetadata : patch.providerMetadata, - deletedAt: deleting ? new Date() : secret.deletedAt, - updatedAt: new Date(), - }) - .where(eq(companySecrets.id, secret.id)) - .returning() - .then((rows) => rows[0] ?? null); + const preCheckSecret = await getById(secretId); + if (!preCheckSecret) throw notFound("Secret not found"); + return withAccountHomeSecretMutationLock(undefined, preCheckSecret.companyId, () => + updateUnlocked(secretId, patch), + ); }, createBinding: async (input: {