fix(security): sanitize child env and redact service account token in op_run (v4.0.2)

CHANGELOG.md

@@ -6,6 +6,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 
+## [4.0.2] - 2026-08-31
+
+### Security
+
+- **Fixed service account token leak in `op_run`** — `op_run` now strips the MCP server's credentials (`OP_SERVICE_ACCOUNT_TOKEN`, `OP_KEYCHAIN_SERVICE`, `OP_KEYCHAIN_ACCOUNT`) from the child process environment before execution, preventing ambient token leakage to subprocesses.
+- **Defense-in-depth output redaction** — Added the server's master service account token to the output redaction targets so any ambient or direct echoing in stdout, stderr, spawn errors, or thrown exceptions is masked with `«REDACTED:OP_SERVICE_ACCOUNT_TOKEN»`.
+- **Boundary-safe secret redaction** — Full redaction is now applied prior to output truncation, preventing secret values that straddle the 5 MiB stream cap from surviving as partial unredacted substrings.
+- Thanks to independent security researcher **Syed Anas Mohiuddin** for responsibly discovering, analyzing, and reporting this vulnerability.
+
 ## [4.0.1] - 2026-07-29
 
 ### Changed

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@takescake/1password-mcp",
-  "version": "4.0.1",
+  "version": "4.0.2",
   "private": false,
   "type": "module",
   "description": "Security-first MCP server for 1Password — vault/item tools, prompts, resources, and op_run secret injection (MCP 2026-07-28)",

server.json

@@ -6,12 +6,12 @@
     "url": "https://github.com/CakeRepository/1Password-MCP.git",
     "source": "github"
   },
