cloneIncomingMessage does not handle `headers` field in Node.js v16

[hrsh7th]

I met the problem described in the title. I've investigated it and found the cause.
The src/interceptors/ClientRequest/utils/cloneIncomingMessage is not properly.
But I don't know how to fix it properly. I'm planning to consider it but first I share it via this issue.

--- Edit

I removed the meaningless tests.

[hrsh7th]

Sorry. The proposed test improvements don't seem to make sense.

It seems to pass on Node.js v12 but not on the Node.js v16 or higher.

[trevoro]

The issue here seems to be related to the fact that cloneIncomingMessage doesn't really return a ClonedIncomingMessage, it returns a PassThrough. But a PassThrough doesn't have a headers method. Forcing the type signature to unknown -> ClonedIncomingMessage seems to make the other methods that call this method also behave incorrectly. I'm still unclear what the best way is to fix it, but the issue seems to be in src/interceptors/ClientRequest/utils/cloneIncomingMessage.ts

[hrsh7th]

The test seems to expect to clone the headers feild.

[kettanaito]

Hey, folks. Thanks for elaborating on this.

I'll get to reproduce this in Node 16 when I have a minute.

Overall, the incoming message cloning is perhaps the most unstable part of this library. I couldn't find any good resources on how to do that, and the only approach I found was to split IncomingMessage into two PassThrough, which is precisely how it works at the moment.

Cloning headers may look like a viable solution for now, but I think it'd be much better to find a reliable way to clone IncomingMessage without having to manually add headers and other data potentially missing from PassThrough. I'd highly appreciate any input on this!

Also, the only reason we need to clone the incoming message is to be able to read it on the library's side and don't block the reading on the consumer's side. If we can't find a reliable way to clone incoming message, the last resort would be to drop the response body from the response event in the library, so that developers could read the body if they want by IncomingMessage reference (which would block any upstream reading still).

[hrsh7th]

The IncomingMessage seems to use getter/setter with its own prototype.

https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L107

So currently implementation doesn't clone headers field because the cloneIncomingMessage clones only the instance properties.

I try to clone the IncomingMessage.prototype in my local environment and passes the failing tests.

However, it is a bit scary code.

  const stream = message.pipe(new PassThrough())

  const proto = {}
  for (const prop of [
    ...Object.getOwnPropertyNames(IncomingMessage.prototype),
    ...Object.getOwnPropertySymbols(IncomingMessage.prototype),
  ]) {
    Object.defineProperty(proto, prop, {
      ...Object.getOwnPropertyDescriptor(IncomingMessage.prototype, prop),
    });
  }
  for (const prop of [
    ...Object.getOwnPropertyNames(PassThrough.prototype),
    ...Object.getOwnPropertySymbols(PassThrough.prototype),
  ]) {
    Object.defineProperty(proto, prop, {
      ...Object.getOwnPropertyDescriptor(PassThrough.prototype, prop),
    });
  }
  Object.setPrototypeOf(stream, proto);

[kettanaito]

That's a good observation, @hrsh7th. Then we can extract property descriptors from IncomingMessage and infer those when cloning. Do you think that'd work?

Object.getOwnPropertyDescriptors should return both plain properties and getter/setters (and Symbols).

Okay, that seems to be how we're doing it now:

interceptors/src/interceptors/ClientRequest/utils/cloneIncomingMessage.ts

Lines 29 to 32 in 4563ce7

	Object.defineProperty(stream, propertyName, {
	...propertyDescriptor,
	value: message[propertyName],
	})

I wonder why proprertyDescriptor doesn't clone the getter/setter for headers.

[hrsh7th]

The current implementation will clone properties defined in instance but not clone properties defined in prototype.

So I think we should add it. I'll make PR soon.

[hrsh7th]

I've submitted the PR. I'm not an English speaker so I'm sorry. I'll talk about this via actual codes.

[trevoro]

Awesome find. I tried cloning it with the clone npm library and it worked, minus successfully adding the IS_CLONE property which broke a few other things.

…

On Sat, Feb 26, 2022 at 5:25 AM hrsh7th ***@***.***> wrote: The current implementation will clone properties defined in instance but not clone properties defined in prototype. So I think we should add it. I'll make PR soon. — Reply to this email directly, view it on GitHub <#212 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFACBL2KTG3PZ5BFEN7ZTU5DIFJANCNFSM5PKKVPVQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[hrsh7th]

Oh. If we just want to clone (without using PassThrough), it's can use clone module, I think so too.

[lionralfs]

#195 seems related

Also nodejs/node#36550 could have valuable background info related to the lazy IncomingMessage.headers change (nodejs/node#35281)