Skip to content
This repository was archived by the owner on Aug 14, 2019. It is now read-only.

Remove forced ssl upgrade for localhost http requests#859

Merged
vvisigoth merged 2 commits intourbit:release-candidatefrom
jglukasik:remove-localhost-forced-ssl
Oct 16, 2018
Merged

Remove forced ssl upgrade for localhost http requests#859
vvisigoth merged 2 commits intourbit:release-candidatefrom
jglukasik:remove-localhost-forced-ssl

Conversation

@jglukasik
Copy link
Copy Markdown
Contributor

I've been playing around trying to glue urbit and IPFS together. For my first iteration, I tried to +curl my local IPFS API from the Dojo. Because go-ipfs doesn't implement HTTPS, I made sure to do a +curl "http://localhost:5001...", but I was still getting an ssl related error:

> +curl "http://localhost:5001/api/v0/get?arg=QmagqTLSZvdqjWp2hVFokDyThekNAACsUG8r2XGhBWPQh6"
< http://localhost:5001/api/v0/get?arg=QmagqTLSZvdqjWp2hVFokDyThekNAACsUG8r2XGhBWPQh6
http: fail (170, 504): ssl handshake failure
HTTP 504

Talking to @Fang- at the last meetup, he reassured me this should not be happening, and specifying "http" should do a regular non-ssl request. I dug in a bit and found the issue: in the definition for ++ auri there is a conditional to check if this is a localhost request (using ++ hoke), and if so, it forces it to use SSL.

This PR removes this forced upgrade, so i can specify either an http or https request to localhost.

There might be a reason for this check, but I couldn't find one. I did some git spelunking and found that this was added in commit 2c3c4c8 , so maybe @cgyarvin knows if this is still needed or not.

Also, I don't fully grok the implications of creating this "iron gate" here; I cargo-culted from a similar-looking ++ aurf definition above and it worked :)

Comment thread sys/zuse.hoon Outdated
Copy link
Copy Markdown
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

That was fast! I said I'd insta-merge, but I have a question about your specific implementation, maybe it can be simplified further.

Also, @cgyarvin, you put the forced-secure in like 2 years ago. If you can recall a good reason for it, speak up now.

Now that the uri parsing is not being modified by the localhost forced ssl
upgrade, this cook is not needed.
Copy link
Copy Markdown
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Perfect, thanks so much!

I'll give @cgyarvin a couple days to respond, and then merge. (:

@joemfb
Copy link
Copy Markdown
Contributor

joemfb commented Oct 13, 2018

I've assumed that this was related to the way in which user-agents treat localhost as a secure origin (for certain web apis, mixed-content warnings, etc.). But it's a major pain when it comes to making requests instead of receiving them. Maybe we still want to include something similar in the latter case, but definitely not in the former. Thanks @jglukasik!

@vvisigoth vvisigoth merged commit 7da389b into urbit:release-candidate Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants