tagomoris / presto-client-node

Distributed query engine Presto client library for node.js
MIT License
126 stars 57 forks source link

Fix following 301 redirects for https upgrades #84

Closed MasterOdin closed 11 months ago

MasterOdin commented 12 months ago

PR fixes a bug where if a client responds with a http URL for nextUri while using https for the overall client, you would hit some error on the request. This was due to the fact that:

  1. We were using the https adapter for making the http call (not allowed)
  2. If we did redirect, then we'd be using a https.Agent configured with http details, which would cause SSL cert issues as the port used would be for http instead of https.

The fix here was that:

  1. Set the adapter to use based on what we detect from the opts object, instead of the overall client.adapter setting
  2. Use separate agents for http vs https, which follow-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 the nextUri 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:

openssl req -x509 -nodes -days 365000 -newkey rsa:2048 -keyout nginx-selfsigned.key -out nginx-selfsigned.crt

where the certs would need to be refreshed in 1000 years.

tagomoris commented 11 months ago

Thank you for the fix!