fix!: harden the vault against injection, silent data loss, and weak keys

[jahvon]

Summary

A security and correctness pass over the whole package, prompted by an audit done while
scoping curated external-provider presets. Building presets on top of the library as it
stood would have meant shipping a secrets feature on a foundation that could corrupt and
leak the secrets it held.

Every fix here replaces behaviour that failed silently — no error, no log, no crash.
That is the common thread, and it is why none of it had been noticed.

Three findings were verified empirically rather than argued from reading the code:

1. Command injection on every SetSecret. SetSecret rendered the plaintext into a
   command string that is then parsed and executed by a shell, and the template engine does
   no quoting. This needed no attacker — an ordinary password was enough. p@$$w0rd had
   $$ expanded to the process ID and a different secret was stored, silently;
   correct horse battery word-split and stored correct; anything containing ; ran
   arbitrary commands. A set/get round trip through the real shell now survives
   $(id), backticks, quotes, globs, spaces and newlines byte-exact, and an injection
   payload provably does not execute.

2. Concurrent writes lost secrets. load() ran once at construction and every save
   rewrote the whole file from that snapshot, with nothing synchronizing writers across
   processes. Measured directly: with the new lock disabled, 8 concurrent writes collapse
   to 1 surviving secret.

3. scrypt ran at N=2²⁰ (~1 GiB and seconds per derivation). Concurrent derivations were
   a trivial local denial of service. The crypto test suite drops from 35s to 0.7s as a
   direct result of correcting it.

Notable Changes

Secret handling

• Secrets travel to external backends over stdin only. Configs referencing {{value}}
  or {{password}} in any command template are rejected at load rather than silently
  corrupting values.
• The input-template plumbing never worked: SetSecret, DeleteSecret and ListSecrets
  each guarded on their own InputTemplate but rendered Get's, Metadata gated on
  List's, and the value was never exposed to input templates at all — so the shipped
  pass example's stdin-based set has never functioned.
• execute() no longer merges stderr into stdout on success, which was concatenating
  backend warnings (gpg: WARNING: ...) onto the secret value itself.

Data loss

• A zero-length vault file was read as "no vault here", so the constructor initialized and
  immediately overwrote it — every secret gone, success reported. Now ErrVaultCorrupt,
  file left untouched.
• Mutations take a cross-process advisory lock and re-load inside it. The per-instance
  RWMutex never spanned two providers, let alone two processes.
• The temp file had a fixed <path>.tmp name written with os.WriteFile, so concurrent
  savers truncated each other and a planted symlink was followed with 0600 silently not
  applied. Now os.CreateTemp + O_EXCL, explicit chmod, fsync before rename, dir fsync after.

Crypto

• DeriveKey tested salt == nil, but []byte("") from a variable string is non-nil, so
  the natural "generate me a salt" call ran scrypt unsalted — a deterministic,
  precomputable KEK identical for every user with the same passphrase.
• aes.NewCipher accepts 16/24/32 bytes, so a short key silently downgraded an "AES256"
  vault to AES-128/192. Now exactly 32 bytes.
• Salts carry the parameters that produced them, so future cost changes cannot silently
  change an existing key.

Crashes, validation, lifecycle

• Every accessor guarded against nil state instead of panicking after Close().
• Vault IDs are charset-validated and the resolved path is proved to stay inside the storage
  directory. filepath.Clean resolves traversal rather than sanitizing it:
  Clean("vault-../../../tmp/evil.enc") is "../tmp/evil.enc".
• Keyring entry names are length-prefixed; "%s-secret-%s" let vault a/key b-secret-c
  collide with vault a-secret-b/key c.
• New() returns a genuinely nil Provider on error rather than a nil pointer in a
  non-nil interface.
• AgeVault refuses to create a vault, or remove a recipient, that would leave no recipient
  matching your own identity — one mistyped public key previously produced a permanently
  unreadable vault.
• expandEnv no longer mutates the shared config map under a read lock (an unrecoverable
  concurrent map writes fault, guaranteed to recur with the pass example).
• HasSecret no longer re-enters RLock, which deadlocks if a writer arrives in between.

Examples

• Three of the four shipped configs were rejected by the new validation; the fourth had
  never worked. All rewritten, plus correctness fixes independent of the injection issue —
  bw get item matches by substring (so keys api and api_key broke each other), AWS
  used a listSeparator key that is not the field name (separator) so it was never
  applied, and pass's exists decrypted via GPG just to answer a boolean.
• New TestShippedExampleProvidersAreUsable loads every config and renders all six
  operations. Nothing exercised these files before, which is how they drifted; it caught two
  broken output templates during this work.

Tests

• The previous mock ignored both cmd and input and returned the first value in a map, so
  no test could observe a wrongly-rendered template — precisely why these bugs shipped.
  Replaced with a capturing mock. 126 tests pass under -race.

Compatibility

Breaking changes are documented in a new "Upgrading to v0.3.0" table in the README. Local
vault files written by earlier versions are read without migration.

Downstream cost was measured, not estimated: pointing flowexec/flow at this branch breaks
exactly 3 call sites, all from the Metadata() signature — internal/vault/demo.go:64
and internal/io/vault/view.go:226-227.

One audit finding was deliberately not fixed, and is documented rather than papered
over: SecureBytes/Zero() cannot protect the provider state maps, which hold immutable Go
strings whose bytes are neither addressable nor zeroable, and PlainTextString() hands out
more un-zeroable copies by design. Converting the maps to []byte would change the on-disk
format of every local vault to buy protection the next API call gives away again.

Change Type

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

🤖 Generated with Claude Code

https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p