Skip to content

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

Open
ricktaylor wants to merge 1 commit into
mainfrom
fix/bpv7-extension-editor-accept-set
Open

ricktaylor wants to merge 1 commit into
mainfrom
fix/bpv7-extension-editor-accept-set

Conversation

@ricktaylor

@ricktaylor ricktaylor commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

ExtensionEditor could accept an edit at call time that then failed at finish(), or that produced bytes the bpv7 parser rejects. This PR makes the editor refuse those edits up front, with the parser's own error. It also fixes Builder::build, which could produce bundles its own parser rejects.

The gaps came out of the external review of #712. The BPA's filter editor mirrors ExtensionEditor, and a Rewriter that hits one of these gaps aborts the node under the Rewriter fail-stop rule, so they need closing in bpv7 first. #712 and the rest of the train are rebased onto this branch.

Changes

  • ExtensionEditor refusals. insert now refuses, as extension_editor::Error::Invalid carrying the error the parser raises for the same input:

    • a report_on_failure flag on an administrative-record or null-source bundle (RFC 9171 §4.2.3-4/-5): InvalidFlags;
    • an unrecognised CRC type: InvalidCrc;
    • a Previous Node, Bundle Age or Hop Count body that doesn't decode as its type: that type's decode error.

    replace also checks well-known bodies. There's one new variant wrapping crate::Error, rather than duplicates of errors the crate already has.

  • One statement of the RFC rule. PrimaryBlock::forbids_report_on_failure() is the single place RFC 9171 §4.2.3-4/-5 is written. The parser's block check and the editor's refusal both use it.

  • Builder::build fix. with_hop_count sets report_on_failure, so any null-source or admin-record bundle with a hop limit failed to parse. build() now clears the flag on every block of such a bundle, the same way it normalises the fragment flag.

  • Docs. The CHANGELOG has entries under Added (the refusals, the predicate) and Fixed (the builder). The TODO has a new entry for a fuzz target that pins the invariant: for every parseable bundle and every ExtensionEditor operation sequence, if finish() succeeds, the flattened result parses. The refusal list is kept in step with the parser by hand, so a future parser rule could reopen the gap.

API impact

ExtensionEditor is unreleased (listed under Unreleased → Added), so the new Error::Invalid variant breaks no released API. PrimaryBlock::forbids_report_on_failure() is a new public method. Callers of Builder that relied on the forbidden flag being emitted were producing bundles the parser rejects.

Tests

New tests in bpv7/tests/editor.rs and bpv7/tests/parse.rs:

  • the forbidden-flag refusal, for both an admin-record bundle and a null-source bundle;
  • the unrecognised-CRC refusal;
  • undecodable well-known bodies refused on insert and on replace;
  • a null-source / admin-record bundle built with a hop count clears report_on_failure and round-trips through parse.

Each refusal test asserts the specific wrapped parser error, not just is_err().

Verification

Run locally: cargo fmt --check, workspace cargo clippy --all-targets --all-features -- -D warnings, and cargo test --workspace --all-features. The only failures are environment-only: the storage-harness Postgres/S3 suites (no services here) and tcpclv4 [::1] (no IPv6). The no-std thumb build pair runs in CI only, and this PR needs it green.

Merge order

This merges first, then #712, #717, #718, #719 and #752. Until it merges, #712's diff also shows this commit.

🤖 Generated with Claude Code

`ExtensionEditor::insert` passed flags, CRC type and block bodies
through unchecked, so an edit could be accepted at call time and then
fail at `finish()` or produce bytes the parser rejects: a
`report_on_failure` flag on an administrative-record or null-source
bundle (RFC 9171 §4.2.3-4/-5), an unrecognised CRC type, or a Previous
Node / Bundle Age / Hop Count body that does not decode. Each is now
refused at call time as `Error::Invalid`, carrying the error the parser
raises for it (`InvalidFlags`, `InvalidCrc`, or the body's own decode
error); `replace` validates well-known bodies too.

The RFC rule gets one statement, `PrimaryBlock::forbids_report_on_failure`,
which the parser's block check now consults.

`Builder::build` emitted bundles its own parser rejects: `with_hop_count`
sets `report_on_failure`, so any null-source or admin-record bundle with
a hop limit was unparseable. `build()` now clears the flag on every
block of such a bundle, normalised like the fragment flag.

Found by the external review of #712 (CRIT-1, HIGH-2, MED-3): the BPA's
filter editor mirrors this one, and a Rewriter tripping the gap aborts
the node.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Rick Taylor <rtaylor@aalyria.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant