Skip to main content

max / balanced_breakfast

Harden security, sync, and query subsystems Code fuzz remediation: crypto key zeroing, sync mutex, download validation, bookmark XSS fix, path traversal fix, open-file safety, query filter accuracy, and regex precompilation.
Co-Authored-By
Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author: Max J. <87768334+MaxJMath@users.noreply.github.com> · 2026-05-02 23:13 UTC
Commit: 15c8da3f1ccb8e18d075c3d82f8e8ddf99c6315a
Parent: 41f9224
22 files changed, +567 insertions, -162 deletions
M docs/todo.md +58
@@ -7,6 +7,64 @@
7 7
8 8 ---
9 9
10 + ## Fuzz Findings (2026-04-27)
11 +
12 + Findings from adversarial code review. Ordered by severity.
13 +
14 + ### HIGH
15 +
16 + - [x] **XSS in bookmark HTML export** (`commands/bookmarks.rs:403`). The `body` field (attacker-controlled RSS content) is interpolated into the HTML template without escaping. `title`/`url`/`author` are escaped but `body` is not. Fix: pass `body` through `html_escape` or sanitize.
17 + - [x] **Path traversal in `download_and_open`** (`commands/items.rs:384-388`). URL filename extracted via `rsplit('/')` is passed to `dir.join()` without sanitizing `..` components. Fix: strip path separators and `..` from derived filename.
18 + - [x] **Arbitrary code exec via `open::that`** (`commands/items.rs:404`). Downloaded files are opened with the system default handler with no extension validation. A `.exe`/`.command`/`.scpt` URL would be downloaded and launched. Fix: allowlist safe extensions or skip auto-open for dangerous ones.
19 + - [x] **Source+unread/starred filter bypass** (`bb-feed/generator/query.rs:29-61`). The `if/else if` chain gives `source` priority over `unread_only`/`starred_only`. When both are set (no search), the unread/starred filter is silently dropped. Fix: add `list_by_busser_unread`/`list_by_busser_starred` queries, or combine the filters in the existing chain.
20 + - [x] **Regex recompilation per-item** (`ordering.rs:197` + `query.rs:103`). `retain` calls `matches()` which calls `compile_regexes()` per item. O(N×M) regex compilations. Fix: pre-compile once before the `retain` loop.
21 +
22 + ### MEDIUM — sync service
23 +
24 + - [x] **No mutex on `perform_sync`** (`sync_service/mod.rs`, `sync_scheduler.rs`, `commands/sync.rs`). Fixed: added `tokio::sync::Mutex` in `AppState`, acquired in both `sync_now` and `sync_scheduler`.
25 + - [x] **Skipped changelog entries still marked as pushed** (`sync_service/upload.rs:77`). Fixed: track individual pushed IDs; only mark those as pushed. Skipped entries remain `pushed=0` and will be retried.
26 + - [x] **`INSERT OR REPLACE` with partial JSON nulls columns** (`sync_service/download.rs:158-199`). Fixed: validate primary key columns exist before upserting; log debug warning for other missing columns so they're visible but don't block the sync.
27 + - [x] **`applying_remote` flag not in transaction** (`sync_service/download.rs:53-58`). Fixed: flag set, data changes, and flag clear are all wrapped in a single SQLite transaction. A crash mid-apply rolls back the flag too.
28 +
29 + ### MEDIUM — crypto
30 +
31 + - [x] **TOCTOU race in `load_or_create_key`** (`crypto.rs:29-51`). Fixed: uses `OpenOptions::create_new(true)` for atomic check-and-create.
32 + - [x] **Keychain migration deletes file before verifying durability** (`crypto.rs:78-82`). Fixed: read-back and compare before deleting file.
33 + - [x] **Key creation failure silently degrades to plaintext** (`state.rs:68-76`). Fixed: encryption key loading is now a hard startup error. App will not start without a working encryption key.
34 + - [x] **Decrypt failure passes raw ciphertext to plugins** (`crypto.rs:199-200`). Fixed: clears field to empty string on decrypt failure instead of passing ciphertext through.
35 + - [x] **No key zeroing on drop** (`crypto.rs:21-25`). Fixed: all key material wrapped in `zeroize::Zeroizing<[u8; 32]>` via `EncryptionKey` type alias. Keys are zeroed from memory when dropped.
36 +
37 + ### MEDIUM — database
38 +
39 + - [x] **Missing transaction in `BookmarksRepository::create`** (`repository.rs:1047-1085`). Bookmark insert + tag inserts are not transactional; partial failure leaves bookmark with incomplete tags.
40 + - [x] **Missing transaction in `BookmarksRepository::set_tags`** (`repository.rs:1180-1198`). Delete-all + inserts not transactional; failure mid-insert loses tags permanently.
41 + - [x] **`update_config` does not update `updated_at`** (`repository.rs:316-323`). Every other mutation method updates `updated_at`; this one doesn't, which could break sync/cache-invalidation.
42 +
43 + ### MEDIUM — plugin sandbox
44 +
45 + - [x] **No `set_max_string_size`/`set_max_array_size` on Rhai engine** (`rhai_plugin/mod.rs:271-274`). Fixed: set `max_string_size` (2 MB), `max_array_size` (5000), `max_map_size` (2000) on both engine creation sites. Complements the existing per-node `validate_dynamic_sizes` on return values.
46 + - [x] **Rhai `import` may not be disabled** (`rhai_plugin/mod.rs:271`). Fixed: set `DummyModuleResolver` on both `load_plugin` and `create_engine` engines.
47 + - [x] **DNS rebinding bypasses URL blocklist** (`rhai_plugin/host_functions.rs:35-86`). Fixed: strip `user@` from host before blocklist check; added `100.x` (CGNAT/Tailscale) block. Note: DNS rebinding itself is inherent to string-level checks; mitigated by trust model (local plugins only).
48 +
49 + ### MEDIUM — other
50 +
51 + - [x] **`count()` ignores most filters** (`bb-feed/generator/query.rs:160-168`). Fixed: added `starred_only` support and combined source+unread/starred via `list_filtered`.
52 + - [x] **`get_all_items()` ignores `feed_tags` filter** (`bb-feed/generator/query.rs:137-156`). Fixed: added feed_tags filtering before `apply()`.
53 + - [x] **No URL scheme validation in `extract_reader_view`** (`commands/query_feeds.rs:192-210`). Fixed: validates http/https before calling reader script.
54 + - [x] **HTML URL cleaner only matches double-quoted attributes** (`url_cleaner.rs:88`). Single-quoted URLs (`href='...'`) bypass tracking parameter removal entirely.
55 +
56 + ### LOW / NOTE
57 +
58 + - [x] **No size limit on OPML import** (`commands/opml.rs:70`). Fixed: capped at 10 MB before parsing.
59 + - [x] **Bookmark duplicate check not transactional** (`commands/bookmarks.rs:143-145`). Kept check-then-insert (no UNIQUE constraint on url column). Acceptable for desktop single-user app. Documented.
60 + - [x] **No bookmark tag validation** (`commands/bookmarks.rs:274-291`). Fixed: added `validate_bookmark_tags` using same `tagtree::validate_with` rules as feed tags. Applied to `create_bookmark`, `create_bookmark_from_item`, and `set_bookmark_tags`.
61 + - [x] **Error leakage**: raw sqlx errors forwarded to frontend (`commands/error.rs:81-84`). Fixed: log full error server-side, send generic message to frontend.
62 + - [x] **`expect()` on `AppState::new()` in startup** (`lib.rs:39`). Fixed: replaced `expect` with `match` that returns `Err` to Tauri setup, logging + printing the error. No more panic on DB/key failure.
63 + - [x] **`serde_json::to_string` fallback replaces config with `{}`** (`orchestrator.rs:389-390`). Fixed: serialization failure now logs an error and skips the update, preserving existing config.
64 + - [x] **`FeedItemId::to_combined`/`from_combined` roundtrip fails when source contains `:`** (`feed_item.rs:24-32`). Fixed: added `debug_assert` that source must not contain `:`. Documented the invariant. Source IDs are simple ASCII identifiers by convention.
65 +
66 + ---
67 +
10 68 ## Phase 7: Plugin OAuth (Post-beta)
11 69
12 70 ### 7A: OAuth Infrastructure
M src-tauri/Cargo.toml +3
@@ -66,6 +66,9 @@
66 66 tracing.workspace = true
67 67 tracing-subscriber.workspace = true
68 68
69 + # HTML sanitization for export
70 + regex.workspace = true
71 +
69 72 # Tag standard
70 73 tagtree.workspace = true
71 74
M src-tauri/src/lib.rs +11 -4
@@ -34,9 +34,14 @@
34 34 .setup(|app| {
35 35 let app_handle = app.handle().clone();
36 36 tauri::async_runtime::block_on(async move {
37 - let state = AppState::new(&app_handle)
38 - .await
39 - .expect("Failed to initialize app state");
37 + let state = match AppState::new(&app_handle).await {
38 + Ok(s) => s,
39 + Err(e) => {
40 + tracing::error!(error = %e, "Fatal: failed to initialize app state");
41 + eprintln!("Fatal: failed to initialize app state: {e}");
42 + return Err(e);
43 + }
44 + };
40 45 let state = Arc::new(state);
41 46 app_handle.manage(state.clone());
42 47
@@ -49,7 +54,9 @@
49 54 // Start sync scheduler
50 55 let sync_handle = sync_scheduler::start_sync_scheduler(app_handle);
51 56 state.set_sync_scheduler_handle(sync_handle);
52 - });
57 + Ok(())
58 + })
59 + .map_err(|e: String| -> Box<dyn std::error::Error> { e.into() })?;
53 60
54 61 // Check for OTA updates after a short delay (desktop only)
55 62 #[cfg(not(any(target_os = "ios", target_os = "android")))]
M src-tauri/src/state.rs +10 -10
@@ -21,6 +21,8 @@
21 21 pub sync_client: RwLock<Option<Arc<SyncKitClient>>>,
22 22 /// App data directory for key persistence.
23 23 pub data_dir: PathBuf,
24 + /// Guard preventing concurrent sync operations (manual + scheduler).
25 + pub sync_mutex: tokio::sync::Mutex<()>,
24 26 /// Handle to abort the background auto-fetch task on shutdown.
25 27 auto_fetch_handle: parking_lot::Mutex<Option<AbortHandle>>,
26 28 /// Handle to abort the background stale-item cleanup task on shutdown.
@@ -63,17 +65,14 @@
63 65 .await
64 66 .map_err(|e| format!("Failed to create orchestrator: {}", e))?;
65 67
66 - // Load or create encryption key for plugin secrets (keychain preferred, file fallback)
68 + // Load or create encryption key for plugin secrets (keychain preferred, file fallback).
69 + // This is a hard requirement — without an encryption key, plugin secrets
70 + // would be stored in plaintext, which is a security risk.
67 71 let key_path = app_data_dir.join("encryption.key");
68 - match bb_core::crypto::load_or_create_key_from_keychain(&key_path) {
69 - Ok(key) => {
70 - info!("Encryption key loaded");
71 - orchestrator.set_encryption_key(key);
72 - }
73 - Err(e) => {
74 - tracing::warn!(error = %e, "Failed to load encryption key, secrets will not be encrypted");
75 - }
76 - }
72 + let key = bb_core::crypto::load_or_create_key_from_keychain(&key_path)
73 + .map_err(|e| format!("Failed to load encryption key: {e}"))?;
74 + info!("Encryption key loaded");
75 + orchestrator.set_encryption_key(key);
77 76
78 77 info!("Orchestrator created, running migrations");
79 78
@@ -106,6 +105,7 @@
106 105 orchestrator,
107 106 sync_client: RwLock::new(sync_client.map(Arc::new)),
108 107 data_dir: app_data_dir,
108 + sync_mutex: tokio::sync::Mutex::new(()),
109 109 auto_fetch_handle: parking_lot::Mutex::new(None),
110 110 cleanup_handle: parking_lot::Mutex::new(None),
111 111 sync_scheduler_handle: parking_lot::Mutex::new(None),
M src-tauri/src/sync_scheduler.rs +3
@@ -161,6 +161,9 @@
161 161 }
162 162 }
163 163
164 + // Prevent concurrent sync operations (manual + scheduler).
165 + let _sync_guard = state.sync_mutex.lock().await;
166 +
164 167 // Perform sync
165 168 match sync_service::perform_sync(pool, &client).await {
166 169 Ok(result) => {
M src-tauri/tests/pipeline.rs +2 -2
M crates/bb-core/src/crypto.rs +69 -39
M crates/bb-core/src/orchestrator.rs +11 -5
M crates/bb-core/src/url_cleaner.rs +10 -4
M crates/bb-db/src/repository.rs +69 -5
M crates/bb-feed/src/ordering.rs +2 -2
M crates/bb-interface/src/feed_item.rs +13 -2
M src-tauri/src/commands/bookmarks.rs +43 -3
M src-tauri/src/commands/error.rs +14 -4
M src-tauri/src/commands/items.rs +23 -3
M src-tauri/src/commands/opml.rs +10
M src-tauri/src/commands/query_feeds.rs +6
M src-tauri/src/commands/sync.rs +3
M src-tauri/src/sync_service/download.rs +94 -22
M src-tauri/src/sync_service/upload.rs +44 -20
M crates/bb-core/src/rhai_plugin/mod.rs +13
M crates/bb-feed/src/generator/query.rs +56 -37