From 937bcb7aeebd97462ce39c4117b258bcb55b5b01 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 02:45:35 +0000 Subject: [PATCH] Guard streamTagFinal() against being made eager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework against review finding B-A on the branch. The claim was false as written. Commit 8a200be's message, the PR body and the comments on both sides said the repurposed pinning test in test/crypto/stream.test.ts fails if streamTagFinal() is ever made eager. It does not: substituting const EAGER: number = sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL; export const streamTagFinal = (): number => EAGER; leaves the whole suite green. Under vitest, sodium is already initialised in the worker process by the time a source module is evaluated, so an eager read picks up a real value there and value equality cannot see the difference. The danger it claimed to cover is real: a direct probe of the vendored libsodium-wrappers-sumo gives undefined before await sodium.ready and 3 after, so an eager read would ship a library that rejects every valid download as truncated for anyone importing it in plain Node ESM, while make check stayed green. Rather than drop the claim, this makes it true. A new test reproduces the plain-Node ESM ordering that vitest hides: vi.resetModules() plus a doMock'd stand-in sodium whose TAG_FINAL property is absent while src/crypto/stream.ts is evaluated and appears only afterwards, exactly as libsodium attaches its constants inside ready.then(...). A call-time read sees the value that appeared after evaluation; a module-level read binds undefined and the test fails. The stand-in hands back a sentinel rather than 3, so a read that somehow reached the real library, or a return to a hardcoded literal, fails too. Demonstrated rather than argued: with the two-line eager variant above in place, make test reports 1 failed | 140 passed, "expected undefined to be 42", exit 2. Restored with git checkout -- src/crypto/stream.ts, make test reports 141 passed. The eager variant was applied with the editor and reverted with git, not by scripted substitution. The pinning test keeps its original job — value equality against the tag observed on a real final chunk — and its doc comment now says only that. The comment on the accessor in src/crypto/stream.ts names the guard test, and records why the ordinary tests cannot see the bug on their own. --- src/crypto/stream.ts | 19 +++++++++---- test/crypto/stream.test.ts | 57 +++++++++++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/crypto/stream.ts b/src/crypto/stream.ts index cb48818..d2bde06 100644 --- a/src/crypto/stream.ts +++ b/src/crypto/stream.ts @@ -12,11 +12,20 @@ export const STREAM_CHUNK_OVERHEAD = 17; // marks the last chunk of a stream. Exported so callers (the download layer) // can detect truncation without importing sodium themselves. // -// This is a function rather than a constant because libsodium attaches its -// constants to the module object only once `sodium.ready` has resolved, which -// is long after this module is evaluated — a module-level read would bind -// `undefined`. Reading it at call time returns the library's own value, so -// there is no second copy of a protocol constant to keep in sync. +// This is a function rather than a constant, and that is load-bearing: +// libsodium attaches its constants to the module object inside +// `ready.then(...)`, which resolves long after this module is evaluated. A +// module-level read would bind `undefined`, every downstream comparison +// against it would then be false, and every valid download would be rejected +// as truncated. Reading at call time returns the library's own value, so +// there is also no second copy of a protocol constant to keep in sync. +// +// Under vitest sodium is already initialised in the worker before this module +// is evaluated, so an eager read would pick up a real value there and the +// ordinary tests could not tell the difference. The regression guard is +// "streamTagFinal() reads the constant at call time, not at import time" in +// test/crypto/stream.test.ts, which reproduces the plain-Node ESM ordering +// against a stand-in sodium module; it goes red if this becomes eager. export const streamTagFinal = (): number => sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL; diff --git a/test/crypto/stream.test.ts b/test/crypto/stream.test.ts index f4b2707..331317f 100644 --- a/test/crypto/stream.test.ts +++ b/test/crypto/stream.test.ts @@ -18,6 +18,9 @@ * - The chunk-size constants match Ente's expectations, and the * re-exported `streamTagFinal()` matches the tag a real final chunk * carries. + * - `streamTagFinal()` reads libsodium's constant at call time rather than + * at import time, which it must, because the constant does not exist yet + * when this library's modules are evaluated. * - The pull state can decrypt a multi-chunk stream produced by * sodium.crypto_secretstream_xchacha20poly1305_push, in order. * - The tag byte is propagated to the caller. @@ -25,7 +28,7 @@ */ import sodium from "libsodium-wrappers-sumo"; -import { beforeAll, describe, expect, it } from "vitest"; +import { beforeAll, describe, expect, it, vi } from "vitest"; import { init, initStreamPull, @@ -60,9 +63,9 @@ describe("crypto stream constants", () => { * evaluated. Reading the value at call time yields libsodium's number; * reading it at module scope would yield `undefined`, and every * truncation check downstream would then compare against `undefined` and - * reject good downloads. This test pins the behaviour end to end — it - * compares against the tag observed on a real final chunk pulled back off - * the wire format, so it fails if the accessor is ever made eager. + * reject good downloads. The eagerness itself is guarded by the next test; + * this one pins the value, comparing it against the tag observed on a real + * final chunk pulled back off the wire format. */ it("streamTagFinal() is the tag carried by a real final chunk", async () => { await init(); @@ -81,6 +84,52 @@ describe("crypto stream constants", () => { const state = initStreamPull(push.header, key); expect(pullStreamChunk(state, ciphertext).tag).toBe(streamTagFinal()); }); + + /** + * The regression guard for the eagerness property described above. + * + * Under vitest, sodium is already initialised in the worker process by + * the time any source module is evaluated, so an eager module-level read + * would happen to pick up a real value and no ordinary test could tell + * the difference. This test recreates the ordering that a plain Node ESM + * consumer sees: a stand-in sodium module whose `TAG_FINAL` property does + * not exist yet when `src/crypto/stream.ts` is evaluated and only appears + * afterwards, exactly as libsodium attaches its constants inside + * `ready.then(...)`. + * + * A call-time read observes the value that appeared after evaluation; a + * module-level read binds `undefined` and this test fails. The stand-in + * uses a sentinel rather than the real tag number so that a read which + * somehow reached the real libsodium would fail too. + */ + it("streamTagFinal() reads the constant at call time, not at import time", async () => { + const SENTINEL = 42; + const late: { tagFinal: number | undefined } = { tagFinal: undefined }; + + vi.resetModules(); + vi.doMock("libsodium-wrappers-sumo", () => ({ + default: { + ready: Promise.resolve(), + get crypto_secretstream_xchacha20poly1305_TAG_FINAL() { + return late.tagFinal; + }, + }, + })); + + try { + // Evaluated while the constant is still absent, as it is before + // `sodium.ready` resolves. + const fresh = await import("../../src/crypto/stream.js"); + expect(late.tagFinal).toBeUndefined(); + + // libsodium attaches its constants; a lazy accessor sees them. + late.tagFinal = SENTINEL; + expect(fresh.streamTagFinal()).toBe(SENTINEL); + } finally { + vi.doUnmock("libsodium-wrappers-sumo"); + vi.resetModules(); + } + }); }); describe("crypto.initStreamPull / pullStreamChunk", () => {