diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 046bf8ab2186..b482a2e9c549 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -72,6 +72,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.serverDisableAcpRegistryProvider]: AuthOrchestrationOperateScope, [WS_METHODS.serverLogoutAcpRegistry]: AuthOrchestrationOperateScope, [WS_METHODS.serverDiscoverSourceControl]: AuthOrchestrationReadScope, + [WS_METHODS.serverListOnePasswordAccounts]: AuthOrchestrationReadScope, [WS_METHODS.serverGetTraceDiagnostics]: AuthOrchestrationReadScope, [WS_METHODS.serverGetProcessDiagnostics]: AuthOrchestrationReadScope, [WS_METHODS.serverGetHostResources]: AuthOrchestrationReadScope, diff --git a/apps/server/src/orchestration-v2/ProviderAdapterDriver.ts b/apps/server/src/orchestration-v2/ProviderAdapterDriver.ts index 765abd21af0f..9af3843cd62e 100644 --- a/apps/server/src/orchestration-v2/ProviderAdapterDriver.ts +++ b/apps/server/src/orchestration-v2/ProviderAdapterDriver.ts @@ -1,12 +1,9 @@ -import { - ProviderDriverKind, - ProviderInstanceId, - type ProviderInstanceEnvironment, -} from "@t3tools/contracts"; +import { ProviderDriverKind, ProviderInstanceId } from "@t3tools/contracts"; import * as Schema from "effect/Schema"; import type * as Effect from "effect/Effect"; import type * as Scope from "effect/Scope"; +import type { ResolvedProviderEnvironment } from "../provider/ProviderInstanceEnvironment.ts"; import type { ProviderAdapterV2Shape } from "./ProviderAdapter.ts"; export class ProviderAdapterDriverCreateError extends Schema.TaggedError()( @@ -27,7 +24,7 @@ export interface ProviderAdapterDriverCreateInput { readonly instanceId: ProviderInstanceId; readonly displayName: string | undefined; readonly accentColor?: string | undefined; - readonly environment: ProviderInstanceEnvironment; + readonly environment: ResolvedProviderEnvironment; readonly enabled: boolean; readonly config: Config; } diff --git a/apps/server/src/orchestration-v2/ProviderAdapterRegistry.test.ts b/apps/server/src/orchestration-v2/ProviderAdapterRegistry.test.ts index c2cfdbebca7b..299564b19023 100644 --- a/apps/server/src/orchestration-v2/ProviderAdapterRegistry.test.ts +++ b/apps/server/src/orchestration-v2/ProviderAdapterRegistry.test.ts @@ -2,6 +2,7 @@ import * as NodeServices from "@effect/platform-node/NodeServices"; import { assert, it } from "@effect/vitest"; import { ProviderDriverKind, + ProviderInstanceEnvironment, ProviderInstanceId, ProviderSessionId, ThreadId, @@ -38,6 +39,13 @@ const driver = ProviderDriverKind.make("codex"); const personalId = ProviderInstanceId.make("codex_personal"); const workId = ProviderInstanceId.make("codex_work"); +const HOME_ACCOUNT = "my.1password.com"; +const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); +const onePasswordVariable = (name: string, reference: string) => ({ + name, + value: { kind: "1password" as const, reference, account: HOME_ACCOUNT }, +}); + const makeAdapter = (instanceId: ProviderInstanceId): ProviderAdapterV2Shape => ({ instanceId, @@ -347,19 +355,20 @@ it.effect("opens v2 sessions with resolved secrets and rebuilds them when a secr Effect.gen(function* () { const variables = []; const unresolved = []; - for (const variable of environment ?? []) { - if (variable.value === apiKeyReference) { - variables.push({ ...variable, value: yield* Ref.get(apiKey) }); - } else if (variable.value.startsWith("op://")) { - unresolved.push(variable.name); + for (const { name, value, sensitive } of environment ?? []) { + if (typeof value !== "string" && value.reference === apiKeyReference) { + variables.push({ name, value: yield* Ref.get(apiKey), sensitive: true }); + } else if (typeof value !== "string") { + unresolved.push(name); } else { - variables.push(variable); + variables.push({ name, value, sensitive }); } } return { variables, unresolved }; }), prime: () => Effect.void, invalidate: Effect.void, + listOnePasswordAccounts: Effect.succeed([]), }; const secretDriver: ProviderDriver> = { driverKind: driver, @@ -408,11 +417,11 @@ it.effect("opens v2 sessions with resolved secrets and rebuilds them when a secr configMap: { [secretInstanceId]: { driver, - environment: [ - { name: "OPENAI_API_KEY", value: apiKeyReference, sensitive: true }, - { name: "ANTHROPIC_API_KEY", value: "op://Vault/Locked/api-key", sensitive: true }, + environment: decodeEnvironment([ + onePasswordVariable("OPENAI_API_KEY", apiKeyReference), + onePasswordVariable("ANTHROPIC_API_KEY", "op://Vault/Locked/api-key"), { name: "CODEX_PROFILE", value: "work", sensitive: false }, - ], + ]), config: {}, }, }, diff --git a/apps/server/src/orchestration-v2/ProviderAdapterRegistry.ts b/apps/server/src/orchestration-v2/ProviderAdapterRegistry.ts index 5279100e6206..79649f1e3e4d 100644 --- a/apps/server/src/orchestration-v2/ProviderAdapterRegistry.ts +++ b/apps/server/src/orchestration-v2/ProviderAdapterRegistry.ts @@ -13,6 +13,7 @@ import * as Layer from "effect/Layer"; import * as Schema from "effect/Schema"; import * as Scope from "effect/Scope"; +import { literalProviderInstanceEnvironment } from "../provider/ProviderInstanceEnvironment.ts"; import * as ProviderInstanceRegistry from "../provider/Services/ProviderInstanceRegistry.ts"; import { ProviderAdapterDriverCreateError, @@ -271,7 +272,7 @@ const createAdapterEntryFromConfigEntry = Effect.fn( instanceId: input.instanceId, displayName: input.entry.displayName, accentColor: input.entry.accentColor, - environment: input.entry.environment ?? [], + environment: literalProviderInstanceEnvironment(input.entry.environment), enabled: input.entry.enabled ?? decodedConfigEnabled(typedConfig) ?? true, config: typedConfig, }) diff --git a/apps/server/src/project/AgentSessionScanner.ts b/apps/server/src/project/AgentSessionScanner.ts index bc30691784db..e494be5fc8a4 100644 --- a/apps/server/src/project/AgentSessionScanner.ts +++ b/apps/server/src/project/AgentSessionScanner.ts @@ -50,6 +50,7 @@ import { normalizeProjectPathForComparison } from "@t3tools/shared/path"; import * as ServerConfig from "../config.ts"; import * as ProjectStore from "../orchestration-v2/ProjectStore.ts"; import { resolveCodexHomeLayout } from "../provider/Drivers/CodexHomeLayout.ts"; +import { literalProviderInstanceEnvironment } from "../provider/ProviderInstanceEnvironment.ts"; import { expandHomePath } from "../pathExpansion.ts"; import * as ServerSettings from "../serverSettings.ts"; import { @@ -1127,8 +1128,9 @@ export const make = Effect.gen(function* () { for (const { instanceId, config: instance } of instances) { const homeVariable = source === "claudeAgent" ? "CLAUDE_CONFIG_DIR" : "CODEX_HOME"; const environmentHome = - instance.environment?.findLast((variable) => variable.name === homeVariable)?.value ?? - hostEnvironment[homeVariable]; + literalProviderInstanceEnvironment(instance.environment).findLast( + (variable) => variable.name === homeVariable, + )?.value ?? hostEnvironment[homeVariable]; let homePath: string; if (source === "claudeAgent") { diff --git a/apps/server/src/provider/Drivers/ClaudeCredential.ts b/apps/server/src/provider/Drivers/ClaudeCredential.ts index 295add87aff5..8e82895f7a75 100644 --- a/apps/server/src/provider/Drivers/ClaudeCredential.ts +++ b/apps/server/src/provider/Drivers/ClaudeCredential.ts @@ -49,8 +49,8 @@ const VERIFY_TIMEOUT = Duration.seconds(10); * The OAuth token a Claude instance was configured with, if any. * * Reads the same variable the CLI itself reads, so an instance whose - * environment carries an `op://` reference is checked with whatever that - * reference resolved to. + * environment reads the token from a secret store is checked with whatever + * that secret resolved to. */ export function claudeOAuthTokenFromEnvironment( environment: NodeJS.ProcessEnv, diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts index 85fa7df25b6d..5d90e83a0fff 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -37,6 +37,7 @@ import { ProviderDriverKind, type ProviderInstanceConfig, type ProviderInstanceConfigMap, + ProviderInstanceEnvironment, ProviderInstanceId, } from "@t3tools/contracts"; import { HostProcessPlatform, isHostWindows } from "@t3tools/shared/hostProcess"; @@ -48,6 +49,7 @@ import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; import * as Ref from "effect/Ref"; +import * as Schema from "effect/Schema"; import * as Stream from "effect/Stream"; import { HttpClient, HttpClientResponse } from "effect/unstable/http"; @@ -871,6 +873,8 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { ), ); + const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); + const codexDriverKind = ProviderDriverKind.make("codex"); const firstId = ProviderInstanceId.make("codex_first"); const secondId = ProviderInstanceId.make("codex_second"); @@ -878,7 +882,16 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { driver: codexDriverKind, displayName: "Codex (first)", enabled: false, - environment: [{ name: "OP_TOKEN", value: "op://Vault/Item/token", sensitive: true }], + environment: decodeEnvironment([ + { + name: "OP_TOKEN", + value: { + kind: "1password", + reference: "op://Vault/Item/token", + account: "my.1password.com", + }, + }, + ]), config: makeCodexConfig({ homePath: "/home/julius/.codex_first" }), }; const secondEntry: ProviderInstanceConfig = { diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 0d860e383068..594fe83e5239 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -240,7 +240,7 @@ const buildEntry = (input: { instanceId, displayName: entry.displayName, accentColor: entry.accentColor, - environment: resolvedEnvironment.variables ?? [], + environment: resolvedEnvironment.variables, enabled: resolveEntryEnabled(entry, typedConfig), config: typedConfig, }) diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index d01966f489bb..4a392c33f2c0 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -24,6 +24,7 @@ import { CodexSettings, DEFAULT_SERVER_SETTINGS, ProviderDriverKind, + ProviderInstanceEnvironment, ProviderInstanceId, ServerSettings, type ServerProvider, @@ -65,12 +66,20 @@ import { } from "../providerStatusCache.ts"; import { COMPACT_SLASH_COMMAND } from "../providerSnapshot.ts"; import type { ProviderInstance } from "../ProviderDriver.ts"; +import { literalProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; import * as ProviderInstanceRegistry from "../Services/ProviderInstanceRegistry.ts"; import * as ProviderRegistry from "../Services/ProviderRegistry.ts"; import { makeManualOnlyProviderMaintenanceCapabilities } from "../providerMaintenance.ts"; const decodeServerSettings = Schema.decodeSync(ServerSettings); const encodeServerSettings = Schema.encodeSync(ServerSettings); const encodedDefaultServerSettings = encodeServerSettings(DEFAULT_SERVER_SETTINGS); +const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); +const HOME_ACCOUNT = "my.1password.com"; +const onePasswordVariable = (name: string, reference: string, sensitive = true) => ({ + name, + value: { kind: "1password" as const, reference, account: HOME_ACCOUNT }, + sensitive, +}); const defaultClaudeSettings: ClaudeSettings = Schema.decodeSync(ClaudeSettings)({}); const defaultCodexSettings: CodexSettings = Schema.decodeSync(CodexSettings)({}); @@ -1609,9 +1618,14 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test const invalidations = yield* Ref.make(0); const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { - resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + resolve: (environment) => + Effect.succeed({ + variables: literalProviderInstanceEnvironment(environment), + unresolved: [], + }), prime: () => Effect.void, invalidate: Ref.update(invalidations, (count) => count + 1), + listOnePasswordAccounts: Effect.succeed([]), }); const instanceRegistryLayer = Layer.succeed( ProviderInstanceRegistry.ProviderInstanceRegistry, @@ -1623,9 +1637,9 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test rebuildInstanceWhen: (instanceId, shouldRebuild) => shouldRebuild({ driver: codexDriver, - environment: [ - { name: "CODEX_TOKEN", value: "op://Vault/Item/token", sensitive: true }, - ], + environment: decodeEnvironment([ + onePasswordVariable("CODEX_TOKEN", "op://Vault/Item/token"), + ]), }) ? Ref.update(rebuiltIds, (previous) => [...previous, instanceId]).pipe( Effect.as(true), @@ -1687,9 +1701,14 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { - resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + resolve: (environment) => + Effect.succeed({ + variables: literalProviderInstanceEnvironment(environment), + unresolved: [], + }), prime: () => Effect.void, invalidate: Effect.void, + listOnePasswordAccounts: Effect.succeed([]), }); const instanceRegistryLayer = Layer.succeed( ProviderInstanceRegistry.ProviderInstanceRegistry, @@ -1700,9 +1719,9 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test rebuildInstanceWhen: (instanceId, shouldRebuild) => shouldRebuild({ driver: codexDriver, - environment: [ - { name: "CODEX_TOKEN", value: "op://Vault/Item/token", sensitive: true }, - ], + environment: decodeEnvironment([ + onePasswordVariable("CODEX_TOKEN", "op://Vault/Item/token"), + ]), }) ? Ref.update(rebuiltIds, (previous) => [...previous, instanceId]).pipe( Effect.as(true), @@ -1764,14 +1783,21 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test const primed = yield* Ref.make>>([]); const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { - resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + resolve: (environment) => + Effect.succeed({ + variables: literalProviderInstanceEnvironment(environment), + unresolved: [], + }), prime: (references) => - Ref.update(primed, (previous) => [...previous, references]).pipe(Effect.asVoid), + Ref.update(primed, (previous) => [ + ...previous, + references.map((secret) => secret.reference), + ]).pipe(Effect.asVoid), invalidate: Effect.void, + listOnePasswordAccounts: Effect.succeed([]), }); - const environmentFor = (reference: string) => [ - { name: "TOKEN", value: reference, sensitive: true }, - ]; + const environmentFor = (reference: string) => + decodeEnvironment([onePasswordVariable("TOKEN", reference)]); const instanceRegistryLayer = Layer.succeed( ProviderInstanceRegistry.ProviderInstanceRegistry, { @@ -2926,14 +2952,30 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test displayName: "Claude Secret", enabled: false, environment: [ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: claudeReference, sensitive: true }, + { + name: "CLAUDE_CODE_OAUTH_TOKEN", + value: { + kind: "1password", + reference: claudeReference, + account: HOME_ACCOUNT, + }, + }, ], }, codex_secret: { driver: "codex", displayName: "Codex Secret", enabled: false, - environment: [{ name: "TOKEN", value: codexReference, sensitive: true }], + environment: [ + { + name: "TOKEN", + value: { + kind: "1password", + reference: codexReference, + account: HOME_ACCOUNT, + }, + }, + ], }, } as unknown as ContractServerSettings["providerInstances"], }), @@ -2944,14 +2986,18 @@ it.layer(Layer.mergeAll(TestNodeServices, ServerSettingsModule.layerTest(), Test const recordingSecretResolverLayer = Layer.succeed(ProviderSecretResolver, { resolve: (environment) => Ref.update(calls, (previous) => [...previous, "resolve"]).pipe( - Effect.as({ variables: environment, unresolved: [] }), + Effect.as({ + variables: literalProviderInstanceEnvironment(environment), + unresolved: [], + }), ), prime: (references) => Ref.update(calls, (previous) => [ ...previous, - `prime:${Array.from(references).join(",")}`, + `prime:${references.map((secret) => secret.reference).join(",")}`, ]).pipe(Effect.asVoid), invalidate: Effect.void, + listOnePasswordAccounts: Effect.succeed([]), }); const scope = yield* Scope.make(); diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts index a7d8cba5ff49..4bdab502fe54 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -1,6 +1,6 @@ // @effect-diagnostics nodeBuiltinImport:off - asserts against the fake op CLI script this test writes to disk. import { describe, it, assert } from "@effect/vitest"; -import { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import { OnePasswordAccount, ProviderInstanceEnvironment } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Schema from "effect/Schema"; @@ -10,6 +10,7 @@ import { ChildProcessSpawner } from "effect/unstable/process"; import * as NodeFileSystem from "@effect/platform-node/NodeFileSystem"; import * as NodeFS from "node:fs"; +import { OnePasswordSecretReference } from "../ProviderSecretReference.ts"; import { ProviderSecretResolverLive } from "./ProviderSecretResolverLive.ts"; import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; @@ -18,6 +19,23 @@ const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); const TOKEN_REFERENCE = "op://Private/claude-code/credential"; +const HOME_ACCOUNT = "my.1password.com"; +const WORK_ACCOUNT = "acme.1password.com"; + +const onePasswordVariable = (name: string, reference: string, account: string) => ({ + name, + value: { kind: "1password" as const, reference, account }, +}); + +const onePassword = (reference: string, account: string) => { + const [variable] = decodeEnvironment([onePasswordVariable("TOKEN", reference, account)]); + const source = variable?.value; + if (source === undefined || typeof source === "string") { + throw new Error("expected a 1Password source"); + } + return new OnePasswordSecretReference({ reference: source.reference, account: source.account }); +}; + /** * Spawner that answers every `op read` with `result` and records the argv it * was handed, so tests can assert both the substituted value and how many @@ -61,7 +79,16 @@ describe("ProviderSecretResolverLive", () => { const resolved = yield* resolver.resolve(environment); - assert.deepStrictEqual(resolved, { variables: environment, unresolved: [] }); + assert.deepStrictEqual(resolved, { + variables: [ + { + name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", + value: "/home/u/.claude/work", + sensitive: false, + }, + ], + unresolved: [], + }); assert.strictEqual(spawner.invocations.length, 0); }).pipe( Effect.provide( @@ -80,7 +107,7 @@ describe("ProviderSecretResolverLive", () => { const resolved = yield* resolver.resolve( decodeEnvironment([ { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]), ); @@ -91,7 +118,9 @@ describe("ProviderSecretResolverLive", () => { ["CLAUDE_CODE_OAUTH_TOKEN", "sk-live-token"], ], ); - assert.deepStrictEqual(spawner.invocations, [["read", "--no-newline", TOKEN_REFERENCE]]); + assert.deepStrictEqual(spawner.invocations, [ + ["--cache=false", "read", "--account", HOME_ACCOUNT, "--no-newline", TOKEN_REFERENCE], + ]); }).pipe( Effect.provide( ProviderSecretResolverLive.pipe( @@ -106,7 +135,7 @@ describe("ProviderSecretResolverLive", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; const environment = decodeEnvironment([ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]); // Every thread start and every instance rebuild resolves again; none of @@ -142,7 +171,7 @@ describe("ProviderSecretResolverLive", () => { const resolved = yield* resolver.resolve( decodeEnvironment([ { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]), ); @@ -168,7 +197,7 @@ describe("ProviderSecretResolverLive", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; const environment = decodeEnvironment([ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]); yield* resolver.resolve(environment); @@ -264,11 +293,17 @@ describe("ProviderSecretResolverLive.prime", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; - yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + yield* resolver.prime([ + onePassword(TOKEN_REFERENCE, HOME_ACCOUNT), + onePassword(SECOND_REFERENCE, HOME_ACCOUNT), + ]); assert.strictEqual(spawner.invocations.length, 1); - assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []).slice(0, 2), [ + assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []).slice(0, 5), [ + "--cache=false", "inject", + "--account", + HOME_ACCOUNT, "-i", ]); @@ -276,11 +311,11 @@ describe("ProviderSecretResolverLive.prime", () => { // one authorization the batch already paid for. const claude = yield* resolver.resolve( decodeEnvironment([ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]), ); const codex = yield* resolver.resolve( - decodeEnvironment([{ name: "CODEX_TOKEN", value: SECOND_REFERENCE, sensitive: true }]), + decodeEnvironment([onePasswordVariable("CODEX_TOKEN", SECOND_REFERENCE, HOME_ACCOUNT)]), ); assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); @@ -307,21 +342,24 @@ describe("ProviderSecretResolverLive.prime", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; - yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + yield* resolver.prime([ + onePassword(TOKEN_REFERENCE, HOME_ACCOUNT), + onePassword(SECOND_REFERENCE, HOME_ACCOUNT), + ]); // The batch is still attempted; it is the recovery that is per reference. - assert.strictEqual(spawner.invocations[0]?.[0], "inject"); + assert.strictEqual(spawner.invocations[0]?.[1], "inject"); // A batch that cannot be trusted leaves the cache cold rather than // caching a failure for every reference in it, so the good reference // still resolves and only the bad one is reported unresolved. const claude = yield* resolver.resolve( decodeEnvironment([ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), ]), ); const codex = yield* resolver.resolve( - decodeEnvironment([{ name: "CODEX_TOKEN", value: SECOND_REFERENCE, sensitive: true }]), + decodeEnvironment([onePasswordVariable("CODEX_TOKEN", SECOND_REFERENCE, HOME_ACCOUNT)]), ); assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); @@ -352,14 +390,23 @@ describe("ProviderSecretResolverLive.prime", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; - yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + yield* resolver.prime([ + onePassword(TOKEN_REFERENCE, HOME_ACCOUNT), + onePassword(SECOND_REFERENCE, HOME_ACCOUNT), + ]); // `op` only reads piped input from a named pipe, and Node hands a child // a socket pair, so a template offered on stdin is never seen and the // batch fails every time. The `-i` path is the delivery that works. const args = Array.from(spawner.invocations[0] ?? []); - assert.deepStrictEqual(args.slice(0, 2), ["inject", "-i"]); - assert.isTrue((args[2] ?? "").length > 0); + assert.deepStrictEqual(args.slice(0, 5), [ + "--cache=false", + "inject", + "--account", + HOME_ACCOUNT, + "-i", + ]); + assert.isTrue((args[5] ?? "").length > 0); assert.deepStrictEqual(spawner.stdinUses, [false]); // The file `op` was pointed at held both references and nothing else, @@ -391,7 +438,10 @@ describe("ProviderSecretResolverLive.prime", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; - yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + yield* resolver.prime([ + onePassword(TOKEN_REFERENCE, HOME_ACCOUNT), + onePassword(SECOND_REFERENCE, HOME_ACCOUNT), + ]); assert.isTrue(templatePath.length > 0); assert.isFalse(NodeFS.existsSync(templatePath)); @@ -409,7 +459,7 @@ describe("ProviderSecretResolverLive.prime", () => { return Effect.gen(function* () { const resolver = yield* ProviderSecretResolver; - yield* resolver.prime([TOKEN_REFERENCE]); + yield* resolver.prime([onePassword(TOKEN_REFERENCE, HOME_ACCOUNT)]); // One reference is one prompt either way, and `op read` names the // reference it could not resolve. @@ -423,3 +473,159 @@ describe("ProviderSecretResolverLive.prime", () => { ); }); }); + +describe("ProviderSecretResolverLive with 1Password accounts", () => { + it.effect("reads a 1Password source from the account it names", () => { + const spawner = recordingOpSpawner({ stdout: "sk-work-token", stderr: "", code: 0 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + const resolved = yield* resolver.resolve( + decodeEnvironment([onePasswordVariable("CODEX_TOKEN", TOKEN_REFERENCE, WORK_ACCOUNT)]), + ); + + assert.deepStrictEqual(resolved, { + variables: [{ name: "CODEX_TOKEN", value: "sk-work-token", sensitive: true }], + unresolved: [], + }); + assert.deepStrictEqual(spawner.invocations, [ + ["--cache=false", "read", "--account", WORK_ACCOUNT, "--no-newline", TOKEN_REFERENCE], + ]); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); + }); + + it.effect("keeps the same reference in two accounts apart", () => { + const spawner = scriptedOpSpawner((args) => ({ + stdout: args.includes(WORK_ACCOUNT) ? "sk-work-token" : "sk-home-token", + stderr: "", + code: 0, + })); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + const resolved = yield* resolver.resolve( + decodeEnvironment([ + onePasswordVariable("HOME_TOKEN", TOKEN_REFERENCE, HOME_ACCOUNT), + onePasswordVariable("WORK_TOKEN", TOKEN_REFERENCE, WORK_ACCOUNT), + ]), + ); + + assert.deepStrictEqual(resolved.variables, [ + { name: "HOME_TOKEN", value: "sk-home-token", sensitive: true }, + { name: "WORK_TOKEN", value: "sk-work-token", sensitive: true }, + ]); + assert.strictEqual(spawner.invocations.length, 2); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); + }); + + it.effect("primes one batch per account", () => { + const spawner = scriptedOpSpawner((args, template) => { + if (!args.includes("inject")) { + return { stdout: "should-not-be-read-one-at-a-time", stderr: "", code: 0 }; + } + const prefix = args.includes(WORK_ACCOUNT) ? "work" : "home"; + return { + stdout: [`${prefix}-claude`, `${prefix}-codex`].join(separatorOf(template)), + stderr: "", + code: 0, + }; + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([ + onePassword(TOKEN_REFERENCE, HOME_ACCOUNT), + onePassword(TOKEN_REFERENCE, WORK_ACCOUNT), + onePassword(SECOND_REFERENCE, HOME_ACCOUNT), + onePassword(SECOND_REFERENCE, WORK_ACCOUNT), + ]); + + assert.deepStrictEqual( + spawner.invocations.map((args) => Array.from(args).slice(0, 5)), + [ + ["--cache=false", "inject", "--account", HOME_ACCOUNT, "-i"], + ["--cache=false", "inject", "--account", WORK_ACCOUNT, "-i"], + ], + ); + + const resolved = yield* resolver.resolve( + decodeEnvironment([ + onePasswordVariable("HOME_CODEX", SECOND_REFERENCE, HOME_ACCOUNT), + onePasswordVariable("WORK_CLAUDE", TOKEN_REFERENCE, WORK_ACCOUNT), + ]), + ); + assert.deepStrictEqual(resolved.variables, [ + { name: "HOME_CODEX", value: "home-codex", sensitive: true }, + { name: "WORK_CLAUDE", value: "work-claude", sensitive: true }, + ]); + assert.strictEqual(spawner.invocations.length, 2); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); + }); +}); + +describe("ProviderSecretResolverLive.listOnePasswordAccounts", () => { + const provideResolver = (spawner: ReturnType) => + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ); + + it.effect("offers each signed-in account by the address `op --account` accepts", () => { + const spawner = recordingOpSpawner({ + stdout: JSON.stringify([ + { url: HOME_ACCOUNT, email: "me@example.com", user_uuid: "U1", account_uuid: "A1" }, + { url: WORK_ACCOUNT, email: "me@acme.example", user_uuid: "U2", account_uuid: "A2" }, + ]), + stderr: "", + code: 0, + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + const accounts = yield* resolver.listOnePasswordAccounts; + + assert.deepStrictEqual(accounts, [ + { account: OnePasswordAccount.make(HOME_ACCOUNT), email: "me@example.com" }, + { account: OnePasswordAccount.make(WORK_ACCOUNT), email: "me@acme.example" }, + ]); + assert.deepStrictEqual(spawner.invocations, [ + ["--cache=false", "account", "list", "--format", "json"], + ]); + }).pipe(provideResolver(spawner)); + }); + + it.effect("offers no accounts when `op` fails", () => { + const spawner = recordingOpSpawner({ stdout: "", stderr: "not configured", code: 1 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + assert.deepStrictEqual(yield* resolver.listOnePasswordAccounts, []); + }).pipe(provideResolver(spawner)); + }); + + it.effect("offers no accounts when `op` prints something unexpected", () => { + const spawner = recordingOpSpawner({ stdout: "not json", stderr: "", code: 0 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + assert.deepStrictEqual(yield* resolver.listOnePasswordAccounts, []); + }).pipe(provideResolver(spawner)); + }); +}); diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts index a5398bc9c51c..b1fd3ee9e25b 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -2,16 +2,18 @@ * ProviderSecretResolverLive: 1Password-backed implementation of * `ProviderSecretResolver`. * - * Resolution is `op read `, which is the same command the user - * would run by hand and inherits their existing `op` session, so there is no - * second place to configure credentials. Reads run one at a time: two + * Resolution is `op read --account `, which is the same + * command the user would run by hand and inherits their existing `op` session, + * so there is no second place to configure credentials. The account is what + * keeps a user signed into several 1Password accounts from reading against + * whichever one `op` picks by default. Reads run one at a time: two * concurrent reads against a locked vault stack up two biometric prompts. * * The prompt is charged per `op` invocation rather than per secret, so reading * one reference at a time makes the whole fleet cost one authorization each. * `prime` exists for that: `op inject` substitutes any number of references in * a single process, so the caller that is about to build every instance pays - * one prompt for all of them. + * one prompt per account for all of them. * * Failures are cached alongside successes. If the vault is locked when the * first thread starts, every later thread in that session would otherwise @@ -21,21 +23,24 @@ * * @module provider/Layers/ProviderSecretResolverLive */ -import type { - ProviderInstanceEnvironment, - ProviderInstanceEnvironmentVariable, -} from "@t3tools/contracts"; +import { OnePasswordAccount, OnePasswordAccountSummary } from "@t3tools/contracts"; import * as Cache from "effect/Cache"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as FileSystem from "effect/FileSystem"; import * as Option from "effect/Option"; +import * as Schema from "effect/Schema"; import * as NodeCrypto from "node:crypto"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; -import { hasProviderSecretReference, providerSecretReference } from "../ProviderSecretReference.ts"; +import type { ResolvedProviderEnvironmentVariable } from "../ProviderInstanceEnvironment.ts"; +import { + providerSecretReference, + type OnePasswordSecretReference, + type ProviderSecretReference, +} from "../ProviderSecretReference.ts"; import { spawnAndCollect } from "../providerSnapshot.ts"; import { ProviderSecretResolver, @@ -44,6 +49,15 @@ import { const ONE_PASSWORD_BINARY = "op"; +/** + * Every `op` call skips its cache. The cache lives in an `op daemon` that `op` + * spawns on its own, which hangs on macOS permission prompts nobody can see + * from a background server and leaks zombie processes on Linux. Reads are + * already held in memory here, so the daemon only adds risk. + */ +const resolveOnePasswordCommand = (args: ReadonlyArray) => + resolveSpawnCommand(ONE_PASSWORD_BINARY, ["--cache=false", ...args]); + /** * Bound on a single `op read`. Long enough for a user to reach for the * fingerprint reader, short enough that a vault that will never answer does @@ -59,7 +73,7 @@ const SECRET_READ_TIMEOUT = Duration.seconds(45); const SECRET_CACHE_CAPACITY = 64; /** - * Read many references in one `op inject`. + * Read many references from one account in one `op inject`. * * `op inject` substitutes references inside a template, so the template is the * references themselves joined by a separator, and the output is the secrets @@ -81,6 +95,7 @@ const SECRET_CACHE_CAPACITY = 64; * text; the secrets themselves come back on stdout and never touch disk. */ const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( + account: OnePasswordAccount, references: ReadonlyArray, ) { const fileSystem = yield* FileSystem.FileSystem; @@ -88,8 +103,10 @@ const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( const template = references.map((reference) => `{{ ${reference} }}`).join(separator); const templatePath = yield* fileSystem.makeTempFileScoped({ prefix: "t3code-op-inject-" }); yield* fileSystem.writeFileString(templatePath, template); - const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, [ + const spawnCommand = yield* resolveOnePasswordCommand([ "inject", + "--account", + account, "-i", templatePath, ]); @@ -103,6 +120,7 @@ const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( // `op` names the reference it could not resolve on stderr and never echoes // a secret, so this is safe to log verbatim. yield* Effect.logWarning("Could not batch-read provider secrets from 1Password", { + account, references: references.length, exitCode: result.code, detail: result.stderr.trim(), @@ -123,9 +141,14 @@ const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( }); }); -const readSecret = Effect.fn("readSecret")(function* (reference: string) { - const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, [ +const readOnePasswordSecret = Effect.fn("readOnePasswordSecret")(function* ({ + reference, + account, +}: OnePasswordSecretReference) { + const spawnCommand = yield* resolveOnePasswordCommand([ "read", + "--account", + account, "--no-newline", reference, ]); @@ -138,6 +161,7 @@ const readSecret = Effect.fn("readSecret")(function* (reference: string) { // and never echoes the secret itself, so this is safe to log verbatim. yield* Effect.logWarning("Could not read provider secret from 1Password", { reference, + account, exitCode: result.code, detail: result.stderr.trim(), }); @@ -147,6 +171,34 @@ const readSecret = Effect.fn("readSecret")(function* (reference: string) { return secret.length > 0 ? secret : undefined; }); +const OnePasswordAccountListJson = Schema.fromJsonString( + Schema.Array(Schema.Struct({ url: OnePasswordAccount, email: Schema.String })), +); + +const readOnePasswordAccounts = Effect.fn("readOnePasswordAccounts")(function* () { + const spawnCommand = yield* resolveOnePasswordCommand(["account", "list", "--format", "json"]); + const result = yield* spawnAndCollect( + ONE_PASSWORD_BINARY, + ChildProcess.make(spawnCommand.command, spawnCommand.args, { shell: spawnCommand.shell }), + ); + if (result.code !== 0) { + yield* Effect.logWarning("Could not list 1Password accounts", { + exitCode: result.code, + detail: result.stderr.trim(), + }); + return []; + } + const accounts = yield* Schema.decodeUnknownEffect(OnePasswordAccountListJson)(result.stdout); + return accounts.map(({ url, email }): OnePasswordAccountSummary => ({ account: url, email })); +}); + +const readSecret = (secret: ProviderSecretReference) => { + switch (secret._tag) { + case "1password": + return readOnePasswordSecret(secret); + } +}; + export const ProviderSecretResolverLive = Layer.effect( ProviderSecretResolver, Effect.gen(function* () { @@ -162,13 +214,14 @@ export const ProviderSecretResolverLive = Layer.effect( // No time to live: a resolved secret is held until the user asks for a // provider refresh. Expiring on a timer would reintroduce the surprise // biometric prompt mid-session that the cache exists to remove. - lookup: (reference: string) => + lookup: (reference: ProviderSecretReference) => readSecret(reference).pipe( Effect.timeoutOption(SECRET_READ_TIMEOUT), Effect.map(Option.getOrUndefined), Effect.catch((error) => Effect.logWarning("Could not run 1Password to read a provider secret", { - reference, + reference: reference.reference, + account: reference.account, detail: String(error), }).pipe(Effect.as(undefined)), ), @@ -177,63 +230,105 @@ export const ProviderSecretResolverLive = Layer.effect( const resolve: ProviderSecretResolverShape["resolve"] = (environment) => Effect.gen(function* () { - if (!hasProviderSecretReference(environment)) { - return { variables: environment, unresolved: [] }; - } - const resolved: Array = []; + const resolved: Array = []; const unresolved: Array = []; - for (const variable of environment ?? []) { - const reference = providerSecretReference(variable.value); + for (const { name, value, sensitive } of environment ?? []) { + const reference = providerSecretReference(value); if (reference === undefined) { - resolved.push(variable); + if (typeof value === "string") { + resolved.push({ name, value, sensitive }); + } continue; } const secret = yield* Cache.get(cache, reference); if (secret === undefined) { - unresolved.push(variable.name); + unresolved.push(name); continue; } - resolved.push({ ...variable, value: secret }); + resolved.push({ name, value: secret, sensitive: true }); } - return { variables: resolved as ProviderInstanceEnvironment, unresolved }; + return { variables: resolved, unresolved }; }); + const primeOnePassword = Effect.fn("primeOnePassword")(function* ( + account: OnePasswordAccount, + wanted: ReadonlyArray, + ) { + // One reference costs one prompt whichever command reads it, so there + // is nothing to save and `op read` gives the better error. + if (wanted.length < 2) { + return; + } + const values = yield* readSecretsTogether( + account, + wanted.map((secret) => secret.reference), + ).pipe( + Effect.scoped, + Effect.timeoutOption(SECRET_READ_TIMEOUT), + Effect.map(Option.getOrUndefined), + Effect.catch((error) => + Effect.logWarning("Could not run 1Password to batch-read provider secrets", { + account, + references: wanted.length, + detail: String(error), + }).pipe(Effect.as(undefined)), + ), + ); + if (values === undefined) { + return; + } + yield* Effect.forEach(wanted, (secret, index) => Cache.set(cache, secret, values[index]), { + discard: true, + }); + }); + const prime: ProviderSecretResolverShape["prime"] = (references) => Effect.gen(function* () { - const wanted: Array = []; - for (const reference of new Set(references)) { - if (!(yield* Cache.has(cache, reference))) { - wanted.push(reference); + // `op inject` reads every reference in its template from one account, + // so a batch is one call per account. + const onePasswordByAccount = new Map< + OnePasswordAccount, + Array + >(); + for (const reference of references) { + if (yield* Cache.has(cache, reference)) { + continue; + } + switch (reference._tag) { + case "1password": { + const group = onePasswordByAccount.get(reference.account) ?? []; + if (!group.some((seen) => seen.reference === reference.reference)) { + group.push(reference); + } + onePasswordByAccount.set(reference.account, group); + break; + } } - } - // One reference costs one prompt whichever command reads it, so there - // is nothing to save and `op read` gives the better error. - if (wanted.length < 2) { - return; - } - const values = yield* readSecretsTogether(wanted).pipe( - Effect.scoped, - Effect.timeoutOption(SECRET_READ_TIMEOUT), - Effect.map(Option.getOrUndefined), - Effect.catch((error) => - Effect.logWarning("Could not run 1Password to batch-read provider secrets", { - references: wanted.length, - detail: String(error), - }).pipe(Effect.as(undefined)), - ), - ); - if (values === undefined) { - return; } yield* Effect.forEach( - wanted, - (reference, index) => Cache.set(cache, reference, values[index]), - { - discard: true, - }, + onePasswordByAccount, + ([account, wanted]) => primeOnePassword(account, wanted), + { discard: true }, ); }).pipe(Effect.provideContext(primeContext)); - return { resolve, prime, invalidate: Cache.invalidateAll(cache) }; + const listOnePasswordAccounts: ProviderSecretResolverShape["listOnePasswordAccounts"] = + readOnePasswordAccounts().pipe( + Effect.timeoutOption(SECRET_READ_TIMEOUT), + Effect.map(Option.getOrElse(() => [])), + Effect.catch((error) => + Effect.logWarning("Could not run 1Password to list accounts", { + detail: String(error), + }).pipe(Effect.as([])), + ), + Effect.provideContext(primeContext), + ); + + return { + resolve, + prime, + invalidate: Cache.invalidateAll(cache), + listOnePasswordAccounts, + }; }), ); diff --git a/apps/server/src/provider/ProviderDriver.ts b/apps/server/src/provider/ProviderDriver.ts index 4f92df965c8a..432efd5d31f8 100644 --- a/apps/server/src/provider/ProviderDriver.ts +++ b/apps/server/src/provider/ProviderDriver.ts @@ -28,7 +28,6 @@ import type { AcpRegistryOperationError, AcpRegistrySetProviderInput, ProviderDriverKind, - ProviderInstanceEnvironment, ProviderInstanceId, ServerProvider, } from "@t3tools/contracts"; @@ -37,6 +36,7 @@ import type * as Schema from "effect/Schema"; import type * as Scope from "effect/Scope"; import type { TextGeneration } from "../textGeneration/TextGeneration.ts"; +import type { ResolvedProviderEnvironment } from "./ProviderInstanceEnvironment.ts"; import type { ProviderAdapterV2Shape } from "../orchestration-v2/ProviderAdapter.ts"; import type { ProviderDriverError } from "./Errors.ts"; import type { ProviderAuthController } from "./Services/ProviderAuthService.ts"; @@ -142,7 +142,7 @@ export interface ProviderDriverCreateInput { readonly instanceId: ProviderInstanceId; readonly displayName: string | undefined; readonly accentColor?: string | undefined; - readonly environment: ProviderInstanceEnvironment; + readonly environment: ResolvedProviderEnvironment; readonly enabled: boolean; readonly config: Config; } diff --git a/apps/server/src/provider/ProviderInstanceEnvironment.test.ts b/apps/server/src/provider/ProviderInstanceEnvironment.test.ts index 7d6bbe61a2aa..ba105b492cbf 100644 --- a/apps/server/src/provider/ProviderInstanceEnvironment.test.ts +++ b/apps/server/src/provider/ProviderInstanceEnvironment.test.ts @@ -5,7 +5,13 @@ import { describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Path from "effect/Path"; -import { mergeProviderInstanceEnvironment } from "./ProviderInstanceEnvironment.ts"; +import { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import * as Schema from "effect/Schema"; + +import { + literalProviderInstanceEnvironment, + mergeProviderInstanceEnvironment, +} from "./ProviderInstanceEnvironment.ts"; describe("mergeProviderInstanceEnvironment", () => { it.effect.each([ @@ -66,3 +72,23 @@ describe("mergeProviderInstanceEnvironment", () => { }); }); }); + +const decodeProviderInstanceEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); + +describe("literalProviderInstanceEnvironment", () => { + it("keeps every literal and leaves out secret sources", () => { + const environment = decodeProviderInstanceEnvironment([ + { name: "CODEX_HOME", value: "~/.codex-work" }, + { name: "REFERENCE_LOOKALIKE", value: "op://Private/item/field" }, + { + name: "TOKEN", + value: { kind: "1password", reference: "op://Private/item/field", account: "my" }, + }, + ]); + + expect(literalProviderInstanceEnvironment(environment)).toEqual([ + { name: "CODEX_HOME", value: "~/.codex-work", sensitive: false }, + { name: "REFERENCE_LOOKALIKE", value: "op://Private/item/field", sensitive: false }, + ]); + }); +}); diff --git a/apps/server/src/provider/ProviderInstanceEnvironment.ts b/apps/server/src/provider/ProviderInstanceEnvironment.ts index 77c0c6c2dc88..ac76da1dcaaa 100644 --- a/apps/server/src/provider/ProviderInstanceEnvironment.ts +++ b/apps/server/src/provider/ProviderInstanceEnvironment.ts @@ -1,9 +1,44 @@ -import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import type { + ProviderInstanceEnvironment, + ProviderInstanceEnvironmentVariableName, +} from "@t3tools/contracts"; import { expandHomePath } from "../pathExpansion.ts"; -export function mergeProviderInstanceEnvironment( +/** + * A provider environment variable whose value is ready for a child process. + * Configured variables may name a secret source instead of a value; only + * `ProviderSecretResolver` turns those into this shape, so a source cannot + * reach a process by accident. A value read from a secret store is always + * sensitive. + */ +export interface ResolvedProviderEnvironmentVariable { + readonly name: ProviderInstanceEnvironmentVariableName; + readonly value: string; + readonly sensitive: boolean; +} + +export type ResolvedProviderEnvironment = ReadonlyArray; + +/** + * The configured variables that hold literal values, for callers that build a + * process environment without resolving secrets. Variables that read from a + * secret store are left out. + */ +export function literalProviderInstanceEnvironment( environment: ProviderInstanceEnvironment | undefined, +): ResolvedProviderEnvironment { + const literals: Array = []; + for (const { name, value, sensitive } of environment ?? []) { + if (typeof value === "string") { + literals.push({ name, value, sensitive }); + } + } + return literals; +} + +export function mergeProviderInstanceEnvironment( + environment: ResolvedProviderEnvironment | undefined, baseEnv: NodeJS.ProcessEnv = process.env, ): NodeJS.ProcessEnv { if (!environment || environment.length === 0) { diff --git a/apps/server/src/provider/ProviderSecretReference.test.ts b/apps/server/src/provider/ProviderSecretReference.test.ts index a288451d1ae0..631f2239a2ba 100644 --- a/apps/server/src/provider/ProviderSecretReference.test.ts +++ b/apps/server/src/provider/ProviderSecretReference.test.ts @@ -5,31 +5,42 @@ import * as Schema from "effect/Schema"; import { collectProviderSecretReferences, hasProviderSecretReference, + OnePasswordSecretReference, providerSecretReference, } from "./ProviderSecretReference.ts"; const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); -describe("providerSecretReference", () => { - it("reads a 1Password reference", () => { - expect(providerSecretReference("op://Private/claude-code/credential")).toBe( - "op://Private/claude-code/credential", - ); +const ACCOUNT = "my.1password.com"; + +const onePasswordVariable = (name: string, reference: string, account = ACCOUNT) => ({ + name, + value: { kind: "1password" as const, reference, account }, +}); + +const secret = (reference: string) => + new OnePasswordSecretReference({ + reference, + account: ACCOUNT as OnePasswordSecretReference["account"], }); - it("trims a reference pasted with surrounding whitespace", () => { - expect(providerSecretReference(" op://Private/claude-code/credential\n")).toBe( - "op://Private/claude-code/credential", +describe("providerSecretReference", () => { + it("reads a 1Password source with its account", () => { + const [variable] = decodeEnvironment([ + onePasswordVariable("TOKEN", "op://Private/claude-code/credential", "my.1password.com"), + ]); + expect(providerSecretReference(variable!.value)).toEqual( + new OnePasswordSecretReference({ + reference: "op://Private/claude-code/credential", + account: "my.1password.com" as OnePasswordSecretReference["account"], + }), ); }); - it("treats a literal value as a literal", () => { + it("treats every string as a literal, including one that looks like a reference", () => { expect(providerSecretReference("sk-live-token")).toBeUndefined(); expect(providerSecretReference("/home/u/.claude/work")).toBeUndefined(); - }); - - it("ignores a bare scheme with nothing behind it", () => { - expect(providerSecretReference("op://")).toBeUndefined(); + expect(providerSecretReference("op://Private/claude-code/credential")).toBeUndefined(); }); }); @@ -44,7 +55,7 @@ describe("hasProviderSecretReference", () => { hasProviderSecretReference( decodeEnvironment([ { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: "op://Private/claude-code/credential" }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", "op://Private/claude-code/credential"), ]), ), ).toBe(true); @@ -61,25 +72,53 @@ describe("hasProviderSecretReference", () => { }); }); +describe("hasProviderSecretReference with secret sources", () => { + it("is true for a 1Password source", () => { + expect( + hasProviderSecretReference( + decodeEnvironment([ + onePasswordVariable("TOKEN", "op://Private/item/field", "my.1password.com"), + ]), + ), + ).toBe(true); + }); +}); + describe("collectProviderSecretReferences", () => { it("returns each distinct reference once, in first-seen order", () => { const shared = "op://Private/shared/credential"; const environments = [ decodeEnvironment([ - { name: "CLAUDE_CODE_OAUTH_TOKEN", value: shared, sensitive: true }, + onePasswordVariable("CLAUDE_CODE_OAUTH_TOKEN", shared), { name: "HOME", value: "/home/u" }, ]), undefined, decodeEnvironment([ - { name: "CODEX_TOKEN", value: "op://Private/codex/credential", sensitive: true }, + onePasswordVariable("CODEX_TOKEN", "op://Private/codex/credential"), // The same item behind two providers is one read, not two. - { name: "OTHER_TOKEN", value: shared, sensitive: true }, + onePasswordVariable("OTHER_TOKEN", shared), ]), ]; expect(Array.from(collectProviderSecretReferences(environments))).toEqual([ - shared, - "op://Private/codex/credential", + secret(shared), + secret("op://Private/codex/credential"), + ]); + }); + + it("keeps one reference read from different accounts apart", () => { + const shared = "op://Private/shared/credential"; + const references = collectProviderSecretReferences([ + decodeEnvironment([ + onePasswordVariable("HOME_TOKEN", shared, "my.1password.com"), + onePasswordVariable("WORK_TOKEN", shared, "acme.1password.com"), + onePasswordVariable("HOME_AGAIN", shared, "my.1password.com"), + ]), + ]); + + expect(references.map(({ reference, account }) => [reference, account])).toEqual([ + [shared, "my.1password.com"], + [shared, "acme.1password.com"], ]); }); diff --git a/apps/server/src/provider/ProviderSecretReference.ts b/apps/server/src/provider/ProviderSecretReference.ts index f9d69301a858..e2e4e8a70ae7 100644 --- a/apps/server/src/provider/ProviderSecretReference.ts +++ b/apps/server/src/provider/ProviderSecretReference.ts @@ -1,35 +1,55 @@ /** * Provider environment values that name a secret instead of carrying one. * - * A user who keeps a provider credential in 1Password can paste the item's - * secret reference (`op://Vault/Item/field`) as an environment variable's - * value instead of the secret itself. `ProviderSecretResolver` swaps the - * reference for the real value on the way into the provider process, so the - * credential never lands in `settings.json` or the on-disk secret store, and - * rotating it in 1Password rotates it here. + * A user who keeps a provider credential in 1Password configures the variable + * with a 1Password secret source (`{ kind: "1password", reference, account }`) + * instead of the secret itself. `ProviderSecretResolver` swaps the source for + * the real value on the way into the provider process, so the credential never + * lands in `settings.json` or the on-disk secret store, and rotating it in + * 1Password rotates it here. * * These helpers are pure so the instance registry can ask "does this * environment read from a secret store?" without depending on the resolver. * * @module provider/ProviderSecretReference */ -import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import type { + OnePasswordAccount, + ProviderInstanceEnvironment, + ProviderInstanceEnvironmentVariable, +} from "@t3tools/contracts"; +import * as Data from "effect/Data"; +import * as Equal from "effect/Equal"; -/** URI scheme 1Password uses for secret references; `op read` consumes these. */ -const PROVIDER_SECRET_REFERENCE_PREFIX = "op://"; +/** + * One secret to read from 1Password. Structurally comparable, so it doubles as + * the resolver's cache key: the same reference in two accounts is two secrets. + */ +export class OnePasswordSecretReference extends Data.TaggedClass("1password")<{ + readonly reference: string; + readonly account: OnePasswordAccount; +}> {} + +/** Every secret a provider environment value can name. */ +export type ProviderSecretReference = OnePasswordSecretReference; /** - * The secret reference an environment value names, or `undefined` when the - * value is a literal. The value is trimmed first: a reference copied out of a - * password manager routinely arrives with surrounding whitespace, which `op` - * rejects. + * The secret an environment value names, or `undefined` when the value is a + * literal. */ -export function providerSecretReference(value: string): string | undefined { - const trimmed = value.trim(); - if (!trimmed.startsWith(PROVIDER_SECRET_REFERENCE_PREFIX)) { +export function providerSecretReference( + value: ProviderInstanceEnvironmentVariable["value"], +): ProviderSecretReference | undefined { + if (typeof value === "string") { return undefined; } - return trimmed.length > PROVIDER_SECRET_REFERENCE_PREFIX.length ? trimmed : undefined; + switch (value.kind) { + case "1password": + return new OnePasswordSecretReference({ + reference: value.reference, + account: value.account, + }); + } } /** @@ -46,25 +66,25 @@ export function hasProviderSecretReference( } /** - * Every distinct secret reference across a set of environments, in the order - * they were first seen. + * Every distinct secret across a set of environments, in the order they were + * first seen. * - * Resolution is per reference but unlocking is per `op` invocation, so the - * caller that is about to build many instances wants the whole list up front: - * one call covering every reference costs one authorization, where one call - * per instance costs one each. + * Resolution is per secret but unlocking is per `op` invocation, so the caller + * that is about to build many instances wants the whole list up front: one + * call covering every secret costs one authorization, where one call per + * instance costs one each. */ export function collectProviderSecretReferences( environments: Iterable, -): ReadonlyArray { - const references = new Set(); +): ReadonlyArray { + const references: Array = []; for (const environment of environments) { for (const variable of environment ?? []) { const reference = providerSecretReference(variable.value); - if (reference !== undefined) { - references.add(reference); + if (reference !== undefined && !references.some((seen) => Equal.equals(seen, reference))) { + references.push(reference); } } } - return Array.from(references); + return references; } diff --git a/apps/server/src/provider/Services/ProviderSecretResolver.ts b/apps/server/src/provider/Services/ProviderSecretResolver.ts index 4ff448c08938..679f9ff43269 100644 --- a/apps/server/src/provider/Services/ProviderSecretResolver.ts +++ b/apps/server/src/provider/Services/ProviderSecretResolver.ts @@ -1,21 +1,25 @@ /** - * ProviderSecretResolver: turns `op://` environment values into the secrets - * they name, once, and holds them in memory. + * ProviderSecretResolver: turns environment values that name a secret (such as + * a 1Password secret source) into the secrets they name, once, and holds them + * in memory. * * Every provider instance resolves its environment when the driver builds it, * and a single instance can rebuild several times per session. Shelling out * to `op` on each of those is slow (seconds) and, worse, can put a biometric * prompt in front of a user who only started a thread. The resolver therefore - * caches by reference for the lifetime of the process; `invalidate` is wired + * caches by secret (reference plus account) for the lifetime of the process; `invalidate` is wired * to the Settings refresh button, which is the user's way of saying "go read * it again" after rotating a credential. * * @module provider/Services/ProviderSecretResolver */ -import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import type { OnePasswordAccountSummary, ProviderInstanceEnvironment } from "@t3tools/contracts"; import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; +import type { ResolvedProviderEnvironment } from "../ProviderInstanceEnvironment.ts"; +import type { ProviderSecretReference } from "../ProviderSecretReference.ts"; + /** * An instance environment after its secret references have been read. * @@ -27,15 +31,14 @@ import * as Effect from "effect/Effect"; * one, and it hides as "authenticated". */ export interface ResolvedProviderInstanceEnvironment { - readonly variables: ProviderInstanceEnvironment | undefined; + readonly variables: ResolvedProviderEnvironment; readonly unresolved: ReadonlyArray; } export interface ProviderSecretResolverShape { /** - * Replace every secret reference in the environment with its value. - * Literal values pass through untouched, and an environment with no - * references is returned as-is. + * Replace every secret the environment names with its value. Literal values + * pass through untouched. * * Never fails. A reference that cannot be read (1Password locked, `op` not * installed, item deleted) is reported as unresolved rather than @@ -62,7 +65,7 @@ export interface ProviderSecretResolverShape { * found it, so `resolve` falls back to reading one reference at a time with * the same per-variable failure isolation it has always had. */ - readonly prime: (references: ReadonlyArray) => Effect.Effect; + readonly prime: (references: ReadonlyArray) => Effect.Effect; /** * Drop every cached secret. The next `resolve` re-reads from the store. * Callers that need the new value to reach a running provider must also @@ -70,21 +73,43 @@ export interface ProviderSecretResolverShape { * spawned with. */ readonly invalidate: Effect.Effect; + /** + * The 1Password accounts signed in on this machine, so settings can offer + * them instead of asking the user to type one. Reads local `op` config only, + * so it never prompts. Empty when `op` is missing or fails. + */ + readonly listOnePasswordAccounts: Effect.Effect>; } /** - * Defaults to handing every environment back untouched, which is what a build + * Defaults to handing every literal back untouched, which is what a build * without secret-store integration behaves like, and what tests want unless - * they are testing resolution itself: an `op://` value stays an `op://` - * value, and the provider reports whatever the CLI makes of it. + * they are testing resolution itself. A secret source has no literal form, so + * it is reported unresolved. */ +const resolveWithoutSecretStore = ( + environment: ProviderInstanceEnvironment | undefined, +): ResolvedProviderInstanceEnvironment => { + const variables: Array = []; + const unresolved: Array = []; + for (const { name, value, sensitive } of environment ?? []) { + if (typeof value === "string") { + variables.push({ name, value, sensitive }); + } else { + unresolved.push(name); + } + } + return { variables, unresolved }; +}; + export class ProviderSecretResolver extends Context.Reference( "t3/provider/Services/ProviderSecretResolver", { defaultValue: () => ({ - resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + resolve: (environment) => Effect.sync(() => resolveWithoutSecretStore(environment)), prime: () => Effect.void, invalidate: Effect.void, + listOnePasswordAccounts: Effect.succeed([]), }), }, ) {} diff --git a/apps/server/src/provider/acp/AcpRegistryAuthenticationState.ts b/apps/server/src/provider/acp/AcpRegistryAuthenticationState.ts index 1f4799e65c24..17ef8971a87b 100644 --- a/apps/server/src/provider/acp/AcpRegistryAuthenticationState.ts +++ b/apps/server/src/provider/acp/AcpRegistryAuthenticationState.ts @@ -1,9 +1,5 @@ import * as NodeCrypto from "node:crypto"; -import type { - AcpRegistrySettings, - ProviderInstanceEnvironment, - ProviderInstanceId, -} from "@t3tools/contracts"; +import type { AcpRegistrySettings, ProviderInstanceId } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Path from "effect/Path"; @@ -12,6 +8,7 @@ import * as Schema from "effect/Schema"; import * as Semaphore from "effect/Semaphore"; import { writeFileStringAtomically } from "../../atomicWrite.ts"; +import type { ResolvedProviderEnvironment } from "../ProviderInstanceEnvironment.ts"; const decodeState = Schema.decodeUnknownEffect( Schema.fromJsonString(Schema.Struct({ binding: Schema.String, authenticated: Schema.Boolean })), @@ -25,7 +22,7 @@ export const makeAcpRegistryAuthenticationState = Effect.fn("makeAcpRegistryAuth readonly cacheDir: string; readonly instanceId: ProviderInstanceId; readonly settings: AcpRegistrySettings; - readonly environment: ProviderInstanceEnvironment; + readonly environment: ResolvedProviderEnvironment; readonly processEnvironment: NodeJS.ProcessEnv; }) { const fs = yield* FileSystem.FileSystem; diff --git a/apps/server/src/provider/providerInstallation.ts b/apps/server/src/provider/providerInstallation.ts index 62931e2e0418..858b288ae46f 100644 --- a/apps/server/src/provider/providerInstallation.ts +++ b/apps/server/src/provider/providerInstallation.ts @@ -18,7 +18,10 @@ import * as AntigravityInstallation from "./AntigravityInstallation.ts"; import { deriveProviderInstanceConfigMap } from "./Layers/ProviderInstanceRegistryHydration.ts"; import * as ProviderInstanceRegistry from "./Services/ProviderInstanceRegistry.ts"; import * as ProviderRegistry from "./Services/ProviderRegistry.ts"; -import { mergeProviderInstanceEnvironment } from "./ProviderInstanceEnvironment.ts"; +import { + literalProviderInstanceEnvironment, + mergeProviderInstanceEnvironment, +} from "./ProviderInstanceEnvironment.ts"; const ANTIGRAVITY = ProviderDriverKind.make("antigravity"); const hasBinaryPath = Schema.is(Schema.Struct({ binaryPath: Schema.String })); @@ -140,7 +143,9 @@ export const makeProviderInstallation = Effect.fn("makeProviderInstallation")(fu } const binaryPath = entry.config.binaryPath.trim(); return resolveCommandPath(binaryPath, { - env: mergeProviderInstanceEnvironment(entry.environment), + env: mergeProviderInstanceEnvironment( + literalProviderInstanceEnvironment(entry.environment), + ), }).pipe( Effect.map((resolved) => [binaryPath, resolved]), Effect.orElseSucceed(() => [binaryPath]), diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index 512e3dc09dbb..989202f285fe 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -551,7 +551,7 @@ const RuntimeCoreDependenciesBaseLive = Layer.mergeAll( // `providerInstances` hydration merges `settings.providers.` // with explicit `providerInstances` entries on boot. Layer.provideMerge(ProviderInstanceRegistryHydrationLive), - // Resolves `op://` environment values for both halves above: the instance + // Resolves secret-source environment values for both halves above: the instance // registry reads secrets while building an instance, and // `ProviderRegistryLive` drops the cached values when the user refreshes. Layer.provideMerge(ProviderSecretResolverLive), diff --git a/apps/server/src/serverSettings.test.ts b/apps/server/src/serverSettings.test.ts index 51d81dc9f1ea..bc0c35574bfe 100644 --- a/apps/server/src/serverSettings.test.ts +++ b/apps/server/src/serverSettings.test.ts @@ -5,6 +5,7 @@ import { ProjectId, ProjectScript, ProviderDriverKind, + ProviderInstanceEnvironment, ProviderInstanceId, resolveProviderInstanceEnabled, ServerSettings, @@ -1417,6 +1418,47 @@ it.layer(NodeServices.layer)("server settings", (it) => { ); } + it.effect("stores 1Password secret sources as written and never as sensitive", () => + Effect.gen(function* () { + const serverSettings = yield* ServerSettingsModule.ServerSettingsService; + const serverConfig = yield* ServerConfig.ServerConfig; + const fileSystem = yield* FileSystem.FileSystem; + const instanceId = ProviderInstanceId.make("codex_personal"); + const source = { + kind: "1password", + reference: "op://Home Lab/Codex/api key", + account: "my.1password.com", + } as const; + const decodeEnvironment = Schema.decodeEffect(ProviderInstanceEnvironment); + const environment = yield* decodeEnvironment([ + { name: "OPENAI_API_KEY", value: source, sensitive: true, valueRedacted: true }, + ]); + const expected = yield* decodeEnvironment([ + { name: "OPENAI_API_KEY", value: source, sensitive: false }, + ]); + + const next = yield* serverSettings.updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment, + config: {}, + }, + }, + }); + + assert.deepEqual(next.providerInstances[instanceId]?.environment, expected); + assert.deepEqual( + ServerSettingsModule.redactServerSettingsForClient(next).providerInstances[instanceId] + ?.environment, + expected, + ); + const raw = yield* fileSystem.readFileString(serverConfig.settingsPath); + // @effect-diagnostics-next-line preferSchemaOverJson:off + assert.deepEqual(JSON.parse(raw).providerInstances.codex_personal.environment, expected); + }).pipe(Effect.provide(makeServerSettingsLayer())), + ); + it.effect("stores sensitive provider instance environment values outside settings.json", () => Effect.gen(function* () { const serverSettings = yield* ServerSettingsModule.ServerSettingsService; diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts index 55754dcde0be..c71570ae1657 100644 --- a/apps/server/src/serverSettings.ts +++ b/apps/server/src/serverSettings.ts @@ -166,6 +166,12 @@ const redactSecret = (value: string) => (value.length > 0 ? SECRET_REDACTED : "" function redactProviderEnvironmentVariable( variable: ProviderInstanceEnvironmentVariable, ): ProviderInstanceEnvironmentVariable { + const { value } = variable; + // A secret source names a secret without carrying it, so it is never + // sensitive and is stored as written. + if (typeof value !== "string") { + return { name: variable.name, value, sensitive: false }; + } if (!variable.sensitive) { const { valueRedacted: _omit, ...rest } = variable; return rest; @@ -173,7 +179,7 @@ function redactProviderEnvironmentVariable( return { ...variable, value: "", - ...(variable.value.length > 0 || variable.valueRedacted ? { valueRedacted: true } : {}), + ...(value.length > 0 || variable.valueRedacted ? { valueRedacted: true } : {}), }; } @@ -820,7 +826,11 @@ const make = Effect.gen(function* () { if (!instance.environment) continue; const environment: ProviderInstanceEnvironmentVariable[] = []; for (const variable of instance.environment) { - if (!variable.sensitive || !variable.valueRedacted) { + if ( + !variable.sensitive || + !variable.valueRedacted || + typeof variable.value !== "string" + ) { environment.push(variable); continue; } @@ -925,7 +935,8 @@ const make = Effect.gen(function* () { const environment: ProviderInstanceEnvironmentVariable[] = []; for (const variable of instance.environment) { const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name }); - if (!variable.sensitive) { + const { value: configuredValue } = variable; + if (!variable.sensitive || typeof configuredValue !== "string") { changes.push({ kind: "remove", secretName, @@ -945,10 +956,13 @@ const make = Effect.gen(function* () { ) : undefined; const inlineValue = - previous?.sensitive && !previous.valueRedacted && previous.value.length > 0 + previous?.sensitive && + !previous.valueRedacted && + typeof previous.value === "string" && + previous.value.length > 0 ? previous.value : undefined; - const value = inlineValue ?? variable.value; + const value = inlineValue ?? configuredValue; if (!variable.valueRedacted || inlineValue !== undefined) { if (value.length > 0) { changes.push({ diff --git a/apps/server/src/terminal/Manager.ts b/apps/server/src/terminal/Manager.ts index 6122c8606f0b..053af154f385 100644 --- a/apps/server/src/terminal/Manager.ts +++ b/apps/server/src/terminal/Manager.ts @@ -62,7 +62,10 @@ import * as Semaphore from "effect/Semaphore"; import * as SynchronizedRef from "effect/SynchronizedRef"; import * as ServerConfig from "../config.ts"; -import { mergeProviderInstanceEnvironment } from "../provider/ProviderInstanceEnvironment.ts"; +import { + literalProviderInstanceEnvironment, + mergeProviderInstanceEnvironment, +} from "../provider/ProviderInstanceEnvironment.ts"; import { resolveCodexHomeLayout } from "../provider/Drivers/CodexHomeLayout.ts"; import { makeClaudeEnvironment } from "../provider/Drivers/ClaudeHome.ts"; import { deriveProviderInstanceConfigMap } from "../provider/Layers/ProviderInstanceRegistryHydration.ts"; @@ -1401,7 +1404,10 @@ export const resolveProviderInstanceTerminalEnvironment = Effect.fn( return yield* new TerminalProviderInstanceNotFoundError({ providerInstanceId }); } - let resolved = mergeProviderInstanceEnvironment(instance.environment, input.env ?? {}); + let resolved = mergeProviderInstanceEnvironment( + literalProviderInstanceEnvironment(instance.environment), + input.env ?? {}, + ); if (instance.driver === "codex") { const config = decodeCodexSettings(instance.config ?? {}); if (Option.isSome(config)) { diff --git a/apps/server/src/usage/UsageService.ts b/apps/server/src/usage/UsageService.ts index a3b07d64d71b..f0e8d9b826fa 100644 --- a/apps/server/src/usage/UsageService.ts +++ b/apps/server/src/usage/UsageService.ts @@ -49,7 +49,10 @@ import { expandHomePath } from "../pathExpansion.ts"; import * as ServerSettings from "../serverSettings.ts"; import { resolveCodexHomeLayout } from "../provider/Drivers/CodexHomeLayout.ts"; import { resolveAntigravityInstanceDirectories } from "../provider/antigravityAuthSupport.ts"; -import { mergeProviderInstanceEnvironment } from "../provider/ProviderInstanceEnvironment.ts"; +import { + literalProviderInstanceEnvironment, + mergeProviderInstanceEnvironment, +} from "../provider/ProviderInstanceEnvironment.ts"; import { readOpenCodeUsage } from "./opencodeUsageReader.ts"; import { readAntigravityUsage } from "./antigravityUsageReader.ts"; import { readCursorAccountUsage } from "./cursorUsageReader.ts"; @@ -283,7 +286,10 @@ export const make = Effect.gen(function* () { }); } for (const instance of instances) { - const environment = mergeProviderInstanceEnvironment(instance.environment, hostEnvironment); + const environment = mergeProviderInstanceEnvironment( + literalProviderInstanceEnvironment(instance.environment), + hostEnvironment, + ); const provider = driver === "claudeAgent" ? "claude" : driver; let home: string; if (driver === "codex") { diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 2a0006ddb24b..b8c78c5e28d9 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -161,6 +161,7 @@ import { } from "./observability/RpcInstrumentation.ts"; import * as ProviderRegistry from "./provider/Services/ProviderRegistry.ts"; import * as ProviderInstanceRegistry from "./provider/Services/ProviderInstanceRegistry.ts"; +import { ProviderSecretResolver } from "./provider/Services/ProviderSecretResolver.ts"; import * as AcpRegistrySupport from "./provider/acp/AcpRegistrySupport.ts"; import * as AcpRegistryRuntimeCoordinator from "./provider/acp/AcpRegistryRuntimeCoordinator.ts"; import * as ModelManifest from "./provider/ModelManifest.ts"; @@ -1199,6 +1200,7 @@ const makeWsRpcLayer = ( ); const serverAuth = yield* EnvironmentAuth.EnvironmentAuth; const sourceControlDiscovery = yield* SourceControlDiscovery.SourceControlDiscovery; + const providerSecretResolver = yield* ProviderSecretResolver; const automaticGitFetchInterval = serverSettings.getSettings.pipe( Effect.map( (settings) => resolveServerBackgroundActivitySettings(settings).automaticGitFetchInterval, @@ -2526,6 +2528,14 @@ const makeWsRpcLayer = ( "rpc.aggregate": "server", }, ), + [WS_METHODS.serverListOnePasswordAccounts]: (_input) => + observeRpcEffect( + WS_METHODS.serverListOnePasswordAccounts, + providerSecretResolver.listOnePasswordAccounts, + { + "rpc.aggregate": "server", + }, + ), [WS_METHODS.serverGetTraceDiagnostics]: (_input) => observeRpcEffect( WS_METHODS.serverGetTraceDiagnostics, diff --git a/apps/web/src/components/settings/ProviderInstanceCard.test.ts b/apps/web/src/components/settings/ProviderInstanceCard.test.ts index e4cc01013286..ac8fa6f1716f 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.test.ts +++ b/apps/web/src/components/settings/ProviderInstanceCard.test.ts @@ -4,6 +4,8 @@ import { describe, expect, it } from "vite-plus/test"; import { ProviderDriverKind, ProviderInstanceId, + type OnePasswordAccount, + type OnePasswordSecretReference, type ServerProvider, type ServerProviderModel, } from "@t3tools/contracts"; @@ -14,6 +16,7 @@ import { providerEnvironmentWithoutNames, ProviderInstanceCard, readProviderEnvironmentVariable, + splitDedicatedProviderEnvironment, } from "./ProviderInstanceCard"; describe("deriveProviderModelsForDisplay", () => { @@ -231,4 +234,37 @@ describe("provider environment helpers", () => { { name: "EXTRA_FLAG", value: "1", sensitive: false }, ]); }); + + it("keeps a dedicated variable read from 1Password in the generic editor", () => { + const source = { + kind: "1password" as const, + reference: "op://Private/cursor/credential" as OnePasswordSecretReference, + account: "my" as OnePasswordAccount, + }; + const environment = [ + { name: "CURSOR_API_KEY", value: source, sensitive: false }, + { name: "EXTRA_FLAG", value: "1", sensitive: false }, + ]; + + expect(splitDedicatedProviderEnvironment(environment, new Set(["CURSOR_API_KEY"]))).toEqual({ + dedicated: [], + generic: environment, + }); + }); + + it("keeps a plain op:// value in the generic editor so its account can be picked", () => { + const legacy = { + name: "CURSOR_API_KEY", + value: "op://Private/cursor/credential", + sensitive: false, + }; + const literal = { name: "OTHER_KEY", value: "cursor-key", sensitive: true }; + + expect( + splitDedicatedProviderEnvironment( + [legacy, literal], + new Set(["CURSOR_API_KEY", "OTHER_KEY"]), + ), + ).toEqual({ dedicated: [literal], generic: [legacy] }); + }); }); diff --git a/apps/web/src/components/settings/ProviderInstanceCard.tsx b/apps/web/src/components/settings/ProviderInstanceCard.tsx index b0a727f9ee5a..289078f8acce 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.tsx +++ b/apps/web/src/components/settings/ProviderInstanceCard.tsx @@ -10,16 +10,23 @@ import { LockIcon, LockOpenIcon, ExternalLinkIcon, + KeyRoundIcon, PlusIcon, Trash2Icon, + TypeIcon, XIcon, } from "lucide-react"; import * as Arr from "effect/Array"; import * as Result from "effect/Result"; +import * as Schema from "effect/Schema"; import { useEffect, useRef, useState, type ReactElement, type ReactNode } from "react"; import { isProviderDriverKind, + OnePasswordAccount, + OnePasswordSecretReference, resolveProviderInstanceEnabled, + type OnePasswordAccountSummary, + type OnePasswordSecretSource, type ProviderInstanceConfig, type ProviderInstanceEnvironmentVariable, type ProviderInstanceId, @@ -37,12 +44,15 @@ import { toCustomModelSetting, } from "@t3tools/shared/model"; import { cn } from "../../lib/utils"; +import { useEnvironmentQuery } from "../../state/query"; +import { serverEnvironment } from "../../state/server"; import { useCopyToClipboard } from "../../hooks/useCopyToClipboard"; import { normalizeProviderAccentColor } from "../../providerInstances"; import { Badge } from "../ui/badge"; import { Button } from "../ui/button"; import { DraftInput } from "../ui/draft-input"; import { Popover, PopoverPopup, PopoverTrigger } from "../ui/popover"; +import { Select, SelectItem, SelectPopup, SelectTrigger, SelectValue } from "../ui/select"; import { Switch } from "../ui/switch"; import { stackedThreadToast, toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; @@ -85,27 +95,138 @@ function ProviderStatusDiagnostic({ let environmentVariableDraftId = 0; const nextEnvironmentVariableDraftId = () => `provider-env-${environmentVariableDraftId++}`; +type EnvironmentDraftSource = "plain" | "1password"; + +const PLAIN_SOURCE_OPTION = "plain"; +const EMPTY_ONE_PASSWORD_ACCOUNTS: ReadonlyArray = []; +const ONE_PASSWORD_SOURCE_OPTION_PREFIX = "1password:"; + +/** + * The source select's option for a row: a plain value, or the 1Password account + * it reads from. When the account is typed instead, every 1Password row shares + * one option. + */ +function environmentDraftSourceOption(row: EnvironmentDraftRow, typesAccount: boolean): string { + if (row.source !== "1password") return PLAIN_SOURCE_OPTION; + return `${ONE_PASSWORD_SOURCE_OPTION_PREFIX}${typesAccount ? "" : row.account}`; +} + +function EnvironmentDraftSourceLabel(props: { readonly option: string }) { + if (!props.option.startsWith(ONE_PASSWORD_SOURCE_OPTION_PREFIX)) { + return ( + + + Value + + ); + } + const account = props.option.slice(ONE_PASSWORD_SOURCE_OPTION_PREFIX.length); + return ( + + + {account.length > 0 ? account : "1Password"} + + ); +} + type EnvironmentDraftRow = { readonly id: string; readonly name: string; + readonly source: EnvironmentDraftSource; readonly value: string; + readonly reference: string; + readonly account: string; readonly sensitive: boolean; readonly valueRedacted?: boolean; + /** The name this row is saved under, so an unfinished edit can fall back to it. */ + readonly savedName?: string; }; +/** + * Plain `op://` values were once read from 1Password. They are literals now, + * so they open as a 1Password source that still needs its account. + */ +function isLegacyOnePasswordReference(variable: ProviderInstanceEnvironmentVariable): boolean { + return ( + typeof variable.value === "string" && + variable.valueRedacted !== true && + variable.value.trim().startsWith("op://") + ); +} + function makeEnvironmentDraftRow( variable: ProviderInstanceEnvironmentVariable, index: number, ): EnvironmentDraftRow { + const id = `${index}:${variable.name}`; + if (typeof variable.value !== "string") { + return { + id, + name: variable.name, + source: "1password", + savedName: variable.name, + value: "", + reference: variable.value.reference, + account: variable.value.account, + sensitive: false, + }; + } + if (isLegacyOnePasswordReference(variable)) { + return { + id, + name: variable.name, + source: "1password", + savedName: variable.name, + value: "", + reference: variable.value.trim(), + account: "", + sensitive: false, + }; + } return { - id: `${index}:${variable.name}`, + id, name: variable.name, + source: "plain", + savedName: variable.name, value: variable.value, + reference: "", + account: "", sensitive: variable.sensitive, ...(variable.valueRedacted !== undefined ? { valueRedacted: variable.valueRedacted } : {}), }; } +const decodeOnePasswordSecretReference = Schema.decodeUnknownResult(OnePasswordSecretReference); +const decodeOnePasswordAccount = Schema.decodeUnknownResult(OnePasswordAccount); + +/** + * The 1Password source a draft row describes, or the message explaining why + * it is not one yet. + */ +function onePasswordSourceFromDraft( + row: EnvironmentDraftRow, +): Result.Result { + const reference = decodeOnePasswordSecretReference(row.reference); + if (Result.isFailure(reference)) return Result.fail(reference.failure.message); + const account = decodeOnePasswordAccount(row.account); + if (Result.isFailure(account)) return Result.fail(account.failure.message); + return Result.succeed({ + kind: "1password", + reference: reference.success, + account: account.success, + }); +} + +function environmentValuesEqual( + left: ProviderInstanceEnvironmentVariable["value"], + right: ProviderInstanceEnvironmentVariable["value"], +): boolean { + if (typeof left === "string" || typeof right === "string") return left === right; + return ( + left.kind === right.kind && left.reference === right.reference && left.account === right.account + ); +} + function providerEnvironmentsEqual( left: ReadonlyArray, right: ReadonlyArray, @@ -117,7 +238,7 @@ function providerEnvironmentsEqual( return ( other !== undefined && variable.name === other.name && - variable.value === other.value && + environmentValuesEqual(variable.value, other.value) && variable.sensitive === other.sensitive && variable.valueRedacted === other.valueRedacted ); @@ -214,6 +335,32 @@ export function providerEnvironmentWithoutNames( return (environment ?? []).filter((variable) => !names.has(variable.name)); } +/** + * Dedicated fields only edit plain values, so a variable read from a secret + * store, or one that still has to become one, stays in the generic editor, + * which can show its source. + */ +export function splitDedicatedProviderEnvironment( + environment: ReadonlyArray | undefined, + names: ReadonlySet, +): { + readonly dedicated: ReadonlyArray; + readonly generic: ReadonlyArray; +} { + const dedicated: ProviderInstanceEnvironmentVariable[] = []; + const generic: ProviderInstanceEnvironmentVariable[] = []; + for (const variable of environment ?? []) { + if ( + names.has(variable.name) && + typeof variable.value === "string" && + !isLegacyOnePasswordReference(variable) + ) { + dedicated.push(variable); + } else generic.push(variable); + } + return { dedicated, generic }; +} + export function nextProviderEnvironmentWithFieldValue( environment: ReadonlyArray | undefined, field: ProviderEnvironmentFieldDefinition, @@ -257,10 +404,17 @@ function ProviderEnvironmentFieldRow(props: { readonly onRemove: (field: ProviderEnvironmentFieldDefinition) => void; }) { const inputId = `${props.idPrefix}-environment-${props.field.name}`; - const value = props.variable?.valueRedacted ? "" : (props.variable?.value ?? ""); - const placeholder = props.variable?.valueRedacted - ? "Stored secret - enter a new value to replace" - : props.field.placeholder; + const configuredValue = props.variable?.value; + const readFromSecretStore = + props.variable !== undefined && + (typeof configuredValue !== "string" || isLegacyOnePasswordReference(props.variable)); + const value = + props.variable?.valueRedacted || typeof configuredValue !== "string" ? "" : configuredValue; + const placeholder = readFromSecretStore + ? "Read from 1Password - edit it under Environment" + : props.variable?.valueRedacted + ? "Stored secret - enter a new value to replace" + : props.field.placeholder; return ( props.onCommit(props.field, next)} placeholder={placeholder} + disabled={readFromSecretStore} spellCheck={false} /> - {props.variable ? ( + {props.variable && !readFromSecretStore ? ( - } - /> - - {variable.sensitive ? "Sensitive, stored separately" : "Plain text"} - - - - - ))} + updateVariable(variable.id, { name: name.trim() })} + placeholder="VARIABLE_NAME" + spellCheck={false} + aria-label={`Environment variable name ${index + 1}`} + /> + + + {isOnePassword ? ( + <> + + updateVariable(variable.id, { reference: reference.trim() }) + } + placeholder="op://vault/item/field" + spellCheck={false} + aria-invalid={sourceIssue !== undefined || undefined} + aria-label={`Environment variable 1Password reference ${index + 1}`} + /> + {typesAccount ? ( + updateVariable(variable.id, { account })} + placeholder="my.1password.com" + spellCheck={false} + aria-invalid={sourceIssue !== undefined || undefined} + aria-label={`Environment variable 1Password account ${index + 1}`} + /> + ) : null} + + ) : ( + <> + updateVariable(variable.id, { value })} + type={variable.sensitive ? "password" : undefined} + autoComplete="off" + placeholder={ + variable.valueRedacted + ? "Stored secret, enter a new value to replace" + : "value" + } + spellCheck={false} + aria-label={`Environment variable value ${index + 1}`} + /> + + { + const sensitive = !variable.sensitive; + updateVariable(variable.id, { + sensitive, + ...(sensitive && variable.valueRedacted === undefined + ? {} + : { + valueRedacted: sensitive ? variable.valueRedacted : false, + }), + }); + }} + aria-pressed={variable.sensitive} + aria-label={`Mark environment variable ${variable.name || index + 1} as sensitive`} + > + {variable.sensitive ? ( + + ) : ( + + )} + + } + /> + + {variable.sensitive ? "Sensitive, stored separately" : "Plain text"} + + + + )} + {sourceIssue !== undefined ? ( +

{sourceIssue}

+ ) : null} + + ); + })}

