diff --git a/.changeset/tidy-onepassword-latency.md b/.changeset/tidy-onepassword-latency.md new file mode 100644 index 000000000..a9e826e8d --- /dev/null +++ b/.changeset/tidy-onepassword-latency.md @@ -0,0 +1,13 @@ +--- +"@executor-js/plugin-onepassword": patch +--- + +**1Password-backed connections no longer pay a 1Password read on every tool call** + +Each tool call resolves its connection's credential, and for 1Password-backed connections every resolution shelled out to the `op` CLI — roughly a second per call under desktop-app auth, multiplying the latency of every call several times over. The spawn was also synchronous, so one slow resolution (for example `op` waiting on a 1Password approval prompt) blocked the whole local server for every other request, with no timeout on that path. + +Three changes: + +- Successful resolutions are now served from memory for a short TTL (default 60s, `secretCacheTtlMs`). The cache keys by a fingerprint of the provider config, so editing or removing an account drops all cached secrets at once; not-found, ambiguity, and failure outcomes are never retained. Concurrent resolutions of the same ref share one backend read even with the TTL set to `0`. +- The `op` CLI now runs as an asynchronous spawn with a hard deadline (the plugin's existing `timeoutMs`), so a stuck `op` fails with the troubleshooting message instead of freezing the server. Auth reaches the child per spawn (service-account token via the environment, desktop account via `--account`) instead of through the previous backend's process-global token state. +- Services are memoized per auth identity, so the SDK fallback reuses one authenticated client instead of re-authenticating per resolution. diff --git a/bun.lock b/bun.lock index 43add4725..057c64e75 100644 --- a/bun.lock +++ b/bun.lock @@ -1036,7 +1036,6 @@ "name": "@executor-js/plugin-onepassword", "version": "1.6.3", "dependencies": { - "@1password/op-js": "^0.1.13", "@1password/sdk": "^0.4.1-beta.1", "@effect/atom-react": "catalog:", "@executor-js/sdk": "workspace:*", @@ -1291,8 +1290,6 @@ "packages": { "7zip-bin": ["7zip-bin@5.2.0", "", {}, "sha512-ukTPVhqG4jNzMro2qA9HSCSSVJN3aN7tlb+hfqYCt3ER0yWroeA2VR38MNrOHLQ/cVj+DaIMad0kFCtWWowh/A=="], - "@1password/op-js": ["@1password/op-js@0.1.13", "", { "dependencies": { "lookpath": "^1.2.2", "semver": "^7.6.2" } }, "sha512-ZZBLxVqywFdvIbLv2xWw2N1ImSi183rRKf90vV19KRMReNyLwuD0dv6IrKrIdrJU33IuV3Gz85Z4K2a1PJTBDg=="], - "@1password/sdk": ["@1password/sdk@0.4.1-beta.1", "", { "dependencies": { "@1password/sdk-core": "0.4.1-beta.1" } }, "sha512-QWY6eIIEs9at6kAFbgjZ1vVxg7ygp/MwXJ6/BoBdPkqIk8vfulXu4pjDsVQ87HhddJAD5LVGBXSbQIa+svI3bA=="], "@1password/sdk-core": ["@1password/sdk-core@0.4.1-beta.1", "", {}, "sha512-/otbg1JVhsEn6oUIeReoT9TmFr8J7KBwr9UuRVfJFwwGG3bHPF8ewT+LhRimQeJtypqQ69ZVuOYkxknD4iQHxw=="], @@ -4565,8 +4562,6 @@ "longest-streak": ["longest-streak@3.1.0", "", {}, "sha512-9Ri+o0JYgehTaVBBDoMqIl8GXtbWg711O3srftcHhZ0dqnETqLaoIK0x17fUw9rFSlK/0NlsKe0Ahhyl5pXE2g=="], - "lookpath": ["lookpath@1.2.3", "", { "bin": { "lookpath": "bin/lookpath.js" } }, "sha512-kthRVhf4kH4+HW3anM4UBHxsw/XFESf13euCEldhXr6GpBdmBoa7rDd7WO5G0Mhd4G5XtKTcEy8OR0iRZXpS3Q=="], - "loose-envify": ["loose-envify@1.4.0", "", { "dependencies": { "js-tokens": "^3.0.0 || ^4.0.0" }, "bin": { "loose-envify": "cli.js" } }, "sha512-lyuxPGr/Wfhrlem2CL/UcnUc1zcqKAImBDzukY7Y5F/yQiNdko6+fRLevlw1HgMySw7f611UIY408EtxRSoK3Q=="], "lowercase-keys": ["lowercase-keys@2.0.0", "", {}, "sha512-tqNXrS78oMOE73NMxK4EMLQsQowWf8jKooH9g7xPavRT706R6bkQJ6DY2Te7QukaZsulxa30wQ7bk0pm4XiHmA=="], @@ -5871,8 +5866,6 @@ "zwitch": ["zwitch@2.0.4", "", {}, "sha512-bXE4cR/kVZhKZX/RjPEflHaKVhUVl85noU3v6b8apfQEc1x4A+zBxjZ4lN8LqGd6WZ3dl98pY4o717VFmoPp+A=="], - "@1password/op-js/semver": ["semver@7.7.4", "", { "bin": { "semver": "bin/semver.js" } }, "sha512-vFKC2IEtQnVhpT78h1Yp8wzwrf8CM+MzKMHGJZfBtzhZNycRFnXsHk6E5TxIkkMsgNS7mdX3AGB7x2QM2di4lA=="], - "@antfu/install-pkg/package-manager-detector": ["package-manager-detector@1.6.0", "", {}, "sha512-61A5ThoTiDG/C8s8UMZwSorAGwMJ0ERVGj2OjoW5pAalsNOg15+iQiPzrLJ4jhZ1HJzmC2PIHT2oEiH3R5fzNA=="], "@astrojs/cloudflare/vite": ["vite@7.3.2", "", { "dependencies": { "esbuild": "^0.27.0", "fdir": "^6.5.0", "picomatch": "^4.0.3", "postcss": "^8.5.6", "rollup": "^4.43.0", "tinyglobby": "^0.2.15" }, "optionalDependencies": { "fsevents": "~2.3.3" }, "peerDependencies": { "@types/node": "^20.19.0 || >=22.12.0", "jiti": ">=1.21.0", "less": "^4.0.0", "lightningcss": "^1.21.0", "sass": "^1.70.0", "sass-embedded": "^1.70.0", "stylus": ">=0.54.8", "sugarss": "^5.0.0", "terser": "^5.16.0", "tsx": "^4.8.1", "yaml": "^2.4.2" }, "optionalPeers": ["@types/node", "jiti", "less", "lightningcss", "sass", "sass-embedded", "stylus", "sugarss", "terser", "tsx", "yaml"], "bin": { "vite": "bin/vite.js" } }, "sha512-Bby3NOsna2jsjfLVOHKes8sGwgl4TT0E6vvpYgnAYDIF/tie7MRaFthmKuHx1NSXjiTueXH3do80FMQgvEktRg=="], diff --git a/packages/plugins/onepassword/package.json b/packages/plugins/onepassword/package.json index 069d687f9..5af0fba19 100644 --- a/packages/plugins/onepassword/package.json +++ b/packages/plugins/onepassword/package.json @@ -53,7 +53,6 @@ "typecheck:slow": "bunx tsc --noEmit -p tsconfig.json" }, "dependencies": { - "@1password/op-js": "^0.1.13", "@1password/sdk": "^0.4.1-beta.1", "@effect/atom-react": "catalog:", "@executor-js/sdk": "workspace:*" diff --git a/packages/plugins/onepassword/src/sdk/op-cli.ts b/packages/plugins/onepassword/src/sdk/op-cli.ts new file mode 100644 index 000000000..94e87211b --- /dev/null +++ b/packages/plugins/onepassword/src/sdk/op-cli.ts @@ -0,0 +1,77 @@ +import { execFile } from "node:child_process"; + +// Raw `op` CLI spawn boundary. Kept as its own module so the service can be +// tested against a fake without mocking node builtins. The spawn is +// asynchronous on purpose: the previous backend (`@1password/op-js`) ran `op` +// with execFileSync, so an `op` stuck on a 1Password approval prompt blocked +// the host's entire event loop — on the single-threaded local daemon that +// froze every in-flight request until the prompt was answered. + +/** Spawn outcome as plain data. The promise always resolves; the service + * layer owns failure typing and message shaping (redaction, truncation). */ +export type OpCliResult = + | { readonly ok: true; readonly stdout: string } + | { + readonly ok: false; + /** True when the child was killed by the spawn timeout. */ + readonly timedOut: boolean; + readonly message: string; + }; + +export interface OpCliInvocation { + readonly args: readonly string[]; + readonly env: Readonly>; + /** Hard deadline for the child; on expiry it is killed and the result + * carries `timedOut: true`. */ + readonly timeoutMs: number; + /** Fiber interruption reaches the child through this signal. */ + readonly signal: AbortSignal; +} + +const isExecFileError = ( + error: unknown, +): error is NodeJS.ErrnoException & { readonly killed?: boolean } => + typeof error === "object" && error !== null && "message" in error; + +const describeSpawnError = (error: unknown): string => { + if (isExecFileError(error)) { + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: normalizing the untyped execFile callback error into plain result data + return error.message; + } + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: last-resort stringification of a non-Error spawn failure + return String(error); +}; + +/** Run `op` once. stdout carries the successful payload; stderr carries the + * CLI's human-readable diagnostics, so a non-zero exit reports stderr when + * present (falling back to the spawn error, e.g. `spawn op ENOENT`). */ +export const opCliExec = ({ + args, + env, + timeoutMs, + signal, +}: OpCliInvocation): Promise => + new Promise((resolve) => { + execFile( + "op", + args, + { + env: env as NodeJS.ProcessEnv, + timeout: timeoutMs, + signal, + maxBuffer: 16 * 1024 * 1024, + }, + (error, stdout, stderr) => { + if (error === null) { + resolve({ ok: true, stdout }); + return; + } + const stderrText = stderr.trim(); + resolve({ + ok: false, + timedOut: isExecFileError(error) && error.killed === true, + message: stderrText.length > 0 ? stderrText : describeSpawnError(error), + }); + }, + ); + }); diff --git a/packages/plugins/onepassword/src/sdk/plugin.test.ts b/packages/plugins/onepassword/src/sdk/plugin.test.ts index 61e1f4ef6..7cec7de17 100644 --- a/packages/plugins/onepassword/src/sdk/plugin.test.ts +++ b/packages/plugins/onepassword/src/sdk/plugin.test.ts @@ -1,11 +1,17 @@ import { describe, it, expect } from "@effect/vitest"; import { Effect } from "effect"; +import { TestClock } from "effect/testing"; import { ProviderKey, ToolAddress, createExecutor } from "@executor-js/sdk"; import { makeInMemoryBlobStore, pluginBlobStore } from "@executor-js/sdk/core"; import { makeTestConfig } from "@executor-js/sdk/testing"; -import { makeOnePasswordStore, onepasswordPlugin, resolveConfiguredRef } from "./plugin"; +import { + makeCachedRefResolver, + makeOnePasswordStore, + onepasswordPlugin, + resolveConfiguredRef, +} from "./plugin"; import type { OnePasswordService } from "./service"; import { OnePasswordError } from "./errors"; import { OnePasswordAccount, OnePasswordConfig, DesktopAppAuth } from "./types"; @@ -524,3 +530,121 @@ describe("resolveConfiguredRef", () => { }), ); }); + +// --------------------------------------------------------------------------- +// Cached ref resolution — the executor resolves a connection's credential on +// every tool call, so successful resolutions are served from memory for a +// short TTL instead of paying a 1Password round trip per call. +// --------------------------------------------------------------------------- + +describe("makeCachedRefResolver", () => { + const countingBackend = () => { + let resolves = 0; + const serviceFor = (account: OnePasswordAccount) => + Effect.succeed({ + resolveSecret: (uri) => + Effect.sync(() => { + resolves += 1; + return `secret:${account.id}:${uri}`; + }), + listVaults: () => Effect.succeed([]), + listItems: () => Effect.succeed([]), + }); + return { serviceFor, resolveCount: () => resolves }; + }; + + it.effect("serves a repeated resolution from memory within the TTL", () => + Effect.gen(function* () { + const backend = countingBackend(); + const resolve = makeCachedRefResolver(backend.serviceFor, 60_000); + + const first = yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + const second = yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + + expect(first).toEqual({ + kind: "resolved", + value: "secret:acct-default:op://vault-123/item-1/credential", + }); + expect(second).toEqual(first); + expect(backend.resolveCount()).toBe(1); + }), + ); + + it.effect("asks the backend again once the TTL has passed", () => + Effect.gen(function* () { + const backend = countingBackend(); + const resolve = makeCachedRefResolver(backend.serviceFor, 60_000); + + yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + yield* TestClock.adjust("61 seconds"); + yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + + expect(backend.resolveCount()).toBe(2); + }), + ); + + it.effect("never retains a not-found outcome", () => + Effect.gen(function* () { + // A bare ref against empty vault listings resolves to not-found; the + // item may be created a moment later, so the miss must not stick. + const backend = countingBackend(); + let listings = 0; + const serviceFor = (account: OnePasswordAccount) => + backend.serviceFor(account).pipe( + Effect.map((service) => ({ + ...service, + listItems: () => + Effect.sync(() => { + listings += 1; + return []; + }), + })), + ); + const resolve = makeCachedRefResolver(serviceFor, 60_000); + + const first = yield* resolve(oneAccountConfig, "missing-item"); + const second = yield* resolve(oneAccountConfig, "missing-item"); + + expect(first).toEqual({ kind: "not-found" }); + expect(second).toEqual({ kind: "not-found" }); + // Two vaults in the config, listed once per resolution. + expect(listings).toBe(4); + }), + ); + + it.effect("drops every cached secret the moment the config changes", () => + Effect.gen(function* () { + const backend = countingBackend(); + const resolve = makeCachedRefResolver(backend.serviceFor, 60_000); + + yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + // Same ref, edited config (one vault removed): a removed account or + // vault must not keep serving secrets it used to grant. + const edited = OnePasswordConfig.make({ + accounts: [ + OnePasswordAccount.make({ + id: "acct-default", + name: "1Password", + auth: desktopAuth, + vaults: [{ id: "vault-123", name: "Personal" }], + }), + ], + }); + yield* resolve(edited, "op://vault-123/item-1/credential"); + + expect(backend.resolveCount()).toBe(2); + }), + ); + + it.effect("with a zero TTL every sequential resolution reaches the backend", () => + Effect.gen(function* () { + const backend = countingBackend(); + const resolve = makeCachedRefResolver(backend.serviceFor, 0); + + yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential"); + + expect(backend.resolveCount()).toBe(2); + }), + ); +}); diff --git a/packages/plugins/onepassword/src/sdk/plugin.ts b/packages/plugins/onepassword/src/sdk/plugin.ts index 995146584..c2505db21 100644 --- a/packages/plugins/onepassword/src/sdk/plugin.ts +++ b/packages/plugins/onepassword/src/sdk/plugin.ts @@ -1,4 +1,4 @@ -import { Effect, Schema } from "effect"; +import { Cache, Data, Duration, Effect, Exit, Schema } from "effect"; import { definePlugin, @@ -37,6 +37,17 @@ import { makeOnePasswordService, type ResolvedAuth, type OnePasswordService } fr const CREDENTIAL_FIELD = "credential"; const DEFAULT_TIMEOUT_MS = 15_000; +// How long a resolved secret may be served from memory before 1Password is +// asked again. Every 1Password read is at least one `op` spawn or SDK IPC +// round trip (~1s with desktop-app auth), and the executor resolves the +// connection's credential on every tool call — uncached, a 1Password-backed +// connection multiplied each call's latency several times over. One minute +// keeps a revoked or rotated item's window small while collapsing an agent's +// call burst onto a single backend read. This cache is deliberately scoped to +// this plugin; other credential providers stay uncached. +const DEFAULT_SECRET_CACHE_TTL_MS = 60_000; +const SECRET_CACHE_CAPACITY = 256; +const SERVICE_CACHE_CAPACITY = 16; const CONFIG_KEY = "config"; const PROVIDER_KEY = ProviderKey.make("onepassword"); @@ -188,12 +199,49 @@ const resolveAuth = (auth: OnePasswordAuth): ResolvedAuth => ? { kind: "desktop-app", accountName: auth.accountName } : { kind: "service-account", token: auth.token }; -/** One service per account: each account carries its own auth, so a shared - * client can never leak one account's credential into another's calls. */ -const serviceForAccount = - (timeoutMs: number, preferSdk: boolean | undefined) => - (account: OnePasswordAccount): Effect.Effect => - makeOnePasswordService(resolveAuth(account.auth), { timeoutMs, preferSdk }); +/** Cache keys with structural equality (`Data.Class`), one per auth shape so + * the two kinds can never collide. Every account row carrying the same + * credential identity maps onto one cached service. */ +class DesktopAuthKey extends Data.Class<{ readonly accountName: string }> {} +class ServiceAccountKey extends Data.Class<{ readonly token: string }> {} +type AuthKey = DesktopAuthKey | ServiceAccountKey; + +const authKey = (auth: ResolvedAuth): AuthKey => + auth.kind === "desktop-app" + ? new DesktopAuthKey({ accountName: auth.accountName }) + : new ServiceAccountKey({ token: auth.token }); + +const authFromKey = (key: AuthKey): ResolvedAuth => + key instanceof DesktopAuthKey + ? { kind: "desktop-app", accountName: key.accountName } + : { kind: "service-account", token: key.token }; + +/** One service per auth identity, memoized for the plugin instance's + * lifetime. Keying by the resolved auth keeps accounts isolated (a shared + * client can never leak one account's credential into another's calls) while + * reusing the constructed service — and with it the SDK backend's cached + * client — across calls instead of re-authenticating per resolution. + * Construction failures are not retained, so a transient backend problem + * does not poison the account until restart. */ +export const makeServiceForAuth = ( + timeoutMs: number, + preferSdk: boolean | undefined, +): ((auth: OnePasswordAuth) => Effect.Effect) => { + const cache = Effect.runSync( + Cache.makeWith( + (key: AuthKey) => makeOnePasswordService(authFromKey(key), { timeoutMs, preferSdk }), + { + capacity: SERVICE_CACHE_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? Duration.infinity : Duration.zero), + }, + ), + ); + return (auth) => Cache.get(cache, authKey(resolveAuth(auth))); +}; + +type ServiceForAccount = ( + account: OnePasswordAccount, +) => Effect.Effect; // --------------------------------------------------------------------------- // Explicit ref resolution. @@ -359,6 +407,61 @@ export const resolveConfiguredRef = ( }); }; +// --------------------------------------------------------------------------- +// Cached ref resolution — the hot path. +// +// The executor resolves a connection's credential on EVERY tool call, and for +// this provider each resolution is at least one `op` spawn or SDK IPC round +// trip (~1s under desktop-app auth; a bare ref pays a per-vault item listing +// on top). Successful resolutions are therefore served from memory for a +// short TTL. The cache is keyed by (config fingerprint, ref): editing or +// removing an account changes the fingerprint, which drops the whole cached +// generation immediately — a removed account's secrets never outlive the +// config that granted them, and no explicit invalidation wiring is needed. +// Only `kind: "resolved"` entries get the TTL; not-found, ambiguity, and +// failures are never retained, so a just-created item resolves on the next +// call. Concurrent lookups for the same key share one backend read even when +// the TTL is 0. +// --------------------------------------------------------------------------- + +export type CachedRefResolver = ( + config: OnePasswordConfig, + ref: string, +) => Effect.Effect; + +export const makeCachedRefResolver = ( + serviceFor: ServiceForAccount, + ttlMs: number, +): CachedRefResolver => { + // Single generation: config edits are rare and refs from an older config + // must not be served, so the previous generation's cache is discarded + // rather than kept alongside. + let generation: { + readonly fingerprint: string; + readonly cache: Cache.Cache; + } | null = null; + + return (config, ref) => + Effect.suspend(() => { + const fingerprint = JSON.stringify(config.accounts); + if (generation === null || generation.fingerprint !== fingerprint) { + generation = { + fingerprint, + cache: Effect.runSync( + Cache.makeWith((key: string) => resolveConfiguredRef(config, serviceFor, key), { + capacity: SECRET_CACHE_CAPACITY, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value.kind === "resolved" + ? Duration.millis(ttlMs) + : Duration.zero, + }), + ), + }; + } + return Cache.get(generation.cache, ref); + }); +}; + // --------------------------------------------------------------------------- // CredentialProvider — read-only, resolves op:// URIs or vault-scoped lookups. // @@ -371,10 +474,9 @@ export const resolveConfiguredRef = ( const makeProvider = ( ctx: PluginCtx, - timeoutMs: number, - preferSdk: boolean | undefined, + serviceFor: ServiceForAccount, + resolveRef: CachedRefResolver, ): CredentialProvider => { - const serviceFor = serviceForAccount(timeoutMs, preferSdk); return { key: PROVIDER_KEY, writable: false, @@ -387,7 +489,7 @@ const makeProvider = ( Effect.flatMap((config) => { if (!config) return Effect.succeed(null as string | null); - return resolveConfiguredRef(config, serviceFor, id).pipe( + return resolveRef(config, id).pipe( // Backend unreachability degrades to "no value", matching the other // providers. Ambiguity does NOT: silently picking a vault (or // silently failing) hides a real conflict, so it surfaces as a @@ -463,10 +565,9 @@ const ownerForCtx = (ctx: PluginCtx): Owner => const makeOnePasswordExtension = ( ctx: PluginCtx, - timeoutMs: number, - preferSdk: boolean | undefined, + serviceForAuth: (auth: OnePasswordAuth) => Effect.Effect, ) => { - const serviceFor = serviceForAccount(timeoutMs, preferSdk); + const serviceFor: ServiceForAccount = (account) => serviceForAuth(account.auth); const accountStatus = (account: OnePasswordAccount): Effect.Effect => serviceFor(account).pipe( @@ -582,10 +683,7 @@ const makeOnePasswordExtension = ( listVaults: (auth: OnePasswordAuth) => Effect.gen(function* () { - const svc = yield* makeOnePasswordService(resolveAuth(auth), { - timeoutMs, - preferSdk, - }); + const svc = yield* serviceForAuth(auth); const vaults = yield* svc.listVaults(); return vaults .map((v) => Vault.make({ id: v.id, name: v.title })) @@ -640,18 +738,32 @@ export interface OnePasswordPluginOptions { readonly timeoutMs?: number; /** Force use of the native SDK instead of the CLI (default: false) */ readonly preferSdk?: boolean; + /** How long a successfully resolved secret may be served from memory + * before 1Password is asked again (default: 60000). `0` disables reuse + * while still collapsing concurrent resolutions of the same ref onto one + * backend read. */ + readonly secretCacheTtlMs?: number; } export const onepasswordPlugin = definePlugin((options?: OnePasswordPluginOptions) => { const timeoutMs = options?.timeoutMs ?? DEFAULT_TIMEOUT_MS; const preferSdk = options?.preferSdk; + const secretCacheTtlMs = options?.secretCacheTtlMs ?? DEFAULT_SECRET_CACHE_TTL_MS; + + // Shared across the extension and the credential provider so both reuse the + // same per-account services (and the SDK backend's cached client). The + // secret cache keys by config fingerprint, so sharing it across executors + // built from this factory cannot cross owner partitions. + const serviceForAuth = makeServiceForAuth(timeoutMs, preferSdk); + const serviceFor: ServiceForAccount = (account) => serviceForAuth(account.auth); + const resolveRef = makeCachedRefResolver(serviceFor, secretCacheTtlMs); return { id: "onepassword" as const, packageName: "@executor-js/plugin-onepassword", storage: ({ blobs }) => makeOnePasswordStore(blobs), - extension: (ctx) => makeOnePasswordExtension(ctx, timeoutMs, preferSdk), + extension: (ctx) => makeOnePasswordExtension(ctx, serviceForAuth), staticIntegrations: (self) => [ { @@ -720,7 +832,7 @@ export const onepasswordPlugin = definePlugin((options?: OnePasswordPluginOption }, ], - credentialProviders: (ctx) => [makeProvider(ctx, timeoutMs, preferSdk)], + credentialProviders: (ctx) => [makeProvider(ctx, serviceFor, resolveRef)], }; // HTTP transport (routes/handlers/extensionService) is layered on by // the api-aware factory in `@executor-js/plugin-onepassword/api`. Hosts diff --git a/packages/plugins/onepassword/src/sdk/service.test.ts b/packages/plugins/onepassword/src/sdk/service.test.ts index 13d4c5f97..96133af67 100644 --- a/packages/plugins/onepassword/src/sdk/service.test.ts +++ b/packages/plugins/onepassword/src/sdk/service.test.ts @@ -4,14 +4,11 @@ import { Effect } from "effect"; import { vi } from "vitest"; import { OnePasswordError } from "./errors"; +import type { OpCliInvocation, OpCliResult } from "./op-cli"; import { makeOnePasswordService } from "./service"; const opMocks = vi.hoisted(() => ({ - setGlobalFlags: vi.fn(), - setServiceAccount: vi.fn(), - vaultList: vi.fn(), - itemList: vi.fn(), - readParse: vi.fn(), + opCliExec: vi.fn<(invocation: OpCliInvocation) => Promise>(), })); const sdkMocks = vi.hoisted(() => ({ @@ -23,22 +20,32 @@ const sdkMocks = vi.hoisted(() => ({ }, })); -vi.mock("@1password/op-js", () => ({ - setGlobalFlags: opMocks.setGlobalFlags, - setServiceAccount: opMocks.setServiceAccount, - vault: { list: opMocks.vaultList }, - item: { list: opMocks.itemList }, - read: { parse: opMocks.readParse }, +vi.mock("./op-cli", () => ({ + opCliExec: opMocks.opCliExec, })); vi.mock("@1password/sdk", () => sdkMocks.exports); +/** A failed spawn: op-cli always resolves, reporting failure as plain data. */ +const cliFailure = (message: string, timedOut = false): OpCliResult => ({ + ok: false, + timedOut, + message, +}); + +const cliAnswers = (answer: (invocation: OpCliInvocation) => string) => { + opMocks.opCliExec.mockImplementation((invocation) => + Promise.resolve({ ok: true, stdout: answer(invocation) }), + ); +}; + describe("makeOnePasswordService", () => { beforeEach(() => { vi.clearAllMocks(); - opMocks.vaultList.mockReturnValue([]); - opMocks.itemList.mockReturnValue([]); - opMocks.readParse.mockReturnValue("secret"); + cliAnswers((invocation) => { + if (invocation.args[0] === "read") return "secret\n"; + return "[]"; + }); sdkMocks.exports.createClient = sdkMocks.createClient; sdkMocks.exports.DesktopAuth = sdkMocks.DesktopAuth; sdkMocks.createClient.mockResolvedValue({ @@ -48,18 +55,57 @@ describe("makeOnePasswordService", () => { }); }); - it.effect("falls back to the SDK when the CLI throws while listing vaults", () => + it.effect("resolves a secret via the CLI and strips the trailing newline", () => Effect.gen(function* () { - const sdkVaultsList = vi.fn(async () => [{ id: "sdk-vault", title: "SDK Vault" }]); - opMocks.vaultList.mockImplementation(() => { - // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing - throw new Error("spawn op ENOENT"); - }); - sdkMocks.createClient.mockResolvedValue({ - secrets: { resolve: vi.fn(async () => "secret") }, - vaults: { list: sdkVaultsList }, - items: { list: vi.fn(async () => []) }, - }); + const service = yield* makeOnePasswordService( + { kind: "service-account", token: "ops_test_token" }, + { timeoutMs: 1_000 }, + ); + const secret = yield* service.resolveSecret("op://vault/item/field"); + + expect(secret).toBe("secret"); + const invocation = opMocks.opCliExec.mock.calls[0]?.[0]; + expect(invocation?.args).toEqual(["read", "op://vault/item/field"]); + expect(invocation?.timeoutMs).toBe(1_000); + }), + ); + + it.effect("passes the service-account token through the child environment only", () => + Effect.gen(function* () { + const service = yield* makeOnePasswordService( + { kind: "service-account", token: "ops_test_token" }, + { timeoutMs: 1_000 }, + ); + yield* service.resolveSecret("op://vault/item/field"); + + const invocation = opMocks.opCliExec.mock.calls[0]?.[0]; + expect(invocation?.env["OP_SERVICE_ACCOUNT_TOKEN"]).toBe("ops_test_token"); + expect(invocation?.args.some((arg) => arg.includes("ops_test_token"))).toBe(false); + expect(invocation?.args.some((arg) => arg.startsWith("--account"))).toBe(false); + }), + ); + + it.effect("routes desktop-app auth through --account with no inherited token", () => + Effect.gen(function* () { + process.env["OP_SERVICE_ACCOUNT_TOKEN"] = "ops_inherited_token"; + const service = yield* makeOnePasswordService( + { kind: "desktop-app", accountName: "my.1password.com" }, + { timeoutMs: 1_000 }, + ); + yield* service.resolveSecret("op://vault/item/field"); + delete process.env["OP_SERVICE_ACCOUNT_TOKEN"]; + + const invocation = opMocks.opCliExec.mock.calls[0]?.[0]; + expect(invocation?.args).toContain("--account=my.1password.com"); + // An inherited token would override --account and silently reroute the + // call to the parent process's service account. + expect(invocation?.env["OP_SERVICE_ACCOUNT_TOKEN"]).toBeUndefined(); + }), + ); + + it.effect("lists vaults through the CLI's JSON output", () => + Effect.gen(function* () { + cliAnswers(() => JSON.stringify([{ id: "vault-1", name: "Personal", extra: "ignored" }])); const service = yield* makeOnePasswordService( { kind: "service-account", token: "ops_test_token" }, @@ -67,26 +113,23 @@ describe("makeOnePasswordService", () => { ); const vaults = yield* service.listVaults(); - expect(vaults).toEqual([{ id: "sdk-vault", title: "SDK Vault" }]); - expect(sdkMocks.createClient).toHaveBeenCalledTimes(1); - expect(sdkVaultsList).toHaveBeenCalledTimes(1); + expect(vaults).toEqual([{ id: "vault-1", title: "Personal" }]); + const invocation = opMocks.opCliExec.mock.calls[0]?.[0]; + expect(invocation?.args).toEqual(["vault", "list", "--format=json"]); }), ); - it.effect("includes the backend cause when both vault listing backends fail", () => + it.effect("maps a killed-by-timeout spawn onto the troubleshooting message", () => Effect.gen(function* () { - opMocks.vaultList.mockImplementation(() => { - // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing - throw new Error("spawn op ENOENT"); - }); + opMocks.opCliExec.mockResolvedValue(cliFailure("", true)); sdkMocks.createClient.mockResolvedValue({ - secrets: { resolve: vi.fn(async () => "secret") }, - vaults: { - list: vi.fn(async () => { + secrets: { + resolve: vi.fn(async () => { // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped 1Password SDK rejecting - throw new Error("desktop approval refused for account"); + throw new Error("sdk unavailable"); }), }, + vaults: { list: vi.fn(async () => []) }, items: { list: vi.fn(async () => []) }, }); @@ -94,117 +137,120 @@ describe("makeOnePasswordService", () => { { kind: "service-account", token: "ops_test_token" }, { timeoutMs: 1_000 }, ).pipe( - Effect.flatMap((service) => service.listVaults()), + Effect.flatMap((service) => service.resolveSecret("op://vault/item/field")), Effect.flip, ); expect(error).toBeInstanceOf(OnePasswordError); // oxlint-disable executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed message; asserting its contents - expect(error.message).toContain("1Password SDK vault listing failed:"); - expect(error.message).toContain("desktop approval refused for account"); - expect(error.message).not.toBe("1Password CLI vault listing failed"); + expect(error.message).toContain("timed out after 1s"); + expect(error.message).toContain("1Password desktop app is open and unlocked"); // oxlint-enable executor/no-unknown-error-message }), ); - it.effect("reports a clear SDK load error when the compiled namespace is empty", () => + it.effect("redacts a service-account token leaked into CLI stderr", () => Effect.gen(function* () { - opMocks.vaultList.mockImplementation(() => { - // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing - throw new Error("spawn op ENOENT"); + opMocks.opCliExec.mockResolvedValue(cliFailure("[ERROR] token ops_test_token was rejected")); + sdkMocks.createClient.mockResolvedValue({ + secrets: { + resolve: vi.fn(async () => { + // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped 1Password SDK rejecting + throw new Error("sdk unavailable"); + }), + }, + vaults: { list: vi.fn(async () => []) }, + items: { list: vi.fn(async () => []) }, }); - sdkMocks.exports.createClient = undefined; - sdkMocks.exports.DesktopAuth = undefined; const error = yield* makeOnePasswordService( { kind: "service-account", token: "ops_test_token" }, { timeoutMs: 1_000 }, ).pipe( - Effect.flatMap((service) => service.listVaults()), + Effect.flatMap((service) => service.resolveSecret("op://vault/item/field")), Effect.flip, ); - expect(error).toBeInstanceOf(OnePasswordError); - expect(error.operation).toBe("sdk module load"); // oxlint-disable executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed message; asserting its contents - expect(error.message).toContain("did not expose createClient and DesktopAuth"); - expect(error.message).toContain("/opt/homebrew/bin"); + expect(error.message).not.toContain("ops_test_token"); + expect(error.message).toContain("[redacted 1Password token]"); // oxlint-enable executor/no-unknown-error-message }), ); - // ------------------------------------------------------------------------- - // Service-account token lifetime. - // - // `op-js` parks the token on a process-global (`cli.serviceAccountToken`) and - // reads it when spawning `op`. Nothing in the library clears it, so without - // the `ensuring` in `makeCliService` a token set to serve one resolve stays - // readable for the rest of the process's life. - // - // Both halves are pinned on purpose: clearing it is only correct if it is - // still SET while the call runs. A change that cleared it too early would - // pass a "no longer parked" assertion and silently break authentication. - // - // The "still set" half has to be observed from INSIDE the spawn: `op-js` - // builds the child's env from the global at spawn time, and the mock's call - // ledger cannot see the ordering between the token set and the spawn — a - // clear hoisted before `fn()` leaves [token, "", ""], which keeps both - // after-the-fact assertions green. - // ------------------------------------------------------------------------- - - it.effect("clears the service-account token from the op-js global after a CLI call", () => + it.effect("falls back to the SDK when the CLI is not installed", () => Effect.gen(function* () { - let tokenAtSpawn: unknown; - opMocks.readParse.mockImplementation(() => { - tokenAtSpawn = opMocks.setServiceAccount.mock.lastCall?.[0]; - return "resolved-secret"; + const sdkVaultsList = vi.fn(async () => [{ id: "sdk-vault", title: "SDK Vault" }]); + opMocks.opCliExec.mockResolvedValue(cliFailure("spawn op ENOENT")); + sdkMocks.createClient.mockResolvedValue({ + secrets: { resolve: vi.fn(async () => "secret") }, + vaults: { list: sdkVaultsList }, + items: { list: vi.fn(async () => []) }, }); const service = yield* makeOnePasswordService( { kind: "service-account", token: "ops_test_token" }, { timeoutMs: 1_000 }, ); - const secret = yield* service.resolveSecret("op://vault/item/field"); + const vaults = yield* service.listVaults(); - expect(secret).toBe("resolved-secret"); - // Still set while the spawn ran — clearing any earlier would break auth... - expect(tokenAtSpawn).toBe("ops_test_token"); - // ...and gone by the time the call is over. - expect(opMocks.setServiceAccount).toHaveBeenLastCalledWith(""); + expect(vaults).toEqual([{ id: "sdk-vault", title: "SDK Vault" }]); + expect(sdkMocks.createClient).toHaveBeenCalledTimes(1); + expect(sdkVaultsList).toHaveBeenCalledTimes(1); }), ); - it.effect("clears the token even when the CLI call fails", () => + it.effect("includes the backend cause when both vault listing backends fail", () => Effect.gen(function* () { - // The failure path is the one that matters most: an error unwinding past a - // manual "clear it afterwards" line is exactly how a token gets stranded. - let tokenAtSpawn: unknown; - opMocks.readParse.mockImplementation(() => { - tokenAtSpawn = opMocks.setServiceAccount.mock.lastCall?.[0]; - // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing - throw new Error("spawn op ENOENT"); - }); + opMocks.opCliExec.mockResolvedValue(cliFailure("spawn op ENOENT")); sdkMocks.createClient.mockResolvedValue({ - secrets: { - resolve: vi.fn(async () => { + secrets: { resolve: vi.fn(async () => "secret") }, + vaults: { + list: vi.fn(async () => { // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped 1Password SDK rejecting - throw new Error("sdk unavailable"); + throw new Error("desktop approval refused for account"); }), }, - vaults: { list: vi.fn(async () => []) }, items: { list: vi.fn(async () => []) }, }); - yield* makeOnePasswordService( + const error = yield* makeOnePasswordService( { kind: "service-account", token: "ops_test_token" }, { timeoutMs: 1_000 }, ).pipe( - Effect.flatMap((service) => service.resolveSecret("op://vault/item/field")), + Effect.flatMap((service) => service.listVaults()), Effect.flip, ); - expect(tokenAtSpawn).toBe("ops_test_token"); - expect(opMocks.setServiceAccount).toHaveBeenLastCalledWith(""); + expect(error).toBeInstanceOf(OnePasswordError); + // oxlint-disable executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed message; asserting its contents + expect(error.message).toContain("1Password SDK vault listing failed:"); + expect(error.message).toContain("desktop approval refused for account"); + expect(error.message).not.toBe("1Password CLI vault listing failed"); + // oxlint-enable executor/no-unknown-error-message + }), + ); + + it.effect("reports a clear SDK load error when the compiled namespace is empty", () => + Effect.gen(function* () { + opMocks.opCliExec.mockResolvedValue(cliFailure("spawn op ENOENT")); + sdkMocks.exports.createClient = undefined; + sdkMocks.exports.DesktopAuth = undefined; + + const error = yield* makeOnePasswordService( + { kind: "service-account", token: "ops_test_token" }, + { timeoutMs: 1_000 }, + ).pipe( + Effect.flatMap((service) => service.listVaults()), + Effect.flip, + ); + + expect(error).toBeInstanceOf(OnePasswordError); + expect(error.operation).toBe("sdk module load"); + // oxlint-disable executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed message; asserting its contents + expect(error.message).toContain("did not expose createClient and DesktopAuth"); + expect(error.message).toContain("/opt/homebrew/bin"); + // oxlint-enable executor/no-unknown-error-message }), ); }); diff --git a/packages/plugins/onepassword/src/sdk/service.ts b/packages/plugins/onepassword/src/sdk/service.ts index bf725c1b6..ca078f8f8 100644 --- a/packages/plugins/onepassword/src/sdk/service.ts +++ b/packages/plugins/onepassword/src/sdk/service.ts @@ -1,7 +1,7 @@ -import { Context, Duration, Effect, Semaphore } from "effect"; -import * as op from "@1password/op-js"; +import { Context, Duration, Effect, Schema, Semaphore } from "effect"; import { OnePasswordError } from "./errors"; +import { opCliExec } from "./op-cli"; import type { OnePasswordSdkModule } from "./onepassword-sdk"; // --------------------------------------------------------------------------- @@ -175,63 +175,106 @@ export const makeNativeSdkService = ( }).pipe(Effect.withSpan("onepassword.sdk.make_service")); // --------------------------------------------------------------------------- -// CLI backend — uses @1password/op-js (shells out to `op` CLI) +// CLI backend — spawns the `op` CLI asynchronously (see op-cli.ts for why the +// spawn must never be synchronous). Auth travels per spawn: the service +// account token goes through the child's environment and the desktop account +// through `--account`, so there is no process-global credential state. // --------------------------------------------------------------------------- -const cliAuthLock = Semaphore.makeUnsafe(1); +/** The 1Password desktop app shows at most one CLI-authorization prompt at a + * time, so desktop-app spawns are serialized to keep concurrent calls from + * queueing invisible prompts behind each other. Service-account spawns never + * prompt and run unrestricted. */ +const cliPromptLock = Semaphore.makeUnsafe(1); + +const cliEnv = (auth: ResolvedAuth): Record => { + const env: Record = { ...process.env }; + // An inherited token would override `--account` and silently route a + // desktop-app call to whatever service account the parent process carries. + delete env["OP_SERVICE_ACCOUNT_TOKEN"]; + if (auth.kind === "service-account") env["OP_SERVICE_ACCOUNT_TOKEN"] = auth.token; + return env; +}; + +const cliArgs = (auth: ResolvedAuth, base: readonly string[]): readonly string[] => + auth.kind === "desktop-app" ? [...base, `--account=${auth.accountName}`] : base; + +const decodeCliVaultRows = Schema.decodeUnknownEffect( + Schema.fromJsonString(Schema.Array(Schema.Struct({ id: Schema.String, name: Schema.String }))), +); +const decodeCliItemRows = Schema.decodeUnknownEffect( + Schema.fromJsonString(Schema.Array(Schema.Struct({ id: Schema.String, title: Schema.String }))), +); export const makeCliService = ( auth: ResolvedAuth, + timeoutMs: number = DEFAULT_TIMEOUT_MS, ): Effect.Effect => Effect.sync(() => { - const wrapSync = (fn: () => A, operation: string): Effect.Effect => - cliAuthLock - .withPermits(1)( - Effect.try({ - try: () => { - if (auth.kind === "service-account") { - op.setGlobalFlags({}); - op.setServiceAccount(auth.token); - } else { - op.setServiceAccount(""); - op.setGlobalFlags({ account: auth.accountName }); - } - return fn(); - }, - catch: (cause) => - new OnePasswordError({ - operation, - message: messageWithCause(`1Password CLI ${operation} failed`, cause), - }), - }).pipe( - // `op-js` keeps the service-account token in a PROCESS-GLOBAL - // (`cli.serviceAccountToken`, a field on the module's single CLI - // instance) and reads it when spawning `op`. Nothing in the library - // clears it, so a token set to serve one resolve stayed readable for - // the rest of the executor's life — long after the call that needed - // it, and with no reader. The account-name branch above happens to - // blank it, but only if a differently-authenticated call comes next, - // which in a service-account-only deployment never happens. - // - // So clear it as soon as the call is done: on success, on failure and - // on interruption alike. Safe because every write and every read of - // that global happens inside this same semaphore, so the next - // operation re-sets the token before it spawns anything. - Effect.ensuring(Effect.sync(() => op.setServiceAccount(""))), + const run = ( + base: readonly string[], + operation: string, + ): Effect.Effect => { + const spawn = Effect.promise((signal) => + opCliExec({ args: cliArgs(auth, base), env: cliEnv(auth), timeoutMs, signal }), + ).pipe( + Effect.flatMap((result) => { + if (result.ok) return Effect.succeed(result.stdout); + return Effect.fail( + result.timedOut + ? new OnePasswordError({ + operation, + message: makeTimeoutMessage(operation, timeoutMs), + }) + : new OnePasswordError({ + operation, + message: messageWithCause(`1Password CLI ${operation} failed`, result), + }), + ); + }), + ); + return (auth.kind === "desktop-app" ? cliPromptLock.withPermits(1)(spawn) : spawn).pipe( + Effect.withSpan(`onepassword.cli.${operation}`), + ); + }; + + const runJson = ( + base: readonly string[], + operation: string, + decodeRows: (raw: string) => Effect.Effect, + ): Effect.Effect => + run([...base, "--format=json"], operation).pipe( + Effect.flatMap((raw) => + decodeRows(raw).pipe( + Effect.mapError( + (cause) => + new OnePasswordError({ + operation, + message: messageWithCause( + `1Password CLI ${operation} returned unexpected output`, + cause, + ), + }), + ), ), - ) - .pipe(Effect.withSpan(`onepassword.cli.${operation}`)); + ), + ); return OnePasswordServiceTag.of({ - resolveSecret: (uri) => wrapSync(() => op.read.parse(uri), "secret resolution"), + // `op read` appends a trailing newline to the field value; strip it the + // same way op-js's `read.parse` did so stored secrets stay unchanged. + resolveSecret: (uri) => + run(["read", uri], "secret resolution").pipe( + Effect.map((raw) => raw.replace(/[\r\n]+$/, "")), + ), listVaults: () => - wrapSync(() => op.vault.list(), "vault listing").pipe( + runJson(["vault", "list"], "vault listing", decodeCliVaultRows).pipe( Effect.map((vaults) => vaults.map((v) => ({ id: v.id, title: v.name }))), ), listItems: (vaultId) => - wrapSync(() => op.item.list({ vault: vaultId }), "item listing").pipe( + runJson(["item", "list", "--vault", vaultId], "item listing", decodeCliItemRows).pipe( Effect.map((items) => items.map((i) => ({ id: i.id, title: i.title }))), ), }); @@ -270,7 +313,7 @@ export const makeOnePasswordService = ( } return Effect.gen(function* () { - const cliService = yield* makeCliService(auth); + const cliService = yield* makeCliService(auth, timeoutMs); const sdkService = yield* Effect.cached(makeNativeSdkService(auth, timeoutMs)); const withSdkFallback = (