<!-- review: timestamp=2026-05-22T12:18:15Z  repo=danieljustus/OpenPass  head=5ac18549160f39cc35447fc04a77a405457ca6bc -->

# Code Review — 2026-05-22 (follow-up)

A first review earlier today (`.github/review/2026-05-22T09-00-00Z.md`) covered
the `handleGenerateTemplate` write path, SSRF in user-defined API templates,
the `envWhitelist` terminology, and the `cmd/root.go` migration shim. This
follow-up explores areas that pass left untouched — execution-flow safety in
`execute_with_secret`, the pseudonymized list path, and the v3→v4 deprecation
stubs — and produces fresh, non-overlapping TODOs.

---

- [ ] **[Security] Agent-supplied `env_vars` in `execute_with_secret` bypass the env allowlist**
  - **Status quo:** `internal/secrets/runner.go:63-66` builds `cmd.Env` from `envfilter.DefaultWhitelist()` (PATH, HOME, SSH_AUTH_SOCK, …) and then appends every entry from `opts.Env` verbatim. `opts.Env` is fed directly from the agent's `env_vars` argument (`internal/mcp/server/tools_execute_with_secret.go:118-131`) with no filtering. An agent with `canRunCommands` can therefore set `LD_PRELOAD=/tmp/x.so`, `DYLD_INSERT_LIBRARIES=…`, `NODE_OPTIONS=--require ./evil.js`, `PYTHONSTARTUP=evil.py`, `BASH_ENV=evil.sh`, or `RUBYOPT=…` and have them apply to any subsequent command — including a benign-looking `git pull` or `npm install` that the human just approved. The approval summary at `tools_execute_with_secret.go:249` is generic ("agent X requests to execute a command with secret injection") and never shows the env overlay to the approver, so the user has no chance to spot the hijack in the prompt.
  - **Proposed solution:** Introduce a hard denylist of interpreter/loader-injection variables in `internal/envfilter` (at minimum: `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_*`, `NODE_OPTIONS`, `PYTHONSTARTUP`, `PYTHONPATH`, `BASH_ENV`, `ENV`, `RUBYOPT`, `PERL5OPT`, `PERL5LIB`). Apply it to agent-supplied env keys in `handleExecuteWithSecret` and `handleRun` before merging into `resolvedEnv` and reject the call with `ERR_VALIDATION_FAILED` listing the offending keys. Optionally extend the approval `Intent.Summary` to include the resolved command and the keys of `env_vars` (values redacted) so approvals are auditable.
  - **Effort/Impact:** Low effort / high impact — one denylist constant plus 3-5 lines at each MCP entry point; closes a meaningful approval-prompt bypass without restricting any normal use case.

- [ ] **[Performance] `listPseudonymized` is single-threaded and uncached — every `find_entries` decrypts the entire vault**
  - **Status quo:** `internal/vault/search.go:303-364` walks `entries/` with `filepath.WalkDir`, sequentially reading and `vaultcrypto.Decrypt`-ing every `.age` file just to recover plaintext paths, with no caching whatsoever. By contrast the non-pseudonymized `List` path uses both `cachedList` (30 s TTL with mtime validation, `search.go:163-194`) and `listEntriesFast` (no decryption). On a pseudonymized vault, `FindWithOptions` calls `List → listPseudonymized` and then can decrypt a subset *again* in its second pass (`search.go:476-566`), so a single MCP `find_entries` query against a 50k-entry vault performs up to ~100k sequential age decryptions. The design comment at `search.go:5-90` explicitly justifies the bounded worker pool used by `FindWithOptions` but `listPseudonymized` doesn't use it.
  - **Proposed solution:** (a) Add an in-memory cache for pseudonymized listings keyed on `vaultDir`, invalidated by `InvalidateListCache` and an entries-dir mtime check, mirroring `cachedList`. (b) Replace the serial `WalkDir` decrypt with the same bounded worker pool used by `FindWithOptions` (`opts.MaxWorkers`, default 4). (c) Cache the decrypted entries from the listing pass and reuse them in the second pass of `FindWithOptions` when pseudonymization is enabled, to avoid the double-decrypt.
  - **Effort/Impact:** Medium effort / high impact — closes a ~10× wall-clock regression for pseudonymized power users and aligns the two list paths with the documented performance contract in the design comment.

- [ ] **[UX/DX] Deprecated v3 commands still advertise full v3 help text — only `RunE` errors out**
  - **Status quo:** `cmd/mcp/mcp_install.go:15-46`, `cmd/mcp/mcp_config.go:20-50`, and `cmd/mcp/mcp_token.go:14-40` register cobra commands whose `Short`, `Long`, and `Example` blocks describe the full pre-v4 behavior ("Automatically detect and configure a supported AI agent…", "By default, output uses stdio transport; use --http for HTTP transport…", "openpass mcp token create --name 'ide-tools' --tools list,get"). Only the `RunE` returns the v4 deprecation error (`ExitNotFound`, "use openpass agent install"). A user running `openpass mcp install --help` or following v3 scripts in upstream docs sees a confident, fully-documented command and only learns it no longer works when they actually invoke it — which contradicts the v4.0 CHANGELOG entry ("Deprecation stubs remain in v4.0 (print replacement, exit 2) and will be removed in v4.1").
  - **Proposed solution:** Replace `Short`/`Long`/`Example` on each of the three stubs with a one-line deprecation notice that names the v4 replacement and the removal target (e.g. `Short: "[Deprecated v4.0, removed in v4.1] Use 'openpass agent install [agent]'"`). Optionally mark them `Hidden: true` so they no longer appear in top-level `--help` listings. Also have `RunE` print the replacement on stdout in addition to returning the `CLIError`, so users running with `--quiet` or capturing stderr still see the migration hint.
  - **Effort/Impact:** Low effort / medium impact — ~30-line patch across three files; removes a real "the docs lie" moment during the v3→v4 migration window.

---

**Quick win:** The `env_vars` denylist in `internal/envfilter` is a single small change with the highest security payoff — a constant slice plus a five-line filter at the two MCP execution entry points (`tools_execute_with_secret.go`, `tools_run.go`). Worth pairing with surfacing the resolved command and env-var keys in the approval summary so future audit logs and human approvals reflect what was actually executed.

**Next step:** `/02-gh-plan --from-review .github/review/2026-05-22T12-18-15Z.md`
