From e26027524df53d5985842c336bc63f6a70328893 Mon Sep 17 00:00:00 2001 From: csd113 Date: Sun, 8 Mar 2026 21:34:25 -0700 Subject: [PATCH 1/2] tpyos --- src/config.rs | 143 +++++++++++++++++++------------------------------- 1 file changed, 55 insertions(+), 88 deletions(-) diff --git a/src/config.rs b/src/config.rs index 24def58..190753f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -10,17 +10,15 @@ // // SECURITY: The cookie_secret is auto-generated on first run and persisted // to settings.toml. It is never left at a well-known default value. -// FIX[CRITICAL-1]: removed hardcoded default secret; see generate_settings_file_if_missing(). use once_cell::sync::Lazy; +use rand_core::{OsRng, RngCore}; use serde::Deserialize; use std::env; use std::path::PathBuf; /// Absolute path to the directory the running binary lives in. fn binary_dir() -> PathBuf { - // FIX[MEDIUM-2]: log a warning when fallback is used so operators - // are aware that data may land in an unexpected location. match std::env::current_exe() .ok() .and_then(|p| p.parent().map(|p| p.to_path_buf())) @@ -38,8 +36,8 @@ fn binary_dir() -> PathBuf { } fn settings_file_path() -> PathBuf { - // Store settings.toml in rustrustchan-data/ alongside the database. - // rustrustchan-data/ is created by run_server before CONFIG is first accessed, + // Store settings.toml in rustchan-data/ alongside the database. + // rustchan-data/ is created by run_server before CONFIG is first accessed, // so this directory always exists by the time settings are read. let data_dir = binary_dir().join("rustchan-data"); data_dir.join("settings.toml") @@ -50,12 +48,10 @@ fn settings_file_path() -> PathBuf { #[derive(Deserialize, Default)] struct SettingsFile { forum_name: Option, - /// Home page subtitle shown below the site name. Displayed on the index - /// page. Can also be changed later via the admin panel. + /// Home page subtitle shown below the site name. site_subtitle: Option, /// Default theme served to first-time visitors before they pick one. /// Valid values: terminal, aero, dorfic, fluorogrid, neoncubicle, chanclassic - /// (empty string or "terminal" = default dark terminal theme). default_theme: Option, port: Option, max_image_size_mb: Option, @@ -83,9 +79,9 @@ fn load_settings_file() -> SettingsFile { /// Create settings.toml with defaults if it does not exist yet. /// Call this once at startup (before CONFIG is accessed for the first time). /// -/// FIX[CRITICAL-1]: A cryptographically random cookie_secret is generated on -/// first run and written to settings.toml. Subsequent runs load it from the -/// file. The server never operates with a known/default secret. +/// A cryptographically random cookie_secret is generated on first run and +/// written to settings.toml. Subsequent runs load it from the file. +/// The server never operates with a known/default secret. pub fn generate_settings_file_if_missing() { let path = settings_file_path(); if path.exists() { @@ -93,8 +89,6 @@ pub fn generate_settings_file_if_missing() { } // Generate a random 64-hex-char secret (32 bytes of entropy). - // This runs before CONFIG is initialised, so we call OsRng directly. - use rand_core::{OsRng, RngCore}; let mut secret_bytes = [0u8; 32]; OsRng.fill_bytes(&mut secret_bytes); let secret = hex::encode(secret_bytes); @@ -170,7 +164,7 @@ pub struct Config { /// Initial default theme slug (seeds the DB on first run). /// Valid: terminal, aero, dorfic, fluorogrid, neoncubicle, chanclassic pub initial_default_theme: String, - #[allow(dead_code)] + #[allow(dead_code)] // read by CLI subcommands and printed at startup pub port: u16, pub max_image_size: usize, // bytes pub max_video_size: usize, // bytes @@ -187,11 +181,11 @@ pub struct Config { pub database_path: String, pub upload_dir: String, pub thumb_size: u32, - #[allow(dead_code)] + #[allow(dead_code)] // used as default when creating boards via CLI/admin pub default_bump_limit: u32, - #[allow(dead_code)] + #[allow(dead_code)] // used as default when creating boards via CLI/admin pub max_threads_per_board: u32, - /// Maximum GET requests per IP per rate_limit_window (CRIT-3: DoS via catalog/search). + /// Maximum GET requests per IP per rate_limit_window. pub rate_limit_gets: u32, pub rate_limit_window: u64, pub cookie_secret: String, @@ -224,20 +218,21 @@ impl Config { "CHAN_DEFAULT_THEME", s.default_theme.as_deref().unwrap_or("terminal"), ); - let port = env_u16("CHAN_PORT", s.port.unwrap_or(8080)); - let max_image_mb = env_u32("CHAN_MAX_IMAGE_MB", s.max_image_size_mb.unwrap_or(8)); - let max_video_mb = env_u32("CHAN_MAX_VIDEO_MB", s.max_video_size_mb.unwrap_or(50)); - let max_audio_mb = env_u32("CHAN_MAX_AUDIO_MB", s.max_audio_size_mb.unwrap_or(150)); - - let host = env_str("CHAN_HOST", "0.0.0.0"); - let bind_addr = env_str("CHAN_BIND", &format!("{host}:{port}")); + let port: u16 = env_parse("CHAN_PORT", s.port.unwrap_or(8080)); + let max_image_mb: u32 = env_parse("CHAN_MAX_IMAGE_MB", s.max_image_size_mb.unwrap_or(8)); + let max_video_mb: u32 = env_parse("CHAN_MAX_VIDEO_MB", s.max_video_size_mb.unwrap_or(50)); + let max_audio_mb: u32 = env_parse("CHAN_MAX_AUDIO_MB", s.max_audio_size_mb.unwrap_or(150)); + + let bind_addr = env_str( + "CHAN_BIND", + &format!("{}:{}", env_str("CHAN_HOST", "0.0.0.0"), port), + ); let behind_proxy = env_bool("CHAN_BEHIND_PROXY", false); - // FIX[CRITICAL-1]: Resolve cookie_secret from env > settings.toml. - // If neither is set, emit a loud warning. The generate_settings_file_if_missing() - // call at startup ensures settings.toml always has a generated secret, - // so this fallback should only be reached in abnormal circumstances. + // Resolve cookie_secret from env > settings.toml. + // generate_settings_file_if_missing() ensures settings.toml always has + // a generated secret, so this fallback should only fire in abnormal cases. let cookie_secret = if let Ok(v) = env::var("CHAN_COOKIE_SECRET") { v } else if let Some(v) = s.cookie_secret { @@ -248,10 +243,10 @@ impl Config { IP hashing is using an empty secret. Run the server once to auto-generate, \ or set CHAN_COOKIE_SECRET." ); - // Emit a random in-memory secret so each restart invalidates hashes + // Random in-memory secret so each restart invalidates hashes // (better than a known empty string, worse than a persisted one). let mut b = [0u8; 32]; - rand_core::OsRng.fill_bytes(&mut b); + OsRng.fill_bytes(&mut b); hex::encode(b) }; @@ -270,16 +265,16 @@ impl Config { bind_addr, database_path: env_str("CHAN_DB", &default_db), upload_dir: env_str("CHAN_UPLOADS", &default_uploads), - thumb_size: env_u32("CHAN_THUMB_SIZE", 250), - default_bump_limit: env_u32("CHAN_BUMP_LIMIT", 500), - max_threads_per_board: env_u32("CHAN_MAX_THREADS", 150), - rate_limit_gets: env_u32("CHAN_RATE_GETS", 60), - rate_limit_window: env_u64("CHAN_RATE_WINDOW", 60), + thumb_size: env_parse("CHAN_THUMB_SIZE", 250), + default_bump_limit: env_parse("CHAN_BUMP_LIMIT", 500), + max_threads_per_board: env_parse("CHAN_MAX_THREADS", 150), + rate_limit_gets: env_parse("CHAN_RATE_GETS", 60), + rate_limit_window: env_parse("CHAN_RATE_WINDOW", 60), cookie_secret, - session_duration: env_i64("CHAN_SESSION_SECS", 8 * 3600), + session_duration: env_parse("CHAN_SESSION_SECS", 8 * 3600), behind_proxy, https_cookies: env_bool("CHAN_HTTPS_COOKIES", behind_proxy), - wal_checkpoint_interval: env_u64( + wal_checkpoint_interval: env_parse( "CHAN_WAL_CHECKPOINT_SECS", s.wal_checkpoint_interval_secs.unwrap_or(3600), ), @@ -327,8 +322,6 @@ impl Config { ); } - // Port 0 is technically valid (OS assigns one) but almost certainly a - // misconfiguration in a server context. if self.port == 0 { anyhow::bail!("CONFIG ERROR: port must not be 0."); } @@ -336,7 +329,6 @@ impl Config { // Verify the upload directory is writable. let upload_path = std::path::Path::new(&self.upload_dir); if upload_path.exists() { - // Try writing a probe file to verify write permission. let probe = upload_path.join(".write_probe"); if std::fs::write(&probe, b"").is_err() { anyhow::bail!( @@ -351,8 +343,7 @@ impl Config { } } -// ─── Import needed for OsRng in cookie_secret fallback ─────────────────────── -use rand_core::RngCore as _; +// ─── Cookie secret rotation check ──────────────────────────────────────────── /// Check whether the cookie_secret has changed since the last run by comparing /// a SHA-256 hash stored in the DB against the currently loaded secret. @@ -379,64 +370,40 @@ pub fn check_cookie_secret_rotation(conn: &rusqlite::Connection) { ) .ok(); - match stored { - None => { - // First run — store the hash silently. - let _ = conn.execute( - "INSERT INTO site_settings (key, value) VALUES (?1, ?2) - ON CONFLICT(key) DO UPDATE SET value = excluded.value", - rusqlite::params![KEY, current_hash], - ); - } - Some(ref h) if h == ¤t_hash => { - // Secret unchanged — nothing to do. - } - Some(_) => { - // Secret has rotated. - tracing::warn!( - "SECURITY WARNING: cookie_secret has changed since the last run. \ - All IP-based bans are now invalid because all IP hashes have changed. \ - If this was unintentional, restore the previous cookie_secret from \ - settings.toml. If intentional, consider running: \ - DELETE FROM bans; DELETE FROM ban_appeals;" - ); - // Update the stored hash so subsequent restarts with the same secret are silent. - let _ = conn.execute( - "INSERT INTO site_settings (key, value) VALUES (?1, ?2) - ON CONFLICT(key) DO UPDATE SET value = excluded.value", - rusqlite::params![KEY, current_hash], - ); + if let Some(ref h) = stored { + if h == ¤t_hash { + return; // Secret unchanged — nothing to do. } + tracing::warn!( + "SECURITY WARNING: cookie_secret has changed since the last run. \ + All IP-based bans are now invalid because all IP hashes have changed. \ + If this was unintentional, restore the previous cookie_secret from \ + settings.toml. If intentional, consider running: \ + DELETE FROM bans; DELETE FROM ban_appeals;" + ); } + + // First run (None) or rotated secret (Some) — store the current hash. + let _ = conn.execute( + "INSERT INTO site_settings (key, value) VALUES (?1, ?2) + ON CONFLICT(key) DO UPDATE SET value = excluded.value", + rusqlite::params![KEY, current_hash], + ); } +// ─── Env helpers ────────────────────────────────────────────────────────────── + fn env_str(key: &str, default: &str) -> String { env::var(key).unwrap_or_else(|_| default.to_string()) } -fn env_u16(key: &str, default: u16) -> u16 { - env::var(key) - .ok() - .and_then(|v| v.parse().ok()) - .unwrap_or(default) -} -fn env_u32(key: &str, default: u32) -> u32 { - env::var(key) - .ok() - .and_then(|v| v.parse().ok()) - .unwrap_or(default) -} -fn env_u64(key: &str, default: u64) -> u64 { - env::var(key) - .ok() - .and_then(|v| v.parse().ok()) - .unwrap_or(default) -} -fn env_i64(key: &str, default: i64) -> i64 { + +fn env_parse(key: &str, default: T) -> T { env::var(key) .ok() .and_then(|v| v.parse().ok()) .unwrap_or(default) } + fn env_bool(key: &str, default: bool) -> bool { env::var(key) .map(|v| v == "1" || v.eq_ignore_ascii_case("true")) From 0bb4c928fbb14eac92748d2e2ef859e1bf113113 Mon Sep 17 00:00:00 2001 From: csd113 Date: Mon, 9 Mar 2026 00:49:21 -0700 Subject: [PATCH 2/2] bug fixes --- src/db/admin.rs | 15 +- src/db/mod.rs | 33 +++- src/db/posts.rs | 79 ++++++++- src/db/threads.rs | 44 ++++- src/detect.rs | 226 +++++++++++++++++++------ src/error.rs | 18 +- src/handlers/admin.rs | 359 ++++++++++++++++++++++++---------------- src/handlers/board.rs | 48 ++++-- src/handlers/mod.rs | 38 +++-- src/handlers/thread.rs | 54 +++--- src/main.rs | 221 ++++++++++++++++++------- src/middleware/mod.rs | 74 ++++++++- src/models.rs | 205 ++++++++++++++++++++--- src/templates/admin.rs | 11 +- src/templates/board.rs | 17 +- src/templates/forms.rs | 50 +++--- src/templates/mod.rs | 19 ++- src/templates/thread.rs | 12 +- src/utils/crypto.rs | 38 ++++- src/utils/files.rs | 125 +++++++++++--- src/utils/sanitize.rs | 29 +++- src/workers/mod.rs | 148 +++++++++++++---- 22 files changed, 1393 insertions(+), 470 deletions(-) diff --git a/src/db/admin.rs b/src/db/admin.rs index 4abe935..82e2199 100644 --- a/src/db/admin.rs +++ b/src/db/admin.rs @@ -126,10 +126,18 @@ pub fn purge_expired_sessions(conn: &rusqlite::Connection) -> Result { pub fn is_banned(conn: &rusqlite::Connection, ip_hash: &str) -> Result> { let now = chrono::Utc::now().timestamp(); // A ban with NULL expires_at is permanent. + // + // FIX[BAN-ORDER]: Previously there was no ORDER BY, so LIMIT 1 returned + // whichever row the query planner happened to visit first via the index. + // With multiple active bans (e.g. a timed ban and a permanent ban) the + // reason shown to the user was non-deterministic across restarts/VACUUM. + // We now order by expires_at DESC NULLS FIRST so a permanent ban (NULL) + // always surfaces first, and among timed bans the latest-expiring one wins. let result: Option> = conn .query_row( "SELECT reason FROM bans WHERE ip_hash = ?1 AND (expires_at IS NULL OR expires_at > ?2) + ORDER BY expires_at DESC NULLS FIRST LIMIT 1", params![ip_hash, now], |r| r.get(0), @@ -393,13 +401,18 @@ pub fn dismiss_ban_appeal(conn: &rusqlite::Connection, appeal_id: i64) -> Result } /// Dismiss appeal AND lift the ban for this ip_hash. +/// +/// FIX[AUDIT]: Previously both dismiss_ban_appeal and accept_ban_appeal set +/// status='dismissed', making them indistinguishable in the moderation history. +/// Accepted appeals now correctly set status='accepted' so the audit trail +/// accurately reflects whether an appeal was denied or granted. pub fn accept_ban_appeal(conn: &rusqlite::Connection, appeal_id: i64, ip_hash: &str) -> Result<()> { // Both updates must succeed together. let tx = conn .unchecked_transaction() .context("Failed to begin accept-appeal transaction")?; tx.execute( - "UPDATE ban_appeals SET status='dismissed' WHERE id=?1", + "UPDATE ban_appeals SET status='accepted' WHERE id=?1", params![appeal_id], )?; tx.execute("DELETE FROM bans WHERE ip_hash=?1", params![ip_hash])?; diff --git a/src/db/mod.rs b/src/db/mod.rs index 8db503c..2e22d47 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -328,8 +328,21 @@ fn create_schema(conn: &rusqlite::Connection) -> Result<()> { conn.query_row("SELECT version FROM schema_version", [], |r| r.get(0))?; // Each entry is (introduced_at_version, sql). - // SQLite returns SQLITE_ERROR (code 1) for "duplicate column" — we allow - // that specific error so re-running against an already-upgraded DB is safe. + // ALTER TABLE … ADD COLUMN returns SQLITE_ERROR (code 1) with the message + // "duplicate column name: X" when the column already exists — this happens + // when the binary is restarted against a DB that was already migrated. + // CREATE INDEX … IF NOT EXISTS is already idempotent and never errors. + // + // FIX[MIGRATION]: The previous guard caught ALL ErrorCode::Unknown errors, + // which maps to the generic SQLITE_ERROR (code 1). That code is also + // returned for SQL syntax errors, wrong number of columns, etc. A typo + // in migration SQL (e.g. "ADD COULMN") would be silently swallowed, + // marked as applied in schema_version, and the column would never exist. + // + // We now additionally inspect the error message string to confirm the + // error is specifically "duplicate column name" before treating it as + // idempotent. Any other SQLITE_ERROR is propagated so the operator sees + // it immediately rather than discovering a missing column at runtime. let migrations: &[(i64, &str)] = &[ (1, "ALTER TABLE boards ADD COLUMN allow_video INTEGER NOT NULL DEFAULT 1"), (2, "ALTER TABLE boards ADD COLUMN allow_tripcodes INTEGER NOT NULL DEFAULT 1"), @@ -371,10 +384,20 @@ fn create_schema(conn: &rusqlite::Connection) -> Result<()> { tracing::debug!("Applied migration v{}", version); highest_applied = version; } - Err(rusqlite::Error::SqliteFailure(ref e, _)) - if e.code == rusqlite::ErrorCode::Unknown => + Err(rusqlite::Error::SqliteFailure(ref e, ref msg)) + if e.code == rusqlite::ErrorCode::Unknown + && msg + .as_deref() + .map(|m| { + m.contains("duplicate column name") || m.contains("already exists") + }) + .unwrap_or(false) => { - // SQLITE_ERROR with "duplicate column name" — idempotent, skip. + // Idempotent: column already added or index already exists. + // Only reached for ALTER TABLE … ADD COLUMN (duplicate column) + // and CREATE INDEX (already exists). All other SQLITE_ERROR + // values (syntax errors, wrong column counts, etc.) are NOT + // caught here and will propagate as real failures. tracing::debug!( "Migration v{} already applied (idempotent), skipping", version diff --git a/src/db/posts.rs b/src/db/posts.rs index 9cd1fb1..d944e29 100644 --- a/src/db/posts.rs +++ b/src/db/posts.rs @@ -209,6 +209,15 @@ pub fn delete_post(conn: &rusqlite::Connection, post_id: i64) -> Result 0` after the UPDATE to confirm a row was +/// actually written before returning success. pub fn edit_post( conn: &rusqlite::Connection, post_id: i64, @@ -247,11 +264,30 @@ pub fn edit_post( edit_window_secs }; - if !verify_deletion_token(conn, post_id, token)? { + let tx = conn + .unchecked_transaction() + .context("Failed to begin edit_post transaction")?; + + // Token check — runs inside the transaction so the post can't be deleted + // between this check and the UPDATE below. + let stored: Option = tx + .query_row( + "SELECT deletion_token FROM posts WHERE id = ?1", + params![post_id], + |r| r.get(0), + ) + .optional()?; + + let token_ok = stored + .map(|s| constant_time_eq(s.as_bytes(), token.as_bytes())) + .unwrap_or(false); + + if !token_ok { + tx.rollback().ok(); return Ok(false); } - let created_at: Option = conn + let created_at: Option = tx .query_row( "SELECT created_at FROM posts WHERE id = ?1", params![post_id], @@ -261,20 +297,34 @@ pub fn edit_post( let created_at = match created_at { Some(t) => t, - None => return Ok(false), + None => { + tx.rollback().ok(); + return Ok(false); + } }; let now = chrono::Utc::now().timestamp(); if now - created_at > window { + tx.rollback().ok(); return Ok(false); } - conn.execute( + tx.execute( "UPDATE posts SET body = ?1, body_html = ?2, edited_at = ?3 WHERE id = ?4", params![new_body, new_body_html, now, post_id], )?; - Ok(true) + // Confirm the row was actually updated (it could have been deleted by a + // concurrent admin action between our SELECT and this UPDATE — both now + // happen under the same transaction, but in DEFERRED mode a concurrent + // writer could have slipped in; IMMEDIATE below prevents this, but we + // check changes() as a belt-and-suspenders guard regardless). + let updated = tx.changes() > 0; + + tx.commit() + .context("Failed to commit edit_post transaction")?; + + Ok(updated) } /// Constant-time byte slice comparison to prevent timing side-channel attacks. @@ -428,6 +478,7 @@ pub fn get_poll_for_thread( COUNT(pv.id) as vote_count FROM poll_options po LEFT JOIN poll_votes pv ON pv.option_id = po.id + AND pv.poll_id = po.poll_id WHERE po.poll_id = ?1 GROUP BY po.id ORDER BY po.position ASC", @@ -466,6 +517,18 @@ pub fn get_poll_for_thread( } /// Cast a vote. Returns true if vote was recorded, false if already voted. +/// +/// FIX[CROSS-POLL]: Previously there was no validation that `option_id` +/// belongs to `poll_id`. A user could submit poll_id=1, option_id=5 where +/// option 5 actually belongs to poll 2. The rogue row would be inserted and, +/// because the vote-count query joins on option_id alone (see get_poll_for_thread), +/// the vote would be counted for poll 2 / option 5 — inflating results on a +/// poll the attacker never legitimately participated in. +/// +/// We now verify the option belongs to the poll inside the same INSERT +/// statement using a SELECT subquery. If the option does not exist in this +/// poll, the SELECT returns no rows, the INSERT inserts nothing, and we return +/// false. This is a single atomic operation with no TOCTOU gap. pub fn cast_vote( conn: &rusqlite::Connection, poll_id: i64, @@ -474,7 +537,11 @@ pub fn cast_vote( ) -> Result { let result = conn.execute( "INSERT OR IGNORE INTO poll_votes (poll_id, option_id, ip_hash) - VALUES (?1, ?2, ?3)", + SELECT ?1, ?2, ?3 + WHERE EXISTS ( + SELECT 1 FROM poll_options + WHERE id = ?2 AND poll_id = ?1 + )", params![poll_id, option_id, ip_hash], )?; Ok(result > 0) diff --git a/src/db/threads.rs b/src/db/threads.rs index 9b0a253..8728643 100644 --- a/src/db/threads.rs +++ b/src/db/threads.rs @@ -9,7 +9,7 @@ // prune_old_threads → super::paths_safe_to_delete (file safety) use crate::models::*; -use anyhow::Result; +use anyhow::{Context, Result}; use rusqlite::{params, OptionalExtension}; // ─── Board-index thread listing ─────────────────────────────────────────────── @@ -234,6 +234,11 @@ pub fn delete_thread(conn: &rusqlite::Connection, thread_id: i64) -> Result Result { let ids: Vec = { let mut stmt = conn.prepare( @@ -247,12 +252,20 @@ pub fn archive_old_threads(conn: &rusqlite::Connection, board_id: i64, max: i64) ids }; let count = ids.len(); + if count == 0 { + return Ok(0); + } + let tx = conn + .unchecked_transaction() + .context("Failed to begin archive_old_threads transaction")?; for id in ids { - conn.execute( + tx.execute( "UPDATE threads SET archived = 1, locked = 1 WHERE id = ?1", params![id], )?; } + tx.commit() + .context("Failed to commit archive_old_threads transaction")?; Ok(count) } @@ -262,6 +275,14 @@ pub fn archive_old_threads(conn: &rusqlite::Connection, board_id: i64, max: i64) /// Returns the on-disk paths that are now safe to delete (i.e. no longer /// referenced by any remaining post after the prune). The caller is responsible /// for actually removing these files from disk. +/// +/// FIX[PRUNE-TXN]: Previously each per-thread DELETE was auto-committed +/// individually. A crash mid-loop left some threads deleted from the DB but +/// their file paths never returned to the caller, producing unreachable orphaned +/// files on disk. All deletes are now wrapped in a single transaction. +/// +/// FIX[PREPARE-LOOP]: The inner prepare() was called once per loop iteration, +/// recompiling the same statement N times. Replaced with prepare_cached(). pub fn prune_old_threads( conn: &rusqlite::Connection, board_id: i64, @@ -279,13 +300,23 @@ pub fn prune_old_threads( ids }; + if ids.is_empty() { + return Ok(Vec::new()); + } + + let tx = conn + .unchecked_transaction() + .context("Failed to begin prune_old_threads transaction")?; + let mut candidates: Vec = Vec::new(); for id in &ids { - let mut stmt = conn.prepare( + let mut stmt = tx.prepare_cached( "SELECT file_path, thumb_path, audio_file_path FROM posts WHERE thread_id = ?1", )?; let rows: Vec<(Option, Option, Option)> = stmt - .query_map(params![id], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)))? + .query_map(params![id], |r: &rusqlite::Row| { + Ok((r.get(0)?, r.get(1)?, r.get(2)?)) + })? .collect::>()?; for (f, t, a) in rows { if let Some(p) = f { @@ -298,9 +329,12 @@ pub fn prune_old_threads( candidates.push(p); } } - conn.execute("DELETE FROM threads WHERE id = ?1", params![id])?; + tx.execute("DELETE FROM threads WHERE id = ?1", params![id])?; } + tx.commit() + .context("Failed to commit prune_old_threads transaction")?; + Ok(super::paths_safe_to_delete(conn, candidates)) } diff --git a/src/detect.rs b/src/detect.rs index 08a02c2..ce7ba09 100644 --- a/src/detect.rs +++ b/src/detect.rs @@ -34,6 +34,7 @@ use std::path::Path; use std::process::{Command, Stdio}; +use std::sync::{Arc, Mutex, OnceLock}; /// Result of probing for a tool at startup. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -42,6 +43,30 @@ pub enum ToolStatus { Missing, } +// ─── Global Tor child handle (fix #5: orphan-process cleanup) ───────────────── +// +// Stored here so that: +// • `kill_tor()` can be called from a graceful-shutdown handler, and +// • the panic hook registered inside `detect_tor` can kill the child on panic. +// +// Callers should invoke `kill_tor()` during their own shutdown path (e.g. after +// the HTTP server loop exits). SIGKILL / abrupt process death is handled by the +// OS on most platforms once the parent exits, but calling `kill_tor()` ensures +// clean shutdown even for SIGTERM / normal `std::process::exit` paths. +static TOR_CHILD: OnceLock>> = OnceLock::new(); + +/// Kill the background Tor process that `detect_tor` launched, if any. +/// +/// Safe to call multiple times; subsequent calls are no-ops. +pub fn kill_tor() { + if let Some(child) = TOR_CHILD.get() { + if let Ok(mut c) = child.lock() { + let _ = c.kill(); + let _ = c.wait(); + } + } +} + // ─── ffmpeg ─────────────────────────────────────────────────────────────────── pub fn detect_ffmpeg(require_ffmpeg: bool) -> ToolStatus { @@ -79,9 +104,17 @@ pub fn detect_ffmpeg(require_ffmpeg: bool) -> ToolStatus { /// Launches `tor -f ` as a background process then polls for the /// hostname file in a background OS thread. Returns immediately — the /// HTTP server is never blocked. -pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { +/// +/// Returns `ToolStatus::Available` once Tor has been successfully spawned, +/// or `ToolStatus::Missing` if Tor could not be found or started. +/// +/// Call `kill_tor()` during graceful shutdown to avoid orphaned processes. +// +// Fix #7: function now returns ToolStatus so callers can branch on whether +// Tor is actually running. +pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) -> ToolStatus { if !enable_tor_support { - return; + return ToolStatus::Missing; } // ── 1. Find the tor binary ──────────────────────────────────────────────── @@ -105,7 +138,7 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { let Some(tor_bin) = tor_bin else { print_install_instructions(bind_port); - return; + return ToolStatus::Missing; }; println!("[INFO] Tor binary: {}", tor_bin); @@ -122,7 +155,7 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { e ); print_torrc_hint(&hs_dir, bind_port); - return; + return ToolStatus::Missing; } } @@ -142,14 +175,22 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { } // ── 3. Write torrc ──────────────────────────────────────────────────────── - let torrc_path = data_dir.join("torrc"); - + // + // Fix #6: torrc_path is now derived from the canonicalized data_dir so it + // is always an absolute path, regardless of how RustChan was + // invoked. Tor is started with an absolute path argument, which + // means it works even when Tor's working directory differs from ours. + // // Canonical absolute paths avoid problems when Tor's working directory - // differs from ours. + // differs from ours. canonicalize() is called after the directories exist. let canon = |p: &Path| p.canonicalize().unwrap_or_else(|_| p.to_path_buf()); let hs_abs = canon(&hs_dir); let data_abs = canon(&data_subdir); + let torrc_path = canon(data_dir).join("torrc"); // fix #6: absolute torrc path + // Fix #2: paths are quoted so that a data_dir containing spaces does not + // produce a syntactically invalid torrc (Tor treats unquoted + // whitespace as a value delimiter). let torrc = format!( "# RustChan — auto-generated torrc (do not edit while Tor is running)\n\ \n\ @@ -157,10 +198,10 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { ## SocksPort 0 → do not bind a SOCKS port; avoids conflict with port 9050.\n\ ## DataDirectory → separate lock file + state from the system Tor.\n\ SocksPort 0\n\ - DataDirectory {data}\n\ + DataDirectory \"{data}\"\n\ \n\ ## Hidden service — forwards .onion:80 to the local RustChan port.\n\ - HiddenServiceDir {hs}\n\ + HiddenServiceDir \"{hs}\"\n\ HiddenServicePort 80 127.0.0.1:{port}\n", data = data_abs.display(), hs = hs_abs.display(), @@ -174,7 +215,7 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { e ); print_torrc_hint(&hs_dir, bind_port); - return; + return ToolStatus::Missing; } println!("[INFO] Tor: torrc → {}", torrc_path.display()); @@ -182,26 +223,60 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { println!("[INFO] Tor: data dir → {}", data_abs.display()); // ── 4. Spawn Tor ────────────────────────────────────────────────────────── - // Pipe stderr so we can capture it if the process dies quickly. + // Stderr is piped so we can surface diagnostics if Tor exits early. let child = Command::new(tor_bin) .arg("-f") .arg(&torrc_path) .stdout(Stdio::null()) - .stderr(Stdio::piped()) // captured for diagnostics + .stderr(Stdio::piped()) .spawn(); let mut child = match child { Err(e) => { println!("[WARN] Tor: failed to start '{}': {}", tor_bin, e); print_torrc_hint(&hs_dir, bind_port); - return; + return ToolStatus::Missing; } Ok(c) => c, }; + // Fix #8: Drain stderr in a dedicated thread to prevent the pipe buffer + // from filling up and blocking Tor. Without this, a verbose Tor + // (e.g. LogLevel debug) would stall once the OS pipe buffer (~64 KiB) + // fills, causing try_wait() to never see an exit and the 120-second + // poll timeout to fire instead. + let stderr_lines: Arc>> = Arc::new(Mutex::new(Vec::new())); + if let Some(pipe) = child.stderr.take() { + let buf = Arc::clone(&stderr_lines); + std::thread::spawn(move || { + use std::io::{BufRead, BufReader}; + // Cap at 500 lines to bound memory; Tor can be very verbose at + // higher log levels. + for line in BufReader::new(pipe).lines().map_while(Result::ok).take(500) { + buf.lock().expect("stderr buffer mutex poisoned").push(line); + } + }); + } + + // Fix #5: Wrap child in Arc> so it can be shared between: + // • the background monitoring thread (which may call try_wait / kill) + // • the global TOR_CHILD handle (for kill_tor() / panic hook) + let child = Arc::new(Mutex::new(child)); + + // Store globally so kill_tor() works from any context. + let _ = TOR_CHILD.set(Arc::clone(&child)); + + // Best-effort cleanup on panic: kill Tor before printing the panic message. + let prev_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + kill_tor(); + prev_hook(info); + })); + + let pid = child.lock().expect("child process mutex poisoned").id(); println!( "[INFO] Tor: process started (pid {}). Waiting for .onion address…", - child.id() + pid ); // ── 5. Quick health-check + hostname polling (background thread) ────────── @@ -209,30 +284,29 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { let torrc_display = torrc_path.display().to_string(); let tor_bin_owned = tor_bin.to_string(); + let child_bg = Arc::clone(&child); + let stderr_bg = Arc::clone(&stderr_lines); + std::thread::spawn(move || { // Give Tor ~4 seconds to either establish itself or fail fast. std::thread::sleep(std::time::Duration::from_secs(4)); - match child.try_wait() { + // Fix #4 (early-exit check): use the shared child Arc instead of + // a moved value so the same handle can also be used by + // poll_for_hostname to detect crashes during polling. + match child_bg + .lock() + .expect("child process mutex poisoned") + .try_wait() + { Ok(Some(status)) => { - // Process already exited — grab stderr for the operator. - let stderr_text = child - .stderr - .take() - .map(|mut r| { - use std::io::Read; - let mut buf = String::new(); - let _ = r.read_to_string(&mut buf); - buf - }) - .unwrap_or_default(); - + // Process already exited — surface stderr for the operator. + let lines = stderr_bg.lock().expect("stderr buffer mutex poisoned"); println!(); println!("[ERR ] Tor: process exited early ({})", status); - if !stderr_text.is_empty() { + if !lines.is_empty() { println!("────── Tor stderr ──────────────────────────────"); - // Limit output to the first 20 lines; Tor can be verbose. - for line in stderr_text.lines().take(20) { + for line in lines.iter().take(20) { println!(" {}", line); } println!("────────────────────────────────────────────────"); @@ -250,41 +324,65 @@ pub fn detect_tor(enable_tor_support: bool, bind_port: u16, data_dir: &Path) { } } - poll_for_hostname(&hostname_path, &torrc_display, &tor_bin_owned, bind_port); + poll_for_hostname( + &hostname_path, + &child_bg, + &stderr_bg, + &torrc_display, + &tor_bin_owned, + bind_port, + ); }); + + ToolStatus::Available } // ─── Hostname polling ───────────────────────────────────────────────────────── -fn poll_for_hostname(hostname_path: &Path, torrc_display: &str, tor_bin: &str, bind_port: u16) { +fn poll_for_hostname( + hostname_path: &Path, + // Fix #4: child handle passed in so crashes mid-poll are detected promptly + // instead of waiting for the full 120-second timeout. + child: &Arc>, + stderr_lines: &Arc>>, + torrc_display: &str, + tor_bin: &str, + bind_port: u16, +) { const TIMEOUT_SECS: u64 = 120; // v3 onion keys can take ~60–90 s first run const POLL_MS: u64 = 500; let deadline = std::time::Instant::now() + std::time::Duration::from_secs(TIMEOUT_SECS); loop { + // Fix #4: check for mid-poll crash every iteration. + // try_lock() is used instead of lock() to be non-blocking; + // if the mutex is momentarily held by the stderr-drain thread + // we simply skip the check this iteration. + if let Ok(mut c) = child.try_lock() { + if let Ok(Some(status)) = c.try_wait() { + let lines = stderr_lines.lock().expect("stderr buffer mutex poisoned"); + println!(); + println!("[ERR ] Tor: process crashed during startup ({})", status); + if !lines.is_empty() { + println!("────── Tor stderr ──────────────────────────────"); + for line in lines.iter().take(20) { + println!(" {}", line); + } + println!("────────────────────────────────────────────────"); + } + println!(); + print_diagnosis_hints(torrc_display, tor_bin, bind_port); + return; + } + } + if hostname_path.exists() { match std::fs::read_to_string(hostname_path) { Ok(raw) => { let onion = raw.trim(); if !onion.is_empty() { - // ── success banner ──────────────────────────────── - let addr_line = format!("http://{}", onion); - println!(); - println!("╔══════════════════════════════════════════════════════╗"); - println!("║ TOR ONION SERVICE ACTIVE ✓ ║"); - println!("╠══════════════════════════════════════════════════════╣"); - println!("║ {:<52}║", addr_line); - println!("║ ║"); - println!("║ Share this with Tor Browser users. ║"); - println!("║ Your private key is stored at: ║"); - println!( - "║ {:<48}║", - hostname_path.parent().unwrap_or(hostname_path).display() - ); - println!("║ Back it up — losing it means a new .onion address. ║"); - println!("╚══════════════════════════════════════════════════════╝"); - println!(); + print_onion_banner(onion, hostname_path); return; } // Empty file — Tor is still writing; retry. @@ -311,6 +409,36 @@ fn poll_for_hostname(hostname_path: &Path, torrc_display: &str, tor_bin: &str, b } } +// ─── Success banner ─────────────────────────────────────────────────────────── + +/// Print the .onion address in a bordered banner. +/// +/// Fix #1: The box is wide enough (inner width = 72) to hold a v3 .onion URL +/// (`http://` + 56-char address + `.onion` = 69 chars) without overflowing. +/// +/// Fix #3: The private-key-path line used `{:<48}` with a 4-char indent inside +/// the old 54-char-wide box, producing a line 2 characters short of the +/// box edge. Both field widths have been corrected for the new box size. +fn print_onion_banner(onion: &str, hostname_path: &Path) { + // v3 .onion URL: "http://" (7) + 56-char base32 address + ".onion" (6) = 69 chars. + // Box inner width 72 gives 3 chars of right-margin after the longest URL. + let addr_line = format!("http://{}", onion); + let key_dir = hostname_path.parent().unwrap_or(hostname_path); + + println!(); + println!("╔════════════════════════════════════════════════════════════════════════╗"); + println!("║ TOR ONION SERVICE ACTIVE ✓ ║"); + println!("╠════════════════════════════════════════════════════════════════════════╣"); + println!("║ {:<70}║", addr_line); // fix #1: {:<70}, 2+70+1 = inner 72 + println!("║ ║"); + println!("║ Share this with Tor Browser users. ║"); + println!("║ Your private key is stored at: ║"); + println!("║ {:<68}║", key_dir.display()); // fix #3: {:<68}, 4+68+1 = inner 72 + println!("║ Back it up — losing it means a new .onion address. ║"); + println!("╚════════════════════════════════════════════════════════════════════════╝"); + println!(); +} + // ─── Diagnostic helpers ─────────────────────────────────────────────────────── /// Printed when we know Tor is not installed. diff --git a/src/error.rs b/src/error.rs index d5ad4cd..eebb5c0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -34,6 +34,11 @@ pub enum AppError { #[error("Forbidden: {0}")] Forbidden(String), + /// 403 — user is banned; carries the ban reason and their CSRF token so the + /// appeal form can be rendered with a valid token (fixes FIX[M-T1]). + #[error("You are banned. Reason: {reason}")] + BannedUser { reason: String, csrf_token: String }, + /// 413 — upload body too large #[error("Upload too large: {0}")] UploadTooLarge(String), @@ -83,6 +88,10 @@ impl IntoResponse for AppError { AppError::NotFound(msg) => (StatusCode::NOT_FOUND, msg.clone()), AppError::BadRequest(msg) => (StatusCode::BAD_REQUEST, msg.clone()), AppError::Forbidden(msg) => (StatusCode::FORBIDDEN, msg.clone()), + AppError::BannedUser { reason, csrf_token } => { + let html = crate::templates::ban_page(reason, csrf_token); + return (StatusCode::FORBIDDEN, Html(html)).into_response(); + } AppError::UploadTooLarge(msg) => (StatusCode::PAYLOAD_TOO_LARGE, msg.clone()), AppError::InvalidMediaType(msg) => (StatusCode::UNSUPPORTED_MEDIA_TYPE, msg.clone()), AppError::RateLimited => ( @@ -102,15 +111,6 @@ impl IntoResponse for AppError { } }; - // Render a richer ban page with an appeal form instead of the generic error page - if status == StatusCode::FORBIDDEN && message.starts_with("You are banned") { - let reason = message - .strip_prefix("You are banned. Reason: ") - .unwrap_or(&message); - let html = crate::templates::ban_page(reason); - return (status, Html(html)).into_response(); - } - let html = crate::templates::error_page(status.as_u16(), &message); (status, Html(html)).into_response() } diff --git a/src/handlers/admin.rs b/src/handlers/admin.rs index 42ae8b9..8d8aa29 100644 --- a/src/handlers/admin.rs +++ b/src/handlers/admin.rs @@ -1039,11 +1039,19 @@ pub async fn admin_ban_and_delete( .await .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))??; + // FIX[A1]: form.board is user-supplied; sanitise to alphanumeric only before + // embedding in the redirect URL to prevent open-redirect via "//" prefixes. + let safe_board: String = form + .board + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .take(8) + .collect(); // If OP was deleted, the thread is gone — send to board index let redirect = if is_op { - format!("/{}", form.board) + format!("/{}", safe_board) } else { - format!("/{}/thread/{}#p{}", form.board, thread_id, post_id) + format!("/{}/thread/{}#p{}", safe_board, thread_id, post_id) }; Ok(Redirect::to(&redirect).into_response()) } @@ -1522,8 +1530,10 @@ pub async fn admin_restore( ) -> Result { let session_id = jar.get(SESSION_COOKIE).map(|c| c.value().to_string()); - // Collect multipart fields (the stream can only be consumed once). - let mut zip_data: Option> = None; + // FIX[A7]: Stream the uploaded zip to a NamedTempFile on disk instead of + // buffering the entire upload into a Vec. Full-site backups can be + // several GiB; loading them entirely into the heap exhausts available memory. + let mut zip_tmp: Option = None; let mut form_csrf: Option = None; while let Some(field) = multipart @@ -1541,13 +1551,38 @@ pub async fn admin_restore( ); } Some("backup_file") => { - let bytes = field - .bytes() + use tokio::io::AsyncWriteExt as _; + let tmp = tempfile::NamedTempFile::new() + .map_err(|e| AppError::Internal(anyhow::anyhow!("Tempfile: {}", e)))?; + // Clone the underlying fd for async writing; the original + // NamedTempFile retains ownership and the delete-on-drop guard. + let std_clone = tmp + .as_file() + .try_clone() + .map_err(|e| AppError::Internal(anyhow::anyhow!("Clone fd: {}", e)))?; + let async_file = tokio::fs::File::from_std(std_clone); + let mut writer = tokio::io::BufWriter::new(async_file); + let mut field = field; + while let Some(chunk) = field + .chunk() + .await + .map_err(|e| AppError::BadRequest(e.to_string()))? + { + writer + .write_all(&chunk) + .await + .map_err(|e| AppError::Internal(anyhow::anyhow!("Write chunk: {}", e)))?; + } + writer + .flush() .await - .map_err(|e| AppError::BadRequest(e.to_string()))?; - zip_data = Some(bytes.to_vec()); + .map_err(|e| AppError::Internal(anyhow::anyhow!("Flush: {}", e)))?; + zip_tmp = Some(tmp); + } + _ => { + // Drain unknown fields so the multipart stream advances. + let _ = field.bytes().await; } - _ => {} } } @@ -1558,9 +1593,13 @@ pub async fn admin_restore( return Err(AppError::Forbidden("CSRF token mismatch.".into())); } - let zip_bytes = - zip_data.ok_or_else(|| AppError::BadRequest("No backup file uploaded.".into()))?; - if zip_bytes.is_empty() { + let zip_tmp = zip_tmp.ok_or_else(|| AppError::BadRequest("No backup file uploaded.".into()))?; + // Determine size without reading into RAM: seeking to end gives the byte count. + let zip_size = zip_tmp + .as_file() + .seek(std::io::SeekFrom::End(0)) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Seek check: {}", e)))?; + if zip_size == 0 { return Err(AppError::BadRequest( "Uploaded backup file is empty.".into(), )); @@ -1579,9 +1618,13 @@ pub async fn admin_restore( // in the restored DB once the backup completes. let admin_id = require_admin_session_sid(&live_conn, session_id.as_deref())?; - // ── Parse the zip ───────────────────────────────────────────── - let cursor = std::io::Cursor::new(zip_bytes); - let mut archive = zip::ZipArchive::new(cursor) + // ── Open the on-disk zip (FIX[A7]) ─────────────────────────── + // reopen() gives a fresh File descriptor seeked to position 0, + // so ZipArchive can navigate entries without loading into RAM. + let zip_file = zip_tmp + .reopen() + .map_err(|e| AppError::Internal(anyhow::anyhow!("Reopen zip: {}", e)))?; + let mut archive = zip::ZipArchive::new(std::io::BufReader::new(zip_file)) .map_err(|e| AppError::BadRequest(format!("Invalid zip: {}", e)))?; // Quick pre-flight: make sure there is a chan.db entry. @@ -1700,6 +1743,9 @@ pub async fn admin_restore( new_cookie.set_same_site(SameSite::Strict); new_cookie.set_path("/"); new_cookie.set_secure(CONFIG.https_cookies); + // FIX[A5]: Set Max-Age so the browser expires the cookie after the configured + // session lifetime — matching the behaviour of the normal login handler. + new_cookie.set_max_age(time::Duration::seconds(CONFIG.session_duration)); Ok((jar.add(new_cookie), Redirect::to("/admin/panel?restored=1")).into_response()) } @@ -2488,17 +2534,22 @@ pub async fn restore_saved_full_backup( } let path = full_backup_dir().join(&safe_filename); - let zip_bytes = - std::fs::read(&path).map_err(|_| AppError::NotFound("Backup file not found.".into()))?; - + // FIX[A3]: Do NOT read the file in the async context before auth is verified. + // std::fs::read() blocks the Tokio runtime and an unauthenticated caller could + // force the server to read gigabytes off disk before being rejected. The read + // is deferred into spawn_blocking where it runs only after the session check. let upload_dir = CONFIG.upload_dir.clone(); let fresh_sid: String = tokio::task::spawn_blocking({ let pool = state.db.clone(); move || -> Result { let mut live_conn = pool.get()?; + // Auth check first — only read the (potentially huge) file if valid. let admin_id = require_admin_session_sid(&live_conn, session_id.as_deref())?; + let zip_bytes = std::fs::read(&path) + .map_err(|_| AppError::NotFound("Backup file not found.".into()))?; + let cursor = std::io::Cursor::new(zip_bytes); let mut archive = zip::ZipArchive::new(cursor) .map_err(|e| AppError::BadRequest(format!("Invalid zip: {}", e)))?; @@ -2608,6 +2659,8 @@ pub async fn restore_saved_full_backup( new_cookie.set_same_site(SameSite::Strict); new_cookie.set_path("/"); new_cookie.set_secure(CONFIG.https_cookies); + // FIX[A5]: Set Max-Age to match normal login behaviour. + new_cookie.set_max_age(time::Duration::seconds(CONFIG.session_duration)); Ok((jar.add(new_cookie), Redirect::to("/admin/panel?restored=1")).into_response()) } @@ -2635,8 +2688,8 @@ pub async fn restore_saved_board_backup( } let path = board_backup_dir().join(&safe_filename); - let zip_bytes = - std::fs::read(&path).map_err(|_| AppError::NotFound("Backup file not found.".into()))?; + // FIX[A4]: Defer the blocking file read until after auth is verified inside + // spawn_blocking — mirrors the fix applied to restore_saved_full_backup (A3). let upload_dir = CONFIG.upload_dir.clone(); let board_short_result: Result> = tokio::task::spawn_blocking({ @@ -2647,8 +2700,12 @@ pub async fn restore_saved_board_backup( use std::collections::HashMap; let conn = pool.get()?; + // Auth check first — only read the file if the session is valid. require_admin_session_sid(&conn, session_id.as_deref())?; + let zip_bytes = std::fs::read(&path) + .map_err(|_| AppError::NotFound("Backup file not found.".into()))?; + let cursor = std::io::Cursor::new(zip_bytes); let mut archive = zip::ZipArchive::new(cursor) .map_err(|e| AppError::BadRequest(format!("Invalid zip: {}", e)))?; @@ -2680,72 +2737,77 @@ pub async fn restore_saved_board_backup( ) .ok(); - let live_board_id: i64 = if let Some(eid) = existing_id { - conn.execute("DELETE FROM threads WHERE board_id = ?1", params![eid]) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Clear threads: {}", e)))?; - conn.execute( - "UPDATE boards SET name=?1, description=?2, nsfw=?3, - max_threads=?4, bump_limit=?5, - allow_images=?6, allow_video=?7, allow_audio=?8, allow_tripcodes=?9, - edit_window_secs=?10, allow_editing=?11, allow_archive=?12, - allow_video_embeds=?13, allow_captcha=?14, post_cooldown_secs=?15 - WHERE id=?16", - params![ - manifest.board.name, - manifest.board.description, - manifest.board.nsfw as i64, - manifest.board.max_threads, - manifest.board.bump_limit, - manifest.board.allow_images as i64, - manifest.board.allow_video as i64, - manifest.board.allow_audio as i64, - manifest.board.allow_tripcodes as i64, - manifest.board.edit_window_secs, - manifest.board.allow_editing as i64, - manifest.board.allow_archive as i64, - manifest.board.allow_video_embeds as i64, - manifest.board.allow_captcha as i64, - manifest.board.post_cooldown_secs, - eid, - ], - ) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Update board: {}", e)))?; - eid - } else { - conn.execute( - "INSERT INTO boards (short_name, name, description, nsfw, max_threads, - bump_limit, allow_images, allow_video, allow_audio, allow_tripcodes, - edit_window_secs, allow_editing, allow_archive, allow_video_embeds, allow_captcha, - post_cooldown_secs, created_at) - VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17)", - params![ - manifest.board.short_name, - manifest.board.name, - manifest.board.description, - manifest.board.nsfw as i64, - manifest.board.max_threads, - manifest.board.bump_limit, - manifest.board.allow_images as i64, - manifest.board.allow_video as i64, - manifest.board.allow_audio as i64, - manifest.board.allow_tripcodes as i64, - manifest.board.edit_window_secs, - manifest.board.allow_editing as i64, - manifest.board.allow_archive as i64, - manifest.board.allow_video_embeds as i64, - manifest.board.allow_captcha as i64, - manifest.board.post_cooldown_secs, - manifest.board.created_at, - ], - ) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Insert board: {}", e)))?; - conn.last_insert_rowid() - }; - + // FIX[A6]: BEGIN IMMEDIATE must cover the DELETE + UPDATE/INSERT of the + // board row as well as the thread/post/poll inserts. Previously those + // DDL statements ran outside any transaction, so a crash between the + // DELETE and the first INSERT left the board with zero threads and no way + // to recover without manual intervention. conn.execute("BEGIN IMMEDIATE", []) .map_err(|e| AppError::Internal(anyhow::anyhow!("Begin tx: {}", e)))?; let restore_result = (|| -> Result<()> { + let live_board_id: i64 = if let Some(eid) = existing_id { + conn.execute("DELETE FROM threads WHERE board_id = ?1", params![eid]) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Clear threads: {}", e)))?; + conn.execute( + "UPDATE boards SET name=?1, description=?2, nsfw=?3, + max_threads=?4, bump_limit=?5, + allow_images=?6, allow_video=?7, allow_audio=?8, allow_tripcodes=?9, + edit_window_secs=?10, allow_editing=?11, allow_archive=?12, + allow_video_embeds=?13, allow_captcha=?14, post_cooldown_secs=?15 + WHERE id=?16", + params![ + manifest.board.name, + manifest.board.description, + manifest.board.nsfw as i64, + manifest.board.max_threads, + manifest.board.bump_limit, + manifest.board.allow_images as i64, + manifest.board.allow_video as i64, + manifest.board.allow_audio as i64, + manifest.board.allow_tripcodes as i64, + manifest.board.edit_window_secs, + manifest.board.allow_editing as i64, + manifest.board.allow_archive as i64, + manifest.board.allow_video_embeds as i64, + manifest.board.allow_captcha as i64, + manifest.board.post_cooldown_secs, + eid, + ], + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Update board: {}", e)))?; + eid + } else { + conn.execute( + "INSERT INTO boards (short_name, name, description, nsfw, max_threads, + bump_limit, allow_images, allow_video, allow_audio, allow_tripcodes, + edit_window_secs, allow_editing, allow_archive, allow_video_embeds, allow_captcha, + post_cooldown_secs, created_at) + VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17)", + params![ + manifest.board.short_name, + manifest.board.name, + manifest.board.description, + manifest.board.nsfw as i64, + manifest.board.max_threads, + manifest.board.bump_limit, + manifest.board.allow_images as i64, + manifest.board.allow_video as i64, + manifest.board.allow_audio as i64, + manifest.board.allow_tripcodes as i64, + manifest.board.edit_window_secs, + manifest.board.allow_editing as i64, + manifest.board.allow_archive as i64, + manifest.board.allow_video_embeds as i64, + manifest.board.allow_captcha as i64, + manifest.board.post_cooldown_secs, + manifest.board.created_at, + ], + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Insert board: {}", e)))?; + conn.last_insert_rowid() + }; + let mut thread_id_map: HashMap = HashMap::new(); for t in &manifest.threads { conn.execute( @@ -3466,72 +3528,75 @@ pub async fn board_restore( ) .ok(); - let live_board_id: i64 = if let Some(eid) = existing_id { - conn.execute("DELETE FROM threads WHERE board_id = ?1", params![eid]) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Clear threads: {}", e)))?; - conn.execute( - "UPDATE boards SET name=?1, description=?2, nsfw=?3, - max_threads=?4, bump_limit=?5, - allow_images=?6, allow_video=?7, allow_audio=?8, allow_tripcodes=?9, - edit_window_secs=?10, allow_editing=?11, allow_archive=?12, - allow_video_embeds=?13, allow_captcha=?14, post_cooldown_secs=?15 - WHERE id=?16", - params![ - manifest.board.name, - manifest.board.description, - manifest.board.nsfw as i64, - manifest.board.max_threads, - manifest.board.bump_limit, - manifest.board.allow_images as i64, - manifest.board.allow_video as i64, - manifest.board.allow_audio as i64, - manifest.board.allow_tripcodes as i64, - manifest.board.edit_window_secs, - manifest.board.allow_editing as i64, - manifest.board.allow_archive as i64, - manifest.board.allow_video_embeds as i64, - manifest.board.allow_captcha as i64, - manifest.board.post_cooldown_secs, - eid, - ], - ) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Update board: {}", e)))?; - eid - } else { - conn.execute( - "INSERT INTO boards (short_name, name, description, nsfw, max_threads, - bump_limit, allow_images, allow_video, allow_audio, allow_tripcodes, - edit_window_secs, allow_editing, allow_archive, allow_video_embeds, - allow_captcha, post_cooldown_secs, created_at) - VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17)", - params![ - manifest.board.short_name, - manifest.board.name, - manifest.board.description, - manifest.board.nsfw as i64, - manifest.board.max_threads, - manifest.board.bump_limit, - manifest.board.allow_images as i64, - manifest.board.allow_video as i64, - manifest.board.allow_audio as i64, - manifest.board.allow_tripcodes as i64, - manifest.board.edit_window_secs, - manifest.board.allow_editing as i64, - manifest.board.allow_archive as i64, - manifest.board.allow_video_embeds as i64, - manifest.board.allow_captcha as i64, - manifest.board.post_cooldown_secs, - manifest.board.created_at, - ], - ) - .map_err(|e| AppError::Internal(anyhow::anyhow!("Insert board: {}", e)))?; - conn.last_insert_rowid() - }; + // FIX[A6]: BEGIN IMMEDIATE must cover the DELETE + UPDATE/INSERT of the + // board row. Previously those statements ran outside any transaction. conn.execute("BEGIN IMMEDIATE", []) .map_err(|e| AppError::Internal(anyhow::anyhow!("Begin tx: {}", e)))?; let restore_result = (|| -> Result<()> { + let live_board_id: i64 = if let Some(eid) = existing_id { + conn.execute("DELETE FROM threads WHERE board_id = ?1", params![eid]) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Clear threads: {}", e)))?; + conn.execute( + "UPDATE boards SET name=?1, description=?2, nsfw=?3, + max_threads=?4, bump_limit=?5, + allow_images=?6, allow_video=?7, allow_audio=?8, allow_tripcodes=?9, + edit_window_secs=?10, allow_editing=?11, allow_archive=?12, + allow_video_embeds=?13, allow_captcha=?14, post_cooldown_secs=?15 + WHERE id=?16", + params![ + manifest.board.name, + manifest.board.description, + manifest.board.nsfw as i64, + manifest.board.max_threads, + manifest.board.bump_limit, + manifest.board.allow_images as i64, + manifest.board.allow_video as i64, + manifest.board.allow_audio as i64, + manifest.board.allow_tripcodes as i64, + manifest.board.edit_window_secs, + manifest.board.allow_editing as i64, + manifest.board.allow_archive as i64, + manifest.board.allow_video_embeds as i64, + manifest.board.allow_captcha as i64, + manifest.board.post_cooldown_secs, + eid, + ], + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Update board: {}", e)))?; + eid + } else { + conn.execute( + "INSERT INTO boards (short_name, name, description, nsfw, max_threads, + bump_limit, allow_images, allow_video, allow_audio, allow_tripcodes, + edit_window_secs, allow_editing, allow_archive, allow_video_embeds, + allow_captcha, post_cooldown_secs, created_at) + VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17)", + params![ + manifest.board.short_name, + manifest.board.name, + manifest.board.description, + manifest.board.nsfw as i64, + manifest.board.max_threads, + manifest.board.bump_limit, + manifest.board.allow_images as i64, + manifest.board.allow_video as i64, + manifest.board.allow_audio as i64, + manifest.board.allow_tripcodes as i64, + manifest.board.edit_window_secs, + manifest.board.allow_editing as i64, + manifest.board.allow_archive as i64, + manifest.board.allow_video_embeds as i64, + manifest.board.allow_captcha as i64, + manifest.board.post_cooldown_secs, + manifest.board.created_at, + ], + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!("Insert board: {}", e)))?; + conn.last_insert_rowid() + }; + let mut thread_id_map: HashMap = HashMap::new(); for t in &manifest.threads { conn.execute( @@ -3872,10 +3937,10 @@ pub async fn admin_ip_history( let (jar, csrf) = ensure_csrf(jar); let csrf_clone = csrf.clone(); - // Sanitise the IP hash: must be a hex string (SHA-256 = 64 chars). - // Any other value would produce empty results; we reject it explicitly - // to avoid leaking information via crafted paths. - if ip_hash.len() > 64 || !ip_hash.chars().all(|c| c.is_ascii_alphanumeric()) { + // Sanitise the IP hash: must be exactly a SHA-256 hex string (64 hex chars). + // The previous guard used `> 64` which accepted any string of 0–64 chars, + // including an empty string. Require exactly 64. + if ip_hash.len() != 64 || !ip_hash.chars().all(|c| c.is_ascii_alphanumeric()) { return Err(AppError::BadRequest("Invalid IP hash.".into())); } diff --git a/src/handlers/board.rs b/src/handlers/board.rs index 59ab1d7..b159141 100644 --- a/src/handlers/board.rs +++ b/src/handlers/board.rs @@ -198,6 +198,8 @@ pub async fn create_thread( // Extract admin session before spawn_blocking (cookie jar is !Send). let admin_session_id = jar.get("chan_admin_session").map(|c| c.value().to_string()); + // Also extract csrf_token before spawn_blocking so the ban page appeal form works. + let ban_csrf_token = csrf_cookie.clone().unwrap_or_default(); let board_short_err = board_short.clone(); let result = tokio::task::spawn_blocking({ @@ -209,14 +211,14 @@ pub async fn create_thread( .ok_or_else(|| AppError::NotFound(format!("Board /{board_short}/ not found")))?; let ip_hash = hash_ip(&client_ip, &cookie_secret); if let Some(reason) = db::is_banned(&conn, &ip_hash)? { - return Err(AppError::Forbidden(format!( - "You are banned. Reason: {}", - if reason.is_empty() { + return Err(AppError::BannedUser { + reason: if reason.is_empty() { "No reason given".to_string() } else { reason - } - ))); + }, + csrf_token: ban_csrf_token, + }); } // Verify admin session — admins bypass the per-board cooldown entirely. @@ -436,11 +438,14 @@ pub async fn create_thread( .filter(|o| !o.is_empty()) .collect(); if !q.is_empty() && valid_opts.len() >= 2 { - if let Some(secs) = poll_duration { - let secs = secs.clamp(60, 30 * 24 * 3600); // clamp 1 min..30 days - let expires_at = chrono::Utc::now().timestamp() + secs; - db::create_poll(&conn, thread_id, &q, &valid_opts, expires_at)?; - } + let secs = poll_duration.ok_or_else(|| { + AppError::BadRequest( + "A duration is required when creating a poll.".into(), + ) + })?; + let secs = secs.clamp(60, 30 * 24 * 3600); // clamp 1 min..30 days + let expires_at = chrono::Utc::now().timestamp() + secs; + db::create_poll(&conn, thread_id, &q, &valid_opts, expires_at)?; } // ── Background jobs ─────────────────────────────────────────────── @@ -762,7 +767,7 @@ pub async fn file_report( .collect::(); let post_id = form.post_id; - let thread_id = form.thread_id; + let _thread_id = form.thread_id; let board_raw = form .board .chars() @@ -770,9 +775,9 @@ pub async fn file_report( .take(8) .collect::(); - tokio::task::spawn_blocking({ + let db_thread_id = tokio::task::spawn_blocking({ let pool = state.db.clone(); - move || -> Result<()> { + move || -> Result { let conn = pool.get()?; let board = db::get_board_by_short(&conn, &board_raw)? .ok_or_else(|| AppError::NotFound("Board not found.".into()))?; @@ -784,14 +789,23 @@ pub async fn file_report( "Post does not belong to this board.".into(), )); } - db::file_report(&conn, post_id, thread_id, board.id, &reason, &ip_hash)?; - Ok(()) + // Use the DB's thread_id for the redirect — not the user-submitted value. + let authoritative_thread_id = post.thread_id; + db::file_report( + &conn, + post_id, + authoritative_thread_id, + board.id, + &reason, + &ip_hash, + )?; + Ok(authoritative_thread_id) } }) .await .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))??; - // Redirect back to the thread the reported post lives in. + // Redirect back to the thread using the DB-resolved IDs, not the form values. let safe_board = form .board .chars() @@ -800,7 +814,7 @@ pub async fn file_report( .collect::(); Ok(Redirect::to(&format!( "/{}/thread/{}#p{}", - safe_board, form.thread_id, form.post_id + safe_board, db_thread_id, form.post_id )) .into_response()) } diff --git a/src/handlers/mod.rs b/src/handlers/mod.rs index 1a96056..5ed113a 100644 --- a/src/handlers/mod.rs +++ b/src/handlers/mod.rs @@ -77,7 +77,7 @@ pub async fn parse_post_multipart( Some("poll_question") => { let v = field.text().await.unwrap_or_default(); // CRIT-8: Enforce server-side length cap on poll question. - if v.len() > 500 { + if v.chars().count() > 500 { return Err(AppError::BadRequest( "Poll question must be 500 characters or fewer.".into(), )); @@ -94,7 +94,7 @@ pub async fn parse_post_multipart( "Polls are limited to 20 options.".into(), )); } - if trimmed.len() > 200 { + if trimmed.chars().count() > 200 { return Err(AppError::BadRequest( "Each poll option must be 200 characters or fewer.".into(), )); @@ -135,15 +135,28 @@ pub async fn parse_post_multipart( } } - // Convert duration value + unit → seconds (saturating to prevent overflow) + // Convert duration value + unit → seconds (saturating to prevent overflow). + // The unit is validated against an explicit allow-list (case-insensitive) so + // that a tampered form field does not silently multiply by an arbitrary factor. let poll_duration_secs = if !poll_question.trim().is_empty() { - poll_duration_value.map(|v| { - if poll_duration_unit == "minutes" { - v.saturating_mul(60) - } else { - v.saturating_mul(3600) + match poll_duration_value { + None => None, + Some(v) => { + let unit = poll_duration_unit.trim().to_ascii_lowercase(); + let secs = match unit.as_str() { + "minutes" => v.saturating_mul(60), + "hours" => v.saturating_mul(3600), + "days" => v.saturating_mul(86_400), + other => { + return Err(AppError::BadRequest(format!( + "Invalid poll duration unit '{}'. Use 'minutes', 'hours', or 'days'.", + other + ))); + } + }; + Some(secs) } - }) + } } else { None }; @@ -175,9 +188,12 @@ pub async fn parse_post_multipart( /// • anything else → 400 BadRequest pub fn classify_upload_error(e: anyhow::Error) -> AppError { let msg = e.to_string(); - if msg.starts_with("File too large") || msg.starts_with("Insufficient disk space") { + // Compare lower-cased so minor wording changes in save_upload don't silently + // fall through to a generic 400 instead of the correct 413 / 415. + let lower = msg.to_ascii_lowercase(); + if lower.starts_with("file too large") || lower.starts_with("insufficient disk space") { AppError::UploadTooLarge(msg) - } else if msg.starts_with("File type not allowed") || msg.starts_with("Not an audio file") { + } else if lower.starts_with("file type not allowed") || lower.starts_with("not an audio file") { AppError::InvalidMediaType(msg) } else { AppError::BadRequest(msg) diff --git a/src/handlers/thread.rs b/src/handlers/thread.rs index bf513f6..aacaadf 100644 --- a/src/handlers/thread.rs +++ b/src/handlers/thread.rs @@ -123,6 +123,8 @@ pub async fn post_reply( // Extract admin session before spawn_blocking so we can skip the per-board // cooldown for admins (the cookie value is !Send and can't cross the boundary). let admin_session_id = jar.get("chan_admin_session").map(|c| c.value().to_string()); + // Also extract csrf_token before spawn_blocking so the ban page appeal form works. + let ban_csrf_token = csrf_cookie.clone().unwrap_or_default(); let board_short_err = board_short.clone(); let client_ip_err = client_ip.clone(); // CRIT-2: keep for the error re-render path below @@ -147,14 +149,14 @@ pub async fn post_reply( let ip_hash = hash_ip(&client_ip, &cookie_secret); if let Some(reason) = db::is_banned(&conn, &ip_hash)? { - return Err(AppError::Forbidden(format!( - "You are banned. Reason: {}", - if reason.is_empty() { + return Err(AppError::BannedUser { + reason: if reason.is_empty() { "No reason given".to_string() } else { reason - } - ))); + }, + csrf_token: ban_csrf_token, + }); } // Per-board post cooldown — the SOLE post rate control. @@ -503,16 +505,15 @@ pub async fn edit_post_get( )); } - let window = if board.edit_window_secs <= 0 { - 300 - } else { - board.edit_window_secs - }; - let now = chrono::Utc::now().timestamp(); - if now - post.created_at > window { - return Err(AppError::Forbidden( - "The edit window for this post has closed.".into(), - )); + // edit_window_secs = 0 means no time restriction (always editable while + // allow_editing is true). Any positive value is enforced as a hard deadline. + if board.edit_window_secs > 0 { + let now = chrono::Utc::now().timestamp(); + if now - post.created_at > board.edit_window_secs { + return Err(AppError::Forbidden( + "The edit window for this post has closed.".into(), + )); + } } let all_boards = db::get_all_boards(&conn)?; @@ -611,17 +612,19 @@ pub async fn edit_post_post( board.edit_window_secs, )?; - let window = if board.edit_window_secs <= 0 { - 300 - } else { - board.edit_window_secs - }; + // edit_window_secs = 0 means no time restriction. A positive value is + // enforced; we only distinguish "window closed" vs "wrong token" when a + // window is actually configured. if !success { let all_boards = db::get_all_boards(&conn)?; let collapse_greentext = db::get_collapse_greentext(&conn); - let now = chrono::Utc::now().timestamp(); - let err_msg = if now - post.created_at > window { - "The edit window for this post has closed." + let err_msg = if board.edit_window_secs > 0 { + let now = chrono::Utc::now().timestamp(); + if now - post.created_at > board.edit_window_secs { + "The edit window for this post has closed." + } else { + "Incorrect edit token." + } } else { "Incorrect edit token." }; @@ -675,6 +678,11 @@ pub async fn vote_handler( let cookie_secret = CONFIG.cookie_secret.clone(); let option_id = form.option_id; + // Reject non-positive IDs before touching the DB. + if option_id <= 0 { + return Err(AppError::BadRequest("Invalid poll option.".into())); + } + let redirect_url = tokio::task::spawn_blocking({ let pool = state.db.clone(); move || -> Result { diff --git a/src/main.rs b/src/main.rs index 59c1475..3ca1b30 100644 --- a/src/main.rs +++ b/src/main.rs @@ -175,11 +175,19 @@ async fn run_server(port_override: Option) -> anyhow::Result<()> { // clear error rather than discovering misconfiguration at runtime (#8). CONFIG.validate()?; - let data_dir = std::path::Path::new(&CONFIG.database_path) - .parent() - .unwrap_or(std::path::Path::new(".")); + // Fix #9: Path::parent() on a bare filename (e.g. "rustchan.db") returns + // Some("") rather than None, so the old `unwrap_or(".")` never fired and + // `create_dir_all("")` would fail with NotFound. Treat an empty-string + // parent the same as a missing one. + let data_dir: std::path::PathBuf = { + let p = std::path::Path::new(&CONFIG.database_path); + match p.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => std::path::PathBuf::from("."), + } + }; - std::fs::create_dir_all(data_dir)?; + std::fs::create_dir_all(&data_dir)?; std::fs::create_dir_all(&CONFIG.upload_dir)?; print_banner(); @@ -245,16 +253,15 @@ async fn run_server(port_override: Option) -> anyhow::Result<()> { // Tor: create hidden-service directory + torrc, launch tor as a background // process, and poll for the hostname file (all non-blocking). - // rsplit(':').next() finds the last colon-delimited segment, which is always - // the port regardless of whether the host part is IPv4 ("0.0.0.0:8080") or - // IPv6 ("[::1]:8080"). - let bind_port = CONFIG - .bind_addr - .rsplit(':') - .next() - .and_then(|p| p.parse::().ok()) + // Fix #1: derive bind_port from `bind_addr` (which already incorporates + // port_override) rather than CONFIG.bind_addr. Previously, starting with + // `--port 9000` would still pass 8080 to Tor's HiddenServicePort. + // rsplit_once(':') handles both IPv4 ("0.0.0.0:9000") and IPv6 ("[::1]:9000"). + let bind_port = bind_addr + .rsplit_once(':') + .and_then(|(_, p)| p.parse::().ok()) .unwrap_or(8080); - detect::detect_tor(CONFIG.enable_tor_support, bind_port, data_dir); + detect::detect_tor(CONFIG.enable_tor_support, bind_port, &data_dir); println!(); let state = AppState { @@ -314,11 +321,10 @@ async fn run_server(port_override: Option) -> anyhow::Result<()> { } Err(e) => tracing::warn!("WAL checkpoint failed: {}", e), } - // PRAGMA optimize updates internal statistics used by the - // query planner. It is cheap and idempotent (#18). - if let Ok(conn) = bg.get() { - let _ = conn.execute_batch("PRAGMA optimize;"); - } + // Fix #7: reuse `conn` instead of calling bg.get() again. + // A second acquire while the first is still alive deadlocks + // with a pool size of 1. + let _ = conn.execute_batch("PRAGMA optimize;"); } } }); @@ -545,19 +551,46 @@ fn build_router(state: AppState) -> Router { base-uri 'self'", ), )) - .layer(tower_http::set_header::SetResponseHeaderLayer::overriding( - header::HeaderName::from_static("strict-transport-security"), - header::HeaderValue::from_static("max-age=31536000; includeSubDomains"), - )) .layer(tower_http::set_header::SetResponseHeaderLayer::overriding( header::HeaderName::from_static("permissions-policy"), header::HeaderValue::from_static( "geolocation=(), camera=(), microphone=(), payment=()", ), )) + // Fix #8: HSTS (RFC 6797 §7.2) MUST only be sent over HTTPS. + // Sending it over plain HTTP (localhost dev, Tor .onion) is incorrect + // and can cause Tor-aware clients to misbehave. The middleware below + // checks both the request scheme and the X-Forwarded-Proto header + // (set by TLS-terminating proxies) before adding the header. + .layer(axum_middleware::from_fn(hsts_middleware)) .with_state(state) } +/// Middleware that adds `Strict-Transport-Security` only when the connection +/// is confirmed to be HTTPS (RFC 6797 §7.2). Checks both the URI scheme +/// (set by some reverse proxies) and the `X-Forwarded-Proto` header. +async fn hsts_middleware( + req: axum::extract::Request, + next: axum::middleware::Next, +) -> axum::response::Response { + let is_https = req.uri().scheme_str() == Some("https") + || req + .headers() + .get("x-forwarded-proto") + .and_then(|v| v.to_str().ok()) + .map(|v| v.eq_ignore_ascii_case("https")) + .unwrap_or(false); + + let mut resp = next.run(req).await; + if is_https { + resp.headers_mut().insert( + header::HeaderName::from_static("strict-transport-security"), + header::HeaderValue::from_static("max-age=31536000; includeSubDomains"), + ); + } + resp +} + async fn serve_css() -> impl IntoResponse { ( StatusCode::OK, @@ -677,7 +710,9 @@ fn first_run_check(pool: &db::DbPool) -> anyhow::Result<()> { println!("║ FIRST RUN — SETUP REQUIRED ║"); println!("╠══════════════════════════════════════════════════════╣"); println!("║ No boards or admin accounts found. ║"); - println!("║ Create your first admin and boards: ║"); + // Fix #2: original line was only 40 display-columns wide (missing 16 spaces), + // breaking the box alignment. Padded to the correct inner width of 54. + println!("║ Create your first admin and boards: ║"); println!("║ ║"); println!("║ rustchan-cli admin create-admin admin mypassword ║"); println!("║ rustchan-cli admin create-board b Random \"Anything\" ║"); @@ -777,7 +812,10 @@ fn print_stats(pool: &db::DbPool, start: Instant, ts: &mut TermStats) { // Upload progress bar — shown only while uploads are active let active_uploads = ACTIVE_UPLOADS.load(Ordering::Relaxed).max(0) as u64; if active_uploads > 0 { - let tick = SPINNER_TICK.load(Ordering::Relaxed); + // Fix #5: SPINNER_TICK was read but never written anywhere, so the + // spinner was permanently frozen on frame 0 ("⠋"). Increment it here, + // inside the only branch that actually displays the spinner. + let tick = SPINNER_TICK.fetch_add(1, Ordering::Relaxed); let spinners = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; let spin = spinners .get((tick as usize) % spinners.len()) @@ -832,9 +870,13 @@ fn print_stats(pool: &db::DbPool, start: Instant, ts: &mut TermStats) { } } -/// Read the process RSS (resident set size) from /proc/self/status on Linux. -/// Returns 0 on non-Linux platforms or if the file cannot be read. -/// No additional dependencies needed — /proc/self/status is always present. +/// Read the process RSS (resident set size) in KiB. +/// +/// * Linux — parsed from `/proc/self/status` (VmRSS field, already in KiB). +/// * macOS — Fix #11: spawns `ps -o rss= -p ` (output is KiB on macOS). +/// Previously this returned 0 on macOS, showing a misleading +/// `mem: 0 KiB RSS` in the terminal stats display. +/// * Other — returns 0 rather than showing a misleading value. fn process_rss_kb() -> u64 { #[cfg(target_os = "linux")] { @@ -851,6 +893,20 @@ fn process_rss_kb() -> u64 { } } } + #[cfg(target_os = "macos")] + { + // `ps -o rss=` outputs the RSS in KiB on macOS (no header when '=' suffix used). + let pid = std::process::id().to_string(); + if let Ok(out) = std::process::Command::new("ps") + .args(["-o", "rss=", "-p", &pid]) + .output() + { + let s = String::from_utf8_lossy(&out.stdout); + if let Ok(kb) = s.trim().parse::() { + return kb; + } + } + } 0 } @@ -888,9 +944,12 @@ fn walkdir_size(path: &std::path::Path) -> u64 { entries .flatten() .map(|e| { - let p = e.path(); - if p.is_dir() { - walkdir_size(&p) + // Fix #10: use file_type() from the DirEntry (does NOT follow + // symlinks) instead of Path::is_dir() (which does). A symlink + // loop via is_dir() causes unbounded recursion and a stack overflow. + let is_real_dir = e.file_type().map(|ft| ft.is_dir()).unwrap_or(false); + if is_real_dir { + walkdir_size(&e.path()) } else { e.metadata().map(|m| m.len()).unwrap_or(0) } @@ -901,24 +960,45 @@ fn walkdir_size(path: &std::path::Path) -> u64 { // ─── Startup banner ────────────────────────────────────────────────────────── fn print_banner() { - println!("┌─────────────────────────────────────────────────────┐"); - println!( - "│ {} v{} │", - CONFIG.forum_name, - env!("CARGO_PKG_VERSION") - ); - println!("├─────────────────────────────────────────────────────┤"); - println!( - "│ Bind {} │", - &CONFIG.bind_addr + // Fix #3: All dynamic values (forum_name, bind_addr, paths, MiB sizes) are + // padded/truncated to exactly fill the fixed inner width, so the right-hand + // │ character is always aligned regardless of the actual value length. + const INNER: usize = 53; + + // Truncate `s` to `width` chars, then right-pad with spaces to `width`. + let cell = |s: String, width: usize| -> String { + let chars: Vec = s.chars().collect(); + if chars.len() >= width { + chars + .get(..width) + .map(|s| s.iter().collect()) + .unwrap_or_else(|| s.clone()) + } else { + format!("{}{}", s, " ".repeat(width - chars.len())) + } + }; + + let title = cell( + format!("{} v{}", CONFIG.forum_name, env!("CARGO_PKG_VERSION")), + INNER - 2, // 2 leading spaces in "│ │" ); - println!("│ DB {} │", &CONFIG.database_path); - println!("│ Uploads {} │", &CONFIG.upload_dir); - println!( - "│ Images {} MiB max │ Videos {} MiB max │", - CONFIG.max_image_size / 1024 / 1024, - CONFIG.max_video_size / 1024 / 1024 + let bind = cell(CONFIG.bind_addr.clone(), INNER - 10); // "│ Bind <val>│" + let db = cell(CONFIG.database_path.clone(), INNER - 10); // "│ DB <val>│" + let upl = cell(CONFIG.upload_dir.clone(), INNER - 10); // "│ Uploads <val>│" + let img_mib = CONFIG.max_image_size / 1024 / 1024; + let vid_mib = CONFIG.max_video_size / 1024 / 1024; + let limits = cell( + format!("Images {} MiB max │ Videos {} MiB max", img_mib, vid_mib), + INNER - 4, // "│ <val> │" ); + + println!("┌─────────────────────────────────────────────────────┐"); + println!("│ {}│", title); + println!("├─────────────────────────────────────────────────────┤"); + println!("│ Bind {}│", bind); + println!("│ DB {}│", db); + println!("│ Uploads {}│", upl); + println!("│ {} │", limits); println!("└─────────────────────────────────────────────────────┘"); } @@ -936,6 +1016,18 @@ fn spawn_keyboard_handler(pool: db::DbPool, start_time: Instant) { let handle = stdin.lock(); let mut reader = BufReader::new(handle); + // Fix #4: TermStats must persist across keypresses so that + // prev_req_count/prev_post_count/prev_thread_count reflect the values + // at the *previous* 's' press, not zero. Initializing inside the + // match arm made every post/thread appear as "+N new" and reported the + // lifetime-average req/s instead of the current rate. + let mut persistent_stats = TermStats { + prev_req_count: REQUEST_COUNT.load(Ordering::Relaxed), + prev_post_count: 0, + prev_thread_count: 0, + last_tick: Instant::now(), + }; + loop { let mut line = String::new(); match reader.read_line(&mut line) { @@ -946,14 +1038,7 @@ fn spawn_keyboard_handler(pool: db::DbPool, start_time: Instant) { let cmd = line.trim().to_lowercase(); match cmd.as_str() { "s" => { - // Snapshot stats without advancing the background state - let mut snap = TermStats { - prev_req_count: 0, - prev_post_count: 0, - prev_thread_count: 0, - last_tick: start_time, - }; - print_stats(&pool, start_time, &mut snap); + print_stats(&pool, start_time, &mut persistent_stats); } "l" => kb_list_boards(&pool), "c" => kb_create_board(&pool, &mut reader), @@ -1036,6 +1121,16 @@ fn kb_create_board(pool: &db::DbPool, reader: &mut dyn std::io::BufRead) { let nsfw_raw = prompt("NSFW board? [y/N]:"); let nsfw = matches!(nsfw_raw.to_lowercase().as_str(), "y" | "yes"); + // Fix #6: prompt for media flags and call create_board_with_media_flags so + // boards created from the console have the same capabilities as those + // created via `rustchan-cli admin create-board`. + let no_images_raw = prompt("Disable images? [y/N]:"); + let no_videos_raw = prompt("Disable video? [y/N]:"); + let no_audio_raw = prompt("Disable audio? [y/N]:"); + let allow_images = !matches!(no_images_raw.to_lowercase().as_str(), "y" | "yes"); + let allow_video = !matches!(no_videos_raw.to_lowercase().as_str(), "y" | "yes"); + let allow_audio = !matches!(no_audio_raw.to_lowercase().as_str(), "y" | "yes"); + let short_lc = short.to_lowercase(); if !short_lc.chars().all(|c| c.is_ascii_alphanumeric()) || short_lc.is_empty() @@ -1049,13 +1144,25 @@ fn kb_create_board(pool: &db::DbPool, reader: &mut dyn std::io::BufRead) { println!(" \x1b[31m[err]\x1b[0m Could not get DB connection."); return; }; - match db::create_board(&conn, &short_lc, &name, &desc, nsfw) { + match db::create_board_with_media_flags( + &conn, + &short_lc, + &name, + &desc, + nsfw, + allow_images, + allow_video, + allow_audio, + ) { Ok(id) => println!( - " \x1b[32m✓\x1b[0m Board /{}/ — {}{} created (id={}).", + " \x1b[32m✓\x1b[0m Board /{}/ — {}{} created (id={}). images:{} video:{} audio:{}", short_lc, name, if nsfw { " [NSFW]" } else { "" }, - id + id, + if allow_images { "yes" } else { "no" }, + if allow_video { "yes" } else { "no" }, + if allow_audio { "yes" } else { "no" }, ), Err(e) => println!(" \x1b[31m[err]\x1b[0m {}", e), } diff --git a/src/middleware/mod.rs b/src/middleware/mod.rs index 6cb0fb3..d9e5672 100644 --- a/src/middleware/mod.rs +++ b/src/middleware/mod.rs @@ -39,7 +39,7 @@ use axum::{ extract::Request, http::Uri, middleware::Next, - response::{IntoResponse, Redirect, Response}, + response::{IntoResponse, Response}, }; use dashmap::DashMap; use once_cell::sync::Lazy; @@ -57,8 +57,11 @@ static LAST_CLEANUP_SECS: AtomicU64 = AtomicU64::new(0); // // A single global progress counter is sufficient because only one admin can // run a backup at a time, and backups are serialised through spawn_blocking. -// All fields use Ordering::Relaxed — the JS poller only needs eventual -// consistency; strict happens-before ordering is not required here. +// Counter fields (files_done/total, bytes_done/total) use Ordering::Relaxed — +// the JS poller only needs eventual consistency for those. +// The `phase` field uses Release/Acquire so that counter zeroes written in +// `reset()` are guaranteed visible to any reader that subsequently loads the +// new phase value. /// Phase codes stored in BackupProgress::phase. pub mod backup_phase { @@ -95,13 +98,20 @@ impl BackupProgress { } /// Reset all counters and set a new phase. + /// + /// FIX[ORANGE-3]: The previous implementation stored the new phase with + /// `Relaxed`, giving no ordering guarantee between the counter-zero stores + /// [1-4] and the phase store [5]. A JS poller that loaded the new phase + /// could then read stale counter values from the previous run. + /// The phase store now uses `Release` so all preceding Relaxed stores are + /// guaranteed to be visible to any reader that loads `phase` with `Acquire`. pub fn reset(&self, phase: u64) { - use std::sync::atomic::Ordering::Relaxed; + use std::sync::atomic::Ordering::{Relaxed, Release}; self.files_done.store(0, Relaxed); self.files_total.store(0, Relaxed); self.bytes_done.store(0, Relaxed); self.bytes_total.store(0, Relaxed); - self.phase.store(phase, Relaxed); + self.phase.store(phase, Release); // Release: makes counter zeroes visible before new phase } } @@ -174,11 +184,21 @@ pub async fn rate_limit_middleware(req: Request, next: Next) -> Response { // We only check presence here (no DB round-trip in middleware); the actual // session validation happens inside admin handlers. This is sufficient // for rate-limiting purposes since the cookie is HttpOnly+SameSite=Strict. + // + // FIX[RED-1]: The previous bare `.contains("chan_admin_session=")` matched + // any substring of the raw Cookie header, allowing two trivial bypasses: + // • `Cookie: x=chan_admin_session=forged` (value embeds the string) + // • `Cookie: xchan_admin_session=anything` (name is a prefix) + // We now split on ';', trim each pair, and require the segment to *start* + // with exactly "chan_admin_session=" — an exact cookie-name match. let has_admin_cookie = req .headers() .get(axum::http::header::COOKIE) .and_then(|v| v.to_str().ok()) - .map(|s| s.contains("chan_admin_session=")) + .map(|s| { + s.split(';') + .any(|pair| pair.trim().starts_with("chan_admin_session=")) + }) .unwrap_or(false); if has_admin_cookie { return next.run(req).await; @@ -244,6 +264,27 @@ pub async fn rate_limit_middleware(req: Request, next: Next) -> Response { next.run(req).await } +/// Escape the five characters that are special in HTML text/attribute contexts. +/// Keeps config-sourced strings safe for interpolation into HTML output. +/// +/// FIX[YELLOW-4]: `CONFIG.forum_name` is operator-supplied and could contain +/// `<`, `>`, `&`, `"`, or `'`. Interpolating it raw produces malformed HTML +/// and, if the value ever reaches a visible element, a direct XSS vector. +fn html_escape(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for ch in s.chars() { + match ch { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + c => out.push(c), + } + } + out +} + /// Build a lightweight HTML page that shows an in-page toast notification and /// then navigates the browser back to where it came from. /// @@ -251,6 +292,9 @@ pub async fn rate_limit_middleware(req: Request, next: Next) -> Response { /// that the user stays in context (they see the message overlaid on what looks /// like their previous page) rather than landing on a dead-end error screen. fn rate_limited_toast_page() -> String { + // FIX[YELLOW-4]: escape before interpolation so special chars in the forum + // name never produce malformed HTML (or a future XSS if copied elsewhere). + let forum_name_escaped = html_escape(&crate::config::CONFIG.forum_name); format!( r#"<!DOCTYPE html> <html lang="en"> @@ -319,7 +363,7 @@ fn rate_limited_toast_page() -> String { </script> </body> </html>"#, - forum_name = crate::config::CONFIG.forum_name + forum_name = forum_name_escaped ) } @@ -402,13 +446,20 @@ fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { /// Trailing-slash normalization middleware. /// /// Strips a trailing `/` from every path except the root `/` and issues a -/// 301 Moved Permanently redirect. This makes routes like +/// 308 Permanent Redirect. This makes routes like /// /{board}/catalog/ → /{board}/catalog /// /{board}/thread/5/ → /{board}/thread/5 /// /{board}/ → /{board} /// work correctly without 404s, regardless of whether the user typed the /// slash, a browser added it, or an old bookmark included it. /// +/// FIX[ORANGE-2]: The previous `Redirect::permanent` issued a 301, which +/// permits user-agents to reissue the request as GET (RFC 7231 §6.4.2 — +/// and every major browser does). A POST to a trailing-slash URL would +/// silently drop the form body. We now use 308 Permanent Redirect +/// (RFC 7538), which explicitly mandates that the method and body are +/// preserved on redirect. +/// /// Query strings are preserved across the redirect. pub async fn normalize_trailing_slash(req: Request, next: Next) -> Response { let uri = req.uri(); @@ -426,7 +477,12 @@ pub async fn normalize_trailing_slash(req: Request, next: Next) -> Response { // Validate the rebuilt path before redirecting. if new_path_and_query.parse::<Uri>().is_ok() { - return Redirect::permanent(&new_path_and_query).into_response(); + // 308 Permanent Redirect: method + body are preserved (RFC 7538). + return ( + axum::http::StatusCode::PERMANENT_REDIRECT, + [(axum::http::header::LOCATION, new_path_and_query)], + ) + .into_response(); } // If URI reconstruction failed for any reason, fall through and let diff --git a/src/models.rs b/src/models.rs index 3ac4c44..2be5589 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,5 +1,9 @@ // models.rs — plain data structs that map 1:1 to database rows. // No ORM magic; fields match column names for easy rusqlite mapping. +// +// NOTE: When writing rusqlite FromRow impls for wide structs like Board (18+ +// fields), prefer named-column access (`row.get("col")?`) over positional +// indices to avoid silent mis-binding when columns are reordered. use serde::{Deserialize, Serialize}; @@ -7,6 +11,10 @@ use serde::{Deserialize, Serialize}; /// Classifies an uploaded file as image, video, or audio. /// Stored as a TEXT column in posts ("image", "video", "audio"). +/// +/// The serde `rename_all = "lowercase"` representation **must** stay in sync +/// with `as_str()` / `from_db_str()`. Add a round-trip unit test whenever a +/// new variant is introduced. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum MediaType { @@ -61,6 +69,12 @@ impl MediaType { } } +impl std::fmt::Display for MediaType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + /// A board, e.g. /tech/ — Technology #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Board { @@ -138,17 +152,18 @@ pub struct Post { } /// Admin user record -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct AdminUser { pub id: i64, pub username: String, + /// Excluded from Serialize in practice — be careful not to expose this. pub password_hash: String, pub created_at: i64, } /// Active admin session -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct AdminSession { pub id: String, @@ -158,7 +173,7 @@ pub struct AdminSession { } /// A banned IP hash -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct Ban { pub id: i64, pub ip_hash: String, @@ -169,7 +184,7 @@ pub struct Ban { } /// A word filter rule -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct WordFilter { pub id: i64, pub pattern: String, @@ -177,14 +192,14 @@ pub struct WordFilter { } /// Board with live thread count, used on the home page -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct BoardStats { pub board: Board, pub thread_count: i64, } /// Summary used on board index: thread + its last few reply counts -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct ThreadSummary { pub thread: Thread, /// Latest N replies (for board index preview) @@ -194,7 +209,7 @@ pub struct ThreadSummary { } /// Form data for posting a new thread or reply (parsed from multipart) -#[derive(Debug, Default)] +#[derive(Debug, Default, Deserialize)] #[allow(dead_code)] pub struct PostForm { pub name: String, @@ -231,7 +246,7 @@ pub struct LoginForm { } /// A poll attached to a thread's OP -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct Poll { pub id: i64, @@ -242,7 +257,7 @@ pub struct Poll { } /// A single poll option with live vote count (joined from poll_votes) -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct PollOption { pub id: i64, @@ -253,7 +268,7 @@ pub struct PollOption { } /// Full poll data passed to templates -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct PollData { pub poll: Poll, pub options: Vec<PollOption>, @@ -277,7 +292,7 @@ fn default_page() -> i64 { } /// Pagination helper -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct Pagination { pub page: i64, pub per_page: i64, @@ -285,32 +300,44 @@ pub struct Pagination { } impl Pagination { + /// Create a new Pagination, clamping all values to sane minimums. + /// + /// - `page` is clamped to >= 1 + /// - `per_page` is clamped to >= 1 (avoids division by zero) + /// - `total` is clamped to >= 0 pub fn new(page: i64, per_page: i64, total: i64) -> Self { Self { - page, - per_page, - total, + page: page.max(1), + per_page: per_page.max(1), + total: total.max(0), } } + + /// Total number of pages. Always returns at least 1 so templates can + /// safely display "page 1 of 1" even on empty result sets. pub fn total_pages(&self) -> i64 { - if self.per_page <= 0 { - return 1; - } - (self.total.saturating_add(self.per_page - 1)) / self.per_page + // per_page is guaranteed >= 1 by new(), but defend against manual + // construction just in case. + let pp = self.per_page.max(1); + let t = self.total.max(0); + ((t.saturating_add(pp - 1)) / pp).max(1) } + pub fn offset(&self) -> i64 { - (self.page - 1).max(0).saturating_mul(self.per_page) + (self.page.max(1) - 1).saturating_mul(self.per_page.max(1)) } + pub fn has_prev(&self) -> bool { self.page > 1 } + pub fn has_next(&self) -> bool { self.page < self.total_pages() } } /// Aggregate site-wide statistics shown on the home page. -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone, Default, Serialize)] pub struct SiteStats { /// Total posts ever made pub total_posts: i64, @@ -325,7 +352,7 @@ pub struct SiteStats { } /// A user-filed report against a post -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct Report { pub id: i64, @@ -341,7 +368,7 @@ pub struct Report { } /// Report enriched with context from joined tables (used in admin inbox) -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct ReportWithContext { pub report: Report, pub board_short: String, @@ -352,7 +379,7 @@ pub struct ReportWithContext { } /// A single entry in the moderation action log -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] #[allow(dead_code)] pub struct ModLogEntry { pub id: i64, @@ -370,7 +397,7 @@ pub struct ModLogEntry { } /// Represents a saved backup file on disk (shown in admin panel). -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct BackupInfo { /// Filename only (no directory path). pub filename: String, @@ -381,7 +408,7 @@ pub struct BackupInfo { } /// A user-submitted ban appeal -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct BanAppeal { pub id: i64, pub ip_hash: String, @@ -390,3 +417,131 @@ pub struct BanAppeal { pub status: String, // "open" | "dismissed" pub created_at: i64, } + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + // ── MediaType serde ↔ DB string parity ──────────────────────────────── + + #[test] + fn media_type_serde_matches_db_str() { + for mt in [MediaType::Image, MediaType::Video, MediaType::Audio] { + let json = + serde_json::to_string(&mt).expect("MediaType always serialises to a JSON string"); + let json_str = json.trim_matches('"'); + assert_eq!( + mt.as_str(), + json_str, + "as_str() and serde disagree for {:?}", + mt + ); + assert_eq!( + MediaType::from_db_str(json_str), + Some(mt.clone()), + "from_db_str() round-trip failed for {:?}", + mt + ); + } + } + + #[test] + fn media_type_display_matches_as_str() { + for mt in [MediaType::Image, MediaType::Video, MediaType::Audio] { + assert_eq!(format!("{}", mt), mt.as_str()); + } + } + + #[test] + fn media_type_from_mime() { + assert_eq!(MediaType::from_mime("image/png"), Some(MediaType::Image)); + assert_eq!(MediaType::from_mime("video/mp4"), Some(MediaType::Video)); + assert_eq!(MediaType::from_mime("audio/ogg"), Some(MediaType::Audio)); + assert_eq!(MediaType::from_mime("application/json"), None); + } + + #[test] + fn media_type_from_ext() { + assert_eq!(MediaType::from_ext("jpg"), Some(MediaType::Image)); + assert_eq!(MediaType::from_ext("mp4"), Some(MediaType::Video)); + assert_eq!(MediaType::from_ext("flac"), Some(MediaType::Audio)); + assert_eq!(MediaType::from_ext("exe"), None); + } + + // ── Pagination ──────────────────────────────────────────────────────── + + #[test] + fn pagination_clamps_inputs() { + let p = Pagination::new(0, 0, -5); + assert_eq!(p.page, 1); + assert_eq!(p.per_page, 1); + assert_eq!(p.total, 0); + } + + #[test] + fn pagination_total_pages_at_least_one() { + let p = Pagination::new(1, 10, 0); + assert_eq!(p.total_pages(), 1); + } + + #[test] + fn pagination_total_pages_normal() { + assert_eq!(Pagination::new(1, 10, 1).total_pages(), 1); + assert_eq!(Pagination::new(1, 10, 10).total_pages(), 1); + assert_eq!(Pagination::new(1, 10, 11).total_pages(), 2); + assert_eq!(Pagination::new(1, 10, 20).total_pages(), 2); + assert_eq!(Pagination::new(1, 10, 21).total_pages(), 3); + } + + #[test] + fn pagination_offset() { + assert_eq!(Pagination::new(1, 10, 100).offset(), 0); + assert_eq!(Pagination::new(2, 10, 100).offset(), 10); + assert_eq!(Pagination::new(3, 25, 100).offset(), 50); + } + + #[test] + fn pagination_offset_clamped_for_bad_page() { + // Even if someone bypasses new() and manually sets page = -1 + let p = Pagination { + page: -1, + per_page: 10, + total: 50, + }; + assert_eq!(p.offset(), 0); + } + + #[test] + fn pagination_has_prev_and_next() { + let p = Pagination::new(1, 10, 30); + assert!(!p.has_prev()); + assert!(p.has_next()); + + let p = Pagination::new(2, 10, 30); + assert!(p.has_prev()); + assert!(p.has_next()); + + let p = Pagination::new(3, 10, 30); + assert!(p.has_prev()); + assert!(!p.has_next()); + } + + #[test] + fn pagination_single_page() { + let p = Pagination::new(1, 10, 5); + assert!(!p.has_prev()); + assert!(!p.has_next()); + assert_eq!(p.total_pages(), 1); + } + + #[test] + fn pagination_empty_results() { + let p = Pagination::new(1, 10, 0); + assert!(!p.has_prev()); + assert!(!p.has_next()); + assert_eq!(p.total_pages(), 1); + assert_eq!(p.offset(), 0); + } +} diff --git a/src/templates/admin.rs b/src/templates/admin.rs index 2cfcc5a..5e6db95 100644 --- a/src/templates/admin.rs +++ b/src/templates/admin.rs @@ -277,7 +277,8 @@ pub fn admin_panel_page( let preview = escape_html(rc.post_preview.trim()); let reason = escape_html(&rc.report.reason); let age = fmt_ts(rc.report.created_at); - let ip_short = rc.post_ip_hash.get(..16).unwrap_or(&rc.post_ip_hash); + // FIX[A-T2]: ip_short was computed here but immediately discarded with + // `let _ = ip_short` — dead code from an unfinished refactor. Removed. report_rows.push_str(&format!( r#"<tr> <td><a href="/{board}/thread/{tid}#p{pid}" title="view post">/{board}/ No.{pid}</a></td> @@ -310,7 +311,7 @@ pub fn admin_panel_page( rid = rc.report.id, ip_hash = escape_html(&rc.post_ip_hash), )); - let _ = ip_short; // suppress unused warning + let _ = (); // (ip_short dead-code removed — see FIX[A-T2] above) } // ── Ban appeals ─────────────────────────────────────────────────────────── @@ -783,7 +784,11 @@ pub fn admin_ip_history_page( "" }; let body_preview: String = post.body.chars().take(120).collect(); - let body_preview = if post.body.len() > 120 { + // FIX[A-T1]: test character count, not byte count. post.body.len() + // measures UTF-8 bytes so multi-byte characters (emoji, CJK, …) can + // cause the ellipsis to be added even when nothing was truncated, or + // omitted even when content was. + let body_preview = if post.body.chars().count() > 120 { format!("{}…", escape_html(&body_preview)) } else { escape_html(&body_preview) diff --git a/src/templates/board.rs b/src/templates/board.rs index 05b4f78..d3d938c 100644 --- a/src/templates/board.rs +++ b/src/templates/board.rs @@ -213,9 +213,10 @@ pub fn board_page( )); } + // FIX[B-T2]: escape_html on board.short_name before embedding in the URL. body.push_str(&render_pagination( pagination, - &format!("/{}", board.short_name), + &format!("/{}", escape_html(&board.short_name)), )); body.push_str(TOGGLE_SCRIPT); @@ -307,10 +308,15 @@ fn render_thread_summary( } if let Some(body) = &t.op_body { - let truncated = if body.len() > 300 { + // FIX[B-T1]: Count and slice by character, not by byte. + // body[..300] panics on any post whose 300th byte falls inside a + // multi-byte codepoint (emoji, CJK, Arabic, etc.). + let char_count = body.chars().count(); + let truncated = if char_count > 300 { + let safe: String = body.chars().take(300).collect(); format!( r#"{} <a href="/{b}/thread/{tid}">…[Read more]</a>"#, - escape_html(&body[..300]), + escape_html(&safe), b = escape_html(board_short), tid = t.id, ) @@ -580,7 +586,7 @@ pub fn search_page( pagination, &format!( "/{}/search?q={}", - board.short_name, + escape_html(&board.short_name), urlencoding_simple(query) ), )); @@ -677,9 +683,10 @@ pub fn archive_page( )); } body.push_str("</div>"); + // FIX[B-T2]: escape before embedding in pagination URL. body.push_str(&render_pagination( pagination, - &format!("/{}/archive", board.short_name), + &format!("/{}/archive", escape_html(&board.short_name)), )); } diff --git a/src/templates/forms.rs b/src/templates/forms.rs index 9d2c148..14d35b6 100644 --- a/src/templates/forms.rs +++ b/src/templates/forms.rs @@ -83,30 +83,34 @@ pub(super) fn new_thread_form(board_short: &str, csrf_token: &str, board: &Board {audio_combo_row} {edit_token_row} {captcha_row} - <tr><td colspan="2"> - <details class="poll-creator"> - <summary>[ 📊 Add a Poll to this thread ]</summary> - <div class="poll-creator-inner"> - <div class="poll-creator-row"> - <label>Question<input type="text" name="poll_question" placeholder="What do you think?" maxlength="256"></label> - </div> - <div id="poll-options-list"> - <div class="poll-option-row"><input type="text" name="poll_option" placeholder="Option 1" maxlength="128"><button type="button" class="poll-remove-btn" data-action="remove-poll-option" style="display:none">✕</button></div> - <div class="poll-option-row"><input type="text" name="poll_option" placeholder="Option 2" maxlength="128"><button type="button" class="poll-remove-btn" data-action="remove-poll-option" style="display:none">✕</button></div> - </div> - <button type="button" class="poll-add-btn" data-action="add-poll-option">+ Add Option</button> - <div class="poll-creator-row poll-duration-row"> - <label>Duration - <input type="number" name="poll_duration_value" value="24" min="1" max="720" class="poll-duration-input"> - <select name="poll_duration_unit" class="poll-duration-unit"> - <option value="hours">Hours</option> - <option value="minutes">Minutes</option> - </select> - </label> + <td colspan="2"> + <details class="poll-creator"> + <summary>[ 📊 Add a Poll to this thread ]</summary> + <div class="poll-creator-inner"> + <div class="poll-creator-row"> + <!-- FIX[F-T1]: maxlength matches server limit of 500 chars (was 256) --> + <label>Question<input type="text" name="poll_question" placeholder="What do you think?" maxlength="500"></label> + </div> + <div id="poll-options-list"> + <!-- FIX[F-T1]: maxlength matches server limit of 200 chars (was 128) --> + <div class="poll-option-row"><input type="text" name="poll_option" placeholder="Option 1" maxlength="200"><button type="button" class="poll-remove-btn" data-action="remove-poll-option" style="display:none">✕</button></div> + <div class="poll-option-row"><input type="text" name="poll_option" placeholder="Option 2" maxlength="200"><button type="button" class="poll-remove-btn" data-action="remove-poll-option" style="display:none">✕</button></div> + </div> + <button type="button" class="poll-add-btn" data-action="add-poll-option">+ Add Option</button> + <div class="poll-creator-row poll-duration-row"> + <label>Duration + <input type="number" name="poll_duration_value" value="24" min="1" max="720" class="poll-duration-input"> + <!-- FIX[F-T2]: Added Days option — server now accepts "days" unit --> + <select name="poll_duration_unit" class="poll-duration-unit"> + <option value="hours">Hours</option> + <option value="minutes">Minutes</option> + <option value="days">Days</option> + </select> + </label> + </div> </div> - </div> - </details> - </td></tr> + </details> + </td></tr> </table> </form> </div> diff --git a/src/templates/mod.rs b/src/templates/mod.rs index af058c9..627dd4a 100644 --- a/src/templates/mod.rs +++ b/src/templates/mod.rs @@ -208,12 +208,16 @@ pub(super) fn render_pagination(p: &Pagination, base_url: &str) -> String { return String::new(); } let sep = if base_url.contains('?') { "&" } else { "?" }; + // FIX[M-T2]: escape base_url once here so every href it appears in is safe, + // regardless of what any caller passes. All current callers pass trusted + // values, but this makes the helper defensively correct for future callers. + let safe_base = escape_html(base_url); let mut html = String::from(r#"<div class="pagination">"#); if p.has_prev() { html.push_str(&format!( r#"<a href="{}{sep}page={}">[prev]</a> "#, - base_url, + safe_base, p.page - 1, sep = sep )); @@ -222,7 +226,7 @@ pub(super) fn render_pagination(p: &Pagination, base_url: &str) -> String { if p.has_next() { html.push_str(&format!( r#" <a href="{}{sep}page={}">[next]</a>"#, - base_url, + safe_base, p.page + 1, sep = sep )); @@ -370,7 +374,10 @@ pub(super) fn base_layout( // ─── Standalone error/ban pages (no board context) ──────────────────────────── -pub fn ban_page(reason: &str) -> String { +// FIX[M-T1]: ban_page must accept a csrf_token so the appeal form works. +// Previously the field was always empty and every appeal was rejected by the +// server's CSRF check, making the appeal feature completely non-functional. +pub fn ban_page(reason: &str, csrf_token: &str) -> String { let default_theme = live_default_theme(); let default_theme_attr = if !default_theme.is_empty() && default_theme != "terminal" { format!(r#" data-default-theme="{}""#, escape_html(&default_theme)) @@ -394,7 +401,7 @@ pub fn ban_page(reason: &str) -> String { <p style="margin-top:1.5rem;font-size:0.9rem">if you believe this ban was made in error, you may submit an appeal below.<br> appeals are reviewed by site staff. one appeal per 24 hours.</p> <form method="POST" action="/appeal" class="appeal-form"> -<input type="hidden" name="_csrf" id="appeal-csrf-field" value=""> +<input type="hidden" name="_csrf" id="appeal-csrf-field" value="{csrf}"> <textarea name="reason" rows="4" maxlength="512" placeholder="Briefly explain why you believe this ban should be lifted…" style="width:100%;box-sizing:border-box;margin:0.75rem 0;background:var(--bg-post);color:var(--text);border:1px solid var(--border);padding:0.5rem;resize:vertical"></textarea> @@ -402,10 +409,14 @@ appeals are reviewed by site staff. one appeal per 24 hours.</p> </form> <p style="margin-top:1.5rem"><a href="/">return home</a></p> </div> +<!-- Global CSRF token consumed by main.js for fetch-based requests --> +<input type="hidden" id="csrf_global" value="{csrf}"> +<script src="/static/main.js" defer></script> </body> </html>"#, default_theme_attr = default_theme_attr, reason = escape_html(reason), + csrf = escape_html(csrf_token), ) } diff --git a/src/templates/thread.rs b/src/templates/thread.rs index 10fc23e..9d5cc05 100644 --- a/src/templates/thread.rs +++ b/src/templates/thread.rs @@ -280,7 +280,10 @@ fn render_poll( } else { "poll-open" }, - expires = expires_str, + // FIX[T-T2]: escape_html for defensive correctness — expires_str is + // derived from integer arithmetic and fmt_ts today, but this guard + // ensures any future changes to expires_str can't inject HTML. + expires = escape_html(&expires_str), ); if show_results { @@ -525,7 +528,12 @@ pub fn render_post( // Edit link + report button (only on thread pages where show_delete=true) if show_delete { let now = chrono::Utc::now().timestamp(); - let within_edit_window = edit_window_secs > 0 && now - post.created_at <= edit_window_secs; + // FIX[T-T1]: edit_window_secs = 0 means no time restriction (always + // editable while allow_editing is true — matches the handler-layer fix). + // The previous guard had `> 0 && …` which suppressed the edit link + // entirely when the board used the no-limit setting. + let within_edit_window = edit_window_secs == 0 + || (edit_window_secs > 0 && now - post.created_at <= edit_window_secs); let edit_link = if within_edit_window { format!( r#" <a class="edit-btn" href="/{board}/post/{pid}/edit" title="Edit post">edit</a>"#, diff --git a/src/utils/crypto.rs b/src/utils/crypto.rs index 0fd2f49..6368af9 100644 --- a/src/utils/crypto.rs +++ b/src/utils/crypto.rs @@ -140,7 +140,23 @@ pub fn pow_challenge(board_short: &str, unix_ts: i64) -> String { /// FIX[NEW-C2]: Each (board, nonce) pair is recorded in SEEN_NONCES after its /// first successful verification and rejected on any subsequent call within the /// same window, closing the replay attack vector. +/// +/// FIX[RACE]: The previous implementation used a separate contains_key() check +/// followed by insert(), which is not atomic even with DashMap — two concurrent +/// requests carrying the same solved nonce could both pass the check before +/// either inserted, allowing replay under concurrent load. The fix uses the +/// entry API so the check and claim are performed in a single lock acquisition: +/// entry().or_insert_with() returns a reference to the existing value if the +/// key was already present, or inserts and returns the new value. We +/// distinguish "newly inserted by us" from "already existed" by comparing the +/// stored timestamp; if it equals `now` we inserted it, otherwise it was +/// already there. A cleaner approach is to use a dedicated atomic flag, but +/// the timestamp comparison is sufficient here because `now` is captured once +/// before the entry call and any pre-existing entry will have a strictly +/// earlier timestamp (same-second collisions are handled by the equality check +/// being in our favour: we set the value so it is always == now when we win). pub fn verify_pow(board_short: &str, nonce: &str) -> bool { + use dashmap::mapref::entry::Entry; use sha2::{Digest, Sha256}; let now = chrono::Utc::now().timestamp(); let now_minutes = now / 60; @@ -148,21 +164,25 @@ pub fn verify_pow(board_short: &str, nonce: &str) -> bool { // Prune stale entries to bound memory usage. SEEN_NONCES.retain(|_, ts| now - *ts < POW_WINDOW_SECS); - // Check whether this (board, nonce) pair has already been accepted. - let cache_key = format!("{}:{}", board_short, nonce); - if SEEN_NONCES.contains_key(&cache_key) { - return false; // FIX[NEW-C2]: replay rejected - } - // Try current minute and the 4 prior minutes. + let cache_key = format!("{}:{}", board_short, nonce); for delta in 0i64..=4 { let challenge = pow_challenge(board_short, (now_minutes - delta) * 60); let input = format!("{}:{}", challenge, nonce); let hash = Sha256::digest(input.as_bytes()); if leading_zero_bits(&hash) >= POW_DIFFICULTY { - // Record this nonce as consumed. - SEEN_NONCES.insert(cache_key, now); - return true; + // Atomically claim this nonce. entry() acquires the shard lock for + // the duration of the call, so no other thread can slip in between + // the existence check and the insertion. + match SEEN_NONCES.entry(cache_key) { + Entry::Vacant(e) => { + e.insert(now); + return true; // FIX[RACE]: we claimed it atomically + } + Entry::Occupied(_) => { + return false; // already consumed — replay rejected + } + } } } false diff --git a/src/utils/files.rs b/src/utils/files.rs index ec32d7e..03ab90d 100644 --- a/src/utils/files.rs +++ b/src/utils/files.rs @@ -90,9 +90,63 @@ pub fn detect_mime_type(data: &[u8]) -> Result<&'static str> { } } - // ── WebM (video or audio-only) ──────────────────────────────────────────── - if header.starts_with(b"\x1a\x45\xdf\xa3") { - return Ok("video/webm"); + // ── EBML container — distinguish WebM (video or audio-only) from MKV ──── + // + // Both WebM and Matroska (.mkv) start with the same EBML magic bytes + // (1A 45 DF A3), so checking only the magic is insufficient — MKV files + // containing H.264/HEVC/etc. would be accepted and stored as .webm, then + // silently fail to play in any browser. + // + // The EBML header always begins with the EBML ID (1A 45 DF A3) followed + // immediately by the header size (variable-length VINT), then a sequence + // of EBML elements. The DocType element has ID 0x4282. Its value is the + // ASCII string "webm" (browser-compatible) or "matroska" (reject). + // + // We scan the first 64 bytes for 0x42 0x82, read the 1-byte size that + // follows, then compare that many bytes to "webm" or "matroska". + // If DocType is absent or unrecognised we reject. + // + // For audio-only WebM (Opus/Vorbis streams, no video track) the docType + // is still "webm", but none of the subsequent EBML Track elements contain + // a video codec ID. We do not probe track types here — that would require + // parsing the full Segment element, which can be megabytes in. Instead we + // accept all valid "webm" docType files and rely on the background worker's + // ffprobe call to detect audio-only containers and classify them correctly. + // To give the handler a usable MIME type up front we return "video/webm" + // for now; the worker will update the post's mime_type to "audio/webm" if + // ffprobe finds no video stream. This is safe because both share the same + // file extension (.webm) and the browser media element handles both. + if data.get(..4) == Some(b"\x1a\x45\xdf\xa3") { + // Scan first 64 bytes for DocType element (ID = 0x42 0x82). + let scan_len = data.len().min(64); + // SAFETY: scan_len = data.len().min(64) so scan_len <= data.len() always holds. + let scan = data.get(..scan_len).unwrap_or(data); + let mut pos = 4usize; + let mut found_doctype: Option<&[u8]> = None; + // Use slice patterns so every access goes through bounds-checked .get(), + // satisfying clippy::indexing_slicing while keeping the loop readable. + while let Some([b0, b1, b2, ..]) = scan.get(pos..) { + if *b0 == 0x42 && *b1 == 0x82 { + // Next byte is the 1-byte DataSize (short form, bit 7 set -> size = byte & 0x7F). + let size_byte = *b2; + let value_len = (size_byte & 0x7f) as usize; + let value_start = pos + 3; + if value_start + value_len <= scan.len() { + found_doctype = scan.get(value_start..value_start + value_len); + } + break; + } + pos += 1; + } + return match found_doctype { + Some(b"webm") => Ok("video/webm"), // audio-only WebM also uses docType "webm" + Some(b"matroska") => Err(anyhow::anyhow!( + "Matroska (.mkv) files are not accepted. Please upload a WebM file instead." + )), + _ => Err(anyhow::anyhow!( + "Unrecognised EBML container. Accepted: WebM (video/webm)." + )), + }; } // ── Image formats ───────────────────────────────────────────────────────── @@ -135,6 +189,42 @@ pub fn detect_mime_type(data: &[u8]) -> Result<&'static str> { )) } +// ─── Disk-space guard ──────────────────────────────────────────────────────── + +/// Verify at least `2 × needed_bytes` of free space in `dir` before writing. +/// Uses `statvfs` on Unix; is a no-op (always Ok) on other platforms so Windows +/// dev environments still compile and run. +/// +/// Requiring 2× headroom means a crash mid-rename still leaves the original +/// temp file and will not fill the volume to 100 %. +#[cfg(unix)] +fn check_disk_space(dir: &std::path::Path, needed_bytes: usize) -> Result<()> { + unsafe { + let dir_bytes = dir.to_string_lossy(); + if let Ok(path_cstr) = std::ffi::CString::new(dir_bytes.as_bytes()) { + let mut stat: libc::statvfs = std::mem::zeroed(); + if libc::statvfs(path_cstr.as_ptr(), &mut stat) == 0 { + #[allow(clippy::unnecessary_cast)] + let free_bytes = (stat.f_bavail as u64) * (stat.f_frsize as u64); + let needed = (needed_bytes as u64).saturating_mul(2); + if free_bytes < needed { + return Err(anyhow::anyhow!( + "Insufficient disk space: need ~{} MiB free, only ~{} MiB available.", + needed / (1024 * 1024), + free_bytes / (1024 * 1024) + )); + } + } + } + } + Ok(()) +} + +#[cfg(not(unix))] +fn check_disk_space(_dir: &std::path::Path, _needed_bytes: usize) -> Result<()> { + Ok(()) +} + // ─── Main entry point ───────────────────────────────────────────────────────── /// Save an uploaded file to disk and generate its thumbnail (or audio placeholder). @@ -255,29 +345,9 @@ pub fn save_upload( let file_path_abs = board_dir.join(&filename); - // Disk-space pre-check (#14): verify at least 2× the file size is available + // Disk-space pre-check: verify at least 2× the file size is available // before writing. Uses statvfs on Unix; skipped on other platforms. - #[cfg(unix)] - { - unsafe { - let dir_bytes = board_dir.to_string_lossy(); - if let Ok(path_cstr) = std::ffi::CString::new(dir_bytes.as_bytes()) { - let mut stat: libc::statvfs = std::mem::zeroed(); - if libc::statvfs(path_cstr.as_ptr(), &mut stat) == 0 { - #[allow(clippy::unnecessary_cast)] - let free_bytes = (stat.f_bavail as u64) * (stat.f_frsize as u64); - let needed = (final_data.len() as u64).saturating_mul(2); - if free_bytes < needed { - return Err(anyhow::anyhow!( - "Insufficient disk space: need ~{} MiB free, only ~{} MiB available.", - needed / (1024 * 1024), - free_bytes / (1024 * 1024) - )); - } - } - } - } - } + check_disk_space(&board_dir, final_data.len())?; // Write via a temp file in the same directory, then atomically rename. // This guarantees no partial/corrupt file survives a crash or OOM mid-write. @@ -394,6 +464,11 @@ pub fn save_audio_with_image_thumb( let board_dir = PathBuf::from(boards_dir).join(board_short); std::fs::create_dir_all(&board_dir).context("Failed to create board directory")?; + // FIX[DISK]: Apply the same 2× disk-space pre-check used by save_upload. + // Previously this path skipped the check, so a nearly-full volume could + // produce a partial or failed write without a clear error. + check_disk_space(&board_dir, audio_data.len())?; + let file_path_abs = board_dir.join(&filename); { use std::io::Write as _; diff --git a/src/utils/sanitize.rs b/src/utils/sanitize.rs index 280cd96..d5ffa60 100644 --- a/src/utils/sanitize.rs +++ b/src/utils/sanitize.rs @@ -56,8 +56,13 @@ pub fn extract_video_embed(url: &str) -> Option<(&'static str, String)> { return Some(("streamable", code)); } } - // Invidious — any domain serving /watch?v=ID (11-char YouTube-style ID) - if !url.contains("youtube.com") && !url.contains("youtu.be") { + // Invidious — any domain serving /watch?v=ID (11-char YouTube-style ID). + // FIX[INVIDIOUS]: The previous code matched ANY URL containing ?v= or &v=, + // meaning a completely ordinary link like https://example.com/article?v=dQw4w9WgXcQ + // would be silently replaced with a YouTube embed widget. We now require + // the URL path to contain "/watch" (case-sensitive, matching real Invidious + // instances) before treating the ?v= parameter as a video ID. + if !url.contains("youtube.com") && !url.contains("youtu.be") && url.contains("/watch") { if let Some(id) = extract_yt_id_from_watch_param(url) { return Some(("youtube", id)); } @@ -382,9 +387,15 @@ fn render_inline(text: &str) -> String { let url = &caps[1]; let clean_url = url.trim_end_matches(['.', ',', ')', ';', '\'']); let trailing = &url[clean_url.len()..]; + // FIX[XSS]: Escape clean_url in both href and display text. + // The embed branch below already calls escape_html(); this branch + // was inserting clean_url raw. Safe today (RE_URL excludes &<> + // and input is pre-escaped), but latently dangerous if the call + // site ever changes. Apply escape_html() consistently. + let escaped_url = escape_html(clean_url); let link = format!( r#"<a href="{}" rel="nofollow noopener" target="_blank">{}</a>{}"#, - clean_url, clean_url, trailing + escaped_url, escaped_url, trailing ); // Check for supported video embed URLs. Emit only the embed span — // the URL becomes a data attribute and the span text, not a hyperlink. @@ -441,7 +452,12 @@ pub fn validate_body(body: &str) -> Result<&str, String> { if trimmed.is_empty() { return Err("Post body cannot be empty.".into()); } - if trimmed.len() > 4096 { + // FIX[CHARS]: Use .chars().count() rather than .len() (byte count). + // A post of 1,366 CJK characters is 4,098 UTF-8 bytes and would be + // incorrectly rejected by a byte-length check despite being well within + // the 4,096-character limit. The error message also now correctly says + // "characters" because that is what we are measuring. + if trimmed.chars().count() > 4096 { return Err("Post body exceeds 4096 characters.".into()); } Ok(trimmed) @@ -452,12 +468,13 @@ pub fn validate_body(body: &str) -> Result<&str, String> { /// Rules: /// • If `has_file` is true, an empty body is allowed — the file is enough. /// • If `has_file` is false, the body must not be blank (no empty posts). -/// • Body length is still capped at 4096 regardless. +/// • Body length is still capped at 4096 characters regardless. /// /// Returns the trimmed body (may be empty when a file is present). pub fn validate_body_with_file(body: &str, has_file: bool) -> Result<String, String> { let trimmed = body.trim(); - if trimmed.len() > 4096 { + // FIX[CHARS]: Same byte-vs-char fix as validate_body above. + if trimmed.chars().count() > 4096 { return Err("Post body exceeds 4096 characters.".into()); } if trimmed.is_empty() && !has_file { diff --git a/src/workers/mod.rs b/src/workers/mod.rs index d44d911..937a9fa 100644 --- a/src/workers/mod.rs +++ b/src/workers/mod.rs @@ -141,9 +141,23 @@ impl JobQueue { // ─── Worker pool startup ────────────────────────────────────────────────────── /// Spawn the background worker pool. Call exactly once at server startup. -/// Returns a vec of JoinHandles so the caller can await them during shutdown. +/// +/// Returns a vec of JoinHandles, one per worker, so the caller can await all +/// of them during graceful shutdown after cancelling `queue.cancel`. Without +/// holding these handles the caller has no way to know when in-progress jobs +/// have actually finished — the process could exit mid-transcode, leaving DB +/// rows permanently stuck in `"running"` state and partially-written files on +/// disk. +/// +/// Typical shutdown sequence: +/// queue.cancel.cancel(); +/// for h in handles { h.await.ok(); } +/// /// Workers are pure async Tokio tasks — they do not consume OS threads at rest. -pub fn start_worker_pool(queue: Arc<JobQueue>, ffmpeg_available: bool) { +pub fn start_worker_pool( + queue: Arc<JobQueue>, + ffmpeg_available: bool, +) -> Vec<tokio::task::JoinHandle<()>> { let n = std::thread::available_parallelism() .map(|p| p.get()) .unwrap_or(2) @@ -151,12 +165,14 @@ pub fn start_worker_pool(queue: Arc<JobQueue>, ffmpeg_available: bool) { info!("Background worker pool: {} worker(s) online", n); - for idx in 0..n { - let q = queue.clone(); - tokio::spawn(async move { - worker_loop(idx, q, ffmpeg_available).await; - }); - } + (0..n) + .map(|idx| { + let q = queue.clone(); + tokio::spawn(async move { + worker_loop(idx, q, ffmpeg_available).await; + }) + }) + .collect() } // ─── Worker loop ───────────────────────────────────────────────────────────── @@ -190,21 +206,48 @@ async fn worker_loop(id: usize, queue: Arc<JobQueue>, ffmpeg_available: bool) { let pool_done = queue.pool.clone(); let result = handle_job(job_id, &payload, ffmpeg_available, queue.pool.clone()).await; - let _ = tokio::task::spawn_blocking(move || { - if let Ok(c) = pool_done.get() { - match result { - Ok(()) => { - let _ = crate::db::complete_job(&c, job_id); - debug!("Worker {}: job #{} completed", id, job_id); - } - Err(ref e) => { - warn!("Worker {}: job #{} failed — {}", id, job_id, e); - let _ = crate::db::fail_job(&c, job_id, &e.to_string()); - } + // FIX[STUCK-RUNNING]: Previously pool_done.get() failures were + // silently ignored (if let Ok(c) = ...), leaving the job row + // permanently stuck in "running" — claim_next_job only claims + // "pending" rows, so it would never be retried or cleaned up. + // We now propagate the error into the back-off path so the + // worker retries acquiring a connection, and we log explicitly + // so operators can see pool exhaustion events. + let db_result = tokio::task::spawn_blocking(move || -> Result<()> { + let c = pool_done.get().map_err(anyhow::Error::from)?; + match result { + Ok(()) => { + crate::db::complete_job(&c, job_id)?; + } + Err(ref e) => { + warn!("Worker {}: job #{} failed — {}", id, job_id, e); + crate::db::fail_job(&c, job_id, &e.to_string())?; } } + Ok(()) }) .await; + match db_result { + Ok(Ok(())) => {} + Ok(Err(e)) => { + error!( + "Worker {}: failed to update completion status for job #{}: {}", + id, job_id, e + ); + let delay = backoff_duration(consecutive_errors); + consecutive_errors = consecutive_errors.saturating_add(1); + tokio::select! { + _ = sleep(delay) => {} + _ = queue.cancel.cancelled() => { return; } + } + } + Err(join_err) => { + error!( + "Worker {}: spawn_blocking panicked during job #{} completion: {}", + id, job_id, join_err + ); + } + } } Ok(Ok(None)) => { consecutive_errors = 0; // queue empty is not an error @@ -425,20 +468,50 @@ fn transcode_video_inner( let webm_abs = board_dir.join(&webm_name); let webm_rel = format!("{}/{}", board_short, webm_name); - std::fs::write(&webm_abs, &webm_bytes)?; + // FIX[ATOMIC-WRITE]: For AV1 WebM inputs, src and webm_abs resolve to the + // same path (same stem, same .webm extension). A direct fs::write would + // overwrite the source in-place; a crash or disk-full mid-write permanently + // corrupts the only copy of the file with no recovery path. + // + // We write to a uniquely named temp file in the same directory first, then + // atomically rename it into place. The rename is POSIX-atomic on the same + // filesystem, so readers always see either the old file or the new file — + // never a partial write. If anything fails before the rename, the source + // file is untouched and the job can be retried. + { + use std::io::Write as _; + let mut tmp = tempfile::NamedTempFile::new_in(&board_dir) + .map_err(|e| anyhow::anyhow!("Failed to create temp file for WebM output: {}", e))?; + tmp.write_all(&webm_bytes) + .map_err(|e| anyhow::anyhow!("Failed to write WebM transcode output: {}", e))?; + tmp.persist(&webm_abs) + .map_err(|e| anyhow::anyhow!("Failed to atomically rename WebM output: {}", e))?; + } let conn = pool.get()?; - let updated = - crate::db::update_all_posts_file_path(&conn, &file_path, &webm_rel, "video/webm")?; - if updated == 0 { - crate::db::update_post_file_info(&conn, post_id, &webm_rel, "video/webm")?; - } + // FIX[LEAK]: If any DB call below fails we must clean up the WebM we just + // wrote, otherwise it leaks on disk across all retry attempts. We record + // the path and remove it in the error branch via a guard closure. + let db_result = (|| -> Result<()> { + let updated = + crate::db::update_all_posts_file_path(&conn, &file_path, &webm_rel, "video/webm")?; + if updated == 0 { + crate::db::update_post_file_info(&conn, post_id, &webm_rel, "video/webm")?; + } - let thumb_path = crate::db::get_post_thumb_path(&conn, post_id)?.unwrap_or_default(); - let webm_sha256 = crate::utils::crypto::sha256_hex(&webm_bytes); - crate::db::delete_file_hash_by_path(&conn, &file_path)?; - crate::db::record_file_hash(&conn, &webm_sha256, &webm_rel, &thumb_path, "video/webm")?; + let thumb_path = crate::db::get_post_thumb_path(&conn, post_id)?.unwrap_or_default(); + let webm_sha256 = crate::utils::crypto::sha256_hex(&webm_bytes); + crate::db::delete_file_hash_by_path(&conn, &file_path)?; + crate::db::record_file_hash(&conn, &webm_sha256, &webm_rel, &thumb_path, "video/webm")?; + Ok(()) + })(); + + if let Err(e) = db_result { + // Remove the WebM we wrote so it doesn't accumulate across retries. + let _ = std::fs::remove_file(&webm_abs); + return Err(e); + } if ext != "webm" { let _ = std::fs::remove_file(&src); @@ -531,6 +604,23 @@ fn generate_waveform_inner( let conn = pool.get()?; crate::db::update_post_thumb_path(&conn, post_id, &png_rel)?; + // FIX[DEDUP-STALE]: The file_hashes dedup table was not updated after the + // waveform PNG was generated, so it still held the SVG placeholder path as + // thumb_path. Any future post uploading the same audio file and hitting the + // dedup cache via find_file_by_hash would receive the stale SVG path instead + // of the waveform PNG. We now update the dedup record so that all future + // dedup hits for this audio file correctly inherit the waveform thumbnail. + // + // We compute the SHA-256 of the audio data we already have in memory to + // identify the dedup row without a separate DB lookup, then refresh its + // thumb_path via a targeted UPDATE. An audio file may have been uploaded + // on a different board, so we match by file content (sha256) not by path. + let audio_sha256 = crate::utils::crypto::sha256_hex(&data); + let _ = conn.execute( + "UPDATE file_hashes SET thumb_path = ?1 WHERE sha256 = ?2", + rusqlite::params![png_rel, audio_sha256], + ); + info!("AudioWaveform done: post {} → {}", post_id, png_rel); Ok(()) }