Fix tunneling after redirection from https

[falms]

Current stable module (request@2.65.0) cannot handle https-to-http redirection with proxy properly.

When redirected from https to http with options.tunnel = undefined,
the http request should be proxied without CONNECT tunneling.

This pull-request includes these changes:

1. use options again after redirection.
2. fix test case for https-to-http redirection.
3. fix description about tunnel option.

This pull-request solves this problem.

[simov]

I don't feel comfortable altering that logic. Why do you think that the current implementation is wrong?

I would like @nylen to chime in here as well.

[falms]

Thanks for your review!

There are some reasons:

1. HTTP CONNECT tunneling describes about first https request. It is not about http request after redirection.
2. Major useragents (i.e. IE, Chrome, Firefox) just GET http url via proxy without tunneling even if it is after redirection from https url.
3. Current tunnel = undefined is not compatible with major/default squid configurations. Squid without SSL_ports port 80 denies CONNECT server:80.

For these reasons, I changed tunneling logic.

[nylen]

I spent a good 20 minutes looking through the history of this code. As far as I can tell, in #1374 I was just trying to preserve the existing behavior of the library, which was a bit convoluted.

I don't have a problem with the change to the tunneling logic given that (1) existing browsers behave this new way and (2) this behavior is very well-tested.

Leaving some inline notes about changes that look unrelated to the tunneling behavior.

[nylen]

This does not appear related to the tunneling changes, can you explain?

[falms]

You are right.. This is related to init changes in redirect.js, not tunneling.

[simov]

@nylen yep, those lines you commented are probably not going in, my question was about the tunneling logic only. I'll take a look at the issues you mentioned 👍

[falms]

Thanks for your reviews, @simov @nylen!

Here is another way. Please see this commit.
https://github.com/falms/request/commit/f4e9b0b95cd714ce564c5c61dfda8b266d5cd1bb

If this is better, I will commit this to fix-tunneling-after-redirection branch too.

[simov]

@falms take a look at this commit simov@7943dce My thinking is that the tunnelOverride flag initialization happens only once, and the right place for it is in the Tunnel's constructor.

Now, it's a bit weird that the request argument in the constructor holds the initial user options as well, but that's just how request is currently implemented.

Let me know what do you think.

[falms]

@simov Holding and re-using initial user options is not so good as I thought a few days ago. I want to merge simov@7943dce into falms:fix-tunneling-after-redirection.

Could you tell me the best way to merge, please?

[simov]

Check out your branch fix-tunneling-after-redirection-simple containing your last proposal and pull mine:

git pull git@github.com:simov/request.git fix-tunneling-after-redirection-simple

Then you can push it into another PR - I think that's the easiest way.

[nylen]

I prefer the first version of this change (https://github.com/falms/request/commit/d678bfc14e7170546abfdbf9b0e3967711e04839) over https://github.com/falms/request/commit/f4e9b0b95cd714ce564c5c61dfda8b266d5cd1bb - it's a simpler change that doesn't require adding an extra property. @simov may disagree though - I haven't been active here recently so will leave it up to him.

[simov]

@nylen the extra state property is needed, because you want the check in isEnabled to be executed each time. The old code was checking for the tunnel state first, which always is the last one used - that's the fix.

Take a look at the final version of the function and the initialization this is what we are talking about.

[falms]

I created new PR #1894.

Well, both versions have extra property.
first version: request.options
final version: tunnel.tunnelOverride

In addition, changing init requires additional modifications unrelated to tunneling like delete request.options.auth.
Therefore, I pushed safer version.