Fix ipfs address generation #655
Conversation
adonesky1
force-pushed
the
fix-ipfs-address-generation
branch
from
4683159
to
803fa01
Compare
adonesky1
force-pushed
the
fix-ipfs-address-generation
branch
from
0fd43a1
to
6837f4a
Compare
adonesky1
force-pushed
the
fix-ipfs-address-generation
branch
from
b6efc25
to
a32fe61
Compare
src/util.ts
Outdated
| // (CID is everything preceding first forward slash, path is everything after) | ||
| const index = url.indexOf('/'); | ||
| const cid = index !== -1 ? url.substring(0, index) : url; | ||
| const path = index !== -1 ? url.substring(index) : ''; |
There was a problem hiding this comment.
Nit: Could we default to undefined here for path instead, if it's not present? That would match the type signature for this function.
There was a problem hiding this comment.
done here: d4c8ade
src/util.ts
Outdated
| const gatewayHost = new URL(addUrlProtocolPrefix(ipfsGateway)).host; | ||
| const { cid, path } = getIpfsCIDv1AndPath(ipfsUrl); | ||
| return `https://${cid}.ipfs.${gatewayHost}${path}`; |
There was a problem hiding this comment.
It would be better to extract the protocol from the IPFS gateway as well, if it's specified. Users might want to use alternative protocols.
| const gatewayHost = new URL(addUrlProtocolPrefix(ipfsGateway)).host; | |
| const { cid, path } = getIpfsCIDv1AndPath(ipfsUrl); | |
| return `https://${cid}.ipfs.${gatewayHost}${path}`; | |
| const { host, protocol } = new URL(addUrlProtocolPrefix(ipfsGateway)); | |
| const { cid, path } = getIpfsCIDv1AndPath(ipfsUrl); | |
| return `${protocol}//${cid}.ipfs.${host}${path}`; |
There was a problem hiding this comment.
done here: d4c8ade
src/util.ts
Outdated
| /** | ||
| * Formats url correctly for use retrieving assets hosted on IPFS. | ||
| * | ||
| * @param ipfsGateway - the user preferred ipfsGateway. |
There was a problem hiding this comment.
Nit: These descriptions could be better. Keep in mind that these aren't just code comments, they are inline documentation. This gets used to build our API documentation (which isn't automatically built/hosted right now, but it will be soon, and it can be generated manually). Better to use proper case (start the sentence with a capital letter, capitalize acronyms like IPFS, use words and not variable names, etc.), and to include relevant details like what sort of string this is.
For example, it looks like this is expected to be the URL for the IPFS gateway, except that the protocol is optional, and the trailing /ipfs path is optional.
The same general point applies to most of the other inline docs as well.
|
Overall this looks good! I left some suggestions for improvements |
| * @returns A URL with a https:// prepended. | ||
| */ | ||
| export function addUrlProtocolPrefix(urlString: string): string { | ||
| if (!urlString.match(/(^http:\/\/)|(^https:\/\/)/u)) { |
There was a problem hiding this comment.
NIt: This still only allows http and https protocols, so the protocol support added below only helps with HTTP support. We could instead check here to ensure any protocol is set.
This might be an extreme edge case though. I don't know of any IPFS gateways run on non-HTTP/HTTPS protocols 🤔 I don't know much about this space though.
There was a problem hiding this comment.
Will research and handle in follow up PR!
| ); | ||
| }); | ||
|
|
||
| it('should return a URL as is if https:// is already prepended', () => { |
There was a problem hiding this comment.
Nit: It's probably also worth ensuring that http URLs are handled correctly as well
There was a problem hiding this comment.
Will handle in follow up PR!
* use ipfs subdomain routes and convert cid's from v0 to v1 * add a couple more tests * wip * refactor * wip * tests fixed * tests fixed * addressing feedback * improve comment
* use ipfs subdomain routes and convert cid's from v0 to v1 * add a couple more tests * wip * refactor * wip * tests fixed * tests fixed * addressing feedback * improve comment
CHANGED:
-BREAKING Change IPFS URL generation to use subdomains and cidV1s over cidV0s, in order to enhance origin based security in our use of IPFS assets