http2: do not allow socket manipulation

apapirovski · gibfahn · commit e6e99eb4472a · 2017-10-31T00:14:53.000Z
Because of the specific serialization and processing requirements of HTTP/2, sockets should not be directly manipulated. This forbids any interactions with destroy, emit, end, pause, read, resume and write methods of the socket. It also redirects setTimeout to session instead of socket. PR-URL: #16330 Fixes: #16252 Refs: #16211 Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -725,8 +725,8 @@ reached.
 <a id="ERR_HTTP2_NO_SOCKET_MANIPULATION"></a>
 ### ERR_HTTP2_NO_SOCKET_MANIPULATION
 
-Used when attempting to read, write, pause, and/or resume a socket attached to
-an `Http2Session`.
+Used when attempting to directly manipulate (e.g read, write, pause, resume,
+etc.) a socket attached to an `Http2Session`.
 
 <a id="ERR_HTTP2_OUT_OF_STREAMS"></a>
 ### ERR_HTTP2_OUT_OF_STREAMS
diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -463,12 +463,16 @@ added: v8.4.0
 
 * Value: {net.Socket|tls.TLSSocket}
 
-A reference to the [`net.Socket`][] or [`tls.TLSSocket`][] to which this
-`Http2Session` instance is bound.
+Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
+limits available methods to ones safe to use with HTTP/2.
 
-*Note*: It is not recommended for user code to interact directly with a
-`Socket` bound to an `Http2Session`. See [Http2Session and Sockets][] for
-details.
+`destroy`, `emit`, `end`, `pause`, `read`, `resume`, and `write` will throw
+an error with code `ERR_HTTP2_NO_SOCKET_MANIPULATION`. See
+[Http2Session and Sockets][] for more information.
+
+`setTimeout` method will be called on this `Http2Session`.
+
+All other interactions will be routed directly to the socket.
 
 #### http2session.state
 <!-- YAML
@@ -2138,10 +2142,10 @@ Returns `request`.
 added: v8.4.0
 -->
 
-* {net.Socket}
+* {net.Socket|tls.TLSSocket}
 
-Returns a Proxy object that acts as a `net.Socket` but applies getters,
-setters and methods based on HTTP/2 logic.
+Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
+applies getters, setters and methods based on HTTP/2 logic.
 
 `destroyed`, `readable`, and `writable` properties will be retrieved from and
 set on `request.stream`.
@@ -2293,7 +2297,7 @@ will result in a [`TypeError`][] being thrown.
 added: v8.4.0
 -->
 
-* {net.Socket}
+* {net.Socket|tls.TLSSocket}
 
 See [`response.socket`][].
 
@@ -2510,10 +2514,10 @@ Returns `response`.
 added: v8.4.0
 -->
 
-* {net.Socket}
+* {net.Socket|tls.TLSSocket}
 
-Returns a Proxy object that acts as a `net.Socket` but applies getters,
-setters and methods based on HTTP/2 logic.
+Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
+applies getters, setters and methods based on HTTP/2 logic.
 
 `destroyed`, `readable`, and `writable` properties will be retrieved from and
 set on `response.stream`.
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -199,8 +199,7 @@ E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed');
 E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK',
   (max) => `Maximum number of pending settings acknowledgements (${max})`);
 E('ERR_HTTP2_NO_SOCKET_MANIPULATION',
-  'HTTP/2 sockets should not be directly read from, written to, ' +
-  'paused and/or resumed.');
+  'HTTP/2 sockets should not be directly manipulated (e.g. read and written)');
 E('ERR_HTTP2_OUT_OF_STREAMS',
   'No stream ID is available because maximum stream ID has been reached');
 E('ERR_HTTP2_PAYLOAD_FORBIDDEN',
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
@@ -5,6 +5,7 @@ const Readable = Stream.Readable;
 const binding = process.binding('http2');
 const constants = binding.constants;
 const errors = require('internal/errors');
+const { kSocket } = require('internal/http2/util');
 
 const kFinish = Symbol('finish');
 const kBeginSend = Symbol('begin-send');
@@ -176,15 +177,15 @@ const proxySocketHandler = {
         throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
       default:
         const ref = stream.session !== undefined ?
-          stream.session.socket : stream;
+          stream.session[kSocket] : stream;
         const value = ref[prop];
         return typeof value === 'function' ? value.bind(ref) : value;
     }
   },
   getPrototypeOf(stream) {
     if (stream.session !== undefined)
-      return stream.session.socket.constructor.prototype;
-    return stream.prototype;
+      return Reflect.getPrototypeOf(stream.session[kSocket]);
+    return Reflect.getPrototypeOf(stream);
   },
   set(stream, prop, value) {
     switch (prop) {
@@ -201,9 +202,9 @@ const proxySocketHandler = {
       case 'setTimeout':
         const session = stream.session;
         if (session !== undefined)
-          session[prop] = value;
+          session.setTimeout = value;
         else
-          stream[prop] = value;
+          stream.setTimeout = value;
         return true;
       case 'write':
       case 'read':
@@ -212,7 +213,7 @@ const proxySocketHandler = {
         throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
       default:
         const ref = stream.session !== undefined ?
-          stream.session.socket : stream;
+          stream.session[kSocket] : stream;
         ref[prop] = value;
         return true;
     }
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -38,6 +38,7 @@ const {
   getSettings,
   getStreamState,
   isPayloadMeaningless,
+  kSocket,
   mapToHeaders,
   NghttpError,
   sessionName,
@@ -70,10 +71,10 @@ const kOptions = Symbol('options');
 const kOwner = Symbol('owner');
 const kProceed = Symbol('proceed');
 const kProtocol = Symbol('protocol');
+const kProxySocket = Symbol('proxy-socket');
 const kRemoteSettings = Symbol('remote-settings');
 const kServer = Symbol('server');
 const kSession = Symbol('session');
-const kSocket = Symbol('socket');
 const kState = Symbol('state');
 const kType = Symbol('type');
 
@@ -672,6 +673,48 @@ function finishSessionDestroy(self, socket) {
   debug(`[${sessionName(self[kType])}] nghttp2session destroyed`);
 }
 
+const proxySocketHandler = {
+  get(session, prop) {
+    switch (prop) {
+      case 'setTimeout':
+        return session.setTimeout.bind(session);
+      case 'destroy':
+      case 'emit':
+      case 'end':
+      case 'pause':
+      case 'read':
+      case 'resume':
+      case 'write':
+        throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
+      default:
+        const socket = session[kSocket];
+        const value = socket[prop];
+        return typeof value === 'function' ? value.bind(socket) : value;
+    }
+  },
+  getPrototypeOf(session) {
+    return Reflect.getPrototypeOf(session[kSocket]);
+  },
+  set(session, prop, value) {
+    switch (prop) {
+      case 'setTimeout':
+        session.setTimeout = value;
+        return true;
+      case 'destroy':
+      case 'emit':
+      case 'end':
+      case 'pause':
+      case 'read':
+      case 'resume':
+      case 'write':
+        throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
+      default:
+        session[kSocket][prop] = value;
+        return true;
+    }
+  }
+};
+
 // Upon creation, the Http2Session takes ownership of the socket. The session
 // may not be ready to use immediately if the socket is not yet fully connected.
 class Http2Session extends EventEmitter {
@@ -707,6 +750,7 @@ class Http2Session extends EventEmitter {
     };
 
     this[kType] = type;
+    this[kProxySocket] = null;
     this[kSocket] = socket;
 
     // Do not use nagle's algorithm
@@ -756,7 +800,10 @@ class Http2Session extends EventEmitter {
 
   // The socket owned by this session
   get socket() {
-    return this[kSocket];
+    const proxySocket = this[kProxySocket];
+    if (proxySocket === null)
+      return this[kProxySocket] = new Proxy(this, proxySocketHandler);
+    return proxySocket;
   }
 
   // The session type
@@ -957,6 +1004,7 @@ class Http2Session extends EventEmitter {
     // Disassociate from the socket and server
     const socket = this[kSocket];
     // socket.pause();
+    delete this[kProxySocket];
     delete this[kSocket];
     delete this[kServer];
 
@@ -2155,30 +2203,6 @@ function socketDestroy(error) {
   this.destroy(error);
 }
 
-function socketOnResume() {
-  if (this._paused)
-    return this.pause();
-  if (this._handle && !this._handle.reading) {
-    this._handle.reading = true;
-    this._handle.readStart();
-  }
-}
-
-function socketOnPause() {
-  if (this._handle && this._handle.reading) {
-    this._handle.reading = false;
-    this._handle.readStop();
-  }
-}
-
-function socketOnDrain() {
-  const needPause = 0 > this._writableState.highWaterMark;
-  if (this._paused && !needPause) {
-    this._paused = false;
-    this.resume();
-  }
-}
-
 // When an Http2Session emits an error, first try to forward it to the
 // server as a sessionError; failing that, forward it to the socket as
 // a sessionError; failing that, destroy, remove the error listener, and
@@ -2267,9 +2291,6 @@ function connectionListener(socket) {
   }
 
   socket.on('error', socketOnError);
-  socket.on('resume', socketOnResume);
-  socket.on('pause', socketOnPause);
-  socket.on('drain', socketOnDrain);
   socket.on('close', socketOnClose);
 
   // Set up the Session
@@ -2426,9 +2447,6 @@ function connect(authority, options, listener) {
   }
 
   socket.on('error', socketOnError);
-  socket.on('resume', socketOnResume);
-  socket.on('pause', socketOnPause);
-  socket.on('drain', socketOnDrain);
   socket.on('close', socketOnClose);
 
   const session = new ClientHttp2Session(options, socket);
diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
@@ -3,6 +3,8 @@
 const binding = process.binding('http2');
 const errors = require('internal/errors');
 
+const kSocket = Symbol('socket');
+
 const {
   NGHTTP2_SESSION_CLIENT,
   NGHTTP2_SESSION_SERVER,
@@ -551,6 +553,7 @@ module.exports = {
   getSettings,
   getStreamState,
   isPayloadMeaningless,
+  kSocket,
   mapToHeaders,
   NghttpError,
   sessionName,
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
@@ -1,10 +1,13 @@
+// Flags: --expose-internals
+
 'use strict';
 
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 const assert = require('assert');
 const h2 = require('http2');
+const { kSocket } = require('internal/http2/util');
 
 {
   const server = h2.createServer();
@@ -13,7 +16,7 @@ const h2 = require('http2');
     common.mustCall(() => {
       const destroyCallbacks = [
         (client) => client.destroy(),
-        (client) => client.socket.destroy()
+        (client) => client[kSocket].destroy()
       ];
 
       let remaining = destroyCallbacks.length;
@@ -23,9 +26,9 @@ const h2 = require('http2');
         client.on(
           'connect',
           common.mustCall(() => {
-            const socket = client.socket;
+            const socket = client[kSocket];
 
-            assert(client.socket, 'client session has associated socket');
+            assert(socket, 'client session has associated socket');
             assert(
               !client.destroyed,
               'client has not been destroyed before destroy is called'
@@ -41,7 +44,7 @@ const h2 = require('http2');
             destroyCallback(client);
 
             assert(
-              !client.socket,
+              !client[kSocket],
               'client.socket undefined after destroy is called'
             );
 
diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js
@@ -1,9 +1,13 @@
+// Flags: --expose-internals
+
 'use strict';
 
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 const h2 = require('http2');
+const { kSocket } = require('internal/http2/util');
+
 const body =
   '<html><head></head><body><h1>this is some data</h2></body></html>';
 
@@ -32,7 +36,7 @@ server.on('listening', common.mustCall(function() {
 
   req.on('response', common.mustCall(() => {
     // send a premature socket close
-    client.socket.destroy();
+    client[kSocket].destroy();
   }));
   req.on('data', common.mustNotCall());
 
diff --git a/test/parallel/test-http2-compat-socket-set.js b/test/parallel/test-http2-compat-socket-set.js
@@ -13,8 +13,8 @@ const h2 = require('http2');
 const errMsg = {
   code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION',
   type: Error,
-  message: 'HTTP/2 sockets should not be directly read from, written to, ' +
-           'paused and/or resumed.'
+  message: 'HTTP/2 sockets should not be directly manipulated ' +
+           '(e.g. read and written)'
 };
 
 const server = h2.createServer();
diff --git a/test/parallel/test-http2-compat-socket.js b/test/parallel/test-http2-compat-socket.js
@@ -15,8 +15,8 @@ const net = require('net');
 const errMsg = {
   code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION',
   type: Error,
-  message: 'HTTP/2 sockets should not be directly read from, written to, ' +
-           'paused and/or resumed.'
+  message: 'HTTP/2 sockets should not be directly manipulated ' +
+           '(e.g. read and written)'
 };
 
 const server = h2.createServer();
diff --git a/test/parallel/test-http2-create-client-secure-session.js b/test/parallel/test-http2-create-client-secure-session.js
diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js
diff --git a/test/parallel/test-http2-server-socketerror.js b/test/parallel/test-http2-server-socketerror.js
diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js
