From ed623a7befbafdfd8299d7a53df021298b16d321 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 26 Jul 2026 00:43:34 -0400 Subject: [PATCH] M10.7d: download attachment bytes into a content-addressed store (task 2107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client half of task 1942's server work. Metadata already rides the delta feed; this fetches the payload so a synced image exists on the device. Blobs are filed under their own sha256, so the same image attached to five notes is stored once and re-downloading it is free — the dedupe the task asks for falls out of content addressing rather than needing bookkeeping. The hash is also the integrity check, applied on the way IN. Bytes that don't hash to what the server advertised are refused rather than filed under a name that lies about them — and because the blob then still counts as missing, the next sync simply tries again. SECURITY: the hash arrives in a server response and becomes a FILENAME, so it is validated as 64 hex characters before touching the filesystem. Without that, a hostile or buggy server could send "../../..." and steer a write outside the blob directory. Tested. A failed attachment never fails the sync. Notes are the primary data and have already landed; aborting here would let one unreachable file block every future sync. Counted, logged, surfaced in the UI as "they'll retry on the next sync", and retried because the blob is still absent. sha2 is pure Rust, so the Windows cross-compile lane pays nothing for it — the constraint recorded in ci-requirements.md. SPLIT, deliberately: this stores the bytes but does NOT yet render them in the webview. That half needs a custom URI scheme or the asset protocol, whose URL form differs by platform (Windows uses http://scheme.localhost/, others scheme://localhost/) — and CI cannot verify webview rendering at all, being headless with no webview. Guessing at it here would ship an unverifiable change on the most fragile lane. Follow-up filed; synced images will show as broken until it lands. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SreJkbxB4gx8pPsu8QbLPi --- desktop/src-tauri/Cargo.toml | 4 + desktop/src-tauri/src/lib.rs | 5 + desktop/src-tauri/src/sync/blobs.rs | 148 +++++++++++++++++++++++++ desktop/src-tauri/src/sync/client.rs | 32 ++++++ desktop/src-tauri/src/sync/commands.rs | 8 +- desktop/src-tauri/src/sync/engine.rs | 10 +- desktop/src-tauri/src/sync/mod.rs | 1 + desktop/src-tauri/src/sync/pull.rs | 80 ++++++++++++- frontend/src/desktop/bridge.ts | 3 + frontend/src/views/SyncView.vue | 15 ++- 10 files changed, 297 insertions(+), 9 deletions(-) create mode 100644 desktop/src-tauri/src/sync/blobs.rs diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index df86d99..588d640 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -38,6 +38,10 @@ log = "0.4" # native-tls uses OpenSSL, whose headers (libssl-dev) ci-tauri already ships. # default-features off drops http2/charset we don't need for a JSON API. reqwest = { version = "0.12", default-features = false, features = ["json", "native-tls"] } +# Verifying downloaded attachment bytes against the sha256 the server advertised. +# Pure Rust (no C/asm beyond optional cpufeatures), so it costs the Windows +# cross-compile lane nothing — see ci-requirements.md on why that matters here. +sha2 = "0.10" # Tauri's default release profile: smaller, faster shipped binaries. [profile.release] diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 8a31636..b73ffee 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -46,6 +46,11 @@ pub fn run() { let db = local::open(&db_path)?; log::info!("local store ready — {}", local::summary(&db)); app.manage(db); + // Attachment bytes live beside the database, filed by content hash, so a + // synced image is readable with no network (M10.7d). + let blobs = sync::blobs::BlobStore::new(dir.join("blobs"))?; + log::info!("attachment store ready: {}", blobs.root().display()); + app.manage(blobs); Ok(()) }) .invoke_handler(tauri::generate_handler![ diff --git a/desktop/src-tauri/src/sync/blobs.rs b/desktop/src-tauri/src/sync/blobs.rs new file mode 100644 index 0000000..c20db0c --- /dev/null +++ b/desktop/src-tauri/src/sync/blobs.rs @@ -0,0 +1,148 @@ +//! Local storage for attachment bytes (M10.7d). +//! +//! Content-addressed: a blob is filed under its own sha256, so the same image +//! attached to five notes is stored once and re-downloading it is free. The hash is +//! also the integrity check — bytes that don't hash to what the server advertised +//! are refused rather than filed under a name that lies about them. +//! +//! Attachment METADATA rides the delta feed; only the bytes come through here +//! (docs/sync.md). + +use std::fs; +use std::path::{Path, PathBuf}; + +/// A sha256 in lowercase hex, and nothing else. +/// +/// This is a **path-safety** check, not a formatting nicety: the hash is taken +/// straight from a server response and used as a filename. Without it, a hostile or +/// buggy server could send `../../…` and steer a write outside the blob directory. +fn is_hash(candidate: &str) -> bool { + candidate.len() == 64 && candidate.bytes().all(|b| b.is_ascii_hexdigit()) +} + +fn digest(bytes: &[u8]) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(bytes); + hasher + .finalize() + .iter() + .map(|b| format!("{b:02x}")) + .collect() +} + +pub struct BlobStore { + root: PathBuf, +} + +impl BlobStore { + /// Open (creating if needed) the blob directory. + pub fn new(root: PathBuf) -> std::io::Result { + fs::create_dir_all(&root)?; + Ok(Self { root }) + } + + pub fn root(&self) -> &Path { + &self.root + } + + /// Where a blob lives, or `None` if the hash isn't one. + pub fn path(&self, sha256: &str) -> Option { + let lower = sha256.to_ascii_lowercase(); + is_hash(&lower).then(|| self.root.join(lower)) + } + + /// Whether we already hold these bytes. Drives the "don't download it twice" + /// skip, which is the entire point of keying by content. + pub fn has(&self, sha256: &str) -> bool { + self.path(sha256).is_some_and(|p| p.is_file()) + } + + /// File bytes under `expected`, refusing them if they don't hash to it. + /// + /// Verifying on the way IN rather than on the way out means a corrupted transfer + /// can never be served later as if it were genuine — and the next sync simply + /// tries again, because the blob still counts as missing. + pub fn store(&self, expected: &str, bytes: &[u8]) -> Result { + let path = self + .path(expected) + .ok_or_else(|| format!("refusing an attachment with a malformed hash: {expected}"))?; + let actual = digest(bytes); + if actual != expected.to_ascii_lowercase() { + return Err(format!( + "attachment failed its integrity check (expected {expected}, got {actual})" + )); + } + fs::write(&path, bytes).map_err(|e| format!("couldn't save an attachment: {e}"))?; + Ok(path) + } + + pub fn read(&self, sha256: &str) -> Option> { + fs::read(self.path(sha256)?).ok() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A blob store in a throwaway directory. No tempfile dependency for one test + /// fixture — the process id keeps concurrent runs apart. + fn store(tag: &str) -> BlobStore { + let dir = std::env::temp_dir().join(format!("ts-blobs-{}-{tag}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + BlobStore::new(dir).expect("store") + } + + /// sha256("hello") — a fixed vector, so a broken digest can't agree with itself. + const HELLO: &str = "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"; + + #[test] + fn digest_matches_a_known_vector() { + assert_eq!(digest(b"hello"), HELLO); + } + + #[test] + fn stores_and_reads_back() { + let store = store("roundtrip"); + assert!(!store.has(HELLO)); + store.store(HELLO, b"hello").expect("store"); + assert!(store.has(HELLO)); + assert_eq!(store.read(HELLO).as_deref(), Some(&b"hello"[..])); + } + + #[test] + fn refuses_bytes_that_dont_match_the_hash() { + // A corrupted or substituted transfer must never be filed under a name that + // claims it's genuine. + let store = store("mismatch"); + let err = store.store(HELLO, b"goodbye").expect_err("must reject"); + assert!(err.contains("integrity"), "got {err}"); + assert!(!store.has(HELLO), "nothing should have been written"); + } + + #[test] + fn rejects_a_hash_that_could_escape_the_directory() { + // The hash arrives from a server response and becomes a filename. + let store = store("traversal"); + assert!(store.path("../../etc/passwd").is_none()); + assert!(store.store("../../etc/passwd", b"x").is_err()); + assert!(store.path("").is_none()); + assert!(store.path("nothex!!").is_none()); + } + + #[test] + fn accepts_an_uppercase_hash() { + // The wire format isn't guaranteed to be lowercase; the filename is. + let store = store("case"); + store.store(&HELLO.to_ascii_uppercase(), b"hello").expect("store"); + assert!(store.has(HELLO), "should be found under the lowercase name"); + } + + #[test] + fn missing_blob_reads_as_none() { + let store = store("missing"); + assert!(store.read(HELLO).is_none()); + assert!(!store.has(HELLO)); + } +} diff --git a/desktop/src-tauri/src/sync/client.rs b/desktop/src-tauri/src/sync/client.rs index 17f6d9a..eb8bc1d 100644 --- a/desktop/src-tauri/src/sync/client.rs +++ b/desktop/src-tauri/src/sync/client.rs @@ -223,6 +223,38 @@ pub async fn fetch_changes( .map_err(|e| format!("Couldn't read the change feed from {base_url}: {e}")) } +/// Download one attachment's bytes. +/// +/// Metadata already arrived on the delta feed; this is only the payload, fetched +/// over the same route the web app uses (owner/shared scoped server-side). +pub async fn fetch_attachment( + base_url: &str, + token: &str, + note_id: &str, + attachment_id: &str, +) -> Result, String> { + let url = format!("{base_url}/api/notes/{note_id}/attachments/{attachment_id}"); + let request = prepare(http_with(SYNC_TIMEOUT)?.get(url), Some(token)); + let response = request + .send() + .await + .map_err(|e| describe_transport_error(base_url, &e))?; + + let status = response.status(); + if status == StatusCode::UNAUTHORIZED { + return Err(TOKEN_REJECTED.to_string()); + } + if !status.is_success() { + return Err(unexpected_status(base_url, status)); + } + + response + .bytes() + .await + .map(|b| b.to_vec()) + .map_err(|e| format!("Couldn't download an attachment from {base_url}: {e}")) +} + /// Send a batch of changes and hand back the raw reply. /// /// Returns text rather than parsed results so this module stays pure transport — diff --git a/desktop/src-tauri/src/sync/commands.rs b/desktop/src-tauri/src/sync/commands.rs index 976f0ef..3f97bc3 100644 --- a/desktop/src-tauri/src/sync/commands.rs +++ b/desktop/src-tauri/src/sync/commands.rs @@ -7,6 +7,7 @@ use serde::{Deserialize, Serialize}; use tauri::State; use crate::local::Db; +use crate::sync::blobs::BlobStore; use crate::sync::client::{self, Identity, ProbeResult}; use crate::sync::compat::Compatibility; use crate::sync::engine; @@ -142,9 +143,12 @@ fn credentials(db: &State<'_, Db>) -> Result<(String, String), String> { /// separately inside the crate, but offering a bare "pull" would let the UI overwrite /// unsent local edits — the ordering isn't a suggestion, it's what keeps them. #[tauri::command] -pub async fn sync_now(db: State<'_, Db>) -> Result { +pub async fn sync_now( + db: State<'_, Db>, + blobs: State<'_, BlobStore>, +) -> Result { let (base_url, token) = credentials(&db)?; - engine::run_cycle(db.inner(), &base_url, &token).await + engine::run_cycle(db.inner(), blobs.inner(), &base_url, &token).await } /// Whether anything is waiting to be sent. Lets the UI show an honest "unsynced diff --git a/desktop/src-tauri/src/sync/engine.rs b/desktop/src-tauri/src/sync/engine.rs index d6f4d25..0f6b8e0 100644 --- a/desktop/src-tauri/src/sync/engine.rs +++ b/desktop/src-tauri/src/sync/engine.rs @@ -7,6 +7,7 @@ use chrono::{SecondsFormat, Utc}; use serde::Serialize; +use super::blobs::BlobStore; use super::pull; use super::push; use super::state; @@ -31,9 +32,14 @@ pub struct SyncOutcome { /// A failed push aborts before the pull. Pulling anyway would take the exact rows we /// just failed to save and overwrite them — turning a recoverable network error into /// lost work. -pub async fn run_cycle(db: &Db, base_url: &str, token: &str) -> Result { +pub async fn run_cycle( + db: &Db, + blobs: &BlobStore, + base_url: &str, + token: &str, +) -> Result { let push = push::run(db, base_url, token).await?; - let pull = pull::run(db, base_url, token).await?; + let pull = pull::run(db, blobs, base_url, token).await?; if pull.clobbered_dirty > 0 { // Push ran first and reported success, so nothing should still have been diff --git a/desktop/src-tauri/src/sync/mod.rs b/desktop/src-tauri/src/sync/mod.rs index 53b19ae..dc78966 100644 --- a/desktop/src-tauri/src/sync/mod.rs +++ b/desktop/src-tauri/src/sync/mod.rs @@ -12,6 +12,7 @@ //! The engine that moves notes — push, pull, last-write-wins — lands in M10.7b/c and //! consults `compat` before it does anything. +pub mod blobs; pub mod client; pub mod commands; pub mod compat; diff --git a/desktop/src-tauri/src/sync/pull.rs b/desktop/src-tauri/src/sync/pull.rs index 90878f8..f6e334c 100644 --- a/desktop/src-tauri/src/sync/pull.rs +++ b/desktop/src-tauri/src/sync/pull.rs @@ -10,6 +10,7 @@ use chrono::{SecondsFormat, Utc}; use rusqlite::{params, Connection, OptionalExtension}; use serde::Serialize; +use super::blobs::BlobStore; use super::client; use super::state; use super::wire; @@ -33,6 +34,10 @@ pub struct PullSummary { /// top. Should be 0 in the normal cycle, because push runs first; anything higher /// means local work was overwritten, which is worth saying out loud. pub clobbered_dirty: usize, + pub blobs_downloaded: usize, + /// Attachments whose bytes couldn't be fetched or failed verification. Counted + /// rather than fatal — see `download_missing_blobs`. + pub blobs_failed: usize, } impl PullSummary { @@ -43,10 +48,66 @@ impl PullSummary { self.labels_applied += other.labels_applied; self.labels_deleted += other.labels_deleted; self.clobbered_dirty += other.clobbered_dirty; + self.blobs_downloaded += other.blobs_downloaded; + self.blobs_failed += other.blobs_failed; self.cursor = other.cursor; } } +/// `(note_id, attachment_id, sha256)` for every attachment that advertises a hash. +/// The caller filters against the blob store — which blobs we hold isn't a SQL +/// question. +pub fn hashed_attachments(conn: &Connection) -> rusqlite::Result> { + let mut stmt = conn.prepare( + "SELECT note_id, id, sha256 FROM attachments + WHERE sha256 IS NOT NULL AND sha256 <> ''", + )?; + let rows = stmt.query_map([], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)))?; + rows.collect() +} + +/// Fetch the bytes for any attachment we have metadata for but no blob. +/// +/// A failed attachment NEVER fails the sync. Notes are the primary data and they've +/// already landed; an image that didn't arrive is retried on the next cycle simply +/// because its blob still counts as missing. Aborting here would mean one unreachable +/// file could block every future sync. +async fn download_missing_blobs( + db: &Db, + blobs: &BlobStore, + base_url: &str, + token: &str, +) -> Result<(usize, usize), String> { + let wanted = { + let conn = db.0.lock().map_err(|e| e.to_string())?; + hashed_attachments(&conn).map_err(|e| e.to_string())? + }; + + let mut downloaded = 0; + let mut failed = 0; + for (note_id, attachment_id, sha256) in wanted { + // Content-addressed, so this skips blobs we already hold — including the same + // image attached to a different note. + if blobs.has(&sha256) { + continue; + } + match client::fetch_attachment(base_url, token, ¬e_id, &attachment_id).await { + Ok(bytes) => match blobs.store(&sha256, &bytes) { + Ok(_) => downloaded += 1, + Err(e) => { + log::warn!("attachment {attachment_id}: {e}"); + failed += 1; + } + }, + Err(e) => { + log::warn!("attachment {attachment_id}: {e}"); + failed += 1; + } + } + } + Ok((downloaded, failed)) +} + fn now() -> String { Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true) } @@ -339,7 +400,12 @@ fn position_of(explicit: i64, index: usize) -> i64 { /// NOTE ON ORDERING: the full cycle is push-then-pull (docs/sync.md). Running this /// against a store with unpushed edits lets the server's version land on top of them /// — counted as `clobbered_dirty` and logged, rather than hidden. -pub async fn run(db: &Db, base_url: &str, token: &str) -> Result { +pub async fn run( + db: &Db, + blobs: &BlobStore, + base_url: &str, + token: &str, +) -> Result { let mut total = PullSummary::default(); loop { @@ -377,12 +443,24 @@ pub async fn run(db: &Db, base_url: &str, token: &str) -> Result 0 { log::warn!( "pull overwrote {} note(s) that still had unpushed local edits", total.clobbered_dirty ); } + if total.blobs_failed > 0 { + log::warn!( + "pull: {} attachment(s) couldn't be downloaded; will retry next sync", + total.blobs_failed + ); + } log::info!( "pull complete: {} page(s), {} note(s) applied, {} deleted, {} label(s) applied, cursor {}", total.pages, diff --git a/frontend/src/desktop/bridge.ts b/frontend/src/desktop/bridge.ts index 9eafae9..f2f3964 100644 --- a/frontend/src/desktop/bridge.ts +++ b/frontend/src/desktop/bridge.ts @@ -114,6 +114,9 @@ export interface PullSummary { labels_deleted: number; cursor: number; clobbered_dirty: number; + blobs_downloaded: number; + /** Attachments whose bytes didn't arrive. Retried next sync, never fatal. */ + blobs_failed: number; } export interface SyncOutcome { diff --git a/frontend/src/views/SyncView.vue b/frontend/src/views/SyncView.vue index 0c9b1a2..c40d374 100644 --- a/frontend/src/views/SyncView.vue +++ b/frontend/src/views/SyncView.vue @@ -119,10 +119,17 @@ async function syncNow() { pending.value = await syncBridge.hasPending(); const received = outcome.pull.notes_applied + outcome.pull.notes_deleted; const sent = outcome.push.created + outcome.push.applied; - lastResult.value = - received === 0 && sent === 0 - ? "Already up to date." - : `Sent ${sent}, received ${received}.`; + const blobs = outcome.pull.blobs_downloaded; + const parts: string[] = []; + if (sent > 0) parts.push(`sent ${sent}`); + if (received > 0) parts.push(`received ${received}`); + if (blobs > 0) parts.push(`${blobs} attachment${blobs === 1 ? "" : "s"}`); + lastResult.value = parts.length ? `Synced — ${parts.join(", ")}.` : "Already up to date."; + // Attachments that didn't arrive are retried next sync, so this is a note, not + // an error — but saying nothing would leave a missing image unexplained. + if (outcome.pull.blobs_failed > 0) { + lastResult.value += ` ${outcome.pull.blobs_failed} attachment(s) didn't download — they'll retry on the next sync.`; + } // Rejections are the server refusing a specific change — surfaced, never // swallowed, because only the person can resolve them. if (outcome.push.rejected > 0) {