From f24790d921e09dd3b5043c7293dee44dfa11ef89 Mon Sep 17 00:00:00 2001 From: CakeRepository Date: Mon, 31 Aug 2026 13:15:28 -0500 Subject: [PATCH] fix(security): sanitize child env and redact service account token in op_run (v4.0.2) - Strip OP_SERVICE_ACCOUNT_TOKEN, OP_KEYCHAIN_SERVICE, and OP_KEYCHAIN_ACCOUNT from child environment before spawning commands - Add server's OP_SERVICE_ACCOUNT_TOKEN to defense-in-depth output redaction targets - Redact secrets prior to output truncation to prevent boundary leaks - Add unit tests for credential stripping, echo redaction, and boundary truncation - Credit independent security researcher Syed Anas Mohiuddin in CHANGELOG.md --- CHANGELOG.md | 9 ++++ package-lock.json | 4 +- package.json | 2 +- server.json | 4 +- src/config.ts | 2 +- src/tools/op-run.ts | 97 ++++++++++++++++++++++++++++++++++++-------- tests/op-run.test.ts | 85 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 181 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87e29cb..7b34dd4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/package-lock.json b/package-lock.json index 65abe5a..3666b05 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@takescake/1password-mcp", - "version": "4.0.0", + "version": "4.0.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@takescake/1password-mcp", - "version": "4.0.0", + "version": "4.0.1", "license": "Apache-2.0", "dependencies": { "@1password/sdk": "^0.3.1", diff --git a/package.json b/package.json index 5a6d69d..cf460c1 100644 --- a/package.json +++ b/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)", diff --git a/server.json b/server.json index 668e974..66edd4a 100644 --- a/server.json +++ b/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" }, diff --git a/src/config.ts b/src/config.ts index 0c8c3db..b18f5a7 100644 --- a/src/config.ts +++ b/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 { diff --git a/src/tools/op-run.ts b/src/tools/op-run.ts index 4b550e0..7966b5e 100644 --- a/src/tools/op-run.ts +++ b/src/tools/op-run.ts @@ -7,6 +7,7 @@ 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"; @@ -14,6 +15,12 @@ import { isSecretRef, parseSecretRef, assertVaultAllowed } from "../secret-ref.j 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; @@ -21,6 +28,11 @@ interface ResolvedEnvEntry { 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 | 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(); + + // 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); } diff --git a/tests/op-run.test.ts b/tests/op-run.test.ts index 9976b10..66713eb 100644 --- a/tests/op-run.test.ts +++ b/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"); + }); });