Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: support subdomains in isIPFS.url(url) #32

Merged
merged 10 commits into from
Apr 5, 2020
Merged

feat: support subdomains in isIPFS.url(url) #32

merged 10 commits into from
Apr 5, 2020

Conversation

lidel
Copy link
Member

@lidel lidel commented Mar 22, 2020 •
edited
Loading

This PR updates isIPFS.url(url) to return true if isIPFS.subdomain(url) does,
and improves isIPFS.ipnsSubdomain(url) to match potential DNSLink just like isIPFS.ipnsPath(url) already did.

This is needed for ipfs/ipfs-companion#853:

BREAKING CHANGES

  • BREAKING CHANGE: isIPFS.subdomain now returns true for <domain.tld>.ipns.localhost
  • BREAKING CHANGE: isIPFS.subdomainPattern|pathPattern|urlPattern are replaced with pathPattern|pathGatewayPattern|subdomainGatewayPattern
  • BREAKING CHANGE: isIPFS.url now returns true if isIPFS.subdomain did

Due to this I plan to release this as v1.0.0

lidel added 4 commits March 20, 2020 22:15
@lidel
feat: support DNSLink subdomains
cc40cd4 
This change adds support for DNSLink subdomains on localhost gateway
(ipfs/kubo#6096)

Example: en.wikipedia-on-ipfs.org.ipfs.localhost:8080

BREAKING CHANGE: `isIPFS.subdomain` now returns true for <domain.tld>.ipns.localhost
BREAKING CHANGE: `isIPFS.subdomainPattern` changed

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
test: support peer multiaddr with /p2p/
2644db0 
Context: libp2p/libp2p#79

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
fix: explicitly ignore URL param and hash
72c0344 
.url and .path now return true when validating:
https://ipfs.io/ipfs/<CID>?filename=name.png#foo

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
refactor: simplify dnslinkSubdomain
f1823cc 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel requested a review from alanshaw March 22, 2020 23:31
@lidel lidel mentioned this pull request Mar 22, 2020
Merged
20 tasks
alanshaw
alanshaw reviewed Mar 22, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md

### `isIPFS.ipfsSubdomain(url)`

Returns `true` if the provided string includes a valid IPFS subdomain or `false` otherwise.
Returns `true` if the provided `url` string includes a valid IPFS subdomain (case-insensitive CIDv1) or `false` otherwise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it check for a case insensitive encoding or is this actually just base32?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web browsers force-lowercase hostname in URL and is-ipfs does the same before we check if CID in subdomain is valid:
https://github.com/ipfs/is-ipfs/blob/f1823cc0b12e97ac1f202ba7c06663b5b9b5d265/src/index.js#L83-L87

README.md Outdated

isIPFS.path('/ipfs/QmYjtig7VJQ6XsnUjqqJvj7QaMcCAwtrgNdahSiFofrE7o') // true
isIPFS.path('/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?filename=guardian.jpg') // true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a thing now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ?filename= sets proper Content-Disposition header and all that jazz. Folks use it as more efficient way of sharing single file, as it keeps the same CID while enables filename tweaks (does not require wrapping in a directory)

I've added it to README as its important to show that URL params do not impact CID validation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it does that for URLs on the gateways but I didn't know if we'd formalised it in IPFS paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get your point now.

No, it is not formalized in the context of IPFS paths – ?filename=foo.jpg is just a valid filename in IPFS.

See this example:

$ ipfs object patch add-link QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn "?filename=test.jpg" QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR
QmbcBPAwCDxRMB1Qe7CRQmxdrTSkxKwM9y6rZw2FjGtbsb

$ ipfs ls /ipfs/QmbcBPAwCDxRMB1Qe7CRQmxdrTSkxKwM9y6rZw2FjGtbsb
QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR 119762 ?filename=test.jpg

When exposed on the gateway, filenames are URIencoded:
https://ipfs.io/ipfs/QmbcBPAwCDxRMB1Qe7CRQmxdrTSkxKwM9y6rZw2FjGtbsb/%3Ffilename=test.jpg

Here I just forgot / before ?, but I agree the naming overlap is confusing, so I changed it to be something else:

Suggested change
isIPFS.path('/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?filename=guardian.jpg') // true
isIPFS.path('/ipfs/QmbcBPAwCDxRMB1Qe7CRQmxdrTSkxKwM9y6rZw2FjGtbsb/?weird-filename=test.jpg') // true

lidel added a commit that referenced this pull request Mar 24, 2020
@lidel
fix: url() check should include subdomain()
bd61cb6 
When .url was created we only had path gateways.  When .subdomain was
added, we did not update .url to test for subdomain gateways, which in
the long run will confuse people and feels like a bug.

Let's fix this: .url() will now check for both subdomain and path gateways

#32 (comment)

BREAKING CHANGE: .url(url) now returns true if .subdomain(url) is true

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
fix: url() check should include subdomain()
c520efc 
When .url was created we only had path gateways.  When .subdomain was
added, we did not update .url to test for subdomain gateways, which in
the long run will confuse people and feels like a bug.

Let's fix this: .url() will now check for both subdomain and path gateways

#32 (comment)

BREAKING CHANGE: .url(url) now returns true if .subdomain(url) is true

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/dnslink-subdomains branch from bd61cb6 to c520efc Compare March 24, 2020 23:27
@lidel lidel requested a review from alanshaw March 24, 2020 23:27
@lidel
refactor: merge DNSLink check into ipnsSubdomain()
d9e7082 
This makes subdomain checks follow what path gateway checks do, removing
confusion.

In both cases (IPNS and DNSLink) user needs to perform online record
check, so this is just a handy way of detecting potential matches.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel changed the title feat: isIPFS.dnslinkSubdomain(url) feat: support subdomains in isIPFS.url(url) Mar 25, 2020
@lidel
docs: update examples
d5717e9 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/dnslink-subdomains branch from 8066bad to d5717e9 Compare April 3, 2020 00:10
@lidel lidel requested a review from hugomrdias April 3, 2020 00:29
hugomrdias
hugomrdias requested changes Apr 3, 2020
View reviewed changes
src/index.js Show resolved Hide resolved
@lidel
refactor: switch to iso-url
87d746a 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/dnslink-subdomains branch from 978a5ed to 87d746a Compare April 5, 2020 15:22
lidel added 2 commits April 5, 2020 17:33
@lidel
refactor: lint-package-json
404957e 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
chore: update deps
18d80d4 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel merged commit 22d001d into master Apr 5, 2020
@lidel lidel deleted the feat/dnslink-subdomains branch April 5, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugomrdias hugomrdias hugomrdias requested changes

@alanshaw alanshaw Awaiting requested review from alanshaw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lidel @alanshaw @hugomrdias