From ec974e19da14134a31dedbdb24e2075423e75b47 Mon Sep 17 00:00:00 2001 From: AdaWorldAPI Date: Sun, 21 Jun 2026 18:36:11 +0200 Subject: [PATCH] fix(handlers): XSS in titles/headlines + URL-encode slugs + RM_BIND fail-fast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the codex items left open across my Phase 0/1 PRs: # P1 — XSS via raw user content in handler-built HTML (#10, #12) `wrap_in_doc(title, body)` interpolated the raw `title` straight into the document's `` element. With user-controlled inputs (issue subjects, time-entry spent_on, project names, user display names) reaching this fn, a payload like `` is a stored XSS. The `render_detail(..., headline_html, ...)` parameter is contractually pre-rendered HTML (kit `|safe`s it through the template), so handlers that compose it from user data must escape themselves. W1 (Issue), W2 (Project), W3 (TimeEntry), W4a (User), W4b (Role) all did this with raw interpolation. Fix: - `common::html_escape` — minimal `& < > " '` escape, no extra dep. - `wrap_in_doc` escapes its `title` parameter. - Every handler that composes `headline_html` from user data now calls `html_escape` first. - New regression test `detail_escapes_xss_in_subject_for_title_and_headline` in `issues.rs` exercises the full XSS surface. # P2 — URL slug encoding (#13) `/roles/`, `/projects/`, `/users/` paths interpolated the slug raw. A name with `#`/`?`/`/` would break the browser's URL parse (`#` = fragment, `/` = path segment). Fix: - `common::encode_path_segment` — percent-encodes everything outside RFC-3986 unreserved + path-safe chars. No new dep (rule is small). - Roles, projects, users all route hrefs through it. - New regression test `list_percent_encodes_role_names_with_reserved_chars` in `roles.rs` (Q/A → Q%2FA, R&D → R%26D). # P1 — RM_BIND fail-fast (#7) `std::env::var("RM_BIND").ok().and_then(|s| s.parse().ok())` collapsed unset and malformed into the same fallback. A typo silently started the server on the default. Now we distinguish: - Unset → default - Malformed → return `io::Error::other(...)` immediately # Skipped - #9 (auth routes not reachable) — already fixed by #10's `build_router_with` refactor that mounts `rm_auth::router(cfg)`. - #8 (lowercase vs PascalCase OGAR table names) — schema- divergence noted in W3's doc, lands in a dedicated PR; needs the per-port DDL emission story W4-followups will revisit. cargo fmt --check + cargo clippy --all-targets -- -D warnings + cargo test --workspace all green locally; 108 tests across 5 crates. --- crates/rm-handlers/src/common.rs | 124 ++++++++++++++++++++++++- crates/rm-handlers/src/issues.rs | 53 ++++++++++- crates/rm-handlers/src/lib.rs | 5 +- crates/rm-handlers/src/projects.rs | 13 ++- crates/rm-handlers/src/roles.rs | 42 ++++++++- crates/rm-handlers/src/time_entries.rs | 9 +- crates/rm-handlers/src/users.rs | 11 ++- crates/rm-server/src/main.rs | 16 +++- 8 files changed, 248 insertions(+), 25 deletions(-) diff --git a/crates/rm-handlers/src/common.rs b/crates/rm-handlers/src/common.rs index b41defa..8f1a7b8 100644 --- a/crates/rm-handlers/src/common.rs +++ b/crates/rm-handlers/src/common.rs @@ -1,10 +1,15 @@ //! Helpers every resource handler shares. Today's set: //! - [`AppState`] — shared axum `State` //! - [`wrap_in_doc`] — HTML shell until G1 lands the master template +//! (escapes the title — XSS guard, codex P1 on PR #10) //! - [`record_id_to_u64`] — URL → render-kit u64 adapter for //! record-keyed resources (Issue) //! - [`identifier_to_u64`] — same adapter for slug-keyed resources //! (Project) and any future resource whose URL key is a string +//! - [`html_escape`] — minimal `& < > " '` escape, no extra dep +//! - [`encode_path_segment`] — percent-encode a single URL path +//! segment so slug-keyed resources tolerate `# ? /` in the key +//! (codex P2 on PR #13) //! - [`HandlerError`] — the per-resource error enum; factored out //! when W3 landed as the third caller (Plan §1.6 "three points //! form a line") @@ -27,16 +32,91 @@ pub struct AppState { pub store: Store, } +/// HTML-escape the five attribute-/content-significant chars +/// (`& < > " '`). No external dep — askama isn't a direct dep of +/// rm-handlers (it's transitive via ogar-render-askama; pulling it +/// in here re-triggers the askama_axum macro-expansion issue we hit +/// in W0.1). +/// +/// Used for any user-controlled string that lands in handler-built +/// HTML *outside* the askama kit's render path — most importantly +/// document titles ([`wrap_in_doc`]) and the pre-rendered +/// `headline_html` passed to `render_detail` (the kit treats that +/// parameter as raw HTML per the cell-template contract; user data +/// must be escaped before it goes in). +#[must_use] +pub fn html_escape(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for c in s.chars() { + match c { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + other => out.push(other), + } + } + out +} + +/// Percent-encode a single URL path segment. Encodes everything +/// that isn't an RFC-3986 unreserved char (`A-Z a-z 0-9 - . _ ~`) +/// or the path-segment-safe `:` `@`. +/// +/// Project identifiers + role names are arbitrary admin labels; +/// without encoding a `#`/`?`/`/` in the name breaks the URL +/// (codex P2 on PR #13). No external `percent-encoding` dep — +/// the rule is small. +#[must_use] +pub fn encode_path_segment(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for b in s.bytes() { + let safe = matches!( + b, + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' | b':' | b'@' + ); + if safe { + out.push(b as char); + } else { + out.push('%'); + out.push(hex_nibble(b >> 4)); + out.push(hex_nibble(b & 0x0F)); + } + } + out +} + +#[inline] +fn hex_nibble(n: u8) -> char { + match n { + 0..=9 => (b'0' + n) as char, + 10..=15 => (b'A' + n - 10) as char, + _ => unreachable!(), + } +} + /// Wrap a fragment in a minimal HTML5 document. G1 (Plan §5 GUI /// chrome) will replace this with the master `base.askama` template /// — at that point this fn deletes and every handler uses /// `askama_axum::Template` directly. +/// +/// The `title` parameter is **HTML-escaped** — handlers pass +/// user-controlled strings (issue subjects, project names) and the +/// raw value can't reach the `` element verbatim (codex P1 +/// on PR #10, stored XSS via a subject like +/// ``). +/// +/// `body` is treated as already-rendered HTML — askama-emitted by +/// the kit's `render_list` / `render_detail`, which run their own +/// `escape = "html"` on data-derived strings. #[must_use] pub fn wrap_in_doc(title: &str, body: &str) -> String { + let escaped_title = html_escape(title); format!( "\n\n\ \ - {title} · Redmine RS\ + {escaped_title} · Redmine RS\ {body}" ) } @@ -114,6 +194,45 @@ mod tests { assert!(html.contains("

hello

")); } + #[test] + fn wrap_in_doc_escapes_xss_in_title() { + // Codex P1 on PR #10: a subject like `", "

body

"); + assert!( + !html.contains("".to_string(), + description: None, + }) + .await + .unwrap(); + let rid = inserted.id.unwrap(); + let key = rid.key.to_sql(); + let app = router(AppState { store }); + let res = app + .oneshot( + Request::builder() + .uri(format!("/issues/{key}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let body = res.into_body().collect().await.unwrap().to_bytes(); + let s = std::str::from_utf8(&body).unwrap(); + assert!( + !s.contains("