Skip to content

Make host file saves atomic and route skill reads through one capped read - #6335

Merged
ymichael merged 5 commits into
mainfrom
bb/fix-unsafe-host-write-file-and-unbounded-read-fi-thr_v5h7wv4b4f
Oct 26, 2026
Merged

ymichael merged 5 commits into
mainfrom
bb/fix-unsafe-host-write-file-and-unbounded-read-fi-thr_v5h7wv4b4f

Conversation

@ymichael

@ymichael ymichael commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Human comments

What was wrong

host.write_file without expectedSha256 overwrote its target in place, so a crash or full disk partway through left a truncated file. host.read_file_relative loaded whole files with no size limit and sent them as one base64 WebSocket message. Both bugs came from duplicated paths. There were two whole-file reads: read_file_relative was built for the status-folder server, removed in June, and its only remaining caller was the provider-skill read. There were also three hand-written copies of the temp-file, hash-check, rename save, so a fix in one never reached the others.

What changed

  • One atomic save: new dependency-free package @bb/file-transfer. verified-file-transfer.ts (from Stream prompt attachments from server to daemon without buffering #6332/Transfer skill trees by manifest, fetching only missing files #6330) moved into it unchanged, apart from the durable and mode arguments below. A new replaceFile does the whole save:
    1. check the expected hash;
    2. write and fsync a fixed-length .bb-write-<uuid> temp file next to the target;
    3. put it in place:
      • no hash: rename over the target;
      • expectedSha256: null: no-clobber link;
      • a hash: re-check, then rename.
    • Who uses it: writeHostFile keeps only root confinement, parent creation and error mapping. The server-owned skill save in skill-listing.ts calls replaceFile directly instead of its own copy.
    • Modes: an existing file keeps its mode exactly. A requested mode on a new file goes through open(), so the umask applies, as on main.
    • Read-only folders: an unconditional write falls back to overwriting in place, as on main. A hash-checked save fails with EACCES, as on main.
    • fsync: writeVerifiedFile takes durable (user saves fsync; attachments and skill trees don't) and an explicit create-or-exact mode.
  • One whole-file read: removed host.read_file_relative. The skill read now uses host.read_file, which has the 25 MB (10 MB image) cap.
    • Symlinked skill folders: host.read_file gains a required followRootSymlink boolean, matching host.list_files, so linked skill folders still resolve (Fix skill details for symlinked skill directories #5556). Skill reads pass true; every other caller passes false. The schema rejects true with a git ref or without a rootPath.
    • Server-side path checks: the server rejects absolute, .. and dotfile skill paths before calling the host, using the same check server-owned skills already used.
    • Errors: an oversized skill file now returns 413 instead of 502.
  • Wire change: HOST_DAEMON_PROTOCOL_VERSION goes from 255 to 256, for the removed command and the new required field. host.write_file's fields and results are unchanged.
  • Docs: updated the symlinked-skill paragraph in docs/debugging-and-qa.md.
  • Behaviour note: saves replace the file, so hard links, xattrs and ACLs on an overwritten file aren't carried over. Hash-checked saves already worked this way.

How you verified

  • New tests:
    • @bb/file-transfer:
      • an interrupted write leaves the original intact;
      • read-only folder: an unconditional write lands in place, a hash-checked save is refused;
      • a requested mode respects the umask;
      • a 240-character filename saves.
    • daemon: an oversized read under a followed root is rejected.
    • server:
      • oversized skill file → 413;
      • absolute, .. and dotfile skill paths are rejected without calling the host;
      • server-owned skill save writes on a fresh revision and returns 409 on a stale one.
    • Each failed before its fix.
  • Commands:
    • pnpm exec turbo run typecheck lint --filter='...[origin/main]': 133/133 tasks pass.
    • pnpm exec turbo run test for @bb/file-transfer (7), @bb/host-daemon-contract (76), @bb/host-daemon (833, 12 skipped) and @bb/server (3655 of 3656). The one server failure is a timeout in builtin-plugins.test.ts, which nothing on this branch touches. A different test in that file timed out on each full run, both under heavy machine load, and that file's 28 tests pass when run alone.
  • Manual: a pnpm start:worktree instance with a fixture project, checked by a script of 24 CLI/API checks, all passing:
    • symlinked skill reads, the 27 MB → 413 case, rejected paths;
    • daemon- and server-saved skills: fresh save lands, stale save returns 409, no temp files left behind;
    • bb file write: unconditional, stale hash, read-only folder;
    • the umask and long-filename cases.

AGENT GENERATED

🤖 Generated with Claude Code

review and others added 5 commits October 26, 2026 13:37
host.write_file without expectedSha256 wrote straight over the target, so
a crash or full disk mid-write could leave a truncated file. Every write
now goes to a synced temp file next to the target and is renamed into
place. Existing files keep their exact mode; mode still applies only on
creation for unconditional writes.

host.read_file_relative read whole files with no size check. It now
applies the same file_too_large limit as host.read_file, and the
provider-skill read maps that error to 413 like the server-owned path
instead of surfacing a 502.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
writeHostFile kept its own temp-file write and create-only hard link,
duplicating writeVerifiedFile and publishFileWithoutReplacing from the
attachment and skill-tree transfers. It now stages through
writeVerifiedFile and publishes by rename, no-clobber link, or hash check
then rename.

writeVerifiedFile takes a durable flag. User file saves fsync before
publishing; attachments and skill trees stay unsynced, since they can be
re-fetched and fsync costs about 5 ms per file on macOS.

When the parent directory is not writable, an existing file is
overwritten in place instead of failing with EACCES, as it was before
writes went through a temp file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
host.read_file_relative was built to serve status-folder URLs, which were
removed in June. Its only remaining caller was the provider-skill read,
and as a second whole-file read it had drifted from host.read_file (it
had no size cap until the previous commit).

host.read_file gains a required followRootSymlink flag, matching
host.list_files, so linked skill directories still resolve on the host.
The server now rejects absolute, "..", and dotfile skill paths before
asking the host, the same check server-owned skills already use. Every
other caller passes false.

Bump HOST_DAEMON_PROTOCOL_VERSION to 256 for the removed command and the
new required field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The server saved server-owned bb user skills with its own copy of the
temp-file, revision-check, rename sequence that host.write_file also
implements, so fixes to one (like the read-only directory fallback)
missed the other.

Move verified-file-transfer into a dependency-free @bb/file-transfer
package and add replaceFile there: it holds the revision check, mode
handling, durable temp write, rename or no-clobber publish, and in-place
fallback. writeHostFile keeps its root confinement, parent creation, and
error mapping and calls replaceFile; the server-owned skill save calls it
directly. The atomic-write tests move with the logic, and a new route
test covers the server-owned save and its 409.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…havior

Review of the shared replace found three regressions against main:

- A requested mode on a new file was chmodded exactly, bypassing the
  process umask, so mode 0o666 produced a world-writable file. Requested
  modes now only go to open(), as on main; exact chmod is kept for
  preserving an existing file's mode. writeVerifiedFile takes an explicit
  create-or-exact mode for this.
- The temp file name appended 46 characters to the target's name, so a
  long but valid filename failed with ENAMETOOLONG. The temp file is now
  a fixed-length dotfile in the target's directory.
- The in-place fallback for read-only directories also covered
  revision-checked saves, which failed safely on main. It now applies
  only to unconditional writes, which overwrote in place on main anyway.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ymichael
ymichael force-pushed the bb/fix-unsafe-host-write-file-and-unbounded-read-fi-thr_v5h7wv4b4f branch from 689923b to 6b59ad0 Compare October 26, 2026 20:40
@ymichael
ymichael merged commit 60fd1b1 into main Oct 26, 2026
95 of 96 checks passed
@ymichael
ymichael deleted the bb/fix-unsafe-host-write-file-and-unbounded-read-fi-thr_v5h7wv4b4f branch October 26, 2026 21:30
ymichael added a commit that referenced this pull request Oct 27, 2026
…d file transfer (#6331)

## Human comments

## What was wrong

Two server-to-daemon downloads still held the whole payload in memory,
and they skipped the verification that #6330 added for skills and
attachments.

- **Daemon self-update** read the host package with
`response.arrayBuffer()`. It checked the sha256 only if the server sent
`x-bb-artifact-sha256`. It then wrote `bb-app-update-<pid>.tgz` with
`writeFile`, which is not atomic and leaves a partial file behind if the
daemon crashes mid-write.
- **The install route.** `GET /install/bb-app.tgz` called `readFile` on
the whole tarball for every request.
- **Plugin host artifacts.** `fetchPluginHostArtifact` collected chunks
into one buffer of up to `HOST_ARTIFACT_MAX_BYTES` (256 MiB), so the
cache held two copies during a write. Every worker start or bridge
launch also read the whole cached `host.mjs` back and re-hashed it with
`readFile`.

## What changed

Both downloads now go through `writeVerifiedFile` from
`@bb/file-transfer` (added in #6330, moved into the package by #6335);
no parallel download code is added.

**Self-update** (`apps/host-daemon/src/protocol-self-update.ts`)
- Streams into `<dataDir>/host-package-download/bb-app.tgz.partial`
through `writeVerifiedFile` with a sha256 and an expected size. On
success it renames the file to `bb-app.tgz` and runs `npm install -g` on
it. The rename also gives npm the `.tgz` name it uses to recognize a
tarball.
- The download directory is cleared before each attempt and after it, so
a crash mid-download can't leave tarballs behind.
- **The digest is now required.** A 200 response without a valid
`x-bb-artifact-sha256` fails the attempt and nothing is installed.
- `content-length` becomes the expected size when present. There is also
a 1 GiB cap that rejects an oversized `content-length` before any bytes
are read.

**Install route** (`apps/server/src/server.ts`,
`apps/server/src/internal/host-artifact-response.ts`)
- `GET /install/bb-app.tgz` streams the file through the existing
`hostArtifactFileResponse`, which now takes the response headers as a
parameter. The plugin artifact route passes its own headers.
- Status codes, headers (`etag`, `x-bb-artifact-sha256`,
`content-length`, `content-type`, `cache-control`), the 304 path and the
response bytes are all unchanged.
- If the cached tarball is missing or has a different size than
recorded, the route returns the existing `host_package_unavailable` 500.

**Plugin host artifacts** (`server-client.ts`, `node-artifact-cache.ts`,
`plugin-host-artifact-cache.ts`, `plugin-host-manager.ts`)
- `fetchPluginHostArtifact` returns the response body as a stream after
the status and `content-length` checks. `readHostArtifactBytes` is
deleted.
- The cache writes `.staged-<uuid>.tmp` through `writeVerifiedFile` with
the digest, the byte length and the 256 MiB cap, then renames it into
place.
- It still retries a verification failure once. Network errors still
propagate. Partial files are always removed.
- **No re-hash of unchanged cache files.** A verified cache file is
remembered by `(dev, ino, size, mtimeNs, ctimeNs)`, mirroring #6330's
`SkillTreeRegistry`. The same racy-timestamp guard applies: a file whose
mtime or ctime is within 2 s of now is not remembered. A changed stat
key triggers a full re-hash; if that fails, the file is deleted and
downloaded again.
- Memo entries are dropped when stale digests are pruned, so there is at
most one per plugin.

### Compatibility (no `HOST_DAEMON_PROTOCOL_VERSION` bump)

The wire format is unchanged: same routes, request headers, response
headers and bytes.

- **Self-update across versions.** The download only starts when
`server.protocolVersion > HOST_DAEMON_PROTOCOL_VERSION`; an older or
equal server is refused before any download. So this daemon only ever
downloads from servers built after this change. Every server since #2969
(2026-09-02) sends `x-bb-artifact-sha256` and `content-length` on the
200, so requiring the digest cannot strand a daemon that is updating
from a newer server.
- **Older daemons against the new server.** They get byte-identical
responses with the same headers. Daemons from before #2969 ignore the
digest. Later ones verify it when present and send `if-none-match`,
which still gets a 304. `install.sh` and the Windows installer download
the same URL.
- **Plugin artifacts.** The server route is unchanged. The server's
`compress()` middleware gzips `text/javascript` and removes
`content-length`. The daemon already tolerated a missing
`content-length`, and it still verifies the exact byte length and digest
from the session payload.
- **Forward-compatibility limit.** The 1 GiB self-update cap is now
built into shipped daemons. The host package is about 1.4 MB today, but
a package over 1 GiB would need a manual install on older daemons.

## How you verified

**Tests**
- New `apps/host-daemon/src/node-artifact-cache.test.ts`:
  - streamed multi-chunk download
  - corrupt download: retried once, then fails with no files left
  - corrupt download followed by an intact retry
- oversized endless stream: reading stops at the expected length and the
generator is closed
- interrupted stream: the error propagates without a retry and no files
are left
- stat-key memo: hashes inside the racy window, stops hashing after it,
and a same-size tamper is re-hashed and re-downloaded
- `apps/host-daemon/src/protocol-self-update.test.ts` now uses
server-shaped responses and adds:
- one table of failure cases (digest mismatch, missing digest header,
body longer or shorter than `content-length`, connection reset
mid-body). Each asserts no install and no files left in the data dir.
  - an over-cap `content-length` that cancels the body before it is read
  - leftovers from an interrupted earlier attempt are cleared
- the installed file is the verified `bb-app.tgz`, never the `.partial`
- `apps/server/test/app/skeleton.test.ts` asserts the streamed
`content-length`, and that a cached tarball whose size changed returns
`host_package_unavailable`.

**Commands**
- `pnpm exec turbo run typecheck test --filter=@bb/host-daemon
--filter=@bb/server`:
  - typecheck passes for both packages
  - host-daemon: 849 passed, 12 skipped
- server: 3692 passed, 1 failed, in `builtin-plugins.test.ts` (builtin
plugin hot-reload, which this PR doesn't touch). It failed under load
average ~23. Run alone, a different test in that file failed once, then
the whole file passed twice.
- `oxfmt --check` and `oxlint` are clean on the changed daemon and test
files.

**Memory, measured on the real code paths**
- **Server side:** the real `createApp` install route (test harness) and
the real `registerInternalPluginHostArtifactRoutes` behind `compress()`,
served over HTTP with `@hono/node-server`.
- **Daemon side:** a separate Node process running the real
`createProtocolSelfUpdater` (stub `installTarball`; `/install/version`
rewritten to protocol+1) and the real
`createServerClient().fetchPluginHostArtifact` with
`ensureCachedPluginHostArtifact`.
- **Payloads:** the real files are this machine's cached host tarball
(1.4 MB) and the provider-claude-code `host.mjs` (2.1 MB). The 64 and
256 MiB payloads are those same files repeated.
- **Method:** "before" swaps in this PR's base versions of the 8 changed
source files. Each figure is the median of 3 fresh processes. Daemon
figures are `process.resourceUsage().maxRSS` growth during one download.
Server figures are peak `arrayBuffers` growth.

| payload | | self-update daemon peak RSS | install route server
arrayBuffers | plugin download daemon peak RSS | cached artifact reuse,
per call |
|---|---|---|---|---|---|
| real | before | +13.2 MiB | +2.7 MiB | +24.1 MiB | 1.77 ms (re-reads
and re-hashes 2.1 MB) |
| real | after | +11.9 MiB | +2.9 MiB | +23.0 MiB | 0.29 ms (stat only)
|
| 64 MiB | before | +225.2 MiB | +112.5 MiB | +166.9 MiB | 31 ms |
| 64 MiB | after | +64.7 MiB | +23.0 MiB | +62.6 MiB | 0.28 ms |
| 256 MiB | before | +809.8 MiB | +485.2 MiB | +572.6 MiB | 105 ms |
| 256 MiB | after | +76.0 MiB | +30.9 MiB | +63.4 MiB | 0.23 ms |

- **At today's sizes** the memory difference is within noise. The
visible gain is cached artifact reuse, which no longer reads and hashes
the file on every worker start.
- **At larger sizes** the old paths grow with the payload and the new
ones stay flat. The remaining ~64–76 MiB at 64 and 256 MiB is
fetch/undici buffering on loopback, not file contents.
- The first reuse call after a download still hashes once, because the
racy guard keeps a freshly written file out of the memo.

**Not changed**
- `apps/host-daemon/src/server-move/download.ts` has its own streamed,
verified download for server moves. It overlaps with `writeVerifiedFile`
and could be folded in separately.
- `bb-app-artifact.ts` still reads the packed tarball once per server
process to compute its digest. That happens at build time, not per
request.

> AGENT GENERATED

Co-authored-by: review <review@local>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant