Skip to content

fix(security): harden item_get, vault allow-list, op_run, and CI (v4.0.4) - #32

Closed
CakeRepository wants to merge 1 commit into
masterfrom
claude/security-vulnerabilities-review-5d20ad
Closed

fix(security): harden item_get, vault allow-list, op_run, and CI (v4.0.4)#32
CakeRepository wants to merge 1 commit into
masterfrom
claude/security-vulnerabilities-review-5d20ad

Conversation

@CakeRepository

@CakeRepository CakeRepository commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Security release v4.0.4, from an internal review of the repo. Full details are in the CHANGELOG.md 4.0.4 entry.

Vulnerabilities fixed

Severity Issue Fix
High Any GitHub user could trigger mcp-v2-migration.yml (a leftover one-time runner with contents: write) by commenting /run-mcp-v2-migration on a PR. It ran npm install with the write token persisted in .git/config. Workflow deleted
High item_get with reveal: false returned SSH private keys, TOTP seeds and card numbers in plaintext; only Concealed fields were masked. Deny-by-default masking: only known non-secret field types are shown
Medium OP_MCP_ALLOWED_VAULTS was enforced by 2 of 15 tools, so it could be bypassed with password_read/item_get. It also checked only the reference as written. New src/vault-access.ts: enforced by every tool and resource; references are also checked by the vault they resolve to; fails closed
Medium op_run redaction was order-dependent. A shorter secret inside a longer one (e.g. a username inside user:password) got masked first, which leaked the rest. Encoded forms weren't masked at all. New src/redaction.ts: single pass over the original output with overlapping matches merged; also masks base64 (all 3 alignments), JSON/URL-escaped and multi-line/CRLF forms
Medium op_run buffered output without limit; the 5 MiB cap applied only after exit, so a command like yes could exhaust memory and crash the server The cap is enforced while the command runs; a secret cut off at the cap never survives as a partial prefix
Low–Med The op_run timeout killed only the shell, and a backgrounded process holding the pipes could hang the call forever Kills the whole process tree (process group on POSIX, taskkill /T on Windows) and always returns within timeout + 2.5 s
Low The child-env credential scrub was case-sensitive, which Windows env names are not Case-insensitive scrub
Low publish.yml interpolated ${{ github.event.release.tag_name }} into shell, and dependency code ran in the job holding id-token: write Tag passed via env; persist-credentials: false; build/test/pack (npm ci --ignore-scripts, no OIDC) split from a publish-only job
Low macOS Keychain lookup resolved security through PATH Uses /usr/bin/security
Low A token passed on the command line is visible in process listings Startup warning plus docs

The op_run tool description no longer claims plaintext is "NEVER" returned. Redaction is best effort and protects against accidental disclosure; it is not a sandbox.

Behaviour changes (also under Changed in the CHANGELOG)

  • The vault allow-list is now server-wide. Tools that take a vaultId need an ID, not a name. With an allow-list set, each guarded call makes one extra vaults.list read.
  • item_get hides more fields by default. SSH keys, OTP seeds and card numbers now need reveal: true.
  • Malformed op:// references are rejected locally by item_get and password_read.

Testing

  • npm run clean && npm ci && npm run build && npm run lint && npm test on Windows (Node 22): 386 passed, 1 skipped (POSIX-only).
  • The same on Linux/WSL with Node 24.13 and Node 20.20: 387 passed each.
  • 104 tests on master → 387 here. The new tests cover each fix, including:
    • mutation checks and a 3000-case redaction fuzz against a naive reference;
    • real process-tree, timeout and memory-flood tests.
  • npm ci --ignore-scripts + build + test verified on Linux Node 24 before adding it to the publish build job.
  • End-to-end stdio smoke test of the built server:
    • initialize and tools/list (15 tools);
    • op_run child does not see the server token;
    • an echoed token comes back redacted.
  • publish.yml was validated structurally (YAML parse, needs/outputs/artifact linkage, no ${{ in run:), and its pack step was simulated locally.

Not in this PR

  • The 1password:// resources were already unreadable: new URL() rejects a scheme that starts with a digit. That is tracked separately.

🤖 Generated with Claude Code

@CakeRepository @claude
fix(security): harden item_get, vault allow-list, op_run, and CI (v4.…
d9905bd 
…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 <noreply@anthropic.com>
@CakeRepository CakeRepository mentioned this pull request Oct 4, 2026
Merged
CakeRepository added a commit that referenced this pull request Oct 4, 2026
@CakeRepository @claude
fix: readable onepassword:// resources and security hardening (v5.0.0) (
9287e77 
#33)

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@CakeRepository

CakeRepository commented Oct 4, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #33, which merged these changes together with the resource URI fix (onepassword://). They shipped in v5.0.0 (https://github.com/CakeRepository/1Password-MCP/releases/tag/v5.0.0) instead of a separate 4.0.4 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@CakeRepository