fix(ipfs): verify CIDs by recomputing them, not by comparing content - #948
Merged
Kingsman-99 merged 3 commits intoSep 28, 2026
Merged
Conversation
verifyCID() fetched the bytes for a CID and compared their SHA-256 against
the SHA-256 of the caller-supplied content. That only proved the fetch
round-tripped — it never checked the CID at all, so verifyCID('garbage',
anyContentThatMatchesTheResponse) returned true. The old docstring
acknowledged this ('we can't recompute the exact CID without the multihash
library'), but a content address whose integrity check is optional is not
much of a content address.
A CID embeds the multihash of the content it names, so verification means
recomputing it and comparing:
- Implement CIDv0 (base58btc) and CIDv1 (base32 dag-pb) computation over a
sha2-256 multihash. Both encoders are implemented directly rather than
adding a dependency, keeping the SDK's dependency surface unchanged;
multiformats is only present transitively via WalletConnect.
- Recompute in the same CID version as the one supplied — a v0 and v1 CID
name identical bytes, so comparing across versions would always fail.
- When content is supplied the check is now purely local, with no network
round-trip, and genuinely proves the bytes hash to the claimed address.
- verifyCID() now accepts an optional content argument; omitting it verifies
whatever the CID resolves to, which is what detects a gateway serving
tampered bytes under a valid CID.
- Tolerate CIDs written as ipfs:// URIs or with a /ipfs/ path segment.
- Add verifyCIDDetailed() returning a CIDVerificationResult, and have
verifyCIDOrThrow() report the CID the content actually produced so a
mismatch distinguishes a wrong address from wrong bytes.
The encoders are checked against an independently computed sha2-256
multihash vector.
closes Stellar-split#848
Three existing tests asserted verifyCID() returned true for invented CIDs like 'QmVerifyCid1234567890...' that were never hashes of the test content. They passed only because verification was broken. Replace them with CIDs computed from the content via computeCidV0, and add coverage for: - a fabricated CID rejecting content that round-trips through the gateway - CIDv1 content addresses, and v0/v1 not matching each other - ipfs:// URI form - verification of fetched bytes when content is omitted (tamper detection) - verifyCIDDetailed returning the computed CID and reporting fetch errors as valid: false rather than throwing 65 tests pass in this file.
|
@maztah1 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! 🚀 |
7 tasks
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.
What the issue was
#848 asked for IPFS invoice metadata pinning with CID verification, listing
verifyCID(cid, content)as a requirement and "CID verification detects tampering" as an acceptance criterion.The pinning, gateway/kubo backends and
enrichInvoicewere already implemented. The verification was not sound, though — and I want to be direct that this is a correctness/security fix rather than a new feature.The bug
verifyCID()did this:It compared content against content. The CID was never validated — it was only used as a fetch key. So
verifyCID("THIS-IS-NOT-A-REAL-CID", <anything the gateway returns>)returnstrue. The old docstring admitted the limitation: "Since we can't recompute the exact CID without the multihash library…" — but the function was still exported as the tamper check, andverifyCIDOrThrowdelegated to it.Three existing tests asserted
verifyCIDreturnedtruefor invented CIDs like"QmVerifyCid123456789012345678901234567890", which were never hashes of the test content. They passed because verification was broken. That is what convinced me this mattered: the test suite was pinning the bug in place.The fix
A CID embeds the multihash of the content it names, so verification means recomputing it and comparing:
Qm…, base58btc) and CIDv1 (bafy…, base32 dag-pb) computation over a sha2-256 multihash. Both encoders implemented directly rather than pulling in a dependency —multiformatsis present only transitively via WalletConnect, and depending on that would be fragile.verifyCID(cid)with content omitted verifies whatever the CID resolves to — that is what detects a gateway serving tampered bytes under an otherwise-valid CID.ipfs://URI and/ipfs/path forms are tolerated.verifyCIDDetailed()returning aCIDVerificationResult;verifyCIDOrThrownow reports the CID the content actually produced, so a mismatch tells you whether the address or the bytes are wrong.I verified the encoders against an independently computed sha2-256 multihash vector before wiring them in — the CIDv0 output matched a from-scratch base58 implementation.
How it was tested
63 tests in
test/ipfs.test.ts, up from 54. The three fake-CID tests are replaced with CIDs computed from the content viacomputeCidV0. New coverage:ipfs://URI formverifyCIDDetailedreturning the computed CID, and reporting fetch errors asvalid: falserather than throwingFull suite: 213 passed, 1 skipped, 0 failed.
tsc --noEmitdiffed against base — no new errors.Behaviour change to be aware of
verifyCIDandverifyCIDOrThrownow returnfalsefor CIDs that were never real hashes of the content. That is the point of the fix, but it will surface callers who were relying on the loose check.contentis now optional on both.closes #848