Guard streamTagFinal() against being made eager
All checks were successful
check / check (push) Successful in 4s
All checks were successful
check / check (push) Successful in 4s
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.
This commit was merged in pull request #20.
This commit is contained in:
@@ -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;
|
||||
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user