core: control-char-safe Display for consumer-string error variants
Escapes the interpolated strings in the ReservedName/InvalidName/ InvalidSpec (and, for the same consumer-string interpolation class, Codec) Display arms with char::escape_debug so rendered error messages cannot carry raw control bytes from consumer input — review 004 item 14's log-forging finding. The error taxonomy (variants, kinds, match sites, ADR-008 §5) is unchanged; only the rendered text's hygiene moved. Pinned by error_display_escapes_consumer_control_chars.
This commit is contained in:
1 parent
9301bb926b
commit
7700079158
3 files changed
+101
-11
No files matched your search
+18
-4
@@ -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<dyn std::error::Error + Send + Sync>),
|
||||
}
|
||||
|
||||
/// 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.
|
||||
|
||||
@@ -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"));
|
||||
|
||||
@@ -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
|
||||
- `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.
|
||||
Reference in new issue
Block a user