<!-- review: timestamp=2026-05-22T09:00:00Z  repo=danieljustus/OpenPass  head=5ac18549160f39cc35447fc04a77a405457ca6bc -->

# Code Review — 2026-05-22

---

- [ ] **[Security] Arbitrary filesystem write via agent-controlled `output_path` in `handleGenerateTemplate`**
  - **Status quo:** `internal/mcp/server/tools_template.go:55` calls `os.WriteFile(outputPath, []byte(output), 0600)` where `outputPath` is taken directly from the agent MCP request (`req.GetString("output_path", "")`). Any agent with `canWrite: true` can write rendered template content to any path the process has write access to — including `<vault>/templates/`, shell init files, or `.ssh/authorized_keys`. The only guard is the `canWrite` capability check, which is too coarse for a file-write-to-any-path operation.
  - **Proposed solution:** Validate `outputPath` against a user-configured allowlist of permitted output directories (e.g., a `template_output_dirs` config key) or restrict writes to within the vault directory by default. Reject paths that resolve outside the allowed root via `filepath.Rel`.
  - **Effort/Impact:** Low effort / high impact — one validation function before the `os.WriteFile` call.

- [ ] **[Security] User-defined API templates lack SSRF protection on `base_url`**
  - **Status quo:** `internal/mcp/apitemplates/template.go:137-149` loads user-defined templates from `<vault>/templates/<name>.yaml` with no validation of the `base_url` field against private/loopback/link-local IP ranges. An agent can name any user-defined template and trigger requests to internal services (AWS IMDS at `169.254.169.254`, `localhost`, `192.168.x.x`). The `normalizeEndpoint` function in `tools_execute_api_request.go:260` validates only the path, not the host. Built-in templates are safe; user-defined templates are not.
  - **Proposed solution:** In `parseTemplate`, parse `base_url` with `net/url.Parse` and reject URLs whose resolved host is a loopback, link-local (169.254.x.x), private (RFC 1918), or metadata-service address. Provide an escape hatch (`allow_private: true`) for intentional local-API use cases.
  - **Effort/Impact:** Medium effort / high impact — prevents credential-bearing requests from reaching internal infrastructure.

- [ ] **[UX/DX] `envWhitelist` YAML key uses deprecated "whitelist" terminology**
  - **Status quo:** `internal/config/config.go:38` exposes `EnvWhitelist []string \`yaml:"envWhitelist"\`` as a user-facing config key. "Whitelist/blacklist" are deprecated by NIST SP 800-53 and most style guides in favor of "allowlist/denylist". Users reading docs or editing `config.yaml` encounter outdated language on an otherwise modern tool.
  - **Proposed solution:** Add `env_allowlist` as the canonical key, keep `envWhitelist` as a deprecated alias that is merged in `config_merge.go` with a one-time deprecation warning on load. Update `internal/config/dottedpath.go:27` and documentation. Deprecation window of one major version (v5+).
  - **Effort/Impact:** Low effort / medium impact — improves first-impression quality for security-conscious users and aligns with current industry terminology.

- [ ] **[Architecture] `cmd/root.go` is an incomplete migration shim with 25+ re-export aliases**
  - **Status quo:** `cmd/root.go:1-60` contains 25+ `var X = cli.X` aliases after moving logic into `internal/cli/`. The file is now a shim that re-exports every symbol callers needed during the migration. This means two authoritative locations for CLI surface decisions: `cmd/` (where cobra commands still live) and `internal/cli/` (where the actual logic lives). PRs that touch CLI behavior must check both packages.
  - **Proposed solution:** Complete the migration in one direction: either inline `internal/cli` back into `cmd/` (simpler) or update all remaining `cmd/*.go` files that use `cli.X` references directly and delete the aliases from `cmd/root.go`. The `internal/cli` package boundary is not providing testability gains since it's not separately tested.
  - **Effort/Impact:** Medium effort / medium impact — reduces cognitive load for future contributors and eliminates the dual-location confusion.

---

**Quick win:** The arbitrary write path in `handleGenerateTemplate` (`tools_template.go:55`) is a one-function fix with the highest security impact — add a `filepath.Rel(vaultDir, outputPath)` guard before `os.WriteFile` and reject paths that escape the vault directory.

**Next step:** `/02-gh-plan --from-review .github/review/2026-05-22T09-00-00Z.md`
