max / balanced_breakfast
- Co-Authored-By
- Claude Opus 4.6 (1M context) <noreply@anthropic.com>
20 files changed,
+319 insertions,
-131 deletions
| @@ -7,6 +7,17 @@ | |||
| 7 | 7 | ||
| 8 | 8 | --- | |
| 9 | 9 | ||
| 10 | + | ## Sync Monetization | |
| 11 | + | ||
| 12 | + | BB is free. Cloud sync is the only revenue source. See `MNW/server/docs/internal/business/app_sync_pricing.md` for full pricing rationale. | |
| 13 | + | ||
| 14 | + | - [ ] Stripe product + prices: $1/mo monthly, $8/yr annual | |
| 15 | + | - [ ] Sync gate: check subscription status before enabling SyncKit sync | |
| 16 | + | - [ ] Subscription UI: in-app purchase/manage flow (settings panel) | |
| 17 | + | - [ ] Annual billing messaging: explain why annual is preferred (Stripe fee transparency) | |
| 18 | + | ||
| 19 | + | --- | |
| 20 | + | ||
| 10 | 21 | ## Fuzz Findings (2026-04-27) | |
| 11 | 22 | ||
| 12 | 23 | Findings from adversarial code review. Ordered by severity. | |
| @@ -65,6 +76,47 @@ | |||
| 65 | 76 | ||
| 66 | 77 | --- | |
| 67 | 78 | ||
| 79 | + | ## Fuzz Findings (2026-05-02) | |
| 80 | + | ||
| 81 | + | Findings from adversarial code review. Ordered by severity. | |
| 82 | + | ||
| 83 | + | ### SERIOUS | |
| 84 | + | ||
| 85 | + | - [x] **`UnreadFirst` sorts read items first** (`ordering.rs:57-61`). Fixed: swapped to `a.is_read.cmp(&b.is_read)`. Fixed test to assert correct ordering. | |
| 86 | + | - [x] **`PRAGMA foreign_keys` never enabled** (`bb-db/src/lib.rs:26-31`). Fixed: added `after_connect` hook that runs `PRAGMA foreign_keys = ON` on every connection. | |
| 87 | + | - [x] **Feed edit "Save" is broken** (`sources.js:470`). Fixed: pass `feed.id` (UUID) instead of `source.id` (busser_id). | |
| 88 | + | - [x] **SSRF via HTTP redirect** (`host_functions.rs:183-196`). Fixed: created shared `ureq::Agent` with `redirects(0)`. Plugins no longer follow redirects. | |
| 89 | + | - [x] **Error response body read without size limit** (`host_functions.rs:133-157`). Fixed: `format_ureq_error` now reads error bodies via `.into_reader().take(MAX_RESPONSE_BYTES)`. | |
| 90 | + | - [x] **Tokio RwLock held across blocking HTTP** (`orchestrator.rs:199-202`). Fixed: wrapped `plugins.fetch()` in `tokio::task::spawn_blocking`. | |
| 91 | + | - [x] **Key file world-readable between write and chmod** (`crypto.rs:46-53`). Fixed: use `OpenOptionsExt::mode(0o600)` on Unix to set permissions atomically at creation. | |
| 92 | + | - [x] **Raw SQLite errors leaked to frontend in sync service** (`sync_service/*.rs`). Fixed: added `db_err()` helper that logs full error and returns generic message. Applied to all 27 call sites. | |
| 93 | + | ||
| 94 | + | ### MINOR | |
| 95 | + | ||
| 96 | + | - [x] **`count()` loads all rows into memory** (`generator/query.rs:162-176`). Fixed: added `count_filtered()` to `ItemsRepository` using `SELECT COUNT(*)` with dynamic WHERE clause. | |
| 97 | + | - [x] **`100.` prefix blocking overly broad** (`host_functions.rs:63`). Fixed: replaced with `is_cgnat()` that checks `100.64.0.0/10` only. | |
| 98 | + | - [x] **`validate_url` doesn't block encoded IPs** (`host_functions.rs:35-88`). Fixed: block hex-prefixed hosts, pure-numeric hosts (decimal IPs), and octal-prefixed octets. | |
| 99 | + | - [ ] ~~**Non-deterministic synthesized IDs** (`conversions.rs:280-286`)~~. False positive: `DefaultHasher::new()` uses fixed keys and IS deterministic across runs. | |
| 100 | + | - [x] **`truncate` with negative `max_len`** (`host_functions.rs:272-283`). Fixed: `max_len.max(0)` before cast to usize. | |
| 101 | + | - [x] **`bookmark_tags` INSERT without `OR IGNORE`** (`repository.rs:1253`). Fixed: added `OR IGNORE`. | |
| 102 | + | - [x] **Only first feed per plugin used** (`orchestrator.rs:189`). Fixed: track all feed IDs; record success/failure for every feed, not just first. | |
| 103 | + | - [x] **Decryption failure silently clears secret** (`crypto.rs:228-230`). Fixed: upgraded log level to `error` so it's visible in logs. The clear behavior is intentional (prevents ciphertext leakage to plugins). | |
| 104 | + | - [x] **Initial snapshot race** (`sync_scheduler.rs:150-165`). Fixed: moved snapshot creation after acquiring sync mutex. | |
| 105 | + | - [x] **Temp files never cleaned up** (`commands/items.rs:410-414`). Fixed: clean up `bb-downloads/` on startup; use PID-prefixed filenames to prevent collisions. | |
| 106 | + | - [x] **OAuth callback thread leak** (`commands/sync.rs:114-159`). Fixed: generation counter cancels previous callback servers when a new auth flow starts. | |
| 107 | + | - [x] **`Score` ordering NULL as 0** (`ordering.rs:46-52`). Fixed: use `i64::MIN` for None so scoreless items sort after all scored items. | |
| 108 | + | ||
| 109 | + | ### NOTE | |
| 110 | + | ||
| 111 | + | - [ ] **Aggregate memory per fetch uncapped** (`host_functions.rs:21,25`). 100 requests × 2MB = 200MB during execution. Acceptable: per-plugin, user-installed, single-threaded. | |
| 112 | + | - [x] **`run_reader_script` no timeout/validation** (`rhai_plugin/mod.rs:354-369`). Fixed: reader scripts now use 60-second aggregate deadline via `create_engine_inner(true)`. | |
| 113 | + | - [x] **`sanitizeHtml` missing `<meta>` tag** (`frontend/js/utils.js:21`). Fixed: added `meta` and `link` to `DANGEROUS_ELEMENTS`. | |
| 114 | + | - [ ] ~~**FTS5 rowid instability after VACUUM**~~ (`migrations/sqlite/005_create_fts.sql`). Won't fix: app never runs VACUUM. Documented for awareness. | |
| 115 | + | - [x] **`get_all_items` fetches then filters** (`generator/query.rs:119-153`). Fixed: use `list_filtered()` to push source/unread/starred/search to SQL. | |
| 116 | + | - [x] **Plugin errors leak internals** (`commands/error.rs:104`). Fixed: log full error server-side, send generic message to frontend. | |
| 117 | + | ||
| 118 | + | --- | |
| 119 | + | ||
| 68 | 120 | ## Phase 7: Plugin OAuth (Post-beta) | |
| 69 | 121 | ||
| 70 | 122 | ### 7A: OAuth Infrastructure |
| @@ -99,6 +99,14 @@ | |||
| 99 | 99 | // Initialize SyncKit client from saved key or env vars | |
| 100 | 100 | let sync_client = load_sync_client(&app_data_dir); | |
| 101 | 101 | ||
| 102 | + | // Clean up old download temp files from previous sessions | |
| 103 | + | let downloads_dir = std::env::temp_dir().join("bb-downloads"); | |
| 104 | + | if downloads_dir.exists() { | |
| 105 | + | if let Err(e) = std::fs::remove_dir_all(&downloads_dir) { | |
| 106 | + | debug!(error = %e, "Failed to clean up old download temp files"); | |
| 107 | + | } | |
| 108 | + | } | |
| 109 | + | ||
| 102 | 110 | info!("Application state initialized"); | |
| 103 | 111 | ||
| 104 | 112 | Ok(Self { |
| @@ -146,7 +146,11 @@ | |||
| 146 | 146 | } | |
| 147 | 147 | } | |
| 148 | 148 | ||
| 149 | - | // Create initial snapshot on first sync | |
| 149 | + | // Prevent concurrent sync operations (manual + scheduler). | |
| 150 | + | let _sync_guard = state.sync_mutex.lock().await; | |
| 151 | + | ||
| 152 | + | // Create initial snapshot on first sync (inside mutex to prevent | |
| 153 | + | // duplicate snapshots from concurrent manual + scheduled sync). | |
| 150 | 154 | let snapshot_done = sync_service::get_sync_state(pool, "initial_snapshot_done") | |
| 151 | 155 | .await | |
| 152 | 156 | .unwrap_or_default(); | |
| @@ -161,9 +165,6 @@ | |||
| 161 | 165 | } | |
| 162 | 166 | } | |
| 163 | 167 | ||
| 164 | - | // Prevent concurrent sync operations (manual + scheduler). | |
| 165 | - | let _sync_guard = state.sync_mutex.lock().await; | |
| 166 | - | ||
| 167 | 168 | // Perform sync | |
| 168 | 169 | match sync_service::perform_sync(pool, &client).await { | |
| 169 | 170 | Ok(result) => { |
| @@ -36,22 +36,26 @@ | |||
| 36 | 36 | use std::io::Write; | |
| 37 | 37 | ||
| 38 | 38 | // Try to create the file exclusively first (atomic check-and-create). | |
| 39 | - | match std::fs::OpenOptions::new() | |
| 39 | + | #[cfg(unix)] | |
| 40 | + | let open_result = { | |
| 41 | + | use std::os::unix::fs::OpenOptionsExt; | |
| 42 | + | std::fs::OpenOptions::new() | |
| 43 | + | .write(true) | |
| 44 | + | .create_new(true) | |
| 45 | + | .mode(0o600) | |
| 46 | + | .open(path) | |
| 47 | + | }; | |
| 48 | + | #[cfg(not(unix))] | |
| 49 | + | let open_result = std::fs::OpenOptions::new() | |
| 40 | 50 | .write(true) | |
| 41 | 51 | .create_new(true) | |
| 42 | - | .open(path) | |
| 43 | - | { | |
| 52 | + | .open(path); | |
| 53 | + | ||
| 54 | + | match open_result { | |
| 44 | 55 | Ok(mut file) => { | |
| 45 | 56 | let key = generate_key(); | |
| 46 | 57 | file.write_all(&*key) | |
| 47 | 58 | .map_err(|e| format!("Failed to write encryption key: {e}"))?; | |
| 48 | - | #[cfg(unix)] | |
| 49 | - | { | |
| 50 | - | use std::os::unix::fs::PermissionsExt; | |
| 51 | - | let perms = std::fs::Permissions::from_mode(0o600); | |
| 52 | - | std::fs::set_permissions(path, perms) | |
| 53 | - | .map_err(|e| format!("Failed to set key file permissions: {e}"))?; | |
| 54 | - | } | |
| 55 | 59 | Ok(key) | |
| 56 | 60 | } | |
| 57 | 61 | Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { | |
| @@ -226,7 +230,7 @@ | |||
| 226 | 230 | obj.insert(field.key.clone(), serde_json::Value::String(decrypted)); | |
| 227 | 231 | } | |
| 228 | 232 | Err(e) => { | |
| 229 | - | tracing::warn!(field = %field.key, error = %e, "Failed to decrypt secret, clearing field to prevent ciphertext leakage"); | |
| 233 | + | tracing::error!(field = %field.key, error = %e, "Failed to decrypt secret, clearing field to prevent ciphertext leakage. Feed may need re-configuration."); | |
| 230 | 234 | obj.insert(field.key.clone(), serde_json::Value::String(String::new())); | |
| 231 | 235 | } | |
| 232 | 236 | } |
| @@ -184,22 +184,29 @@ | |||
| 184 | 184 | &self, | |
| 185 | 185 | plugin_id: &str, | |
| 186 | 186 | ) -> Result<usize, OrchestratorError> { | |
| 187 | - | // Get feed ID for this busser | |
| 187 | + | // Get all feed IDs for this busser (usually one, but can be multiple) | |
| 188 | 188 | let feeds = self.db.feeds().get_by_busser(plugin_id).await?; | |
| 189 | - | let feed_id = feeds.first().map(|f| f.id); | |
| 189 | + | let feed_ids: Vec<bb_db::FeedId> = feeds.iter().map(|f| f.id).collect(); | |
| 190 | 190 | ||
| 191 | - | let Some(feed_id) = feed_id else { | |
| 191 | + | if feed_ids.is_empty() { | |
| 192 | 192 | debug!(%plugin_id, "No feeds configured for plugin, skipping store"); | |
| 193 | 193 | return Ok(0); | |
| 194 | - | }; | |
| 194 | + | } | |
| 195 | + | // Primary feed for item storage (all items go under first feed) | |
| 196 | + | let feed_id = feed_ids[0]; | |
| 195 | 197 | ||
| 196 | - | // Acquire the read lock only for the fetch call, then release it | |
| 197 | - | // before any async DB operations to avoid holding the lock across | |
| 198 | - | // blocking network I/O. | |
| 199 | - | let fetch_result = { | |
| 200 | - | let plugins = self.plugins.read().await; | |
| 201 | - | plugins.fetch(plugin_id, None) | |
| 202 | - | }; | |
| 198 | + | // Run the blocking fetch on a dedicated thread to avoid starving | |
| 199 | + | // the tokio runtime. Plugin fetches do synchronous HTTP inside the | |
| 200 | + | // Rhai engine, which would block worker threads if run inline. | |
| 201 | + | let plugins = self.plugins.clone(); | |
| 202 | + | let pid = plugin_id.to_string(); | |
| 203 | + | let fetch_result = tokio::task::spawn_blocking(move || { | |
| 204 | + | let handle = tokio::runtime::Handle::current(); | |
| 205 | + | let plugins = handle.block_on(plugins.read()); | |
| 206 | + | plugins.fetch(&pid, None) | |
| 207 | + | }) | |
| 208 | + | .await | |
| 209 | + | .map_err(|e| OrchestratorError::Config(format!("Fetch task panicked: {e}")))?; | |
| 203 | 210 | ||
| 204 | 211 | let result = match fetch_result { | |
| 205 | 212 | Ok(r) => r, | |
| @@ -211,24 +218,26 @@ | |||
| 211 | 218 | category = %structured.category, | |
| 212 | 219 | "Fetch failed" | |
| 213 | 220 | ); | |
| 214 | - | match self | |
| 215 | - | .db | |
| 216 | - | .feeds() | |
| 217 | - | .record_fetch_failure_structured(feed_id, &structured) | |
| 218 | - | .await | |
| 219 | - | { | |
| 220 | - | Ok(tripped) => { | |
| 221 | - | if tripped { | |
| 222 | - | info!( | |
| 223 | - | %feed_id, %plugin_id, | |
| 224 | - | category = %structured.category, | |
| 225 | - | "Circuit breaker tripped for feed" | |
| 226 | - | ); | |
| 221 | + | for fid in &feed_ids { | |
| 222 | + | match self | |
| 223 | + | .db | |
| 224 | + | .feeds() | |
| 225 | + | .record_fetch_failure_structured(*fid, &structured) | |
| 226 | + | .await | |
| 227 | + | { | |
| 228 | + | Ok(tripped) => { | |
| 229 | + | if tripped { | |
| 230 | + | info!( | |
| 231 | + | feed_id = %fid, %plugin_id, | |
| 232 | + | category = %structured.category, | |
| 233 | + | "Circuit breaker tripped for feed" | |
| 234 | + | ); | |
| 235 | + | } | |
| 236 | + | } | |
| 237 | + | Err(db_err) => { | |
| 238 | + | error!(error = %db_err, "Failed to record fetch failure"); | |
| 227 | 239 | } | |
| 228 | 240 | } | |
| 229 | - | Err(db_err) => { | |
| 230 | - | error!(error = %db_err, "Failed to record fetch failure"); | |
| 231 | - | } | |
| 232 | 241 | } | |
| 233 | 242 | return Err(e.into()); | |
| 234 | 243 | } | |
| @@ -261,8 +270,10 @@ | |||
| 261 | 270 | } | |
| 262 | 271 | } | |
| 263 | 272 | ||
| 264 | - | // Record successful fetch (resets failure counter) | |
| 265 | - | self.db.feeds().record_fetch_success(feed_id).await?; | |
| 273 | + | // Record successful fetch for all feeds (resets failure counter) | |
| 274 | + | for fid in &feed_ids { | |
| 275 | + | self.db.feeds().record_fetch_success(*fid).await?; | |
| 276 | + | } | |
| 266 | 277 | ||
| 267 | 278 | info!(count, %plugin_id, "Fetched items from plugin"); | |
| 268 | 279 | Ok(count) |