Add input validation for web endpoints
Introduces a new `validate` module to centralize input validation for web requests. This module contains functions to check the shape and format of various user-supplied strings before they are processed by the main application logic. Specifically, this commit adds validation for: - Login slugs: Ensures slugs conform to a strict pattern (lowercase alphanumerics, `_`, `-`) to prevent path traversal and other attacks. - Magic link tokens: Validates token length and character set to ensure they are URL-safe and within expected bounds. - Recorded at timestamps: Enforces a strict RFC3339 format with second granularity (`YYYY-MM-DDTHH:MM:SSZ`) to prevent filename parsing issues and ensure consistent data grouping. These validators are integrated into the `handle_login_submit`, `handle_consume` (magic link), and `handle_upload` routes. This change enhances security by preventing malformed inputs from reaching sensitive parts of the application and improves robustness by catching data format errors early. Unit and integration tests have been added to verify the functionality of these validators and their integration into the respective endpoints.
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -38,7 +38,19 @@ pub async fn handle_login_submit(
|
||||
jar: CookieJar,
|
||||
Form(form): Form<LoginForm>,
|
||||
) -> Result<Response, AppError> {
|
||||
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);
|
||||
|
||||
@@ -103,6 +103,15 @@ pub async fn handle_consume(
|
||||
jar: CookieJar,
|
||||
Query(q): Query<ConsumeQuery>,
|
||||
) -> 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)
|
||||
|
||||
@@ -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?;
|
||||
|
||||
|
||||
@@ -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<T>` / `Form<T>` /
|
||||
//! `Query<T>` 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 (`<data_path>/<slug>/`), 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());
|
||||
}
|
||||
}
|
||||
@@ -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<u8>) -> Request<Body> {
|
||||
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);
|
||||
}
|
||||
Reference in New Issue
Block a user