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.

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 Suborigin header to gateway responses #3914

Merged
merged 1 commit into from May 18, 2017
Merged

Conversation

jes
Copy link
Contributor

@jes jes commented May 10, 2017

This existed before but was disabled in 912a972 because the Suborigin spec
changed and it became incompatible.

This commit updates the generated Suborigin header to be conformant with the
latest spec.

Thank you Stskeeps, Kubuxu, and TUSF on IRC for holding my hand with this.

@jes jes force-pushed the master branch 2 times, most recently from b8bdc0b to 9a76674 Compare May 10, 2017 12:24
//
// NOTE: This is not yet widely supported by browsers.
if !ipnsHostname {
// e.g.: 0="ipfs.io", 1="ipfs", 2="QmYuNaKwY...", ...
Copy link

Choose a reason for hiding this comment

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

Part 0 is simply empty, urlPath is something like /ipfs/QmFoo/a/b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, I'll fix.

if !ipnsHostname {
// e.g.: 0="ipfs.io", 1="ipfs", 2="QmYuNaKwY...", ...
pathComponents := strings.SplitN(urlPath, "/", 4)
suboriginRaw := []byte(strings.ToLower(pathComponents[2]))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right. You can't 'ToLower' encoded value as it will change the IPFS hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the code isn't clear enough -- this part handles the /ipns/EXAMPLE.COM/ case; when the second part will decode as base58, it is handled accordingly. I'll add a comment to explain this.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I missed that. Yeah, it is better not to be clever in places like that.

This existed before but was disabled in 912a972 because the Suborigin spec
changed and it became incompatible.

This commit updates the generated Suborigin header to be conformant with the
latest spec.

License: MIT
Signed-off-by: James Stanley <james@incoherency.co.uk>
@whyrusleeping
Copy link
Member

@lgierth @Kubuxu hows this one looking?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

One more small thing, otherwise LGTM 👍


base32Encoded, err := multibase.Encode(multibase.Base32, suboriginRaw)
if err != nil {
internalWebError(w, err)
Copy link

Choose a reason for hiding this comment

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

This should be a 4xx error via webError(), probably just 400 Bad Request. It's an error that can only be recovered from by altering the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how base32 encoding can fail -- if it does, surely that's an internal server error, and nothing to do with the client?

Copy link

Choose a reason for hiding this comment

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

Mh -- yeah in this case the error can only come from an unsupported encoding parameter: https://github.com/multiformats/go-multibase/blob/master/multibase.go

LGTM then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) Thanks mate.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

@whyrusleeping whyrusleeping merged commit 5ef2f42 into ipfs:master May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants