-
Notifications
You must be signed in to change notification settings - Fork 25
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!: only accept CIDs, PeerIds or strings as values #255
Changes from 2 commits
6a85abf
968e537
76b0799
4f832ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 slight concern is that (iiuc) we are generating test vectors on the fly using JS functions from @achingbrain thoughts on including tests against binary fixtures There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,19 @@ | ||
/* eslint-env mocha */ | ||
|
||
import { randomBytes } from '@libp2p/crypto' | ||
import { generateKeyPair } from '@libp2p/crypto/keys' | ||
import { generateKeyPair, unmarshalPrivateKey } from '@libp2p/crypto/keys' | ||
import { peerIdFromKeys, peerIdFromString } from '@libp2p/peer-id' | ||
import { createEd25519PeerId } from '@libp2p/peer-id-factory' | ||
import { expect } from 'aegir/chai' | ||
import * as cbor from 'cborg' | ||
import { base58btc } from 'multiformats/bases/base58' | ||
import { CID } from 'multiformats/cid' | ||
import { toString as uint8ArrayToString } from 'uint8arrays' | ||
import { fromString as uint8ArrayFromString } from 'uint8arrays/from-string' | ||
import * as ERRORS from '../src/errors.js' | ||
import * as ipns from '../src/index.js' | ||
import { IpnsEntry } from '../src/pb/ipns.js' | ||
import { extractPublicKey, peerIdToRoutingKey, parseCborData, createCborData } from '../src/utils.js' | ||
import { extractPublicKey, peerIdToRoutingKey, parseCborData, createCborData, ipnsRecordDataForV2Sig } from '../src/utils.js' | ||
import { ipnsValidator } from '../src/validator.js' | ||
import type { PeerId } from '@libp2p/interface-peer-id' | ||
|
||
|
@@ -142,37 +144,44 @@ describe('ipns', function () { | |
await ipnsValidator(peerIdToRoutingKey(peerId), ipns.marshal(record)) | ||
}) | ||
|
||
it('should normalize value when creating an ipns record (string v0 cid)', async () => { | ||
const inputValue = 'QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq' | ||
const expectedValue = '/ipfs/bafybeidvkqhl6dwsdzx5km7tupo33ywt7czkl5topwogxx6lybko2d7pua' | ||
it('should normalize value when creating an ipns record (arbitrary string path)', async () => { | ||
const inputValue = '/foo/bar/baz' | ||
const expectedValue = '/foo/bar/baz' | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
expect(record.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when creating an ipns record (string v1 cid)', async () => { | ||
const inputValue = 'bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
const expectedValue = '/ipfs/bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
it('should normalize value when creating a recursive ipns record (peer id)', async () => { | ||
const inputValue = await createEd25519PeerId() | ||
const expectedValue = `/ipns/${inputValue.toString()}` | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
expect(record.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when creating an ipns record (bytes v0 cid)', async () => { | ||
const inputValue = uint8ArrayFromString('QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq') | ||
it('should normalize value when creating a recursive ipns record (peer id as CID)', async () => { | ||
const inputValue = await createEd25519PeerId() | ||
const expectedValue = `/ipns/${inputValue.toString()}` | ||
const record = await ipns.create(peerId, inputValue.toCID(), 0, 1000000) | ||
expect(record.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when creating an ipns record (v0 cid)', async () => { | ||
const inputValue = CID.parse('QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq') | ||
const expectedValue = '/ipfs/bafybeidvkqhl6dwsdzx5km7tupo33ywt7czkl5topwogxx6lybko2d7pua' | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
expect(record.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when creating an ipns record (bytes v1 cid)', async () => { | ||
const inputValue = uint8ArrayFromString('bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu') | ||
it('should normalize value when creating an ipns record (v1 cid)', async () => { | ||
const inputValue = CID.parse('bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu') | ||
const expectedValue = '/ipfs/bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
expect(record.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when reading an ipns record (bytes v0 cid)', async () => { | ||
const inputValue = 'QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq' | ||
const expectedValue = '/ipfs/bafybeidvkqhl6dwsdzx5km7tupo33ywt7czkl5topwogxx6lybko2d7pua' | ||
it('should normalize value when reading an ipns record (string v0 cid path)', async () => { | ||
const inputValue = '/ipfs/QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq' | ||
const expectedValue = '/ipfs/QmWEekX7EZLUd9VXRNMRXW3LXe4F6x7mB8oPxY5XLptrBq' | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
|
||
const pb = IpnsEntry.decode(ipns.marshal(record)) | ||
|
@@ -183,8 +192,8 @@ describe('ipns', function () { | |
expect(modifiedRecord.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should normalize value when reading an ipns record (bytes v1 cid)', async () => { | ||
const inputValue = 'bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
it('should normalize value when reading an ipns record (string v1 cid path)', async () => { | ||
const inputValue = '/ipfs/bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
const expectedValue = '/ipfs/bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu' | ||
const record = await ipns.create(peerId, inputValue, 0, 1000000) | ||
|
||
|
@@ -196,6 +205,20 @@ describe('ipns', function () { | |
expect(modifiedRecord.value).to.equal(expectedValue) | ||
}) | ||
|
||
it('should fail to normalize non-path value', async () => { | ||
const inputValue = 'hello' | ||
|
||
await expect(ipns.create(peerId, inputValue, 0, 1000000)).to.eventually.be.rejected | ||
.with.property('code', ERRORS.ERR_INVALID_VALUE) | ||
}) | ||
|
||
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) | ||
}) | ||
Comment on lines
+216
to
+221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ sgtm, forces people to provide value under a meaningful namespace 👍 |
||
|
||
it('should fail to validate a v1 (deprecated legacy) message', async () => { | ||
const sequence = 0 | ||
const validity = 1000000 | ||
|
@@ -345,4 +368,32 @@ describe('ipns', function () { | |
bytes: peerId.publicKey | ||
}) | ||
}) | ||
|
||
it('should unmarshal a record with raw CID bytes', async () => { | ||
// we may encounter these in the wild due to older versions of this module | ||
// but IPNS records should have string path values | ||
|
||
// create a dummy record with an arbitrary string path | ||
const input = await ipns.create(peerId, '/foo', 0n, 10000, { | ||
v1Compatible: false | ||
}) | ||
|
||
// we will store the raw bytes from this CID | ||
const cid = CID.parse('bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu') | ||
|
||
// override data with raw CID bytes | ||
const data = cbor.decode(input.data) | ||
data.Value = cid.bytes | ||
input.data = cbor.encode(data) | ||
|
||
// re-sign record | ||
const privateKey = await unmarshalPrivateKey(peerId.privateKey ?? new Uint8Array(0)) | ||
const sigData = ipnsRecordDataForV2Sig(input.data) | ||
input.signatureV2 = await privateKey.sign(sigData) | ||
|
||
const buf = ipns.marshal(input) | ||
const record = ipns.unmarshal(buf) | ||
|
||
expect(record).to.have.property('value', '/ipfs/bafkqae3imvwgy3zamzzg63janjzs22lqnzzqu') | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Do you mean people are extracting
12foo
out of/ipns/12foo
and trying to decode it as aCID
? Seems like user error rather than a bug if so.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 encodedPeerID
and{thing}
in/ipfs/{thing}
is always aCID
(which could be alibp2p-key
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, as long as we're fine with it being
/ipns/{maybe-CID,maybe-base58btcpeerid}
I guess?