fix(bpv7): the extension editor refuses what the parser rejects

bpa/src/filter/validity.rs

@@ -21,10 +21,11 @@ pub struct BundleValidityFilter;
 #[async_trait]
 impl ReadFilter for BundleValidityFilter {
     async fn filter(&self, bundle: &Bundle, _data: &[u8]) -> Result<ReadResult, crate::Error> {
-        if let Some(u) = bundle.primary().flags.unrecognised {
+        let unrecognised = bundle.primary().flags.unrecognised;
+        if unrecognised != 0 {
             debug!(
                 bundle_id = %bundle.id(),
-                "Bundle primary block has unrecognised flag bits set: {u:#x}"
+                "Bundle primary block has unrecognised flag bits set: {unrecognised:#x}"
             );
         }
 

bpv7/CHANGELOG.md

@@ -8,13 +8,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Added
 - `bpsec::DecryptingReader`: a `reader::Reader` that decrypts BCB-covered blocks on demand and memoises each block's outcome — plaintext, no-key, or decrypt-failure — for the reader's lifetime, so a chain of consumers sharing one reader through the trait costs one decrypt attempt per covered block (the give doors re-run a cached failure to report its exact cause). The trait impl lends (cache borrows, zeroized when the reader drops); the inherent `block_data()` gives (owned plaintext, typed errors carrying the diagnostic cause, and `Ok(None)` for non-resident extents), preserving the free `bpsec::block_data` contract; `into_block_data()` is its consuming form, which moves a covered block's plaintext out instead of cloning it, for callers that read one block and drop the reader.
-- `extension_editor::ExtensionEditor`: scoped extension-block editing over a parsed bundle — insert/replace/remove of extension blocks only. Some owner privileges are absent from the type (the primary-field setters, BIB/BCB creation and management, coverage stripping); target gates refuse the rest at runtime with a typed error — the primary and payload blocks, the reserved block types, security blocks as targets, and covered targets (unprovable coverage refuses conservatively). Blocks inserted through the editor are valid targets for its own replace/remove; `finish()` materialises the accumulated edits, or `None` when untouched.
+- `extension_editor::ExtensionEditor`: scoped extension-block editing over a parsed bundle — insert/replace/remove of extension blocks only. Some owner privileges are absent from the type (the primary-field setters, BIB/BCB creation and management, coverage stripping); target gates refuse the rest at runtime with a typed error — the primary and payload blocks, the reserved block types, security blocks as targets, and covered targets (unprovable coverage refuses conservatively). Blocks inserted through the editor are valid targets for its own replace/remove; `finish()` materialises the accumulated edits, or `None` when untouched. An edit that would fail at `finish()` or yield bytes a receiver rejects is refused at call time instead, as `Error::Invalid` carrying the rejecting check's error: the parser's `InvalidFlags` for a `report_on_failure` flag the bundle forbids and `InvalidCrc` for an unrecognised CRC type, and the type's own decode error for Previous Node, Bundle Age, or Hop Count data that does not decode as its type (on `insert` and `replace`) — data the parser never decodes but a receiving BPA does.
+- `PrimaryBlock::forbids_report_on_failure()`: the one statement of RFC 9171 §4.2.3-4/-5 — an administrative-record or null-source bundle forbids the `report_on_failure` flag on every block. The parser rejects the combination through it, `Builder::build` clears the flag, and `ExtensionEditor` refuses it; the owner `Editor` writes flags as given.
 - `reader::ReaderExt`, blanket-implemented for every `Reader` (trait objects included): `extract<T>()` CBOR-decodes a block's payload, with `Ok(None)` for absent-or-unavailable and `Err` only for decode failures.
+- `block::Flags` and `bundle::Flags` derive `Hash`, as `rfc9173::ScopeFlags` does.
+- `canonicalize()` on `block::Flags`, `bundle::Flags`, and `rfc9173::ScopeFlags`: it folds each bit of `unrecognised` that names a flag into that flag's field, mirroring `block::Type::canonicalize()`. `rfc9173::ScopeFlags` gains `From<u64>` and `From<&ScopeFlags> for u64`, the conversions its CBOR codec delegates to.
 
 ### Removed
 - **BREAKING:** the free `bpsec::block_data()` function — `bpsec::DecryptingReader::block_data` is the same contract (typed errors carrying the cause; owned decrypted plaintext) plus outcome memoisation and an explicit `Ok(None)` for non-resident extents. A one-shot caller migrates to `DecryptingReader::new(..).into_block_data(n)`, which, like the removed function, returns the decrypted plaintext without an extra copy.
 
 ### Changed
+- **BREAKING:** the `unrecognised` field of `block::Flags`, `bundle::Flags`, and `rfc9173::ScopeFlags` is a plain `u64`, zero when no unrecognised bits are set (was `Option<u64>`, where `None` and `Some(0)` meant the same). Migration: `None` becomes `0` and `Some(bits)` becomes `bits`, so `if let Some(bits) = flags.unrecognised` becomes `if flags.unrecognised != 0`. The serde shape of `block::Flags` and `bundle::Flags` is unchanged for every value a parse or builder produces — the field is omitted when zero — except that an explicit `null` no longer deserializes.
 - **BREAKING:** the block read abstraction is renamed and lifted to the crate root, joining the `Builder`/`Editor`/`Signer`/`Encryptor` role-noun family: trait `bpsec::BlockSet` is now `reader::Reader`, and `bpsec::PlainBlockSet` is now `reader::PlainReader`. No behavioural change — signatures and semantics are otherwise identical, and plain block reading no longer requires the `bpsec` module path. `PlainReader`, two shared references, also derives `Clone`, `Copy` and `Debug`.
 - **BREAKING:** `reader::Reader::block` reports the payload slot as a four-state `reader::Availability` instead of `Option<Payload>`: `Available(Payload)`, `NotResident` (extents beyond the resident bytes), `NoKey` (BCB-covered, no usable key), and `NotDecryptable` (BCB-covered, and this node cannot produce the plaintext despite trying — the ciphertext failed to authenticate, or the security context or parameters are unsupported; the give doors report which). A present block's unavailable payload no longer conflates "not held in memory" with "not decryptable by this node", so a caller can respond to each state differently; callers to whom every unavailable state is equivalent use `Availability::available()`.
 - **BREAKING (behaviour):** the structural parse marks `BibCoverage::Maybe` only on BCB-covered blocks. As built under RFC 9172 §3.9 ("when adding a BCB"), an encrypted BIB targets only blocks a BCB also covers, so on a bundle carrying an encrypted BIB, a block no BCB covers — typically the per-hop PreviousNode, HopCount and BundleAge blocks, and always the primary block — now reads `None` where it previously read `Maybe`. Editing, signing or encrypting such a block (`Editor`, `ExtensionEditor`, `Signer`, `Encryptor`) no longer refuses with `MaybeHasBib`, so a keyless relay performs its RFC 9171 per-hop rewrites on a signed-then-encrypted bundle instead of refusing them. Residual: a security acceptor that decrypts a target but not the BIB over it (`bpsec::edit::remove_encryption` does, for one target), or a non-conformant sender, leaves an encrypted BIB over a block no BCB covers; an edit to that block breaks the hidden signature, which fails closed at the downstream key-holder.
@@ -24,6 +28,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - CBOR tags at grammar positions that permit none — every scalar and structured field, and the block-array head — are now rejected from the first byte of the tag run, without reading it (decoded via the new `hardy-cbor` `Untagged` wrapper; its cbor-level `UnexpectedTag` is translated to each error domain's `NotCanonical`, so the observable classification is unchanged from previous releases, which read the entire run before rejecting). The one position permitting a tag (`#6.24` on block data) enforces the same fixed-byte bound by hand. This keeps ingress rejection of adversarial tag runs free of per-tag work and per-tag allocation (a scalar-field reject still boxes its one constant field-label error).
 
 ### Fixed
+- `Builder::build` no longer makes bundles the parser rejects when the bundle is an administrative record or has a null source: `report_on_failure` is cleared on every block (RFC 9171 §4.2.3-4/-5), normalised like the fragment flag. Previously `with_hop_count`, which sets the flag, made any such bundle with a hop limit unparseable.
+- A named flag bit carried in the `unrecognised` field of `block::Flags`, `bundle::Flags`, or `rfc9173::ScopeFlags` is now the flag it encodes. A hand-built `unrecognised` encodes bit for bit, so a named bit carried there set the flag on the wire while its field read false: it slipped past `ExtensionEditor`'s `report_on_failure` refusal and `Builder::build`'s normalisation into bytes the parser rejects, and in a BPSec scope it left the primary block out of the IPPT/AAD the security source computed while the emitted parameter told the receiver to include it. Every method that takes flags now canonicalizes them (the new `canonicalize()`, above) — `Builder::with_flags`, both `BlockBuilder::with_flags`s, `Editor::with_bundle_flags`, `ExtensionEditor::insert`, and the BIB-HMAC-SHA2 and BCB-AES-GCM operations' scope, with the builder's block template as the backstop — so an alias is refused, normalised, or covered as the flag it encodes, and the `Bundle` a builder returns matches its bytes. Parsed values are already canonical, so parsed round-trips are unchanged. Serde deserialization and direct field writes are not canonicalized.
 - `Editor::push_block`, `Editor::insert_block`, and `Builder::add_extension_block` canonicalize their block-type argument through the new `block::Type::canonicalize()`, which folds an `Unrecognised` alias of a known type code back to its named variant. A hand-built `Type::Unrecognised(v)` encodes as the raw code `v`, so `Unrecognised(11)` or `Unrecognised(12)` previously bypassed `push_block`'s security-block refusal and emitted a block the next node parses as a real BIB/BCB without going through the Signer/Encryptor; `Unrecognised(0)` bypassed both primary-block refusals, and `Unrecognised(1 | 6 | 7 | 10)` bypassed the singleton-duplicate rules (producing bundles the receiving parser rejects). All three caller-supplied-type doors canonicalize, and `BlockTemplate::new` canonicalizes as the constructor invariant, so a stored template never holds an alias and replace-by-type matches are sound. Scope of the refusals is per door: the editor doors refuse the reserved codes and enforce the singleton rules whatever variant carries them; the builder door refuses only wire code 0 — its contract deliberately permits security blocks and duplicates, as the test-crafting surface. Any other policy that matches on named variants can canonicalize first.
 - `Editor::insert_block`'s replace-by-type path handles BPSec coverage exactly like `update_block`: the replaced block is stripped from its covering BIB/BCB target lists, and the replace refuses (`BibIsEncrypted`, `MaybeHasBib`) where coverage cannot be safely updated. Previously it built a fresh template with no BIB handling, so replacing a BIB-covered block (e.g. the forwarding path's hop-count update on a bundle whose HopCount a peer had signed) emitted a wire bundle whose BIB still targeted the changed body — failing verification downstream — while the in-memory rebuilt `Bundle` reported the block uncovered.
 - A CBOR tag on the status flag of a status-report assertion was silently accepted — the bare `bool` decode folds tag presence into a canonical flag the caller discarded. It is now rejected (`InvalidField("status")` wrapping `NotCanonical`).

bpv7/docs/TODO.md

@@ -397,3 +397,9 @@ Open items from the `refactor/bpv7-parse` deep review (`references/reviews/bpv7-
 **Pre-existing broken intra-doc links, and no `cargo doc` gate (review 2.9).** `RUSTDOCFLAGS="-D warnings" cargo doc -p hardy-bpv7 --no-deps --all-features` fails on two links older than the reader-editor work: `Block::payload`'s doc links `parse::parse`, which `block.rs` does not import, and `Builder::build`'s doc links `Bundle`, which `builder.rs` does not import (qualify both, e.g. `crate::parse::parse`). No workflow runs `cargo doc` (`docs.yml` builds only the mkdocs site), so nothing catches a new broken link either. Fix the two links, then add a workspace `cargo doc --no-deps --all-features` step with `RUSTDOCFLAGS="-D warnings"` to the CI checks job.
 
 **`remove_encryption` leaves an encrypted BIB over a decrypted target (re-review R-3).** With BCB-AES-GCM the Encryptor emits one BCB per target, so a signed-then-encrypted block X carries one BCB over X and another over the BIB that covers X. `bpsec::edit::remove_encryption(X)` decrypts under X's BCB only: the same-BCB BIB handling it documents is unreachable with a context that cannot share a BCB. The result is conformant and verifiable by a key-holder, but X is no longer BCB-covered, so a keyless parse reads its coverage as `None` rather than `Maybe`, and an editor (a relay's per-hop rewrite, or a Stage 2 Rewriter through `ExtensionEditor`) modifies it; the key-holder's check of the hidden result then fails closed. Fix direction: when the decrypted target's BIB sits under a different BCB, `remove_encryption` also decrypts and strips that BCB if it holds the key, restoring a plaintext BIB (which a relay's per-hop rewrite then strips by policy), and refuses otherwise; the `bundle remove-encryption` CLI inherits the change. Partial acceptors in other implementations can produce the same state, so the parser-side residual remains, as the `BibCoverage::Maybe` doc and the CHANGELOG record.
+
+## ExtensionEditor accept-set invariant (external #712 review, 2026-09-30)
+
+`ExtensionEditor` refuses at call time every edit its own `finish()` or a receiver would reject: the forbidden `report_on_failure` flag (`PrimaryBlock::forbids_report_on_failure`) and unrecognised CRC types, which the parser rejects, and Previous Node / Bundle Age / Hop Count data that does not decode as its type, which the parser never inspects but a receiving BPA's decode of those blocks rejects. That list is kept in step by hand, so a future parser or decode rule can widen the gap again, and a caller that treats a failed `finish()` or an unreadable result as fatal aborts on it. Pin the invariant with a fuzz target beside `random_bundles`: for every parseable bundle and every `ExtensionEditor` operation sequence, `finish()` succeeding implies the flattened result parses and its Previous Node, Bundle Age, and Hop Count blocks decode as their types (`Block::extract`).
+
+**Write doors take the parsed-only `CrcType::Unrecognised`.** `Builder::with_crc_type`, both `BlockBuilder::with_crc_type`s (builder and editor), `Editor::with_bundle_crc_type`, and `BundleTemplate.crc_type` take the wire `CrcType`, whose `Unrecognised(code)` only a parse produces meaningfully; the write fails late, at `crc::append_crc_value`, as `builder::Error::InternalError`. `ExtensionEditor::insert`'s call-time refusal is the stopgap. Fix direction: a write-side `crc::Kind` (`None`, `Crc16`, `Crc32`) taken by the write doors, with `From<Kind> for CrcType` and `TryFrom<CrcType> for Kind`, while `CrcType` stays the read type; the tools' `ArgCrcType` (`bpv7/tools/src/flags.rs`) already spells that set.

bpv7/src/block.rs

@@ -15,9 +15,21 @@ use hardy_cbor::{
 use crate::{Error, crc};
 /// Represents the processing control flags for a BPv7 block.
 ///
-/// These flags, defined in RFC 9171 Section 4.2.2, control how a node should
+/// These flags, defined in RFC 9171 Section 4.2.4, control how a node should
 /// process the block, especially in cases of failure or fragmentation.
-#[derive(Default, Debug, Clone, PartialEq, Eq)]
+///
+/// A bit carried in [`unrecognised`](Self::unrecognised) that names a flag
+/// is an alias of that flag: it encodes as the flag's bit, so the next
+/// parser reads the flag set. Equality is structural, so an alias and its
+/// named flag compare unequal although they encode identically; policy that
+/// reads the named fields must [`canonicalize`](Self::canonicalize) first.
+/// Parsed values are canonical, and the methods that take block flags
+/// canonicalize them:
+/// [`builder::BlockBuilder::with_flags`](crate::builder::BlockBuilder::with_flags),
+/// [`editor::BlockBuilder::with_flags`](crate::editor::BlockBuilder::with_flags),
+/// and [`ExtensionEditor::insert`](crate::extension_editor::ExtensionEditor::insert).
+/// Serde deserialization and direct field writes are not canonicalized.
+#[derive(Default, Debug, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct Flags {
     /// If set, the block must be replicated in every fragment of the bundle.
@@ -45,17 +57,38 @@ pub struct Flags {
     )]
     pub delete_block_on_failure: bool,
 
-    #[cfg_attr(
-        feature = "serde",
-        serde(default, skip_serializing_if = "Option::is_none")
-    )]
-    /// A bitmask of any unrecognized flags encountered during parsing.
-    pub unrecognised: Option<u64>,
+    /// A bitmask of the flag bits this implementation does not name; zero
+    /// when there are none.
+    ///
+    /// Encoding carries every bit set here; a parsed `Flags` never holds a
+    /// named bit in it.
+    #[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "is_zero"))]
+    pub unrecognised: u64,
+}
+
+// serde's `skip_serializing_if` predicate for the `unrecognised` fields.
+#[cfg(feature = "serde")]
+pub(crate) const fn is_zero(bits: &u64) -> bool {
+    *bits == 0
+}
+
+impl Flags {
+    /// Folds every bit of [`unrecognised`](Self::unrecognised) that names a
+    /// flag into its named field; genuinely unrecognised bits are kept.
+    ///
+    /// A hand-built `unrecognised` encodes bit for bit, so `1 << 1`
+    /// *is* `report_on_failure` to the next parser. Policy that reads the
+    /// named fields must canonicalize first, or it misreads the flags the
+    /// bytes carry.
+    #[must_use]
+    pub fn canonicalize(self) -> Self {
+        Self::from(u64::from(&self))
+    }
 }
 
 impl From<&Flags> for u64 {
     fn from(value: &Flags) -> Self {
-        let mut flags = value.unrecognised.unwrap_or(0);
+        let mut flags = value.unrecognised;
         if value.must_replicate {
             flags |= 1 << 0;
         }
@@ -77,26 +110,24 @@ impl From<u64> for Flags {
         let mut flags = Self::default();
         let mut unrecognised = value;
 
-        if (value & 1) != 0 {
+        if (value & (1 << 0)) != 0 {
             flags.must_replicate = true;
-            unrecognised &= !1;
+            unrecognised &= !(1 << 0);
         }
-        if (value & 2) != 0 {
+        if (value & (1 << 1)) != 0 {
             flags.report_on_failure = true;
-            unrecognised &= !2;
+            unrecognised &= !(1 << 1);
         }
-        if (value & 4) != 0 {
+        if (value & (1 << 2)) != 0 {
             flags.delete_bundle_on_failure = true;
-            unrecognised &= !4;
+            unrecognised &= !(1 << 2);
         }
-        if (value & 16) != 0 {
+        if (value & (1 << 4)) != 0 {
             flags.delete_block_on_failure = true;
-            unrecognised &= !16;
+            unrecognised &= !(1 << 4);
         }
 
-        if unrecognised != 0 {
-            flags.unrecognised = Some(unrecognised);
-        }
+        flags.unrecognised = unrecognised;
         flags
     }
 }
@@ -127,7 +158,7 @@ impl Flags {
             report_on_failure: true,
             delete_bundle_on_failure: true,
             delete_block_on_failure: false,
-            unrecognised: None,
+            unrecognised: 0,
         }
     }
 }