- Sensitive values are stored separately and never returned to the app. + Sensitive values are stored separately and never returned to the app. 1Password + references are read with the 1Password CLI each time the provider starts.

) : null} @@ -690,17 +1007,21 @@ export function ProviderInstanceCard({ // the generic editor only shows the remaining variables. const environmentFields = driverOption?.environmentFields ?? []; const environmentFieldNames = new Set(environmentFields.map((field) => field.name)); - const genericEnvironment = providerEnvironmentWithoutNames( - instance.environment, - environmentFieldNames, + const onePasswordAccounts = useEnvironmentQuery( + environmentId === undefined + ? null + : serverEnvironment.onePasswordAccounts({ environmentId, input: {} }), ); + const { dedicated: dedicatedEnvironment, generic: genericEnvironment } = + splitDedicatedProviderEnvironment(instance.environment, environmentFieldNames); const updateGenericEnvironment = ( environment: ReadonlyArray, ) => { - const dedicatedEnvironment = (instance.environment ?? []).filter((variable) => - environmentFieldNames.has(variable.name), - ); - updateEnvironment([...dedicatedEnvironment, ...environment]); + const genericNames = new Set(environment.map((variable) => variable.name)); + updateEnvironment([ + ...providerEnvironmentWithoutNames(dedicatedEnvironment, genericNames), + ...environment, + ]); }; const updateEnvironmentField = (field: ProviderEnvironmentFieldDefinition, value: string) => { updateEnvironment(nextProviderEnvironmentWithFieldValue(instance.environment, field, value)); @@ -1143,6 +1464,7 @@ export function ProviderInstanceCard({ > {environmentId !== undefined && liveProvider?.driver === "acpRegistry" ? ( diff --git a/docs/fork/0016-provider-secrets-live-in-1password.md b/docs/fork/0016-provider-secrets-live-in-1password.md index 00d2cf991f93..34836b6393aa 100644 --- a/docs/fork/0016-provider-secrets-live-in-1password.md +++ b/docs/fork/0016-provider-secrets-live-in-1password.md @@ -5,10 +5,10 @@ ## What you can do now -- Give a provider its credential without giving T3 Code the credential. Paste a - 1Password `op://` secret reference as an environment variable value on any - provider instance, and T3 Code reads the value from the 1Password CLI when it - starts the agent. What gets saved is the reference; the secret itself is +- Give a provider its credential without giving T3 Code the credential. Set any + provider instance's environment variable to a 1Password secret reference + plus the account it lives in, and T3 Code reads the value from the 1Password + CLI when it starts the agent. What gets saved is the reference; the secret itself is never written to the settings file or to T3 Code's secret store. - Unlock your vault once instead of all day. Each reference is read a single time and held in memory for the life of the server, so starting a thread, diff --git a/docs/internals/providers.md b/docs/internals/providers.md index e03e3254266a..f3fa3543c83d 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -128,21 +128,25 @@ current client support. ## Secret references in provider environments -A provider instance's `environment` can hold values that start with `op://`. Those are secret -references, and +A provider instance's `environment` can hold secret references: a `ProviderSecretSource` value such +as `{ kind: "1password", reference, account }`. A plain string is always a literal, including one +that starts with `op://`. [`ProviderSecretResolver`](../../apps/server/src/provider/Services/ProviderSecretResolver.ts) swaps each one for the value the 1Password CLI returns. This happens once per instance in [`ProviderInstanceRegistryLive`](../../apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts), before `driver.create`, so drivers and the orchestration v2 adapters built from them only ever see -resolved values. The registry keeps the raw `op://` config, which is what makes a later rebuild -possible. +resolved values; `ResolvedProviderEnvironment` is the type that enforces it. The registry keeps the +raw config, which is what makes a later rebuild possible. Paths that build a process environment +without the resolver (terminals, usage, installation, session scanning) use +`literalProviderInstanceEnvironment`, which leaves references out rather than passing them through. The parsing half lives in [`ProviderSecretReference.ts`](../../apps/server/src/provider/ProviderSecretReference.ts) and knows nothing about how a secret is fetched, so the registry can ask "does this instance read from a secret store?" without depending on the resolver. [`ProviderSecretResolverLive`](../../apps/server/src/provider/Layers/ProviderSecretResolverLive.ts) -is the half that shells out to `op read --no-newline`. +is the half that shells out to `op read --account --no-newline`. The account is part of the +cache key, because the same reference in two accounts is two different secrets. Three decisions are load-bearing: @@ -155,8 +159,8 @@ Three decisions are load-bearing: `HostProcessEnvironment` with every unresolved name removed. - **Reads are batched across instances, and sequential within one.** The store charges an unlock per `op` invocation, not per secret, and every instance resolves its own environment as it is built, - so a fleet would otherwise cost one prompt per provider. `prime` reads the whole set in a single - `op inject` before the builds start, called from the settings watcher (which covers boot) and from + so a fleet would otherwise cost one prompt per provider. `prime` reads the whole set with one + `op inject` per account before the builds start, called from the settings watcher (which covers boot) and from `reloadSecretBackedInstances` (which covers the refresh button). Whatever `prime` misses, `resolve` still walks with a plain loop rather than `Effect.forEach` with concurrency, so it produces one prompt rather than several simultaneous ones. diff --git a/docs/user/provider-secrets.md b/docs/user/provider-secrets.md index 7156e8f3b028..ae42187db8d1 100644 --- a/docs/user/provider-secrets.md +++ b/docs/user/provider-secrets.md @@ -7,22 +7,33 @@ For provider setup itself, see [Codex](./providers-codex.md) and [Claude](./prov ## I Do Not Want To Paste My Token Into T3 Code -Paste the 1Password reference instead of the value. +Point the variable at 1Password instead of pasting the value. -In the provider's Environment variables section in Settings, use the `op://` secret reference as the -value: +In the provider's Environment variables section in Settings, add the variable, pick the 1Password +account it lives in as its source, and fill in the secret reference: ```text -Name: CLAUDE_CODE_OAUTH_TOKEN -Value: op://Private/claude-code/credential +Name: CLAUDE_CODE_OAUTH_TOKEN +Source: my.1password.com +Reference: op://Private/claude-code/credential ``` +The source list offers every account the 1Password CLI is signed in to on the machine running the +T3 Code server. Naming the account means a reference keeps resolving against the right one when you +are signed in to more than one. If the server cannot list any accounts, type the account instead: +anything `op --account` accepts works. + T3 Code reads the value with the 1Password CLI right before it starts the agent, and hands the resolved value to the agent process only. The reference is what T3 Code stores; the secret itself never lands in your settings file or in T3 Code's secret store. -Any value beginning with `op://` is treated this way. Everything else is used exactly as typed, so -mixing literal variables and references on the same provider is fine. +Plain values are used exactly as typed, so mixing literal variables and references on the same +provider is fine. + +A plain value is never read from 1Password, even one that starts with `op://`. Settings saved with a +plain `op://` value open as a 1Password source that still needs its account; pick it and the +variable resolves again. A sensitive `op://` value is hidden from the app, so it cannot be +recognized; add it again with 1Password as its source. To copy a reference in 1Password, open the item, use the field's overflow menu, and choose **Copy Secret Reference**. @@ -35,7 +46,7 @@ running the T3 Code server. Confirm it works from a normal shell first: ```bash -op read --no-newline "op://Private/claude-code/credential" +op read --account my.1password.com --no-newline "op://Private/claude-code/credential" ``` If that command prints your secret, T3 Code can read it too. If it asks you to sign in, sign in @@ -46,10 +57,8 @@ The vault has to be reachable from wherever `npx t3` or the desktop app is actua ## Do I Still Mark It Sensitive -You do not need to. A reference is not a secret, so there is nothing to protect by storing it as one. - -Marking it sensitive still works if you prefer the redacted field in the UI, and the reference is -resolved the same way either way. +No. A reference is not a secret, so there is nothing to protect by storing it as one, and variables +that read from 1Password are always stored as written. ## How Often Does It Ask Me To Unlock @@ -60,8 +69,9 @@ sending a message, and the background provider status check all reuse the value read, so a locked vault prompts you once rather than every few minutes. One unlock covers every reference T3 Code needs, across every provider. Starting the server and -refreshing provider status both read the whole set in a single request to 1Password, so five -providers backed by references cost the same one approval that one provider does. +refreshing provider status both read the whole set in a single request per 1Password account, so +five providers backed by references in one account cost the same one approval that one provider +does. ## I Rotated The Secret, How Do I Pick Up The New One @@ -70,6 +80,10 @@ Refresh provider status in Settings. Refresh drops everything that was held in memory and rebuilds the providers that use references, so the next read goes back to 1Password. Providers with only literal variables are left alone. +The 1Password CLI's own cache is never used, so a refresh cannot be answered with the old value. Skipping +it also means T3 Code never starts the CLI's background `op daemon`, which can hang behind a macOS +permission prompt when the server runs in the background. + Threads that are already running keep the process they were given. Work started after the refresh uses the new value. @@ -91,4 +105,4 @@ The server log records which reference failed and what the 1Password CLI said ab ## Can I Use A Different Password Manager -Not yet. `op://` references are the only form T3 Code resolves today. +Not yet. 1Password is the only secret store T3 Code reads from today. diff --git a/packages/client-runtime/src/state/server.ts b/packages/client-runtime/src/state/server.ts index a5077aa74f39..ee6ae1fe8afd 100644 --- a/packages/client-runtime/src/state/server.ts +++ b/packages/client-runtime/src/state/server.ts @@ -1055,6 +1055,10 @@ export function createServerEnvironmentAtoms( label: "environment-data:server:trace-diagnostics", tag: WS_METHODS.serverGetTraceDiagnostics, }), + onePasswordAccounts: createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:server:one-password-accounts", + tag: WS_METHODS.serverListOnePasswordAccounts, + }), processDiagnostics: createEnvironmentRpcQueryAtomFamily(runtime, { label: "environment-data:server:process-diagnostics", tag: WS_METHODS.serverGetProcessDiagnostics, diff --git a/packages/contracts/src/index.ts b/packages/contracts/src/index.ts index 8690bb1b2390..25369301ef89 100644 --- a/packages/contracts/src/index.ts +++ b/packages/contracts/src/index.ts @@ -14,6 +14,7 @@ export * from "./remoteAccess.ts"; export * from "./ipc.ts"; export * from "./terminal.ts"; export * from "./provider.ts"; +export * from "./onePassword.ts"; export * from "./providerInstance.ts"; export * from "./providerSetup.ts"; export * from "./providerRuntime.ts"; diff --git a/packages/contracts/src/onePassword.test.ts b/packages/contracts/src/onePassword.test.ts new file mode 100644 index 000000000000..f1f8cff9e550 --- /dev/null +++ b/packages/contracts/src/onePassword.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vite-plus/test"; +import * as Schema from "effect/Schema"; + +import { OnePasswordSecretSource } from "./onePassword.ts"; + +const decodeSource = Schema.decodeUnknownSync(OnePasswordSecretSource); + +describe("OnePasswordSecretSource", () => { + const onePassword = (reference: string, account = "my.1password.com") => ({ + kind: "1password", + reference, + account, + }); + + it.each([ + "op://Private/claude-code/credential", + "op://Home Lab/Claude Code/API Key", + "op://Private/claude-code/login section/password", + "op://Private/github/one-time password?attribute=otp", + ])("accepts the reference %s", (reference) => { + expect(decodeSource(onePassword(reference))).toEqual(onePassword(reference)); + }); + + it.each([ + ["leading whitespace", " op://Private/item/field"], + ["trailing newline", "op://Private/item/field\n"], + ["control character", "op://Private/it\u0007em/field"], + ["missing scheme", "Private/item/field"], + ["nothing after the scheme", "op://"], + ["template delimiter", "op://Private/item}}/field"], + ])("rejects a reference with %s", (_label, reference) => { + expect(() => decodeSource(onePassword(reference))).toThrow(); + }); + + it.each(["my", "my.1password.com", "team-acme.1password.eu", "ABCDEFGHIJKLMNOPQRSTUVWXYZ"])( + "accepts the account %s", + (account) => { + expect(decodeSource(onePassword("op://Private/item/field", account))).toMatchObject({ + account, + }); + }, + ); + + it.each([ + ["empty", ""], + ["a space", "my account"], + ["a leading dash", "--help"], + ["surrounding whitespace", " my "], + ])("rejects an account with %s", (_label, account) => { + expect(() => decodeSource(onePassword("op://Private/item/field", account))).toThrow(); + }); + + it("rejects a source without an account", () => { + expect(() => + decodeSource({ kind: "1password", reference: "op://Private/item/field" }), + ).toThrow(); + }); +}); diff --git a/packages/contracts/src/onePassword.ts b/packages/contracts/src/onePassword.ts new file mode 100644 index 000000000000..c3e957b93fdc --- /dev/null +++ b/packages/contracts/src/onePassword.ts @@ -0,0 +1,76 @@ +import * as Schema from "effect/Schema"; + +const ONE_PASSWORD_SECRET_REFERENCE_PREFIX = "op://"; +const ONE_PASSWORD_SECRET_REFERENCE_MAX_CHARS = 1024; +const ONE_PASSWORD_ACCOUNT_MAX_CHARS = 253; +const ONE_PASSWORD_ACCOUNT_PATTERN = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; + +function hasControlCharacter(value: string): boolean { + for (let index = 0; index < value.length; index++) { + const code = value.charCodeAt(index); + if (code < 0x20 || code === 0x7f) return true; + } + return false; +} + +function onePasswordSecretReferenceIssue(value: string): string | undefined { + if (value !== value.trim()) { + return "1Password secret reference must not start or end with whitespace."; + } + if (hasControlCharacter(value)) { + return "1Password secret reference must not contain control characters."; + } + if (!value.startsWith(ONE_PASSWORD_SECRET_REFERENCE_PREFIX)) { + return "1Password secret reference must start with op://."; + } + // The resolver embeds references in an `op inject` template, where `{{` and + // `}}` delimit a reference. + if (value.includes("{{") || value.includes("}}")) { + return "1Password secret reference must not contain {{ or }}."; + } + if (value.length === ONE_PASSWORD_SECRET_REFERENCE_PREFIX.length) { + return "1Password secret reference must name a vault, item, and field after op://."; + } + return undefined; +} + +/** + * A 1Password secret reference such as `op://vault/item/field`. Only what + * T3 Code itself depends on is checked here; `op` validates the rest and + * reports which part it could not resolve. + */ +export const OnePasswordSecretReference = Schema.String.check( + Schema.isMaxLength(ONE_PASSWORD_SECRET_REFERENCE_MAX_CHARS), + Schema.makeFilter((value: string) => onePasswordSecretReferenceIssue(value) ?? true), +).pipe(Schema.brand("OnePasswordSecretReference")); +export type OnePasswordSecretReference = typeof OnePasswordSecretReference.Type; + +/** + * The 1Password account a reference is read from, in any form `op --account` + * accepts: account shorthand, sign-in address (`my.1password.com`), account ID, + * or user ID. It is passed to the CLI as an argument, so it can never start + * with `-`. + */ +export const OnePasswordAccount = Schema.String.check( + Schema.isMaxLength(ONE_PASSWORD_ACCOUNT_MAX_CHARS), + Schema.makeFilter((value: string) => + ONE_PASSWORD_ACCOUNT_PATTERN.test(value) + ? true + : "1Password account must be a shorthand, sign-in address, or ID with no spaces.", + ), +).pipe(Schema.brand("OnePasswordAccount")); +export type OnePasswordAccount = typeof OnePasswordAccount.Type; + +export const OnePasswordSecretSource = Schema.Struct({ + kind: Schema.Literal("1password"), + reference: OnePasswordSecretReference, + account: OnePasswordAccount, +}); +export type OnePasswordSecretSource = typeof OnePasswordSecretSource.Type; + +/** A 1Password account signed in on the server, as `op account list` reports it. */ +export const OnePasswordAccountSummary = Schema.Struct({ + account: OnePasswordAccount, + email: Schema.String, +}); +export type OnePasswordAccountSummary = typeof OnePasswordAccountSummary.Type; diff --git a/packages/contracts/src/providerInstance.test.ts b/packages/contracts/src/providerInstance.test.ts index 1645eae48259..441bc902d23f 100644 --- a/packages/contracts/src/providerInstance.test.ts +++ b/packages/contracts/src/providerInstance.test.ts @@ -6,6 +6,7 @@ import { ProviderInstanceConfig, ProviderInstanceConfigMap, ProviderInstanceId, + ProviderInstanceEnvironmentVariable, ProviderInstanceRef, } from "./providerInstance.ts"; @@ -14,6 +15,7 @@ const decodeProviderInstanceId = Schema.decodeUnknownSync(ProviderInstanceId); const decodeProviderInstanceRef = Schema.decodeUnknownSync(ProviderInstanceRef); const decodeProviderInstanceConfig = Schema.decodeUnknownSync(ProviderInstanceConfig); const decodeProviderInstanceConfigMap = Schema.decodeUnknownSync(ProviderInstanceConfigMap); +const decodeEnvironmentVariable = Schema.decodeUnknownSync(ProviderInstanceEnvironmentVariable); describe("provider slug validation (shared by driver + instance ids)", () => { const cases = [ @@ -206,3 +208,17 @@ describe("ProviderInstanceConfigMap", () => { ).toThrow(); }); }); + +describe("ProviderInstanceEnvironmentVariable secret sources", () => { + it("decodes a 1Password source", () => { + const value = { kind: "1password", reference: "op://Private/item/field", account: "my" }; + expect(decodeEnvironmentVariable({ name: "API_KEY", value }).value).toEqual(value); + }); + + it("decodes a string value as a literal, even one that looks like a reference", () => { + expect(decodeEnvironmentVariable({ name: "API_KEY" }).value).toBe(""); + expect( + decodeEnvironmentVariable({ name: "API_KEY", value: "op://Private/item/field" }).value, + ).toBe("op://Private/item/field"); + }); +}); diff --git a/packages/contracts/src/providerInstance.ts b/packages/contracts/src/providerInstance.ts index c47bf992a4f4..49a416510f7d 100644 --- a/packages/contracts/src/providerInstance.ts +++ b/packages/contracts/src/providerInstance.ts @@ -36,6 +36,7 @@ import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; import { TrimmedNonEmptyString } from "./baseSchemas.ts"; +import { OnePasswordSecretSource } from "./onePassword.ts"; const PROVIDER_SLUG_MAX_CHARS = 64; /** @@ -101,9 +102,22 @@ export const ProviderInstanceEnvironmentVariableName = TrimmedNonEmptyString.che export type ProviderInstanceEnvironmentVariableName = typeof ProviderInstanceEnvironmentVariableName.Type; +/** + * An environment value read from an external secret store at the moment the + * provider process starts, instead of a literal. Discriminated on `kind`. + */ +export const ProviderSecretSource = Schema.Union([OnePasswordSecretSource]); +export type ProviderSecretSource = typeof ProviderSecretSource.Type; + +/** + * `sensitive` and `valueRedacted` only describe literal string values; a + * secret source is not itself a secret and is stored as written. + */ export const ProviderInstanceEnvironmentVariable = Schema.Struct({ name: ProviderInstanceEnvironmentVariableName, - value: Schema.String.pipe(Schema.withDecodingDefault(Effect.succeed(""))), + value: Schema.Union([Schema.String, ProviderSecretSource]).pipe( + Schema.withDecodingDefault(Effect.succeed("")), + ), sensitive: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(false))), valueRedacted: Schema.optionalKey(Schema.Boolean), }); diff --git a/packages/contracts/src/rpc.ts b/packages/contracts/src/rpc.ts index ed6df13a6947..253e3130f1c3 100644 --- a/packages/contracts/src/rpc.ts +++ b/packages/contracts/src/rpc.ts @@ -10,6 +10,7 @@ import * as Schema from "effect/Schema"; import * as Rpc from "effect/unstable/rpc/Rpc"; import * as RpcGroup from "effect/unstable/rpc/RpcGroup"; import { NonNegativeInt, TrimmedNonEmptyString } from "./baseSchemas.ts"; +import { OnePasswordAccountSummary } from "./onePassword.ts"; import { CodexAuthCallbackInput, CodexAuthCallbackState, @@ -442,6 +443,7 @@ export const WS_METHODS = { serverGetSettings: "server.getSettings", serverUpdateSettings: "server.updateSettings", serverDiscoverSourceControl: "server.discoverSourceControl", + serverListOnePasswordAccounts: "server.listOnePasswordAccounts", serverSearchAcpRegistry: "server.searchAcpRegistry", serverPrepareAcpRegistryAgent: "server.prepareAcpRegistryAgent", serverUninstallAcpRegistryManagedBinary: "server.uninstallAcpRegistryManagedBinary", @@ -718,6 +720,12 @@ const WsServerDiscoverSourceControlRpc = Rpc.make(WS_METHODS.serverDiscoverSourc error: EnvironmentAuthorizationError, }); +const WsServerListOnePasswordAccountsRpc = Rpc.make(WS_METHODS.serverListOnePasswordAccounts, { + payload: Schema.Struct({}), + success: Schema.Array(OnePasswordAccountSummary), + error: EnvironmentAuthorizationError, +}); + const WsServerSearchAcpRegistryRpc = Rpc.make(WS_METHODS.serverSearchAcpRegistry, { payload: AcpRegistrySearchInput, success: AcpRegistrySearchResult, @@ -1710,6 +1718,7 @@ export const WsRpcGroup = RpcGroup.make( WsServerGetSettingsRpc, WsServerUpdateSettingsRpc, WsServerDiscoverSourceControlRpc, + WsServerListOnePasswordAccountsRpc, WsServerSearchAcpRegistryRpc, WsServerPrepareAcpRegistryAgentRpc, WsServerUninstallAcpRegistryManagedBinaryRpc,