lib/node.js: 'https' interfaces should use tls$TLSSocket instead of net$Socket

[cakoose]

The issue:

// @flow
const http = require('http');
const https = require('https');

http.request({}, res => {
    console.log(res.socket.getPeerCertificate());  // <-- true positive
});
https.request({}, res => {
    console.log(res.socket.getPeerCertificate());  // <-- false positive
});

Before, Flow would flag both with the message: Cannot call res.socket.getPeerCertificate because property getPeerCertificate is missing in net$Socket [1].

After this change, Flow only flags the first.

My fix adds a SocketT type parameter to a few base classes. Not sure if that's the best way to do it.

[nmote]

At a glance, this looks reasonable. Could you link API docs that support this change?

[cakoose]

It's not spelled out explicitly, but:

• The "http" module doc says that response.socket references the "underlying socket" (link)
• The "https" module doc (link)
  • Has several instances of of "https.blah() is like http.blah() but for HTTPS"
  • The "https.request()" doc says that options from "tls.connect()" are accepted
• There's a Node.js bug report relating to getPeerCertificate() and the responses from the Node.js team members seem ok with relying on it from https.request() (link).

[nmote]

Okay, that's good enough for me. Could you also do a quick sanity check that things behave, at runtime, the way your new Flow types claim? I'm not asking for anything really in-depth, maybe just a short node transcript.

Could you also update the classes for which you have added type parameters to have net$Socket as the default if no type parameter is provided? That will make the rollout easier for code which references e.g. http$Agent. We can just change it to http$Agent<>. The syntax for doing this is declare class Foo<T = DefaultType> { ... }.

After that, I'll import and assuming that the rollout burden doesn't look too bad on our main repositories, I'll merge.

Thanks for the PR!

[cakoose]

Example of using getPeerCertificate:

$ cat flow-bug-7215.js 
const https = require('https');
https.get('https://httpbin.org/get', res => {
    console.log('getPeerCertificate', res.socket.getPeerCertificate());
});
$ node --version
v8.15.0
$ node flow-bug-7215.js 
getPeerCertificate { subject: { CN: 'httpbin.org' },
  issuer: 
   { C: 'US',
     O: 'Let\'s Encrypt',
     CN: 'Let\'s Encrypt Authority X3' },
  subjectaltname: 'DNS:httpbin.org, DNS:www.httpbin.org',
  infoAccess: 
   { 'OCSP - URI': [ 'http://ocsp.int-x3.letsencrypt.org' ],
     'CA Issuers - URI': [ 'http://cert.int-x3.letsencrypt.org/' ] },
  modulus: 'CF4902A6832E71528359004D3EDFC4BF93273246B8B877443941744DB91BDC650F0166A064C9421159A7834BE12EDFC1C78C4CF5A9223D5D122CD9B8DE445FE6529616E63AAAD047331BAE029D2482D6E4AE508D80949F502D0D5369A09E33261D67586A334D97ACEFDF390CC9E1C565BE7A0783DA37F82DB85D19541BB3F2BBE3A48DAC0E13CBA402BF5097EA16F67D8538EDFB13960F4C512262CE7C7E7BAD0AC91DB6AE4D1924F897960D804CD01C95FCBE2105EF5684865992DAC8ACCF1EEA25574CAEFD66619DDD98AD92C80EDBD9CEFBFFF3666F32DEBEE0F1EAF3FCB6F2B346E1722A9C22A905A31DA8BD7B523F2D2DB58782CD466F6E5F964F9A28FD',
  exponent: '0x10001',
  valid_from: 'Jan  8 23:16:03 2019 GMT',
  valid_to: 'Apr  8 23:16:03 2019 GMT',
  fingerprint: '2B:F0:48:9D:78:B4:DE:E9:69:E2:73:E0:14:D0:DC:CC:A8:D8:3B:40',
  ext_key_usage: [ '1.3.6.1.5.5.7.3.1', '1.3.6.1.5.5.7.3.2' ],
  serialNumber: '03C4F2421FB591ECDBF678FDC0310B31F469',
  raw: <Buffer 30 82 05 5f 30 82 04 47 a0 03 02 01 02 02 12 03 c4 f2 42 1f b5 91 ec db f6 78 fd c0 31 0b 31 f4 69 30 0d 06 09 2a 86 48 86 f7 0d 01 01 0b 05 00 30 4a ... > }

[cakoose]

Added the SocketT default and rebased on to latest origin/master. Feel free to tweak/squash/replace my commits; authorship is not important to me.

(Too bad we can't write Agent instead of Agent<> when all type parameters have defaults. Oh well...)

[nmote]

Yeah, I'm not sure what went into the decision to require the <> even when all type parameters have defaults. Way back when, we actually let you write a type without type parameters and Flow would fill in any for all the type parameters (e.g. Foo is equivalent to Foo<any, any> for a type Foo with two type parameters). That unsafe behavior is long gone, but I wonder if we needed a distinction between Foo and Foo<> for that purpose. Maybe it would be worth revisiting that behavior.

Anyway, I'll import this, see how involved the rollout would be, and get back to you.

[facebook-github-bot]

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cakoose]

Oh, that makes sense. Java did something similar when to aid the transition to generics in 1.5.

[nmote]

I think we're good here. My only concern was that anybody who references these types would have to add the <> on rollout, but I don't think that will be a huge issue. I only found a few references to these types in our main internal codebases, and I asked in the Discord to see if anybody had concerns and nobody seemed worried about it.

I'll make sure it gets mentioned specifically in the release notes when it goes out.

I just need to get someone to approve this internally and then I'll land it. Thanks for the contribution!

[nmote]

FYI I made some tweaks since this PR ended up causing us some rollout pain internally. The specific issue and the fix are in the commit message: 6a4c5a0

This change will go out with v0.94.0. In the meantime we are just going to suppress the errors.