fix(ipfs-metadata): bind document_type to a MIME allow-list per type - #1239
Merged
nanaf6203-bit merged 2 commits intoSep 26, 2026
Merged
nanaf6203-bit merged 2 commits into
nanaf6203-bit merged 2 commits into
Conversation
`register_ipfs_document` accepted any MIME type for any
`DocumentType`, so a JPEG could be registered as a deed and the
registry's provenance claim became cosmetic.
The `allowed_mime_types` check that does exist is gated on
`!allowed_mime_types.is_empty()`, and the constructor seeds that list
empty ("populate via update"). On a freshly deployed registry the check
therefore never fires — the fail-open default is the actual hole, not
the absence of a check.
Add `document_type_permits_mime`, a pure function pairing each
DocumentType with the encodings that can honestly represent it:
- Deed, Title, Legal, TaxRecords, Insurance -> document encodings only.
No raster or video types, which is what closes the deed-as-image case.
- Images -> image types only, so a PDF cannot be filed as a photograph.
- Appraisal, Survey, FloorPlans, Inspection -> document or raster, since
these are commonly delivered either way.
- Other -> defers to the admin list; "other" states no claim to keep
honest.
It is enforced unconditionally and deliberately not admin-overridable.
An `add_allowed_mime_type` path that re-opened image/png for a deed
would restore exactly the guarantee this removes. The admin list now
narrows policy further rather than being the only defense.
Also corrects the docs on `verify_content_hash`, which claimed to verify
content while only comparing two caller-supplied hashes against each
other, and the `content_hash` field and `ContentHashVerified` event docs
that repeated the claim. Renames the local `is_valid` to
`echoes_registered_hash` so the code stops implying otherwise.
Ten unit tests cover the accepted and rejected pairings, that the policy
does not over-reject, that an admin cannot widen it, that
SizeLimitExceeded is reachable, and that a rejected upload leaves no
state behind.
Refs MettaChain#1189
Refs MettaChain#1188
A captured `cargo test` run from a CI container (/workspaces/PropChain-contract) showing `running 1 test` and a single passing `tests_pause::test_pause_resume_flow`. Committed output is stale the moment it lands, and this snapshot in particular implies the crate has one test when it has a large `tests_pause` module behind a `#[cfg(test)]` gate that was not fully enabled when the file was captured. CI artifacts are the right place for test evidence. Deleting. Refs MettaChain#1187
|
@1Psalm Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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
Binds
DocumentTypeto a MIME allow-list in the IPFS metadata registry, so the registry's provenance claim means something. Also corrects documentation that claims content verification the contract cannot perform.Addresses #1189 (primary). Partially addresses #1188, and removes the stale snapshot from #1187. #1190 is not addressed — see the per-issue notes.
What the issue got wrong, and what the actual hole was
The issue says
file_size,mime_typeanddocument_typeare "stored unverified" and that "the Error enum offersFileTypeNotAllowedandSizeLimitExceededbut nothing enforces them". Half of that is not accurate, and the accurate half matters:file_sizewas already enforced.register_ipfs_documentcompares it againstvalidation_rules.max_file_sizeand returnsSizeLimitExceeded. That error was already reachable. A test is added to pin that down.mime_typehad a check, but it never fired. The guard isif !allowed_mime_types.is_empty() && !contains(&mime_type), and the constructor seedsallowed_mime_types: Vec::new()with the comment "Initialize empty, populate via update". An empty list means "no restriction", so on any freshly deployed registry the MIME branch is skipped entirely. The defect is a fail-open default, not a missing check — which is why the fix below adds an always-on check rather than trying to make the existing one stricter.is_pinnedcannot be asserted at registration.register_ipfs_documenthas nois_pinnedparameter; it hardcodesis_pinned: falseand pinning happens through a separatepin_documentmessage. There is no assertion channel to reject, so that part of the proposal has nothing to act on.The fix
New pure function
document_type_permits_mime(&DocumentType, &str) -> bool, enforced unconditionally on every registration, before the admin list:Deed,Title,Legal,TaxRecords,Insuranceapplication/pdf,application/x-pdf,application/octet-streamImagesimage/jpeg,image/png,image/webp,image/tiffAppraisal,Survey,FloorPlans,InspectionOtherThree deliberate choices:
allowed_mime_types, so it holds regardless of admin configuration. The admin list now narrows policy further instead of being the only defense — which is what makes the empty default harmless.document_typeis that a consumer can trust it. Anadd_allowed_mime_typepath that re-openedimage/pngfor a deed would put the guarantee straight back, so the per-type policy is closed. There is a test asserting an admin cannot widen it.Imagesrejectsapplication/pdftoo, so a document cannot be filed as a photograph either.Inspectionaccepts raster because the existing testmedia_registration_and_duplicate_cid_rejectedregisters an inspection report asimage/jpeg, and inspection evidence is legitimately photographic. The other existing registration tests useapplication/pdf, which the policy permits, so no existing test changes meaning.#1188: the docs correction, and what is deliberately not here
verify_content_hash(document_id, provided_hash)compares the caller'sprovided_hashagainst the uploader's storedcontent_hashand returnstrueon a match. Both sides come from callers, so it only proves the supplied hash equals the one on record — a uploader registers whatever hash they like and then trivially "verifies" it. Downstream consumers readingContentHashVerifiedare being told content was attested when nothing was.Corrected in this PR: the
verify_content_hashdoc now states plainly that the contract cannot reach IPFS and thattruemeans "this hash is the one on record", not "these bytes are attested"; thecontent_hashfield docs on bothPropertyMetadataandIpfsDocumentno longer say "for verification"; theContentHashVerifiedevent doc no longer says "when content hash is verified"; and the localis_validis renamedechoes_registered_hashso the code stops implying otherwise.Not implemented, because it needs dependencies this crate does not have (
ipfs-metadatadepends only onink,scale,scale-info,propchain-traits):Both are security-critical additions that should land with a build and a test suite behind them, not blind. The public message name
verify_content_hashis also left alone despite overclaiming, since renaming changes the ink! selector and breaks callers; it should be renamed together with the real fix.#1190: not addressed
Two problems with the issue as written, and one reason not to attempt it here:
get_properties_by_ownerdoes not exist incontracts/metadata/src/lib.rs. The real unbounded listings areget_property_version_history(line 587),get_legal_documents(line 643) andget_properties_by_type(line 655), all returningVec<_>with no bound.Paginating those blind, in a different contract, on top of an unrelated change, is how you ship a broken registry. Left for a dedicated PR against
contracts/metadata.Tests
Ten added, none executed (see below):
deed_rejects_image_mime,title_rejects_image_mime,legal_instruments_reject_video_mime— the rejections the issue is about.deed_accepts_pdf,images_and_inspection_accept_photos— the policy does not over-reject.images_type_rejects_pdf— the policy is symmetric.other_type_defers_to_admin_list—Otherfollows the admin list when one is set, and is unrestricted when it is not.admin_cannot_widen_the_deed_policy—add_allowed_mime_type("image/png")does not make a PNG deed registrable.oversized_document_rejected— readsmax_file_sizefrom the contract rather than hardcoding it, so it does not silently drift from the constructor.rejected_upload_writes_no_state— after a rejection the counter is unchanged, the property has no documents, and the same CID is still free.No validation was performed
No
cargocommand was run — no build, no test, no clippy, nofmt. The tests above have not been executed and are not demonstrated to compile or pass.Specific risks a reviewer should check first:
implblock.TITLE_DOCUMENT_MIMESand friends are declared in the contract's inherentimpl. Valid Rust, but worth confirming against the ink! macro expansion.&'static strunderno_std. String literals are fine, and the crate already usesink::prelude::string::String, but the const arrays are a new shape here.cid_for(seed)buildsb-prefixed CIDs of 13 characters, satisfyingvalidate_ipfs_cid'sb+len >= 10rule, and varying one trailing character so the registry's duplicate-CID check does not fire within a test.DocumentTypederivesDebug, Clone, PartialEq, Eq, which the tests rely on for.clone()and for{document_type:?}assertion messages.Per-issue notes
document_type/mime_typepairing is now enforced and no longer depends on admin configuration.FileTypeNotAllowedandSizeLimitExceededare both reachable and covered. Acceptance criteria met.contracts/lib/test_output_lib.txtdeleted (a committed CI snapshot showingrunning 1 test, captured from/workspaces/PropChain-contract). File removed from git. The issue also asks to re-runcargo testto confirm the surface is green, which the no-validation constraint forbids; that belongs to CI.#1188 and #1190 are closed per the assignment convention. Read #1189 and #1187 as delivered; do not read #1188 or #1190 as delivered.
#1190 Metadata registry exposes unbounded listing queries (get_properties_by_type, get_properties_by_owner) with no pagination Link to this issue
#1187 test_output_lib.txt is a stale single-test snapshot committed to the repo - remove or regenerate via CI artifacts Link to this issue
#1189 IPFS metadata file_size, mime_type and document type are stored unverified - any uploader can claim a doc is a deed Link to this issue
#1188