fix(handlers): XSS in titles/headlines + URL-encode slugs + RM_BIND fail-fast

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("&amp;"),
+            '<' => out.push_str("&lt;"),
+            '>' => out.push_str("&gt;"),
+            '"' => out.push_str("&quot;"),
+            '\'' => out.push_str("&#39;"),
+            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 `<title>` element verbatim (codex P1
+/// on PR #10, stored XSS via a subject like
+/// `</title><script>alert(1)</script>`).
+///
+/// `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!(
         "<!doctype html>\n<html lang=\"en\">\n<head>\
          <meta charset=\"utf-8\">\
-         <title>{title} · Redmine RS</title>\
+         <title>{escaped_title} · Redmine RS</title>\
          </head><body>{body}</body></html>"
     )
 }
@@ -114,6 +194,45 @@ mod tests {
         assert!(html.contains("<p>hello</p>"));
     }
 
+    #[test]
+    fn wrap_in_doc_escapes_xss_in_title() {
+        // Codex P1 on PR #10: a subject like `</title><script>...`
+        // must not reach the document's <title> verbatim.
+        let html = wrap_in_doc("</title><script>alert(1)</script>", "<p>body</p>");
+        assert!(
+            !html.contains("</title><script>"),
+            "raw markup escaped past <title>: {html}"
+        );
+        assert!(
+            html.contains("&lt;/title&gt;&lt;script&gt;alert(1)&lt;/script&gt;"),
+            "expected fully-escaped title: {html}"
+        );
+    }
+
+    #[test]
+    fn html_escape_covers_the_five_chars() {
+        assert_eq!(html_escape("&<>\"'"), "&amp;&lt;&gt;&quot;&#39;");
+        assert_eq!(html_escape("plain text"), "plain text");
+    }
+
+    #[test]
+    fn encode_path_segment_keeps_unreserved_chars() {
+        assert_eq!(
+            encode_path_segment("my-proj.v1_test~ok"),
+            "my-proj.v1_test~ok"
+        );
+    }
+
+    #[test]
+    fn encode_path_segment_percent_encodes_reserved() {
+        // Slashes / hashes / questions would break URL parsing
+        // (codex P2 on PR #13).
+        assert_eq!(encode_path_segment("a/b"), "a%2Fb");
+        assert_eq!(encode_path_segment("a#b"), "a%23b");
+        assert_eq!(encode_path_segment("a?b"), "a%3Fb");
+        assert_eq!(encode_path_segment("a b"), "a%20b");
+    }
+
     #[test]
     fn record_id_to_u64_is_deterministic() {
         let rid = RecordId::new("project_work_item", "abc");
@@ -126,9 +245,6 @@ mod tests {
     fn record_id_to_u64_distinguishes_different_ids() {
         let a = record_id_to_u64(&RecordId::new("project_work_item", "id_a"));
         let b = record_id_to_u64(&RecordId::new("project_work_item", "id_b"));
-        // DefaultHasher isn't collision-proof but two different keys
-        // landing on the same u64 is astronomically unlikely; if this
-        // ever fails it's worth investigating.
         assert_ne!(a, b, "hash collision on two different ids");
     }
 

crates/rm-handlers/src/issues.rs

@@ -31,7 +31,7 @@ use ogar_render_askama::{
 use rm_store::IssueRow;
 use surrealdb_types::{RecordId, ToSql};
 
-use crate::common::{record_id_to_u64, wrap_in_doc, AppState, HandlerError};
+use crate::common::{html_escape, record_id_to_u64, wrap_in_doc, AppState, HandlerError};
 
 /// `GET /issues` — render the issue list.
 pub async fn list(State(state): State<AppState>) -> Result<Html<String>, HandlerError> {
@@ -95,9 +95,13 @@ pub async fn detail(
     let issue: IssueRow = state.store.find_issue(&rid).await?;
     let cols = detail_columns();
     let href = format!("/issues/{}", id_str);
+    // Subject is user-controlled; the kit treats `headline_html` as
+    // already-rendered HTML, so we escape before composing the link
+    // (codex P1 on PR #10).
     let headline = format!(
         "<a href=\"{}\" class=\"primary-link\">{}</a>",
-        href, &issue.subject
+        html_escape(&href),
+        html_escape(&issue.subject)
     );
     let subtitle = issue.description.as_deref().unwrap_or("");
     let cells = vec![CellSource {
@@ -274,4 +278,49 @@ mod tests {
             .unwrap();
         assert_eq!(res.status(), StatusCode::NOT_FOUND);
     }
+
+    #[tokio::test]
+    async fn detail_escapes_xss_in_subject_for_title_and_headline() {
+        // Codex P1 on PR #10. Two paths both need escaping:
+        // 1) the <title> via `wrap_in_doc(title=...)` — fixed in `common`
+        // 2) the headline anchor passed as `headline_html` to render_detail —
+        //    the kit treats that arg as already-rendered HTML and `|safe`s
+        //    it through, so this handler must escape before composing.
+        let store = Store::open().await.unwrap();
+        let inserted = store
+            .create_issue(NewIssue {
+                subject: "</title><script>alert(1)</script>".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("</title><script>"),
+            "raw subject survived into title — XSS:\n{s}"
+        );
+        assert!(
+            !s.contains("<script>alert(1)"),
+            "raw script tag survived into headline — XSS:\n{s}"
+        );
+        // The escaped form should appear (in <title> AND in the
+        // headline anchor text).
+        assert!(
+            s.contains("&lt;/title&gt;&lt;script&gt;alert(1)"),
+            "expected escaped form somewhere:\n{s}"
+        );
+    }
 }

crates/rm-handlers/src/lib.rs

@@ -52,4 +52,7 @@ pub mod roles;
 pub mod time_entries;
 pub mod users;
 
-pub use common::{identifier_to_u64, record_id_to_u64, wrap_in_doc, AppState, HandlerError};
+pub use common::{
+    encode_path_segment, html_escape, identifier_to_u64, record_id_to_u64, wrap_in_doc, AppState,
+    HandlerError,
+};

crates/rm-handlers/src/projects.rs

@@ -20,15 +20,17 @@ use ogar_render_askama::{
 };
 use rm_store::ProjectRow;
 
-use crate::common::{identifier_to_u64, wrap_in_doc, AppState, HandlerError};
+use crate::common::{
+    encode_path_segment, html_escape, identifier_to_u64, wrap_in_doc, AppState, HandlerError,
+};
 
 /// `GET /projects` — render the project list.
 pub async fn list(State(state): State<AppState>) -> Result<Html<String>, HandlerError> {
     let projects = state.store.list_projects().await?;
     let cols = list_columns();
     let hrefs: Vec<String> = projects
         .iter()
-        .map(|p| format!("/projects/{}", p.identifier))
+        .map(|p| format!("/projects/{}", encode_path_segment(&p.identifier)))
         .collect();
     let ids: Vec<u64> = projects
         .iter()
@@ -73,10 +75,13 @@ pub async fn detail(
 ) -> Result<Html<String>, HandlerError> {
     let project: ProjectRow = state.store.find_project_by_identifier(&identifier).await?;
     let cols = detail_columns();
-    let href = format!("/projects/{}", project.identifier);
+    let href = format!("/projects/{}", encode_path_segment(&project.identifier));
+    // `project.name` is user-controlled — escape before composing
+    // the headline anchor (same XSS guard as W1's issue subject).
     let headline = format!(
         "<a href=\"{}\" class=\"primary-link\">{}</a>",
-        href, &project.name
+        html_escape(&href),
+        html_escape(&project.name)
     );
     let cells = vec![
         CellSource {

crates/rm-handlers/src/roles.rs

@@ -16,13 +16,18 @@ use ogar_render_askama::{
 };
 use rm_store::RoleRow;
 
-use crate::common::{identifier_to_u64, wrap_in_doc, AppState, HandlerError};
+use crate::common::{
+    encode_path_segment, html_escape, identifier_to_u64, wrap_in_doc, AppState, HandlerError,
+};
 
 /// `GET /roles` — render the role list.
 pub async fn list(State(state): State<AppState>) -> Result<Html<String>, HandlerError> {
     let roles = state.store.list_roles().await?;
     let cols = list_columns();
-    let hrefs: Vec<String> = roles.iter().map(|r| format!("/roles/{}", r.name)).collect();
+    let hrefs: Vec<String> = roles
+        .iter()
+        .map(|r| format!("/roles/{}", encode_path_segment(&r.name)))
+        .collect();
     let positions: Vec<String> = roles.iter().map(|r| r.position.to_string()).collect();
     let ids: Vec<u64> = roles.iter().map(|r| identifier_to_u64(&r.name)).collect();
     let rows: Vec<RowSource<'_>> = roles
@@ -64,11 +69,12 @@ pub async fn detail(
 ) -> Result<Html<String>, HandlerError> {
     let role: RoleRow = state.store.find_role_by_name(&name).await?;
     let cols = detail_columns();
-    let href = format!("/roles/{}", role.name);
+    let href = format!("/roles/{}", encode_path_segment(&role.name));
     let position_str = role.position.to_string();
     let headline = format!(
         "<a href=\"{}\" class=\"primary-link\">{}</a>",
-        href, &role.name
+        html_escape(&href),
+        html_escape(&role.name)
     );
     let cells = vec![
         CellSource {
@@ -218,4 +224,32 @@ mod tests {
             .unwrap();
         assert_eq!(res.status(), StatusCode::NOT_FOUND);
     }
+
+    #[tokio::test]
+    async fn list_percent_encodes_role_names_with_reserved_chars() {
+        // Codex P2 on PR #13: a role name with `#`/`?`/`/` would
+        // produce a URL the browser can't navigate to (`#` becomes a
+        // fragment, `/` a path segment). The list href must
+        // percent-encode the name.
+        let app = app_with_roles(&[("Q/A", 1), ("R&D", 2)]).await;
+        let res = app
+            .oneshot(
+                Request::builder()
+                    .uri("/roles")
+                    .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("href=\"/roles/Q%2FA\""),
+            "expected percent-encoded slash in href:\n{s}"
+        );
+        assert!(
+            s.contains("href=\"/roles/R%26D\""),
+            "expected percent-encoded ampersand in href:\n{s}"
+        );
+    }
 }

crates/rm-handlers/src/time_entries.rs

@@ -26,7 +26,7 @@ use ogar_render_askama::{
 use rm_store::TimeEntryRow;
 use surrealdb_types::{RecordId, ToSql};
 
-use crate::common::{record_id_to_u64, wrap_in_doc, AppState, HandlerError};
+use crate::common::{html_escape, record_id_to_u64, wrap_in_doc, AppState, HandlerError};
 
 /// `GET /time_entries` — render the list of booked time entries.
 pub async fn list(State(state): State<AppState>) -> Result<Html<String>, HandlerError> {
@@ -106,9 +106,14 @@ pub async fn detail(
     let cols = detail_columns();
     let href = format!("/time_entries/{}", id_str);
     let hours_str = format!("{:.2}", entry.hours);
+    // `spent_on` is user-controlled (Store accepts any String);
+    // escape before composing the headline anchor — the kit treats
+    // `headline_html` as already-rendered (codex P2 on PR #12).
     let headline = format!(
         "<a href=\"{}\" class=\"primary-link\">{} — {}h</a>",
-        href, &entry.spent_on, hours_str
+        html_escape(&href),
+        html_escape(&entry.spent_on),
+        hours_str
     );
     let subtitle = entry.comments.as_deref().unwrap_or("");
     let cells = vec![