docs: all seven audit findings closed; record the deferred L-1 v2 design

Four fixed outright (M-1, M-2, L-4, L-7, L-8), one mitigated with the
derivation change deliberately deferred (L-1), two surfaced rather than
changed because their resolution is frozen for parity (L-2, L-3), and two
resolved as documented-behaviour with the reasoning written down (L-5, L-6).

The L-1 v2 design is now written up in TODO.md rather than left as a one-line
recommendation: keep v1 frozen, add derivation_version alongside the existing
gen_version pattern, domain-separate with an HMAC step rather than new BIP-85
app numbers, land both implementations together with cross-impl fixtures, and
make the upgrade opt-in per entry. Sequenced after the outside review on
purpose.

Also recorded: three defects found while fixing, none of which the audit had
reached — Python's publish_delta manifest republish, the fetch-limit downgrade
path, and merge_index_payloads mutating its argument.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN
This commit is contained in:
thePR0M3TH3ANandClaude Opus 5 committed 2026-08-19 09:11:09 -04:00
1 parent 276aca65b6
commit 4b64d26273
2 files changed
+76 -24

No files matched your search

+51 -16
View File
@@ -251,22 +251,57 @@ the review independent in the sense that matters.
`js/packages/cli/test/passwordPolicy.test.ts`, four cases against Python-computed
values, each also asserted unequal to the pre-fix output; mutation-verified (drop
`basePolicy` from the merge and all four go red).
- [ ] **M-1 — restore can return a stale snapshot when two syncs land in the same second.**
Manifests sort by `created_at` (whole-second Nostr resolution) with the event id as
tie-break, which is arbitrary with respect to time, so the older of two same-second
snapshots wins roughly half the time and `nostr restore` silently drops the entries
created between them. Reproduces as the intermittent failure of "syncs the vault to
the relay and restores after local destruction" (~1 in 4 runs). **This one is a
protocol change, not a local fix:** the fix is a monotonic sequence number inside the
signed manifest, used as the secondary sort key, and it has to land in Python at the
same time or the two implementations will disagree about which snapshot is latest.
Decide the wire format before writing code.
- [ ] **L-1 — BIP-85 app-32 path is shared across password, SSH and PGP derivation.**
Protocol-level, both implementations, needs versioning. Same care as M-1.
- [ ] **L-2 / L-3 — merge-level data loss that is silent rather than wrong:** concurrent
same-id creation on two replicas discards one entry at merge; the 2048-entry
tombstone cap allows deletion replay. Both are arguably working as designed; the fix
for each is to surface the event rather than to change the merge.
- [x] **(fixed 2026-08-19) M-1 — restore could return a stale snapshot when two syncs
landed in the same second.** Manifests now carry `published_ms`, a monotonic
publication timestamp inside the signed JSON, and both implementations order by it.
Python additionally stopped delegating "which snapshot is newest" to the relay (it
asked for `limit(1)` and restored whatever came back). Backwards compatible: a
manifest without the field orders at the start of its `created_at` second. The nostr
CLI test went from ~1-in-4 failures to 6/6 clean runs.
**Found while fixing:** Python's `publish_delta` republishes the manifest and needed
the field too (without it the manifest carrying the newest `delta_since` sorted below
the snapshot it superseded, so readers fetched deltas from a stale watermark), and
raising the fetch limit turned a vestigial fallback loop into a live downgrade path —
now it skips relay noise but refuses to reach past the newest manifest that parses.
- [x] **(mitigated 2026-08-19) L-1 — BIP-85 app-32 is shared across password, SSH and PGP.**
Detection shipped in both implementations (`findDerivationCollisions` /
`find_derivation_collisions`), wired into import, `--inspect`, restore's merge, the
TUI, and a new `util check-derivation`. The lead test in each derives the keys and
compares bytes, so the claim is verified rather than asserted.
**The derivation change is deliberately deferred**, see the v2 design below.
- [x] **(surfaced 2026-08-19) L-2 / L-3 — merge-level loss that is silent rather than
wrong.** Both implementations gained an optional `MergeReport`: same-id conflicts
(naming the kept and discarded entries) and a count of tombstones evicted at the
retention cap. `nostr restore` reports both. The resolution itself is unchanged —
it is frozen for parity — and the collector is write-only, asserted in both
implementations, because if observing could change the outcome it would be a
divergence rather than an observation.
**Found while fixing:** Python's `merge_index_payloads` mutated its `current`
argument (shallow copy sharing the nested entries dict), and an existing idempotency
test was passing only because of that aliasing.
### L-1 v2 derivation: designed, deliberately not yet implemented
Domain-separate the three app-32 uses so `ssh@N`, `pgp@N` and `password@N` stop
sharing key material. Sketch, for whoever picks this up:
- Keep v1 exactly as it is, forever — existing SSH keys are on servers and in
forges, existing PGP keys are published. Nothing already derived may change.
- Add `derivation_version` to ssh/pgp entries, absent meaning 1, exactly as
`gen_version` works for passwords. New entries are stamped 2.
- v2 takes the app-32 entropy and applies one HMAC-SHA256 step with a
kind-specific info string (`seedpass:v2:ssh`, `seedpass:v2:pgp`,
`seedpass:v2:password`). Prefer this over new BIP-85 app numbers, which risk
colliding with future registrations.
- Land it in both implementations in one change, with cross-impl fixtures, and
an opt-in per-entry upgrade action — never a bulk migration. The same
"this changes the key; rotate it at the server first" confirmation the
password v2 upgrade needs.
**Sequencing:** this is a new derivation path, and this project's own rule is
that a derivation bug does not throw — it quietly produces a secret that
cannot be recovered. Adding one immediately before the first outside review
inverts the point of the review. Do it after.
- [x] **(fixed 2026-08-18) L-4 — inert settings, and the unported subsystem behind them.**
`additional_backup_path` was stored, confirmed by the TUI ("Additional backups will
be written to X") and used by nothing — false assurance in a disaster-recovery
+25 -8
View File
@@ -25,7 +25,7 @@ failure is flaky and is the direct evidence for finding M-1.
## Findings
### M-1 (Medium) — Restore can non-deterministically return a stale snapshot when two syncs share a second
### M-1 (Medium) — Restore can non-deterministically return a stale snapshot when two syncs share a second — **FIXED 2026-08-19**
- **Where:** `js/packages/core/src/sync/syncFlows.ts:115-117` (`fetchLatestSnapshot` sort)
- **What:** Manifests are ordered by `created_at` (Nostr grants only whole-second
@@ -83,7 +83,7 @@ failure is flaky and is the direct evidence for finding M-1.
pre-fix code produced. Mutation-verified: dropping `basePolicy` from the merge in
`secrets.ts` turns all four red.
### L-1 (Low) — BIP-85 app-32 path is shared across password, SSH, and PGP derivation
### L-1 (Low) — BIP-85 app-32 path is shared across password, SSH, and PGP derivation — **MITIGATED 2026-08-19 (detection shipped; derivation change deferred)**
- **Where:** `derive/ssh.ts:47-49`, `derive/pgp.ts:171-175`, `derive/password.ts:175` — all
three derive from `m/83696968'/32'/{index}'`.
@@ -109,7 +109,7 @@ failure is flaky and is the direct evidence for finding M-1.
document the password-prefix relationship alongside the existing known limit, and warn
on import/merge when two entries of different kinds share an app-32 index.
### L-2 (Low) — Concurrent same-id creation on two replicas silently discards one entry at merge
### L-2 (Low) — Concurrent same-id creation on two replicas silently discards one entry at merge — **SURFACED 2026-08-19**
- **Where:** `js/packages/core/src/sync/merge.ts:347-365` (per-id last-writer-wins).
- **What:** The allocation watermark is per-replica. Two devices working offline can both
@@ -125,7 +125,7 @@ failure is flaky and is the direct evidence for finding M-1.
sync/restore summary instead of resolving silently. A protocol-level fix (replica-scoped
id ranges or random ids) would need coordination with Python.
### L-3 (Low) — Tombstone retention cap enables deletion replay (documented; confirmed)
### L-3 (Low) — Tombstone retention cap enables deletion replay (documented; confirmed) — **SURFACED 2026-08-19**
- **Where:** `merge.ts:17, 405-416` — `TOMBSTONE_RETENTION_CAP = 2048`, oldest evicted.
- **What:** After 2048 deletions, older tombstones are dropped; merging a stale replica or
@@ -316,11 +316,11 @@ pnpm -r test # core: 234/234 pass (jsdom env: 230 pass, 4 skipped)
| Finding | State | Where |
|---|---|---|
| M-1 sync tie-break | **open** | needs a signed-manifest sequence number in both implementations |
| M-1 sync tie-break | fixed | signed `published_ms` ordering in both implementations |
| M-2 config password policy | fixed | `secrets.ts` `basePolicy`; `test/passwordPolicy.test.ts` |
| L-1 shared BIP-85 app-32 path | open | protocol change, both implementations |
| L-2 concurrent same-id creation | open | surface at merge rather than change it |
| L-3 tombstone replay | open (documented) | as the docs' own roadmap has it |
| L-1 shared BIP-85 app-32 path | mitigated | collision detection in both; derivation change deferred to after review |
| L-2 concurrent same-id creation | surfaced | `MergeReport.conflicts`, reported by `nostr restore` |
| L-3 tombstone replay | surfaced | `MergeReport.tombstonesEvicted` |
| L-4 inert settings | fixed | `backups.ts` ports BackupManager's write side; Quick Unlock now says it is unimplemented |
| L-5 unanchored `label_regex` | resolved as-is | matches Python's `re.search`; documented in help, `capabilities()`, the parity matrix, and pinned by tests |
| L-6 exec allowlist scope | documented | help, `capabilities().tokens.exec_allowlist_semantics`, enforcement-point comment |
@@ -331,5 +331,22 @@ Every fix above is mutation-verified: the guard or hook is disabled, the
matching tests are confirmed red, and the change is restored. A passing suite
was not treated as evidence on its own.
**Every finding in this report is now closed.** Four were fixed outright, one
is mitigated with the derivation change deliberately deferred (adding a new
derivation path immediately before the first outside review inverts the point
of the review), and two are surfaced rather than changed because their
resolution is frozen for cross-implementation parity.
Three defects were found while fixing them, none of which the audit had
reached:
- Python's `publish_delta` republished the manifest without the new ordering
field, sorting the manifest carrying the newest `delta_since` below the
snapshot it superseded.
- Raising Python's manifest fetch limit turned a vestigial fallback loop into
a live downgrade path.
- Python's `merge_index_payloads` mutated its `current` argument, and an
existing idempotency test was passing only because of that aliasing.
Still outstanding, and unchanged by this round: no GPT-family review has run.
Findings from one model family are one perspective, not independence.