Error messages leak internals and offer no recovery path for corrupted key files #47

New Issue

clawbot · 2026-08-09T03:43:37+02:00

clawbot commented

2026-08-09 03:43:37 +02:00

From the 1.0 security survey. Lowest-severity of the security-adjacent items, but it is the difference between a recoverable vault and a user who believes their secrets are gone.

Problem

No recovery guidance. Only two places in the codebase suggest a next step: internal/cli/init.go:145-147 ("Run 'secret generate mnemonic' ...") and internal/cli/unlockers.go:486 ("Run 'secret unlocker add passphrase' to create one."). Every other failure surfaces as a bare wrapped I/O error.

A corrupt or missing priv.age, longterm.age, pub.age, metadata.age, or current produces messages like failed to read current version file: ... (internal/secret/version.go:534, and similarly at :211, :239, :295, :333, :413; secret.go:293; passphraseunlocker.go:52; pgpunlocker.go:97). None of them mentions the single most important fact available: the vault is recoverable from the BIP39 mnemonic. A user who hits this sees an I/O error and reasonably concludes the data is lost, when secret vault import would restore access.

Internal detail in user-facing errors. Raw filesystem paths and Go-level failures are wrapped through to the user throughout — failed to open file %s: %w (internal/cli/secrets.go:615), keychain item not found: %s with the item name (keychainunlocker.go:543). Severity is genuinely low for a local single-user CLI, since the leaked data is the user's own paths, but the messages are inconsistent in voice and detail level, which makes the important ones harder to spot.

The plumbing to fix this already exists: internal/vault/errors.go defines sentinel errors, and PR #29's err113 refactor extracted many more.

Definition of done

Failures to read or decrypt vault key material produce an error that states what could not be read, and names the recovery path — secret vault import with the mnemonic — where that recovery genuinely applies.
Recovery guidance is only offered where it is actually true. Suggesting mnemonic recovery for a failure it cannot fix is worse than saying nothing.
User-facing messages are consistent in voice and do not include Go type names, struct internals, or stack detail. Filesystem paths may stay where they help the user find the problem.
Errors keep their wrapped causes for errors.Is/errors.As. Do not flatten error chains to make messages prettier — the sentinel-based control flow depends on them, and breaking it silently changes which failures callers treat as fatal.
Detailed internals remain available under the existing debug logging (internal/secret/debug.go, the GODEBUG flag), so nothing is lost for diagnosis.
Tests pin the message text for the main recoverable-failure paths, so this does not regress.
make check green. TODO.md updated in the same commit.

Implementation requirements

Sequence after PR #29, which already rewrote a large number of these error sites. Doing it first guarantees conflicts.
Do not invent new sentinel errors where internal/vault/errors.go already has one.
Be careful that added guidance is not misleading for the keychain and Secure Enclave unlockers, where the recovery story differs — a hardware-backed key that is gone is gone, and only the mnemonic path survives.
Those two unlocker files are Darwin-gated and are not compiled on the Linux CI runner. If their messages change, state how that was verified.
Resist scope creep into rewording every message in the codebase. Target the failures where a user could reasonably conclude their data is unrecoverable when it is not — that is the whole point of this issue.

From the 1.0 security survey. Lowest-severity of the security-adjacent items, but it is the difference between a recoverable vault and a user who believes their secrets are gone. ## Problem **No recovery guidance.** Only two places in the codebase suggest a next step: `internal/cli/init.go:145-147` ("Run 'secret generate mnemonic' ...") and `internal/cli/unlockers.go:486` ("Run 'secret unlocker add passphrase' to create one."). Every other failure surfaces as a bare wrapped I/O error. A corrupt or missing `priv.age`, `longterm.age`, `pub.age`, `metadata.age`, or `current` produces messages like `failed to read current version file: ...` (`internal/secret/version.go:534`, and similarly at `:211`, `:239`, `:295`, `:333`, `:413`; `secret.go:293`; `passphraseunlocker.go:52`; `pgpunlocker.go:97`). None of them mentions the single most important fact available: **the vault is recoverable from the BIP39 mnemonic.** A user who hits this sees an I/O error and reasonably concludes the data is lost, when `secret vault import` would restore access. **Internal detail in user-facing errors.** Raw filesystem paths and Go-level failures are wrapped through to the user throughout — `failed to open file %s: %w` (`internal/cli/secrets.go:615`), `keychain item not found: %s` with the item name (`keychainunlocker.go:543`). Severity is genuinely low for a local single-user CLI, since the leaked data is the user's own paths, but the messages are inconsistent in voice and detail level, which makes the important ones harder to spot. The plumbing to fix this already exists: `internal/vault/errors.go` defines sentinel errors, and PR #29's `err113` refactor extracted many more. ## Definition of done - Failures to read or decrypt vault key material produce an error that states what could not be read, and names the recovery path — `secret vault import` with the mnemonic — where that recovery genuinely applies. - Recovery guidance is only offered where it is actually true. Suggesting mnemonic recovery for a failure it cannot fix is worse than saying nothing. - User-facing messages are consistent in voice and do not include Go type names, struct internals, or stack detail. Filesystem paths may stay where they help the user find the problem. - Errors keep their wrapped causes for `errors.Is`/`errors.As`. **Do not flatten error chains to make messages prettier** — the sentinel-based control flow depends on them, and breaking it silently changes which failures callers treat as fatal. - Detailed internals remain available under the existing debug logging (`internal/secret/debug.go`, the `GODEBUG` flag), so nothing is lost for diagnosis. - Tests pin the message text for the main recoverable-failure paths, so this does not regress. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Sequence **after** PR #29, which already rewrote a large number of these error sites. Doing it first guarantees conflicts. - Do not invent new sentinel errors where `internal/vault/errors.go` already has one. - Be careful that added guidance is not misleading for the keychain and Secure Enclave unlockers, where the recovery story differs — a hardware-backed key that is gone is gone, and only the mnemonic path survives. - Those two unlocker files are Darwin-gated and are not compiled on the Linux CI runner. If their messages change, state how that was verified. - Resist scope creep into rewording every message in the codebase. Target the failures where a user could reasonably conclude their data is unrecoverable when it is not — that is the whole point of this issue.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:37 +02:00

clawbot referenced this issue

2026-08-09 03:46:20 +02:00

Tests assert on error message fragments, never on sentinel identity — error-handling regressions pass silently #49

clawbot referenced this issue

2026-08-09 03:46:49 +02:00

Darwin-only code is never compiled, linted, or tested by CI, including both hardware-backed unlockers #50

Sign in to join this conversation.