feat: MCP_INSPECTOR_SECRET_KEY_FILE, and document secret storage for every runtime

[cliffhall]

Closes #2447

Rescoped after the first review rounds. This PR started as documentation only. It now also adds MCP_INSPECTOR_SECRET_KEY_FILE, a loud Docker warning, and a written threat model for the file store. #2447 has been updated to match.

Behavior change: MCP_INSPECTOR_SECRET_KEY_FILE

The file store's passphrase could only come from MCP_INSPECTOR_SECRET_KEY. In a container that value is readable through docker inspect, docker exec and the process environment, and Docker and Compose secrets, which deliver a file, could not supply it.

core/auth/node/file-secret-store.ts:

• New resolveSecretPassphrase(env). It reads the passphrase from MCP_INSPECTOR_SECRET_KEY, or from the file named by MCP_INSPECTOR_SECRET_KEY_FILE with trailing line breaks stripped. A blank MCP_INSPECTOR_SECRET_KEY still counts as unset.
• A key problem never becomes plaintext. Setting both variables, or naming a key file that is missing, unreadable or empty, is a key problem, not "no passphrase". While there is one:
  • readMap throws, so set refuses, get returns null, readAll throws (the keychain hand-off leaves the file alone), and delete stays a silent no-op;
  • readOnDiskEncryption reports unreadable with the reason, so the startup warning and the settings footer show File (unreadable) and say why.
• An explicit passphrase option still wins over the environment.
• The key-mismatch messages and the "set a key" advice (the core caveat and SecretStorageFooter) now name both variables.

Tests in file-secret-store.test.ts cover:

• resolution: neither set, env only, file with CRLF/LF stripping, a blank env plus a file, a relative path, both set, a missing file, an empty file;
• the store: encrypting from the file, refusing to write (with no file created) when the key file is missing, leaving an existing file byte-identical under a key problem, and the explicit option overriding the environment.

Loud callout for the plaintext host fallback

On a host with no keychain (Linux without libsecret or a Secret Service, headless or SSH sessions, Termux), the Inspector automatically stores secrets in a plaintext file.

• docs/secret-storage.md, README.md and docs/environment-variables.md each now carry a [!WARNING] saying so. The guide's warning lists the three ways out.
• warnAboutSecretStorage now ends its fallback or caveat output with a link to the guide (SECRET_STORAGE_DOCS_URL, on main). The ordinary keychain run stays silent, and three new tests cover it.

Docs

• New docs/secret-storage.md, covering every runtime:
  • what counts as a secret;
  • the selection order, with host rows in the "which store do I get" table;
  • the memory store;
  • the file store: location, encryption (including the key file), permissions and locking;
  • "What the file store protects against", a ✅/❌ threat model for plaintext and encrypted files. Encryption helps against the file leaking on its own. It does not help against anyone who can read the key, root or the docker group, or code running as the same user. Stdio servers get only the SDK's environment allowlist plus their configured env:, but they run as the same user;
  • getting a keychain back;
  • where the active store is reported;
  • changing the store.
• docs/docker.md: the section is cut down to what's container-specific, plus a [!WARNING] callout:
  • mounting the volume turns on plaintext file storage unless a key is supplied;
  • supply the key as a file (docker run bind mount and Compose secrets: examples), readable by uid 1000;
  • even encrypted, on-disk storage carries moderate risk, with a link to the threat model.
• docs/environment-variables.md: a new MCP_INSPECTOR_SECRET_KEY_FILE row, links to the new guide, and a fix for the section's claim that headers are stored as secrets (they stay in mcp.json).
• docs/mcp-server-configuration.md, docs/publishing.md, README.md: links and the Documentation table updated.

Follow-ups

• docs: update docs with session client (mcpdo), latest docker config, and secret storage on modelcontextprotocol.io #2450: bring the modelcontextprotocol.io Inspector pages up to date (same shape as docs: new modelcontextprotocol.io Inspector documentation for v2 (legacy vs. modern era, more screenshots) #1803 / modelcontextprotocol#3143).

UI change

SecretStorageFooter's plaintext advice now reads "Set MCP_INSPECTOR_SECRET_KEY or MCP_INSPECTOR_SECRET_KEY_FILE to encrypt." It's a copy-only change, and the footer layout is unchanged.

npm run local:gate passed.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Several documentation accuracy and cross-reference issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

• Clarify file mount requirements for secret persistence · New
• Clarify runtime differences for MCP_STORAGE_DIR · New

What changed in this PR

Moves secret-storage documentation into a runtime-independent guide and updates related references.

Changes:

• Documents store selection, persistence, encryption, migration, and reporting.
• Narrows Docker documentation to container-specific guidance.
• Updates configuration references and the README index.

