Removed need for HTTPS sites to be proxied by tunnels only. Fixes #1293 #1344
Conversation
|
There was nothing in the test server that was actually responding to that new request. Here's a working test: nylen/request@b16edd3 If you can pull that commit in and clean up the docs for the tunnel option (in the API section) we'll be good to go. Thanks for the PR. |
|
Derp. That makes sense, thanks! I will let you know when the update is in. |
|
Alright, I have |
|
Oh... I think I missed something here... By default, we still want to tunnel proxied requests to a HTTPS endpoint. If we had had a test for that behavior (like we really should have) then that test would have broken with this change. What we should be adding here is the option to disable tunneling HTTPS requests. Make sense? |
|
Yep, I was considering this a breaking change by not adopting that behavior. I felt like the HTTPS/tunnelling default was a strawman that nobody uses. I will add in the new logic. |
twolfson
force-pushed
the
dev/dont.require.tunneling.sqwished
branch
2 times, most recently
from
8fa9d9c
to
203e2a6
Compare
|
Alright, I have fixed it. For what it's worth, it looks like |
|
I am going to change my implementation of |
twolfson
force-pushed
the
dev/dont.require.tunneling.sqwished
branch
from
203e2a6
to
8714f79
Compare
|
Alright, all done and ready for review. |
|
The default behavior when requesting a HTTPS endpoint should definitely be to tunnel. Otherwise the proxy can see the request which is not desired unless you know what you're doing. I'm working on some more tests to get more comfortable with this change. |
|
Ah, right. Sorry I was being fixated on my use case =/ |
|
No worries. It's a good use case, sorry it took me a little while to think through the implications. It's time to rewrite That way we can test all combinations of (proxy HTTP, HTTPS) x (endpoint HTTP, HTTPS) x ( I'm working on it, probably won't be ready until this weekend though. |
|
No worries. This PR isn't blocking me; I have been happily using my fork for the past day. |
Before, tunneling would default to false for http destinations and be forced to true for https destinations. Now, allow specifying `tunnel : false` to perform a regular GET request to HTTPS destinations. This is insecure, so make a note accordingly. Supersedes request#1344 and fixes request#1293.
|
Sweet, thanks! |
As discussed in #1293, it is possible to want to do the same HTTP to HTTP proxying when talking to an HTTPS server. This PR allows that by removing the enforcement of HTTPS going to tunneling. In this PR: