From 1f894bad0e888c4b3c054e17528059ca7030b1f6 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 01:51:49 +0000 Subject: [PATCH 1/4] Add failing tests for download truncation detection and atomic writes Covers, for both downloadFile and downloadThumbnail: - a multi-chunk body whose TAG_FINAL chunk never arrived is rejected with a truncation error; - an empty body is rejected as truncation rather than written as a zero-byte file; - after a truncation or chunk-authentication failure the destination path does not exist and no temporary scratch file is left behind; - an existing file at the destination survives a failed download byte for byte, and is replaced atomically by a successful one; - the existing success cases still produce identical bytes and an identical DownloadResult. Adds an encryptMultiChunkBody helper that frames leading chunks at exactly STREAM_CHUNK_SIZE so the downloader's fixed-size re-splitting lines up, plus a multi-chunk success case as the positive control. Also pins the new STREAM_TAG_FINAL crypto export against libsodium's own constant, since it must be declared as a literal: libsodium attaches its constants only after sodium.ready resolves, well after this library's modules are evaluated. These fail until the implementation lands, per the repo's TDD workflow. --- test/crypto/stream.test.ts | 20 +- test/download/download.test.ts | 345 ++++++++++++++++++++++++++++++++- 2 files changed, 354 insertions(+), 11 deletions(-) diff --git a/test/crypto/stream.test.ts b/test/crypto/stream.test.ts index db67bc6..b3544d3 100644 --- a/test/crypto/stream.test.ts +++ b/test/crypto/stream.test.ts @@ -15,7 +15,8 @@ * stream ended on a `TAG_FINAL` chunk and was therefore not truncated. * * These tests pin: - * - The chunk-size constants match Ente's expectations. + * - The chunk-size constants match Ente's expectations, and the + * re-exported `STREAM_TAG_FINAL` matches libsodium's own constant. * - 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. @@ -30,6 +31,7 @@ import { pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, + STREAM_TAG_FINAL, } from "../../src/crypto/index.js"; describe("crypto stream constants", () => { @@ -45,6 +47,22 @@ describe("crypto stream constants", () => { it("STREAM_CHUNK_OVERHEAD is 17 bytes", () => { expect(STREAM_CHUNK_OVERHEAD).toBe(17); }); + + /** + * `STREAM_TAG_FINAL` is re-exported so callers can detect a truncated + * stream (a body that ended on a non-final chunk) without importing + * libsodium themselves. It has to be declared as a literal, because + * libsodium only attaches its own constants to the module object after + * `sodium.ready` resolves — long after this library's modules are + * evaluated. This test is what keeps the literal honest. + */ + it("STREAM_TAG_FINAL equals libsodium's TAG_FINAL", async () => { + await init(); + await sodium.ready; + expect(STREAM_TAG_FINAL).toBe( + sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL, + ); + }); }); describe("crypto.initStreamPull / pullStreamChunk", () => { diff --git a/test/download/download.test.ts b/test/download/download.test.ts index 1bcd625..2209857 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -9,20 +9,45 @@ * secretstream ciphertext chunks. Each chunk is at most * `STREAM_CHUNK_SIZE + STREAM_CHUNK_OVERHEAD` bytes (4 MiB + 17 bytes). * The download function buffers incoming network data, splits it on the - * chunk boundary, and feeds each piece to `pullStreamChunk`. The last - * chunk carries `TAG_FINAL`; any truncation is detected because the tag - * will be missing. + * chunk boundary, and feeds each piece to `pullStreamChunk`. + * + * Two contracts are load-bearing for anyone using this library as a backup + * tool, and both are documented by the tests below: + * + * 1. **Truncation is an error, never a short file.** Only the final chunk of + * a secretstream carries `TAG_FINAL`. A download cut short by a dropped + * connection still decrypts cleanly up to the last whole chunk, so without + * an explicit `TAG_FINAL` check a truncated body is indistinguishable from + * a complete one. `streamDecrypt` therefore refuses to return unless the + * stream ended on `TAG_FINAL`, and the error says the stream was truncated. + * + * 2. **The destination path is written atomically.** Plaintext goes to a + * temporary sibling file first and is `rename`d into place only after the + * whole stream has decrypted and verified. A caller that sees no exception + * can rely on the destination containing the complete, authenticated file; + * a caller that sees an exception can rely on the destination being + * untouched — whatever was there before is still there, byte for byte, and + * no partial file has appeared. This matters because `runBackup` skips any + * existing non-empty file, so a partial write would be treated as complete + * forever after. * * These tests build synthetic encrypted files using sodium's push API, * serve them from a mock fetch, and verify the decrypted output on disk. */ -import { existsSync, readFileSync, rmSync, mkdtempSync } from "node:fs"; +import { + existsSync, + readdirSync, + readFileSync, + rmSync, + mkdtempSync, + writeFileSync, +} from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import sodium from "libsodium-wrappers-sumo"; import { beforeAll, afterAll, describe, expect, it } from "vitest"; -import { init, toBase64 } from "../../src/crypto/index.js"; +import { init, toBase64, STREAM_CHUNK_SIZE } from "../../src/crypto/index.js"; import { ApiClient } from "../../src/api/client.js"; import { downloadFile, downloadThumbnail } from "../../src/download/index.js"; import type { EnteFile, FileMetadata } from "../../src/model/types.js"; @@ -64,6 +89,79 @@ const encryptFileBody = ( return { header: push.header, ciphertext }; }; +/** + * Encrypt a body that spans more than one secretstream chunk, the way the + * server does for files larger than the 4 MiB plaintext chunk size. + * + * Framing matters here: the downloader splits the byte stream on fixed + * `STREAM_CHUNK_SIZE + STREAM_CHUNK_OVERHEAD` boundaries, so every chunk + * except the last must carry exactly `STREAM_CHUNK_SIZE` plaintext bytes. + * Only the last chunk is tagged `TAG_FINAL`; the leading ones are + * `TAG_MESSAGE`. + * + * Returns the header, the concatenated body, the plaintext it decrypts to, + * and `finalChunkOffset` — the byte offset at which the `TAG_FINAL` chunk + * begins, so a test can slice it off to simulate a connection that dropped + * before the end of the file. + */ +const encryptMultiChunkBody = ( + key: Uint8Array, + leadingChunks: number, + finalChunkPlainSize: number, +): { + header: Uint8Array; + body: Uint8Array; + plaintext: Uint8Array; + finalChunkOffset: number; +} => { + const push = sodium.crypto_secretstream_xchacha20poly1305_init_push(key); + const cipherParts: Uint8Array[] = []; + const plainParts: Uint8Array[] = []; + + for (let i = 0; i < leadingChunks; i++) { + const plain = sodium.randombytes_buf(STREAM_CHUNK_SIZE); + plainParts.push(plain); + cipherParts.push( + sodium.crypto_secretstream_xchacha20poly1305_push( + push.state, + plain, + null, + sodium.crypto_secretstream_xchacha20poly1305_TAG_MESSAGE, + ), + ); + } + + const finalPlain = sodium.randombytes_buf(finalChunkPlainSize); + plainParts.push(finalPlain); + const finalCipher = sodium.crypto_secretstream_xchacha20poly1305_push( + push.state, + finalPlain, + null, + sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL, + ); + + const finalChunkOffset = cipherParts.reduce((n, c) => n + c.length, 0); + cipherParts.push(finalCipher); + + return { + header: push.header, + body: concat(cipherParts), + plaintext: concat(plainParts), + finalChunkOffset, + }; +}; + +const concat = (parts: Uint8Array[]): Uint8Array => { + const total = parts.reduce((n, p) => n + p.length, 0); + const out = new Uint8Array(total); + let offset = 0; + for (const p of parts) { + out.set(p, offset); + offset += p.length; + } + return out; +}; + const buildMockEnteFile = ( key: Uint8Array, fileHeader: Uint8Array, @@ -90,6 +188,41 @@ const mockFetchForBody = (body: Uint8Array) => { return fake as typeof globalThis.fetch; }; +/** + * A multi-chunk fixture shared by the truncation tests. Building it costs a + * few MiB of encryption, so it is built once: one full 4 MiB `TAG_MESSAGE` + * chunk followed by a small `TAG_FINAL` chunk. + */ +let multiChunk: ReturnType; +let multiChunkKey: Uint8Array; + +beforeAll(() => { + multiChunkKey = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + multiChunk = encryptMultiChunkBody(multiChunkKey, 1, 1024); +}); + +/** + * Build an EnteFile plus ApiClient whose file *and* thumbnail streams both + * serve `body` under `header`. The download path under test is otherwise + * identical for the two, so every truncation/atomicity case below runs + * against both entry points from a single fixture. + */ +const fixtureFor = ( + key: Uint8Array, + header: Uint8Array, + body: Uint8Array, +): { api: ApiClient; file: EnteFile } => ({ + api: new ApiClient({ fetch: mockFetchForBody(body) }), + file: buildMockEnteFile(key, header, header), +}); + +// The two entry points share `streamDecrypt` and the atomic-write wrapper, +// so the contract tests are written once and run against both. +const entryPoints = [ + { name: "downloadFile", download: downloadFile }, + { name: "downloadThumbnail", download: downloadThumbnail }, +]; + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -113,8 +246,14 @@ describe("downloadFile", () => { const result = await downloadFile(api, file, outPath); - expect(result.path).toBe(outPath); - expect(result.bytesWritten).toBe(plaintext.length); + // The whole DownloadResult shape is asserted, not just its fields: + // callers depend on `path` being the destination they asked for + // (never the temporary file used along the way) and on + // `bytesWritten` being the plaintext length. + expect(result).toEqual({ + path: outPath, + bytesWritten: plaintext.length, + }); expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); }); @@ -131,6 +270,7 @@ describe("downloadFile", () => { const result = await downloadFile(api, file); expect(result.path).toBe("fallback-name.png"); + expect(readFileSync(result.path)).toEqual(Buffer.from(plaintext)); // Clean up since it writes to cwd if (existsSync(result.path)) rmSync(result.path); }); @@ -139,8 +279,6 @@ describe("downloadFile", () => { // Most photos are under 4 MiB and therefore a single secretstream // chunk. This test exercises a non-trivial payload size with // random binary data (not just ASCII) to verify no encoding bugs. - // Multi-chunk (>4 MiB) decryption is verified by the live - // integration test against real photos from the dev account. const plaintext = sodium.randombytes_buf(100_000); const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptFileBody(plaintext, key); @@ -155,6 +293,28 @@ describe("downloadFile", () => { expect(result.bytesWritten).toBe(100_000); expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); }); + + it("decrypts a body that spans several secretstream chunks", async () => { + // Files over 4 MiB arrive as several ciphertext chunks concatenated + // into one HTTP body. The downloader has to re-split them on the + // exact chunk boundary; getting that wrong corrupts every large + // photo in an account. This is also the positive control for the + // truncation tests below: it proves the multi-chunk fixture itself + // decrypts cleanly when nothing has been removed from it. + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + multiChunk.body, + ); + const outPath = join(testDir, "multi-chunk.bin"); + + const result = await downloadFile(api, file, outPath); + + expect(result.bytesWritten).toBe(multiChunk.plaintext.length); + expect(readFileSync(outPath)).toEqual( + Buffer.from(multiChunk.plaintext), + ); + }); }); describe("downloadThumbnail", () => { @@ -173,7 +333,172 @@ describe("downloadThumbnail", () => { const result = await downloadThumbnail(api, file, outPath); - expect(result.bytesWritten).toBe(4); + expect(result).toEqual({ path: outPath, bytesWritten: 4 }); expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); }); }); + +// --------------------------------------------------------------------------- +// Truncation detection and atomic writes +// +// Everything below is the failure contract. It is deliberately written once +// per entry point via `entryPoints`, because `downloadFile` and +// `downloadThumbnail` must behave identically here: a corrupt thumbnail is +// just as unacceptable as a corrupt original, and `runBackup` trusts both. +// --------------------------------------------------------------------------- + +describe.each(entryPoints)( + "$name truncation handling", + ({ name, download }) => { + /** A fresh, empty directory so leftover-file assertions are meaningful. */ + const freshDir = (): string => { + const dir = mkdtempSync(join(testDir, `${name}-`)); + return dir; + }; + + it("rejects a body whose final TAG_FINAL chunk never arrived", async () => { + // Simulate a connection that dropped after the first 4 MiB chunk. + // Every byte that did arrive decrypts and authenticates perfectly — + // that is precisely the danger. The only signal that the file is + // incomplete is the absence of a chunk tagged TAG_FINAL, so the + // downloader must treat "stream ended on TAG_MESSAGE" as a hard + // error rather than returning a short file. + const truncatedBody = multiChunk.body.slice( + 0, + multiChunk.finalChunkOffset, + ); + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + truncatedBody, + ); + const outPath = join(freshDir(), "truncated.bin"); + + await expect(download(api, file, outPath)).rejects.toThrow( + /truncated/i, + ); + }); + + it("rejects an empty body instead of writing a zero-byte file", async () => { + // Ente always emits at least one chunk, even for empty content: + // `encryptBlob` shows that a zero-length plaintext still produces a + // TAG_FINAL chunk. A body with no chunks at all therefore means the + // transfer failed, not that the file is empty. Writing a zero-byte + // file here would be the worst outcome, because `runBackup` would + // then see a file it considers present and never retry it. + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + new Uint8Array(0), + ); + const outPath = join(freshDir(), "empty.bin"); + + await expect(download(api, file, outPath)).rejects.toThrow( + /truncated/i, + ); + }); + + it("leaves no file at the destination after a truncated download", async () => { + // The caller's contract: if the promise rejects, the destination + // path does not exist. Nothing downstream should ever have to guess + // whether a leftover file is complete. + const truncatedBody = multiChunk.body.slice( + 0, + multiChunk.finalChunkOffset, + ); + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + truncatedBody, + ); + const dir = freshDir(); + const outPath = join(dir, "absent.bin"); + + await expect(download(api, file, outPath)).rejects.toThrow( + /truncated/i, + ); + + expect(existsSync(outPath)).toBe(false); + // And no temporary scratch file is left behind either: the download + // stages plaintext in a sibling temp file, which must be removed on + // the failure path so repeated failures cannot fill the disk. + expect(readdirSync(dir)).toEqual([]); + }); + + it("leaves no file at the destination when a chunk fails authentication", async () => { + // The same guarantee has to hold for every failure mode, not just + // truncation. Here a byte of ciphertext is flipped, so Poly1305 + // verification fails inside `pullStreamChunk`. The error must + // propagate unchanged (it is the real diagnosis) and the destination + // must still be untouched. + const plaintext = sodium.randombytes_buf(256); + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody(plaintext, key); + const corrupted = Uint8Array.from(ciphertext); + corrupted[10] ^= 0xff; + + const { api, file } = fixtureFor(key, header, corrupted); + const dir = freshDir(); + const outPath = join(dir, "corrupt.bin"); + + await expect(download(api, file, outPath)).rejects.toThrow( + /authentication failed/i, + ); + + expect(existsSync(outPath)).toBe(false); + expect(readdirSync(dir)).toEqual([]); + }); + + it("does not clobber an existing file when the download fails", async () => { + // The repair case. A user re-running a backup over a directory that + // already holds good originals must never end up worse off: a failed + // download leaves the previous contents exactly as they were, so the + // old good copy survives until a complete new one is available to + // replace it in a single rename. + const existing = new TextEncoder().encode( + "previously downloaded, known-good contents", + ); + const truncatedBody = multiChunk.body.slice( + 0, + multiChunk.finalChunkOffset, + ); + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + truncatedBody, + ); + const dir = freshDir(); + const outPath = join(dir, "existing.bin"); + writeFileSync(outPath, existing); + + await expect(download(api, file, outPath)).rejects.toThrow( + /truncated/i, + ); + + expect(readFileSync(outPath)).toEqual(Buffer.from(existing)); + expect(readdirSync(dir)).toEqual(["existing.bin"]); + }); + + it("replaces an existing file when the download succeeds", async () => { + // The mirror image of the previous test: a complete download does + // overwrite whatever was at the destination, atomically, via rename. + const existing = new TextEncoder().encode("stale contents"); + const plaintext = sodium.randombytes_buf(512); + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody(plaintext, key); + + const { api, file } = fixtureFor(key, header, ciphertext); + const dir = freshDir(); + const outPath = join(dir, "replaced.bin"); + writeFileSync(outPath, existing); + + const result = await download(api, file, outPath); + + expect(result).toEqual({ path: outPath, bytesWritten: 512 }); + expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); + // The temp file is gone once the rename has happened, so a + // successful download leaves exactly one file behind. + expect(readdirSync(dir)).toEqual(["replaced.bin"]); + }); + }, +); -- 2.49.1 From 99905277a3f5605caef96b827547dd20259cde22 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 01:59:28 +0000 Subject: [PATCH 2/4] Verify secretstream TAG_FINAL and write downloads atomically (closes #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit streamDecrypt discarded the secretstream tag, so a download cut short by a dropped connection decrypted cleanly up to the last whole chunk and was returned as a success. downloadFile and downloadThumbnail then wrote straight to the destination, and runBackup skips any existing non-empty file, so a truncated original was treated as complete on every subsequent run and never repaired. streamDecrypt now tracks the tag of each chunk it pulls and throws if the stream ended on anything other than TAG_FINAL, or if the body carried no chunks at all — Ente always emits at least one chunk, as encryptBlob shows by producing a TAG_FINAL chunk even for zero-length plaintext, so an empty body is a failed transfer rather than an empty file. Both error messages say the stream was truncated. Plaintext is now staged in a temporary sibling file (same directory, so the rename cannot cross a filesystem boundary; random UUID suffix, so concurrent downloads cannot collide) and renamed into place only after the whole stream has decrypted and verified. On any error the temporary file is removed and the original error is rethrown unchanged, so a cleanup failure never masks the real diagnosis. A failed download therefore leaves the destination exactly as it was. Public signatures and the DownloadResult shape are unchanged. The download layer keeps its no-direct-sodium-import shape: TAG_FINAL is re-exported from src/crypto as STREAM_TAG_FINAL, which decryptBlob now uses too. Also moves the pullStreamChunk doc comment off decryptBlob, where it had been sitting. Retry and backoff remain out of scope; they stay the Next Step in TODO.md and are tracked separately. --- TODO.md | 4 ++ src/crypto/index.ts | 1 + src/crypto/stream.ts | 20 +++++++-- src/download/index.ts | 58 +++++++++++++++++++++++--- test/download/download.test.ts | 75 +++++++++++++++++++++++++--------- 5 files changed, 129 insertions(+), 29 deletions(-) diff --git a/TODO.md b/TODO.md index 9efc22a..f1e961d 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,10 @@ downloads, cover it with mock-server tests, and update the README TODO checkbox. # Completed Steps +- 2026-08-09: Downloads verify the secretstream terminated on `TAG_FINAL` and + write output atomically: a truncated body is rejected instead of landing on + disk as a short file, and plaintext is staged in a sibling temp file and + renamed into place, so a failed download leaves the destination untouched. - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile shims, README Entrypoints section - 2026-06-10: Decrypted collections shared by other users (sealed-box keys); diff --git a/src/crypto/index.ts b/src/crypto/index.ts index ef129f7..3cd569a 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -14,5 +14,6 @@ export { pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, + STREAM_TAG_FINAL, type StreamPullState, } from "./stream.js"; diff --git a/src/crypto/stream.ts b/src/crypto/stream.ts index f255c59..7799cdd 100644 --- a/src/crypto/stream.ts +++ b/src/crypto/stream.ts @@ -8,6 +8,18 @@ export const STREAM_CHUNK_SIZE = 4 * 1024 * 1024; // 16 bytes of Poly1305 tag plus 1 byte of secretstream tag. export const STREAM_CHUNK_OVERHEAD = 17; +// The secretstream tag that marks the last chunk of a stream, i.e. +// libsodium's crypto_secretstream_xchacha20poly1305_TAG_FINAL. Exported so +// callers (the download layer) can detect truncation without importing +// sodium themselves. +// +// Written as a literal because libsodium only attaches its constants to the +// module object once `sodium.ready` has resolved, which is long after this +// module is evaluated. The value is fixed at 3 by the secretstream wire +// format; test/crypto/stream.test.ts pins it against libsodium's own +// constant so the two cannot drift apart unnoticed. +export const STREAM_TAG_FINAL = 3; + // Encrypt a small blob as a single secretstream chunk with TAG_FINAL. // Returns the header and ciphertext. Used for encrypting thumbnails // and metadata before upload. @@ -37,9 +49,6 @@ export const initStreamPull = ( ): StreamPullState => sodium.crypto_secretstream_xchacha20poly1305_init_pull(header, key); -// Decrypt one ciphertext chunk. Returns the plaintext and the secretstream -// tag (0=MESSAGE, 1=PUSH, 2=REKEY, 3=FINAL). The caller should verify the -// stream ended on TAG_FINAL to detect truncation. // Decrypt a small blob that was encrypted as a single secretstream chunk // with TAG_FINAL. Ente uses this form ("blob") for file metadata and // magic metadata — anything under ~1 MiB that isn't chunked. @@ -50,12 +59,15 @@ export const decryptBlob = ( ): Uint8Array => { const state = initStreamPull(header, key); const { plaintext, tag } = pullStreamChunk(state, ciphertext); - if (tag !== sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL) { + if (tag !== STREAM_TAG_FINAL) { throw new Error(`decryptBlob: expected TAG_FINAL (3), got tag ${tag}`); } return plaintext; }; +// Decrypt one ciphertext chunk. Returns the plaintext and the secretstream +// tag (0=MESSAGE, 1=PUSH, 2=REKEY, 3=FINAL). The caller must verify the +// stream ended on TAG_FINAL to detect truncation. export const pullStreamChunk = ( state: StreamPullState, ciphertext: Uint8Array, diff --git a/src/download/index.ts b/src/download/index.ts index 10610d7..0d6dd30 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -1,10 +1,13 @@ -import { writeFile } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { rename, rm, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; import { fromBase64, initStreamPull, pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, + STREAM_TAG_FINAL, } from "../crypto/index.js"; import type { ApiClient } from "../api/client.js"; import type { EnteFile } from "../model/types.js"; @@ -26,6 +29,8 @@ const streamDecrypt = async ( let buffer = new Uint8Array(0); const plainChunks: Uint8Array[] = []; let totalPlain = 0; + let chunksPulled = 0; + let lastTag = -1; for (;;) { const { done, value } = await reader.read(); @@ -39,21 +44,41 @@ const streamDecrypt = async ( while (buffer.length >= ENC_CHUNK_SIZE) { const encChunk = buffer.slice(0, ENC_CHUNK_SIZE); buffer = buffer.slice(ENC_CHUNK_SIZE); - const { plaintext } = pullStreamChunk(state, encChunk); + const { plaintext, tag } = pullStreamChunk(state, encChunk); plainChunks.push(plaintext); totalPlain += plaintext.length; + chunksPulled++; + lastTag = tag; } if (done) { if (buffer.length > 0) { - const { plaintext } = pullStreamChunk(state, buffer); + const { plaintext, tag } = pullStreamChunk(state, buffer); plainChunks.push(plaintext); totalPlain += plaintext.length; + chunksPulled++; + lastTag = tag; } break; } } + // Only the last chunk of a secretstream carries TAG_FINAL. Everything a + // dropped connection did deliver still decrypts and authenticates, so the + // absence of TAG_FINAL is the only evidence that the body was cut short. + // Returning a short plaintext here would put a corrupt file on disk that + // later backup runs would treat as complete. + if (chunksPulled === 0) { + throw new Error( + "download: stream truncated: response body contained no secretstream chunks", + ); + } + if (lastTag !== STREAM_TAG_FINAL) { + throw new Error( + `download: stream truncated: last chunk tag ${lastTag}, expected TAG_FINAL (${STREAM_TAG_FINAL})`, + ); + } + const result = new Uint8Array(totalPlain); let offset = 0; for (const chunk of plainChunks) { @@ -63,6 +88,29 @@ const streamDecrypt = async ( return result; }; +// Write `plaintext` to `destination` atomically: stage it in a temporary +// sibling file (same directory, so the rename cannot cross a filesystem +// boundary) and rename it into place. Callers therefore never observe a +// partially written destination, and a pre-existing file at that path is +// replaced only once the new contents are complete on disk. +const writeAtomic = async ( + destination: string, + plaintext: Uint8Array, +): Promise => { + // The random suffix keeps concurrent downloads of the same destination + // from stepping on each other's temporary file. + const tmpPath = join(dirname(destination), `.quak-${randomUUID()}.tmp`); + try { + await writeFile(tmpPath, plaintext); + await rename(tmpPath, destination); + } catch (err) { + // Best-effort cleanup. A failure to remove the temporary file must + // never replace the error that actually explains what went wrong. + await rm(tmpPath, { force: true }).catch(() => undefined); + throw err; + } +}; + export const downloadFile = async ( api: ApiClient, file: EnteFile, @@ -72,7 +120,7 @@ export const downloadFile = async ( const stream = await api.getFileStream(file.id); const header = fromBase64(file.file.decryptionHeader); const plaintext = await streamDecrypt(stream, header, file.key); - await writeFile(resolvedPath, plaintext); + await writeAtomic(resolvedPath, plaintext); return { path: resolvedPath, bytesWritten: plaintext.length }; }; @@ -85,6 +133,6 @@ export const downloadThumbnail = async ( const stream = await api.getThumbnailStream(file.id); const header = fromBase64(file.thumbnail.decryptionHeader); const plaintext = await streamDecrypt(stream, header, file.key); - await writeFile(resolvedPath, plaintext); + await writeAtomic(resolvedPath, plaintext); return { path: resolvedPath, bytesWritten: plaintext.length }; }; diff --git a/test/download/download.test.ts b/test/download/download.test.ts index 2209857..579991a 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -45,6 +45,7 @@ import { } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; +import { createHash } from "node:crypto"; import sodium from "libsodium-wrappers-sumo"; import { beforeAll, afterAll, describe, expect, it } from "vitest"; import { init, toBase64, STREAM_CHUNK_SIZE } from "../../src/crypto/index.js"; @@ -188,6 +189,42 @@ const mockFetchForBody = (body: Uint8Array) => { return fake as typeof globalThis.fetch; }; +/** + * Encrypt a body consisting of one chunk that is *not* tagged TAG_FINAL. + * + * This is the cheap way to present a stream that ended without its final + * chunk: the downloader pulls it, authenticates it, and finds the stream + * over on a TAG_MESSAGE chunk — the same terminal condition as a large file + * whose last chunk was lost, without paying for a 4 MiB fixture. The + * multi-chunk fixture above covers the realistic wire shape; this one is + * used where the test is really about what happens on disk afterwards. + */ +const encryptNonFinalBody = ( + plaintext: Uint8Array, + key: Uint8Array, +): { header: Uint8Array; ciphertext: Uint8Array } => { + const push = sodium.crypto_secretstream_xchacha20poly1305_init_push(key); + const ciphertext = sodium.crypto_secretstream_xchacha20poly1305_push( + push.state, + plaintext, + null, + sodium.crypto_secretstream_xchacha20poly1305_TAG_MESSAGE, + ); + return { header: push.header, ciphertext }; +}; + +/** + * Compare file contents by digest rather than with `toEqual`. Vitest's deep + * equality walks multi-megabyte buffers byte by byte, which costs seconds on + * the 4 MiB fixtures; a digest comparison is exact and effectively free. + */ +const expectSameBytes = (actual: Uint8Array, expected: Uint8Array): void => { + expect(actual.length).toBe(expected.length); + expect(createHash("sha256").update(actual).digest("hex")).toBe( + createHash("sha256").update(expected).digest("hex"), + ); +}; + /** * A multi-chunk fixture shared by the truncation tests. Building it costs a * few MiB of encryption, so it is built once: one full 4 MiB `TAG_MESSAGE` @@ -291,7 +328,7 @@ describe("downloadFile", () => { const result = await downloadFile(api, file, outPath); expect(result.bytesWritten).toBe(100_000); - expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); + expectSameBytes(readFileSync(outPath), plaintext); }); it("decrypts a body that spans several secretstream chunks", async () => { @@ -311,9 +348,7 @@ describe("downloadFile", () => { const result = await downloadFile(api, file, outPath); expect(result.bytesWritten).toBe(multiChunk.plaintext.length); - expect(readFileSync(outPath)).toEqual( - Buffer.from(multiChunk.plaintext), - ); + expectSameBytes(readFileSync(outPath), multiChunk.plaintext); }); }); @@ -402,15 +437,18 @@ describe.each(entryPoints)( // The caller's contract: if the promise rejects, the destination // path does not exist. Nothing downstream should ever have to guess // whether a leftover file is complete. - const truncatedBody = multiChunk.body.slice( - 0, - multiChunk.finalChunkOffset, - ); - const { api, file } = fixtureFor( - multiChunkKey, - multiChunk.header, - truncatedBody, + // + // The body here is a single chunk that was never tagged TAG_FINAL, + // which puts the downloader in exactly the terminal state a lost + // last chunk produces, without the cost of a 4 MiB fixture. What + // this test is really about is the state of the filesystem after + // the rejection. + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptNonFinalBody( + sodium.randombytes_buf(256), + key, ); + const { api, file } = fixtureFor(key, header, ciphertext); const dir = freshDir(); const outPath = join(dir, "absent.bin"); @@ -458,15 +496,12 @@ describe.each(entryPoints)( const existing = new TextEncoder().encode( "previously downloaded, known-good contents", ); - const truncatedBody = multiChunk.body.slice( - 0, - multiChunk.finalChunkOffset, - ); - const { api, file } = fixtureFor( - multiChunkKey, - multiChunk.header, - truncatedBody, + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptNonFinalBody( + sodium.randombytes_buf(256), + key, ); + const { api, file } = fixtureFor(key, header, ciphertext); const dir = freshDir(); const outPath = join(dir, "existing.bin"); writeFileSync(outPath, existing); -- 2.49.1 From 8a200be8a75eb5d51e83c286832ab2daa155d5af Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 02:23:39 +0000 Subject: [PATCH 3/4] Read TAG_FINAL from libsodium, detect partial chunks, cover the atomic write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework against review findings on the branch. Test runtime (B1): the suite's cost was never the 4 MiB fixture, it was filling that fixture from the CSPRNG. sodium.randombytes_buf goes through the wasm wrapper a byte at a time and takes ~20s for 4 MiB, against ~108ms to encrypt the same buffer. Fixture content is not load-bearing anywhere in the file — only length and tag are — so payloads now come from a seeded LCG instead, which also makes them deterministic and reproducible as the README asks. A seeded generator rather than a constant fill, so a downloader that reordered or repeated chunks would still be caught. make test goes from over the 30s cap in script/test (it was failing outright, then rerunning verbose) back to 8.96s, against 10.02s on main. Atomic write coverage (B2): every failure the suite injected originated in streamDecrypt, which runs before anything is written, so no test observed a temp file existing or being cleaned up and the catch block in writeAtomic was dead code. rename is now intercepted in the test file, which adds two cases per entry point: one asserting the staged file exists at rename time and is a sibling of the destination, and one failing the rename itself so the cleanup path runs with a temp file genuinely on disk. Deleting writeAtomic in favour of a plain writeFile now turns the suite red. TAG_FINAL (M1): STREAM_TAG_FINAL was a hardcoded 3 plus a test to detect drift. The premise was right — libsodium attaches its constants inside ready.then(...), so an eager module-level read binds undefined — but a lazy read works, and decryptBlob was doing exactly that before. Replaced with streamTagFinal(), which reads the library's own value at call time. The drift test is repurposed to pin the accessor against the tag observed on a real final chunk, which fails if it is ever made eager again. Partial trailing chunk (M2): a transfer that stopped mid-chunk surfaced as "authentication failed", which reads as corruption and sends the user after the wrong problem. A final chunk that arrived in full always authenticates, so trailing bytes that do not are reported as the truncation they almost always are, with the authentication failure kept as the error's cause. Poly1305 cannot separate a partial chunk from a corrupt one, so the message names both possibilities; a corrupt whole chunk mid-stream is still reported as an authentication failure, and both are now tested. The pre-existing test that wrote its output to the process working directory, i.e. the repo root, now writes into the test's temp directory. --- src/crypto/index.ts | 2 +- src/crypto/stream.ts | 27 ++-- src/download/index.ts | 31 +++- test/crypto/stream.test.ts | 36 +++-- test/download/download.test.ts | 274 +++++++++++++++++++++++++++++---- 5 files changed, 313 insertions(+), 57 deletions(-) diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 3cd569a..4e3e53b 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -14,6 +14,6 @@ export { pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, - STREAM_TAG_FINAL, + streamTagFinal, type StreamPullState, } from "./stream.js"; diff --git a/src/crypto/stream.ts b/src/crypto/stream.ts index 7799cdd..cb48818 100644 --- a/src/crypto/stream.ts +++ b/src/crypto/stream.ts @@ -8,17 +8,17 @@ export const STREAM_CHUNK_SIZE = 4 * 1024 * 1024; // 16 bytes of Poly1305 tag plus 1 byte of secretstream tag. export const STREAM_CHUNK_OVERHEAD = 17; -// The secretstream tag that marks the last chunk of a stream, i.e. -// libsodium's crypto_secretstream_xchacha20poly1305_TAG_FINAL. Exported so -// callers (the download layer) can detect truncation without importing -// sodium themselves. +// libsodium's crypto_secretstream_xchacha20poly1305_TAG_FINAL: the tag that +// marks the last chunk of a stream. Exported so callers (the download layer) +// can detect truncation without importing sodium themselves. // -// Written as a literal because libsodium only attaches its constants to the -// module object once `sodium.ready` has resolved, which is long after this -// module is evaluated. The value is fixed at 3 by the secretstream wire -// format; test/crypto/stream.test.ts pins it against libsodium's own -// constant so the two cannot drift apart unnoticed. -export const STREAM_TAG_FINAL = 3; +// 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. +export const streamTagFinal = (): number => + sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL; // Encrypt a small blob as a single secretstream chunk with TAG_FINAL. // Returns the header and ciphertext. Used for encrypting thumbnails @@ -59,8 +59,11 @@ export const decryptBlob = ( ): Uint8Array => { const state = initStreamPull(header, key); const { plaintext, tag } = pullStreamChunk(state, ciphertext); - if (tag !== STREAM_TAG_FINAL) { - throw new Error(`decryptBlob: expected TAG_FINAL (3), got tag ${tag}`); + const tagFinal = streamTagFinal(); + if (tag !== tagFinal) { + throw new Error( + `decryptBlob: expected TAG_FINAL (${tagFinal}), got tag ${tag}`, + ); } return plaintext; }; diff --git a/src/download/index.ts b/src/download/index.ts index 0d6dd30..6e7a6b1 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -7,7 +7,7 @@ import { pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, - STREAM_TAG_FINAL, + streamTagFinal, } from "../crypto/index.js"; import type { ApiClient } from "../api/client.js"; import type { EnteFile } from "../model/types.js"; @@ -53,11 +53,27 @@ const streamDecrypt = async ( if (done) { if (buffer.length > 0) { - const { plaintext, tag } = pullStreamChunk(state, buffer); - plainChunks.push(plaintext); - totalPlain += plaintext.length; + // Whatever is left over once every whole chunk has been + // consumed must be the stream's final chunk, and a final + // chunk that actually arrived in full authenticates. If it + // does not, the body stopped part-way through a chunk — the + // ordinary shape of a dropped connection. Poly1305 cannot + // tell a partial chunk from a corrupt one, so this is + // reported as the truncation it almost always is, with the + // authentication failure kept as the error's cause. + let pulled; + try { + pulled = pullStreamChunk(state, buffer); + } catch (err) { + throw new Error( + `download: stream truncated: response body ended with ${buffer.length} trailing bytes that did not authenticate as a final chunk (transfer stopped mid-chunk, or the data is corrupt)`, + { cause: err }, + ); + } + plainChunks.push(pulled.plaintext); + totalPlain += pulled.plaintext.length; chunksPulled++; - lastTag = tag; + lastTag = pulled.tag; } break; } @@ -73,9 +89,10 @@ const streamDecrypt = async ( "download: stream truncated: response body contained no secretstream chunks", ); } - if (lastTag !== STREAM_TAG_FINAL) { + const tagFinal = streamTagFinal(); + if (lastTag !== tagFinal) { throw new Error( - `download: stream truncated: last chunk tag ${lastTag}, expected TAG_FINAL (${STREAM_TAG_FINAL})`, + `download: stream truncated: last chunk tag ${lastTag}, expected TAG_FINAL (${tagFinal})`, ); } diff --git a/test/crypto/stream.test.ts b/test/crypto/stream.test.ts index b3544d3..f4b2707 100644 --- a/test/crypto/stream.test.ts +++ b/test/crypto/stream.test.ts @@ -16,7 +16,8 @@ * * These tests pin: * - The chunk-size constants match Ente's expectations, and the - * re-exported `STREAM_TAG_FINAL` matches libsodium's own constant. + * re-exported `streamTagFinal()` matches the tag a real final chunk + * carries. * - 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. @@ -31,7 +32,7 @@ import { pullStreamChunk, STREAM_CHUNK_OVERHEAD, STREAM_CHUNK_SIZE, - STREAM_TAG_FINAL, + streamTagFinal, } from "../../src/crypto/index.js"; describe("crypto stream constants", () => { @@ -49,19 +50,36 @@ describe("crypto stream constants", () => { }); /** - * `STREAM_TAG_FINAL` is re-exported so callers can detect a truncated + * `streamTagFinal()` is re-exported so callers can detect a truncated * stream (a body that ended on a non-final chunk) without importing - * libsodium themselves. It has to be declared as a literal, because - * libsodium only attaches its own constants to the module object after - * `sodium.ready` resolves — long after this library's modules are - * evaluated. This test is what keeps the literal honest. + * libsodium themselves. + * + * It is a function, not a constant, and that is load-bearing: libsodium + * attaches its own constants to the module object only after + * `sodium.ready` resolves, which is long after this library's modules are + * 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. */ - it("STREAM_TAG_FINAL equals libsodium's TAG_FINAL", async () => { + it("streamTagFinal() is the tag carried by a real final chunk", async () => { await init(); await sodium.ready; - expect(STREAM_TAG_FINAL).toBe( + + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const push = + sodium.crypto_secretstream_xchacha20poly1305_init_push(key); + const ciphertext = sodium.crypto_secretstream_xchacha20poly1305_push( + push.state, + new TextEncoder().encode("last chunk"), + null, sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL, ); + + const state = initStreamPull(push.header, key); + expect(pullStreamChunk(state, ciphertext).tag).toBe(streamTagFinal()); }); }); diff --git a/test/download/download.test.ts b/test/download/download.test.ts index 579991a..db50a9d 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -20,6 +20,8 @@ * an explicit `TAG_FINAL` check a truncated body is indistinguishable from * a complete one. `streamDecrypt` therefore refuses to return unless the * stream ended on `TAG_FINAL`, and the error says the stream was truncated. + * A transfer that stopped part-way through a chunk is reported the same + * way, since a final chunk that arrived in full always authenticates. * * 2. **The destination path is written atomically.** Plaintext goes to a * temporary sibling file first and is `rename`d into place only after the @@ -29,7 +31,8 @@ * untouched — whatever was there before is still there, byte for byte, and * no partial file has appeared. This matters because `runBackup` skips any * existing non-empty file, so a partial write would be treated as complete - * forever after. + * forever after. The staging file and the rename are observed directly (see + * the `rename` hook below), not inferred from an empty directory. * * These tests build synthetic encrypted files using sodium's push API, * serve them from a mock fetch, and verify the decrypted output on disk. @@ -43,11 +46,19 @@ import { mkdtempSync, writeFileSync, } from "node:fs"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { createHash } from "node:crypto"; import sodium from "libsodium-wrappers-sumo"; -import { beforeAll, afterAll, describe, expect, it } from "vitest"; +import { + beforeAll, + beforeEach, + afterAll, + describe, + expect, + it, + vi, +} from "vitest"; import { init, toBase64, STREAM_CHUNK_SIZE } from "../../src/crypto/index.js"; import { ApiClient } from "../../src/api/client.js"; import { downloadFile, downloadThumbnail } from "../../src/download/index.js"; @@ -57,6 +68,52 @@ import type { EnteFile, FileMetadata } from "../../src/model/types.js"; // Test helpers // --------------------------------------------------------------------------- +/** + * `rename` is intercepted so that the tests can observe — and fail — the + * final step of the atomic write. + * + * Every other failure in this file is injected inside `streamDecrypt`, which + * runs before anything is written to disk. Those tests therefore cannot tell + * an atomic write from a plain `writeFile`: under both, nothing was ever + * created, so an empty directory proves nothing about cleanup. This hook is + * what closes that gap. It records each rename the downloader performs, + * including whether the source existed at that moment (i.e. that the staged + * temp file really was written), and can be told to fail the rename so the + * cleanup path runs with a temp file genuinely on disk. + * + * `vi.hoisted` is required: `vi.mock` factories are hoisted above the imports, + * so a plain module-level `const` would still be in its temporal dead zone by + * the time the factory runs. + */ +const renameHook = vi.hoisted(() => ({ + calls: [] as { from: string; to: string; sourceExisted: boolean }[], + failWith: null as Error | null, +})); + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + const { existsSync: sourceExists } = await import("node:fs"); + return { + ...actual, + rename: async (from: string, to: string): Promise => { + renameHook.calls.push({ + from, + to, + sourceExisted: sourceExists(from), + }); + if (renameHook.failWith !== null) { + throw renameHook.failWith; + } + await actual.rename(from, to); + }, + }; +}); + +beforeEach(() => { + renameHook.calls.length = 0; + renameHook.failWith = null; +}); + let testDir: string; beforeAll(async () => { @@ -71,6 +128,35 @@ afterAll(() => { } }); +/** + * Deterministic stand-in for random test payloads. + * + * Fixture *content* is never load-bearing here — the assertions turn on + * length, framing, and the secretstream tag — but it must not be a constant + * fill either, or a downloader that reordered or repeated chunks would still + * produce the expected bytes. A seeded linear congruential generator gives + * both: byte patterns that differ across every offset and seed, reproducible + * on any machine, which is what the README asks of fixtures. + * + * It is also the difference between a fast suite and a broken one. + * `sodium.randombytes_buf` goes through the wasm wrapper a byte at a time and + * costs roughly 20 seconds for the 4 MiB chunk below — about two hundred + * times what it costs to encrypt the same buffer, and on its own enough to + * push `make test` past the 30-second cap in `script/test`. This loop fills + * 4 MiB in a few milliseconds. + */ +const patternBytes = (length: number, seed: number): Uint8Array => { + const out = new Uint8Array(length); + let x = seed >>> 0; + for (let i = 0; i < length; i++) { + // Numerical Recipes' LCG constants; the high byte is used because + // the low bits of an LCG have short periods. + x = (Math.imul(x, 1664525) + 1013904223) >>> 0; + out[i] = (x >>> 24) & 0xff; + } + return out; +}; + /** * Encrypt `plaintext` as a secretstream file body (single chunk with * TAG_FINAL). Returns the key, header, and ciphertext that the mock CDN @@ -120,7 +206,7 @@ const encryptMultiChunkBody = ( const plainParts: Uint8Array[] = []; for (let i = 0; i < leadingChunks; i++) { - const plain = sodium.randombytes_buf(STREAM_CHUNK_SIZE); + const plain = patternBytes(STREAM_CHUNK_SIZE, i + 1); plainParts.push(plain); cipherParts.push( sodium.crypto_secretstream_xchacha20poly1305_push( @@ -132,7 +218,7 @@ const encryptMultiChunkBody = ( ); } - const finalPlain = sodium.randombytes_buf(finalChunkPlainSize); + const finalPlain = patternBytes(finalChunkPlainSize, leadingChunks + 1); plainParts.push(finalPlain); const finalCipher = sodium.crypto_secretstream_xchacha20poly1305_push( push.state, @@ -226,9 +312,11 @@ const expectSameBytes = (actual: Uint8Array, expected: Uint8Array): void => { }; /** - * A multi-chunk fixture shared by the truncation tests. Building it costs a - * few MiB of encryption, so it is built once: one full 4 MiB `TAG_MESSAGE` - * chunk followed by a small `TAG_FINAL` chunk. + * A multi-chunk fixture shared by the truncation tests: one full 4 MiB + * `TAG_MESSAGE` chunk followed by a small `TAG_FINAL` chunk. It is built once + * and shared because encrypting 4 MiB costs about 100ms. Its plaintext is + * generated rather than drawn from the CSPRNG, which is what keeps that + * encryption the whole cost of the fixture. */ let multiChunk: ReturnType; let multiChunkKey: Uint8Array; @@ -295,28 +383,33 @@ describe("downloadFile", () => { }); it("uses metadata.title as filename when outPath is omitted", async () => { + // With no `outPath`, the destination is `metadata.title`, used + // verbatim as a path. The title here is therefore given inside the + // test's temporary directory: a bare relative name would resolve + // against the process working directory, i.e. the repo root, and + // `make check` must not create files in the repo — a failure between + // the write and any cleanup would leave one behind. const plaintext = new Uint8Array([1, 2, 3]); const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptFileBody(plaintext, key); const thumbPush = sodium.crypto_secretstream_xchacha20poly1305_init_push(key); const file = buildMockEnteFile(key, header, thumbPush.header); - file.metadata.title = "fallback-name.png"; + const titlePath = join(testDir, "fallback-name.png"); + file.metadata.title = titlePath; const api = new ApiClient({ fetch: mockFetchForBody(ciphertext) }); const result = await downloadFile(api, file); - expect(result.path).toBe("fallback-name.png"); + expect(result.path).toBe(titlePath); expect(readFileSync(result.path)).toEqual(Buffer.from(plaintext)); - // Clean up since it writes to cwd - if (existsSync(result.path)) rmSync(result.path); }); it("handles a larger single-chunk file (random binary payload)", async () => { // Most photos are under 4 MiB and therefore a single secretstream // chunk. This test exercises a non-trivial payload size with - // random binary data (not just ASCII) to verify no encoding bugs. - const plaintext = sodium.randombytes_buf(100_000); + // arbitrary binary data (not just ASCII) to verify no encoding bugs. + const plaintext = patternBytes(100_000, 11); const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptFileBody(plaintext, key); const thumbPush = @@ -414,6 +507,49 @@ describe.each(entryPoints)( ); }); + it("rejects a body whose final chunk arrived only in part", async () => { + // The likelier shape of a dropped connection: the transfer stops + // in the middle of a chunk rather than neatly between two. The + // bytes that arrived are a prefix of a complete chunk, so Poly1305 + // rejects them — which is, cryptographically, indistinguishable + // from corruption of a whole chunk. + // + // It is still reported as truncation, because that is what it + // almost always is and because this library's entire reason for + // checking TAG_FINAL is to make a short transfer visible. Calling + // a short transfer "authentication failed" would send a user + // hunting for a corrupt file when their network is at fault. The + // underlying authentication failure is kept as the error's + // `cause`, so the real diagnosis is never lost. + const shortBody = multiChunk.body.slice( + 0, + multiChunk.body.length - 8, + ); + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + shortBody, + ); + const dir = freshDir(); + const outPath = join(dir, "partial-final.bin"); + + const err = await download(api, file, outPath).catch( + (e: unknown) => e, + ); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toMatch(/truncated/i); + expect((err as Error).cause).toBeInstanceOf(Error); + expect(((err as Error).cause as Error).message).toMatch( + /authentication failed/i, + ); + + // And, as with every other failure, the destination is untouched + // and no staged temp file survives. + expect(existsSync(outPath)).toBe(false); + expect(readdirSync(dir)).toEqual([]); + }); + it("rejects an empty body instead of writing a zero-byte file", async () => { // Ente always emits at least one chunk, even for empty content: // `encryptBlob` shows that a zero-length plaintext still produces a @@ -445,7 +581,7 @@ describe.each(entryPoints)( // the rejection. const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptNonFinalBody( - sodium.randombytes_buf(256), + patternBytes(256, 21), key, ); const { api, file } = fixtureFor(key, header, ciphertext); @@ -463,19 +599,25 @@ describe.each(entryPoints)( expect(readdirSync(dir)).toEqual([]); }); - it("leaves no file at the destination when a chunk fails authentication", async () => { - // The same guarantee has to hold for every failure mode, not just - // truncation. Here a byte of ciphertext is flipped, so Poly1305 - // verification fails inside `pullStreamChunk`. The error must - // propagate unchanged (it is the real diagnosis) and the destination - // must still be untouched. - const plaintext = sodium.randombytes_buf(256); - const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); - const { header, ciphertext } = encryptFileBody(plaintext, key); - const corrupted = Uint8Array.from(ciphertext); + it("reports a corrupt whole chunk as an authentication failure, not truncation", async () => { + // The counterpart to the partial-final-chunk case above, and the + // reason the two are distinguishable at all. A byte is flipped + // inside the first chunk of a multi-chunk body: that chunk arrives + // complete — the stream goes on past it — so its failure cannot be + // a short transfer. It is corruption, and the caller is told so, + // with `pullStreamChunk`'s error propagated unchanged because it is + // the real diagnosis. + // + // The same on-disk guarantee holds for this failure mode as for + // every other: nothing at the destination, nothing left over. + const corrupted = Uint8Array.from(multiChunk.body); corrupted[10] ^= 0xff; - const { api, file } = fixtureFor(key, header, corrupted); + const { api, file } = fixtureFor( + multiChunkKey, + multiChunk.header, + corrupted, + ); const dir = freshDir(); const outPath = join(dir, "corrupt.bin"); @@ -498,7 +640,7 @@ describe.each(entryPoints)( ); const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptNonFinalBody( - sodium.randombytes_buf(256), + patternBytes(256, 22), key, ); const { api, file } = fixtureFor(key, header, ciphertext); @@ -518,7 +660,7 @@ describe.each(entryPoints)( // The mirror image of the previous test: a complete download does // overwrite whatever was at the destination, atomically, via rename. const existing = new TextEncoder().encode("stale contents"); - const plaintext = sodium.randombytes_buf(512); + const plaintext = patternBytes(512, 23); const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); const { header, ciphertext } = encryptFileBody(plaintext, key); @@ -535,5 +677,81 @@ describe.each(entryPoints)( // successful download leaves exactly one file behind. expect(readdirSync(dir)).toEqual(["replaced.bin"]); }); + + it("stages the plaintext in a sibling temp file and renames it into place", async () => { + // The atomic write, observed directly rather than inferred from an + // empty directory. Every other failure in this file is injected + // inside `streamDecrypt`, which runs before anything is written — + // so under those tests a plain `writeFile` to the destination would + // look identical. This one watches the rename itself. + // + // Two properties are load-bearing. The staging file must exist on + // disk when the rename happens: that is what makes the destination + // appear complete or not at all, instead of filling up as bytes + // land. And it must be a sibling of the destination, because + // `rename` is only atomic within one filesystem — staging in + // `/tmp` and renaming across a mount point would silently become a + // copy, reintroducing the partial-file window this exists to close. + const plaintext = patternBytes(512, 31); + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody(plaintext, key); + const { api, file } = fixtureFor(key, header, ciphertext); + const dir = freshDir(); + const outPath = join(dir, "staged.bin"); + + await download(api, file, outPath); + + expect(renameHook.calls).toHaveLength(1); + const staged = renameHook.calls[0]!; + expect(staged.to).toBe(outPath); + expect(staged.from).not.toBe(outPath); + expect(dirname(staged.from)).toBe(dir); + expect(staged.sourceExisted).toBe(true); + + // Afterwards the temp file is gone and only the destination is + // left, holding the complete plaintext. + expect(existsSync(staged.from)).toBe(false); + expect(readFileSync(outPath)).toEqual(Buffer.from(plaintext)); + expect(readdirSync(dir)).toEqual(["staged.bin"]); + }); + + it("removes the staged temp file when the rename itself fails", async () => { + // The cleanup path. It can only run when something fails at or + // after the write, which no amount of bad network data can + // produce: by the time anything is written the whole stream has + // already decrypted and verified. Failing the rename is what + // reaches it — a real possibility on a full disk, a read-only + // mount, or a destination that has become a directory. + // + // This is the only case in which the temp file is on disk at the + // moment of failure, so it is the only one that can show it is + // actually removed rather than merely never created. It also pins + // that the caller sees the original failure: a cleanup that threw + // over the top of it would hide why the download failed. + const existing = new TextEncoder().encode("known-good contents"); + const plaintext = patternBytes(512, 32); + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody(plaintext, key); + const { api, file } = fixtureFor(key, header, ciphertext); + const dir = freshDir(); + const outPath = join(dir, "rename-fails.bin"); + writeFileSync(outPath, existing); + + renameHook.failWith = new Error("simulated rename failure"); + + await expect(download(api, file, outPath)).rejects.toThrow( + "simulated rename failure", + ); + + expect(renameHook.calls).toHaveLength(1); + const staged = renameHook.calls[0]!; + expect(staged.sourceExisted).toBe(true); + expect(existsSync(staged.from)).toBe(false); + + // The previous contents are still there, untouched, and the + // directory holds nothing else. + expect(readFileSync(outPath)).toEqual(Buffer.from(existing)); + expect(readdirSync(dir)).toEqual(["rename-fails.bin"]); + }); }, ); -- 2.49.1 From 937bcb7aeebd97462ce39c4117b258bcb55b5b01 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 02:45:35 +0000 Subject: [PATCH 4/4] 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", () => { -- 2.49.1