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

achingbrain · 2023-08-30T12:10:46Z

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 "/".

This module has accepted `Uint8Array`s 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 · 2023-08-30T12:12:53Z

@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 · 2023-09-04T08:49:24Z

@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 · 2023-09-04T11:23:12Z

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 · 2023-09-04T11:42:34Z

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)

lidel · 2023-09-07T00:46:46Z

src/utils.ts

+ * Normalizes the given record value. It ensures it is a PeerID, a CID or a
+ * string starting with '/'. PeerIDs become `/ipns/${peerId}`, CIDs become
+ * `/ipfs/${cidAsV1}`.
 */
-export const normalizeValue = (value: string | Uint8Array): string => {
- const str = typeof value === 'string' ? value : uint8ArrayToString(value)
+export const normalizeValue = (value?: CID | PeerId | string | Uint8Array): string => {


nit: iirc we try to do opposite and want to remove use of PeerIDs in IPNS context, as it causes bugs where someone tries to parse 12.. one as CID (example) .

In various places we've been normalizing to CIDv1 with libp2p-key and base36 (this way it looks the same on subdomains, and implementers only need to support CIDs, which is more robust for interop).

@achingbrain would it be ok to switch /ipns/ paths to use CIDv1 with libp2p-key? (base does not matter as much as CIDv1)

we try to do opposite and want to remove use of PeerIDs in IPNS context

What I really want to do here is take a PeerID and do the right thing, rather than relying on users to know what the right thing to do is.

it causes bugs where someone tries to parse 12.. one as CID (example) .

Do you mean people are extracting 12foo out of /ipns/12foo and trying to decode it as a CID? Seems like user error rather than a bug if so.

would it be ok to switch /ipns/ paths to use CIDv1 with libp2p-key?

Do you mean accept a PeerId and output "/ipns/base32-encoded-libp2p-key-cid" ?

Maybe outputting "/ipfs/base32-encoded-libp2p-key-cid" would be better? Will Kubo resolve IPNS values like this recursively?

Then {thing} in /ipns/{thing} is always a base58btc encoded PeerID and {thing} in /ipfs/{thing} is always a CID (which could be a libp2p-key)?

@achingbrain I think the goal moving forward into the future is to always refer to /ipns/{libp2p-encoded-cid} and not as a base56btc: https://specs.ipfs.tech/ipns/ipns-record/#string-representation, including on the gateway.

The current gateway implementation does the redirect even.

Ok, as long as we're fine with it being /ipns/{maybe-CID,maybe-base58btcpeerid} I guess?

achingbrain · 2023-09-08T15:08:24Z

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

lidel

@achingbrain sgtm, pushed jsdoc update to reflect that its cidV1.

See comment inline about also testing against static vectors from ipfs/gateway-conformance#157

lidel · 2023-09-12T15:37:21Z

test/index.spec.ts

+ it('should fail to normalize path value that is too short', async () => {
+ const inputValue = '/'
+
+ await expect(ipns.create(peerId, inputValue, 0, 1000000)).to.eventually.be.rejected
+ .with.property('code', ERRORS.ERR_INVALID_VALUE)
+ })


ℹ️ sgtm, forces people to provide value under a meaningful namespace 👍

lidel · 2023-09-12T15:39:30Z

src/utils.ts

+ // if we have a CID, turn it into an ipfs path
+ const cid = CID.asCID(value)
+ if (cid != null) {
+ // PeerID encoded as a CID
+ if (cid.code === LIBP2P_CID_CODEC) {
+ return `/ipns/${cid.toString(base36)}`
+ }
+
+ return `/ipfs/${cid.toV1().toString()}`


ℹ️ good quality of life improvement, thanks 👍

lidel · 2023-09-12T15:47:42Z

test/index.spec.ts

💭 slight concern is that (iiuc) we are generating test vectors on the fly using JS functions from src/utils.ts, and if we change JS code at any point in the future, we may not catch regression until there is a hiccup somewhere in production when interacting with other implementation.

@achingbrain thoughts on including tests against binary fixtures /fixtures/ipns_records/*.ipns-record in
ipfs/gateway-conformance#157? This will be a solid way to ensure interop with GO and any other languages. We could do it in follow-up PR but

Sounds good!

achingbrain requested review from hacdias and lidel August 30, 2023 12:10

fix: treat a PeerId encoded as CID as a PeerId

968e537

lidel requested changes Sep 7, 2023

View reviewed changes

fix: encode all /ipns/* keys as base36 encoded CIDv1 libp2p-cid

76b0799

achingbrain requested a review from lidel September 8, 2023 15:44

docs: update jsdoc about

4f832ba

lidel approved these changes Sep 12, 2023

View reviewed changes

achingbrain merged commit cedb3f2 into validate-v2 Sep 12, 2023
16 checks passed

achingbrain deleted the fix/tighten-up-input-types branch September 12, 2023 16:14

lidel mentioned this pull request Sep 13, 2023

feat!: opt-in V2-only records, IPIP-428 verification #234

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achingbrain commented Aug 30, 2023

achingbrain commented Aug 30, 2023 •

edited

Loading

hacdias commented Sep 4, 2023

achingbrain commented Sep 4, 2023

achingbrain commented Sep 4, 2023

lidel Sep 7, 2023 •

edited

Loading

achingbrain Sep 7, 2023

hacdias Sep 7, 2023 •

edited

Loading

achingbrain Sep 7, 2023

achingbrain commented Sep 8, 2023

lidel left a comment •

edited

Loading

lidel Sep 12, 2023

lidel Sep 12, 2023

lidel Sep 12, 2023

achingbrain Sep 12, 2023

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

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

Conversation

achingbrain commented Aug 30, 2023

achingbrain commented Aug 30, 2023 • edited Loading

hacdias commented Sep 4, 2023

achingbrain commented Sep 4, 2023

achingbrain commented Sep 4, 2023

lidel Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

achingbrain Sep 7, 2023

Choose a reason for hiding this comment

hacdias Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

achingbrain Sep 7, 2023

Choose a reason for hiding this comment

achingbrain commented Sep 8, 2023

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Sep 12, 2023

Choose a reason for hiding this comment

lidel Sep 12, 2023

Choose a reason for hiding this comment

lidel Sep 12, 2023

Choose a reason for hiding this comment

achingbrain Sep 12, 2023

Choose a reason for hiding this comment

achingbrain commented Aug 30, 2023 •

edited

Loading

lidel Sep 7, 2023 •

edited

Loading

hacdias Sep 7, 2023 •

edited

Loading

lidel left a comment •

edited

Loading