search

ipfs / js-ipns

Utilities for creating, parsing, and validating IPNS records
Other
83 stars 25 forks source link

fix!: only accept CIDs, PeerIds or strings as values #255

Closed achingbrain closed 1 year ago

achingbrain commented 1 year ago

This module has accepted Uint8Arrays as values to store in IPNS records which means it has been mis-used to create records with raw CID bytes.

To ensure this doesn't happen in future, accept only CIDs, PeerIds or arbitrary path strings prefixed with "/".

achingbrain commented 1 year ago

@hacdias @lidel what do you think to tightening up the inputs here, since #234 is a breaking change.

I think this module has only ever accepted Uint8Arrays for ...reasons* - making the value explicitly CID | PeerId | string should make it harder to mis-use in the future?

*= the reason is probably because the protobuf definition has bytes and not string for the record value

hacdias commented 1 year ago

@achingbrain I do like the idea. I'm just concerned about the corner case where a CID is actually a libp2p-key encoded Peer ID. Then we'd be generating a /ipfs/{libp2p-key} instead of a /ipns/{libp2p-key-cid}. I'm not sure if more or less magic would be the best at this point. Sadly, there's no path type to take this issues away.

achingbrain commented 1 year ago

No, but it sounds like an edge case we can just handle. I'd love to get to the point where we can just treat these values as opaque and the module would just do the right thing rather than the user having to know that some types have to be encoded some ways and other types have to be encoded other ways.

achingbrain commented 1 year ago

FWIW Kubo doesn't seem to handle this edge case - trying to publish a peer ID encoded as a CID gives HTTPError: could not choose a decoder: no decoder registered for multicodec code 114 (0x72)

achingbrain commented 1 year ago

@lidel @hacdias I've switched /ipns/* to always be normalised to /ipns/{base36-encoded-cidv1-libp2p-key}