New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix following 301 redirects for https upgrades #84
Conversation
f7c16e3
to
9da01f8
Compare
opts.agent = new adapter.Agent(opts); // Otherwise SSL params are ignored. | ||
opts.agents = { | ||
http: new http.Agent({ keepAlive: false }), | ||
https: new https.Agent({ ...client.ssl, keepAlive: false }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to not pass in the opts
object as is into the agent, as I don't think most users would need to have host
, port
, etc. set explicitly on the agent, vs letting that get picked up automatically from the request object. If wanted to do this, then would need to override the port
value of opts
if opts.protocol === 'http:'
, which just seemed overly complicated for little benefit. Additionally, passing the client.ssl
object into the http.Agent
was probably a no-op in the common case as the values there were ignored, so I think better to pass it only into the https.Agent
.
Note, when passing no agent, the default behavior was changed in Node 20 such that keepAlive: true
is used, so I still pass in an agent to explicitly turn that off to maintain BC, as well as since these are all one off requests, there's no point to having keepAlive: true
.
Finally, as a note, doing a spread of undefined
returns empty object essentially, so if client.ssl === undefined
, then doing https.Agent({ ...client.ssl, keepAlive: false }) === https.Agent({ keepAlive: false })
essentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This opts.agents
attribute is obviously different from other opts attributes because others are to specify http/https request parameters, but this one is to control the follow-redirects
-specific behaviors.
So I want a comment here about what this controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment in bcd7789, let me know if you'd like it tweaked.
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
node-version: [ 16.x, 18.x, 20.x ] | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- run: docker compose up --detach --wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does the same thing as using the services key, where --wait
gets the command to wait until the containers are marked as healthy before continuing, similar to what services
was doing.
This simplifies the test setup to better match local development, and that there was no way to mount a volume from the local repository (as do nginx configuration) via the services
setup.
var adapters = { | ||
'http:': http, | ||
'https:': https, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed the the "raw" http
and https
modules to do the agent assignment below, and so then doing:
const http = require('follow-redirects/http');
const https = require('follow-redirects/https');
var adapterFor = function(protocol) {
return adapters[protocol];
}
and it felt like it'd be easier to just not have the adapterFor
function at all and just index the adapters
object directly. Let me know if you'd like me to bring back the helper function.
Thank you for the fix! |
PR fixes a bug where if a client responds with a
http
URL fornextUri
while usinghttps
for the overall client, you would hit some error on the request. This was due to the fact that:https
adapter for making the http call (not allowed)https.Agent
configured withhttp
details, which would cause SSL cert issues as the port used would be forhttp
instead ofhttps
.The fix here was that:
opts
object, instead of the overallclient.adapter
settingfollow-redirects
nicely provides a mechanism to accomplish.This PR (along with #82) should render the change in #41 unneeded as
nextUri
should be properly followed regardless of http/https on its response, and that I would argue that the port for thenextUri
request (if not in the response) should match the protocol of the URI, and not the overall client one.The certs used for nginx were generated using the command:
where the certs would need to be refreshed in 1000 years.