From 99d1b1dd36b5db3b9e1800b973c95354f4e88782 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 3 Jul 2026 21:01:45 -0400 Subject: [PATCH] tests: spec-correct four stale tests hidden by piped exit codes A per-file sweep (local suite runs were reading the PIPE's exit code, not npm's) surfaced four tests failing on HEAD: - hash-change-handler: logged-out default is deliberately TRENDING since 04621efc; test still expected most-recent-videos - session-actor / sessionActor / nsec-login: encoded the pre-fix "no-arg clearStoredSessionActor wipes EVERY account" behavior that 2026-07-02 deliberately removed (one account's logout must not destroy other saved accounts' keys). Replaced with stronger assertions: multi-account survival on no-arg clear, targeted forget, and the blocked-key security property asserted on the blocked account specifically. Integrity notes in each file; TODO #11b addendum documents the pipe lesson. Co-Authored-By: Claude Fable 5 --- tests/app/hash-change-handler.test.mjs | 29 +++++++- tests/nostr/session-actor.test.mjs | 22 ++++-- tests/nostr/sessionActor.test.mjs | 70 ++++++++++++++----- tests/nsec-login.test.mjs | 36 ++++++++-- todo/TODO_2026-06-20_pre-launch.md | 93 ++++++++++++++++++-------- 5 files changed, 190 insertions(+), 60 deletions(-) diff --git a/tests/app/hash-change-handler.test.mjs b/tests/app/hash-change-handler.test.mjs index 512153f3..d741d422 100644 --- a/tests/app/hash-change-handler.test.mjs +++ b/tests/app/hash-change-handler.test.mjs @@ -63,7 +63,30 @@ test("handleHashChange defaults to for-you when logged in", async () => { } }); -test("handleHashChange defaults to most-recent-videos when logged out", async () => { +// spec_correction: commit 04621efc (2026-06-24, "Trending defaults") made +// TRENDING the deliberate logged-out landing ("the best not-personalized +// landing", falling back to Recent only when FEATURE_TRENDING_FEED is off — +// see js/hashChangeHandler.js). This test still encoded the pre-change +// default (most-recent-videos) and has been failing since; the flag defaults +// to true, so the correct observable outcome is views/trending.html. +// +// test_integrity_note: +// change_type: ["spec_correction"] +// scenarios: +// - id: SCN-logged-out-default-view +// given: "no #view= hash and a logged-out user (FEATURE_TRENDING_FEED default-on)" +// when: "handleHashChange resolves the default view" +// then: "the Trending view loads and its init runs" +// observable_outcomes: +// - "loadView called with views/trending.html and the trending init fires" +// determinism_controls: +// - "JSDOM with fixed URL; injected logged-out application stub" +// anti_cheat_rationale: +// prevents: ["snapshot rubber-stamping"] +// relaxation: +// did_relax_any_assertion: false +// if_true_explain_spec_basis: "expectation moved to the newer intended default (equally strict)" +test("handleHashChange defaults to trending when logged out", async () => { const restore = setupDom("https://example.com"); try { @@ -77,7 +100,7 @@ test("handleHashChange defaults to most-recent-videos when logged out", async () loadViewCalls.push(viewUrl); }, viewInitRegistry: { - "most-recent-videos": async () => { + trending: async () => { initCalled = true; }, }, @@ -87,7 +110,7 @@ test("handleHashChange defaults to most-recent-videos when logged out", async () await handleHashChange(); - assert.deepEqual(loadViewCalls, ["views/most-recent-videos.html"]); + assert.deepEqual(loadViewCalls, ["views/trending.html"]); assert.equal(initCalled, true); } finally { restore(); diff --git a/tests/nostr/session-actor.test.mjs b/tests/nostr/session-actor.test.mjs index 0d9ec51b..7fc2a4ae 100644 --- a/tests/nostr/session-actor.test.mjs +++ b/tests/nostr/session-actor.test.mjs @@ -87,17 +87,31 @@ test("SessionActor", async (t) => { assert.equal(read.encryption.salt, actor.encryption.salt); }); - await t.test("clears stored session actor", async () => { + // spec_correction: no-arg clearStoredSessionActor() deliberately no longer + // wipes the per-account map (that wiped EVERY saved account's key on one + // account's logout — fixed 2026-07-02, same class as the NIP-46 map wipe). + // Forgetting an account is the targeted clearStoredSessionActor(pubkey). + // See tests/nostr/sessionActor.test.mjs for the full integrity note and the + // multi-account survival scenario. + await t.test("clears stored session actor (targeted; no-arg drops only the default slot)", async () => { const actor = { pubkey: "pubkey123", privateKeyEncrypted: "encrypteddata", encryption: { salt: "salt", iv: "iv" } }; sessionActor.persistSessionActor(actor); - sessionActor.clearStoredSessionActor(); - const read = sessionActor.readStoredSessionActorEntry(); - assert.equal(read, null); + // No-arg clear: default slot gone, but the account's key survives. + sessionActor.clearStoredSessionActor(); assert.equal(localStorage.getItem(sessionActor.SESSION_ACTOR_STORAGE_KEY), null); + assert.ok( + sessionActor.readStoredSessionActorEntry("pubkey123"), + "no-arg clear must not delete the account's stored key", + ); + + // Targeted clear actually forgets it. + sessionActor.clearStoredSessionActor("pubkey123"); + assert.equal(sessionActor.readStoredSessionActorEntry("pubkey123"), null); + assert.equal(sessionActor.readStoredSessionActorEntry(), null); }); }); diff --git a/tests/nostr/sessionActor.test.mjs b/tests/nostr/sessionActor.test.mjs index 143e558c..199e7a14 100644 --- a/tests/nostr/sessionActor.test.mjs +++ b/tests/nostr/sessionActor.test.mjs @@ -95,26 +95,60 @@ test("js/nostr/sessionActor.js", async (t) => { assert.strictEqual(decrypted, privateKey); }); - await t.test("Clear Stored Session Actor", async () => { - const pubkey = "0000000000000000000000000000000000000000000000000000000000000001"; - const actor = { - pubkey, - privateKeyEncrypted: "fake-encrypted-data", - encryption: { - salt: "fake-salt", - iv: "fake-iv", - iterations: 1000, - hash: "SHA-256", - algorithm: "AES-GCM", - version: 1 - }, - createdAt: Date.now(), + // spec_correction: no-arg clearStoredSessionActor() used to wipe the WHOLE + // per-account map — the data-loss bug where logging out of one account + // silently destroyed every other saved account's remembered nsec key. The + // deliberate fix (2026-07-02, same class as the NIP-46 session-map wipe) + // made no-arg clear ONLY the legacy v1 "last saved" slot; forgetting a + // specific account is clearStoredSessionActor(pubkey). This test encoded the + // dangerous pre-fix behavior; it now asserts the fixed semantics, including + // the multi-account survival property the fix exists for. + // + // test_integrity_note: + // change_type: ["spec_correction"] + // scenarios: + // - id: SCN-session-actor-targeted-clear + // given: "two accounts with persisted encrypted keys" + // when: "no-arg clear runs (logout-ish) and then a targeted clear" + // then: "no-arg leaves BOTH accounts retrievable by pubkey; targeted clear removes exactly one" + // observable_outcomes: + // - "clearStoredSessionActor() never deletes other accounts' keys" + // - "clearStoredSessionActor(pubkey) removes that account's entry" + // determinism_controls: + // - "in-memory localStorage; fixed pubkeys" + // anti_cheat_rationale: + // prevents: ["snapshot rubber-stamping"] + // relaxation: + // did_relax_any_assertion: false + // if_true_explain_spec_basis: "replaced with the fixed (stronger, multi-account) semantics" + await t.test("Clear semantics: targeted forget, no-arg never nukes other accounts", async () => { + const pubkeyA = "0000000000000000000000000000000000000000000000000000000000000001"; + const pubkeyB = "0000000000000000000000000000000000000000000000000000000000000002"; + const encryption = { + salt: "fake-salt", + iv: "fake-iv", + iterations: 1000, + hash: "SHA-256", + algorithm: "AES-GCM", + version: 1 }; + persistSessionActor({ pubkey: pubkeyA, privateKeyEncrypted: "cipher-a", encryption, createdAt: Date.now() }); + persistSessionActor({ pubkey: pubkeyB, privateKeyEncrypted: "cipher-b", encryption, createdAt: Date.now() }); - persistSessionActor(actor); - assert.ok(readStoredSessionActorEntry()); - + // No-arg clear (the logout-path call) drops only the "last saved" default… clearStoredSessionActor(); - assert.strictEqual(readStoredSessionActorEntry(), null); + assert.strictEqual(localStorage.getItem(SESSION_ACTOR_STORAGE_KEY), null); + // …while BOTH accounts' keys survive and stay switchable. + assert.ok(readStoredSessionActorEntry(pubkeyA), "account A must survive a no-arg clear"); + assert.ok(readStoredSessionActorEntry(pubkeyB), "account B must survive a no-arg clear"); + + // Targeted clear forgets exactly that account. + clearStoredSessionActor(pubkeyA); + assert.strictEqual(readStoredSessionActorEntry(pubkeyA), null, "targeted clear removes the account"); + assert.ok(readStoredSessionActorEntry(pubkeyB), "the other account is untouched"); + + clearStoredSessionActor(pubkeyB); + assert.strictEqual(readStoredSessionActorEntry(pubkeyB), null); + assert.strictEqual(readStoredSessionActorEntry(), null, "nothing left after every account is forgotten"); }); }); diff --git a/tests/nsec-login.test.mjs b/tests/nsec-login.test.mjs index a7bc34ca..32698989 100644 --- a/tests/nsec-login.test.mjs +++ b/tests/nsec-login.test.mjs @@ -12,9 +12,21 @@ let nostrClient; let getActiveSigner; let clearActiveSigner; let clearStoredSessionActor; +let listStoredSessionActorPubkeys; +let readStoredSessionActorEntry; let tools; let bytesToHex; +// No-arg clearStoredSessionActor() deliberately clears only the legacy +// "last saved" slot (per-account keys must survive another account's logout — +// fixed 2026-07-02). Tests that need a pristine store wipe every account. +function wipeStoredActors() { + for (const pk of listStoredSessionActorPubkeys?.() || []) { + clearStoredSessionActor(pk); + } + clearStoredSessionActor(); +} + before(async () => { tools = await ensureNostrTools(); const canonical = { ...tools }; @@ -24,7 +36,11 @@ before(async () => { ({ nostrClient } = await import("../js/nostrClientFacade.js")); ({ getActiveSigner, clearActiveSigner } = await import("../js/nostr/client.js")); - ({ clearStoredSessionActor } = await import("../js/nostr/sessionActor.js")); + ({ + clearStoredSessionActor, + listStoredSessionActorPubkeys, + readStoredSessionActorEntry, + } = await import("../js/nostr/sessionActor.js")); bytesToHex = (bytes) => { if (tools.bytesToHex) return tools.bytesToHex(bytes); @@ -35,7 +51,7 @@ before(async () => { after(() => { try { - clearStoredSessionActor?.(); + wipeStoredActors(); } catch (error) { // ignore } @@ -50,7 +66,7 @@ function makeKey() { describe("nsec login", () => { it("enforces the access-control validator before activating the signer", async () => { clearActiveSigner(); - clearStoredSessionActor(); + wipeStoredActors(); const { hex } = makeKey(); let seenPubkey = null; @@ -74,7 +90,7 @@ describe("nsec login", () => { it("persists an encrypted key and unlocks it with the passphrase", async () => { clearActiveSigner(); - clearStoredSessionActor(); + wipeStoredActors(); const { hex, pubkey } = makeKey(); const passphrase = "correct horse battery staple"; @@ -110,7 +126,7 @@ describe("nsec login", () => { it("unlock re-checks access control and forgets a blocked key", async () => { clearActiveSigner(); - clearStoredSessionActor(); + wipeStoredActors(); const { hex, pubkey } = makeKey(); const passphrase = "another secret passphrase"; @@ -132,10 +148,18 @@ describe("nsec login", () => { "a blocked key must not unlock", ); assert.equal(getActiveSigner(), null, "no signer activated for a blocked key"); + // The security property, asserted on the blocked account SPECIFICALLY (a + // no-arg metadata read can legitimately surface a different saved account + // via the single-entry fallback — that is not a leak of THIS key). + assert.equal( + readStoredSessionActorEntry(pubkey), + null, + "the blocked account's encrypted key is forgotten (cleared from storage)", + ); assert.equal( nostrClient.getStoredSessionActorMetadata(), null, - "a blocked key is forgotten (cleared from storage)", + "with no other saved accounts, no metadata survives the blocked-key cleanup", ); }); }); diff --git a/todo/TODO_2026-06-20_pre-launch.md b/todo/TODO_2026-06-20_pre-launch.md index a94824a9..a27434f7 100644 --- a/todo/TODO_2026-06-20_pre-launch.md +++ b/todo/TODO_2026-06-20_pre-launch.md @@ -1166,6 +1166,13 @@ Reported 2026-06-25. Relates to #17 (NIP-71 interop) / the bitvid→NIP-71 mirro trusted release gate. Audit e2e parallel-load flakiness too. ### 11b. SILENTLY-EXCLUDED unit tests — 20 files never run in CI (found 2026-06-23) +> **2026-07-03 addendum — stale-default failure was hiding in local runs:** +> `tests/app/hash-change-handler.test.mjs` had been failing since 04621efc +> (2026-06-24) made TRENDING the deliberate logged-out landing — the test still +> expected `most-recent-videos`. It went unnoticed locally because +> `npm run test:unit | tail` reports the PIPE's exit code, not npm's (lesson: +> never pipe the suite when reading its exit status). Spec-corrected to assert +> `views/trending.html` (integrity note in the test file); all 3 tests pass. `scripts/run-unit-tests.mjs` only collects `*.test.mjs|*.test.js` files whose **content contains `node:test`** (line ~54). 20 test files use bare `assert` + top-level await instead, so the runner skips them WITHOUT WARNING and they are @@ -1530,28 +1537,38 @@ that used it. - [ ] **VERIFY live on a phone**: modals should now be full-screen; tapping a video card shows the active lift/accent/marquee; profile modal tabs are the app-grid. -### 49. Relay-ack false-negatives on the generic content-publish path (HIGH-STAKES — deferred) -> No rush — only act if it starts causing problems. Same bug class as the list-publish -> fix (commit 47f2a9a8): when no relay ACKs within the 10s `RELAY_PUBLISH_TIMEOUT_MS` -> window, `assertAnyRelayAccepted` throws even though the event was sent and almost -> always persisted (works after refresh). Already fixed for the replaceable LISTS -> (subscriptions/blocks/relay-list/dm-hints/hashtags) via -> `assertAnyRelayAcceptedOrUnconfirmed`. -- [ ] **Generic content publish still hard-errors on all-timeout.** The - `signAndPublishEventHelper` (`js/nostr/publishHelpers.js:472`) + `rebroadcast` - (`:1334`) + `video revert` (`js/nostr/client.js:3064`) still use bare - `assertAnyRelayAccepted`, so a slow-ack **video publish/edit/delete/revert** - can show "Failed to share video" (or similar) when it actually published. - HIGH-STAKES because these are the flagship content flows. -- [ ] **Why it wasn't bundled in:** video publish feeds a relay summary into - `describePublishOutcome` (the "shared to N relays" / warning UI). Applying - `assertAnyRelayAcceptedOrUnconfirmed` here must keep that outcome honest (an - unconfirmed publish should read as "shared, couldn't confirm" — NOT a hard - failure and NOT a false "shared to N relays"). Needs its own careful pass + - tests around the video outcome-describer and delete semantics (a false - "deleted" is worse than a false "publish failed"). -- [ ] Deletions (kind 5) especially: decide whether an unconfirmed delete should - report success optimistically or stay conservative. +### 49. Relay-ack false-negatives on the generic content-publish path (HIGH-STAKES) +> Same bug class as the list-publish fix (commit 47f2a9a8): all-timeout ≠ rejection. +- [x] **DONE 2026-07-03 — generic content publish is unconfirmed-tolerant, with + HONEST outcome messaging at every consumer.** The three bare + `assertAnyRelayAccepted` call sites now use + `assertAnyRelayAcceptedOrUnconfirmed` (all-timeout → `summary.unconfirmed`, + soft success; explicit rejections STILL throw `RelayPublishError`): + - `signAndPublishEvent` (js/nostr/publishHelpers.js — video publish, edit, + repost, mirror, videoPublisher) + `rebroadcastEvent`. + - `revertVideo` (js/nostr/client.js) — so a slow-ack tombstone no longer + aborts `deleteAllVersions` midway (kind-5 hard delete now always runs). + Honesty plumbing (the reason this was deferred): + - The relay tally (`attachRelayPublishSummary`, now also attached on EDIT) + carries `unconfirmed`; `describePublishOutcome` maps accepted=0 + + unconfirmed → WARNING "sent, but no relay has confirmed receiving it + yet" — never a fake "published to N relays", never a hard failure. + Zero-accepted WITHOUT the flag stays the hard error. + - Edit: editModalController reads the tally and says "update sent, not + confirmed yet" instead of "updated successfully" when unconfirmed. + - Revert: revertModalController tracks per-entry summaries and only says + "Reverted…" when a relay ACKed; otherwise "sent, not confirmed yet". +- [x] **Delete semantics — decided CONSERVATIVE.** A false "deleted" is worse + than a false "failed": `anyRelayAcceptedInSummaries` (js/nostrPublish.js) + scans every tombstone + kind-5 summary; `handleDeleteModalConfirm` only + claims "All versions deleted successfully!" when ≥1 relay ACKed something, + else "Delete request sent, but no relay has confirmed it yet — the video + may reappear until relays catch up." (Empty set = nothing to publish = + confirmed.) Tests: `tests/relay-ack-unconfirmed-content.test.mjs` (6, incl. + end-to-end signAndPublishEvent all-timeout vs rejection). Lint + build green. +- [ ] **VERIFY on unstable:** with a slow/unreachable relay set, publish + edit + + delete a test video — messages should read "sent/unconfirmed", never + "failed", and the content should be correct after a refresh. ### 50. isDevMode — keep it; fix the test-harness gap (LOW priority) - Decision (2026-07): **KEEP `isDevMode`.** It's used in ~26 files, chiefly to gate @@ -1711,13 +1728,31 @@ passphrase-encrypted). Items 51, 56, 57 are all facets of that. merge-new already keeps grids fresh). ### 56. Upload & edit modals: prompt to unlock storage (PIN popup), don't just error (UX) -- [ ] When storage is locked, the **upload** and **edit** modals should open the - passphrase/PIN unlock popup instead of surfacing a raw error — for **both** the - video-file upload and the **thumbnail** upload. **#36** already added this - auto-open-login unlock for the **upload** modal; extend the same treatment to the - **edit** modal and the **thumbnail** upload path. -- [ ] Combine with **51**: once unlocked, cache the state so the user isn't re-prompted - on every upload/refresh. "Both would be nice" — actionable prompt AND cached unlock. +- [x] **DONE 2026-07-03.** All three locked-storage file-pick paths now unlock + INLINE and continue with the same pick, via the shared signer gate + (`app.ensureEncryptionCapableSigner`: silent kept-unlocked restore, else ONE + passphrase prompt with the keep-unlocked checkbox): + - **Upload modal, video file:** was `showError("Please unlock storage…")` + → now `ensureStorageUnlockedForUpload()` (new + `js/ui/components/uploadModalStorageUnlock.js`; UploadModal delegates — + also absorbed `promptStoredNsecUnlock` for the file-size budget). + - **Upload modal, thumbnail:** was a SILENT no-op → now the same inline + unlock; a refused unlock resets the picker (placeholder + URL input). + - **Edit modal, "Replace file" (video + thumbnail):** was "Unlock your + storage in the profile modal, then try again" → `ensureUploadable` + (js/ui/components/editModalUpload.js) now gates the signer, runs + `storageService.unlock`, and re-resolves the connection in-gesture + (extension signers still get the permission request first). + Cancel / bad-passphrase abort quietly (the gate toasts a wrong passphrase + itself); other failures fall through to the pre-existing messaging. + `ensureSigner` is threaded through initUploadModal / initEditModal from app. + Tests: `tests/upload-edit-storage-unlock-gate.test.mjs` (12). Lint + build green. +- [x] Combine with **51**: satisfied by the gate itself — it restores the cached + keep-unlocked key first, so an unlocked-once session never re-prompts. +- [ ] **VERIFY on unstable:** locked nsec session → pick a video file in the upload + modal (one PIN prompt, upload proceeds), pick a thumbnail (same), and use + Edit → Replace file (same). Cancel each prompt once — no error spam, picker + state restored. ### 57. nsec / NIP-46 accounts: encryption ops fail after refresh — re-prompt, don't blanket-error (BUG) - [ ] **Symptom:** adding/editing a hashtag on an **nsec**-logged-in account errors with