File	Review
README.md	Adds the new guide to the documentation index. No issues found.
docs/​secret-storage.md	Adds cross-runtime guidance. Nits: Describe lazy selection on first secret-backed access rather than startup (1 vote); include whitespace-only store values as unset (1 vote); distinguish web MCP_STORAGE_DIR from CLI/TUI MCP_CLIENT_CONFIG_PATH on lines 55 and 65 (2 votes); distinguish mounted file storage from a container’s ephemeral writable layer (1 vote); correct the salt to per-write or omit its frequency (1 vote); clarify that CLI/TUI warnings appear only when secret storage is first used (1 vote).
docs/​mcp-server-configuration.md	Updates the secret-storage link. No issues found.
docs/​environment-variables.md	Corrects secret descriptions and links. Nit: Call these “Inspector secrets” or “secrets,” since the enterprise IdP client secret is not server-scoped (1 vote).
docs/​docker.md	Retains container-specific guidance. Nits: Clarify that automatic durability detection requires mounting the secret file’s parent directory, unless the file store is explicitly selected (3 votes); update the stale docs/publishing.md cross-reference to the new guide or split the links (1 vote).

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot review round 1: all addressed in 14cde9e

Inline (each one also has a reply in its own thread):

• Mount the file's parent directory: fixed in docker.md. I advise against a single-file bind mount rather than pairing it with MCP_INSPECTOR_SECRET_STORE=file, because saves replace the file by rename, which a bind-mounted file can't take.
• MCP_STORAGE_DIR and client.json: fixed. client.json moves only for the web backend; the CLI and TUI use MCP_CLIENT_CONFIG_PATH.

Nits from the overview table (these have no threads):

• Lazy selection: fixed. The web backend selects at startup, the CLI and TUI on their first access to a secret (defaultSecretStore() → DeferredSecretStore). The "when is it reported" and "re-tightened" wording follows from that.
• Whitespace-only MCP_INSPECTOR_SECRET_STORE: fixed. It counts as unset.
• Mounted file vs. writable layer: fixed. The explicit-store table row now says file in a container is durable only on a volume.
• Salt frequency: fixed. writeMap generates a fresh salt on every write. The Docker guide's old "per-file" wording was already wrong, and the guide now says per write.
• "Server secrets" in environment-variables.md: now "the Inspector's secrets", since the IdP client secret is install-level.
• Stale docs/publishing.md link: split so it points to both the Docker guide and the new guide.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The new guide contains several inaccurate or non-portable behavioral claims that should be corrected before approval.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Clarify runtime differences for MCP_STORAGE_DIR
• Clarify file mount requirements for secret persistence

Previously missed (2)

In code that hasn't changed since last review

Refer to the configured secrets-file path

docs/​secret-storage.md:65

Recovery is described using the literal default filename, but this guide has just documented that MCP_INSPECTOR_SECRET_FILE and MCP_STORAGE_DIR can relocate the file. Users with either override would delete the wrong file and continue getting the unreadable-file error; refer to the configured secrets-file path instead.

Clarify migration behavior for existing keychain values

docs/​secret-storage.md:90

The migration does not copy values that already exist in the keychain: it deliberately keeps the keychain value and only writes missing entries, then deletes the file once every entry is either present or successfully written. Saying the file is removed only when every value was "copied" is therefore inaccurate; describe the existing-keychain case too.

[cliffhall]

Copilot review round 2: both "previously missed" items addressed in 67b23bd

This round had no inline comments, so there are no threads to reply into. Both items were real:

• Refer to the configured secrets-file path (secret-storage.md:65): fixed. Recovery now says to delete the file at its configured path, and points to "Where the file is" as well as the startup warning and settings footer, which both print that path.
• Hand-off wording for existing keychain values (secret-storage.md:90): fixed. A value already in the keychain is kept and not copied, and only missing entries are written. The file is removed once every entry is accounted for, meaning each was either already present or written. A file with no entries is left in place. This matches handOff() in secret-store-selection.ts.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Several secret-storage claims need clarification to avoid misleading users about persistence, paths, migration, and permissions.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Clarify custom lock-file naming and environment-variable precedence in the new guide.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Document lock path based on configured secrets file · New

[cliffhall]

Copilot review round 4: addressed in 2b96781

• Lock path follows the configured secrets file: fixed, and replied in the thread. The lock is <secrets-file>.lock (secrets.json.lock by default).

Round 3 came back with no findings. The round-4 headline also mentions "environment-variable precedence", but no comment or finding covers it, and the lookup order is already spelled out in "Where the file is" and "How the store is chosen".

[Copilot]

Copilot review overview

🔵 Needs a closer look

Several documentation inaccuracies about configuration sources, platform permissions, keyring selection, paths, migration, and runtime-specific reporting must be corrected.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Document lock path based on configured secrets file

[cliffhall]

Copilot review round 5: no findings and no inline comments. The round-4 lock-path thread is marked resolved. The round's headline is a generic summary that no finding backs, so there's nothing to act on. That makes two rounds with no findings (3 and 5), so I'm ending the review loop here.

[Copilot]

Copilot review overview

🟡 Changes recommended

The empty key-file setting and lone-CR handling issues must be fixed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Blank key-file setting incorrectly falls back to plaintext · New
• Missing footer test for MCP_INSPECTOR_SECRET_KEY_FILE tooltip · New
• Advice test does not verify key-file variable guidance · New

[cliffhall]

Copilot review round 6: all three addressed in bf4beba (each one also has a reply in its own thread)

