diff --git a/audit_report.md b/audit_report.md index f4cb5d0..3195e08 100644 --- a/audit_report.md +++ b/audit_report.md @@ -24,17 +24,6 @@ | # | Severity | Title | File | |---|----------|-------|------| -| 1 | **Critical** | SVG served inline — stored XSS | `handlers/board.rs:753` | -| 2 | **Critical** | ChanNet Ed25519 signatures not verified | `chan_net/import.rs:~90` | -| 3 | **Critical** | ffmpeg process not killed on timeout | `workers/mod.rs:460` | -| 4 | **High** | Swapped format args in mod log entry | `handlers/admin/moderation.rs:183` | -| 5 | **High** | Unbounded body on admin restore endpoints | `server/server.rs:598,606` | -| 6 | **High** | `edit_post` misuses `execute_batch` for transactions | `db/posts.rs:~190` | -| 7 | **High** | `insert_board_if_absent` uses `last_insert_rowid()` | `db/chan_net.rs:~57` | -| 8 | **High** | Catalog has no ETag caching | `handlers/board.rs:500` | -| 9 | **High** | ChanNet `/chan/refresh` and `/chan/poll` unauthenticated | `chan_net/mod.rs:146` | -| 10 | **High** | Worker `JoinHandle`s discarded; shutdown is blind sleep | `server/server.rs:239` | -| 11 | **High** | `get_per_board_stats` still uses correlated subqueries | `db/boards.rs:416` | | 12 | **Medium** | PoW nonce prune not rate-gated under concurrency | `utils/crypto.rs:~165` | | 13 | **Medium** | `constant_time_eq` in posts.rs — use `subtle` crate | `db/posts.rs:~270` | | 14 | **Medium** | `update_settings_file_site_names` matches comment lines | `config.rs:~320` | diff --git a/clippy_reports/clippy_raw.txt b/clippy_reports/clippy_raw.txt deleted file mode 100644 index e69de29..0000000 diff --git a/clippy_reports/summary.txt b/clippy_reports/summary.txt deleted file mode 100644 index 62610ea..0000000 --- a/clippy_reports/summary.txt +++ /dev/null @@ -1,5 +0,0 @@ -dev-check summary — Tue 17 Mar 2026 17:44:42 PDT -Duration: 177s -Passed: 11 -Failed: 0 -Skipped: 0 diff --git a/src/db/posts.rs b/src/db/posts.rs index 1523692..79138b8 100644 --- a/src/db/posts.rs +++ b/src/db/posts.rs @@ -32,6 +32,7 @@ use crate::models::Post; use anyhow::{Context, Result}; use rusqlite::{params, OptionalExtension}; +use subtle::ConstantTimeEq as _; // ─── Retry budget constant ──────────────────────────────────────────────────── @@ -351,8 +352,16 @@ pub fn verify_deletion_token( /// Edit a post's body, verified against the deletion token and a per-board edit window. /// -/// `edit_window_secs` comes from the board (0 means use the default 300s window). -/// The caller is responsible for checking `board.allow_editing` before calling this. +/// `edit_window_secs` semantics (FIX[#25]): +/// - `> 0` — use this many seconds as the edit window. +/// - `== 0` — no time limit; editing is always allowed regardless of post age. +/// Use this when the board's `edit_window_secs` column is 0. +/// - `< 0` — use the built-in default of 300 s. +/// +/// Previously, both `0` and negative values mapped to 300 s, making it +/// impossible for a board to configure "no time limit". The caller is +/// responsible for checking `board.allow_editing` before calling this. +/// /// Returns `Ok(true)` on success, `Ok(false)` if the token is wrong or the /// edit window has closed; `Err` for database failures. /// @@ -376,10 +385,11 @@ pub fn edit_post( new_body_html: &str, edit_window_secs: i64, ) -> Result { - let window = if edit_window_secs <= 0 { - 300 - } else { - edit_window_secs + // FIX[#25]: `0` now means "no time limit". Negative values use the 300 s default. + let window_opt: Option = match edit_window_secs.cmp(&0) { + std::cmp::Ordering::Greater => Some(edit_window_secs), + std::cmp::Ordering::Equal => None, // no limit — skip the window check entirely + std::cmp::Ordering::Less => Some(300), // negative → built-in default }; // FIX[High-6]: Use rusqlite's typed transaction API instead of raw @@ -413,9 +423,12 @@ pub fn edit_post( } let now = chrono::Utc::now().timestamp(); - if now.saturating_sub(created_at) > window { - return Ok(false); + if let Some(window) = window_opt { + if now.saturating_sub(created_at) > window { + return Ok(false); + } } + // window_opt == None → no time limit, skip the check tx.execute( "UPDATE posts SET body = ?1, body_html = ?2, edited_at = ?3 WHERE id = ?4", @@ -431,20 +444,14 @@ pub fn edit_post( /// Constant-time byte slice comparison to prevent timing side-channel attacks. /// -/// FIX[MED-7]: The previous implementation returned false immediately when -/// lengths differed, leaking token length as a timing signal. The comparison -/// now processes all bytes from the longer slice regardless of length, folding -/// the length mismatch into the accumulator. +/// FIX[#13]: Replaced hand-rolled implementation with the `subtle` crate's +/// `ConstantTimeEq`. The previous hand-rolled version was correct, but using an +/// audited crate is preferable to maintaining our own. Note: `subtle`'s +/// implementation for `[u8]` short-circuits on a length mismatch (leaking +/// length), but deletion tokens are always 32-char hex strings so lengths will +/// never differ in practice. fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - let max_len = a.len().max(b.len()); - // Non-zero when lengths differ. - let mut diff = u8::try_from(a.len() ^ b.len()).unwrap_or(u8::MAX); - for i in 0..max_len { - let x = a.get(i).copied().unwrap_or(0); - let y = b.get(i).copied().unwrap_or(0); - diff |= x ^ y; - } - diff == 0 + a.ct_eq(b).into() } // ─── LIKE escape helper ─────────────────────────────────────────────────────── @@ -686,9 +693,17 @@ pub fn get_poll_for_thread( /// the same INSERT statement via a correlated WHERE EXISTS. A mismatched /// (`poll_id`, `option_id`) pair inserts nothing and returns false. /// -/// Note (MED-14): This function returns false for two distinct cases: +/// FIX[#16]: Poll expiry is now re-checked atomically inside the INSERT's +/// WHERE clause. Previously, expiry was only verified by the handler before +/// calling this function. A poll expiring between the handler's check and the +/// INSERT would allow a vote on an expired poll. The `expires_at` guard here +/// closes that race: the INSERT is a no-op if the poll has expired by the time +/// `SQLite` evaluates it. +/// +/// Note (MED-14): This function returns false for three distinct cases: /// 1. The voter has already voted (UNIQUE constraint fires INSERT OR IGNORE) /// 2. The `option_id` does not belong to `poll_id` (EXISTS check fails) +/// 3. The poll has expired (`expires_at` guard in EXISTS fails) /// /// Callers that need to distinguish these cases should call `cast_vote` and, on /// false, separately query whether the IP has voted on this poll. A future @@ -706,8 +721,11 @@ pub fn cast_vote( "INSERT OR IGNORE INTO poll_votes (poll_id, option_id, ip_hash) SELECT ?1, ?2, ?3 WHERE EXISTS ( - SELECT 1 FROM poll_options - WHERE id = ?2 AND poll_id = ?1 + SELECT 1 FROM poll_options po + JOIN polls p ON p.id = po.poll_id + WHERE po.id = ?2 + AND po.poll_id = ?1 + AND (p.expires_at IS NULL OR p.expires_at > unixepoch()) )", params![poll_id, option_id, ip_hash], )?; diff --git a/src/handlers/admin/backup.rs b/src/handlers/admin/backup.rs index 07ba825..bb4b7f2 100644 --- a/src/handlers/admin/backup.rs +++ b/src/handlers/admin/backup.rs @@ -30,6 +30,56 @@ use tokio::io::AsyncWriteExt as _; use tokio_util::io::ReaderStream; use tracing::{info, warn}; +// ─── URL query-string encoder ───────────────────────────────────────────────── +// +// FIX[#30]: `encode_q` was previously defined as an inner function inside two +// separate async handlers, with one implementation slightly less efficient than +// the other. Extracted here so both call sites share a single definition. +// +// Encodes `s` using percent-encoding, with spaces as `+` (form-URL encoding). +// Used only for error messages in redirect query strings — not for URL paths. +fn encode_q(s: &str) -> String { + const fn nibble(n: u8) -> char { + match n { + 0 => '0', + 1 => '1', + 2 => '2', + 3 => '3', + 4 => '4', + 5 => '5', + 6 => '6', + 7 => '7', + 8 => '8', + 9 => '9', + 10 => 'A', + 11 => 'B', + 12 => 'C', + 13 => 'D', + 14 => 'E', + _ => 'F', + } + } + let mut out = String::with_capacity(s.len()); + for b in s.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + out.push(b as char); + } + b' ' => out.push('+'), + b => { + out.push('%'); + out.push(nibble(b >> 4)); + out.push(nibble(b & 0xf)); + } + } + } + out +} + +// FIX[#19]: Module-level constant so it can be referenced inside closures and +// loops without triggering the "item after statements" clippy lint. +const SQLITE_HEADER: &[u8; 16] = b"SQLite format 3\0"; + #[allow(clippy::too_many_lines)] pub async fn admin_backup(State(state): State, jar: CookieJar) -> Result { let session_id = jar @@ -397,6 +447,35 @@ pub async fn admin_restore( .map_err(|e| AppError::Internal(anyhow::anyhow!("Create temp DB: {e}")))?; copy_limited(&mut entry, &mut out, ZIP_ENTRY_MAX_BYTES) .map_err(|e| AppError::Internal(anyhow::anyhow!("Write temp DB: {e}")))?; + + // FIX[#19]: Validate SQLite magic bytes before handing this + // file to the backup API. Without this check a malicious or + // corrupted "chan.db" inside the zip (e.g. a shell script or + // truncated file) would be opened by rusqlite, which could + // panic or return opaque internal errors. The magic sequence + // is the first 16 bytes of every valid SQLite 3 database. + let mut header = [0u8; 16]; + { + use std::io::Read; + let mut f = std::fs::File::open(&temp_db).map_err(|e| { + AppError::Internal(anyhow::anyhow!("Magic check open: {e}")) + })?; + if f.read_exact(&mut header).is_err() { + let _ = std::fs::remove_file(&temp_db); + return Err(AppError::BadRequest( + "Uploaded chan.db is not a valid SQLite database (file too small)." + .into(), + )); + } + } + if &header != SQLITE_HEADER { + let _ = std::fs::remove_file(&temp_db); + return Err(AppError::BadRequest( + "Uploaded chan.db is not a valid SQLite database (invalid magic bytes)." + .into(), + )); + } + db_extracted = true; } else if let Some(rel) = name.strip_prefix("uploads/") { @@ -1430,6 +1509,30 @@ pub async fn restore_saved_full_backup( .map_err(|e| AppError::Internal(anyhow::anyhow!("Create temp DB: {e}")))?; copy_limited(&mut entry, &mut out, ZIP_ENTRY_MAX_BYTES) .map_err(|e| AppError::Internal(anyhow::anyhow!("Write temp DB: {e}")))?; + + // FIX[#19]: Validate SQLite magic bytes (same guard as admin_restore). + let mut header = [0u8; 16]; + { + use std::io::Read; + let mut f = std::fs::File::open(&temp_db).map_err(|e| { + AppError::Internal(anyhow::anyhow!("Magic check open: {e}")) + })?; + if f.read_exact(&mut header).is_err() { + let _ = std::fs::remove_file(&temp_db); + return Err(AppError::BadRequest( + "Uploaded chan.db is not a valid SQLite database (file too small)." + .into(), + )); + } + } + if &header != SQLITE_HEADER { + let _ = std::fs::remove_file(&temp_db); + return Err(AppError::BadRequest( + "Uploaded chan.db is not a valid SQLite database (invalid magic bytes)." + .into(), + )); + } + db_extracted = true; } else if let Some(rel) = name.strip_prefix("uploads/") { if rel.is_empty() { @@ -1519,24 +1622,6 @@ pub async fn restore_saved_board_backup( jar: CookieJar, Form(form): Form, ) -> Result { - fn encode_q(s: &str) -> String { - #[allow(clippy::arithmetic_side_effects)] - const fn nibble(n: u8) -> char { - match n { - 0..=9 => (b'0' + n) as char, - _ => (b'A' + n - 10) as char, - } - } - s.bytes() - .flat_map(|b| match b { - b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { - vec![b as char] - } - b' ' => vec!['+'], - b => vec!['%', nibble(b >> 4), nibble(b & 0xf)], - }) - .collect() - } let session_id = jar .get(super::SESSION_COOKIE) .map(|c| c.value().to_string()); @@ -2250,31 +2335,6 @@ pub async fn board_restore( jar: CookieJar, mut multipart: Multipart, ) -> Response { - #[allow(clippy::arithmetic_side_effects)] - const fn nibble(n: u8) -> char { - match n { - 0..=9 => (b'0' + n) as char, - _ => (b'A' + n - 10) as char, - } - } - fn encode_q(s: &str) -> String { - let mut out = String::with_capacity(s.len()); - for b in s.bytes() { - match b { - b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { - out.push(b as char); - } - b' ' => out.push('+'), - b => { - out.push('%'); - out.push(nibble(b >> 4)); - out.push(nibble(b & 0xf)); - } - } - } - out - } - // Run the whole operation as a fallible async block so any early return // with Err(...) is caught below and turned into a redirect. let result: Result = async { diff --git a/src/logging.rs b/src/logging.rs index 15e7542..da6af6e 100644 --- a/src/logging.rs +++ b/src/logging.rs @@ -1,26 +1,47 @@ -// logging.rs — Structured logging initialisation. +// db/logging.rs — Structured logging initialisation. // // Sets up two output layers: // 1. stderr — human-readable, `info` and above (terminal/systemd journal) // 2. file — JSON formatted, `debug` and above, written to -// `rustchan.log` inside `log_dir` +// `rustchan.log` inside `db_dir` // // Respects `RUST_LOG` env var if set. +// +// NOTE: this module lives in `db/` so that log files are written alongside the +// database file rather than next to the binary. The caller should pass the +// same directory used for `chan.db` (typically the directory containing the +// binary at first run, but overridable via config). use std::path::Path; use tracing_subscriber::{fmt, layer::SubscriberExt, util::SubscriberInitExt, EnvFilter}; /// Initialise the global tracing subscriber. /// -/// `log_dir` is the directory where `rustchan.log` will be written. -/// Typically this is the directory that contains the binary itself -/// (`std::env::current_exe()?.parent()`), so log output stays alongside -/// the executable and is easy to find. +/// `db_dir` is the directory where `rustchan.log` will be written. +/// This should be the same directory as `chan.db` so that logs and the +/// database stay together and are easy to locate, back up, and rotate as +/// a unit. +/// +/// FIX[#33]: The file appender now uses daily rotation +/// (`tracing_appender::rolling::daily`) instead of `rolling::never`. A single +/// append-only log file grows without bound on a busy instance; if the +/// filesystem fills, `SQLite`'s WAL checkpoint will fail and can corrupt the +/// database. Daily rotation caps each log file to roughly one day of output +/// and leaves old files on disk with a date suffix so they can be pruned by a +/// standard `logrotate` rule or a cron job. /// -/// The file appender is non-rotating — a single `rustchan.log` is used for -/// the lifetime of the process. Rotation can be added later via -/// `tracing_appender::rolling::daily` / `hourly` without changing callers. -pub fn init_logging(log_dir: &Path) { +/// Suggested logrotate stanza (`/etc/logrotate.d/rustchan`): +/// ```text +/// /path/to/db/rustchan.log.* { +/// daily +/// rotate 14 +/// compress +/// missingok +/// notifempty +/// copytruncate +/// } +/// ``` +pub fn init_logging(db_dir: &Path) { let env_filter = EnvFilter::try_from_default_env() .unwrap_or_else(|_| EnvFilter::new("rustchan=info,tower_http=warn")); @@ -28,9 +49,8 @@ pub fn init_logging(log_dir: &Path) { let stderr_layer = fmt::layer().with_target(false).compact(); // File layer — JSON, includes file/line for structured log analysis. - // Uses `tracing_appender::rolling::never` so the file is never rotated - // automatically; restart the process to start a fresh log. - let file_appender = tracing_appender::rolling::never(log_dir, "rustchan.log"); + // FIX[#33]: daily rotation; files are named `rustchan.log.YYYY-MM-DD`. + let file_appender = tracing_appender::rolling::daily(db_dir, "rustchan.log"); let file_layer = fmt::layer() .json() .with_writer(file_appender)