From 01753c8cd96daacc050a3c574dfe5bb89aa8caa0 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 20:27:58 -0400 Subject: [PATCH] fix(server): the batched 1Password read reaches op at all op only accepts piped input from a named pipe, and a child spawned from Node is handed a socket pair, so the batch template was never seen and every fleet fell back to one authorization per provider. Signed-off-by: Yordis Prieto --- .../Layers/ProviderSecretResolverLive.test.ts | 164 ++++++++++++++++-- .../Layers/ProviderSecretResolverLive.ts | 34 +++- 2 files changed, 171 insertions(+), 27 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts index 7f818a492fe3..4fd8afa4399c 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -6,6 +6,8 @@ import * as Schema from "effect/Schema"; import * as Sink from "effect/Sink"; import * as Stream from "effect/Stream"; import { ChildProcessSpawner } from "effect/unstable/process"; +import * as NodeFileSystem from "@effect/platform-node/NodeFileSystem"; +import * as NodeFS from "node:fs"; import { ProviderSecretResolverLive } from "./ProviderSecretResolverLive.ts"; import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; @@ -60,7 +62,13 @@ describe("ProviderSecretResolverLive", () => { assert.deepStrictEqual(resolved, { variables: environment, unresolved: [] }); assert.strictEqual(spawner.invocations.length, 0); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("swaps a secret reference for the value 1Password returns", () => { @@ -83,7 +91,13 @@ describe("ProviderSecretResolverLive", () => { ], ); assert.deepStrictEqual(spawner.invocations, [["read", "--no-newline", TOKEN_REFERENCE]]); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("reads a reference once and holds it until the caller invalidates", () => { @@ -103,7 +117,13 @@ describe("ProviderSecretResolverLive", () => { yield* resolver.invalidate; yield* resolver.resolve(environment); assert.strictEqual(spawner.invocations.length, 2); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("drops a variable whose reference cannot be read", () => { @@ -133,7 +153,13 @@ describe("ProviderSecretResolverLive", () => { // leaving it out would hand the provider whatever the server itself was // started with under that name. assert.deepStrictEqual(resolved.unresolved, ["CLAUDE_CODE_OAUTH_TOKEN"]); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("holds a failed read too, so a locked vault prompts once", () => { @@ -148,7 +174,13 @@ describe("ProviderSecretResolverLive", () => { yield* resolver.resolve(environment); assert.strictEqual(spawner.invocations.length, 1); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); }); @@ -156,7 +188,12 @@ const SECOND_REFERENCE = "op://Private/codex/credential"; /** * Spawner that answers each `op` invocation from `handler`, which is handed the - * argv and, for `op inject`, the template that was piped to stdin. + * argv and, for `op inject`, the template `op` would have read. + * + * The template is read back off disk through the `-i` path in the argv, which + * is how `op` itself receives it. Reading it any other way would let a change + * that stops writing the file pass, and that is the shape of the bug this + * spawner exists to catch. * * The template matters: `prime` picks a random separator per call, so a test * cannot hard-code the output. Recovering the separator from the template is @@ -169,20 +206,22 @@ function scriptedOpSpawner( ) => { stdout: string; stderr: string; code: number }, ) { const invocations: Array> = []; + const stdinUses: Array = []; const layer = Layer.succeed( ChildProcessSpawner.ChildProcessSpawner, ChildProcessSpawner.make((command) => - Effect.gen(function* () { + Effect.sync(() => { const cmd = command as unknown as { args: ReadonlyArray; options?: { stdin?: Stream.Stream }; }; invocations.push(cmd.args); - const stdin = cmd.options?.stdin; - const chunks = stdin === undefined ? [] : yield* Stream.runCollect(stdin); - const template = Array.from(chunks) - .map((chunk) => new TextDecoder().decode(chunk)) - .join(""); + stdinUses.push(cmd.options?.stdin !== undefined); + const inputPath = cmd.args[cmd.args.indexOf("-i") + 1]; + const template = + cmd.args.includes("-i") && inputPath !== undefined + ? NodeFS.readFileSync(inputPath, "utf8") + : ""; const result = handler(cmd.args, template); return ChildProcessSpawner.makeHandle({ pid: ChildProcessSpawner.ProcessId(1), @@ -200,7 +239,7 @@ function scriptedOpSpawner( }), ), ); - return { layer, invocations }; + return { layer, invocations, stdinUses }; } /** The separator `prime` chose, read back out of the template it built. */ @@ -227,7 +266,10 @@ describe("ProviderSecretResolverLive.prime", () => { yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); assert.strictEqual(spawner.invocations.length, 1); - assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []), ["inject"]); + assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []).slice(0, 2), [ + "inject", + "-i", + ]); // Both instances resolve out of the primed cache, so the fleet costs the // one authorization the batch already paid for. @@ -243,7 +285,13 @@ describe("ProviderSecretResolverLive.prime", () => { assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); assert.strictEqual(codex.variables?.[0]?.value, "sk-codex-token"); assert.strictEqual(spawner.invocations.length, 1); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("falls back to one read at a time when the batch fails", () => { @@ -261,7 +309,7 @@ describe("ProviderSecretResolverLive.prime", () => { yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); // The batch is still attempted; it is the recovery that is per reference. - assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []), ["inject"]); + assert.strictEqual(spawner.invocations[0]?.[0], "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 @@ -278,7 +326,81 @@ describe("ProviderSecretResolverLive.prime", () => { assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); assert.deepStrictEqual(Array.from(claude.unresolved), []); assert.deepStrictEqual(Array.from(codex.unresolved), ["CODEX_TOKEN"]); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); + }); + + it.effect("hands the template to a file `op` will actually read", () => { + let seenTemplate = ""; + const spawner = scriptedOpSpawner((args, template) => { + if (args.includes("inject")) { + seenTemplate = template; + return { + stdout: ["sk-claude-token", "sk-codex-token"].join(separatorOf(template)), + stderr: "", + code: 0, + }; + } + return { stdout: "should-not-be-read-one-at-a-time", stderr: "", code: 0 }; + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + + // `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(spawner.stdinUses, [false]); + + // The file `op` was pointed at held both references and nothing else, + // so a secret never reaches the disk. + assert.isTrue(seenTemplate.includes(TOKEN_REFERENCE)); + assert.isTrue(seenTemplate.includes(SECOND_REFERENCE)); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); + }); + + it.effect("leaves no template behind once the batch is done", () => { + let templatePath = ""; + const spawner = scriptedOpSpawner((args, template) => { + if (args.includes("inject")) { + templatePath = args[args.indexOf("-i") + 1] ?? ""; + return { + stdout: ["sk-claude-token", "sk-codex-token"].join(separatorOf(template)), + stderr: "", + code: 0, + }; + } + return { stdout: "", stderr: "", code: 0 }; + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + + assert.isTrue(templatePath.length > 0); + assert.isFalse(NodeFS.existsSync(templatePath)); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); it.effect("does not spawn a batch for a single reference", () => { @@ -291,6 +413,12 @@ describe("ProviderSecretResolverLive.prime", () => { // One reference is one prompt either way, and `op read` names the // reference it could not resolve. assert.strictEqual(spawner.invocations.length, 0); - }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }).pipe( + Effect.provide( + ProviderSecretResolverLive.pipe( + Layer.provide(Layer.merge(spawner.layer, NodeFileSystem.layer)), + ), + ), + ); }); }); diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts index 548ee6658a67..8c5f7fa9fb4b 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -29,8 +29,8 @@ 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 Stream from "effect/Stream"; import * as NodeCrypto from "node:crypto"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; @@ -72,18 +72,31 @@ const SECRET_CACHE_CAPACITY = 64; * it. The caller treats that as "not primed" and reads one at a time, which is * both the per-variable failure isolation and the way the user finds out which * reference is the broken one. + * + * The template goes to `op` through a scoped temp file rather than stdin. + * `op` only reads piped input from a named pipe, and a child spawned from Node + * is handed a socket pair, so a piped template is never seen and the batch + * fails with "expected data on stdin but none found" every single time. The + * file holds only `op://` references, which already sit in settings in plain + * text; the secrets themselves come back on stdout and never touch disk. */ const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( references: ReadonlyArray, ) { + const fileSystem = yield* FileSystem.FileSystem; const separator = `__t3-secret-${NodeCrypto.randomUUID()}__`; const template = references.map((reference) => `{{ ${reference} }}`).join(separator); - const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, ["inject"]); + const templatePath = yield* fileSystem.makeTempFileScoped({ prefix: "t3code-op-inject-" }); + yield* fileSystem.writeFileString(templatePath, template); + const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, [ + "inject", + "-i", + templatePath, + ]); const result = yield* spawnAndCollect( ONE_PASSWORD_BINARY, ChildProcess.make(spawnCommand.command, spawnCommand.args, { shell: spawnCommand.shell, - stdin: Stream.make(new TextEncoder().encode(template)), }), ); if (result.code !== 0) { @@ -137,14 +150,16 @@ const readSecret = Effect.fn("readSecret")(function* (reference: string) { export const ProviderSecretResolverLive: Layer.Layer< ProviderSecretResolver, never, - ChildProcessSpawner.ChildProcessSpawner + ChildProcessSpawner.ChildProcessSpawner | FileSystem.FileSystem > = Layer.effect( ProviderSecretResolver, Effect.gen(function* () { - // The service tag declares `prime` as `Effect`, so the spawner it - // needs is captured here rather than asked of the caller, the same way - // the cache's own lookup captures it. - const spawnerContext = yield* Effect.context(); + // The service tag declares `prime` as `Effect`, so the spawner and + // the filesystem it needs are captured here rather than asked of the + // caller, the same way the cache's own lookup captures the spawner. + const primeContext = yield* Effect.context< + ChildProcessSpawner.ChildProcessSpawner | FileSystem.FileSystem + >(); const cache = yield* Cache.make({ capacity: SECRET_CACHE_CAPACITY, @@ -201,6 +216,7 @@ export const ProviderSecretResolverLive: Layer.Layer< return; } const values = yield* readSecretsTogether(wanted).pipe( + Effect.scoped, Effect.timeoutOption(SECRET_READ_TIMEOUT), Effect.map(Option.getOrUndefined), Effect.catch((error) => @@ -220,7 +236,7 @@ export const ProviderSecretResolverLive: Layer.Layer< discard: true, }, ); - }).pipe(Effect.provideContext(spawnerContext)); + }).pipe(Effect.provideContext(primeContext)); return { resolve, prime, invalidate: Cache.invalidateAll(cache) }; }),