feat(riff)!: carry the C2PA manifest store in the C2PA chunk - #515
Open
justin13888 wants to merge 7 commits into
Open
justin13888 wants to merge 7 commits into
justin13888 wants to merge 7 commits into
Conversation
C2PA 2.4 §A.3.7 embeds a manifest store in a RIFF `C2PA` chunk that "shall appear as the last sub-chunk of the first RIFF header chunk". `MetadataChunks` gains a fourth borrowed carrier for it, on the same verbatim terms as `ICCP`/`EXIF`/`XMP `, and the writer emits it after the metadata *and* after the unknown chunks §2.7.1.6 asks it to preserve. `c2pa_span` reports the chunk's whole on-disk span — identifier, size field and payload — which is what a `c2pa.hash.data` exclusion covers (§18.5): an update manifest may resize the store, changing the size field's value as well as the bytes after it. The RIFF pad byte that follows an odd-length store is framing the container adds, so it stays outside the span, exactly as `Chunk::payload` excludes it. RFC 9649 §2.5's `VP8X` flag byte defines no C2PA bit, so presence is decided by the chunk and never by a flag. `WebpChunkId::C2pa` makes the store recognised rather than unknown, so a read/modify/write cycle re-emits it once, in its mandated place, instead of twice. BREAKING CHANGE: `MetadataChunks` gains a `c2pa` field. The struct is exhaustive by design, so a struct literal needs `c2pa: None` or `..Default::default()`.
`WebpEncoder::with_c2pa` embeds a finished C2PA manifest store as a `C2PA` chunk (C2PA 2.4 §A.3.7); `with_c2pa_reserved` leaves room for one that cannot exist yet, because its hard binding digests the finished file (§15.12.1.1). Either way the chunk goes last, as §A.3.7 requires, and no `VP8X` feature flag advertises it — RFC 9649 §2.5 defines no C2PA bit. `encode_with_report` returns the file together with a `WebpEncodeReport` naming the chunk's whole byte span: identifier, size field and payload, which is what a `c2pa.hash.data` assertion excludes (§18.5). The range is read back out of the finished bytes with the same locator the read side uses, so a writer and a reader can never disagree about it, and the object-safe `EncodeImage` seam is left untouched. `c2pa_span` recovers the range from any WebP file and `WebpMetadata::c2pa` its bytes. gamut carries the store: it never builds, hashes, signs or validates one.
The span arithmetic ran before the match test, so the pad-byte term was computed and thrown away on the chunk being looked for. The doc's parity aside was also backwards: the chunk header is eight bytes, so the span's parity is the store's, not its opposite.
`write_extended_preserving` emitted the preserved unknown chunks before
the configured store, so a `C2PA` chunk arriving through `unknown` was
written first — and every reader here takes the *first* chunk of a kind.
A file built with `unknown = [C2PA "STALE"]` and `c2pa: Some("FRESH")`
came out as `[VP8X, VP8L, C2PA, C2PA]`, with both `MetadataChunks::read`
and `c2pa_span` reporting the stale copy. A signer would then exclude a
range over bytes it never wrote and hash its own store as content.
`MetadataChunks::c2pa` now owns the chunk: a `C2PA` chunk among the
preserved chunks is dropped rather than emitted, so a file this function
writes carries at most one store. Nothing is lost — `WebpLayout::parse`
never puts a `C2PA` chunk in `unknown`, since the crate recognises it —
and `write_extended` stays unfiltered for a caller assembling by hand.
Also pins `c2pa_span`'s documented `# Errors` contract, which had no
assertion reaching it in either direction.
Three defences so a WebP file carries exactly one C2PA manifest store, which the encoder's docs already claimed but nothing enforced. `with_unknown_chunks` now rejects a FourCC the encoder writes itself — `VP8X`, `VP8 `, `VP8L`, `ALPH`, `ICCP`, `EXIF`, `XMP `, `C2PA`. Each has a dedicated setter, so passing one through emitted the chunk twice and `gamut-riff`'s "first of each kind wins" resolved it in favour of the pass-through copy. It is the only setter that takes a FourCC from the caller rather than only a payload, so it is the only one with an invalid input to reject. `encode_with_report` now reads the store back out of the finished file and refuses to report a range whose bytes are not the configured store. The previous guard fired only when a configured store could not be found at all, which cannot happen; the failure that could happen — a range over somebody else's bytes — returned `Ok`. `with_c2pa_reserved` rejects a length past the `uint32` a RIFF chunk size field holds instead of reaching `vec![0; len]`, which panics with "capacity overflow" on a length no allocator can serve. CLAUDE.md forbids a panic on a library path. The no-cap policy is unchanged: the bound is what the container can express, not a limit on a signer's reserve size. BREAKING CHANGE: `WebpEncoder::with_unknown_chunks` returns `Result<Self>` and rejects a reserved FourCC; add `?` at the call site.
Dropping a `C2PA` chunk from the preserved chunks unconditionally was heavier than the defect needed. Displacement requires a configured store to displace; with none, the chunk is the manifest store of the file it was read out of, and discarding it lost a foreign store with no signal. It is now kept, and written in the store's place rather than wherever the caller's list put it — in that file it *is* the store, so §A.3.7's "last sub-chunk" applies to it. With a store configured the copy is still dropped, so the field keeps sole ownership of the slot and the range `c2pa_span` reports is still always over the configured bytes. The layer asymmetry is deliberate and now documented: this function is total, while `gamut-webp`'s builder rejects the same input with a typed error. The builder can name the offending call; the writer cannot, and must stay usable for a re-wrap that has to succeed.
… them The reserved-FourCC gate restated eight of `WebpChunkId::from`'s ten arms by hand and had already drifted: `ANIM` and `ANMF` were missing, so `with_unknown_chunks(&[(FourCc::ANIM, ..)])` was accepted, encoded, and produced a file `WebpLayout::parse` then refused — "reconstruction chunks are out of order". The encoder could be steered into writing a file it cannot read back. Asking `WebpChunkId::from` covers all ten arms and cannot drift from `gamut-riff` again, per CLAUDE.md's no-duplication rule. The error now names the offending FourCC through `Error::with_detail`, the workspace's owned-context channel — used with `format!` in this crate already — instead of making the caller diff a list in a static message. `FourCc`'s `Display` escapes non-printable bytes, so a hostile code is safe to render. The reservation limit moves into `reservation_len`, so the `uint32` ceiling is pinned at the boundary without allocating the 4 GiB reaching it would need; the assertion that stood in for that pinned a property of `core` and could not fail. `encode_with_report`'s cross-check stays a live runtime guard — this is a signing path — but its `# Errors` clause now says plainly that it is defence in depth and unreachable through this API. The `c2pa_span` `# Errors` pin here is dropped: it reached a one-line delegation whose only mutants another test already kills, and asserted a `gamut-riff` property that crate now pins inline. BREAKING CHANGE: `WebpEncoder::with_c2pa_reserved` also returns `Result<Self>`, which the footer of 46019a5 omitted; it named only `with_unknown_chunks`. Both need `?` at the call site. Additionally `with_unknown_chunks` now rejects `ANIM` and `ANMF`, which it previously accepted and mis-encoded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Carries the C2PA manifest store in a RIFF
C2PAchunk (C2PA 2.4 §A.3.7), throughgamut-riffandout of
gamut-webp, on exactly the verbatim-passthrough termsICCP/EXIF/XMPalready get.The store is opaque here: nothing parses, signs or validates it.
gamut-riff(breaking)MetadataChunksgains a fourth borrowed carrier,c2pa: Option<&'a [u8]>. The struct isexhaustive by design, so this is the crate's first breaking change since v1; the migration is
c2pa: Noneor..Default::default().write_extended_preservingemits the chunk as the last sub-chunk of theRIFF/WEBPform —after
EXIF, afterXMP, and after the unknown chunks RFC 9649 §2.7.1.6 asks writers topreserve. §A.3.7 is stricter than "at the end of the file", and nothing else in the crate ordered
a chunk behind the preserved ones.
C2PA_FOURCCandWebpChunkId::C2paname the chunk. Making it recognised rather than unknownis what stops a read/modify/write cycle emitting it twice — once out of
unknown, once out ofmetadata.c2pa_span(data) -> Result<Option<Range<usize>>>reports the chunk's whole on-disk span:identifier, size field and payload. That is what a
c2pa.hash.dataexclusion covers (§18.5),because an update manifest may resize the store and so change the size field's value as well as
the bytes after it. The RIFF pad byte after an odd-length store is framing the container adds, so
it is outside the span — the same boundary
Chunk::payloadalready draws.VP8Xfeature flag: RFC 9649 §2.5's flag byte defines no C2PA bit and its reserved bits "MUSTbe 0", so presence is decided by the chunk alone.
C2PAamong the chunks a reader may fail a file over, so
reconstruction_rankleaves it unconstrainedalongside metadata and unknown chunks.
gamut-webp(one breaking signature; otherwise additive)WebpEncoder::with_c2pa(store)embeds a caller-computed store;with_c2pa_reserved(len)writeslenzero bytes to be filled in later. Last call wins, whichever of the two it was.encode_with_report(image) -> Result<(Vec<u8>, WebpEncodeReport)>returns the file plus where thestore landed. The object-safe
EncodeImageseam is untouched, per the C-portability rule.WebpMetadata.c2pasurfaces the store on read (the struct is#[non_exhaustive], so additive),and
gamut_webp::c2pa_spanreports the exclusion span of any WebP file without decoding pixels.with_unknown_chunksnow returnsResult<Self>and refuses a FourCC the encoder writes itself.Exactly one store per file, enforced rather than assumed (review round 2)
A read-only review found that a
C2PAchunk arriving through the preserved unknown chunks wasemitted before the configured store, and every reader here takes the first chunk of a kind — so
c2pa_spanandMetadataChunks::readboth returned the stale copy andencode_with_reportreturned
Okwith a range over bytes the caller never wrote. A signer would have excluded the wrongspan and hashed its real store as content. Reproduced on the branch before fixing:
unknown = [C2PA "STALE-STORE"]withc2pa: Some(b"FRESH-STORE")gave["VP8X","VP8L","C2PA","C2PA"]and bothreaders returned
STALE-STORE. Three independent defences now close it:gamut_riff::write_extended_preservingdrops aC2PAchunk from theunknownlist it forwards —MetadataChunks::c2paowns that chunk.write_extendedstays unfiltered as the hand-assemblyescape hatch.
WebpEncoder::with_unknown_chunksrejectsVP8X/VP8/VP8L/ALPH/ICCP/EXIF/XMP/C2PAwith a typed
InvalidInput, so the mistake is caught at the call that made it. This is the onlysetter that takes a FourCC rather than only a payload, hence the only one with an invalid input.
encode_with_reportreads the store back out of its own output and refuses to report a rangewhose bytes are not the configured store. The old guard fired only on a case that cannot occur (a
configured store that could not be found); the case that could occur returned
Ok.Also from that review:
with_c2pa_reservednow rejects a length past theuint32RIFF size fieldinstead of reaching
vec![0; len], which panics with "capacity overflow" forlen > isize::MAX—CLAUDE.md forbids a panic on a library path. The "no cap" policy is unchanged (decision 12 below);
this is only the representability check.
Round 3 (review of
46019a5; Medium and both Lows verified closed, five Lows raised):WebpChunkId::from's tenarms by hand and omitted
ANIM/ANMF— sowith_unknown_chunks(&[(FourCc::ANIM, ..)])wasaccepted, encoded, and produced a file
WebpLayout::parserefuses as "reconstruction chunks areout of order": an encoder steered into writing a file it cannot read back. Probed and confirmed on
the branch before fixing. It now asks
gamut-riff—matches!(WebpChunkId::from(fc), Unknown(_))— which covers all ten arms and cannot drift again.ANIM/ANMFare pinned.Error::with_detail(format!("{fourcc}")), the workspace'sowned-context channel, which this crate already uses at
backend.rs:417. My earlier claim thatthe message had to be static was wrong: it read only the constructor's
&'static str, not thediagnostic channel.
FourCc'sDisplayescapes non-printable bytes, so a hostile code is safe.reservation_len, testedat
u32::MAXand one past it; the previous assertion testedcoreand could not fail.c2pa_span# Errorspin reached aone-line delegation whose only mutants another test already kills.
BREAKING CHANGE:footer of46019a5named only one of two breaks; a follow-up commit'sfooter names
with_c2pa_reservedtoo, since release-plz publishes it as the changelog note.Under decision D1, the
C2PAfilter is now conditional: a carriedC2PAchunk is droppedonly when a store is configured to displace. With none, it is kept and written in the store's place —
in that file it is the store, and losing it silently would be worse than carrying it.
Validation
All commands were run in this PR's worktree. Rows marked r3 were re-run at
9a7fba9after there-review fixes. Workspace-wide gates ran inside a memory-capped systemd scope (
--slice=agents.slice -p MemoryMax=16G -p MemorySwapMax=0) withCARGO_BUILD_JOBS=2 CMAKE_BUILD_PARALLEL_LEVEL=2.cargo test -p gamut-riff --all-featuresr3cargo test -p gamut-webp --all-featuresr3cargo clippy -p gamut-riff -p gamut-webp --all-targets --all-features -- -D warningsr3__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmtthenmise run fmt-checkr3mise run check-testsr3mise run check-commitsr3mise run fetch-av1-oraclesmise run lint(whole workspace)d7d1c41; crate-scoped clippy re-run each round, and CI's Clippy & Doctests covers every head)mise run test(whole workspace, in scope) r3test result: oklines, 0 failuresmise run mutants-diff(in scope) r3mise run mutants-crate gamut-riff --shard 0/2d7d1c41)mise run mutants-crate gamut-riff --shard 1/2d7d1c41)Both defects found by review were reproduced against the branch before being fixed, with
throwaway tests removed before committing: the round-2 Medium (
["VP8X","VP8L","C2PA","C2PA"], bothreaders returning
STALE-STORE) and the round-3ANIMdrift (accepted, encoded, thenWebpLayout::parse→ "reconstruction chunks are out of order"). The permanent pins area_c2pa_chunk_in_unknown_never_displaces_the_configured_storeandwith_unknown_chunks_refuses_every_chunk_the_container_defines.Whole-crate mutation testing of
gamut-webpwas not run: the crate has 3409 mutants, and thischange touches only
encoder.rs/metadata.rs, whichmutants-diffcovers in full. Nothing hereis evidenced by a run that did not complete.
mise run check-release-deps,check-ffi-featuresandcheck-ffi-headerwere not run and arenot applicable: no
Cargo.tomlchanged and no C-surface type changed.mise run coveragewas notrun — no new module was added, and every new item is exercised by the suites above.
Note on the environment:
mise run fmt/fmt-checkwere run with the__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel)prefix, which is required in a nestednested worktree; without it cargo walks past the worktree root to the primary
checkout's manifests and the task exits 101 on an untouched tree.
Risks and rollout
gamut-riffconsumers. Only struct literals ofMetadataChunksbreak, and theonly in-tree consumer,
gamut-webp, is updated in this PR. release-plz will takegamut-riffto2.0 from the
BREAKING CHANGE:footer.gamut-webpconsumers, added in review round 2:with_unknown_chunksreturnsResult<Self>, so a call site needs?or.expect(..).gamut-webpis0.3.1, so release-plzbumps it to
0.4.0— still a minor version step, as the record's semver line said, and the onlycallers in the tree are this crate's own tests. The alternative was to accept a reserved FourCC and
resolve the duplicate silently, which is the defect this PR is fixing.
with_c2pa*call produces byte-identical output to before:
MetadataChunks::is_emptystill decides simple vsextended, and the new field is
None.C2PAchunk in an existing file changes classification. A file that already carried aC2PAchunk used to arrive inWebpLayout::unknown; it now arrives inmetadata.c2pa. Code thathand-scanned
unknownfor it must read the new field. This is the correct behaviour — it is whatprevents the chunk being written out twice — and is covered by a test.
with_unknown_chunksnow rejects input it used to accept. A caller passing any chunk thecontainer defines through it was already getting a wrong file — a doubled chunk whose first copy
won, or, for
ANIM/ANMF, one gamut's own strict reader refuses. That is now a typed error namingthe FourCC. No in-tree caller did this.
Issue
Closes #445.
Decisions taken
No human approved this plan. This is an unattended automation run: the decision record below is the
authority the work was built under, and it is what a human reviews after the fact.
Appended during delivery, in the record's shape:
Decided by the orchestrator on review of
d7d1c41, and implemented in1c3824c/46019a5:Decided by the orchestrator on re-review of
46019a5, and implemented ina3fe617/9a7fba9:Unresolved review notes
encode_with_reportis an inherent method because
EncodeImagehas nowhere to put a report, andgamut-avifandgamut-pngmade the same choice. Whether the trait should grow one — and what that means for theC ABI, where each monomorphization has to be reachable — is a workspace-wide question that three
format crates now have a stake in. Nothing here blocks on it.
encode_with_report's error arm is defensive, and now says so in its own# Errorsclause(decision 17). No public API can make the encoder write a
C2PAchunk that is not the configuredstore, so the arm is unreachable by construction rather than tested from the outside. What is
pinned is the claim it enforces — for every store the encoder accepts, the reported span contains
exactly that store — and the
!=in the check is mutation-visible, so removing it fails thesuite. Exercising the arm directly would need a seam letting a test write a rogue chunk, a wider
API change than this issue.
with_unknown_chunksnow refusesANIMandANMF, so the encoder can no longer be steered into writing a fileWebpLayout::parserejectsby that route. Whether any other public path can still produce a file gamut cannot read back was
not surveyed here — it is outside this issue, and the animation chunks are out of scope for the
crate under the image-first charter.
chunk identifier (byte 0) to the padding byte, if any, inclusive" — i.e. it includes the pad
byte, where the range reported here excludes it. The two are different assertions
(
c2pa.hash.boxesversus thec2pa.hash.datathis PR serves, per decision 4), and no code herecomputes either hash, so nothing is inconsistent today. A reviewer who intends
gamutto servec2pa.hash.boxeslater should decide then whether that wants a second reported range rather thana change to this one.
Range::len() % 2recovers the pad byte's presence from the reported span.