sneak/quak
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Downloads: verify secretstream TAG_FINAL and write output atomically #1

New Issue
Closed
opened 2026-08-09 03:42:25 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 03:42:25 +02:00
Collaborator
Copy Link

Problem

streamDecrypt in src/download/index.ts discards the secretstream tag returned by
pullStreamChunk (lines 42 and 49) and never checks that the stream terminated on
TAG_FINAL, even though the comment in src/crypto/stream.ts:40-42 explicitly says the
caller must. A download truncated by a dropped connection therefore decrypts cleanly up to
the last whole chunk and is written to disk as a successful result.

Both downloadFile (src/download/index.ts:66-77) and downloadThumbnail (:79-90)
then writeFile straight to the final destination path. runBackup (src/backup.ts:91)
skips any existing file whose size is greater than zero, so a truncated original is treated
as complete on every subsequent run and is never repaired. A resilient backup that silently
writes corrupt files is worse than one that crashes.

This is also a prerequisite for the retry-policy work: a retry can never fire on a truncated
download while truncation is indistinguishable from success.

Definition of done

  1. streamDecrypt throws when the stream ends without a chunk carrying
    crypto_secretstream_xchacha20poly1305_TAG_FINAL. The error message must say the stream
    was truncated.
  2. downloadFile and downloadThumbnail write to a temporary path in the same directory
    as the destination and rename into place only after decryption has completed
    successfully.
  3. On any failure (truncation, auth failure, network error) no file exists at the destination
    path, and the temporary file is removed.
  4. The public signatures and the DownloadResult shape are unchanged.
  5. Tests added to test/download/download.test.ts covering, for both downloadFile and
    downloadThumbnail:
    • a body whose final chunk is missing is rejected with a truncation error;
    • after such a rejection, no file exists at the destination path;
    • an existing file at the destination path is not clobbered by a failed download;
    • the existing success cases still produce identical bytes and identical DownloadResult.
  6. make check is green.
  7. TODO.md is updated in the same commit (add to Completed Steps; leave the retry policy as
    the Next Step, since that is tracked separately).

Out of scope

Retry and backoff. That is a separate issue and must not be started here.

## Problem `streamDecrypt` in `src/download/index.ts` discards the secretstream tag returned by `pullStreamChunk` (lines 42 and 49) and never checks that the stream terminated on `TAG_FINAL`, even though the comment in `src/crypto/stream.ts:40-42` explicitly says the caller must. A download truncated by a dropped connection therefore decrypts cleanly up to the last whole chunk and is written to disk as a **successful** result. Both `downloadFile` (`src/download/index.ts:66-77`) and `downloadThumbnail` (`:79-90`) then `writeFile` straight to the final destination path. `runBackup` (`src/backup.ts:91`) skips any existing file whose size is greater than zero, so a truncated original is treated as complete on every subsequent run and is never repaired. A resilient backup that silently writes corrupt files is worse than one that crashes. This is also a prerequisite for the retry-policy work: a retry can never fire on a truncated download while truncation is indistinguishable from success. ## Definition of done 1. `streamDecrypt` throws when the stream ends without a chunk carrying `crypto_secretstream_xchacha20poly1305_TAG_FINAL`. The error message must say the stream was truncated. 2. `downloadFile` and `downloadThumbnail` write to a temporary path in the **same directory** as the destination and `rename` into place only after decryption has completed successfully. 3. On any failure (truncation, auth failure, network error) no file exists at the destination path, and the temporary file is removed. 4. The public signatures and the `DownloadResult` shape are unchanged. 5. Tests added to `test/download/download.test.ts` covering, for **both** `downloadFile` and `downloadThumbnail`: - a body whose final chunk is missing is rejected with a truncation error; - after such a rejection, no file exists at the destination path; - an existing file at the destination path is not clobbered by a failed download; - the existing success cases still produce identical bytes and identical `DownloadResult`. 6. `make check` is green. 7. `TODO.md` is updated in the same commit (add to Completed Steps; leave the retry policy as the Next Step, since that is tracked separately). ## Out of scope Retry and backoff. That is a separate issue and must not be started here.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:25 +02:00
clawbot self-assigned this 2026-08-09 03:42:25 +02:00
clawbot commented 2026-08-09 03:42:47 +02:00
Author
Collaborator
Copy Link

Implementation requirements

Where the code lives

  • src/download/index.ts — streamDecrypt (:19-64), downloadFile (:66-77),
    downloadThumbnail (:79-90).
  • src/crypto/stream.ts — pullStreamChunk (:59-71) already returns
    { plaintext, tag }. decryptBlob (:46-57) is the existing precedent for how to check
    the tag; follow that pattern (compare against
    sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL).

Tag verification

  • Keep the last observed tag in streamDecrypt as chunks are pulled. After the read loop
    exits, throw if the last tag was not TAG_FINAL. Also throw if zero chunks were pulled
    (an empty body is a truncated stream, not a zero-byte file — Ente always emits at least one
    chunk, and encryptBlob proves a zero-length plaintext still produces a TAG_FINAL chunk).
  • Do not import libsodium-wrappers-sumo directly into src/download/index.ts. Export a
    named constant from src/crypto/stream.ts (e.g. STREAM_TAG_FINAL) and re-export it via
    src/crypto/index.ts, so the download layer keeps its existing "no direct sodium import"
    shape.
  • While you are in src/crypto/stream.ts, fix the misplaced comment: lines 40-42 document
    pullStreamChunk but sit directly above decryptBlob. Move it to the right function.

Atomic write

  • Temp file must be a sibling of the destination so rename stays on one filesystem. Derive
    the directory with node:path dirname, and give the temp file a random suffix
    (node:crypto randomBytes/randomUUID) so concurrent downloads cannot collide.
  • Use node:fs/promises writeFile then rename. On any thrown error, rm the temp file
    with { force: true } and rethrow the original error — never mask the original failure
    with a cleanup failure.
  • resolvedPath is computed before the request today; keep that ordering so the destination
    name is unchanged.

Do not

  • Do not add retry, backoff, sleeping, or timeouts. Separate issue.
  • Do not change runBackup's skip heuristic. Separate issue.
  • Do not sanitize file.metadata.title. Separate issue.
  • Do not add new runtime dependencies. Everything needed is stdlib.

Tests

  • test/download/download.test.ts already has a mockFetchForBody helper (:83-87) that
    returns the same body for every call. Build the truncated case by slicing the encrypted
    body so the last chunk is dropped; assert with await expect(...).rejects.toThrow(...).
  • Use mkdtempSync for temp directories in tests, per the repo README.
  • Comment the new tests thoroughly enough that a reader learns the truncation contract from
    them alone — tests are this repo's canonical API documentation.

Process

  • Branch off main. First commit is the failing tests, per the README development workflow;
    the implementation lands in a later commit on the same branch.
  • Verify only with make check / script/ entrypoints. Never invoke vitest, eslint,
    prettier, or tsc directly.
  • TODO.md change goes in the same commit as the implementation.
## Implementation requirements **Where the code lives** - `src/download/index.ts` — `streamDecrypt` (`:19-64`), `downloadFile` (`:66-77`), `downloadThumbnail` (`:79-90`). - `src/crypto/stream.ts` — `pullStreamChunk` (`:59-71`) already returns `{ plaintext, tag }`. `decryptBlob` (`:46-57`) is the existing precedent for how to check the tag; follow that pattern (compare against `sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL`). **Tag verification** - Keep the last observed tag in `streamDecrypt` as chunks are pulled. After the read loop exits, throw if the last tag was not `TAG_FINAL`. Also throw if zero chunks were pulled (an empty body is a truncated stream, not a zero-byte file — Ente always emits at least one chunk, and `encryptBlob` proves a zero-length plaintext still produces a `TAG_FINAL` chunk). - Do not import `libsodium-wrappers-sumo` directly into `src/download/index.ts`. Export a named constant from `src/crypto/stream.ts` (e.g. `STREAM_TAG_FINAL`) and re-export it via `src/crypto/index.ts`, so the download layer keeps its existing "no direct sodium import" shape. - While you are in `src/crypto/stream.ts`, fix the misplaced comment: lines 40-42 document `pullStreamChunk` but sit directly above `decryptBlob`. Move it to the right function. **Atomic write** - Temp file must be a sibling of the destination so `rename` stays on one filesystem. Derive the directory with `node:path` `dirname`, and give the temp file a random suffix (`node:crypto` `randomBytes`/`randomUUID`) so concurrent downloads cannot collide. - Use `node:fs/promises` `writeFile` then `rename`. On any thrown error, `rm` the temp file with `{ force: true }` and rethrow the original error — never mask the original failure with a cleanup failure. - `resolvedPath` is computed before the request today; keep that ordering so the destination name is unchanged. **Do not** - Do not add retry, backoff, sleeping, or timeouts. Separate issue. - Do not change `runBackup`'s skip heuristic. Separate issue. - Do not sanitize `file.metadata.title`. Separate issue. - Do not add new runtime dependencies. Everything needed is stdlib. **Tests** - `test/download/download.test.ts` already has a `mockFetchForBody` helper (`:83-87`) that returns the same body for every call. Build the truncated case by slicing the encrypted body so the last chunk is dropped; assert with `await expect(...).rejects.toThrow(...)`. - Use `mkdtempSync` for temp directories in tests, per the repo README. - Comment the new tests thoroughly enough that a reader learns the truncation contract from them alone — tests are this repo's canonical API documentation. **Process** - Branch off `main`. First commit is the failing tests, per the README development workflow; the implementation lands in a later commit on the same branch. - Verify only with `make check` / `script/` entrypoints. Never invoke `vitest`, `eslint`, `prettier`, or `tsc` directly. - `TODO.md` change goes in the same commit as the implementation.
clawbot commented 2026-08-09 03:48:07 +02:00
Author
Collaborator
Copy Link

