err: TypeError: Client request error: Cannot read private member #context from an object whose class did not declare it

[KrayzeeKev]

Runtime

NodeJS

Runtime version

v20

Module version

18.0.1

Last module version without issue

No response

Used with

simple-oauth2

Any other relevant information

Can not work out if this is a wreck issue or a nodejs issue. Or perhaps even a simple-oauth2 problem.

What are you trying to achieve or the steps to reproduce?

Using simple-oauth2 to get a token and that modules uses @hapi/wreck

simple-oauth2 does nothing more than this: this.#client = Wreck.defaults(httpOptions); const response = await this.#client.post(url, options); where url is a string.

It feels like something wreck is doing is triggering the NodeJS internal url library to try do something weird although I can't see how an external library like wreck could be doing anything so bad as to cause such an issue. I am pretty certain this is a NodeJS issue but something special is happening here because it's unfathomable that v20.10 has come along and nobody usin

What was the result you got?

err: TypeError: Client request error: Cannot read private member #context from an object whose class did not declare it

I added a console.log(new Error().stack)) to on Error and got this - not particularly informative: Error at ClientRequest.onError (/opt/APP/node_modules/@hapi/wreck/lib/index.js:192:13) at Object.onceWrapper (node:events:629:26) at ClientRequest.emit (node:events:514:28) at _destroy (node:_http_client:875:13) at onSocketNT (node:_http_client:895:5) at process.processTicksAndRejections (node:internal/process/task_queues:83:21) 2023-11-20T09:41:01+11:00 [ERROR] - Failed to Get Token - err: TypeError: Client request error: Cannot read private member #context from an object whose class did not declare it

The actual error is thrown in lib/internal/url.js which is new to NodeJS v20. I've raised https://github.com/nodejs/node/issues/50891 against NodeJs to try get at this from both directions but it's strange we've got to v20.10 and nobody else is experiencing this. It happens in v20.0 (which is when url.js got introduced).

What result did you expect?

No error :-)

[KrayzeeKev]

Point of clarity: The error itself is generated by NodeJS and appears to be triggered by something in its internal URL library. But normal usage of that library is fine. This error only turns up for us when going via wreck. We're trying to work out what's special about this call.

[voxpelli]

@KrayzeeKev I'm failing to reproduce this.

I'm running the example from the docs:

const Wreck = require('@hapi/wreck');

const example = async function () {

    const { res, payload } = await Wreck.get('http://example.com');
    console.log(payload.toString());
};

try {
    example();
}
catch (ex) {
    console.error(ex);
}

And it works without errors on when using Node.js v20.9.0 on MacOS, at least when installing it fresh so that the package-lock.json contains this:

    "node_modules/@hapi/boom": {
      "version": "10.0.1",
      "resolved": "https://registry.npmjs.org/@hapi/boom/-/boom-10.0.1.tgz",
      "integrity": "sha512-ERcCZaEjdH3OgSJlyjVk8pHIFeus91CjKP3v+MpgBNp5IvGzP2l/bRiD78nqYcKPaZdbKkK5vDBVPd2ohHBlsA==",
      "dependencies": {
        "@hapi/hoek": "^11.0.2"
      }
    },
    "node_modules/@hapi/bourne": {
      "version": "3.0.0",
      "resolved": "https://registry.npmjs.org/@hapi/bourne/-/bourne-3.0.0.tgz",
      "integrity": "sha512-Waj1cwPXJDucOib4a3bAISsKJVb15MKi9IvmTI/7ssVEm6sywXGjVJDhl6/umt1pK1ZS7PacXU3A1PmFKHEZ2w=="
    },
    "node_modules/@hapi/hoek": {
      "version": "11.0.2",
      "resolved": "https://registry.npmjs.org/@hapi/hoek/-/hoek-11.0.2.tgz",
      "integrity": "sha512-aKmlCO57XFZ26wso4rJsW4oTUnrgTFw2jh3io7CAtO9w4UltBNwRXvXIVzzyfkaaLRo3nluP/19msA8vDUUuKw=="
    },
    "node_modules/@hapi/wreck": {
      "version": "18.0.1",
      "resolved": "https://registry.npmjs.org/@hapi/wreck/-/wreck-18.0.1.tgz",
      "integrity": "sha512-OLHER70+rZxvDl75xq3xXOfd3e8XIvz8fWY0dqg92UvhZ29zo24vQgfqgHSYhB5ZiuFpSLeriOisAlxAo/1jWg==",
      "dependencies": {
        "@hapi/boom": "^10.0.1",
        "@hapi/bourne": "^3.0.0",
        "@hapi/hoek": "^11.0.2"
      }
    }

Considering that 18.0.1 requires those very dependency versions I'm pretty sure you have the same ones though if you're also on 18.0.1.

And the tests passes in general for this module, so maybe simple-oauth2 is doing something that the tests here doesn't account for, will dive deeper.

[voxpelli]

@KrayzeeKev Can you specify which version of simple-oauth2 you are running and which OS and OS-version you are running?

[voxpelli]

