-
Notifications
You must be signed in to change notification settings - Fork 918
Add behaviors to enable setting strictSSL
option for file downloads #2212
#2214
Add behaviors to enable setting strictSSL
option for file downloads #2212
#2214
Conversation
… using the `request` package. Implements LightTable#2212
@mluisbrown Thanks for the PR! The changes look fine. Does this work with the current version of the request package? One problem the rest of the core team might have with these changes is not having a way for us to personally QA them. We've run into the same problem with the change to bump the version of the request package (#2208). If you could help us, or anyone, configure a proxy on our computers that simulates or is equivalent to your proxy setup, then we could QA the changes ourselves and merge the changes right away. |
@cldwalker @rundis Thoughts? |
@kenny-evitt unfortunately no. I had to update the request package to 2.72.0 to get this to work. Proxy support generally seems to be broken in the current version (2.14.0). If you're running OS X you can use Charles Proxy as a local proxy and also configure it to use a MITM SSL certificate to intercept and decrypt SSL traffic, which accurately simulates the environment this PR is trying to address. Charles proxy automatically sets itself up running on http://127.0.0.1:8888. Because of the hoops you have to jump through to get environment variables picked up by GUI apps launched the normal way in OS X, it's easier if you set them in the shell and then launch LT from the shell:
Also, I found that I had to disabled the "Menu -> Proxy -> Mac OS X Proxy" option in Charles. This makes Charles run as the OS X system proxy for all network requests, but we want to only proxy requests explicitly sent via 127.0.0.1:8888. Once you've configured Charles to intercept SSL traffic to If you're running on Windows you should be able to user Fiddler instead of Charles to achieve something similar (it also can do SSL intercepts). |
@mluisbrown Thanks. I'll try to give setting-up a proxy another go sometime soonish. |
@kenny-evitt let me know if you need any help setting up a proxy. With Charles or Fiddler (on OS X or Windows respectively) it's really just a question of downloading and running the proxy. They both auto-configure to 127.0.0.1:8888 and as soon as you quit the app your system is back to normal. |
@mluisbrown Help would be extremely appreciated. I think I can figure out how to download and install Charles or Fiddler, but detailed instructions on setting-up a proxy would be great. We'd also need to do the same on Linux, but just being able to reliably QA this (and similar) issues would be a huge step forward. |
@mluisbrown Thanks for the PR. Seems reasonable given your use case. As @kenny-evitt mentioned detailed, instructions to setting up a proxy would be great as it enables us to QA and support this feature going forward
If proxying functionality requires 2.72.0 we should pull that into this PR. Alternatively, since there is only one place where we use |
@kenny-evitt and @cldwalker: I will post detailed instructions for setting up and configuring a proxy here as soon as I can.
I will see how easy it is to configure proxy support (for both http and https) with the standard http lib. It would appear that it's not totally straightforward. |
Here are the steps required to setup a proxy on OS X: Setup Proxy
Launch Light Table with proxying enabledAs it's a little fiddly to set environment variables in OS X for GUI apps, it's easier for the purpose of testing set the proxy environment variables on the command line and launch Light Table from the command line (so that it picks them up):
Now you can try updating the Plugin Metadata ( Setup SSL proxying to simulate MITM certificate
And click OK.
@kenny-evitt let me know if this is detailed enough or if you need any more info. @cldwalker I looked into the feasibility of using the standard http lib instead of the |
@mluisbrown I'll try this out and let you know if it works for me. As for merging PRs, we can just leave them separate for now. But if we wanted to merge, just fetch the relevant branches (i.e. you fetch the branch from my fork, or vice versa), merge (locally), and then push the new version of the branch of your fork (or mine) to GitHub (and optionally force the push if you had to |
@mluisbrown I seem to be running into a bit of trouble when trying to get Light Table to play nicely with the proxy environment variables needed for Charles to pick up its activity. On Ubuntu, when I set everything up as directed, an error is thrown about unescaped characters when forcing LT to retrieve the plugin metadata... It does not appear that LT successfully makes a connection before the error since Charles shows no activity. However, if I was to unset the proxy environment variables then it will function normally (and not go through Charles). Did you encounter anything like this? |
@sbauer322 that is the symptom of the broken proxy support in v2.14.0 of the request npm package that is used by LT for making HTTP requests. See the comments in #1984. Getting proxy support to work at all requires updating the version of the This PR is specifically about adding support for disabling strict SSL support when using a proxy, but that is only meaningful if proxy support is working. |
Ah, yes - looks like I had checked out your branch rather than merging it into my branch with the updated request package. Sorry for the confusion! 😨 Will give this another try tonight. |
Yep, works perfectly with the updated request package, @mluisbrown. As far as I can tell, this PR is ready to be merged once the request package is updated. I have opened a PR for updating the request package, PR #2331. |
Awesome! And thanks for opening #2231 @sbauer322 👍 |
@LightTable/maintainers - I will be merging this after 2017-04-29 unless there is concerns or feedback. |
🎉 Thanks Scott 🎉 |
Implements #2212.
@kenny-evitt as I didn't hear anything more from you on the issue, I went ahead and implemented my suggestion. Let me know what you think.