MatAtBread / MatAtBread/matbot
The default Vault impl can leak secrets into child processes via process.env
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 5
- Forks
- 2
- Avg merge
- 2h 31m
- Merged PRs (30d)
- 12
Description
Problem
Every secret the Vault holds is readable by any tool that can spawn a process or read a file. A turn only has to run env.
The mechanism is four steps, none of which is individually unreasonable:
loadDotEnvpromotes.envintoprocess.env.apps/cli/src/config.ts:70-71writes every parsed key intoprocess.envunless already set. It does this becauseEnvFileVaultreads from an env snapshot, not from the file — both construction sites passprocess.env(index.ts:695,index.ts:846) — so promotion is currently load-bearing, not incidental.bashcopies all ofprocess.envinto every child shell.plugins/bash/src/index.ts:251merges the whole environment beforespawn, with the caller'senvlayered on top. So{"script": "env"}returns every API key in the vault.backgroundandmcpdo the same.background:238spreadsprocess.envinto detached children;mcp/client.ts:40spreads it into every stdio MCP server — so a third-party MCP server is handed the full set on launch..envsits in the project directory, whichdocker-bashmounts.docker-bashgets this right at the process level — it passesenv: {}(docker-bash:495) and inherits nothing. But the project root is mounted read-only at/app, and.envis in it (config.ts:64), socat /app/.envrecovers what the env cleanliness denied.
This runs against the existing doctrine that secrets reach tools only through the Vault and only by substitution — the reason http is deliberately kept away from the Vault entirely. The ${NAME} indirection is doing its job at the API surface while the values sit in ambient state underneath it.
This is a property of the default Vault implementation (a .env file), not of the Vault API
Worth being precise about what is broken here: the Vault interface is fine, and the ${NAME} indirection is doing what it was designed to do. The leak is a property of the medium the default node backend chose — a .env file, read through an environment snapshot. Everything above follows from that one decision: the file is in the project directory (so it is mountable and readable), and its values are in process.env (so they are inheritable).
EnvFileVault already acknowledges that its medium constrains it — it overrides unstorableKey to narrow the base vault's name policy to what a .env file can round-trip. The confidentiality constraint is the same kind of thing, just unstated.
A backend built on OS credential storage — macOS Keychain, the Windows Credential Manager, libsecret, or a broker like gpg-agent/ssh-agent/Vault/a cloud secret manager — does not have this shape at all:
- There is no file in the project directory, so there is nothing for a
:romount or acatto recover (fix 4 becomes moot). - Nothing is promoted into
process.env, so there is nothing for a child process to inherit (fix 1 becomes moot, and fix 2 stops mattering for vault secrets). - Several of these media have their own per-process consent prompt, which is the same shape as fix 3 and a useful precedent for how it should feel.
The swap point already exists and is already documented as a swap point: register('Vault', impl) behind the capture-safe forwarding proxy, so a reference held across the swap keeps resolving to the live backend. A more secure vault is a plugin, not a port.
Two things that do not follow from this, and are the reason the fixes below still stand:
- Fix 2 is independent. An operator who exports secrets in the real environment (CI,
docker run -e, a systemd unit) is leaking them to every child regardless of which vault is registered. Dropping blanket inheritance frombash/background/mcpis worth doing on its own terms. - Fix 3 becomes more valuable, not less. Once the ambient paths are closed,
vault.getis the remaining path, and it is the one the design intends. A gate there is what makes "which tool asked for which secret" an answerable question.
So the honest framing is: fixes 1 and 4 are mitigations for a medium that is the default because it is zero-configuration, and the durable answer for an installation that cares is a different registered backend. That backend does not exist yet, and is probably its own issue.
Not the fix: trapping process.env
The tempting answer is to interpose on process in the tool execution environment and prompt on access. It doesn't work here, for two independent reasons.
It misses the mechanism. Steps 2–4 are child_process and fs, not property access on process.env. A proxy over the accessor stops none of them.
It isn't enforceable in-process. Plugins are loaded with a plain dynamic import() and get the real globals; there is no module isolation and the design does not claim any. A shadowed process is bypassed by globalThis.process, await import('node:process'), createRequire('process'), /proc/self/environ, or reading .env directly. Against a hostile plugin it is theatre; against a careless one it has the value of a lint rule, and should be priced as one.
The general point: once a plugin is loaded it has full Node capability, so an in-process capability trap is not a boundary. The gate belongs at the secret, not at the accessor.
Proposed fixes
In value order. The first three are small and independent.
1. Stop promoting .env into process.env
loadDotEnv returns the parsed map instead of mutating; the vault is constructed from { ...dotEnv, ...definedProcessEnv }, preserving today's precedence (a real environment variable wins, which is what the if (!(key in process.env)) guard means today). The wizard's carry-across at index.ts:701 becomes an in-memory merge rather than an assignment to process.env.
Cheap: the return value of loadDotEnv is discarded at both call sites (index.ts:777, index.ts:787), so changing its shape breaks nothing. This alone de-fangs steps 2 and 3 for every secret that lives in .env rather than the real environment.
2. Stop inheriting the environment in child-spawning tools
docker-bash already demonstrates the target behaviour (env: {}). Make bash match: no inherited environment by default, with inheritance available explicitly. background and mcp likewise — an MCP server should receive the variables its config names, not everything.
This is a behaviour change and needs a migration note: a script relying on an inherited PATH, HOME or TERM will break. Options are an allowlist of non-secret variables, or full inheritance behind an explicit opt-in. The allowlist is probably right for bash, since the common need is shell hygiene rather than secrets.
3. Gate vault.get per tool
The intended path deserves a gate even after the leaks are closed. ctx.vault is already on ToolContext, so this is one wrapper at the same seam as the other gates — gate id vault.read:<NAME>, decided by the PermissionGate from #62. Absent a gate, it asks; an install that doesn't want to be asked registers a gate plugin.
4. Move .env out of the project directory
Closes step 4, which the first three don't touch: docker-bash keeps its clean environment and can no longer read the file either. Needs a decision about where (alongside .data? outside it?) and a migration for existing installs, so it is the most disruptive of the four and the easiest to defer.
Blast radius
| Change | Files | Size |
|---|---|---|
1. loadDotEnv returns a map; vault built from the merge |
apps/cli/src/config.ts, apps/cli/src/index.ts (3 sites) |
~15 lines |
2. bash env allowlist / opt-in |
plugins/bash/src/index.ts |
~15 lines + contract doc |
2. background, mcp |
plugins/background/src/index.ts, plugins/mcp/src/client.ts |
~10 lines |
3. vault.get gate |
core executor wrap (shared with #62) | ~15 lines |
4. .env relocation |
apps/cli/src/config.ts, env-vault.ts, docs, migration |
larger — defer |
| Tests | a turn that runs env recovers nothing; precedence preserved; MCP server receives only its configured vars |
new |
Nothing in plugin-api changes, and no frontend changes.
Relation to #62
Independent of #62 for fixes 1, 2 and 4 — those are leaks and should be fixed regardless of whether a permission gate exists. Fix 3 consumes #62's PermissionGate and should land after it.
Worth stating the resulting honest boundary in both places: a loaded plugin has full Node capability, so this issue is not about containing a hostile plugin. It is about not leaving every secret in ambient process state where an ordinary tool call — or a prompt-injected LLM composing a function_tool lambda — recovers the lot by accident.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with apps/cli/src/config.ts and apps/cli/src/index.ts, then inspect the child-spawning entry points in plugins/bash/src/index.ts, plugins/background/src/index.ts, plugins/mcp/src/client.ts, and docker-bash. Map the environment and .env flows before choosing the scoped fixes, noting the dependency on #62 for vault.get gating. Done means the proposed environment-leak scenarios are covered by tests, precedence is preserved, and migration or behavior changes are documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- backend, cli, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100