Allow explicitly disabling tunneling for proxied https destinations #1374
Conversation
|
@twolfson can you confirm that this works for your use case? @simov @seanstrom @FredKSchott review? |
|
Yep, made a gist as a proof of concept. Looks good |
|
one question I still have is how this works when "a previous request in the redirect chain used a tunneling proxy" - I'll add some tests in this PR. |
|
Ill have time later today to review this, unless someone else beats me to it |
|
@seanstrom sounds good, not sure if I'll be able to get to the additional tests today though. |
|
@seanstrom this is ready, and I've got another PR based on top of it waiting (#1380 with some cleanups). think you'll be able to review today? |
| @@ -133,7 +133,7 @@ function constructProxyHeaderWhiteList(headers, proxyHeaderWhiteList) { | |||
| }, {}) | |||
| } | |||
|
|
|||
| function construcTunnelOptions(request) { | |||
| function constructTunnelOptions(request) { | |||
|
Good idea to make the code cleaner, done. I tweaked your logic some, luckily I tested the crap out of this behavior so I'm pretty comfortable that it's working as intended. Let me know if you think this looks OK now, and I'll clean up the history. |
|
@nylen I think it looks good, thanks for those changes :). And comments thank you for those too. |
|
Ill merge this shortly, unless someone else has some review |
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. Also add a whole bunch of tests for tunneling behavior in different situations. Closes request#1344 and fixes request#1293.
It hasn't been easy to test proxying HTTPS URLs until the previous change to allow proxying to HTTPS destinations without tunneling.
nylen
force-pushed
the
allow-https-tunnel-false
branch
from
331e208
to
eab45ea
Compare
|
history cleaned. |
Allow explicitly disabling tunneling for proxied https destinations
Before, tunneling would default to false for http destinations and be
forced to true for https destinations.
Now, allow specifying
tunnel : falseto perform a regular GET requestto HTTPS destinations. This is insecure, so make a note accordingly.
Supersedes #1344 and fixes #1293.
Also add some more tests for proxy environment variables - it hasn't been easy to test proxying HTTPS URLs without tunneling until this change.