Implementation plan

Branch download-tag-final-atomic-write off main, TDD per the README workflow: first commit is the failing tests, implementation lands in a later commit.

Commit 1 — failing tests (test/download/download.test.ts)

New helpers alongside the existing encryptFileBody / mockFetchForBody:

  • encryptMultiChunkBody(chunks) — pushes N secretstream chunks, the first N-1 with TAG_MESSAGE at exactly STREAM_CHUNK_SIZE plaintext bytes each (so the download reader's chunk framing lines up) and the last with TAG_FINAL. Returns the header, the full body, and the byte offset where the final chunk starts, so a test can slice the body to drop it.

New cases, for both downloadFile and downloadThumbnail:

  1. multi-chunk body with the TAG_FINAL chunk sliced off rejects with an error whose message says the stream was truncated;
  2. an empty body (zero chunks pulled) rejects the same way — Ente always emits at least one chunk, so an empty body is truncation, not a zero-byte file;
  3. after a truncation rejection, existsSync(destination) is false;
  4. a pre-existing file at the destination is left byte-for-byte intact when the download fails (truncation and a bad-key auth failure);
  5. no stray temp files remain in the destination directory after a failed download (asserted by reading the directory listing);
  6. the existing success cases keep producing identical bytes and an identical DownloadResult (path, bytesWritten).

Every case gets prose comments explaining why the behavior matters, since the tests are this repo's canonical API documentation.

Commit 2 — implementation + TODO.md

src/crypto/stream.ts:

  • export STREAM_TAG_FINAL (= sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL), re-exported from src/crypto/index.ts, so src/download/index.ts keeps its no-direct-sodium-import shape;
  • move the misplaced pullStreamChunk doc comment (currently sitting above decryptBlob) onto pullStreamChunk.

src/download/index.ts:

  • streamDecrypt tracks the last observed tag and a pulled-chunk count; after the read loop it throws a truncation error if zero chunks were pulled or the last tag was not STREAM_TAG_FINAL;
  • downloadFile / downloadThumbnail keep computing resolvedPath before the request, then writeFile to a sibling temp path (dirname(resolvedPath) + random suffix from node:crypto randomUUID) and rename into place only after decryption succeeded. Any thrown error triggers rm(tmp, { force: true }) and rethrows the original error — a cleanup failure never masks the real one.
  • Public signatures and the DownloadResult shape are unchanged.

TODO.md: add a Completed Steps entry for this work; the retry policy stays as the Next Step (tracked as #2). Markdown formatted with make fmt.

Out of scope, deliberately untouched

No retry/backoff/timeouts, no change to runBackup's skip heuristic, no file.metadata.title sanitizing, no new runtime dependencies.

Verification

make check (test + lint + fmt-check) only; no direct vitest/eslint/prettier/tsc invocations. make build is known-broken on main (#3) and is not in scope.

## Implementation plan Branch `download-tag-final-atomic-write` off `main`, TDD per the README workflow: first commit is the failing tests, implementation lands in a later commit. ### Commit 1 — failing tests (`test/download/download.test.ts`) New helpers alongside the existing `encryptFileBody` / `mockFetchForBody`: - `encryptMultiChunkBody(chunks)` — pushes N secretstream chunks, the first N-1 with `TAG_MESSAGE` at exactly `STREAM_CHUNK_SIZE` plaintext bytes each (so the download reader's chunk framing lines up) and the last with `TAG_FINAL`. Returns the header, the full body, and the byte offset where the final chunk starts, so a test can slice the body to drop it. New cases, for both `downloadFile` and `downloadThumbnail`: 1. multi-chunk body with the `TAG_FINAL` chunk sliced off rejects with an error whose message says the stream was truncated; 2. an empty body (zero chunks pulled) rejects the same way — Ente always emits at least one chunk, so an empty body is truncation, not a zero-byte file; 3. after a truncation rejection, `existsSync(destination)` is false; 4. a pre-existing file at the destination is left byte-for-byte intact when the download fails (truncation and a bad-key auth failure); 5. no stray temp files remain in the destination directory after a failed download (asserted by reading the directory listing); 6. the existing success cases keep producing identical bytes and an identical `DownloadResult` (`path`, `bytesWritten`). Every case gets prose comments explaining why the behavior matters, since the tests are this repo's canonical API documentation. ### Commit 2 — implementation + `TODO.md` `src/crypto/stream.ts`: - export `STREAM_TAG_FINAL` (= `sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL`), re-exported from `src/crypto/index.ts`, so `src/download/index.ts` keeps its no-direct-sodium-import shape; - move the misplaced `pullStreamChunk` doc comment (currently sitting above `decryptBlob`) onto `pullStreamChunk`. `src/download/index.ts`: - `streamDecrypt` tracks the last observed tag and a pulled-chunk count; after the read loop it throws a truncation error if zero chunks were pulled or the last tag was not `STREAM_TAG_FINAL`; - `downloadFile` / `downloadThumbnail` keep computing `resolvedPath` before the request, then `writeFile` to a sibling temp path (`dirname(resolvedPath)` + random suffix from `node:crypto` `randomUUID`) and `rename` into place only after decryption succeeded. Any thrown error triggers `rm(tmp, { force: true })` and rethrows the original error — a cleanup failure never masks the real one. - Public signatures and the `DownloadResult` shape are unchanged. `TODO.md`: add a Completed Steps entry for this work; the retry policy stays as the Next Step (tracked as #2). Markdown formatted with `make fmt`. ### Out of scope, deliberately untouched No retry/backoff/timeouts, no change to `runBackup`'s skip heuristic, no `file.metadata.title` sanitizing, no new runtime dependencies. ### Verification `make check` (test + lint + fmt-check) only; no direct `vitest`/`eslint`/`prettier`/`tsc` invocations. `make build` is known-broken on `main` (#3) and is not in scope.
clawbot commented 2026-08-09 04:01:20 +02:00
Author
Collaborator
Copy Link

Implemented in PR #20: #20

Branch download-tag-final-atomic-write, two commits in TDD order — 1f894ba is the tests in a failing state (verified red: 9 failures), 9990527 is the implementation plus the TODO.md update. All seven definition-of-done items are addressed; make check is green (134 tests, eslint and prettier clean).

One deviation worth flagging: STREAM_TAG_FINAL is exported from src/crypto/stream.ts and re-exported from src/crypto/index.ts as requested, but it is declared as the literal 3 rather than read from sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL. libsodium only attaches its constants to the module object after sodium.ready resolves, so a module-level read would bind undefined. test/crypto/stream.test.ts pins the literal against libsodium's own constant so they cannot drift.

The quadratic buffer accumulation in streamDecrypt that this work surfaced is filed separately as #21 and was not touched here.

Implemented in PR #20: https://git.eeqj.de/sneak/quak/pulls/20 Branch `download-tag-final-atomic-write`, two commits in TDD order — `1f894ba` is the tests in a failing state (verified red: 9 failures), `9990527` is the implementation plus the `TODO.md` update. All seven definition-of-done items are addressed; `make check` is green (134 tests, eslint and prettier clean). One deviation worth flagging: `STREAM_TAG_FINAL` is exported from `src/crypto/stream.ts` and re-exported from `src/crypto/index.ts` as requested, but it is declared as the literal `3` rather than read from `sodium.crypto_secretstream_xchacha20poly1305_TAG_FINAL`. libsodium only attaches its constants to the module object after `sodium.ready` resolves, so a module-level read would bind `undefined`. `test/crypto/stream.test.ts` pins the literal against libsodium's own constant so they cannot drift. The quadratic buffer accumulation in `streamDecrypt` that this work surfaced is filed separately as #21 and was not touched here.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI / make check is not green on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR is awaiting adversarial review
 needs-rework
Review failed; PR needs changes
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/quak#1
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?