From 88fa17560146be6742aefe558534d090547372fa Mon Sep 17 00:00:00 2001 From: Albert Hazan Date: Wed, 15 Jul 2026 21:37:47 -0400 Subject: [PATCH 1/2] feat(tools): add op_run and op_check_ref for using secrets without revealing plaintext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit op_run executes a local command with op://vault/item/field references resolved into environment variables. The resolved plaintext is injected into the child process env only, redacted from all returned stdout/stderr and error messages, and never logged — the MCP equivalent of `op run`. This lets an agent USE a secret in a command/API call without ever placing the plaintext in the model context or transcript. op_check_ref validates an op:// reference and returns only non-secret metadata (vault, item, field label/type) confirming it resolves, so a reference can be checked before use without revealing the value. Both accept an optional OP_MCP_ALLOWED_VAULTS / --allowed-vaults allow-list; when unset (the default) any accessible vault is permitted, matching existing tool behavior. Adds a shared secret-ref parser and 19 tests (build + 98 tests green). Co-Authored-By: Claude Opus 4.8 --- README.md | 19 ++- src/config.ts | 22 ++++ src/secret-ref.ts | 54 ++++++++ src/tools/index.ts | 4 + src/tools/op-check-ref.ts | 84 +++++++++++++ src/tools/op-run.ts | 252 +++++++++++++++++++++++++++++++++++++ tests/op-check-ref.test.ts | 160 +++++++++++++++++++++++ tests/op-run.test.ts | 181 ++++++++++++++++++++++++++ tests/secret-ref.test.ts | 74 +++++++++++ tests/tools.test.ts | 6 +- 10 files changed, 853 insertions(+), 3 deletions(-) create mode 100644 src/secret-ref.ts create mode 100644 src/tools/op-check-ref.ts create mode 100644 src/tools/op-run.ts create mode 100644 tests/op-check-ref.test.ts create mode 100644 tests/op-run.test.ts create mode 100644 tests/secret-ref.test.ts diff --git a/README.md b/README.md index ae68fbb..a59b471 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ A community-built [Model Context Protocol (MCP)](https://modelcontextprotocol.io ## Features -### Tools (13) +### Tools (15) | Tool | Description | |------|-------------| @@ -29,6 +29,8 @@ A community-built [Model Context Protocol (MCP)](https://modelcontextprotocol.io | `password_update` | Rotate/update an existing password | | `password_generate` | Generate a cryptographically secure random password | | `password_generate_memorable` | Generate a memorable passphrase from ~500 dictionary words | +| `op_run` | Run a local command with `op://` references injected as env vars; resolved secret plaintext is redacted from stdout/stderr and never logged (the MCP equivalent of `op run`) | +| `op_check_ref` | Validate an `op://vault/item/field` reference and return only non-secret metadata (vault, item, field) confirming it resolves — never the value | ### Prompts (4) @@ -121,6 +123,20 @@ Then set `OP_SERVICE_ACCOUNT_TOKEN` in your shell/session/CI environment. On macOS, you can also omit `OP_SERVICE_ACCOUNT_TOKEN` and set `OP_KEYCHAIN_SERVICE` (plus optional `OP_KEYCHAIN_ACCOUNT`) to read the token from Keychain at startup. +#### Restricting `op_run` / `op_check_ref` to specific vaults (optional) + +By default, `op_run` and `op_check_ref` may resolve `op://` references from any +vault the service account can access. To restrict them to an allow-list, set +`OP_MCP_ALLOWED_VAULTS` (or pass `--allowed-vaults`) to a comma-separated list of +vault names or IDs. References to any other vault are rejected before resolution. + +```jsonc +"env": { + "OP_SERVICE_ACCOUNT_TOKEN": "YOUR_SERVICE_ACCOUNT_TOKEN", + "OP_MCP_ALLOWED_VAULTS": "Automation, CI" +} +``` + ### CLI Options ``` @@ -128,6 +144,7 @@ On macOS, you can also omit `OP_SERVICE_ACCOUNT_TOKEN` and set `OP_KEYCHAIN_SERV --log-level Log level: error, warn, info, debug (default: info) --integration-name Custom integration name for 1Password SDK --integration-version Custom integration version +--allowed-vaults Comma-separated vault allow-list for op_run/op_check_ref (default: unrestricted) ``` --- diff --git a/src/config.ts b/src/config.ts index 9de1bc1..a718342 100644 --- a/src/config.ts +++ b/src/config.ts @@ -33,6 +33,23 @@ export interface ServerConfig { serviceAccountToken: string | undefined; /** Where the token came from. */ tokenSource: "args" | "env" | "keychain" | "missing"; + /** + * Vault names/IDs that `op_run`/`op_check_ref` may resolve `op://` references + * from. An empty list means no restriction (any accessible vault is allowed). + */ + allowedVaults: string[]; +} + +/** + * Parse a comma-separated vault allow-list. An unset/blank value yields an + * empty list, which the reference resolvers treat as "no restriction". + */ +export function parseAllowedVaults(raw: string | undefined): string[] { + if (!raw || !raw.trim()) return []; + return raw + .split(",") + .map((entry) => entry.trim()) + .filter(Boolean); } let _config: ServerConfig | undefined; @@ -129,6 +146,10 @@ export function getConfig(): ServerConfig { const { serviceAccountToken, tokenSource } = resolveServiceAccountToken({ tokenFromArgs }); + const allowedVaults = parseAllowedVaults( + getArgValue("allowed-vaults") ?? process.env.OP_MCP_ALLOWED_VAULTS, + ); + _config = { logLevel: logLevelRaw, logLevelValue, @@ -136,6 +157,7 @@ export function getConfig(): ServerConfig { integrationVersion, serviceAccountToken, tokenSource, + allowedVaults, }; return _config; diff --git a/src/secret-ref.ts b/src/secret-ref.ts new file mode 100644 index 0000000..877a8cc --- /dev/null +++ b/src/secret-ref.ts @@ -0,0 +1,54 @@ +/** + * Shared helpers for parsing and validating `op://vault/item/field` secret + * references, used by `op_run` and `op_check_ref`. + */ + +import { getConfig } from "./config.js"; + +export interface ParsedSecretRef { + /** Raw reference string, e.g. "op://Private/github/token". */ + raw: string; + /** Vault segment (name or ID) as written in the reference. */ + vault: string; + /** Item segment (name or ID) as written in the reference. */ + item: string; + /** Field segment (name or ID) as written in the reference. */ + field: string; +} + +const OP_REF_PATTERN = /^op:\/\/([^/]+)\/([^/]+)\/(.+)$/; + +/** True if the given string looks like an `op://` secret reference. */ +export function isSecretRef(value: string): boolean { + return OP_REF_PATTERN.test(value); +} + +/** Parse an `op://vault/item/field` reference into its segments. */ +export function parseSecretRef(raw: string): ParsedSecretRef { + const match = OP_REF_PATTERN.exec(raw); + if (!match) { + throw new Error( + `Invalid secret reference '${raw}'. Expected format: op://vault/item/field`, + ); + } + const [, vault, item, field] = match; + return { raw, vault, item, field }; +} + +/** + * Throw if the reference's vault segment is not in the server's configured + * vault allow-list. An empty allow-list (the default) permits any vault. + * Comparison is case-insensitive against vault names/IDs. + */ +export function assertVaultAllowed(vault: string): void { + const { allowedVaults } = getConfig(); + if (allowedVaults.length === 0) return; // no restriction configured + const normalized = vault.toLowerCase(); + const allowed = allowedVaults.some((entry) => entry.toLowerCase() === normalized); + if (!allowed) { + throw new Error( + `Vault '${vault}' is not in the allowed vault list (${allowedVaults.join(", ")}). ` + + "Configure OP_MCP_ALLOWED_VAULTS or --allowed-vaults to permit it.", + ); + } +} diff --git a/src/tools/index.ts b/src/tools/index.ts index 4cc0f1f..1fdc1ae 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -16,6 +16,8 @@ import { registerPasswordRead } from "./password-read.js"; import { registerPasswordUpdate } from "./password-update.js"; import { registerPasswordGenerate } from "./password-generate.js"; import { registerPasswordGenerateMemorable } from "./password-generate-memorable.js"; +import { registerOpRun } from "./op-run.js"; +import { registerOpCheckRef } from "./op-check-ref.js"; /** Register every tool with the MCP server. */ export function registerAllTools(server: McpServer): void { @@ -32,4 +34,6 @@ export function registerAllTools(server: McpServer): void { registerPasswordUpdate(server); registerPasswordGenerate(server); registerPasswordGenerateMemorable(server); + registerOpRun(server); + registerOpCheckRef(server); } diff --git a/src/tools/op-check-ref.ts b/src/tools/op-check-ref.ts new file mode 100644 index 0000000..3bf484f --- /dev/null +++ b/src/tools/op-check-ref.ts @@ -0,0 +1,84 @@ +/** + * op_check_ref — Validate an op://vault/item/field secret reference and + * return metadata about it (vault, item, field label/type) WITHOUT ever + * returning the secret value. Use this to sanity-check a reference before + * passing it to op_run. + */ + +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { getClient } from "../client.js"; +import { log, logError } from "../logger.js"; +import { jsonResult, errorResult } from "../utils.js"; +import { parseSecretRef, assertVaultAllowed } from "../secret-ref.js"; + +export function registerOpCheckRef(server: McpServer): void { + server.tool( + "op_check_ref", + "Validate an op://vault/item/field secret reference and return only non-secret metadata (vault name, item title, field label, field type) confirming it resolves — the field VALUE is never returned. Use this to check a reference is correct before using it with op_run; do not use password_read/item_get with reveal just to check a reference exists.", + { + secretReference: z + .string() + .min(1) + .describe("Secret reference to validate, in op://vault/item/field format."), + }, + async ({ secretReference }) => { + try { + log("debug", "Tool call: op_check_ref.", { secretReference: true }); + + const ref = parseSecretRef(secretReference); + assertVaultAllowed(ref.vault); + + const client = await getClient(); + if (!client?.secrets?.resolveAll || !client?.items?.get) { + throw new Error( + "Your @1password/sdk version does not support resolving secret references.", + ); + } + + const resolved = await client.secrets.resolveAll([secretReference]); + const response = resolved.individualResponses[secretReference]; + if (!response?.content) { + const reason = response?.error?.type ?? "unknown"; + throw new Error( + `Could not resolve secret reference '${secretReference}' (${reason}).`, + ); + } + + const { vaultId, itemId } = response.content; + const item: any = await client.items.get(vaultId, itemId); + + const desiredField = ref.field.toLowerCase(); + const fields: any[] = item.fields ?? []; + const match = fields.find((candidate: any) => { + const idMatch = candidate.id?.toLowerCase() === desiredField; + const titleMatch = candidate.title?.toLowerCase() === desiredField; + const labelMatch = candidate.label?.toLowerCase() === desiredField; + return idMatch || titleMatch || labelMatch; + }); + + if (!match) { + throw new Error( + `Field '${ref.field}' not found on item '${item.title}'.`, + ); + } + + // Deliberately omit match.value — this tool never returns secret plaintext. + return jsonResult({ + resolved: true, + vault: { id: vaultId, name: ref.vault }, + item: { id: item.id, title: item.title, category: item.category }, + field: { + id: match.id, + title: match.title, + type: match.fieldType ?? match.type, + section: match.sectionId, + }, + }); + } catch (error) { + logError("op_check_ref failed.", error); + return errorResult(error); + } + }, + ); +} diff --git a/src/tools/op-run.ts b/src/tools/op-run.ts new file mode 100644 index 0000000..d0970dd --- /dev/null +++ b/src/tools/op-run.ts @@ -0,0 +1,252 @@ +/** + * op_run — Execute a local command with 1Password secrets injected as + * environment variables, without ever returning secret plaintext to the + * caller. This is the MCP equivalent of `op run -- `. + */ + +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { spawn } from "node:child_process"; +import { z } from "zod"; +import { getClient } from "../client.js"; +import { log, logError } from "../logger.js"; +import { jsonResult, errorResult } from "../utils.js"; +import { isSecretRef, parseSecretRef, assertVaultAllowed } from "../secret-ref.js"; + +const MAX_OUTPUT_BYTES = 5 * 1024 * 1024; // 5 MiB safety cap per stream +const DEFAULT_TIMEOUT_MS = 120_000; + +interface ResolvedEnvEntry { + name: string; + value: string; + /** True if this env var came from an op:// reference and must be redacted from output. */ + secret: boolean; +} + +/** Resolve the `env` map into plaintext values, keeping track of which ones are secrets. */ +async function resolveEnvEntries( + env: Record | undefined, +): Promise { + if (!env) return []; + const client = await getClient(); + if (!client?.secrets?.resolve) { + throw new Error( + "Your @1password/sdk version does not support resolving secrets.", + ); + } + + const entries: ResolvedEnvEntry[] = []; + for (const [name, rawValue] of Object.entries(env)) { + if (isSecretRef(rawValue)) { + const ref = parseSecretRef(rawValue); + assertVaultAllowed(ref.vault); + const value = await client.secrets.resolve(rawValue); + entries.push({ name, value, secret: true }); + } else { + entries.push({ name, value: rawValue, secret: false }); + } + } + return entries; +} + +/** Replace every occurrence of every secret value with a redaction marker. */ +function redact(text: string, secrets: ResolvedEnvEntry[]): string { + let redacted = text; + for (const entry of secrets) { + if (!entry.secret || entry.value.length === 0) continue; + // split/join instead of a RegExp so secret values with special + // characters ($, *, (, etc.) are matched literally. + redacted = redacted.split(entry.value).join(`«REDACTED:${entry.name}»`); + } + return redacted; +} + +function truncate(buffer: Buffer): { text: string; truncated: boolean } { + if (buffer.length <= MAX_OUTPUT_BYTES) { + return { text: buffer.toString("utf8"), truncated: false }; + } + return { + text: buffer.subarray(0, MAX_OUTPUT_BYTES).toString("utf8"), + truncated: true, + }; +} + +export function registerOpRun(server: McpServer): void { + server.tool( + "op_run", + "Run a local shell command with 1Password secrets injected as environment variables — plaintext secret values are NEVER returned to the caller or written to any log; every resolved secret value is redacted from stdout/stderr before the result is returned. This is the safe way to USE a secret in a command, API call, or script: prefer op_run with op://vault/item/field references in `env` over reading a secret with password_read/item_get and pasting it into a command yourself, which would put the plaintext in the model context and transcript.", + { + command: z + .string() + .optional() + .describe( + "Shell command line to execute (run via the platform shell). Provide either `command` or `argv`, not both.", + ), + argv: z + .array(z.string()) + .optional() + .describe( + "Argument vector [executable, ...args] to execute directly without a shell (safer against quoting/injection issues). Provide either `command` or `argv`, not both.", + ), + cwd: z + .string() + .optional() + .describe("Working directory to run the command in, e.g. a repo checkout path."), + env: z + .record(z.string(), z.string()) + .optional() + .describe( + "Map of ENV_VAR_NAME -> value. A value matching op://vault/item/field is resolved via 1Password and injected as plaintext into the child process env only; any other value is passed through unchanged as a literal.", + ), + shell: z + .string() + .optional() + .describe( + "Optional shell executable to use with `command` (e.g. 'powershell.exe', 'C:/Program Files/Git/bin/bash.exe', '/bin/bash'). Defaults to the platform shell (cmd.exe on Windows, /bin/sh elsewhere). Ignored when `argv` is used.", + ), + timeout_ms: z + .number() + .int() + .positive() + .max(600_000) + .optional() + .describe("Kill the process if it runs longer than this many milliseconds. Default 120000 (2 minutes)."), + stdin: z + .string() + .optional() + .describe("Optional text to write to the process's stdin."), + }, + async ({ command, argv, cwd, env, shell, timeout_ms, stdin }) => { + const startedAt = Date.now(); + let resolvedEnv: ResolvedEnvEntry[] = []; + try { + log("debug", "Tool call: op_run.", { + hasCommand: Boolean(command), + hasArgv: Boolean(argv), + cwd, + envKeys: env ? Object.keys(env) : [], + shell, + timeout_ms, + }); + + if (!command && !argv) { + throw new Error("Provide either `command` or `argv`."); + } + if (command && argv) { + throw new Error("Provide only one of `command` or `argv`, not both."); + } + if (argv && argv.length === 0) { + throw new Error("`argv` must contain at least the executable name."); + } + + resolvedEnv = await resolveEnvEntries(env); + const childEnv: NodeJS.ProcessEnv = { ...process.env }; + for (const entry of resolvedEnv) { + childEnv[entry.name] = entry.value; + } + + const timeout = timeout_ms ?? DEFAULT_TIMEOUT_MS; + + const result = await new Promise<{ + exitCode: number | null; + signal: NodeJS.Signals | null; + stdout: Buffer; + stderr: Buffer; + timedOut: boolean; + spawnError?: Error; + }>((resolve) => { + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let timedOut = false; + + const child = argv + ? spawn(argv[0], argv.slice(1), { + cwd, + env: childEnv, + shell: false, + timeout, + killSignal: "SIGTERM", + }) + : spawn(command as string, { + cwd, + env: childEnv, + shell: shell ?? true, + timeout, + killSignal: "SIGTERM", + }); + + child.on("error", (spawnError) => { + resolve({ + exitCode: null, + signal: null, + stdout: Buffer.concat(stdoutChunks), + stderr: Buffer.concat(stderrChunks), + timedOut, + spawnError, + }); + }); + + child.stdout?.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)); + child.stderr?.on("data", (chunk: Buffer) => stderrChunks.push(chunk)); + + if (stdin !== undefined && child.stdin) { + child.stdin.write(stdin); + } + child.stdin?.end(); + + child.on("close", (code, signal) => { + if (signal === "SIGTERM" || signal === "SIGKILL") { + // Node sets a timeout-triggered kill signal; heuristically + // treat it as a timeout when we hit/exceed the deadline. + timedOut = Date.now() - startedAt >= timeout; + } + resolve({ + exitCode: code, + signal, + stdout: Buffer.concat(stdoutChunks), + stderr: Buffer.concat(stderrChunks), + timedOut, + }); + }); + }); + + const durationMs = Date.now() - startedAt; + const { text: stdoutRaw, truncated: stdoutTruncated } = truncate(result.stdout); + const { text: stderrRaw, truncated: stderrTruncated } = truncate(result.stderr); + + const stdout = redact(stdoutRaw, resolvedEnv); + const stderr = redact(stderrRaw, resolvedEnv); + + if (result.spawnError) { + const message = redact(result.spawnError.message, resolvedEnv); + logError("op_run spawn failed.", new Error(message)); + return errorResult(new Error(message)); + } + + log("debug", "op_run completed.", { + exitCode: result.exitCode, + signal: result.signal, + durationMs, + timedOut: result.timedOut, + }); + + return jsonResult({ + exitCode: result.exitCode, + signal: result.signal, + timedOut: result.timedOut, + stdout, + stderr, + stdoutTruncated, + stderrTruncated, + durationMs, + }); + } catch (error) { + // Redact even on the error path in case a partially-resolved secret + // ended up embedded in the thrown error's message. + const message = error instanceof Error ? redact(error.message, resolvedEnv) : String(error); + const safeError = new Error(message); + logError("op_run failed.", safeError); + return errorResult(safeError); + } + }, + ); +} diff --git a/tests/op-check-ref.test.ts b/tests/op-check-ref.test.ts new file mode 100644 index 0000000..2ef7af3 --- /dev/null +++ b/tests/op-check-ref.test.ts @@ -0,0 +1,160 @@ +/** + * Tests for the op_check_ref tool — validates an op:// reference and + * returns metadata only, never the secret value. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../src/client.js", () => ({ + getClient: vi.fn(), + requireServiceAccountToken: vi.fn(() => "mock-token"), + resetClient: vi.fn(), +})); + +vi.mock("../src/logger.js", () => ({ + log: vi.fn(), + logError: vi.fn(), +})); + +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { getClient } from "../src/client.js"; +import { resetConfig } from "../src/config.js"; +import { registerAllTools } from "../src/tools/index.js"; + +const mockedGetClient = vi.mocked(getClient); + +describe("op_check_ref", () => { + let server: McpServer; + let registeredTools: Map; + const originalEnv = { ...process.env }; + + beforeEach(() => { + vi.clearAllMocks(); + resetConfig(); + delete process.env.OP_MCP_ALLOWED_VAULTS; + + server = new McpServer({ name: "test", version: "0.0.0" }); + registeredTools = new Map(); + const originalTool = server.tool.bind(server); + vi.spyOn(server, "tool").mockImplementation((...args: any[]) => { + if (args.length === 4) { + registeredTools.set(args[0], { + description: args[1], + schema: args[2], + handler: args[3], + }); + } + return originalTool(...args); + }); + registerAllTools(server); + }); + + afterEach(() => { + Object.keys(process.env).forEach((key) => { + if (!(key in originalEnv)) delete process.env[key]; + else process.env[key] = originalEnv[key]; + }); + resetConfig(); + }); + + function handler() { + return registeredTools.get("op_check_ref")!.handler; + } + + it("returns metadata for a resolvable reference without the value", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { + resolveAll: vi.fn().mockResolvedValue({ + individualResponses: { + "op://Private/github/token": { + content: { secret: "s3cr3t", itemId: "i1", vaultId: "v1" }, + }, + }, + }), + }, + items: { + get: vi.fn().mockResolvedValue({ + id: "i1", + title: "GitHub", + category: "Login", + fields: [ + { id: "token", title: "token", fieldType: "Concealed", value: "s3cr3t-value" }, + ], + }), + }, + } as any); + + const result = await handler()({ secretReference: "op://Private/github/token" }); + const data = JSON.parse(result.content[0].text); + const raw = result.content[0].text; + + expect(result.isError).toBeUndefined(); + expect(data.resolved).toBe(true); + expect(data.vault.name).toBe("Private"); + expect(data.item.title).toBe("GitHub"); + expect(data.field.id).toBe("token"); + expect(data.field.type).toBe("Concealed"); + expect(data.field.value).toBeUndefined(); + expect(raw).not.toContain("s3cr3t-value"); + }); + + it("errors when the reference does not resolve", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { + resolveAll: vi.fn().mockResolvedValue({ + individualResponses: { + "op://Private/missing/token": { error: { type: "not_found" } }, + }, + }), + }, + items: { get: vi.fn() }, + } as any); + + const result = await handler()({ secretReference: "op://Private/missing/token" }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Could not resolve secret reference"); + }); + + it("errors when the field is not found on the item", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { + resolveAll: vi.fn().mockResolvedValue({ + individualResponses: { + "op://Private/github/nope": { + content: { secret: "x", itemId: "i1", vaultId: "v1" }, + }, + }, + }), + }, + items: { + get: vi.fn().mockResolvedValue({ + id: "i1", + title: "GitHub", + fields: [{ id: "token", title: "token", fieldType: "Concealed", value: "x" }], + }), + }, + } as any); + + const result = await handler()({ secretReference: "op://Private/github/nope" }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not found on item"); + }); + + it("rejects a vault outside a configured allow-list before resolving", async () => { + process.env.OP_MCP_ALLOWED_VAULTS = "Private"; + resetConfig(); + const resolveAll = vi.fn(); + mockedGetClient.mockResolvedValue({ + secrets: { resolveAll }, + items: { get: vi.fn() }, + } as any); + + const result = await handler()({ secretReference: "op://personal/github/token" }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not in the allowed vault list"); + expect(resolveAll).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/op-run.test.ts b/tests/op-run.test.ts new file mode 100644 index 0000000..56340ff --- /dev/null +++ b/tests/op-run.test.ts @@ -0,0 +1,181 @@ +/** + * Tests for the op_run tool — executes local commands with 1Password + * secrets injected as env vars, without ever returning secret plaintext. + * + * Uses the real Node binary (process.execPath) as the child process so + * these tests are platform-independent (no reliance on /bin/sh vs cmd.exe + * shell syntax) and do not depend on any external tool being on PATH. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../src/client.js", () => ({ + getClient: vi.fn(), + requireServiceAccountToken: vi.fn(() => "mock-token"), + resetClient: vi.fn(), +})); + +vi.mock("../src/logger.js", () => ({ + log: vi.fn(), + logError: vi.fn(), +})); + +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { getClient } from "../src/client.js"; +import { resetConfig } from "../src/config.js"; +import { registerAllTools } from "../src/tools/index.js"; + +const mockedGetClient = vi.mocked(getClient); +const node = process.execPath; + +describe("op_run", () => { + let server: McpServer; + let registeredTools: Map; + const originalEnv = { ...process.env }; + + beforeEach(() => { + vi.clearAllMocks(); + resetConfig(); + delete process.env.OP_MCP_ALLOWED_VAULTS; + + server = new McpServer({ name: "test", version: "0.0.0" }); + registeredTools = new Map(); + const originalTool = server.tool.bind(server); + vi.spyOn(server, "tool").mockImplementation((...args: any[]) => { + if (args.length === 4) { + registeredTools.set(args[0], { + description: args[1], + schema: args[2], + handler: args[3], + }); + } + return originalTool(...args); + }); + registerAllTools(server); + }); + + afterEach(() => { + Object.keys(process.env).forEach((key) => { + if (!(key in originalEnv)) delete process.env[key]; + else process.env[key] = originalEnv[key]; + }); + resetConfig(); + }); + + function handler() { + return registeredTools.get("op_run")!.handler; + } + + it("resolves an op:// reference into env and the child process sees the value", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { resolve: vi.fn().mockResolvedValue("my-secret-value") }, + } as any); + + const result = await handler()({ + argv: [ + node, + "-e", + "process.exit(process.env.MY_SECRET === 'my-secret-value' ? 0 : 1)", + ], + env: { MY_SECRET: "op://Private/github/token" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.exitCode).toBe(0); + }); + + it("redacts the resolved secret value out of stdout even if the command echoes it", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { resolve: vi.fn().mockResolvedValue("my-secret-value") }, + } as any); + + const result = await handler()({ + argv: [node, "-e", "process.stdout.write('token=' + process.env.MY_SECRET)"], + env: { MY_SECRET: "op://Private/github/token" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.stdout).not.toContain("my-secret-value"); + expect(data.stdout).toBe("token=«REDACTED:MY_SECRET»"); + }); + + it("passes literal (non op://) env values through unchanged without calling 1Password", async () => { + const resolve = vi.fn(); + mockedGetClient.mockResolvedValue({ secrets: { resolve } } as any); + + const result = await handler()({ + argv: [ + node, + "-e", + "process.exit(process.env.PLAIN_VAR === 'plain-value' ? 0 : 1)", + ], + env: { PLAIN_VAR: "plain-value" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(resolve).not.toHaveBeenCalled(); + }); + + it("captures a nonzero exit code and stderr", async () => { + mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); + + const result = await handler()({ + argv: [node, "-e", "process.stderr.write('boom'); process.exit(7)"], + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(7); + expect(data.stderr).toContain("boom"); + }); + + it("rejects an op:// reference from a vault outside a configured allow-list", async () => { + process.env.OP_MCP_ALLOWED_VAULTS = "Private"; + resetConfig(); + const resolve = vi.fn(); + mockedGetClient.mockResolvedValue({ secrets: { resolve } } as any); + + const result = await handler()({ + argv: [node, "-e", "process.exit(0)"], + env: { MY_SECRET: "op://personal/github/token" }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not in the allowed vault list"); + expect(resolve).not.toHaveBeenCalled(); + }); + + it("errors when neither command nor argv is provided", async () => { + mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); + + const result = await handler()({}); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Provide either `command` or `argv`"); + }); + + it("errors when both command and argv are provided", async () => { + mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); + + const result = await handler()({ command: "echo hi", argv: [node, "-e", "1"] }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("only one of"); + }); + + it("respects cwd when provided", async () => { + mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); + const cwd = process.cwd(); + const normalize = (p: string) => p.toLowerCase().replace(/\\/g, "/").replace(/\/$/, ""); + + const result = await handler()({ + argv: [node, "-e", "process.stdout.write(process.cwd())"], + cwd, + }); + const data = JSON.parse(result.content[0].text); + + expect(normalize(data.stdout)).toBe(normalize(cwd)); + }); +}); diff --git a/tests/secret-ref.test.ts b/tests/secret-ref.test.ts new file mode 100644 index 0000000..eb22642 --- /dev/null +++ b/tests/secret-ref.test.ts @@ -0,0 +1,74 @@ +/** + * Tests for src/secret-ref.ts — op:// reference parsing and vault allow-listing. + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { resetConfig } from "../src/config.js"; +import { + isSecretRef, + parseSecretRef, + assertVaultAllowed, +} from "../src/secret-ref.js"; + +describe("secret-ref", () => { + const originalEnv = { ...process.env }; + + beforeEach(() => { + resetConfig(); + delete process.env.OP_MCP_ALLOWED_VAULTS; + }); + + afterEach(() => { + Object.keys(process.env).forEach((key) => { + if (!(key in originalEnv)) delete process.env[key]; + else process.env[key] = originalEnv[key]; + }); + resetConfig(); + }); + + describe("isSecretRef", () => { + it("recognizes a well-formed op:// reference", () => { + expect(isSecretRef("op://Private/github/token")).toBe(true); + }); + + it("rejects plain literal strings", () => { + expect(isSecretRef("plain-value")).toBe(false); + expect(isSecretRef("https://example.com")).toBe(false); + }); + }); + + describe("parseSecretRef", () => { + it("splits vault/item/field segments", () => { + const ref = parseSecretRef("op://Private/github/token"); + expect(ref.vault).toBe("Private"); + expect(ref.item).toBe("github"); + expect(ref.field).toBe("token"); + }); + + it("allows slashes within the field segment", () => { + const ref = parseSecretRef("op://Private/github/section/nested-field"); + expect(ref.field).toBe("section/nested-field"); + }); + + it("throws on a malformed reference", () => { + expect(() => parseSecretRef("not-a-ref")).toThrow(/Invalid secret reference/); + expect(() => parseSecretRef("op://only-vault")).toThrow(/Invalid secret reference/); + }); + }); + + describe("assertVaultAllowed", () => { + it("allows any vault when no allow-list is configured (default)", () => { + expect(() => assertVaultAllowed("Private")).not.toThrow(); + expect(() => assertVaultAllowed("anything-else")).not.toThrow(); + }); + + it("restricts to the configured vaults when OP_MCP_ALLOWED_VAULTS is set", () => { + process.env.OP_MCP_ALLOWED_VAULTS = "personal, work"; + resetConfig(); + expect(() => assertVaultAllowed("personal")).not.toThrow(); + expect(() => assertVaultAllowed("work")).not.toThrow(); + expect(() => assertVaultAllowed("WORK")).not.toThrow(); // case-insensitive + expect(() => assertVaultAllowed("Private")).toThrow(/not in the allowed vault list/); + }); + }); +}); diff --git a/tests/tools.test.ts b/tests/tools.test.ts index 3d9e60b..48e4e20 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -49,8 +49,8 @@ describe("MCP Tools", () => { registerAllTools(server); }); - it("registers all 13 tools", () => { - expect(registeredTools.size).toBe(13); + it("registers all 15 tools", () => { + expect(registeredTools.size).toBe(15); expect(registeredTools.has("vault_list")).toBe(true); expect(registeredTools.has("item_lookup")).toBe(true); expect(registeredTools.has("item_delete")).toBe(true); @@ -64,6 +64,8 @@ describe("MCP Tools", () => { expect(registeredTools.has("password_update")).toBe(true); expect(registeredTools.has("password_generate")).toBe(true); expect(registeredTools.has("password_generate_memorable")).toBe(true); + expect(registeredTools.has("op_run")).toBe(true); + expect(registeredTools.has("op_check_ref")).toBe(true); }); it("all tools have descriptions", () => { From c27a5744376997c4bc6ab5d480e47acf710136a5 Mon Sep 17 00:00:00 2001 From: Albert Hazan Date: Thu, 23 Jul 2026 08:36:07 -0400 Subject: [PATCH 2/2] perf(op_run): resolve environment secrets in bulk --- README.md | 2 +- src/tools/op-run.ts | 30 +++++++++++++++++------- tests/op-run.test.ts | 56 ++++++++++++++++++++++++++++++-------------- 3 files changed, 61 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index a59b471..6bd3144 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ A community-built [Model Context Protocol (MCP)](https://modelcontextprotocol.io | `password_update` | Rotate/update an existing password | | `password_generate` | Generate a cryptographically secure random password | | `password_generate_memorable` | Generate a memorable passphrase from ~500 dictionary words | -| `op_run` | Run a local command with `op://` references injected as env vars; resolved secret plaintext is redacted from stdout/stderr and never logged (the MCP equivalent of `op run`) | +| `op_run` | Run a local command with `op://` references injected as env vars; all references for that command resolve in one bulk SDK request, and plaintext is redacted from stdout/stderr and never logged (the MCP equivalent of `op run`) | | `op_check_ref` | Validate an `op://vault/item/field` reference and return only non-secret metadata (vault, item, field) confirming it resolves — never the value | ### Prompts (4) diff --git a/src/tools/op-run.ts b/src/tools/op-run.ts index d0970dd..15404b9 100644 --- a/src/tools/op-run.ts +++ b/src/tools/op-run.ts @@ -22,25 +22,39 @@ interface ResolvedEnvEntry { secret: boolean; } -/** Resolve the `env` map into plaintext values, keeping track of which ones are secrets. */ +/** Resolve every secret reference needed by one command in one bulk SDK request. */ async function resolveEnvEntries( env: Record | undefined, ): Promise { if (!env) return []; + const envEntries = Object.entries(env); + const references = envEntries.map(([, value]) => value).filter(isSecretRef); + const entries: ResolvedEnvEntry[] = []; + + if (references.length === 0) { + return envEntries.map(([name, value]) => ({ name, value, secret: false })); + } + + for (const reference of references) { + assertVaultAllowed(parseSecretRef(reference).vault); + } + const client = await getClient(); - if (!client?.secrets?.resolve) { + if (!client?.secrets?.resolveAll) { throw new Error( "Your @1password/sdk version does not support resolving secrets.", ); } + const resolved = await client.secrets.resolveAll(references); - const entries: ResolvedEnvEntry[] = []; - for (const [name, rawValue] of Object.entries(env)) { + for (const [name, rawValue] of envEntries) { if (isSecretRef(rawValue)) { - const ref = parseSecretRef(rawValue); - assertVaultAllowed(ref.vault); - const value = await client.secrets.resolve(rawValue); - entries.push({ name, value, secret: true }); + const response = resolved.individualResponses[rawValue]; + if (!response?.content) { + const reason = response?.error?.type ?? "unknown"; + throw new Error(`Could not resolve secret reference '${rawValue}' (${reason}).`); + } + entries.push({ name, value: response.content.secret, secret: true }); } else { entries.push({ name, value: rawValue, secret: false }); } diff --git a/tests/op-run.test.ts b/tests/op-run.test.ts index 56340ff..1ea172b 100644 --- a/tests/op-run.test.ts +++ b/tests/op-run.test.ts @@ -66,10 +66,18 @@ describe("op_run", () => { return registeredTools.get("op_run")!.handler; } + function mockBulkResolve(values: Record) { + const resolveAll = vi.fn().mockImplementation(async (references: string[]) => ({ + individualResponses: Object.fromEntries( + references.map((reference) => [reference, { content: { secret: values[reference] ?? "" } }]), + ), + })); + mockedGetClient.mockResolvedValue({ secrets: { resolveAll } } as any); + return resolveAll; + } + it("resolves an op:// reference into env and the child process sees the value", async () => { - mockedGetClient.mockResolvedValue({ - secrets: { resolve: vi.fn().mockResolvedValue("my-secret-value") }, - } as any); + mockBulkResolve({ "op://Private/github/token": "my-secret-value" }); const result = await handler()({ argv: [ @@ -86,9 +94,7 @@ describe("op_run", () => { }); it("redacts the resolved secret value out of stdout even if the command echoes it", async () => { - mockedGetClient.mockResolvedValue({ - secrets: { resolve: vi.fn().mockResolvedValue("my-secret-value") }, - } as any); + mockBulkResolve({ "op://Private/github/token": "my-secret-value" }); const result = await handler()({ argv: [node, "-e", "process.stdout.write('token=' + process.env.MY_SECRET)"], @@ -102,9 +108,6 @@ describe("op_run", () => { }); it("passes literal (non op://) env values through unchanged without calling 1Password", async () => { - const resolve = vi.fn(); - mockedGetClient.mockResolvedValue({ secrets: { resolve } } as any); - const result = await handler()({ argv: [ node, @@ -116,7 +119,7 @@ describe("op_run", () => { const data = JSON.parse(result.content[0].text); expect(data.exitCode).toBe(0); - expect(resolve).not.toHaveBeenCalled(); + expect(mockedGetClient).not.toHaveBeenCalled(); }); it("captures a nonzero exit code and stderr", async () => { @@ -134,8 +137,8 @@ describe("op_run", () => { it("rejects an op:// reference from a vault outside a configured allow-list", async () => { process.env.OP_MCP_ALLOWED_VAULTS = "Private"; resetConfig(); - const resolve = vi.fn(); - mockedGetClient.mockResolvedValue({ secrets: { resolve } } as any); + const resolveAll = vi.fn(); + mockedGetClient.mockResolvedValue({ secrets: { resolveAll } } as any); const result = await handler()({ argv: [node, "-e", "process.exit(0)"], @@ -144,12 +147,10 @@ describe("op_run", () => { expect(result.isError).toBe(true); expect(result.content[0].text).toContain("not in the allowed vault list"); - expect(resolve).not.toHaveBeenCalled(); + expect(resolveAll).not.toHaveBeenCalled(); }); it("errors when neither command nor argv is provided", async () => { - mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); - const result = await handler()({}); expect(result.isError).toBe(true); @@ -157,8 +158,6 @@ describe("op_run", () => { }); it("errors when both command and argv are provided", async () => { - mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); - const result = await handler()({ command: "echo hi", argv: [node, "-e", "1"] }); expect(result.isError).toBe(true); @@ -166,7 +165,6 @@ describe("op_run", () => { }); it("respects cwd when provided", async () => { - mockedGetClient.mockResolvedValue({ secrets: { resolve: vi.fn() } } as any); const cwd = process.cwd(); const normalize = (p: string) => p.toLowerCase().replace(/\\/g, "/").replace(/\/$/, ""); @@ -178,4 +176,26 @@ describe("op_run", () => { expect(normalize(data.stdout)).toBe(normalize(cwd)); }); + + it("resolves multiple secret references with one bulk SDK request", async () => { + const resolveAll = mockBulkResolve({ + "op://Private/first/token": "first-value", + "op://Private/second/token": "second-value", + }); + + const result = await handler()({ + argv: [node, "-e", "process.exit(process.env.FIRST && process.env.SECOND ? 0 : 1)"], + env: { + FIRST: "op://Private/first/token", + SECOND: "op://Private/second/token", + }, + }); + + expect(result.isError).toBeUndefined(); + expect(resolveAll).toHaveBeenCalledOnce(); + expect(resolveAll).toHaveBeenCalledWith([ + "op://Private/first/token", + "op://Private/second/token", + ]); + }); });