mirror of
https://github.com/PR0M3TH3AN/bitvid.git
synced 2026-10-27 07:52:24 +00:00
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 <[email protected]>
This commit is contained in:
1 parent
1934f9386a
commit
99d1b1dd36
5 files changed
+190
-60
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user