Transport-refactor review — deferred findings (CLI + low/info backlog) #52

New issue

Open

opened 2026-06-27 11:20:09 +00:00 by padreug · 0 comments

padreug commented

2026-06-27 11:20:09 +00:00

Owner

Tracking record for the findings from the multi-agent review of the NDK→nostr-tools refactor (PRs #43/#45/#47) that we chose not to action yet. No critical/high were found; the daemon-side mediums are already fixed.

Already fixed (for reference)

CS-3 (boot-DM teardown / unhandled rejection) → #49
RP-4 / RP-2 / RP-1 (connect timeout, stop-race, cross-relay dedup) → #50
AD-1 / CS-2 / AD-2 (admin pending-Map leak + late-approval latch) → #51

Deferred — CLI (medium, peripheral debug tool, not a daemon path)

CL-1 — Nip46Client is nip44-only; dropped NDK's adaptive nip04/nip44 negotiation. Against a nip04-only signer the response is silently dropped → opaque <method> timed out. Fix: try nip44, fall back to nip04 keyed off ?iv= (mirror src/daemon/nip46/transport.ts), or at least log decrypt failures instead of catch { return }.
CL-2 — local client key written world-readable (fs.writeFileSync(.../private.key, pk) with no mode → ~0644). Fix: { mode: 0o600 } + dir 0o700.

Deferred — low / info backlog

CL-3 / CS-5 — signed event's pubkey not asserted against the requested remotePubkey (responses matched by id only). Bind expected peer before id-match.
CL-4 — fixed 120s request timeout vs NDK's effectively-infinite wait on interactive connect/create_account. Re-arm on auth_url or lengthen.
CL-5 — create_account email dropped in the bare-domain / user@domain path (content overwritten to username,domain).
CL-6 — no way to pass a connection token (no bunker:// parse, no --token), so token-based pairing is CLI-unreachable.
RP-3 — subscribeAwaitingEose has no timeout; while all relays are down startup blocks until one connects (self-healing; benign).
RP-6 — healthy() with an empty registry is trivially true before the first sub is registered. Treat empty registry as idle/not-healthy.
RP-7 / RP-8 — activeCount counts REQ-attempted not EOSE-confirmed; publish() worst-case ~23.5s on a fully-degraded set with no first-success short-circuit / concurrency cap (signet has one).
BE-3 — success sendResponse shares one try with dispatch, so a publish failure after a successful sign emits a second (error) response for the same id. Split the two.
AD-3 — reqEncryption keyed by client-chosen request id; a same-id interleave could clobber the scheme. Rare; improvement over NDK's shared field.
AD-5 / CS-1 — ACL approval responses routed by id only (no event.pubkey ∈ admin npubs check); the id is a ~50-bit value carried only inside nip44 envelopes, so it behaves as a bearer capability. Cheap hardening: bind expected admin pubkey into the pending entry.
CS-6 — long-lived daemon pools have no SIGTERM/SIGINT stop() hook; the watchdog setInterval isn't stored/unref'd. Fine for a supervised daemon; clean-shutdown polish.
UT-1 — secretKeyBytes hex branch does no validation (the all-zero worst case is refuted — getPublicKey throws before persist; residual: a non-zero malformed hex yields a valid-but-wrong identity, admin-gated, npub visibly differs). Use nostrUtils.hexToBytes + length check.
UT-4 — nip05.queryProfile skips a dotless dev/regtest host (_@localhost) in the NIP-89 self-check (NIP05_REGEX needs a dot); production domains are dotted.

Several review observations were improvements over NDK (per-request encryption threading, explicit verifyEvent, verbatim template signing, stronger pool.healthy()) — recorded as info only, no action.

Full report: the review run produced an overallAssessment of "fundamentally sound, no critical/high, already deployed and signing on aio-demo."

Tracking record for the findings from the multi-agent review of the NDK→nostr-tools refactor (PRs #43/#45/#47) that we chose **not** to action yet. No critical/high were found; the daemon-side mediums are already fixed. ## Already fixed (for reference) - **CS-3** (boot-DM teardown / unhandled rejection) → #49 - **RP-4 / RP-2 / RP-1** (connect timeout, stop-race, cross-relay dedup) → #50 - **AD-1 / CS-2 / AD-2** (admin pending-Map leak + late-approval latch) → #51 ## Deferred — CLI (medium, peripheral debug tool, not a daemon path) - **CL-1** — `Nip46Client` is nip44-only; dropped NDK's adaptive nip04/nip44 negotiation. Against a nip04-only signer the response is silently dropped → opaque `<method> timed out`. *Fix:* try nip44, fall back to nip04 keyed off `?iv=` (mirror `src/daemon/nip46/transport.ts`), or at least log decrypt failures instead of `catch { return }`. - **CL-2** — local client key written world-readable (`fs.writeFileSync(.../private.key, pk)` with no mode → ~0644). *Fix:* `{ mode: 0o600 }` + dir `0o700`. ## Deferred — low / info backlog - **CL-3 / CS-5** — signed event's `pubkey` not asserted against the requested `remotePubkey` (responses matched by id only). Bind expected peer before id-match. - **CL-4** — fixed 120s request timeout vs NDK's effectively-infinite wait on interactive connect/create_account. Re-arm on `auth_url` or lengthen. - **CL-5** — `create_account` email dropped in the bare-domain / `user@domain` path (content overwritten to `username,domain`). - **CL-6** — no way to pass a connection token (no `bunker://` parse, no `--token`), so token-based pairing is CLI-unreachable. - **RP-3** — `subscribeAwaitingEose` has no timeout; while *all* relays are down startup blocks until one connects (self-healing; benign). - **RP-6** — `healthy()` with an empty registry is trivially true before the first sub is registered. Treat empty registry as idle/not-healthy. - **RP-7 / RP-8** — `activeCount` counts REQ-attempted not EOSE-confirmed; `publish()` worst-case ~23.5s on a fully-degraded set with no first-success short-circuit / concurrency cap (signet has one). - **BE-3** — success `sendResponse` shares one `try` with `dispatch`, so a publish failure *after* a successful sign emits a second (error) response for the same id. Split the two. - **AD-3** — `reqEncryption` keyed by client-chosen request id; a same-id interleave could clobber the scheme. Rare; improvement over NDK's shared field. - **AD-5 / CS-1** — ACL approval *responses* routed by id only (no `event.pubkey ∈ admin npubs` check); the id is a ~50-bit value carried only inside nip44 envelopes, so it behaves as a bearer capability. Cheap hardening: bind expected admin pubkey into the pending entry. - **CS-6** — long-lived daemon pools have no SIGTERM/SIGINT `stop()` hook; the watchdog `setInterval` isn't stored/`unref`'d. Fine for a supervised daemon; clean-shutdown polish. - **UT-1** — `secretKeyBytes` hex branch does no validation (the all-zero worst case is *refuted* — `getPublicKey` throws before persist; residual: a non-zero malformed hex yields a valid-but-wrong identity, admin-gated, npub visibly differs). Use `nostrUtils.hexToBytes` + length check. - **UT-4** — `nip05.queryProfile` skips a dotless dev/regtest host (`_@localhost`) in the NIP-89 self-check (`NIP05_REGEX` needs a dot); production domains are dotted. Several review observations were *improvements* over NDK (per-request encryption threading, explicit `verifyEvent`, verbatim template signing, stronger `pool.healthy()`) — recorded as info only, no action. Full report: the review run produced an `overallAssessment` of "fundamentally sound, no critical/high, already deployed and signing on aio-demo."

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

aiolabs/nsecbunkerd#52

No description provided.

Rows
Columns