There is one test that fails on Windows starting with Node 18:

   89) read() handles requests that close early:

      Payload stream closed prematurely

      at IncomingMessage.onResAborted (D:\a\wreck\wreck\lib\index.js:422:94)
      at Object.onceWrapper (node:events:631:28)
      at IncomingMessage.emit (node:events:517:28)
      at IncomingMessage._destroy (node:_http_incoming:224:10)
      at _destroy (node:internal/streams/destroy:109:10)
      at IncomingMessage.destroy (node:internal/streams/destroy:71:5)
      at abortIncoming (node:_http_server:766:9)
      at socketOnClose (node:_http_server:760:3)
      at Socket.emit (node:events:529:35)
      at TCP.<anonymous> (node:net:350:12)

But that appears to not be the same, though shows that OS can be of importance here

[KrayzeeKev]

simple-oauth2 5.0.0 OS: Oracle Enterprise Linux (RHEL based) 7.0

I believe I've narrowed it down some more and @hapi may be off the hook:

#!/usr/bin/env node
use strict';

const { ClientCredentials } = require('simple-oauth2');
const { HttpsProxyAgent } = require('https-proxy-agent');

const client = new ClientCredentials({
  client: {
    id: 'some id',
    secret: 'some secret',
  },
  auth: {
    tokenHost: 'https://auth.externaldomain.com',
    tokenPath: '/oauth2/token',
  },
  http: {
    agent: new HttpsProxyAgent('http://myproxy.internal.com:443'),
  },
});

client.getToken({ scope: 'scope' })
  .then( res => console.log(res) )
  .catch( err => console.log(err) );

It's definitely the proxy agent. Axios via the proxy seems happy so it's something HttpsProxyAgent is doing. I will investigate further and see if it's purely that module or if it's the way these 3 modules (wreck, simple-oauth2, https-proxy-agent) interact.

Thanks for you assistance so far.

[KrayzeeKev]

Found it. It's @hapi/hoek/lib/clone as used by @hapi/hoek/lib/applyToDefaults as used by Wreck Specifically, this minimal case will kill it:

const url = new URL('http://proxy:443');
const proto = Object.getPrototypeOf(url);
const copy = Object.create(proto);
console.log(copy);

I've read elsewhere that the nature of private variables (used in the URL class/object in this case, for instance) means that you can't wrap them in a Proxy without doing some trickery. I wonder if this is related. I have a nodejs issue open similarly here and I will update that with this minimal case. The story about the Proxy implied this was by design so that would mean that the @hapi/hoek clone needs a new special case or something. Here's the SO article: https://stackoverflow.com/questions/67416881/es6-proxied-class-access-private-property-cannot-read-private-member-hidden-f

[KrayzeeKev]

The following comments are from Jordan Harband - TC39 (JS standards body) member, contributor to NodeJS, author of npm - on slack

Yes; you can’t naively copy any objects that might have special behavior - internal slots, private fields, or even just identity tracked in a weak collection somewhere. I’d file a bug on wreck.

the only way to copy things correctly is to hardcode known object kinds, like URL

there simply is no generic way (nor will there ever be) to copy or clone any kind of object

Since wreck may know the full list of possible config options it can get passed, it might be possible to explicitly check for URL in the hoek/clone. It's ugly but there does not seem to be any other choice other than giving up on the deep clone.

[kanongil]

Thanks for the investigation!

This is just an another special object case for Hoek.clone() to handle. It already handles many other language level types this way. I made a fix for this in https://github.com/hapijs/hoek/pull/392.

[KrayzeeKev]

Thanks. Now I just have to hope that all the semvers along the way mean I'll get the latest because the module I'm using directly was last updated 12 months ago.

[KrayzeeKev]

Still not 100% sure why wreck needs to deep clone its config params at all. Are callers really keeping references to sub objects and modifying them or something?

[voxpelli]

Some cloning could historically be helpful, especially in a pre-typed world, where it could avoid tricky bugs where a changed value in one part of the code propagated into another part of the code as one were accidentally modifying the very same object

Now it can be better to prohibit that by typing those options with readonly types and enforcing it using static code analysis.

Another alternative nowadays can be Object.freeze, but as far as I remember that has enough of a performance impact to not always be an option.

Ideally one shouldn’t have a deep nested configuration object and thus don’t need deep cloning but rather be able to achieve one’s need through eg simple destructuring

[voxpelli]

Eg: Associative arrays in PHP are not sent as references to the same associative array but rather sent as a copy of the values, so if one wants to mimic eg. that style in JS then one has to do copy the values oneself, like seems to be done here

[Marsup]

I'm not seeing any modification to the url parameter in wreck's source code. hoek's PR seems valid, but we could also add url to shallowOptions. Thoughts?

[voxpelli]

@Marsup Would that make it so that wreck tells hoek not to clone it?

[kanongil]

We use the clone to guard against subsequent manipulation by the caller. Eg.

const baseUrl = new URL('https://hapi.dev/');
const wreck = Wreck.defaults({ baseUrl });

baseUrl.hostname = 'google.com';

await wreck.get('/api/');

Without the clone, this would request from https://google.com/api/.

It is a general principle in the hapi ecosystem to clone passed options when used async. This eliminates a class of hard to debug problems, when users inadvertently modify a passed property after calling the method.

[voxpelli]

@kanongil One can achieve that without using a generic deep clone though?

[Nargonath]

@KrayzeeKev new hoek version is released as 11.0.3.