-  "version": "4.0.1",
+  "version": "4.0.2",
   "packages": [
     {
       "registryType": "npm",
       "identifier": "@takescake/1password-mcp",
-      "version": "4.0.1",
+      "version": "4.0.2",
       "transport": {
         "type": "stdio"
       },

src/config.ts

@@ -6,7 +6,7 @@ import { execFileSync } from "node:child_process";
 import { LOG_LEVEL_VALUES, type LogLevel } from "./types.js";
 
 export const SERVER_NAME = "1password-mcp";
-export const SERVER_VERSION = "4.0.1";
+export const SERVER_VERSION = "4.0.2";
 
 /** Parse a `--flag value` or `--flag=value` argument from process.argv. */
 function getArgValue(name: string): string | undefined {

src/tools/op-run.ts

@@ -7,20 +7,32 @@ import type { McpServer } from "@modelcontextprotocol/server";
 import { spawn } from "node:child_process";
 import { z } from "zod";
 import { getClient } from "../client.js";
+import { getConfig } from "../config.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;
 
+const SENSITIVE_SERVER_ENV_VARS = [
+  "OP_SERVICE_ACCOUNT_TOKEN",
+  "OP_KEYCHAIN_SERVICE",
+  "OP_KEYCHAIN_ACCOUNT",
+] as const;
+
 interface ResolvedEnvEntry {
   name: string;
   value: string;
   /** True if this env var came from an op:// reference and must be redacted from output. */
   secret: boolean;
 }
 
+interface RedactionTarget {
+  name: string;
+  value: string;
+}
+
 /** Resolve every secret reference needed by one command in one bulk SDK request. */
 async function resolveEnvEntries(
   env: Record<string, string> | undefined,
@@ -61,24 +73,69 @@ async function resolveEnvEntries(
   return entries;
 }
 
+/** Collect every secret string that must be redacted from outputs/errors. */
+function getRedactionTargets(
+  resolvedEnv: ResolvedEnvEntry[],
+): RedactionTarget[] {
+  const targets: RedactionTarget[] = [];
+  const seen = new Set<string>();
+
+  // 1. Secrets resolved from op:// references
+  for (const entry of resolvedEnv) {
+    if (entry.secret && entry.value.length > 0 && !seen.has(entry.value)) {
+      targets.push({ name: entry.name, value: entry.value });
+      seen.add(entry.value);
+    }
+  }
+
+  // 2. Defense-in-depth: Server's own service account token (from config or process.env)
+  try {
+    const configToken = getConfig().serviceAccountToken;
+    if (configToken && configToken.length > 0 && !seen.has(configToken)) {
+      targets.push({ name: "OP_SERVICE_ACCOUNT_TOKEN", value: configToken });
+      seen.add(configToken);
+    }
+  } catch {
+    // Ignore config lookup error
+  }
+
+  const envToken = process.env.OP_SERVICE_ACCOUNT_TOKEN;
+  if (envToken && envToken.length > 0 && !seen.has(envToken)) {
+    targets.push({ name: "OP_SERVICE_ACCOUNT_TOKEN", value: envToken });
+    seen.add(envToken);
+  }
+
+  return targets;
+}
+
 /** Replace every occurrence of every secret value with a redaction marker. */
-function redact(text: string, secrets: ResolvedEnvEntry[]): string {
+function redact(text: string, targets: RedactionTarget[]): string {
   let redacted = text;
-  for (const entry of secrets) {
-    if (!entry.secret || entry.value.length === 0) continue;
+  for (const target of targets) {
+    if (target.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}»`);
+    redacted = redacted.split(target.value).join(`«REDACTED:${target.name}»`);
   }
   return redacted;
 }
 
-function truncate(buffer: Buffer): { text: string; truncated: boolean } {
-  if (buffer.length <= MAX_OUTPUT_BYTES) {
-    return { text: buffer.toString("utf8"), truncated: false };
+function truncateAndRedact(
+  buffer: Buffer,
+  targets: RedactionTarget[],
+): { text: string; truncated: boolean } {
+  // Redact the full text first so secret values spanning the truncation
+  // boundary are matched and masked completely before slicing.
+  const rawText = buffer.toString("utf8");
+  const redactedText = redact(rawText, targets);
+  const redactedBuffer = Buffer.from(redactedText, "utf8");
+
+  if (redactedBuffer.length <= MAX_OUTPUT_BYTES) {
+    return { text: redactedText, truncated: false };
   }
+
   return {
-    text: buffer.subarray(0, MAX_OUTPUT_BYTES).toString("utf8"),
+    text: redactedBuffer.subarray(0, MAX_OUTPUT_BYTES).toString("utf8"),
     truncated: true,
   };
 }
@@ -149,6 +206,9 @@ export function registerOpRun(server: McpServer): void {
 
                 resolvedEnv = await resolveEnvEntries(env);
                 const childEnv: NodeJS.ProcessEnv = { ...process.env };
+                for (const envVar of SENSITIVE_SERVER_ENV_VARS) {
+                  delete childEnv[envVar];
+                }
                 for (const entry of resolvedEnv) {
                   childEnv[entry.name] = entry.value;
                 }
@@ -219,14 +279,18 @@ export function registerOpRun(server: McpServer): void {
                 });
 
                 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);
+                const redactionTargets = getRedactionTargets(resolvedEnv);
+                const { text: stdout, truncated: stdoutTruncated } = truncateAndRedact(
+                  result.stdout,
+                  redactionTargets,
+                );
+                const { text: stderr, truncated: stderrTruncated } = truncateAndRedact(
+                  result.stderr,
+                  redactionTargets,
+                );
 
                 if (result.spawnError) {
-                  const message = redact(result.spawnError.message, resolvedEnv);
+                  const message = redact(result.spawnError.message, redactionTargets);
                   logError("op_run spawn failed.", new Error(message));
                   return errorResult(new Error(message));
                 }
@@ -251,8 +315,9 @@ export function registerOpRun(server: McpServer): void {
               } 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);
+                const targets = getRedactionTargets(resolvedEnv);
+                const rawMessage = error instanceof Error ? error.message : String(error);
+                const safeError = new Error(redact(rawMessage, targets));
                 logError("op_run failed.", safeError);
                 return errorResult(safeError);
               }

tests/op-run.test.ts

@@ -196,4 +196,89 @@ describe("op_run", () => {
       "op://Private/second/token",
     ]);
   });
+
+  it("strips server OP_SERVICE_ACCOUNT_TOKEN and keychain credentials from child environment", async () => {
+    process.env.OP_SERVICE_ACCOUNT_TOKEN = "ops_secret_master_token_12345";
+    process.env.OP_KEYCHAIN_SERVICE = "my-keychain-svc";
+    process.env.OP_KEYCHAIN_ACCOUNT = "my-keychain-acc";
+    resetConfig();
+
+    const result = await handler()({
+      argv: [
+        node,
+        "-e",
+        "process.stdout.write(JSON.stringify({ token: process.env.OP_SERVICE_ACCOUNT_TOKEN, svc: process.env.OP_KEYCHAIN_SERVICE, acc: process.env.OP_KEYCHAIN_ACCOUNT }))",
+      ],
+    });
+    const data = JSON.parse(result.content[0].text);
+
+    expect(data.exitCode).toBe(0);
+    const envObserved = JSON.parse(data.stdout);
+    expect(envObserved.token).toBeUndefined();
+    expect(envObserved.svc).toBeUndefined();
+    expect(envObserved.acc).toBeUndefined();
+  });
+
+  it("redacts server OP_SERVICE_ACCOUNT_TOKEN from output as defense-in-depth", async () => {
+    process.env.OP_SERVICE_ACCOUNT_TOKEN = "ops_secret_master_token_12345";
+    resetConfig();
+
+    const result = await handler()({
+      argv: [
+        node,
+        "-e",
+        "process.stdout.write('leaked=' + 'ops_secret_master_token_12345'); process.stderr.write('err=' + 'ops_secret_master_token_12345')",
+      ],
+    });
+    const data = JSON.parse(result.content[0].text);
+
+    expect(data.exitCode).toBe(0);
+    expect(data.stdout).not.toContain("ops_secret_master_token_12345");
+    expect(data.stderr).not.toContain("ops_secret_master_token_12345");
+    expect(data.stdout).toBe("leaked=«REDACTED:OP_SERVICE_ACCOUNT_TOKEN»");
+    expect(data.stderr).toBe("err=«REDACTED:OP_SERVICE_ACCOUNT_TOKEN»");
+  });
+
+  it("allows caller to explicitly inject OP_SERVICE_ACCOUNT_TOKEN via op:// reference and redacts it", async () => {
+    process.env.OP_SERVICE_ACCOUNT_TOKEN = "original-server-token";
+    resetConfig();
+    mockBulkResolve({ "op://Private/custom/token": "injected-custom-token" });
+
+    const result = await handler()({
+      argv: [
+        node,
+        "-e",
+        "process.stdout.write('active=' + process.env.OP_SERVICE_ACCOUNT_TOKEN)",
+      ],
+      env: { OP_SERVICE_ACCOUNT_TOKEN: "op://Private/custom/token" },
+    });
+    const data = JSON.parse(result.content[0].text);
+
+    expect(data.exitCode).toBe(0);
+    expect(data.stdout).not.toContain("injected-custom-token");
+    expect(data.stdout).not.toContain("original-server-token");
+    expect(data.stdout).toBe("active=«REDACTED:OP_SERVICE_ACCOUNT_TOKEN»");
+  });
+
+  it("redacts secret value before truncation when output exceeds max buffer size", async () => {
+    mockBulkResolve({ "op://Private/secret/key": "supersecretkey999" });
+
+    // Secret straddles the 5 MiB boundary (bytes 5,242,870 to 5,242,887)
+    const paddingLength = 5 * 1024 * 1024 - 10;
+    const result = await handler()({
+      argv: [
+        node,
+        "-e",
+        `process.stdout.write('A'.repeat(${paddingLength}) + process.env.MY_SECRET + 'trailing')`,
+      ],
+      env: { MY_SECRET: "op://Private/secret/key" },
+    });
+    const data = JSON.parse(result.content[0].text);
+
+    expect(data.exitCode).toBe(0);
+    expect(data.stdoutTruncated).toBe(true);
+    expect(data.stdout).not.toContain("supersecretkey999");
+    expect(data.stdout).not.toContain("supersecre");
+    expect(data.stdout).toContain("«REDACTED");
+  });
 });