-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add validation of CID in Subdomains #20
Conversation
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.
Super cool. Really nice to see this module getting some ❤️
README.md
Outdated
isIPFS.multibase('bafybeie5gq4jxvzmsym6hjlwxej4rwdoxt7wadqvmmwbqi7r27fclha2va') // 'base32' | ||
isIPFS.multibase('zdj7WWeQ43G6JJvLWQWZpyHuAMq6uYWRjkBXFad11vE2LHhQ7') // 'base58btc' | ||
isIPFS.multibase('QmYjtig7VJQ6XsnUjqqJvj7QaMcCAwtrgNdahSiFofrE7o') // false (no multibase prefix in CIDv0) | ||
isIPFS.multibase('noop') // false |
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.
This feels out of place to me. The rest of the API returns true/false. This is a simple call-through to multibase.isEncoded
.
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.
Should it be changed to true
/false
or just removed? Not sure how useful it is, maybe just remove it?
(I think I've added it only because isIPFS.base32cid
introduced multibase as a dependency)
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.
Personally, I'd remove it. I considered that isIPFS.base32cid
uses multibase as a dependency, and was thinking of suggesting to check like:
typeof cid === 'string' && cid.slice(0, 4) === 'bafy' && isCID(cid)
...but js-cid depends on multibase so you're not saving any bundle bytes by depending on it and using it.
README.md
Outdated
@@ -71,29 +82,52 @@ isIPFS.ipfsPath('/ipfs/invalid-hash') // false | |||
|
|||
isIPFS.ipnsPath('/ipfs/QmYjtig7VJQ6XsnUjqqJvj7QaMcCAwtrgNdahSiFofrE7o') // false | |||
isIPFS.ipnsPath('/ipns/github.com') // true | |||
|
|||
isIPFS.subdomain('http://bafybeiabc2xofh6tdi6vutusorpumwcikw3hf3st4ecjugo6j52f6xwc6q.ipns.dweb.link') // true |
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.
Is this true for http://bafybeie5gq4jxvzmsym6hjlwxej4rwdoxt7wadqvmmwbqi7r27fclha2va.ipfs.dweb.link
also? Maybe add an example here :D
@alanshaw removed multibase, Jenkins works as expected. Ok to merge? |
@@ -2,22 +2,37 @@ | |||
|
|||
const base58 = require('bs58') | |||
const multihash = require('multihashes') | |||
const multibase = require('multibase') |
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.
You still depend directly on multibase - it should be in dependencies
README.md
Outdated
## API | ||
# API | ||
|
||
A suite of util methods provides efficient validation. |
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.
...that
provides...
src/index.js
Outdated
base32cid: (cid) => (isMultibase(cid) === 'base32' && isCID(cid)), | ||
ipfsSubdomain: (url) => isIpfs(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch), | ||
ipnsSubdomain: (url) => isIpns(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch), | ||
subdomain: (url) => (isIpfs(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch) || isIpns(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch)), |
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.
As you're calling (ipfsSubdomain || ipnsSubdomain)
, you should declare them before the module.exports
. That way if any of those change, you won't need to change that code in two places.
To make things clearer:
const ipfsSubdomain = (url) => isIpfs(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch)
const ipnsSubdomain = (url) => isIpns(url, fqdnPattern, fqdnProtocolMatch, fqdnHashMatch)
module.exports = {
multihash: isMultihash,
cid: isCID,
base32cid: (cid) => (isMultibase(cid) === 'base32' && isCID(cid)),
ipfsSubdomain: ipfsSubdomain,
ipnsSubdomain: ipnsSubdomain,
subdomain: (url) => (ipfsSubdomain(url) || ipnsSubdomain(url)),
...
}
(Parent issues: ipfs/ipfs-companion#526, ipfs/in-web-browsers#89, https://github.com/ipfs/ipfs/issues/337)
This PR adds below utility methods:
cidv1b32.ip(f|n)s.domain.tld
suggested in Detect CID in subdomains ipfs/ipfs-companion#526 (comment)cidv1b32
isIPFS.url
on purpose.People already use it and we don't want to change its meaning, because
isIPFS.url == true
means it is safe to take URLs pathname as-is, which is not the case when subdomain is used.isIPFS.nativeUrl
foripfs://
for the same reasonpath
||url
||subdomain
||nativeUrl
) could be namedisIPFS.any
cc @lgierth @kyledrake – thoughts?
(We need this for Companion to support cidv1b32 and subdomains, and I want to ship it before DWEB)