fix(MockHttpSocket): forward tls socket properties #556
Conversation
kettanaito
changed the base branch from
main
to
feat/yet-another-socket-interceptor
| Reflect.set(this, 'encrypted', true) | ||
| // The server certificate is not the same as a CA | ||
| // passed to the TLS socket connection options. | ||
| Reflect.set(this, 'authorized', false) |
There was a problem hiding this comment.
@mikicho, note that the .authorized must be set to false. It's false in a bypassed HTTPS communication as well.
It will only be true if the certificate you pass to socket options is the same one that has signed the server you are requesting. That's false in most cases.
kettanaito
changed the title
| @@ -169,6 +169,28 @@ export class MockHttpSocket extends MockSocket { | |||
| } | |||
| } | |||
|
|
|||
| // Forward TLS Socket properties onto this Socket instance | |||
| // in the case of a TLS/SSL connection. | |||
| if (Reflect.get(socket, 'encrypted')) { | |||
There was a problem hiding this comment.
Can we check the protocol instead?
There was a problem hiding this comment.
We can but that should be the discerning property to tell apart Socket from TLSSocket, per Node.js docs:
This may be used to distinguish TLS sockets from regular
net.Socketinstances.
— Node.js docs / TLS
I don't mind checking the protocol but if the HTTPS request does have a TLSSocket, it's constructed incorrectly.
Is there a particular test case that fails with this check?
There was a problem hiding this comment.
Oh, it's a weird choice by Node. But if it's official, I'm in.
kettanaito
merged commit 430c65e
into
feat/yet-another-socket-interceptor
Forwards the following TLS socket properties onto the
MockHttpSocket(for both mocked and passthrough responses):encryptedauthorizedgetSession()getProtocol()isSessionReused()