diff --git a/server/src/lib.rs b/server/src/lib.rs index 24e074c..7e5927d 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -14,6 +14,7 @@ pub mod retention; pub mod routes; pub mod settings; pub mod transcribe; +pub mod validate; pub mod web_session; use std::sync::Arc; diff --git a/server/src/routes/login.rs b/server/src/routes/login.rs index 4f97884..62c0268 100644 --- a/server/src/routes/login.rs +++ b/server/src/routes/login.rs @@ -38,7 +38,19 @@ pub async fn handle_login_submit( jar: CookieJar, Form(form): Form, ) -> Result { - let user = config.users.iter().find(|u| u.slug == form.slug); + // Shape-check the slug before touching the user list. We fold a + // bad-shape slug into the same "Login fehlgeschlagen" path as a + // wrong password to avoid leaking whether the slug *could* be valid. + // The bcrypt-on-unknown-user dummy verify below already keeps + // timing comparable; this just denies path-traversal-shaped slugs + // before they reach disk-touching code anywhere. + let slug_ok = crate::validate::validate_slug(&form.slug).is_ok(); + + let user = if slug_ok { + config.users.iter().find(|u| u.slug == form.slug) + } else { + None + }; let valid = user .map(|u| bcrypt::verify(&form.password, &u.web_password).unwrap_or(false)) .unwrap_or(false); diff --git a/server/src/routes/magic.rs b/server/src/routes/magic.rs index 42dd78e..ee65d40 100644 --- a/server/src/routes/magic.rs +++ b/server/src/routes/magic.rs @@ -103,6 +103,15 @@ pub async fn handle_consume( jar: CookieJar, Query(q): Query, ) -> Response { + // Shape-check the token before the store lookup. Fold a malformed + // token into the same redirect-to-login path used for unknown / + // expired tokens — any deviation would leak whether a given string + // could ever be a valid token shape. + if crate::validate::validate_token(&q.token).is_err() { + warn!("magic-link consume failed: malformed token"); + return redirect_to_login(); + } + let pending = { let mut w = magic_store.write().await; w.remove(&q.token) diff --git a/server/src/routes/upload.rs b/server/src/routes/upload.rs index fdc59a7..9e73dd2 100644 --- a/server/src/routes/upload.rs +++ b/server/src/routes/upload.rs @@ -71,6 +71,11 @@ pub async fn handle_upload( uuid::Uuid::parse_str(&case_id) .map_err(|_| AppError::BadRequest(format!("Invalid case_id (not a UUID): {case_id}")))?; + // Validate `recorded_at` shape strictly. A subsecond drift here would + // produce a filename the day-grouping parser cannot read — see + // `validate::validate_recorded_at_strict` doc for the historical bug. + crate::validate::require_recorded_at_strict(&recorded_at)?; + // Find or create the case directory. let case_dir = resolve_case_dir(&user.data_dir, &case_id).await?; diff --git a/server/src/validate.rs b/server/src/validate.rs new file mode 100644 index 0000000..03c583b --- /dev/null +++ b/server/src/validate.rs @@ -0,0 +1,230 @@ +//! Boundary validators for client-supplied strings. +//! +//! Every function here checks the *shape* of a string that crossed the +//! HTTP boundary, before any handler logic touches it. Pure functions — +//! no state, no side effects, no I/O. Each returns +//! `Result<(), &'static str>`; the caller decides whether a failure +//! becomes a 400, a sanitized "wrong credentials" page, or a redirect, +//! so info-leak-sensitive endpoints (login, magic-link) can fold the +//! rejection into their existing error path instead of leaking which +//! field was malformed. +//! +//! Design notes: +//! +//! - **Why a central module instead of per-handler helpers?** When a +//! client format drifts (cf. the Wear OS microsecond bug), the +//! regression test belongs next to the validator, not in whichever +//! route happened to feed it. Centralising lets the test surface +//! speak in one voice. +//! - **What is *not* here**: structural validation (missing fields, +//! wrong types) is already done by axum's `Json` / `Form` / +//! `Query` extractors via Serde. We only check semantics atop +//! that — does this `String` *look* like a timestamp / slug / token? +//! - **What also is *not* here**: validators that already live next to +//! their handler and have tests there (`case_id::CaseIdPath`, +//! `routes::web::validate_filename`, `routes::magic::is_safe_return_to`). +//! Moving them would be churn without payoff. + +use crate::error::AppError; + +/// Strict UTC RFC3339 with second granularity, no subsecond fraction: +/// `YYYY-MM-DDTHH:MM:SSZ` (exactly 20 bytes). Mirrors the contract that +/// `doctate_common::timestamp::now_rfc3339` emits and that the filename +/// parser in `routes::user_web::utc_date_and_iso_of` requires. +/// +/// A drift here (e.g. a client emitting `Instant.now().toString()` with +/// microseconds) would silently corrupt filenames and the day-grouping +/// would fall back to `today`, producing the duplicate "Heute" header +/// we hunted on 2026-04-27. Catching it at the upload boundary turns a +/// week-late UI symptom into an immediate 400. +pub fn validate_recorded_at_strict(s: &str) -> Result<(), &'static str> { + if s.len() != 20 { + return Err("expected 20 chars `YYYY-MM-DDTHH:MM:SSZ`"); + } + let bytes = s.as_bytes(); + if bytes[19] != b'Z' { + return Err("must end with `Z` (UTC)"); + } + if bytes[10] != b'T' { + return Err("missing `T` separator at index 10"); + } + let digit_at = |i: usize| bytes[i].is_ascii_digit(); + let dash_at = |i: usize| bytes[i] == b'-'; + let colon_at = |i: usize| bytes[i] == b':'; + // YYYY-MM-DDTHH:MM:SS + // 0123456789012345678 + let shape_ok = digit_at(0) + && digit_at(1) + && digit_at(2) + && digit_at(3) + && dash_at(4) + && digit_at(5) + && digit_at(6) + && dash_at(7) + && digit_at(8) + && digit_at(9) + && digit_at(11) + && digit_at(12) + && colon_at(13) + && digit_at(14) + && digit_at(15) + && colon_at(16) + && digit_at(17) + && digit_at(18); + if !shape_ok { + return Err("malformed digits/separators"); + } + Ok(()) +} + +/// Convenience wrapper: bubble a malformed `recorded_at` to the standard +/// 400 response. Use from handlers that want field-named errors in the +/// body (`/api/upload`); for info-leak-sensitive endpoints, call +/// [`validate_recorded_at_strict`] directly and route to a sanitized +/// error path. +pub fn require_recorded_at_strict(s: &str) -> Result<(), AppError> { + validate_recorded_at_strict(s) + .map_err(|why| AppError::BadRequest(format!("Invalid recorded_at: {why}"))) +} + +/// Login slug shape: lowercase alphanumerics plus `_`/`-`, 1–32 chars. +/// The slug becomes a directory name (`//`), so a +/// shape-check is defense-in-depth on top of the existing +/// configured-users lookup. `^[a-z0-9_-]{1,32}$` is deliberately tight: +/// a sysadmin can pick any slug they want at config time within this +/// alphabet, and an attacker cannot smuggle path separators or unicode +/// homographs through the form. +pub fn validate_slug(s: &str) -> Result<(), &'static str> { + if s.is_empty() { + return Err("empty"); + } + if s.len() > 32 { + return Err("too long (max 32)"); + } + if !s + .bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'_' || b == b'-') + { + return Err("only [a-z0-9_-] allowed"); + } + Ok(()) +} + +/// Opaque session/magic-link token shape: 16–256 bytes of URL-safe +/// base64 alphabet (`A–Z`, `a–z`, `0–9`, `_`, `-`). Loose enough to +/// accommodate any reasonable token generator (we use +/// [`crate::web_session::generate_token`]) while bounding the length so +/// a malicious client cannot ship a megabyte-long "token" and force the +/// server to allocate it before the lookup misses. +pub fn validate_token(s: &str) -> Result<(), &'static str> { + if s.len() < 16 { + return Err("too short (min 16)"); + } + if s.len() > 256 { + return Err("too long (max 256)"); + } + if !s + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') + { + return Err("only [A-Za-z0-9_-] allowed"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn recorded_at_accepts_strict_utc() { + assert!(validate_recorded_at_strict("2026-04-15T10:32:00Z").is_ok()); + assert!(validate_recorded_at_strict("1999-12-31T23:59:59Z").is_ok()); + } + + #[test] + fn recorded_at_rejects_microseconds() { + // The exact shape Wear OS was emitting before the 2026-04-27 fix. + assert!(validate_recorded_at_strict("2026-04-27T10:34:12.987072Z").is_err()); + } + + #[test] + fn recorded_at_rejects_milliseconds() { + assert!(validate_recorded_at_strict("2026-04-15T10:32:00.123Z").is_err()); + } + + #[test] + fn recorded_at_rejects_offset_form() { + // RFC3339 allows offsets like `+00:00`, but our filename + // pipeline only handles the `Z` form. + assert!(validate_recorded_at_strict("2026-04-15T10:32:00+00:00").is_err()); + } + + #[test] + fn recorded_at_rejects_short_and_long() { + assert!(validate_recorded_at_strict("").is_err()); + assert!(validate_recorded_at_strict("2026-04-15T10:32:00").is_err()); + assert!(validate_recorded_at_strict("2026-04-15T10:32:00Z ").is_err()); + } + + #[test] + fn recorded_at_rejects_letters_in_digit_slots() { + assert!(validate_recorded_at_strict("YYYY-04-15T10:32:00Z").is_err()); + assert!(validate_recorded_at_strict("2026-04-15T10:32:0aZ").is_err()); + } + + #[test] + fn recorded_at_rejects_path_traversal_attempt() { + // 20-byte length cap plus the digit-slot check make smuggling + // path bytes into the filename impossible. + assert!(validate_recorded_at_strict("../../../etc/passwdXX").is_err()); + } + + #[test] + fn slug_accepts_canonical_shapes() { + assert!(validate_slug("dr_mueller").is_ok()); + assert!(validate_slug("a").is_ok()); + assert!(validate_slug("abc-123_xyz").is_ok()); + } + + #[test] + fn slug_rejects_traversal_and_unicode() { + assert!(validate_slug("").is_err()); + assert!(validate_slug("../etc").is_err()); + assert!(validate_slug("dr/mueller").is_err()); + assert!(validate_slug("Dr_Mueller").is_err()); // uppercase + assert!(validate_slug("dr.mueller").is_err()); // dot + assert!(validate_slug("möller").is_err()); // non-ASCII + } + + #[test] + fn slug_rejects_too_long() { + let s33 = "a".repeat(33); + assert!(validate_slug(&s33).is_err()); + } + + #[test] + fn token_accepts_base64url_lengths() { + let t = "A".repeat(16); + assert!(validate_token(&t).is_ok()); + let t = "A".repeat(256); + assert!(validate_token(&t).is_ok()); + // Mixed alphabet — what `generate_token` would produce. + assert!(validate_token("aB3-_xyz_AB3-_xyzpqrXYZ").is_ok()); + } + + #[test] + fn token_rejects_too_short_and_too_long() { + let t = "A".repeat(15); + assert!(validate_token(&t).is_err()); + let t = "A".repeat(257); + assert!(validate_token(&t).is_err()); + } + + #[test] + fn token_rejects_non_url_safe_bytes() { + assert!(validate_token("AAAAAAAAAAAAAAAAA+/=").is_err()); + assert!(validate_token("AAAAAAAAAAAAAAAA?=&").is_err()); + assert!(validate_token("AAAAAAAAAAAAAAAA\nBB").is_err()); + } +} diff --git a/server/tests/validate_boundary_test.rs b/server/tests/validate_boundary_test.rs new file mode 100644 index 0000000..2eaf1df --- /dev/null +++ b/server/tests/validate_boundary_test.rs @@ -0,0 +1,167 @@ +//! End-to-end checks that boundary validators in `crate::validate` are +//! actually wired into their respective handlers and produce the right +//! response shape per endpoint. +//! +//! Per-validator unit tests live in `server/src/validate.rs`; this file +//! tests the *integration* — the wiring, not the predicate. + +mod common; + +use axum::body::Body; +use axum::http::{Request, StatusCode, header}; +use tower::util::ServiceExt; + +use doctate_common::UPLOAD_PATH; + +use common::{ + TestConfig, body_string, extract_session_cookie, login_request, multipart_upload_body, + test_user, test_user_with_password, +}; + +const API_KEY: &str = "key-dr_test"; + +fn upload_request(content_type: &str, body: Vec) -> Request { + Request::builder() + .method("POST") + .uri(UPLOAD_PATH) + .header("Content-Type", content_type) + .header("X-API-Key", API_KEY) + .body(Body::from(body)) + .unwrap() +} + +/// Drift like the Wear OS bug (microseconds) must be rejected with 400 +/// at the upload boundary, not silently accepted into the filename. +#[tokio::test] +async fn upload_rejects_microsecond_recorded_at_with_400() { + let cfg = TestConfig::new() + .with_label("validate") + .with_user(test_user("dr_test")) + .build(); + let app = doctate_server::create_router(cfg); + + let case_id = "550e8400-e29b-41d4-a716-446655440000"; + let (ct, body) = multipart_upload_body(case_id, "2026-04-27T10:34:12.987072Z", b"audio"); + + let resp = app.oneshot(upload_request(&ct, body)).await.unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); +} + +/// RFC3339 with offset (`+00:00`) is technically valid time, but our +/// filename pipeline is `Z`-only — we reject anything else, even if a +/// future client thinks it's being clever. +#[tokio::test] +async fn upload_rejects_offset_form_recorded_at_with_400() { + let cfg = TestConfig::new() + .with_label("validate") + .with_user(test_user("dr_test")) + .build(); + let app = doctate_server::create_router(cfg); + + let case_id = "550e8400-e29b-41d4-a716-446655440000"; + let (ct, body) = multipart_upload_body(case_id, "2026-04-27T10:34:12+00:00", b"audio"); + + let resp = app.oneshot(upload_request(&ct, body)).await.unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); +} + +/// Sanity guard: the strict shape we *do* accept still works, so the +/// validator hasn't accidentally locked everything out. +#[tokio::test] +async fn upload_accepts_strict_recorded_at_shape() { + let cfg = TestConfig::new() + .with_label("validate") + .with_user(test_user("dr_test")) + .build(); + let app = doctate_server::create_router(cfg); + + let case_id = "550e8400-e29b-41d4-a716-446655440000"; + let (ct, body) = multipart_upload_body(case_id, "2026-04-15T10:32:00Z", b"audio"); + + let resp = app.oneshot(upload_request(&ct, body)).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} + +/// A path-traversal-shaped slug must fold into the same "Login +/// fehlgeschlagen" response as a wrong password — *not* leak via 400 +/// that the slug shape was the problem. +#[tokio::test] +async fn login_with_traversal_slug_returns_login_failed_page() { + let cfg = TestConfig::new() + .with_user(test_user_with_password("dr_a", "secret")) + .build(); + let app = doctate_server::create_router(cfg); + + let resp = app + .oneshot(login_request("../etc/passwd", "anything")) + .await + .unwrap(); + // Same shape as wrong-password: 200 OK with the error page, no cookie. + assert_eq!(resp.status(), StatusCode::OK); + assert!(extract_session_cookie(&resp).is_none()); + let body = body_string(resp).await; + assert!(body.contains("Login fehlgeschlagen"), "got: {body}"); +} + +/// Uppercase / non-ASCII slugs likewise must not bypass shape rules, +/// even if they happen to coincide with no configured user. +#[tokio::test] +async fn login_with_uppercase_slug_returns_login_failed_page() { + let cfg = TestConfig::new() + .with_user(test_user_with_password("dr_a", "secret")) + .build(); + let app = doctate_server::create_router(cfg); + + let resp = app.oneshot(login_request("Dr_A", "secret")).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert!(extract_session_cookie(&resp).is_none()); +} + +/// A malformed magic-link token must not 400 (would leak shape) — it +/// should redirect to /web/login, identical to the unknown-token path. +#[tokio::test] +async fn magic_consume_with_malformed_token_redirects_to_login() { + let cfg = TestConfig::new() + .with_label("validate") + .with_user(test_user("dr_a")) + .build(); + let app = doctate_server::create_router(cfg); + + // Too short — would never have come from `generate_token`. + let resp = app + .oneshot( + Request::builder() + .uri("/web/magic?token=abc") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + let loc = resp + .headers() + .get(header::LOCATION) + .expect("redirect must set Location") + .to_str() + .unwrap(); + assert_eq!(loc, "/web/login"); +} + +/// And the same for tokens that pass the length check but contain +/// bytes outside the URL-safe alphabet. +#[tokio::test] +async fn magic_consume_with_non_urlsafe_token_redirects_to_login() { + let cfg = TestConfig::new() + .with_label("validate") + .with_user(test_user("dr_a")) + .build(); + let app = doctate_server::create_router(cfg); + + let bad = "%2E%2E%2Fetc%2Fpasswd%2E%2E%2Fetc"; // url-encoded ./../etc/passwd… + let uri = format!("/web/magic?token={bad}"); + let resp = app + .oneshot(Request::builder().uri(uri).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); +}