diff --git a/CHANGELOG.md b/CHANGELOG.md index 179136b..e2d5b31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,41 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **error ergonomics (#33):** removed the `terrors::OneOf` error-union layer in + favour of purpose-built `thiserror` enums. **Breaking** (MINOR under 0.x): + - `MatterBuilder::{from_qualified_base64, from_qualified_base2, build}` now return + `Result<_, MatterBuildError>` (variants `Parsing`, `Validation`) instead of + `OneOf<(ParsingError, ValidationError)>`. + - `crypto::verify` now returns `Result<(), VerificationError>` (variants + `Signature`, `CodeMismatch`) instead of `OneOf<(SignatureError, CodeMismatchError)>`. + - The indexer builder's parse/validation methods (`from_qb64`, `from_qb2`, + `with_index`, `with_indices`, `with_raw`) return the bare `IndexerParseError` / + `IndexerValidationError` (previously wrapped in a single-element `OneOf`). + - Consumers matching on these results switch from `.take::` / `.narrow::` to a + normal `match` on the new enums / bare types. + - `SerderError` gains a new `UnparseablePrimitive` variant (see _Fixed_ below). + As `SerderError` is public and not `#[non_exhaustive]`, this is **breaking** for + downstream exhaustive `match` on it. + - The `terrors` dependency is dropped. + +### Fixed + +- **serder (#33):** a malformed-but-unparseable field value no longer collapses a + `ParsingError` into `ValidationError::UnknownMatterCode(..)` via string + formatting; a new `SerderError::UnparseablePrimitive { field, source }` variant + carries the parsing error in its own failure domain. +- **stream (#33):** removed an `unreachable!()` panic on the matter-parse error path + in `stream::parse::parse_matter`; the error mapping is now a total `match`. +- **core (#33):** `MatterBuilder::from_qualified_base64` no longer panics + (`range end index N out of range for slice of length 0`) on a malformed qb64 whose + decoded buffer is shorter than the code's declared lead size (e.g. `5BAA`). The + lead-byte slices are now bounds-checked and return + `MatterBuildError::Validation(ValidationError::StructuralIntegrityError)`. Found by + the `deep-fuzz` `matter_from_qb64` target; the crash input is committed as a fuzz + corpus regression seed. + ### Added - **crypto/devx (#69):** indexed signatures (`Siger`, the form attached to KERI diff --git a/CLAUDE.md b/CLAUDE.md index cdb1b72..fc6befd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -167,10 +167,23 @@ Write clean, self-documenting code. Do not add comments explaining what the code ## Error Handling -- `thiserror` for error enums. -- `terrors::OneOf` for error type unions in return types (`Result>`). -- In tests, use `.err().unwrap().take::()` to extract from `OneOf` — not `.unwrap_err()` + deref. -- When `OneOf` grows large (many variants), group errors into a `thiserror` enum instead. +- `thiserror` for error enums, **including error unions**. When an operation can fail + in two or more distinct domains, model the union as a dedicated `thiserror` enum with + one `#[from]` variant per source error (e.g. + `MatterBuildError { Parsing(#[from] ParsingError), Validation(#[from] ValidationError) }`). + `#[from]` keeps `?` propagation ergonomic and preserves the source chain; prefer + `#[error(transparent)]` on a variant that simply forwards to its source's `Display`. +- A single-domain fallible operation returns its **bare** error type — never wrap a lone + error in a union type. +- In tests, match the error enum directly (`matches!(e, MatterBuildError::Parsing(_))` or a + `let ... else` bind) and assert the specific variant — do not stringify. +- Name a union enum after its operation/domain, following the one-error-enum-per-module + convention (`MatterBuildError`, `VerificationError`). + +> Historical note: the crate previously used `terrors::OneOf<(E1, E2, ...)>` for error +> unions. It was removed in #33 (error-ergonomics pass) in favour of the `thiserror`-enum +> convention above, which is matchable without a runtime downcast and drops a dependency. +> Do **not** reintroduce `terrors`. ## Mandatory Rules @@ -191,7 +204,7 @@ These rules are ported from the nexus codebase, where each one earned its place ### 3. Error Handling -Builds on the [Error Handling](#error-handling) section above (`thiserror`, `terrors::OneOf`). Additionally: +Builds on the [Error Handling](#error-handling) section above (`thiserror` enums, including unions). Additionally: - **One variant = one failure domain.** Never jam unrelated errors into an existing variant. A malformed-length error and an invalid-base64 error are different domains — give them different variants. - **Never discard the original error** with `|_|` in `map_err`. Wrap it via `#[source]`/`#[from]`, or at minimum preserve its message. diff --git a/Cargo.lock b/Cargo.lock index 9582187..300b8d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -180,7 +180,6 @@ dependencies = [ "sha3", "signature", "strum", - "terrors", "thiserror", "tokio", "tokio-util", @@ -1325,12 +1324,6 @@ dependencies = [ "windows-sys", ] -[[package]] -name = "terrors" -version = "0.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "987fd8c678ca950df2a18b2c6e9da6ca511d449278fab3565efe0d49c0c07a5d" - [[package]] name = "thiserror" version = "2.0.18" diff --git a/Cargo.toml b/Cargo.toml index 1260869..e94de11 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,7 +46,7 @@ alloc = [ # Module gates b64 = ["dep:num-traits"] -core = ["b64", "dep:base64", "dep:num-traits", "dep:strum", "dep:terrors", "dep:zeroize"] +core = ["b64", "dep:base64", "dep:num-traits", "dep:strum", "dep:zeroize"] crypto = [ "core", "dep:blake2", @@ -93,7 +93,6 @@ sha2 = { version = "0.10.9", default-features = false, optional = true } sha3 = { version = "0.10.8", default-features = false, optional = true } signature = { version = "2.2.0", default-features = false, optional = true } strum = { version = "0.28.0", default-features = false, features = ["derive", "phf"], optional = true } -terrors = { version = "0.3.3", optional = true } thiserror = { version = "2.0.18", default-features = false } tokio-util = { version = "0.7.15", default-features = false, features = ["codec"], optional = true } zeroize = { version = "1.8.2", default-features = false, features = ["derive"], optional = true } diff --git a/_typos.toml b/_typos.toml index 696943e..1d388cf 100644 --- a/_typos.toml +++ b/_typos.toml @@ -23,3 +23,7 @@ extend-exclude = [ ba = "ba" iz = "iz" dentifier = "dentifier" +# `UnparseablePrimitive` is the chosen SerderError variant name; "unparseable" +# is a valid English spelling the default dictionary would "correct" to +# "unparsable". Keep the deliberate spelling used across code, spec, and plan. +unparseable = "unparseable" diff --git a/docs/superpowers/plans/2026-07-04-33-remove-oneof-error-ergonomics.md b/docs/superpowers/plans/2026-07-04-33-remove-oneof-error-ergonomics.md new file mode 100644 index 0000000..645e1e9 --- /dev/null +++ b/docs/superpowers/plans/2026-07-04-33-remove-oneof-error-ergonomics.md @@ -0,0 +1,613 @@ +# Remove `terrors::OneOf` — Error Ergonomics Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace every `terrors::OneOf<(...)>` return with a purpose-built `thiserror` enum (or a bare error type where the union has one member), and drop the `terrors` dependency entirely. + +**Architecture:** Two new domain error enums — `MatterBuildError` (matter builder) and `VerificationError` (crypto verify) — each a 2-variant `thiserror` enum with `#[from]` on both variants so `?` keeps working. The indexer's single-element `OneOf<(E,)>` unions collapse to bare `E`. Serder gains a new `UnparseablePrimitive` variant so a `ParsingError` is no longer string-jammed into a `ValidationError`. No `#[non_exhaustive]` (pre-1.0; exhaustive matching serves tag-pinning consumers). + +**Tech Stack:** Rust (edition 2024, stable 1.95.0), `thiserror`, `nix flake check` gate. + +**Spec:** `docs/superpowers/specs/2026-07-04-33-remove-oneof-error-ergonomics-design.md` + +**Branch:** `feat/33-remove-oneof` (already created off `origin/main`). + +--- + +## File Map + +| File | Change | +|---|---| +| `src/core/matter/error.rs` | **Add** `MatterBuildError` enum + unit tests | +| `src/core/matter/builder.rs` | Swap 4 return types `OneOf<(ParsingError, ValidationError)>` → `MatterBuildError`; rewrite `OneOf::new(x)` → `MatterBuildError::from(x)` / `.map_err(OneOf::new)` → `.map_err(MatterBuildError::from)`; drop `use terrors::OneOf`; delete two `#[allow(clippy::type_complexity …)]` | +| `src/crypto/error.rs` | **Add** `VerificationError` enum + unit tests | +| `src/crypto/verify.rs` | Swap 2 return types → `VerificationError`; rewrite `OneOf::new`/`.map_err(OneOf::new)`; drop `use terrors::OneOf`; migrate test `.narrow::()` → `matches!` on enum | +| `src/core/indexer/builder.rs` | Swap 5 return types to bare `IndexerParseError` / `IndexerValidationError`; rewrite `OneOf::new(x)` → `x`; drop `use terrors::OneOf`; migrate test `.take::()` → drop the call | +| `src/serder/error.rs` | **Add** `UnparseablePrimitive { field, source: ParsingError }` variant + `use` for `ParsingError` | +| `src/serder/deserialize.rs` | Change `map_qb64_error` param to `MatterBuildError`, rewrite as a `match`; add regression test | +| `Cargo.toml` | Remove `dep:terrors` from `core` feature (line 49) and the `terrors = …` dep (line 96) | +| `CHANGELOG.md` | Add `### Changed` entry under `[Unreleased]` | + +**Ordering rationale:** matter first (its enum is consumed by serder), then crypto and indexer (independent), then serder (depends on `MatterBuildError`), then Cargo.toml removal (only safe once all `OneOf` uses are gone), then the full gate. + +> **Plan correction (discovered during execution):** the initial survey used `grep "OneOf"`, which missed `src/stream/parse.rs` — that file consumes the matter/indexer builder errors via terrors' `.narrow::()` / `.take()` methods (by type inference) and never writes the literal `OneOf`. A **Task 6b** was inserted to migrate it (rewrite `parse_matter`'s `map_err` to a total `match` on `MatterBuildError` — removing an `unreachable!()` panic — and `parse_indexer`'s `.take()` to a bare `.map_err(ParseError::from)`). Task 6b must land before Task 7 (dep drop). Lesson: survey for the terrors *method surface* (`.narrow::<`, `.take()`), not just the `OneOf` type name. + +--- + +## Task 1: Add `MatterBuildError` to matter/error.rs + +**Files:** +- Modify: `src/core/matter/error.rs` (append after `ValidationError`, before any `#[cfg(test)]`) + +- [ ] **Step 1: Write the failing test** + +Append a test module at the end of `src/core/matter/error.rs`: + +```rust +#[cfg(test)] +mod build_error_tests { + use super::{MatterBuildError, ParsingError, ValidationError}; + + #[test] + fn from_parsing_error_lands_in_parsing_variant() { + let e: MatterBuildError = ParsingError::EmptyStream.into(); + assert_eq!(e, MatterBuildError::Parsing(ParsingError::EmptyStream)); + } + + #[test] + fn from_validation_error_lands_in_validation_variant() { + let ve = ValidationError::StructuralIntegrityError; + let e: MatterBuildError = ve.clone().into(); + assert_eq!(e, MatterBuildError::Validation(ve)); + } + + #[test] + fn display_is_transparent_to_source() { + let e: MatterBuildError = ParsingError::EmptyStream.into(); + assert_eq!(e.to_string(), ParsingError::EmptyStream.to_string()); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `nix develop --command cargo test --features core --lib core::matter::error::build_error_tests 2>&1 | tail -20` +Expected: FAIL — `cannot find type MatterBuildError in this scope`. + +- [ ] **Step 3: Add the enum** + +Insert immediately after the closing `}` of `ValidationError` (currently line 171), before the test module: + +```rust +/// Error returned by [`MatterBuilder`](super::builder::MatterBuilder) parse and +/// build operations: either the input was structurally unparseable +/// ([`ParsingError`]) or it parsed but violated a CESR validation rule +/// ([`ValidationError`]). +#[derive(Debug, ThisError, PartialEq, Eq)] +pub enum MatterBuildError { + /// The input could not be parsed into a CESR primitive. + #[error(transparent)] + Parsing(#[from] ParsingError), + + /// The input parsed but failed a validation constraint. + #[error(transparent)] + Validation(#[from] ValidationError), +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `nix develop --command cargo test --features core --lib core::matter::error::build_error_tests 2>&1 | tail -20` +Expected: PASS (3 tests). + +- [ ] **Step 5: Export the type** + +Confirm `MatterBuildError` is re-exported wherever `ParsingError` / `ValidationError` are. Run: +`grep -rn "ParsingError" src/core/matter/mod.rs src/core/mod.rs src/lib.rs` +For each place that re-exports `ValidationError`, add `MatterBuildError` alongside it (same `pub use` line). If `ParsingError`/`ValidationError` are not re-exported there, do nothing. + +- [ ] **Step 6: Commit** + +```bash +git add src/core/matter/error.rs src/core/matter/mod.rs +git commit -m "feat(#33): add MatterBuildError enum replacing OneOf union" +``` + +--- + +## Task 2: Migrate matter/builder.rs to `MatterBuildError` + +**Files:** +- Modify: `src/core/matter/builder.rs` + +- [ ] **Step 1: Change the import** + +Delete line 18 `use terrors::OneOf;`. Add `MatterBuildError` to the `super::error` import on line 4: + +```rust +use super::{ + MatterPart, + code::{CesrCode, MatterCode}, + error::{MatterBuildError, ParsingError, ValidationError}, + matter::Matter, + sizage::{Sizage, SizeType}, +}; +``` + +- [ ] **Step 2: Change the four return types** + +At lines 101, 219, 367, 420 replace: +`) -> Result, OneOf<(ParsingError, ValidationError)>> {` +and `) -> Result, OneOf<(ParsingError, ValidationError)>> {` +and `pub fn build(self) -> Result, OneOf<(ParsingError, ValidationError)>> {` +with the same signature but `MatterBuildError` in place of `OneOf<(ParsingError, ValidationError)>`. E.g.: + +```rust +pub fn build(self) -> Result, MatterBuildError> { +``` + +- [ ] **Step 3: Rewrite the error-construction sites** + +Apply these two mechanical rules across the whole file (matches every remaining `OneOf` occurrence except the ones already removed): + +1. `OneOf::new(EXPR)` → `MatterBuildError::from(EXPR)` — works for both a `ParsingError` and a `ValidationError` value via `#[from]`. +2. `.map_err(OneOf::new)` → `.map_err(MatterBuildError::from)`. + +Concretely this covers, e.g.: +- `return Err(OneOf::new(ParsingError::EmptyStream));` → `return Err(MatterBuildError::from(ParsingError::EmptyStream));` +- `MatterCode::from_base64_stream(&stream).map_err(OneOf::new)?` → `.map_err(MatterBuildError::from)?` +- `.map_err(|err| OneOf::new(ParsingError::InvalidUtf8(err)))?` → `.map_err(|err| MatterBuildError::from(ParsingError::InvalidUtf8(err)))?` +- `.ok_or_else(|| OneOf::new(ParsingError::EmptyStream))?` → `.ok_or_else(|| MatterBuildError::from(ParsingError::EmptyStream))?` + +Verify none remain: `grep -n "OneOf" src/core/matter/builder.rs` must print nothing. + +- [ ] **Step 4: Delete the two `type_complexity` allows** + +Remove both attribute blocks (currently at lines ~355–358 and ~408–411): + +```rust +#[allow( + clippy::type_complexity, + reason = "OneOf error union is inherently complex" +)] +``` + +The return type is now a plain enum, so the lint no longer fires and the allow would be unused. + +- [ ] **Step 5: Migrate the in-file test extraction site** + +At line ~734 the test uses `.narrow::()`. Replace the extraction with a `match`/`matches!` on `MatterBuildError`. Read the surrounding test (lines ~725–745) and rewrite, e.g.: + +```rust +// before: let ve = err.narrow::().unwrap(); assert on ve +// after: +let MatterBuildError::Validation(ve) = err else { + panic!("expected Validation variant, got {err:?}"); +}; +// ... existing assertions on `ve` unchanged +``` + +- [ ] **Step 6: Run the matter tests to verify they pass** + +Run: `nix develop --command cargo test --features core --lib core::matter 2>&1 | tail -20` +Expected: PASS (all existing matter tests, now returning `MatterBuildError`). + +- [ ] **Step 7: Commit** + +```bash +git add src/core/matter/builder.rs +git commit -m "refactor(#33): matter builder returns MatterBuildError, drop OneOf" +``` + +--- + +## Task 3: Add `VerificationError` to crypto/error.rs + +**Files:** +- Modify: `src/crypto/error.rs` + +Note: `SignatureError` and `CodeMismatchError` do **not** derive `PartialEq`, so `VerificationError` cannot derive `PartialEq` either. Tests use `matches!`, not `assert_eq!`. + +- [ ] **Step 1: Write the failing test** + +Add to the existing `#[cfg(test)] mod tests` in `src/crypto/error.rs`: + +```rust + #[test] + fn verification_error_from_signature_error() { + let e: VerificationError = SignatureError::Invalid.into(); + assert!(matches!(e, VerificationError::Signature(SignatureError::Invalid))); + } + + #[test] + fn verification_error_from_code_mismatch() { + let cm = CodeMismatchError::IncompatibleCodes { + verkey: "Ed25519".into(), + signature: "ECDSA256k1Sig".into(), + }; + let e: VerificationError = cm.into(); + assert!(matches!( + e, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) + )); + } + + #[test] + fn verification_error_display_is_transparent() { + let e: VerificationError = SignatureError::Invalid.into(); + assert_eq!(e.to_string(), SignatureError::Invalid.to_string()); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `nix develop --command cargo test --features crypto --lib crypto::error 2>&1 | tail -20` +Expected: FAIL — `cannot find type VerificationError in this scope`. + +- [ ] **Step 3: Add the enum** + +Insert after `CodeMismatchError` (currently ends line 95), before the `#[cfg(test)]` module: + +```rust +/// Error returned by signature verification ([`verify`](crate::crypto::verify)): +/// either the verifying-key/signature codes are incompatible +/// ([`CodeMismatchError`]) or the cryptographic check failed +/// ([`SignatureError`]). +#[derive(Debug, thiserror::Error)] +pub enum VerificationError { + /// The cryptographic verification failed or the key/signature bytes were malformed. + #[error(transparent)] + Signature(#[from] SignatureError), + + /// The signature's CESR code does not belong to the verifying key's algorithm. + #[error(transparent)] + CodeMismatch(#[from] CodeMismatchError), +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `nix develop --command cargo test --features crypto --lib crypto::error 2>&1 | tail -20` +Expected: PASS. + +- [ ] **Step 5: Export the type** + +`grep -rn "CodeMismatchError" src/crypto/mod.rs src/lib.rs` — add `VerificationError` to the same `pub use` lines that export `CodeMismatchError`/`SignatureError`. + +- [ ] **Step 6: Commit** + +```bash +git add src/crypto/error.rs src/crypto/mod.rs +git commit -m "feat(#33): add VerificationError enum replacing OneOf union" +``` + +--- + +## Task 4: Migrate crypto/verify.rs to `VerificationError` + +**Files:** +- Modify: `src/crypto/verify.rs` + +- [ ] **Step 1: Change imports** + +Delete line 9 `use terrors::OneOf;`. Change line 12 to include `VerificationError`: + +```rust +use crate::crypto::error::{CodeMismatchError, SignatureError, VerificationError}; +``` + +- [ ] **Step 2: Change both return types** + +Lines 42 and 66: `) -> Result<(), OneOf<(SignatureError, CodeMismatchError)>> {` → +`) -> Result<(), VerificationError> {` + +- [ ] **Step 3: Rewrite the error-construction sites** + +- Lines 52 and 68: `Err(OneOf::new(CodeMismatchError::IncompatibleCodes { … }))` → + `Err(VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { … }))` +- Line 73: `A::verify_bytes(verfer.raw(), data, sig.raw()).map_err(OneOf::new)` → + `A::verify_bytes(verfer.raw(), data, sig.raw()).map_err(VerificationError::from)` + +Verify: `grep -n "OneOf" src/crypto/verify.rs` prints nothing. + +- [ ] **Step 4: Migrate the test extraction sites** + +Four tests use `err.narrow::()`. Rewrite each as a `matches!` on the enum: + +- Line ~202–207 (`verify_rejects_wrong_data_standalone`): +```rust + let err = verify(&verfer, b"wrong", &sig).err().unwrap(); + assert!(matches!(err, VerificationError::Signature(SignatureError::Invalid))); +``` +- Line ~280–285 (`verify_secp256k1_sig_with_ed25519_verfer_fails`): +```rust + let err = verify(&verfer_e, b"test", &sig_k).err().unwrap(); + assert!(matches!( + err, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) + )); +``` +- Line ~466–471 (`verify_indexed_rejects_tampered_data`): +```rust + let err = verify(&verfer, b"tampered", &siger).err().unwrap(); + assert!(matches!(err, VerificationError::Signature(SignatureError::Invalid))); +``` +- Line ~482–487 (`verify_indexed_rejects_cross_algorithm_code`): +```rust + let err = verify(&ed_verfer, b"event", &k1_siger).err().unwrap(); + assert!(matches!( + err, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) + )); +``` + +Verify: `grep -n "narrow::" src/crypto/verify.rs` prints nothing. + +- [ ] **Step 5: Run the crypto tests to verify they pass** + +Run: `nix develop --command cargo test --features crypto --lib crypto::verify 2>&1 | tail -25` +Expected: PASS (all verify tests). + +- [ ] **Step 6: Commit** + +```bash +git add src/crypto/verify.rs +git commit -m "refactor(#33): crypto verify returns VerificationError, drop OneOf" +``` + +--- + +## Task 5: Migrate indexer/builder.rs to bare error types + +**Files:** +- Modify: `src/core/indexer/builder.rs` + +The single-element `OneOf<(E,)>` unions carry no information beyond `E`, so they collapse to bare `E`. + +- [ ] **Step 1: Change the import** + +Delete line 11 `use terrors::OneOf;`. Confirm `IndexerParseError` and `IndexerValidationError` are already imported (they are used in the bodies); if not, add them from `super::error`. + +- [ ] **Step 2: Change the five return types** + +- Lines 84, 198: `) -> Result<(Indexer<'static>, usize), OneOf<(IndexerParseError,)>> {` → + `) -> Result<(Indexer<'static>, usize), IndexerParseError> {` +- Line 296, 330: `) -> Result, OneOf<(IndexerValidationError,)>> {` → + `) -> Result, IndexerValidationError> {` +- Line 385: `) -> Result, OneOf<(IndexerValidationError,)>> {` → + `) -> Result, IndexerValidationError> {` + +- [ ] **Step 3: Rewrite the error-construction sites** + +Apply the mechanical rule `OneOf::new(EXPR)` → `EXPR` across the whole file. This covers every remaining `OneOf::new(...)` — e.g.: +- `.ok_or_else(|| OneOf::new(IndexerParseError::EmptyStream))?` → `.ok_or(IndexerParseError::EmptyStream)?` +- `return Err(OneOf::new(IndexerParseError::StreamTooShort { … }));` → `return Err(IndexerParseError::StreamTooShort { … });` +- `.map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?` → `.map_err(|_| IndexerParseError::InvalidBase64)?` +- `.map_err(|e| OneOf::new(IndexerParseError::from(e)))?` → `.map_err(IndexerParseError::from)?` +- `return Err(OneOf::new(IndexerValidationError::IndexTooLarge { … }));` → `return Err(IndexerValidationError::IndexTooLarge { … });` + +Verify: `grep -n "OneOf" src/core/indexer/builder.rs` prints nothing. + +- [ ] **Step 4: Migrate the four test extraction sites** + +At lines 560, 580, 598, 612 the tests end with `.err().unwrap().take::();`. The builder now returns the bare error, so delete the `.take::()` call — `.err().unwrap()` already yields an `IndexerValidationError`. E.g.: + +```rust + let err = IndexerBuilder::new() + .with_code(IndexedSigCode::Ed25519) + .with_index(64) + .err() + .unwrap(); + assert_eq!( + err, + IndexerValidationError::IndexTooLarge { code: IndexedSigCode::Ed25519, index: 64, max: 63 } + ); +``` + +Verify: `grep -n "take::" src/core/indexer/builder.rs` prints nothing. + +- [ ] **Step 5: Run the indexer tests to verify they pass** + +Run: `nix develop --command cargo test --features core --lib core::indexer 2>&1 | tail -20` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add src/core/indexer/builder.rs +git commit -m "refactor(#33): indexer builder returns bare error types, drop OneOf" +``` + +--- + +## Task 6: Serder — new `UnparseablePrimitive` variant + fix `map_qb64_error` + +**Files:** +- Modify: `src/serder/error.rs` +- Modify: `src/serder/deserialize.rs` + +This fixes the latent bug where a `ParsingError` is jammed into `ValidationError::UnknownMatterCode(err.to_string())`. + +- [ ] **Step 1: Write the failing regression test** + +Add to the `#[cfg(test)] mod tests` in `src/serder/deserialize.rs` (create the module if none exists; otherwise append). The test drives a genuine *parse* failure (unknown/malformed qb64 code) through `map_qb64_error` and asserts it surfaces as the new parsing-domain variant: + +```rust + #[test] + fn unparseable_qb64_field_surfaces_as_parsing_domain_error() { + // A malformed qb64 primitive (bad code) is a parse failure, not a + // validation failure — it must not be collapsed into UnknownMatterCode. + let err = super::parse_qb64_diger("!!not-qb64!!", "d").unwrap_err(); + assert!( + matches!(err, SerderError::UnparseablePrimitive { field: "d", .. }), + "expected UnparseablePrimitive, got {err:?}" + ); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `nix develop --command cargo test --features serder --lib serder::deserialize::tests::unparseable 2>&1 | tail -20` +Expected: FAIL — either `no variant named UnparseablePrimitive` or the match fails because the error is currently an `InvalidPrimitive`/`UnknownMatterCode`. + +- [ ] **Step 3: Add the new SerderError variant** + +In `src/serder/error.rs`, add the import (after line 11's `use crate::core::matter::error::ValidationError;`): + +```rust +use crate::core::matter::error::ParsingError; +``` + +Add the variant inside `SerderError` (after `InvalidPrimitive`, ~line 49): + +```rust + /// Field value could not be parsed as a CESR primitive (malformed code or + /// length) — distinct from a value that parsed but failed validation. + #[error("unparseable primitive in field '{field}': {source}")] + UnparseablePrimitive { + /// The JSON field name. + field: &'static str, + /// The underlying CESR parsing error. + source: ParsingError, + }, +``` + +- [ ] **Step 4: Rewrite `map_qb64_error`** + +In `src/serder/deserialize.rs`, change the import on line 9 to also bring in `MatterBuildError` and `ParsingError`: + +```rust +use crate::core::matter::error::{MatterBuildError, ParsingError, ValidationError}; +``` + +Replace the whole `map_qb64_error` function (currently lines 420–434) with: + +```rust +fn map_qb64_error(field: &'static str, err: MatterBuildError) -> SerderError { + match err { + MatterBuildError::Validation(source) => SerderError::InvalidPrimitive { field, source }, + MatterBuildError::Parsing(source) => SerderError::UnparseablePrimitive { field, source }, + } +} +``` + +(The `ParsingError` import is used by this match arm's type; if clippy reports it unused, remove it — but it is referenced via `SerderError::UnparseablePrimitive`'s inferred type, so keep unless the compiler says otherwise.) + +- [ ] **Step 5: Run the regression test to verify it passes** + +Run: `nix develop --command cargo test --features serder --lib serder::deserialize::tests::unparseable 2>&1 | tail -20` +Expected: PASS. + +- [ ] **Step 6: Run the full serder suite** + +Run: `nix develop --command cargo test --features serder --lib serder 2>&1 | tail -25` +Expected: PASS (existing serder tests unaffected; `parse_qb64_diger`'s `?` now propagates the new error type transparently). + +- [ ] **Step 7: Commit** + +```bash +git add src/serder/error.rs src/serder/deserialize.rs +git commit -m "fix(#33): serder routes ParsingError to a parsing-domain variant, not a stringified ValidationError" +``` + +--- + +## Task 7: Drop the `terrors` dependency + +**Files:** +- Modify: `Cargo.toml` + +- [ ] **Step 1: Verify no source references remain** + +Run: `grep -rn "terrors\|OneOf\|\.narrow::\|\.take::<" src/` +Expected: no matches in `src/`. If any remain, fix them before proceeding (they will fail to compile without `terrors` anyway). + +- [ ] **Step 2: Remove from the `core` feature** + +In `Cargo.toml` line 49, delete `"dep:terrors", ` from the `core` feature list: + +```toml +core = ["b64", "dep:base64", "dep:num-traits", "dep:strum", "dep:zeroize"] +``` + +- [ ] **Step 3: Remove the dependency declaration** + +Delete line 96: `terrors = { version = "0.3.3", optional = true }`. + +- [ ] **Step 4: Verify it builds without terrors** + +Run: `nix develop --command cargo build --features serder 2>&1 | tail -15` +Expected: builds clean, no `terrors` in the resolved graph. + +Run: `nix develop --command bash -c "cargo tree --features serder 2>/dev/null | grep -c terrors"` +Expected: `0`. + +- [ ] **Step 5: Commit** + +```bash +git add Cargo.toml Cargo.lock +git commit -m "chore(#33): drop terrors dependency" +``` + +--- + +## Task 8: CHANGELOG + full gate + +**Files:** +- Modify: `CHANGELOG.md` + +- [ ] **Step 1: Add the CHANGELOG entry** + +Under `## [Unreleased]`, add a `### Changed` section (after the existing `### Added`): + +```markdown +### Changed + +- **error ergonomics (#33):** removed the `terrors::OneOf` error-union layer in + favour of purpose-built `thiserror` enums. **Breaking** (MINOR under 0.x): + - `MatterBuilder::{from_qualified_base64, from_qualified_base2, build}` now return + `Result<_, MatterBuildError>` (variants `Parsing`, `Validation`) instead of + `OneOf<(ParsingError, ValidationError)>`. + - `crypto::verify` now returns `Result<(), VerificationError>` (variants + `Signature`, `CodeMismatch`) instead of `OneOf<(SignatureError, CodeMismatchError)>`. + - The indexer builder's parse/validation methods return the bare + `IndexerParseError` / `IndexerValidationError` (previously wrapped in a + single-element `OneOf`). + - Consumers matching on these results switch from `.take::` / `.narrow::` to a + normal `match` on the new enums / bare types. + - The `terrors` dependency is dropped. + +### Fixed + +- **serder (#33):** a malformed-but-parseable field value no longer collapses a + `ParsingError` into `ValidationError::UnknownMatterCode(..)` via string + formatting; a new `SerderError::UnparseablePrimitive { field, source }` variant + carries the parsing error in its own failure domain. +``` + +- [ ] **Step 2: Commit the CHANGELOG** + +```bash +git add CHANGELOG.md +git commit -m "docs(#33): changelog for OneOf removal + serder parsing-domain fix" +``` + +- [ ] **Step 3: Run the full gate** + +Run: `nix flake check 2>&1 | tail -40` +Expected: all checks pass (clippy god-level, rustfmt, taplo, audit, deny, nextest across feature combos, doctest, `cesr-wasm`, `cesr-nostd`). + +If clippy flags a now-unused import or a leftover allow, fix it and re-run. If `cargo fmt` reports diffs, run `nix develop --command cargo fmt` and amend the relevant commit. + +- [ ] **Step 4: Final verification of the outcome** + +Run: `grep -rn "terrors\|OneOf" src/ Cargo.toml` +Expected: no matches anywhere. + +--- + +## Self-Review Notes + +- **Spec coverage:** Task 1–2 (MatterBuildError), Task 3–4 (VerificationError), Task 5 (indexer bare types), Task 6 (serder variant + bug fix), Task 7 (drop terrors), Task 8 (CHANGELOG + gate). All spec sections mapped. +- **Type consistency:** `MatterBuildError { Parsing, Validation }` and `VerificationError { Signature, CodeMismatch }` used identically in their definition tasks and all consuming/test tasks. `map_qb64_error(field, err: MatterBuildError)` matches Task 1's enum. +- **`PartialEq` caveat** is called out where it matters: matter/indexer errors derive it (tests use `assert_eq!`); crypto errors do not (tests use `matches!`). diff --git a/docs/superpowers/specs/2026-07-04-33-remove-oneof-error-ergonomics-design.md b/docs/superpowers/specs/2026-07-04-33-remove-oneof-error-ergonomics-design.md new file mode 100644 index 0000000..1acd26a --- /dev/null +++ b/docs/superpowers/specs/2026-07-04-33-remove-oneof-error-ergonomics-design.md @@ -0,0 +1,173 @@ +# Remove `terrors::OneOf` — error-ergonomics review (#33) + +**Date:** 2026-07-04 +**Issue:** #33 · P2.3 — Error ergonomics review (Phase 2 · DevX & API) +**Status:** Approved design, ready for implementation plan + +## Problem + +`cesr` returns error unions as `terrors::OneOf<(...)>`. From a downstream seat this +is awkward: + +- Consumers must extract variants with a runtime downcast, `.take::() -> Option` + / `.narrow::()`, instead of a compile-checked `match`. No exhaustiveness, no + compiler help when a new failure mode is added. +- Two of the call sites use **single-element** `OneOf<(E,)>` — a union of one type. + That is pure ceremony: the caller unwraps a "union" that was never a union. +- The union type leaks `terrors` into every downstream `match` and adds a dependency + to the `cargo audit` / `cargo deny` surface. + +CLAUDE.md already points at the exit: *"When `OneOf` grows large (many variants), +group errors into a `thiserror` enum instead."* The unions here are 1–2 variants — +they never earned the machinery. + +## Scope of the actual usage + +The entire crate has only **three distinct `OneOf` shapes**, in four files: + +| Shape | Sites | File | +|---|---|---| +| `OneOf<(ParsingError, ValidationError)>` | `from_qualified_base64`, `from_qualified_base2`, `build` (×2) | `src/core/matter/builder.rs` | +| `OneOf<(SignatureError, CodeMismatchError)>` | `verify`, `verify_*` (2 fns) | `src/crypto/verify.rs` | +| `OneOf<(IndexerParseError,)>` / `OneOf<(IndexerValidationError,)>` | `from_qb64`, `from_qb2`, `with_index`, `with_indices`, `with_raw` (5 sites) | `src/core/indexer/builder.rs` | + +`src/serder/deserialize.rs` does **not** expose `OneOf` publicly — its public API +returns `SerderError`. It only *consumes* the matter builder's `OneOf` inside the +private helper `map_qb64_error`. + +## Design + +### Decisions (locked) + +1. **Replacement:** per-call-site `thiserror` enum (one error enum per module domain, + per CLAUDE.md naming conventions). `#[from]` on each variant so `?` keeps working. +2. **`#[non_exhaustive]`:** **not** added now. Pre-1.0, a new error variant is an + intentional breaking MINOR bump (CLAUDE.md Versioning). Exhaustive matching gives + tag-pinning consumers a compile error exactly when they upgrade into a new failure + mode — that is the desired behaviour until the 1.0 freeze, when `#[non_exhaustive]` + is added deliberately. +3. **Scope:** full removal in one PR. `terrors` dropped from `Cargo.toml` entirely. + +### New / changed error types + +| Module | Today | After | Location | +|---|---|---|---| +| `core::matter` | `OneOf<(ParsingError, ValidationError)>` | **new** `MatterBuildError { Parsing, Validation }` | `src/core/matter/error.rs` | +| `crypto::verify` | `OneOf<(SignatureError, CodeMismatchError)>` | **new** `VerificationError { Signature, CodeMismatch }` | `src/crypto/error.rs` | +| `core::indexer` | `OneOf<(IndexerParseError,)>` / `OneOf<(IndexerValidationError,)>` | **bare** `IndexerParseError` / `IndexerValidationError` | no new type | +| `serder` | consumes matter `OneOf` internally | consumes `MatterBuildError` via `match` | public API unchanged | + +Both new enums follow this shape: + +```rust +#[derive(Debug, Error)] +pub enum MatterBuildError { + #[error(transparent)] + Parsing(#[from] ParsingError), + #[error(transparent)] + Validation(#[from] ValidationError), +} +``` + +```rust +#[derive(Debug, Error)] +pub enum VerificationError { + #[error(transparent)] + Signature(#[from] SignatureError), + #[error(transparent)] + CodeMismatch(#[from] CodeMismatchError), +} +``` + +### Naming rationale + +Noun-form (`MatterBuildError`, `VerificationError`) matches the existing house style +(`SignatureError`, `ValidationError`, `SerderError`, `IndexerParseError`) rather than +action-form (`VerifyError`). + +### Call-site changes + +- **matter builder** (`from_qualified_base64`, `from_qualified_base2`, both `build` + sites returning the 2-variant shape): return type `OneOf<…>` → `MatterBuildError`. + Internal `.map_err(...)` conversions become `?` where `#[from]` covers them. The + existing `build` site that already returns bare `ValidationError` is unchanged. +- **crypto verify** (both functions): `OneOf<…>` → `VerificationError`. +- **indexer builder** (5 sites): drop the `OneOf` wrapper; return the bare + `IndexerParseError` / `IndexerValidationError`. +- **serder** — add a new parsing-domain variant to `SerderError`, since the existing + `InvalidPrimitive { field, source: ValidationError }` is typed to `ValidationError` + and cannot faithfully hold a `ParsingError`: + ```rust + /// Field value could not be parsed as a CESR primitive (malformed code/length). + #[error("unparseable primitive in field '{field}': {source}")] + UnparseablePrimitive { + field: &'static str, + source: ParsingError, + }, + ``` + Then rewrite `map_qb64_error` as: + ```rust + match err { + MatterBuildError::Validation(ve) => SerderError::InvalidPrimitive { field, source: ve }, + MatterBuildError::Parsing(pe) => SerderError::UnparseablePrimitive { field, source: pe }, + } + ``` + This **fixes a latent bug**: today the `Err(remainder)` branch jams a `ParsingError` + into `ValidationError::UnknownMatterCode(parsing_err.to_string())`, collapsing a + parsing failure into a validation variant via string (violates "one variant = one + failure domain" and "never erase structured errors"). A new variant — rather than + retyping `InvalidPrimitive.source` — is used because `InvalidPrimitive` is also + raised by genuinely-validation paths (`parse_sn`, `narrow::()`) that + must keep a `ValidationError` source. In scope because the migration forces a + rewrite of that `match`. + +### Dependency + +- Remove `terrors` from `Cargo.toml`: the `dep:terrors` entry in the `core` feature + (line 49) and the `terrors = { version = "0.3.3", optional = true }` declaration + (line 96). +- Remove the 4 `use terrors::…` / `terrors::` references in `src/`. + +## Testing + +Per CLAUDE.md "Testing — Categories First" and "Test Quality": + +1. **Variant-reachability / source-chain tests** — for each new enum, a test that + drives the real builder/verify path to each variant and asserts + `matches!(e, MatterBuildError::Parsing(_))` etc., and that the `#[from]` source is + preserved (`std::error::Error::source()` chain intact). +2. **Migrate existing extraction tests** — every + `.err().unwrap().take::()` / `.narrow::()` becomes an exhaustive `match` + or `matches!` on the new enum. Assert the specific variant, not "some error". +3. **serder bug-fix regression test** — a genuine *parsing* failure (malformed qb64 + code) fed to the serder deserialize path must now surface as a **parsing-domain** + `SerderError`, not `UnknownMatterCode`. This test must fail against the current + code and pass after the fix. +4. **Cross-feature-combination** — the new enums compile and behave under every + feature combo that reaches them; covered by `nix flake check` running nextest + across combinations plus the wasm and no_std builds. + +## Verification gate + +Full `nix flake check`: clippy (god-level), rustfmt, taplo, `cargo audit`, +`cargo deny`, `cargo nextest` across feature combinations, `cargo test --doc`, +`cesr-wasm`, `cesr-nostd`. + +## Breaking-change note (for PR + CHANGELOG) + +Public return types change on the matter builder (`from_qualified_base64`, +`from_qualified_base2`, `build`), `crypto::verify`, and the indexer builder. Under the +`0.x` convention this is a **MINOR** bump. Consumers matching on these results must +switch from `OneOf` extraction (`.take::` / `.narrow::`) to matching the new enums +(`MatterBuildError`, `VerificationError`) or the bare indexer error types. A new +`SerderError::UnparseablePrimitive` variant is added (additive, but public-enum +change). Call out in the PR description and `CHANGELOG`. + +## Out of scope + +- No change to the underlying component errors (`ParsingError`, `ValidationError`, + `SignatureError`, `CodeMismatchError`, `IndexerParseError`, `IndexerValidationError`) + beyond `#[from]` wiring — their variants are unchanged. +- No `miette`/`snafu` adoption. `thiserror` remains the enum derive; this change only + removes the `OneOf` union layer. +- `#[non_exhaustive]` deferred to the 1.0 stabilization. diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index a657cec..820711c 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -161,7 +161,7 @@ dependencies = [ [[package]] name = "cesr-rs" -version = "0.1.1" +version = "0.3.0" dependencies = [ "base64", "bytes", @@ -173,7 +173,6 @@ dependencies = [ "serde", "serde_json", "strum", - "terrors", "thiserror", "zeroize", ] @@ -831,12 +830,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "terrors" -version = "0.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "987fd8c678ca950df2a18b2c6e9da6ca511d449278fab3565efe0d49c0c07a5d" - [[package]] name = "thiserror" version = "2.0.18" diff --git a/fuzz/tests/__fuzz__/matter_from_qb64/corpus/regression-lead-bytes-oob-5baa b/fuzz/tests/__fuzz__/matter_from_qb64/corpus/regression-lead-bytes-oob-5baa new file mode 100644 index 0000000..5dee4c2 --- /dev/null +++ b/fuzz/tests/__fuzz__/matter_from_qb64/corpus/regression-lead-bytes-oob-5baa @@ -0,0 +1 @@ +5BAA \ No newline at end of file diff --git a/src/core/indexer/builder.rs b/src/core/indexer/builder.rs index 98f9f29..103743f 100644 --- a/src/core/indexer/builder.rs +++ b/src/core/indexer/builder.rs @@ -8,7 +8,6 @@ use alloc::{borrow::ToOwned, format, vec, vec::Vec}; use core::num::NonZeroUsize; use base64::{Engine, engine::general_purpose as b64}; -use terrors::OneOf; use super::code::{IndexMode, IndexedSigCode, hardage}; use super::error::{IndexerParseError, IndexerValidationError}; @@ -78,29 +77,23 @@ impl IndexerBuilder { clippy::too_many_lines, reason = "sequential parsing steps that are clearer together" )] - pub fn from_qb64( - self, - stream: &[u8], - ) -> Result<(Indexer<'static>, usize), OneOf<(IndexerParseError,)>> { - let &first_byte = stream - .first() - .ok_or_else(|| OneOf::new(IndexerParseError::EmptyStream))?; + pub fn from_qb64(self, stream: &[u8]) -> Result<(Indexer<'static>, usize), IndexerParseError> { + let &first_byte = stream.first().ok_or(IndexerParseError::EmptyStream)?; let first_char = char::from(first_byte); let hard_size = hardage(first_char) - .ok_or_else(|| OneOf::new(IndexerParseError::UnknownCode(format!("{first_char}"))))?; + .ok_or_else(|| IndexerParseError::UnknownCode(format!("{first_char}")))?; if stream.len() < hard_size { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: hard_size, got: stream.len(), - })); + }); } let hard = core::str::from_utf8(&stream[..hard_size]) - .map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?; - let code = - IndexedSigCode::from_hard(hard).map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + .map_err(|_| IndexerParseError::InvalidBase64)?; + let code = IndexedSigCode::from_hard(hard).map_err(IndexerParseError::from)?; let xizage = code.get_xizage(); let hs = usize::from(xizage.hs); @@ -111,28 +104,24 @@ impl IndexerBuilder { let ms = ss - os; if stream.len() < cs { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: cs, got: stream.len(), - })); + }); } let index_str = core::str::from_utf8(&stream[hs..hs + ms]) - .map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?; - let index: u32 = - decode_int(index_str).map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + .map_err(|_| IndexerParseError::InvalidBase64)?; + let index: u32 = decode_int(index_str).map_err(IndexerParseError::from)?; let ondex = match code.mode() { IndexMode::CurrentOnly => { if os > 0 { let ondex_str = core::str::from_utf8(&stream[hs + ms..hs + ms + os]) - .map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?; - let ondex_val: u32 = decode_int(ondex_str) - .map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + .map_err(|_| IndexerParseError::InvalidBase64)?; + let ondex_val: u32 = decode_int(ondex_str).map_err(IndexerParseError::from)?; if ondex_val != 0 { - return Err(OneOf::new(IndexerParseError::OndexNotZeroForCurrentOnly( - ondex_val, - ))); + return Err(IndexerParseError::OndexNotZeroForCurrentOnly(ondex_val)); } } None @@ -140,9 +129,8 @@ impl IndexerBuilder { IndexMode::Both => { if os > 0 { let ondex_str = core::str::from_utf8(&stream[hs + ms..hs + ms + os]) - .map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?; - let ondex_val: u32 = decode_int(ondex_str) - .map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + .map_err(|_| IndexerParseError::InvalidBase64)?; + let ondex_val: u32 = decode_int(ondex_str).map_err(IndexerParseError::from)?; Some(ondex_val) } else { Some(index) @@ -163,10 +151,10 @@ impl IndexerBuilder { }; if stream.len() < fs { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: fs, got: stream.len(), - })); + }); } let ps = cs % 4; @@ -176,7 +164,7 @@ impl IndexerBuilder { temp.extend_from_slice(payload); let decoded = b64::URL_SAFE_NO_PAD .decode(&temp) - .map_err(|_| OneOf::new(IndexerParseError::InvalidBase64))?; + .map_err(|_| IndexerParseError::InvalidBase64)?; let skip = if ps != 0 { ps } else { ls }; let raw = decoded[skip..].to_vec(); @@ -192,41 +180,33 @@ impl IndexerBuilder { /// /// Returns [`IndexerParseError`] if the stream is empty, too short, contains /// invalid data, or has an unrecognized code. - pub fn from_qb2( - self, - stream: &[u8], - ) -> Result<(Indexer<'static>, usize), OneOf<(IndexerParseError,)>> { - let &first_byte = stream - .first() - .ok_or_else(|| OneOf::new(IndexerParseError::EmptyStream))?; + pub fn from_qb2(self, stream: &[u8]) -> Result<(Indexer<'static>, usize), IndexerParseError> { + let &first_byte = stream.first().ok_or(IndexerParseError::EmptyStream)?; let first_sextet = first_byte >> 2; let hs: usize = match first_sextet { 0..=51 => 1, 52..=56 => 2, _ => { - return Err(OneOf::new(IndexerParseError::UnknownCode(format!( + return Err(IndexerParseError::UnknownCode(format!( "binary lead byte 0x{first_byte:02x}", - )))); + ))); } }; let bhs = (hs * 3).div_ceil(4); if stream.len() < bhs { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: bhs, got: stream.len(), - })); + }); } - let char_len = NonZeroUsize::new(hs).ok_or_else(|| { - OneOf::new(IndexerParseError::UnknownCode("zero hard size".to_owned())) - })?; - let hard_b64 = encode_binary(&stream[..bhs], char_len) - .map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + let char_len = NonZeroUsize::new(hs) + .ok_or_else(|| IndexerParseError::UnknownCode("zero hard size".to_owned()))?; + let hard_b64 = encode_binary(&stream[..bhs], char_len).map_err(IndexerParseError::from)?; let hard = &hard_b64[..hs]; - let code = - IndexedSigCode::from_hard(hard).map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + let code = IndexedSigCode::from_hard(hard).map_err(IndexerParseError::from)?; let xizage = code.get_xizage(); let ss = usize::from(xizage.ss); @@ -234,24 +214,21 @@ impl IndexerBuilder { let bcs = (cs * 3).div_ceil(4); if stream.len() < bcs { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: bcs, got: stream.len(), - })); + }); } - let cs_nz = NonZeroUsize::new(cs).ok_or_else(|| { - OneOf::new(IndexerParseError::UnknownCode("zero code size".to_owned())) - })?; - let both_b64 = encode_binary(&stream[..bcs], cs_nz) - .map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + let cs_nz = NonZeroUsize::new(cs) + .ok_or_else(|| IndexerParseError::UnknownCode("zero code size".to_owned()))?; + let both_b64 = encode_binary(&stream[..bcs], cs_nz).map_err(IndexerParseError::from)?; let soft = &both_b64[hs..cs]; let os = usize::from(xizage.os); let ms = ss - os; - let index: u32 = - decode_int(&soft[..ms]).map_err(|e| OneOf::new(IndexerParseError::from(e)))?; + let index: u32 = decode_int(&soft[..ms]).map_err(IndexerParseError::from)?; let fs: usize = match xizage.fs { XizageSize::Fixed(n) => usize::from(n), @@ -267,10 +244,10 @@ impl IndexerBuilder { let bfs = fs * 3 / 4; if stream.len() < bfs { - return Err(OneOf::new(IndexerParseError::StreamTooShort { + return Err(IndexerParseError::StreamTooShort { need: bfs, got: stream.len(), - })); + }); } let qb64 = b64::URL_SAFE_NO_PAD.encode(&stream[..bfs]); @@ -290,16 +267,16 @@ impl IndexerBuilder { /// /// Returns [`IndexerValidationError::IndexTooLarge`] if `index` exceeds the /// code's maximum. - pub fn with_index( + pub const fn with_index( self, index: u32, - ) -> Result, OneOf<(IndexerValidationError,)>> { + ) -> Result, IndexerValidationError> { if index > self.state.code.max_index() { - return Err(OneOf::new(IndexerValidationError::IndexTooLarge { + return Err(IndexerValidationError::IndexTooLarge { code: self.state.code, index, max: self.state.code.max_index(), - })); + }); } let ondex = match self.state.code.mode() { IndexMode::Both => Some(index), @@ -327,39 +304,37 @@ impl IndexerBuilder { self, index: u32, ondex: u32, - ) -> Result, OneOf<(IndexerValidationError,)>> { + ) -> Result, IndexerValidationError> { // Reject CurrentOnly codes — they have no ondex field. if self.state.code.mode() == IndexMode::CurrentOnly { - return Err(OneOf::new(IndexerValidationError::OndexOnCurrentOnly( - self.state.code, - ))); + return Err(IndexerValidationError::OndexOnCurrentOnly(self.state.code)); } // Validate index. if index > self.state.code.max_index() { - return Err(OneOf::new(IndexerValidationError::IndexTooLarge { + return Err(IndexerValidationError::IndexTooLarge { code: self.state.code, index, max: self.state.code.max_index(), - })); + }); } // Validate ondex. if let Some(max_ondex) = self.state.code.max_ondex() && ondex > max_ondex { - return Err(OneOf::new(IndexerValidationError::OndexTooLarge { + return Err(IndexerValidationError::OndexTooLarge { code: self.state.code, ondex, max: max_ondex, - })); + }); } // When os=0 the wire format has no space for a separate ondex, so // ondex must equal index (matching keripy's InvalidVarIndexError). if self.state.code.get_xizage().os() == 0 && ondex != index { - return Err(OneOf::new(IndexerValidationError::OndexMustEqualIndex { + return Err(IndexerValidationError::OndexMustEqualIndex { code: self.state.code, index, ondex, - })); + }); } Ok(IndexerBuilder { state: IWithIndex { @@ -382,15 +357,15 @@ impl IndexerBuilder { pub fn with_raw<'a>( self, raw: impl Into>, - ) -> Result, OneOf<(IndexerValidationError,)>> { + ) -> Result, IndexerValidationError> { let raw_bytes = raw.into(); let expected = self.state.code.raw_size(); if raw_bytes.len() != expected { - return Err(OneOf::new(IndexerValidationError::UnexpectedRawSize { + return Err(IndexerValidationError::UnexpectedRawSize { code: self.state.code, expected, got: raw_bytes.len(), - })); + }); } Ok(Indexer::new( self.state.code, @@ -556,8 +531,7 @@ mod tests { .with_code(IndexedSigCode::Ed25519) .with_index(64) .err() - .unwrap() - .take::(); + .unwrap(); assert_eq!( err, IndexerValidationError::IndexTooLarge { @@ -576,8 +550,7 @@ mod tests { .unwrap() .with_raw(&[0u8; 64]) .err() - .unwrap() - .take::(); + .unwrap(); assert_eq!( err, IndexerValidationError::UnexpectedRawSize { @@ -594,8 +567,7 @@ mod tests { .with_code(IndexedSigCode::ECDSA256k1Crt) .with_indices(0, 0) .err() - .unwrap() - .take::(); + .unwrap(); assert_eq!( err, IndexerValidationError::OndexOnCurrentOnly(IndexedSigCode::ECDSA256k1Crt) @@ -608,8 +580,7 @@ mod tests { .with_code(IndexedSigCode::Ed25519Big) .with_indices(0, 4096) // max_ondex for Ed25519Big (os=2) is 4095 .err() - .unwrap() - .take::(); + .unwrap(); assert_eq!( err, IndexerValidationError::OndexTooLarge { diff --git a/src/core/matter/builder.rs b/src/core/matter/builder.rs index 2f77d49..5b29e40 100644 --- a/src/core/matter/builder.rs +++ b/src/core/matter/builder.rs @@ -1,7 +1,7 @@ use super::{ MatterPart, code::{CesrCode, MatterCode}, - error::{ParsingError, ValidationError}, + error::{MatterBuildError, ParsingError, ValidationError}, matter::Matter, sizage::{Sizage, SizeType}, }; @@ -15,7 +15,6 @@ use alloc::borrow::Cow; use alloc::{borrow::ToOwned, format, string::String, string::ToString, vec, vec::Vec}; use base64::{Engine, decoded_len_estimate, engine::general_purpose as b64}; use core::num::NonZeroUsize; -use terrors::OneOf; /// Marker trait for the type-state pattern used by [`MatterBuilder`]. pub trait MatterBuilderState {} @@ -98,40 +97,42 @@ impl MatterBuilder { pub fn from_qualified_base64<'a>( self, input: impl Into>, - ) -> Result, OneOf<(ParsingError, ValidationError)>> { + ) -> Result, MatterBuildError> { let stream = input.into(); if stream.is_empty() { - return Err(OneOf::new(ParsingError::EmptyStream)); + return Err(MatterBuildError::from(ParsingError::EmptyStream)); } - let code = MatterCode::from_base64_stream(&stream).map_err(OneOf::new)?; + let code = MatterCode::from_base64_stream(&stream)?; let hs = code.get_sizage().hs(); let ss = code.get_sizage().ss(); let cs = hs + ss; if stream.len() < cs { - return Err(OneOf::new(ParsingError::StreamTooShort(MatterPart::Soft))); + return Err(MatterBuildError::from(ParsingError::StreamTooShort( + MatterPart::Soft, + ))); } let soft_full = &stream[hs..cs]; let xs = code.get_sizage().xs(); let xtra = &soft_full[..xs]; let soft_tail = &soft_full[xs..]; if xtra != Matter::::PAD.repeat(xs).as_bytes() { - return Err(OneOf::new(ParsingError::MalformedCode { + return Err(MatterBuildError::from(ParsingError::MalformedCode { part: MatterPart::Xtra, found: Matter::::PAD.repeat(xs), })); } - let soft_str = - str::from_utf8(soft_tail).map_err(|err| OneOf::new(ParsingError::InvalidUtf8(err)))?; + let soft_str = str::from_utf8(soft_tail) + .map_err(|err| MatterBuildError::from(ParsingError::InvalidUtf8(err)))?; let fs = if let SizeType::Fixed(fixed) = code.get_sizage().fs() { usize::from(*fixed) } else { - let size: usize = - decode_int(soft_str).map_err(|err| OneOf::new(ParsingError::Conversion(err)))?; + let size: usize = decode_int(soft_str) + .map_err(|err| MatterBuildError::from(ParsingError::Conversion(err)))?; (size * 4) + cs }; if stream.len() < fs { - return Err(OneOf::new(ValidationError::IncorrectRawSize { + return Err(MatterBuildError::from(ValidationError::IncorrectRawSize { code: code.to_string(), expected: fs, found: stream.len(), @@ -148,7 +149,7 @@ impl MatterBuilder { let mut buf: Vec = Vec::with_capacity(estimate); b64::URL_SAFE .decode_vec(temp, &mut buf) - .map_err(|err| OneOf::new(ParsingError::Base64(err)))?; + .map_err(|err| MatterBuildError::from(ParsingError::Base64(err)))?; if ps != 0 { let pbs = 2 * ps; @@ -158,26 +159,34 @@ impl MatterBuilder { pi = (pi << 8) | u32::from(byte); } if (pi & mask) != 0 { - return Err(OneOf::new(ValidationError::NonCanonicalEncoding( - MatterPart::PadBits, - ))); + return Err(MatterBuildError::from( + ValidationError::NonCanonicalEncoding(MatterPart::PadBits), + )); } let ls = code.get_sizage().ls(); - let lead_bytes = &buf[ps..(ps + ls)]; + let Some(lead_bytes) = buf.get(ps..(ps + ls)) else { + return Err(MatterBuildError::from( + ValidationError::StructuralIntegrityError, + )); + }; if ls > 0 && lead_bytes.iter().any(|&b| b != 0) { - return Err(OneOf::new(ValidationError::NonCanonicalEncoding( - MatterPart::LeadBytes, - ))); + return Err(MatterBuildError::from( + ValidationError::NonCanonicalEncoding(MatterPart::LeadBytes), + )); } buf.drain(..(ps + ls)); } else { let ls = code.get_sizage().ls(); if ls > 0 { - let lead_bytes = &buf[..ls]; + let Some(lead_bytes) = buf.get(..ls) else { + return Err(MatterBuildError::from( + ValidationError::StructuralIntegrityError, + )); + }; if lead_bytes.iter().any(|&b| b != 0) { - return Err(OneOf::new(ValidationError::NonCanonicalEncoding( - MatterPart::LeadBytes, - ))); + return Err(MatterBuildError::from( + ValidationError::NonCanonicalEncoding(MatterPart::LeadBytes), + )); } buf.drain(..ls); } @@ -190,12 +199,12 @@ impl MatterBuilder { let soft: Cow<'a, str> = match &stream { Cow::Borrowed(b) => { let s = str::from_utf8(&b[soft_start..soft_end]) - .map_err(|err| OneOf::new(ParsingError::InvalidUtf8(err)))?; + .map_err(|err| MatterBuildError::from(ParsingError::InvalidUtf8(err)))?; Cow::Borrowed(s) } Cow::Owned(v) => { let s = str::from_utf8(&v[soft_start..soft_end]) - .map_err(|err| OneOf::new(ParsingError::InvalidUtf8(err)))?; + .map_err(|err| MatterBuildError::from(ParsingError::InvalidUtf8(err)))?; Cow::Owned(s.to_owned()) } }; @@ -216,30 +225,32 @@ impl MatterBuilder { pub fn from_qualified_base2( self, stream: &[u8], - ) -> Result, OneOf<(ParsingError, ValidationError)>> { + ) -> Result, MatterBuildError> { if stream.is_empty() { - return Err(OneOf::new(ParsingError::EmptyStream)); + return Err(MatterBuildError::from(ParsingError::EmptyStream)); } - let code = MatterCode::from_stream(stream).map_err(OneOf::new)?; + let code = MatterCode::from_stream(stream)?; let hs = code.get_sizage().hs(); let ss = code.get_sizage().ss(); let cs = hs + ss; let bcs = (cs * 3).div_ceil(4); if stream.len() < bcs { - return Err(OneOf::new(ParsingError::StreamTooShort(MatterPart::Soft))); + return Err(MatterBuildError::from(ParsingError::StreamTooShort( + MatterPart::Soft, + ))); } - let char_len = - NonZeroUsize::new(cs).ok_or_else(|| OneOf::new(ParsingError::EmptyStream))?; + let char_len = NonZeroUsize::new(cs) + .ok_or_else(|| MatterBuildError::from(ParsingError::EmptyStream))?; let both = encode_binary(&stream[..bcs], char_len) - .map_err(|err| OneOf::new(ParsingError::Conversion(err)))?; + .map_err(|err| MatterBuildError::from(ParsingError::Conversion(err)))?; let soft_full = &both[hs..cs]; let xs = code.get_sizage().xs(); let xtra = &soft_full[..xs]; let soft_tail = &soft_full[xs..]; if xtra != Matter::::PAD.repeat(xs) { - return Err(OneOf::new(ParsingError::MalformedCode { + return Err(MatterBuildError::from(ParsingError::MalformedCode { part: MatterPart::Xtra, found: Matter::::PAD.repeat(xs), })); @@ -247,13 +258,13 @@ impl MatterBuilder { let fs = if let SizeType::Fixed(fixed) = code.get_sizage().fs() { usize::from(*fixed) } else { - let size: usize = - decode_int(soft_tail).map_err(|err| OneOf::new(ParsingError::Conversion(err)))?; + let size: usize = decode_int(soft_tail) + .map_err(|err| MatterBuildError::from(ParsingError::Conversion(err)))?; (size * 4) + cs }; let bfs = (fs * 3).div_ceil(4); if stream.len() < bfs { - return Err(OneOf::new(ValidationError::IncorrectRawSize { + return Err(MatterBuildError::from(ValidationError::IncorrectRawSize { code: code.to_string(), expected: bfs, found: stream.len(), @@ -263,9 +274,9 @@ impl MatterBuilder { let ls = code.get_sizage().ls(); let lead_end = bcs .checked_add(ls) - .ok_or_else(|| OneOf::new(ValidationError::StructuralIntegrityError))?; + .ok_or_else(|| MatterBuildError::from(ValidationError::StructuralIntegrityError))?; if lead_end > trimmed.len() { - return Err(OneOf::new(ValidationError::IncorrectRawSize { + return Err(MatterBuildError::from(ValidationError::IncorrectRawSize { code: code.to_string(), expected: lead_end, found: trimmed.len(), @@ -282,21 +293,23 @@ impl MatterBuilder { let mut pi = trimmed[bcs - 1]; pi &= 2_u8.pow(pbs) - 1; if pi != 0 { - return Err(OneOf::new(ValidationError::NonCanonicalEncoding( - MatterPart::PadBits, - ))); + return Err(MatterBuildError::from( + ValidationError::NonCanonicalEncoding(MatterPart::PadBits), + )); } } let li = &trimmed[bcs..lead_end]; if ls > 0 && li.iter().any(|&b| b != 0) { - return Err(OneOf::new(ValidationError::NonCanonicalEncoding( - MatterPart::LeadBytes, - ))); + return Err(MatterBuildError::from( + ValidationError::NonCanonicalEncoding(MatterPart::LeadBytes), + )); } let raw = &trimmed[lead_end..]; if raw.len() != trimmed.len() - lead_end { - return Err(OneOf::new(ValidationError::StructuralIntegrityError)); + return Err(MatterBuildError::from( + ValidationError::StructuralIntegrityError, + )); } Ok(Matter::new( @@ -353,10 +366,6 @@ impl MatterBuilder> { } } -#[allow( - clippy::type_complexity, - reason = "OneOf error union is inherently complex" -)] impl<'a, C: CesrCode> MatterBuilder> { /// Builds the [`Matter`] from raw bytes alone (no soft value). /// @@ -364,24 +373,24 @@ impl<'a, C: CesrCode> MatterBuilder> { /// /// Returns [`ParsingError`] or [`ValidationError`] if the code requires /// a soft value, or the raw size is incorrect. - pub fn build(self) -> Result, OneOf<(ParsingError, ValidationError)>> { + pub fn build(self) -> Result, MatterBuildError> { let WithRaw { code, raw: raw_val } = self.state; let mc = code.to_matter_code(); let Sizage { ss, fs, .. } = mc.get_sizage(); match fs { SizeType::Fixed(_) => { if ss > 0 { - return Err(OneOf::new(ValidationError::MissingSoft { + return Err(MatterBuildError::from(ValidationError::MissingSoft { code: mc.to_string(), })); } - let raw_size = mc.raw_size().map_err(OneOf::new)?; - let trimmed = validate_and_trim_raw(mc, raw_val, raw_size).map_err(OneOf::new)?; + let raw_size = mc.raw_size()?; + let trimmed = validate_and_trim_raw(mc, raw_val, raw_size)?; Ok(Matter::new(code, trimmed, Cow::from(""))) } - _ => Err(OneOf::new(ValidationError::InvalidSizingOperation( - mc.to_string(), - ))), + _ => Err(MatterBuildError::from( + ValidationError::InvalidSizingOperation(mc.to_string()), + )), } } @@ -406,10 +415,6 @@ impl<'a, C: CesrCode> MatterBuilder> { } } -#[allow( - clippy::type_complexity, - reason = "OneOf error union is inherently complex" -)] impl<'a, C: CesrCode> MatterBuilder> { /// Builds the [`Matter`] from raw bytes and a soft value. /// @@ -417,7 +422,7 @@ impl<'a, C: CesrCode> MatterBuilder> { /// /// Returns [`ParsingError`] or [`ValidationError`] if sizes are invalid /// or the soft format is incorrect. - pub fn build(self) -> Result, OneOf<(ParsingError, ValidationError)>> { + pub fn build(self) -> Result, MatterBuildError> { let WithRawAndSoft { soft, code, @@ -428,19 +433,18 @@ impl<'a, C: CesrCode> MatterBuilder> { match fs { SizeType::Fixed(_) => { if ss == 0 { - let raw_size = mc.raw_size().map_err(OneOf::new)?; - let trimmed = - validate_and_trim_raw(mc, raw_val, raw_size).map_err(OneOf::new)?; + let raw_size = mc.raw_size()?; + let trimmed = validate_and_trim_raw(mc, raw_val, raw_size)?; return Ok(Matter::new(code, trimmed, Cow::from(""))); } - let final_soft = extract_soft(mc, ss, xs, soft).map_err(OneOf::new)?; - let raw_size = mc.raw_size().map_err(OneOf::new)?; - let trimmed = validate_and_trim_raw(mc, raw_val, raw_size).map_err(OneOf::new)?; + let final_soft = extract_soft(mc, ss, xs, soft)?; + let raw_size = mc.raw_size()?; + let trimmed = validate_and_trim_raw(mc, raw_val, raw_size)?; Ok(Matter::new(code, trimmed, Cow::Borrowed(final_soft))) } - _ => Err(OneOf::new(ValidationError::InvalidSizingOperation( - mc.to_string(), - ))), + _ => Err(MatterBuildError::from( + ValidationError::InvalidSizingOperation(mc.to_string()), + )), } } } @@ -542,10 +546,29 @@ fn validate_and_trim_raw( #[cfg(test)] #[allow(clippy::panic, reason = "tests use panic via unwrap/assert macros")] mod tests { - use super::{MatterBuilder, Start, validate_and_trim_raw}; + use super::{MatterBuildError, MatterBuilder, Start, ValidationError, validate_and_trim_raw}; use crate::core::matter::code::MatterCode; use std::{format, string::String, vec, vec::Vec}; + #[test] + fn qb64_lead_bytes_slice_does_not_panic_on_short_buffer() { + // Regression (deep-fuzz `matter_from_qb64`): input `5BAA` panicked with + // "range end index 1 out of range for slice of length 0" — the lead-byte + // slice indexed past a decoded buffer shorter than the code's declared lead + // size. Parsing untrusted bytes must never panic; it must return a typed error. + let err = MatterBuilder::new() + .from_qualified_base64(b"5BAA".as_slice()) + .err() + .expect("`5BAA` must be rejected, not accepted"); + assert!( + matches!( + err, + MatterBuildError::Validation(ValidationError::StructuralIntegrityError) + ), + "expected StructuralIntegrityError, got {err:?}" + ); + } + #[test] fn should_extract_raw_size_based() { use alloc::borrow::Cow; @@ -730,9 +753,9 @@ mod tests { .from_qualified_base2(crafted) .err() .expect("crafted qb2 with bfs < bcs + ls must be rejected, not parsed"); - let validation = err - .narrow::() - .expect("expected a ValidationError for a decoded size too small to hold lead+raw"); + let crate::core::matter::error::MatterBuildError::Validation(validation) = err else { + panic!("expected Validation variant, got {err:?}"); + }; assert!( matches!( validation, diff --git a/src/core/matter/error.rs b/src/core/matter/error.rs index c10d714..e936835 100644 --- a/src/core/matter/error.rs +++ b/src/core/matter/error.rs @@ -169,3 +169,55 @@ pub enum ValidationError { )] StructuralIntegrityError, } + +/// Error returned by [`MatterBuilder`](super::builder::MatterBuilder) parse and +/// build operations. +/// +/// The input was either structurally unparseable ([`ParsingError`]) or it +/// parsed but violated a CESR validation rule ([`ValidationError`]). +#[derive(Debug, ThisError, PartialEq, Eq)] +pub enum MatterBuildError { + /// The input could not be parsed into a CESR primitive. + #[error(transparent)] + Parsing(#[from] ParsingError), + + /// The input parsed but failed a validation constraint. + #[error(transparent)] + Validation(#[from] ValidationError), +} + +#[cfg(test)] +mod build_error_tests { + use super::{MatterBuildError, ParsingError, ValidationError}; + use alloc::string::ToString; + + #[test] + fn from_parsing_error_lands_in_parsing_variant() { + let e: MatterBuildError = ParsingError::EmptyStream.into(); + assert_eq!(e, MatterBuildError::Parsing(ParsingError::EmptyStream)); + } + + #[test] + fn from_validation_error_lands_in_validation_variant() { + let e: MatterBuildError = ValidationError::StructuralIntegrityError.into(); + assert_eq!( + e, + MatterBuildError::Validation(ValidationError::StructuralIntegrityError) + ); + } + + #[test] + fn display_is_transparent_to_source() { + let e: MatterBuildError = ParsingError::EmptyStream.into(); + assert_eq!(e.to_string(), ParsingError::EmptyStream.to_string()); + } + + #[test] + fn validation_display_is_transparent_to_source() { + let e: MatterBuildError = ValidationError::StructuralIntegrityError.into(); + assert_eq!( + e.to_string(), + ValidationError::StructuralIntegrityError.to_string() + ); + } +} diff --git a/src/crypto/error.rs b/src/crypto/error.rs index cd828db..7b87845 100644 --- a/src/crypto/error.rs +++ b/src/crypto/error.rs @@ -94,6 +94,22 @@ pub enum CodeMismatchError { }, } +/// Error returned by signature verification. +/// +/// Either the verifying-key and signature CESR codes are incompatible +/// ([`CodeMismatchError`]) or the cryptographic check failed +/// ([`SignatureError`]). +#[derive(Debug, thiserror::Error)] +pub enum VerificationError { + /// The cryptographic verification failed or the key/signature bytes were malformed. + #[error(transparent)] + Signature(#[from] SignatureError), + + /// The signature's CESR code does not belong to the verifying key's algorithm. + #[error(transparent)] + CodeMismatch(#[from] CodeMismatchError), +} + #[cfg(test)] mod tests { use super::*; @@ -226,4 +242,49 @@ mod tests { let debug = format!("{err:?}"); assert!(debug.contains("IncompatibleCodes")); } + + #[test] + fn verification_error_from_signature_error() { + let e: VerificationError = SignatureError::Invalid.into(); + assert!(matches!( + e, + VerificationError::Signature(SignatureError::Invalid) + )); + } + + #[test] + fn verification_error_from_code_mismatch() { + let cm = CodeMismatchError::IncompatibleCodes { + verkey: "Ed25519".into(), + signature: "ECDSA256k1Sig".into(), + }; + let e: VerificationError = cm.into(); + assert!(matches!( + e, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) + )); + } + + #[test] + fn verification_error_code_mismatch_display_is_transparent() { + let cm = CodeMismatchError::IncompatibleCodes { + verkey: "Ed25519".into(), + signature: "ECDSA256k1Sig".into(), + }; + let e: VerificationError = cm.into(); + assert_eq!( + e.to_string(), + CodeMismatchError::IncompatibleCodes { + verkey: "Ed25519".into(), + signature: "ECDSA256k1Sig".into(), + } + .to_string() + ); + } + + #[test] + fn verification_error_display_is_transparent() { + let e: VerificationError = SignatureError::Invalid.into(); + assert_eq!(e.to_string(), SignatureError::Invalid.to_string()); + } } diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index 9d984ba..c2d4f87 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -24,7 +24,7 @@ pub mod verify; // Re-exports for convenience pub use algo::{Algorithm, Ed25519, Secp256k1, Secp256r1}; pub use digest::digest; -pub use error::{CodeMismatchError, DigestError, KeyError, SignatureError}; +pub use error::{CodeMismatchError, DigestError, KeyError, SignatureError, VerificationError}; pub use keypair::KeyPair; pub use signature::Signature; pub use verify::verify; diff --git a/src/crypto/verify.rs b/src/crypto/verify.rs index 5e3bab7..abef868 100644 --- a/src/crypto/verify.rs +++ b/src/crypto/verify.rs @@ -6,10 +6,9 @@ use crate::core::primitives::Verfer; reason = "alloc prelude items; subset used per cfg/feature combination" )] use alloc::{format, string::ToString, vec}; -use terrors::OneOf; use crate::crypto::algo::{Algorithm, Ed25519, Secp256k1, Secp256r1}; -use crate::crypto::error::{CodeMismatchError, SignatureError}; +use crate::crypto::error::{CodeMismatchError, SignatureError, VerificationError}; use crate::crypto::signature::Signature; /// Verifies `sig` over `data` using the algorithm indicated by `verfer`'s CESR @@ -31,15 +30,16 @@ use crate::crypto::signature::Signature; /// /// # Errors /// -/// Returns [`CodeMismatchError`] if the signature's code does not match -/// `verfer`'s algorithm (or the verkey code is unsupported, e.g. Ed448), -/// [`SignatureError::Invalid`] if the signature does not match, or another -/// [`SignatureError`] if the key or signature bytes are malformed. +/// Returns [`VerificationError::CodeMismatch`] if the signature's code does not +/// match `verfer`'s algorithm (or the verkey code is unsupported, e.g. Ed448), +/// or [`VerificationError::Signature`] wrapping [`SignatureError::Invalid`] if +/// the signature does not match — or another [`SignatureError`] if the key or +/// signature bytes are malformed. pub fn verify( verfer: &Verfer<'_>, data: &[u8], sig: &S, -) -> Result<(), OneOf<(SignatureError, CodeMismatchError)>> { +) -> Result<(), VerificationError> { match verfer.code() { VerKeyCode::Ed25519 | VerKeyCode::Ed25519N => verify_as::(verfer, data, sig), VerKeyCode::ECDSA256k1 | VerKeyCode::ECDSA256k1N => { @@ -48,12 +48,12 @@ pub fn verify( VerKeyCode::ECDSA256r1 | VerKeyCode::ECDSA256r1N => { verify_as::(verfer, data, sig) } - VerKeyCode::Ed448 | VerKeyCode::Ed448N => { - Err(OneOf::new(CodeMismatchError::IncompatibleCodes { + VerKeyCode::Ed448 | VerKeyCode::Ed448N => Err(VerificationError::CodeMismatch( + CodeMismatchError::IncompatibleCodes { verkey: format!("{:?}", verfer.code()), signature: sig.code_name(), - })) - } + }, + )), } } @@ -63,14 +63,16 @@ fn verify_as( verfer: &Verfer<'_>, data: &[u8], sig: &S, -) -> Result<(), OneOf<(SignatureError, CodeMismatchError)>> { +) -> Result<(), VerificationError> { if !sig.belongs_to::() { - return Err(OneOf::new(CodeMismatchError::IncompatibleCodes { - verkey: format!("{:?}", verfer.code()), - signature: sig.code_name(), - })); + return Err(VerificationError::CodeMismatch( + CodeMismatchError::IncompatibleCodes { + verkey: format!("{:?}", verfer.code()), + signature: sig.code_name(), + }, + )); } - A::verify_bytes(verfer.raw(), data, sig.raw()).map_err(OneOf::new) + A::verify_bytes(verfer.raw(), data, sig.raw()).map_err(VerificationError::from) } pub(crate) fn verify_ed25519(key: &[u8], data: &[u8], sig: &[u8]) -> Result<(), SignatureError> { @@ -201,8 +203,8 @@ mod tests { let err = verify(&verfer, b"wrong", &sig).err().unwrap(); assert!(matches!( - err.narrow::(), - Ok(SignatureError::Invalid) + err, + VerificationError::Signature(SignatureError::Invalid) )); } @@ -279,8 +281,8 @@ mod tests { // is rejected as a code mismatch before any crypto is attempted. let err = verify(&verfer_e, b"test", &sig_k).err().unwrap(); assert!(matches!( - err.narrow::(), - Ok(CodeMismatchError::IncompatibleCodes { .. }) + err, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) )); } @@ -465,8 +467,8 @@ mod tests { let err = verify(&verfer, b"tampered", &siger).err().unwrap(); assert!(matches!( - err.narrow::(), - Ok(SignatureError::Invalid) + err, + VerificationError::Signature(SignatureError::Invalid) )); } @@ -481,8 +483,8 @@ mod tests { let err = verify(&ed_verfer, b"event", &k1_siger).err().unwrap(); assert!(matches!( - err.narrow::(), - Ok(CodeMismatchError::IncompatibleCodes { .. }) + err, + VerificationError::CodeMismatch(CodeMismatchError::IncompatibleCodes { .. }) )); } diff --git a/src/serder/deserialize.rs b/src/serder/deserialize.rs index cca0521..385e5e2 100644 --- a/src/serder/deserialize.rs +++ b/src/serder/deserialize.rs @@ -6,7 +6,7 @@ use crate::core::matter::builder::MatterBuilder; use crate::core::matter::code::{DigestCode, MatterCode, VerKeyCode}; -use crate::core::matter::error::ValidationError; +use crate::core::matter::error::{MatterBuildError, ValidationError}; use crate::core::primitives::{Diger, Prefixer, Saider, Seqner, Tholder, Verfer}; use crate::keri::{ ConfigTrait, DelegatedInceptionEvent, DelegatedRotationEvent, Identifier, Ilk, InceptionEvent, @@ -417,19 +417,10 @@ fn parse_qb64_saider(s: &str, field: &'static str) -> Result, Se parse_qb64_diger(s, field) } -fn map_qb64_error( - field: &'static str, - err: terrors::OneOf<(crate::core::matter::error::ParsingError, ValidationError)>, -) -> SerderError { - match err.narrow::() { - Ok(ve) => SerderError::InvalidPrimitive { field, source: ve }, - Err(remainder) => { - let parsing_err = remainder.take::(); - SerderError::InvalidPrimitive { - field, - source: ValidationError::UnknownMatterCode(parsing_err.to_string()), - } - } +fn map_qb64_error(field: &'static str, err: MatterBuildError) -> SerderError { + match err { + MatterBuildError::Validation(source) => SerderError::InvalidPrimitive { field, source }, + MatterBuildError::Parsing(source) => SerderError::UnparseablePrimitive { field, source }, } } @@ -673,6 +664,7 @@ mod tests { use super::*; use crate::core::matter::builder::MatterBuilder; use crate::core::matter::code::{CesrCode, DigestCode, VerKeyCode}; + use crate::core::matter::error::ParsingError; use crate::core::primitives::{Diger, Prefixer, Saider, Seqner, Tholder, Verfer}; use crate::keri::{ DelegatedInceptionEvent, DelegatedRotationEvent, InceptionEvent, InteractionEvent, @@ -1307,4 +1299,48 @@ mod tests { let (n, d) = parse_weight("1").unwrap(); assert_eq!((n, d), (1, 1)); } + + // ----------------------------------------------------------------------- + // Parse-failure routing: a malformed qb64 code is a parsing-domain error + // ----------------------------------------------------------------------- + + #[test] + fn unparseable_qb64_field_surfaces_as_parsing_domain_error() { + // A malformed qb64 primitive (bad code) is a parse failure, not a + // validation failure — it must not be collapsed into a ValidationError. + // `Diger`/`Matter` does not implement `Debug`, so `matches!` on the + // whole `Result` avoids requiring the `Ok` value to be printable. + let result = parse_qb64_diger("!!not-qb64!!", "d"); + assert!( + matches!( + result, + Err(SerderError::UnparseablePrimitive { field: "d", .. }) + ), + "expected UnparseablePrimitive parse-domain error" + ); + } + + #[test] + fn map_qb64_error_routes_validation_to_invalid_primitive() { + // The Validation arm must land in InvalidPrimitive — the other half of the + // routing the bug corrupted (it previously misrouted Parsing into a + // stringified ValidationError). Pin both directions. + let err = map_qb64_error( + "d", + MatterBuildError::Validation(ValidationError::StructuralIntegrityError), + ); + assert!( + matches!(err, SerderError::InvalidPrimitive { field: "d", .. }), + "expected InvalidPrimitive, got {err:?}" + ); + } + + #[test] + fn map_qb64_error_routes_parsing_to_unparseable_primitive() { + let err = map_qb64_error("d", MatterBuildError::Parsing(ParsingError::EmptyStream)); + assert!( + matches!(err, SerderError::UnparseablePrimitive { field: "d", .. }), + "expected UnparseablePrimitive, got {err:?}" + ); + } } diff --git a/src/serder/error.rs b/src/serder/error.rs index af9387b..fae2922 100644 --- a/src/serder/error.rs +++ b/src/serder/error.rs @@ -8,7 +8,7 @@ use alloc::string::FromUtf8Error; )] use alloc::string::String; -use crate::core::matter::error::ValidationError; +use crate::core::matter::error::{ParsingError, ValidationError}; use crate::stream::error::ParseError; /// Errors during KERI event serialization, deserialization, and SAID computation. @@ -48,6 +48,16 @@ pub enum SerderError { source: ValidationError, }, + /// Field value could not be parsed as a CESR primitive (malformed code or + /// length) — distinct from a value that parsed but failed validation. + #[error("unparseable primitive in field '{field}': {source}")] + UnparseablePrimitive { + /// The JSON field name. + field: &'static str, + /// The underlying CESR parsing error. + source: ParsingError, + }, + /// Digest computation failed. #[error("digest error: {0}")] DigestError(String), diff --git a/src/stream/parse.rs b/src/stream/parse.rs index be3edda..d0d80e1 100644 --- a/src/stream/parse.rs +++ b/src/stream/parse.rs @@ -17,8 +17,7 @@ use crate::core::matter::code::SignatureCode; use crate::core::matter::code::TexterCode; use crate::core::matter::code::VerKeyCode; use crate::core::matter::code::VerserCode; -use crate::core::matter::error::ParsingError as MatterParsingError; -use crate::core::matter::error::ValidationError as MatterValidationError; +use crate::core::matter::error::MatterBuildError; use crate::core::matter::sizage::SizeType; use crate::core::primitives::Cigar; use crate::core::primitives::Diger; @@ -78,14 +77,10 @@ pub(crate) fn parse_matter( let matter = MatterBuilder::new() .from_qualified_base64(&input[..fs]) - .map_err( - |oneof_err| match oneof_err.narrow::() { - Ok(pe) => ParseError::from(pe), - Err(remaining) => remaining - .narrow::() - .map_or_else(|_| unreachable!(), ParseError::from), - }, - )? + .map_err(|err| match err { + MatterBuildError::Parsing(pe) => ParseError::from(pe), + MatterBuildError::Validation(ve) => ParseError::from(ve), + })? .into_static(); Ok((matter, &input[fs..])) @@ -100,7 +95,7 @@ pub(crate) fn parse_indexer(input: &[u8]) -> Result<(Indexer<'static>, &[u8]), P } let (indexer, consumed) = IndexerBuilder::new() .from_qb64(input) - .map_err(|e| ParseError::from(e.take()))?; + .map_err(ParseError::from)?; Ok((indexer, &input[consumed..])) }