fix(cli): verify metaboard bytes against the describedByMetaV1 hash - #284
Conversation
fetch_for_contract queried the metaboard by the contract's meta hash and decoded the answer without hashing it, so any endpoint could serve any words for any hash. keccak256 of the returned bytes is now checked against the hash before they are decoded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
1 participant
Closes #210.
The defect
describedByMetaV1()returns the keccak256 of the meta bytes that describe thecontract. That binding is enforced on the way in by
LibDescribedByMeta.emitForDescribedAddress, which revertsMetadataMismatchunless
keccak256(meta) == described.describedByMetaV1(), and reproduced by thesubgraph, which indexes
metaHash = keccak256(event.params.meta).AuthoringMetaV2::fetch_for_contractqueried the metaboard by that hash andthen cbor-decoded whatever came back without ever hashing it. A metaboard
endpoint — or a buggy or compromised indexer behind it — could answer any hash
with any words and the fetch succeeded, turning a content-addressed lookup into
a trust-the-server lookup.
The change
Before the returned bytes are decoded,
keccak256(meta_bytes)is compared tothe hash the contract named; a mismatch is a new
AuthoringMetaV2Error::MetaHashMismatch { expected, actual }, wrapped in theusual
FetchAuthoringMetaV2WordErrorwith the same address/rpcs/metaboardcontext as every other failure. The guard sits before
cbor_decode, sounverified bytes are never parsed at all. Nothing else in the pipeline moves,
and an honest metaboard sees no behaviour change.
The issue leaves open that a caller-supplied metaboard URL may be considered
trusted, in which case the old behaviour was by design. This resolves it toward
verifying: the check is one keccak256 over bytes already in memory, and the hash
is the only thing that ties the on-chain claim to the off-chain bytes. Say so if
trust-the-server is the intended contract and this is the PR to reject.
How this relates to the adjacent work
metas[0]/decoded[0]) is untouched. The guardreads the same
metas[0], hoisted into ameta_bytesbinding, so AuthoringMetaV2::fetch_for_contract panic-indexes metas[0] and decoded[0], relying on other crates' non-empty guarantees #211's fix(
metas.first()with an explicit error) drops in on top of it unchanged.and including obtaining the metahash; this touches only the metaboard half
that runs after an RPC has answered. Disjoint, and fix(cli): fail over across every RPC in fetch_for_contract #250's
wrap_errorclosurewould absorb this guard's error construction if it lands first.
overlaps: it turns
metas[0]into a loop. Merged together, the hash guardbelongs inside that loop, on each meta before it is decoded — the subgraph
filters on
metaHash, so every entry it returns claims the same hash and eachone must earn it. Whichever lands second should carry that resolution rather
than leaving only the first entry verified.
QA
Discriminating tests:
test_fetch_for_contract_rejects_meta_bytes_not_matching_the_hash— a wellformed authoring meta document served under
0x11…11, which is not itshash; asserts
MetaHashMismatchand both of its fields.test_fetch_for_contract_checks_the_hash_before_decoding— bytes that areneither the hashed content nor valid cbor; asserts the mismatch is what is
reported, i.e. nothing unverified reaches the decoder.
Both verified failing on base by deleting the guard and rerunning the module:
16 passed; 2 failed, the first panicking onunwrap_err()ofOk(AuthoringMetaV2 { … "description 1" … })— base returns the unverifiedwords — and the second reporting
MetaError(SerdeCborError(…))whereMetaHashMismatchis expected.Two existing tests are changed in place rather than duplicated:
test_fetch_for_contract_success_decodes_first_metaandtest_fetch_for_contract_invalid_cbor_is_meta_errorboth mockeddescribedByMetaV1()as0x01…01while serving bytes hashing to somethingelse — the exact condition now rejected. They now mock the hash of the bytes
they serve (new
meta_hex_hashtest helper), which is what an honest metaboardanswers with. They pass on base as well, so they are guards against
over-fixing into "reject everything", not base discriminators.
Mutations applied (each ran all 18 module tests; the tree was restored and
verified byte-identical afterwards):
2 failed).
if meta_bytes_hash.0 != metahash->==-> 14 passed, 4 failed: both newtests plus
success_decodes_first_metaandinvalid_cbor_is_meta_error, sothe condition is pinned in both directions.
expected/actualfields swapped -> killed only byrejects_meta_bytes_not_matching_the_hash(17 passed, 1 failed), the testthat asserts both fields.
cbor_decode+try_into-> killed only bychecks_the_hash_before_decoding(17 passed, 1 failed), pinning theordering.
Oracle, from outside the function under test:
LibDescribedByMeta(
src/lib/LibDescribedByMeta.sol) hashes the raw meta bytes and reverts unlessthey match
describedByMetaV1();subgraph/src/metaBoard.tssetsmetaHash = keccak256(event.params.meta);get_metabytes_by_hashreturnsthose same bytes hex-decoded. So keccak256 over the returned bytes verbatim —
no rain-meta-document re-encoding — is the hash the contract names. Test
expectations are computed by the test helper from the served bytes, not by the
code under test, and the word fixtures are the pre-existing chisel-encoded
WORDS_PAYLOAD_HEX.Category check: the issue asks for (a) verifying
keccak256(meta_bytes)against the
describedByMetaV1hash — done, before any decode; and notes (b)that further entries under the same hash are ignored without a consistency
check — the bytes actually used are now verified, which is this issue's half,
while reachability of the later entries is fetch_for_contract only considers metas[0] and the first cbor document: a valid authoring meta later in the list is unreachable #157 and stays in PR fix: scan every metaboard meta and cbor document for the authoring meta #251.
Local verification, in the repo's
nix develop:cargo test -p rain-metadata --lib -- authoring::v2-> 18 passed, 0 failed.cargo clippy -p rain-metadata --all-targets -- -D warnings-> clean.cargo fmt --all -- --check-> clean.The full suite was not run locally (shared machine); CI covers it.
🤖 Generated with Claude Code