• A blank MCP_INSPECTOR_SECRET_KEY_FILE fell back to plaintext: fixed. A variable that is present but blank is now a key problem, and the "both set" check uses presence. A lone trailing CR is stripped too, which the headline mentioned without a comment.
• Footer tooltip coverage: a new hover test asserts both variable names.
• Caveat coverage: the test now asserts the exact sentence.

npm run local:gate passed.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Documentation must be aligned with the implemented store-selection and decryption-failure behavior.

Review effort: Balanced
Findings: 2

Open (2)

• Advice test does not verify key-file variable guidance
• Missing footer test for MCP_INSPECTOR_SECRET_KEY_FILE tooltip

Resolved since last review (1)

• Blank key-file setting incorrectly falls back to plaintext

[cliffhall]

Copilot review round 7: no new comments. The two items it still lists as open, footer tooltip coverage and caveat advice coverage, are the round-6 threads. Both were fixed in bf4beba and have replies in their threads. The tests are SecretStorageFooter.test.tsx "offers both key variables in the plaintext tooltip" and the exact-sentence assertion in secret-storage-info.test.ts. The headline's "decryption-failure behavior" isn't backed by any comment. The docs say a file that can't be decrypted reads as empty and refuses writes, which matches get returning null and set throwing SecretFileKeyMismatchError. Requesting one confirming round.

[Copilot]

Copilot review overview

🟡 Changes recommended

Resolve the remaining documentation mismatches, path-selection inaccuracies, and insecure Docker key-file example.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

• Update encryption documentation for MCP_INSPECTOR_SECRET_KEY_FILE · New
• Clarify conflict applies only to two nonblank keys · New

Resolved since last review (2)

• Advice test does not verify key-file variable guidance
• Missing footer test for MCP_INSPECTOR_SECRET_KEY_FILE tooltip

[cliffhall]

Fixed in 8e4cfff. The file-secret-store.ts module header (the "bounded three ways" paragraph and the Key. paragraph) and the store list in secret-store.ts now say encryption comes from MCP_INSPECTOR_SECRET_KEY or the file named by MCP_INSPECTOR_SECRET_KEY_FILE. While there, I corrected the header's "per-file" salt: writeMap regenerates the salt on every write.

[cliffhall]

Following up on this thread, which round 9 still lists as open. The comments it named were updated in 8e4cfff (the file-secret-store.ts module header and the FileSecretStore entry in secret-store.ts). b4af7e4 covers the two remaining mentions: the plaintext doc on SecretStorageInfo and the readOnDiskEncryption comment. MCP_INSPECTOR_SECRET_KEY now never appears alone as the only way to encrypt.

[cliffhall]

Copilot review round 8: addressed in 8e4cfff (each inline comment also has a reply in its own thread)

• Source comments still named only MCP_INSPECTOR_SECRET_KEY: fixed in file-secret-store.ts and secret-store.ts, along with the stale "per-file salt".
• "Both set" should mean both non-blank: fixed in all three docs.
• "Insecure Docker key-file example" (named in the headline, with no comment): real, and fixed. The example wrote the key with the default umask, often 0644, and the Compose example kept it in the project directory. It now creates the key with umask 077 under ~/.config/mcp-inspector/, and Compose references it via ${HOME}. It also explains that the 0600 file must be owned by uid 1000 (chown, not chmod), since without Swarm, Compose secrets are bind mounts.

npm run local:gate passed.

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical key-file collision can corrupt the secret store, and the Docker guidance also needs correction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Reject key files that resolve to the secrets file · New
• Update encryption documentation for MCP_INSPECTOR_SECRET_KEY_FILE

Resolved since last review (1)

• Clarify conflict applies only to two nonblank keys

[cliffhall]

Copilot review round 9: addressed in b4af7e4 (both inline comments also have replies in their threads)

• Reject key files that resolve to the secrets file: real, and fixed. It's checked by path and by device/inode, before reading, so symlinks and hard links are caught too. There are four tests.
• Encryption documentation for _FILE (carried over from round 8): the last two source comments that named only MCP_INSPECTOR_SECRET_KEY are updated.
• The headline also says "the Docker guidance also needs correction", but no comment or finding backs it, so there's nothing specific to act on.

npm run local:gate passed.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Security-sensitive storage behavior warrants human approval, and minor documentation accuracy comments remain.

Review effort: Balanced
Findings: 1

Open (1)

• Update encryption documentation for MCP_INSPECTOR_SECRET_KEY_FILE

Resolved since last review (1)

• Reject key files that resolve to the secrets file

[cliffhall]

Copilot review round 10: no new comments. The round-9 same-file finding is marked resolved. The one item still listed as open, Update encryption documentation for _FILE, has been addressed twice (8e4cfff and b4af7e4). A grep of core/ and clients/web/ finds no remaining comment that gives MCP_INSPECTOR_SECRET_KEY as the only way to encrypt; every other mention either names both variables or refers to the direct variable on purpose (the blank-means-off rule). The headline asks for human approval of the security-sensitive storage behavior, which is the maintainer's call. I'm ending the Copilot loop here.