Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: @helia/* modules validate CID codec #643

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tabcat
Copy link
Contributor

@tabcat tabcat commented Oct 3, 2024

Title

fix: @helia/* modules validate CID codec

Description

Closes: #377

@helia/* modules related to importing structured data, e.g. @helia/json; @helia/strings, check CID that is handed to .get method.
If the CID code does not match the codec of the importer then it rejects with a TypeError.

  • @helia/json
  • @helia/strings
  • @helia/dag-json
  • @helia/dag-cbor
  • @helia/unixfs

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Comment on lines +118 to 122
if (cid.code !== jsonCodec.code) {
throw new TypeError('The passed CID had an incorrect codec, it may correspond to a non-JSON block')
}

const buf = await this.components.blockstore.get(cid, options)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks that the codec codes match inside of the JSON.get implementation.

@@ -115,6 +115,10 @@ class DefaultJSON implements JSON {
}

async get <T> (cid: CID, options: Partial<GetOptions> = {}): Promise<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random question, was looking at the GetOptions type. It contains codec: BlockCodec property which is passed to the blockstore.get call. Is this ever used?

@tabcat
Copy link
Contributor Author

tabcat commented Oct 3, 2024

Before I finish the other modules, does this approach look ok?

@tabcat tabcat changed the title fix: @helia/modules validate CID codec fix: @helia/* modules validate CID codec Oct 4, 2024
@SgtPooki
Copy link
Member

Before I finish the other modules, does this approach look ok?

This looks good to me. It should be pretty straightforward

@tabcat
Copy link
Contributor Author

tabcat commented Oct 24, 2024

The @helia/strings.get method accepts a codec as an option. Either the option is removed (specifying a codec like raw, the current default) or validation cannot happen on the CID passed to get.

@tabcat
Copy link
Contributor Author

tabcat commented Oct 25, 2024

Actually I can just check the CID.code is raw if no codec option is passed to get.
IMO it would be best to not allow custom codecs to be passed to @helia/strings and just use 'utf8' encoding. If users are bringing their own codec then why not just interact with the blockstore directly?

@tabcat
Copy link
Contributor Author

tabcat commented Dec 6, 2024

Removing @helia/unixfs from todo list for this PR as it's a bit different and more involved than the other importers.

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.

@helia/* modules should throw readable errors if CIDs with mismatched codecs are passed
2 participants