From d9905bd6e695b30839b373ad2920bf00a99cb47a Mon Sep 17 00:00:00 2001 From: CakeRepository Date: Sun, 4 Oct 2026 03:33:58 -0500 Subject: [PATCH 1/4] fix(security): harden item_get, vault allow-list, op_run, and CI (v4.0.4) Security release from an internal review. - item_get: deny-by-default field masking. SSH private keys, TOTP seeds, and card numbers were returned in plaintext without reveal. - Vault allow-list (OP_MCP_ALLOWED_VAULTS) is now enforced by every tool and resource, and op:// references are also checked by the vault they resolve to, not only as written (new src/vault-access.ts). - op_run redaction (new src/redaction.ts): single-pass masking over the original output, so overlapping secrets no longer leak each other's remainder; also masks base64 (any alignment), JSON/URL-escaped, and multi-line/CRLF forms. - op_run: output cap enforced while the command runs (memory-exhaustion DoS), timeouts kill the whole process tree and always return, the credential env scrub is case-insensitive, and the tool description no longer overclaims. - CI: remove the leftover issue_comment-triggered mcp-v2-migration.yml (contents: write, triggerable by any GitHub user). publish.yml passes the release tag via env, does not persist credentials, and splits build/test/pack (npm ci --ignore-scripts, no OIDC) from a publish-only job that alone holds id-token: write. - macOS Keychain lookup runs /usr/bin/security; startup warning when the token is passed on the command line. - Docs and CHANGELOG updated; version bumped to 4.0.4. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 2 + .github/workflows/mcp-v2-migration.yml | 198 ------- .github/workflows/publish.yml | 114 +++- CHANGELOG.md | 24 + CONTRIBUTING.md | 16 +- README.md | 49 +- agents.md | 26 +- package-lock.json | 4 +- package.json | 2 +- server.json | 6 +- src/config.ts | 37 +- src/index.ts | 10 +- src/redaction.ts | 292 ++++++++++ src/resources/index.ts | 9 +- src/secret-ref.ts | 6 +- src/tools/item-archive.ts | 2 + src/tools/item-delete.ts | 2 + src/tools/item-edit.ts | 2 + src/tools/item-get.ts | 48 +- src/tools/item-list.ts | 2 + src/tools/item-lookup.ts | 2 + src/tools/note-create.ts | 2 + src/tools/op-check-ref.ts | 4 + src/tools/op-run.ts | 342 +++++++----- src/tools/password-create.ts | 2 + src/tools/password-read.ts | 24 +- src/tools/password-update.ts | 2 + src/tools/vault-list.ts | 8 +- src/vault-access.ts | 135 +++++ tests/config.test.ts | 30 +- tests/op-check-ref.test.ts | 82 +++ tests/op-run.test.ts | 705 ++++++++++++++++++++++++- tests/redaction.test.ts | 604 +++++++++++++++++++++ tests/tools.test.ts | 329 +++++++++++- tests/vault-access.test.ts | 326 ++++++++++++ tests/vault-allowlist.test.ts | 544 +++++++++++++++++++ 36 files changed, 3559 insertions(+), 433 deletions(-) delete mode 100644 .github/workflows/mcp-v2-migration.yml create mode 100644 src/redaction.ts create mode 100644 src/vault-access.ts create mode 100644 tests/redaction.test.ts create mode 100644 tests/vault-access.test.ts create mode 100644 tests/vault-allowlist.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 574aeaf..47e2b29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,6 +20,8 @@ jobs: steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Use Node.js ${{ matrix.node-version }} uses: actions/setup-node@v4 diff --git a/.github/workflows/mcp-v2-migration.yml b/.github/workflows/mcp-v2-migration.yml deleted file mode 100644 index 6be79bb..0000000 --- a/.github/workflows/mcp-v2-migration.yml +++ /dev/null @@ -1,198 +0,0 @@ -# One-time runner; remove after the generated migration lands. -# Runner revision 4: v2 registration test harness. -name: MCP v2 migration - -on: - workflow_dispatch: - issue_comment: - types: [created] - push: - branches: [master] - paths: - - .github/workflows/mcp-v2-migration.yml - -permissions: - contents: write - -jobs: - migrate: - if: github.event_name != 'issue_comment' || (github.event.issue.pull_request && github.event.comment.body == '/run-mcp-v2-migration') - runs-on: ubuntu-latest - - steps: - - uses: actions/checkout@v6 - with: - ref: feat/mcp-2026-07-28 - - - uses: actions/setup-node@v6 - with: - node-version: 24 - registry-url: https://registry.npmjs.org - - - name: Run official MCP v1 to v2 codemod - run: npx --yes @modelcontextprotocol/codemod@2.0.0-beta.5 v1-to-v2 . - - - name: Enable 2026-07-28 and legacy stdio negotiation - run: | - cat > src/index.ts <<'EOF' - /** - * 1Password MCP Server — main entrypoint. - * - * Builds one server instance per stdio connection and lets the MCP v2 - * transport negotiate either protocol revision 2026-07-28 or a legacy - * 2025-era connection. - */ - - import { McpServer } from "@modelcontextprotocol/server"; - import { serveStdio } from "@modelcontextprotocol/server/stdio"; - import { SERVER_NAME, SERVER_VERSION, getConfig } from "./config.js"; - import { log, logError } from "./logger.js"; - import { registerAllTools } from "./tools/index.js"; - import { registerAllPrompts } from "./prompts/index.js"; - import { registerAllResources } from "./resources/index.js"; - - export function buildServer(): McpServer { - const server = new McpServer({ - name: SERVER_NAME, - version: SERVER_VERSION, - }); - - registerAllTools(server); - registerAllPrompts(server); - registerAllResources(server); - - return server; - } - - process.on("uncaughtException", (error) => { - logError("Uncaught exception.", error); - }); - - process.on("unhandledRejection", (reason) => { - logError("Unhandled rejection.", reason); - }); - - async function main(): Promise { - const config = getConfig(); - - log("info", "Starting MCP server.", { - name: SERVER_NAME, - version: SERVER_VERSION, - integrationName: config.integrationName, - integrationVersion: config.integrationVersion, - node: process.version, - tokenSource: config.tokenSource, - }); - - log("info", "Starting MCP stdio protocol negotiation."); - await serveStdio(() => buildServer()); - } - - main().catch((error) => { - logError(`Failed to start ${SERVER_NAME}.`, error); - process.exit(1); - }); - EOF - - - name: Apply major-version metadata - run: | - node <<'NODE' - const fs = require('fs'); - - const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); - pkg.version = '4.0.0'; - pkg.engines = { node: '>=20' }; - pkg.dependencies = pkg.dependencies ?? {}; - pkg.dependencies.zod = '^4.0.0'; - fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n'); - - const manifest = JSON.parse(fs.readFileSync('server.json', 'utf8')); - manifest.version = '4.0.0'; - for (const entry of manifest.packages ?? []) entry.version = '4.0.0'; - fs.writeFileSync('server.json', JSON.stringify(manifest, null, 2) + '\n'); - - const configPath = 'src/config.ts'; - const config = fs.readFileSync(configPath, 'utf8').replace( - /export const SERVER_VERSION = "[^"]+";/, - 'export const SERVER_VERSION = "4.0.0";', - ); - fs.writeFileSync(configPath, config); - - const changelogPath = 'CHANGELOG.md'; - const changelog = fs.readFileSync(changelogPath, 'utf8'); - if (!changelog.includes('## 4.0.0 - 2026-07-29')) { - const entry = [ - '## 4.0.0 - 2026-07-29', - '', - '### Changed', - '', - '- Migrated to the stable MCP TypeScript SDK v2 package family.', - '- Added stdio negotiation for MCP 2026-07-28 while retaining legacy client compatibility.', - '- Raised the minimum supported Node.js version from 18 to 20.', - '- Migrated tools, prompts, and resources to the v2 registration APIs and Zod 4 schemas.', - '', - ].join('\n'); - const marker = changelog.indexOf('\n## '); - const updated = marker >= 0 - ? changelog.slice(0, marker + 1) + '\n' + entry + changelog.slice(marker + 1) - : changelog + '\n' + entry; - fs.writeFileSync(changelogPath, updated); - } - NODE - - - name: Install migrated dependencies - run: npm install - - - name: Migrate test registration harnesses - run: | - node <<'NODE' - const fs = require('fs'); - const files = [ - 'tests/tools.test.ts', - 'tests/op-run.test.ts', - 'tests/op-check-ref.test.ts', - ]; - - for (const file of files) { - let text = fs.readFileSync(file, 'utf8'); - text = text.replace( - /const originalTool = server\.tool\.bind\(server\);[\s\S]*?return originalTool\(\.\.\.args\);\n\s*\}\);/, - `const originalTool = server.registerTool.bind(server);\n vi.spyOn(server, "registerTool").mockImplementation(((...args: any[]) => {\n const [name, config, handler] = args;\n registeredTools.set(name, {\n description: config.description,\n schema: config.inputSchema,\n handler,\n });\n return originalTool(...(args as Parameters));\n }) as any);`, - ); - fs.writeFileSync(file, text); - } - - { - const file = 'tests/prompts.test.ts'; - let text = fs.readFileSync(file, 'utf8'); - text = text.replace( - /const originalPrompt = server\.prompt\.bind\(server\);[\s\S]*?return originalPrompt\(\.\.\.args\);\n\s*\}\);/, - `const originalPrompt = server.registerPrompt.bind(server);\n vi.spyOn(server, "registerPrompt").mockImplementation(((...args: any[]) => {\n const [name, config, handler] = args;\n registeredPrompts.set(name, {\n description: config.description,\n params: config.argsSchema,\n handler,\n });\n return originalPrompt(...(args as Parameters));\n }) as any);`, - ); - fs.writeFileSync(file, text); - } - NODE - - - name: Fail on unresolved codemod markers - run: | - if grep -R "@mcp-codemod-error" src tests package.json; then - exit 1 - fi - - - name: Validate migration - run: | - npm run lint - npm run build - npm test - - - name: Commit generated migration - run: | - if git diff --quiet; then - echo "No migration changes to commit." - exit 0 - fi - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git add package.json package-lock.json server.json CHANGELOG.md src tests - git commit -m "feat: support MCP protocol 2026-07-28 [mcp-v2-generated]" - git push origin HEAD:feat/mcp-2026-07-28 diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 3e7a03e..5087026 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -7,34 +7,36 @@ on: permissions: contents: read - id-token: write jobs: - publish: + # Runs dependency code (npm ci, build, tests) with a read-only token and no + # OIDC access, then hands the packed tarball to the publish job. + build: + name: Build, test, and pack runs-on: ubuntu-latest if: github.event_name == 'release' || github.event_name == 'workflow_dispatch' + permissions: + contents: read + outputs: + package_name: ${{ steps.pack.outputs.package_name }} + package_version: ${{ steps.pack.outputs.package_version }} steps: - - uses: actions/checkout@v6 + - name: Check out repository + uses: actions/checkout@v6 + with: + persist-credentials: false - name: Use Node.js 24 uses: actions/setup-node@v6 with: node-version: 24 - registry-url: https://registry.npmjs.org package-manager-cache: false - - name: Verify trusted-publishing toolchain - run: | - node --version - npm --version - node -e "const [major, minor] = process.versions.node.split('.').map(Number); if (major < 22 || (major === 22 && minor < 14)) process.exit(1)" - npm install --global npm@latest - npm --version - node -e "const { execSync } = require('child_process'); const version = execSync('npm --version', { encoding: 'utf8' }).trim().split('.').map(Number); if (version[0] < 11 || (version[0] === 11 && version[1] < 5) || (version[0] === 11 && version[1] === 5 && version[2] < 1)) process.exit(1)" - + # Dependency install scripts never run in the job that builds the + # published tarball; build and tests do not need them. - name: Install dependencies - run: npm ci + run: npm ci --ignore-scripts - name: Validate version alignment run: | @@ -53,9 +55,10 @@ jobs: - name: Validate release tag matches package version if: github.event_name == 'release' + env: + RELEASE_TAG: ${{ github.event.release.tag_name }} run: | PACKAGE_VERSION=$(node -p "require('./package.json').version") - RELEASE_TAG="${{ github.event.release.tag_name }}" EXPECTED_TAG="v${PACKAGE_VERSION}" echo "release tag: ${RELEASE_TAG}" @@ -66,12 +69,81 @@ jobs: exit 1 fi - - name: Check if version already exists on npm - id: npm_check + - name: Build + run: npm run build + + - name: Test + run: npm test + + - name: Pack npm tarball + id: pack run: | PACKAGE_NAME=$(node -p "require('./package.json').name") PACKAGE_VERSION=$(node -p "require('./package.json').version") + # Pack into a fresh directory so the new tarball is the only .tgz + # there, whatever npm names it, then give it a fixed name. + PACK_DIR=$(mktemp -d) + npm pack --ignore-scripts --pack-destination "${PACK_DIR}" + + set -- "${PACK_DIR}"/*.tgz + if [ "$#" -ne 1 ] || [ ! -f "$1" ]; then + echo "Expected exactly one tarball from npm pack." + exit 1 + fi + mv "$1" package.tgz + echo "Packed ${PACKAGE_NAME}@${PACKAGE_VERSION} as package.tgz" + + echo "package_name=${PACKAGE_NAME}" >> "$GITHUB_OUTPUT" + echo "package_version=${PACKAGE_VERSION}" >> "$GITHUB_OUTPUT" + + - name: Upload npm package artifact + uses: actions/upload-artifact@v7 + with: + name: npm-package + path: package.tgz + if-no-files-found: error + retention-days: 1 + + # The only job with OIDC access (npm trusted publishing). It never checks out + # the repository or installs/runs dependency code; it only publishes the + # tarball built above. + publish: + name: Publish to npm + needs: build + runs-on: ubuntu-latest + permissions: + contents: read + id-token: write + + steps: + - name: Use Node.js 24 + uses: actions/setup-node@v6 + with: + node-version: 24 + registry-url: https://registry.npmjs.org + package-manager-cache: false + + - name: Verify trusted-publishing toolchain + run: | + node --version + npm --version + node -e "const [major, minor] = process.versions.node.split('.').map(Number); if (major < 22 || (major === 22 && minor < 14)) process.exit(1)" + npm install --global npm@latest + npm --version + node -e "const { execSync } = require('child_process'); const version = execSync('npm --version', { encoding: 'utf8' }).trim().split('.').map(Number); if (version[0] < 11 || (version[0] === 11 && version[1] < 5) || (version[0] === 11 && version[1] === 5 && version[2] < 1)) process.exit(1)" + + - name: Download npm package artifact + uses: actions/download-artifact@v8 + with: + name: npm-package + + - name: Check if version already exists on npm + id: npm_check + env: + PACKAGE_NAME: ${{ needs.build.outputs.package_name }} + PACKAGE_VERSION: ${{ needs.build.outputs.package_version }} + run: | if npm view "${PACKAGE_NAME}@${PACKAGE_VERSION}" version >/dev/null 2>&1; then echo "already_published=true" >> "$GITHUB_OUTPUT" echo "Version ${PACKAGE_VERSION} is already published. Skipping publish." @@ -80,12 +152,6 @@ jobs: echo "Version ${PACKAGE_VERSION} is not published yet." fi - - name: Build - run: npm run build - - - name: Test - run: npm test - - name: Publish to npm with OIDC if: steps.npm_check.outputs.already_published == 'false' - run: npm publish --access public + run: npm publish ./package.tgz --access public --ignore-scripts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2176818..c5cf1dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,30 @@ All notable changes to this project will be documented in this file. 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.4] - 2026-10-04 + +### Security + +- **`item_get` no longer returns secrets held in non-`Concealed` fields** — Only `Concealed` fields were masked, so SSH private keys, one-time-password (TOTP) seeds, and card numbers came back in plaintext without `reveal: true`. Masking is now deny-by-default: only known non-secret field types (text, URL, email, phone, date, month/year, menu, card type, address, reference) are shown, and every other type, including any added by future SDK versions, needs `reveal: true`. Notes are still returned as-is, matching `op item get`; don't keep secrets in the notes of vaults an agent can read. +- **Vault allow-list is now enforced server-wide** — `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` only covered `op_run` and `op_check_ref`, and only compared the vault segment as written in an `op://` reference. It now applies to every tool and resource that touches a vault: `vault_list` and `1password://vaults` are filtered, and vault IDs are checked before anything is returned or modified. `op://` references are checked both as written and by the vault they actually resolve to. It fails closed if vaults can't be listed. +- **`op_run` redaction no longer leaks the remainder of overlapping secrets** — Redaction now masks every occurrence of every secret in a single pass over the original output, so overlapping or nested secrets can't leak: a username that is a prefix of a credential string is no longer replaced first, leaving the password visible. It also masks common encodings of each secret: base64 (including inside a larger base64 payload such as an HTTP Basic auth header), JSON-escaped and URL-encoded forms, and multi-line secrets line by line and with CRLF line endings. +- **`op_run` output cap no longer exhausts memory** — The 5 MiB per-stream cap is now applied while the command runs. Excess output is discarded instead of buffered, which fixes a memory-exhaustion crash, and a secret cut off at the cap never survives as a partial prefix. +- **`op_run` timeouts kill the whole process tree** — On timeout the process group (POSIX) or process tree via `taskkill /T` (Windows) is killed, and `op_run` always returns shortly afterwards, even if a background process holds the output pipes. Previously it could hang forever and leave processes running with injected secrets. +- **Case-insensitive credential scrub in `op_run`** — The server's own credential variables (`OP_SERVICE_ACCOUNT_TOKEN`, `OP_KEYCHAIN_SERVICE`, `OP_KEYCHAIN_ACCOUNT`) are now stripped from the child environment regardless of letter case. +- **Removed a leftover CI workflow and hardened publishing** — The one-time `mcp-v2-migration.yml` is deleted: any GitHub user could trigger it with a PR comment, and it ran `npm install` with a write-scoped token. `publish.yml` no longer interpolates the release tag into shell, checkouts no longer persist credentials, and publishing is split into two jobs: build, test, and pack run without OIDC access (and without dependency install scripts), and a separate job that alone has `id-token: write` publishes the prebuilt tarball with `--ignore-scripts`. +- **macOS Keychain lookup uses an absolute path** — The token lookup runs `/usr/bin/security` instead of resolving `security` through `PATH`, so a binary planted earlier on `PATH` can't hand the server an attacker-chosen service account token. +- **Warning when the token is passed on the command line** — `--service-account-token` / `--token` put the token in the process arguments, which other local processes (including commands run through `op_run`) can read. The server now logs a startup warning; prefer `OP_SERVICE_ACCOUNT_TOKEN` or, on macOS, the Keychain. +- All of the above came out of an internal security review. + +### Changed + +- **Behavior change: the vault allow-list is server-wide** — If you set `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` expecting it to affect only `op_run` and `op_check_ref`, it now restricts every tool and resource. Tools that take a `vaultId` need the vault's ID, not its name. With an allow-list set, each guarded call makes one extra `vaults.list` read (mind service account rate limits). It is defense in depth; scope the service account's own vault access in 1Password first. +- **`item_get` hides more by default** — SSH private keys, OTP seeds, and card numbers now show `[concealed]` unless you pass `reveal: true`. +- **Stricter `op://` validation** — Malformed references passed to `item_get` and `password_read` are now rejected locally with a clear error. +- **More candid `op_run` description** — The tool description no longer claims plaintext is "NEVER" returned. Redaction is best effort and protects against accidental disclosure; it is not a sandbox, because a command can deliberately transform or transmit a secret it was given. +- **Publishing** — `prepublishOnly` now only matters for manual `npm publish`; the automated workflow publishes a prebuilt tarball. +- **Documentation** — README, CONTRIBUTING, and agents.md cover the server-wide allow-list, best-effort redaction, unconcealed notes, and the two-job publish workflow. + ## [4.0.3] - 2026-10-04 ### Security diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ac6f12d..4125ff7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,7 +39,9 @@ src/ ├── logger.ts # Structured logging to stderr ├── config.ts # CLI args, env vars, Keychain, allow-list ├── client.ts # 1Password SDK client singleton -├── secret-ref.ts # op:// parsing and vault allow-list +├── secret-ref.ts # op:// parsing and reference checks +├── vault-access.ts # Server-wide vault allow-list enforcement +├── redaction.ts # op_run output redaction ├── utils.ts # Result helpers, password generation ├── tools/ # MCP tool handlers (15) │ ├── index.ts @@ -68,11 +70,14 @@ tests/ ├── tools.test.ts ├── prompts.test.ts ├── secret-ref.test.ts +├── vault-access.test.ts +├── vault-allowlist.test.ts +├── redaction.test.ts ├── op-run.test.ts └── op-check-ref.test.ts ``` -Version must stay aligned across `package.json`, `server.json`, and `SERVER_VERSION` in `src/config.ts`. See [AGENTS.md](AGENTS.md). +Version must stay aligned across `package.json`, `package-lock.json`, `server.json`, and `SERVER_VERSION` in `src/config.ts`. See [agents.md](agents.md). ## Guidelines @@ -80,6 +85,7 @@ Version must stay aligned across `package.json`, `server.json`, and `SERVER_VERS - **Errors** — Use `errorResult()` from `utils.ts` for tool failures; set protocol-friendly error responses. - **Logging** — Use `log()` / `logError()` from `logger.ts`. Never write to `stdout` (reserved for MCP). - **Secrets** — Default to metadata-only responses. New tools that can expose plaintext must opt in explicitly (e.g. `reveal` / `returnSecret`). Prefer documenting `op_run` for “use without reveal.” +- **Vault allow-list** — Anything that reads or writes a vault must go through the helpers in `vault-access.ts` (`assertVaultIdAllowed`, `filterAllowedVaults`) so `OP_MCP_ALLOWED_VAULTS` applies server-wide. - **Schemas** — Tool/prompt inputs use Zod 4 and the MCP v2 registration APIs. - **Tests** — Add or update Vitest coverage for new tools, prompts, and utilities. - **Commits** — [Conventional Commits](https://www.conventionalcommits.org/) (e.g. `feat: add item_archive tool`, `docs: refresh README for MCP 2026-07-28`). @@ -93,16 +99,16 @@ Version must stay aligned across `package.json`, `server.json`, and `SERVER_VERS ## Release process (maintainers) -Automated publish runs from GitHub Releases via `publish.yml` (trusted publishing). Manual steps: +Automated publish runs from GitHub Releases via `publish.yml` (trusted publishing). It has two jobs: `build` installs dependencies without running their install scripts, validates versions and the tag, builds, tests, and packs the tarball without any OIDC access, and a separate `publish` job, the only one with `id-token: write`, publishes that prebuilt tarball (`npm publish ./package.tgz --ignore-scripts`). Maintainer steps: -1. Bump version in `package.json`, `server.json`, and `src/config.ts` (`SERVER_VERSION`). +1. Bump version in `package.json`, `package-lock.json`, `server.json`, and `src/config.ts` (`SERVER_VERSION`). 2. Update `CHANGELOG.md`. 3. Merge to `master`, then create a GitHub Release tagged `vX.Y.Z` matching the package version. 4. Confirm the publish workflow succeeds on npm. CI (`ci.yml`) builds and tests on push/PR to `master`. The published package requires **Node ≥ 20**. -For agent-oriented publish checklists, see [AGENTS.md](AGENTS.md). +For agent-oriented publish checklists, see [agents.md](agents.md). ## License diff --git a/README.md b/README.md index 230a956..77c7661 100644 --- a/README.md +++ b/README.md @@ -28,8 +28,8 @@ Built on the **MCP TypeScript SDK v2** with protocol negotiation for **[2026-07- ## Why teams pick this server -- **Security-first defaults** — `password_read` and `item_get` return metadata unless you opt in with `reveal: true`. -- **`op_run` (the MCP equivalent of `op run`)** — inject `op://vault/item/field` into a local command’s environment; plaintext is redacted from stdout/stderr and never logged back to the model. +- **Security-first defaults** — `password_read` returns metadata, and `item_get` hides secret-bearing fields (passwords, SSH keys, OTP seeds, card numbers), unless you opt in with `reveal: true`. +- **`op_run` (the MCP equivalent of `op run`)** — inject `op://vault/item/field` into a local command’s environment; resolved secrets are redacted from the returned stdout/stderr on a best-effort basis (including common encodings). - **Full vault toolkit** — list, search, get, edit, create logins & notes, rotate passwords, archive, or delete. - **Guided prompts** — password generation, credential rotation, vault audit, and secret-reference helpers. - **Browsable resources** — vault and item catalogs over `1password://…` URIs (no secrets in resource payloads). @@ -47,7 +47,7 @@ Grouped the way agents and humans actually use them. | Tool | What it does | |------|----------------| -| `vault_list` | List vaults the service account can access (id, name, description, type). | +| `vault_list` | List vaults the service account can access (id, name, description, type), limited to the allow-list if one is set. | | `item_lookup` | Search a vault by title substring; optional `limit` (max 200). | | `item_list` | List every item in a vault (id, title, category, tags, `updatedAt`) — never secrets. | @@ -55,7 +55,7 @@ Grouped the way agents and humans actually use them. | Tool | What it does | |------|----------------| -| `item_get` | Full item: title, category, tags, notes, fields. Concealed values stay hidden unless `reveal: true`. Accepts `op://…` **or** `vaultId` + `itemId`. | +| `item_get` | Full item: title, category, tags, notes, fields. Secret-bearing values (passwords and other concealed fields, SSH private keys, OTP seeds, card numbers) stay hidden unless `reveal: true`; only known non-secret field types are shown. **Notes are returned as-is.** Accepts `op://…` **or** `vaultId` + `itemId`. | | `password_read` | Read one field (default `password`) via `op://…` or ids. **Metadata-only unless `reveal: true`.** Prefer `op_run` to *use* a secret. | | `op_check_ref` | Validate `op://vault/item/field` and return non-secret metadata only (vault, item, field). Never the value. | @@ -74,7 +74,7 @@ Grouped the way agents and humans actually use them. | Tool | What it does | |------|----------------| -| `op_run` | Run a local command (`command` **or** `argv`) with env vars. Values matching `op://…` are resolved into the **child process only**; resolved secrets are redacted from returned output. Optional `cwd`, `shell`, `timeout_ms`, `stdin`. | +| `op_run` | Run a local command (`command` **or** `argv`) with env vars. Values matching `op://…` are resolved into the **child process only**; resolved secrets are redacted from returned output (best effort — see Security & privacy). Output is capped at 5 MiB per stream, and a timeout kills the whole process tree. Optional `cwd`, `shell`, `timeout_ms`, `stdin`. | #### Soft-delete & destroy @@ -97,7 +97,7 @@ Grouped the way agents and humans actually use them. | URI | Contents | |-----|----------| | `1password://config` | Non-secret server config (name, version, log level, token source, Node version). | -| `1password://vaults` | JSON list of accessible vaults. | +| `1password://vaults` | JSON list of accessible vaults (limited to the allow-list if one is set). | | `1password://vaults/{vaultId}/items` | JSON item metadata for one vault (no secret values). | --- @@ -159,7 +159,7 @@ Store the token in Keychain, then point the server at it: } ``` -**Token resolution order:** CLI (`--service-account-token` / `--token`) → `OP_SERVICE_ACCOUNT_TOKEN` → macOS Keychain. `OP_KEYCHAIN_ACCOUNT` is optional when the service name alone is unique. +**Token resolution order:** CLI (`--service-account-token` / `--token`) → `OP_SERVICE_ACCOUNT_TOKEN` → macOS Keychain. `OP_KEYCHAIN_ACCOUNT` is optional when the service name alone is unique. Avoid the CLI flags: command-line arguments are visible to other local processes, and the server logs a warning at startup if you use them. ### OpenAI Codex (TOML) @@ -187,9 +187,9 @@ Set `OP_SERVICE_ACCOUNT_TOKEN` in your shell or CI. Note: `codex mcp add ... --e On macOS you can omit the token env and use `OP_KEYCHAIN_SERVICE` (+ optional `OP_KEYCHAIN_ACCOUNT`) instead. -### Optional: lock `op_run` / `op_check_ref` to certain vaults +### Optional: restrict the server to certain vaults -By default those tools may resolve `op://` references from any vault the service account can see. To allow-list vaults: +By default the server can use any vault the service account can see. To allow-list vaults: ```json { @@ -200,7 +200,16 @@ By default those tools may resolve `op://` references from any vault the service } ``` -Names or IDs work. References outside the list are rejected before resolution. Same setting via `--allowed-vaults`. +Names or IDs work (case-insensitive). Same setting via `--allowed-vaults`. The allow-list applies **server-wide**, to every tool and resource that touches a vault, not only `op_run` / `op_check_ref`: + +- `vault_list` and `1password://vaults` show only allowed vaults. +- Tools that take a `vaultId` must be given the vault’s ID (not its name); vaults outside the list are rejected. +- `op://` references are checked both as written and by the vault they actually resolve to. +- When an allow-list is set, each guarded call makes one extra `vaults.list` read (mind service-account rate limits), and the server fails closed if vaults can’t be listed. + +> **Upgrading from 4.0.3 or earlier?** If you set this expecting it to affect only `op_run` / `op_check_ref`, it now restricts everything. + +This is defense in depth: **scope the service account’s own vault access in 1Password first.** --- @@ -240,7 +249,7 @@ Prefer `argv` over a shell `command` string when you can — fewer quoting surpr | `OP_SERVICE_ACCOUNT_TOKEN` | Usually yes | Service account token. Not required on macOS if Keychain vars are set. | | `OP_KEYCHAIN_SERVICE` | No | macOS: Keychain service name for the token. | | `OP_KEYCHAIN_ACCOUNT` | No | macOS: optional account to narrow the Keychain lookup. | -| `OP_MCP_ALLOWED_VAULTS` | No | Comma-separated vault names/IDs allowed for `op_run` / `op_check_ref`. Empty = unrestricted. | +| `OP_MCP_ALLOWED_VAULTS` | No | Comma-separated vault names/IDs (case-insensitive) the server may use, enforced server-wide. Empty = unrestricted. | | `OP_INTEGRATION_NAME` | No | Name reported to the 1Password SDK (default: `1password-mcp`). | | `OP_INTEGRATION_VERSION` | No | Version reported to the SDK (default: package version). | | `MCP_LOG_LEVEL` | No | `debug` \| `info` \| `warn` \| `error` (default: `info`). | @@ -249,12 +258,12 @@ Prefer `argv` over a shell `command` string when you can — fewer quoting surpr ### CLI flags ``` ---service-account-token 1Password service account token +--service-account-token 1Password service account token (avoid: visible to other local processes) --token Alias for --service-account-token --log-level error | warn | info | debug (default: info) --integration-name Custom integration name for the 1Password SDK --integration-version Custom integration version ---allowed-vaults Comma-separated allow-list for op_run / op_check_ref +--allowed-vaults Comma-separated vault allow-list (names or IDs), applied server-wide ``` --- @@ -268,8 +277,12 @@ Prefer `argv` over a shell `command` string when you can — fewer quoting surpr - **Best fit** — Automation credentials: CI tokens, bot accounts, disposable env secrets. - **Avoid** — Banking, primary personal logins, recovery codes, or anything you cannot afford to expose to a model provider. - **Token = master key** — Scope the service account tightly; rotate immediately if leaked; never commit tokens or MCP configs with secrets. +- **Prefer the env var or Keychain for the token** — `--token` / `--service-account-token` puts it in the process arguments, which other local processes can read (the server warns at startup). A same-user process can generally read the server’s environment too (for example `/proc//environ` on Linux), so on macOS Keychain is the strongest option: it keeps the token out of config files and the process environment. - **Prefer references** — `op://…` + `op_run` beat pasting passwords into prompts or files. -- **Least privilege** — Dedicated automation vaults beat sharing your whole account. +- **`op_run` runs arbitrary commands as your user** — Keep your MCP client’s approval prompts on for it; don’t auto-approve it. +- **Redaction is best effort** — `op_run` masks resolved secrets in returned output, including common encodings (base64, JSON- and URL-encoded forms, multi-line values). That protects against accidental disclosure. It is not a sandbox: a command can deliberately transform or transmit a secret it was given. +- **Notes are not concealed** — `item_get` returns notes as-is (like `op item get`). Don’t keep secrets in the notes of vaults an agent can read. +- **Least privilege** — Dedicated automation vaults beat sharing your whole account. `OP_MCP_ALLOWED_VAULTS` is a second fence, not a substitute. - **Reporting vulnerabilities** — Open a public issue; see [SECURITY.md](SECURITY.md). --- @@ -308,7 +321,9 @@ src/ config.ts # CLI / env / Keychain / allow-list client.ts # 1Password SDK client logger.ts # Structured logs on stderr (stdout is protocol) - secret-ref.ts # op:// parsing & allow-list checks + secret-ref.ts # op:// parsing & reference checks + vault-access.ts # Server-wide vault allow-list enforcement + redaction.ts # op_run output redaction utils.ts # Result helpers, password generation tools/ # All 15 MCP tools prompts/ # Interactive workflow prompts @@ -316,13 +331,13 @@ src/ tests/ ``` -See [CONTRIBUTING.md](CONTRIBUTING.md). Maintainers / agents: [AGENTS.md](AGENTS.md). +See [CONTRIBUTING.md](CONTRIBUTING.md). Maintainers / agents: [agents.md](agents.md). --- ## Changelog -See [CHANGELOG.md](CHANGELOG.md) for version history, including the **4.0.0** MCP v2 / 2026-07-28 migration and the **3.0.0** `op_run` / reveal-opt-in security changes. +See [CHANGELOG.md](CHANGELOG.md) for version history, including the **4.0.4** security release (read its **Changed** notes before upgrading), the **4.0.0** MCP v2 / 2026-07-28 migration, and the **3.0.0** `op_run` / reveal-opt-in security changes. --- diff --git a/agents.md b/agents.md index 0b7df26..d31edb6 100644 --- a/agents.md +++ b/agents.md @@ -15,11 +15,12 @@ Follow [Semantic Versioning](https://semver.org/). Update the version in **all** of: 1. `package.json` -2. `server.json` -3. `src/config.ts` (`SERVER_VERSION`) -4. `CHANGELOG.md` +2. `package-lock.json` (root `version` and `packages[""].version`) +3. `server.json` (top-level `version` and `packages[0].version`) +4. `src/config.ts` (`SERVER_VERSION`) +5. `CHANGELOG.md` -The publish workflow fails if those three version strings disagree, or if a GitHub Release tag is not `v` + package version. +The publish workflow fails if the `package.json`, `server.json`, and `src/config.ts` versions disagree (it does not check the lockfile or changelog), or if a GitHub Release tag is not `v` + package version. ## Build and validation @@ -41,7 +42,7 @@ Runtime requirement: **Node.js ≥ 20**. 1. Merge version + changelog to `master`. 2. Create a GitHub Release on `master` with tag `vX.Y.Z`. -3. `publish.yml` builds, validates versions, and publishes (trusted publishing / `NPM_TOKEN` as configured). +3. `publish.yml` runs two jobs (see CI/CD): `build` installs, validates versions and the tag, builds, tests, and packs the tarball with no OIDC access; `publish` then publishes that tarball via npm trusted publishing (OIDC, no `NPM_TOKEN`). ### Manual @@ -50,7 +51,7 @@ npm login npm publish --access public ``` -`prepublishOnly` runs `clean`, `build`, and `test` before upload. +`prepublishOnly` runs `clean`, `build`, and `test` before a manual `npm publish`. The automated workflow publishes a prebuilt tarball (`npm publish ./package.tgz --ignore-scripts`), which skips lifecycle scripts, so its `build` job runs build and test instead. ## Configuration variables @@ -59,13 +60,13 @@ npm publish --access public | `OP_SERVICE_ACCOUNT_TOKEN` | Primary auth. Required unless macOS Keychain fallback is used. | | `OP_KEYCHAIN_SERVICE` | macOS only: Keychain service name for the token. | | `OP_KEYCHAIN_ACCOUNT` | macOS only: optional account for Keychain lookup. | -| `OP_MCP_ALLOWED_VAULTS` | Optional comma-separated vault names/IDs for `op_run` / `op_check_ref`. | +| `OP_MCP_ALLOWED_VAULTS` | Optional comma-separated vault names/IDs (case-insensitive). Enforced server-wide by every tool and resource that touches a vault; tools that take a `vaultId` need an ID. | | `OP_INTEGRATION_NAME` | Optional; default `1password-mcp`. | | `OP_INTEGRATION_VERSION` | Optional; default `SERVER_VERSION`. | | `MCP_LOG_LEVEL` | Optional: `debug`, `info`, `warn`, `error` (default `info`). | | `MCP_DEBUG` | Optional; if set, forces debug logging. | -CLI equivalents: `--service-account-token` / `--token`, `--log-level`, `--integration-name`, `--integration-version`, `--allowed-vaults`. +CLI equivalents: `--service-account-token` / `--token`, `--log-level`, `--integration-name`, `--integration-version`, `--allowed-vaults`. Avoid the token flags: argv is visible to other local processes, and the server logs a warning at startup. ## Public surface (keep docs in sync) @@ -81,10 +82,17 @@ When tools, prompts, or resources change, update **README.md** (npm’s face), * - Prefer `op_run` + `op://` over `reveal: true`. - Prefer `op_check_ref` over revealing just to validate a path. - Default create/update responses should not echo secrets (`returnSecret` / `reveal` opt-in). +- The vault allow-list is server-wide: any new tool or resource that touches a vault must go through `src/vault-access.ts` (`assertVaultIdAllowed` / `filterAllowedVaults`). +- `op_run` redaction is best effort, not a sandbox: don't run untrusted commands with injected secrets. +- `item_get` returns notes as-is, so don't store secrets in notes. - Never commit tokens or MCP configs containing secrets. ## CI/CD - `ci.yml` — build/test on push and PRs to `master`. -- `publish.yml` — npm publish on GitHub Release / manual dispatch. +- `publish.yml` — npm publish on GitHub Release / manual dispatch, as two jobs: + - `build` (`contents: read`, no OIDC): `npm ci --ignore-scripts` (no dependency install scripts), version and tag validation, build, test, then `npm pack --ignore-scripts` into `package.tgz`, uploaded as the `npm-package` artifact. + - `publish` (`needs: build`): the only job with `id-token: write`. It never checks out the repo or installs dependencies; it downloads the tarball and runs `npm publish ./package.tgz --access public --ignore-scripts` (skipped if that version is already on npm). + - Keep it that way: dependency code must never run in a job that holds `id-token: write`. npm's trusted-publisher config is bound to the file name `publish.yml`, so don't rename it. +- Workflow hygiene: pass `${{ ... }}` values to scripts through `env:` instead of interpolating them into `run:`, check out with `persist-credentials: false`, and don't add `issue_comment`-triggered or write-scoped one-off runner workflows (the leftover `mcp-v2-migration.yml` was removed in 4.0.4 for that reason). - Registry package name: `io.github.CakeRepository/1password` (`mcpName` / `server.json`). diff --git a/package-lock.json b/package-lock.json index 9f68653..ae8e85a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "4.0.4", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "4.0.4", "license": "Apache-2.0", "dependencies": { "@1password/sdk": "^0.3.1", diff --git a/package.json b/package.json index 71dd906..b8ef503 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "4.0.4", "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 227d9ad..744e104 100644 --- a/server.json +++ b/server.json @@ -6,12 +6,12 @@ "url": "https://github.com/CakeRepository/1Password-MCP.git", "source": "github" }, - "version": "4.0.3", + "version": "4.0.4", "packages": [ { "registryType": "npm", "identifier": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "4.0.4", "transport": { "type": "stdio" }, @@ -39,7 +39,7 @@ }, { "name": "OP_MCP_ALLOWED_VAULTS", - "description": "Optional comma-separated vault names or IDs that op_run and op_check_ref may resolve (empty = unrestricted)", + "description": "Optional comma-separated vault names or IDs (case-insensitive) the server may use. Enforced by every tool and resource that touches a vault (empty = unrestricted)", "isRequired": false, "isSecret": false, "format": "string" diff --git a/src/config.ts b/src/config.ts index b44df0f..f72b2e2 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.3"; +export const SERVER_VERSION = "4.0.4"; /** Parse a `--flag value` or `--flag=value` argument from process.argv. */ function getArgValue(name: string): string | undefined { @@ -34,15 +34,17 @@ export interface ServerConfig { /** 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). + * Vault names/IDs (case-insensitive) the server may read from or write to. + * Enforced server-wide: by every tool and resource that touches a vault (see + * `vault-access.ts`) and, for `op://` references, by `secret-ref.ts`. 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". + * empty list, which the vault guards treat as "no restriction". */ export function parseAllowedVaults(raw: string | undefined): string[] { if (!raw || !raw.trim()) return []; @@ -54,6 +56,14 @@ export function parseAllowedVaults(raw: string | undefined): string[] { let _config: ServerConfig | undefined; +/** + * macOS `security` CLI, invoked by absolute path rather than resolved through + * `PATH`: a binary planted earlier on `PATH` could otherwise hand the server an + * attacker-chosen service-account token and silently redirect secrets into the + * attacker's 1Password account. + */ +const MACOS_SECURITY_BINARY = "/usr/bin/security"; + interface MacOsKeychainLookupOptions { service?: string; account?: string; @@ -74,7 +84,7 @@ export function readMacOsKeychainToken({ args.push("-s", service, "-w"); try { - const token = execFileSyncImpl("security", args, { + const token = execFileSyncImpl(MACOS_SECURITY_BINARY, args, { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], }).trim(); @@ -118,6 +128,23 @@ export function resolveServiceAccountToken({ return { serviceAccountToken, tokenSource }; } +/** + * Startup warning for a weak token source, or `undefined` when none applies. + * Takes only the source (never the token) so the message cannot leak the secret. + */ +export function getTokenSourceWarning( + tokenSource: ServerConfig["tokenSource"], +): string | undefined { + if (tokenSource !== "args") return undefined; + return ( + "Service account token was passed on the command line " + + "(--service-account-token / --token). Command-line arguments are visible " + + "to other local processes (process listings, /proc//cmdline), " + + "including commands run via op_run. Prefer the OP_SERVICE_ACCOUNT_TOKEN " + + "environment variable or, on macOS, the Keychain (OP_KEYCHAIN_SERVICE)." + ); +} + /** Build and cache the server configuration. */ export function getConfig(): ServerConfig { if (_config) return _config; diff --git a/src/index.ts b/src/index.ts index b7f86aa..f377b0e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,7 +8,12 @@ import { McpServer } from "@modelcontextprotocol/server"; import { serveStdio } from "@modelcontextprotocol/server/stdio"; -import { SERVER_NAME, SERVER_VERSION, getConfig } from "./config.js"; +import { + SERVER_NAME, + SERVER_VERSION, + getConfig, + getTokenSourceWarning, +} from "./config.js"; import { log, logError } from "./logger.js"; import { registerAllTools } from "./tools/index.js"; import { registerAllPrompts } from "./prompts/index.js"; @@ -47,6 +52,9 @@ async function main(): Promise { tokenSource: config.tokenSource, }); + const tokenWarning = getTokenSourceWarning(config.tokenSource); + if (tokenWarning) log("warn", tokenWarning); + log("info", "Starting MCP stdio protocol negotiation."); await serveStdio(() => buildServer()); } diff --git a/src/redaction.ts b/src/redaction.ts new file mode 100644 index 0000000..6b5a14f --- /dev/null +++ b/src/redaction.ts @@ -0,0 +1,292 @@ +/** + * Output redaction for op_run: builds the strings that must not reach the + * caller (each resolved secret plus the encodings tools commonly emit for it), + * masks them in captured output, and enforces the per-stream size cap without + * ever exposing a secret that the cap cut in half. + * + * Everything here is pure so it can be unit-tested without spawning processes. + */ + +export interface RedactionTarget { + /** Env var name shown in the redaction marker. */ + name: string; + /** Plaintext secret value. */ + value: string; +} + +/** A literal string to mask. Derived encodings of a secret carry that secret's name. */ +export type RedactionPattern = RedactionTarget; + +export interface FinalizedOutput { + text: string; + truncated: boolean; +} + +/** Shortest base64 fragment or secret line distinctive enough to mask on its own. */ +const MIN_PARTIAL_LENGTH = 8; + +/** + * Leading base64 characters that mix in the bytes before the secret, indexed by + * the secret's offset within its 3-byte group. + */ +const BASE64_UNSTABLE_PREFIX = [0, 2, 3] as const; + +/** + * Base64 substrings that appear whenever `value` is embedded at any offset of a + * larger base64-encoded payload (e.g. `Authorization: Basic ...`, k8s secrets). + */ +function base64Fragments(value: string): string[] { + const secret = Buffer.from(value, "utf8"); + const fragments: string[] = []; + for (let shift = 0; shift < 3; shift++) { + const bytes = Buffer.concat([Buffer.alloc(shift), secret]); + let encoded = bytes.toString("base64").replace(/=+$/, ""); + // A trailing partial group is completed by whichever byte follows the secret. + if (bytes.length % 3 !== 0) encoded = encoded.slice(0, -1); + // The leading characters mix in the unknown bytes before the secret. + encoded = encoded.slice(BASE64_UNSTABLE_PREFIX[shift]); + if (encoded.length >= MIN_PARTIAL_LENGTH) fragments.push(encoded); + } + return fragments; +} + +/** + * Forms of a multi-line secret that survive re-wrapping: the CRLF rendering and + * each long-enough line on its own (mirrors per-line masking in CI systems). + */ +function* multilineVariants(value: string): Generator { + yield value.replace(/\r?\n/g, "\r\n"); + for (const line of value.split("\n")) { + const trimmed = line.trim(); + if (trimmed.length >= MIN_PARTIAL_LENGTH) yield trimmed; + } +} + +/** Every literal form of one secret value that should be masked. */ +function* secretVariants(value: string): Generator { + yield value; + yield JSON.stringify(value).slice(1, -1); + try { + yield encodeURIComponent(value); + } catch { + // URIError for lone surrogates: such a value has no URL-encoded form. + } + yield* base64Fragments(value); + if (value.includes("\n")) yield* multilineVariants(value); +} + +/** + * Expand secrets into the full, de-duplicated list of strings to mask. Empty + * values are ignored and, when two secrets share a form, the first name wins. + */ +export function buildRedactionPatterns( + targets: readonly RedactionTarget[], +): RedactionPattern[] { + const patterns: RedactionPattern[] = []; + const seen = new Set(); + for (const { name, value } of targets) { + if (value.length === 0) continue; + for (const variant of secretVariants(value)) { + if (variant.length === 0 || seen.has(variant)) continue; + seen.add(variant); + patterns.push({ name, value: variant }); + } + } + return patterns; +} + +/** Longest pattern in UTF-8 bytes: how much extra output must be kept past the cap. */ +export function maxPatternByteLength(patterns: readonly RedactionPattern[]): number { + let max = 0; + for (const { value } of patterns) { + max = Math.max(max, Buffer.byteLength(value, "utf8")); + } + return max; +} + +interface Cursor { + /** Position in the pattern list; earlier patterns win ties so names follow declaration order. */ + order: number; + name: string; + value: string; + /** Start of this pattern's next unconsumed occurrence in the text. */ + next: number; +} + +/** Index of the cursor whose next occurrence starts first, or -1 when none remain. */ +function earliest(cursors: readonly Cursor[]): number { + let best = -1; + for (let i = 0; i < cursors.length; i++) { + if ( + best === -1 || + cursors[i].next < cursors[best].next || + (cursors[i].next === cursors[best].next && cursors[i].order < cursors[best].order) + ) { + best = i; + } + } + return best; +} + +/** Move a cursor to its next occurrence (overlaps included), dropping it when exhausted. */ +function advance(cursors: Cursor[], index: number, text: string): void { + const cursor = cursors[index]; + cursor.next = text.indexOf(cursor.value, cursor.next + 1); + if (cursor.next === -1) { + cursors[index] = cursors[cursors.length - 1]; + cursors.pop(); + } +} + +/** + * Mask every occurrence of every pattern. Occurrences are located in the + * original text, so the result does not depend on pattern order, and + * overlapping or touching occurrences collapse into one marker. A marker names + * the unique patterns it covers in order of first appearance: `«REDACTED:A»` + * or `«REDACTED:A,B»`. + * + * Nothing at or after `limit` is emitted, except that a masked run starting + * before `limit` is always emitted whole (its marker is safe and the plaintext + * it replaced must not be partially shown). + */ +export function redact( + text: string, + patterns: readonly RedactionPattern[], + limit: number = text.length, +): string { + const cursors: Cursor[] = []; + patterns.forEach(({ name, value }, order) => { + const next = value.length === 0 ? -1 : text.indexOf(value); + if (next !== -1) cursors.push({ order, name, value, next }); + }); + if (cursors.length === 0) return limit >= text.length ? text : text.slice(0, limit); + + const parts: string[] = []; + let copied = 0; + while (cursors.length > 0) { + let index = earliest(cursors); + const start = cursors[index].next; + if (start >= limit) break; + + // Absorb every occurrence that overlaps or touches the run so far. + let end = start; + const names: string[] = []; + while (index !== -1 && cursors[index].next <= end) { + const cursor = cursors[index]; + end = Math.max(end, cursor.next + cursor.value.length); + if (!names.includes(cursor.name)) names.push(cursor.name); + advance(cursors, index, text); + index = earliest(cursors); + } + + if (start > copied) parts.push(text.slice(copied, start)); + parts.push(`«REDACTED:${names.join(",")}»`); + copied = end; + } + if (copied < limit) parts.push(text.slice(copied, limit)); + return parts.join(""); +} + +/** Drop a trailing UTF-8 sequence that was cut off mid-character. */ +function trimIncompleteUtf8(buffer: Buffer): Buffer { + const length = buffer.length; + for (let back = 1; back <= Math.min(3, length); back++) { + const byte = buffer[length - back]; + if ((byte & 0xc0) === 0x80) continue; // continuation byte: keep looking for its lead byte + const needed = byte >= 0xf0 ? 4 : byte >= 0xe0 ? 3 : byte >= 0xc0 ? 2 : 1; + return needed > back ? buffer.subarray(0, length - back) : buffer; + } + return buffer; +} + +/** Length of the longest suffix of `text` that is a proper prefix of `pattern` (KMP). */ +function partialMatchLength(text: string, pattern: string): number { + const max = Math.min(pattern.length - 1, text.length); + if (max < 1) return 0; + + const failure = new Int32Array(max); + for (let i = 1, k = 0; i < max; i++) { + while (k > 0 && pattern.charCodeAt(i) !== pattern.charCodeAt(k)) k = failure[k - 1]; + if (pattern.charCodeAt(i) === pattern.charCodeAt(k)) k++; + failure[i] = k; + } + + let matched = 0; + for (let i = text.length - max; i < text.length; i++) { + const code = text.charCodeAt(i); + while (matched > 0 && code !== pattern.charCodeAt(matched)) matched = failure[matched - 1]; + if (code === pattern.charCodeAt(matched)) matched++; + } + return matched; +} + +/** Longest tail of `text` that could be the start of a secret the output was cut inside. */ +function partialSecretLength(text: string, patterns: readonly RedactionPattern[]): number { + let longest = 0; + for (const { value } of patterns) { + longest = Math.max(longest, partialMatchLength(text, value)); + } + return longest; +} + +/** + * Turn captured bytes into the text returned to the caller: decode, mask, and + * cap at `maxBytes` on a character boundary. `overflowed` means the capture + * dropped further output, so the buffer ends at an arbitrary point and may + * end inside a secret. Both are handled: + * + * - an incomplete trailing UTF-8 sequence is trimmed before decoding, so a cut + * character cannot become U+FFFD and hide the start of a secret; + * - the tail that forms a proper prefix of any pattern is dropped. This is + * decided on the decoded text and applied during masking, so it also removes + * such a tail when it overlaps a masked run. It matters even though the + * capture keeps extra bytes: markers are shorter than what they replace, which + * pulls the end of the window back inside the cap. + */ +export function finalizeOutput( + captured: Buffer, + overflowed: boolean, + patterns: readonly RedactionPattern[], + maxBytes: number, +): FinalizedOutput { + const text = (overflowed ? trimIncompleteUtf8(captured) : captured).toString("utf8"); + const limit = overflowed ? text.length - partialSecretLength(text, patterns) : text.length; + const redacted = redact(text, patterns, limit); + + if (Buffer.byteLength(redacted, "utf8") <= maxBytes) { + return { text: redacted, truncated: overflowed }; + } + const capped = trimIncompleteUtf8(Buffer.from(redacted, "utf8").subarray(0, maxBytes)); + return { text: capped.toString("utf8"), truncated: true }; +} + +/** + * Collects a child's output up to a fixed byte budget. Bytes past the budget + * are discarded (the caller keeps consuming so the child never blocks on a + * full pipe) and recorded as `overflowed`. + */ +export class OutputCapture { + overflowed = false; + private readonly limit: number; + private readonly chunks: Buffer[] = []; + private size = 0; + + constructor(limit: number) { + this.limit = limit; + } + + push(chunk: Buffer): void { + const room = this.limit - this.size; + if (chunk.length > room) { + this.overflowed = true; + if (room <= 0) return; + chunk = chunk.subarray(0, room); + } + this.chunks.push(chunk); + this.size += chunk.length; + } + + toBuffer(): Buffer { + return Buffer.concat(this.chunks, this.size); + } +} diff --git a/src/resources/index.ts b/src/resources/index.ts index 2d6e98d..279af06 100644 --- a/src/resources/index.ts +++ b/src/resources/index.ts @@ -5,6 +5,7 @@ import type { McpServer } from "@modelcontextprotocol/server"; import { getClient } from "../client.js"; import { getConfig, SERVER_NAME, SERVER_VERSION } from "../config.js"; import { log, logError } from "../logger.js"; +import { assertVaultIdAllowed, filterAllowedVaults } from "../vault-access.js"; /** Register all MCP resources on the server. */ export function registerAllResources(server: McpServer): void { @@ -51,7 +52,7 @@ export function registerAllResources(server: McpServer): void { "1password://vaults", { description: - "List of all 1Password vaults accessible to the service account.", + "List of the 1Password vaults accessible to the service account (limited to the allow-listed vaults when OP_MCP_ALLOWED_VAULTS / --allowed-vaults is configured).", mimeType: "application/json", }, async () => { @@ -62,8 +63,9 @@ export function registerAllResources(server: McpServer): void { if (!listFn) { throw new Error("Cannot list vaults with this SDK version."); } - const vaults = await listFn.call(client.vaults); - const summary = (vaults ?? []).map((vault: any) => ({ + const vaults: any[] = (await listFn.call(client.vaults)) ?? []; + const visibleVaults = filterAllowedVaults(vaults); + const summary = visibleVaults.map((vault: any) => ({ id: vault.id, name: vault.name ?? vault.title, description: vault.description, @@ -121,6 +123,7 @@ export function registerAllResources(server: McpServer): void { log("debug", "Resource: vault-items.", { vaultId }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); const listFn = client?.items?.list ?? (client?.items as any)?.listAll; if (!listFn) { diff --git a/src/secret-ref.ts b/src/secret-ref.ts index 877a8cc..0d36405 100644 --- a/src/secret-ref.ts +++ b/src/secret-ref.ts @@ -1,6 +1,10 @@ /** * Shared helpers for parsing and validating `op://vault/item/field` secret - * references, used by `op_run` and `op_check_ref`. + * references, used by `op_run`, `op_check_ref`, `item_get`, and `password_read`. + * + * `assertVaultAllowed` is only a textual pre-check on the vault segment as + * written in a reference. The vault a reference actually resolves to is checked + * by ID with the helpers in `vault-access.ts`. */ import { getConfig } from "./config.js"; diff --git a/src/tools/item-archive.ts b/src/tools/item-archive.ts index b9f18a8..b92d0eb 100644 --- a/src/tools/item-archive.ts +++ b/src/tools/item-archive.ts @@ -6,6 +6,7 @@ import { z } from "zod"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerItemArchive(server: McpServer): void { server.registerTool("item_archive", { description: "Archive an item in a 1Password vault. The item is moved to the archive and hidden from regular views, rather than being permanently deleted.", inputSchema: z.object({ @@ -15,6 +16,7 @@ export function registerItemArchive(server: McpServer): void { try { log("debug", "Tool call: item_archive.", { vaultId, itemId }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.archive) { throw new Error( "Your @1password/sdk version does not support archiving items.", diff --git a/src/tools/item-delete.ts b/src/tools/item-delete.ts index 5f26a46..a14fb59 100644 --- a/src/tools/item-delete.ts +++ b/src/tools/item-delete.ts @@ -6,6 +6,7 @@ import { z } from "zod"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerItemDelete(server: McpServer): void { server.registerTool("item_delete", { description: "Permanently delete an item from a 1Password vault. This action cannot be undone.", inputSchema: z.object({ @@ -15,6 +16,7 @@ export function registerItemDelete(server: McpServer): void { try { log("debug", "Tool call: item_delete.", { vaultId, itemId }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!(client?.items as any)?.delete) { throw new Error( "Your @1password/sdk version does not support deleting items.", diff --git a/src/tools/item-edit.ts b/src/tools/item-edit.ts index d404103..a47dcfd 100644 --- a/src/tools/item-edit.ts +++ b/src/tools/item-edit.ts @@ -17,6 +17,7 @@ import { import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; /** A field upsert request from the caller. */ const fieldInput = z.object({ @@ -86,6 +87,7 @@ export function registerItemEdit(server: McpServer): void { }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.get) { throw new Error( "Your @1password/sdk version does not support getting items.", diff --git a/src/tools/item-get.ts b/src/tools/item-get.ts index 8165c2f..ac5b227 100644 --- a/src/tools/item-get.ts +++ b/src/tools/item-get.ts @@ -1,5 +1,5 @@ /** - * item_get — Retrieve a full 1Password item, with concealed values hidden by default. + * item_get — Retrieve a full 1Password item, with secret-bearing values hidden by default. */ import type { McpServer } from "@modelcontextprotocol/server"; import { z } from "zod"; @@ -7,13 +7,37 @@ import { ItemFieldType, type Item, type ItemField } from "@1password/sdk"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { parseSecretRef, assertVaultAllowed } from "../secret-ref.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; -/** Placeholder returned in place of a concealed value when reveal is false. */ +/** Placeholder returned in place of a secret-bearing value when reveal is false. */ const CONCEALED_PLACEHOLDER = "[concealed]"; /** - * Shape a single field for output. Concealed field values are replaced with a - * placeholder unless the caller explicitly asks to reveal them. + * Field types whose values are known to be non-secret and are shown without + * `reveal`. Deny-by-default: every other type is treated as secret-bearing, + * including Concealed, SshKey (the value is the private key), Totp (the value + * is the one-time-password seed), CreditCardNumber, Unsupported, and any type + * a future SDK adds. + */ +const NON_SECRET_FIELD_TYPES: ReadonlySet = new Set([ + ItemFieldType.Text, + ItemFieldType.Url, + ItemFieldType.Email, + ItemFieldType.Phone, + ItemFieldType.Date, + ItemFieldType.MonthYear, + ItemFieldType.Menu, + ItemFieldType.CreditCardType, + ItemFieldType.Address, + ItemFieldType.Reference, +]); + +/** + * Shape a single field for output. The value of any field whose type is not in + * the non-secret allow-list is replaced with a placeholder unless the caller + * explicitly asks to reveal it. `field.details` (computed OTP codes, SSH key + * attributes, ...) is never included. */ function summarizeField( field: ItemField, @@ -25,8 +49,8 @@ function summarizeField( section?: string; value: string; } { - const concealed = field.fieldType === ItemFieldType.Concealed; - const value = concealed && !reveal ? CONCEALED_PLACEHOLDER : field.value; + const secretBearing = !NON_SECRET_FIELD_TYPES.has(field.fieldType); + const value = secretBearing && !reveal ? CONCEALED_PLACEHOLDER : field.value; return { id: field.id, title: field.title, @@ -37,7 +61,7 @@ function summarizeField( } export function registerItemGet(server: McpServer): void { - server.registerTool("item_get", { description: "Retrieve a full 1Password item — title, category, tags, notes, and all fields (id, title, type, section). Concealed field values are hidden unless reveal is true. Accepts a secret reference (op://vault/item/field) or vault ID + item ID. Revealing a secret puts it in the model context/transcript — to USE a secret in a command or API call, prefer op_run with op:// references instead.", inputSchema: z.object({ + server.registerTool("item_get", { description: "Retrieve a full 1Password item — title, category, tags, notes, and all fields (id, title, type, section). Secret-bearing field values (passwords and other concealed fields, SSH private keys, one-time-password seeds, card numbers) are hidden unless reveal is true; only known non-secret types (text, URL, email, phone, date, menu, card type, address, reference) are shown by default. Accepts a secret reference (op://vault/item/field) or vault ID + item ID. Revealing a secret puts it in the model context/transcript — to USE a secret in a command or API call, prefer op_run with op:// references instead.", inputSchema: z.object({ secretReference: z .string() .optional() @@ -56,7 +80,7 @@ export function registerItemGet(server: McpServer): void { .boolean() .optional() .describe( - "If true, include concealed field values in plaintext — this puts the secret in the model context/transcript. Defaults to false; prefer op_run to use a secret without revealing it.", + "If true, include secret-bearing field values (passwords, SSH private keys, one-time-password seeds, card numbers) in plaintext — this puts the secret in the model context/transcript. Defaults to false; prefer op_run to use a secret without revealing it.", ), }) }, async ({ secretReference, vaultId, itemId, reveal }) => { try { @@ -66,6 +90,12 @@ export function registerItemGet(server: McpServer): void { itemId, reveal, }); + if (secretReference) { + // Pre-check the vault segment as written; the vault the + // reference actually resolves to is verified below. + assertVaultAllowed(parseSecretRef(secretReference).vault); + } + const client = await getClient(); if (!client?.items?.get) { throw new Error( @@ -100,6 +130,8 @@ export function registerItemGet(server: McpServer): void { ); } + await assertVaultIdAllowed(client, resolvedVaultId); + const item: Item = await client.items.get(resolvedVaultId, resolvedItemId); const shouldReveal = reveal === true; diff --git a/src/tools/item-list.ts b/src/tools/item-list.ts index 9e05219..a79d879 100644 --- a/src/tools/item-list.ts +++ b/src/tools/item-list.ts @@ -7,6 +7,7 @@ import type { ItemOverview } from "@1password/sdk"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerItemList(server: McpServer): void { server.registerTool("item_list", { description: "List all items in a 1Password vault, returning id, title, category, tags, and updatedAt for each. Never returns secret values.", inputSchema: z.object({ @@ -15,6 +16,7 @@ export function registerItemList(server: McpServer): void { try { log("debug", "Tool call: item_list.", { vaultId }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.list) { throw new Error( "Your @1password/sdk version does not support listing items.", diff --git a/src/tools/item-lookup.ts b/src/tools/item-lookup.ts index 4bf4091..885bd41 100644 --- a/src/tools/item-lookup.ts +++ b/src/tools/item-lookup.ts @@ -6,6 +6,7 @@ import { z } from "zod"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; import type { ItemSummary } from "../types.js"; export function registerItemLookup(server: McpServer): void { @@ -30,6 +31,7 @@ export function registerItemLookup(server: McpServer): void { limit, }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); const listFn = client?.items?.list ?? (client?.items as any)?.listAll; if (!listFn) { diff --git a/src/tools/note-create.ts b/src/tools/note-create.ts index 6e05bd1..2f674cf 100644 --- a/src/tools/note-create.ts +++ b/src/tools/note-create.ts @@ -12,6 +12,7 @@ import { import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; /** An optional custom field to attach to the note. */ const fieldInput = z.object({ @@ -53,6 +54,7 @@ export function registerNoteCreate(server: McpServer): void { fieldCount: fields?.length ?? 0, }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.create) { throw new Error( "Your @1password/sdk version does not support creating items.", diff --git a/src/tools/op-check-ref.ts b/src/tools/op-check-ref.ts index f1c089b..e06df94 100644 --- a/src/tools/op-check-ref.ts +++ b/src/tools/op-check-ref.ts @@ -10,6 +10,7 @@ import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; import { parseSecretRef, assertVaultAllowed } from "../secret-ref.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerOpCheckRef(server: McpServer): void { server.registerTool("op_check_ref", { description: "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.", inputSchema: z.object({ @@ -41,6 +42,9 @@ export function registerOpCheckRef(server: McpServer): void { } const { vaultId, itemId } = response.content; + // The vault segment passed the textual pre-check above; also + // verify the vault the reference actually resolved to. + await assertVaultIdAllowed(client, vaultId); const item: any = await client.items.get(vaultId, itemId); const desiredField = ref.field.toLowerCase(); diff --git a/src/tools/op-run.ts b/src/tools/op-run.ts index 7966b5e..c030694 100644 --- a/src/tools/op-run.ts +++ b/src/tools/op-run.ts @@ -1,25 +1,42 @@ /** * 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 -- `. + * environment variables. This is the MCP equivalent of `op run -- `. + * Resolved values reach only the child process environment and are redacted + * from the captured output on a best-effort basis (see ../redaction.ts). */ import type { McpServer } from "@modelcontextprotocol/server"; -import { spawn } from "node:child_process"; +import { execFile, spawn, type ChildProcess } from "node:child_process"; +import { join } from "node:path"; import { z } from "zod"; import { getClient } from "../client.js"; import { getConfig } from "../config.js"; import { log, logError } from "../logger.js"; +import { + OutputCapture, + buildRedactionPatterns, + finalizeOutput, + maxPatternByteLength, + redact, + type RedactionPattern, + type RedactionTarget, +} from "../redaction.js"; import { jsonResult, errorResult } from "../utils.js"; import { isSecretRef, parseSecretRef, assertVaultAllowed } from "../secret-ref.js"; +import { assertVaultIdsAllowed } from "../vault-access.js"; const MAX_OUTPUT_BYTES = 5 * 1024 * 1024; // 5 MiB safety cap per stream const DEFAULT_TIMEOUT_MS = 120_000; +/** How long a timed-out command may take to die after SIGTERM before it is SIGKILLed. */ +const KILL_GRACE_MS = 2_000; +/** How long to wait for the pipes to close after SIGKILL before abandoning them. */ +const ABANDON_DELAY_MS = 500; -const SENSITIVE_SERVER_ENV_VARS = [ +/** Server credentials that must never reach a child (compared case-insensitively). */ +const SENSITIVE_SERVER_ENV_VARS: ReadonlySet = new Set([ "OP_SERVICE_ACCOUNT_TOKEN", "OP_KEYCHAIN_SERVICE", "OP_KEYCHAIN_ACCOUNT", -] as const; +]); interface ResolvedEnvEntry { name: string; @@ -28,12 +45,12 @@ interface ResolvedEnvEntry { secret: boolean; } -interface RedactionTarget { - name: string; - value: string; -} - -/** Resolve every secret reference needed by one command in one bulk SDK request. */ +/** + * Resolve every secret reference needed by one command in one bulk SDK request. + * The vault segment of each reference is pre-checked against the allow-list as + * written; once resolved, the vaults the references actually point at are + * checked by ID too, before any value is returned. + */ async function resolveEnvEntries( env: Record | undefined, ): Promise { @@ -58,6 +75,7 @@ async function resolveEnvEntries( } const resolved = await client.secrets.resolveAll(references); + const vaultIds: string[] = []; for (const [name, rawValue] of envEntries) { if (isSecretRef(rawValue)) { const response = resolved.individualResponses[rawValue]; @@ -65,11 +83,17 @@ async function resolveEnvEntries( const reason = response?.error?.type ?? "unknown"; throw new Error(`Could not resolve secret reference '${rawValue}' (${reason}).`); } + vaultIds.push(response.content.vaultId); entries.push({ name, value: response.content.secret, secret: true }); } else { entries.push({ name, value: rawValue, secret: false }); } } + + // A reference can name an allowed vault yet resolve to another one. Check the + // resolved vaults (one listing for all of them) before returning any value, + // so nothing is injected and no child is spawned for a disallowed vault. + await assertVaultIdsAllowed(client, vaultIds); return entries; } @@ -108,40 +132,172 @@ function getRedactionTargets( return targets; } -/** Replace every occurrence of every secret value with a redaction marker. */ -function redact(text: string, targets: RedactionTarget[]): string { - let redacted = text; - 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(target.value).join(`«REDACTED:${target.name}»`); +/** Everything that must be masked in this command's output, derived encodings included. */ +function buildPatterns(resolvedEnv: ResolvedEnvEntry[]): RedactionPattern[] { + return buildRedactionPatterns(getRedactionTargets(resolvedEnv)); +} + +/** Copy the server environment minus its own credentials, then apply the caller's entries. */ +function buildChildEnv(resolvedEnv: ResolvedEnvEntry[]): NodeJS.ProcessEnv { + const childEnv: NodeJS.ProcessEnv = { ...process.env }; + // The copy is a plain object, so match names case-insensitively ourselves: + // Windows environment names are, and `Op_Service_Account_Token` would + // otherwise survive a `delete childEnv["OP_SERVICE_ACCOUNT_TOKEN"]`. + for (const key of Object.keys(childEnv)) { + if (SENSITIVE_SERVER_ENV_VARS.has(key.toUpperCase())) { + delete childEnv[key]; + } } - return redacted; + for (const entry of resolvedEnv) { + childEnv[entry.name] = entry.value; + } + return childEnv; } -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 }; +/** + * Kill a spawned command and everything it started. POSIX children are spawned + * detached as process-group leaders, so the whole group is signalled; Windows + * has no groups, so `taskkill /T` walks the process tree instead (it cannot + * find descendants of a parent that has already exited). + */ +function killProcessTree(child: ChildProcess, signal: NodeJS.Signals): void { + const pid = child.pid; + if (pid === undefined || pid <= 1) return; + + if (process.platform === "win32") { + const systemRoot = process.env.SystemRoot; + const taskkill = systemRoot ? join(systemRoot, "System32", "taskkill.exe") : "taskkill"; + const killDirectChild = (): void => { + try { + child.kill(); + } catch { + // Already gone. + } + }; + try { + // Errors are ignored beyond a fallback: taskkill also fails when the child already exited. + execFile(taskkill, ["/pid", String(pid), "/T", "/F"], { windowsHide: true }, (error) => { + if (error) killDirectChild(); + }); + } catch { + killDirectChild(); + } + return; } - return { - text: redactedBuffer.subarray(0, MAX_OUTPUT_BYTES).toString("utf8"), - truncated: true, - }; + try { + process.kill(-pid, signal); + } catch { + try { + child.kill(signal); + } catch { + // Already gone. + } + } +} + +interface RunOptions { + command?: string; + argv?: string[]; + cwd?: string; + env: NodeJS.ProcessEnv; + shell?: string; + stdin?: string; + timeoutMs: number; + /** Maximum bytes retained per output stream. */ + retainBytes: number; +} + +interface RunResult { + exitCode: number | null; + signal: NodeJS.Signals | null; + stdout: OutputCapture; + stderr: OutputCapture; + timedOut: boolean; + spawnError?: Error; +} + +/** + * Run the command with bounded output capture and a timeout that covers the + * whole process tree. Always resolves, within roughly the timeout plus the + * kill grace, even if a descendant keeps the stdio pipes open. + */ +function runCommand(options: RunOptions): Promise { + const { command, argv, cwd, env, shell, stdin, timeoutMs, retainBytes } = options; + + return new Promise((resolve) => { + const stdout = new OutputCapture(retainBytes); + const stderr = new OutputCapture(retainBytes); + let timedOut = false; + let settled = false; + let exitCode: number | null = null; + let exitSignal: NodeJS.Signals | null = null; + let timeoutTimer: NodeJS.Timeout | undefined; + let killTimer: NodeJS.Timeout | undefined; + let abandonTimer: NodeJS.Timeout | undefined; + + const spawnOptions = { + cwd, + env, + windowsHide: true, + // On POSIX the child leads its own process group so a timeout can signal its descendants too. + detached: process.platform !== "win32", + }; + const child = argv + ? spawn(argv[0], argv.slice(1), { ...spawnOptions, shell: false }) + : spawn(command as string, { ...spawnOptions, shell: shell ?? true }); + + const settle = (spawnError?: Error): void => { + if (settled) return; + settled = true; + clearTimeout(timeoutTimer); + clearTimeout(killTimer); + clearTimeout(abandonTimer); + resolve({ exitCode, signal: exitSignal, stdout, stderr, timedOut, spawnError }); + }; + + child.on("error", (spawnError) => settle(spawnError)); + child.on("exit", (code, signal) => { + exitCode = code; + exitSignal = signal; + }); + child.on("close", (code, signal) => { + exitCode = code; + exitSignal = signal; + settle(); + }); + + // Keep consuming (and discarding) output past the cap so the child never blocks on a full pipe. + child.stdout?.on("data", (chunk: Buffer) => stdout.push(chunk)); + child.stderr?.on("data", (chunk: Buffer) => stderr.push(chunk)); + // A child that exits without reading its stdin would otherwise raise an unhandled EPIPE. + for (const stream of [child.stdin, child.stdout, child.stderr]) { + stream?.on("error", () => {}); + } + + if (child.stdin) { + if (stdin !== undefined) child.stdin.write(stdin); + child.stdin.end(); + } + + timeoutTimer = setTimeout(() => { + timedOut = true; + killProcessTree(child, "SIGTERM"); + killTimer = setTimeout(() => { + killProcessTree(child, "SIGKILL"); + // If a descendant outside the tree still holds the pipes, stop waiting for them. + abandonTimer = setTimeout(() => { + child.stdout?.destroy(); + child.stderr?.destroy(); + settle(); + }, ABANDON_DELAY_MS); + }, KILL_GRACE_MS); + }, timeoutMs); + }); } export function registerOpRun(server: McpServer): void { - server.registerTool("op_run", { description: "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.", inputSchema: z.object({ + server.registerTool("op_run", { description: "Run a local shell command with 1Password secrets injected as environment variables. Resolved values go only into the child process environment, are never logged by the server, and are redacted from stdout/stderr on a best-effort basis (including common encodings such as JSON, URL and base64) — a command that deliberately transforms or transmits a secret can still expose it. This is the preferred 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.", inputSchema: z.object({ command: z .string() .optional() @@ -176,7 +332,7 @@ export function registerOpRun(server: McpServer): void { .positive() .max(600_000) .optional() - .describe("Kill the process if it runs longer than this many milliseconds. Default 120000 (2 minutes)."), + .describe("Kill the process and everything it started if it runs longer than this many milliseconds. Default 120000 (2 minutes)."), stdin: z .string() .optional() @@ -205,96 +361,42 @@ 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; - } + // Built before spawning: the capture needs to know how much extra output + // keeps a secret that straddles the size cap intact. + const patterns = buildPatterns(resolvedEnv); - 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 result = await runCommand({ + command, + argv, + cwd, + env: buildChildEnv(resolvedEnv), + shell, + stdin, + timeoutMs: timeout_ms ?? DEFAULT_TIMEOUT_MS, + retainBytes: MAX_OUTPUT_BYTES + maxPatternByteLength(patterns), }); const durationMs = Date.now() - startedAt; - 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, redactionTargets); + const message = redact(result.spawnError.message, patterns); logError("op_run spawn failed.", new Error(message)); return errorResult(new Error(message)); } + const { text: stdout, truncated: stdoutTruncated } = finalizeOutput( + result.stdout.toBuffer(), + result.stdout.overflowed, + patterns, + MAX_OUTPUT_BYTES, + ); + const { text: stderr, truncated: stderrTruncated } = finalizeOutput( + result.stderr.toBuffer(), + result.stderr.overflowed, + patterns, + MAX_OUTPUT_BYTES, + ); + log("debug", "op_run completed.", { exitCode: result.exitCode, signal: result.signal, @@ -315,9 +417,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 targets = getRedactionTargets(resolvedEnv); + const patterns = buildPatterns(resolvedEnv); const rawMessage = error instanceof Error ? error.message : String(error); - const safeError = new Error(redact(rawMessage, targets)); + const safeError = new Error(redact(rawMessage, patterns)); logError("op_run failed.", safeError); return errorResult(safeError); } diff --git a/src/tools/password-create.ts b/src/tools/password-create.ts index ff634ba..0ecc9b9 100644 --- a/src/tools/password-create.ts +++ b/src/tools/password-create.ts @@ -7,6 +7,7 @@ import * as sdk from "@1password/sdk"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerPasswordCreate(server: McpServer): void { server.registerTool("password_create", { description: "Create a new password/login item in a 1Password vault with optional username, URL, tags, and notes.", inputSchema: z.object({ @@ -51,6 +52,7 @@ export function registerPasswordCreate(server: McpServer): void { try { log("debug", "Tool call: password_create.", { vaultId, title }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.create) { throw new Error( "Your @1password/sdk version does not support creating items.", diff --git a/src/tools/password-read.ts b/src/tools/password-read.ts index a32d778..52580bc 100644 --- a/src/tools/password-read.ts +++ b/src/tools/password-read.ts @@ -6,6 +6,8 @@ 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"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerPasswordRead(server: McpServer): void { server.registerTool("password_read", { description: "Retrieve a secret from 1Password using either a secret reference (op://vault/item/field) or vault ID + item ID. Supports field selection and optional value reveal (defaults to metadata-only). Revealing a secret puts it in the model context/transcript — to USE a secret in a command or API call, prefer op_run with op:// references instead.", inputSchema: z.object({ @@ -42,20 +44,34 @@ export function registerPasswordRead(server: McpServer): void { field, reveal, }); + if (secretReference) { + // Pre-check the vault segment as written; the vault the + // reference actually resolves to is verified below. + assertVaultAllowed(parseSecretRef(secretReference).vault); + } + const client = await getClient(); const shouldReveal = reveal === true; if (secretReference) { - if (!client?.secrets?.resolve) { + if (!client?.secrets?.resolveAll) { throw new Error( "Your @1password/sdk version does not support resolving secrets.", ); } - const value = await client.secrets.resolve(secretReference); + 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}).`, + ); + } + await assertVaultIdAllowed(client, response.content.vaultId); if (!shouldReveal) { return jsonResult({ resolved: true }); } - return jsonResult({ value }); + return jsonResult({ value: response.content.secret }); } if (!vaultId || !itemId) { @@ -64,6 +80,8 @@ export function registerPasswordRead(server: McpServer): void { ); } + await assertVaultIdAllowed(client, vaultId); + if (!client?.items?.get) { throw new Error( "Your @1password/sdk version does not support getting items.", diff --git a/src/tools/password-update.ts b/src/tools/password-update.ts index 8143b38..f4c2c1e 100644 --- a/src/tools/password-update.ts +++ b/src/tools/password-update.ts @@ -7,6 +7,7 @@ import * as sdk from "@1password/sdk"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { assertVaultIdAllowed } from "../vault-access.js"; export function registerPasswordUpdate(server: McpServer): void { server.registerTool("password_update", { description: "Update (rotate) a password or concealed field on an existing 1Password item. If the target field does not exist, it will be created.", inputSchema: z.object({ @@ -33,6 +34,7 @@ export function registerPasswordUpdate(server: McpServer): void { field, }); const client = await getClient(); + await assertVaultIdAllowed(client, vaultId); if (!client?.items?.get) { throw new Error( "Your @1password/sdk version does not support getting items.", diff --git a/src/tools/vault-list.ts b/src/tools/vault-list.ts index fd5f93b..421622a 100644 --- a/src/tools/vault-list.ts +++ b/src/tools/vault-list.ts @@ -5,11 +5,12 @@ import type { McpServer } from "@modelcontextprotocol/server"; import { getClient } from "../client.js"; import { log, logError } from "../logger.js"; import { jsonResult, errorResult } from "../utils.js"; +import { filterAllowedVaults } from "../vault-access.js"; import type { VaultSummary } from "../types.js"; import { z } from "zod"; export function registerVaultList(server: McpServer): void { - server.registerTool("vault_list", { description: "List all 1Password vaults accessible to the service account. Returns vault IDs, names, descriptions, and types.", inputSchema: z.object({}) }, async () => { + server.registerTool("vault_list", { description: "List the 1Password vaults accessible to the service account (limited to the allow-listed vaults when OP_MCP_ALLOWED_VAULTS / --allowed-vaults is configured). Returns vault IDs, names, descriptions, and types.", inputSchema: z.object({}) }, async () => { try { log("debug", "Tool call: vault_list."); const client = await getClient(); @@ -19,8 +20,9 @@ export function registerVaultList(server: McpServer): void { "Your @1password/sdk version does not support listing vaults.", ); } - const vaults = await listFn.call(client.vaults); - const summary: VaultSummary[] = (vaults ?? []).map( + const vaults: any[] = (await listFn.call(client.vaults)) ?? []; + const visibleVaults = filterAllowedVaults(vaults); + const summary: VaultSummary[] = visibleVaults.map( (vault: any) => ({ id: vault.id, name: vault.name ?? vault.title, diff --git a/src/vault-access.ts b/src/vault-access.ts new file mode 100644 index 0000000..65e3cab --- /dev/null +++ b/src/vault-access.ts @@ -0,0 +1,135 @@ +/** + * Server-wide vault allow-list enforcement. + * + * `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` (vault names or IDs, + * case-insensitive) restricts which vaults this server may read from or write + * to. `assertVaultAllowed` in `secret-ref.ts` only checks the vault segment as + * it is written in an `op://` reference, but most tools address vaults by ID + * and a reference can name a vault differently from the allow-list entry. These + * helpers therefore resolve the allow-list to the concrete IDs of the vaults + * visible to the service account and check the vault ID a tool is about to use. + * + * A vault is allowed exactly when its own ID, title, or name matches an + * allow-list entry (`vaultMatchesAllowList`). The ID checks and the listing + * filter both use that one definition. + * + * An empty allow-list (the default) means no restriction: every helper returns + * immediately and makes NO SDK calls. With a non-empty allow-list the ID checks + * fail closed: if vaults cannot be listed, nothing is allowed. + */ + +import { getConfig } from "./config.js"; + +/** A vault listing entry: the fields used for allow-list matching (SDK `VaultOverview`; older SDKs used `name`). */ +export interface VaultLike { + id: string; + title?: string; + name?: string; +} + +/** + * Minimal structural view of the SDK client: just enough to list vaults. The + * client returned by `getClient()` satisfies it. + */ +export interface VaultAccessClient { + vaults?: { + list?: () => Promise; + /** Legacy SDK name for `list`. */ + listAll?: () => Promise; + }; +} + +/** Build the error thrown for a vault that is outside the allow-list. */ +function vaultNotAllowedError(vaultId: string, allowedVaults: readonly string[]): Error { + return new Error( + `Vault '${vaultId}' is not in the allowed vault list (${allowedVaults.join(", ")}). ` + + "Configure OP_MCP_ALLOWED_VAULTS or --allowed-vaults to permit it.", + ); +} + +/** + * True if the vault's own ID, title, or name case-insensitively equals an + * allow-list entry. The single definition of "allowed vault", shared by the ID + * checks (`resolveAllowedVaultIds`) and the listing filter (`filterAllowedVaults`). + */ +function vaultMatchesAllowList( + vault: VaultLike, + allowedVaults: readonly string[], +): boolean { + const labels = [vault?.id, vault?.title, vault?.name] + .filter((label): label is string => typeof label === "string") + .map((label) => label.toLowerCase()); + return allowedVaults.some((entry) => labels.includes(entry.toLowerCase())); +} + +/** + * Resolve the allow-list to the lower-cased IDs of the vaults it permits, by + * listing the vaults visible to the service account and matching each one. + */ +async function resolveAllowedVaultIds( + client: VaultAccessClient, + allowedVaults: readonly string[], +): Promise> { + const listFn = client?.vaults?.list ?? client?.vaults?.listAll; + if (!listFn) { + throw new Error( + "Your @1password/sdk version does not support listing vaults, which is required to enforce the vault allow-list.", + ); + } + + const vaults = (await listFn.call(client.vaults)) ?? []; + const allowedIds = new Set(); + for (const vault of vaults) { + if ( + typeof vault?.id === "string" && + vaultMatchesAllowList(vault, allowedVaults) + ) { + allowedIds.add(vault.id.toLowerCase()); + } + } + return allowedIds; +} + +/** + * Throw if `vaultId` is not the ID of an allow-listed vault. IDs are compared + * case-insensitively. A no-op (no SDK calls) when no allow-list is configured. + */ +export async function assertVaultIdAllowed( + client: VaultAccessClient, + vaultId: string, +): Promise { + await assertVaultIdsAllowed(client, [vaultId]); +} + +/** + * Throw if any of `vaultIds` is not the ID of an allow-listed vault, listing + * vaults only once however many IDs are given. A no-op (no SDK calls) when no + * allow-list is configured or `vaultIds` is empty. + */ +export async function assertVaultIdsAllowed( + client: VaultAccessClient, + vaultIds: readonly string[], +): Promise { + const { allowedVaults } = getConfig(); + if (allowedVaults.length === 0 || vaultIds.length === 0) return; + + const allowedIds = await resolveAllowedVaultIds(client, allowedVaults); + for (const vaultId of vaultIds) { + if (typeof vaultId !== "string" || !allowedIds.has(vaultId.toLowerCase())) { + throw vaultNotAllowedError(String(vaultId), allowedVaults); + } + } +} + +/** + * Return only the vaults that the allow-list permits, preserving order. Makes + * no SDK call: each vault is matched on its own `id`/`title`/`name`, so pass the + * entries returned by `vaults.list()`. Returns `vaults` untouched when no + * allow-list is configured. + */ +export function filterAllowedVaults(vaults: T[]): T[] { + const { allowedVaults } = getConfig(); + if (allowedVaults.length === 0) return vaults; + + return vaults.filter((vault) => vaultMatchesAllowList(vault, allowedVaults)); +} diff --git a/tests/config.test.ts b/tests/config.test.ts index 002fd8f..5c52fe1 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -6,6 +6,7 @@ import { readFileSync } from "node:fs"; import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { getConfig, + getTokenSourceWarning, readMacOsKeychainToken, resetConfig, resolveServiceAccountToken, @@ -123,7 +124,8 @@ describe("config", () => { }); expect(token).toBe("keychain-token"); - expect(execFileSyncImpl).toHaveBeenCalledWith("security", [ + // Absolute path: the binary must never be resolved through PATH. + expect(execFileSyncImpl).toHaveBeenCalledWith("/usr/bin/security", [ "find-generic-password", "-a", "alice", @@ -184,6 +186,32 @@ describe("config", () => { expect(readKeychainToken).not.toHaveBeenCalled(); }); + it("warns when the token was passed on the command line", () => { + const warning = getTokenSourceWarning("args"); + + expect(warning).toBeDefined(); + expect(warning).toContain("--service-account-token"); + expect(warning).toContain("OP_SERVICE_ACCOUNT_TOKEN"); + expect(warning).toContain("OP_KEYCHAIN_SERVICE"); + }); + + it.each(["env", "keychain", "missing"] as const)( + "does not warn when the token source is %s", + (tokenSource) => { + expect(getTokenSourceWarning(tokenSource)).toBeUndefined(); + }, + ); + + it("never includes the token value in the token source warning", () => { + const token = "ops_super-secret-token"; + process.argv = ["node", "index.js", "--service-account-token", token]; + + const warning = getTokenSourceWarning(getConfig().tokenSource); + + expect(warning).toBeDefined(); + expect(warning).not.toContain(token); + }); + it("uses default integration name/version", () => { process.argv = ["node", "index.js"]; const config = getConfig(); diff --git a/tests/op-check-ref.test.ts b/tests/op-check-ref.test.ts index c3891ea..eaa14e8 100644 --- a/tests/op-check-ref.test.ts +++ b/tests/op-check-ref.test.ts @@ -155,4 +155,86 @@ describe("op_check_ref", () => { expect(result.content[0].text).toContain("not in the allowed vault list"); expect(resolveAll).not.toHaveBeenCalled(); }); + + describe("resolved-vault check", () => { + const vaults = [ + { id: "vlt-private-0001", title: "Private" }, + { id: "vlt-prod-0002", title: "Prod" }, + ]; + + function setAllowList(value: string) { + process.env.OP_MCP_ALLOWED_VAULTS = value; + resetConfig(); + } + + /** A client whose `reference` resolves to a secret living in `resolvedVaultId`. */ + function mockClient(reference: string, resolvedVaultId: string) { + const client = { + vaults: { list: vi.fn().mockResolvedValue(vaults) }, + secrets: { + resolveAll: vi.fn().mockResolvedValue({ + individualResponses: { + [reference]: { + content: { secret: "s3cr3t", itemId: "i1", vaultId: resolvedVaultId }, + }, + }, + }), + }, + items: { + get: vi.fn().mockResolvedValue({ + id: "i1", + title: "GitHub", + category: "Login", + fields: [{ id: "token", title: "token", fieldType: "Concealed", value: "s3cr3t-value" }], + }), + }, + }; + mockedGetClient.mockResolvedValue(client as any); + return client; + } + + it("rejects a reference that names an allowed vault but resolves to a vault outside the allowed set", async () => { + setAllowList("Private"); + const reference = "op://Private/github/token"; + const client = mockClient(reference, "vlt-prod-0002"); + + const result = await handler()({ secretReference: reference }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not in the allowed vault list"); + expect(result.content[0].text).toContain("vlt-prod-0002"); + expect(client.secrets.resolveAll).toHaveBeenCalledTimes(1); + expect(client.items.get).not.toHaveBeenCalled(); + }); + + it.each([ + ["by name", "Private", "op://Private/github/token"], + ["by ID", "vlt-private-0001", "op://vlt-private-0001/github/token"], + ["by ID, in a different case", "VLT-PRIVATE-0001", "op://vlt-private-0001/github/token"], + ])( + "allows a reference that resolves to an allow-listed vault (%s)", + async (_label, allowList, reference) => { + setAllowList(allowList); + const client = mockClient(reference, "vlt-private-0001"); + + const result = await handler()({ secretReference: reference }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.resolved).toBe(true); + expect(client.items.get).toHaveBeenCalledWith("vlt-private-0001", "i1"); + expect(result.content[0].text).not.toContain("s3cr3t"); + }, + ); + + it("does not list vaults when no allow-list is configured", async () => { + const reference = "op://Private/github/token"; + const client = mockClient(reference, "vlt-prod-0002"); + + const result = await handler()({ secretReference: reference }); + + expect(result.isError).toBeUndefined(); + expect(client.vaults.list).not.toHaveBeenCalled(); + }); + }); }); diff --git a/tests/op-run.test.ts b/tests/op-run.test.ts index 66713eb..d8cb501 100644 --- a/tests/op-run.test.ts +++ b/tests/op-run.test.ts @@ -1,6 +1,6 @@ /** * Tests for the op_run tool — executes local commands with 1Password - * secrets injected as env vars, without ever returning secret plaintext. + * secrets injected as env vars, redacting them from the returned output. * * 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 @@ -8,6 +8,9 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { McpServer } from "@modelcontextprotocol/server"; vi.mock("../src/client.js", () => ({ @@ -22,10 +25,52 @@ vi.mock("../src/logger.js", () => ({ })); import { getClient } from "../src/client.js"; import { resetConfig } from "../src/config.js"; +import { buildRedactionPatterns, maxPatternByteLength } from "../src/redaction.js"; import { registerAllTools } from "../src/tools/index.js"; +// Every test spawns real processes, which can take seconds on a loaded Windows machine. +vi.setConfig({ testTimeout: 30_000 }); + const mockedGetClient = vi.mocked(getClient); const node = process.execPath; +const isWindows = process.platform === "win32"; +const MIB = 1024 * 1024; + +/** True while the process exists (a killed-but-unreaped zombie counts as gone). */ +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + if (process.platform === "linux") { + try { + return !/^\d+ \(.*\) Z/.test(readFileSync(`/proc/${pid}/stat`, "utf8")); + } catch { + return false; + } + } + return true; +} + +async function waitUntilDead(pid: number, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (isAlive(pid)) { + if (Date.now() > deadline) return false; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return true; +} + +/** Clean up a process a test deliberately left running. */ +function killQuietly(pid: number): void { + if (!Number.isInteger(pid) || pid <= 1) return; + try { + process.kill(pid, "SIGKILL"); + } catch { + // Already gone. + } +} describe("op_run", () => { let server: McpServer; @@ -64,14 +109,36 @@ describe("op_run", () => { return registeredTools.get("op_run")!.handler; } - function mockBulkResolve(values: Record) { + /** + * Mock the SDK's bulk `secrets.resolveAll`. By default responses carry only the + * secret and the client has no `vaults`, like the original callers expect. + * `vaultIds` sets the vault ID each reference resolves to; `vaults` adds a + * `vaults.list` mock, exposed as `resolveAll.vaultsList`. + */ + function mockBulkResolve( + values: Record, + options: { + vaultIds?: Record; + vaults?: { id: string; title: string }[]; + } = {}, + ) { const resolveAll = vi.fn().mockImplementation(async (references: string[]) => ({ individualResponses: Object.fromEntries( - references.map((reference) => [reference, { content: { secret: values[reference] ?? "" } }]), + references.map((reference) => { + const vaultId = options.vaultIds?.[reference]; + return [ + reference, + { content: { secret: values[reference] ?? "", ...(vaultId !== undefined ? { vaultId } : {}) } }, + ]; + }), ), })); - mockedGetClient.mockResolvedValue({ secrets: { resolveAll } } as any); - return resolveAll; + const vaultsList = vi.fn().mockResolvedValue(options.vaults ?? []); + mockedGetClient.mockResolvedValue({ + secrets: { resolveAll }, + ...(options.vaults ? { vaults: { list: vaultsList } } : {}), + } as any); + return Object.assign(resolveAll, { vaultsList }); } it("resolves an op:// reference into env and the child process sees the value", async () => { @@ -281,4 +348,632 @@ describe("op_run", () => { expect(data.stdout).not.toContain("supersecre"); expect(data.stdout).toContain("«REDACTED"); }); + + it("describes redaction as best-effort rather than a guarantee", () => { + const description: string = registeredTools.get("op_run")!.description; + + expect(description).toMatch(/best-effort/i); + expect(description).not.toMatch(/never returned/i); + expect(description).toContain("op://"); + }); + + it("runs a shell command line when `command` is given", async () => { + const result = await handler()({ command: "echo shell-ok" }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.timedOut).toBe(false); + expect(data.stdout.trim()).toBe("shell-ok"); + }); + + it("returns an error result when the executable cannot be spawned, redacting secrets in it", async () => { + mockBulkResolve({ "op://Private/api/key": "hunter2-hunter2" }); + + const result = await handler()({ + argv: ["no-such-binary-hunter2-hunter2"], + env: { MY_SECRET: "op://Private/api/key" }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("ENOENT"); + expect(result.content[0].text).not.toContain("hunter2-hunter2"); + }); + + describe("redaction of overlapping and encoded secrets", () => { + it("masks a secret that contains another secret without leaking the remainder", async () => { + mockBulkResolve({ + "op://Private/api/user": "svc-bot", + "op://Private/api/credentials": "svc-bot:Zx9-real-secret", + }); + const script = + "process.stdout.write('auth=' + process.env.API_CREDENTIALS + ' user=' + process.env.API_USER)"; + + for (const env of [ + { API_USER: "op://Private/api/user", API_CREDENTIALS: "op://Private/api/credentials" }, + { API_CREDENTIALS: "op://Private/api/credentials", API_USER: "op://Private/api/user" }, + ]) { + const result = await handler()({ argv: [node, "-e", script], env }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.stdout).not.toContain("Zx9-real-secret"); + expect(data.stdout).not.toContain("svc-bot"); + expect(data.stdout).toMatch(/^auth=«REDACTED:API_[A-Z_,]+» user=«REDACTED:API_USER»$/); + } + }); + + it("masks a secret embedded in a base64 payload at every alignment", async () => { + const secret = "Zx9-real-secret-value"; + mockBulkResolve({ "op://Private/api/key": secret }); + + const script = [ + "const secret = process.env.SECRET;", + "const lines = [];", + "for (let p = 0; p <= 5; p++) lines.push(Buffer.from('x'.repeat(p) + secret + 'tail').toString('base64'));", + "lines.push(Buffer.from('user:' + secret).toString('base64'));", + "process.stdout.write(lines.join('\\n'));", + ].join("\n"); + const payloads = [0, 1, 2, 3, 4, 5] + .map((p) => Buffer.from("x".repeat(p) + secret + "tail").toString("base64")) + .concat(Buffer.from("user:" + secret).toString("base64")); + + const result = await handler()({ + argv: [node, "-e", script], + env: { SECRET: "op://Private/api/key" }, + }); + const data = JSON.parse(result.content[0].text); + const lines: string[] = data.stdout.split("\n"); + + expect(data.exitCode).toBe(0); + expect(lines).toHaveLength(payloads.length); + payloads.forEach((payload, index) => { + expect(data.stdout).not.toContain(payload); + // Only a few edge characters of the payload may survive around the marker. + expect(lines[index]).not.toContain(payload.slice(6, -6)); + expect(lines[index]).toContain("«REDACTED:SECRET»"); + }); + }); + + it("masks JSON-escaped and URL-encoded forms of a secret", async () => { + const secret = 'pa"ss\\wo/rd+x=y z'; + mockBulkResolve({ "op://Private/api/pass": secret }); + + const script = + "const s = process.env.SECRET; process.stdout.write([JSON.stringify({ password: s }), encodeURIComponent(s), 'plain ' + s].join('\\n'))"; + const result = await handler()({ + argv: [node, "-e", script], + env: { SECRET: "op://Private/api/pass" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.stdout).not.toContain(secret); + expect(data.stdout).not.toContain(JSON.stringify(secret).slice(1, -1)); + expect(data.stdout).not.toContain(encodeURIComponent(secret)); + expect(data.stdout.split("\n")).toEqual([ + '{"password":"«REDACTED:SECRET»"}', + "«REDACTED:SECRET»", + "plain «REDACTED:SECRET»", + ]); + }); + + it("masks a multi-line secret printed with CRLF line endings or re-indented", async () => { + const lines = [ + "-----BEGIN TEST KEY-----", + "MIIEvQIBADANBgkqhkiG9w0BAQEFAASC", + "KcwggSjAgEAAoIBAQC7VJTUt9Us8cKj", + "-----END TEST KEY-----", + ]; + mockBulkResolve({ "op://Private/ssh/key": lines.join("\n") }); + + const script = + "const s = process.env.KEY; process.stdout.write(s.replace(/\\n/g, '\\r\\n') + '\\n--\\n' + s.split('\\n').map((l) => ' ' + l).join('\\n'))"; + const result = await handler()({ + argv: [node, "-e", script], + env: { KEY: "op://Private/ssh/key" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + for (const line of lines) { + expect(data.stdout).not.toContain(line); + } + expect(data.stdout).toBe( + ["«REDACTED:KEY»", "--", ...lines.map(() => " «REDACTED:KEY»")].join("\n"), + ); + }); + + it("masks encodings of the server's own service account token", async () => { + const token = "ops_server_token_0123456789abcdef"; + process.env.OP_SERVICE_ACCOUNT_TOKEN = token; + resetConfig(); + + const result = await handler()({ + argv: [ + node, + "-e", + `process.stdout.write(Buffer.from('Bearer ${token}').toString('base64') + ' ' + encodeURIComponent('${token}'))`, + ], + }); + const data = JSON.parse(result.content[0].text); + + expect(data.stdout).not.toContain(Buffer.from(`Bearer ${token}`).toString("base64").slice(8, -8)); + expect(data.stdout).toContain("«REDACTED:OP_SERVICE_ACCOUNT_TOKEN»"); + }); + }); + + describe("output limits", () => { + it("caps retained output while draining a child that writes far more than the limit", async () => { + const script = [ + "const chunk = Buffer.alloc(64 * 1024, 'x');", + "let remaining = 320;", // 20 MiB per stream + "(function write() {", + " while (remaining > 0) {", + " remaining--;", + " process.stderr.write(chunk);", + " if (!process.stdout.write(chunk)) { process.stdout.once('drain', write); return; }", + " }", + "})();", + ].join("\n"); + + const result = await handler()({ argv: [node, "-e", script] }); + const data = JSON.parse(result.content[0].text); + + // The child can only finish if the server keeps draining its pipes. + expect(data.exitCode).toBe(0); + expect(data.timedOut).toBe(false); + expect(data.stdoutTruncated).toBe(true); + expect(data.stderrTruncated).toBe(true); + expect(Buffer.byteLength(data.stdout, "utf8")).toBeLessThanOrEqual(5 * MIB); + expect(Buffer.byteLength(data.stderr, "utf8")).toBeLessThanOrEqual(5 * MIB); + expect(data.stdout.length).toBeGreaterThan(5 * MIB - 1024); + }); + + it("never emits a secret prefix that the retained window cut off", async () => { + delete process.env.OP_SERVICE_ACCOUNT_TOKEN; // keeps the pattern set, and so the window size, fixed + resetConfig(); + const bulk = "L".repeat(300); + const tail = "tail-secret-0123456789"; + mockBulkResolve({ "op://Private/bulk/key": bulk, "op://Private/tail/key": tail }); + const patterns = buildRedactionPatterns([ + { name: "BULK", value: bulk }, + { name: "TAIL", value: tail }, + ]); + const windowBytes = 5 * MIB + maxPatternByteLength(patterns); + const cutOff = 15; // characters of `tail` that still fit in the retained window + + // Masking the repeated bulk secret shrinks the output, pulling the cut-off + // prefix of `tail` back under the 5 MiB cap unless it is removed explicitly. + const script = [ + "const head = (process.env.BULK + '\\n').repeat(50);", + `const filler = 'A'.repeat(${windowBytes} - ${cutOff} - head.length);`, + "process.stdout.write(head + filler + process.env.TAIL + '-and-more');", + ].join("\n"); + const result = await handler()({ + argv: [node, "-e", script], + env: { BULK: "op://Private/bulk/key", TAIL: "op://Private/tail/key" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.stdoutTruncated).toBe(true); + expect(data.stdout).toContain("«REDACTED:BULK»"); + expect(data.stdout).not.toContain("tail-"); + expect(data.stdout.endsWith("A")).toBe(true); + expect(Buffer.byteLength(data.stdout, "utf8")).toBeLessThan(5 * MIB); + }); + + it("survives a child that exits without reading its stdin", async () => { + const result = await handler()({ + argv: [node, "-e", "process.exit(0)"], + stdin: "x".repeat(8 * MIB), + }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.exitCode).toBe(0); + }); + }); + + describe("timeouts", () => { + // Time the child gets to start and print before its timeout fires; creating + // a process can take seconds on a loaded Windows machine. + const START_BUDGET_MS = 3000; + // Longest a timed-out call may take beyond its timeout: SIGKILL grace, + // time to abandon the pipes, and scheduling slack. + const OVERRUN_MS = 2000 + 500 + 2500; + + async function runTimed(script: string) { + const startedAt = Date.now(); + const result = await handler()({ argv: [node, "-e", script], timeout_ms: START_BUDGET_MS }); + const elapsed = Date.now() - startedAt; + const data = JSON.parse(result.content[0].text); + return { data, elapsed, pid: Number(String(data.stdout).trim().split("\n")[0]) }; + } + + function expectPid(pid: number): void { + expect( + Number.isInteger(pid) && pid > 1, + "no pid printed: the child was killed before it started, raise START_BUDGET_MS", + ).toBe(true); + } + + it("kills the whole process tree on timeout and returns promptly", async () => { + const { data, elapsed, pid } = await runTimed( + [ + "const { spawn } = require('node:child_process');", + "const grandchild = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'inherit' });", + "process.stdout.write(String(grandchild.pid) + '\\n');", + "setTimeout(() => {}, 60000);", + ].join("\n"), + ); + + try { + expect(data.timedOut).toBe(true); + expect(elapsed).toBeLessThan(START_BUDGET_MS + OVERRUN_MS); + expectPid(pid); + expect(await waitUntilDead(pid), "grandchild survived the timeout").toBe(true); + } finally { + killQuietly(pid); + } + }); + + it("returns around the timeout when the child exits but a descendant keeps the pipes open", async () => { + const { data, elapsed, pid } = await runTimed( + [ + "const { spawn } = require('node:child_process');", + "const grandchild = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'inherit' });", + "process.stdout.write(String(grandchild.pid) + '\\n', () => process.exit(0));", + ].join("\n"), + ); + + try { + expect(elapsed).toBeLessThan(START_BUDGET_MS + OVERRUN_MS); + expectPid(pid); + expect(await waitUntilDead(pid), "grandchild survived the timeout").toBe(true); + // Windows ties a child's descendants to its lifetime, so there is nothing left to time out there. + if (!isWindows) expect(data.timedOut).toBe(true); + } finally { + killQuietly(pid); + } + }); + + it("stops waiting for pipes held by a descendant outside the process tree", async () => { + // `detached` moves the grandchild into its own session / out of the job, + // so neither the group kill nor taskkill can reach it. + const { data, elapsed, pid } = await runTimed( + [ + "const { spawn } = require('node:child_process');", + "const grandchild = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'inherit', detached: true });", + "process.stdout.write(String(grandchild.pid) + '\\n', () => process.exit(0));", + ].join("\n"), + ); + + try { + expectPid(pid); + expect(data.timedOut).toBe(true); + expect(elapsed).toBeGreaterThanOrEqual(START_BUDGET_MS - 100); + expect(elapsed).toBeLessThan(START_BUDGET_MS + OVERRUN_MS); + } finally { + killQuietly(pid); + } + }); + + it("escalates to SIGKILL for a command that ignores SIGTERM", async () => { + const { data, elapsed } = await runTimed( + "process.on('SIGTERM', () => {}); process.stdout.write('ready\\n'); setTimeout(() => {}, 60000);", + ); + + expect(data.stdout, "the child was killed before it started, raise START_BUDGET_MS").toContain("ready"); + expect(data.timedOut).toBe(true); + expect(elapsed).toBeLessThan(START_BUDGET_MS + OVERRUN_MS); + if (!isWindows) { + expect(data.signal).toBe("SIGKILL"); + expect(elapsed).toBeGreaterThanOrEqual(START_BUDGET_MS + 1900); // timeout + SIGTERM grace + } + }); + + it.skipIf(isWindows)("returns around the timeout when a shell backgrounds a process that holds the pipes", async () => { + const startedAt = Date.now(); + const result = await handler()({ command: "sleep 100000 &", timeout_ms: 1000 }); + const elapsed = Date.now() - startedAt; + const data = JSON.parse(result.content[0].text); + + expect(data.timedOut).toBe(true); + expect(elapsed).toBeLessThan(1000 + OVERRUN_MS); + }); + + it("does not report a timeout for a command that finishes in time", async () => { + const result = await handler()({ + argv: [node, "-e", "process.stdout.write('done')"], + timeout_ms: 30_000, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(data.timedOut).toBe(false); + expect(data.stdout).toBe("done"); + }); + }); + + describe("child environment", () => { + it("scrubs server credentials whatever the case of their names", async () => { + process.env["Op_Service_Account_Token"] = "ops_mixed_case_token_12345"; + process.env["op_keychain_service"] = "mixed-case-service"; + process.env["OP_Keychain_Account"] = "mixed-case-account"; + resetConfig(); + + const result = await handler()({ + argv: [ + node, + "-e", + "process.stdout.write(JSON.stringify(Object.keys(process.env).filter((key) => /^op_(service_account_token|keychain_service|keychain_account)$/i.test(key))))", + ], + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(JSON.parse(data.stdout)).toEqual([]); + }); + + it("still applies caller-supplied values for those names", async () => { + process.env["Op_Keychain_Service"] = "server-value"; + resetConfig(); + + const result = await handler()({ + argv: [node, "-e", "process.stdout.write(String(process.env.OP_KEYCHAIN_SERVICE))"], + env: { OP_KEYCHAIN_SERVICE: "caller-value" }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.stdout).toBe("caller-value"); + }); + }); + + describe("vault allow-list on the vault a reference resolves to", () => { + const VAULTS = [ + { id: "vault-private", title: "Private" }, + { id: "vault-prod", title: "Prod" }, + ]; + const SECRET = "the-secret-value"; + let markerDir: string; + let marker: string; + + beforeEach(() => { + markerDir = mkdtempSync(join(tmpdir(), "op-run-allowlist-")); + marker = join(markerDir, "child-ran"); + }); + + afterEach(() => { + rmSync(markerDir, { recursive: true, force: true }); + }); + + function setAllowList(value: string): void { + process.env.OP_MCP_ALLOWED_VAULTS = value; + resetConfig(); + } + + /** Creates the marker file (proof it ran) and exits 0 only if it sees the secret. */ + function markerCommand(): string[] { + return [ + node, + "-e", + `require('node:fs').writeFileSync(process.env.MARKER, 'ran'); process.exit(process.env.MY_SECRET === '${SECRET}' ? 0 : 3)`, + ]; + } + + it("rejects a reference that resolves to a vault outside the allow-list, before running anything", async () => { + setAllowList("Private"); + const resolveAll = mockBulkResolve( + { "op://Private/api/key": SECRET }, + { vaultIds: { "op://Private/api/key": "vault-prod" }, vaults: VAULTS }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://Private/api/key", MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not in the allowed vault list"); + expect(result.content[0].text).toContain("vault-prod"); + expect(result.content[0].text).not.toContain(SECRET); + // The written vault segment passed the textual pre-check, so it was the ID check that stopped it. + expect(resolveAll).toHaveBeenCalledOnce(); + expect(resolveAll.vaultsList).toHaveBeenCalledOnce(); + expect(existsSync(marker)).toBe(false); + }); + + it("injects the value when the same reference resolves to an allow-listed vault", async () => { + setAllowList("Private"); + const resolveAll = mockBulkResolve( + { "op://Private/api/key": SECRET }, + { vaultIds: { "op://Private/api/key": "vault-private" }, vaults: VAULTS }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://Private/api/key", MARKER: marker }, + }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.exitCode).toBe(0); // 0 only if the child saw the secret + expect(existsSync(marker)).toBe(true); // the same command that never ran above + expect(resolveAll.vaultsList).toHaveBeenCalledOnce(); + }); + + it("accepts an allow-list entry and a reference that are both written as the vault ID", async () => { + setAllowList("vault-private"); + mockBulkResolve( + { "op://vault-private/api/key": SECRET }, + { vaultIds: { "op://vault-private/api/key": "vault-private" }, vaults: VAULTS }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://vault-private/api/key", MARKER: marker }, + }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.exitCode).toBe(0); + expect(existsSync(marker)).toBe(true); + }); + + it("rejects a reference written with an allowed vault ID that resolves elsewhere", async () => { + setAllowList("vault-private"); + mockBulkResolve( + { "op://vault-private/api/key": SECRET }, + { vaultIds: { "op://vault-private/api/key": "vault-prod" }, vaults: VAULTS }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://vault-private/api/key", MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("not in the allowed vault list"); + expect(existsSync(marker)).toBe(false); + }); + + it("does not list vaults when no allow-list is configured", async () => { + const resolveAll = mockBulkResolve( + { "op://Private/api/key": SECRET }, + { vaultIds: { "op://Private/api/key": "vault-prod" }, vaults: VAULTS }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://Private/api/key", MARKER: marker }, + }); + const data = JSON.parse(result.content[0].text); + + expect(result.isError).toBeUndefined(); + expect(data.exitCode).toBe(0); + expect(existsSync(marker)).toBe(true); + expect(resolveAll).toHaveBeenCalledOnce(); + expect(resolveAll.vaultsList).not.toHaveBeenCalled(); + }); + + it("lists vaults once however many references are resolved", async () => { + setAllowList("Private,Prod"); + const references = { FIRST: "op://Private/a/token", SECOND: "op://Prod/b/token" }; + const resolveAll = mockBulkResolve( + { [references.FIRST]: "first-value", [references.SECOND]: "second-value" }, + { + vaultIds: { [references.FIRST]: "vault-private", [references.SECOND]: "vault-prod" }, + vaults: VAULTS, + }, + ); + + const result = await handler()({ + argv: [node, "-e", "process.exit(process.env.FIRST === 'first-value' && process.env.SECOND === 'second-value' ? 0 : 1)"], + env: references, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(resolveAll).toHaveBeenCalledOnce(); + expect(resolveAll.vaultsList).toHaveBeenCalledOnce(); + }); + + it("runs nothing if any one of several references resolves to a disallowed vault", async () => { + setAllowList("Private,Prod"); + const references = { FIRST: "op://Private/a/token", SECOND: "op://Prod/b/token" }; + const resolveAll = mockBulkResolve( + { [references.FIRST]: "first-value", [references.SECOND]: "second-value" }, + { + vaultIds: { [references.FIRST]: "vault-private", [references.SECOND]: "vault-elsewhere" }, + vaults: VAULTS, + }, + ); + + const result = await handler()({ + argv: markerCommand(), + env: { ...references, MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("vault-elsewhere"); + expect(result.content[0].text).not.toContain("first-value"); + expect(resolveAll.vaultsList).toHaveBeenCalledOnce(); + expect(existsSync(marker)).toBe(false); + }); + + it("reports an unresolvable reference before checking any vault", async () => { + setAllowList("Private"); + const resolveAll = vi.fn().mockResolvedValue({ + individualResponses: { + "op://Private/a/token": { content: { secret: "first-value", vaultId: "vault-prod" } }, + "op://Private/b/token": { error: { type: "itemNotFound" } }, + }, + }); + const list = vi.fn().mockResolvedValue(VAULTS); + mockedGetClient.mockResolvedValue({ secrets: { resolveAll }, vaults: { list } } as any); + + const result = await handler()({ + argv: markerCommand(), + env: { FIRST: "op://Private/a/token", SECOND: "op://Private/b/token", MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + "Could not resolve secret reference 'op://Private/b/token' (itemNotFound)", + ); + expect(result.content[0].text).not.toContain("allowed vault list"); + expect(list).not.toHaveBeenCalled(); + expect(existsSync(marker)).toBe(false); + }); + + it("fails closed, with the error still redacted, when the vaults cannot be listed", async () => { + const token = "ops_secret_master_token_12345"; + process.env.OP_SERVICE_ACCOUNT_TOKEN = token; + setAllowList("Private"); + const resolveAll = mockBulkResolve( + { "op://Private/api/key": SECRET }, + { vaultIds: { "op://Private/api/key": "vault-private" }, vaults: [] }, + ); + resolveAll.vaultsList.mockRejectedValue(new Error(`request failed for ${token}`)); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://Private/api/key", MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("request failed for «REDACTED:OP_SERVICE_ACCOUNT_TOKEN»"); + expect(result.content[0].text).not.toContain(token); + expect(existsSync(marker)).toBe(false); + }); + + it("fails closed when the SDK client cannot list vaults at all", async () => { + setAllowList("Private"); + mockBulkResolve({ "op://Private/api/key": SECRET }, { vaultIds: { "op://Private/api/key": "vault-private" } }); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: "op://Private/api/key", MARKER: marker }, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("does not support listing vaults"); + expect(existsSync(marker)).toBe(false); + }); + + it("does not touch the SDK for a literal-only env, even with an allow-list", async () => { + setAllowList("Private"); + + const result = await handler()({ + argv: markerCommand(), + env: { MY_SECRET: SECRET, MARKER: marker }, + }); + const data = JSON.parse(result.content[0].text); + + expect(data.exitCode).toBe(0); + expect(existsSync(marker)).toBe(true); + expect(mockedGetClient).not.toHaveBeenCalled(); + }); + }); }); diff --git a/tests/redaction.test.ts b/tests/redaction.test.ts new file mode 100644 index 0000000..9797aab --- /dev/null +++ b/tests/redaction.test.ts @@ -0,0 +1,604 @@ +/** + * Unit tests for the pure op_run redaction module: pattern derivation, + * order-independent masking, and output finalization (cap + cut-off secrets). + */ + +import { describe, it, expect } from "vitest"; +import { + OutputCapture, + buildRedactionPatterns, + finalizeOutput, + maxPatternByteLength, + redact, + type RedactionPattern, +} from "../src/redaction.js"; + +const MIB = 1024 * 1024; + +function valuesOf(value: string, name = "S"): string[] { + return buildRedactionPatterns([{ name, value }]).map((pattern) => pattern.value); +} + +/** Deterministic PRNG so the randomized tests are reproducible. */ +function makeRandom(seed: number): () => number { + let state = seed >>> 0; + return () => { + state = (Math.imul(state, 1664525) + 1013904223) >>> 0; + return state / 0x100000000; + }; +} + +describe("buildRedactionPatterns", () => { + it("includes the raw value and tags every pattern with the secret's name", () => { + const patterns = buildRedactionPatterns([{ name: "TOKEN", value: "abc123xyz789" }]); + + expect(patterns[0]).toEqual({ name: "TOKEN", value: "abc123xyz789" }); + expect(patterns.every((pattern) => pattern.name === "TOKEN")).toBe(true); + }); + + it("ignores empty values and de-duplicates by value, keeping the first name", () => { + const patterns = buildRedactionPatterns([ + { name: "EMPTY", value: "" }, + { name: "FIRST", value: "shared-secret-value" }, + { name: "SECOND", value: "shared-secret-value" }, + ]); + + expect(patterns.length).toBeGreaterThan(0); + expect(patterns.every((pattern) => pattern.name === "FIRST")).toBe(true); + const values = patterns.map((pattern) => pattern.value); + expect(new Set(values).size).toBe(values.length); + }); + + it("adds no extra literal forms when JSON and URL encoding leave the value unchanged", () => { + // Raw value plus its three base64 alignments, nothing else. + expect(valuesOf("plainvalue1")).toHaveLength(4); + }); + + it("adds the JSON-escaped form", () => { + const secret = 'pa"ss\\wo\nrd'; + expect(valuesOf(secret)).toContain(JSON.stringify(secret).slice(1, -1)); + }); + + it("adds the URL-encoded form", () => { + expect(valuesOf("p w/d+x=y")).toContain("p%20w%2Fd%2Bx%3Dy"); + }); + + it("skips the URL-encoded form for lone surrogates instead of throwing", () => { + const secret = "ab\ud800cdefgh"; + expect(() => buildRedactionPatterns([{ name: "S", value: secret }])).not.toThrow(); + const values = valuesOf(secret); + expect(values).toContain(secret); + expect(values.some((value) => value.includes("%"))).toBe(false); + }); + + describe("base64 fragments", () => { + const secrets = [ + "password", + "Zx9-real-secret-value", + "pässwörd-€-🔑-xyz", + "supersecretkey999", + "????>>>>>>~~~~~~", + ]; + + it("appear in the encoding of any payload embedding the secret at any offset", () => { + for (const secret of secrets) { + const fragments = valuesOf(secret); + for (let prefixLength = 0; prefixLength <= 5; prefixLength++) { + const payload = Buffer.from("x".repeat(prefixLength) + secret + "tail").toString("base64"); + expect( + fragments.some((fragment) => payload.includes(fragment)), + `prefix length ${prefixLength}, secret ${secret}`, + ).toBe(true); + } + } + }); + + it("are independent of the bytes surrounding the secret", () => { + const random = makeRandom(1234); + const randomBytes = (length: number) => + Buffer.from(Array.from({ length }, () => Math.floor(random() * 256))); + + for (const secret of secrets) { + const fragments = valuesOf(secret); + for (let prefixLength = 0; prefixLength <= 8; prefixLength++) { + for (let suffixLength = 0; suffixLength <= 4; suffixLength++) { + const payload = Buffer.concat([ + randomBytes(prefixLength), + Buffer.from(secret, "utf8"), + randomBytes(suffixLength), + ]).toString("base64"); + expect( + fragments.some((fragment) => payload.includes(fragment)), + `prefix ${prefixLength}, suffix ${suffixLength}, secret ${secret}`, + ).toBe(true); + } + } + } + }); + + it("are skipped when shorter than eight characters", () => { + expect(valuesOf("abc")).toEqual(["abc"]); + }); + + it("are standard base64, not base64url", () => { + const fragments = valuesOf("????>>>>>>~~~~~~").filter((value) => /^[A-Za-z0-9+/]+$/.test(value)); + expect(fragments.some((value) => /[+/]/.test(value))).toBe(true); + expect(valuesOf("????>>>>>>~~~~~~").some((value) => /[-_]/.test(value))).toBe(false); + }); + }); + + describe("multi-line secrets", () => { + const secret = [ + "-----BEGIN KEY-----", + "AAAABBBBCCCCDDDD", + "short", + " EEEEFFFFGGGG \r", + "-----END KEY-----", + ].join("\n"); + + it("adds the CRLF rendering", () => { + expect(valuesOf(secret)).toContain(secret.replace(/\r?\n/g, "\r\n")); + }); + + it("adds each trimmed line of at least eight characters", () => { + const values = valuesOf(secret); + + expect(values).toContain("-----BEGIN KEY-----"); + expect(values).toContain("AAAABBBBCCCCDDDD"); + expect(values).toContain("EEEEFFFFGGGG"); + expect(values).toContain("-----END KEY-----"); + expect(values).not.toContain("short"); + }); + + it("adds neither for single-line secrets", () => { + const values = valuesOf("single-line-secret-value"); + expect(values.some((value) => value.includes("\r\n"))).toBe(false); + }); + + it("keeps a secret with a trailing newline detectable without it", () => { + expect(valuesOf("token-with-newline\n")).toContain("token-with-newline"); + }); + }); +}); + +describe("redact", () => { + const pattern = (name: string, value: string): RedactionPattern => ({ name, value }); + + it("returns the text untouched when there are no patterns or no matches", () => { + const text = "nothing to see here"; + expect(redact(text, [])).toBe(text); + expect(redact(text, [pattern("S", "missing-value")])).toBe(text); + expect(redact("", [pattern("S", "abc")])).toBe(""); + }); + + it("ignores empty pattern values", () => { + expect(redact("abc", [pattern("S", "")])).toBe("abc"); + }); + + it("replaces a single match with the exact marker", () => { + expect(redact("token=hunter22!", [pattern("MY_SECRET", "hunter22!")])).toBe( + "token=«REDACTED:MY_SECRET»", + ); + }); + + it("replaces every occurrence and keeps the surrounding text", () => { + expect(redact("x abc y abc z", [pattern("S", "abc")])).toBe( + "x «REDACTED:S» y «REDACTED:S» z", + ); + }); + + it("treats secret values and names literally, not as regexes or replacement templates", () => { + const secret = "a$&(b)*c.$1"; + expect(redact(`x${secret}y${secret}`, [pattern("N$&M", secret)])).toBe( + "x«REDACTED:N$&M»y«REDACTED:N$&M»", + ); + }); + + it("masks secrets containing multi-byte characters", () => { + expect(redact("a pw-🔑-secret b", [pattern("S", "pw-🔑-secret")])).toBe("a «REDACTED:S» b"); + }); + + it("does not leak the remainder of a longer secret that contains a shorter one", () => { + const user = pattern("API_USER", "svc-bot"); + const credentials = pattern("API_CREDENTIALS", "svc-bot:Zx9-real-secret"); + const text = "auth=svc-bot:Zx9-real-secret user=svc-bot"; + + for (const patterns of [[user, credentials], [credentials, user]]) { + const redacted = redact(text, patterns); + expect(redacted).not.toContain("Zx9-real-secret"); + expect(redacted).not.toContain("svc-bot"); + expect(redacted).toMatch(/^auth=«REDACTED:API_[A-Z_,]+» user=«REDACTED:API_USER»$/); + } + expect(redact(text, [user, credentials])).toBe( + "auth=«REDACTED:API_USER,API_CREDENTIALS» user=«REDACTED:API_USER»", + ); + }); + + it("merges partially overlapping matches into one marker naming both", () => { + expect(redact("xabcdefx", [pattern("A", "abcd"), pattern("B", "cdef")])).toBe( + "x«REDACTED:A,B»x", + ); + }); + + it("merges adjacent matches into one marker", () => { + expect(redact("abcd", [pattern("A", "ab"), pattern("B", "cd")])).toBe("«REDACTED:A,B»"); + expect(redact("tokentoken", [pattern("T", "token")])).toBe("«REDACTED:T»"); + }); + + it("merges a nested match into the enclosing one, listing names by first appearance", () => { + const outer = pattern("OUTER", "xx-secret-yy"); + const inner = pattern("INNER", "secret"); + + expect(redact("a xx-secret-yy b", [inner, outer])).toBe("a «REDACTED:OUTER,INNER» b"); + }); + + it("breaks ties between matches starting together by pattern order", () => { + const text = "abcdef"; + expect(redact(text, [pattern("SHORT", "ab"), pattern("LONG", "abcdef")])).toBe( + "«REDACTED:SHORT,LONG»", + ); + expect(redact(text, [pattern("LONG", "abcdef"), pattern("SHORT", "ab")])).toBe( + "«REDACTED:LONG,SHORT»", + ); + }); + + it("collapses overlapping occurrences of the same pattern", () => { + expect(redact("aaaaaaa", [pattern("A", "aaaa")])).toBe("«REDACTED:A»"); + expect(redact("ababab", [pattern("A", "abab")])).toBe("«REDACTED:A»"); + }); + + it("keeps separate markers for matches that neither overlap nor touch", () => { + expect(redact("ab-ab", [pattern("A", "ab")])).toBe("«REDACTED:A»-«REDACTED:A»"); + }); + + it("lists each name once when several patterns of the same secret merge", () => { + expect(redact("abcd", [pattern("A", "abc"), pattern("A", "bcd")])).toBe("«REDACTED:A»"); + }); + + it("names a merged run in order of first appearance, not pattern order", () => { + expect(redact("xxzz", [pattern("B", "zz"), pattern("A", "xx")])).toBe("«REDACTED:A,B»"); + }); + + it("matches a naive reference implementation on random inputs", () => { + const naive = (text: string, patterns: RedactionPattern[]): string => { + const matches: { start: number; end: number; order: number; name: string }[] = []; + patterns.forEach(({ name, value }, order) => { + for (let i = 0; i + value.length <= text.length; i++) { + if (text.startsWith(value, i)) matches.push({ start: i, end: i + value.length, order, name }); + } + }); + matches.sort((a, b) => a.start - b.start || a.order - b.order); + + let output = ""; + let copied = 0; + let i = 0; + while (i < matches.length) { + const start = matches[i].start; + let end = matches[i].end; + const names: string[] = []; + while (i < matches.length && matches[i].start <= end) { + end = Math.max(end, matches[i].end); + if (!names.includes(matches[i].name)) names.push(matches[i].name); + i++; + } + output += text.slice(copied, start) + `«REDACTED:${names.join(",")}»`; + copied = end; + } + return output + text.slice(copied); + }; + + const random = makeRandom(42); + const pick = (alphabet: string) => alphabet[Math.floor(random() * alphabet.length)]; + for (let iteration = 0; iteration < 3000; iteration++) { + const alphabet = random() < 0.5 ? "ab" : "abc"; + const text = Array.from({ length: 1 + Math.floor(random() * 40) }, () => pick(alphabet)).join(""); + const patterns = Array.from({ length: 1 + Math.floor(random() * 4) }, () => ({ + name: pick("ABC"), + value: Array.from({ length: 1 + Math.floor(random() * 5) }, () => pick(alphabet)).join(""), + })); + + expect(redact(text, patterns), JSON.stringify({ text, patterns })).toBe(naive(text, patterns)); + } + }); + + describe("limit", () => { + const patterns = [pattern("A", "abc-def")]; + + it("cuts plain text at the limit", () => { + expect(redact("hello world", [pattern("S", "zzz")], 5)).toBe("hello"); + expect(redact("hello abc-def world", patterns, 3)).toBe("hel"); + }); + + it("emits a marker for a run that starts before the limit even if it ends after it", () => { + expect(redact("xx abc-def yy", patterns, 6)).toBe("xx «REDACTED:A»"); + }); + + it("drops runs that start at or after the limit", () => { + expect(redact("xx abc-def yy", patterns, 3)).toBe("xx "); + }); + }); + + it("redacts a 5 MiB output quickly, even when every position matches", () => { + const secret = "Zx9-real-secret-0-with-some-length"; + const noise = "lorem ipsum dolor sit amet ".repeat(Math.ceil((5 * MIB) / 27)); + const patterns = buildRedactionPatterns([{ name: "S", value: secret }]); + + const started = Date.now(); + expect(redact(noise, patterns)).toBe(noise); + expect(redact(("a".repeat(MIB * 5)), [pattern("A", "aaaaaaaa")])).toBe("«REDACTED:A»"); + expect(redact(`${secret}\n`.repeat(100_000), patterns)).toBe("«REDACTED:S»\n".repeat(100_000)); + expect(Date.now() - started).toBeLessThan(10_000); + }); +}); + +describe("maxPatternByteLength", () => { + it("is zero without patterns", () => { + expect(maxPatternByteLength([])).toBe(0); + }); + + it("measures UTF-8 bytes, not characters", () => { + expect( + maxPatternByteLength([ + { name: "A", value: "abc" }, + { name: "B", value: "é€🔑" }, + ]), + ).toBe(9); + }); +}); + +describe("finalizeOutput", () => { + const MARKER_BYTES = (name: string) => Buffer.byteLength(`«REDACTED:${name}»`); + + it("changes nothing when there are no patterns and the output is within the cap", () => { + expect(finalizeOutput(Buffer.from("hello"), false, [], 100)).toEqual({ + text: "hello", + truncated: false, + }); + expect(finalizeOutput(Buffer.alloc(0), false, [], 100)).toEqual({ text: "", truncated: false }); + }); + + it("does not strip anything from output that did not overflow", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "supersecretkey999" }]); + expect(finalizeOutput(Buffer.from("A super"), false, patterns, 100)).toEqual({ + text: "A super", + truncated: false, + }); + }); + + it("still caps and flags output when there are no patterns", () => { + expect(finalizeOutput(Buffer.from("x".repeat(10)), false, [], 4)).toEqual({ + text: "xxxx", + truncated: true, + }); + expect(finalizeOutput(Buffer.from("x".repeat(4)), false, [], 4)).toEqual({ + text: "xxxx", + truncated: false, + }); + }); + + it("flags overflowed output as truncated even when it fits the cap", () => { + expect(finalizeOutput(Buffer.from("short"), true, [], 100)).toEqual({ + text: "short", + truncated: true, + }); + }); + + it("masks secrets", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "abc123xyz789" }]); + expect(finalizeOutput(Buffer.from("t=abc123xyz789"), false, patterns, 100)).toEqual({ + text: "t=«REDACTED:S»", + truncated: false, + }); + }); + + it("flags truncation when markers make the masked text larger than the cap", () => { + const patterns = buildRedactionPatterns([{ name: "PIN", value: "pin1" }]); + const raw = Buffer.from("pin1 pin1 pin1 pin1"); + expect(raw.length).toBeLessThan(MARKER_BYTES("PIN") * 4); + + const result = finalizeOutput(raw, false, patterns, 40); + + expect(result.truncated).toBe(true); + expect(Buffer.byteLength(result.text)).toBeLessThanOrEqual(40); + expect(result.text).not.toContain("pin1"); + }); + + it("masks a secret that straddles the cap before cutting", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "supersecretkey999" }]); + const raw = Buffer.from("A".repeat(30) + "supersecretkey999" + "trailing"); + + const result = finalizeOutput(raw, false, patterns, 40); + + // 30 bytes of padding plus the first 10 bytes of the marker (« is two bytes). + expect(result.truncated).toBe(true); + expect(result.text).toBe("A".repeat(30) + "«REDACTED"); + expect(result.text).not.toContain("supers"); + }); + + describe("when the capture overflowed", () => { + it("drops a secret prefix that markers pulled back inside the cap", () => { + const bulk = "L".repeat(40); + const tail = "supersecretkey999"; + const patterns = buildRedactionPatterns([ + { name: "BULK", value: bulk }, + { name: "TAIL", value: tail }, + ]); + const cap = 120; + const window = cap + maxPatternByteLength(patterns); + const cutOff = 15; // characters of `tail` that fit in the retained window + const head = ("A" + bulk).repeat(3); + const filler = window - cutOff - head.length; + expect(filler).toBeGreaterThan(0); + const stream = Buffer.from(head + "B".repeat(filler) + tail + "-and-more"); + const captured = stream.subarray(0, window); + + // Plain masking leaves the cut-off prefix behind, comfortably inside the cap... + const masked = redact(captured.toString("utf8"), patterns); + expect(masked.endsWith(tail.slice(0, cutOff))).toBe(true); + expect(Buffer.byteLength(masked)).toBeLessThan(cap); + + // ...so finalization must remove it. + const result = finalizeOutput(captured, true, patterns, cap); + expect(result.truncated).toBe(true); + expect(result.text).toBe(("A«REDACTED:BULK»").repeat(3) + "B".repeat(filler)); + expect(result.text).not.toContain("s"); + }); + + it("leaves no prefix of the secret at the end when the window cuts it", () => { + const secret = "supersecretkey999"; + const patterns = buildRedactionPatterns([{ name: "S", value: secret }]); + + for (let kept = 1; kept < secret.length; kept++) { + const captured = Buffer.from("A".repeat(10) + secret.slice(0, kept)); + const { text, truncated } = finalizeOutput(captured, true, patterns, 100); + + expect(truncated).toBe(true); + expect(text).toBe("A".repeat(10)); + } + }); + + it("drops the longest cut-off prefix when several patterns could match", () => { + const patterns = buildRedactionPatterns([ + { name: "ONE", value: "xyz-long-secret-one" }, + { name: "TWO", value: "yz-long-secret-two" }, + ]); + // "xyz-lo" starts ONE and its tail "yz-lo" starts TWO; the longer one decides the cut. + const captured = Buffer.from("q xyz-lo"); + + expect(finalizeOutput(captured, true, patterns, 100).text).toBe("q "); + }); + + it("keeps a complete secret at the very end masked", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "supersecretkey999" }]); + const captured = Buffer.from("id=supersecretkey999"); + + expect(finalizeOutput(captured, true, patterns, 100)).toEqual({ + text: "id=«REDACTED:S»", + truncated: true, + }); + }); + + it("drops the cut-off prefix of one secret that overlaps a masked run of another", () => { + const patterns = buildRedactionPatterns([ + { name: "A", value: "abc-def" }, + { name: "B", value: "def-ghijklmn" }, + ]); + // "def-ghij" is the start of B; its first three characters are also the end of A. + const captured = Buffer.from("xx abc-def-ghij"); + + const { text } = finalizeOutput(captured, true, patterns, 100); + + expect(text).toBe("xx «REDACTED:A»"); + expect(text).not.toContain("ghij"); + }); + + it("keeps a masked run that ends exactly where the cut-off secret begins", () => { + const patterns = buildRedactionPatterns([ + { name: "A", value: "alpha-key-1" }, + { name: "B", value: "supersecretkey999" }, + ]); + + expect(finalizeOutput(Buffer.from("id=alpha-key-1supersec"), true, patterns, 100).text).toBe( + "id=«REDACTED:A»", + ); + }); + + it("trims a multi-byte character cut in half so the secret before it is still recognised", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "secret-é-more" }]); + const full = Buffer.from("AAAA secret-é-more"); + const captured = full.subarray(0, full.indexOf(0xc3) + 1); + expect(captured.toString("utf8").endsWith("secret-�")).toBe(true); + + const result = finalizeOutput(captured, true, patterns, 100); + + expect(result.text).toBe("AAAA "); + expect(result.text).not.toContain("�"); + }); + + it("keeps decoding a partial character leniently when nothing was dropped", () => { + const cutEmoji = Buffer.from("ab🔑").subarray(0, 4); + expect(finalizeOutput(cutEmoji, false, [], 100).text).toContain("�"); + expect(finalizeOutput(cutEmoji, true, [], 100).text).toBe("ab"); + }); + }); + + describe("character boundaries", () => { + // a = 1 byte, é = 2, 🔑 = 4, z = 1 + const raw = Buffer.from("aé🔑z"); + + it("never cuts through a multi-byte character", () => { + const expected: Record = { + 0: "", + 1: "a", + 2: "a", + 3: "aé", + 4: "aé", + 5: "aé", + 6: "aé", + 7: "aé🔑", + 8: "aé🔑z", + }; + for (const [maxBytes, text] of Object.entries(expected)) { + const result = finalizeOutput(raw, false, [], Number(maxBytes)); + expect(result.text, `maxBytes ${maxBytes}`).toBe(text); + expect(result.text).not.toContain("�"); + expect(result.truncated).toBe(Number(maxBytes) < raw.length); + } + }); + + it("never cuts through a multi-byte character inside a marker", () => { + const patterns = buildRedactionPatterns([{ name: "S", value: "supersecretkey999" }]); + // The marker starts with « (2 bytes); a one-byte budget after the text would split it. + const result = finalizeOutput(Buffer.from("xyzsupersecretkey999"), false, patterns, 4); + + expect(result.text).toBe("xyz"); + expect(result.truncated).toBe(true); + }); + }); +}); + +describe("OutputCapture", () => { + it("starts empty and not overflowed", () => { + const capture = new OutputCapture(10); + expect(capture.toBuffer()).toHaveLength(0); + expect(capture.overflowed).toBe(false); + }); + + it("keeps everything up to the limit without flagging overflow", () => { + const capture = new OutputCapture(10); + capture.push(Buffer.from("hello")); + capture.push(Buffer.from("world")); + + expect(capture.toBuffer().toString()).toBe("helloworld"); + expect(capture.overflowed).toBe(false); + }); + + it("cuts the chunk that crosses the limit and discards the rest", () => { + const capture = new OutputCapture(8); + capture.push(Buffer.from("hello")); + capture.push(Buffer.from("world")); + capture.push(Buffer.from("more")); + + expect(capture.toBuffer().toString()).toBe("hellowor"); + expect(capture.overflowed).toBe(true); + }); + + it("retains only the budget while a flood of data passes through", () => { + const limit = 5 * MIB; + const capture = new OutputCapture(limit); + const chunk = Buffer.alloc(64 * 1024); + let sent = 0; + for (let i = 0; sent < 20 * MIB; i++) { + chunk.fill(i % 251); + capture.push(Buffer.from(chunk)); + sent += chunk.length; + } + + const kept = capture.toBuffer(); + expect(kept).toHaveLength(limit); + expect(capture.overflowed).toBe(true); + // The retained bytes are the first ones written. + expect(kept[0]).toBe(0); + expect(kept[kept.length - 1]).toBe(Math.floor((limit - 1) / chunk.length) % 251); + }); +}); diff --git a/tests/tools.test.ts b/tests/tools.test.ts index f92a6d4..a7c7c91 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -2,7 +2,7 @@ * Tests for MCP tool handlers with mocked 1Password client. */ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { McpServer } from "@modelcontextprotocol/server"; // Mock the client module before importing tools @@ -18,6 +18,7 @@ vi.mock("../src/logger.js", () => ({ logError: vi.fn(), })); import { getClient } from "../src/client.js"; +import { resetConfig } from "../src/config.js"; import { registerAllTools } from "../src/tools/index.js"; const mockedGetClient = vi.mocked(getClient); @@ -25,9 +26,13 @@ const mockedGetClient = vi.mocked(getClient); describe("MCP Tools", () => { let server: McpServer; let registeredTools: Map; + const originalEnv = { ...process.env }; beforeEach(() => { vi.clearAllMocks(); + // These tests exercise unrestricted behavior; the allow-list has its own tests. + resetConfig(); + delete process.env.OP_MCP_ALLOWED_VAULTS; server = new McpServer({ name: "test", version: "0.0.0" }); // Spy on server.tool to capture registered handlers @@ -46,6 +51,14 @@ describe("MCP Tools", () => { registerAllTools(server); }); + afterEach(() => { + Object.keys(process.env).forEach((key) => { + if (!(key in originalEnv)) delete process.env[key]; + else process.env[key] = originalEnv[key]; + }); + resetConfig(); + }); + it("registers all 15 tools", () => { expect(registeredTools.size).toBe(15); expect(registeredTools.has("vault_list")).toBe(true); @@ -82,6 +95,17 @@ describe("MCP Tools", () => { ); }); + it("documents that item_get hides secret-bearing fields unless reveal is true", () => { + const itemGet = registeredTools.get("item_get")!; + + for (const text of [itemGet.description, itemGet.schema.shape.reveal.description]) { + expect(text).toContain("SSH private keys"); + expect(text).toContain("one-time-password seeds"); + expect(text).toContain("card numbers"); + } + expect(itemGet.description).toContain("hidden unless reveal is true"); + }); + it("documents note_create custom fields as id or title", () => { const noteCreate = registeredTools.get("note_create")!; const fieldInput = noteCreate.schema.shape.fields.unwrap().element; @@ -210,56 +234,98 @@ describe("MCP Tools", () => { }); describe("password_read", () => { - it("resolves a secret reference and returns the value when reveal is true", async () => { - mockedGetClient.mockResolvedValue({ - secrets: { - resolve: vi.fn().mockResolvedValue("my-secret-value"), + const reference = "op://vault/item/password"; + + /** Mock a client whose resolveAll returns `secret` for `reference`. */ + function mockResolvedReference(secret: string) { + const resolveAll = vi.fn().mockResolvedValue({ + individualResponses: { + [reference]: { content: { secret, itemId: "i1", vaultId: "v1" } }, }, - } as any); + }); + mockedGetClient.mockResolvedValue({ secrets: { resolveAll } } as any); + return resolveAll; + } + + it("resolves a secret reference and returns the value when reveal is true", async () => { + const resolveAll = mockResolvedReference("my-secret-value"); const handler = registeredTools.get("password_read")!.handler; const result = await handler({ - secretReference: "op://vault/item/password", + secretReference: reference, reveal: true, }); const data = JSON.parse(result.content[0].text); - expect(data.value).toBe("my-secret-value"); + expect(data).toEqual({ value: "my-secret-value" }); + expect(resolveAll).toHaveBeenCalledWith([reference]); }); it("returns metadata only by default (reveal omitted)", async () => { - mockedGetClient.mockResolvedValue({ - secrets: { - resolve: vi.fn().mockResolvedValue("my-secret-value"), - }, - } as any); + mockResolvedReference("my-secret-value"); const handler = registeredTools.get("password_read")!.handler; const result = await handler({ - secretReference: "op://vault/item/password", + secretReference: reference, }); const data = JSON.parse(result.content[0].text); - expect(data.resolved).toBe(true); - expect(data.value).toBeUndefined(); + expect(data).toEqual({ resolved: true }); + expect(result.content[0].text).not.toContain("my-secret-value"); }); it("returns metadata only when reveal is false", async () => { - mockedGetClient.mockResolvedValue({ - secrets: { - resolve: vi.fn().mockResolvedValue("secret"), - }, - } as any); + mockResolvedReference("hunter2-secret-value"); const handler = registeredTools.get("password_read")!.handler; const result = await handler({ - secretReference: "op://vault/item/password", + secretReference: reference, reveal: false, }); const data = JSON.parse(result.content[0].text); - expect(data.resolved).toBe(true); - expect(data.value).toBeUndefined(); + expect(data).toEqual({ resolved: true }); + expect(result.content[0].text).not.toContain("hunter2-secret-value"); + }); + + it("errors when the secret reference does not resolve", async () => { + mockedGetClient.mockResolvedValue({ + secrets: { + resolveAll: vi.fn().mockResolvedValue({ + individualResponses: { [reference]: { error: { type: "itemNotFound" } } }, + }), + }, + } as any); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ secretReference: reference, reveal: true }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + `Could not resolve secret reference '${reference}' (itemNotFound)`, + ); + }); + + it("errors when the SDK cannot resolve secret references", async () => { + mockedGetClient.mockResolvedValue({ secrets: {} } as any); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ secretReference: reference }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("does not support resolving secrets"); + }); + + it("errors on a malformed secret reference without calling the SDK", async () => { + const resolveAll = vi.fn(); + mockedGetClient.mockResolvedValue({ secrets: { resolveAll } } as any); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ secretReference: "not-a-reference" }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Invalid secret reference"); + expect(resolveAll).not.toHaveBeenCalled(); }); it("errors when neither secretReference nor vaultId/itemId provided", async () => { @@ -271,6 +337,62 @@ describe("MCP Tools", () => { expect(result.isError).toBe(true); expect(result.content[0].text).toContain("Provide secretReference or both vaultId and itemId"); }); + + describe("by vault ID + item ID", () => { + function mockItem() { + const get = vi.fn().mockResolvedValue({ + id: "i1", + title: "GitHub", + fields: [ + { id: "username", title: "username", fieldType: "Text", value: "octocat" }, + { id: "password", title: "password", fieldType: "Concealed", value: "s3cr3t" }, + ], + }); + mockedGetClient.mockResolvedValue({ items: { get } } as any); + return get; + } + + it("returns field metadata only by default", async () => { + const get = mockItem(); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ vaultId: "v1", itemId: "i1" }); + const data = JSON.parse(result.content[0].text); + + expect(get).toHaveBeenCalledWith("v1", "i1"); + expect(data).toEqual({ + id: "i1", + title: "GitHub", + field: "password", + fieldType: "Concealed", + }); + expect(result.content[0].text).not.toContain("s3cr3t"); + }); + + it("returns the requested field value when reveal is true", async () => { + mockItem(); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ + vaultId: "v1", + itemId: "i1", + field: "Username", + reveal: true, + }); + + expect(JSON.parse(result.content[0].text)).toEqual({ value: "octocat" }); + }); + + it("errors when the field is not on the item", async () => { + mockItem(); + + const handler = registeredTools.get("password_read")!.handler; + const result = await handler({ vaultId: "v1", itemId: "i1", field: "nope" }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Field 'nope' not found on item"); + }); + }); }); describe("item_get", () => { @@ -325,6 +447,165 @@ describe("MCP Tools", () => { expect(password.value).toBe("s3cr3t"); }); + describe("field types", () => { + /** Secret-bearing field types: hidden by default, including unknown/future ones. */ + const secretBearing = [ + { fieldType: "Concealed", value: "hunter2-password" }, + { + fieldType: "SshKey", + value: + "-----BEGIN OPENSSH PRIVATE KEY-----\nb3BlbnNzaC1rZXktdjEAAAAA\n-----END OPENSSH PRIVATE KEY-----", + }, + { + fieldType: "Totp", + value: "otpauth://totp/GitHub:octocat?secret=JBSWY3DPEHPK3PXP&issuer=GitHub", + }, + { fieldType: "CreditCardNumber", value: "4111111111111111" }, + { fieldType: "Unsupported", value: "opaque-unsupported-payload" }, + { fieldType: "SomeFutureSecretType", value: "future-type-secret" }, + ]; + + /** Known non-secret field types: shown by default. */ + const nonSecret = [ + { fieldType: "Text", value: "octocat" }, + { fieldType: "Url", value: "https://github.com/login" }, + { fieldType: "Email", value: "octocat@example.com" }, + { fieldType: "Phone", value: "+1 555 0100" }, + { fieldType: "Date", value: "2024-01-31" }, + { fieldType: "MonthYear", value: "2027-04" }, + { fieldType: "Menu", value: "Visa" }, + { fieldType: "CreditCardType", value: "visa" }, + { fieldType: "Address", value: "1 Main St, Springfield" }, + { fieldType: "Reference", value: "ref-to-another-item" }, + ]; + + function itemWith(fields: Array<{ fieldType: string; value: string }>) { + return { + ...sampleItem, + fields: fields.map((field, index) => ({ + id: `f${index}`, + title: `field-${field.fieldType}`, + ...field, + })), + }; + } + + async function getItem(item: unknown, reveal?: boolean) { + mockedGetClient.mockResolvedValue({ + items: { get: vi.fn().mockResolvedValue(item) }, + } as any); + const handler = registeredTools.get("item_get")!.handler; + const result = await handler({ vaultId: "v1", itemId: "i1", reveal }); + return { result, data: JSON.parse(result.content[0].text) }; + } + + /** The form a string takes inside the JSON response text (newlines etc. escaped). */ + const asJsonText = (value: string) => JSON.stringify(value).slice(1, -1); + + it.each(secretBearing)( + "conceals $fieldType values by default", + async ({ fieldType, value }) => { + const { result, data } = await getItem(itemWith([{ fieldType, value }])); + + expect(data.fields[0].type).toBe(fieldType); + expect(data.fields[0].value).toBe("[concealed]"); + expect(result.content[0].text).not.toContain(asJsonText(value)); + }, + ); + + it.each(secretBearing)( + "conceals $fieldType values when reveal is false", + async ({ fieldType, value }) => { + const { result, data } = await getItem(itemWith([{ fieldType, value }]), false); + + expect(data.fields[0].value).toBe("[concealed]"); + expect(result.content[0].text).not.toContain(asJsonText(value)); + }, + ); + + it.each(secretBearing)( + "reveals $fieldType values when reveal is true", + async ({ fieldType, value }) => { + const { data } = await getItem(itemWith([{ fieldType, value }]), true); + + expect(data.fields[0].value).toBe(value); + }, + ); + + it.each(nonSecret)( + "shows $fieldType values by default", + async ({ fieldType, value }) => { + const { data } = await getItem(itemWith([{ fieldType, value }])); + + expect(data.fields[0].value).toBe(value); + }, + ); + + it("treats a field with no type as secret-bearing", async () => { + const item = { + ...sampleItem, + fields: [{ id: "mystery", title: "mystery", value: "typeless-secret" }], + }; + const { result, data } = await getItem(item); + + expect(data.fields[0].value).toBe("[concealed]"); + expect(result.content[0].text).not.toContain("typeless-secret"); + }); + + it("conceals only the secret-bearing fields of a mixed item", async () => { + const { data } = await getItem( + itemWith([ + { fieldType: "Text", value: "octocat" }, + { fieldType: "Totp", value: "otpauth://totp/x?secret=ABC" }, + { fieldType: "Url", value: "https://example.com" }, + { fieldType: "SshKey", value: "private-key-material" }, + ]), + ); + + expect(data.fields.map((f: any) => f.value)).toEqual([ + "octocat", + "[concealed]", + "https://example.com", + "[concealed]", + ]); + }); + + it("never returns field details, even with reveal: true", async () => { + const item = { + ...sampleItem, + fields: [ + { + id: "otp", + title: "one-time password", + fieldType: "Totp", + value: "otpauth://totp/x?secret=ABC", + details: { type: "Otp", content: { code: "987654" } }, + }, + { + id: "key", + title: "private key", + fieldType: "SshKey", + value: "private-key-material", + details: { + type: "SshKey", + content: { publicKey: "ssh-ed25519 AAAA", fingerprint: "SHA256:fp", keyType: "Ed25519" }, + }, + }, + ], + }; + + for (const reveal of [false, true]) { + const { result, data } = await getItem(item, reveal); + + for (const field of data.fields) { + expect(field.details).toBeUndefined(); + } + expect(result.content[0].text).not.toContain("987654"); + expect(result.content[0].text).not.toContain("SHA256:fp"); + } + }); + }); + it("resolves vault/item from a secret reference", async () => { const get = vi.fn().mockResolvedValue(sampleItem); mockedGetClient.mockResolvedValue({ diff --git a/tests/vault-access.test.ts b/tests/vault-access.test.ts new file mode 100644 index 0000000..0ded076 --- /dev/null +++ b/tests/vault-access.test.ts @@ -0,0 +1,326 @@ +/** + * Tests for src/vault-access.ts — server-wide vault allow-list enforcement by + * vault ID, resolved through the vaults visible to the service account. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { resetConfig } from "../src/config.js"; +import { + assertVaultIdAllowed, + assertVaultIdsAllowed, + filterAllowedVaults, +} from "../src/vault-access.js"; + +const PROD = { id: "vlt-prod-0001", title: "Prod" }; +const DEV = { id: "vlt-dev-0002", title: "Dev" }; +const PRIVATE = { id: "vlt-private-0003", title: "Private" }; + +/** A client whose vaults.list resolves to `vaults`. */ +function makeClient(vaults: unknown[] = [PROD, DEV, PRIVATE]) { + const list = vi.fn().mockResolvedValue(vaults); + return { client: { vaults: { list } }, list }; +} + +function setAllowList(value: string) { + process.env.OP_MCP_ALLOWED_VAULTS = value; + resetConfig(); +} + +describe("vault-access", () => { + 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("with no allow-list configured (default)", () => { + it("allows any vault ID without calling the SDK", async () => { + const { client, list } = makeClient(); + + await expect(assertVaultIdAllowed(client, "anything")).resolves.toBeUndefined(); + await expect( + assertVaultIdsAllowed(client, ["a", "b", "c"]), + ).resolves.toBeUndefined(); + expect(list).not.toHaveBeenCalled(); + }); + + it("works with a client that cannot list vaults", async () => { + await expect(assertVaultIdAllowed({}, "anything")).resolves.toBeUndefined(); + await expect(assertVaultIdsAllowed({}, ["anything"])).resolves.toBeUndefined(); + }); + + it("treats a blank allow-list as no restriction", async () => { + setAllowList(" , ,"); + const { client, list } = makeClient(); + const vaults = [PROD, DEV]; + + await expect(assertVaultIdAllowed(client, "anything")).resolves.toBeUndefined(); + expect(list).not.toHaveBeenCalled(); + expect(filterAllowedVaults(vaults)).toBe(vaults); + }); + + it("returns the vaults from filterAllowedVaults untouched", () => { + const vaults = [PROD, DEV, PRIVATE]; + + expect(filterAllowedVaults(vaults)).toBe(vaults); + }); + }); + + describe("assertVaultIdAllowed", () => { + it("allows a vault whose title is in the allow-list", async () => { + setAllowList("Prod"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, PROD.id)).resolves.toBeUndefined(); + }); + + it("allows a vault whose ID is in the allow-list", async () => { + setAllowList(DEV.id); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, DEV.id)).resolves.toBeUndefined(); + }); + + it("rejects vaults that are not allow-listed, in the existing error style", async () => { + setAllowList("Prod"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, DEV.id)).rejects.toThrow( + `Vault '${DEV.id}' is not in the allowed vault list (Prod). ` + + "Configure OP_MCP_ALLOWED_VAULTS or --allowed-vaults to permit it.", + ); + }); + + it("lists every configured entry in the error", async () => { + setAllowList("Prod, vlt-dev-0002"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, PRIVATE.id)).rejects.toThrow( + /\(Prod, vlt-dev-0002\)/, + ); + }); + + it("compares allow-list entries, vault titles, and IDs case-insensitively", async () => { + setAllowList("pROD, VLT-DEV-0002"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, PROD.id)).resolves.toBeUndefined(); + await expect( + assertVaultIdAllowed(client, PROD.id.toUpperCase()), + ).resolves.toBeUndefined(); + await expect(assertVaultIdAllowed(client, DEV.id)).resolves.toBeUndefined(); + await expect(assertVaultIdAllowed(client, PRIVATE.id)).rejects.toThrow( + /not in the allowed vault list/, + ); + }); + + it("matches the legacy `name` property when a vault has no title", async () => { + setAllowList("Legacy"); + const { client } = makeClient([{ id: "vlt-legacy-0009", name: "Legacy" }]); + + await expect(assertVaultIdAllowed(client, "vlt-legacy-0009")).resolves.toBeUndefined(); + }); + + it("does not treat a vault title as a vault ID", async () => { + setAllowList("Prod"); + const { client } = makeClient(); + + // Items APIs take vault IDs; the title of an allowed vault is not an allowed ID. + await expect(assertVaultIdAllowed(client, "Prod")).rejects.toThrow( + /not in the allowed vault list/, + ); + }); + + it("rejects every vault when no visible vault matches the allow-list", async () => { + setAllowList("Nonexistent"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, PROD.id)).rejects.toThrow( + /not in the allowed vault list/, + ); + }); + + it("rejects an empty or non-string vault ID when restricted", async () => { + setAllowList("Prod"); + const { client } = makeClient(); + + await expect(assertVaultIdAllowed(client, "")).rejects.toThrow( + /not in the allowed vault list/, + ); + await expect( + assertVaultIdAllowed(client, undefined as unknown as string), + ).rejects.toThrow(/not in the allowed vault list/); + }); + + it("lists vaults once per check", async () => { + setAllowList("Prod"); + const { client, list } = makeClient(); + + await assertVaultIdAllowed(client, PROD.id); + + expect(list).toHaveBeenCalledTimes(1); + }); + + it("fails closed when the SDK cannot list vaults", async () => { + setAllowList("Prod"); + + await expect(assertVaultIdAllowed({}, PROD.id)).rejects.toThrow( + /does not support listing vaults/, + ); + await expect(assertVaultIdAllowed({ vaults: {} }, PROD.id)).rejects.toThrow( + /does not support listing vaults/, + ); + }); + + it("fails closed when listing vaults fails", async () => { + setAllowList("Prod"); + const client = { vaults: { list: vi.fn().mockRejectedValue(new Error("network down")) } }; + + await expect(assertVaultIdAllowed(client, PROD.id)).rejects.toThrow("network down"); + }); + + it("falls back to listAll on older SDKs", async () => { + setAllowList("Prod"); + const listAll = vi.fn().mockResolvedValue([PROD, DEV]); + + await expect( + assertVaultIdAllowed({ vaults: { listAll } }, PROD.id), + ).resolves.toBeUndefined(); + await expect( + assertVaultIdAllowed({ vaults: { listAll } }, DEV.id), + ).rejects.toThrow(/not in the allowed vault list/); + }); + }); + + describe("assertVaultIdsAllowed", () => { + it("allows several vaults with a single vaults.list call", async () => { + setAllowList("Prod, Dev"); + const { client, list } = makeClient(); + + await expect( + assertVaultIdsAllowed(client, [PROD.id, DEV.id, PROD.id]), + ).resolves.toBeUndefined(); + expect(list).toHaveBeenCalledTimes(1); + }); + + it("rejects when any one of the vaults is not allowed", async () => { + setAllowList("Prod, Dev"); + const { client, list } = makeClient(); + + await expect( + assertVaultIdsAllowed(client, [PROD.id, PRIVATE.id, DEV.id]), + ).rejects.toThrow(`Vault '${PRIVATE.id}' is not in the allowed vault list`); + expect(list).toHaveBeenCalledTimes(1); + }); + + it("makes no SDK call for an empty list of vault IDs", async () => { + setAllowList("Prod"); + const { client, list } = makeClient(); + + await expect(assertVaultIdsAllowed(client, [])).resolves.toBeUndefined(); + expect(list).not.toHaveBeenCalled(); + }); + + it("accepts the vault IDs of resolveAll-style responses", async () => { + setAllowList("Prod"); + const { client } = makeClient(); + const individualResponses = { + "op://Prod/a/b": { content: { secret: "x", itemId: "i1", vaultId: PROD.id } }, + "op://Prod/c/d": { content: { secret: "y", itemId: "i2", vaultId: DEV.id } }, + }; + const vaultIds = Object.values(individualResponses).map( + (response) => response.content.vaultId, + ); + + await expect(assertVaultIdsAllowed(client, vaultIds)).rejects.toThrow( + `Vault '${DEV.id}' is not in the allowed vault list`, + ); + }); + }); + + describe("filterAllowedVaults", () => { + it("keeps only allow-listed vaults, preserving order and objects", () => { + setAllowList("Private, vlt-prod-0001"); + const vaults = [DEV, PROD, PRIVATE]; + + const filtered = filterAllowedVaults(vaults); + + expect(filtered).toEqual([PROD, PRIVATE]); + expect(filtered[0]).toBe(PROD); + expect(filtered[1]).toBe(PRIVATE); + expect(vaults).toEqual([DEV, PROD, PRIVATE]); + }); + + it("matches each vault's own ID, title, and name case-insensitively", () => { + setAllowList("pROD, VLT-DEV-0002, legacy"); + const legacy = { id: "vlt-legacy-0009", name: "Legacy" }; + + expect(filterAllowedVaults([PROD, DEV, PRIVATE, legacy])).toEqual([PROD, DEV, legacy]); + }); + + it("returns an empty list when no vault is allow-listed", () => { + setAllowList("Nonexistent"); + + expect(filterAllowedVaults([PROD, DEV])).toEqual([]); + }); + + it("preserves extra properties of the input vaults", () => { + setAllowList("Prod"); + const rich = { ...PROD, description: "production", type: "USER_CREATED" }; + + expect(filterAllowedVaults([rich, DEV])).toEqual([rich]); + }); + + it("matches a vault that carries no title or name only by its ID", () => { + setAllowList("Prod"); + expect(filterAllowedVaults([{ id: PROD.id }])).toEqual([]); + + setAllowList(PROD.id); + expect(filterAllowedVaults([{ id: PROD.id }])).toEqual([{ id: PROD.id }]); + }); + + it("drops malformed entries instead of throwing when restricted", () => { + setAllowList("Prod"); + const malformed = [null, undefined, {}, PROD] as unknown as typeof PROD[]; + + expect(filterAllowedVaults(malformed)).toEqual([PROD]); + }); + + it("agrees with the ID checks on which vaults are allowed", async () => { + const vaults = [PROD, DEV, PRIVATE, { id: "vlt-legacy-0009", name: "Legacy" }]; + const { client } = makeClient(vaults); + + for (const allowList of [ + "Prod", + "vlt-dev-0002", + "prod, LEGACY", + "Private, Dev, nope", + "nope", + ]) { + setAllowList(allowList); + const assertedIds: string[] = []; + for (const vault of vaults) { + try { + await assertVaultIdAllowed(client, vault.id); + assertedIds.push(vault.id); + } catch { + // not allowed + } + } + + expect(filterAllowedVaults(vaults).map((vault) => vault.id), allowList).toEqual( + assertedIds, + ); + } + }); + }); +}); diff --git a/tests/vault-allowlist.test.ts b/tests/vault-allowlist.test.ts new file mode 100644 index 0000000..b88378b --- /dev/null +++ b/tests/vault-allowlist.test.ts @@ -0,0 +1,544 @@ +/** + * Tests that the server-wide vault allow-list (OP_MCP_ALLOWED_VAULTS) is + * enforced by every tool and resource that touches a vault — on the vault ID the + * SDK call would actually use, and before any SDK read or write happens. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { McpServer } from "@modelcontextprotocol/server"; + +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 { getClient } from "../src/client.js"; +import { resetConfig } from "../src/config.js"; +import { registerAllTools } from "../src/tools/index.js"; +import { registerAllResources } from "../src/resources/index.js"; + +const mockedGetClient = vi.mocked(getClient); + +const NOT_ALLOWED = "not in the allowed vault list"; + +const PROD = { id: "vlt-prod-0001", title: "Prod" }; +const STAGING = { id: "vlt-staging-0002", title: "Staging" }; + +type SdkMethod = "list" | "get" | "put" | "create" | "delete" | "archive"; + +/** Every tool that takes a vault ID, with the SDK call that does its real work. */ +const BY_ID_TOOLS: Array<{ + tool: string; + sdkMethod: SdkMethod; + args: (vaultId: string) => Record; +}> = [ + { tool: "item_list", sdkMethod: "list", args: (vaultId) => ({ vaultId }) }, + { tool: "item_lookup", sdkMethod: "list", args: (vaultId) => ({ vaultId }) }, + { + tool: "item_get", + sdkMethod: "get", + args: (vaultId) => ({ vaultId, itemId: "i1", reveal: true }), + }, + { + tool: "item_edit", + sdkMethod: "put", + args: (vaultId) => ({ vaultId, itemId: "i1", title: "Renamed" }), + }, + { tool: "item_delete", sdkMethod: "delete", args: (vaultId) => ({ vaultId, itemId: "i1" }) }, + { tool: "item_archive", sdkMethod: "archive", args: (vaultId) => ({ vaultId, itemId: "i1" }) }, + { + tool: "password_read", + sdkMethod: "get", + args: (vaultId) => ({ vaultId, itemId: "i1", reveal: true }), + }, + { + tool: "password_update", + sdkMethod: "put", + args: (vaultId) => ({ vaultId, itemId: "i1", newPassword: "new-pass" }), + }, + { + tool: "password_create", + sdkMethod: "create", + args: (vaultId) => ({ vaultId, title: "Created", password: "pw" }), + }, + { + tool: "note_create", + sdkMethod: "create", + args: (vaultId) => ({ vaultId, title: "Created", notes: "body" }), + }, +]; + +describe("vault allow-list enforcement", () => { + let server: McpServer; + let registeredTools: Map; + let registeredResources: 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(); + registeredResources = new Map(); + const originalTool = server.registerTool.bind(server); + vi.spyOn(server, "registerTool").mockImplementation(((...args: any[]) => { + const [name, config, handler] = args; + registeredTools.set(name, { + description: config.description, + schema: config.inputSchema, + handler, + }); + return originalTool(...(args as Parameters)); + }) as any); + const originalResource = server.registerResource.bind(server); + vi.spyOn(server, "registerResource").mockImplementation(((...args: any[]) => { + const [name, , , handler] = args; + registeredResources.set(name, handler); + return originalResource(...(args as Parameters)); + }) as any); + registerAllTools(server); + registerAllResources(server); + }); + + afterEach(() => { + Object.keys(process.env).forEach((key) => { + if (!(key in originalEnv)) delete process.env[key]; + else process.env[key] = originalEnv[key]; + }); + resetConfig(); + }); + + function setAllowList(value: string) { + process.env.OP_MCP_ALLOWED_VAULTS = value; + resetConfig(); + } + + /** A mock client where every SDK operation succeeds; installed as getClient()'s result. */ + function makeClient(vaults: unknown[] = [PROD, STAGING]) { + const timestamp = new Date("2024-01-01T00:00:00.000Z"); + const itemIn = (vaultId: string) => ({ + id: "i1", + title: "Item", + category: "Login", + vaultId, + tags: [], + notes: "", + sections: [], + websites: [], + fields: [{ id: "password", title: "password", fieldType: "Concealed", value: "pw-value" }], + version: 1, + files: [], + createdAt: timestamp, + updatedAt: timestamp, + }); + const client = { + vaults: { list: vi.fn().mockResolvedValue(vaults) }, + items: { + list: vi.fn().mockResolvedValue([ + { + id: "i1", + title: "Item", + category: "Login", + vaultId: PROD.id, + tags: [], + websites: [], + state: "active", + createdAt: timestamp, + updatedAt: timestamp, + }, + ]), + get: vi.fn().mockImplementation(async (vaultId: string) => itemIn(vaultId)), + put: vi.fn().mockImplementation(async (item: unknown) => item), + create: vi.fn().mockImplementation(async (params: any) => ({ + id: "new1", + title: params.title, + vaultId: params.vaultId, + category: params.category, + tags: [], + fields: [], + })), + delete: vi.fn().mockResolvedValue(undefined), + archive: vi.fn().mockResolvedValue(undefined), + }, + secrets: { + resolve: vi.fn(), + resolveAll: vi.fn(), + }, + }; + mockedGetClient.mockResolvedValue(client as any); + return client; + } + + /** Assert that no item or secret SDK operation ran. */ + function expectNoSdkOperations(client: ReturnType) { + for (const fn of [ + ...Object.values(client.items), + client.secrets.resolve, + client.secrets.resolveAll, + ]) { + expect(fn).not.toHaveBeenCalled(); + } + } + + function call(tool: string, args: Record) { + return registeredTools.get(tool)!.handler(args); + } + + describe.each(BY_ID_TOOLS)("$tool", ({ tool, sdkMethod, args }) => { + it("rejects a vault outside the allow-list before any SDK operation", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const result = await call(tool, args(STAGING.id)); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain(NOT_ALLOWED); + expect(result.content[0].text).toContain(STAGING.id); + expectNoSdkOperations(client); + }); + + it("rejects a vault ID that matches no vault at all", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const result = await call(tool, args("vlt-unknown-9999")); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain(NOT_ALLOWED); + expectNoSdkOperations(client); + }); + + it.each([ + ["by name", "Prod"], + ["by name, in a different case", "pROD"], + ["by ID", PROD.id], + ["by ID, in a different case", PROD.id.toUpperCase()], + ["among several entries", "Archive, Prod , Other"], + ])("allows an allow-listed vault (%s)", async (_label, allowList) => { + setAllowList(allowList); + const client = makeClient(); + + const result = await call(tool, args(PROD.id)); + + expect(result.isError).toBeUndefined(); + expect(client.items[sdkMethod]).toHaveBeenCalledTimes(1); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("fails closed when the vaults cannot be listed", async () => { + setAllowList("Prod"); + const client = makeClient(); + client.vaults.list.mockRejectedValue(new Error("vault listing unavailable")); + + const result = await call(tool, args(PROD.id)); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("vault listing unavailable"); + expectNoSdkOperations(client); + }); + + it("does not list vaults when no allow-list is configured", async () => { + const client = makeClient(); + + const result = await call(tool, args(STAGING.id)); + + expect(result.isError).toBeUndefined(); + expect(client.items[sdkMethod]).toHaveBeenCalledTimes(1); + expect(client.vaults.list).not.toHaveBeenCalled(); + }); + }); + + describe("secret references (item_get, password_read)", () => { + const refTools = ["item_get", "password_read"]; + + /** Make resolveAll resolve `reference` to a secret in `vaultId`. */ + function resolveTo( + client: ReturnType, + reference: string, + vaultId: string, + ) { + client.secrets.resolveAll.mockResolvedValue({ + individualResponses: { + [reference]: { content: { secret: "ref-secret-value", itemId: "i1", vaultId } }, + }, + }); + } + + describe.each(refTools)("%s", (tool) => { + it("rejects a reference naming a vault outside the allow-list before resolving", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const result = await call(tool, { + secretReference: "op://Staging/db/password", + reveal: true, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain(NOT_ALLOWED); + expectNoSdkOperations(client); + expect(client.vaults.list).not.toHaveBeenCalled(); + expect(mockedGetClient).not.toHaveBeenCalled(); + }); + + it("rejects when an allow-listed reference resolves to a vault outside the allowed set", async () => { + setAllowList("Prod"); + const client = makeClient(); + resolveTo(client, "op://Prod/db/password", STAGING.id); + + const result = await call(tool, { + secretReference: "op://Prod/db/password", + reveal: true, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain(NOT_ALLOWED); + expect(result.content[0].text).toContain(STAGING.id); + expect(result.content[0].text).not.toContain("ref-secret-value"); + expect(client.secrets.resolveAll).toHaveBeenCalledWith(["op://Prod/db/password"]); + expect(client.items.get).not.toHaveBeenCalled(); + }); + + it.each([ + ["by name", "Prod", "op://Prod/db/password"], + ["by ID", PROD.id, `op://${PROD.id}/db/password`], + ["by name, in a different case", "prod", "op://PROD/db/password"], + ])( + "resolves a reference to an allow-listed vault (%s)", + async (_label, allowList, reference) => { + setAllowList(allowList); + const client = makeClient(); + resolveTo(client, reference, PROD.id); + + const result = await call(tool, { secretReference: reference, reveal: true }); + + expect(result.isError).toBeUndefined(); + const data = JSON.parse(result.content[0].text); + if (tool === "password_read") { + expect(data).toEqual({ value: "ref-secret-value" }); + } else { + expect(client.items.get).toHaveBeenCalledWith(PROD.id, "i1"); + expect(data.vaultId).toBe(PROD.id); + } + }, + ); + + it("does not list vaults when no allow-list is configured", async () => { + const client = makeClient(); + resolveTo(client, "op://Staging/db/password", STAGING.id); + + const result = await call(tool, { secretReference: "op://Staging/db/password" }); + + expect(result.isError).toBeUndefined(); + expect(client.vaults.list).not.toHaveBeenCalled(); + }); + + it("ignores vaultId/itemId when a secretReference is given", async () => { + setAllowList("Prod"); + const client = makeClient(); + resolveTo(client, "op://Prod/db/password", PROD.id); + + const result = await call(tool, { + secretReference: "op://Prod/db/password", + vaultId: STAGING.id, + itemId: "i9", + }); + + expect(result.isError).toBeUndefined(); + }); + }); + + it("password_read never calls secrets.resolve, so the vault can always be checked", async () => { + const client = makeClient(); + resolveTo(client, "op://Prod/db/password", PROD.id); + + await call("password_read", { secretReference: "op://Prod/db/password" }); + + expect(client.secrets.resolve).not.toHaveBeenCalled(); + expect(client.secrets.resolveAll).toHaveBeenCalledWith(["op://Prod/db/password"]); + }); + }); + + describe("vault_list", () => { + it("returns only the allow-listed vaults (by name) with a single vaults.list call", async () => { + setAllowList("Prod"); + const client = makeClient([ + { ...PROD, description: "production" }, + { ...STAGING, description: "staging" }, + ]); + + const result = await call("vault_list", {}); + const data = JSON.parse(result.content[0].text); + + expect(data.vaults.map((v: any) => v.id)).toEqual([PROD.id]); + expect(data.vaults[0].name).toBe("Prod"); + expect(data.vaults[0].description).toBe("production"); + expect(result.content[0].text).not.toContain(STAGING.id); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("returns only the allow-listed vaults (by ID, case-insensitive) with a single vaults.list call", async () => { + setAllowList(STAGING.id.toUpperCase()); + const client = makeClient(); + + const result = await call("vault_list", {}); + const data = JSON.parse(result.content[0].text); + + expect(data.vaults.map((v: any) => v.id)).toEqual([STAGING.id]); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("returns an empty list when no vault matches the allow-list", async () => { + setAllowList("Nonexistent"); + makeClient(); + + const result = await call("vault_list", {}); + + expect(JSON.parse(result.content[0].text)).toEqual({ vaults: [] }); + }); + + it("surfaces a vaults.list failure instead of listing anything", async () => { + setAllowList("Prod"); + const client = makeClient(); + client.vaults.list.mockRejectedValue(new Error("vault listing unavailable")); + + const result = await call("vault_list", {}); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("vault listing unavailable"); + }); + + it("returns every vault with a single vaults.list call when unrestricted", async () => { + const client = makeClient(); + + const result = await call("vault_list", {}); + const data = JSON.parse(result.content[0].text); + + expect(data.vaults.map((v: any) => v.id)).toEqual([PROD.id, STAGING.id]); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + }); + + describe("1password://vaults resource", () => { + async function readVaults() { + const result = await registeredResources.get("vault-list")(); + return JSON.parse(result.contents[0].text); + } + + it("returns only the allow-listed vaults (by name) with a single vaults.list call", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const data = await readVaults(); + + expect(data.vaults.map((v: any) => v.id)).toEqual([PROD.id]); + expect(JSON.stringify(data)).not.toContain(STAGING.id); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("returns only the allow-listed vaults (by ID, case-insensitive) with a single vaults.list call", async () => { + setAllowList(STAGING.id.toUpperCase()); + const client = makeClient(); + + const data = await readVaults(); + + expect(data.vaults.map((v: any) => v.id)).toEqual([STAGING.id]); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("returns an empty list when no vault matches the allow-list", async () => { + setAllowList("Nonexistent"); + makeClient(); + + const data = await readVaults(); + + expect(data).toEqual({ vaults: [] }); + }); + + it("reports a vaults.list failure instead of listing anything", async () => { + setAllowList("Prod"); + const client = makeClient(); + client.vaults.list.mockRejectedValue(new Error("vault listing unavailable")); + + const data = await readVaults(); + + expect(data.error).toContain("vault listing unavailable"); + expect(data.vaults).toBeUndefined(); + }); + + it("returns every vault with a single vaults.list call when unrestricted", async () => { + const client = makeClient(); + + const data = await readVaults(); + + expect(data.vaults.map((v: any) => v.id)).toEqual([PROD.id, STAGING.id]); + expect(client.vaults.list).toHaveBeenCalledTimes(1); + }); + }); + + describe("1password://vaults/{vaultId}/items resource", () => { + async function readItems(vaultId: string) { + // Passed as a string: the handler accepts string or URL, and a URL object + // cannot be built for the `1password:` scheme (a scheme can't start with a digit). + const result = await registeredResources.get("vault-items")( + `1password://vaults/${vaultId}/items`, + ); + return JSON.parse(result.contents[0].text); + } + + it("rejects a vault outside the allow-list before listing items", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const data = await readItems(STAGING.id); + + expect(data.error).toContain(NOT_ALLOWED); + expect(data.items).toBeUndefined(); + expect(client.items.list).not.toHaveBeenCalled(); + }); + + it.each([ + ["by name", "Prod"], + ["by ID", PROD.id], + ])("lists items of an allow-listed vault (%s)", async (_label, allowList) => { + setAllowList(allowList); + const client = makeClient(); + + const data = await readItems(PROD.id); + + expect(data.error).toBeUndefined(); + expect(data.vaultId).toBe(PROD.id); + expect(data.count).toBe(1); + expect(client.items.list).toHaveBeenCalledWith(PROD.id); + }); + + it("does not list vaults when no allow-list is configured", async () => { + const client = makeClient(); + + const data = await readItems(STAGING.id); + + expect(data.error).toBeUndefined(); + expect(client.items.list).toHaveBeenCalledWith(STAGING.id); + expect(client.vaults.list).not.toHaveBeenCalled(); + }); + }); + + describe("tools that never touch a vault", () => { + it("password_generate works with an allow-list and no vault listing", async () => { + setAllowList("Prod"); + const client = makeClient(); + + const result = await call("password_generate", {}); + + expect(result.isError).toBeUndefined(); + expect(client.vaults.list).not.toHaveBeenCalled(); + }); + }); +}); From 4cb988b3453740930bb3c914556a7feaf1a2eff1 Mon Sep 17 00:00:00 2001 From: CakeRepository Date: Sun, 4 Oct 2026 04:54:41 -0500 Subject: [PATCH 2/4] fix(resources): make resources readable with onepassword:// URIs (v5.0.0) Every resources/read failed with -32602 "Resource URI ... is invalid": the SDK parses the URI with new URL() before dispatching, and a scheme cannot start with a digit (RFC 3986 3.1), so no 1password:// URI ever reached a handler. - Resource URIs now use the onepassword:// scheme. Breaking for anything that hard-coded the old URIs, hence 5.0.0. - onepassword://vaults/{vaultId}/items is a ResourceTemplate and reads vaultId from the percent-decoded template variables. It was a static resource whose URI was the literal template string, so no concrete vault URI could match it. - buildServer() moved to src/server.ts (re-exported from index.ts) so tests can build the real server without starting stdio. - tests/resources.e2e.test.ts: a real MCP client reads every advertised resource from serveStdio(() => buildServer()) over an in-memory transport, in both the 2025 and 2026-07-28 protocol eras. Adds the @modelcontextprotocol/client dev dependency. - README, agents.md, CONTRIBUTING, and CHANGELOG updated. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 18 ++++ CONTRIBUTING.md | 4 +- README.md | 15 +-- agents.md | 2 +- package-lock.json | 138 ++++++++++++++++++++++++- package.json | 3 +- server.json | 4 +- src/config.ts | 2 +- src/index.ts | 18 +--- src/resources/index.ts | 75 +++++++++----- src/server.ts | 25 +++++ tests/resources.e2e.test.ts | 201 ++++++++++++++++++++++++++++++++++++ 12 files changed, 447 insertions(+), 58 deletions(-) create mode 100644 src/server.ts create mode 100644 tests/resources.e2e.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2176818..97a01cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to this project will be documented in this file. 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). +## [5.0.0] - 2026-10-04 + +Major release because the resource URIs changed (see **Breaking** below). Tools, prompts, configuration, and the Node.js requirement are unchanged. + +### Changed + +- **Breaking: resource URIs use the `onepassword://` scheme** — `1password://config` → `onepassword://config`, `1password://vaults` → `onepassword://vaults`, and `1password://vaults/{vaultId}/items` → `onepassword://vaults/{vaultId}/items`. Replace any hard-coded `1password://` URIs. The old URIs could never be read (see **Fixed**), so this only affects code or configuration that hard-codes them. They were still published identifiers with no possible alias, because the SDK rejects them before the server sees the request; hence the major version. +- `buildServer()` moved to `src/server.ts` (still re-exported from the entrypoint), so tests can build the real server without starting stdio. + +### Fixed + +- **Resources are now readable by MCP clients** — Every `resources/read` failed with `-32602 Resource URI … is invalid`. The SDK parses the URI with WHATWG `new URL()` before dispatching, and a URI scheme must start with a letter (RFC 3986 §3.1), so no `1password://` URI ever reached the server's handlers. +- **Per-vault items is a real resource template** — `onepassword://vaults/{vaultId}/items` is registered as a `ResourceTemplate` (listed by `resources/templates/list`) and reads `vaultId` from the template variables, percent-decoded. It used to be a static resource whose URI was the literal template string, so no concrete vault URI could ever match it. + +### Added + +- **End-to-end resource tests** — A real MCP client reads the resources from `serveStdio(() => buildServer())` over an in-memory transport, in both the 2025 (`initialize`) and 2026-07-28 protocol eras, so an advertised URI that the SDK cannot parse or route fails CI. Adds the `@modelcontextprotocol/client` dev dependency (tests only). + ## [4.0.3] - 2026-10-04 ### Security diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ac6f12d..de4b7c4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,6 +35,7 @@ You do not need a live service account token for unit tests. For manual MCP smok ``` src/ ├── index.ts # Server entrypoint (stdio + MCP negotiation) +├── server.ts # buildServer(): registers tools, prompts, resources ├── types.ts # Shared types ├── logger.ts # Structured logging to stderr ├── config.ts # CLI args, env vars, Keychain, allow-list @@ -69,7 +70,8 @@ tests/ ├── prompts.test.ts ├── secret-ref.test.ts ├── op-run.test.ts -└── op-check-ref.test.ts +├── op-check-ref.test.ts +└── resources.e2e.test.ts # Real MCP client ↔ server over an in-memory transport ``` Version must stay aligned across `package.json`, `server.json`, and `SERVER_VERSION` in `src/config.ts`. See [AGENTS.md](AGENTS.md). diff --git a/README.md b/README.md index 230a956..ead6d4e 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ Built on the **MCP TypeScript SDK v2** with protocol negotiation for **[2026-07- - **`op_run` (the MCP equivalent of `op run`)** — inject `op://vault/item/field` into a local command’s environment; plaintext is redacted from stdout/stderr and never logged back to the model. - **Full vault toolkit** — list, search, get, edit, create logins & notes, rotate passwords, archive, or delete. - **Guided prompts** — password generation, credential rotation, vault audit, and secret-reference helpers. -- **Browsable resources** — vault and item catalogs over `1password://…` URIs (no secrets in resource payloads). +- **Browsable resources** — vault and item catalogs over `onepassword://…` URIs (no secrets in resource payloads). - **Modern MCP** — stdio transport, Zod 4 schemas, MCP 2026-07-28 negotiation with legacy client compatibility. --- @@ -96,9 +96,11 @@ Grouped the way agents and humans actually use them. | URI | Contents | |-----|----------| -| `1password://config` | Non-secret server config (name, version, log level, token source, Node version). | -| `1password://vaults` | JSON list of accessible vaults. | -| `1password://vaults/{vaultId}/items` | JSON item metadata for one vault (no secret values). | +| `onepassword://config` | Non-secret server config (name, version, log level, token source, Node version). | +| `onepassword://vaults` | JSON list of accessible vaults. | +| `onepassword://vaults/{vaultId}/items` | URI template (listed by `resources/templates/list`): JSON item metadata for one vault (no secret values). | + +> **Upgrading from 4.x:** resource URIs used to start with `1password://`, which MCP clients could never read (a URI scheme can't start with a digit). Replace any hard-coded `1password://` URIs with `onepassword://`. --- @@ -305,6 +307,7 @@ Watch mode: `npm run dev`. ``` src/ index.ts # Entrypoint — MCP stdio + protocol negotiation + server.ts # buildServer() — registers tools, prompts, resources config.ts # CLI / env / Keychain / allow-list client.ts # 1Password SDK client logger.ts # Structured logs on stderr (stdout is protocol) @@ -312,7 +315,7 @@ src/ utils.ts # Result helpers, password generation tools/ # All 15 MCP tools prompts/ # Interactive workflow prompts - resources/ # 1password:// resources + resources/ # onepassword:// resources tests/ ``` @@ -322,7 +325,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md). Maintainers / agents: [AGENTS.md](AGENTS ## Changelog -See [CHANGELOG.md](CHANGELOG.md) for version history, including the **4.0.0** MCP v2 / 2026-07-28 migration and the **3.0.0** `op_run` / reveal-opt-in security changes. +See [CHANGELOG.md](CHANGELOG.md) for version history, including the **5.0.0** resource URI change (`1password://` → `onepassword://`), the **4.0.0** MCP v2 / 2026-07-28 migration, and the **3.0.0** `op_run` / reveal-opt-in security changes. --- diff --git a/agents.md b/agents.md index 0b7df26..f6c845c 100644 --- a/agents.md +++ b/agents.md @@ -73,7 +73,7 @@ When tools, prompts, or resources change, update **README.md** (npm’s face), * - **15 tools:** `vault_list`, `item_lookup`, `item_list`, `item_get`, `item_edit`, `item_delete`, `item_archive`, `note_create`, `password_create`, `password_read`, `password_update`, `password_generate`, `password_generate_memorable`, `op_run`, `op_check_ref` - **4 prompts:** `generate-secure-password`, `credential-rotation`, `vault-audit`, `secret-reference-helper` -- **3 resources:** `1password://config`, `1password://vaults`, `1password://vaults/{vaultId}/items` +- **3 resources:** `onepassword://config`, `onepassword://vaults`, and the `ResourceTemplate` `onepassword://vaults/{vaultId}/items`. The SDK parses every read URI with `new URL()`, so a scheme must start with a letter (never `1password://`), and parameterized URIs must be registered as a `ResourceTemplate`. `tests/resources.e2e.test.ts` reads every advertised resource through a real client to catch both mistakes. - **Protocol:** MCP SDK v2, stdio negotiation for **2026-07-28** + legacy clients ## Agent security conventions diff --git a/package-lock.json b/package-lock.json index 9f68653..1bd6449 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "5.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "5.0.0", "license": "Apache-2.0", "dependencies": { "@1password/sdk": "^0.3.1", @@ -17,6 +17,7 @@ "1password-mcp": "bin/1password-mcp.js" }, "devDependencies": { + "@modelcontextprotocol/client": "^2.0.0", "@types/node": "^22.0.0", "typescript": "^5.7.0", "vitest": "^4.1.11" @@ -47,6 +48,25 @@ "dev": true, "license": "MIT" }, + "node_modules/@modelcontextprotocol/client": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@modelcontextprotocol/client/-/client-2.0.0.tgz", + "integrity": "sha512-8f1OghQ2rjzIOfqgUCP+8GiUWqRs89njoWLNqAe8kWmDePv3s1fZXseej+QXemssEuuOvLLmLO/kqM3IQHtISw==", + "dev": true, + "license": "MIT", + "dependencies": { + "@modelcontextprotocol/core": "2.0.0", + "cross-spawn": "^7.0.5", + "eventsource": "^3.0.2", + "eventsource-parser": "^3.0.0", + "jose": "^6.1.3", + "pkce-challenge": "^5.0.0", + "zod": "^4.2.0" + }, + "engines": { + "node": ">=20" + } + }, "node_modules/@modelcontextprotocol/core": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/@modelcontextprotocol/core/-/core-2.0.0.tgz", @@ -544,6 +564,21 @@ "dev": true, "license": "MIT" }, + "node_modules/cross-spawn": { + "version": "7.0.6", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", + "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", + "dev": true, + "license": "MIT", + "dependencies": { + "path-key": "^3.1.0", + "shebang-command": "^2.0.0", + "which": "^2.0.1" + }, + "engines": { + "node": ">= 8" + } + }, "node_modules/detect-libc": { "version": "2.1.2", "resolved": "https://registry.npmjs.org/detect-libc/-/detect-libc-2.1.2.tgz", @@ -571,6 +606,29 @@ "@types/estree": "^1.0.0" } }, + "node_modules/eventsource": { + "version": "3.0.7", + "resolved": "https://registry.npmjs.org/eventsource/-/eventsource-3.0.7.tgz", + "integrity": "sha512-CRT1WTyuQoD771GW56XEZFQ/ZoSfWid1alKGDYMmkt2yl8UXrVR4pspqWNEcqKvVIzg6PAltWjxcSSPrboA4iA==", + "dev": true, + "license": "MIT", + "dependencies": { + "eventsource-parser": "^3.0.1" + }, + "engines": { + "node": ">=18.0.0" + } + }, + "node_modules/eventsource-parser": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/eventsource-parser/-/eventsource-parser-3.1.1.tgz", + "integrity": "sha512-EKN1vKAMcZ8MlYMpaNuxN6R9yakzH6uajHcHVTqWJzvu5pWw9DyhbP35HH8MVBQ+dZjAfDxk+A8NiR9KWaXiyQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=18.0.0" + } + }, "node_modules/expect-type": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/expect-type/-/expect-type-1.3.0.tgz", @@ -614,6 +672,23 @@ "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } }, + "node_modules/isexe": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", + "integrity": "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==", + "dev": true, + "license": "ISC" + }, + "node_modules/jose": { + "version": "6.2.12", + "resolved": "https://registry.npmjs.org/jose/-/jose-6.2.12.tgz", + "integrity": "sha512-9NiFmJEex0sy2Dk58j2UGBSHgUs2ypF9eZSu4L6vjOX3Dp96Sw1F3uL+H+D1sx02jZZdzUT0HgvCy59CuvXcWw==", + "dev": true, + "license": "MIT", + "funding": { + "url": "https://github.com/sponsors/panva" + } + }, "node_modules/lightningcss": { "version": "1.33.0", "resolved": "https://registry.npmjs.org/lightningcss/-/lightningcss-1.33.0.tgz", @@ -930,6 +1005,16 @@ "node": ">=12.20.0" } }, + "node_modules/path-key": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/path-key/-/path-key-3.1.1.tgz", + "integrity": "sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/pathe": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/pathe/-/pathe-2.0.3.tgz", @@ -957,6 +1042,16 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/pkce-challenge": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/pkce-challenge/-/pkce-challenge-5.0.1.tgz", + "integrity": "sha512-wQ0b/W4Fr01qtpHlqSqspcj3EhBvimsdh0KlHhH8HRZnMsEa0ea2fTULOXOS9ccQr3om+GcGRk4e+isrZWV8qQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=16.20.0" + } + }, "node_modules/postcss": { "version": "8.5.28", "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.28.tgz", @@ -1020,6 +1115,29 @@ "@rolldown/binding-win32-x64-msvc": "1.2.12" } }, + "node_modules/shebang-command": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", + "integrity": "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==", + "dev": true, + "license": "MIT", + "dependencies": { + "shebang-regex": "^3.0.0" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/shebang-regex": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-3.0.0.tgz", + "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/siginfo": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/siginfo/-/siginfo-2.0.0.tgz", @@ -1284,6 +1402,22 @@ } } }, + "node_modules/which": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/which/-/which-2.0.2.tgz", + "integrity": "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==", + "dev": true, + "license": "ISC", + "dependencies": { + "isexe": "^2.0.0" + }, + "bin": { + "node-which": "bin/node-which" + }, + "engines": { + "node": ">= 8" + } + }, "node_modules/why-is-node-running": { "version": "2.3.0", "resolved": "https://registry.npmjs.org/why-is-node-running/-/why-is-node-running-2.3.0.tgz", diff --git a/package.json b/package.json index 71dd906..419e882 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "5.0.0", "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)", @@ -61,6 +61,7 @@ "@modelcontextprotocol/server": "^2.0.0-beta.5" }, "devDependencies": { + "@modelcontextprotocol/client": "^2.0.0", "@types/node": "^22.0.0", "typescript": "^5.7.0", "vitest": "^4.1.11" diff --git a/server.json b/server.json index 227d9ad..37b0cda 100644 --- a/server.json +++ b/server.json @@ -6,12 +6,12 @@ "url": "https://github.com/CakeRepository/1Password-MCP.git", "source": "github" }, - "version": "4.0.3", + "version": "5.0.0", "packages": [ { "registryType": "npm", "identifier": "@takescake/1password-mcp", - "version": "4.0.3", + "version": "5.0.0", "transport": { "type": "stdio" }, diff --git a/src/config.ts b/src/config.ts index b44df0f..147abad 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.3"; +export const SERVER_VERSION = "5.0.0"; /** Parse a `--flag value` or `--flag=value` argument from process.argv. */ function getArgValue(name: string): string | undefined { diff --git a/src/index.ts b/src/index.ts index b7f86aa..f322484 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,26 +6,12 @@ * 2025-era connection. */ -import { McpServer } from "@modelcontextprotocol/server"; import { serveStdio } from "@modelcontextprotocol/server/stdio"; import { SERVER_NAME, SERVER_VERSION, getConfig } from "./config.js"; import { log, logError } from "./logger.js"; -import { registerAllTools } from "./tools/index.js"; -import { registerAllPrompts } from "./prompts/index.js"; -import { registerAllResources } from "./resources/index.js"; +import { buildServer } from "./server.js"; -export function buildServer(): McpServer { - const server = new McpServer({ - name: SERVER_NAME, - version: SERVER_VERSION, - }); - - registerAllTools(server); - registerAllPrompts(server); - registerAllResources(server); - - return server; -} +export { buildServer }; process.on("uncaughtException", (error) => { logError("Uncaught exception.", error); diff --git a/src/resources/index.ts b/src/resources/index.ts index 2d6e98d..8339b8b 100644 --- a/src/resources/index.ts +++ b/src/resources/index.ts @@ -1,29 +1,54 @@ /** * MCP Resources — browsable data exposed by the 1Password MCP server. + * + * URIs use the `onepassword:` scheme. The SDK parses every `resources/read` + * URI with WHATWG `new URL()` before dispatching, and a scheme must start with + * a letter (RFC 3986 §3.1), so a `1password:` URI could never be read. */ -import type { McpServer } from "@modelcontextprotocol/server"; +import { + ResourceTemplate, + type McpServer, + type Variables, +} from "@modelcontextprotocol/server"; import { getClient } from "../client.js"; import { getConfig, SERVER_NAME, SERVER_VERSION } from "../config.js"; import { log, logError } from "../logger.js"; +/** + * Read the `vaultId` template variable. The SDK hands over the matched URI + * segment still percent-encoded, while RFC 6570 expansion (including the + * SDK's own `UriTemplate.expand`) percent-encodes the value, so decode it. + */ +function readVaultId(variables: Variables): string { + const raw = variables.vaultId; + if (typeof raw !== "string") { + throw new Error("Invalid resource URI: could not extract vaultId."); + } + try { + return decodeURIComponent(raw); + } catch { + throw new Error("Invalid resource URI: vaultId is not valid percent-encoding."); + } +} + /** Register all MCP resources on the server. */ export function registerAllResources(server: McpServer): void { - // ─── 1password://config ─────────────────────────────────────────── + // ─── onepassword://config ───────────────────────────────────────── server.registerResource( "server-config", - "1password://config", + "onepassword://config", { description: "Current 1Password MCP server configuration (non-secret values only).", mimeType: "application/json", }, - async () => { + async (uri) => { const config = getConfig(); return { contents: [ { - uri: "1password://config", + uri: uri.href, mimeType: "application/json", text: JSON.stringify( { @@ -44,17 +69,17 @@ export function registerAllResources(server: McpServer): void { }, ); - // ─── 1password://vaults ─────────────────────────────────────────── + // ─── onepassword://vaults ───────────────────────────────────────── server.registerResource( "vault-list", - "1password://vaults", + "onepassword://vaults", { description: "List of all 1Password vaults accessible to the service account.", mimeType: "application/json", }, - async () => { + async (uri) => { try { const client = await getClient(); const listFn = @@ -72,20 +97,20 @@ export function registerAllResources(server: McpServer): void { return { contents: [ { - uri: "1password://vaults", + uri: uri.href, mimeType: "application/json", text: JSON.stringify({ vaults: summary }, null, 2), }, ], }; } catch (error) { - logError("Resource 1password://vaults failed.", error); + logError("Resource onepassword://vaults failed.", error); const message = error instanceof Error ? error.message : String(error); return { contents: [ { - uri: "1password://vaults", + uri: uri.href, mimeType: "application/json", text: JSON.stringify({ error: message }), }, @@ -95,29 +120,24 @@ export function registerAllResources(server: McpServer): void { }, ); - // ─── 1password://vaults/{vaultId}/items ─────────────────────────── + // ─── onepassword://vaults/{vaultId}/items ───────────────────────── server.registerResource( "vault-items", - "1password://vaults/{vaultId}/items", + new ResourceTemplate("onepassword://vaults/{vaultId}/items", { + // No per-vault entries in resources/list: that would call the + // 1Password API on every listing. Clients discover the template via + // resources/templates/list. + list: undefined, + }), { description: "List of items within a specific 1Password vault (metadata only, no secrets).", mimeType: "application/json", }, - async (uri) => { + async (uri, variables) => { try { - // Extract vaultId from the URI - const uriStr = typeof uri === "string" ? uri : uri.href; - const match = uriStr.match( - /1password:\/\/vaults\/([^/]+)\/items/, - ); - const vaultId = match?.[1]; - if (!vaultId) { - throw new Error( - "Invalid resource URI: could not extract vaultId.", - ); - } + const vaultId = readVaultId(variables); log("debug", "Resource: vault-items.", { vaultId }); const client = await getClient(); @@ -137,7 +157,7 @@ export function registerAllResources(server: McpServer): void { return { contents: [ { - uri: uriStr, + uri: uri.href, mimeType: "application/json", text: JSON.stringify( { vaultId, items: summary, count: summary.length }, @@ -151,11 +171,10 @@ export function registerAllResources(server: McpServer): void { logError("Resource vault-items failed.", error); const message = error instanceof Error ? error.message : String(error); - const uriStr = typeof uri === "string" ? uri : uri.href; return { contents: [ { - uri: uriStr, + uri: uri.href, mimeType: "application/json", text: JSON.stringify({ error: message }), }, diff --git a/src/server.ts b/src/server.ts new file mode 100644 index 0000000..61b4955 --- /dev/null +++ b/src/server.ts @@ -0,0 +1,25 @@ +/** + * MCP server factory — builds one fully registered server instance. + * + * Kept apart from the stdio entrypoint (`index.ts`) so tests can build the + * real server without starting a stdio connection. + */ + +import { McpServer } from "@modelcontextprotocol/server"; +import { SERVER_NAME, SERVER_VERSION } from "./config.js"; +import { registerAllTools } from "./tools/index.js"; +import { registerAllPrompts } from "./prompts/index.js"; +import { registerAllResources } from "./resources/index.js"; + +export function buildServer(): McpServer { + const server = new McpServer({ + name: SERVER_NAME, + version: SERVER_VERSION, + }); + + registerAllTools(server); + registerAllPrompts(server); + registerAllResources(server); + + return server; +} diff --git a/tests/resources.e2e.test.ts b/tests/resources.e2e.test.ts new file mode 100644 index 0000000..b3de128 --- /dev/null +++ b/tests/resources.e2e.test.ts @@ -0,0 +1,201 @@ +/** + * End-to-end tests for the MCP resources. + * + * A real MCP client talks to the production server composition + * (`serveStdio(() => buildServer())`) over an in-memory transport pair, so + * every request goes through the SDK's own `resources/list`, + * `resources/templates/list`, and `resources/read` handling — URI parsing, + * static lookup, and template matching — instead of calling our callbacks + * directly. Only the 1Password SDK client is mocked. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + Client, + ProtocolErrorCode, + UriTemplate, + type ClientOptions, +} from "@modelcontextprotocol/client"; +import { InMemoryTransport } from "@modelcontextprotocol/server"; +import { serveStdio } from "@modelcontextprotocol/server/stdio"; + +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 { getClient } from "../src/client.js"; +import { resetConfig, SERVER_NAME, SERVER_VERSION } from "../src/config.js"; +import { buildServer } from "../src/server.js"; + +const mockedGetClient = vi.mocked(getClient); + +const PROD = { id: "vlt-prod-0001", title: "Prod", description: "Production", type: "USER_CREATED" }; +const CI = { id: "vlt-ci-0002", title: "CI", type: "USER_CREATED" }; +const DEPLOY_KEY = { id: "itm-0001", title: "Deploy key", category: "SshKey", vaultId: PROD.id }; + +const ERAS: { era: "legacy" | "modern"; options: ClientOptions }[] = [ + { era: "legacy", options: {} }, + { era: "modern", options: { versionNegotiation: { mode: { pin: "2026-07-28" } } } }, +]; + +describe.each(ERAS)("MCP resources end-to-end ($era protocol era)", ({ era, options }) => { + const originalEnv = { ...process.env }; + let client: Client; + let closeServer: () => Promise; + let opClient: { + vaults: { list: ReturnType }; + items: { list: ReturnType }; + }; + + beforeEach(async () => { + vi.clearAllMocks(); + opClient = { + vaults: { list: vi.fn().mockResolvedValue([PROD, CI]) }, + items: { list: vi.fn().mockResolvedValue([DEPLOY_KEY]) }, + }; + mockedGetClient.mockResolvedValue(opClient as any); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const handle = serveStdio(() => buildServer(), { transport: serverTransport }); + closeServer = () => handle.close(); + + client = new Client({ name: "resources-e2e", version: "0.0.0" }, options); + await client.connect(clientTransport); + }); + + afterEach(async () => { + await client.close(); + await closeServer(); + process.env = { ...originalEnv }; + resetConfig(); + }); + + async function readJson(uri: string) { + const result = await client.readResource({ uri }); + expect(result.contents).toHaveLength(1); + const [content] = result.contents; + expect(content.uri).toBe(uri); + expect(content.mimeType).toBe("application/json"); + return JSON.parse((content as { text: string }).text); + } + + it(`negotiates the ${era} protocol era`, () => { + expect(client.getProtocolEra()).toBe(era); + }); + + it("lists the static resources under onepassword:// URIs", async () => { + const { resources } = await client.listResources(); + + expect(resources.map((r) => r.uri).sort()).toEqual([ + "onepassword://config", + "onepassword://vaults", + ]); + }); + + it("lists the per-vault items resource as a URI template", async () => { + const { resourceTemplates } = await client.listResourceTemplates(); + + expect(resourceTemplates).toEqual([ + expect.objectContaining({ + name: "vault-items", + uriTemplate: "onepassword://vaults/{vaultId}/items", + mimeType: "application/json", + }), + ]); + }); + + it("can read every advertised resource and template", async () => { + const { resources } = await client.listResources(); + const { resourceTemplates } = await client.listResourceTemplates(); + const uris = [ + ...resources.map((r) => r.uri), + ...resourceTemplates.map((t) => { + const template = new UriTemplate(t.uriTemplate); + return template.expand( + Object.fromEntries(template.variableNames.map((name) => [name, "sample"])), + ); + }), + ]; + + expect(uris).toHaveLength(3); + for (const uri of uris) { + expect((await readJson(uri)).error, uri).toBeUndefined(); + } + }); + + it("reads onepassword://config without the service account token", async () => { + process.env.OP_SERVICE_ACCOUNT_TOKEN = "ops_e2e-sentinel-token"; + resetConfig(); + + const result = await client.readResource({ uri: "onepassword://config" }); + const text = (result.contents[0] as { text: string }).text; + + expect(JSON.parse(text)).toMatchObject({ + serverName: SERVER_NAME, + serverVersion: SERVER_VERSION, + tokenSource: "env", + nodeVersion: process.version, + }); + expect(text).not.toContain("ops_e2e-sentinel-token"); + }); + + it("reads onepassword://vaults", async () => { + const data = await readJson("onepassword://vaults"); + + expect(data).toEqual({ + vaults: [ + { id: PROD.id, name: "Prod", description: "Production", type: "USER_CREATED" }, + { id: CI.id, name: "CI", type: "USER_CREATED" }, + ], + }); + expect(opClient.vaults.list).toHaveBeenCalledTimes(1); + }); + + it("reads onepassword://vaults/{vaultId}/items for the vault named in the URI", async () => { + const data = await readJson(`onepassword://vaults/${PROD.id}/items`); + + expect(data).toEqual({ + vaultId: PROD.id, + items: [{ id: DEPLOY_KEY.id, title: "Deploy key", category: "SshKey", vaultId: PROD.id }], + count: 1, + }); + expect(opClient.items.list).toHaveBeenCalledExactlyOnceWith(PROD.id); + }); + + it("percent-decodes the vaultId template variable", async () => { + const data = await readJson("onepassword://vaults/vault%20one/items"); + + expect(data.vaultId).toBe("vault one"); + expect(opClient.items.list).toHaveBeenCalledExactlyOnceWith("vault one"); + }); + + it("rejects a malformed percent-encoded vaultId without calling 1Password", async () => { + const data = await readJson("onepassword://vaults/%E0%A4%A/items"); + + expect(data.error).toContain("not valid percent-encoding"); + expect(opClient.items.list).not.toHaveBeenCalled(); + }); + + it("reports a 1Password failure in the resource payload", async () => { + opClient.items.list.mockRejectedValue(new Error("vault not found")); + + const data = await readJson("onepassword://vaults/vlt-missing/items"); + + expect(data).toEqual({ error: "vault not found" }); + }); + + it("rejects the old 1password:// scheme as an invalid URI", async () => { + // Why the scheme changed: the SDK parses the URI with `new URL()` before + // dispatching, and a scheme cannot start with a digit. + await expect( + client.readResource({ uri: "1password://config" }), + ).rejects.toMatchObject({ code: ProtocolErrorCode.InvalidParams }); + expect(mockedGetClient).not.toHaveBeenCalled(); + }); +}); From ae34267a52dec3a9c2866c16e41c911c2963331e Mon Sep 17 00:00:00 2001 From: CakeRepository Date: Sun, 4 Oct 2026 05:02:58 -0500 Subject: [PATCH 3/4] test(resources): cover the vault allow-list end to end Read onepassword://vaults and the items template through the real MCP client with OP_MCP_ALLOWED_VAULTS set, in both protocol eras. The vault listing is filtered, items of a vault outside the list are refused before items.list() runs, and the check sees the percent-decoded vaultId, so an encoded ID can't slip past it. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- tests/resources.e2e.test.ts | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cea9b7b..168ad25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ Major release because the resource URIs changed (see **Breaking** below). Tools, ### Added -- **End-to-end resource tests** — A real MCP client reads the resources from `serveStdio(() => buildServer())` over an in-memory transport, in both the 2025 (`initialize`) and 2026-07-28 protocol eras, so an advertised URI that the SDK cannot parse or route fails CI. Adds the `@modelcontextprotocol/client` dev dependency (tests only). +- **End-to-end resource tests** — A real MCP client reads the resources from `serveStdio(() => buildServer())` over an in-memory transport, in both the 2025 (`initialize`) and 2026-07-28 protocol eras, so an advertised URI that the SDK cannot parse or route fails CI. They also cover the vault allow-list through `resources/read`, including that a percent-encoded vault ID can't bypass it. Adds the `@modelcontextprotocol/client` dev dependency (tests only). ## [4.0.4] - 2026-10-04 diff --git a/tests/resources.e2e.test.ts b/tests/resources.e2e.test.ts index b3de128..acb1543 100644 --- a/tests/resources.e2e.test.ts +++ b/tests/resources.e2e.test.ts @@ -198,4 +198,35 @@ describe.each(ERAS)("MCP resources end-to-end ($era protocol era)", ({ era, opti ).rejects.toMatchObject({ code: ProtocolErrorCode.InvalidParams }); expect(mockedGetClient).not.toHaveBeenCalled(); }); + + describe("with OP_MCP_ALLOWED_VAULTS", () => { + beforeEach(() => { + process.env.OP_MCP_ALLOWED_VAULTS = "Prod"; + resetConfig(); + }); + + it("lists only the allow-listed vaults", async () => { + const data = await readJson("onepassword://vaults"); + + expect(data.vaults.map((v: { id: string }) => v.id)).toEqual([PROD.id]); + }); + + it("refuses items of a vault outside the allow-list without listing them", async () => { + const data = await readJson(`onepassword://vaults/${CI.id}/items`); + + expect(data.error).toContain("not in the allowed vault list"); + expect(opClient.items.list).not.toHaveBeenCalled(); + }); + + it("checks the decoded vaultId, so percent-encoding cannot bypass the allow-list", async () => { + const encode = (id: string) => [...id].map((c) => `%${c.charCodeAt(0).toString(16)}`).join(""); + + const refused = await readJson(`onepassword://vaults/${encode(CI.id)}/items`); + const allowed = await readJson(`onepassword://vaults/${encode(PROD.id)}/items`); + + expect(refused.error).toContain("not in the allowed vault list"); + expect(allowed.vaultId).toBe(PROD.id); + expect(opClient.items.list).toHaveBeenCalledExactlyOnceWith(PROD.id); + }); + }); }); From 4edf37ff41b1d45014f24b3432cc05c40349e156 Mon Sep 17 00:00:00 2001 From: CakeRepository Date: Sun, 4 Oct 2026 05:11:35 -0500 Subject: [PATCH 4/4] docs(changelog): fold the unreleased 4.0.4 notes into 5.0.0 The security hardening from #32 ships in the same release as the resource URI change, so 4.0.4 is never published. Move its Security and Changed notes into the 5.0.0 entry, name the items template in the allow-list note, and point the README and agents.md at 5.0.0. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 33 ++++++++++++++------------------- README.md | 2 +- agents.md | 2 +- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 168ad25..bb6ebf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,28 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [5.0.0] - 2026-10-04 -Major release because the resource URIs changed (see **Breaking** below). Tools, prompts, configuration, and the Node.js requirement are unchanged. - -### Changed - -- **Breaking: resource URIs use the `onepassword://` scheme** — `1password://config` → `onepassword://config`, `1password://vaults` → `onepassword://vaults`, and `1password://vaults/{vaultId}/items` → `onepassword://vaults/{vaultId}/items`. Replace any hard-coded `1password://` URIs. The old URIs could never be read (see **Fixed**), so this only affects code or configuration that hard-codes them. They were still published identifiers with no possible alias, because the SDK rejects them before the server sees the request; hence the major version. -- `buildServer()` moved to `src/server.ts` (still re-exported from the entrypoint), so tests can build the real server without starting stdio. - -### Fixed - -- **Resources are now readable by MCP clients** — Every `resources/read` failed with `-32602 Resource URI … is invalid`. The SDK parses the URI with WHATWG `new URL()` before dispatching, and a URI scheme must start with a letter (RFC 3986 §3.1), so no `1password://` URI ever reached the server's handlers. -- **Per-vault items is a real resource template** — `onepassword://vaults/{vaultId}/items` is registered as a `ResourceTemplate` (listed by `resources/templates/list`) and reads `vaultId` from the template variables, percent-decoded. It used to be a static resource whose URI was the literal template string, so no concrete vault URI could ever match it. - -### Added - -- **End-to-end resource tests** — A real MCP client reads the resources from `serveStdio(() => buildServer())` over an in-memory transport, in both the 2025 (`initialize`) and 2026-07-28 protocol eras, so an advertised URI that the SDK cannot parse or route fails CI. They also cover the vault allow-list through `resources/read`, including that a percent-encoded vault ID can't bypass it. Adds the `@modelcontextprotocol/client` dev dependency (tests only). - -## [4.0.4] - 2026-10-04 +Major release: the resource URIs changed (see **Breaking** below), and it includes security hardening from an internal review. Read **Changed** before upgrading: the vault allow-list now applies server-wide, and `item_get` hides more field types by default. Tool names and input parameters, prompts, and the Node.js requirement are unchanged. ### Security - **`item_get` no longer returns secrets held in non-`Concealed` fields** — Only `Concealed` fields were masked, so SSH private keys, one-time-password (TOTP) seeds, and card numbers came back in plaintext without `reveal: true`. Masking is now deny-by-default: only known non-secret field types (text, URL, email, phone, date, month/year, menu, card type, address, reference) are shown, and every other type, including any added by future SDK versions, needs `reveal: true`. Notes are still returned as-is, matching `op item get`; don't keep secrets in the notes of vaults an agent can read. -- **Vault allow-list is now enforced server-wide** — `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` only covered `op_run` and `op_check_ref`, and only compared the vault segment as written in an `op://` reference. It now applies to every tool and resource that touches a vault: `vault_list` and `1password://vaults` are filtered, and vault IDs are checked before anything is returned or modified. `op://` references are checked both as written and by the vault they actually resolve to. It fails closed if vaults can't be listed. +- **Vault allow-list is now enforced server-wide** — `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` only covered `op_run` and `op_check_ref`, and only compared the vault segment as written in an `op://` reference. It now applies to every tool and resource that touches a vault: `vault_list` and `onepassword://vaults` are filtered, `onepassword://vaults/{vaultId}/items` refuses vaults outside the list, and vault IDs are checked before anything is returned or modified. `op://` references are checked both as written and by the vault they actually resolve to. It fails closed if vaults can't be listed. - **`op_run` redaction no longer leaks the remainder of overlapping secrets** — Redaction now masks every occurrence of every secret in a single pass over the original output, so overlapping or nested secrets can't leak: a username that is a prefix of a credential string is no longer replaced first, leaving the password visible. It also masks common encodings of each secret: base64 (including inside a larger base64 payload such as an HTTP Basic auth header), JSON-escaped and URL-encoded forms, and multi-line secrets line by line and with CRLF line endings. - **`op_run` output cap no longer exhausts memory** — The 5 MiB per-stream cap is now applied while the command runs. Excess output is discarded instead of buffered, which fixes a memory-exhaustion crash, and a secret cut off at the cap never survives as a partial prefix. - **`op_run` timeouts kill the whole process tree** — On timeout the process group (POSIX) or process tree via `taskkill /T` (Windows) is killed, and `op_run` always returns shortly afterwards, even if a background process holds the output pipes. Previously it could hang forever and leave processes running with injected secrets. @@ -40,12 +24,23 @@ Major release because the resource URIs changed (see **Breaking** below). Tools, ### Changed +- **Breaking: resource URIs use the `onepassword://` scheme** — `1password://config` → `onepassword://config`, `1password://vaults` → `onepassword://vaults`, and `1password://vaults/{vaultId}/items` → `onepassword://vaults/{vaultId}/items`. Replace any hard-coded `1password://` URIs. The old URIs could never be read (see **Fixed**), so this only affects code or configuration that hard-codes them. They were still published identifiers with no possible alias, because the SDK rejects them before the server sees the request; hence the major version. - **Behavior change: the vault allow-list is server-wide** — If you set `OP_MCP_ALLOWED_VAULTS` / `--allowed-vaults` expecting it to affect only `op_run` and `op_check_ref`, it now restricts every tool and resource. Tools that take a `vaultId` need the vault's ID, not its name. With an allow-list set, each guarded call makes one extra `vaults.list` read (mind service account rate limits). It is defense in depth; scope the service account's own vault access in 1Password first. - **`item_get` hides more by default** — SSH private keys, OTP seeds, and card numbers now show `[concealed]` unless you pass `reveal: true`. - **Stricter `op://` validation** — Malformed references passed to `item_get` and `password_read` are now rejected locally with a clear error. - **More candid `op_run` description** — The tool description no longer claims plaintext is "NEVER" returned. Redaction is best effort and protects against accidental disclosure; it is not a sandbox, because a command can deliberately transform or transmit a secret it was given. - **Publishing** — `prepublishOnly` now only matters for manual `npm publish`; the automated workflow publishes a prebuilt tarball. -- **Documentation** — README, CONTRIBUTING, and agents.md cover the server-wide allow-list, best-effort redaction, unconcealed notes, and the two-job publish workflow. +- **Documentation** — README, CONTRIBUTING, and agents.md cover the new resource URIs, the server-wide allow-list, best-effort redaction, unconcealed notes, and the two-job publish workflow. +- `buildServer()` moved to `src/server.ts` (still re-exported from the entrypoint), so tests can build the real server without starting stdio. + +### Fixed + +- **Resources are now readable by MCP clients** — Every `resources/read` failed with `-32602 Resource URI … is invalid`. The SDK parses the URI with WHATWG `new URL()` before dispatching, and a URI scheme must start with a letter (RFC 3986 §3.1), so no `1password://` URI ever reached the server's handlers. +- **Per-vault items is a real resource template** — `onepassword://vaults/{vaultId}/items` is registered as a `ResourceTemplate` (listed by `resources/templates/list`) and reads `vaultId` from the template variables, percent-decoded. It used to be a static resource whose URI was the literal template string, so no concrete vault URI could ever match it. + +### Added + +- **End-to-end resource tests** — A real MCP client reads the resources from `serveStdio(() => buildServer())` over an in-memory transport, in both the 2025 (`initialize`) and 2026-07-28 protocol eras, so an advertised URI that the SDK cannot parse or route fails CI. They also cover the vault allow-list through `resources/read`, including that a percent-encoded vault ID can't bypass it. Adds the `@modelcontextprotocol/client` dev dependency (tests only). ## [4.0.3] - 2026-10-04 diff --git a/README.md b/README.md index 2368f89..a2d3af6 100644 --- a/README.md +++ b/README.md @@ -340,7 +340,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md). Maintainers / agents: [agents.md](agents ## Changelog -See [CHANGELOG.md](CHANGELOG.md) for version history, including the **5.0.0** resource URI change (`1password://` → `onepassword://`), the **4.0.4** security release (read its **Changed** notes before upgrading), the **4.0.0** MCP v2 / 2026-07-28 migration, and the **3.0.0** `op_run` / reveal-opt-in security changes. +See [CHANGELOG.md](CHANGELOG.md) for version history, including the **5.0.0** release (resource URIs moved from `1password://` to `onepassword://`, plus security hardening; read its **Changed** notes before upgrading), the **4.0.0** MCP v2 / 2026-07-28 migration, and the **3.0.0** `op_run` / reveal-opt-in security changes. --- diff --git a/agents.md b/agents.md index 5c81317..29d64c0 100644 --- a/agents.md +++ b/agents.md @@ -94,5 +94,5 @@ When tools, prompts, or resources change, update **README.md** (npm’s face), * - `build` (`contents: read`, no OIDC): `npm ci --ignore-scripts` (no dependency install scripts), version and tag validation, build, test, then `npm pack --ignore-scripts` into `package.tgz`, uploaded as the `npm-package` artifact. - `publish` (`needs: build`): the only job with `id-token: write`. It never checks out the repo or installs dependencies; it downloads the tarball and runs `npm publish ./package.tgz --access public --ignore-scripts` (skipped if that version is already on npm). - Keep it that way: dependency code must never run in a job that holds `id-token: write`. npm's trusted-publisher config is bound to the file name `publish.yml`, so don't rename it. -- Workflow hygiene: pass `${{ ... }}` values to scripts through `env:` instead of interpolating them into `run:`, check out with `persist-credentials: false`, and don't add `issue_comment`-triggered or write-scoped one-off runner workflows (the leftover `mcp-v2-migration.yml` was removed in 4.0.4 for that reason). +- Workflow hygiene: pass `${{ ... }}` values to scripts through `env:` instead of interpolating them into `run:`, check out with `persist-credentials: false`, and don't add `issue_comment`-triggered or write-scoped one-off runner workflows (the leftover `mcp-v2-migration.yml` was removed in 5.0.0 for that reason). - Registry package name: `io.github.CakeRepository/1password` (`mcpName` / `server.json`).