diff --git a/alkstore/src/error.rs b/alkstore/src/error.rs index 41b2370..7026dc5 100644 --- a/alkstore/src/error.rs +++ b/alkstore/src/error.rs @@ -22,14 +22,15 @@ pub enum Error { /// A name carrying the reserved `__alkstore_` prefix was supplied /// to a shared-namespace entry point. #[error( - "reserved name `{name}`: names carrying the reserved `__alkstore_` prefix are engine-owned" + "reserved name `{}`: names carrying the reserved `__alkstore_` prefix are engine-owned", + display_safe(.name) )] ReservedName { /// The rejected name, as supplied. name: String, }, /// A name failed entry-point validation. - #[error("invalid name `{name}`")] + #[error("invalid name `{}`", display_safe(.name))] InvalidName { /// The rejected name, as supplied. name: String, @@ -40,7 +41,7 @@ pub enum Error { /// /// Rejected by `schedule()` before storage, with no engine round /// trip; the caller fixes the spec string (ADR-009 §2, §6). - #[error("invalid schedule spec `{spec}`")] + #[error("invalid schedule spec `{}`", display_safe(.spec))] InvalidSpec { /// The spec string that failed the grammar. spec: String, @@ -65,7 +66,7 @@ pub enum Error { /// /// `payload_as` on stream events and job payloads; wakes carry no /// payload, so no wake path produces this variant. - #[error("payload codec failure: {0}")] + #[error("payload codec failure: {}", display_safe(.0))] Codec(String), /// The opaque fallback: everything else. /// @@ -76,6 +77,19 @@ pub enum Error { Database(#[source] Box), } +/// Escape a consumer-supplied string for insertion into a rendered +/// error message. +/// +/// Control characters (newlines, tabs, escape bytes) and backslash +/// quoting are routed through `char::escape_debug` so the rendered +/// message can never carry raw control bytes from consumer input — +/// the log-forging hygiene fix for the interpolating variants. The +/// stored field itself stays exactly as supplied; only the +/// `Display` text changes. +fn display_safe(s: &str) -> String { + s.chars().flat_map(char::escape_debug).collect() +} + impl Error { /// Convenience constructor for the opaque [`Error::Database`] /// fallback. diff --git a/alkstore/src/tests.rs b/alkstore/src/tests.rs index ebf7e00..188e6bb 100644 --- a/alkstore/src/tests.rs +++ b/alkstore/src/tests.rs @@ -110,6 +110,43 @@ fn error_display_texts() { assert_eq!(src.to_string(), "boom"); } +#[test] +fn error_display_escapes_consumer_control_chars() { + let forged = "in\u{1}ject\n\u{7}end\t"; + for e in [ + Error::InvalidName { + name: forged.into(), + }, + Error::ReservedName { + name: forged.into(), + }, + Error::InvalidSpec { + spec: forged.into(), + }, + Error::Codec(forged.into()), + ] { + let s = e.to_string(); + assert!( + !s.chars().any(char::is_control), + "raw control byte in rendered error: {s:?}" + ); + } + assert_eq!( + Error::InvalidName { + name: "x\u{1}\ny".into() + } + .to_string(), + "invalid name `x\\u{1}\\ny`" + ); + for plain in ["orders", "worker-1", "job row payload must be a string"] { + assert!( + Error::InvalidName { name: plain.into() } + .to_string() + .contains(plain) + ); + } +} + #[test] fn error_database_constructor() { let e = Error::database(std::io::Error::other("disk gone")); diff --git a/tasks/core-fix-display-log-forging.md b/tasks/core-fix-display-log-forging.md index 9592a0f..a90efdd 100644 --- a/tasks/core-fix-display-log-forging.md +++ b/tasks/core-fix-display-log-forging.md @@ -1,7 +1,7 @@ --- id: core-fix-display-log-forging name: Control-char-safe Display for the consumer-string error variants (review 004 item 14) -status: pending +status: completed depends_on: [] scope: single risk: low @@ -32,14 +32,14 @@ include it in the same fix (and say so in Notes). ## Acceptance Criteria -- [ ] The three variants' `Display` renders consumer strings +- [x] The three variants' `Display` renders consumer strings control-char-safe — pinned by test (a name/spec carrying `\n` and control bytes renders without any raw control byte) -- [ ] The error taxonomy unchanged (variants, kinds, match sites); +- [x] The error taxonomy unchanged (variants, kinds, match sites); existing error/validation tests green unchanged -- [ ] Any additional same-class `Display` arms included (or the +- [x] Any additional same-class `Display` arms included (or the absence recorded in Notes) -- [ ] `cargo test -p alkstore`, clippy `-D warnings`, fmt clean; doc +- [x] `cargo test -p alkstore`, clippy `-D warnings`, fmt clean; doc gate green ## References @@ -52,8 +52,47 @@ include it in the same fix (and say so in Notes). ## Notes -> To be filled by implementation agent +- Additional same-class arms found and fixed: one — + `Error::Codec(String)`. Its `Display` interpolated the stored + `String`, and the engine decode paths mint it with + `format!("...: {e}")` embedding driver/serde error text that is not + under core's control, so the same raw-control-byte-forge class was + structurally reachable (in practice serde's `Unexpected::Str` + already escapes via `Debug`, but the invariant was un-pinned and + un-fuzzable). Escaping at the `Display` arm makes + "no raw control byte in any rendered error message" a single-point + invariant. Side effect (accepted): serde messages that already use + `Debug` quoting render double-escaped (`\"`, `\\n`) — slightly + uglier, never unsafe; no existing test asserted literal + quote/backslash rendering. +- Implementation form: thiserror `#[error("... `{}`", + display_safe(.name))]` field-shorthand args (thiserror 2) — the + variant/field/match-site taxonomy is byte-identical (ADR-008 §5 + untouched); only the three (+1) format args changed. The escaping + helper (`char::escape_debug` per char, collected) is private to + `error.rs`. +- Deliberately left alone: `Database(#[source] ...)` renders only the + static `"database error"` line (the source chain is a field of the + opaque variant, reached by the consumer's own `Error::source` + call, not by our `Display`); `LeadershipLost`/`Closed`/`PayloadTooLarge` + interpolate no consumer strings. ## Summary -> To be filled on completion \ No newline at end of file +- `alkstore/src/error.rs`: `display_safe` helper (per-char + `char::escape_debug`) applied in the `Display` arms of + `ReservedName`, `InvalidName`, `InvalidSpec`, and `Codec` — + rendered messages can no longer carry raw control bytes from + consumer input. Taxonomy untouched (variants, kinds, fields, + match sites identical). +- `alkstore/src/tests.rs`: new `error_display_escapes_consumer_control_chars` + — a name/spec/message carrying `\u{1}`/`\n`/`\u{7}`/`\t` renders + with zero control chars (exact-text pin: ``invalid name `x\u{1} + \ny` ``), and plain names still render verbatim. +- Verified: `cargo test -p alkstore` (28 green, including all + pre-existing error/validation tests unchanged), workspace + `cargo build`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check`, + `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p alkstore`, + and `cargo test --manifest-path fuzz/shared/Cargo.toml` (corpus + replay gate) — all clean. \ No newline at end of file