https.request is broken by agent-base included by https-proxy-agent

[davidnikdel-epic]

Package + Version

• @sentry/node

Version:

4.6.6
5.3.0

Description

Some time after 4.1.1 sentry started depending on https-proxy-agent. https-proxy-agent depends on agent-base which does a little nasty thing where it patches over node-core's https.request here

Problems are:

1. this patch only supports the 2 parameter form.
2. this patch does not support any forms where a URL object is passed in (instead of the string) due to its reliance on Object.assign

Docs on node's https.request

Unfortunately this means any app that depends on @sentry/node also receives this lovely bug.

[BYK]

Just hit this today myself. Looks like introduced by 58c9111

I think we can make the import optional based on a proxy being set or not to limit the damage. The ideal solution would be to patch and upgrade agent-base but @TooTallNate doesn't seem to be responding to PRs on the repo. Worst case, we can fork it and use yarn resolution overrides to fix this upstream.

[BYK]

This part needs patching: https://github.com/TooTallNate/node-agent-base/blob/master/patch-core.js#L33-L37

[BYK]

Submitted TooTallNate/node-agent-base#27

[jiangfengming]

Still has problem

require('@sentry/node')
const https = require('https')

const req = https.request('https://www.example.com/', {}, res => {
  console.log(res.statusCode)
})

req.end()

Throws:

events.js:59
    throw new ERR_INVALID_ARG_TYPE('listener', 'Function', listener);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object
    at checkListener (events.js:59:11)
    at ClientRequest.once (events.js:301:3)
    at new ClientRequest (_http_client.js:176:10)
    at Object.request (https.js:305:10)
    at Object.request (/Users/fenix/projects/tests/node_modules/.registry.npmjs.org/agent-base/4.3.0/node_modules/agent-base/patch-core.js:25:22)
    at Object.<anonymous> (/Users/fenix/projects/tests/https.request.js:4:19)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)

[BYK]

@jiangfengming this fix has not been released yet, so that's probably why you are still having this issue.

[kamilogorek]

@jiangfengming 5.4.2 with the fix included has been released, give it a try :)

[jiangfengming]

Upgraded to 5.4.2, still throws. From the error stack, I saw agent-base is v4.3.0.

/Users/fenix/projects/tests/node_modules/.registry.npmjs.org/agent-base/4.3.0/node_modules/agent-base/patch-core.js:25:22

[BYK]

@jiangfengming can you share the new stack trace for us to investigate please?

[davidnikdel-epic]

Doesn't appear that the patch TooTallNate/node-agent-base#27 addresses http.request (line 13). Only fixes http.get. http.request also supports a 3 param form as well now according to nodejs docs linked above.

Additionally Object.assign will not copy any properties from a URL object because they are not own-properties so that part is a bug as well.

[jiangfengming]

@BYK

events.js:59
    throw new ERR_INVALID_ARG_TYPE('listener', 'Function', listener);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object
    at checkListener (events.js:59:11)
    at ClientRequest.once (events.js:301:3)
    at new ClientRequest (_http_client.js:176:10)
    at Object.request (https.js:305:10)
    at Object.request (/Users/fenix/projects/tests/node_modules/.registry.npmjs.org/agent-base/4.3.0/node_modules/agent-base/patch-core.js:25:22)
    at Object.<anonymous> (/Users/fenix/projects/tests/https.request.js:4:19)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)

[BYK]

@davidnikdel-epic @jiangfengming - thanks for the responses. My bad confusing the two methods. Will re-open this issue and try to fix it soon!

[kamilogorek]

PR pending TooTallNate/node-agent-base#28

[sanchezjjose]

Hi we are seeing this issue still. Is there any eta on releasing a possible fix? Specifically seeing new relic external calls breaking, as described here: #2155

[kamilogorek]

Pinged the owner of the node-agent-base repo. If it won't be fixed soon, we'll fork the repo ourselves and/or use different solution.

[BYK]

@kamilogorek no need to ping the owner as we also have access. I'll try to spend some time on this today and tomorrow.

[kamilogorek]

TIL 🙃

[kunal15595]

Any progress here?

[jstewmon]

Now that this issue page is working again, this can be closed since #2355 was merged, right?