Skip to content
This repository has been archived by the owner on May 5, 2023. It is now read-only.

Update https.request signature #29

Closed
AaronSharp5 opened this issue Jul 25, 2019 · 2 comments · Fixed by #36
Closed

Update https.request signature #29

AaronSharp5 opened this issue Jul 25, 2019 · 2 comments · Fixed by #36

Comments

@AaronSharp5
Copy link

Tell us about your environment:

  • Agent-base version: 4.3.0
  • Node.js version: 10.15.3

In this issue: #24, you updated node-agent-base so that users can still use the 3-argument https.get function specified by the node documentation: https://nodejs.org/api/https.html#https_https_get_url_options_callback.

The node documentation also specifies a 3-argument version of https.request: https://nodejs.org/api/https.html#https_https_request_url_options_callback.

Please apply the same fix to https.request that you applied to https.get.

@emersonford
Copy link

emersonford commented Jul 26, 2019

@meimei13 and I discovered this is causing some pretty nasty bugs. Reproduced these bugs using https://github.com/meimei13/https-reproduction.

First, any usage of the url in https://nodejs.org/api/https.html#https_https_request_url_options_callback will not work with the patched request. As this patches request for the whole runtime, it's really difficult to trace where the error came from. In addition, the bugs manifest in two ways:

  1. If url is a string, then the request will error out with TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object.
  2. If url is a url.URL object, the host and hostname will be dropped and default to localhost. This can either result in a ECONNREFUSED or, potentially even worse, fail silently if something is listening on localhost:443.

Pull request #30 updates the signature.

@emersonford
Copy link

I think it's probably worth moving the if (null == options.port) { options.port = 443; } and options.secureEndpoint = true bits upstream into nodejs's request method as the getting out of sync can cause some serious bugs from the looks of it.

@meimei13 is planning on creating a pull request to push these upstream as suggested by the commenting on the patched request method.

TooTallNate added a commit that referenced this issue Dec 2, 2019
Instead of patching over `https.request()` and `https.get()` manually
to add the `secureEndpoint` boolean, utilize the stack trace of the
current call logic to determine whether or not the `https.js` core
module is invoking `new ClientRequest()`.

In theory, this is still fragile logic, but should be more future-proof
and cleaner than patching over a core module, which is never really
desirable.

Fixes #29.
Closes #30.
Closes #35.
TooTallNate added a commit that referenced this issue Dec 6, 2019
* Remove `https` core module patching logic

Instead of patching over `https.request()` and `https.get()` manually
to add the `secureEndpoint` boolean, utilize the stack trace of the
current call logic to determine whether or not the `https.js` core
module is invoking `new ClientRequest()`.

In theory, this is still fragile logic, but should be more future-proof
and cleaner than patching over a core module, which is never really
desirable.

Fixes #29.
Closes #30.
Closes #35.

* Remove Node 10-specific test case

* Prettier
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants