[keyman] audit + remediation plan, and phase 1: CLI error boundary
docs/AUDIT.md and docs/PLAN.md record the review and the ten phases it turns into. This commit is phase 1. keyman.cli.ts fell through to an interactive session for --help, ignored unknown flags, and called keyman() unawaited — so Ctrl-C at any prompt, and any rejection inside the menu loop, became an unhandled-rejection stack trace. flagValue() also read `--channel --force` as the channel "--force", which reached the dist-tag lookup as a key that cannot exist and reported an unreachable registry. New keyman.args.ts owns the parse: both --flag value and --flag=value, a UsageError for an unknown flag or command, --channel validated against the three real channels, and self-update-only flags rejected rather than silently ignored. It is a separate module because cli.ts is excluded from coverage and these are rules, not wiring. --help short-circuits before tokenising, so it answers a line the parser would otherwise reject. Usage errors exit 2; ExitPromptError is caught by name (@inquirer/core is transitive here and does not resolve) and prints Goodbye. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,656 @@
|
||||
# keyman audit
|
||||
|
||||
A review of `packages/keyman` for defects, unimplemented features, and drift
|
||||
between the code and the documents that describe it.
|
||||
|
||||
Severity is about what it costs a user:
|
||||
|
||||
- **🔴 broken** — normal use produces a crash, data loss, or a silently wrong result.
|
||||
- **🟠 misleading** — the code or a document states something that is not true.
|
||||
- **🟡 gap** — something real that nothing mentions, or dead weight nobody uses.
|
||||
|
||||
Verified against `75983ab` with no uncommitted changes in the package. Line
|
||||
numbers are from that state. Findings marked **verified** were reproduced by
|
||||
running the code, not inferred from reading it; the reproduction is quoted.
|
||||
|
||||
Baseline: 162 tests pass, 98.9 % lines / 96.2 % branches. High coverage is
|
||||
context for §1.2, not a defence of it.
|
||||
|
||||
---
|
||||
|
||||
## Contents
|
||||
|
||||
- [1. Defects](#1-defects)
|
||||
- [2. Security](#2-security)
|
||||
- [3. Unimplemented and dead](#3-unimplemented-and-dead)
|
||||
- [4. Public API and packaging](#4-public-api-and-packaging)
|
||||
- [5. Documentation drift](#5-documentation-drift)
|
||||
- [6. Checked and accurate](#6-checked-and-accurate)
|
||||
- [Suggested order of attack](#suggested-order-of-attack)
|
||||
|
||||
---
|
||||
|
||||
## 1. Defects
|
||||
|
||||
### 1.1 🔴 `keysDir` and `tmpDir` are honoured by half the tool
|
||||
|
||||
`resolveConfigPaths()` (`keyman.config.ts:249-259`) resolves all four paths from
|
||||
the config, and `keyman.main.ts:18-21` prints them. But dispatch is inconsistent
|
||||
about what it hands each operation:
|
||||
|
||||
| Operation | receives | uses |
|
||||
| --- | --- | --- |
|
||||
| `listKeys` (`main.ts:67`) | `paths.keysDir` | the configured directory |
|
||||
| `generateKey` (`main.ts:73`) | `paths.keysDir`, `paths.tmpDir` | the configured directories |
|
||||
| `copyKey` (`main.ts:70`) | `paths.tmpDir` | the configured directory |
|
||||
| `encryptKeys` (`main.ts:76`) | `paths.vaultRoot` | **hardcoded** `<vaultRoot>/keys` (`encrypt.ts:38`) |
|
||||
| `decryptKeys` (`main.ts:84`) | `paths.vaultRoot` | **hardcoded** `<vaultRoot>/keys` (`decrypt.ts:7`) and `<vaultRoot>/tmp` (`decrypt.ts:39,43`) |
|
||||
|
||||
With the defaults the two halves agree, which is why this is invisible. Set
|
||||
either sub-directory and the vault splits in two.
|
||||
|
||||
**Verified.** With `{"vaultRoot":"./v","keysDir":"encrypted","tmpDir":"plain"}`,
|
||||
`keyman --print-config` reports:
|
||||
|
||||
```json
|
||||
{"...":"...","keysDir":"…/v/encrypted","tmpDir":"…/v/plain","keyPath":"…/v/age.key"}
|
||||
```
|
||||
|
||||
so `generate` writes to `v/encrypted/<name>/` and `list` scans `v/encrypted`,
|
||||
while `encrypt` writes to `v/keys`, and `decrypt` reads `v/keys` and writes
|
||||
`v/tmp` — never touching either configured directory. Concretely:
|
||||
|
||||
- encrypt a key, then list it → the list shows nothing in `[Vault]`.
|
||||
- generate a key, then decrypt it → "⚠️ No encrypted keys found."
|
||||
- decrypt to local, then encrypt → the key is not offered, because `encrypt`
|
||||
reads the configured `tmpDir` while `decrypt` wrote to the hardcoded one.
|
||||
|
||||
No error at any point. The user has two vaults and one of them is invisible to
|
||||
whichever operation they try next.
|
||||
|
||||
The current behaviour is locked in by tests: `main.test.ts:164-187` asserts
|
||||
`vaultRoot` is what encrypt and decrypt receive ("encrypts keys into the vault
|
||||
root"), and `encrypt.test.ts:95` / `decrypt.test.ts:53` assert the literal
|
||||
`keys` segment. Fixing this means changing those assertions.
|
||||
|
||||
**Fix.** Pass `paths.keysDir` and `paths.tmpDir` into `encryptKeys` and
|
||||
`decryptKeys` and delete the three `path.join(vaultDir, 'keys' | 'tmp')` calls.
|
||||
Neither function has a use for `vaultRoot` once that is done, so the parameter
|
||||
goes away rather than becoming a second source of truth.
|
||||
|
||||
See also §5.1 — `DOCS-AUDIT.md` currently lists this layout under *checked and
|
||||
accurate*.
|
||||
|
||||
### 1.2 🔴 Encrypt and decrypt crash with a raw stack trace on a first run
|
||||
|
||||
Three `readdirSync` calls have no `existsSync` guard:
|
||||
|
||||
- `encrypt.ts:13` — `~/.ssh`, which nothing creates.
|
||||
- `encrypt.ts:16` — the tmp directory.
|
||||
- `decrypt.ts:8` — `<vault>/keys`, which nothing creates either.
|
||||
|
||||
`keyman.main.ts:40-41` creates `vaultRoot` and `tmpDir`. It does **not** create
|
||||
`keysDir`, so `decrypt` on a fresh vault throws instead of printing its
|
||||
"⚠️ No encrypted keys found." message — the message is unreachable until the
|
||||
directory exists for some other reason.
|
||||
|
||||
**Verified**, calling both functions directly against a vault laid out the way
|
||||
`main.ts` lays it out:
|
||||
|
||||
```
|
||||
--- A: decryptKeys with no vault/keys directory ---
|
||||
THREW: Error ENOENT ENOENT: no such file or directory, scandir '…/vault/keys'
|
||||
--- B: encryptKeys with no ~/.ssh directory ---
|
||||
THREW: Error ENOENT ENOENT: no such file or directory, scandir '…/home/.ssh'
|
||||
```
|
||||
|
||||
What the user sees is worse than the exception, because of `keyman.cli.ts:82`:
|
||||
|
||||
```ts
|
||||
keyman();
|
||||
```
|
||||
|
||||
Not awaited, no `.catch`. Any rejection anywhere in the menu loop becomes an
|
||||
unhandled rejection: Node prints the stack and exits non-zero, and the menu loop
|
||||
— whose whole point (`README.md:97`) is that you can run several operations in
|
||||
one session — is gone.
|
||||
|
||||
`copyKey` guards (`copy.ts:8`) and `listKeys` guards all three of its
|
||||
directories (`list.ts:22,50,78`). Encrypt and decrypt are the outliers, not the
|
||||
rule.
|
||||
|
||||
Worth noting where the coverage numbers sit: `keyman.encrypt.ts` and
|
||||
`keyman.decrypt.ts` are both at **100 % lines, 100 % branches**. Every test
|
||||
creates the directories in `beforeEach` (`encrypt.test.ts:46-47`,
|
||||
`decrypt.test.ts:54-55`), so the missing guard is not a branch that went
|
||||
uncovered — it is a branch that was never written. Line coverage measures lines
|
||||
executed, not inputs considered.
|
||||
|
||||
**Half closed (Phase 1).** `keyman()` is now awaited inside a `catch`, so a
|
||||
rejection is one line rather than an unhandled-rejection stack trace. The missing
|
||||
`existsSync` guards — and with them the menu loop surviving a failed operation —
|
||||
are Phase 2.
|
||||
|
||||
### 1.3 🔴 A missing `age.key` becomes `age -r null`
|
||||
|
||||
`keyman.main.ts:73` and `:80` assert away a null:
|
||||
|
||||
```ts
|
||||
extractAgePublicKey(paths.keyPath)!
|
||||
```
|
||||
|
||||
`extractAgePublicKey` returns `string | null` (`utils.ts:8-22`) and returns null
|
||||
in three cases: the file is missing, it is unreadable, or it parses but has no
|
||||
`# public key:` line. In all three it prints an error and returns — and the
|
||||
non-null assertion carries that null straight into an `execa` argv.
|
||||
|
||||
**Verified**, both halves:
|
||||
|
||||
```
|
||||
❌ ERROR: Age key file not found at /nope/age.key
|
||||
extractAgePublicKey(missing) = null
|
||||
execa with null recipient THREW: ExecaError | Command failed with exit code 1: age -r null -o /tmp/x.age /etc/hosts
|
||||
```
|
||||
|
||||
execa stringifies the null, so the recipient becomes the literal `"null"`.
|
||||
|
||||
The two call sites fail differently, and the generate path fails worse:
|
||||
|
||||
- `generateKey` runs `ssh-keygen` **first** (`generate.ts:59`) and `age` second
|
||||
(`generate.ts:68`). Its `try/catch` swallows the failure into "❌ Error
|
||||
generating/encrypting key", but by then the private key is on disk in `tmpDir`
|
||||
in plaintext, and the user has been told the operation failed. Nothing tells
|
||||
them a key was left behind.
|
||||
- `encryptKeys` has no `try/catch` at all, so it takes the §1.2 path: unhandled
|
||||
rejection, stack trace, session over.
|
||||
|
||||
**Fix.** Resolve the recipient once, before dispatch, and treat null as a
|
||||
recoverable condition: print what to run (`age-keygen -o <keyPath>`) and return
|
||||
to the menu. The type already says this is possible; the `!` is the only thing
|
||||
claiming otherwise.
|
||||
|
||||
### 1.4 🔴 Decrypting into `~/.ssh` silently overwrites an existing key
|
||||
|
||||
`decrypt.ts:47-49` writes the decrypted key and copies the public key with no
|
||||
existence check, no confirmation, and no backup.
|
||||
|
||||
**Verified** that `age -o` does not refuse an existing file:
|
||||
|
||||
```
|
||||
before: PRECIOUS EXISTING KEY
|
||||
age -o exit=0 (overwrote)
|
||||
after: secret
|
||||
```
|
||||
|
||||
So selecting `prod` with the `SSH (~/.ssh)` destination replaces
|
||||
`~/.ssh/id_prod` outright. If the vault copy is stale, or the folder name
|
||||
happens to collide with an unrelated local key, the local key is gone — and this
|
||||
is the one operation in the tool that writes outside the vault, into the
|
||||
directory the user's actual SSH access depends on.
|
||||
|
||||
The `Local (vault/tmp)` destination has the same behaviour but a much lower cost,
|
||||
since `vault/tmp` is scratch space by design.
|
||||
|
||||
**Fix.** Check both output paths before decrypting anything and prompt per
|
||||
collision, or refuse and name the file. A `--force` equivalent can come later;
|
||||
the current default should not be "overwrite".
|
||||
|
||||
### 1.5 🟠 `age` or `ssh-keygen` missing is unhandled in encrypt and decrypt
|
||||
|
||||
Same missing `try/catch` as §1.3. `generateKey` (`generate.ts:51-76`) and
|
||||
`copyKey` (`copy.ts:43-57`) both wrap their `execa` calls and report a failure;
|
||||
`encryptKeys` and `decryptKeys` do not. On a machine without `age` on `PATH` —
|
||||
the one hard external requirement, per `CLAUDE.md` — choosing Encrypt from the
|
||||
menu produces an `ENOENT` stack trace rather than "install age".
|
||||
|
||||
### 1.6 🟠 Encrypt copies `.pub` unconditionally and aborts the batch midway
|
||||
|
||||
`encrypt.ts:45`:
|
||||
|
||||
```ts
|
||||
fs.copyFileSync(`${keyPath}.pub`, path.join(vaultPath, `${key}.pub`));
|
||||
```
|
||||
|
||||
The selection list is built from *private* keys only (`encrypt.ts:14,17` filter
|
||||
out `.pub`), so a private key with no `.pub` sibling is offered — and that is a
|
||||
legal state, since `ssh-keygen -y` regenerates a public key on demand and people
|
||||
do delete them.
|
||||
|
||||
When it happens, `age` has already written the `.age` file, so the throw leaves
|
||||
the vault holding an encrypted key with no public key. Worse, the throw escapes
|
||||
the `for` loop: every remaining selected key is skipped, with no output saying
|
||||
so, and the process dies via §1.2.
|
||||
|
||||
`generate.ts:71` has the same shape but is far less likely to fire, since
|
||||
`ssh-keygen` just wrote the file.
|
||||
|
||||
**Fix.** Derive the public key with `ssh-keygen -y -f <key>` when the sibling is
|
||||
absent, and wrap the loop body so one bad key costs one key rather than the
|
||||
batch.
|
||||
|
||||
### 1.7 🟠 `/home/<user>` is hardcoded
|
||||
|
||||
`keyman.main.ts:33`:
|
||||
|
||||
```ts
|
||||
const homeDir = user === '@current' ? process.env.HOME || '' : `/home/${user}`;
|
||||
```
|
||||
|
||||
On macOS other users live under `/Users/`, and this tool is otherwise
|
||||
macOS-specific (§1.9). Nothing checks the directory exists, so a wrong guess
|
||||
feeds a nonexistent `sshDir` into §1.2 rather than into an error message.
|
||||
`main.test.ts:198-204` locks in `/home/deploy/.ssh`.
|
||||
|
||||
**Fix.** `os.userInfo()` for the current user, and for a named user either look
|
||||
the home directory up (`getent passwd` / `dscl`) or ask for the path outright.
|
||||
Failing that, check `existsSync` and say so.
|
||||
|
||||
### 1.8 🟡 Keys not named `id_*` are invisible, silently
|
||||
|
||||
Every discovery filter requires the prefix: `copy.ts:9`, `encrypt.ts:14,17`,
|
||||
`list.ts:23,51`, and `decrypt.ts:9` reconstructs `id_${dir}`. A key called
|
||||
`deploy_ed25519` cannot be listed, copied, or encrypted, and nothing says why —
|
||||
it simply is not in the menu.
|
||||
|
||||
`generateKey` enforces the prefix (`generate.ts:43`), so keys keyman creates are
|
||||
always fine. The gap only bites pre-existing keys, which is exactly the
|
||||
population a key manager is adopted to take over.
|
||||
|
||||
### 1.9 🟡 `pbcopy` is hardcoded
|
||||
|
||||
`copy.ts:49`, with the comment above it admitting the shortcut:
|
||||
|
||||
```ts
|
||||
// Since the environment is Darwin, we prioritize pbcopy, but we can add others for completeness
|
||||
```
|
||||
|
||||
On Linux or Windows, Copy public key always fails. It fails *cleanly* — the
|
||||
`try/catch` reports "❌ Failed to copy to clipboard" — but the package declares
|
||||
only `"node": ">=22"` in `engines` and the README says nothing, so nothing warns
|
||||
before install. `xclip`/`wl-copy`/`clip.exe` by platform is a handful of lines;
|
||||
alternatively print the key to stdout as a fallback so the operation is never a
|
||||
dead end.
|
||||
|
||||
### 1.10 🟡 Smaller things
|
||||
|
||||
- **`listKeys` throws on a broken symlink.** `list.ts:80` calls `fs.statSync` on
|
||||
every entry in the keys directory; a dangling symlink throws `ENOENT`, and
|
||||
`listKeys` has no `try/catch`, so it exits via §1.2. `lstatSync`, or a
|
||||
`withFileTypes` readdir, or a guard.
|
||||
- **`key.replace('id_', '')` is unanchored** (`encrypt.ts:38`, `generate.ts:63`).
|
||||
Every input is prefix-filtered today, so the first match *is* the prefix and
|
||||
the behaviour is correct — it is a trap left for whoever loosens §1.8.
|
||||
`replace(/^id_/, '')` costs nothing.
|
||||
- **An `age` failure leaves an empty vault directory.** `generate.ts:65` creates
|
||||
`<keysDir>/<name>/` before `generate.ts:68` runs `age`.
|
||||
`generate.test.ts:150-163` asserts the `.pub` is absent afterwards but not the
|
||||
directory, so this passes today. It makes the folder show up in
|
||||
`decrypt`'s scan as a candidate that filters back out — harmless, untidy.
|
||||
- **Debug output still in shipped code.** `encrypt.ts:18-19`
|
||||
(`console.log(tmpKeys); console.log(sshKeys);`) is already tracked as
|
||||
`DOCS-AUDIT.md` §6.4. `decrypt.ts:10` — a `console.log(keyfile)` *inside a
|
||||
`filter` callback*, printing one line per vault directory — is not, and is the
|
||||
more visible of the two.
|
||||
|
||||
---
|
||||
|
||||
## 2. Security
|
||||
|
||||
### 2.1 🔴 Decrypted private keys are world-readable before the chmod
|
||||
|
||||
`decrypt.ts:47-50` decrypts, then copies, then chmods — in three separate
|
||||
processes:
|
||||
|
||||
```ts
|
||||
await execa('age', ['-d', '-i', ageKey, '-o', privateKeyOut, encryptedKey]);
|
||||
await execa('cp', [publicKey, publicKeyOut]);
|
||||
await execa('chmod', ['600', privateKeyOut]);
|
||||
```
|
||||
|
||||
**Verified** what `age` creates, and what `mkdirSync` at `main.ts:41` creates:
|
||||
|
||||
```
|
||||
-rw-r--r-- …/out ← the decrypted private key, as age leaves it
|
||||
drwxr-xr-x …/tmpdir ← vault/tmp, as keyman creates it
|
||||
```
|
||||
|
||||
So a plaintext private key exists at `0644` for the lifetime of two process
|
||||
spawns, inside a `0755` directory any local user can traverse. If the `chmod`
|
||||
fails or the process is killed in between, it stays `0644` — and because
|
||||
`decryptKeys` has no `try/catch` (§1.5), a failing `chmod` also kills the
|
||||
session before the next key is even attempted.
|
||||
|
||||
**Fix.** `fs.chmodSync` immediately after `age` returns rather than a third
|
||||
spawn; create `tmpDir` with `{mode: 0o700}` and `~/.ssh` likewise if it is
|
||||
missing. Replacing `cp` and `chmod` with `fs.copyFileSync` / `fs.chmodSync` also
|
||||
removes two shell-outs that do not work on Windows and cuts three spawns per key
|
||||
to one.
|
||||
|
||||
### 2.2 🟠 The passphrase is passed on the `ssh-keygen` command line
|
||||
|
||||
`generate.ts:53`:
|
||||
|
||||
```ts
|
||||
const args = ['-t', algorithm, '-f', keyPath, '-N', password, '-C', identity];
|
||||
```
|
||||
|
||||
argv is world-readable on both Linux (`/proc/<pid>/cmdline`) and macOS
|
||||
(`ps -o command`) for the lifetime of the process. Any other user on the machine
|
||||
can read the passphrase of a key being generated. `generate.test.ts:78-87`
|
||||
asserts this exact argv.
|
||||
|
||||
**Fix.** Omit `-N` entirely and let `ssh-keygen` prompt on the tty — it already
|
||||
asks twice and confirms, so keyman's own password prompt (`generate.ts:26-33`)
|
||||
can go away rather than being replaced. That keeps the passphrase off argv
|
||||
without keyman ever holding it.
|
||||
|
||||
### 2.3 🟠 The age recipient is trusted from a comment, never verified
|
||||
|
||||
`extractAgePublicKey` (`utils.ts:16`) regexes the recipient out of a comment
|
||||
line in the identity file:
|
||||
|
||||
```ts
|
||||
fileContents.match(/^# public key:\s*(age1[^\s]+)/m)
|
||||
```
|
||||
|
||||
Nothing checks it corresponds to the private key in that same file. Edit the
|
||||
comment — or concatenate two key files — and every subsequent encryption goes to
|
||||
a recipient the local identity cannot decrypt. The failure surfaces only later,
|
||||
at decrypt time, on keys that may no longer exist in plaintext anywhere.
|
||||
|
||||
**Fix.** `age-keygen -y <keyPath>` derives the public key *from the private key*
|
||||
and is exactly the tool for this. Note that `age-keygen` is currently not
|
||||
invoked anywhere in the source, despite `CLAUDE.md` listing it among the
|
||||
binaries keyman shells out to (§5.5).
|
||||
|
||||
### 2.4 🟡 Nothing manages the plaintext left in `vault/tmp`
|
||||
|
||||
Decrypted keys accumulate in `vault/tmp` indefinitely. There is no shred
|
||||
operation, no warning on exit, and keyman never writes the `.gitignore` its own
|
||||
README (`README.md:25-26`, `:148`) tells the user to write by hand. The only
|
||||
signal is the 🔓 marker in `listKeys`, which the user has to go looking for.
|
||||
|
||||
A "Clear decrypted keys" menu entry and a `.gitignore` written alongside the
|
||||
vault on first run would cost little and close the most likely way a private key
|
||||
reaches a public repository — which is the threat this tool exists to address.
|
||||
|
||||
---
|
||||
|
||||
## 3. Unimplemented and dead
|
||||
|
||||
### 3.1 🟠 There is no `--help`
|
||||
|
||||
`keyman.cli.ts` handles `--print-config`, `--version`/`-V`, and
|
||||
`self-update`/`upgrade`, then falls through to the interactive session. `--help`
|
||||
is not among them, and neither is any unknown-flag handling.
|
||||
|
||||
**Verified.** `keyman --help` with no tty:
|
||||
|
||||
```
|
||||
📁 Vault Root: …
|
||||
? Specify USER (default: @current): (@current)
|
||||
…/@inquirer/core/dist/lib/create-prompt.js:67
|
||||
reject(new ExitPromptError(`User force closed the prompt with ${code} ${signal}`));
|
||||
```
|
||||
|
||||
Two problems in one output. `--help` starts a session instead of describing the
|
||||
tool, and because of §1.2 the resulting `ExitPromptError` is an unhandled
|
||||
rejection with a stack trace. That second half is what a user gets from **Ctrl-C
|
||||
at any prompt** — the normal way to leave an interactive CLI produces a crash
|
||||
dump.
|
||||
|
||||
`keyman --vault foo` is likewise accepted and ignored.
|
||||
|
||||
**Fix.** `--help` listing the flags, the two subcommands, and the `KEYMAN_*`
|
||||
environment variables (§5.3); an unknown-flag error; and a `catch` in
|
||||
`keyman.cli.ts` that treats `ExitPromptError` as "goodbye" and anything else as
|
||||
a one-line error. nopy uses Commander for this; keyman need not, but it does
|
||||
need the behaviour.
|
||||
|
||||
**Closed (Phase 1).** `src/keyman.args.ts` owns the parse and the help text; the
|
||||
`catch` around `keyman()` in `keyman.cli.ts` turns `ExitPromptError` into
|
||||
"👋 Goodbye!" and exit 0, and anything else into one line and exit 1.
|
||||
|
||||
### 3.2 🟡 `flagValue` accepts things that are not values
|
||||
|
||||
`keyman.cli.ts:24-27` is `args.indexOf(name)` and `args[index + 1]`:
|
||||
|
||||
- `--channel=main` is not recognised.
|
||||
- `--channel` as the last argument yields `undefined`.
|
||||
- `keyman self-update --channel --force` sets the channel to `"--force"`, which
|
||||
is cast to `Channel` (`cli.ts:47`) and flows into the dist-tag lookup at
|
||||
`update.ts:176` as a key that cannot exist. The registry answers, the tag is
|
||||
absent, and the user is told "Could not reach <registry>" — which is false.
|
||||
|
||||
Validating against the three legal channels would turn all three into one clear
|
||||
error.
|
||||
|
||||
**Closed (Phase 1).** `parseArgs` accepts both `--flag value` and `--flag=value`,
|
||||
rejects a flag swallowed as another flag's value, and validates `--channel`
|
||||
against `CHANNELS`.
|
||||
|
||||
### 3.3 🟡 The `resolution` merge machinery has no effect
|
||||
|
||||
`keyman.config.ts` carries `ResolutionStrategy`, `KeymanResolutionConfig`,
|
||||
`mergeValue` and `mergeConfigs` — roughly 45 lines, imported from nopy's design.
|
||||
Every property in `KeymanConfigSchema` is a `z.string()`. For two strings,
|
||||
`mergeValue` returns `childValue` in the `override` branch (`:121-123`) and
|
||||
returns `childValue` again from the primitive fallthrough (`:156`). The two
|
||||
strategies are indistinguishable for every key the schema permits, and the
|
||||
array-concat and deep-merge branches are unreachable through a valid config —
|
||||
unknown keys pass through the merge but are then stripped by
|
||||
`KeymanConfigSchema.parse` (§3.5).
|
||||
|
||||
So the documented knob does nothing. The doc comment at `:186-194` advertises it:
|
||||
|
||||
```json
|
||||
{ "vaultRoot": "../vault", "resolution": { "vaultRoot": "override" } }
|
||||
```
|
||||
|
||||
and `config.test.ts:212` — "honours an explicit override strategy" — passes for
|
||||
a case where plain merge gives the same answer, so the test does not distinguish
|
||||
them either.
|
||||
|
||||
This is a choice to make, not a bug to fix. Either drop the machinery and the
|
||||
comment, or keep it deliberately as the shape a future object-valued or
|
||||
array-valued option would need — and say so in a comment, since right now it
|
||||
reads as functional.
|
||||
|
||||
### 3.4 🟡 `getConfigPaths()` is exported, tested, and called by nothing
|
||||
|
||||
`keyman.config.ts:265` is used only by `config.test.ts:122,131`. It is not
|
||||
re-exported from `src/index.ts` and not called by the CLI. nopy's equivalent
|
||||
feeds `nopy.main.ts:64`.
|
||||
|
||||
The absence is felt: `--print-config` prints the *resolved paths* only, so there
|
||||
is no way to ask which config files were consulted. That information exists only
|
||||
as a stderr side effect of `loadConfig` ("✅ Loaded configuration from …"), which
|
||||
is not machine-readable and is interleaved with warnings. Folding
|
||||
`getConfigPaths()` into the `--print-config` JSON makes the function earn its
|
||||
keep and makes the escape hatch answer the question it is for.
|
||||
|
||||
### 3.5 🟡 A typo in `.keymanrc.json` is silent
|
||||
|
||||
`KeymanConfigSchema` is a plain `z.object`, which strips unknown keys.
|
||||
|
||||
**Verified.** With `{"vaultRoot":"./v","vaultroot":"typo", …}`, the lowercase key
|
||||
is dropped without a word and `--print-config` reports the vault from the
|
||||
correct key. Had only the typo been present, the user would get the `vault`
|
||||
default and no clue.
|
||||
|
||||
`.strict()` — or keeping the strip and logging the leftover keys as a warning —
|
||||
turns a silently wrong vault into one line of output. Since `loadConfig` already
|
||||
degrades to defaults rather than throwing, a warning fits the module's existing
|
||||
posture better than a hard failure.
|
||||
|
||||
### 3.6 🟠 "Support for key rotation" does not exist
|
||||
|
||||
`README.md:11`. `grep -rn "rotat" packages/keyman/src/` returns nothing. Already
|
||||
tracked as `DOCS-AUDIT.md` §2.10, still open. Rotation is a genuinely useful
|
||||
operation for this tool — generate a replacement, encrypt it, keep the old one
|
||||
until the new one is deployed — so this is worth building rather than deleting.
|
||||
|
||||
### 3.7 🟡 "Copy public key and create README"
|
||||
|
||||
`encrypt.ts:44` says it; no README is written. Suggestively,
|
||||
`decrypt.test.ts:75` places a `README.md` inside the keys directory as a
|
||||
fixture, so a per-key README appears to have been the intent once. Either build
|
||||
it or drop the half of the comment that lies.
|
||||
|
||||
---
|
||||
|
||||
## 4. Public API and packaging
|
||||
|
||||
### 4.1 🟡 A shebang on the library entry point
|
||||
|
||||
`src/index.ts:1` is `#!/usr/bin/env node`. The bin is `dist/keyman.cli.js`
|
||||
(`package.json:28`); `index.ts` is the `exports["."]` target and is only ever
|
||||
imported. nopy's `src/index.ts` has no shebang. Harmless, and a copy-paste
|
||||
artefact.
|
||||
|
||||
### 4.2 🟡 The exported functions' types are not exported
|
||||
|
||||
`src/index.ts:2` exports `loadConfig` and `resolveConfigPaths`. It does not
|
||||
export `KeymanConfig`, `KeymanConfigFile`, `ResolutionStrategy` or
|
||||
`KeymanResolutionConfig`, so a TypeScript consumer cannot name what `loadConfig`
|
||||
returns or what `resolveConfigPaths` takes. This is the same one-line omission
|
||||
`CLAUDE.md` already records for nopy's `CubePackageRef`.
|
||||
|
||||
### 4.3 🟡 `export * from './keyman.main.js'` exports only `keyman()`
|
||||
|
||||
The five operation modules and `extractAgePublicKey` are not on the public
|
||||
surface, so the package is consumable as a library only as "run the entire
|
||||
interactive menu". That may well be intended — but then `loadConfig` and
|
||||
`resolveConfigPaths` being exported is the odd part, since a consumer can obtain
|
||||
the paths and do nothing with them.
|
||||
|
||||
### 4.4 🟡 Update-module constants are half re-exported
|
||||
|
||||
`keyman.update.ts` exports `SCOPE`, `UPDATE_CACHE_DIR`, `UPDATE_CACHE_FILE`,
|
||||
`DEFAULT_FETCH_TIMEOUT_MS` and `DEFAULT_CONFIG_TIMEOUT_MS`; `src/index.ts:12-31`
|
||||
re-exports neither, while re-exporting `DEFAULT_CHECK_INTERVAL_MS` and
|
||||
`NPMJS_REGISTRY`. Pick one rule.
|
||||
|
||||
---
|
||||
|
||||
## 5. Documentation drift
|
||||
|
||||
### 5.1 🟠 `DOCS-AUDIT.md` lists §1.1 under *checked and accurate*
|
||||
|
||||
`DOCS-AUDIT.md:826-827`:
|
||||
|
||||
> **keyman config** — priority (`VAULT_ROOT` > file > defaults), the four default
|
||||
> values, and the vault layout match `keyman.config.ts` and `keyman.encrypt.ts`.
|
||||
|
||||
The first two clauses are correct. The third holds only because
|
||||
`keyman.encrypt.ts` hardcodes `keys` — checking the documented layout against
|
||||
the file that ignores the config is what made §1.1 invisible. The entry should
|
||||
move out of section 7 and point at §1.1.
|
||||
|
||||
### 5.2 🟠 `README.md` operations list — still open
|
||||
|
||||
`DOCS-AUDIT.md` §2.10, re-verified: `README.md:90-96` lists four menu entries;
|
||||
`main.ts:54-61` has six. `Copy public key` and `Generate key` are undocumented —
|
||||
the latter being the only in-tool way to create a key, which is why the Quick
|
||||
Start at `README.md:33` tells the user to run `ssh-keygen` by hand.
|
||||
`README.md:93` says encrypt takes keys "from `vault/tmp/`"; `encrypt.ts:12-20`
|
||||
unions `~/.ssh` and tmp and offers both.
|
||||
|
||||
### 5.3 🟠 The README documents none of the CLI surface
|
||||
|
||||
`README.md` covers the interactive menu and the config file. It does not mention:
|
||||
|
||||
- `self-update` / `upgrade`, `--dry-run`, `--force`, `--channel`, `--registry`
|
||||
- `--version` / `-V`, `--print-config`
|
||||
- `KEYMAN_REGISTRY`, `KEYMAN_REGISTRY_TOKEN`, `KEYMAN_NO_UPDATE_CHECK`,
|
||||
`KEYMAN_PACKAGE_MANAGER`
|
||||
- the once-a-day update check, or that it is disabled when `CI` is set
|
||||
|
||||
`README.PUBLISH.md:552-578` documents all of it, but `package.json:37-41` ships
|
||||
only `dist`, `README.md` and `LICENSE` — so a reader on the registry sees none of
|
||||
it. This is the same shape as the nopy README problem closed as
|
||||
`DOCS-AUDIT.md` §2.9, and keyman is now the worse of the two.
|
||||
|
||||
### 5.4 🟠 The README presents a configurable layout that is half-real
|
||||
|
||||
`README.md:46-70` documents `keysDir` and `tmpDir` as configuration, and
|
||||
`:72-86` draws the default tree. Per §1.1 the first is only half true. Whichever
|
||||
way §1.1 is resolved, this section needs an edit.
|
||||
|
||||
### 5.5 🟡 `CLAUDE.md` names a binary keyman never runs
|
||||
|
||||
> Encryption shells out to `age` / `age-keygen` / `ssh-keygen`, which must be on
|
||||
> `PATH`.
|
||||
|
||||
`age-keygen` appears nowhere in `packages/keyman/src`. It appears in
|
||||
`README.md:22` as a manual setup step, which is presumably where the claim came
|
||||
from. Either note it as a prerequisite the user runs rather than something
|
||||
keyman invokes, or make §2.3 true and turn the claim into fact.
|
||||
|
||||
`CLAUDE.md` also does not mention that `decryptKeys` shells out to `cp` and
|
||||
`chmod` (`decrypt.ts:49-50`) — see §2.1, where the recommendation is to stop.
|
||||
|
||||
### 5.6 ✅ The update module has not drifted from nopy's
|
||||
|
||||
`keyman.update.ts` and `nopy.update.ts` are described in `CLAUDE.md` as "two
|
||||
near-identical copies of one module", the duplication deliberate. Diffed with
|
||||
package names normalised: **every difference is a doc comment.** No behavioural
|
||||
drift at all. The stated risk of the duplication has not materialised; nopy's
|
||||
copy simply carries fuller comments, and porting the better ones over would cost
|
||||
nothing.
|
||||
|
||||
---
|
||||
|
||||
## 6. Checked and accurate
|
||||
|
||||
- **Config precedence.** `VAULT_ROOT` > config file > defaults
|
||||
(`config.ts:249-259`), matching `README.md:59-70`. Verified via
|
||||
`--print-config`.
|
||||
- **Upward traversal and the home-directory config.** `findConfigFiles`
|
||||
(`config.ts:85-110`) collects root-first and de-duplicates the home config
|
||||
when it is also an ancestor (`:105`).
|
||||
- **`loadConfig` never throws.** Invalid JSON is skipped per file (`:220-226`)
|
||||
and a failed final validation degrades to defaults (`:232-241`) — which is the
|
||||
documented difference from nopy's behaviour, and it holds.
|
||||
- **`extractAgePublicKey` is honest about failure.** It returns `null` in every
|
||||
failure mode and prints why; the defect in §1.3 is entirely in the caller's
|
||||
`!`.
|
||||
- **The menu loop.** Returns to the menu after every operation
|
||||
(`main.ts:45-91`), as `README.md:97` says.
|
||||
- **The four default values** and the `id_<name>.age` / `id_<name>.pub` layout
|
||||
inside a per-key folder, as drawn at `README.md:72-86`.
|
||||
- **`listKeys` status logic** (`list.ts:127-128`) matches its legend and the
|
||||
README's, including the 🔓 state.
|
||||
- **The update module**, in full — see §5.6.
|
||||
|
||||
---
|
||||
|
||||
## Suggested order of attack
|
||||
|
||||
**1 — the crashes, together.** §1.2, §1.3, §1.5 and §1.10's `statSync` are all
|
||||
the same shape: an unguarded call in a function with no error boundary, reaching
|
||||
a `keyman()` that is never awaited. One `catch` in `keyman.cli.ts` that
|
||||
distinguishes `ExitPromptError` from a real failure, plus `existsSync` guards and
|
||||
`try/catch` in encrypt and decrypt, closes all of them and most of §3.1's second
|
||||
half. This is the smallest change with the largest effect on what a first run
|
||||
feels like.
|
||||
|
||||
**2 — §1.4 and §2.1.** Both are in `decryptKeys`, both are about writing outside
|
||||
the vault, and one of them destroys data. Replacing `cp`/`chmod` with the `fs`
|
||||
equivalents is part of the same edit.
|
||||
|
||||
**3 — §1.1.** Mechanical, but it changes four test assertions, so it wants to be
|
||||
its own commit. Fix `DOCS-AUDIT.md` §5.1 in the same one.
|
||||
|
||||
**4 — decide on §3.3 and §3.6.** Both are features the documentation claims and
|
||||
the code does not have; both are decisions rather than fixes. Rotation is worth
|
||||
building. The `resolution` machinery probably is not, and deleting it would take
|
||||
`keyman.config.ts` from 267 lines to around 220.
|
||||
|
||||
**5 — §5.2, §5.3 and §5.4** are one rewrite of `README.md`. It is the only
|
||||
document that ships, and it currently describes two thirds of the menu and none
|
||||
of the command line.
|
||||
|
||||
**6 — the rest.** §2.2 (drop the passphrase prompt, let `ssh-keygen` ask), §2.3
|
||||
(`age-keygen -y`), §2.4 (a shred operation), §1.6 through §1.9, §3.2, §3.4,
|
||||
§3.5, and the §4 one-liners.
|
||||
Reference in New Issue
Block a user