From 10706c9d9645b36389f88594f4d1df4964c767ad Mon Sep 17 00:00:00 2001
From: Fedor Indutny <fedor@indutny.com>
Date: Wed, 6 Jan 2016 17:00:27 -0500
Subject: [PATCH] http: overridable `clientError`

Make default `clientError` behavior (close socket immediately)
overridable. With this APIs it is possible to write a custom error
handler, and to send, for example, a 400 HTTP response.

    http.createServer(...).on('clientError', function(err, socket) {
      socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
      socket.destroy();
    });

Fix: #4543
PR-URL: https://github.com/nodejs/node/pull/4557
Reviewed-By: Brian White <mscdex@mscdex.net>
---
 doc/api/http.markdown                         |  5 +++
 lib/_http_server.js                           | 15 +++----
 lib/https.js                                  |  5 ++-
 test/parallel/test-http-client-abort.js       |  6 ---
 .../parallel/test-http-server-client-error.js | 39 +++++++++++++++++++
 test/parallel/test-https-timeout-server.js    |  1 +
 6 files changed, 56 insertions(+), 15 deletions(-)
 create mode 100644 test/parallel/test-http-server-client-error.js

diff --git a/doc/api/http.markdown b/doc/api/http.markdown
index aedc35208f6db1..34e3417b518d63 100644
--- a/doc/api/http.markdown
+++ b/doc/api/http.markdown
@@ -421,6 +421,11 @@ not be emitted.
 `function (exception, socket) { }`
 
 If a client connection emits an `'error'` event, it will be forwarded here.
+Listener of this event is responsible for closing/destroying the underlying
+socket. For example, one may wish to more gracefully close the socket with an
+HTTP '400 Bad Request' response instead of abruptly severing the connection.
+
+Default behavior is to destroy the socket immediately on malformed request.
 
 `socket` is the [`net.Socket`][] object that the error originated from.
 
diff --git a/lib/_http_server.js b/lib/_http_server.js
index f524790fb2b13a..7534ceba95e9e1 100644
--- a/lib/_http_server.js
+++ b/lib/_http_server.js
@@ -237,10 +237,6 @@ function Server(requestListener) {
 
   this.addListener('connection', connectionListener);
 
-  this.addListener('clientError', function(err, conn) {
-    conn.destroy(err);
-  });
-
   this.timeout = 2 * 60 * 1000;
 
   this._pendingResponseData = 0;
@@ -353,7 +349,12 @@ function connectionListener(socket) {
 
   // TODO(isaacs): Move all these functions out of here
   function socketOnError(e) {
-    self.emit('clientError', e, this);
+    // Ignore further errors
+    this.removeListener('error', socketOnError);
+    this.on('error', () => {});
+
+    if (!self.emit('clientError', e, this))
+      this.destroy(e);
   }
 
   function socketOnData(d) {
@@ -372,7 +373,7 @@ function connectionListener(socket) {
   function onParserExecuteCommon(ret, d) {
     if (ret instanceof Error) {
       debug('parse error');
-      socket.destroy(ret);
+      socketOnError.call(socket, ret);
     } else if (parser.incoming && parser.incoming.upgrade) {
       // Upgrade or CONNECT
       var bytesParsed = ret;
@@ -418,7 +419,7 @@ function connectionListener(socket) {
 
     if (ret instanceof Error) {
       debug('parse error');
-      socket.destroy(ret);
+      socketOnError.call(socket, ret);
       return;
     }
 
diff --git a/lib/https.js b/lib/https.js
index 8e79d295bf5ea6..7008a79131c663 100644
--- a/lib/https.js
+++ b/lib/https.js
@@ -29,8 +29,9 @@ function Server(opts, requestListener) {
     this.addListener('request', requestListener);
   }
 
-  this.addListener('clientError', function(err, conn) {
-    conn.destroy();
+  this.addListener('tlsClientError', function(err, conn) {
+    if (!this.emit('clientError', err, conn))
+      conn.destroy(err);
   });
 
   this.timeout = 2 * 60 * 1000;
diff --git a/test/parallel/test-http-client-abort.js b/test/parallel/test-http-client-abort.js
index c3353bb72201b6..28998c70500be7 100644
--- a/test/parallel/test-http-client-abort.js
+++ b/test/parallel/test-http-client-abort.js
@@ -21,12 +21,6 @@ var server = http.Server(function(req, res) {
       server.close();
     }
   });
-
-  // since there is already clientError, maybe that would be appropriate,
-  // since "error" is magical
-  req.on('clientError', function() {
-    console.log('Got clientError');
-  });
 });
 
 var responses = 0;
diff --git a/test/parallel/test-http-server-client-error.js b/test/parallel/test-http-server-client-error.js
new file mode 100644
index 00000000000000..619a4c45175995
--- /dev/null
+++ b/test/parallel/test-http-server-client-error.js
@@ -0,0 +1,39 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const http = require('http');
+const net = require('net');
+
+const server = http.createServer(common.mustCall(function(req, res) {
+  res.end();
+}));
+
+server.on('clientError', common.mustCall(function(err, socket) {
+  socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
+
+  server.close();
+}));
+
+server.listen(common.PORT, function() {
+  function next() {
+    // Invalid request
+    const client = net.connect(common.PORT);
+    client.end('Oopsie-doopsie\r\n');
+
+    var chunks = '';
+    client.on('data', function(chunk) {
+      chunks += chunk;
+    });
+    client.once('end', function() {
+      assert.equal(chunks, 'HTTP/1.1 400 Bad Request\r\n\r\n');
+    });
+  }
+
+  // Normal request
+  http.get({ port: common.PORT, path: '/' }, function(res) {
+    assert.equal(res.statusCode, 200);
+    res.resume();
+    res.once('end', next);
+  });
+});
diff --git a/test/parallel/test-https-timeout-server.js b/test/parallel/test-https-timeout-server.js
index f6d5d75a88abbe..0cfc9a6eec33c1 100644
--- a/test/parallel/test-https-timeout-server.js
+++ b/test/parallel/test-https-timeout-server.js
@@ -32,6 +32,7 @@ server.on('clientError', function(err, conn) {
   assert.equal(conn._secureEstablished, false);
   server.close();
   clientErrors++;
+  conn.destroy();
 });
 
 server.listen(common.PORT, function() {