`get` writes a patch view's blobs under their claimed hash without checking the content, overwriting already-verified blobs in place

[mikolalysenko]

[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register comment.

Kind: bug. Source: new finding, register C42 (related to C21, atomic writers, and C41, hash case policy).

Problem

Two code paths write the content-addressed store .socket/blobs/<hash>, and they follow different rules.

• The fetch path (blob_fetcher.rs#L533-L556) recomputes the git-sha256 of the downloaded bytes and refuses a mismatch. It then writes through write_cache_entry_atomic, which stages and renames. That writer's doc comment (blob_fetcher.rs#L451-L471) explains why: get_missing_blobs only checks that a file exists (blob_fetcher.rs#L77-L89), so an entry whose content doesn't hash to its name "is then trusted forever".
• The get path (get.rs#L284-L310) handles the inline blobContent / beforeBlobContent of a patch view. It checks only that the hash is 64-hex, decodes the base64 with a hand-rolled decoder (get.rs#L3814-L3820) although the base64 crate is already a CLI dependency, and then:

  let existed = tokio::fs::try_exists(&target).await.unwrap_or(false);
  tokio::fs::write(&target, &decoded)   // truncate + write in place, no hash check

  One existing unit test pins the missing check: write_blob_entry_accepts_valid_sha256_hash stores "patched\n" under the hash 1111…1.

Proof by execution on 045d7ec: a temporary unit test in get.rs, run twice and not committed. It seeded blobs/<H> with "pristine\n", where H = git_sha256("pristine\n"), which stands in for a live record's verified before-blob. It then called write_blob_entry(blobs, base64("patched\n"), H, …). The result was Ok(false), the file now holds "patched\n", and the hash of its content no longer matches its name. On both runs, the fetch path's own test fetch_missing_blobs_rejects_content_hash_mismatch_and_writes_nothing shows that the same payload is refused there.

Impact

• Store poisoning: a mismatched inline blob from a server bug, a proxy or a stale cache is stored under a name it doesn't match. Because get_missing_blobs checks only presence, apply and repair never fetch it again. Agent-mode apply then fails verification (HashMismatch), and the before-blob rollback needs is wrong, until a user deletes the file by hand.
• Destroying good data: the write overwrites an existing blob that another live record references (the code even probes existed first), so a correct blob is replaced with unverified bytes.
• Torn writes: fs::write truncates in place, so ENOSPC or a kill mid-write leaves a partial file under a valid name, which is exactly the failure write_cache_entry_atomic was written to prevent.

Size: one function, about 30 production lines.

Proposed change

• Move write_cache_entry_atomic into a shared core helper, for example api::blob_fetcher::store_verified_blob(blobs_dir, hash, bytes): verify git_sha256(bytes) against hash with the shared comparison, then stage and rename. Make it the only writer of .socket/blobs/<hash>.
• get::write_blob_entry calls it. When the target already exists and verifies, skip the write entirely, so a verified blob is never rewritten. On a mismatch, report it the same way as a fetch mismatch.
• Replace the hand-rolled base64_decode with base64::engine::general_purpose::STANDARD, keeping the pinned error message as its doc comment requires, and delete the hand-rolled decoder.
• Fix write_blob_entry_accepts_valid_sha256_hash to use a real hash.

Size and scope

crates/socket-patch-cli/src/commands/get.rs and crates/socket-patch-core/src/api/blob_fetcher.rs: about 60 production lines, mostly deletions, plus tests. Out of scope: the fs.rs writer consolidation (C21) and the hash case policy (#707), although the shared comparison should be the one #707 settles on.

Acceptance criteria

• Regression test: get given an inline blob whose content doesn't hash to its afterHash/beforeHash writes nothing and reports a mismatch.
• Regression test: an existing, verified blobs/<H> is byte-identical after get processes a patch view that names H with different bytes.
• Stage and rename only, with no in-place tokio::fs::write of a blob left in get.rs (grep).
• The hand-rolled base64_decode is deleted; the existing decode-error test stays green.
• The existing write_blob_entry_* traversal tests and blob_fetcher_edges_e2e stay green.

Dependencies

None blocking. It touches the same helper as #707 (hash case), and either can land first. Related: #607 (streaming blob downloads), which changes the fetch path's write.

[mikolalysenko]

[agent] Triaged as priority:p3 (cross-cutting CLI/core). Related to #728 (shared stage-and-rename writer) and #707 (hash case policy), but a distinct defect: get::write_blob_entry skips the content-hash check the fetch path enforces. Either can land first.

---

Generated by Claude Code