From 35954dfa1ddeaa97242c705921d533f5eeda3085 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 30 Aug 2026 10:34:12 +0200 Subject: [PATCH] Write a panic down where it can still be read, with nothing in it that names the user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NFR-OPS-2 is two sentences — local crash capture always, upload only on explicit opt-in — and what existed was one `log::error!` in the Android entry point and nothing at all on desktop. So a panic on desktop went to stderr and died with the terminal, and a panic on Android went to a logcat ring buffer that is gone long before anyone reports anything. What the user saw either way was a job that stopped or a control that went dead, with nothing to send. That matters more here than it would in most applications, because NFR-ARCH-4 says no worker error may panic the process and the code is written that way: errors are typed and attached to the image or job they belong to. A panic is therefore by construction a bug — an invariant this codebase believed and got wrong — and it was the one class of failure with no trace. `dr_plat::crash` writes a record to the XDG state directory: version, time, os, arch, thread, panic location, message, backtrace. In dr-plat rather than in either entry point because "where does this platform let an application keep state" is a platform question, and Android's answer is neither XDG nor `temp_dir` — `set_state_dir` takes it from `internal_data_path`, the same place `dr_sync::account::set_data_dir` gets its answer. The hook resolves the directory when it fires rather than when it is installed, which is what lets it go in before everything else and cover the startup it would otherwise miss. **The content rule is the substance of this, not the plumbing.** NFR-SEC-2 forbids credentials in logs and plain files; the same reasoning applies with more force to what this application is actually about, because a user's library is private and so is its shape. `/home/anna/Photos/2019 Divorce/` says something about a person, and a crash record is exactly the file someone attaches to a bug report while trying to be helpful. So `redact` runs over the message *and* the backtrace, and is deliberately blunt: anything containing a slash goes, `content://` and `primary:DCIM/...` included, since SAF names a library just as precisely as a path does; anything beside a word like `password` goes; a long opaque run with letters and digits in it goes, which is the shape of an app password nobody labelled. The one exception is a `.rs` path, which keeps its basename — a backtrace with no filenames is close to useless and `library.rs:1270` says nothing about anybody. Over-redaction costs legibility. Under-redaction costs a user something they cannot take back. Those are not comparable, so the boundary is not the place to be clever. NFR-SEC-5 — face data never in a crash report, under any configuration — is met structurally rather than by filtering: this module reads no catalog, opens no image, touches no account. A record is assembled from the panic hook's own arguments and `std::env::consts`, and there is no code path from here to an embedding. The message length cap is the backstop for a payload some other module formatted something large into. stderr is the one surface that still sees the message unredacted, deliberately: the previous hook is chained rather than replaced, so a developer watching a terminal does not lose the panic because the application started writing files. It is ephemeral, local, and never attached to a report. **No upload path, and not half of one** — no endpoint, no queue, no "send this later" flag. Opt-in upload needs a server to receive it and a consent flow stating what leaves the device (NFR-SEC-4, and the preview-and-consent step NFR-OPS-1 requires of the diagnostics bundle). Neither exists, and a transport built ahead of its consent is the shape of thing that later gets switched on by default. Leaves NFR-OPS-1 cheaper by three things it will want unchanged: `state_dir` (the log belongs at `state_dir()/log` beside `crash/`, so the diagnostics bundle has one directory to collect), `redact` (NFR-OPS-1's "automatic redaction of credentials and tokens" is this function), and `prune` (a size-capped rotation is this, counting bytes instead of files). Co-Authored-By: Claude Opus 5 (1M context) --- apps/darkroom-android/Cargo.toml | 4 + apps/darkroom-android/src/lib.rs | 19 +- apps/darkroom-desktop/Cargo.toml | 4 + apps/darkroom-desktop/src/main.rs | 8 + platform/dr-plat/src/crash.rs | 620 ++++++++++++++++++++++++++++++ platform/dr-plat/src/lib.rs | 7 + 6 files changed, 659 insertions(+), 3 deletions(-) create mode 100644 platform/dr-plat/src/crash.rs diff --git a/apps/darkroom-android/Cargo.toml b/apps/darkroom-android/Cargo.toml index 5699149..c51e112 100644 --- a/apps/darkroom-android/Cargo.toml +++ b/apps/darkroom-android/Cargo.toml @@ -19,6 +19,10 @@ dr-ui.workspace = true # For `account::set_data_dir`: only the platform entry point knows where Android # lets this app keep files, and it must be set before any store is opened. dr-sync.workspace = true +# For the panic hook, and for `crash::set_state_dir` — Android has no XDG +# directories, so the entry point is the only place that knows where a crash +# record may be written. +dr-plat.workspace = true # Directly, not just through dr-ui: `android_main` takes an `AndroidApp` and # calls `slint::android::init`, both of which come from this crate. The backend # feature comes from dr-ui's target-specific dependency. diff --git a/apps/darkroom-android/src/lib.rs b/apps/darkroom-android/src/lib.rs index f8c417f..5757a1f 100644 --- a/apps/darkroom-android/src/lib.rs +++ b/apps/darkroom-android/src/lib.rs @@ -39,9 +39,18 @@ fn android_main(app: slint::android::AndroidApp) { // worker thread that panics is invisible: the process survives, the // channel it was writing to closes, and the UI reports only that // something "failed unexpectedly" with no way to find out what. - std::panic::set_hook(Box::new(|info| { - log::error!("panic: {info}"); - })); + // + // This used to be one `log::error!` of the raw panic, which had two + // problems: logcat is a ring buffer that is gone by the time a user + // reports anything, and the raw message can carry a document URI naming + // their library or a credential a library interpolated into an error + // (NFR-SEC-2). `dr_plat::crash` writes a redacted record to disk and logs + // the redacted form. Nothing uploads it. + // + // Before `set_state_dir` on purpose: the hook resolves the directory when + // it fires, so installing it first covers the startup below rather than + // leaving it uncovered. + dr_plat::crash::install(env!("CARGO_PKG_VERSION")); log::info!("DarkRoom v{}", env!("CARGO_PKG_VERSION")); @@ -54,6 +63,10 @@ fn android_main(app: slint::android::AndroidApp) { match app.internal_data_path() { Some(dir) => { log::info!("data dir: {}", dir.display()); + // Crash records go beside the account data rather than under it: + // both are app-private and neither is a cache, which is the whole + // distinction that matters here (see `dr_plat::crash::state_dir`). + dr_plat::crash::set_state_dir(dir.join("state")); dr_sync::account::set_data_dir(dir); } None => log::error!("no internal data path; settings will not persist"), diff --git a/apps/darkroom-desktop/Cargo.toml b/apps/darkroom-desktop/Cargo.toml index 238b7f6..630f2f2 100644 --- a/apps/darkroom-desktop/Cargo.toml +++ b/apps/darkroom-desktop/Cargo.toml @@ -7,6 +7,10 @@ license.workspace = true [dependencies] dr-ui.workspace = true +# For the panic hook alone. Directly rather than through dr-ui, because it has +# to be installed before `dr_ui::run` — a panic during startup is exactly the +# one this exists to catch. +dr-plat.workspace = true anyhow.workspace = true env_logger.workspace = true log.workspace = true diff --git a/apps/darkroom-desktop/src/main.rs b/apps/darkroom-desktop/src/main.rs index 05713c8..149a5ff 100644 --- a/apps/darkroom-desktop/src/main.rs +++ b/apps/darkroom-desktop/src/main.rs @@ -10,6 +10,14 @@ fn main() -> anyhow::Result<()> { )) .init(); + // Immediately after the logger and before anything that could fail. Until + // now a panic on desktop went to stderr and died with the terminal, which + // means every panic a user has ever hit was unreportable: the process + // survives (the panicking worker does not), a control goes dead, and there + // is nothing on disk to say why. The record is local and stays local — + // there is no upload path, by design; see `dr_plat::crash`. + dr_plat::crash::install(env!("CARGO_PKG_VERSION")); + log::info!("DarkRoom v{}", env!("CARGO_PKG_VERSION")); let paths: Vec = std::env::args().skip(1).map(PathBuf::from).collect(); diff --git a/platform/dr-plat/src/crash.rs b/platform/dr-plat/src/crash.rs new file mode 100644 index 0000000..654ab3c --- /dev/null +++ b/platform/dr-plat/src/crash.rs @@ -0,0 +1,620 @@ +//! TRACES: NFR-OPS-2 | NFR-SEC-2 +//! Local crash capture. +//! +//! # What this is, and the half it deliberately is not +//! +//! NFR-OPS-2 is two sentences: *local crash capture always; upload only on +//! explicit opt-in.* Only the first is built here, and the second is not +//! half-built either — there is no upload path, no endpoint, no queue and no +//! "send this later" flag. Opt-in upload needs a server to receive it and a +//! consent flow that states what leaves the device (NFR-SEC-4, and the same +//! preview-and-consent step NFR-OPS-1 requires of the diagnostics bundle); +//! neither exists, and a transport built ahead of the consent is exactly the +//! shape of thing that later gets switched on by default. +//! +//! So a crash record is a file on the user's own disk. Nothing reads it but a +//! person. +//! +//! # Why a panic is worth writing down at all +//! +//! NFR-ARCH-4 says no worker error may panic the process, and the application +//! is built that way — errors are typed and attached to the image or job they +//! belong to. A panic is therefore, by construction, a *bug*: an invariant +//! this codebase believed and got wrong. Before this, one of those was +//! invisible on desktop (stderr, discarded with the terminal) and one line of +//! `log::error!` on Android. What the user saw was a job that stopped, or a +//! channel that closed and a control that went dead, with nothing to report. +//! +//! # What goes in a record, and what may never +//! +//! The rule the content is chosen under is NFR-SEC-2 — credentials never reach +//! logs or plain files — extended to the thing this application is actually +//! about: **a user's library is private, and its shape is private too.** A +//! path is not a neutral technical detail here. `/home/anna/Photos/2019 +//! Divorce/` names something about a person, and a crash record is a file that +//! gets attached to a bug report by someone trying to be helpful. +//! +//! Hence [`redact`], which is applied to the panic message *and* to the +//! backtrace before either is written, and which is deliberately blunt: it +//! removes anything that looks like a path or a URL, keeping only the basename +//! of `.rs` files so a backtrace is still readable. Over-redaction costs +//! legibility; under-redaction costs a user something they cannot take back. +//! +//! NFR-SEC-5 — face data never enters a diagnostics bundle or crash report +//! "under any configuration" — is met structurally rather than by filtering: +//! this module reads no catalog, opens no image, and touches no account. A +//! record is assembled from the panic hook's own arguments and from +//! [`std::env::consts`], and there is no code path from here to an embedding, +//! a crop, or a cluster. The message length cap is the backstop for the +//! remaining case — a panic payload that some *other* module formatted a large +//! value into. +//! +//! # What this leaves cheaper for NFR-OPS-1 +//! +//! The rotating on-disk log is unbuilt, and it wants three things that are +//! here: [`state_dir`] (the XDG state directory, resolved once, overridable +//! for Android where XDG does not exist), [`redact`] (NFR-OPS-1's "automatic +//! redaction of credentials and tokens" is the same function), and `prune` +//! (size-capped rotation is this counting files instead of bytes). A log +//! belongs at `state_dir().join("log")` beside `crash/`, and the diagnostics +//! bundle then has one directory to collect. + +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; +use std::time::{SystemTime, UNIX_EPOCH}; + +/// How many crash records are kept, newest first. +/// +/// Ten because the useful pattern in a crash record is usually a *repeat* — +/// the same panic three launches running is a far stronger report than one — +/// and because these are a few kilobytes each, so the cap exists to stop a +/// crash loop filling a disk rather than to save space. +pub const KEEP_RECORDS: usize = 10; + +/// The longest panic message written to a record. +/// +/// A backstop, not a redaction: [`redact`] handles what must not be written at +/// all. This bounds what a panic that formatted something enormous into its +/// message — a decoded buffer, a `Vec` of embeddings — can put on disk. +const MAX_MESSAGE: usize = 2000; + +/// Android's per-app directory, once the entry point has said what it is. +static STATE_DIR: OnceLock = OnceLock::new(); + +/// Declare the directory this application may keep state in. +/// +/// Only Android needs to call this, and it must call it before a crash rather +/// than before the hook is installed — [`install`] resolves the directory at +/// crash time precisely so that the hook can go in first, covering the startup +/// it would otherwise miss. Neither `XDG_STATE_HOME` nor `HOME` is set there, +/// and the fallback would resolve to a path the app cannot write. +/// +/// Later calls are ignored rather than racing, matching +/// `dr_sync::account::set_data_dir`, which the same entry point calls for the +/// same reason. +pub fn set_state_dir(dir: PathBuf) { + let _ = STATE_DIR.set(dir); +} + +/// Where this application keeps state that is neither configuration nor cache. +/// +/// `$XDG_STATE_HOME/darkroom`, falling back to `~/.local/state/darkroom`. +/// State rather than cache because a crash record must survive the sweep that +/// a cache directory exists to permit, and rather than config because it is +/// not something the user edits. +pub fn state_dir() -> PathBuf { + if let Some(d) = STATE_DIR.get() { + return d.clone(); + } + std::env::var_os("XDG_STATE_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| { + PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".local/state") + }) + .join("darkroom") +} + +/// Where crash records are written. +pub fn crash_dir() -> PathBuf { + state_dir().join("crash") +} + +/// Install the panic hook. +/// +/// Call once, as early as the entry point can — before the window, before any +/// store, before anything that could itself panic. The directory is resolved +/// lazily inside the hook, so installing this before +/// [`set_state_dir`] is correct rather than merely tolerated. +/// +/// The previously installed hook still runs afterwards. On desktop that is the +/// standard library's, which prints the panic to stderr, and a developer +/// watching a terminal should not lose that because the application started +/// writing files. stderr is the one surface that sees the message unredacted, +/// which is a considered exception: it is ephemeral, local, and never attached +/// to a bug report. +pub fn install(app_version: &str) { + let version = app_version.to_string(); + let previous = std::panic::take_hook(); + + std::panic::set_hook(Box::new(move |info| { + // A panic inside a panic hook aborts the process, which would replace + // a diagnosable crash with an undiagnosable one. Everything below is + // written to be infallible, and this is the admission that "written to + // be" is not the same as "is". + let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let record = compose(&version, info); + // Redacted, because this is a log file and NFR-SEC-2 governs it. + // The unredacted form goes to stderr below, through the hook this + // one chained onto. + log::error!("panic: {}", record.summary); + match write_record(&crash_dir(), &record) { + // The name, not the path: the log this line lands in is a + // sibling of the record, and printing the directory would put + // the user's home in a file they may hand to someone. + Ok(path) => log::error!( + "crash record written: {}", + path.file_name().unwrap_or_default().to_string_lossy() + ), + Err(e) => log::error!("could not write a crash record: {e}"), + } + })); + + previous(info); + })); +} + +/// The crash records on disk, newest first. +/// +/// For a future diagnostics bundle, and for a person looking for the file to +/// attach to a report. +pub fn records() -> Vec { + let Ok(entries) = std::fs::read_dir(crash_dir()) else { + return Vec::new(); + }; + let mut out: Vec<(i64, PathBuf)> = entries + .flatten() + .filter_map(|e| { + let path = e.path(); + Some((timestamp_of(&path)?, path)) + }) + .collect(); + out.sort_by_key(|(when, _)| std::cmp::Reverse(*when)); + out.into_iter().map(|(_, p)| p).collect() +} + +/// One crash, formatted. +struct Record { + /// The whole file. + body: String, + /// One redacted line, for the log. + summary: String, + when: i64, +} + +/// Build a record from what the panic hook was handed. +/// +/// Split from the writing so the *content* rules — what is included, what is +/// redacted, what is capped — are testable without a filesystem, and so a +/// future diagnostics bundle can reuse the same composition. +fn compose(version: &str, info: &std::panic::PanicHookInfo<'_>) -> Record { + let payload = info.payload(); + let message = payload + .downcast_ref::<&str>() + .copied() + .or_else(|| payload.downcast_ref::().map(|s| s.as_str())) + // A panic can carry any `Any`, and `panic_any` is used by some + // libraries. There is nothing to print, and saying so is better than + // an empty field that reads like a bug in this code. + .unwrap_or("(panic payload was not a string)"); + + let message = redact(&truncate(message, MAX_MESSAGE)); + let at = info + .location() + .map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column())) + // `location()` is a compile-time source path, not one of the user's, + // but it goes through the same redaction: a dependency built from a + // registry checkout carries the *builder's* home directory in it. + .map(|s| redact(&s)) + .unwrap_or_else(|| "unknown".to_string()); + let thread = std::thread::current() + .name() + .unwrap_or("unnamed") + .to_string(); + + let when = now(); + let summary = format!("{message} (at {at}, thread {thread})"); + + let body = format!( + "darkroom-crash 1\n\ + version: {version}\n\ + when: {when}\n\ + os: {}\n\ + arch: {}\n\ + thread: {thread}\n\ + at: {at}\n\ + message: {message}\n\ + backtrace:\n{}\n", + std::env::consts::OS, + std::env::consts::ARCH, + redact(&std::backtrace::Backtrace::force_capture().to_string()), + ); + + Record { + body, + summary, + when, + } +} + +/// Write one record into `dir`, then prune. +/// +/// Takes the directory rather than calling [`crash_dir`] so a test can drive +/// the real writing path without an environment variable and without a +/// `OnceLock` it cannot reset. +fn write_record(dir: &Path, record: &Record) -> std::io::Result { + std::fs::create_dir_all(dir)?; + // The pid distinguishes two processes crashing in the same second, which + // is not hypothetical: a background export and the app can be separate + // processes on desktop, and a crash loop retries fast. + let path = dir.join(format!("crash-{}-{}.txt", record.when, std::process::id())); + std::fs::write(&path, &record.body)?; + prune(dir, KEEP_RECORDS); + Ok(path) +} + +/// Delete all but the `keep` newest records in `dir`. +/// +/// Best-effort: failing to delete an old record is no reason to lose the new +/// one, which is already on disk. +fn prune(dir: &Path, keep: usize) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + let mut found: Vec<(i64, PathBuf)> = entries + .flatten() + .filter_map(|e| { + let path = e.path(); + Some((timestamp_of(&path)?, path)) + }) + .collect(); + found.sort_by_key(|(when, _)| std::cmp::Reverse(*when)); + for (_, path) in found.into_iter().skip(keep) { + let _ = std::fs::remove_file(path); + } +} + +/// Pull the timestamp back out of a record's filename. +/// +/// Doubles as the filter that keeps anything else in the directory — an +/// editor's backup file, a log — from being counted as a crash record or +/// pruned as one. +fn timestamp_of(path: &Path) -> Option { + let name = path.file_name()?.to_str()?; + let rest = name.strip_prefix("crash-")?.strip_suffix(".txt")?; + rest.split('-').next()?.parse().ok() +} + +/// Remove from `text` everything that would say something about the user. +/// +/// # The rule, and why it is this blunt one +/// +/// A token containing `/` is treated as a path or a URL and removed. Rust +/// source files are the exception and keep their basename, because a backtrace +/// with no filenames is close to useless and `library.rs:1270` says nothing +/// about anybody. A token that reads like a secret — a long opaque run, or the +/// value beside a word like `password` — is removed regardless of shape. +/// +/// Blunter than a list of known-sensitive shapes on purpose. The cost of +/// over-redaction is a diagnostic that is harder to read; the cost of +/// under-redaction is a directory listing of somebody's photographs in a file +/// they may attach to a public bug report. Those are not comparable, and this +/// is not the place to be clever about the boundary. +/// +/// Known limits, stated rather than hidden: a path with no `/` in it (a bare +/// filename) survives, and a person's name that some *other* module formatted +/// into a panic message survives. Both are bounded by the fact that nothing +/// here reads the catalog — see the module documentation — and the second is +/// why `MAX_MESSAGE` exists. +pub fn redact(text: &str) -> String { + let mut out: Vec = Vec::new(); + let mut redact_next = false; + + for token in text.split_inclusive(char::is_whitespace) { + // Whitespace is preserved exactly — a backtrace is read as a shape as + // much as as text — so the classification runs on the token without + // its trailing space and the space is put back. + let trailing: String = token + .chars() + .skip_while(|c| !c.is_whitespace()) + .collect::(); + let word = &token[..token.len() - trailing.len()]; + + if word.is_empty() { + out.push(token.to_string()); + continue; + } + + let replacement = if redact_next { + Some("".to_string()) + } else { + classify(word) + }; + redact_next = names_a_secret(word); + + out.push(match replacement { + Some(r) => format!("{r}{trailing}"), + None => token.to_string(), + }); + } + + out.join("") +} + +/// What one token should be replaced with, or `None` to keep it. +fn classify(word: &str) -> Option { + // `key=value` and `key: value` written as one token. Split at the first + // separator so `Authorization:Bearer` loses the half that matters. + if let Some((head, tail)) = word.split_once(['=', ':']) { + if names_a_secret(head) && !tail.is_empty() { + return Some(format!("{head}=")); + } + } + + // Strip the punctuation a sentence puts around a path — "opening + // /home/x/y.CR3:" — so the classification sees the path itself, then put + // nothing back: the punctuation is not worth the complexity of restoring + // it around a placeholder. + let bare = word.trim_matches(|c: char| matches!(c, '"' | '\'' | '(' | ')' | ',' | ';' | ':')); + + if bare.contains('/') { + // A Rust source path keeps its basename. Line and column are part of + // the same token in a backtrace ("src/library.rs:1270:9"), and they + // are kept: they are facts about this codebase. + if let Some(rs) = rust_source_tail(bare) { + return Some(format!("…/{rs}")); + } + return Some("".to_string()); + } + if bare.starts_with('~') { + return Some("".to_string()); + } + if looks_opaque(bare) { + return Some("".to_string()); + } + None +} + +/// The `library.rs:1270:9` tail of a path naming a Rust source file. +fn rust_source_tail(word: &str) -> Option<&str> { + let tail = word.rsplit('/').next()?; + // The extension is followed by `:line:col` in a backtrace and by nothing + // in a `Location`, so match on the extension rather than on the end. + if tail.contains(".rs") { + Some(tail) + } else { + None + } +} + +/// Whether this word introduces a value that must not be written down. +fn names_a_secret(word: &str) -> bool { + let w = word + .trim_matches(|c: char| !c.is_alphanumeric() && c != '_' && c != '-') + .to_ascii_lowercase(); + matches!( + w.as_str(), + "password" + | "passwd" + | "app-password" + | "app_password" + | "token" + | "secret" + | "bearer" + | "authorization" + | "apikey" + | "api-key" + | "api_key" + | "credential" + | "credentials" + | "cookie" + ) +} + +/// Whether a word looks like a key rather than like prose. +/// +/// A long run of the characters secrets are encoded in, containing both a +/// letter and a digit — which is what an app password, a bearer token or a +/// base64 blob looks like, and what an English word or a Rust identifier does +/// not. +fn looks_opaque(word: &str) -> bool { + word.len() >= 20 + && word + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '_' | '-' | '=')) + && word.chars().any(|c| c.is_ascii_digit()) + && word.chars().any(|c| c.is_ascii_alphabetic()) +} + +/// Cut `s` to `max` bytes on a character boundary, saying that it was cut. +fn truncate(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + format!("{}… ({} bytes truncated)", &s[..end], s.len() - end) +} + +/// Seconds since the epoch, or 0 if the clock is before it. +fn now() -> i64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_users_photograph_never_reaches_a_record() { + let r = redact("failed to open /home/anna/Photos/2019 Divorce/IMG_0042.CR3"); + assert!(!r.contains("anna"), "{r}"); + assert!(!r.contains("IMG_0042"), "{r}"); + assert!(!r.contains("Divorce"), "{r}"); + assert!(r.contains("failed to open"), "the diagnosis was lost: {r}"); + } + + #[test] + fn an_android_document_uri_is_a_path_too() { + // SAF hands out `content://` URIs and `primary:DCIM/...` refs rather + // than paths, and both name the user's library just as precisely. + let r = redact("no such image: content://com.android.providers/tree/primary%3ADCIM"); + assert!(!r.contains("DCIM"), "{r}"); + let r = redact("source_ref primary:DCIM/Camera/IMG_1.CR3 missing"); + assert!(!r.contains("IMG_1"), "{r}"); + } + + #[test] + fn a_server_url_is_removed_because_it_names_the_user() { + // A Nextcloud URL is the user's own server, often with their login in + // the path. NFR-SEC-3 is about the wire; this is about the disk. + let r = redact("PROPFIND https://cloud.example.org/remote.php/dav/files/anna/ failed"); + assert!(!r.contains("cloud.example.org"), "{r}"); + assert!(!r.contains("anna"), "{r}"); + assert!(r.contains("PROPFIND"), "{r}"); + } + + #[test] + fn a_credential_never_reaches_a_record() { + // NFR-SEC-2, which forbids credentials in logs and plain files. Both + // spellings: the value beside a naming word, and the value alone. + let r = redact("auth failed: password hunter2correcthorse"); + assert!(!r.contains("hunter2correcthorse"), "{r}"); + + let r = redact("Authorization: Bearer aGVsbG90aGVyZTEyMzQ1Njc4OTA="); + assert!(!r.contains("aGVsbG90aGVyZTEyMzQ1Njc4OTA"), "{r}"); + + let r = redact("rejected app-password=abcde-fghij-12345-klmno-pqrst"); + assert!(!r.contains("abcde-fghij"), "{r}"); + } + + #[test] + fn a_bare_secret_is_caught_by_its_shape() { + // Nextcloud app passwords arrive with no label at all when they are + // interpolated into a message by a library this codebase does not own. + let r = redact("login failed for aBcDe1FgHiJ2kLmNo3PqRsT4uV"); + assert!(!r.contains("aBcDe1FgHiJ"), "{r}"); + } + + #[test] + fn a_backtrace_keeps_the_frames_that_make_it_readable() { + // The whole reason the `.rs` exception exists: redacting these to + // `` leaves a backtrace of nothing but symbol names, and the + // line number is a fact about this codebase rather than about anyone. + let r = redact(" 3: dr_ui::library::run_scan\n at ./ui/dr-ui/src/library.rs:1270:9\n"); + assert!(r.contains("library.rs:1270:9"), "{r}"); + assert!(r.contains("dr_ui::library::run_scan"), "{r}"); + assert!(!r.contains("ui/dr-ui/src"), "{r}"); + // And the shape survives, because a backtrace is read as a shape. + assert!(r.contains('\n'), "{r}"); + assert!(r.starts_with(" 3:"), "{r}"); + } + + #[test] + fn ordinary_words_are_left_alone() { + // Over-redaction has a cost too: a record that says nothing is not + // safer, it is just useless. + let msg = "assertion failed: tier_desired was 3, expected 2"; + assert_eq!(redact(msg), msg); + } + + #[test] + fn an_enormous_payload_is_capped() { + // A panic that formatted a decoded buffer — or, the case NFR-SEC-5 + // cares about, an embedding — into its message. + let huge = "9".repeat(MAX_MESSAGE * 3); + let cut = truncate(&huge, MAX_MESSAGE); + assert!(cut.len() < huge.len()); + assert!(cut.contains("truncated"), "{cut}"); + } + + #[test] + fn truncation_does_not_split_a_character() { + let s = "é".repeat(100); + // 3 is mid-character for a 2-byte encoding. + let cut = truncate(&s, 3); + assert!(cut.starts_with('é')); + } + + #[test] + fn records_are_written_and_rotated() { + let dir = std::env::temp_dir().join(format!("dr-crash-test-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + + for i in 0..(KEEP_RECORDS as i64 + 5) { + write_record( + &dir, + &Record { + body: format!("darkroom-crash 1\nmessage: {i}\n"), + summary: String::new(), + // Distinct seconds, or the pid-suffixed names would + // collide and the rotation would have nothing to count. + when: 1_000 + i, + }, + ) + .unwrap(); + } + + let kept: Vec = std::fs::read_dir(&dir) + .unwrap() + .flatten() + .map(|e| e.path()) + .collect(); + assert_eq!(kept.len(), KEEP_RECORDS); + // The newest survive, not the oldest. + let newest = kept.iter().filter_map(|p| timestamp_of(p)).max(); + let oldest = kept.iter().filter_map(|p| timestamp_of(p)).min(); + assert_eq!(newest, Some(1_000 + KEEP_RECORDS as i64 + 4)); + assert_eq!(oldest, Some(1_000 + 5)); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_stray_file_is_neither_listed_nor_pruned() { + let dir = std::env::temp_dir().join(format!("dr-crash-stray-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("darkroom.log"), b"not a crash").unwrap(); + + for i in 0..(KEEP_RECORDS as i64 + 5) { + write_record( + &dir, + &Record { + body: String::new(), + summary: String::new(), + when: 2_000 + i, + }, + ) + .unwrap(); + } + + assert!(dir.join("darkroom.log").is_file(), "the log was pruned"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn the_state_directory_is_neither_config_nor_cache() { + // NFR-OPS-1 puts the log here too, and NFR-OPS-3 keeps preferences + // separate from both. The distinction is what stops a crash record + // being swept away by the thing that is allowed to sweep caches. + let dir = state_dir(); + let s = dir.to_string_lossy(); + assert!(s.ends_with("darkroom"), "{s}"); + assert!(!s.contains("/cache"), "{s}"); + } +} diff --git a/platform/dr-plat/src/lib.rs b/platform/dr-plat/src/lib.rs index 808ed80..77f0b68 100644 --- a/platform/dr-plat/src/lib.rs +++ b/platform/dr-plat/src/lib.rs @@ -4,6 +4,13 @@ //! construction, so `core/` contains no `#[cfg(target_os)]` (NFR-PORT-1, //! ARCH §10). +// Not a trait, and the one module here that is not. It belongs in this crate +// for the same reason the traits do: "where does this platform let an +// application keep state" is a platform question, and both entry points need +// the answer before either has a window. Resolving it in `apps/` would mean +// writing it twice, once per platform, which is the arrangement this crate +// exists to prevent. +pub mod crash; pub mod display; pub mod secrets; pub mod storage;