fix(server): the batched 1Password read reaches op at all (#30)

Author: yordis

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,15 +174,26 @@ 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)),
+        ),
+      ),
+    );
   });
 });
 
 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<ReadonlyArray<string>> = [];
+  const stdinUses: Array<boolean> = [];
   const layer = Layer.succeed(
     ChildProcessSpawner.ChildProcessSpawner,
     ChildProcessSpawner.make((command) =>
-      Effect.gen(function* () {
+      Effect.sync(() => {
         const cmd = command as unknown as {
           args: ReadonlyArray<string>;
           options?: { stdin?: Stream.Stream<Uint8Array> };
         };
         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)),
+        ),
+      ),
+    );
   });
 });

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<string>,
 ) {
+  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<void>`, 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<ChildProcessSpawner.ChildProcessSpawner>();
+    // The service tag declares `prime` as `Effect<void>`, 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) };
   }),